From b78990734621b8fe46c68a6e7edaf1fbd2f7d351 Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 4 Dec 2024 12:48:18 -0500 Subject: [PATCH] wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet Currently, the migration process creates a brand-new descriptor wallet with no connection to the user's legacy wallet when the legacy wallet lacks key material and contains only watch-only scripts. This behavior is not aligned with user expectations. If the legacy wallet contains only watch-only scripts, the migration process should only generate a watch-only wallet instead. --- src/qt/walletcontroller.cpp | 2 +- src/wallet/wallet.cpp | 44 +++++++++++++++++++++++++---- test/functional/wallet_migration.py | 24 ++++++++++++++-- 3 files changed, 60 insertions(+), 10 deletions(-) diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp index dd093e984a..869f9614c5 100644 --- a/src/qt/walletcontroller.cpp +++ b/src/qt/walletcontroller.cpp @@ -468,7 +468,7 @@ void MigrateWalletActivity::migrate(const std::string& name) auto res{node().walletLoader().migrateWallet(name, passphrase)}; if (res) { - m_success_message = tr("The wallet '%1' was migrated successfully.").arg(GUIUtil::HtmlEscape(GUIUtil::WalletDisplayName(res->wallet->getWalletName()))); + m_success_message = tr("The wallet '%1' was migrated successfully.").arg(GUIUtil::HtmlEscape(GUIUtil::WalletDisplayName(name))); if (res->watchonly_wallet_name) { m_success_message += QChar(' ') + tr("Watchonly scripts have been migrated to a new wallet named '%1'.").arg(GUIUtil::HtmlEscape(GUIUtil::WalletDisplayName(res->watchonly_wallet_name.value()))); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index fb5c74c9f7..a6a9a6d3c6 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3872,6 +3872,9 @@ util::Result CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch, return util::Error{Untranslated(STR_INTERNAL_BUG("Error: Legacy wallet data missing"))}; } + // Note: when the legacy wallet has no spendable scripts, it must be empty at the end of the process. + bool has_spendable_material = !data.desc_spkms.empty() || data.master_key.key.IsValid(); + // Get all invalid or non-watched scripts that will not be migrated std::set not_migrated_dests; for (const auto& script : legacy_spkm->GetNotMineScriptPubKeys()) { @@ -3903,9 +3906,9 @@ util::Result CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch, m_external_spk_managers.clear(); m_internal_spk_managers.clear(); - // Setup new descriptors + // Setup new descriptors (only if we are migrating any key material) SetWalletFlagWithDB(local_wallet_batch, WALLET_FLAG_DESCRIPTORS); - if (!IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { + if (has_spendable_material && !IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { // Use the existing master key if we have it if (data.master_key.key.IsValid()) { SetupDescriptorScriptPubKeyMans(local_wallet_batch, data.master_key); @@ -4055,6 +4058,14 @@ util::Result CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch, } } + // If there was no key material in the main wallet, there should be no records on it anymore. + // This wallet will be discarded at the end of the process. Only wallets that contain the + // migrated records will be presented to the user. + if (!has_spendable_material) { + if (!m_address_book.empty()) return util::Error{_("Error: Not all address book records were migrated")}; + if (!mapWallet.empty()) return util::Error{_("Error: Not all transaction records were migrated")}; + } + return {}; // all good } @@ -4292,6 +4303,14 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr } } + // Indicates whether the current wallet is empty after migration. + // Notes: + // When non-empty: the local wallet becomes the main spendable wallet. + // When empty: The local wallet is excluded from the result, as the + // user does not expect an empty spendable wallet after + // migrating only watch-only scripts. + bool empty_local_wallet = false; + { LOCK(local_wallet->cs_wallet); // First change to using SQLite @@ -4300,6 +4319,8 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr // Do the migration of keys and scripts for non-empty wallets, and cleanup if it fails if (HasLegacyRecords(*local_wallet)) { success = DoMigration(*local_wallet, context, error, res); + // No scripts mean empty wallet after migration + empty_local_wallet = local_wallet->GetAllScriptPubKeyMans().empty(); } else { // Make sure that descriptors flag is actually set local_wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); @@ -4313,6 +4334,15 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr // fails to reload. std::set wallet_dirs; if (success) { + Assume(!res.wallet); // We will set it here. + // Check if the local wallet is empty after migration + if (empty_local_wallet) { + // 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); + } + // Migration successful, unload all wallets locally, then reload them. // Note: We use a pointer to the shared_ptr to avoid increasing its reference count, // as 'reload_wallet' expects to be the sole owner (use_count == 1). @@ -4322,12 +4352,14 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr // Save db path and reload wallet wallet_dirs.insert(fs::PathFromString(wallet->GetDatabase().Filename()).parent_path()); success = reload_wallet(wallet); + + // When no wallet is set, set the main wallet. + if (!res.wallet) { + res.wallet_name = wallet->GetName(); + res.wallet = std::move(wallet); + } } } - - // Set main wallet - res.wallet = local_wallet; - res.wallet_name = wallet_name; } if (!success) { // Migration failed, cleanup diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 700d801891..c9548de123 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -3,7 +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.""" - +import os.path import random import shutil import struct @@ -116,9 +116,14 @@ class WalletMigrationTest(BitcoinTestFramework): # 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) + # 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) assert_equal(wallet.getwalletinfo()["descriptors"], True) self.assert_is_sqlite(wallet_name) + # Always verify the backup path exist after migration + assert os.path.exists(migrate_info['backup_path']) return migrate_info, wallet def test_basic(self): @@ -1014,8 +1019,15 @@ class WalletMigrationTest(BitcoinTestFramework): wallet.importaddress(address=p2pk_script.hex()) # Migrate wallet in the latest node res, _ = self.migrate_and_get_rpc("bare_p2pk") - wo_wallet = self.master_node.get_wallet_rpc(res['watchonly_name']) + wo_wallet = self.master_node.get_wallet_rpc(res['wallet_name']) assert_equal(wo_wallet.listdescriptors()['descriptors'][0]['desc'], descsum_create(f'pk({pubkey.hex()})')) + assert_equal(wo_wallet.getwalletinfo()["private_keys_enabled"], False) + + # Ensure that migrating a wallet with watch-only scripts does not create a spendable wallet. + assert_equal('bare_p2pk_watchonly', res['wallet_name']) + assert "bare_p2pk" not in self.master_node.listwallets() + assert "bare_p2pk" not in [w["name"] for w in self.master_node.listwalletdir()["wallets"]] + wo_wallet.unloadwallet() def test_manual_keys_import(self): @@ -1045,7 +1057,9 @@ class WalletMigrationTest(BitcoinTestFramework): res, _ = self.migrate_and_get_rpc("import_pubkeys") # Same as before, there should be descriptors in the watch-only wallet for the imported pubkey - wo_wallet = self.nodes[0].get_wallet_rpc(res['watchonly_name']) + wo_wallet = self.nodes[0].get_wallet_rpc(res['wallet_name']) + # Assert this is a watch-only wallet + assert_equal(wo_wallet.getwalletinfo()["private_keys_enabled"], False) # As we imported the pubkey only, there will be no key origin in the following descriptors pk_desc = descsum_create(f'pk({pubkey_hex})') pkh_desc = descsum_create(f'pkh({pubkey_hex})') @@ -1056,6 +1070,10 @@ class WalletMigrationTest(BitcoinTestFramework): # Verify all expected descriptors were migrated migrated_desc = [item['desc'] for item in wo_wallet.listdescriptors()['descriptors']] assert_equal(expected_descs, migrated_desc) + # Ensure that migrating a wallet with watch-only scripts does not create a spendable wallet. + assert_equal('import_pubkeys_watchonly', res['wallet_name']) + assert "import_pubkeys" not in self.master_node.listwallets() + assert "import_pubkeys" not in [w["name"] for w in self.master_node.listwalletdir()["wallets"]] wo_wallet.unloadwallet() def test_p2wsh(self):