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.

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, rkrux, jeanpablojp

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:

#36167 ([RFC] Enable -Wunused by fanquake)
#36133 (wallet: store multipath descriptor by Sjors)
#36126 (wallet, rpc: Implements set key label functionality by polespinasa)
#36070 (wallet: Add deriveHDKey interface by PraneethGunas)
#36042 (build: Bump g++ minimum supported version to 13 by maflcko)
#36033 ([wip,nomerge,rfc] build: Require C++23 compiler by maflcko)
#36031 (wallet: Remove mapMasterKeys and enforce that only one encryption key can exist by achow101)
#35935 (wallet: Avoid unnecessary wtxvariant rewrites by achow101)
#35786 (wallet: drop spent parents redundant cache invalidation and notification by furszy)
#35716 (wallet: Replace mapWallet and wtxOrdered with a boost::multi_index by achow101)
#35377 (wallet: Allow importing of descriptors without private keys when the wallet has the private keys by achow101)
#34909 (wallet, refactor: modularise wallet by extracting out legacy wallet migration by rkrux)
#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)
#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" [extra “the” breaks the phrase]

Possible places where comparison-specific test macros should replace generic comparisons:

[src/wallet/test/wallet_tests.cpp] BOOST_CHECK_THROW((void)add_key(), std::runtime_error); -> Prefer BOOST_CHECK_EXCEPTION(...) with a predicate that checks the exception message, so the test validates the specific failure reason instead of only the exception type.

2026-08-26 20:47:34

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

achow101 force-pushed on Aug 25, 2026

DrahtBot added the label CI failed on Aug 26, 2026

DrahtBot commented at 12:22 AM on August 26, 2026: contributor

🚧 At least one of the CI tasks failed. Task Windows native, fuzz, VS: https://github.com/bitcoin/bitcoin/actions/runs/32908277972/job/97996971269 LLM reason (✨ experimental): CI failed because the fuzz target rpc crashed with exit code 3221225477 (Windows access violation / segfault).

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

DrahtBot removed the label Needs rebase on Aug 26, 2026

rkrux commented at 9:33 AM on August 26, 2026: contributor

Concept ACK f8b9da8007ad6468618fc8e4235508a5b7038386 because it enforces the handling of database errors, thereby ensuring consistency between in-memory behaviour and databases behaviour.

refactor/test: add wallet failure injection

Add a reusable SQLite-backed wallet database that can reject selected record writes, erases, or transaction commits and inspect stored records.

Co-authored-by: Ava Chow <github@achow101.com>

3a5b3304b5

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, which prevents retry in the same process.

Record both outcomes before returning transaction failures as errors and publishing live state only after commit.

Co-authored-by: Ava Chow <github@achow101.com>

5ba4513118

wallet: abort failed encryption transactions

Wallet encryption publishes master and descriptor key state before the database transaction commits.
It also ignores a failed master-key write and aborts the process after a failed commit.

Use `RunWithinTxn()` for the master key and existing descriptor keys.
Stage descriptor keys in a commit listener and publish the master key after commit, so failed master-key writes and commits leave live state unchanged and encryption can be retried.

dd9bf08662

wallet: restore lock state before re-encryption

`ChangeWalletPassphrase()` temporarily unlocks a locked wallet after validating the old passphrase.
If `EncryptMasterKey()` then fails, the wallet returns without restoring its locked state.

Restore the original lock state immediately after validating the old passphrase, before re-encrypting or writing the master key.

e2fea980f9

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 encrypting a copy of the master key and publishing it only after the database write succeeds.

Co-authored-by: Ava Chow <github@achow101.com>

288d803889

wallet: reject failed passphrase changes

`ChangeWalletPassphrase()` updates the in-memory master key before writing it to the database.
If `WriteMasterKey()` fails, the new passphrase works only in memory while the old passphrase remains on disk.

