From 8eb2736de4879e89b350ac31d8f34ae2af0aea97 Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 4 Dec 2024 12:48:18 -0500 Subject: [PATCH] wallet: migration, don't create 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. This commit implements this behavior. --- src/wallet/wallet.cpp | 52 ++++++++++++++++++++++++----- test/functional/wallet_migration.py | 14 +++++++- 2 files changed, 57 insertions(+), 9 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index b971be5ddda..ac3e8f581b5 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4078,6 +4078,9 @@ util::Result CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch, return util::Error{Untranslated(STR_INTERNAL_BUG("Error: Legacy wallet data missing"))}; } + // When the legacy wallet has no spendable scripts, it will be replaced by the watch-only/solvable wallets. + bool empty_local_wallet = 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()) { @@ -4109,9 +4112,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 (!empty_local_wallet && !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); @@ -4261,6 +4264,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 (empty_local_wallet) { + 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 } @@ -4269,7 +4280,7 @@ bool CWallet::CanGrindR() const return !IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER); } -bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, MigrationResult& res) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) +bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, MigrationResult& res, bool& empty_local_wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) { AssertLockHeld(wallet.cs_wallet); @@ -4378,6 +4389,11 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, return false; } wallet.WalletLogPrintf("Wallet migration complete.\n"); + + // When the legacy wallet had no spendable scripts, the local wallet will contain no information after migration. + // In such case, we notify the caller to not add it to the result. The user is not expecting to have a spendable + // wallet. + empty_local_wallet = data->desc_spkms.empty() && !data->master_key.key.IsValid(); return true; }); } @@ -4487,6 +4503,14 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle } } + // 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 a spendable wallet after migrating + // only watch-only scripts. + bool empty_local_wallet = false; + { LOCK(local_wallet->cs_wallet); // First change to using SQLite @@ -4495,7 +4519,7 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle // Do the migration of keys and scripts for non-blank wallets, and cleanup if it fails success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET); if (!success) { - success = DoMigration(*local_wallet, context, error, res); + success = DoMigration(*local_wallet, context, error, res, empty_local_wallet); } else { // Make sure that descriptors flag is actually set local_wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); @@ -4509,20 +4533,32 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle std::set wallet_dirs; 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()); - success = reload_wallet(local_wallet); - res.wallet = local_wallet; - res.wallet_name = wallet_name; + // Reload and set the main spendable wallet only if needed + if (!empty_local_wallet) { + success = reload_wallet(local_wallet); + res.wallet = local_wallet; + res.wallet_name = wallet_name; + } else { + // At this point, the main wallet is empty after migration, meaning it has no records. + // Therefore, 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); + } if (success && res.watchonly_wallet) { // Reload watchonly wallet_dirs.insert(fs::PathFromString(res.watchonly_wallet->GetDatabase().Filename()).parent_path()); success = reload_wallet(res.watchonly_wallet); + // When no spendable wallet is created, set the wallet name to the watch-only wallet name + if (res.wallet_name.empty()) res.wallet_name = res.watchonly_wallet->GetName(); } if (success && res.solvables_wallet) { // Reload solvables wallet_dirs.insert(fs::PathFromString(res.solvables_wallet->GetDatabase().Filename()).parent_path()); success = reload_wallet(res.solvables_wallet); + // When no spendable neither watch-only wallets are created, set the wallet name to the solvables-only wallet name + if (res.wallet_name.empty()) res.wallet_name = res.solvables_wallet->GetName(); } } if (!success) { diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index fd6ec16065f..5cb61e198ef 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 @@ -111,6 +111,8 @@ 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) + # Always verify backup path exist after migration + assert os.path.exists(migrate_info['backup_path']) return migrate_info, self.master_node.get_wallet_rpc(wallet_name) def test_basic(self): @@ -1018,6 +1020,12 @@ class WalletMigrationTest(BitcoinTestFramework): res, _ = self.migrate_and_get_rpc("bare_p2pk") wo_wallet = self.master_node.get_wallet_rpc(res['watchonly_name']) assert_equal(wo_wallet.listdescriptors()['descriptors'][0]['desc'], descsum_create(f'pk({pubkey.hex()})')) + + # 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): @@ -1062,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 run_test(self):