#35760 wallet: make corrupted transaction records fail wallet loading instead of forcing a rescan

ReviewabilityReviewsAgreementSize
#35760 achow101 wallet: make corrupted transaction records fail wallet loading instead of forcing a rescanWaiting on author0Mild39 + 23 tests
  • Fail wallet loading with a corruption error instead of triggering a full rescan on startup
  • Prevent wallets with corrupted transaction records from running in an inconsistent state
  • Reviewing now is worthwhile as no rebase, CI failure, or pending rework blocks review
  • General agreement that failing fast is better than triggering endless, unreliable rescans
  • Unaddressed objection: failing to load locks users out of funds with no recovery option (vicjuma)
  • Unaddressed objection: error message inverts stored and calculated transaction hashes (jeanpablojp)
  • Approach approval because rescan cannot repair corrupted records (pablomartin4btc)
  • 39 lines added or modified outside tests
  • 23 lines added or modified in tests
  • 23 lines removed in total
  • 8 files, 5 commits
Wallet#9 of 92
P2 · bug fix
  • P2 because it stops corrupted wallets from repeatedly rescanning the whole chain on every startup
  • Prevents running with partially loaded records that bypass spend tracking and conflict caches
  • Ranking pass: Moved up: corrupt-record handling sits with the other wallet-load/storage correctness fixes rather than below feature work.
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

Fails wallet loading with a corruption error instead of triggering a chain rescan when a transaction record has a mismatched hash or cannot be loaded. Removes the `NEED_RESCAN` database error code, the corresponding `rescan_required` handling during chain attachment, and documents restoring from backup as the only supported recovery method for corrupted wallets.

Problem: When a wallet database transaction record is corrupted such that its stored key does not match the deserialized transaction's hash, the wallet previously returned `DBErrors::NEED_RESCAN`. This triggered a full rescan from genesis on every load without fixing the corrupted record, while leaving the affected transaction partially loaded (omitted from spend tracking, conflict tracking, and TXO caches).

Evidence of importance:

  • Author notes in the description that the corrupt transaction "would be inserted to mapWallet, but not be added to mapTxSpends or any of the conflict tracking or to TXO caching" and that "the rescan doesn't actually fix the problem, the incorrect record would persist, which means that the wallet would be rescanning the entire chain every time it is loaded."
  • Commit message c34db6bbef cites consensus in PR #35605: "we should make the only supported corruption recovery mechanism be to restore a backup and not do any of this stuff with maybe being able to recover."

Reviewability

Paused · Waiting on author

Open objections from jeanpablojp, vicjuma with no author reply for 31 days; review would repeat what has been asked.

Waiting on (from the thread read):

  • author: fix inverted stored and calculated hashes in the error message 2026-08-30
  • author: address vicjuma's concern regarding locking users out of wallet funds 2026-10-01

Agreement

Mild

objection that needs an answer has none yet (jeanpablojp, vicjuma)

Thread notes: Added vicjuma as a participant with an objection regarding locking users out of their wallets upon record mismatch, which remains open along with jeanpablojp's comment on inverted hashes.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
polespinasa (member)objection, approachusers with corrupted wallets who lack backups cannot access uncorrupted funds or export descriptorsanswered · author replied in c:5375058547 · v1, not yet re-checkedansweredyes2026-08-21: 'If we disable allowing this corrupted wallets, users that unconsciously did not create a backup will be unable to access any of their funds at all without switching to an old Core version.'
Status from: 2026-08-21: achow101: 'reply on record'
polespinasa (member)objection, correctnessthe unit test passes due to an EOF deserialization failure rather than exercising the hash mismatch corruption code pathfixed · fix pushed · fixed by author in rc:3833484889 · v1, not yet re-checkedansweredyes2026-08-21: 'This test is wrong it is not testing the `DBErrors::CORRUPT` path intended to test.'
Status from: 2026-08-21: achow101: 'Fixed'
jeanpablojp (contributor)objection, correctnessthe error message inverts stored and actual hashes, causing users inspecting the database manually to look for the wrong keyopen · v1, not yet re-checkedansweredno2026-08-30: 'The two are swapped. In the description you call the DB key the stored hash and `wtx.GetHash()` the calculated one, and here the `strprintf` passes them the other way round.'
vicjuma (contributor)objection, approachearly return on mismatch locks users out of their wallet and funds instead of allowing them to export wallet data or access remaining fundsopenansweredno2026-10-01: 'This early return means locking the user out of their wallet and their funds, wallet-load rescan on the other hand somehow sucks.'

