wallet: fix unnamed legacy wallet migration failure

furszy commented at 4:08 am on December 27, 2025: member

Minimal fix for #34128. This is the first part of a series of changes.

The issue occurs during the migration of a legacy unnamed wallet (the legacy “default” wallet). When the migration fails, the cleanup logic is triggered to roll back the state, which involves erasing the newly created descriptor wallets directories. Normally, this only affects the parent directories of named wallets, since they each reside in their own directories. However, because the unnamed wallet resides directly in the top-level /wallets/ folder, this logic accidentally deletes the main directory.

The fix ensures that only the wallet.dat file of the unnamed wallet is touched and restored, preserving the wallet in BDB format and leaving the main /wallets/ directory intact.

Testing Notes: Cherry-pick the test commit on top of master and run it. You will see the failure and realize the reason by reading the test code.

Important Note: I’m pushing it early to alert users and start discussing the solution. As written on the first sentence, this is a minimal fix, and further improvements are possible (and planned). For example, files removal could be specialized as in b78990734621b8fe46c68a6e7edaf1fbd2f7d351, but additional changes are needed to implement it.

DrahtBot added the label Wallet on Dec 27, 2025

DrahtBot commented at 4:08 am on December 27, 2025: 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/34156.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	pablomartin4btc

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

exist -> exists [subject-verb agreement: “the dir already exist” should be “the dir already exists”]

2025-12-28

achow101 added this to the milestone 31.0 on Dec 27, 2025

achow101 requested review from achow101 on Dec 27, 2025

achow101 commented at 4:47 am on December 27, 2025: member

In #34128, the logs say that migration was successful, which is confusing as to how they ended up in the failure case. It looks like that log line is a little misleading since we can hit a failure after it is logged if the migrated wallet(s) cannot be reloaded, which will result in the failure cleanup being hit.

So a followup question is what happened that migration thinks it was successful, but subsequent loading failed.

Anyways, deleting user wallets is super bad and we should fix that ASAP.

in src/wallet/wallet.cpp:4329 in 24095b090b

4323@@ -4324,8 +4324,14 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
4324         for (std::shared_ptr<CWallet>* wallet_ptr : {&local_wallet, &res.watchonly_wallet, &res.solvables_wallet}) {
4325             if (success && *wallet_ptr) {
4326                 std::shared_ptr<CWallet>& wallet = *wallet_ptr;
4327-                // Save db path and load wallet
4328-                wallet_dirs.insert(fs::PathFromString(wallet->GetDatabase().Filename()).parent_path());
4329+                if (wallet->GetName().empty()) {
4330+                    // Only store the migrated wallet.dat for removal, never its parent directory.
4331+                    wallet_dirs_to_remove.insert(fs::PathFromString(wallet->GetDatabase().Filename()));

achow101 commented at 4:49 am on December 27, 2025:

In 24095b090b8784a30a8524207d163ff970af1b97 “wallet: fix unnamed legacy wallet migration failure”

Should use Files() to cleanup the -journal file too.

furszy commented at 5:37 pm on December 27, 2025:

Done as suggested.

DrahtBot added the label CI failed on Dec 27, 2025

achow101 commented at 5:25 am on December 27, 2025: member

This appears to only affect 30.x.

29.x and earlier are unable to deal with default wallet migration failures in the first place as they have an issue with copying the legacy wallet backup.

wallet: fix unnamed legacy wallet migration failure

When migrating the legacy unnamed wallet, a failed migration would
cause the cleanup logic to remove its parent directory. Since the
legacy wallet lives directly in the main '/wallets/' folder, this
resulted in unintentionally erasing all wallets, including the
backup file.

fba31537de

furszy force-pushed on Dec 27, 2025

furszy commented at 5:47 pm on December 27, 2025: member

In #34128, the logs say that migration was successful, which is confusing as to how they ended up in the failure case. It looks like that log line is a little misleading since we can hit a failure after it is logged if the migrated wallet(s) cannot be reloaded, which will result in the failure cleanup being hit.

So a followup question is what happened that migration thinks it was successful, but subsequent loading failed.

Anyways, deleting user wallets is super bad and we should fix that ASAP.

Yeah, the failure must be related to reloading the wallet after migration, still I didn’t want to delay this fix any longer. Any other “fail to migrate” bug would be very minor and harmless compared to removing the /wallets/ directory.

This appears to only affect 30.x.

Thanks for checking, so we just need to back port it there.

furszy force-pushed on Dec 27, 2025

DrahtBot removed the label CI failed on Dec 27, 2025

achow101 added the label Needs backport (30.x) on Dec 27, 2025

test: add coverage for unnamed wallet migration failure

Verifies that a failed migration of the unnamed (default) wallet
does not erase the main /wallets/ directory, and also that the
backup file exists.

e3edb5bbc7

wallet: migration, fix unnamed wallet restore after failure

Previously, the legacy unnamed (default) wallet could not be
restored after a failed migration because its parent directory
was incorrectly treated as a normal wallet directory.
In reality, this parent dir is the main /wallets/ folder, which
always exists.

This change ensures that only the wallet.dat file is checked
and restored for the unnamed wallet, and that it remains in BDB
format.

7af5d1f102

wallet: improve post-migration logging

Right now, after migration the last message users see is "migration completed",
but the migration isn't actually finished yet. We still need to load the new wallets
to ensure consistency, and if that fails, the migration will be rolled back. This
can be confusing for users.

This change logs the post-migration loading step and if a wallet fails to load and
the migration will be rolled back.

f8b9d10e3e

furszy force-pushed on Dec 27, 2025

DrahtBot added the label CI failed on Dec 27, 2025

furszy commented at 11:32 pm on December 27, 2025: member

Added two more commits.

First one improves logging for post-migration failures, making it clearer to users (and us) if a migration is rolled back during the wallets reload process.

Second one fixes a bug where migrated watch-only and solvables wallets were incorrectly named when created from an unnamed default wallet.

wallet: migration, fix watch-only and solvables wallets names

Because the default wallet has no name, the watch-only and solvables
wallets created during migration end up having no name either.

This fixes it by applying the same prefix name we use for the backup
file for an unnamed default wallet.

Before: watch-only wallet named "_watchonly"
After:  watch-only wallet named "default_wallet_watchonly"

6ecfbc7688

furszy force-pushed on Dec 28, 2025

pablomartin4btc commented at 9:55 pm on December 31, 2025: member

Concept ACK

Good fix! Light code review, I’ll test and re-review soon…

wallet: fix unnamed legacy wallet migration failure #34156

Code Coverage & Benchmarks

Reviews

LLM Linter (✨ experimental)