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:

#34176 (wallet: improve error msg when db directory is not writable by furszy)
#34156 (wallet: fix unnamed legacy wallet migration failure by furszy)
#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 w_desc(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-05

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

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.

68505417a2

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.

f76ed32774

furszy force-pushed on Jan 4, 2026

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.

eb5e076b18

furszy force-pushed on Jan 4, 2026

test: add coverage for migrating a non-writable wallet e0feae430e

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.

1d7324850a

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