Multiple refactorings/fixes for LLMQ bases InstantSend and ChainLocks (#2779)

* Remove unused parameters from CInstantSendManager::ProcessTx

* Pass txHash in CheckCanLock by reference instead of pointer

* Dont' allow locking of TXs without inputs

* Remove unused local variable nInstantSendConfirmationsRequired

* Don't subtract 1 from nInstantSendConfirmationsRequired

This was necessary in the old system but is not necessary in the new system.
It also prevented proper retroactive signing of chained TXs in regtest as
it resulted in child TXs to return true immediately for CheckCanLock when
it should actually have waited for the parent TX to become locked first.

* Access chainActive.Height() while cs_main is locked

* Properly read and write lastChainLockBlock

"pindex" is NOT the chainlocked block after the while loop finishes. We
must use the pindex (renamed to pindexChainLock now) given on method entry.

Also, the GetLastChainLockBlock() result was not assigned to,
lastChainLockBlock which resulted in the while loop to run unnecessarily
long.

* Generalize filtering in NewPoWValidBlock and SyncTransaction

We're actually interested in all TXs that have inputs, so no need to
explicitly check for tx types.

* Use tx.IsCoinBase() instead of checking for index 0

* Handle cases where a TX is not received yet in wait_for_instantlock

* Wait on all nodes for the locks

Otherwise we end up with the sender having it locked but other nodes
not yet, failing the test.

* Fix LogPrintf call in CChainLocksHandler::DoInvalidateBlock

qa/rpc-tests/p2p-instantsend.py

@@ -55,7 +55,9 @@ class InstantSendTest(DashTestFramework):
         # instantsend to receiver
         receiver_addr = receiver.getnewaddress()
         is_id = sender.instantsendtoaddress(receiver_addr, 0.9)
-        self.wait_for_instantlock(is_id, sender)
+        for node in self.nodes:
+            if node is not isolated:
+                self.wait_for_instantlock(is_id, node)
         # send doublespend transaction to isolated node
         isolated.sendrawtransaction(dblspnd_tx['hex'])
         # generate block on isolated node with doublespend transaction

qa/rpc-tests/test_framework/test_framework.py

@@ -535,10 +535,14 @@ class DashTestFramework(BitcoinTestFramework):
         start = time()
         locked = False
         while True:
-            is_tx = node.getrawtransaction(txid, True)
+            try:
-            if is_tx['instantlock']:
+                is_tx = node.getrawtransaction(txid, True)
-                locked = True
+                if is_tx['instantlock']:
-                break
+                    locked = True
+                    break
+            except:
+                # TX not received yet?
+                pass
             if time() > start + 10:
                 break
             sleep(0.5)

src/llmq/quorums_chainlocks.cpp

@@ -333,11 +333,8 @@ void CChainLocksHandler::NewPoWValidBlock(const CBlockIndex* pindex, const std::
 
     auto txs = std::make_shared<std::unordered_set<uint256, StaticSaltedHasher>>();
     for (const auto& tx : block->vtx) {
-        if (tx->nVersion == 3) {
+        if (tx->IsCoinBase() || tx->vin.empty()) {
-            if (tx->nType == TRANSACTION_COINBASE ||
+            continue;
-                tx->nType == TRANSACTION_QUORUM_COMMITMENT) {
-                continue;
-            }
         }
         txs->emplace(tx->GetHash());
         txFirstSeenTime.emplace(tx->GetHash(), curTime);
@@ -347,11 +344,8 @@ void CChainLocksHandler::NewPoWValidBlock(const CBlockIndex* pindex, const std::
 
 void CChainLocksHandler::SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, int posInBlock)
 {
-    if (tx.nVersion == 3) {
+    if (tx.IsCoinBase() || tx.vin.empty()) {
-        if (tx.nType == TRANSACTION_COINBASE ||
+        return;
-            tx.nType == TRANSACTION_QUORUM_COMMITMENT) {
-            return;
-        }
     }
 
     LOCK(cs);
@@ -492,7 +486,7 @@ void CChainLocksHandler::DoInvalidateBlock(const CBlockIndex* pindex, bool activ
 
         CValidationState state;
         if (!InvalidateBlock(state, params, pindex2)) {
-            LogPrintf("CChainLocksHandler::UpdatedBlockTip -- InvalidateBlock failed: %s\n", FormatStateMessage(state));
+            LogPrintf("CChainLocksHandler::%s -- InvalidateBlock failed: %s\n", __func__, FormatStateMessage(state));
             // This should not have happened and we are in a state were it's not safe to continue anymore
             assert(false);
         }
@@ -500,7 +494,7 @@ void CChainLocksHandler::DoInvalidateBlock(const CBlockIndex* pindex, bool activ
 
     CValidationState state;
     if (activateBestChain && !ActivateBestChain(state, params)) {
-        LogPrintf("CChainLocksHandler::UpdatedBlockTip -- ActivateBestChain failed: %s\n", FormatStateMessage(state));
+        LogPrintf("CChainLocksHandler::%s -- ActivateBestChain failed: %s\n", __func__, FormatStateMessage(state));
         // This should not have happened and we are in a state were it's not safe to continue anymore
         assert(false);
     }

src/llmq/quorums_instantsend.cpp

@@ -174,7 +174,7 @@ void CInstantSendManager::UnregisterAsRecoveredSigsListener()
     quorumSigningManager->UnregisterRecoveredSigsListener(this);
 }
 
-bool CInstantSendManager::ProcessTx(CNode* pfrom, const CTransaction& tx, CConnman& connman, const Consensus::Params& params)
+bool CInstantSendManager::ProcessTx(const CTransaction& tx, const Consensus::Params& params)
 {
     if (!IsNewInstantSendEnabled()) {
         return true;
@@ -246,13 +246,15 @@ bool CInstantSendManager::CheckCanLock(const CTransaction& tx, bool printDebug,
         return false;
     }
 
-    int nInstantSendConfirmationsRequired = params.nInstantSendConfirmationsRequired;
+    if (tx.vin.empty()) {
+        // can't lock TXs without inputs (e.g. quorum commitments)
+        return false;
+    }
 
-    uint256 txHash = tx.GetHash();
     CAmount nValueIn = 0;
     for (const auto& in : tx.vin) {
         CAmount v = 0;
-        if (!CheckCanLock(in.prevout, printDebug, &txHash, &v, params)) {
+        if (!CheckCanLock(in.prevout, printDebug, tx.GetHash(), &v, params)) {
             return false;
         }
 
@@ -274,7 +276,7 @@ bool CInstantSendManager::CheckCanLock(const CTransaction& tx, bool printDebug,
     return true;
 }
 
-bool CInstantSendManager::CheckCanLock(const COutPoint& outpoint, bool printDebug, const uint256* _txHash, CAmount* retValue, const Consensus::Params& params)
+bool CInstantSendManager::CheckCanLock(const COutPoint& outpoint, bool printDebug, const uint256& txHash, CAmount* retValue, const Consensus::Params& params)
 {
     int nInstantSendConfirmationsRequired = params.nInstantSendConfirmationsRequired;
 
@@ -283,44 +285,36 @@ bool CInstantSendManager::CheckCanLock(const COutPoint& outpoint, bool printDebu
         return true;
     }
 
-    static uint256 txHashNull;
-    const uint256* txHash = &txHashNull;
-    if (_txHash) {
-        txHash = _txHash;
-    }
-
     auto mempoolTx = mempool.get(outpoint.hash);
     if (mempoolTx) {
         if (printDebug) {
             LogPrint("instantsend", "CInstantSendManager::%s -- txid=%s: parent mempool TX %s is not locked\n", __func__,
-                     txHash->ToString(), outpoint.hash.ToString());
+                     txHash.ToString(), outpoint.hash.ToString());
         }
         return false;
     }
 
     Coin coin;
     const CBlockIndex* pindexMined = nullptr;
+    int nTxAge;
     {
         LOCK(cs_main);
         if (!pcoinsTip->GetCoin(outpoint, coin) || coin.IsSpent()) {
             if (printDebug) {
                 LogPrint("instantsend", "CInstantSendManager::%s -- txid=%s: failed to find UTXO %s\n", __func__,
-                         txHash->ToString(), outpoint.ToStringShort());
+                         txHash.ToString(), outpoint.ToStringShort());
             }
             return false;
         }
         pindexMined = chainActive[coin.nHeight];
+        nTxAge = chainActive.Height() - coin.nHeight + 1;
     }
 
-    int nTxAge = chainActive.Height() - coin.nHeight + 1;
+    if (nTxAge < nInstantSendConfirmationsRequired) {
-    // 1 less than the "send IX" gui requires, in case of a block propagating the network at the time
-    int nConfirmationsRequired = nInstantSendConfirmationsRequired - 1;
-
-    if (nTxAge < nConfirmationsRequired) {
         if (!llmq::chainLocksHandler->HasChainLock(pindexMined->nHeight, pindexMined->GetBlockHash())) {
             if (printDebug) {
-                LogPrint("instantsend", "CInstantSendManager::%s -- txid=%s: outpoint %s too new and not ChainLocked. nTxAge=%d, nConfirmationsRequired=%d\n", __func__,
+                LogPrint("instantsend", "CInstantSendManager::%s -- txid=%s: outpoint %s too new and not ChainLocked. nTxAge=%d, nInstantSendConfirmationsRequired=%d\n", __func__,
-                         txHash->ToString(), outpoint.ToStringShort(), nTxAge, nConfirmationsRequired);
+                         txHash.ToString(), outpoint.ToStringShort(), nTxAge, nInstantSendConfirmationsRequired);
             }
             return false;
         }
@@ -719,18 +713,19 @@ void CInstantSendManager::SyncTransaction(const CTransaction& tx, const CBlockIn
     }
 }
 
-void CInstantSendManager::NotifyChainLock(const CBlockIndex* pindex)
+void CInstantSendManager::NotifyChainLock(const CBlockIndex* pindexChainLock)
 {
     uint256 lastChainLockBlock;
     {
         LOCK(cs);
-        db.GetLastChainLockBlock();
+        lastChainLockBlock = db.GetLastChainLockBlock();
     }
 
     // Let's find all islocks that correspond to TXs which are part of the freshly ChainLocked chain and then delete
     // the islocks. We do this because the ChainLocks imply locking and thus it's not needed to further track
     // or propagate the islocks
     std::unordered_set<uint256> toDelete;
+    auto pindex = pindexChainLock;
     while (pindex && pindex->GetBlockHash() != lastChainLockBlock) {
         CBlock block;
         {
@@ -758,7 +753,7 @@ void CInstantSendManager::NotifyChainLock(const CBlockIndex* pindex)
 
     {
         LOCK(cs);
-        db.WriteLastChainLockBlock(pindex ? pindex->GetBlockHash() : uint256());
+        db.WriteLastChainLockBlock(pindexChainLock->GetBlockHash());
     }
 
     RetryLockMempoolTxs(uint256());
@@ -849,7 +844,7 @@ void CInstantSendManager::RetryLockMempoolTxs(const uint256& lockedParentTx)
                      tx->GetHash().ToString());
         }
 
-        ProcessTx(nullptr, *tx, *g_connman, Params().GetConsensus());
+        ProcessTx(*tx, Params().GetConsensus());
     }
 }
 

src/llmq/quorums_instantsend.h

@@ -99,9 +99,9 @@ public:
     void UnregisterAsRecoveredSigsListener();
 
 public:
-    bool ProcessTx(CNode* pfrom, const CTransaction& tx, CConnman& connman, const Consensus::Params& params);
+    bool ProcessTx(const CTransaction& tx, const Consensus::Params& params);
     bool CheckCanLock(const CTransaction& tx, bool printDebug, const Consensus::Params& params);
-    bool CheckCanLock(const COutPoint& outpoint, bool printDebug, const uint256* _txHash, CAmount* retValue, const Consensus::Params& params);
+    bool CheckCanLock(const COutPoint& outpoint, bool printDebug, const uint256& txHash, CAmount* retValue, const Consensus::Params& params);
     bool IsLocked(const uint256& txHash);
     bool IsConflicted(const CTransaction& tx);
     bool GetConflictingTx(const CTransaction& tx, uint256& retConflictTxHash);
@@ -120,7 +120,7 @@ public:
     void UpdateWalletTransaction(const uint256& txid, const CTransactionRef& tx);
 
     void SyncTransaction(const CTransaction &tx, const CBlockIndex *pindex, int posInBlock);
-    void NotifyChainLock(const CBlockIndex* pindex);
+    void NotifyChainLock(const CBlockIndex* pindexChainLock);
     void RemoveFinalISLock(const uint256& hash, const CInstantSendLockPtr& islock);
 
     void RemoveMempoolConflictsForLock(const uint256& hash, const CInstantSendLock& islock);

src/net_processing.cpp

@@ -2165,7 +2165,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
             }
 
             if (nInvType != MSG_TXLOCK_REQUEST) {
-                llmq::quorumInstantSendManager->ProcessTx(pfrom, tx, connman, chainparams.GetConsensus());
+                llmq::quorumInstantSendManager->ProcessTx(tx, chainparams.GetConsensus());
             }
 
             mempool.check(pcoinsTip);
@@ -2213,7 +2213,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
                         }
                         vEraseQueue.push_back(orphanHash);
 
-                        llmq::quorumInstantSendManager->ProcessTx(pfrom, orphanTx, connman, chainparams.GetConsensus());
+                        llmq::quorumInstantSendManager->ProcessTx(orphanTx, chainparams.GetConsensus());
                     }
                     else if (!fMissingInputs2)
                     {

src/rpc/rawtransaction.cpp

@@ -1020,7 +1020,7 @@ UniValue sendrawtransaction(const JSONRPCRequest& request)
                 throw JSONRPCError(RPC_TRANSACTION_ERROR, state.GetRejectReason());
             }
         }
-        llmq::quorumInstantSendManager->ProcessTx(nullptr, *tx, *g_connman, Params().GetConsensus());
+        llmq::quorumInstantSendManager->ProcessTx(*tx, Params().GetConsensus());
     } else if (fHaveChain) {
         throw JSONRPCError(RPC_TRANSACTION_ALREADY_IN_CHAIN, "transaction already in block chain");
     }

src/wallet/wallet.cpp

@@ -1921,7 +1921,7 @@ bool CWalletTx::RelayWalletTransaction(CConnman* connman, const std::string& str
                 }
             }
 
-            llmq::quorumInstantSendManager->ProcessTx(nullptr, *this->tx, *connman, Params().GetConsensus());
+            llmq::quorumInstantSendManager->ProcessTx(*this->tx, Params().GetConsensus());
 
             if (connman) {
                 connman->RelayTransaction((CTransaction)*this);