Encrypt a copy of the master key, persist it, and replace the live value only after the write succeeds.
A failed write now leaves the old passphrase active and allows the change to be retried.

6a0c7abba7

test: characterize descriptor key failures

Descriptor encryption currently reports success after an encrypted-key record write or plaintext-key record erase fails.
Both failures publish wallet and descriptor encryption state and prevent retry.
An ignored erase also commits both records.

Record the existing write and erase failure behavior before propagating either error and staging descriptor memory.

b364f2a24a

wallet: abort failed descriptor key writes

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

Check each write before staging its encrypted value.
`RunWithinTxn()` can then abort without publishing memory, leaving encryption retryable.

eb2923647f

test: cover encrypted descriptor key insertion

The descriptor-key write helper also inserts encrypted keys when no plaintext record exists.
Pin this path before propagating replacement erase failures, so insertion remains successful without attempting a plaintext erase.

3071c62bb1

wallet: abort failed descriptor key erases

`WriteCryptedDescriptorKey()` replaces a plaintext key during wallet encryption.
The same helper can insert an encrypted key when no plaintext record exists.

After writing the encrypted key, erase its plaintext record if it exists and propagate any erase failure.
Keeping both operations in the helper prevents replacement callers from omitting the erase while allowing new encrypted-key insertion.

Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
Co-authored-by: Ava Chow <github@achow101.com>

6d549ed697

test: characterize descriptor insertion failure

Descriptor key insertion currently updates the live key map before writing the key record.
If the write fails, a retry sees the live-only key and skips persistence while reporting success.

Record the plaintext and encrypted cases before making live-map publication conditional on successful database writes.

8d002c15ca

wallet: publish descriptor keys after writes

`AddDescriptorKeyWithDB()` publishes plaintext and encrypted keys before writing their database records.
A failed write leaves a live-only key, and the duplicate check prevents a retry from persisting it.

Write each key record first and update the corresponding live key map only after the write succeeds.
A failed insertion then leaves no key behind and can be retried.

086a6b6005

wallet: return passphrase errors with Expected

Change wallet unlock and passphrase methods from `bool` to `util::Expected<void, WalletError>` so they can return specific error codes and messages.
Update the wallet interfaces while callers continue to use the success state.

c79cb54237

wallet: deduplicate wallet unlock and passphrase change errors

Unlocking and changing the passphrase of a wallet may result in errors.
These functions should produce specific errors with codes and messages
so that callers can handle the errors appropriately.

Additionally, doing so allows us to deduplicate some passphrase error
messages.

10378370c9

wallet: Handle db write failures in ExportWatchOnlyWallet 859bfb6061

wallet: Explicitly mark ignored write failures

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

95571808d8

wallet: Handle descriptor update write failures 98ed1e00f0

wallet: Handle Write failure during loading 37dc1f6857

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.

61d9f47078

wallet: Handle db write failure in IncOrderPosNext fc8b4efabe

wallet: Handle transaction write failures

Handle write failures for WriteOrderPosNext and WriteTx

bf66961b9f

wallet, migration: Handle address book data write failures 9f289d2006

wallet, bench: Ignore db write failures 09fea98b6e

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

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

ad5345e132

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.

c794ae0e64

achow101 force-pushed on Aug 26, 2026

DrahtBot removed the label CI failed on Aug 26, 2026

jeanpablojp commented at 10:24 AM on September 4, 2026: contributor

Concept ACK

Left some comments.

in src/wallet/wallet.cpp:3377 in c794ae0e64

