Implement retroactive IS locking of transactions first seen in blocks instead of mempool (#2770)

* Don't rely on UTXO set in CheckCanLock

The UTXO set only works for TXs in the mempool and won't work when we try
to retroactively lock unlocked TXs from blocks.

This is safe as ProcessTx is only called when a TX was accepted into the
mempool or connected in a block, which means that all input checks were
good.

* Rename RetryLockMempoolTxs to RetryLockTxs and let it retry connected TXs

* Instead of manually calling ProcessTx, let SyncTransaction handle all cases

SyncTransaction is called from AcceptToMemoryPool and when transactions got
connected in a block. So this is the time we want to run TXs through
ProcessTx. This also enables retroactive signing of TXs that were unknown
before a new block appeared.

* Test retroactive signing and safe TXs in LLMQ ChainLocks tests

* Also test for retroactive signing of chained TXs

* Honor lockedParentTx when looking for TXs to retry signing

* Stop scanning for TXs to retry after a depth of 6

* Generate 6 block to avoid retroactive signing overloading Travis

* Avoid retroactive signing

* Don't rely on NewPoWValidBlock and use SyncTransaction to build blockTxs

NewPoWValidBlock is not guaranteed to be called when blocks come in fast.
When a block is accepted in AcceptBlock, NewPoWValidBlock is only called
when the new block is a successor of the currently active tip. This is not
the case when after the first block a second block is accepted immediately
as the first block is not connected yet.

This might be a bug actually in the handling of NewPoWValidBlock, so we
might need to check/fix this later, but currently I prefer to not touch
that part.

Instead, we now use SyncTransaction to gather TXs for blockTxs. This works
because SyncTransaction is called for all transactions in a freshly
connected block in one go. The call also happens before UpdatedBlockTip is
called, so it's fine with the existing logic.

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

Also check for empty vin.

Author: codablock

qa/rpc-tests/autois-mempool.py

@@ -56,6 +56,10 @@ def run_test(self):
         self.log.info("Test old InstantSend")
         self.test_auto();
 
+        # Generate 6 block to avoid retroactive signing overloading Travis
+        self.nodes[0].generate(6)
+        sync_blocks(self.nodes)
+
         self.nodes[0].spork("SPORK_20_INSTANTSEND_LLMQ_BASED", 0)
         self.wait_for_sporks_same()
 
@@ -100,9 +104,17 @@ def test_auto(self, new_is = False):
         assert(not self.send_regular_instantsend(sender, receiver, False) if new_is else self.send_regular_instantsend(sender, receiver))
 
         # generate one block to clean up mempool and retry auto and regular IS
-        # generate 2 more blocks to have enough confirmations for IS
-        self.nodes[0].generate(3)
+        # generate 5 more blocks to avoid retroactive signing (which would overload Travis)
+        set_mocktime(get_mocktime() + 1)
+        set_node_times(self.nodes, get_mocktime())
+        self.nodes[0].spork("SPORK_2_INSTANTSEND_ENABLED", 4070908800)
+        self.wait_for_sporks_same()
+        self.nodes[0].generate(6)
         self.sync_all()
+        set_mocktime(get_mocktime() + 1)
+        set_node_times(self.nodes, get_mocktime())
+        self.nodes[0].spork("SPORK_2_INSTANTSEND_ENABLED", 0)
+        self.wait_for_sporks_same()
         assert(self.send_simple_tx(sender, receiver))
         assert(self.send_regular_instantsend(sender, receiver, not new_is))
 

qa/rpc-tests/llmq-chainlocks.py

@@ -88,6 +88,38 @@ def run_test(self):
         self.wait_for_chainlock(self.nodes[0], self.nodes[1].getbestblockhash())
         assert(self.nodes[0].getbestblockhash() == self.nodes[1].getbestblockhash())
 
+        # Enable LLMQ bases InstantSend, which also enables checks for "safe" transactions
+        self.nodes[0].spork("SPORK_20_INSTANTSEND_LLMQ_BASED", 0)
+        self.wait_for_sporks_same()
+
+        # Isolate a node and let it create some transactions which won't get IS locked
+        self.nodes[0].setnetworkactive(False)
+        txs = []
+        for i in range(3):
+            txs.append(self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1))
+        txs += self.create_chained_txs(self.nodes[0], 1)
+        # Assert that after block generation these TXs are NOT included (as they are "unsafe")
+        self.nodes[0].generate(1)
+        for txid in txs:
+            tx = self.nodes[0].getrawtransaction(txid, 1)
+            assert("confirmations" not in tx)
+        sleep(1)
+        assert(not self.nodes[0].getblock(self.nodes[0].getbestblockhash())["chainlock"])
+        # Disable LLMQ based InstantSend for a very short time (this never gets propagated to other nodes)
+        self.nodes[0].spork("SPORK_20_INSTANTSEND_LLMQ_BASED", 4070908800)
+        # Now the TXs should be included
+        self.nodes[0].generate(1)
+        self.nodes[0].spork("SPORK_20_INSTANTSEND_LLMQ_BASED", 0)
+        # Assert that TXs got included now
+        for txid in txs:
+            tx = self.nodes[0].getrawtransaction(txid, 1)
+            assert("confirmations" in tx and tx["confirmations"] > 0)
+        # Enable network on first node again, which will cause the blocks to propagate and IS locks to happen retroactively
+        # for the mined TXs, which will then allow the network to create a CLSIG
+        self.nodes[0].setnetworkactive(True)
+        connect_nodes(self.nodes[0], 1)
+        self.wait_for_chainlock(self.nodes[0], self.nodes[1].getbestblockhash())
+
     def wait_for_chainlock_tip_all_nodes(self):
         for node in self.nodes:
             tip = node.getbestblockhash()
