highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-01-20 15:19:07 +01:00
Code Issues Packages Projects Releases Wiki Activity

wallet: fix unnamed wallet migration failure

Browse Source 
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:
f6ee59b6e2
Previously, the `fs::copy_file` call was failing for top-level wallets,
which prevented the `fs::remove_all` call from being reached.

Github-Pull: bitcoin/bitcoin#34156
Rebased-From: f4c7e28e80
This commit is contained in:
furszy
2025-12-26 20:22:55 -05:00
committed by Ava Chow
parent d91f56e1e3
commit a074d36254
2 changed files with 54 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files

72
src/wallet/wallet.cpp
View File

@@ -511,10 +511,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)) {
@@ -4559,26 +4571,43 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
}
}
// In case of reloading failure, we need to remember the wallet dirs to remove
// Set is used as it may be populated with the same wallet directory paths multiple times,
// both before and after reloading. This ensures the set is complete even if one of the wallets
// fails to reload.
std::set<fs::path> wallet_dirs;
// 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_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) {
// Migration successful, unload all wallets locally, then reload them.
// Reload the main wallet
wallet_dirs.insert(fs::PathFromString(local_wallet->GetDatabase().Filename()).parent_path());
track_for_cleanup(*local_wallet);
success = reload_wallet(local_wallet);
res.wallet = local_wallet;
res.wallet_name = wallet_name;
if (success && res.watchonly_wallet) {
// Reload watchonly
wallet_dirs.insert(fs::PathFromString(res.watchonly_wallet->GetDatabase().Filename()).parent_path());
track_for_cleanup(*res.watchonly_wallet);
success = reload_wallet(res.watchonly_wallet);
}
if (success && res.solvables_wallet) {
// Reload solvables
wallet_dirs.insert(fs::PathFromString(res.solvables_wallet->GetDatabase().Filename()).parent_path());
track_for_cleanup(*res.solvables_wallet);
success = reload_wallet(res.solvables_wallet);
}
}
@@ -4586,7 +4615,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
// Migration failed, cleanup
// Before deleting the wallet's directory, copy the backup file to the top-level wallets dir
fs::path temp_backup_location = fsbridge::AbsPathJoin(GetWalletDir(), backup_filename);
fs::copy_file(backup_path, temp_backup_location, fs::copy_options::none);
fs::rename(backup_path, temp_backup_location);
// Make list of wallets to cleanup
std::vector<std::shared_ptr<CWallet>> created_wallets;
@@ -4595,8 +4624,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
@@ -4615,9 +4644,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
@@ -4631,8 +4666,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
}
// The wallet directory has been restored, but just in case, copy the previously created backup to the wallet dir
fs::copy_file(temp_backup_location, backup_path, fs::copy_options::none);
fs::remove(temp_backup_location);
fs::rename(temp_backup_location, backup_path);
// Verify that there is no dangling wallet: when the wallet wasn't loaded before, expect null.
// This check is performed after restoration to avoid an early error before saving the backup.

2
test/functional/wallet_backup.py
View File

@@ -136,7 +136,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()