wallet, test: make wallet_fast_rescan robust #34907

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

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.

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.

Code Coverage & Benchmarks

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Bicaru20
Stale ACK	w0xlt

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

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.

rkrux force-pushed on Apr 24, 2026

rkrux commented at 2:40 PM on April 24, 2026: contributor

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

rkrux force-pushed on Apr 24, 2026

DrahtBot added the label CI failed on Apr 24, 2026

rkrux commented at 2:49 PM on April 24, 2026: contributor

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.

DrahtBot removed the label CI failed on Apr 24, 2026

in test/functional/wallet_fast_rescan.py:93 in 292a949863 outdated

 112 | +
 113 | +        self.log.info("Create and broadcast tx unrelated to the wallet")
 114 | +        send_result = funding_wallet.send_self_transfer(from_node=node)
 115 | +        txids_per_block.append([send_result['txid']])
 116 | +
 117 | +        self.log.info("Start generating blocks one by one with associated txs")

w0xlt commented at 8:04 PM on May 21, 2026:

nit:

        assert_equal(len(txids_per_block), NUM_BLOCKS)
        self.log.info("Start generating blocks one by one with associated txs")

in test/functional/wallet_fast_rescan.py:102 in 292a949863

 121 | +            self.log.info(f"Block {i+1}/{NUM_BLOCKS}")
 122 | +            generated_block = self.generateblock(node, output="raw(42)", transactions=block_tx_ids)
 123 | +            # Not asserting the fast rescan message in the last block because it contains wallet-unrelated
 124 | +            # transactions - this block may or may not be fetched due to block filters false positives.
 125 | +            if i < NUM_BLOCKS-1:
 126 | +                fast_rescan_messages.append(f"Fast rescan: inspect block {self.nodes[0].getblockcount()} [{generated_block['hash']}] (filter matched)")

w0xlt commented at 8:04 PM on May 21, 2026:

nit:

                fast_rescan_messages.append(f"Fast rescan: inspect block {node.getblockcount()} [{generated_block['hash']}] (filter matched)")

in test/functional/wallet_fast_rescan.py:67 in 292a949863

  81 | +        self.log.debug(f"-> fixed non-range descriptor address {addr}")
  82 | +        send_result = funding_wallet.send_to(from_node=node, scriptPubKey=spk, amount=10000)
  83 | +        txids_per_block.append([send_result["txid"]])
  84 | +
  85 | +        self.log.info("Create and broadcast txs sending to end range address of each descriptor, triggering top-ups")
  86 | +        for i in range(1, NUM_BLOCKS-1):

w0xlt commented at 8:04 PM on May 21, 2026:

nit:

        for _ in range(1, NUM_BLOCKS-1):

in test/functional/wallet_fast_rescan.py:6 in 292a949863

w0xlt commented at 8:04 PM on May 21, 2026:

nit:

-   top-ups correctly and finds the same transactions than the slow variant."""
+   top-ups correctly and finds the same transactions than as the slow variant."""

w0xlt commented at 8:05 PM on May 21, 2026: contributor

ACK 292a949863b4c13e1f88ac6f1b2622eaac7c4e70 with non-blocking suggestions.

rkrux force-pushed on May 22, 2026

rkrux commented at 10:05 AM on May 22, 2026: contributor

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

w0xlt commented at 5:22 PM on May 27, 2026: contributor

reACK bf0f5d9941fb251692af58c411362c5b0e4e8396

Bicaru20 commented at 1:50 PM on June 4, 2026: none

I'm fine as leaving as it is now. But wouldn’t it be clearer to generate the blocks when we broadcast the transaction? I understand your point about wanting to separate the transaction broadcast and block generation flows to highlight when the top is being done, but I think it makes the test a little bit more confusing. I believe that if the blocks are generated in the same loop where the topups are being done, it can still be clear to follow both flows and will make the test clearer and more compact. I'll leave the diff of how I would do it.

diff --git a/test/functional/wallet_fast_rescan.py b/test/functional/wallet_fast_rescan.py
index ce65855779..096a1b80ef 100755
--- a/test/functional/wallet_fast_rescan.py
+++ b/test/functional/wallet_fast_rescan.py
@@ -54,14 +54,18 @@ class WalletFastRescanTest(BitcoinTestFramework):
                     return descriptor['range'][1]
             raise AssertionError(f"Descriptor not found: {desc_str}")
 
