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

achow101 commented at 9:52 PM on August 17, 2026: member

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. This PR marks all database functions in WalletBatch as [[nodiscard]] so that a compiler will tell us if a return value is implicitly ignored. All of the calls to those functions are updated to either deal with failure, or explicitly ignore it with a rationale.

Based on #35752 which handles failures for the encryption functions.

refactor/test: share SQLite execution blocker

Move the test-only SQLite statement blocker into the wallet test utilities so transaction failure tests can reuse one implementation.

Call the stateless base execution handler directly instead of storing a redundant handler instance.

aa2248e214

test: characterize encryption transaction failures

Wallet encryption currently reports success after a failed master-key write.
A failed transaction commit aborts after publishing master and descriptor encryption state, leaving the same-process wallet unretryable.

Record both outcomes before making the transaction recoverable and publishing memory only after commit.

4c60630cfe

wallet: abort failed encryption transactions

Wallet encryption published master and descriptor key state before the database transaction completed.
A failed master key write was ignored, while a failed commit terminated after memory had changed.

Run the operation through `RunWithinTxn()` and publish the wallet master key only afterward.
Descriptor encryption now requires an active batch transaction because its staged in-memory update is owned by a commit listener.
Only a successful commit invokes that update, so abort and commit-failure paths leave descriptor memory unchanged and retryable.

040bf88fb9

test: characterize passphrase write failure

Wallet passphrase changes currently report success after the master key write fails.
The new passphrase works only in memory while the old passphrase remains on disk.

Record that behavior before making master-key encryption non-mutating and publishing the result only after persistence succeeds.

0fdbf2b65f

wallet: reject failed passphrase changes

`ChangeWalletPassphrase()` updated the in-memory master key before writing it to the database.
If `WriteMasterKey()` failed, the new passphrase worked only in memory while the old passphrase remained on disk.

Encrypt into a local copy.
Update the in-memory master key only after the database write succeeds.

bc380797c7

test: characterize descriptor key write failure

Descriptor encryption currently reports success after an encrypted-key record write fails, publishing wallet and descriptor encryption state even though one encrypted record was not persisted.
The published state also prevents retry.

Record that behavior before propagating the write failure and staging descriptor memory.

f65812231f

wallet: abort failed descriptor key writes

`DescriptorScriptPubKeyMan::Encrypt()` ignored failed encrypted-key writes, allowing the transaction to publish keys whose records were not persisted.

Check each write before staging its encrypted value, so `RunWithinTxn()` aborts without publishing memory.
`WriteCryptedDescriptorKey()` returns before attempting the plaintext erase when the encrypted write fails, and the unchanged state permits retry.

2388d029b7

test: characterize descriptor key erase failure

Descriptor encryption currently reports success after a plaintext-key erase fails.
The transaction commits both plaintext and encrypted records, publishes encrypted descriptor state, and leaves the operation unretryable.

Check both database record types and live descriptor state before propagating the erase failure.

9df456e8ff

wallet: abort failed descriptor key erases

`WriteCryptedDescriptorKey()` ignored failures to erase the corresponding plaintext descriptor key.
Encryption could commit both record forms and publish encrypted descriptor state.

Return the erase result so `RunWithinTxn()` aborts.
The abort preserves the plaintext record, rolls back the encrypted record, and does not run the commit callback, leaving memory unchanged and retryable.

49a08cb5b6

wallet: Handle db write failures in ExportWatchOnlyWallet 8e5088312d

wallet: Explicitly mark ignored write failures

Return values for some writes can be ignored, if they fail, no harm
occurred.

44a211d5c9

wallet: Handle descriptor update write failures 8bb9707fd0

wallet: Handle Write failure during loading 32c0c67a86

wallet: Ignore TxnAbort failure in RunWithinTxn

RunWithinTxn calls TxnAbort, but if this fails, the WalletBatch will go
out of scope anyways, which will call destructors that reach
SQLiteBatch::Close, which will also call TxnAbort, and has more handling
of abort failures.

1529a9e491

wallet: Handle db write failure in IncOrderPosNext 5844a2d12a

wallet: Handle transaction write failures

Handle write failures for WriteOrderPosNext and WriteTx

905504bc82

wallet, migration: Handle address book data write failures f7334c47f2

wallet, bench: Ignore db write failures 49508f4ca7

wallet: Mark WriteBestBlock [[nodiscard]] and handle write errors

Mark WriteBestBlock as [[nodiscard]] and explicitly ignore errors, or
handle them.

362e61e73a

walletdb: Mark all WalletBatch operations [[nodisard]]

All WalletBatch operations that interact with the database (read, write,
erase, transactions) have bool returns values that callers need to check
as database operations may fail.

a2b33bc596

achow101 requested review from polespinasa on Aug 17, 2026

DrahtBot added the label Wallet on Aug 17, 2026

DrahtBot commented at 9:52 PM on August 17, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35998.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	l0rinc

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36008 (wallet: WalletBatch->WriteVersion respect argument by fanquake)
#35935 (wallet: Avoid unnecessary wtxvariant rewrites by achow101)
#35786 (wallet: drop spent parents redundant cache invalidation and notification by furszy)
#35752 (wallet: make encryption state updates atomic by l0rinc)
#35716 (wallet: Replace mapWallet and wtxOrdered with a boost::multi_index by achow101)
#34993 (wallet: NotifyCanGetAddressesChanged when advancing next_index by davidgumberg)
#34861 (wallet: Add importdescriptors interface by polespinasa)
#34681 (wallet: move rescan logic into ChainScanner and wallet/scan by Eunovo)
#32895 (wallet: Prepare for future upgrades by recording versions of last client to open and decrypt by achow101)
#30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)
#29278 (Wallet: Add maxfeerate wallet startup option by ismaelsadeeq)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

_("Error: Unable to write the a tx record") -> _("Error: Unable to write a tx record") [“the a” is a typo and makes the message ungrammatical]

l0rinc commented at 9:58 PM on August 17, 2026: contributor

Concept ACK, thanks for taking over. Please see the latest push in #35752 (comment), feel free to adjust it any way you like.

DrahtBot added the label Needs rebase on Aug 18, 2026

DrahtBot commented at 11:20 PM on August 18, 2026: contributor

🐙 This pull request conflicts with the target branch and needs rebase.