#34879 wallet, test: remove unnecessary node restart from wallet_reindex
https://github.com/bitcoin/bitcoin/pull/34879 · · +90/-93 in 3 files, 2 commits · labels: none
Goal
- Clarify and speed up a wallet functional test by removing misleading steps and excess block generation
Refactors test/functional/wallet_reindex.py and renames it to wallet_birthtime.py. Removes a node restart that did not actually pass -reindex, reduces the number of blocks generated to fund the miner and exceed the rescan window, and switches to the standard bumpmocktime helper.
Problem: The wallet_reindex test name and restart comment were misleading because the test did not pass -reindex, and generating excess blocks slowed down test execution unnecessarily.
Category: Wallet (#77 of 84)
P4 · cleanup
- P4 because it is internal cleanup for a single functional test and does not touch production code
- The performance benefit is marginal, shaving only about two seconds off one test run
P4 because this is internal test cleanup for a single wallet functional test without touching wallet production code or uncovering wallet bugs. The author notes it shaves ~2 seconds off a single test run.
Membership: Modifies test/functional/wallet_birthtime.py testing wallet birthtime updates on rescan.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 0
Reviewability: Paused: Waiting on author
- Reviewing now is worthwhile to help evaluate whether removing the restart drops needed test coverage
polespinasa asked whether removing the restart drops useful test coverage, and the question has been unanswered for 7 days.
Author status: silent since 2026-09-02 push
Open concerns:
- polespinasa questioned whether dropping the node restart loses coverage of birthtime and transaction confirmation preservation across restarts
- polespinasa suggested reverting the comment referring to 'Blank wallets' since a wallet with private keys disabled is blank by definition
Resolved concerns:
- pablomartin4btc pointed out that renaming the test class in commit 1 without renaming the file caused a temporary mismatch, addressed by combining the rename
Agreement: Mild
- Support for simplifying the test and speeding up its execution (pablomartin4btc)
- Concern that removing the restart drops coverage of birthtime preservation across restarts (polespinasa)
pablomartin4btc ACKed, but polespinasa raised an open concern about losing test coverage over node restarts.
pablomartin4btc ACKed the simplification and performance gain, but polespinasa raised a substantive concern about lost restart coverage that remains unresolved.
- pablomartin4btc ACKed: 'The goal is correct — the old "Reindex..." comment was misleading, the restart never used -reindex, and rescanblockchain is sufficient'
- polespinasa questioned: 'By removing this restart we are loosing some test coverage, we now don't that tx confirmations and the birthtime are preserved over restart'
Review verdicts (DrahtBot): 1
- ACK: pablomartin4btc
Files
183 lines under test/bench/ci.
- test/functional/wallet_reindex.py +0/-92
- test/functional/wallet_birthtime.py +89/-0
- test/functional/test_runner.py +1/-1
Card
This PR refactors test/functional/wallet_reindex.py, renaming it to wallet_birthtime.py and removing an unnecessary node restart along with excess block generation. The change clarifies the intention of the test and reduces its execution time by roughly two seconds. pablomartin4btc supported the change, but polespinasa raised an open concern about losing restart coverage that has awaited an author response for a week.