wallet, test: wallet_fast_rescan follow-ups #34907

rkrux commented at 10:40 AM on March 24, 2026: contributor

This patch addresses my own review comments from the review of PR 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 via asserting debug logs that the topup is being indeed done.
I think it's also helpful to highlight when the top is being done exactly, which is when the wallet detects the concerned transaction for the first time, that is when the transaction is first broadcast. So this patch separates the transaction broadcast and block generation flows to highlight this. This also helps in generation all the blocks in one loop instead of being staggered.
Separation of the flows also allows us to unload the wallet before blocks generation, which is not being done at all in the test currently.
The transaction sending to the non-ranged descriptor derives its address kind of out of band because it doesn't use the wallet that already has imported it. This patch addresses it.
One extra block is also generated containing only non-wallet transactions.
There are some other minor changes done that made sense after all the above changes but I can remove them if others find them not helpful.

DrahtBot commented at 10:40 AM on March 24, 2026: contributor

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

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

wallet, test: wallet_fast_rescan follow-ups

This patch addresses my own review comments from the review of PR 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 via asserting debug logs that the topup is
being indeed done.
- I think it's also helpful to highlight when the top is being done exactly,
which is when the wallet detects the concerned transaction for the first time,
that is when the transaction is first broadcast. So this patch separates the
transaction broadcast and block generation flows to highlight this. This also
helps in generating all the blocks in one go instead of being staggered.
- Separation of the flows also allows us to unload the wallet before blocks
generation, which is not being done at all in the test currently.
- The transaction sending to the non-ranged descriptor derives its address kind
of out of band because it doesn't use the wallet that already has imported it.
This patch addresses it.
- One extra block is also generated containing only non-wallet transactions.
- There are some other minor changes done that made sense after all the above
changes but I can remove them if others find them not helpful.

735e9f610b

rkrux force-pushed on Mar 24, 2026

DrahtBot added the label CI failed on Mar 24, 2026

DrahtBot removed the label CI failed on Mar 24, 2026

w0xlt commented at 6:34 PM on March 25, 2026: contributor

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.

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>

w0xlt commented at 7:59 PM on March 25, 2026: contributor

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.

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>

in test/functional/wallet_fast_rescan.py:85 in 735e9f610b

 104 | +        # doesn't needlessly process block generation notifications in the background
 105 | +        w.unloadwallet()
 106 | +
 107 | +        self.log.info("Create and broadcast txs unrelated to the wallet")
 108 | +        send_result = funding_wallet.send_self_transfer_chain(from_node=node, chain_length=3)
 109 | +        txs_per_blocks.append([tx_result['txid'] for tx_result in send_result])

w0xlt commented at 8:07 PM on March 25, 2026:

This part seems more complex than the behavior being asserted. If the goal is just to include a block with wallet-unrelated transactions, would a single unrelated tx be enough ? That would make the intent clearer.

        self.log.info("Create and broadcast tx unrelated to the wallet")
        send_result = funding_wallet.send_self_transfer(from_node=node)
        txs_per_blocks.append([send_result['txid']])

rkrux commented at 3:22 PM on March 27, 2026:

This was intentional on my part but I see it can come across as confusing and is also not necessary. I will remove the chain.

in test/functional/wallet_fast_rescan.py:51 in 735e9f610b

  48 | +
  49 | +        # backup the wallet here so that the restorations later are unaware of the below transactions
  50 |          w.backupwallet(WALLET_BACKUP_FILENAME)
  51 |  
  52 | -        num_txs = 0
  53 | +        txs_per_blocks = []

w0xlt commented at 8:08 PM on March 25, 2026:

nit:

        txids_per_block = []

rkrux commented at 3:22 PM on March 27, 2026: contributor

Good suggestions, I will add them.