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

nit: could be a bit more efficient by only calling and iterating over listunspent once (does not test for txid collisions, but I don't think we need that?)

<details> <summary>git diff on 1a52ca7619</summary>

diff --git a/test/functional/wallet_import_rescan.py b/test/functional/wallet_import_rescan.py
index 1abc9c0c79..928455c355 100755
--- a/test/functional/wallet_import_rescan.py
+++ b/test/functional/wallet_import_rescan.py
@@ -286,13 +286,12 @@ class ImportRescanTest(BitcoinTestFramework):
         # itself and the other half to unspendable/fees. This transaction is only recognizable as
         # belonging to this wallet when the inputs are known. That means the rescan must process the
         # parent before the child.
+        unspent_txid_map = {txin["txid"] : txin for txin in self.nodes[1].listunspent()}
         for variant in mempool_variants:
-            inputs_to_spend = [txin for txin in self.nodes[1].listunspent() if txin["txid"] == variant.initial_txid]
-            assert_equal(len(inputs_to_spend), 1)
             # Send full amount, subtracting fee from outputs, to ensure no change is created.
             child = self.nodes[1].send(
                 add_to_wallet=False,
-                inputs=inputs_to_spend,
+                inputs=[unspent_txid_map[variant.initial_txid]],
                 outputs=[{ADDRESS_BCRT1_UNSPENDABLE : variant.initial_amount}],
                 subtract_fee_from_outputs=[0]
             )

</details>

taken

nit: makes more sense to unpack here imo

        block_to_disconnect = self.generate(self.nodes[0], 1)[0]

I agree, taken

nit: I don't think we're actually checking the confirmations values, so for future maintenance, would this be more straightforward?

                variant.node.gettransaction(variant.initial_txid)  # raises if tx wasn't rescanned
                variant.node.gettransaction(variant.child_txid)    # raises if tx wasn't rescanned

Added a similar comment.

It doesn't seem like we're actually testing the expected balance at any point, rather the amount received, but I may be missing nuance. Would this diff make sense? I don't like adding an extra parameter to an already long function for something so specific, if we can avoid it.

<details> <summary>git diff on 1a52ca7619</summary>

diff --git a/test/functional/wallet_import_rescan.py b/test/functional/wallet_import_rescan.py
index 1abc9c0c79..fa9f43f66c 100755
--- a/test/functional/wallet_import_rescan.py
+++ b/test/functional/wallet_import_rescan.py
@@ -82,7 +82,7 @@ class Variant(collections.namedtuple("Variant", "call data address_type rescan p
             )
             assert_equal(response, [{"success": True}])
 
-    def check(self, txid=None, amount=None, confirmation_height=None, amount_is_balance=True):
+    def check(self, txid=None, amount=None, confirmation_height=None):
         """Verify that listtransactions/listreceivedbyaddress return expected values."""
 
         txs = self.node.listtransactions(label=self.label, count=10000, include_watchonly=True)
@@ -112,8 +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"])
-            if amount_is_balance:
-                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
@@ -227,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()
 
@@ -251,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)
 
@@ -297,7 +296,6 @@ class ImportRescanTest(BitcoinTestFramework):
                 subtract_fee_from_outputs=[0]
             )
             variant.child_txid = child["txid"]
-            variant.expected_balance = 0
             self.nodes[0].sendrawtransaction(child["hex"])
 
         # Mempools should contain the child transactions for each variant.
@@ -321,10 +319,11 @@ class ImportRescanTest(BitcoinTestFramework):
                 # Ensure both transactions were rescanned
                 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, 0, False)
+                variant.check(variant.initial_txid, variant.initial_amount, 0)
             else:
-                variant.expected_balance = 0
+                variant.amount_received = 0
                 variant.expected_txs = 0
                 variant.check()
 

</details>

Thanks, taken.

grammar nit

        # Node which we will use to send funds to the wallet and mine blocks

taken

Is there a reason it's 110 and not COINBASE_MATURITY?

changed

nit: I still think it's unnecessarily confusing for (current and future) reviewers to figure out why confirmations needs to equal 0 when afaiu this is completely unrelated to the test. I would either remove the value comparison or specifically state in the docs that we don't care about the confirmations value, just that the response is a JSON object with a field we recognize?

We are testing that they are marked as in-mempool wallet transactions. If the reorg didn't happen, the value would be 1.

Makes sense, thank you! I failed to appreciate that we're calling the wallet gettransaction RPC, and not the not getrawtransaction RPC.

Ah yeah, it's a wallet-specific RPC. I've added a check that the "confirmations" value is 1 on wallet 0 a few lines up, prior to the reorg. Hopefully that also helps make it clear why we're testing this way.

probably should be removed?

Oops! Removed.

What do you think about adding one line to these docs in each file to reference the other file? Might be useful to e.g. keep tests in sync if problems are found, or when people wonder why this is (at first sight) only tested for legacy or descriptor wallets.

        # inputs of the transaction to detect it, so the parent must be processed before the child.
        # This behaviour is tested for descriptor wallets in wallet_rescan_unconfirmed.py.

Added this comment in the legacy test as I agree it'll be convenient to know what coverage we are/aren't losing when deleting a legacy test.

in c39e82926a:

tiny nit: looks redundant to call add_wallet_options(legacy=False) and then call skip_if_no_sqlite(). If the wallet is enabled and sqlite is not supported, then the only option is a legacy wallet, which is blocked by the add_wallet_options call.

Seems pretty common to specify both. They are technically different; one says no legacy and the other says need descriptors. I know that's effectively the same thing and we don't have 3 types, but I think it's harmless to keep this.

in https://github.com/bitcoin/bitcoin/commit/c39e82926a3740642f8f1bb72c6b10b67a6dc724:

nit: disable_private_keys is false by default. And, because this is a descriptors-only test (per self.add_wallet_options), the descriptors flag is also redundant.

I initially left the same comment about disable_private_keys but think it is helpful to contrast the difference with further down the test where we set disable_private_keys=True. Likewise, I think keeping descriptors=True is helpful self-documentation given the contrast with the test in wallet_import_rescan.py. No strong opinion on either, though.

dropped the extra args (though I tend to prefer being a bit more verbose and self-documenting)

tiny nit: this could be sync_mempools instead.

done

In https://github.com/bitcoin/bitcoin/commit/c39e82926a3740642f8f1bb72c6b10b67a6dc724:

same as above, the descriptors flag isn't needed. Also, if you set disable_private_keys=True, there is no need to set blank=True.

dropped the args

In c39e82926a3:

Can simplify this by only importing the used address descriptor.

descriptors_to_import = [{"desc": w0.getaddressinfo(parent_address)['parent_desc'], "timestamp": 0, "label": "w0 import"}]

nice, taken

In https://github.com/bitcoin/bitcoin/commit/c39e82926a3740642f8f1bb72c6b10b67a6dc724:

Unless you specify a clean chain (which this test does not), the test will have an existing matured one. Which means, you could modify this for

        tester_wallet = MiniWallet(tester_node)
        self.generate(tester_wallet, 1)

Keeping as is because I prefer the peace of mind of knowing tester_wallet has a mature coinbase.

Keeping as is because I prefer the peace of mind of knowing tester_wallet has a mature coinbase.

You shouldn't be concerned about that. See code. If the wallet wouldn't be having mature coins, the test would be always failing due a lack of funds.

or well, np. Its fine.

Deleted this line altogether, there's no reason to generate 1 either.

nit: leftover

        self.log.info("Create a child tx and wait until all wallets are notified")

nit: leftover; should be "import descriptor on another wallet"