wallet: make migration more robust against failures #34193

pull furszy wants to merge 5 commits into bitcoin:master from furszy:2026_wallet_safer_MigrateToSQLite changing 3 files +163 −89
  1. 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:

    1. 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.

    2. 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.

    3. 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.

  2. DrahtBot added the label Wallet on Jan 2, 2026
  3. DrahtBot commented at 8:59 PM on January 2, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34193.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK ryanofsky

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #35266 (rpc, wallet: add an option to not load the wallet after migrating by polespinasa)
    • #34909 (wallet, refactor: modularise wallet by extracting out legacy wallet migration by rkrux)
    • #28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)
    • #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)

    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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

    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

    <sup>2026-02-04 20:54:07</sup>

  4. furszy closed this on Jan 2, 2026
  5. furszy renamed this:
    wallet: make migration more robust
    wallet: make migration more robust against failures
    on Jan 2, 2026
  6. furszy reopened this on Jan 2, 2026
  7. furszy force-pushed on Jan 2, 2026
  8. DrahtBot added the label CI failed on Jan 2, 2026
  9. furszy force-pushed on Jan 4, 2026
  10. furszy force-pushed on Jan 4, 2026
  11. furszy force-pushed on Jan 5, 2026
  12. maflcko commented at 10:16 AM on January 5, 2026: member

    Looks like the CI fails for msvcrt, but not for ucrt:

    Run ./bin/bench_bitcoin.exe -sanity-check
Running with -sanity-check option, output is being suppressed as benchmark results will be useless.
Error: 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

  13. hebasto commented at 12:08 PM on January 5, 2026: member

    Looks like the CI fails for msvcrt, but not for ucrt:

    Run ./bin/bench_bitcoin.exe -sanity-check
Running with -sanity-check option, output is being suppressed as benchmark results will be useless.
Error: 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.

  14. DrahtBot added the label Needs rebase on Jan 7, 2026
  15. 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:

    1. 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.

    2. It adds unnecessary complexity. The write still has to be attempted, so the pre-check duplicates logic without simplifying the code.

    3. 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:

    1. Loading all BDB records into memory.
    2. Deleting the BDB file.
    3. Creating a new sqlite db.
    4. 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.

  16. furszy force-pushed on Jan 7, 2026
  17. DrahtBot removed the label Needs rebase on Jan 7, 2026
  18. furszy force-pushed on Jan 8, 2026
  19. maflcko commented at 12:50 PM on January 19, 2026: member

    Maybe turn into draft while the CI is red?

  20. furszy force-pushed on Jan 19, 2026
  21. 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.

  22. furszy force-pushed on Jan 19, 2026
  23. DrahtBot removed the label CI failed on Jan 19, 2026
  24. DrahtBot added the label Needs rebase on Jan 26, 2026
  25. furszy force-pushed on Jan 26, 2026
  26. DrahtBot removed the label Needs rebase on Jan 26, 2026
  27. DrahtBot added the label CI failed on Jan 26, 2026
  28. 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.

  29. 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.

  30. DrahtBot removed the label CI failed on Jan 26, 2026
  31. furszy force-pushed on Jan 26, 2026
  32. furszy force-pushed on Jan 26, 2026
  33. DrahtBot added the label CI failed on Jan 26, 2026
  34. DrahtBot commented at 11:05 PM on January 26, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

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

    <details><summary>Hints</summary>

    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.

    </details>

  35. furszy force-pushed on Jan 27, 2026
  36. furszy force-pushed on Jan 27, 2026
  37. DrahtBot removed the label CI failed on Jan 27, 2026
  38. DrahtBot added the label Needs rebase on Feb 4, 2026
  39. 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
  40. 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
  41. 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
  42. test: add coverage for migrating a non-writable wallet 85047095f6
  43. 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
  44. furszy force-pushed on Feb 4, 2026
  45. DrahtBot removed the label Needs rebase on Feb 4, 2026
  46. sedited requested review from ryanofsky on May 23, 2026
  47. DrahtBot added the label Needs rebase on May 28, 2026
  48. DrahtBot commented at 8:16 PM on May 28, 2026: contributor

    <!--cf906140f33d8803c4a75a2196329ecb-->

    🐙 This pull request conflicts with the target branch and needs rebase.

  49. in src/wallet/wallet.cpp:3884 in 09a09e48b5 
    3880 | +    fs::remove(db_path, err_db);
