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)

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-02-04 20:54:07

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

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

furszy force-pushed on Jan 8, 2026

maflcko commented at 12:50 pm on January 19, 2026: member

Maybe turn into draft while the CI is red?

furszy force-pushed on Jan 19, 2026

furszy commented at 2:45 pm on January 19, 2026: member

Maybe turn into draft while the CI is red?

thanks for the ping, wasn’t aware of the failure.

furszy force-pushed on Jan 19, 2026

DrahtBot removed the label CI failed on Jan 19, 2026

DrahtBot added the label Needs rebase on Jan 26, 2026

furszy force-pushed on Jan 26, 2026

DrahtBot removed the label Needs rebase on Jan 26, 2026

DrahtBot added the label CI failed on Jan 26, 2026

in src/wallet/wallet.cpp:3866 in e1886ab9b1

3874     // Generate the path for the location of the migrated wallet
3875     // Wallets that are plain files rather than wallet directories will be migrated to be wallet directories.
3876-    const fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), fs::PathFromString(m_name));
3877+    const fs::path dst_wallet_path = fs::weakly_canonical(fsbridge::AbsPathJoin(GetWalletDir(), fs::PathFromString(m_name)));
3878+    // After importing all records, the new DB at tmp_wallet_path will replace the original wallet at dst_wallet_path.
3879+    std::string new_db_name = strprintf("tmp_sqlite_%d", FastRandomContext().rand64());

achow101 commented at 9:21 pm on January 26, 2026:

In e1886ab9b1abfa485bfce25b51f2dd9b566cb26a “wallet: migration, make sqlite creation robust against failures”

Could use a name that is a more informative if there’s an issue with moving the wallet back, maybe tmp_sqlite_<wallet name>_<timestamp>

furszy commented at 10:44 pm on January 26, 2026:

Sure. Done as suggested.

in src/wallet/wallet.cpp:3813 in e1886ab9b1

3808+    for (const auto& entry : fs::directory_iterator(src)) {
3809+        fs::path filename = entry.path().filename();
3810+        fs::path target = dst / filename;
3811+
3812+        if (fs::exists(target)) {
3813+            throw fs::filesystem_error("destination already exists", entry.path(), target, std::make_error_code(std::errc::file_exists));

achow101 commented at 9:24 pm on January 26, 2026:

In e1886ab9b1abfa485bfce25b51f2dd9b566cb26a “wallet: migration, make sqlite creation robust against failures”

Why throw instead of returning an error? MigrateToSQLite already bypasses failed migration cleanup.

furszy commented at 10:46 pm on January 26, 2026:

I coded it with half of my brain. Done. Changed it to return an error instead.

DrahtBot removed the label CI failed on Jan 26, 2026

furszy force-pushed on Jan 26, 2026

DrahtBot added the label CI failed on Jan 26, 2026

DrahtBot commented at 11:05 pm on January 26, 2026: contributor

🚧 At least one of the CI tasks failed. Task Windows-cross to x86_64, ucrt: https://github.com/bitcoin/bitcoin/actions/runs/21376876348/job/61534946085 LLM reason (✨ experimental): Compilation failed: invalid initialization of a const std::string& from a std::filesystem::path in wallet.cpp (PathFromString argument type mismatch).

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.

furszy force-pushed on Jan 27, 2026

DrahtBot removed the label CI failed on Jan 27, 2026

DrahtBot added the label Needs rebase on Feb 4, 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.

0935aae860

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.

8903683200

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.

09a09e48b5

test: add coverage for migrating a non-writable wallet 85047095f6

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.

406c14db5b

furszy force-pushed on Feb 4, 2026

DrahtBot removed the label Needs rebase on Feb 4, 2026

wallet: make migration more robust against failures #34193

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)