Merge #17258: Fix issue with conflicted mempool tx in listsinceblock

436ad436434b94982bcb7dc1d13a21949263ef73 Fix issue with conflicted mempool tx in listsinceblock (Adam Jonas)

Pull request description:

  Closes #8752 by bringing back abandoned #10470.

  This now checks that returned transactions are not conflicting with any transactions that are filtered out by the given blockhash and add a functional test to prevent this in the future.

  For more context, #8757 was closed in favor of #10470.

ACKs for top commit:
  instagibbs:
    utACK 436ad43643
  kallewoof:
    utACK 436ad436434b94982bcb7dc1d13a21949263ef73
  jonatack:
    I'm not qualifed to give an ACK here but 436ad436434b94982bcb7dc1d13a21949263ef73 appears reasonable. Built/ran tests/verified that this test fails without the change in rpcwallet.cpp:

Tree-SHA512: 63d75cd3d3f19fc84dc38899b200c96179b82b24db263cd0116ee5b715265be647157855c2e35912d2fbc49c7b37db9375d6aab0ac672f0f09bece8431de5ea9

src/wallet/rpcwallet.cpp

@@ -1621,7 +1621,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request)
     for (const std::pair<const uint256, CWalletTx>& pairWtx : pwallet->mapWallet) {
         CWalletTx tx = pairWtx.second;
 
-        if (depth == -1 || tx.GetDepthInMainChain() < depth) {
+        if (depth == -1 || abs(tx.GetDepthInMainChain()) < depth) {
             ListTransactions(pwallet, tx, 0, true, transactions, filter, nullptr /* filter_label */);
         }
     }

test/functional/wallet_listsinceblock.py

@@ -5,13 +5,17 @@
 """Test the listsincelast RPC."""
 
 from test_framework.test_framework import BitcoinTestFramework
+from test_framework.messages import BIP125_SEQUENCE_NUMBER
 from test_framework.util import (
     assert_array_result,
     assert_equal,
     assert_raises_rpc_error,
     connect_nodes,
+    isolate_node,
+    reconnect_isolated_node,
 )
 
+from decimal import Decimal
 
 class ListSinceBlockTest(BitcoinTestFramework):
     def set_test_params(self):
@@ -33,6 +37,7 @@ class ListSinceBlockTest(BitcoinTestFramework):
         self.test_reorg()
         self.test_double_spend()
         self.test_double_send()
+        self.double_spends_filtered()
 
     def test_no_blockhash(self):
         txid = self.nodes[2].sendtoaddress(self.nodes[0].getnewaddress(), 1)
@@ -289,5 +294,65 @@ class ListSinceBlockTest(BitcoinTestFramework):
             if tx['txid'] == txid1:
                 assert_equal(tx['confirmations'], 2)
 
+    def double_spends_filtered(self):
+        '''
+        `listsinceblock` was returning conflicted transactions even if they
+        occurred before the specified cutoff blockhash
+        '''
+        spending_node = self.nodes[2]
+        double_spending_node = self.nodes[3]
+        dest_address = spending_node.getnewaddress()
+
+        tx_input = dict(
+            sequence=BIP125_SEQUENCE_NUMBER, **next(u for u in spending_node.listunspent()))
+        rawtx = spending_node.createrawtransaction(
+            [tx_input], {dest_address: tx_input["amount"] - Decimal("0.00051000"),
+                         spending_node.getrawchangeaddress(): Decimal("0.00050000")})
+        double_rawtx = spending_node.createrawtransaction(
+            [tx_input], {dest_address: tx_input["amount"] - Decimal("0.00052000"),
+                         spending_node.getrawchangeaddress(): Decimal("0.00050000")})
+
+        isolate_node(double_spending_node)
+
+        signedtx = spending_node.signrawtransactionwithwallet(rawtx)
+        orig_tx_id = spending_node.sendrawtransaction(signedtx["hex"])
+        original_tx = spending_node.gettransaction(orig_tx_id)
+
+        double_signedtx = spending_node.signrawtransactionwithwallet(double_rawtx)
+        dbl_tx_id = double_spending_node.sendrawtransaction(double_signedtx["hex"])
+        double_tx = double_spending_node.getrawtransaction(dbl_tx_id, 1)
+        lastblockhash = double_spending_node.generate(1)[0]
+
+        reconnect_isolated_node(double_spending_node, 2)
+        self.sync_all()
+        spending_node.invalidateblock(lastblockhash)
+
+        # check that both transactions exist
+        block_hash = spending_node.listsinceblock(
+            spending_node.getblockhash(spending_node.getblockcount()))
+        original_found = False
+        double_found = False
+        for tx in block_hash['transactions']:
+            if tx['txid'] == original_tx['txid']:
+                original_found = True
+            if tx['txid'] == double_tx['txid']:
+                double_found = True
+        assert_equal(original_found, True)
+        assert_equal(double_found, True)
+
+        lastblockhash = spending_node.generate(1)[0]
+
+        # check that neither transaction exists
+        block_hash = spending_node.listsinceblock(lastblockhash)
+        original_found = False
+        double_found = False
+        for tx in block_hash['transactions']:
+            if tx['txid'] == original_tx['txid']:
+                original_found = True
+            if tx['txid'] == double_tx['txid']:
+                double_found = True
+        assert_equal(original_found, False)
+        assert_equal(double_found, False)
+
 if __name__ == '__main__':
     ListSinceBlockTest().main()