test, wallet: overhaul wallet_reindex test to wallet_birthtime #34879

1. 
   rkrux commented at 12:57 pm on March 20, 2026: contributor

   The need for this change was ideated during the review of PR 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

   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.

   Conflicts

   No conflicts as of last run.

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

Contributors
rkrux DrahtBot w0xlt