wallet, test: remove unnecessary node restart from wallet

rkrux commented at 12:57 PM on March 20, 2026: contributor

The need for this change was ideated during the review of #34857.

The node doesn't need to be restarted (with reindex or without) because rescanblockchain RPC already updates the wallet birthtime, which is the property that needs to be tested - this allows us to rename the test class to highlight the change in intention of the test.
Generate 9 blocks fewer to fund the miner wallet.
Generate 30 blocks fewer because 20 are sufficient to surpass the wallet rescan window.
Use common bumpmocktime helper instead of custom advance_time function.
Unload miner wallet before 20 blocks generation to avoid notifications from being processed by that wallet.
Change the order of arguments in assert_equal for consistency.
Add few constant and verbose comments for test clarity.

Note: This overhaul helps in reducing the time taken by the test from 2s to 0s as observed in few runs.

DrahtBot commented at 12:57 PM on March 20, 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/34879.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	pablomartin4btc

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

Conflicts

No conflicts as of last run.

rkrux commented at 11:54 AM on March 21, 2026: contributor

It appears that this overhaul helps in reducing the tests time as well from 2s to 0s, which is nice to see.

Few runs that I checked: wallet_reindex test time in macOS native job in other PR1 and PR2 - 2 sec. wallet_birthtime test time in macOS native job in this PR - 0 sec.

w0xlt commented at 8:04 AM on March 22, 2026: contributor

I’ll take a closer look, but what’s the motivation for those changes?

rkrux commented at 8:34 AM on March 22, 2026: contributor

I’ll take a closer look, but what’s the motivation for those changes?

As mentioned in the last line of the PR description, it started from the review of PR #34857 here: #34857 (review)

rkrux commented at 8:38 AM on March 22, 2026: contributor

Thanks for asking, made me realise that the motivation was obscured away at the end of the PR description. I updated the PR description to highlight the motivation at the start.

DrahtBot added the label Needs rebase on Mar 24, 2026

rkrux force-pushed on Mar 24, 2026

DrahtBot removed the label Needs rebase on Mar 24, 2026

rkrux renamed this:
~~test, wallet: overhaul wallet_reindex test to wallet_birthtime~~
wallet, test: remove unnecessary node restart from wallet_reindex
on Jun 5, 2026

sedited requested review from pablomartin4btc on Aug 10, 2026

sedited requested review from polespinasa on Aug 10, 2026

in test/functional/wallet_birthtime.py:61 in fbbbe75ba9

  56 | +
  57 | +        # Now create a new wallet to import the descriptor in
  58 | +        node.createwallet(wallet_name='watch_only', disable_private_keys=True)
  59 | +        wallet_watch_only = node.get_wallet_rpc('watch_only')
  60 | +        # Blank wallets don't have a birth time
  61 | +        assert 'birthtime' not in wallet_watch_only.getwalletinfo()

pablomartin4btc commented at 10:25 PM on August 12, 2026:

nit: the comment says "Blank wallets" but the wallet isn't created with blank=True — the comment should describe the actual condition instead.

rkrux commented at 11:29 AM on September 2, 2026:

Taken along with other suggestions in the overall review comment.

polespinasa commented at 8:51 AM on September 9, 2026:

Sorry I just saw this after I added my comment: #34879 (review)

The wallet is in-fact a blank wallet, not blank=True does not mean that is not blank. As the wallet does not have private keys enabled, the wallet is at creation moment a blank wallet. The blank parameter is useful for a wallet with private keys to not create new keys on creation and allow the user to import their own keys.

I would rather revert this change.

pablomartin4btc commented at 8:49 PM on September 9, 2026:

I'm fine with that.

pablomartin4btc commented at 10:41 PM on August 12, 2026: member

Concept ACK

The goal is correct — the old "Reindex..." comment was misleading, the restart never used -reindex, and rescanblockchain is sufficient to test birthtime updates (the restart coverage was dropped intentionally). All the optimisations check out: 20 blocks × 10 min = 200 min > 2h window, COINBASE_MATURITY + 1 is the right minimum, and bumpmocktime is cleaner than the custom helper.

In PR description, at bullet point 3, "Generate 30 blocks fewer to accommodate for the wallet scan start time while importing descriptor with 'now' timestamp.", it's more accurate to say "30 fewer because 20 blocks is sufficient to surpass the 2-hour rescan window."

Commit structure:

The commit ordering creates a transient inconsistency: commit 1 renames the class to WalletBirthTimeTest but the file is still wallet_reindex.py, so at that commit the class and file names are mismatched. Cleaner would be to rename both the file and class together in commit 1 (pure rename, git detects with --find-renames), then overhaul the content in commit 2 — that way the class and file names always match across commits. Or a single commit since it's all test-only changes.

