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:

#36133 (wallet: store multipath descriptor by Sjors)
#36126 (wallet, rpc: Implements set key label functionality by polespinasa)
#36070 (wallet: Add deriveHDKey interface by PraneethGunas)
#36031 (wallet: Remove mapMasterKeys and enforce that only one encryption key can exist by achow101)
#35786 (wallet: drop spent parents redundant cache invalidation and notification by furszy)
#35444 (wallet: make descriptor SPKM mutex non-recursive by w0xlt)
#33034 (wallet: Store transactions in a separate sqlite table by achow101)
#32895 (wallet: Prepare for future upgrades by recording versions of last client to open and decrypt 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 the tx record [“the a” is a typo]

2026-09-22 20:07:30

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.

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 outdated

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?

achow101 commented at 8:24 PM on September 8, 2026:

No. 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.

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

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;
        }

achow101 commented at 8:27 PM on September 8, 2026:

This is ok, a failure here results in an exception higher up in the call stack which will result in a crash. Database write failures are supposed to be catastrophic, they are not expected to happen.

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

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?

achow101 commented at 8:30 PM on September 8, 2026:

The retry

What retry?

Write failures are supposed to be catastrophic and an indicator that corruption has likely happened.

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

  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.

achow101 commented at 8:31 PM on September 8, 2026:

it is not necessary to leave review comments describing what is happening in the code.

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

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.

achow101 commented at 8:32 PM on September 8, 2026:

Failure to write is always corruption, regardless of how that may come about. We don't know why the write failed, it could be for reasons other than a disk being full.

DrahtBot added the label Needs rebase on Sep 12, 2026

achow101 force-pushed on Sep 14, 2026

DrahtBot removed the label Needs rebase on Sep 14, 2026

DrahtBot added the label CI failed on Sep 14, 2026

DrahtBot commented at 11:37 PM on September 14, 2026: contributor

🚧 At least one of the CI tasks failed. Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/34902498233/job/104171483904 LLM reason (✨ experimental): IWYU failed because it detected/introduced required #include fixes (e.g., in src/interfaces/wallet.h), causing the CI step to exit non-zero.

<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>

achow101 force-pushed on Sep 14, 2026

DrahtBot removed the label CI failed on Sep 15, 2026

DrahtBot added the label Needs rebase on Sep 17, 2026

wallet: Handle db write failures in ExportWatchOnlyWallet 0fc8e69e4c

wallet: Explicitly mark ignored write failures

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

7a833e4571

wallet: Handle descriptor update write failures 547d853bb5

wallet: Handle Write failure during loading 8be1f0d8ad

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.

6fb87b5d22

wallet: Handle db write failure in IncOrderPosNext 3f54b9ed95

wallet: Handle transaction write failures

Handle write failures for WriteOrderPosNext and WriteTx

ae62f3db44

wallet, migration: Handle address book data write failures ae1dcbcbb5

wallet, bench: Ignore db write failures 1c4c84fc33

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

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

6a900b5b55

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.

d16ed3e405

achow101 force-pushed on Sep 22, 2026

achow101 marked this as ready for review on Sep 22, 2026

DrahtBot removed the label Needs rebase on Sep 22, 2026