Merge bitcoin/bitcoin#28602: descriptors: Disallow hybrid and uncompressed keys when inferring

74c77825e5ab68bfa575dad86444506c43ef6c06 test: Unit test for inferring scripts with hybrid and uncompressed keys (Andrew Chow)
f895f97014ac5fac46d27725c1ea7decf7ff76d4 test: Scripts with hybrid pubkeys are migrated to watchonly (Andrew Chow)
37b9b734770e855b9beff3b5085125f1420dd072 descriptors: Move InferScript's pubkey validity checks to InferPubkey (Andrew Chow)
b7485f11ab3a0f1860b261f222362af3301e0781 descriptors: Check result of InferPubkey (Andrew Chow)

Pull request description:

  `InferDescriptor` was not always checking that the pubkey it was placing into the descriptor was an allowed pubkey. For example, given a P2WPKH script that uses an uncompressed pubkey, it would produce a `wpkh()` with the uncompressed key. Additionally, the hybrid key check was only being done for `pk()` scripts, where it should've been done for all scripts.

  This PR moves the key checking into `InferPubkey`. If the key is not valid for the context, then `nullptr` is returned and the inferring will fall through to the defaults of either `raw()` or `addr()`.

  This also resolves an issue with migrating legacy wallets that contain hybrid pubkeys as such watchonly scripts will become `raw()` or `addr()` and go to the watchonly wallet. Note that a legacy wallet cannot sign for hybrid pubkeys. A test has been added for the migration case.

  Also added unit tests for `InferDescriptor` itself as the edge cases with that function are not covered by the descriptor roundtrip test.

ACKs for top commit:
  furszy:
    ACK 74c77825
  Sjors:
    utACK 74c77825e5ab68bfa575dad86444506c43ef6c06

Tree-SHA512: ed5f63e42a2e46120245a6b0288b90d2a6912860814c6c08fe393332add1cb364dc5eca72f16980352143570aef0c07bf1a91acd294099463bd028b6ce2fe40c

3 files changed, 157 insertions, 16 deletions

diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index ba2334df49..7e62d75583 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -1413,8 +1413,16 @@ std::unique_ptr<PubkeyProvider> ParsePubkey(uint32_t key_exp_index, const Span<c
     return std::make_unique<OriginPubkeyProvider>(key_exp_index, std::move(info), std::move(provider), apostrophe);
 }
 
