highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-01-18 22:35:39 +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: #34156
Rebased-From: f4c7e28e80
This commit is contained in:
furszy
2025-12-26 20:22:55 -05:00
committed by fanquake
parent 454ac8e7db
commit ac4d0956cc
2 changed files with 49 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files

61
src/wallet/wallet.cpp
View File

@@ -479,10 +479,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)) {
@@ -4312,11 +4324,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
@@ -4324,15 +4353,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();
@@ -4355,8 +4384,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
@@ -4375,9 +4404,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

2
test/functional/wallet_backup.py
View File

@@ -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()