@@ -110,6 +142,24 @@ def wait_for_chainlock(self, node, block_hash):
             sleep(0.1)
         raise AssertionError("wait_for_chainlock timed out")
 
+    def create_chained_txs(self, node, amount):
+        txid = node.sendtoaddress(node.getnewaddress(), amount)
+        tx = node.getrawtransaction(txid, 1)
+        inputs = []
+        valueIn = 0
+        for txout in tx["vout"]:
+            inputs.append({"txid": txid, "vout": txout["n"]})
+            valueIn += txout["value"]
+        outputs = {
+            node.getnewaddress(): round(float(valueIn) - 0.0001, 6)
+        }
+
+        rawtx = node.createrawtransaction(inputs, outputs)
+        rawtx = node.signrawtransaction(rawtx)
+        rawtxid = node.sendrawtransaction(rawtx["hex"])
+
+        return [txid, rawtxid]
+
 
 if __name__ == '__main__':
     LLMQChainLocksTest().main()

qa/rpc-tests/p2p-autoinstantsend.py

@@ -39,6 +39,10 @@ def run_test(self):
         self.log.info("Test old InstantSend")
         self.test_auto();
 
+        # Generate 6 block to avoid retroactive signing overloading Travis
+        self.nodes[0].generate(6)
+        sync_blocks(self.nodes)
+
         self.nodes[0].spork("SPORK_20_INSTANTSEND_LLMQ_BASED", 0)
         self.wait_for_sporks_same()
 

qa/rpc-tests/p2p-instantsend.py

@@ -28,6 +28,10 @@ def run_test(self):
         self.log.info("Test old InstantSend")
         self.test_doublespend()
 
+        # Generate 6 block to avoid retroactive signing overloading Travis
+        self.nodes[0].generate(6)
+        sync_blocks(self.nodes)
+
         self.nodes[0].spork("SPORK_20_INSTANTSEND_LLMQ_BASED", 0)
         self.wait_for_sporks_same()
 

src/dsnotificationinterface.cpp

@@ -71,11 +71,6 @@ void CDSNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, con
     llmq::quorumDKGSessionManager->UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload);
 }
 
-void CDSNotificationInterface::NewPoWValidBlock(const CBlockIndex* pindex, const std::shared_ptr<const CBlock>& block)
-{
-    llmq::chainLocksHandler->NewPoWValidBlock(pindex, block);
-}
-
 void CDSNotificationInterface::SyncTransaction(const CTransaction &tx, const CBlockIndex *pindex, int posInBlock)
 {
     llmq::quorumInstantSendManager->SyncTransaction(tx, pindex, posInBlock);

src/dsnotificationinterface.h

@@ -21,7 +21,6 @@ class CDSNotificationInterface : public CValidationInterface
     void AcceptedBlockHeader(const CBlockIndex *pindexNew) override;
     void NotifyHeaderTip(const CBlockIndex *pindexNew, bool fInitialDownload) override;
     void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override;
-    void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& block) override;
     void SyncTransaction(const CTransaction &tx, const CBlockIndex *pindex, int posInBlock) override;
     void NotifyMasternodeListChanged(const CDeterministicMNList& newList) override;
     void NotifyChainLock(const CBlockIndex* pindex) override;

src/llmq/quorums_chainlocks.cpp

@@ -316,41 +316,34 @@ void CChainLocksHandler::TrySignChainTip()
     quorumSigningManager->AsyncSignIfMember(Params().GetConsensus().llmqChainLocks, requestId, msgHash);
 }
 
