wallet, test: wallet_fast_rescan follow-ups #34907

This patch addresses my own review comments from the review of #34667 and adds some more changes that I find helpful. It was observed in the review of the earlier PR that there is a tendency for the test code to cause the topups not being done that defeats the purpose of the test. Combine that with the earlier issue where the block filter was not being updated ever since this test was written, I think it's helpful that some robustness is added in the test.

Exact details are in the commit message.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34907.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

The test already records the txids it creates, but at the end it only checks counts and that fast/slow agree with each other.

It would be stronger to flatten the generated wallet-related txids and assert each rescan result equals that exact expected set.

<details> <summary>diff</summary>

diff --git a/test/functional/wallet_fast_rescan.py b/test/functional/wallet_fast_rescan.py
index b777ed9ced..683443d8a7 100755
--- a/test/functional/wallet_fast_rescan.py
+++ b/test/functional/wallet_fast_rescan.py
@@ -85,7 +85,7 @@ class WalletFastRescanTest(BitcoinTestFramework):
         txs_per_blocks.append([tx_result['txid'] for tx_result in send_result])
 
         self.log.info("Start generating blocks one by one with associated txs")
-        total_wallet_txs = 0
+        expected_wallet_txids = []
         fast_rescan_messages = []
         for i, block_txs in enumerate(txs_per_blocks):
             self.log.info(f"Block {i+1}/{NUM_BLOCKS}")
@@ -94,7 +94,7 @@ class WalletFastRescanTest(BitcoinTestFramework):
             # transactions - this block may or may not be fetched due to block filters false positives.
             if i < NUM_BLOCKS-1:
                 fast_rescan_messages.append(f"Fast rescan: inspect block {self.nodes[0].getblockcount()} [{generated_block['hash']}] (filter matched)")
-                total_wallet_txs += len(block_txs)
+                expected_wallet_txids.extend(block_txs)
 
         self.log.info("Import wallet backup with block filter index")
         with node.assert_debug_log(['fast variant using block filters', *fast_rescan_messages]):
@@ -122,12 +122,11 @@ class WalletFastRescanTest(BitcoinTestFramework):
         txids_slow_nonactive = self.get_wallet_txids(node, 'rescan_slow_nonactive')
 
         self.log.info("Verify that all rescans found the same txs in slow and fast variants")
-        assert_equal(len(txids_slow), total_wallet_txs)
-        assert_equal(len(txids_fast), total_wallet_txs)
-        assert_equal(len(txids_slow_nonactive), total_wallet_txs)
-        assert_equal(len(txids_fast_nonactive), total_wallet_txs)
-        assert_equal(sorted(txids_slow), sorted(txids_fast))
-        assert_equal(sorted(txids_slow_nonactive), sorted(txids_fast_nonactive))
+        expected_wallet_txids.sort()
+        assert_equal(sorted(txids_slow), expected_wallet_txids)
+        assert_equal(sorted(txids_fast), expected_wallet_txids)
+        assert_equal(sorted(txids_slow_nonactive), expected_wallet_txids)
+        assert_equal(sorted(txids_fast_nonactive), expected_wallet_txids)
 
 
 if __name__ == '__main__':

</details>

The test doesn't need to depend on a specific debug-log string. You can assert the same behavior more directly via listdescriptors: get the descriptor's current end_range, send to that address, then verify the range increases.

<details> <summary>diff</summary>

diff --git a/test/functional/wallet_fast_rescan.py b/test/functional/wallet_fast_rescan.py
index b777ed9ced..f3d88c408a 100755
--- a/test/functional/wallet_fast_rescan.py
+++ b/test/functional/wallet_fast_rescan.py
@@ -48,6 +48,12 @@ class WalletFastRescanTest(BitcoinTestFramework):
         # backup the wallet here so that the restorations later are unaware of the below transactions
         w.backupwallet(WALLET_BACKUP_FILENAME)
 
+        def get_descriptor_end_range(desc_str):
+            for descriptor in w.listdescriptors()['descriptors']:
+                if descriptor['desc'] == desc_str:
+                    return descriptor['range'][1]
+            raise AssertionError(f"Descriptor not found: {desc_str}")
+
         txs_per_blocks = []
 
         self.log.info("Create and broadcast tx sending to non-ranged descriptor")
@@ -70,8 +76,8 @@ class WalletFastRescanTest(BitcoinTestFramework):
                 spk = address_to_scriptpubkey(addr)
 
                 self.log.debug(f"-> range [{start_range},{end_range}], last address {addr}")
-                with node.assert_debug_log([f'Detected a used keypool item at index {end_range}, mark all keypool items up to this item as used']):
-                    send_result = funding_wallet.send_to(from_node=node, scriptPubKey=spk, amount=10000)
+                send_result = funding_wallet.send_to(from_node=node, scriptPubKey=spk, amount=10000)
+                self.wait_until(lambda desc_str=desc_str, end_range=end_range: get_descriptor_end_range(desc_str) > end_range)
                 block_txs.append(send_result["txid"])
 
             txs_per_blocks.append(block_txs)

</details>

Good suggestions, I will add them.

Added all the suggestions from w0xlt's review in latest push.

I realised that wait_until is not needed for asserting that the top is being done because adding a simple greater than assertion post send_to call is sufficient, so made that change.

ACK 292a949863b4c13e1f88ac6f1b2622eaac7c4e70 with non-blocking suggestions.

Force pushed to use suggestions from @w0xlt's review: #34907#pullrequestreview-4340246369.

reACK bf0f5d9941fb251692af58c411362c5b0e4e8396