-std::unique_ptr<PubkeyProvider> InferPubkey(const CPubKey& pubkey, ParseScriptContext, const SigningProvider& provider)
+std::unique_ptr<PubkeyProvider> InferPubkey(const CPubKey& pubkey, ParseScriptContext ctx, const SigningProvider& provider)
 {
+    // Key cannot be hybrid
+    if (!pubkey.IsValidNonHybrid()) {
+        return nullptr;
+    }
+    // Uncompressed is only allowed in TOP and P2SH contexts
+    if (ctx != ParseScriptContext::TOP && ctx != ParseScriptContext::P2SH && !pubkey.IsCompressed()) {
+        return nullptr;
+    }
     std::unique_ptr<PubkeyProvider> key_provider = std::make_unique<ConstPubkeyProvider>(0, pubkey, false);
     KeyOriginInfo info;
     if (provider.GetKeyOrigin(pubkey.GetID(), info)) {
@@ -1491,12 +1499,14 @@ struct KeyParser {
         if (miniscript::IsTapscript(m_script_ctx) && end - begin == 32) {
             XOnlyPubKey pubkey;
             std::copy(begin, end, pubkey.begin());
-            m_keys.push_back(InferPubkey(pubkey.GetEvenCorrespondingCPubKey(), ParseContext(), *m_in));
-            return key;
+            if (auto pubkey_provider = InferPubkey(pubkey.GetEvenCorrespondingCPubKey(), ParseContext(), *m_in)) {
+                m_keys.push_back(std::move(pubkey_provider));
+                return key;
+            }
         } else if (!miniscript::IsTapscript(m_script_ctx)) {
-            CPubKey pubkey{begin, end};
-            if (pubkey.IsValidNonHybrid()) {
-                m_keys.push_back(InferPubkey(pubkey, ParseContext(), *m_in));
+            CPubKey pubkey(begin, end);
+            if (auto pubkey_provider = InferPubkey(pubkey, ParseContext(), *m_in)) {
+                m_keys.push_back(std::move(pubkey_provider));
                 return key;
             }
         }
@@ -1512,9 +1522,11 @@ struct KeyParser {
         CKeyID keyid(hash);
         CPubKey pubkey;
         if (m_in->GetPubKey(keyid, pubkey)) {
-            Key key = m_keys.size();
-            m_keys.push_back(InferPubkey(pubkey, ParseContext(), *m_in));
-            return key;
+            if (auto pubkey_provider = InferPubkey(pubkey, ParseContext(), *m_in)) {
+                Key key = m_keys.size();
+                m_keys.push_back(std::move(pubkey_provider));
+                return key;
+            }
         }
         return {};
     }
@@ -1849,8 +1861,8 @@ std::unique_ptr<DescriptorImpl> InferScript(const CScript& script, ParseScriptCo
 
     if (txntype == TxoutType::PUBKEY && (ctx == ParseScriptContext::TOP || ctx == ParseScriptContext::P2SH || ctx == ParseScriptContext::P2WSH)) {
         CPubKey pubkey(data[0]);
-        if (pubkey.IsValidNonHybrid()) {
-            return std::make_unique<PKDescriptor>(InferPubkey(pubkey, ctx, provider));
+        if (auto pubkey_provider = InferPubkey(pubkey, ctx, provider)) {
+            return std::make_unique<PKDescriptor>(std::move(pubkey_provider));
         }
     }
     if (txntype == TxoutType::PUBKEYHASH && (ctx == ParseScriptContext::TOP || ctx == ParseScriptContext::P2SH || ctx == ParseScriptContext::P2WSH)) {
@@ -1858,7 +1870,9 @@ std::unique_ptr<DescriptorImpl> InferScript(const CScript& script, ParseScriptCo
         CKeyID keyid(hash);
         CPubKey pubkey;
         if (provider.GetPubKey(keyid, pubkey)) {
-            return std::make_unique<PKHDescriptor>(InferPubkey(pubkey, ctx, provider));
+            if (auto pubkey_provider = InferPubkey(pubkey, ctx, provider)) {
+                return std::make_unique<PKHDescriptor>(std::move(pubkey_provider));
+            }
         }
     }
     if (txntype == TxoutType::WITNESS_V0_KEYHASH && (ctx == ParseScriptContext::TOP || ctx == ParseScriptContext::P2SH)) {
@@ -1866,16 +1880,24 @@ std::unique_ptr<DescriptorImpl> InferScript(const CScript& script, ParseScriptCo
         CKeyID keyid(hash);
         CPubKey pubkey;
         if (provider.GetPubKey(keyid, pubkey)) {
-            return std::make_unique<WPKHDescriptor>(InferPubkey(pubkey, ctx, provider));
+            if (auto pubkey_provider = InferPubkey(pubkey, ParseScriptContext::P2WPKH, provider)) {
+                return std::make_unique<WPKHDescriptor>(std::move(pubkey_provider));
+            }
         }
     }
     if (txntype == TxoutType::MULTISIG && (ctx == ParseScriptContext::TOP || ctx == ParseScriptContext::P2SH || ctx == ParseScriptContext::P2WSH)) {
+        bool ok = true;
         std::vector<std::unique_ptr<PubkeyProvider>> providers;
         for (size_t i = 1; i + 1 < data.size(); ++i) {
             CPubKey pubkey(data[i]);
-            providers.push_back(InferPubkey(pubkey, ctx, provider));
+            if (auto pubkey_provider = InferPubkey(pubkey, ctx, provider)) {
+                providers.push_back(std::move(pubkey_provider));
+            } else {
+                ok = false;
+                break;
+            }
         }
-        return std::make_unique<MultisigDescriptor>((int)data[0][0], std::move(providers));
+        if (ok) return std::make_unique<MultisigDescriptor>((int)data[0][0], std::move(providers));
     }
     if (txntype == TxoutType::SCRIPTHASH && ctx == ParseScriptContext::TOP) {
         uint160 hash(data[0]);
diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp
index 9b48c48ff8..f4f4e39f40 100644
--- a/src/test/descriptor_tests.cpp
+++ b/src/test/descriptor_tests.cpp
@@ -384,6 +384,54 @@ void Check(const std::string& prv, const std::string& pub, const std::string& no
     }
 }
 
+void CheckInferDescriptor(const std::string& script_hex, const std::string& expected_desc, const std::vector<std::string>& hex_scripts, const std::vector<std::pair<std::string, std::string>>& origin_pubkeys)
+{
+    std::vector<unsigned char> script_bytes{ParseHex(script_hex)};
+    const CScript& script{script_bytes.begin(), script_bytes.end()};
+
+    FlatSigningProvider provider;
+    for (const std::string& prov_script_hex : hex_scripts) {
+        std::vector<unsigned char> prov_script_bytes{ParseHex(prov_script_hex)};
+        const CScript& prov_script{prov_script_bytes.begin(), prov_script_bytes.end()};
+        provider.scripts.emplace(CScriptID(prov_script), prov_script);
+    }
+    for (const auto& [pubkey_hex, origin_str] : origin_pubkeys) {
+        CPubKey origin_pubkey{ParseHex(pubkey_hex)};
+        provider.pubkeys.emplace(origin_pubkey.GetID(), origin_pubkey);
+
+        if (!origin_str.empty()) {
+            using namespace spanparsing;
+            KeyOriginInfo info;
+            Span<const char> origin_sp{origin_str};
+            std::vector<Span<const char>> origin_split = Split(origin_sp, "/");
+            std::string fpr_str(origin_split[0].begin(), origin_split[0].end());
+            auto fpr_bytes = ParseHex(fpr_str);
+            std::copy(fpr_bytes.begin(), fpr_bytes.end(), info.fingerprint);
+            for (size_t i = 1; i < origin_split.size(); ++i) {
+                Span<const char> elem = origin_split[i];
+                bool hardened = false;
+                if (elem.size() > 0) {
+                    const char last = elem[elem.size() - 1];
+                    if (last == '\'' || last == 'h') {
+                        elem = elem.first(elem.size() - 1);
+                        hardened = true;
+                    }
+                }
+                uint32_t p;
+                assert(ParseUInt32(std::string(elem.begin(), elem.end()), &p));
+                info.path.push_back(p | (((uint32_t)hardened) << 31));
+            }
+
+            provider.origins.emplace(origin_pubkey.GetID(), std::make_pair(origin_pubkey, info));
+        }
+    }
+
+    std::string checksum{GetDescriptorChecksum(expected_desc)};
+
+    std::unique_ptr<Descriptor> desc = InferDescriptor(script, provider);
+    BOOST_CHECK_EQUAL(desc->ToString(), expected_desc + "#" + checksum);
+}
+
 }
 
 BOOST_FIXTURE_TEST_SUITE(descriptor_tests, BasicTestingSetup)
@@ -594,6 +642,19 @@ BOOST_AUTO_TEST_CASE(descriptor_test)
     // preimages and the sequence, passes with.)
     Check("tr(a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,{and_v(and_v(v:hash256(ae253ca2a54debcac7ecf414f6734f48c56421a08bb59182ff9f39a6fffdb588),v:pk(KykUPmR5967F4URzMUeCv9kNMU9CNRWycrPmx3ZvfkWoQLabbimL)),older(42)),multi_a(2,adf586a32ad4b0674a86022b000348b681b4c97a811f67eefe4a6e066e55080c,KztMyyi1pXUtuZfJSB7JzVdmJMAz7wfGVFoSRUR5CVZxXxULXuGR)})", "tr(a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,{and_v(and_v(v:hash256(ae253ca2a54debcac7ecf414f6734f48c56421a08bb59182ff9f39a6fffdb588),v:pk(1c9bc926084382e76da33b5a52d17b1fa153c072aae5fb5228ecc2ccf89d79d5)),older(42)),multi_a(2,adf586a32ad4b0674a86022b000348b681b4c97a811f67eefe4a6e066e55080c,14fa4ad085cdee1e2fc73d491b36a96c192382b1d9a21108eb3533f630364f9f)})", "tr(a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,{and_v(and_v(v:hash256(ae253ca2a54debcac7ecf414f6734f48c56421a08bb59182ff9f39a6fffdb588),v:pk(1c9bc926084382e76da33b5a52d17b1fa153c072aae5fb5228ecc2ccf89d79d5)),older(42)),multi_a(2,adf586a32ad4b0674a86022b000348b681b4c97a811f67eefe4a6e066e55080c,14fa4ad085cdee1e2fc73d491b36a96c192382b1d9a21108eb3533f630364f9f)})", MISSING_PRIVKEYS | XONLY_KEYS | SIGNABLE | SIGNABLE_FAILS, {{"51209a3d79db56fbe3ba4d905d827b62e1ed31cd6df1198b8c759d589c0f4efc27bd"}}, OutputType::BECH32M);
     Check("tr(a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,{and_v(and_v(v:hash256(ae253ca2a54debcac7ecf414f6734f48c56421a08bb59182ff9f39a6fffdb588),v:pk(KykUPmR5967F4URzMUeCv9kNMU9CNRWycrPmx3ZvfkWoQLabbimL)),older(42)),multi_a(2,adf586a32ad4b0674a86022b000348b681b4c97a811f67eefe4a6e066e55080c,KztMyyi1pXUtuZfJSB7JzVdmJMAz7wfGVFoSRUR5CVZxXxULXuGR)})", "tr(a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,{and_v(and_v(v:hash256(ae253ca2a54debcac7ecf414f6734f48c56421a08bb59182ff9f39a6fffdb588),v:pk(1c9bc926084382e76da33b5a52d17b1fa153c072aae5fb5228ecc2ccf89d79d5)),older(42)),multi_a(2,adf586a32ad4b0674a86022b000348b681b4c97a811f67eefe4a6e066e55080c,14fa4ad085cdee1e2fc73d491b36a96c192382b1d9a21108eb3533f630364f9f)})", "tr(a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,{and_v(and_v(v:hash256(ae253ca2a54debcac7ecf414f6734f48c56421a08bb59182ff9f39a6fffdb588),v:pk(1c9bc926084382e76da33b5a52d17b1fa153c072aae5fb5228ecc2ccf89d79d5)),older(42)),multi_a(2,adf586a32ad4b0674a86022b000348b681b4c97a811f67eefe4a6e066e55080c,14fa4ad085cdee1e2fc73d491b36a96c192382b1d9a21108eb3533f630364f9f)})", MISSING_PRIVKEYS | XONLY_KEYS | SIGNABLE, {{"51209a3d79db56fbe3ba4d905d827b62e1ed31cd6df1198b8c759d589c0f4efc27bd"}}, OutputType::BECH32M, /*op_desc_id=*/{}, {{}}, /*spender_nlocktime=*/0, /*spender_nsequence=*/42, /*preimages=*/{{ParseHex("ae253ca2a54debcac7ecf414f6734f48c56421a08bb59182ff9f39a6fffdb588"), ParseHex("000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f")}});
+
+    // Basic sh(pkh()) with key origin
+    CheckInferDescriptor("a9141a31ad23bf49c247dd531a623c2ef57da3c400c587", "sh(pkh([deadbeef/0h/0h/0]03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd))", {"76a9149a1c78a507689f6f54b847ad1cef1e614ee23f1e88ac"}, {{"03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd", "deadbeef/0h/0h/0"}});
+    // p2pk script with hybrid key must infer as raw()
+    CheckInferDescriptor("41069228de6902abb4f541791f6d7f925b10e2078ccb1298856e5ea5cc5fd667f930eac37a00cc07f9a91ef3c2d17bf7a17db04552ff90ac312a5b8b4caca6c97aa4ac", "raw(41069228de6902abb4f541791f6d7f925b10e2078ccb1298856e5ea5cc5fd667f930eac37a00cc07f9a91ef3c2d17bf7a17db04552ff90ac312a5b8b4caca6c97aa4ac)", {}, {{"069228de6902abb4f541791f6d7f925b10e2078ccb1298856e5ea5cc5fd667f930eac37a00cc07f9a91ef3c2d17bf7a17db04552ff90ac312a5b8b4caca6c97aa4", ""}});
+    // p2pkh script with hybrid key must infer as addr()
+    CheckInferDescriptor("76a91445ff7c2327866472639d507334a9a00119dfd32688ac", "addr(17P7ge56F2QcdHxxRBa2NyzmejFggPwBJ9)", {}, {{"069228de6902abb4f541791f6d7f925b10e2078ccb1298856e5ea5cc5fd667f930eac37a00cc07f9a91ef3c2d17bf7a17db04552ff90ac312a5b8b4caca6c97aa4", ""}});
+    // p2wpkh script with uncompressed key must infer as addr()
+    CheckInferDescriptor("001422e363a523947a110d9a9eb114820de183aca313", "addr(bc1qyt3k8ffrj3apzrv6n6c3fqsduxp6egcnk2r66j)", {}, {{"049228de6902abb4f541791f6d7f925b10e2078ccb1298856e5ea5cc5fd667f930eac37a00cc07f9a91ef3c2d17bf7a17db04552ff90ac312a5b8b4caca6c97aa4", ""}});
+    // Infer pkh() from p2pkh with uncompressed key
+    CheckInferDescriptor("76a914a31725c74421fadc50d35520ab8751ed120af80588ac", "pkh(04c56fe4a92d401bcbf1b3dfbe4ac3dac5602ca155a3681497f02c1b9a733b92d704e2da6ec4162e4846af9236ef4171069ac8b7f8234a8405b6cadd96f34f5a31)", {}, {{"04c56fe4a92d401bcbf1b3dfbe4ac3dac5602ca155a3681497f02c1b9a733b92d704e2da6ec4162e4846af9236ef4171069ac8b7f8234a8405b6cadd96f34f5a31", ""}});
+    // Infer pk() from p2pk with uncompressed key
+    CheckInferDescriptor("4104032540df1d3c7070a8ab3a9cdd304dfc7fd1e6541369c53c4c3310b2537d91059afc8b8e7673eb812a32978dabb78c40f2e423f7757dca61d11838c7aeeb5220ac", "pk(04032540df1d3c7070a8ab3a9cdd304dfc7fd1e6541369c53c4c3310b2537d91059afc8b8e7673eb812a32978dabb78c40f2e423f7757dca61d11838c7aeeb5220)", {}, {{"04032540df1d3c7070a8ab3a9cdd304dfc7fd1e6541369c53c4c3310b2537d91059afc8b8e7673eb812a32978dabb78c40f2e423f7757dca61d11838c7aeeb5220", ""}});
 }
 
 BOOST_AUTO_TEST_SUITE_END()
diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
index bcd71197bf..286dcb5fda 100755
--- a/test/functional/wallet_migration.py
+++ b/test/functional/wallet_migration.py
@@ -6,8 +6,13 @@
 
 import random
 import shutil
-from test_framework.address import script_to_p2sh
+from test_framework.address import (
+    script_to_p2sh,
+    key_to_p2pkh,
+    key_to_p2wpkh,
+)
 from test_framework.descriptors import descsum_create
+from test_framework.key import ECPubKey
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.messages import COIN, CTransaction, CTxOut
 from test_framework.script_util import key_to_p2pkh_script, script_to_p2sh_script, script_to_p2wsh_script
@@ -770,6 +775,58 @@ class WalletMigrationTest(BitcoinTestFramework):
 
         wallet.unloadwallet()
 
+    def test_hybrid_pubkey(self):
+        self.log.info("Test migration when wallet contains a hybrid pubkey")
+
+        wallet = self.create_legacy_wallet("hybrid_keys")
+
+        # Get the hybrid pubkey for one of the keys in the wallet
+        normal_pubkey = wallet.getaddressinfo(wallet.getnewaddress())["pubkey"]
+        first_byte = bytes.fromhex(normal_pubkey)[0] + 4 # Get the hybrid pubkey first byte
+        parsed_pubkey = ECPubKey()
+        parsed_pubkey.set(bytes.fromhex(normal_pubkey))
+        parsed_pubkey.compressed = False
+        hybrid_pubkey_bytes = bytearray(parsed_pubkey.get_bytes())
+        hybrid_pubkey_bytes[0] = first_byte # Make it hybrid
+        hybrid_pubkey = hybrid_pubkey_bytes.hex()
+
+        # Import the hybrid pubkey
+        wallet.importpubkey(hybrid_pubkey)
+        p2pkh_addr = key_to_p2pkh(hybrid_pubkey)
+        p2pkh_addr_info = wallet.getaddressinfo(p2pkh_addr)
+        assert_equal(p2pkh_addr_info["iswatchonly"], True)
+        assert_equal(p2pkh_addr_info["ismine"], False) # Things involving hybrid pubkeys are not spendable
+
+        # Also import the p2wpkh for the pubkey to make sure we don't migrate it
+        p2wpkh_addr = key_to_p2wpkh(hybrid_pubkey)
+        wallet.importaddress(p2wpkh_addr)
+
+        migrate_info = wallet.migratewallet()
+
+        # Both addresses should only appear in the watchonly wallet
+        p2pkh_addr_info = wallet.getaddressinfo(p2pkh_addr)
+        assert_equal(p2pkh_addr_info["iswatchonly"], False)
+        assert_equal(p2pkh_addr_info["ismine"], False)
+        p2wpkh_addr_info = wallet.getaddressinfo(p2wpkh_addr)
+        assert_equal(p2wpkh_addr_info["iswatchonly"], False)
+        assert_equal(p2wpkh_addr_info["ismine"], False)
+
+        watchonly_wallet = self.nodes[0].get_wallet_rpc(migrate_info["watchonly_name"])
+        watchonly_p2pkh_addr_info = watchonly_wallet.getaddressinfo(p2pkh_addr)
+        assert_equal(watchonly_p2pkh_addr_info["iswatchonly"], False)
+        assert_equal(watchonly_p2pkh_addr_info["ismine"], True)
+        watchonly_p2wpkh_addr_info = watchonly_wallet.getaddressinfo(p2wpkh_addr)
+        assert_equal(watchonly_p2wpkh_addr_info["iswatchonly"], False)
+        assert_equal(watchonly_p2wpkh_addr_info["ismine"], True)
+
+        # There should only be raw or addr descriptors
+        for desc in watchonly_wallet.listdescriptors()["descriptors"]:
+            if desc["desc"].startswith("raw(") or desc["desc"].startswith("addr("):
+                continue
+            assert False, "Hybrid pubkey watchonly wallet has more than just raw() and addr()"
+
+        wallet.unloadwallet()
+
     def run_test(self):
         self.generate(self.nodes[0], 101)
 
@@ -787,6 +844,7 @@ class WalletMigrationTest(BitcoinTestFramework):
         self.test_addressbook()
         self.test_migrate_raw_p2sh()
         self.test_conflict_txs()
+        self.test_hybrid_pubkey()
 
 if __name__ == '__main__':
     WalletMigrationTest().main()