-void CChainLocksHandler::NewPoWValidBlock(const CBlockIndex* pindex, const std::shared_ptr<const CBlock>& block)
+void CChainLocksHandler::SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, int posInBlock)
 {
-    LOCK(cs);
-    if (blockTxs.count(pindex->GetBlockHash())) {
-        // should actually not happen (blocks are only written once to disk and this is when NewPoWValidBlock is called)
-        // but be extra safe here in case this behaviour changes.
-        return;
+    bool handleTx = true;
+    if (tx.IsCoinBase() || tx.vin.empty()) {
+        handleTx = false;
     }
 
-    int64_t curTime = GetAdjustedTime();
+    LOCK(cs);
 
-    // We listen for NewPoWValidBlock so that we can collect all TX ids of all included TXs of newly received blocks
+    if (handleTx) {
+        int64_t curTime = GetAdjustedTime();
+        txFirstSeenTime.emplace(tx.GetHash(), curTime);
+    }
+
+    // We listen for SyncTransaction so that we can collect all TX ids of all included TXs of newly received blocks
     // We need this information later when we try to sign a new tip, so that we can determine if all included TXs are
     // safe.
-
-    auto txs = std::make_shared<std::unordered_set<uint256, StaticSaltedHasher>>();
-    for (const auto& tx : block->vtx) {
-        if (tx->IsCoinBase() || tx->vin.empty()) {
-            continue;
+    if (pindex && posInBlock != CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK) {
+        auto it = blockTxs.find(pindex->GetBlockHash());
+        if (it == blockTxs.end()) {
+            // we want this to be run even if handleTx == false, so that the coinbase TX triggers creation of an empty entry
+            it = blockTxs.emplace(pindex->GetBlockHash(), std::make_shared<std::unordered_set<uint256, StaticSaltedHasher>>()).first;
+        }
+        if (handleTx) {
+            auto& txs = *it->second;
+            txs.emplace(tx.GetHash());
         }
-        txs->emplace(tx->GetHash());
-        txFirstSeenTime.emplace(tx->GetHash(), curTime);
-    }
-    blockTxs[pindex->GetBlockHash()] = txs;
-}
-
-void CChainLocksHandler::SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, int posInBlock)
-{
-    if (tx.IsCoinBase() || tx.vin.empty()) {
-        return;
     }
-
-    LOCK(cs);
-    int64_t curTime = GetAdjustedTime();
-    txFirstSeenTime.emplace(tx.GetHash(), curTime);
 }
 
 bool CChainLocksHandler::IsTxSafeForMining(const uint256& txid)

src/llmq/quorums_chainlocks.h

@@ -87,7 +87,6 @@ class CChainLocksHandler : public CRecoveredSigsListener
     void ProcessNewChainLock(NodeId from, const CChainLockSig& clsig, const uint256& hash);
     void AcceptedBlockHeader(const CBlockIndex* pindexNew);
     void UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockIndex* pindexFork);
-    void NewPoWValidBlock(const CBlockIndex* pindex, const std::shared_ptr<const CBlock>& block);
     void SyncTransaction(const CTransaction &tx, const CBlockIndex *pindex, int posInBlock);
     void TrySignChainTip();
     void EnforceBestChainLock();

src/llmq/quorums_instantsend.cpp

@@ -294,20 +294,23 @@ bool CInstantSendManager::CheckCanLock(const COutPoint& outpoint, bool printDebu
         return false;
     }
 
-    Coin coin;
-    const CBlockIndex* pindexMined = nullptr;
+    CTransactionRef tx;
+    uint256 hashBlock;
+    // this relies on enabled txindex and won't work if we ever try to remove the requirement for txindex for masternodes
+    if (!GetTransaction(outpoint.hash, tx, params, hashBlock, false)) {
+        if (printDebug) {
+            LogPrint("instantsend", "CInstantSendManager::%s -- txid=%s: failed to find parent TX %s\n", __func__,
+                     txHash.ToString(), outpoint.hash.ToString());
+        }
+        return false;
+    }
+
+    const CBlockIndex* pindexMined;
     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());