3881 | +    if (err_db) {
3882 | +        std::string err_help;
3883 | +        const bool perm_issue = err_db.value() == EACCES || err_db.value() == EPERM  || err_db.value() == EROFS;
3884 | +        if (perm_issue) err_help = "Adjust directory or file permissions to proceed with migration.";
3885 | +        error = strprintf(_("Error: Wallet db cannot be updated. %s"), err_help);
    ryanofsky commented at 12:12 AM on June 25, 2026:

    In commit "wallet: fail migration gracefully on non-writable wallets" (09a09e48b53dcf7c10f9f4b89a7d3697589751e1)

    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);. Error messages saying something failed without saying why it failed can be frustrating to deal with.

  50. in src/wallet/wallet.cpp:3838 in 406c14db5b 
    3832 | @@ -3833,6 +3833,36 @@ util::Result<std::reference_wrapper<DescriptorScriptPubKeyMan>> CWallet::AddWall
3833 |      return std::reference_wrapper(*spk_man);
3834 |  }
3835 |  
3836 | +// Move files from `src` into `dst`.
3837 | +// Note: This function doesn't recurse: `src` must not contain subdirectories.
3838 | +util::Result<void> MoveDirContent(const fs::path& src, const fs::path& dst)
    ryanofsky commented at 12:55 AM on June 25, 2026:

    In commit "wallet: migration, make sqlite creation robust against failures" (406c14db5b16c2e1befdef76d6debd0b0ca4f6c2)

    This function returns util::Result but contains a lot of filesystem calls that can throw exceptions (directory_iterator constructor, fs::exists, fs::rename). It could be good to call non throwing version of these function and return error messages if these calls fail, or wrap the function body in a try/catch. Or keep current code but document that this can throw in addition to return a Result error so callers know to handle this.

  51. in src/wallet/wallet.cpp:3847 in 406c14db5b 
    3842 | +
3843 | +    // Validate all dst paths up front to avoid partial moves
3844 | +    std::vector<std::pair<fs::path, fs::path>> files_to_move;
3845 | +    for (const auto& entry : fs::directory_iterator(src)) {
3846 | +        const fs::path target = dst / fs::path(entry.path().filename());
3847 | +        if (fs::exists(target)) {
    ryanofsky commented at 12:58 AM on June 25, 2026:

    In commit "wallet: migration, make sqlite creation robust against failures" (406c14db5b16c2e1befdef76d6debd0b0ca4f6c2)

    fs::exists will follow symlinks which is probably not what we want here. Also this can return false if target is a broken symlink. Could use if (fs::is_symlink(target) || fs::exists(target)) or similar to prevent these things

  52. in src/wallet/wallet.cpp:3943 in 406c14db5b 
    3957 | -            assert(false); // This is a critical error, the new db could not be written to. The original db exists as a backup, but we should not continue execution.
3958 | +            assert(false); // This is a critical error, the new db could not be written to. The original db is untouched, so we can abort safely.
3959 |          }
3960 |      }
3961 |      bool committed = batch->TxnCommit();
3962 |      assert(committed); // This is a critical error, the new db could not be written to. The original db exists as a backup, but we should not continue execution.
    ryanofsky commented at 12:59 AM on June 25, 2026:

    In commit "wallet: migration, make sqlite creation robust against failures" (406c14db5b16c2e1befdef76d6debd0b0ca4f6c2)

    s/exists as a backup/is untouched/ here as well

  53. in src/wallet/wallet.cpp:3933 in 406c14db5b 
    3946 | +
