#35998 wallet: Handle or explicitly ignore `WalletBatch` write failures
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #35998 wallet: Handle or explicitly ignore `WalletBatch` write failures | Waiting on author | 0 | Mild | 155 + 3 tests | |||
|
|
|
|
| |||
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
Mark all database read, write, erase, and transaction operations in WalletBatch as [[nodiscard]]. Update all callers across the wallet subsystem to either handle returned failure codes or explicitly cast them to void with an explanatory comment.
Problem: Wallet database write and erase failures were frequently ignored across the codebase. When database operations fail silently, in-memory state can diverge from persistent on-disk records (such as address indices, transaction records, or address book entries), risking wallet corruption, desynchronization, or data loss.
Evidence of importance:
- PR description: 'A common theme in the wallet is that many database write failures are ignored, when they should probably be handled, or at least documented that a failure is being ignored.'
- Commit 8be1f0d8ad note: 'This is the first time we write to this wallet, if there's a write failure now, there will likely be write failures in the future. Better to stop loading the wallet and not let the user use it at this time.'
Scope: The diff spans several wallet components (migration, exports, transaction insertion, descriptors, scanning progress), touching error control flow across many call sites.
Risk of a mistake: high (wallet, wallet_storage). Modifying error handling for wallet database writes can abort wallet loading, transaction recording, or address derivation if error checks fail or handle rollback incorrectly.
Changed since the previous assessment: Its base dependency PR 35752 has merged, removing encryption and passphrase refactors from this PR's diff and leaving only the nodiscard annotations and handling of remaining database writes.
Reviewability
Paused · Waiting on author
Open objection from vicjuma with no author reply for 26 days; review would repeat what has been asked.
Waiting on (from the thread read):
- author: reply to vicjuma on reporting corruption on disk full or permissions restrictions during wallet load 2026-10-06
Agreement
Mild
objection that needs an answer has none yet (vicjuma)
Thread notes: Added vicjuma as a participant with an objection regarding reporting wallet corruption on disk full or permissions restrictions during wallet load. The PR is currently waiting on the author to respond to vicjuma.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| jeanpablojp (contributor) | objection, safety | failing best block write prevents BackupWallet from creating a backup file when database write failures first occur | answered · author replied that creating a backup when writes fail is undesirable if the database may be corrupted | answered | yes | 2026-09-04: 'With the best block write failing, BackupWallet returns false and no copy is taken; before this change, Backup() still runs. This is the copy you most want when the database starts refusing writes.' Status from: 2026-09-08: achow101: 'If a write fails, it is possible that the database is already corrupted, and we do not want to be making a backup that m' |
| jeanpablojp (contributor) | objection, correctness | write failure in IncOrderPosNext leaves a half-initialized transaction in mapWallet that future calls treat as already inserted | answered · author replied that write failure causes an exception and crash higher up | answered | yes | 2026-09-04: 'this return leaves the entry behind with no nOrderPos, no wtxOrdered slot and no AddToSpends, and every later call with the same txid then takes the !fInsertedNew path and reports success without repa' Status from: 2026-09-08: achow101: 'This is ok, a failure here results in an exception higher up in the call stack which will result in a crash.' |
| jeanpablojp (contributor) | objection, correctness | write failure in TopUp leaves descriptor range out of sync with cache, persisting an invalid range that prevents reloading the wallet | answered · author replied that write failures are catastrophic and indicate corruption | answered | yes | 2026-09-04: 'The retry then derives nothing and WriteDescriptor succeeds, persisting a range wider than the cache behind it. Reopening that wallet stops with Unable to expand wallet descriptor from cache and retur' Status from: 2026-09-08: achow101: 'Write failures are supposed to be catastrophic and an indicator that corruption has likely happened.' |
| jeanpablojp (contributor) | suggestion, approach | use FaultInjectingDatabase to test the three unhandled write failure scenarios in the test suite | answered · author declined, stating comments describing code behavior are unnecessary | answered | yes | 2026-09-04: 'All three of these go unnoticed by the suite as it stands, and FaultInjectingDatabase covers each of them with a single injected failure.' Status from: 2026-09-08: achow101: 'it is not necessary to leave review comments describing what is happening in the code.' |
| jeanpablojp (contributor) | objection, correctness | database write failure during wallet loading reports wallet corruption even when the root cause is a full disk | answered · author replied that any write failure is treated as corruption | answered | yes | 2026-09-04: 'On a genuinely full disk, with just enough room left for the database to open, this reports Error loading <file>: Wallet corrupted while nothing is corrupt.' Status from: 2026-09-08: achow101: 'Failure to write is always corruption, regardless of how that may come about.' |
| vicjuma (contributor) | objection, correctness | reporting BErrors::CORRUPT on disk full or permission restrictions during database loading misguides users and locks them out | open | answered | no | 2026-10-06: 'reporting `BErrors::CORRUPT` may misguide the user giving then unnecessary tensions. For the others, this may fly, for loading the database IMHO, a log might probably help instead of locking them out with the error `...Wallet corrupted`' |
Support:
- l0rinc (member) (Concept ACK, 2026-08-17): thanks for taking over [not substantive]
- rkrux (member) (Concept ACK, 2026-08-26): enforces the handling of database errors, thereby ensuring consistency between in-memory behaviour and databases behaviour
- jeanpablojp (contributor) (Concept ACK, 2026-09-04): Left some comments. [not substantive]
Participants:
- l0rinc (member, support, 1 statements 2026-08-17 to 2026-08-17): Concept ACK, thanked author for taking over from #35752
- rkrux (member, support, 1 statements 2026-08-26 to 2026-08-26): Concept ACK for enforcing DB error handling and database/memory consistency
- jeanpablojp (contributor, objection, 6 statements 2026-09-04 to 2026-09-04): Concept ACK, but raised multiple correctness objections around write failure handling and suggested fault-injection tests
- vicjuma (contributor, objection, 2 statements 2026-10-06 to 2026-10-06): objected that reporting wallet corruption on disk full or permissions restrictions misguides users and locks them out
Files
6 lines under test/bench/ci.
- src/wallet/wallet.cpp +64/-23
- src/wallet/walletdb.h +38/-38
- src/wallet/export.cpp +24/-6
- src/wallet/walletdb.cpp +12/-4
- src/wallet/scriptpubkeyman.cpp +10/-4
- src/wallet/scan.cpp +5/-2
- src/bench/wallet_migration.cpp +3/-3
- src/wallet/wallet.h +2/-2
Card
The short text the ranking pass compares across PRs.
This pull request marks all database interaction methods in WalletBatch as nodiscard and updates callers across the wallet to handle failures or explicitly document ignored errors. Previously, callers frequently ignored write failures silently, allowing in-memory state (such as key generation indices, transaction state updates, and address book metadata) to diverge from persistent disk records without alerting the user. Failure handling now halts or surfaces errors during address generation, loading, wallet export, and wallet migration. The previously required dependency for encryption state updates (#35752) has merged.
Data
thread read through 2026-10-06 (15 statements), head d16ed3e405
labels: Wallet, Needs rebase
Categories
Wallet
P1 · fund safety
Unhandled database write and erase failures risk wallet corruption and silent divergence between in-memory state and persistent on-disk data. The wallet rubric ranks fund safety and database corruption first, making systemic handling of write failures category-critical.
Membership: Has the Wallet label and modifies core wallet database interfaces and callers across src/wallet/.
Factors: security/stability 2, bug 2, performance 0, user value 1, leverage 1