I think commit prefixes should both be test: — no production wallet code is touched.

Left an inline comment...

test: rename wallet_reindex to wallet_birthtime

This rename precedes the change in intention of the test in the subsequent
commit.

3c260e70a1

test: remove unnecessary restart from the test among other optimisations

Changes done:
- The node doesn't need to be restarted (with reindex or without) because
rescanblockchain RPC already updates the wallet birthtime, which is the property
that needs to be tested - this allows us to rename the test class to highlight
the change in intention of the test.
- Generate 9 blocks fewer to fund the miner wallet.
- Generate 30 blocks fewer because 20 are sufficient to surpass the wallet
rescan window.
- Use common bumpmocktime helper instead of custom advance_time function.
- Unload miner wallet before 20 blocks generation to avoid notifications from
being processed by that wallet.
- Change the order of arguments in assert_equal for consistency.
- Add few constants and verbose comments for test clarity while removing
comments that allude to legacy wallets.

4996874b58

rkrux force-pushed on Sep 2, 2026

pablomartin4btc commented at 11:39 PM on September 8, 2026: member

ACK 4996874b58b3f08b586eded89528fc5632fc909b

Thanks for taking the suggestions.

in test/functional/wallet_birthtime.py:17 in 3c260e70a1

  13 | @@ -14,7 +14,7 @@
  14 |  )
  15 |  BLOCK_TIME = 60 * 10
  16 |  
  17 | -class WalletReindexTest(BitcoinTestFramework):
  18 | +class WalletBirthTimeTest(BitcoinTestFramework):

polespinasa commented at 7:58 AM on September 9, 2026:

in 3c260e70a121b77e95e05452486eff56335e7c9f test: rename wallet_reindex to wallet_birthtime

class WalletBirthtimeTest(BitcoinTestFramework):

in test/functional/wallet_birthtime.py:60 in 4996874b58

  68 | -        node.createwallet(wallet_name='watch_only', disable_private_keys=True, load_on_startup=True)
  69 | +        # Now create a new wallet to import the descriptor in
  70 | +        node.createwallet(wallet_name='watch_only', disable_private_keys=True)
  71 |          wallet_watch_only = node.get_wallet_rpc('watch_only')
  72 | -        # Blank wallets don't have a birth time
  73 | +        # Empty wallets don't have a birth time

polespinasa commented at 8:25 AM on September 9, 2026:

in 4996874b58b3f08b586eded89528fc5632fc909b test: remove unnecessary restart from the test among other optimisations

I would rather keep Blank. Although the wallet is not created using the blank parameter, a blank wallet is just a wallet without keys. A wallet that has private keys disabled and did not import yet a public descriptor is in fact a blank wallet.

in test/functional/wallet_birthtime.py:52 in 4996874b58

  56 |  
  57 | -        # Generate 50 blocks, one every 10 min to surpass the 2 hours rescan window the wallet has
  58 | -        for _ in range(50):
  59 | +        # Generate enough blocks every 10 mins to surpass the 2 hours rescan window the wallet has.
  60 | +        # 20 blocks every 10 mins equals 3hrs 20mins from now till last block - 20 mins more than
  61 | +        # the start time of wallet to scan from while importing a descriptor with "now" timestamp.

polespinasa commented at 8:32 AM on September 9, 2026:

in 4996874 test: remove unnecessary restart from the test among other optimisations

I find this comment really hard to read. The test is not mining 20blocks every 10min, it is mining 20 blocks, one every 10min.

in test/functional/wallet_birthtime.py:64 in 4996874b58

  90 |          wallet_watch_only.rescanblockchain()
  91 | -        assert_equal(wallet_watch_only.gettransaction(tx_id)['confirmations'], 50)
  92 | -        assert_equal(wallet_watch_only.getbalances()['mine']['trusted'], 2)
  93 |  
  94 | -        self.log.info("Reindex ...")  # restart_node waits for it to finish
  95 | -        self.restart_node(0, extra_args=[ f'-mocktime={self.node_time}'])

polespinasa commented at 8:35 AM on September 9, 2026:

in 4996874b58b3f08b586eded89528fc5632fc909b test: remove unnecessary restart from the test among other optimisations

By removing this restart we are loosing some test coverage, we now don't that tx confirmations and the birthtime are preserved over restart under reindex.

polespinasa commented at 8:49 AM on September 9, 2026: member

I am not sure about removing the restart.

Friendly pinging @furszy. Do you remember why you added it in the first place? Do you think it is correct to remove it or are we losing useful test coverage with it?

wallet, test: remove unnecessary node restart from wallet_reindex #34879

Code Coverage & Benchmarks

Reviews

Conflicts