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

ReviewabilityReviewsAgreementSize
#34879 rkrux wallet, test: remove unnecessary node restart from wallet_reindexReady0 (+1)Strong0 + 90 tests
  • Clarify wallet birthtime functional test intent and reduce run time by removing an unneeded node restart
  • Ready to review
  • Substantive support, no open objections
  • Agrees rescan tests birthtime updates and node restart was misleading (pablomartin4btc)
  • Concerned removing restart drops coverage of birthtime across restarts, answered by author (polespinasa)
  • 0 lines added or modified outside tests
  • 90 lines added or modified in tests
  • 93 lines removed in total
  • 3 files, 2 commits
Wallet#90 of 92
P4 · cleanup
  • P4 because it is a minor functional test cleanup that trims about two seconds off test execution
  • Touches only test code with no behavioral changes to the wallet itself
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

Renames functional test `wallet_reindex.py` to `wallet_birthtime.py` and removes an unnecessary node restart. It clarifies the test's intent to focus on wallet birthtime updates during rescanning rather than reindexing, reduces the number of blocks generated, unloads the unused miner wallet early, and uses `bumpmocktime` instead of a custom time helper.

Problem: The test was misnamed and slower than necessary. Its primary assertion verifies that `rescanblockchain` updates wallet birthtime after importing a descriptor with a recent timestamp, making the full node restart redundant and block generation excessive.

Evidence of importance:

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

Risk of a mistake: low (tests). A mistake would only cause functional test failures or mask a regression in wallet birthtime handling.

Changed since the previous assessment: No substantive changes to the approach or diff.

Reviewability

Ready

Nothing known that would invalidate a review now.

Waiting on (from the thread read):

  • reviewer: polespinasa to follow up on author's answers and comment updates 2026-10-26

Agreement

Strong

substantive support, no open objection (pablomartin4btc)

Thread notes: Author rkrux addressed polespinasa's inline review comments and objections regarding test coverage and comment clarity. All items are now answered.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
w0xlt (contributor)question, approachwhat is the motivation for the changes?answered · author replied explaining it originated from PR #34857 and updated the PR descriptionansweredyes2026-03-22: 'what’s the motivation for those changes?'
Status from: 2026-03-22: rkrux: 'As mentioned in the last line of the PR description, it started from the review of PR #34857 here'
polespinasa (member)suggestion, maintenancerephrase the comment to clarify that the test mines 20 blocks, one every 10 minutes, rather than 20 blocks every 10 minutesanswered · author agreed the comment was confusing and updated itansweredyes2026-09-09: 'I find this comment really hard to read. The test is not mining 20blocks every 10min, it is mining 20 blocks, one every 10min.'
Status from: 2026-10-26: rkrux: 'True, this comment could have come across as confusing. Updated.'
polespinasa (member)objection, correctnessremoving the restart drops test coverage of transaction confirmations and birthtime being preserved across restartsanswered · author explained test coverage is not lost because birthtime is updated and saved during rescan and preserved on reload without needing a node restartansweredyes2026-09-09: '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.'
Status from: 2026-10-26: rkrux: 'I don't believe any test coverage is being lost here. The intent as per the original comments in this test file show that the correct update of wallet birthtime property needs to be tested'
polespinasa (member)objection, maintenancecomment change inaccurately removes description of wallet as blank, when a wallet without private keys is blank by definitionanswered · author replied that either is fine and used bothansweredyes2026-09-09: '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... I would '
Status from: 2026-10-26: rkrux: 'Either is fine, used both.'

Support:

  • pablomartin4btc (member) (Concept ACK, 2026-08-12) [read]: The goal is correct — the old "Reindex..." comment was misleading, the restart never used -reindex, and rescanblockchain is sufficient to test birthtime updates
  • pablomartin4btc (member) (ACK, 2026-09-08): ACK 4996874b58b3f08b586eded89528fc5632fc909b Thanks for taking the suggestions. [not substantive]

Participants:

  • w0xlt (contributor, question, 1 statements 2026-03-22 to 2026-03-22): asked about the motivation for the changes
  • pablomartin4btc (member, support, 4 statements 2026-08-12 to 2026-09-09): Concept ACKed and ACKed the overhaul after suggestions were addressed
  • polespinasa (member, objection, 6 statements 2026-09-09 to 2026-09-09): objected to removing the restart due to lost test coverage and suggested comment improvements
  • sedited (member, neutral, 1 statements 2026-10-19 to 2026-10-19): pinged the author to follow up on polespinasa's review

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

The short text the ranking pass compares across PRs.

Renames functional test wallet_reindex.py to wallet_birthtime.py and streamlines its execution. The test primarily validates wallet birthtime updates after rescanning, but previously performed an unnecessary node restart and mined redundant blocks. The refactor removes the restart, cuts block generation, unloads the miner wallet early, and replaces a custom mocktime helper. Author testing shows test runtime drops from 2s to sub-second. It has no dependencies.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-28T01:21, confidence high, input hash 2a6652b48055f20d
thread read through 2026-10-26 (20 statements), head 2459a744f3
labels: none

Categories

Wallet

P4 · cleanup

Clarifies test intent and removes a redundant node restart to reduce runtime by 2 seconds. The PR description notes this 'helps in reducing the time taken by the test from 2s to 0s' and clarifies rescan behavior rather than reindexing, making it a minor test cleanup with no functional impact on wallet code.

Membership: Refactors and renames the wallet functional test wallet_reindex.py to wallet_birthtime.py, which tests wallet birthtime updates during rescanning.

Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 0