#34193 wallet: make migration more robust against failures
https://github.com/bitcoin/bitcoin/pull/34193 · · +187/-106 in 5 files, 8 commits · labels: Wallet, Needs rebase
Goal
- Prevent legacy wallet migration from crashing or leaving wallets in a half-converted state on errors
- Ensure the original database remains intact until SQLite migration succeeds so recovery is automated
This pull request improves the robustness of legacy wallet migration by handling exceptions gracefully and retaining the original BerkeleyDB database until SQLite database creation fully succeeds. It prevents abrupt GUI crashes and ensures failed migrations can restore the original wallet automatically without user intervention.
Problem: During legacy wallet migration, unhandled exceptions or filesystem errors (such as non-writable directories) can abort the process mid-migration. This leaves the wallet in an inconsistent half-migrated state requiring manual restoration from backup.
Category: Wallet (#12 of 84)
P2 · bug fix
- P2 because it prevents users from being stranded with corrupt or interrupted wallet conversions
- Protects legacy wallet users during migration by keeping an automated recovery path on write errors
Legacy-to-descriptor migration correctness is an essential reliability safeguard for remaining legacy wallet users. Preventing broken partial migrations and automatic rollback on errors removes a high-friction failure mode that currently requires manual backup recovery.
Membership: Changes wallet migration logic in src/wallet/wallet.cpp and handles database conversion between BDB and SQLite.
Factors: security/stability 2, bug 2, performance 0, user value 2, leverage 1
Reviewability: Stale: Needs rebase
- Needs rebase due to merge conflicts with master before review is worthwhile
The PR has merge conflicts with the base branch and carries the Needs rebase label.
Author status: active, addressed reviewer feedback with force-push on 2026-09-02
Resolved concerns:
- Checking path writability up front risked TOCTOU issues and masked lower-level errors; author switched to catching filesystem exceptions during operations directly (ryanofsky).
- Temporary directory naming and error return instead of throwing in MigrateToSQLite (achow101).
- Handling symlinks, removing temporary SQLite directories on failure, and avoiding unnecessary asserts (ryanofsky).
Agreement: Strong
- Strong support because keeping original database files until migration finishes improves safety (ryanofsky)
- Cleaned up temporary SQLite files and error returns per feedback (achow101, ryanofsky)
- Feedback addressed and previous push received a code review ACK (ryanofsky)
Strong: ryanofsky supported the migration safety improvements, and all inline suggestions were addressed by the author.
ryanofsky gave a detailed code review ACK welcoming the preservation of the original database during migration failures. All suggestions by achow101 and ryanofsky have been resolved in subsequent commits.
- 2026-06-25: ryanofsky approved (Code review ACK 406c14d): 'The last commit provides a potentially significant benefit because it delays deleting the original wallet database until the new sqlite database is fully created'
- 2026-09-02: furszy pushed updates addressing all open inline review points
Objections:
| Reviewer | Kind | Harm | Status | Blocking | Author replied | Quote |
|---|---|---|---|---|---|---|
| ryanofsky | approach | Pre-checking writability introduces TOCTOU risks, duplicates write logic, and masks error reporting | resolved | no | yes | 2026-01-07: '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.' Settled: 2026-06-25: 'Initial concern i had earlier about trying to predict whether paths were writable is resolved and well handled.' |
| achow101 | correctness | Throwing an exception in MigrateToSQLite bypasses failed migration cleanup | resolved | no | yes | 2026-01-26: 'Why throw instead of returning an error? MigrateToSQLite already bypasses failed migration cleanup.' Settled: 2026-01-26: 'Changed it to return an error instead.' |
| ryanofsky | correctness | Filesystem operations could throw or leave temporary wallet directories on failure | resolved | no | yes | 2026-06-25: 'This function returns util::Result but contains a lot of filesystem calls that can throw exceptions... Could delete tmp_wallet_path here.' Settled: 2026-09-02: 'All cases should be covered now, thanks!' |
Support:
- ryanofsky: Delays deleting original database until new SQLite database is fully created so user does not need manual restore
Participants: maflcko (neutral), hebasto (neutral), achow101 (objection), ryanofsky (support), sedited (neutral)
State derived from the lists: substantive support, no open objection (ryanofsky)
Review verdicts (DrahtBot): 0 (+1)
- Stale ACK: ryanofsky
Files
File list not available for this run.
Card
This PR improves legacy wallet migration robustness by preventing crashes and ensuring failures do not leave the database in an inconsistent state. The original BerkeleyDB database is kept intact until SQLite database creation succeeds, avoiding the need for manual backup recovery. Migration correctness is high priority for safe deprecation of legacy wallets, and ryanofsky strongly supported the approach. The PR needs a rebase to resolve merge conflicts before it can proceed.