wallet, test: make wallet_fast_rescan robust#34907
Open
rkrux wants to merge 1 commit into
Open
Conversation
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34907. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
w0xlt
reviewed
Mar 25, 2026
Contributor
There was a problem hiding this comment.
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
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__':
w0xlt
reviewed
Mar 25, 2026
w0xlt
left a comment
Contributor
There was a problem hiding this comment.
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
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)
w0xlt
reviewed
Mar 25, 2026
w0xlt
reviewed
Mar 25, 2026
Contributor
Author
|
Good suggestions, I will add them. |
Contributor
Author
|
Added all the suggestions from w0xlt's review in latest push. |
Contributor
Author
|
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. |
w0xlt
reviewed
May 21, 2026
Contributor
Author
|
Force pushed to use suggestions from @w0xlt's review: #34907 (review). |
w0xlt
reviewed
May 27, 2026
|
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. diffdiff --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]):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. |
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.
Contributor
Author
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. |
Contributor
Author
|
The Windows native, VS failure is a Github issue. |
|
lgtm ACK f217da5 |
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.