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
This commit is contained in:
fanquake 2023-10-11 12:37:16 +02:00
commit 744157ef1a
No known key found for this signature in database
GPG Key ID: 2EEB9F5CC09526C1
3 changed files with 157 additions and 16 deletions

View File

@ -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]);

View File

@ -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()

View File

@ -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()