wallet: fix unnamed wallet migration failure
What changed, and why it matters
This commit fixes a bug in Bitcoin Core's wallet migration feature. When a user tried to migrate an old-style unnamed wallet and the migration failed, the cleanup code accidentally deleted the entire /wallets/ directory, wiping out all wallets and the backup file. The fix changes the cleanup to only remove specific database files created during migration, and only remove newly created empty directories. It also adjusts wallet restore to allow restoring into an existing directory as long as no database file would be overwritten.
Users running versions affected by the bug should avoid migrating legacy unnamed wallets until patched, and should ensure backups are stored outside the wallets directory. Apply this patch and run the updated functional tests.
Security signals we found
Data-loss bug: failed migration could recursively delete the main wallets directory
Overly broad filesystem deletion: fs::remove_all replaced with targeted file removal
Backup destruction: cleanup removed the backup file alongside other wallets
Logic bug introduced by prior commit f6ee59b6e2995a3916fb4f0d4cbe15ece2054494
Fix includes regression test update in wallet_backup.py
Evidence from the diff
The patch modifies MigrateLegacyToDescriptor() in src/wallet/wallet.cpp to replace broad fs::remove_all(dir) cleanup with targeted fs::remove(file) calls on tracked database files, plus fs::remove() on empty directories of created sub-wallets only. It introduces track_for_cleanup() to record individual files and eligible empty parent directories. RestoreWallet() is updated so the destination directory may already exist, provided no existing database file would be overwritten. A functional test message is updated accordingly.
Changed components
src/wallet/wallet.cppBitcoin Core wallet migration (migratewallet RPC)Bitcoin Core wallet restore (restorewallet RPC)test/functional/wallet_backup.pyInspect captured patch +49 / −14
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 9c51fb51..c5b0a496 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -480,10 +480,22 @@ std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& b
return nullptr;
}
+ // Wallet directories are allowed to exist, but must not contain a .dat file.
+ // Any existing wallet database is treated as a hard failure to prevent overwriting.
if (fs::exists(wallet_path)) {
- error = Untranslated(strprintf("Failed to create database path '%s'. Database already exists.", fs::PathToString(wallet_path)));
- status = DatabaseStatus::FAILED_ALREADY_EXISTS;
- return nullptr;
+ // If this is a file, it is the db and we don't want to overwrite it.
+ if (!fs::is_directory(wallet_path)) {
+ error = Untranslated(strprintf("Failed to restore wallet. Database file exists '%s'.", fs::PathToString(wallet_path)));
+ status = DatabaseStatus::FAILED_ALREADY_EXISTS;
+ return nullptr;
+ }
+
+ // Check we are not going to overwrite an existing db file
+ if (fs::exists(wallet_file)) {
+ error = Untranslated(strprintf("Failed to restore wallet. Database file exists in '%s'.", fs::PathToString(wallet_file)));
+ status = DatabaseStatus::FAILED_ALREADY_EXISTS;
+ return nullptr;
+ }
} else {
// The directory doesn't exist, create it
if (!TryCreateDirectories(wallet_path)) {
@@ -4322,11 +4334,28 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
}
}
- // In case of loading failure, we need to remember the wallet dirs to remove.
+ // In case of loading failure, we need to remember the wallet files we have created to remove.
// A `set` is used as it may be populated with the same wallet directory paths multiple times,
// both before and after loading. This ensures the set is complete even if one of the wallets
// fails to load.
- std::set<fs::path> wallet_dirs;
+ std::set<fs::path> wallet_files_to_remove;
+ std::set<fs::path> wallet_empty_dirs_to_remove;
+
+ // Helper to track wallet files and directories for cleanup on failure.
+ // Only directories of wallets created during migration (not the main wallet) are tracked.
+ auto track_for_cleanup = [&](const CWallet& wallet) {
+ const auto files = wallet.GetDatabase().Files();
+ wallet_files_to_remove.insert(files.begin(), files.end());
+ if (wallet.GetName() != wallet_name) {
+ // If this isn’t the main wallet, mark its directory for removal.
+ // This applies to the watch-only and solvable wallets.
+ // Wallets stored directly as files in the top-level directory
+ // (e.g. default unnamed wallets) don’t have a removable parent directory.
+ wallet_empty_dirs_to_remove.insert(fs::PathFromString(wallet.GetDatabase().Filename()).parent_path());
+ }
+ };
+
+
if (success) {
Assume(!res.wallet); // We will set it here.
// Check if the local wallet is empty after migration
@@ -4334,15 +4363,15 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
// This wallet has no records. We can safely remove it.
std::vector<fs::path> paths_to_remove = local_wallet->GetDatabase().Files();
local_wallet.reset();
- for (const auto& path_to_remove : paths_to_remove) fs::remove_all(path_to_remove);
+ for (const auto& path_to_remove : paths_to_remove) fs::remove(path_to_remove);
}
// Migration successful, load all the migrated wallets.
for (std::shared_ptr<CWallet>* wallet_ptr : {&local_wallet, &res.watchonly_wallet, &res.solvables_wallet}) {
if (success && *wallet_ptr) {
std::shared_ptr<CWallet>& wallet = *wallet_ptr;
- // Save db path and load wallet
- wallet_dirs.insert(fs::PathFromString(wallet->GetDatabase().Filename()).parent_path());
+ // Track db path and load wallet
+ track_for_cleanup(*wallet);
assert(wallet.use_count() == 1);
std::string wallet_name = wallet->GetName();
wallet.reset();
@@ -4365,8 +4394,8 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
if (res.solvables_wallet) created_wallets.push_back(std::move(res.solvables_wallet));
// Get the directories to remove after unloading
- for (std::shared_ptr<CWallet>& w : created_wallets) {
- wallet_dirs.emplace(fs::PathFromString(w->GetDatabase().Filename()).parent_path());
+ for (std::shared_ptr<CWallet>& wallet : created_wallets) {
+ track_for_cleanup(*wallet);
}
// Unload the wallets
@@ -4385,9 +4414,15 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
}
}
- // Delete the wallet directories
- for (const fs::path& dir : wallet_dirs) {
- fs::remove_all(dir);
+ // First, delete the db files we have created throughout this process and nothing else
+ for (const fs::path& file : wallet_files_to_remove) {
+ fs::remove(file);
+ }
+
+ // Second, delete the created wallet directories and nothing else. They must be empty at this point.
+ for (const fs::path& dir : wallet_empty_dirs_to_remove) {
+ Assume(fs::is_empty(dir));
+ fs::remove(dir);
}
// Restore the backup
diff --git a/test/functional/wallet_backup.py b/test/functional/wallet_backup.py
index c658af26..2c78f038 100755
--- a/test/functional/wallet_backup.py
+++ b/test/functional/wallet_backup.py
@@ -132,7 +132,7 @@ class WalletBackupTest(BitcoinTestFramework):
backup_file = self.nodes[0].datadir_path / 'wallet.bak'
wallet_name = "res0"
wallet_file = node.wallets_path / wallet_name
- error_message = "Failed to create database path '{}'. Database already exists.".format(wallet_file)
+ error_message = "Failed to restore wallet. Database file exists in '{}'.".format(wallet_file / "wallet.dat")
assert_raises_rpc_error(-36, error_message, node.restorewallet, wallet_name, backup_file)
assert wallet_file.exists()
Why this scored 64/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.