#35998 wallet: Handle or explicitly ignore `WalletBatch` write failures

full analysis

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

ReviewerKindHarmStatusBlockingAuthor repliedQuote
jeanpablojpapproachFailing WriteBestBlock causes BackupWallet to abort without creating a backup, preventing copies when the database first starts rejecting writesresolvednoyes2026-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

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.

Data

dossier JSON · extract JSON · model openrouter/google/gemini-3.8-flash, generated 2026-09-17T21:46, confidence high, input hash c40a5e3ea7ff88da