Support:

  • pablomartin4btc (member) (Approach ACK, 2026-08-07) [read]: When a tx record's stored hash doesn't match the hash of its deserialized content, returning NEED_RESCAN was the wrong response: the rescan is not a reliable repair... failing hard with CORRUPT makes sense
  • jonatack (member) (Concept ACK, 2026-08-19): Concept ACK. Worth a release note? [not substantive]
  • polespinasa (member) (Approach ACK, 2026-08-19): Aproach ACK [not substantive]
  • rkrux (member) (Concept ACK, 2026-08-26) [read]: I do prefer the wallet to stop and fail fast instead of trying to fix rare corrupted scenarios automatically _everytime on load_, which makes the wallet do too much and bloats the codebase as well.
  • jeanpablojp (contributor) (Approach ACK, 2026-08-30) [tested]: Built it, ran the unit tests and the wallet functional tests, and reproduced the corruption on regtest.

Participants:

  • pablomartin4btc (member, support, 5 statements 2026-08-07 to 2026-08-21): Approach ACK; argued that failing hard with CORRUPT is correct, contributed a unit test, and opened doc PR #35944 for wallet recovery guidance
  • jonatack (member, support, 1 statements 2026-08-19 to 2026-08-19): Concept ACK; suggested adding a release note
  • polespinasa (member, objection, 8 statements 2026-08-19 to 2026-08-31): objected that failing to load locks users without backups out of uncorrupted funds; also caught an invalid unit test
  • rkrux (member, support, 1 statements 2026-08-26 to 2026-08-26): Concept ACK; supports failing fast on load rather than automatically attempting unreliable repairs
  • jeanpablojp (contributor, objection, 2 statements 2026-08-30 to 2026-08-30): Approach ACK, but reported that stored and calculated hashes are inverted in the corruption error message
  • vicjuma (contributor, objection, 3 statements 2026-10-01 to 2026-10-01): concerned that returning early on mismatch locks users out of their wallet and funds rather than allowing recovery

Files

23 lines under test/bench/ci.

  • src/wallet/wallet.cpp +9/-18
  • src/wallet/test/walletload_tests.cpp +23/-0
  • doc/managing-wallets.md +19/-0
  • doc/release-notes-35760.md +6/-0
  • src/wallet/walletdb.cpp +3/-2
  • src/wallet/wallet.h +1/-1
  • src/wallet/wallettool.cpp +1/-1
  • src/wallet/walletdb.h +0/-1

Card

The short text the ranking pass compares across PRs.

Makes wallet loading fail immediately with DBErrors::CORRUPT when a transaction record has a mismatched hash or cannot be loaded, removing DBErrors::NEED_RESCAN. Previously, hash mismatches triggered a full chain rescan from genesis on every single load while leaving the transaction partially loaded and missing from spend tracking and TXO caches. The PR also documents restoring from backup as the only supported path for corrupted wallet recovery and adds a unit test for corrupt transaction hashes.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-02T01:25, confidence high, input hash 535f6e0bb5a0ec89
thread read through 2026-10-01 (28 statements), head 1e45c91abb
Discussion read under schema 1 rules: suggestions and questions may be missing, and an objection may combine several harms (schema history).
labels: Wallet

Categories

Wallet

P2 · bug fix (ranking pass: Moved up: corrupt-record handling sits with the other wallet-load/storage correctness fixes rather than below feature work.)

Fails fast on corrupted transaction records instead of triggering an endless full rescan on every startup with an inconsistently loaded wallet. As the author notes, a corrupt record previously caused the wallet to rescan from genesis on every launch while leaving the transaction 'inserted to mapWallet, but not be added to mapTxSpends or any of the conflict tracking or to TXO caching.' Fixing this eliminates a recurring failure loop and establishes a coherent backup-recovery policy for corrupted databases.

Membership: Carries the Wallet label and modifies wallet database loading, error handling, and recovery logic in src/wallet/.

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