From c39b3cfcd1bc5002aa06d1b79c948ce94f3ec8dc Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Sat, 7 Dec 2024 11:34:26 -0500 Subject: [PATCH 1/9] test: Extra verification that migratewallet migrates --- test/functional/wallet_migration.py | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-) diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 62b4756a8ff..c3b306981bb 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -102,6 +102,7 @@ class WalletMigrationTest(BitcoinTestFramework): # Reload to force write that record self.old_node.unloadwallet(wallet_name) self.old_node.loadwallet(wallet_name) + assert_equal(self.old_node.get_wallet_rpc(wallet_name).getwalletinfo()["descriptors"], False) # Now unload so we can copy it to the master node for the migration test self.old_node.unloadwallet(wallet_name) if wallet_name == "": @@ -111,7 +112,10 @@ 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) - return migrate_info, self.master_node.get_wallet_rpc(wallet_name) + wallet = self.master_node.get_wallet_rpc(wallet_name) + assert_equal(wallet.getwalletinfo()["descriptors"], True) + self.assert_is_sqlite(wallet_name) + return migrate_info, wallet def test_basic(self): default = self.master_node.get_wallet_rpc(self.default_wallet_name) @@ -132,10 +136,6 @@ class WalletMigrationTest(BitcoinTestFramework): # Note: migration could take a while. _, basic0 = self.migrate_and_get_rpc("basic0") - # Verify created descriptors - assert_equal(basic0.getwalletinfo()["descriptors"], True) - self.assert_is_sqlite("basic0") - # The wallet should create the following descriptors: # * BIP32 descriptors in the form of "0h/0h/*" and "0h/1h/*" (2 descriptors) # * BIP44 descriptors in the form of "44h/1h/0h/0/*" and "44h/1h/0h/1/*" (2 descriptors) @@ -172,8 +172,6 @@ class WalletMigrationTest(BitcoinTestFramework): addr_gps = basic1.listaddressgroupings() basic1_migrate, basic1 = self.migrate_and_get_rpc("basic1") - assert_equal(basic1.getwalletinfo()["descriptors"], True) - self.assert_is_sqlite("basic1") assert_equal(basic1.getbalance(), bal) self.assert_list_txs_equal(basic1.listtransactions(), txs) @@ -191,8 +189,6 @@ class WalletMigrationTest(BitcoinTestFramework): default = self.master_node.get_wallet_rpc(self.default_wallet_name) self.master_node.loadwallet("basic1") basic1 = self.master_node.get_wallet_rpc("basic1") - assert_equal(basic1.getwalletinfo()["descriptors"], True) - self.assert_is_sqlite("basic1") assert_equal(basic1.getbalance(), bal) self.assert_list_txs_equal(basic1.listtransactions(), txs) @@ -214,8 +210,6 @@ class WalletMigrationTest(BitcoinTestFramework): # Now migrate and test that we still have the same balance/transactions _, basic2 = self.migrate_and_get_rpc("basic2") - assert_equal(basic2.getwalletinfo()["descriptors"], True) - self.assert_is_sqlite("basic2") assert_equal(basic2.getbalance(), basic2_balance) self.assert_list_txs_equal(basic2.listtransactions(), basic2_txs) @@ -240,8 +234,6 @@ class WalletMigrationTest(BitcoinTestFramework): ms_info = multisig0.addmultisigaddress(2, [addr1, addr2, addr3]) _, multisig0 = self.migrate_and_get_rpc("multisig0") - assert_equal(multisig0.getwalletinfo()["descriptors"], True) - self.assert_is_sqlite("multisig0") ms_addr_info = multisig0.getaddressinfo(ms_info["address"]) assert_equal(ms_addr_info["ismine"], True) assert_equal(ms_addr_info["desc"], ms_info["descriptor"]) @@ -275,8 +267,6 @@ class WalletMigrationTest(BitcoinTestFramework): # A new wallet multisig1_watchonly is created which has the multisig address # Transaction to multisig is in multisig1_watchonly and not multisig1 _, multisig1 = self.migrate_and_get_rpc("multisig1") - assert_equal(multisig1.getwalletinfo()["descriptors"], True) - self.assert_is_sqlite("multisig1") assert_equal(multisig1.getaddressinfo(addr1)["ismine"], False) assert_equal(multisig1.getaddressinfo(addr1)["iswatchonly"], False) assert_equal(multisig1.getaddressinfo(addr1)["solvable"], False) @@ -361,8 +351,6 @@ class WalletMigrationTest(BitcoinTestFramework): # Migrate _, imports0 = self.migrate_and_get_rpc("imports0") - assert_equal(imports0.getwalletinfo()["descriptors"], True) - self.assert_is_sqlite("imports0") assert_raises_rpc_error(-5, "Invalid or non-wallet transaction id", imports0.gettransaction, received_watchonly_txid) assert_raises_rpc_error(-5, "Invalid or non-wallet transaction id", imports0.gettransaction, received_sent_watchonly_utxo['txid']) assert_raises_rpc_error(-5, "Invalid or non-wallet transaction id", imports0.gettransaction, sent_watchonly_txid) @@ -918,7 +906,6 @@ class WalletMigrationTest(BitcoinTestFramework): assert_equal(wallet.getwalletinfo()["blank"], True) _, wallet = self.migrate_and_get_rpc("blank") assert_equal(wallet.getwalletinfo()["blank"], True) - assert_equal(wallet.getwalletinfo()["descriptors"], True) def test_avoidreuse(self): self.log.info("Test that avoidreuse persists after migration") From b1ab927bbf2a620ad984910c1d8317ec9bb33297 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 15 Nov 2024 20:22:18 -0500 Subject: [PATCH 2/9] tests: Test migration of additional P2WSH scripts --- test/functional/wallet_migration.py | 159 +++++++++++++++++++++++++++- 1 file changed, 157 insertions(+), 2 deletions(-) diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index c3b306981bb..76045b733dc 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -10,9 +10,10 @@ import struct import time from test_framework.address import ( - script_to_p2sh, key_to_p2pkh, key_to_p2wpkh, + script_to_p2sh, + script_to_p2wsh, ) from test_framework.bdb import BTREE_MAGIC from test_framework.descriptors import descsum_create @@ -1047,6 +1048,159 @@ class WalletMigrationTest(BitcoinTestFramework): assert_equal(expected_descs, migrated_desc) wo_wallet.unloadwallet() + def test_p2wsh(self): + self.log.info("Test that non-multisig P2WSH output scripts are migrated") + def_wallet = self.master_node.get_wallet_rpc(self.default_wallet_name) + + wallet = self.create_legacy_wallet("p2wsh") + + # Craft wsh(pkh(key)) + pubkey = wallet.getaddressinfo(wallet.getnewaddress())["pubkey"] + pkh_script = key_to_p2pkh_script(pubkey).hex() + wsh_pkh_script = script_to_p2wsh_script(pkh_script).hex() + wsh_pkh_addr = script_to_p2wsh(pkh_script) + + # Legacy single key scripts (i.e. pkh(key) and pk(key)) are not inserted into mapScripts + # automatically, they need to be imported directly if we want to receive to P2WSH (or P2SH) + # wrappings of such scripts. + wallet.importaddress(address=pkh_script, p2sh=False) + wallet.importaddress(address=wsh_pkh_script, p2sh=False) + + def_wallet.sendtoaddress(wsh_pkh_addr, 5) + self.generate(self.nodes[0], 6) + assert_equal(wallet.getbalances()['mine']['trusted'], 5) + + _, wallet = self.migrate_and_get_rpc("p2wsh") + + assert_equal(wallet.getbalances()['mine']['trusted'], 5) + addr_info = wallet.getaddressinfo(wsh_pkh_addr) + assert_equal(addr_info["ismine"], True) + assert_equal(addr_info["iswatchonly"], False) + assert_equal(addr_info["solvable"], True) + + wallet.unloadwallet() + + def test_disallowed_p2wsh(self): + self.log.info("Test that P2WSH output scripts with invalid witnessScripts are not migrated and do not cause migration failure") + def_wallet = self.master_node.get_wallet_rpc(self.default_wallet_name) + + wallet = self.create_legacy_wallet("invalid_p2wsh") + + invalid_addrs = [] + + # For a P2WSH output script stored in the legacy wallet's mapScripts, both the native P2WSH + # and the P2SH-P2WSH are detected by IsMine. We need to verify that descriptors for both + # output scripts are added to the resulting descriptor wallet. + # However, this cannot be done using a multisig as wallet migration treats multisigs specially. + # Instead, this is tested by importing a wsh(pkh()) script. But importing this directly will + # insert the wsh() into setWatchOnly which means that the setWatchOnly migration ends up handling + # this case, which we do not want. + # In order to get the wsh(pkh()) into only mapScripts and not setWatchOnly, we need to utilize + # importmulti and wrap the wsh(pkh()) inside of a sh(). This will insert the sh(wsh(pkh())) into + # setWatchOnly but not the wsh(pkh()). + # Furthermore, migration should not migrate the wsh(pkh()) if the key is uncompressed. + comp_wif, comp_pubkey = generate_keypair(compressed=True, wif=True) + comp_pkh_script = key_to_p2pkh_script(comp_pubkey).hex() + comp_wsh_pkh_script = script_to_p2wsh_script(comp_pkh_script).hex() + comp_sh_wsh_pkh_script = script_to_p2sh_script(comp_wsh_pkh_script).hex() + comp_wsh_pkh_addr = script_to_p2wsh(comp_pkh_script) + + uncomp_wif, uncomp_pubkey = generate_keypair(compressed=False, wif=True) + uncomp_pkh_script = key_to_p2pkh_script(uncomp_pubkey).hex() + uncomp_wsh_pkh_script = script_to_p2wsh_script(uncomp_pkh_script).hex() + uncomp_sh_wsh_pkh_script = script_to_p2sh_script(uncomp_wsh_pkh_script).hex() + uncomp_wsh_pkh_addr = script_to_p2wsh(uncomp_pkh_script) + invalid_addrs.append(uncomp_wsh_pkh_addr) + + import_res = wallet.importmulti([ + { + "scriptPubKey": comp_sh_wsh_pkh_script, + "timestamp": "now", + "redeemscript": comp_wsh_pkh_script, + "witnessscript": comp_pkh_script, + "keys": [ + comp_wif, + ], + }, + { + "scriptPubKey": uncomp_sh_wsh_pkh_script, + "timestamp": "now", + "redeemscript": uncomp_wsh_pkh_script, + "witnessscript": uncomp_pkh_script, + "keys": [ + uncomp_wif, + ], + }, + ]) + assert_equal(import_res[0]["success"], True) + assert_equal(import_res[1]["success"], True) + + # Create a wsh(sh(pkh())) - P2SH inside of P2WSH is invalid + comp_sh_pkh_script = script_to_p2sh_script(comp_pkh_script).hex() + wsh_sh_pkh_script = script_to_p2wsh_script(comp_sh_pkh_script).hex() + wsh_sh_pkh_addr = script_to_p2wsh(comp_sh_pkh_script) + invalid_addrs.append(wsh_sh_pkh_addr) + + # Import wsh(sh(pkh())) + wallet.importaddress(address=comp_sh_pkh_script, p2sh=False) + wallet.importaddress(address=wsh_sh_pkh_script, p2sh=False) + + # Create a wsh(wsh(pkh())) - P2WSH inside of P2WSH is invalid + wsh_wsh_pkh_script = script_to_p2wsh_script(comp_wsh_pkh_script).hex() + wsh_wsh_pkh_addr = script_to_p2wsh(comp_wsh_pkh_script) + invalid_addrs.append(wsh_wsh_pkh_addr) + + # Import wsh(wsh(pkh())) + wallet.importaddress(address=wsh_wsh_pkh_script, p2sh=False) + + # The wsh(pkh()) with a compressed key is always valid, so we should see that the wallet detects it as ismine, not + # watchonly, and can provide us information about the witnessScript via "embedded" + comp_wsh_pkh_addr_info = wallet.getaddressinfo(comp_wsh_pkh_addr) + assert_equal(comp_wsh_pkh_addr_info["ismine"], True) + assert_equal(comp_wsh_pkh_addr_info["iswatchonly"], False) + assert "embedded" in comp_wsh_pkh_addr_info + + # The invalid addresses are invalid, so the legacy wallet should not detect them as ismine, + # nor consider them watchonly. However, because the legacy wallet has the witnessScripts/redeemScripts, + # we should see information about those in "embedded" + for addr in invalid_addrs: + addr_info = wallet.getaddressinfo(addr) + assert_equal(addr_info["ismine"], False) + assert_equal(addr_info["iswatchonly"], False) + assert "embedded" in addr_info + + # Fund those output scripts, although the invalid addresses will not have any balance. + # This behavior follows as the addresses are not ismine. + def_wallet.send([{comp_wsh_pkh_addr: 1}] + [{k: i + 1} for i, k in enumerate(invalid_addrs)]) + self.generate(self.nodes[0], 6) + bal = wallet.getbalances() + assert_equal(bal["mine"]["trusted"], 1) + assert_equal(bal["watchonly"]["trusted"], 0) + + res, wallet = self.migrate_and_get_rpc("invalid_p2wsh") + assert "watchonly_name" not in res + assert "solvables_name" not in res + + assert_equal(wallet.getbalances()["mine"]["trusted"], 1) + + # After migration, the wsh(pkh()) with a compressed key is still valid and the descriptor wallet will have + # information about the witnessScript + comp_wsh_pkh_addr_info = wallet.getaddressinfo(comp_wsh_pkh_addr) + assert_equal(comp_wsh_pkh_addr_info["ismine"], True) + assert_equal(comp_wsh_pkh_addr_info["iswatchonly"], False) + assert "embedded" in comp_wsh_pkh_addr_info + + # After migration, the invalid addresses should still not be detected as ismine and not watchonly. + # The descriptor wallet should not have migrated these at all, so there should additionally be no + # information in "embedded" about the witnessScripts/redeemScripts. + for addr in invalid_addrs: + addr_info = wallet.getaddressinfo(addr) + assert_equal(addr_info["ismine"], False) + assert_equal(addr_info["iswatchonly"], False) + assert "embedded" not in addr_info + + wallet.unloadwallet() + def run_test(self): self.master_node = self.nodes[0] self.old_node = self.nodes[1] @@ -1074,7 +1228,8 @@ class WalletMigrationTest(BitcoinTestFramework): self.test_blank() self.test_migrate_simple_watch_only() self.test_manual_keys_import() - + self.test_p2wsh() + self.test_disallowed_p2wsh() if __name__ == '__main__': WalletMigrationTest(__file__).main() From b777e84cd70838fee5e3624a182632e04cc1d24c Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 13 Dec 2024 14:41:00 -0500 Subject: [PATCH 3/9] legacy spkm: Move CanProvide to LegacyDataSPKM This function will be needed in migration --- src/wallet/scriptpubkeyman.cpp | 2 +- src/wallet/scriptpubkeyman.h | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 23e2257b1e7..115b4b0b67e 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -591,7 +591,7 @@ std::unique_ptr LegacyDataSPKM::GetSolvingProvider(const CScrip return std::make_unique(*this); } -bool LegacyScriptPubKeyMan::CanProvide(const CScript& script, SignatureData& sigdata) +bool LegacyDataSPKM::CanProvide(const CScript& script, SignatureData& sigdata) { IsMineResult ismine = IsMineInner(*this, script, IsMineSigVersion::TOP, /* recurse_scripthash= */ false); if (ismine == IsMineResult::SPENDABLE || ismine == IsMineResult::WATCH_ONLY) { diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index d8b6c90178a..c5e45e4fa8f 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -318,6 +318,7 @@ public: uint256 GetID() const override { return uint256::ONE; } // TODO: Remove IsMine when deleting LegacyScriptPubKeyMan isminetype IsMine(const CScript& script) const override; + bool CanProvide(const CScript& script, SignatureData& sigdata) override; // FillableSigningProvider overrides bool HaveKey(const CKeyID &address) const override; @@ -486,8 +487,6 @@ public: bool CanGetAddresses(bool internal = false) const override; - bool CanProvide(const CScript& script, SignatureData& sigdata) override; - bool SignTransaction(CMutableTransaction& tx, const std::map& coins, int sighash, std::map& input_errors) const override; SigningResult SignMessage(const std::string& message, const PKHash& pkhash, std::string& str_sig) const override; std::optional FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, int sighash_type = SIGHASH_DEFAULT, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr, bool finalize = true) const override; From 440ea1ab6393638a49a2ba6daefe0b29778bea7e Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 13 Dec 2024 14:41:32 -0500 Subject: [PATCH 4/9] legacy spkm: use IsMine() to extract watched output scripts Instead of (partially) trying to reverse IsMine() to get the output scripts that a LegacySPKM would track, we can preserve it in migration only code and utilize it to get an accurate set of output scripts. This is accomplished by computing a set of output script candidates from map(Crypted)Keys, mapScripts, and setWatchOnly. This candidate set is an upper bound on the scripts tracked by the wallet. Then IsMine() is used to filter to the exact output scripts that LegacySPKM would track. By changing GetScriptPubKeys() this way, we can avoid complexities in reversing IsMine() and get a more complete set of output scripts. --- src/wallet/scriptpubkeyman.cpp | 101 +++++++++++++++++---------------- src/wallet/scriptpubkeyman.h | 4 ++ 2 files changed, 56 insertions(+), 49 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 115b4b0b67e..504d5db811e 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1700,61 +1700,64 @@ std::set LegacyScriptPubKeyMan::GetKeys() const return set_address; } -std::unordered_set LegacyDataSPKM::GetScriptPubKeys() const +std::unordered_set LegacyDataSPKM::GetCandidateScriptPubKeys() const { LOCK(cs_KeyStore); + std::unordered_set candidate_spks; + + // For every private key in the wallet, there should be a P2PK, P2PKH, P2WPKH, and P2SH-P2WPKH + const auto& add_pubkey = [&candidate_spks](const CPubKey& pub) -> void { + candidate_spks.insert(GetScriptForRawPubKey(pub)); + candidate_spks.insert(GetScriptForDestination(PKHash(pub))); + + CScript wpkh = GetScriptForDestination(WitnessV0KeyHash(pub)); + candidate_spks.insert(wpkh); + candidate_spks.insert(GetScriptForDestination(ScriptHash(wpkh))); + }; + for (const auto& [_, key] : mapKeys) { + add_pubkey(key.GetPubKey()); + } + for (const auto& [_, ckeypair] : mapCryptedKeys) { + add_pubkey(ckeypair.first); + } + + // mapScripts contains all redeemScripts and witnessScripts. Therefore each script in it has + // itself, P2SH, P2WSH, and P2SH-P2WSH as a candidate. + // Invalid scripts such as P2SH-P2SH and P2WSH-P2SH, among others, will be added as candidates. + // Callers of this function will need to remove such scripts. + const auto& add_script = [&candidate_spks](const CScript& script) -> void { + candidate_spks.insert(script); + candidate_spks.insert(GetScriptForDestination(ScriptHash(script))); + + CScript wsh = GetScriptForDestination(WitnessV0ScriptHash(script)); + candidate_spks.insert(wsh); + candidate_spks.insert(GetScriptForDestination(ScriptHash(wsh))); + }; + for (const auto& [_, script] : mapScripts) { + add_script(script); + } + + // Although setWatchOnly should only contain output scripts, we will also include each script's + // P2SH, P2WSH, and P2SH-P2WSH as a precaution. + for (const auto& script : setWatchOnly) { + add_script(script); + } + + return candidate_spks; +} + +std::unordered_set LegacyDataSPKM::GetScriptPubKeys() const +{ + // Run IsMine() on each candidate output script. Any script that is not ISMINE_NO is an output + // script to return. + // This both filters out things that are not watched by the wallet, and things that are invalid. std::unordered_set spks; - - // All keys have at least P2PK and P2PKH - for (const auto& key_pair : mapKeys) { - const CPubKey& pub = key_pair.second.GetPubKey(); - spks.insert(GetScriptForRawPubKey(pub)); - spks.insert(GetScriptForDestination(PKHash(pub))); - } - for (const auto& key_pair : mapCryptedKeys) { - const CPubKey& pub = key_pair.second.first; - spks.insert(GetScriptForRawPubKey(pub)); - spks.insert(GetScriptForDestination(PKHash(pub))); - } - - // For every script in mapScript, only the ISMINE_SPENDABLE ones are being tracked. - // The watchonly ones will be in setWatchOnly which we deal with later - // For all keys, if they have segwit scripts, those scripts will end up in mapScripts - for (const auto& script_pair : mapScripts) { - const CScript& script = script_pair.second; - if (IsMine(script) == ISMINE_SPENDABLE) { - // Add ScriptHash for scripts that are not already P2SH - if (!script.IsPayToScriptHash()) { - spks.insert(GetScriptForDestination(ScriptHash(script))); - } - // For segwit scripts, we only consider them spendable if we have the segwit spk - int wit_ver = -1; - std::vector witprog; - if (script.IsWitnessProgram(wit_ver, witprog) && wit_ver == 0) { - spks.insert(script); - } - } else { - // Multisigs are special. They don't show up as ISMINE_SPENDABLE unless they are in a P2SH - // So check the P2SH of a multisig to see if we should insert it - std::vector> sols; - TxoutType type = Solver(script, sols); - if (type == TxoutType::MULTISIG) { - CScript ms_spk = GetScriptForDestination(ScriptHash(script)); - if (IsMine(ms_spk) != ISMINE_NO) { - spks.insert(ms_spk); - } - } + for (const CScript& script : GetCandidateScriptPubKeys()) { + if (IsMine(script) != ISMINE_NO) { + spks.insert(script); } } - // All watchonly scripts are raw - 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; } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index c5e45e4fa8f..c88abbda6d6 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -302,6 +302,10 @@ protected: virtual bool AddKeyPubKeyInner(const CKey& key, const CPubKey &pubkey); bool AddCryptedKeyInner(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret); + // Helper function to retrieve a conservative superset of all output scripts that may be relevant to this LegacyDataSPKM. + // It may include scripts that are invalid or not actually watched by this LegacyDataSPKM. + // Used only in migration. + std::unordered_set GetCandidateScriptPubKeys() const; public: using ScriptPubKeyMan::ScriptPubKeyMan; From fa1b7cd6e2cfd581679d1fb6bc0fabcd6ee6e70a Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 13 Dec 2024 14:45:32 -0500 Subject: [PATCH 5/9] migration: Skip descriptors which do not parse InferDescriptors can sometimes make descriptors which are actually invalid and cannot be parsed. Detect and skip such descriptors by doing a Parse() check before adding the descriptor to the wallet. --- src/wallet/scriptpubkeyman.cpp | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 504d5db811e..ef96f1cb436 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1928,6 +1928,21 @@ std::optional LegacyDataSPKM::MigrateToDescriptor() // InferDescriptor as that will get us all the solving info if it is there std::unique_ptr desc = InferDescriptor(spk, *GetSolvingProvider(spk)); + + // Past bugs in InferDescriptor have caused it to create descriptors which cannot be re-parsed. + // Re-parse the descriptors to detect that, and skip any that do not parse. + { + std::string desc_str = desc->ToString(); + FlatSigningProvider parsed_keys; + std::string parse_error; + std::vector> parsed_descs = Parse(desc_str, parsed_keys, parse_error); + if (parsed_descs.empty()) { + // Remove this scriptPubKey from the set + it = spks.erase(it); + continue; + } + } + // Get the private keys for this descriptor std::vector scripts; FlatSigningProvider keys; From e8c3efc7d8f06210dfef7c1a47bad6315714f4cd Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 13 Dec 2024 14:46:31 -0500 Subject: [PATCH 6/9] wallet migration: Determine Solvables with CanProvide LegacySPKM would determine whether it could provide any script data to a transaction through the use of the CanProvide function. Instead of partially reversing signing logic to figure out the output scripts of solvable things, we use the same candidate set approach in GetScriptPubKeys() and instead filter the candidate set first for things that are ISMINE_NO, and second with CanProvide(). This should give a more accurate solvables wallet. --- src/wallet/scriptpubkeyman.cpp | 78 +++++++++++++++++----------------- 1 file changed, 40 insertions(+), 38 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index ef96f1cb436..735ee7c7861 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1992,10 +1992,29 @@ std::optional LegacyDataSPKM::MigrateToDescriptor() } } - // Multisigs are special. They don't show up as ISMINE_SPENDABLE unless they are in a P2SH - // So we have to check if any of our scripts are a multisig and if so, add the P2SH - for (const auto& script_pair : mapScripts) { - const CScript script = script_pair.second; + // Make sure that we have accounted for all scriptPubKeys + if (!Assume(spks.empty())) { + LogPrintf("%s\n", STR_INTERNAL_BUG("Error: Some output scripts were not migrated.\n")); + return std::nullopt; + } + + // Legacy wallets can also contain scripts whose P2SH, P2WSH, or P2SH-P2WSH it is not watching for + // but can provide script data to a PSBT spending them. These "solvable" output scripts will need to + // be put into the separate "solvables" wallet. + // These can be detected by going through the entire candidate output scripts, finding the ISMINE_NO scripts, + // and checking CanProvide() which will dummy sign. + for (const CScript& script : GetCandidateScriptPubKeys()) { + // Since we only care about P2SH, P2WSH, and P2SH-P2WSH, filter out any scripts that are not those + if (!script.IsPayToScriptHash() && !script.IsPayToWitnessScriptHash()) { + continue; + } + if (IsMine(script) != ISMINE_NO) { + continue; + } + SignatureData dummy_sigdata; + if (!CanProvide(script, dummy_sigdata)) { + continue; + } // Get birthdate from script meta uint64_t creation_time = 0; @@ -2004,45 +2023,28 @@ std::optional LegacyDataSPKM::MigrateToDescriptor() creation_time = it->second.nCreateTime; } - std::vector> sols; - TxoutType type = Solver(script, sols); - if (type == TxoutType::MULTISIG) { - CScript sh_spk = GetScriptForDestination(ScriptHash(script)); - CTxDestination witdest = WitnessV0ScriptHash(script); - CScript witprog = GetScriptForDestination(witdest); - CScript sh_wsh_spk = GetScriptForDestination(ScriptHash(witprog)); + // InferDescriptor as that will get us all the solving info if it is there + std::unique_ptr desc = InferDescriptor(script, *GetSolvingProvider(script)); + if (!desc->IsSolvable()) { + // The wallet was able to provide some information, but not enough to make a descriptor that actually + // contains anything useful. This is probably because the script itself is actually unsignable (e.g. P2WSH-P2WSH). + continue; + } - // We only want the multisigs that we have not already seen, i.e. they are not watchonly and not spendable - // For P2SH, a multisig is not ISMINE_NO when: - // * All keys are in the wallet - // * The multisig itself is watch only - // * The P2SH is watch only - // For P2SH-P2WSH, if the script is in the wallet, then it will have the same conditions as P2SH. - // For P2WSH, a multisig is not ISMINE_NO when, other than the P2SH conditions: - // * The P2WSH script is in the wallet and it is being watched - std::vector> keys(sols.begin() + 1, sols.begin() + sols.size() - 1); - if (HaveWatchOnly(sh_spk) || HaveWatchOnly(script) || HaveKeys(keys, *this) || (HaveCScript(CScriptID(witprog)) && HaveWatchOnly(witprog))) { - // The above emulates IsMine for these 3 scriptPubKeys, so double check that by running IsMine - assert(IsMine(sh_spk) != ISMINE_NO || IsMine(witprog) != ISMINE_NO || IsMine(sh_wsh_spk) != ISMINE_NO); + // Past bugs in InferDescriptor have caused it to create descriptors which cannot be re-parsed + // Re-parse the descriptors to detect that, and skip any that do not parse. + { + std::string desc_str = desc->ToString(); + FlatSigningProvider parsed_keys; + std::string parse_error; + std::vector> parsed_descs = Parse(desc_str, parsed_keys, parse_error, false); + if (parsed_descs.empty()) { continue; } - assert(IsMine(sh_spk) == ISMINE_NO && IsMine(witprog) == ISMINE_NO && IsMine(sh_wsh_spk) == ISMINE_NO); - - std::unique_ptr sh_desc = InferDescriptor(sh_spk, *GetSolvingProvider(sh_spk)); - out.solvable_descs.emplace_back(sh_desc->ToString(), creation_time); - - const auto desc = InferDescriptor(witprog, *this); - if (desc->IsSolvable()) { - std::unique_ptr wsh_desc = InferDescriptor(witprog, *GetSolvingProvider(witprog)); - out.solvable_descs.emplace_back(wsh_desc->ToString(), creation_time); - std::unique_ptr sh_wsh_desc = InferDescriptor(sh_wsh_spk, *GetSolvingProvider(sh_wsh_spk)); - out.solvable_descs.emplace_back(sh_wsh_desc->ToString(), creation_time); - } } - } - // Make sure that we have accounted for all scriptPubKeys - assert(spks.size() == 0); + out.solvable_descs.emplace_back(desc->ToString(), creation_time); + } // Finalize transaction if (!batch.TxnCommit()) { From 1eb9a2a39fdb20262a5afe21d8feada9bdcbbd18 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 13 Dec 2024 16:07:10 -0500 Subject: [PATCH 7/9] test: Test migration of miniscript in legacy wallets --- test/functional/wallet_migration.py | 55 +++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 76045b733dc..03ec337e570 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -1201,6 +1201,60 @@ class WalletMigrationTest(BitcoinTestFramework): wallet.unloadwallet() + def test_miniscript(self): + # It turns out that due to how signing logic works, legacy wallets that have valid miniscript witnessScripts + # and the private keys for them can still sign and spend them, even though output scripts involving them + # as a witnessScript would not be detected as ISMINE_SPENDABLE. + self.log.info("Test migration of a legacy wallet containing miniscript") + def_wallet = self.master_node.get_wallet_rpc(self.default_wallet_name) + wallet = self.create_legacy_wallet("miniscript") + + privkey, _ = generate_keypair(compressed=True, wif=True) + + # Make a descriptor where we only have some of the keys. This will be migrated to the watchonly wallet. + some_keys_priv_desc = descsum_create(f"wsh(or_b(pk({privkey}),s:pk(029ffbe722b147f3035c87cb1c60b9a5947dd49c774cc31e94773478711a929ac0)))") + some_keys_addr = self.master_node.deriveaddresses(some_keys_priv_desc)[0] + + # Make a descriptor where we have all of the keys. This will stay in the migrated wallet + all_keys_priv_desc = descsum_create(f"wsh(and_v(v:pk({privkey}),1))") + all_keys_addr = self.master_node.deriveaddresses(all_keys_priv_desc)[0] + + imp = wallet.importmulti([ + { + "desc": some_keys_priv_desc, + "timestamp": "now", + }, + { + "desc": all_keys_priv_desc, + "timestamp": "now", + } + ]) + assert_equal(imp[0]["success"], True) + assert_equal(imp[1]["success"], True) + + def_wallet.sendtoaddress(some_keys_addr, 1) + def_wallet.sendtoaddress(all_keys_addr, 1) + self.generate(self.master_node, 6) + # Check that the miniscript can be spent by the legacy wallet + send_res = wallet.send(outputs=[{some_keys_addr: 1},{all_keys_addr: 0.75}], include_watching=True, change_address=def_wallet.getnewaddress()) + assert_equal(send_res["complete"], True) + self.generate(self.old_node, 6) + assert_equal(wallet.getbalances()["watchonly"]["trusted"], 1.75) + + _, wallet = self.migrate_and_get_rpc("miniscript") + + # The miniscript with all keys should be in the migrated wallet + assert_equal(wallet.getbalances()["mine"], {"trusted": 0.75, "untrusted_pending": 0, "immature": 0}) + assert_equal(wallet.getaddressinfo(all_keys_addr)["ismine"], True) + assert_equal(wallet.getaddressinfo(some_keys_addr)["ismine"], False) + + # The miniscript with some keys should be in the watchonly wallet + assert "miniscript_watchonly" in self.master_node.listwallets() + watchonly = self.master_node.get_wallet_rpc("miniscript_watchonly") + assert_equal(watchonly.getbalances()["mine"], {"trusted": 1, "untrusted_pending": 0, "immature": 0}) + assert_equal(watchonly.getaddressinfo(some_keys_addr)["ismine"], True) + assert_equal(watchonly.getaddressinfo(all_keys_addr)["ismine"], False) + def run_test(self): self.master_node = self.nodes[0] self.old_node = self.nodes[1] @@ -1230,6 +1284,7 @@ class WalletMigrationTest(BitcoinTestFramework): self.test_manual_keys_import() self.test_p2wsh() self.test_disallowed_p2wsh() + self.test_miniscript() if __name__ == '__main__': WalletMigrationTest(__file__).main() From 17f01b0795e1dfa264fb42de65339bd18abb7f97 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 13 Dec 2024 16:49:40 -0500 Subject: [PATCH 8/9] test: Test migration of taproot output scripts --- test/functional/wallet_migration.py | 60 +++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 03ec337e570..09a9aba24f0 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -25,6 +25,7 @@ from test_framework.script_util import key_to_p2pkh_script, key_to_p2pk_script, from test_framework.util import ( assert_equal, assert_raises_rpc_error, + find_vout_for_address, sha256sum_file, ) from test_framework.wallet_util import ( @@ -1255,6 +1256,64 @@ class WalletMigrationTest(BitcoinTestFramework): assert_equal(watchonly.getaddressinfo(some_keys_addr)["ismine"], True) assert_equal(watchonly.getaddressinfo(all_keys_addr)["ismine"], False) + def test_taproot(self): + # It turns out that due to how signing logic works, legacy wallets that have the private key for a Taproot + # output key will be able to sign and spend those scripts, even though they would not be detected as ISMINE_SPENDABLE. + self.log.info("Test migration of Taproot scripts") + def_wallet = self.master_node.get_wallet_rpc(self.default_wallet_name) + wallet = self.create_legacy_wallet("taproot") + + privkey, _ = generate_keypair(compressed=True, wif=True) + + rawtr_desc = descsum_create(f"rawtr({privkey})") + rawtr_addr = self.master_node.deriveaddresses(rawtr_desc)[0] + rawtr_spk = self.master_node.validateaddress(rawtr_addr)["scriptPubKey"] + tr_desc = descsum_create(f"tr({privkey})") + tr_addr = self.master_node.deriveaddresses(tr_desc)[0] + tr_spk = self.master_node.validateaddress(tr_addr)["scriptPubKey"] + tr_script_desc = descsum_create(f"tr(9ffbe722b147f3035c87cb1c60b9a5947dd49c774cc31e94773478711a929ac0,pk({privkey}))") + tr_script_addr = self.master_node.deriveaddresses(tr_script_desc)[0] + tr_script_spk = self.master_node.validateaddress(tr_script_addr)["scriptPubKey"] + + wallet.importaddress(rawtr_spk) + wallet.importaddress(tr_spk) + wallet.importaddress(tr_script_spk) + wallet.importprivkey(privkey) + + txid = def_wallet.send([{rawtr_addr: 1},{tr_addr: 2}, {tr_script_addr: 3}])["txid"] + rawtr_vout = find_vout_for_address(self.master_node, txid, rawtr_addr) + tr_vout = find_vout_for_address(self.master_node, txid, tr_addr) + tr_script_vout = find_vout_for_address(self.master_node, txid, tr_script_addr) + self.generate(self.master_node, 6) + assert_equal(wallet.getbalances()["watchonly"]["trusted"], 6) + + # Check that the rawtr can be spent by the legacy wallet + send_res = wallet.send(outputs=[{rawtr_addr: 0.5}], include_watching=True, change_address=def_wallet.getnewaddress(), inputs=[{"txid": txid, "vout": rawtr_vout}]) + assert_equal(send_res["complete"], True) + self.generate(self.old_node, 6) + assert_equal(wallet.getbalances()["watchonly"]["trusted"], 5.5) + assert_equal(wallet.getbalances()["mine"]["trusted"], 0) + + # Check that the tr() cannot be spent by the legacy wallet + send_res = wallet.send(outputs=[{def_wallet.getnewaddress(): 4}], include_watching=True, inputs=[{"txid": txid, "vout": tr_vout}, {"txid": txid, "vout": tr_script_vout}]) + assert_equal(send_res["complete"], False) + + res, wallet = self.migrate_and_get_rpc("taproot") + + # The rawtr should be migrated + assert_equal(wallet.getbalances()["mine"], {"trusted": 0.5, "untrusted_pending": 0, "immature": 0}) + assert_equal(wallet.getaddressinfo(rawtr_addr)["ismine"], True) + assert_equal(wallet.getaddressinfo(tr_addr)["ismine"], False) + assert_equal(wallet.getaddressinfo(tr_script_addr)["ismine"], False) + + # The tr() with some keys should be in the watchonly wallet + assert "taproot_watchonly" in self.master_node.listwallets() + watchonly = self.master_node.get_wallet_rpc("taproot_watchonly") + assert_equal(watchonly.getbalances()["mine"], {"trusted": 5, "untrusted_pending": 0, "immature": 0}) + assert_equal(watchonly.getaddressinfo(rawtr_addr)["ismine"], False) + assert_equal(watchonly.getaddressinfo(tr_addr)["ismine"], True) + assert_equal(watchonly.getaddressinfo(tr_script_addr)["ismine"], True) + def run_test(self): self.master_node = self.nodes[0] self.old_node = self.nodes[1] @@ -1285,6 +1344,7 @@ class WalletMigrationTest(BitcoinTestFramework): self.test_p2wsh() self.test_disallowed_p2wsh() self.test_miniscript() + self.test_taproot() if __name__ == '__main__': WalletMigrationTest(__file__).main() From af76664b12d8611b606a7e755a103a20542ee539 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Mon, 16 Dec 2024 15:07:19 -0500 Subject: [PATCH 9/9] test: Test migration of a solvable script with no privkeys The legacy wallet will be able to solve output scripts where the redeemScript or witnessScript is known, but does not know any of the private keys involved in that script. These should be migrated to the solvables wallet. --- 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 09a9aba24f0..60daba79b7c 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -1314,6 +1314,35 @@ class WalletMigrationTest(BitcoinTestFramework): assert_equal(watchonly.getaddressinfo(tr_addr)["ismine"], True) assert_equal(watchonly.getaddressinfo(tr_script_addr)["ismine"], True) + def test_solvable_no_privs(self): + self.log.info("Test migrating a multisig that we do not have any private keys for") + wallet = self.create_legacy_wallet("multisig_noprivs") + + _, pubkey = generate_keypair(compressed=True, wif=True) + + add_ms_res = wallet.addmultisigaddress(nrequired=1, keys=[pubkey.hex()]) + addr = add_ms_res["address"] + + # The multisig address should be ISMINE_NO but we should have the script info + addr_info = wallet.getaddressinfo(addr) + assert_equal(addr_info["ismine"], False) + assert "hex" in addr_info + + migrate_res, wallet = self.migrate_and_get_rpc("multisig_noprivs") + assert_equal(migrate_res["solvables_name"], "multisig_noprivs_solvables") + solvables = self.master_node.get_wallet_rpc(migrate_res["solvables_name"]) + + # The multisig should not be in the spendable wallet + addr_info = wallet.getaddressinfo(addr) + assert_equal(addr_info["ismine"], False) + assert "hex" not in addr_info + + # The multisig address should be in the solvables wallet + addr_info = solvables.getaddressinfo(addr) + assert_equal(addr_info["ismine"], True) + assert_equal(addr_info["solvable"], True) + assert "hex" in addr_info + def run_test(self): self.master_node = self.nodes[0] self.old_node = self.nodes[1] @@ -1345,6 +1374,7 @@ class WalletMigrationTest(BitcoinTestFramework): self.test_disallowed_p2wsh() self.test_miniscript() self.test_taproot() + self.test_solvable_no_privs() if __name__ == '__main__': WalletMigrationTest(__file__).main()