wallet: make migration more robust against failures #34193
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34193. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
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.
2026-01-08 |
furszy
changed the title
furszy
force-pushed
the
2026_wallet_safer_MigrateToSQLite
branch
from
91aed5f to
3e59872
Compare
furszy
force-pushed
the
2026_wallet_safer_MigrateToSQLite
branch
3 times, most recently
from
0fc9bdd to
1d73248
Compare
|
Looks like the CI fails for msvcrt, but not for ucrt: |
Specifically, the |
src/util/fs_helpers.cpp
Outdated
| } | ||
| } | ||
|
|
||
| bool IsFileWritable(const fs::path& file) |
There was a problem hiding this comment.
In commit "wallet: fail migration gracefully on non-writable wallets" (eb5e076)
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.
There was a problem hiding this comment.
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.
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.
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.
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.
furszy
force-pushed
the
2026_wallet_safer_MigrateToSQLite
branch
from
1d73248 to
396998b
Compare
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.
furszy
force-pushed
the
2026_wallet_safer_MigrateToSQLite
branch
from
396998b to
ad39425
Compare
5 participants
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
MigrateToSQLiterequires user manual intervention.Now the original wallet db will remain untouched, and only be updated once the sqlite db creation fully succeeds.