-        txids_per_block = []
+        expected_wallet_txids = []
+        fast_rescan_messages = []
 
         self.log.info("Create and broadcast tx sending to non-ranged descriptor")
         addr = w.deriveaddresses(non_ranged_descs[0]['desc'])[0]
         spk = address_to_scriptpubkey(addr)
         self.log.debug(f"-> fixed non-range descriptor address {addr}")
         send_result = funding_wallet.send_to(from_node=node, scriptPubKey=spk, amount=10000)
-        txids_per_block.append([send_result["txid"]])
+
+        generated_block = self.generateblock(node, output="raw(42)", transactions=[send_result["txid"]])
+        fast_rescan_messages.append(f"Fast rescan: inspect block {node.getblockcount()} [{generated_block['hash']}] (filter matched)")
+        expected_wallet_txids.extend([send_result["txid"]])
 
         self.log.info("Create and broadcast txs sending to end range address of each descriptor, triggering top-ups")
         for _ in range(1, NUM_BLOCKS-1):
@@ -80,7 +84,9 @@ class WalletFastRescanTest(BitcoinTestFramework):
                 assert_greater_than(get_descriptor_end_range(desc_str), end_range)
                 block_tx_ids.append(send_result["txid"])
 
-            txids_per_block.append(block_tx_ids)
+            generated_block = self.generateblock(node, output="raw(42)", transactions=block_tx_ids)
+            fast_rescan_messages.append(f"Fast rescan: inspect block {node.getblockcount()} [{generated_block['hash']}] (filter matched)")
+            expected_wallet_txids.extend(block_tx_ids)
 
         # wallet w (topup_test) is not required to be loaded from here on, unload so that it
         # doesn't needlessly process block generation notifications in the background
@@ -88,20 +94,6 @@ class WalletFastRescanTest(BitcoinTestFramework):
 
         self.log.info("Create and broadcast tx unrelated to the wallet")
         send_result = funding_wallet.send_self_transfer(from_node=node)
-        txids_per_block.append([send_result['txid']])
-        assert_equal(len(txids_per_block), NUM_BLOCKS)
-
-        self.log.info("Start generating blocks one by one with associated txs")
-        expected_wallet_txids = []
-        fast_rescan_messages = []
-        for i, block_tx_ids in enumerate(txids_per_block):
-            self.log.info(f"Block {i+1}/{NUM_BLOCKS}")
-            generated_block = self.generateblock(node, output="raw(42)", transactions=block_tx_ids)
-            # Not asserting the fast rescan message in the last block because it contains wallet-unrelated
-            # 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 {node.getblockcount()} [{generated_block['hash']}] (filter matched)")
-                expected_wallet_txids.extend(block_tx_ids)
 
         self.log.info("Import wallet backup with block filter index")
         with node.assert_debug_log(['fast variant using block filters', *fast_rescan_messages]):

</details>

As said at the begining I'm fine with merging as is. Seeing other people have already ack maybe is just me who find this flow confusing.

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 the descriptor end ranges 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.
- The `w` wallet is also unloaded post transactions generation and before
the various restore and import cases that follow.
- 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 transaction.
- 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.

f217da5c59

rkrux force-pushed on Jun 4, 2026

rkrux commented at 2:46 PM on June 4, 2026: contributor

But wouldn’t it be clearer to generate the blocks when we broadcast the transaction?

Yeah, that makes sense as it can make the test feel less contrived. I had considered it when I had raised the PR but ended up didn't doing it then. Done now.

rkrux renamed this:
~~wallet, test: wallet_fast_rescan follow-ups~~
wallet, test: make wallet_fast_rescan robust
on Jun 4, 2026

DrahtBot added the label CI failed on Jun 4, 2026

rkrux commented at 11:04 AM on June 5, 2026: contributor

The Windows native, VS failure is a Github issue.

DrahtBot removed the label CI failed on Jun 5, 2026

Bicaru20 commented at 9:22 PM on June 8, 2026: none

lgtm ACK f217da5c59

DrahtBot requested review from w0xlt on Jun 8, 2026