3947 |      // Write existing records into the new DB
3948 |      batch = m_database->MakeBatch();
3949 |      bool began = batch->TxnBegin();
3950 | -    assert(began); // This is a critical error, the new db could not be written to. The original db exists as a backup, but we should not continue execution.
3951 | +    assert(began); // This is a critical error, the new db could not be written to. The original db is untouched, so we can abort safely.
    ryanofsky commented at 1:05 AM on June 25, 2026:

    In commit "wallet: migration, make sqlite creation robust against failures" (406c14db5b16c2e1befdef76d6debd0b0ca4f6c2)

    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. Previously it (sort of) made sense to crash the process at this point since the database was in half-written incomplete state. But now the original database is unchanged, and there isn't actually a critical error anymore.

  54. in src/wallet/wallet.cpp:3957 in 406c14db5b 
    3971 | +    if (err_db) {
3972 | +        std::string err_help;
3973 | +        const bool perm_issue = err_db.value() == EACCES || err_db.value() == EPERM  || err_db.value() == EROFS;
3974 | +        if (perm_issue) err_help = "Adjust directory or file permissions to proceed with migration.";
3975 | +        error = strprintf(_("Error: Wallet db cannot be updated. %s"), err_help);
3976 | +        return false;
    ryanofsky commented at 1:09 AM on June 25, 2026:

    In commit "wallet: migration, make sqlite creation robust against failures" (406c14db5b16c2e1befdef76d6debd0b0ca4f6c2)

    Could delete tmp_wallet_path here

  55. in src/wallet/wallet.cpp:3923 in 406c14db5b 
    3935 |      opts.require_format = DatabaseFormat::SQLITE;
3936 |      DatabaseStatus db_status;
3937 | -    std::unique_ptr<WalletDatabase> new_db = MakeDatabase(wallet_path, opts, db_status, error);
3938 | -    assert(new_db); // This is to prevent doing anything further with this wallet. The original file was deleted, but a backup exists.
3939 | +    std::unique_ptr<WalletDatabase> new_db = MakeDatabase(tmp_wallet_path, opts, db_status, error);
3940 | +    if (!new_db) return false; // error msg appended internally
    ryanofsky commented at 1:12 AM on June 25, 2026:

    In commit "wallet: migration, make sqlite creation robust against failures" (406c14db5b16c2e1befdef76d6debd0b0ca4f6c2)

    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.

  56. in src/wallet/wallet.cpp:3951 in 406c14db5b 
    3965 | +    // ######################################################################
3966 | +    // At this point, the new database has all records.                     #
3967 | +    // We can remove the old db file and move the new db inside the wallet  #
3968 | +    // ######################################################################
3969 | +    std::error_code err_db;
3970 | +    fs::remove(origin_db_path, err_db);
    ryanofsky commented at 1:53 AM on June 25, 2026:

    In commit "wallet: migration, make sqlite creation robust against failures" (406c14db5b16c2e1befdef76d6debd0b0ca4f6c2)

    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 manually restore from a backup. This was also the case with the previous fs::remove call, but that code has had more testing while this code is new. So it might be more likely to have problems. Might be possible to address this by having MigrateLegacyToDescriptor call RestoreWallet in this case.

  57. ryanofsky approved
  58. ryanofsky commented at 1:59 AM on June 25, 2026: contributor

    Code review ACK 406c14db5b16c2e1befdef76d6debd0b0ca4f6c2. I think this looks good, and all the changes here seem safe, but given complexity of the code would want to have another pass of review to have more confidence there are no bugs.

    On the approach: first 4 commits look great and straightforwardly improve error handling. Initial concern i had earlier about trying to predict whether paths were writable is resolved and well handled.

    The last commit should also be a net improvement, but is more of a mixed bag. The last commit provides a potentially significant benefit because it delays deleting the original wallet database until the new sqlite database is fully created, so if there are errors creating it, the old wallet will be left in place, and it it can be migrated again later (or dumped or opened in an old version of bitcoin core) without the user manually needing to restore from the backup file.

    The downside of the last commit is it adds more complexity to code that is already pretty complex, and leaves filesystem in a slightly messier state because if creating and populating the sqlite database fails there will now be two copies of the original bdb database instead of one.

    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, writing the new data to temporary directory. Then do simple renames if the migration succeeds, renaming the old wallet to have a migrated_<timestamp> suffix and renaming the new wallet directories in its place.

Contributors
furszyDrahtBotmaflckohebastoryanofskyachow101
Labels
WalletNeeds rebase
Linked (view graph)
#34156 wallet: fix unnamed legacy wallet migration failure#34176 wallet: crash fix, handle non-writable db directories
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-07-02 07:51 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me