#34193 wallet: make migration more robust against failures
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #34193 wallet: make migration more robust against failures | Needs rebase | 0 (+1) | Strong | 140 + 47 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Hardens legacy wallet migration to SQLite and descriptor formats against mid-process failures and unexpected filesystem exceptions. MigrateToSQLite now stages the new SQLite database in a temporary directory and only deletes the original BerkeleyDB file after the new database is fully written. Exceptions during sub-wallet creation and migration are caught to enable graceful errors and automated restoration of the original wallet from backup without manual intervention.
Problem: During legacy wallet migration, MigrateToSQLite previously deleted the original BerkeleyDB file before verifying that the SQLite database could be fully written, forcing users to manually recover from backups if writing failed. In addition, encountering read-only directories or throwing exceptions during descriptor creation crashed the migration mid-flight and skipped failure cleanup routines.
Evidence of importance:
- PR description: '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.'
- Commit ba250143c4: '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.'
Risk of a mistake: medium (wallet, wallet_storage). A flaw in wallet file staging, copying, or cleanup during migration could delete or corrupt the original database file before the new SQLite wallet is fully written.
Changed since the previous assessment: The core approach remains the same; added cross-filesystem migration handling using copy_file with error codes and associated functional test coverage.
Reviewability
Ready · Needs rebase
Conflicts with master; review the logic now, the rebase is mechanical.
Waiting on (from the thread read):
- author: rebase 2026-10-19
Agreement
Strong
substantive support, no open objection (ryanofsky)
Thread notes: Added sedited's rebase ping; PR is currently waiting on the author to rebase.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| ryanofsky (member) | objection, approach | Checking path writability before writing masks other problems and obscures or regresses error reporting | fixed · fix pushed · addressed by directly handling write failures, acknowledged in reviewer's subsequent review | answered | yes | 2026-01-07: 'It masks other problems. Write failures should surface through the normal error-reporting paths with clear messages, regardless of where they originate.' Status from: 2026-06-25: ryanofsky: 'Initial concern i had earlier about trying to predict whether paths were writable is resolved and well handled.' |
| ryanofsky (member) | objection, approach | Checking path writability duplicates write logic and adds unnecessary complexity | fixed · fix pushed · addressed by directly handling write failures, acknowledged in reviewer's subsequent review | answered | yes | 2026-01-07: 'It adds unnecessary complexity. The write still has to be attempted, so the pre-check duplicates logic without simplifying the code.' Status from: 2026-06-25: ryanofsky: 'Initial concern i had earlier about trying to predict whether paths were writable is resolved and well handled.' |
| ryanofsky (member) | objection, safety | Checking path writability introduces TOCTOU risks where a path writable at check time is not writable at write time | fixed · fix pushed · addressed by directly handling write failures, acknowledged in reviewer's subsequent review | answered | yes | 2026-01-07: 'It introduces TOCTOU risks. A path that is writable at check time may not be writable at write time.' Status from: 2026-06-25: ryanofsky: 'Initial concern i had earlier about trying to predict whether paths were writable is resolved and well handled.' |
| achow101 (member) | suggestion, interface | Use a more informative temporary name if there is an issue moving the wallet back, such as tmp_sqlite_<wallet name>_<timestamp> | fixed · fix pushed · author implemented suggested temporary name format | answered | yes | 2026-01-26: '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>' Status from: 2026-01-26: furszy: 'Sure. Done as suggested.' |
| achow101 (member) | objection, correctness | Throwing an exception in MigrateToSQLite bypasses failed migration cleanup | fixed · fix pushed · author updated MigrateToSQLite to return an error instead of throwing | changed | yes | 2026-01-26: 'Why throw instead of returning an error? MigrateToSQLite already bypasses failed migration cleanup.' Status from: 2026-01-26: furszy: 'Changed it to return an error instead.' |
| ryanofsky (member) | suggestion, approach | Avoid deletes and renames during migration by leaving original data untouched and writing new data to a temporary directory before renaming | answered · author explained past attempt and why such a large change is impractical at this stage | answered | yes | 2026-06-25: 'More ideally the code would get rid of all these deletes and renames and backups and restores and just leave the original data exactly where it is during the migration' Status from: 2026-09-02: furszy: 'I hardly see such a big change landing at this point in time.' |
| ryanofsky (member) | suggestion, interface | Include more details in the error message, such as path and the underlying error message | fixed · fix pushed · author updated the error message as suggested | answered | yes | 2026-06-25: 'Would be good to include more details in the message like strprintf(_("Error: Wallet db cannot be updated. Path: %s, Error: %s.%s"), db_path, err_db.message(), err_help);' Status from: 2026-09-02: furszy: 'done as suggested. Thanks!' |
| ryanofsky (member) | objection, safety | Filesystem operations in a function returning Result can throw exceptions | fixed · fix pushed · author dropped the function entirely in a subsequent push | changed | yes | 2026-06-25: 'This function returns util::Result but contains a lot of filesystem calls that can throw exceptions' Status from: 2026-09-02: furszy: 'Ended up dropping the function entirely. We don't need such machinery.' |
| ryanofsky (member) | objection, correctness | fs::exists follows symlinks and returns false on broken symlinks | fixed · fix pushed · the containing function was dropped entirely | changed | no | 2026-06-25: 'fs::exists will follow symlinks which is probably not what we want here. Also this can return false if target is a broken symlink.' Status from: 2026-09-02: furszy: 'Ended up dropping the function entirely. We don't need such machinery.' |
| ryanofsky (member) | suggestion, safety | Replace asserts with simple failures now that the original database is left in place | fixed · fix pushed · author replaced asserts with simple failures | answered | yes | 2026-06-25: 'Now that original database is kept in place at this point, instead of being deleted, it would seem nice to replace these asserts with simple failures.' Status from: 2026-09-02: furszy: 'Sure, done as suggested. Thanks!' |
| ryanofsky (member) | suggestion, maintenance | Delete tmp_wallet_path on failure | fixed · fix pushed · author implemented deletion of temporary wallet path | answered | yes | 2026-06-25: 'Could delete tmp_wallet_path here' Status from: 2026-09-02: furszy: 'Sure, done as suggested. Thanks!' |
| ryanofsky (member) | suggestion, maintenance | Delete tmp_wallet_path if populating the database throws or fails | fixed · fix pushed · author implemented deletion of temporary wallet path | answered | no | 2026-06-25: 'Could delete tmp_wallet_path here. Also not sure if of the code populating this database will throw, but it would also seem good to delete it in that case too.' Status from: 2026-09-02: furszy: 'Sure, done as suggested. Thanks!' |
| ryanofsky (member) | objection, safety | If anything after fs::remove fails or throws, the wallet directory is left in a bad state requiring manual user restoration | fixed · fix pushed · author covered failure cases in subsequent push | changed | no | 2026-06-25: 'If after anything after this fs::remove fails (like the renames below or the removes or the MakeDatabase call) or throws, then the wallet directory will be an a bad state and the user will have to man' Status from: 2026-09-02: furszy: 'All cases should be covered now, thanks!' |
Support:
- ryanofsky (member) (ACK, 2026-06-25) [read]: First 4 commits straightforwardly improve error handling, and the last commit provides a significant benefit by delaying deleting the original wallet database until the new sqlite database is fully created
Participants:
- hebasto (member, neutral, 1 statements 2026-01-05 to 2026-01-05): pointed out failing WalletMigration benchmark in CI
- maflcko (member, neutral, 2 statements 2026-01-05 to 2026-01-19): reported Windows CI failure and suggested converting to draft
- ryanofsky (member, objection, 10 statements 2026-01-07 to 2026-06-25): raised objections and suggestions on error handling and cleanup, gave Code review ACK
- achow101 (member, objection, 2 statements 2026-01-26 to 2026-01-26): pointed out throwing instead of returning error bypassed migration cleanup
- sedited (member, neutral, 2 statements 2026-08-27 to 2026-10-19): pinged author to rebase
Files
48 lines under test/bench/ci.
- src/wallet/wallet.cpp +126/-104
- test/functional/wallet_migration.py +47/-1
- src/wallet/walletdb.cpp +8/-1
- src/util/fs.h +5/-0
- src/wallet/walletdb.h +1/-0
Card
The short text the ranking pass compares across PRs.
This pull request hardens legacy-to-descriptor wallet migration against mid-process crashes and filesystem permission errors. It stages new SQLite databases in temporary directories before removing original BerkeleyDB files and wraps migration in exception handlers to ensure post-failure cleanup. Without this, migration failures could leave users with partially migrated databases requiring manual recovery from backups. It touches wallet storage logic and currently needs a rebase.
Data
thread read through 2026-10-19 (27 statements), head 5beb0a753f
labels: Wallet, Needs rebase
Categories
Wallet
P1 · bug fix (assessed alone as P2; Raised: mid-migration failure can destroy the original legacy wallet files, and migration correctness is the category's second-ranked concern after fund safety.)
Prevents premature deletion of legacy wallet databases during SQLite migration and handles mid-flight exceptions cleanly. As noted in commit ba250143c4, migration previously removed the original BerkeleyDB database before verifying that the SQLite database was fully written, forcing manual recovery from backups if writing failed.
Membership: Wallet label and changes primarily in src/wallet/wallet.cpp and walletdb files.
Factors: security/stability 2, bug 2, performance 0, user value 2, leverage 0