diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index ed9bb8406d7..8550298d34b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -470,6 +470,8 @@ std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& b const fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), fs::u8path(wallet_name)); auto wallet_file = wallet_path / "wallet.dat"; std::shared_ptr wallet; + bool wallet_file_copied = false; + bool created_parent_dir = false; try { if (!fs::exists(backup_file)) { @@ -478,13 +480,34 @@ std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& b return nullptr; } - if (fs::exists(wallet_path) || !TryCreateDirectories(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; + // 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)) { + // 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)) { + error = Untranslated(strprintf("Failed to restore database path '%s'.", fs::PathToString(wallet_path))); + status = DatabaseStatus::FAILED_ALREADY_EXISTS; + return nullptr; + } + created_parent_dir = true; } fs::copy_file(backup_file, wallet_file, fs::copy_options::none); + wallet_file_copied = true; if (load_after_restore) { wallet = LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings); @@ -497,7 +520,13 @@ std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& b // Remove created wallet path only when loading fails if (load_after_restore && !wallet) { - fs::remove_all(wallet_path); + if (wallet_file_copied) fs::remove(wallet_file); + // Clean up the parent directory if we created it during restoration. + // As we have created it, it must be empty after deleting the wallet file. + if (created_parent_dir) { + Assume(fs::is_empty(wallet_path)); + fs::remove(wallet_path); + } } return wallet; @@ -4062,6 +4091,15 @@ bool CWallet::CanGrindR() const return !IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS); } +// Returns wallet prefix for migration. +// Used to name the backup file and newly created wallets. +// E.g. a watch-only wallet is named "_watchonly". +static std::string MigrationPrefixName(CWallet& wallet) +{ + const std::string& name{wallet.GetName()}; + return name.empty() ? "default_wallet" : name; +} + bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, MigrationResult& res) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) { AssertLockHeld(wallet.cs_wallet); @@ -4093,7 +4131,7 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, DatabaseStatus status; std::vector warnings; - std::string wallet_name = wallet.GetName() + "_watchonly"; + std::string wallet_name = MigrationPrefixName(wallet) + "_watchonly"; std::unique_ptr database = MakeWalletDatabase(wallet_name, options, status, error); if (!database) { error = strprintf(_("Wallet file creation failed: %s"), error); @@ -4132,7 +4170,7 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, DatabaseStatus status; std::vector warnings; - std::string wallet_name = wallet.GetName() + "_solvables"; + std::string wallet_name = MigrationPrefixName(wallet) + "_solvables"; std::unique_ptr database = MakeWalletDatabase(wallet_name, options, status, error); if (!database) { error = strprintf(_("Wallet file creation failed: %s"), error); @@ -4247,7 +4285,7 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr // cases, but in the case where the wallet name is a path to a data file, // the name of the data file is used, and in the case where the wallet name // is blank, "default_wallet" is used. - const std::string backup_prefix = wallet_name.empty() ? "default_wallet" : [&] { + const std::string backup_prefix = wallet_name.empty() ? MigrationPrefixName(*local_wallet) : [&] { // fs::weakly_canonical resolves relative specifiers and remove trailing slashes. const auto legacy_wallet_path = fs::weakly_canonical(GetWalletDir() / fs::PathFromString(wallet_name)); return fs::PathToString(legacy_wallet_path.filename()); @@ -4300,11 +4338,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 @@ -4312,23 +4367,30 @@ 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); } + LogInfo("Loading new wallets after migration...\n"); // 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(); wallet = LoadWallet(context, wallet_name, /*load_on_start=*/std::nullopt, options, status, error, warnings); - success = (wallet != nullptr); + if (!wallet) { + LogError("Failed to load wallet '%s' after migration. Rolling back migration to preserve consistency. " + "Error cause: %s\n", wallet_name, error.original); + success = false; + break; + } - // When no wallet is set, set the main wallet. - if (success && !res.wallet) { + // Set the first successfully loaded wallet as the main one. + // The loop order is intentional and must always start with the local wallet. + if (!res.wallet) { res.wallet_name = wallet->GetName(); res.wallet = std::move(wallet); } @@ -4343,8 +4405,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 @@ -4363,9 +4425,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 c658af2631f..5cf02ba39b9 100755 --- a/test/functional/wallet_backup.py +++ b/test/functional/wallet_backup.py @@ -39,6 +39,7 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, assert_raises_rpc_error, + sha256sum_file, ) @@ -132,10 +133,65 @@ 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() + def test_restore_existent_dir(self): + self.log.info("Test restore on an existent empty directory") + node = self.nodes[3] + backup_file = self.nodes[0].datadir_path / 'wallet.bak' + wallet_name = "restored_wallet" + wallet_dir = node.wallets_path / wallet_name + os.mkdir(wallet_dir) + res = node.restorewallet(wallet_name, backup_file) + assert_equal(res['name'], wallet_name) + node.unloadwallet(wallet_name) + + self.log.info("Test restore succeeds when the target directory contains non-wallet files") + wallet_file = node.wallets_path / wallet_name / "wallet.dat" + os.remove(wallet_file) + extra_file = node.wallets_path / wallet_name / "not_a_wallet.txt" + extra_file.touch() + res = node.restorewallet(wallet_name, backup_file) + assert_equal(res['name'], wallet_name) + assert extra_file.exists() # extra file was not removed by mistake + node.unloadwallet(wallet_name) + + self.log.info("Test restore failure due to existing db file in the destination directory") + original_shasum = sha256sum_file(wallet_file) + error_message = "Failed to restore wallet. Database file exists in '{}'.".format(wallet_dir / "wallet.dat") + assert_raises_rpc_error(-36, error_message, node.restorewallet, wallet_name, backup_file) + # Ensure the wallet file remains untouched + assert wallet_dir.exists() + assert_equal(original_shasum, sha256sum_file(wallet_file)) + + self.log.info("Test restore succeeds when the .dat file in the destination has a different name") + second_wallet = wallet_dir / "hidden_storage.dat" + os.rename(wallet_dir / "wallet.dat", second_wallet) + original_shasum = sha256sum_file(second_wallet) + res = node.restorewallet(wallet_name, backup_file) + assert_equal(res['name'], wallet_name) + assert (wallet_dir / "hidden_storage.dat").exists() + assert_equal(original_shasum, sha256sum_file(second_wallet)) + node.unloadwallet(wallet_name) + + # Clean for follow-up tests + os.remove(wallet_file) + + def test_restore_into_unnamed_wallet(self): + self.log.info("Test restore into a default unnamed wallet") + # This is also useful to test the migration recovery after failure logic + node = self.nodes[3] + backup_file = self.nodes[0].datadir_path / 'wallet.bak' + wallet_name = "" + res = node.restorewallet(wallet_name, backup_file) + assert_equal(res['name'], "") + assert (node.wallets_path / "wallet.dat").exists() + # Clean for follow-up tests + node.unloadwallet("") + os.remove(node.wallets_path / "wallet.dat") + def test_pruned_wallet_backup(self): self.log.info("Test loading backup on a pruned node when the backup was created close to the prune height of the restoring node") node = self.nodes[3] @@ -155,6 +211,13 @@ class WalletBackupTest(BitcoinTestFramework): # the backup to load successfully this close to the prune height node.restorewallet('pruned', node.datadir_path / 'wallet_pruned.bak') + self.log.info("Test restore on a pruned node when the backup was beyond the pruning point") + backup_file = self.nodes[0].datadir_path / 'wallet.bak' + wallet_name = "" + error_message = "Wallet loading failed. Prune: last wallet synchronisation goes beyond pruned data. You need to -reindex (download the whole blockchain again in case of a pruned node)" + assert_raises_rpc_error(-4, error_message, node.restorewallet, wallet_name, backup_file) + assert node.wallets_path.exists() # ensure the wallets dir exists + def run_test(self): self.log.info("Generating initial blockchain") self.generate(self.nodes[0], 1) @@ -219,6 +282,8 @@ class WalletBackupTest(BitcoinTestFramework): assert_equal(res2_rpc.getbalance(), balance2) self.restore_wallet_existent_name() + self.test_restore_existent_dir() + self.test_restore_into_unnamed_wallet() # Backup to source wallet file must fail sourcePaths = [ diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 89989fbf81b..e90c48fa6b0 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -3,6 +3,7 @@ # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test Migrating a wallet from legacy to descriptor.""" +from pathlib import Path import os.path import random import shutil @@ -659,6 +660,14 @@ class WalletMigrationTest(BitcoinTestFramework): assert_equal(bals, wallet.getbalances()) + def clear_default_wallet(self, backup_file): + # Test cleanup: Clear unnamed default wallet for subsequent tests + (self.old_node.wallets_path / "wallet.dat").unlink() + (self.master_node.wallets_path / "wallet.dat").unlink(missing_ok=True) + shutil.rmtree(self.master_node.wallets_path / "default_wallet_watchonly", ignore_errors=True) + shutil.rmtree(self.master_node.wallets_path / "default_wallet_solvables", ignore_errors=True) + backup_file.unlink() + def test_default_wallet(self): self.log.info("Test migration of the wallet named as the empty string") wallet = self.create_legacy_wallet("") @@ -675,6 +684,58 @@ class WalletMigrationTest(BitcoinTestFramework): # migrate_and_get_rpc already checks for backup file existence assert os.path.basename(res["backup_path"]).startswith("default_wallet") + wallet.unloadwallet() + self.clear_default_wallet(backup_file=Path(res["backup_path"])) + + def test_default_wallet_watch_only(self): + self.log.info("Test unnamed (default) watch-only wallet migration") + master_wallet = self.master_node.get_wallet_rpc(self.default_wallet_name) + wallet = self.create_legacy_wallet("", blank=True) + wallet.importaddress(master_wallet.getnewaddress(address_type="legacy")) + + res, wallet = self.migrate_and_get_rpc("") + + info = wallet.getwalletinfo() + assert_equal(info["descriptors"], True) + assert_equal(info["format"], "sqlite") + assert_equal(info["private_keys_enabled"], False) + assert_equal(info["walletname"], "default_wallet_watchonly") + # Check the default wallet is not available anymore + assert not (self.master_node.wallets_path / "wallet.dat").exists() + + wallet.unloadwallet() + self.clear_default_wallet(backup_file=Path(res["backup_path"])) + + def test_default_wallet_failure(self): + self.log.info("Test failure during unnamed (default) wallet migration") + master_wallet = self.master_node.get_wallet_rpc(self.default_wallet_name) + wallet = self.create_legacy_wallet("", blank=True) + wallet.importaddress(master_wallet.getnewaddress(address_type="legacy")) + + # Create wallet directory with the watch-only name and a wallet file. + # Because the wallet dir exists, this will cause migration to fail. + watch_only_dir = self.master_node.wallets_path / "default_wallet_watchonly" + os.mkdir(watch_only_dir) + shutil.copyfile(self.old_node.wallets_path / "wallet.dat", watch_only_dir / "wallet.dat") + + mocked_time = int(time.time()) + self.master_node.setmocktime(mocked_time) + assert_raises_rpc_error(-4, "Failed to create database", self.migrate_and_get_rpc, "") + self.master_node.setmocktime(0) + + # Verify the /wallets/ path exists + assert self.master_node.wallets_path.exists() + # Check backup file exists. Because the wallet has no name, the backup is prefixed with 'default_wallet' + backup_path = self.master_node.wallets_path / f"default_wallet_{mocked_time}.legacy.bak" + assert backup_path.exists() + # Verify the original unnamed wallet was restored + assert (self.master_node.wallets_path / "wallet.dat").exists() + # And verify it is still a BDB wallet + self.assert_is_bdb("") + + # Test cleanup: clear default wallet for next test + self.clear_default_wallet(backup_path) + def test_direct_file(self): self.log.info("Test migration of a wallet that is not in a wallet directory") wallet = self.create_legacy_wallet("plainfile") @@ -1542,6 +1603,37 @@ class WalletMigrationTest(BitcoinTestFramework): self.start_node(self.old_node.index) self.connect_nodes(1, 0) + def unsynced_wallet_on_pruned_node_fails(self): + self.log.info("Test migration of an unsynced wallet on a pruned node fails gracefully") + wallet = self.create_legacy_wallet("", load_on_startup=False) + last_wallet_synced_block = wallet.getwalletinfo()['lastprocessedblock']['height'] + wallet.unloadwallet() + + shutil.copyfile(self.old_node.wallets_path / "wallet.dat", self.master_node.wallets_path / "wallet.dat") + + # Generate blocks just so the wallet best block is pruned + self.restart_node(0, ["-fastprune", "-prune=1", "-nowallet"]) + self.connect_nodes(0, 1) + self.generate(self.master_node, 450, sync_fun=self.no_op) + self.master_node.pruneblockchain(250) + # Ensure next block to sync is unavailable + assert_raises_rpc_error(-1, "Block not available (pruned data)", self.master_node.getblock, self.master_node.getblockhash(last_wallet_synced_block + 1)) + + # Check migration failure + mocked_time = int(time.time()) + self.master_node.setmocktime(mocked_time) + assert_raises_rpc_error(-4, "last wallet synchronisation goes beyond pruned data. You need to -reindex (download the whole blockchain again in case of a pruned node)", self.master_node.migratewallet, wallet_name="") + self.master_node.setmocktime(0) + + # Verify the /wallets/ path exists, the wallet is still BDB and the backup file is there. + assert self.master_node.wallets_path.exists() + self.assert_is_bdb("") + backup_path = self.master_node.wallets_path / f"default_wallet_{mocked_time}.legacy.bak" + assert backup_path.exists() + + self.clear_default_wallet(backup_path) + + def run_test(self): self.master_node = self.nodes[0] self.old_node = self.nodes[1] @@ -1560,7 +1652,9 @@ class WalletMigrationTest(BitcoinTestFramework): self.test_wallet_with_relative_path() self.test_wallet_with_path("path/to/mywallet/") self.test_wallet_with_path("path/that/ends/in/..") + self.test_default_wallet_failure() self.test_default_wallet() + self.test_default_wallet_watch_only() self.test_direct_file() self.test_addressbook() self.test_migrate_raw_p2sh() @@ -1580,5 +1674,8 @@ class WalletMigrationTest(BitcoinTestFramework): self.test_solvable_no_privs() self.test_loading_failure_after_migration() + # Note: After this test the first 250 blocks of 'master_node' are pruned + self.unsynced_wallet_on_pruned_node_fails() + if __name__ == '__main__': WalletMigrationTest(__file__).main()