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

1. 
   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 to accommodate for the wallet scan start time while importing descriptor with "now" timestamp.
   • 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.

2. 
   DrahtBot commented at 12:57 PM on March 20, 2026: contributor

   <!--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/34879.

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

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

   Type	Reviewers
   Concept ACK	pablomartin4btc

   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.

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   No conflicts as of last run.

   <!--5faf32d7da4f0f540f40219e4f7537a3-->

3. 
   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.

4. 
   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?

5. 
   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)

6. 
   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.

7. DrahtBot added the label Needs rebase on Mar 24, 2026
8. test, wallet: overhaul wallet_reindex test

   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 to accommodate for the wallet scan start time while
   importing descriptor with "now" timestamp.
   - 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.

   ea2e314e8b
9. test, wallet: rename wallet_reindex to wallet_birthtime

   This rename follows the change in intention of the test caused by the preceding
   commit.

   fbbbe75ba9
10. rkrux force-pushed on Mar 24, 2026
11. DrahtBot removed the label Needs rebase on Mar 24, 2026
12. 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
13. sedited requested review from pablomartin4btc on Aug 10, 2026
14. sedited requested review from polespinasa on Aug 10, 2026
15. 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.

16. 
    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...

Contributors

rkruxDrahtBotw0xltpablomartin4btc

Review Requested
polespinasa