Merge bitcoin/bitcoin#29179: test: wallet rescan with reorged parent + IsFromMe child in mempool

df30247705940c50c5eaafd74e2abbeb8b0cec07 [test] import descriptor wallet with reorged parent + IsFromMe child in mempool (glozow)
c3d02be536ac3f35c10efa03653186a17ebbfc12 [test] rescan legacy wallet with reorged parent + IsFromMe child in mempool (Gloria Zhao)

Pull request description:

  Originally motivated by #29019, which reverts back to having `requestMempoolTransactions` emit `transactionAddedToMempool` in `mapTx` default order instead of `GetSortedDepthAndScore` order.

  It's important that these notifications happen in topological order, otherwise the wallet rescan may miss transactions that belong to it. Notably, checking whether a transaction `IsFromMe` requires knowing its inputs, which may be from a mempool parent.

  When using `mapTx` order, a parent may come later than its child if it was added from a block disconnected in a reorg.

  This PR adds a test for this case.

ACKs for top commit:
  achow101:
    ACK df30247705940c50c5eaafd74e2abbeb8b0cec07
  furszy:
    Code review ACK df30247705, nits can be disregarded.

Tree-SHA512: 2f1d9ef92313228adbbef94e634e5f7a9ec6e6a2c88e16aa343bdc95ffc9b9f9c82a569b412c9a3841db9d789e52f9283e8b9385731668d59355903e26e58a5d

3 files changed, 131 insertions, 9 deletions

diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index 826705598f..9c8f0eca26 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -336,6 +336,7 @@ BASE_SCRIPTS = [
     'wallet_create_tx.py --descriptors',
     'wallet_inactive_hdchains.py --legacy-wallet',
     'wallet_spend_unconfirmed.py',
+    'wallet_rescan_unconfirmed.py --descriptors',
     'p2p_fingerprint.py',
     'feature_uacomment.py',
     'feature_init.py',
diff --git a/test/functional/wallet_import_rescan.py b/test/functional/wallet_import_rescan.py
index 0ac67607e1..7f01d23941 100755
--- a/test/functional/wallet_import_rescan.py
+++ b/test/functional/wallet_import_rescan.py
@@ -20,7 +20,10 @@ happened previously.
 """
 
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.address import AddressType
+from test_framework.address import (
+    AddressType,
+    ADDRESS_BCRT1_UNSPENDABLE,
+)
 from test_framework.util import (
     assert_equal,
     set_node_times,
@@ -109,7 +112,7 @@ class Variant(collections.namedtuple("Variant", "call data address_type rescan p
 
             address, = [ad for ad in addresses if txid in ad["txids"]]
             assert_equal(address["address"], self.address["address"])
-            assert_equal(address["amount"], self.expected_balance)
+            assert_equal(address["amount"], self.amount_received)
             assert_equal(address["confirmations"], confirmations)
             # Verify the transaction is correctly marked watchonly depending on
             # whether the transaction pays to an imported public key or
@@ -223,11 +226,11 @@ class ImportRescanTest(BitcoinTestFramework):
             variant.node = self.nodes[2 + IMPORT_NODES.index(ImportNode(variant.prune, expect_rescan))]
             variant.do_import(variant.timestamp)
             if expect_rescan:
-                variant.expected_balance = variant.initial_amount
+                variant.amount_received = variant.initial_amount
                 variant.expected_txs = 1
                 variant.check(variant.initial_txid, variant.initial_amount, variant.confirmation_height)
             else:
-                variant.expected_balance = 0
+                variant.amount_received = 0
                 variant.expected_txs = 0
                 variant.check()
 
@@ -247,7 +250,7 @@ class ImportRescanTest(BitcoinTestFramework):
         # Check the latest results from getbalance and listtransactions.
         for variant in IMPORT_VARIANTS:
             self.log.info('Run check for variant {}'.format(variant))
-            variant.expected_balance += variant.sent_amount
+            variant.amount_received += variant.sent_amount
             variant.expected_txs += 1
             variant.check(variant.sent_txid, variant.sent_amount, variant.confirmation_height)
 
@@ -267,14 +270,45 @@ class ImportRescanTest(BitcoinTestFramework):
                 address_type=variant.address_type.value,
             ))
             variant.key = self.nodes[1].dumpprivkey(variant.address["address"])
-            variant.initial_amount = get_rand_amount()
+            variant.initial_amount = get_rand_amount() * 2
             variant.initial_txid = self.nodes[0].sendtoaddress(variant.address["address"], variant.initial_amount)
             variant.confirmation_height = 0
             variant.timestamp = timestamp
 
+        # Mine a block so these parents are confirmed
+        assert_equal(len(self.nodes[0].getrawmempool()), len(mempool_variants))
+        self.sync_mempools()
+        block_to_disconnect = self.generate(self.nodes[0], 1)[0]
+        assert_equal(len(self.nodes[0].getrawmempool()), 0)
+
+        # For each variant, create an unconfirmed child transaction from initial_txid, sending all
+        # the funds to an unspendable address. Importantly, no change output is created so the
+        # transaction can't be recognized using its outputs. The wallet rescan needs to know the
+        # inputs of the transaction to detect it, so the parent must be processed before the child.
+        # An equivalent test for descriptors exists in wallet_rescan_unconfirmed.py.
+        unspent_txid_map = {txin["txid"] : txin for txin in self.nodes[1].listunspent()}
+        for variant in mempool_variants:
+            # Send full amount, subtracting fee from outputs, to ensure no change is created.
+            child = self.nodes[1].send(
+                add_to_wallet=False,
+                inputs=[unspent_txid_map[variant.initial_txid]],
+                outputs=[{ADDRESS_BCRT1_UNSPENDABLE : variant.initial_amount}],
+                subtract_fee_from_outputs=[0]
+            )
+            variant.child_txid = child["txid"]
+            variant.amount_received = 0
+            self.nodes[0].sendrawtransaction(child["hex"])
+
+        # Mempools should contain the child transactions for each variant.
         assert_equal(len(self.nodes[0].getrawmempool()), len(mempool_variants))
         self.sync_mempools()
 
+        # Mock a reorg so the parent transactions are added back to the mempool
+        for node in self.nodes:
+            node.invalidateblock(block_to_disconnect)
+            # Mempools should now contain the parent and child for each variant.
+            assert_equal(len(node.getrawmempool()), 2 * len(mempool_variants))
+
         # For each variation of wallet key import, invoke the import RPC and
         # check the results from getbalance and listtransactions.
         for variant in mempool_variants:
@@ -283,11 +317,15 @@ class ImportRescanTest(BitcoinTestFramework):
             variant.node = self.nodes[2 + IMPORT_NODES.index(ImportNode(variant.prune, expect_rescan))]
             variant.do_import(variant.timestamp)
             if expect_rescan:
-                variant.expected_balance = variant.initial_amount
+                # Ensure both transactions were rescanned. This would raise a JSONRPCError if the
+                # transactions were not identified as belonging to the wallet.
+                assert_equal(variant.node.gettransaction(variant.initial_txid)['confirmations'], 0)
+                assert_equal(variant.node.gettransaction(variant.child_txid)['confirmations'], 0)
+                variant.amount_received = variant.initial_amount
                 variant.expected_txs = 1
-                variant.check(variant.initial_txid, variant.initial_amount)
+                variant.check(variant.initial_txid, variant.initial_amount, 0)
             else:
-                variant.expected_balance = 0
+                variant.amount_received = 0
                 variant.expected_txs = 0
                 variant.check()
 
diff --git a/test/functional/wallet_rescan_unconfirmed.py b/test/functional/wallet_rescan_unconfirmed.py
new file mode 100755
index 0000000000..ad9fa081c2
--- /dev/null
+++ b/test/functional/wallet_rescan_unconfirmed.py
@@ -0,0 +1,83 @@
+#!/usr/bin/env python3
+# Copyright (c) 2024 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test that descriptor wallets rescan mempool transactions properly when importing."""
+
+from test_framework.address import (
+    address_to_scriptpubkey,
+    ADDRESS_BCRT1_UNSPENDABLE,
+)
+from test_framework.messages import COIN
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_equal
+from test_framework.wallet import MiniWallet
+from test_framework.wallet_util import test_address
+
+
+class WalletRescanUnconfirmed(BitcoinTestFramework):
+    def add_options(self, parser):
+        self.add_wallet_options(parser, legacy=False)
+
+    def set_test_params(self):
+        self.num_nodes = 1
+
+    def skip_test_if_missing_module(self):
+        self.skip_if_no_wallet()
+        self.skip_if_no_sqlite()
+
+    def run_test(self):
+        self.log.info("Create wallets and mine initial chain")
+        node = self.nodes[0]
+        tester_wallet = MiniWallet(node)
+
+        node.createwallet(wallet_name='w0', disable_private_keys=False)
+        w0 = node.get_wallet_rpc('w0')
+
+        self.log.info("Create a parent tx and mine it in a block that will later be disconnected")
+        parent_address = w0.getnewaddress()
+        tx_parent_to_reorg = tester_wallet.send_to(
+            from_node=node,
+            scriptPubKey=address_to_scriptpubkey(parent_address),
+            amount=COIN,
+        )
+        assert tx_parent_to_reorg["txid"] in node.getrawmempool()
+        block_to_reorg = self.generate(tester_wallet, 1)[0]
+        assert_equal(len(node.getrawmempool()), 0)
+        node.syncwithvalidationinterfacequeue()
+        assert_equal(w0.gettransaction(tx_parent_to_reorg["txid"])["confirmations"], 1)
+
+        # Create an unconfirmed child transaction from the parent tx, sending all
+        # the funds to an unspendable address. Importantly, no change output is created so the
+        # transaction can't be recognized using its outputs. The wallet rescan needs to know the
+        # inputs of the transaction to detect it, so the parent must be processed before the child.
+        w0_utxos = w0.listunspent()
+
+        self.log.info("Create a child tx and wait for it to propagate to all mempools")
+        # The only UTXO available to spend is tx_parent_to_reorg.
+        assert_equal(len(w0_utxos), 1)
+        assert_equal(w0_utxos[0]["txid"], tx_parent_to_reorg["txid"])
+        tx_child_unconfirmed_sweep = w0.sendall([ADDRESS_BCRT1_UNSPENDABLE])
+        assert tx_child_unconfirmed_sweep["txid"] in node.getrawmempool()
+        node.syncwithvalidationinterfacequeue()
+
+        self.log.info("Mock a reorg, causing parent to re-enter mempools after its child")
+        node.invalidateblock(block_to_reorg)
+        assert tx_parent_to_reorg["txid"] in node.getrawmempool()
+
+        self.log.info("Import descriptor wallet on another node")
+        descriptors_to_import = [{"desc": w0.getaddressinfo(parent_address)['parent_desc'], "timestamp": 0, "label": "w0 import"}]
+
+        node.createwallet(wallet_name="w1", disable_private_keys=True)
+        w1 = node.get_wallet_rpc("w1")
+        w1.importdescriptors(descriptors_to_import)
+
+        self.log.info("Check that the importing node has properly rescanned mempool transactions")
+        # Check that parent address is correctly determined as ismine
+        test_address(w1, parent_address, solvable=True, ismine=True)
+        # This would raise a JSONRPCError if the transactions were not identified as belonging to the wallet.
+        assert_equal(w1.gettransaction(tx_parent_to_reorg["txid"])["confirmations"], 0)
+        assert_equal(w1.gettransaction(tx_child_unconfirmed_sweep["txid"])["confirmations"], 0)
+
+if __name__ == '__main__':
+    WalletRescanUnconfirmed().main()