diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 1f510d1c58e..20f735da12a 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1716,11 +1716,26 @@ std::unordered_set LegacyScriptPubKeyMan::GetScriptPub } // All watchonly scripts are raw - spks.insert(setWatchOnly.begin(), setWatchOnly.end()); + for (const CScript& script : setWatchOnly) { + // As the legacy wallet allowed to import any script, we need to verify the validity here. + // LegacyScriptPubKeyMan::IsMine() return 'ISMINE_NO' for invalid or not watched scripts (IsMineResult::INVALID or IsMineResult::NO). + // e.g. a "sh(sh(pkh()))" which legacy wallets allowed to import!. + if (IsMine(script) != ISMINE_NO) spks.insert(script); + } return spks; } +std::unordered_set LegacyScriptPubKeyMan::GetNotMineScriptPubKeys() const +{ + LOCK(cs_KeyStore); + std::unordered_set spks; + for (const CScript& script : setWatchOnly) { + if (IsMine(script) == ISMINE_NO) spks.insert(script); + } + return spks; +} + std::optional LegacyScriptPubKeyMan::MigrateToDescriptor() { LOCK(cs_KeyStore); diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index ec7b017720f..9d5ce6e1253 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -525,6 +525,12 @@ public: std::set GetKeys() const override; std::unordered_set GetScriptPubKeys() const override; + /** + * Retrieves scripts that were imported by bugs into the legacy spkm and are + * simply invalid, such as a sh(sh(pkh())) script, or not watched. + */ + std::unordered_set GetNotMineScriptPubKeys() const; + /** Get the DescriptorScriptPubKeyMans (with private keys) that have the same scriptPubKeys as this LegacyScriptPubKeyMan. * Does not modify this ScriptPubKeyMan. */ std::optional MigrateToDescriptor(); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 9f65db6c7b9..3622b00c777 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3860,6 +3860,13 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) return false; } + // Get all invalid or non-watched scripts that will not be migrated + std::set not_migrated_dests; + for (const auto& script : legacy_spkm->GetNotMineScriptPubKeys()) { + CTxDestination dest; + if (ExtractDestination(script, dest)) not_migrated_dests.emplace(dest); + } + for (auto& desc_spkm : data.desc_spkms) { if (m_spk_managers.count(desc_spkm->GetID()) > 0) { error = _("Error: Duplicate descriptors created during migration. Your wallet may be corrupted."); @@ -3966,6 +3973,13 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) continue; } } + + // Skip invalid/non-watched scripts that will not be migrated + if (not_migrated_dests.count(addr_pair.first) > 0) { + dests_to_delete.push_back(addr_pair.first); + continue; + } + // Not ours, not in watchonly wallet, and not in solvable error = _("Error: Address book data in wallet cannot be identified to belong to migrated wallets"); return false; diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index c565c879fb6..395044c8b2e 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -6,6 +6,7 @@ import random import shutil +from test_framework.address import script_to_p2sh from test_framework.descriptors import descsum_create from test_framework.test_framework import BitcoinTestFramework from test_framework.messages import COIN, CTransaction, CTxOut @@ -683,6 +684,21 @@ class WalletMigrationTest(BitcoinTestFramework): wallet.rpc.importaddress(address=script_wsh_pkh.hex(), label="raw_spk2", rescan=True, p2sh=False) assert_equal(wallet.getbalances()['watchonly']['trusted'], 5) + # Import sh(pkh()) script, by using importaddress(), with the p2sh flag enabled. + # This will wrap the script under another sh level, which is invalid!, and store it inside the wallet. + # The migration process must skip the invalid scripts and the addressbook records linked to them. + # They are not being watched by the current wallet, nor should be watched by the migrated one. + label_sh_pkh = "raw_sh_pkh" + script_pkh = key_to_p2pkh_script(df_wallet.getaddressinfo(df_wallet.getnewaddress())["pubkey"]) + script_sh_pkh = script_to_p2sh_script(script_pkh) + addy_script_sh_pkh = script_to_p2sh(script_pkh) # valid script address + addy_script_double_sh_pkh = script_to_p2sh(script_sh_pkh) # invalid script address + + # Note: 'importaddress()' will add two scripts, a valid one sh(pkh()) and an invalid one 'sh(sh(pkh()))'. + # Both of them will be stored with the same addressbook label. And only the latter one should + # be discarded during migration. The first one must be migrated. + wallet.rpc.importaddress(address=script_sh_pkh.hex(), label=label_sh_pkh, rescan=False, p2sh=True) + # Migrate wallet and re-check balance info_migration = wallet.migratewallet() wallet_wo = self.nodes[0].get_wallet_rpc(info_migration["watchonly_name"]) @@ -692,6 +708,20 @@ class WalletMigrationTest(BitcoinTestFramework): # The watch-only scripts are no longer part of the main wallet assert_equal(wallet.getbalances()['mine']['trusted'], 0) + # The invalid sh(sh(pk())) script label must not be part of the main wallet anymore + assert label_sh_pkh not in wallet.listlabels() + # But, the standard sh(pkh()) script should be part of the watch-only wallet. + addrs_by_label = wallet_wo.getaddressesbylabel(label_sh_pkh) + assert addy_script_sh_pkh in addrs_by_label + assert addy_script_double_sh_pkh not in addrs_by_label + + # Also, the watch-only wallet should have the descriptor for the standard sh(pkh()) + desc = descsum_create(f"addr({addy_script_sh_pkh})") + assert next(it['desc'] for it in wallet_wo.listdescriptors()['descriptors'] if it['desc'] == desc) + # And doesn't have a descriptor for the invalid one + desc_invalid = descsum_create(f"addr({addy_script_double_sh_pkh})") + assert_equal(next((it['desc'] for it in wallet_wo.listdescriptors()['descriptors'] if it['desc'] == desc_invalid), None), None) + # Just in case, also verify wallet restart self.nodes[0].unloadwallet(info_migration["watchonly_name"]) self.nodes[0].loadwallet(info_migration["watchonly_name"])