diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index b86c6e56e6d..942f7ff9e71 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -479,10 +479,22 @@ std::shared_ptr 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 MigrateLegacyToDescriptor(std::shared_ptr } } - // 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 wallet_dirs; + std::set wallet_files_to_remove; + std::set 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 MigrateLegacyToDescriptor(std::shared_ptr // This wallet has no records. We can safely remove it. std::vector 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* wallet_ptr : {&local_wallet, &res.watchonly_wallet, &res.solvables_wallet}) { if (success && *wallet_ptr) { std::shared_ptr& 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 MigrateLegacyToDescriptor(std::shared_ptr if (res.solvables_wallet) created_wallets.push_back(std::move(res.solvables_wallet)); // Get the directories to remove after unloading - for (std::shared_ptr& w : created_wallets) { - wallet_dirs.emplace(fs::PathFromString(w->GetDatabase().Filename()).parent_path()); + for (std::shared_ptr& wallet : created_wallets) { + track_for_cleanup(*wallet); } // Unload the wallets @@ -4375,9 +4404,15 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr } } - // 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 6f3637a0dda..bc87e64d412 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()