wallet: make migration more robust against failures

furszy commented at 8:59 pm on January 2, 2026: member

This is just me finding a few more edge cases after #34156 and #34176

The goal of the PR is to handle failures in a controlled way. Just so the process can automatically restore the original wallet without requiring user manual intervention.

The covered cases are:

During DoMigration(): There are methods that can throw exceptions and abruptly abort the process.  Instead of crashing (GUI) or returning a generic exception, we now will catch and return the error gracefully. This lets the process restore the original wallet automatically.
Trying to migrate a wallet in a read-only directory throws a filesystem exception and skips cleanup.  Now the process will fail gracefully with a clear error msg, and automatically restore the original wallet.
Any failure during MigrateToSQLite requires user manual intervention.  Now the original wallet db will remain untouched, and only be updated once the sqlite db creation fully succeeds.

DrahtBot added the label Wallet on Jan 2, 2026

DrahtBot commented at 8:59 pm on January 2, 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/34193.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33014 (rpc: Fix internal bug in descriptorprocesspsbt when encountering invalid signatures by b-l-u-e)
#32636 (Split CWallet::Create() into CreateNew and LoadExisting by davidgumberg)

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 places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

[WalletDescriptor(std::move(descs.at(0)), creation_time, 0, 0, 0)] in src/wallet/wallet.cpp
[out_wallet->AddWalletDescriptor(w_desc, keys, “”, false)] in src/wallet/wallet.cpp

2026-01-08

furszy closed this on Jan 2, 2026

furszy renamed this:
~~wallet: make migration more robust~~
wallet: make migration more robust against failures
on Jan 2, 2026

furszy reopened this on Jan 2, 2026

furszy force-pushed on Jan 2, 2026

DrahtBot added the label CI failed on Jan 2, 2026

furszy force-pushed on Jan 4, 2026

furszy force-pushed on Jan 5, 2026

maflcko commented at 10:16 am on January 5, 2026: member

Looks like the CI fails for msvcrt, but not for ucrt:

0Run ./bin/bench_bitcoin.exe -sanity-check
1Running with -sanity-check option, output is being suppressed as benchmark results will be useless.
2Error: filesystem error: cannot copy: File exists [C:\Users\RUNNER~1\AppData\Local\Temp\test_common bitcoin\WalletMigration\bb3603a0a694b3c4c79a\regtest\tmp_sqlite_13776532190043757581] [C:\Users\RUNNER~1\AppData\Local\Temp\test_common bitcoin\WalletMigration\bb3603a0a694b3c4c79a\regtest]

https://github.com/bitcoin/bitcoin/actions/runs/20702313540/job/59427203126?pr=34193#step:9:200

hebasto commented at 12:08 pm on January 5, 2026: member

Looks like the CI fails for msvcrt, but not for ucrt:

0Run ./bin/bench_bitcoin.exe -sanity-check
1Running with -sanity-check option, output is being suppressed as benchmark results will be useless.
2Error: filesystem error: cannot copy: File exists [C:\Users\RUNNER~1\AppData\Local\Temp\test_common bitcoin\WalletMigration\bb3603a0a694b3c4c79a\regtest\tmp_sqlite_13776532190043757581] [C:\Users\RUNNER~1\AppData\Local\Temp\test_common bitcoin\WalletMigration\bb3603a0a694b3c4c79a\regtest]

https://github.com/bitcoin/bitcoin/actions/runs/20702313540/job/59427203126?pr=34193#step:9:200

Specifically, the WalletMigration benchmark fails.

DrahtBot added the label Needs rebase on Jan 7, 2026

wallet: migration, unify watchonly and solvables wallets creation

No need to repeat the exact same code twice.
This will let us introduce new safety checks and improve error
handling for both wallets without the risk of forgetting to
update one of them.

e8b6b8bd9c

wallet: migration, handle exceptions during wallet creation

Exceptions can be thrown internally by 'MakeWalletDatabase',
'CWallet::Create' and 'AddWalletDescriptor'. Instead of
abruptly interrupting the process, catch them and return
error gracefully.

This ensures migration can be cleaned up and the original
wallet is restored from the backup if something goes wrong.

d97a52dac7

wallet: fail migration gracefully on non-writable wallets

Currently, attempting a wallet migration when the database
directory or file is not writable results in a filesystem
exception that abruptly aborts the process, skipping the
post-failure cleanup logic and returning a not particularly
helpful error message.

23f8cfcd65

test: add coverage for migrating a non-writable wallet 47e577fdb1

in src/util/fs_helpers.cpp:308 in eb5e076b18

304@@ -304,6 +305,28 @@ std::optional<fs::perms> InterpretPermString(const std::string& s)
305     }
306 }
307 
308+bool IsFileWritable(const fs::path& file)

ryanofsky commented at 3:18 pm on January 7, 2026:

In commit “wallet: fail migration gracefully on non-writable wallets” (eb5e076b189aa5dadb9572f3ce7b33eb609d7b69)

Was skimming this PR and the first few commits seem reasonable, but I’d advise against the approach in the last two commits of checking whether paths are writable before writing, rather than handling write failures directly. A few concerns:

It masks other problems. Write failures should surface through the normal error-reporting paths with clear messages, regardless of where they originate. Pre-checks bypass those paths and can obscure or regress error reporting, especially if lower-level errors improve over time.
It adds unnecessary complexity. The write still has to be attempted, so the pre-check duplicates logic without simplifying the code.
It introduces TOCTOU risks. A path that is writable at check time may not be writable at write time.

In general, attempting the write and handling failures directly is simpler, more robust, and leads to better error reporting.

furszy commented at 8:42 pm on January 7, 2026:

The issue here is that we are doing low-level operations. Basically, we are:

Loading all BDB records into memory.
Deleting the BDB file.
Creating a new sqlite db.
Writing all records to the new db.

The added checks are meant to avoid failing with a generic filesystem exception at step (2), during the BDB deletion, which can be located in a different directory than the sqlite one we are about to create

But.. thinking further, we could handle this differently by catching the exception directly from the fs::remove call instead. Let me see why I didn’t do it in that way before, there must be a reason.

Also, the last commit is about not removing the original wallet until we are sure the sqlite one has at least been created successfully. If we are not able to write to disk at that point, the migration will fail anyway. This function is just the early “move everything from BDB to sqlite” step; we still need to create and store the descriptors, remove the old records, and so on.

furszy force-pushed on Jan 7, 2026

DrahtBot removed the label Needs rebase on Jan 7, 2026

wallet: migration, make sqlite creation robust against failures

Currently, MigrateToSQLite loads all BDB records into memory,
then deletes the original database just to create the sqlite
db in the same place, and finally writes all cached records
to it.

This is not really the best because it removes the original db
before making sure the sqlite one is fully built. If creation
fails, the wallet ends up partly in sqlite format with legacy
records, and the user has to restore from a backup manually.

This fixes all that by creating the sqlite db in a temporary
directory first. Then, the original BDB files are removed only
once the sqlite db is fully constructed, and the new db is
moved into the original location.

The result is that no user manual intervention is needed if
MigrateToSQLite fails as the original db stays untouched.

ad3942500b

furszy force-pushed on Jan 8, 2026

wallet: make migration more robust against failures #34193

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)