Implement retroactive IS locking of transactions first seen in blocks instead of mempool

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 (posInBlock == 0) {
+        // coinbase can't 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);

src/net_processing.cpp

@@ -2164,10 +2164,6 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
                 instantsend.Vote(tx.GetHash(), connman);
             }
 
-            if (nInvType != MSG_TXLOCK_REQUEST) {
-                llmq::quorumInstantSendManager->ProcessTx(tx, chainparams.GetConsensus());
-            }
-
             mempool.check(pcoinsTip);
             connman.RelayTransaction(tx);
             for (unsigned int i = 0; i < tx.vout.size(); i++) {
@@ -2212,8 +2208,6 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
                             vWorkQueue.emplace_back(orphanHash, i);
                         }
                         vEraseQueue.push_back(orphanHash);
-
-                        llmq::quorumInstantSendManager->ProcessTx(orphanTx, chainparams.GetConsensus());
                     }
                     else if (!fMissingInputs2)
                     {

src/rpc/rawtransaction.cpp

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