From f6ee59b6e2995a3916fb4f0d4cbe15ece2054494 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Wed, 2 Jul 2025 13:58:03 -0700 Subject: [PATCH] wallet: migration: Make backup in walletdir --- src/wallet/wallet.cpp | 14 ++------------ test/functional/wallet_migration.py | 19 ++++++++++++++++--- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 3d83f356f6..c72d6476fd 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4230,9 +4230,8 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr } // Make a backup of the DB - fs::path this_wallet_dir = fs::absolute(fs::PathFromString(local_wallet->GetDatabase().Filename())).parent_path(); fs::path backup_filename = fs::PathFromString(strprintf("%s_%d.legacy.bak", (wallet_name.empty() ? "default_wallet" : wallet_name), GetTime())); - fs::path backup_path = this_wallet_dir / backup_filename; + fs::path backup_path = fsbridge::AbsPathJoin(GetWalletDir(), backup_filename); if (!local_wallet->BackupWallet(fs::PathToString(backup_path))) { return util::Error{_("Error: Unable to make a backup of your wallet")}; } @@ -4314,11 +4313,6 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr } } if (!success) { - // 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); - // Make list of wallets to cleanup std::vector> created_wallets; if (local_wallet) created_wallets.push_back(std::move(local_wallet)); @@ -4354,7 +4348,7 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr // Restore the backup // Convert the backup file to the wallet db file by renaming it and moving it into the wallet's directory. bilingual_str restore_error; - const auto& ptr_wallet = RestoreWallet(context, temp_backup_location, wallet_name, /*load_on_start=*/std::nullopt, status, restore_error, warnings, /*load_after_restore=*/false); + const auto& ptr_wallet = RestoreWallet(context, backup_path, wallet_name, /*load_on_start=*/std::nullopt, status, restore_error, warnings, /*load_after_restore=*/false); if (!restore_error.empty()) { error += restore_error + _("\nUnable to restore backup of wallet."); return util::Error{error}; @@ -4362,10 +4356,6 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr // Verify that the legacy wallet is not loaded after restoring from the backup. assert(!ptr_wallet); - // 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); - return util::Error{error}; } return res; diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index e2f1ec66c1..b2065a153f 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -118,17 +118,30 @@ class WalletMigrationTest(BitcoinTestFramework): for w in wallets["wallets"]: if w["name"] == wallet_name: assert_equal(w["warnings"], ["This wallet is a legacy wallet and will need to be migrated with migratewallet before it can be loaded"]) + + # Mock time so that we can check the backup filename. + mocked_time = int(time.time()) + self.master_node.setmocktime(mocked_time) # Migrate, checking that rescan does not occur with self.master_node.assert_debug_log(expected_msgs=[], unexpected_msgs=["Rescanning"]): migrate_info = self.master_node.migratewallet(wallet_name=wallet_name, **kwargs) + self.master_node.setmocktime(0) # Update wallet name in case the initial wallet was completely migrated to a watch-only wallet # (in which case the wallet name would be suffixed by the 'watchonly' term) - wallet_name = migrate_info['wallet_name'] - wallet = self.master_node.get_wallet_rpc(wallet_name) + migrated_wallet_name = migrate_info['wallet_name'] + wallet = self.master_node.get_wallet_rpc(migrated_wallet_name) assert_equal(wallet.getwalletinfo()["descriptors"], True) - self.assert_is_sqlite(wallet_name) + self.assert_is_sqlite(migrated_wallet_name) # Always verify the backup path exist after migration assert os.path.exists(migrate_info['backup_path']) + if wallet_name == "": + backup_prefix = "default_wallet" + else: + backup_prefix = os.path.basename(os.path.realpath(self.old_node.wallets_path / wallet_name)) + + expected_backup_path = self.master_node.wallets_path / f"{backup_prefix}_{mocked_time}.legacy.bak" + assert_equal(str(expected_backup_path), migrate_info['backup_path']) + return migrate_info, wallet def test_basic(self):