diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 32c902b7680..119a99d601d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -511,10 +511,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)) { @@ -4559,26 +4571,43 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr } } - // 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 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 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) { // 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 MigrateLegacyToDescriptor(std::shared_ptr // 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> created_wallets; @@ -4595,8 +4624,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 @@ -4615,9 +4644,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 @@ -4631,8 +4666,7 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr } // 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. diff --git a/test/functional/wallet_backup.py b/test/functional/wallet_backup.py index 7c88f64dcf3..7ad83bdf870 100755 --- a/test/functional/wallet_backup.py +++ b/test/functional/wallet_backup.py @@ -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()