wallet: fix unnamed legacy wallet migration failure

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

Minimal fix for #34128.

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.

Story Line:

#32273 fixed a different set of issues and, in doing so, uncovered this one. Before the mentioned PR, backups were stored in the same directory as the wallet.dat file. On a migration failure, the backup was then copied to the top-level /wallets/ directory. For the unnamed legacy wallet, the wallet directory is the /wallets/ directory, so the source and destination paths were identical. As a result, we threw early in the fs::copy_file call (here) because the file already existed, as we were trying to copy the file onto itself. This caused the cleanup logic to abort early on and never reach the removal line.

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.

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
ACK	achow101, w0xlt, davidgumberg, willcl-ark
Concept ACK	pablomartin4btc, rkrux

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34198 (wallet: fix ancient wallets migration by furszy)
#34193 (wallet: make migration more robust against failures by furszy)

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.

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.

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

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.

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…

maflcko removed the label CI failed on Jan 1, 2026

achow101 commented at 8:40 pm on January 2, 2026: member

ACK 6ecfbc7688971a506720d29fe99d1431662d2941

DrahtBot requested review from pablomartin4btc on Jan 2, 2026

achow101 removed this from the milestone 31.0 on Jan 2, 2026

achow101 added this to the milestone 30.2 on Jan 2, 2026

furszy force-pushed on Jan 4, 2026

furszy commented at 5:46 pm on January 4, 2026: member

Just for completeness, added an extra test commit checking migration fails gracefully when the wallet last sync is beyond the node’s prune height. Per #34128 (comment).

Nothing else was changed. Sorry for invalidating your ACK achow; took me a few minutes to add and doesn’t hurt to have extra tests.

Ready to go.

furszy force-pushed on Jan 4, 2026

DrahtBot added the label CI failed on Jan 4, 2026

furszy force-pushed on Jan 4, 2026

DrahtBot removed the label CI failed on Jan 4, 2026

achow101 commented at 0:49 am on January 5, 2026: member

ACK ca6bfda55fca7682c8f74818e7833a01d97a0ebf

willcl-ark commented at 11:28 am on January 5, 2026: member

The fix looks correct to me, and I’d be happy to ACK it as-is for now as it is is a pretty urgent fix.

But, based on the potential severity of this I still feel a little unfomfortable that we are essentially adding more special-cased code in an effort to make this operation more robust. Despite the new test added (thanks for that), I worry that a future change to default/named wallets could still break this again in some as-yet unforeseen way.

Perhaps we should consider rethinking the general concept of deleting files in a wallet migration codepath at all, and try to make it generally safer. In addition to the changes here, as a belt-and-suspenders approach, we could remove the fs::delete() call from here:

https://github.com/bitcoin/bitcoin/blob/fba31537def3a5e5c157d431e92ad30292bc2970/src/wallet/wallet.cpp#L4387-L4392

…and instead safely rename() previously-to-be-deleted migration files into a new /wallets/failed-migrations dir?

I think it’s highly preferable to find a “messy” /wallet/ dir full of failed migrations, which can eventually be used to restore from, than an empty one!

rkrux commented at 2:14 pm on January 5, 2026: contributor

Definite Concept ACK due to the nature of the issue, will review.

darosior commented at 3:09 pm on January 5, 2026: member

I am only starting to look into this, but i conceptually agree with @willcl-ark. I think ideally the migration code would never end up deleting the directory in any circumstances. The tradeoff here is to get a fix for the current issue as soon as possible: @furszy @achow101 could you comment on whether this would significantly complicate the implementation and delay the publication of a fix? (Or be otherwise undesirable?) In this case i would be in favour of releasing this ASAP and working on a never-delete patch for a later point release.

darosior commented at 3:44 pm on January 5, 2026: member

whether this would significantly complicate the implementation and delay the publication of a fix?

Maybe an alternative way of getting the failsafe could be to change the location of the backup to not be the wallet’s directory, which may be erased later on, but a different directory? https://github.com/bitcoin/bitcoin/blob/755f0900a28a874a65fb843c74d8257f96b667dc/src/wallet/wallet.cpp#L4244-L4254

furszy commented at 4:08 pm on January 5, 2026: member

But, based on the potential severity of this I still feel a little unfomfortable that we are essentially adding more special-cased code in an effort to make this operation more robust. Despite the new test added (thanks for that), I worry that a future change to default/named wallets could still break this again in some as-yet unforeseen way.

Perhaps we should consider rethinking the general concept of deleting files in a wallet migration codepath at all, and try to make it generally safer. In addition to the changes here, as a belt-and-suspenders approach, we could remove the fs::delete() call from here:

https://github.com/bitcoin/bitcoin/blob/fba31537def3a5e5c157d431e92ad30292bc2970/src/wallet/wallet.cpp#L4387-L4392

…and instead safely rename() previously-to-be-deleted migration files into a new /wallets/failed-migrations dir?

I think it’s highly preferable to find a “messy” /wallet/ dir full of failed migrations, which can eventually be used to restore from, than an empty one!

This will be a long answer, but the short answer is: yes, I’m working on a safer workflow.

This is why I started the PR description with the “Minimal fix” wording and the “This is the first part of a series of changes” sentence. And the reason behind the other PRs I have been pushing these past days.

The code lines you point out are not the only dangerous ones. At that stage, we are deleting the wallets we created throughout the migration procedure; the original BDB wallet is deleted much earlier in the process, prior to generating the sqlite database, inside MigrateToSQLite. This is one of the aspects #34193 improves.

I could detail the entire process if you need it but to be really specific; at the point you are highlighting there, we actually need to delete the wallets, because otherwise the user could end up with a partially migrated sqlite wallet with legacy records inside or, even worse, a descriptors wallet that lacks some information. The key point here is that we must only delete what we have created, and nothing else.

My end goal the past days has been to revive https://github.com/furszy/bitcoin-core/tree/2023_wallet_rewrite_migration (specifically https://github.com/furszy/bitcoin-core/commit/e9c22da457ccec67a99359d595a7469a926b18b7) which is the safest workflow, but this is a major restructuring of the process, which is not that easy to adapt after 3 years (you can check the commit description, even when it is old, it is still accurate enough).

That being said, let me see if I can port the targeted single files removal changes only. The main pain points here are the parent_path() and remove_all() calls, which we might be able to avoid.

darosior commented at 4:25 pm on January 5, 2026: member

So, to be clear, your recommendation here is we should review and ship this PR as soon as possible, and your other PRs can go in a later release?

furszy commented at 4:39 pm on January 5, 2026: member

So, to be clear, your recommendation here is we should review and ship this PR as soon as possible, and your other PRs can go in a later release?

Sorry, it was on the last paragraph of my write up. I have an idea on another minimal change-set I can port here that would let us get-rid of the parent_path() and remove_all() calls. Shouldn’t take me much to complete, will update the PR as soon as the changes are ready.

furszy force-pushed on Jan 5, 2026

furszy commented at 7:21 pm on January 5, 2026: member

Updated per feedback. We no longer use parent_path() or remove_all() in the migration cleanup logic. Only the files created during migration are removed; everything else is left intact. Because of this, I had to slightly adapt RestoreWallet to allow existing directories as the destination. Needed because we no longer remove the original wallet directory.

Now that this is available for review, I’m going to add a restorewallet() test next, just for completeness.

furszy force-pushed on Jan 5, 2026

DrahtBot added the label CI failed on Jan 5, 2026

DrahtBot removed the label CI failed on Jan 5, 2026

in src/wallet/wallet.cpp:4372 in d9e4593afb outdated

