Ask for locked TXs after removing conflicting TXs (#2898)

* Also test conflicts in mempool instead of only in blocks

* Ask for locked TXs after removing conflicting TXs

When we removed a conflicting TX from the mempool, the correct/locked TX
is not available locally as the first-seen rule would have filtered before.
We need to re-request this TX if any other node announced it before.

* Apply suggestions from code review

Co-Authored-By: codablock <ablock84@gmail.com>

qa/rpc-tests/p2p-instantsend.py

@@ -26,7 +26,7 @@ def run_test(self):
         self.mine_quorum()
 
         self.log.info("Test old InstantSend")
-        self.test_doublespend()
+        self.test_block_doublespend()
 
         # Generate 6 block to avoid retroactive signing overloading Travis
         self.nodes[0].generate(6)
@@ -36,23 +36,21 @@ def run_test(self):
         self.wait_for_sporks_same()
 
         self.log.info("Test new InstantSend")
-        self.test_doublespend()
+        self.test_mempool_doublespend()
+        self.test_block_doublespend()
 
-    def test_doublespend(self, new_is = False):
+    def test_block_doublespend(self):
         sender = self.nodes[self.sender_idx]
         receiver = self.nodes[self.receiver_idx]
         isolated = self.nodes[self.isolated_idx]
+
         # feed the sender with some balance
         sender_addr = sender.getnewaddress()
         self.nodes[0].sendtoaddress(sender_addr, 1)
-        # make sender funds mature for InstantSend
-        for i in range(0, 2):
-            set_mocktime(get_mocktime() + 1)
-            set_node_times(self.nodes, get_mocktime())
-            self.nodes[0].generate(1)
+        self.nodes[0].generate(2)
         self.sync_all()
 
-        # create doublepending transaction,  but don't relay it
+        # create doublespending transaction, but don't relay it
         dblspnd_tx = self.create_raw_tx(sender, isolated, 0.5, 1, 100)
         # stop one node to isolate it from network
         isolated.setnetworkactive(False)
@@ -89,5 +87,39 @@ def test_doublespend(self, new_is = False):
         self.nodes[0].generate(2)
         self.sync_all()
 
+    def test_mempool_doublespend(self):
+        sender = self.nodes[self.sender_idx]
+        receiver = self.nodes[self.receiver_idx]
+        isolated = self.nodes[self.isolated_idx]
+
+        # feed the sender with some balance
+        sender_addr = sender.getnewaddress()
+        self.nodes[0].sendtoaddress(sender_addr, 1)
+        self.nodes[0].generate(2)
+        self.sync_all()
+
+        # create doublespending transaction, but don't relay it
+        dblspnd_tx = self.create_raw_tx(sender, isolated, 0.5, 1, 100)
+        dblspnd_txid = bytes_to_hex_str(hash256(hex_str_to_bytes(dblspnd_tx['hex']))[::-1])
+        # isolate one node from network
+        isolated.setnetworkactive(False)
+        # send doublespend transaction to isolated node
+        isolated.sendrawtransaction(dblspnd_tx['hex'])
+        # let isolated node rejoin the network
+        # The previously isolated node should NOT relay the doublespending TX
+        isolated.setnetworkactive(True)
+        for i in range(0, self.isolated_idx):
+            connect_nodes(self.nodes[i], self.isolated_idx)
+        for node in self.nodes:
+            if node is not isolated:
+                assert_raises_jsonrpc(-5, "No such mempool or blockchain transaction", node.getrawtransaction, dblspnd_txid)
+        # instantsend to receiver. The previously isolated node should prune the doublespend TX and request the correct
+        # TX from other nodes.
+        receiver_addr = receiver.getnewaddress()
+        is_id = sender.instantsendtoaddress(receiver_addr, 0.9)
+        for node in self.nodes:
+            self.wait_for_instantlock(is_id, node)
+        assert_raises_jsonrpc(-5, "No such mempool or blockchain transaction", isolated.getrawtransaction, dblspnd_txid)
+
 if __name__ == '__main__':
     InstantSendTest().main()

src/llmq/quorums_instantsend.cpp

@@ -1001,25 +1001,56 @@ void CInstantSendManager::HandleFullyConfirmedBlock(const CBlockIndex* pindex)
 
 void CInstantSendManager::RemoveMempoolConflictsForLock(const uint256& hash, const CInstantSendLock& islock)
 {
-    LOCK(mempool.cs);
-
     std::unordered_map<uint256, CTransactionRef> toDelete;
 
-    for (auto& in : islock.inputs) {
-        auto it = mempool.mapNextTx.find(in);
-        if (it == mempool.mapNextTx.end()) {
-            continue;
+    {
+        LOCK(mempool.cs);
+
+        for (auto& in : islock.inputs) {
+            auto it = mempool.mapNextTx.find(in);
+            if (it == mempool.mapNextTx.end()) {
+                continue;
+            }
+            if (it->second->GetHash() != islock.txid) {
+                toDelete.emplace(it->second->GetHash(), mempool.get(it->second->GetHash()));
+
+                LogPrintf("CInstantSendManager::%s -- txid=%s, islock=%s: mempool TX %s with input %s conflicts with islock\n", __func__,
+                         islock.txid.ToString(), hash.ToString(), it->second->GetHash().ToString(), in.ToStringShort());
+            }
         }
-        if (it->second->GetHash() != islock.txid) {
-            toDelete.emplace(it->second->GetHash(), mempool.get(it->second->GetHash()));
 
-            LogPrintf("CInstantSendManager::%s -- txid=%s, islock=%s: mempool TX %s with input %s conflicts with islock\n", __func__,
-                     islock.txid.ToString(), hash.ToString(), it->second->GetHash().ToString(), in.ToStringShort());
+        for (auto& p : toDelete) {
+            mempool.removeRecursive(*p.second, MemPoolRemovalReason::CONFLICT);
         }
     }
 
-    for (auto& p : toDelete) {
-        mempool.removeRecursive(*p.second, MemPoolRemovalReason::CONFLICT);
+    if (!toDelete.empty()) {
+        AskNodesForLockedTx(islock.txid);
+    }
+}
+
+void CInstantSendManager::AskNodesForLockedTx(const uint256& txid)
+{
+    std::vector<CNode*> nodesToAskFor;
+    g_connman->ForEachNode([&](CNode* pnode) {
+        LOCK(pnode->cs_filter);
+        if (pnode->filterInventoryKnown.contains(txid)) {
+            pnode->AddRef();
+            nodesToAskFor.emplace_back(pnode);
+        }
+    });
+    {
+        LOCK(cs_main);
+        for (CNode* pnode : nodesToAskFor) {
+            LogPrintf("CInstantSendManager::%s -- txid=%s: asking other peer %d for correct TX\n", __func__,
+                      txid.ToString(), pnode->id);
+
+            CInv inv(MSG_TX, txid);
+            pnode->AskFor(inv);
+        }
+    }
+    for (CNode* pnode : nodesToAskFor) {
+        pnode->Release();
     }
 }
 

src/llmq/quorums_instantsend.h

@@ -145,6 +145,7 @@ class CInstantSendManager : public CRecoveredSigsListener
     void HandleFullyConfirmedBlock(const CBlockIndex* pindex);
 
     void RemoveMempoolConflictsForLock(const uint256& hash, const CInstantSendLock& islock);
+    void AskNodesForLockedTx(const uint256& txid);
     bool ProcessPendingRetryLockTxs();
 
     bool AlreadyHave(const CInv& inv);