-            }
-            return false;
-        }
-        pindexMined = chainActive[coin.nHeight];
-        nTxAge = chainActive.Height() - coin.nHeight + 1;
+        pindexMined = mapBlockIndex.at(hashBlock);
+        nTxAge = chainActive.Height() - pindexMined->nHeight + 1;
     }
 
     if (nTxAge < nInstantSendConfirmationsRequired) {
@@ -321,7 +324,7 @@ bool CInstantSendManager::CheckCanLock(const COutPoint& outpoint, bool printDebu
     }
 
     if (retValue) {
-        *retValue = coin.out.nValue;
+        *retValue = tx->vout[outpoint.n].nValue;
     }
 
     return true;
@@ -673,7 +676,7 @@ void CInstantSendManager::ProcessInstantSendLock(NodeId from, const uint256& has
     g_connman->RelayInv(inv);
 
     RemoveMempoolConflictsForLock(hash, islock);
-    RetryLockMempoolTxs(islock.txid);
+    RetryLockTxs(islock.txid);
 
     UpdateWalletTransaction(islock.txid, tx);
 }
@@ -708,8 +711,17 @@ void CInstantSendManager::SyncTransaction(const CTransaction& tx, const CBlockIn
         return;
     }
 
-    if (IsLocked(tx.GetHash())) {
-        RetryLockMempoolTxs(tx.GetHash());
+    if (tx.IsCoinBase() || tx.vin.empty()) {
+        // coinbase can't and TXs with no inputs be locked
+        return;
+    }
+
+    bool locked = IsLocked(tx.GetHash());
+    bool chainlocked = pindex && chainLocksHandler->HasChainLock(pindex->nHeight, pindex->GetBlockHash());
+    if (locked || chainlocked) {
+        RetryLockTxs(tx.GetHash());
+    } else {
+        ProcessTx(tx, Params().GetConsensus());
     }
 }
 
@@ -756,7 +768,7 @@ void CInstantSendManager::NotifyChainLock(const CBlockIndex* pindexChainLock)
         db.WriteLastChainLockBlock(pindexChainLock->GetBlockHash());
     }
 
-    RetryLockMempoolTxs(uint256());
+    RetryLockTxs(uint256());
 }
 
 void CInstantSendManager::RemoveFinalISLock(const uint256& hash, const CInstantSendLockPtr& islock)
@@ -795,9 +807,9 @@ void CInstantSendManager::RemoveMempoolConflictsForLock(const uint256& hash, con
     }
 }
 
-void CInstantSendManager::RetryLockMempoolTxs(const uint256& lockedParentTx)
+void CInstantSendManager::RetryLockTxs(const uint256& lockedParentTx)
 {
-    // Let's retry all mempool TXs which don't have an islock yet and where the parents got ChainLocked now
+    // Let's retry all unlocked TXs from mempool and and recently connected blocks
 
     std::unordered_map<uint256, CTransactionRef> txs;
 
@@ -817,6 +829,57 @@ void CInstantSendManager::RetryLockMempoolTxs(const uint256& lockedParentTx)
             }
         }
     }
+
+    uint256 lastChainLockBlock;
+    const CBlockIndex* pindexLastChainLockBlock = nullptr;
+    const CBlockIndex* pindexWalk = nullptr;
+    {
+        LOCK(cs);
+        lastChainLockBlock = db.GetLastChainLockBlock();
+    }
+    {
+        LOCK(cs_main);
+        if (!lastChainLockBlock.IsNull()) {
+            pindexLastChainLockBlock = mapBlockIndex.at(lastChainLockBlock);
+            pindexWalk = chainActive.Tip();
+        }
+    }
+
+    // scan blocks until we hit the last chainlocked block we know of. Also stop scanning after a depth of 6 to avoid
+    // signing thousands of TXs at once. Also, after a depth of 6, blocks get eligible for ChainLocking even if unsafe
+    // TXs are included, so there is no need to retroactively sign these.
+    int depth = 0;
+    while (pindexWalk && pindexWalk != pindexLastChainLockBlock && depth < 6) {
+        CBlock block;
+        {
+            LOCK(cs_main);
+            if (!ReadBlockFromDisk(block, pindexWalk, Params().GetConsensus())) {
+                pindexWalk = pindexWalk->pprev;
+                continue;
+            }
+        }
+
+        for (const auto& tx : block.vtx) {
+            if (lockedParentTx.IsNull()) {
+                txs.emplace(tx->GetHash(), tx);
+            } else {
+                bool isChild  = false;
+                for (auto& in : tx->vin) {
+                    if (in.prevout.hash == lockedParentTx) {
+                        isChild = true;
+                        break;
+                    }
+                }
+                if (isChild) {
+                    txs.emplace(tx->GetHash(), tx);
+                }
+            }
+        }
+
+        pindexWalk = pindexWalk->pprev;
+        depth++;
+    }
+
     for (auto& p : txs) {
         auto& tx = p.second;
         {

src/llmq/quorums_instantsend.h

@@ -124,7 +124,7 @@ class CInstantSendManager : public CRecoveredSigsListener
     void RemoveFinalISLock(const uint256& hash, const CInstantSendLockPtr& islock);
 
     void RemoveMempoolConflictsForLock(const uint256& hash, const CInstantSendLock& islock);
-    void RetryLockMempoolTxs(const uint256& lockedParentTx);
+    void RetryLockTxs(const uint256& lockedParentTx);
 
     bool AlreadyHave(const CInv& inv);
     bool GetInstantSendLockByHash(const uint256& hash, CInstantSendLock& ret);