3373 | @@ -3348,7 +3374,9 @@ void CWallet::postInitProcess()
3374 |  
3375 |  bool CWallet::BackupWallet(const std::string& strDest) const
3376 |  {
3377 | -    WITH_LOCK(cs_wallet, WriteBestBlock());
3378 | +    if (!WITH_LOCK(cs_wallet, return WriteBestBlock())) {

jeanpablojp commented at 10:24 AM on September 4, 2026:

A stale locator only costs a rescan on the next load, but here it cancels the backup before Backup() is reached. 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. Would logging and carrying on make sense here?

in src/wallet/wallet.cpp:1101 in c794ae0e64

1094 | @@ -1084,7 +1095,11 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const TxState& state, const
1095 |      bool fUpdated = update_wtx && update_wtx(wtx, fInsertedNew);
1096 |      if (fInsertedNew) {
1097 |          wtx.nTimeReceived = GetTime();
1098 | -        wtx.nOrderPos = IncOrderPosNext(&batch);
1099 | +        std::optional<int64_t> pos = IncOrderPosNext(batch);
1100 | +        if (!pos) {
1101 | +            return nullptr;
1102 | +        }

jeanpablojp commented at 10:24 AM on September 4, 2026:

The emplace() into mapWallet happens before IncOrderPosNext(), so 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 repairing it. With CommitTransaction's update_wtx, the tx record still reaches disk while wtxOrdered stays empty. Before this change, the first call returns the wtx and writes the tx record.

        std::optional<int64_t> pos = IncOrderPosNext(batch);
        if (!pos) {
            mapWallet.erase(hash);
            return nullptr;
        }

in src/wallet/scriptpubkeyman.cpp:1156 in c794ae0e64

1151 | @@ -1136,7 +1152,9 @@ bool DescriptorScriptPubKeyMan::TopUpWithDB(WalletBatch& batch, unsigned int siz
1152 |          m_max_cached_index++;
1153 |      }
1154 |      SetRangeEnd(new_range_end);
1155 | -    batch.WriteDescriptor(GetID(), m_wallet_descriptor);
1156 | +    if (!batch.WriteDescriptor(GetID(), m_wallet_descriptor)) {
1157 | +        throw std::runtime_error(std::string(__func__) + ": updating descriptor failed");

jeanpablojp commented at 10:24 AM on September 4, 2026:

The throw propagates out of TopUp before TxnCommit, so the cache items roll back while m_max_cached_index and the new range end stay in memory. 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 returns Error loading <file>: Wallet corrupted; before this change, it reopens fine.

It depends on the descriptor. I hit it with wpkh(<xprv>/0h/*h), which needs a cached xpub per index; a plain wpkh(<xprv>/0h/0/*) reopens with no error.

Worth rolling the in-memory side back when the write fails?

in src/wallet/test/util.h:76 in c794ae0e64

  71 | @@ -68,6 +72,88 @@ class MockableSQLiteDatabase : public InMemoryWalletDatabase
  72 |      std::unique_ptr<DatabaseBatch> MakeBatch() override { return std::make_unique<MockableSQLiteBatch>(*this); }
  73 |  };
  74 |  
  75 | +/** A SQLite wallet database that can fail selected operations for testing. */
  76 | +class FaultInjectingDatabase : public MockableSQLiteDatabase

jeanpablojp commented at 10:24 AM on September 4, 2026:

All three of these go unnoticed by the suite as it stands, and FaultInjectingDatabase covers each of them with a single injected failure. The TopUpWithDB one also needs a file-backed variant, since the in-memory database dies with the wallet and the effect only shows on reload.

in src/wallet/walletdb.cpp:1203 in c794ae0e64

1200 | +        if (!this->WriteVersion(CLIENT_VERSION)) {
1201 | +            // This is the first time we write to this wallet, if there's a write failure now,
1202 | +            // there will likely be write failures in the future. Better to stop loading the wallet
1203 | +            // and not let the user use it at this time.
1204 | +            pwallet->WalletLogPrintf("Error: Unable to update the wallet last client version");
1205 | +            return DBErrors::CORRUPT;

jeanpablojp commented at 10:24 AM on September 4, 2026:

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.

            return DBErrors::LOAD_FAIL;

With that swap, loading still fails, but it is reported only as Error loading <file>. I see EraseMasterKey below already returns CORRUPT, so this may well be deliberate.