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

full analysis

https://github.com/bitcoin/bitcoin/pull/34879 · rkrux · +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

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.

Data

dossier JSON · extract JSON · model openrouter/google/gemini-3.8-flash, generated 2026-09-17T16:18, confidence high, input hash 8e0b6ab0f16a88c0