#35998 wallet: Handle or explicitly ignore `WalletBatch` write failures
https://github.com/bitcoin/bitcoin/pull/35998 · · +542/-230 in 19 files, 25 commits · labels: Wallet, Needs rebase · draft
Goal
- Prevent wallet in-memory state from desynchronizing with disk when database writes fail
- Avoid silent data loss or corruption during key encryption, passphrase updates, and transactions
This PR marks all database read, write, erase, and transaction methods in `WalletBatch` as `[[nodiscard]]`. It audits every call site across the wallet codebase to either handle database failures or explicitly cast them to void with a documenting comment. It also integrates failure handling for wallet encryption and passphrase changes based on #35752 and adds a SQLite fault-injection testing harness.
Problem: Wallet database write failures are frequently ignored across the codebase. When a write or erase fails (for example during encryption, key insertion, or passphrase changes), the wallet may update in-memory state while leaving disk records outdated or unwritten, causing data inconsistency or potential loss of unpersisted keys.
Category: Wallet (#10 of 84)
P2 · fund safety
- P2 because it addresses systematic data integrity and state synchronization risks in wallet persistence
- Eliminates silent failure modes when updating passphrases or encrypting keys
Systematically enforces `[[nodiscard]]` on all database operations in `WalletBatch`, preventing silent persistence failures. Ignored write failures during encryption or address/key generation can leave in-memory keys desynchronized from disk storage, risking fund loss if a process terminates or reloads.
Membership: Touches wallet database layer, scriptpubkeyman, wallet loading, and migration tests.
Factors: security/stability 2, bug 2, performance 0, user value 1, leverage 1
Reviewability: Stale: Needs rebase
- Needs rebase due to merge conflicts with active wallet PRs
- Review #35752 first
The PR has merge conflicts with master and is marked with the 'Needs rebase' label and dirty mergeable state.
Author status: active, force-pushed updates on 2026-09-14 after addressing reviewer questions
Resolved concerns:
- Whether BackupWallet should continue if writing the best block locator fails; author clarified write failures indicate potential corruption where backups should not proceed
- Whether in-memory structures like mapWallet and descriptor caches should be rolled back on write failure; author noted write failures are catastrophic and lead to aborts or fatal errors rather than recoverable retries
- Whether write failure during wallet loading should return DBErrors::CORRUPT or DBErrors::LOAD_FAIL on full disk; author clarified write failure at load time indicates the database is unusable
Agreement: Strong
- Concept approval because it ensures consistency between in-memory and disk state (rkrux, l0rinc)
- Concern about partial in-memory updates on write failure versus treating corruption as fatal (jeanpablojp)
Strong: concept ACKs across three reviewers, with inline concerns about error recovery resolved by author rationale
The change enjoys solid consensus on its approach. Reviewer queries around whether database write failures should be recoverable or treated as catastrophic corruption were addressed by the author, with no remaining open objections.
- rkrux: 'Concept ACK ... enforces the handling of database errors, thereby ensuring consistency between in-memory behaviour and databases behaviour.'
- l0rinc: 'Concept ACK, thanks for taking over.'
- achow101: explained that database write failures are treated as catastrophic corruptions requiring abort rather than retry.
Objections:
| Reviewer | Kind | Harm | Status | Blocking | Author replied | Quote |
|---|---|---|---|---|---|---|
| jeanpablojp | approach | Failing WriteBestBlock causes BackupWallet to abort without creating a backup, preventing copies when the database first starts rejecting writes | resolved | no | 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.' Settled: 2026-09-08: achow101 replied 'If a write fails, it is possible that the database is already corrupted, and we do not want to be making a backup that may not be usable.' No further pushback. |
Support:
- rkrux: enforces the handling of database errors, thereby ensuring consistency between in-memory behaviour and databases behaviour
- l0rinc: Concept ACK, thanks for taking over [not substantive]
- jeanpablojp: Concept ACK [not substantive]
Participants: l0rinc (support), rkrux (support), jeanpablojp (objection)
State derived from the lists: substantive support, no open objection (rkrux)
Review verdicts (DrahtBot): 0
- Concept ACK: l0rinc, rkrux, jeanpablojp
Dependencies
Depends on: #35752
Files
276 lines under test/bench/ci.
- src/wallet/wallet.cpp +117/-73
- src/wallet/test/wallet_tests.cpp +172/-4
- src/wallet/test/util.h +86/-0
- src/wallet/walletdb.h +38/-38
- src/wallet/scriptpubkeyman.cpp +33/-10
- src/qt/askpassphrasedialog.cpp +9/-33
- src/wallet/rpc/encrypt.cpp +17/-23
- src/wallet/export.cpp +24/-6
- src/wallet/walletdb.cpp +17/-8
- src/qt/walletmodel.cpp +4/-13
- src/wallet/wallet.h +4/-4
- src/wallet/scan.cpp +5/-2
- src/bench/wallet_migration.cpp +3/-3
- src/interfaces/wallet.h +3/-3
- src/wallet/interfaces.cpp +3/-3
- test/functional/wallet_migration.py +3/-3
- src/qt/walletmodel.h +2/-3
- test/functional/wallet_encryption.py +1/-1
- src/wallet/scriptpubkeyman.h +1/-0
Card
This PR marks all WalletBatch operations as [[nodiscard]] to ensure database write, erase, and transaction failures are explicitly checked across the wallet. It prevents silent write failures that cause in-memory state (such as newly generated keys, encryption state, or passphrases) to diverge from on-disk database records. It builds on the atomic encryption work in #35752 and adds a SQLite fault-injection testing utility. Review is currently blocked by merge conflicts requiring a rebase.