4354+
4355+                // Keep track of created database files to allow cleanup if the procedure fails.
4356+                const auto files = wallet->GetDatabase().Files();
4357+                wallet_files_to_remove.insert(files.begin(), files.end());
4358+                if (wallet->GetName() != wallet_name) {
4359+                    // Only when this is not the main wallet, it is fully safe to delete the parent path

achow101 commented at 9:15 pm on January 5, 2026:

In d9e4593afb386268fbdd5f7fc1f409ea2253a244 “wallet: fix unnamed legacy wallet migration failure”

nit: use “default wallet” or “unnamed wallet”, not “main wallet”

furszy commented at 10:13 pm on January 5, 2026:

In d9e4593 “wallet: fix unnamed legacy wallet migration failure”

nit: use “default wallet” or “unnamed wallet”, not “main wallet”

This is not about the unnamed wallet only. The goal here is to not remove the original wallet directory.

furszy force-pushed on Jan 5, 2026

furszy commented at 9:20 pm on January 5, 2026: member

As mentioned above, added d046da742b3b3a0db9d455250373246e52c4dd2b to further cover the recovery path through restorewallet().

in test/functional/wallet_migration.py:667 in 6f96371ac5 outdated

659@@ -659,6 +660,13 @@ def test_wallet_with_path(self, wallet_path):
660 
661         assert_equal(bals, wallet.getbalances())
662 
663+    def clear_default_wallet(self, backup_file):
664+        # Test cleanup: Clear unnamed default wallet for subsequent tests
665+        (self.old_node.wallets_path / "wallet.dat").unlink()
666+        (self.master_node.wallets_path / "wallet.dat").unlink(missing_ok=True)
667+        shutil.rmtree(self.master_node.wallets_path / "default_wallet_watchonly", ignore_errors=True)

achow101 commented at 9:38 pm on January 5, 2026:

In 1ee2186d6e1f2a38cae3eae16a0565924fe8c309 “wallet: migration, fix watch-only and solvables wallets names”

For completeness, also do default_wallet_solvables.

furszy commented at 11:49 pm on January 5, 2026:

done as suggested

in test/functional/wallet_migration.py:702 in 6f96371ac5 outdated

696+
697+        info = wallet.getwalletinfo()
698+        assert_equal(info["descriptors"], True)
699+        assert_equal(info["format"], "sqlite")
700+        assert_equal(info["private_keys_enabled"], False)
701+        assert_equal(info["walletname"], "default_wallet_watchonly")

achow101 commented at 9:40 pm on January 5, 2026:

In 1ee2186d6e1f2a38cae3eae16a0565924fe8c309 “wallet: migration, fix watch-only and solvables wallets names”

Could also check that the default wallet doesn’t exist anymore.

furszy commented at 11:49 pm on January 5, 2026:

done as suggested

in test/functional/wallet_migration.py:727 in 6f96371ac5

722@@ -697,14 +723,15 @@ def test_default_wallet_failure(self):
723         # Verify the /wallets/ path exists
724         assert self.master_node.wallets_path.exists()
725         # Check backup file exists. Because the wallet has no name, the backup is prefixed with 'default_wallet'
726-        assert (self.master_node.wallets_path / f"default_wallet_{mocked_time}.legacy.bak").exists()
727+        backup_path = self.master_node.wallets_path / f"default_wallet_{mocked_time}.legacy.bak"
728+        assert backup_path.exists()

achow101 commented at 9:41 pm on January 5, 2026:

In 1ee2186d6e1f2a38cae3eae16a0565924fe8c309 “wallet: migration, fix watch-only and solvables wallets names”

Could be squashed into fa0ccf65b7519b4067577072d56e4b9540486d6c “test: add coverage for unnamed wallet migration failure”

furszy commented at 0:16 am on January 6, 2026:

Done as suggested

achow101 commented at 9:58 pm on January 5, 2026: member

By allowing RestoreWallet to properly restore a default wallet, we accidentally introduce the wallet dir deletion bug in RestoreWallet.

If we are going to load a wallet after restoring (which is what normally occurs when restoring a wallet), then there is a fs::remove_all to remove the restored wallet if it cannot be loaded. This is the same problem as in migration for default wallets when the default wallet cannot be loaded after the restore. In that case, we will once again delete the entire wallet directory.

furszy commented at 10:04 pm on January 5, 2026: member

By allowing RestoreWallet to properly restore a default wallet, we accidentally introduce the wallet dir deletion bug in RestoreWallet.

If we are going to load a wallet after restoring (which is what normally occurs when restoring a wallet), then there is a fs::remove_all to remove the restored wallet if it cannot be loaded. This is the same problem as in migration for default walle

Yes. I have been working on that fix the past hour. Already nuked the fs::remove_all() call in RestoreWallet().

in test/functional/wallet_migration.py:734 in 6bbab8ccd1 outdated

697@@ -698,6 +698,10 @@ def test_default_wallet_failure(self):
698         assert self.master_node.wallets_path.exists()
699         # Check backup file exists. Because the wallet has no name, the backup is prefixed with 'default_wallet'
700         assert (self.master_node.wallets_path / f"default_wallet_{mocked_time}.legacy.bak").exists()
701+        # Verify the original unnamed wallet was restored
702+        assert (self.master_node.wallets_path / "wallet.dat").exists()
703+        # And verify it is still a BDB wallet
704+        self.assert_is_bdb("")

achow101 commented at 10:06 pm on January 5, 2026:

In 6bbab8ccd1315765d944fc20c4f0d9c6b57956d9 “wallet: migration, improve restore of unnamed wallet”

These checks pass when I include them in the previous commit fa0ccf65b7519b4067577072d56e4b9540486d6c “test: add coverage for unnamed wallet migration failure”

In fact, I believe this entire commit is unnecessary, except for these test changes. In the previous commit, when I restore a default wallet, there does not appear to be any issues in the restore.

furszy commented at 11:58 pm on January 5, 2026:

Yeah, it became irrelevant after the last rework -> #34156 (comment). Squashed the test changes and dropped commit.

in src/wallet/wallet.cpp:498 in d9e4593afb outdated

495+            // Safety extra check:
496+            // The directory must not contain a database file.
497+            // Any .dat file indicates an existing wallet, and restoring into this
498+            // directory would overwrite it.
499+            for (const auto& entry : fs::directory_iterator(wallet_path)) {
500+                if (entry.path().extension() == ".dat") {

achow101 commented at 10:08 pm on January 5, 2026:

In d9e4593afb386268fbdd5f7fc1f409ea2253a244 “wallet: fix unnamed legacy wallet migration failure”

I think it would be better to check the filenames specifically:

0                if (entry.path().filename() == wallet_file.filename()) {

This also enables this commit to restore a default wallet if the wallets directory already contains other files with the .dat extension.

furszy commented at 10:25 pm on January 5, 2026:

This also enables this commit to restore a default wallet if the wallets directory already contains other files with the .dat extension.

It seems bad to store several .dat files within the same folder. What do you think about leaving it as is for now and discuss it further in a follow-up? If there is someone doing this, it will receive a clear error message on how to fix it with the current code anyway.

achow101 commented at 10:35 pm on January 5, 2026:

My concern is more to do with people who were doing the old wallet file multiwallet (instead of wallet directory multiwallet) and named those wallet files <something>.dat. I believe in those cases, the default wallet would not be able to be restored after migration with the current code.

furszy commented at 0:16 am on January 6, 2026:

Pushed.

furszy commented at 1:39 am on January 6, 2026:

Just for completeness, added coverage for it too.

in test/functional/wallet_backup.py:166 in d046da742b outdated

155+        assert extra_file.exists() # extra file was not removed by mistake
156+
157+        self.log.info("Test restore failure due to a .dat file in the destination directory")
158+        error_message = "Failed to restore wallet. Database file exists in '{}'.".format(wallet_dir)
159+        assert_raises_rpc_error(-36, error_message, node.restorewallet, wallet_name, backup_file)
160+        assert wallet_dir.exists()

achow101 commented at 10:11 pm on January 5, 2026:

In d046da742b3b3a0db9d455250373246e52c4dd2b “test: restorewallet, coverage for existing dirs and unnamed wallets”

Let’s also check that the previous wallet.dat is still there and is still the same file.

furszy commented at 11:59 pm on January 5, 2026:

Done as suggested.

in test/functional/wallet_backup.py:162 in d046da742b

157+        self.log.info("Test restore failure due to a .dat file in the destination directory")
158+        error_message = "Failed to restore wallet. Database file exists in '{}'.".format(wallet_dir)
159+        assert_raises_rpc_error(-36, error_message, node.restorewallet, wallet_name, backup_file)
160+        assert wallet_dir.exists()
161+        # Clean for follow-up tests
162+        os.remove(node.wallets_path / wallet_name / "wallet.dat")

achow101 commented at 10:14 pm on January 5, 2026:

In 67051872c4c0b0f4410a4389e234352d10fa3011 “test: coverage for migration failure when last sync is beyond prune height”

CI failure here. The wallet needs to be unloaded before we delete it.

furszy commented at 11:59 pm on January 5, 2026:

Fixed.

DrahtBot added the label CI failed on Jan 5, 2026

achow101 commented at 10:55 pm on January 5, 2026: member

A couple additional things I noticed that we should address, perhaps not in this PR though:

If the wallet being migrated is a file and not a directory, and it fails to migrate, we end up restoring that wallet into a wallet directory rather than a file. This is probably surprising to users, but also not that big of a deal IMO.
When the spendable wallet (i.e. main wallet) has no records after migration, we still leave behind a directory in the wallets directory that has no wallet.dat in it.

furszy force-pushed on Jan 5, 2026

furszy commented at 0:05 am on January 6, 2026: member

@achow101 another point is that we are not erasing all BDB-created files. The /database/ directory, the .walletlock file, and the db.log file are still present after migration. Of course, this isn’t urgent, it was a minor headache to have them early on in this PR and in #34193, but I ended up working around them. We could re-add those changes into #34193 later on.

furszy force-pushed on Jan 6, 2026

in test/functional/wallet_backup.py:156 in 69904d4ac5 outdated

151+        self.log.info("Test restore succeeds when the target directory contains non-wallet files")
152+        wallet_file = node.wallets_path / wallet_name / "wallet.dat"
153+        os.remove(wallet_file)
154+        extra_file = node.wallets_path / wallet_name / "not_a_wallet.txt"
155+        extra_file.touch()
156+        res = node.restorewallet(wallet_name, backup_file)

achow101 commented at 1:22 am on January 6, 2026:

In 69904d4ac52495127c33f0349c4fa3d3c4bef129 “test: restorewallet, coverage for existing dirs, unnamed wallet and prune failure”

This still needs to be unloaded before the wallet file is deleted at the end of the test.

furszy commented at 1:36 am on January 6, 2026:

upps, too tired.. thanks. Pushed.

furszy force-pushed on Jan 6, 2026

achow101 commented at 1:44 am on January 6, 2026: member

ACK d4eca7c67ada6142c6937780b26254fc3a61919e

DrahtBot requested review from willcl-ark on Jan 6, 2026

DrahtBot requested review from rkrux on Jan 6, 2026

DrahtBot removed the label CI failed on Jan 6, 2026

in src/wallet/wallet.cpp:4436 in d4eca7c67a outdated

4433-        for (const fs::path& dir : wallet_dirs) {
4434-            fs::remove_all(dir);
4435+        // First, delete the db files we have created throughout this process and nothing else
4436+        for (const fs::path& file : wallet_files_to_remove) {
4437+            fs::remove(file);
4438+        }

w0xlt commented at 8:29 am on January 6, 2026:

nit:

0        for (const fs::path& file : wallet_files_to_remove) {
1            std::error_code ec;
2            if (!fs::remove(file, ec) && ec) {
3                LogWarning("Failed to remove migration artifact '%s': %s\n",
4                            fs::PathToString(file), ec.message());
5            }
6        }

furszy commented at 3:25 pm on January 6, 2026:

Logging without failure isn’t enough. The process would still fail later during restore due to the existing .dat file, returning a confusing error to the user.

That said, we could do better when handling filesystem errors. This is probably follow-up material, as this PR doesn’t materially change that behavior.

in test/functional/wallet_backup.py:219 in d4eca7c67a

210@@ -155,6 +211,13 @@ def test_pruned_wallet_backup(self):
211         # the backup to load successfully this close to the prune height
212         node.restorewallet('pruned', node.datadir_path / 'wallet_pruned.bak')
213 
214+        self.log.info("Test restore on a pruned node when the backup was beyond the pruning point")
215+        backup_file = self.nodes[0].datadir_path / 'wallet.bak'
216+        wallet_name = ""
217+        error_message = "Wallet loading failed. Prune: last wallet synchronisation goes beyond pruned data. You need to -reindex (download the whole blockchain again in case of a pruned node)"
218+        assert_raises_rpc_error(-4, error_message, node.restorewallet, wallet_name, backup_file)
219+        assert(node.wallets_path.exists()) # ensure the wallets dir exists

w0xlt commented at 8:30 am on January 6, 2026:

nit:

0        assert node.wallets_path.exists() # ensure the wallets dir exists

furszy commented at 3:25 pm on January 6, 2026:

will do if have to re-touch.

furszy commented at 7:41 pm on January 6, 2026:

Done as suggested

in src/wallet/wallet.cpp:4369 in d4eca7c67a

4366                 std::shared_ptr<CWallet>& wallet = *wallet_ptr;
4367-                // Save db path and load wallet
4368-                wallet_dirs.insert(fs::PathFromString(wallet->GetDatabase().Filename()).parent_path());
4369+
4370+                // Keep track of created database files to allow cleanup if the procedure fails.
4371+                const auto files = wallet->GetDatabase().Files();

w0xlt commented at 8:38 am on January 6, 2026:

nit: This logic can be de-duplicated.

 0diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
 1index 79e738fc28..9b3d343ea2 100644
 2--- a/src/wallet/wallet.cpp
 3+++ b/src/wallet/wallet.cpp
 4@@ -4349,6 +4349,18 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
 5     // fails to load.
 6     std::set<fs::path> wallet_files_to_remove;
 7     std::set<fs::path> wallet_empty_dirs_to_remove;
 8+
 9+    // Helper to track wallet files and directories for cleanup on failure.
10+    // Only directories of wallets created during migration (not the main wallet) are tracked.
11+    auto track_wallet_for_cleanup = [&](const CWallet& wallet) {
12+        const auto files = wallet.GetDatabase().Files();
13+        wallet_files_to_remove.insert(files.begin(), files.end());
14+        if (wallet.GetName() != wallet_name) {
15+            wallet_empty_dirs_to_remove.insert(fs::PathFromString(wallet.GetDatabase().Filename()).parent_path());
16+        }
17+    };
18+
19+
20     if (success) {
21         Assume(!res.wallet); // We will set it here.
22         // Check if the local wallet is empty after migration
23@@ -4365,14 +4377,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
24             if (success && *wallet_ptr) {
25                 std::shared_ptr<CWallet>& wallet = *wallet_ptr;
26 
27-                // Keep track of created database files to allow cleanup if the procedure fails.
28-                const auto files = wallet->GetDatabase().Files();
29-                wallet_files_to_remove.insert(files.begin(), files.end());
30-                if (wallet->GetName() != wallet_name) {
31-                    // Only when this is not the main wallet, it is fully safe to delete the parent path
32-                    // because we created the wallet during migration
33-                    wallet_empty_dirs_to_remove.insert(fs::PathFromString(wallet->GetDatabase().Filename()).parent_path());
34-                }
35+                track_wallet_for_cleanup(*wallet);
36 
37                 // Load wallet
38                 assert(wallet.use_count() == 1);
39@@ -4404,13 +4409,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
40 
41         // Get the directories to remove after unloading
42         for (std::shared_ptr<CWallet>& wallet : created_wallets) {
43-            const auto files = wallet->GetDatabase().Files();
44-            wallet_files_to_remove.insert(files.begin(), files.end());
45-            if (wallet->GetName() != wallet_name) {
46-                // Only when this is not the main wallet, it is fully safe to delete the parent path
47-                // because we created the wallet during migration
48-                wallet_empty_dirs_to_remove.insert(fs::PathFromString(wallet->GetDatabase().Filename()).parent_path());
49-            }
50+            track_wallet_for_cleanup(*wallet);
51         }
52 
53         // Unload the wallets

furszy commented at 3:26 pm on January 6, 2026:

I thought about it a few times and never really introduced it. But now that the code is mature enough, will do if have to re-touch it.

furszy commented at 7:42 pm on January 6, 2026:

Done as suggested

in src/wallet/wallet.cpp:4359 in d4eca7c67a

4358@@ -4320,20 +4359,36 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
4359             for (const auto& path_to_remove : paths_to_remove) fs::remove_all(path_to_remove);

w0xlt commented at 8:39 am on January 6, 2026:

nit:

0            for (const auto& path_to_remove : paths_to_remove) fs::remove(path_to_remove);

furszy commented at 3:28 pm on January 6, 2026:

will do if have to re-touch. The returned elements are all files, so there is no risk calling remove_all here.

in src/wallet/wallet.cpp:502 in d4eca7c67a outdated

501+                return nullptr;
502+            }
503+        } else {
504+            // The directory doesn't exist, create it
505+            if (!TryCreateDirectories(wallet_path)) {
506+                error = Untranslated(strprintf("Failed to restore database path '%s'.", fs::PathToString(wallet_path)));

w0xlt commented at 8:43 am on January 6, 2026:

nit:

0                error = Untranslated(strprintf("Failed to restore wallet. Could not create database path '%s'.", fs::PathToString(wallet_path)));

furszy commented at 3:29 pm on January 6, 2026:

will do if have to re-touch. Otherwise it is material for a small follow-up.

in test/functional/wallet_migration.py:717 in d4eca7c67a outdated

712+        wallet = self.create_legacy_wallet("", blank=True)
713+        wallet.importaddress(master_wallet.getnewaddress(address_type="legacy"))
714+
715+        # Create wallet directory with the watch-only name and a wallet file.
716+        # Because the wallet dir exists, this will cause migration to fail.
717+        watch_only_dir = self.master_node.wallets_path / "default_wallet_watchonly"

w0xlt commented at 8:46 am on January 6, 2026:

nit: Commit 3 (https://github.com/bitcoin/bitcoin/pull/34156/commits/7d817d8b675e6abb5ab37ccbc75fed094bc34e4e) could use the correct path from the start.

furszy commented at 2:37 pm on January 6, 2026:

The default prefix? the fix for the naming issue comes later, in c8d7c2e48af9061d8d37860f56aba5dbc4465f99.

w0xlt commented at 8:47 am on January 6, 2026: contributor

ACK https://github.com/bitcoin/bitcoin/pull/34156/commits/d4eca7c67ada6142c6937780b26254fc3a61919e

ryanofsky commented at 3:13 pm on January 6, 2026: contributor

Can PR description be updated to mention which PR or commit introduced the bug, how long it has been present?

furszy commented at 3:45 pm on January 6, 2026: member

Can PR description be updated to mention which PR or commit introduced the bug, how long it has been present?

Done. Let me know if anything else is missing.

in src/wallet/wallet.cpp:4363 in 239f786412

4360+                // Keep track of created database files to allow cleanup if the procedure fails.
4361+                const auto files = wallet->GetDatabase().Files();
4362+                wallet_files_to_remove.insert(files.begin(), files.end());
4363+                if (wallet->GetName() != wallet_name) {
4364+                    // Only when this is not the main wallet, it is fully safe to delete the parent path
4365+                    // because we created the wallet during migration

ryanofsky commented at 3:59 pm on January 6, 2026:

In commit “wallet: fix unnamed legacy wallet migration failure” (239f78641256bcce6ca5db3e34c49e50af9df5ca)

I think this comment is misleading because unless the wallet is a bare BDB .dat file located in the top level directory, it is safe to remove the parent path. Normally, when the wallet is not a top-level file, not removing the parent path is just an implementation choice, not a safety issue.

Would suggest rephrasing to “Remove the empty wallet directory if this is a watchonly or solvables wallet, not the main wallet. Otherwise, keep the directory to hold the restored backup. (Also, in the case when the wallet is a top-level file not in its own directory, it is not possible to remove this directory anyway.)”

furszy commented at 7:42 pm on January 6, 2026:

Sure. Done as suggested.

in src/wallet/wallet.cpp:494 in 239f786412 outdated

492+                status = DatabaseStatus::FAILED_ALREADY_EXISTS;
493+                return nullptr;
494+            }
495+
496+            // Check we are not going to overwrite an existing db file
497+            if (fs::exists(wallet_file)) {

ryanofsky commented at 4:29 pm on January 6, 2026:

In commit “wallet: fix unnamed legacy wallet migration failure” (239f78641256bcce6ca5db3e34c49e50af9df5ca)

Would recommend being more conservative and failing if the restore directory contains any file, not just the wallet file. This would also simplify the code significantly, replacing lines 485-507 with just:

0created_parent_dir = TryCreateDirectories(wallet_path);
1if (!fs::is_empty(wallet_path)) {
2    error = Untranslated(strprintf("Failed to restore wallet. Database already exists '%s'.", fs::PathToString(wallet_path)));
3    status = DatabaseStatus::FAILED_ALREADY_EXISTS;
4    return nullptr;
5}

furszy commented at 6:15 pm on January 6, 2026:

The restore directory is always the original wallet directory. And it can contain other files:

if it is the unnamed wallet, we are talking about the top-level dir /wallets/, which can contain other wallets, either as .dat files, directories or also what I will mention next.
If it is a named/unnamed wallet, the directory will have the BDB files we currently don’t remove. See #34156 (comment).

Also, just as a note: before the last push, we had a “no other .dat files are allowed inside the restore directory” check here, but achow convinced me that this restriction would prevent migration for some wallets so I ended adding coverage for it as well (see #34156 (review)).

ryanofsky commented at 6:21 pm on January 6, 2026:

The restore directory is always the original wallet directory. And it can contain other files:

Oh, right. I guess this seems far from ideal but I see how you arrived here.

darosior commented at 3:03 pm on January 7, 2026:

if it is the unnamed wallet, we are talking about the top-level dir /wallets/

Just mentioning in passing that for old installations with no /wallets/ sub-directory, this may even be the datadir itself.

in src/wallet/wallet.cpp:523 in 5ed59557b9 outdated

507@@ -497,7 +508,13 @@ std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& b
508 
509     // Remove created wallet path only when loading fails
510     if (load_after_restore && !wallet) {
511-        fs::remove_all(wallet_path);
512+        if (wallet_file_copied) fs::remove(wallet_file);

ryanofsky commented at 4:35 pm on January 6, 2026:

In commit “wallet: RestoreWallet failure, erase only what was created” (5ed59557b91f85d2c36d911bd2a334cb80b7494c)

Can purpose of this commit be clarified in the commit message? It seems good to replace remove_all with remove here, but is this a fix for a known bug, or just a precautionary change?

furszy commented at 6:21 pm on January 6, 2026:

Can purpose of this commit be clarified in the commit message? It seems good to replace remove_all with remove here, but is this a fix for a known bug, or just a precautionary change?

Without this commit, if RestoreWallet fails during load after the next commit, we would end up erasing the top-level /wallets/ directory (again). In other words, the purpose of this commit is to delete only what we have created and nothing else, just so the next commit fixing the original deletion issue does not reintroduce the same problem elsewhere.

This issue is covered by the prune node test case in 5aff04584e9df9183708be2c201cd564b5e403c8.

ryanofsky commented at 6:50 pm on January 6, 2026:

re: #34156 (review)

Thanks for the explanation. I’d suggest stating the purpose of this change in the commit message to make this clear. Would suggest something like “Using fs::remove_all() instead of fs::remove() in RestoreWallet does not cause any problems currently, but is necessary for the next commit which extends RestoreWallet to work with existing directories, which may contain files that should not be deleted.”

furszy commented at 7:42 pm on January 6, 2026:

Sure. Done as suggested.

in src/wallet/wallet.cpp:515 in 5ed59557b9

507@@ -497,7 +508,13 @@ std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& b
508 
509     // Remove created wallet path only when loading fails
510     if (load_after_restore && !wallet) {
511-        fs::remove_all(wallet_path);
512+        if (wallet_file_copied) fs::remove(wallet_file);
513+        // Clean up the parent directory if we created it during restoration.
514+        // As we have created it, it must be empty after deleting the wallet file.
515+        if (created_parent_dir) {
516+            Assert(fs::is_empty(wallet_path));

ryanofsky commented at 4:46 pm on January 6, 2026:

In commit “wallet: RestoreWallet failure, erase only what was created” (5ed59557b91f85d2c36d911bd2a334cb80b7494c)

Aborting the node doesn’t seem like the best way to handle this error. Letting fs::remove just throw an exception on failure here like it would other places seems sufficient.

If the goal is to detect fs::remove failing for this specific reason as opposed to other reasons, since this one might indicate a bug, then Assume would be appropriate here.

(Same comment also applies to next commit 239f78641256bcce6ca5db3e34c49e50af9df5ca)

furszy commented at 7:43 pm on January 6, 2026:

Sure. Done as suggested in both places.

in src/wallet/wallet.cpp:485 in 5ed59557b9 outdated

479@@ -478,13 +480,22 @@ std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& b
480             return nullptr;
481         }
482 
483-        if (fs::exists(wallet_path) || !TryCreateDirectories(wallet_path)) {
484+        if (fs::exists(wallet_path)) {

ryanofsky commented at 4:56 pm on January 6, 2026:

In commit “wallet: RestoreWallet failure, erase only what was created” (5ed59557b91f85d2c36d911bd2a334cb80b7494c)

This fs::exists call is redundant and could be dropped. TryCreateDirectories already returns true if the directory was created, false if it existed, and it throws an exception on other errors.

So this could simply be:

0created_parent_dir = TryCreateDirectories(wallet_path);
1if (!created_parent_dir) {
2    error = Untranslated(strprintf("Failed to create database path '%s'. Database already exists.", fs::PathToString(wallet_path)));
3    status = DatabaseStatus::FAILED_ALREADY_EXISTS;
4}

furszy commented at 6:53 pm on January 6, 2026:

This fs::exists call is redundant and could be dropped. TryCreateDirectories already returns true if the directory was created, false if it existed, and it throws an exception on other errors.

TryCreateDirectories throws an exception if the provided path is from a file and we should still support files on restorewallet(). E.g. something like the following would throw an exception using that:

0backup_file = whatever.
1wallet_file = node.wallets_path / wallet_name / "wallet.bak"
2node.restorewallet(wallet_file, backup_file)

in src/wallet/wallet.cpp:4441 in 239f786412 outdated

4422+        }
4423+
4424+        // Second, delete the created wallet directories and nothing else. They must be empty at this point.
4425+        for (const fs::path& dir : wallet_empty_dirs_to_remove) {
4426+            Assert(fs::is_empty(dir));
4427+            fs::remove(dir);

ryanofsky commented at 5:25 pm on January 6, 2026:

In commit “wallet: fix unnamed legacy wallet migration failure” (239f78641256bcce6ca5db3e34c49e50af9df5ca)

I think commit message could be improved. It doesn’t look like this bug depends on on the wallet being named or unnamed. If the wallet name is "wallet.dat" or is the name of any top-level database file not inside a dedicated wallet directory, it looks like this bug would still happen. So would recommend replacing “unnamed legacy wallet” with “bare wallet file” or “top-level wallet file”. (For reference the change which started creating wallets as directories rather than bare files was be8ab7d082228d09ca529d1a08730d7d5aacb0ed).

Also, IMO it would be helpful to mention the commit which introduced the directory deletion bug: f6ee59b6e2995a3916fb4f0d4cbe15ece2054494, since if you just look at the remove code, you might think the bug has been around since that code was written, but actually it started more recently after f6ee59b6e2995a3916fb4f0d4cbe15ece2054494 removed a fs::copy_file call that previously failed in this case and prevented the fs::remove_all call from being reached.

achow101 commented at 7:10 pm on January 6, 2026:

is the name of any top-level database file not inside a dedicated wallet directory

That should not be the case. Such files are migrated into dedicated wallet directories which avoids the problem during cleanup. It’s only the unnamed wallet where we don’t do that.

furszy commented at 7:44 pm on January 6, 2026:

Just expanded the description to reference the commit that “started” the issue and explain the reason.

ryanofsky commented at 8:17 pm on January 6, 2026:

re: #34156 (review)

is the name of any top-level database file not inside a dedicated wallet directory

That should not be the case. Such files are migrated into dedicated wallet directories which avoids the problem during cleanup. It’s only the unnamed wallet where we don’t do that.

Oh, that’s interesting. So it seems like a simpler fix for this issue could just be to adjust the local_wallet name to be nonempty after loading it like:

0--- a/src/wallet/wallet.cpp
1+++ b/src/wallet/wallet.cpp
2@@ -4217,6 +4217,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
3 
4     // Make the local wallet
5     std::shared_ptr<CWallet> local_wallet = CWallet::Create(empty_context, wallet_name, std::move(database), options.create_flags, error, warnings);
6+    if (local_wallet->m_name.empty()) local_wallet->m_name = fs::PathToString(fs::PathFromString(local_wallet->GetDatabase().Filename()).filename());
7     if (!local_wallet) {
8         return util::Error{Untranslated("Wallet loading failed.") + Untranslated(" ") + error};
9     }

Replacing the empty name with “wallet.dat”, so after that point migration code would not need to deal with empty wallet names, and MigrationPrefixName “default_wallet” stuff would be unnecessary and support for restoring into nonempty subdirectories would be unnecessary.

Not sure if this would actually work. It just seems like all of this migration code is more complicated than I would expect it to be. Like I don’t even understand at basic level why backup and restore during migration is needed at all. Naively, I would expect migration code to just rename (not copy) the original wallet to back it up, and rename it back if migration fails.

furszy commented at 8:30 pm on January 6, 2026:

Like I don’t even understand at basic level why backup and restore during migration is needed at all. Naively, I would expect migration code to just rename (not copy) the original wallet to back it up, and rename it back if migration fails.

My early comment is related to this (see #34156 (comment)). But in short; I have an old branch where this process was restructured to behave the way you describe.

Replacing the empty name with “wallet.dat”, so after that point migration code would not need to deal with empty wallet names, and MigrationPrefixName “default_wallet” stuff would be unnecessary and support for restoring into nonempty subdirectories would be unnecessary.

I wouldn’t go down that road here; IMO the current changes are much safer. Having no more fs::remove_all() calls and only removing the files we created is the best we can do at this point.

achow101 commented at 8:32 pm on January 6, 2026:

Oh, that’s interesting. So it seems like a simpler fix for this issue could just be to adjust the local_wallet name to be nonempty after loading it like:

A reasonable solution could be do migrate the unnamed wallet into a named wallet. However this has some concerns:

This possibly breaks backups as many users have scripts that produce backups by copying the wallet.dat file, so moving that away can unexpectedly break those backups
Could cause future loading errors/confusion if wallet= is in the bitcoin.conf
What do we even name the migrated wallet, and what happens if a wallet of that name already exists?

But this is also expanding the scope of this PR into possibly confusing behavior to the user. I think such a change would be better suited for a followup.

ryanofsky commented at 8:45 pm on January 6, 2026:

re: #34156 (review)

I wouldn’t go down that road; IMO the current changes are much safer. Having no more fs::remove_all() calls and only removing the files we created is the best we can do at this point.

I do want to get rid of all remove_all calls. I’ve been complaining about remove_all calls for as long as they have existed in the codebase [1] [2] [3] [4].

It would just be nice if there is a small fix for this bug which also simplifies migration code and removes need for name.empty() special cases everywhere, and then the remove_all calls could be dealt with separately.

A reasonable solution could be do migrate the unnamed wallet into a named wallet. However this has some concerns:

I think all these problems would be avoided by using the name wallet.dat for the default wallet as suggested.

I’m ok with current approach and plan to review more. It just seems like maybe there are some opportunities for simplification here.

Would also still be curious to know if backup/restore is even necessary at all, or if the “rename (not copy) the original wallet to back it up, and rename it back if migration fails” approach would work.

achow101 commented at 8:59 pm on January 6, 2026:

Would also still be curious to know if backup/restore is even necessary at all, or if the “rename (not copy) the original wallet to back it up, and rename it back if migration fails” approach would work.

It is probably viable now, but was not viable when this was first implemented. When migration still required BDB to open the wallet file, it is potentially dangerous to copy just the wallet.dat file as data can be stored in the BDB log files. The use of backup was required to ensure that all data from the log files made it into the wallet.dat file. Since migration switched to the BDB parser implementation, we are assuming that the wallet.dat file has all of the data so the backup is just doing a copy of the file directly.

But we still need to do remove() in the cleanup since sqlite will still generate additional files that need to be removed.

However I think actually implementing this is more complicated than it seems on the surface. For example, “rename it back” doesn’t work for wallet files directly in the wallets dir as migration will create a directory of the same name rather than continuing with the file in the wallets dir.

furszy commented at 9:19 pm on January 6, 2026:

It would just be nice if there is a small fix for this bug which also simplifies migration code and removes need for name.empty() special cases everywhere, and then the remove_all calls could be dealt with separately.

I understand your point. It just feels like we’re entering the unknown after what happened if we only change the wallet name and leave the remove_all calls. I think we should take the safest approach and get rid of them. The PR shouldn’t involve too many more lines; +80% of it is about tests.

furszy commented at 2:02 am on January 7, 2026:

re: #34156 (comment)

is the name of any top-level database file not inside a dedicated wallet directory

That should not be the case. Such files are migrated into dedicated wallet directories which avoids the problem during cleanup. It’s only the unnamed wallet where we don’t do that.

Oh, that’s interesting. So it seems like a simpler fix for this issue could just be to adjust the local_wallet name to be nonempty after loading it like:
0--- a/src/wallet/wallet.cpp
1+++ b/src/wallet/wallet.cpp
2@@ -4217,6 +4217,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
3 
4     // Make the local wallet
5     std::shared_ptr<CWallet> local_wallet = CWallet::Create(empty_context, wallet_name, std::move(database), options.create_flags, error, warnings);
6+    if (local_wallet->m_name.empty()) local_wallet->m_name = fs::PathToString(fs::PathFromString(local_wallet->GetDatabase().Filename()).filename());
7     if (!local_wallet) {
8         return util::Error{Untranslated("Wallet loading failed.") + Untranslated(" ") + error};
9     }
Replacing the empty name with “wallet.dat”, so after that point migration code would not need to deal with empty wallet names, and MigrationPrefixName “default_wallet” stuff would be unnecessary and support for restoring into nonempty subdirectories would be unnecessary.

Not sure if this would actually work.

Focusing on your suggestion alone, ~~I’m quite confident~~ your suggestion might not work. Changing the wallet name would only address the naming issue (82caa8193a3e36f248dcc949e0cd41def191efac), not the top-level /wallets/ deletion issue, which is tied to the database path.

I understand where your idea is coming from; I also thought about creating a new wallet with a proper name and database in its own folder, migrating everything there and then renaming it at the end. That’s basically https://github.com/furszy/bitcoin-core/tree/2023_wallet_rewrite_migration (specifically https://github.com/furszy/bitcoin-core/commit/e9c22da457ccec67a99359d595a7469a926b18b7), which migrates the wallet without modifying the original wallet at all. But such a change is not small.

ryanofsky commented at 2:12 am on January 7, 2026:

Changing the wallet name would only address the naming issue (82caa81), not the top-level /wallets/ deletion issue, which is tied to the database path.

It would fix the wallets deletion issue because of:

https://github.com/bitcoin/bitcoin/blob/a9daa6dbd3aeb846796b3374ca3130bbabb9f42c/src/wallet/wallet.cpp#L3800-L3801

If m_name is not empty, the migrated wallet would be created as a directory, not a file, and there would be no problem deleting the migrated wallet directory. At least this is the case if I understand achow’s comment #34156 (review) which this suggestion was a reply to.

furszy commented at 2:41 am on January 7, 2026:

ah, that’s much clearer now. Thanks. While it is an interesting idea, at least at this point, I’m still more inclined to getting rid of the remove_all calls. Keeping them presents an unnecessary risk that in my view we shouldn’t take.

in src/wallet/wallet.cpp:4293 in c8d7c2e48a outdated

4289@@ -4281,7 +4290,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
4290     // cases, but in the case where the wallet name is a path to a data file,
4291     // the name of the data file is used, and in the case where the wallet name
4292     // is blank, "default_wallet" is used.
4293-    const std::string backup_prefix = wallet_name.empty() ? "default_wallet" : [&] {
4294+    const std::string backup_prefix = wallet_name.empty() ? MigrationPrefixName(*local_wallet) : [&] {

ryanofsky commented at 5:32 pm on January 6, 2026:

In commit “wallet: migration, fix watch-only and solvables wallets names” (c8d7c2e48af9061d8d37860f56aba5dbc4465f99)

Seems like this should be simplified to:

0const std::string backup_prefix{MigrationPrefixName(*local_wallet)};

furszy commented at 7:14 pm on January 6, 2026:

Remove the relative path part? I’m pretty sure that such change would fail for the relative path migration tests.

ryanofsky commented at 7:49 pm on January 6, 2026:

re: #34156 (review)

Remove the relative path part? I’m pretty sure that such change would fail for the relative path migration tests.

Oops, sorry this was a bad suggestion. I just misread the code.

furszy commented at 7:49 pm on January 6, 2026:

np, resolved.

ryanofsky approved

ryanofsky commented at 5:44 pm on January 6, 2026: contributor

Code review d4eca7c67ada6142c6937780b26254fc3a61919e. I think this is all good, but so far I only looked at the code, not the tests, and want to look more closely and make sure I understand things before adding an ack. I left a few suggestions below, mostly about improving comments and simplifying code.

wallet: RestoreWallet failure, erase only what was created

Track what RestoreWallet creates so only those files and directories
are removed during a failure and nothing else. Preexisting paths
must be left untouched.

Note:
Using fs::remove_all() instead of fs::remove() in RestoreWallet does
not cause any problems currently, but the change is necessary for the
next commit which extends RestoreWallet to work with existing directories,
which may contain files that must not be deleted.

4ed0693a3f

wallet: fix unnamed wallet migration failure

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

To be fully safe, we will no longer call `fs::remove_all`. Instead,
we only erase the individual db files we have created, leaving
everything else intact. The created wallets parent directories are
erased only if they are empty.
As part of this last change, `RestoreWallet` was modified to allow
an existing directory as the destination, since we no longer remove
the original wallet directory (we only remove the files we created
inside it). This also fixes the restore of top-level default wallets
during failures, which were failing due to the directory existence
check that always returns true for the /wallets/ directory.

This bug started after:
https://github.com/bitcoin/bitcoin/commit/f6ee59b6e2995a3916fb4f0d4cbe15ece2054494
Previously, the `fs::copy_file` call was failing for top-level wallets,
which prevented the `fs::remove_all` call from being reached.

f4c7e28e80

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.

36093bde63

test: restorewallet, coverage for existing dirs, unnamed wallet and prune failure

The first test verifies that restoring into an existing empty directory
or a directory with no .dat db files succeeds, while restoring into a
dir with a .dat file fails.

The second test covers restoring into the default unnamed wallet
(wallet.dat), which also implicitly exercises the recovery path used
after a failed migration.

The third test covers failure during restore on a prune node. When
the wallet last sync was beyond the pruning height.

f011e0f068

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.

d70b159c42

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"

82caa8193a

test: coverage for migration failure when last sync is beyond prune height b7c34d08dd

furszy force-pushed on Jan 6, 2026

furszy commented at 7:47 pm on January 6, 2026: member

Updated per feedback. Thanks @ryanofsky and @w0xlt! Commits messages were expanded as suggested and also took most of the code nits.

achow101 commented at 9:15 pm on January 6, 2026: member

ACK b7c34d08dd9549a95cffc6ec1ffa4bb4f81e35eb

DrahtBot requested review from w0xlt on Jan 6, 2026

in src/wallet/wallet.cpp:487 in f4c7e28e80

485         if (fs::exists(wallet_path)) {
486-            error = Untranslated(strprintf("Failed to create database path '%s'. Database already exists.", fs::PathToString(wallet_path)));
487-            status = DatabaseStatus::FAILED_ALREADY_EXISTS;
488-            return nullptr;
489+            // If this is a file, it is the db and we don't want to overwrite it.
490+            if (!fs::is_directory(wallet_path)) {

davidgumberg commented at 10:13 pm on January 6, 2026:

In f4c7e28e80bf9af50b03a770b641fd309a801589 (wallet: fix unnamed wallet migration failure)

Just remarking: this case will fail anyways during the copy since wallet_file will invalidly refer to a subtree of a file e.g. wallet.dat/wallet.dat

0$ bcli restorewallet "existing.dat" /tmp/scratch/regtest/wallets/default_wallet_watchonly/wallet.dat
1error code: -4
2error message:
3Unexpected exception: filesystem error: cannot copy file: Not a directory [/tmp/scratch/regtest/wallets/default_wallet_watchonly/wallet.dat] [/tmp/scratch/regtest/wallets/existing.dat/wallet.dat]

But this message is more helpful.

A test may be nice, but not blocking.

in test/functional/wallet_backup.py:168 in f011e0f068

163+        error_message = "Failed to restore wallet. Database file exists in '{}'.".format(wallet_dir / "wallet.dat")
164+        assert_raises_rpc_error(-36, error_message, node.restorewallet, wallet_name, backup_file)
165+        # Ensure the wallet file remains untouched
166+        assert wallet_dir.exists()
167+        assert_equal(original_shasum, sha256sum_file(wallet_file))
168+

davidgumberg commented at 10:26 pm on January 6, 2026:

non-blocking-suggestion (cont from #34156 (review))

0
1        self.log.info("Test restore failure due to existing db file is the destination directory")
2        original_shasum = sha256sum_file(wallet_file)
3        error_message = "Failed to restore wallet. Database file exists '{}'.".format(wallet_file)
4        assert_raises_rpc_error(-36, error_message, node.restorewallet, f"{wallet_name}/wallet.dat", backup_file)
5        # Ensure the wallet file remains untouched
6        assert wallet_dir.exists()
7        assert_equal(original_shasum, sha256sum_file(wallet_file))

w0xlt commented at 10:43 pm on January 6, 2026: contributor

ACK https://github.com/bitcoin/bitcoin/commit/b7c34d08dd9549a95cffc6ec1ffa4bb4f81e35eb

davidgumberg commented at 11:32 pm on January 6, 2026: contributor

crACK https://github.com/bitcoin/bitcoin/commit/b7c34d08dd9549a95cffc6ec1ffa4bb4f81e35eb

Did some trivial manual testing that resembles the functional tests added here, and I could produce the issue in master, 30.0, but not in 29.0 or in this branch. I believe this fixes #31428.

Switching all remove_all calls to instead just remove() empty directories (which fails to delete directories if they’re not empty) and specific wallet.dat files corresponding with CWallet objects we’ve created related to migration makes me much more confident that there isn’t a way here for user data to get deleted, but I strongly agree with @willcl-ark’s suggestion above that we should never delete any migration data, or IMO wallet data generally, even if we are pretty confident that the data is not useful or interesting to the user, but to be clear, I think this PR works as a minimal fix.

This doesn’t rise to the same level of criticality, but grepping for remove_all reveals that the wallet tool has a similar issue where it will delete the wallet directory on failure to load from a dump:

 0# !!!!CAUTION!!! DON'T RUN THIS ON A MACHINE WITH ACTUAL WALLETS, IF THE DATADIR IS NOT SET CORRECTLY, THIS MAY DELETE YOUR WALLET AND DATADIR
 1mkdir -p /tmp/tool/wallets
 2# create a wallet
 3./build/bin/bitcoin-wallet -datadir=/tmp/tool -wallet="example" create
 4# dump the wallet
 5./build/bin/bitcoin-wallet -datadir=/tmp/tool -wallet="example" -dumpfile="test.dump" dump
 6# malform the dump checksum by appending an `a`
 7sed -i '$ s/$/a/' test.dump
 8# create an unnamed wallet from a dump
 9./build/bin/bitcoin-wallet -datadir=/tmp/tool -dumpfile="test.dump" createfromdump
10ls /tmp/tool
11# wallets dir is gone

achow101 commented at 0:16 am on January 7, 2026: member

This doesn’t rise to the same level of criticality, but grepping for remove_all reveals that the wallet tool has a similar issue where it will delete the wallet directory on failure to load from a dump:

Oof yeah. Opened #34215 to fix that.

bitcoin deleted a comment on Jan 7, 2026

in src/wallet/wallet.cpp:529 in b7c34d08dd

525+        // Clean up the parent directory if we created it during restoration.
526+        // As we have created it, it must be empty after deleting the wallet file.
527+        if (created_parent_dir) {
528+            Assume(fs::is_empty(wallet_path));
529+            fs::remove(wallet_path);
530+        }

willcl-ark commented at 10:27 am on January 7, 2026:

I am much more re-assured to see that not only do we now track what we’ve created, but we now also use fs::remove() which will fail on non-empty dirs (along with an extra Assume for good measure). This seems like a good fix for the immediate issue.

willcl-ark approved

willcl-ark commented at 10:46 am on January 7, 2026: member

ACK b7c34d08dd9549a95cffc6ec1ffa4bb4f81e35eb

Happy with the fix here for the immediate issue. I think we can do even better longer-term and #34193 has a few ideas I’d like to see make it in, such as resolving the race between bdb and sqlite database files (delete->creation).

Also verified that this is not present on 29.x by backporting the tests, where we see unnamed wallet migration fail at the file copy, and so exit without the problematic removal:

 02026-01-07T10:31:08.168000Z TestFramework (INFO): Test failure during unnamed (default) wallet migration
 12026-01-07T10:31:08.437000Z TestFramework (ERROR): Unexpected exception
 2Traceback (most recent call last):
 3  File "/home/will/src/core/backports/test/functional/test_framework/util.py", line 160, in try_rpc
 4    fun(*args, **kwds)
 5    ~~~^^^^^^^^^^^^^^^
 6  File "/home/will/src/core/backports/./build/test/functional/wallet_migration.py", line 117, in migrate_and_get_rpc
 7    migrate_info = self.master_node.migratewallet(wallet_name=wallet_name, **kwargs)
 8  File "/home/will/src/core/backports/test/functional/test_framework/coverage.py", line 50, in __call__
 9    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
10  File "/home/will/src/core/backports/test/functional/test_framework/authproxy.py", line 151, in __call__
11    raise JSONRPCException(response['error'], status)
12test_framework.authproxy.JSONRPCException: filesystem error: cannot copy file: File exists [/tmp/bitcoin_func_test_yg5hrc0g/node0/regtest/wallets/default_wallet_1767781868.legacy.bak] [/tmp/bitcoin_func_test_yg5hrc0g/node0/regtest/wallets/default_wallet_1767781868.legacy.bak] (-1)
13
14During handling of the above exception, another exception occurred:
15
16Traceback (most recent call last):
17  File "/home/will/src/core/backports/test/functional/test_framework/test_framework.py", line 135, in main
18    self.run_test()
19    ~~~~~~~~~~~~~^^
20  File "/home/will/src/core/backports/./build/test/functional/wallet_migration.py", line 1408, in run_test
21    self.test_default_wallet_failure()
22    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
23  File "/home/will/src/core/backports/./build/test/functional/wallet_migration.py", line 568, in test_default_wallet_failure
24    assert_raises_rpc_error(-4, "Failed to create database", self.migrate_and_get_rpc, "")
25    ~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
26  File "/home/will/src/core/backports/test/functional/test_framework/util.py", line 151, in assert_raises_rpc_error
27    assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
28           ~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
29  File "/home/will/src/core/backports/test/functional/test_framework/util.py", line 164, in try_rpc
30    raise AssertionError("Unexpected JSONRPC error code %i" % e.error["code"])
31AssertionError: Unexpected JSONRPC error code -1
322026-01-07T10:31:08.489000Z TestFramework (INFO): Not stopping nodes as test failed. The dangling processes will be cleaned up later.

which leaves the wallet dir intact on failure:

 0❯ eza -al /tmp/bitcoin_func_test_yg5hrc0g/node0/regtest/wallets/
 1drwxr-xr-x   - will  7 Jan 10:31 _watchonly
 2drwx------   - will  7 Jan 10:31 basic0
 3drwx------   - will  7 Jan 10:31 basic1
 4drwx------   - will  7 Jan 10:31 basic2
 5drwx------   - will  7 Jan 10:31 default_wallet
 6.rw-r--r-- 16k will  7 Jan 10:31 default_wallet_1767781868.legacy.bak
 7drwx------   - will  7 Jan 10:31 encrypted
 8drwx------   - will  7 Jan 10:31 imports0
 9drwx------   - will  7 Jan 10:31 imports0_watchonly
10drwx------   - will  7 Jan 10:31 multisig0
11drwx------   - will  7 Jan 10:31 multisig1
12drwx------   - will  7 Jan 10:31 multisig1_solvables
13drwx------   - will  7 Jan 10:31 multisig1_watchonly
14drwx------   - will  7 Jan 10:31 pkcb
15.rw------- 12k will  7 Jan 10:31 wallet.dat

fanquake merged this on Jan 7, 2026

fanquake closed this on Jan 7, 2026

fanquake referenced this in commit 454ac8e7db on Jan 7, 2026

fanquake referenced this in commit ac4d0956cc on Jan 7, 2026

fanquake referenced this in commit 8e5c02a77f on Jan 7, 2026

fanquake referenced this in commit ac940ac2ca on Jan 7, 2026

fanquake referenced this in commit bef4b1fdee on Jan 7, 2026

fanquake referenced this in commit bc71372c0e on Jan 7, 2026

fanquake referenced this in commit 185ca0e391 on Jan 7, 2026

fanquake removed the label Needs backport (30.x) on Jan 7, 2026

fanquake commented at 11:28 am on January 7, 2026: member

Backported to 30.x in #34209.

pablomartin4btc approved

pablomartin4btc commented at 1:19 pm on January 7, 2026: member

tACK b7c34d08dd9549a95cffc6ec1ffa4bb4f81e35eb

02026-01-06T17:40:58.759997Z TestFramework (INFO): Test failure during unnamed (default) wallet migration
12026-01-06T17:40:58.960588Z TestFramework (ERROR): Unexpected exception
2Traceback (most recent call last):
3  File "/home/pablo/workspace/bitcoin/test/functional/test_framework/test_framework.py", line 142, in main
4    self.run_test()
5  File "/home/pablo/workspace/bitcoin/./build/test/functional/wallet_migration.py", line 1595, in run_test
6    self.test_default_wallet_failure()
7  File "/home/pablo/workspace/bitcoin/./build/test/functional/wallet_migration.py", line 698, in test_default_wallet_failure
8    assert self.master_node.wallets_path.exists()
9AssertionError

fanquake referenced this in commit cd6e4c9235 on Jan 7, 2026

furszy deleted the branch on Jan 7, 2026

in src/wallet/wallet.cpp:4349 in f4c7e28e80

4346+    // Helper to track wallet files and directories for cleanup on failure.
4347+    // Only directories of wallets created during migration (not the main wallet) are tracked.
4348+    auto track_for_cleanup = [&](const CWallet& wallet) {
4349+        const auto files = wallet.GetDatabase().Files();
4350+        wallet_files_to_remove.insert(files.begin(), files.end());
4351+        if (wallet.GetName() != wallet_name) {

darosior commented at 3:21 pm on January 7, 2026:

Note GetName()’s doc string is now somewhat dangerously confusing: it’s documented as a name only used for logging/debugging purposes, but is now relied upon for a funds-critical operation.

furszy commented at 3:27 pm on January 7, 2026:

Not “now”. This has been the case since a long time ago. Migration was introduced relying on it. See how we create the sqlite db here. We use the wallet’s name.

fanquake referenced this in commit abb6ae2ec5 on Jan 7, 2026

wallet: fix unnamed legacy wallet migration failure #34156

Story Line:

Testing Notes:

Code Coverage & Benchmarks

Reviews

Conflicts