Fix issue with conflicted mempool tx in listsinceblock

adamjonas · mchrostowski · adamjonas · commit 436ad436434b · 2019-10-28T10:26:46.000-04:00
listsinceblock now checks that returned transactions are not
conflicting with any transactions that are filtered out by
the given blockhash

Co-Authored-By: Michael Chrostowski &lt;michael.chrostowski@gmail.com&gt;
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
@@ -1591,7 +1591,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(*locked_chain) < depth) {
+        if (depth == -1 || abs(tx.GetDepthInMainChain(*locked_chain)) < depth) {
             ListTransactions(*locked_chain, pwallet, tx, 0, true, transactions, filter, nullptr /* filter_label */);
         }
     }
diff --git a/test/functional/wallet_listsinceblock.py b/test/functional/wallet_listsinceblock.py
@@ -5,13 +5,15 @@
 """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,
 )
 
+from decimal import Decimal
 
 class ListSinceBlockTest(BitcoinTestFramework):
     def set_test_params(self):
@@ -33,6 +35,7 @@ def run_test(self):
         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)
@@ -291,5 +294,51 @@ def test_double_send(self):
             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]
+        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")})
+        signedtx = spending_node.signrawtransactionwithwallet(rawtx)
+        orig_tx_id = spending_node.sendrawtransaction(signedtx["hex"])
+        original_tx = spending_node.gettransaction(orig_tx_id)
+
+        double_tx = spending_node.bumpfee(orig_tx_id)
+
+        # 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()

Original file line number	Diff line number	Diff line change
`@@ -1591,7 +1591,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request)`
`1591`	`1591`	`for (const std::pair<const uint256, CWalletTx>& pairWtx : pwallet->mapWallet) {`
`1592`	`1592`	`CWalletTx tx = pairWtx.second;`
`1593`	`1593`
`1594`		`- if (depth == -1 \|\| tx.GetDepthInMainChain(*locked_chain) < depth) {`
	`1594`	`+ if (depth == -1 \|\| abs(tx.GetDepthInMainChain(*locked_chain)) < depth) {`
`1595`	`1595`	`ListTransactions(locked_chain, pwallet, tx, 0, true, transactions, filter, nullptr / filter_label */);`
`1596`	`1596`	`}`
`1597`	`1597`	`}`