From 1de8a2372ab39386e689b27d15c4d029be239319 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 21 Jul 2023 21:37:31 -0300 Subject: [PATCH 1/2] wallet: disallow migration of invalid or not-watched scripts The legacy wallet allowed to import any raw script, without checking if it was valid or not. Appending it to the watch-only set. This causes a crash in the migration process because we are only expecting to find valid scripts inside the legacy spkm. These stored scripts internally map to `ISMINE_NO` (same as if they weren't stored at all..). So we need to check for these special case, and take into account that the legacy spkm could be storing invalid not watched scripts. Which, in code words, means IsMineInner() returning IsMineResult::INVALID for them. --- src/wallet/scriptpubkeyman.cpp | 17 ++++++++++++++++- src/wallet/scriptpubkeyman.h | 6 ++++++ src/wallet/wallet.cpp | 14 ++++++++++++++ 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 5b110b4d149..c3c027b8aae 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1714,8 +1714,23 @@ 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; } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index bf35c776ae1..3c89a850cfb 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -523,6 +523,12 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv 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 8fa93b97d65..50f54e82fb9 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3920,6 +3920,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."); @@ -4026,6 +4033,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; From 8e7e3e614955e60d3bf9e9a481ef8916bf9e22d9 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 21 Jul 2023 21:43:30 -0300 Subject: [PATCH 2/2] test: wallet, verify migration doesn't crash for an invalid script The migration process must skip any invalid script inside the legacy spkm and all the addressbook records linked to them. These scripts are not being watched by the current wallet, nor should be watched by the migrated one. IsMine() returns ISMINE_NO for them. --- test/functional/wallet_migration.py | 30 +++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 925376e8cd6..b4ef34de6eb 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 @@ -674,6 +675,21 @@ def send_to_script(script, amount): 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"]) @@ -683,6 +699,20 @@ def send_to_script(script, amount): # 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"])