descriptors: Disallow hybrid and uncompressed keys when inferring #28602

pull achow101 wants to merge 4 commits into bitcoin:master from achow101:migrate-hybrid-keys changing 3 files +157 −16
  1. achow101 commented at 6:01 pm on October 5, 2023: member

    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.

  2. DrahtBot commented at 6:01 pm on October 5, 2023: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK furszy, Sjors
    Stale ACK sipa

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #28610 (wallet: Migrate entire address book entries to watchonly and solvables too by achow101)
    • #28609 (wallet: Reload watchonly and solvables wallets after migration by achow101)
    • #22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. achow101 added this to the milestone 26.0 on Oct 5, 2023
  4. DrahtBot added the label Descriptors on Oct 5, 2023
  5. fanquake requested review from darosior on Oct 5, 2023
  6. fanquake requested review from sipa on Oct 5, 2023
  7. in src/script/descriptor.cpp:1465 in 6c80e82b76 outdated 
    1461@@ -1454,8 +1462,11 @@ struct KeyParser {
1462         CPubKey pubkey(begin, end);
1463         if (pubkey.IsValidNonHybrid()) {
1464             Key key = m_keys.size();
1465-            m_keys.push_back(InferPubkey(pubkey, ParseScriptContext::P2WSH, *m_in));
1466-            return key;
1467+            std::unique_ptr<PubkeyProvider> pubkey_provider = InferPubkey(pubkey, ParseScriptContext::P2WSH, *m_in);
    sipa commented at 7:34 pm on October 5, 2023:

    More concisely:

    0if (auto pubkey_provider = InferPubkey(pubkey, ParseScriptContext::P2WSH, *m_in)) {
1    m_keys.push_back(std::move(pubkey_provider));
2    return key;
3}
    achow101 commented at 7:55 pm on October 5, 2023:
    Used the first suggestion. The second does not compile, pubkey_provider is out of scope.
  8. in src/test/descriptor_tests.cpp:394 in 6c80e82b76 outdated 
    389+    std::vector<unsigned char> script_bytes{ParseHex(script_hex)};
390+    const CScript& script{script_bytes.begin(), script_bytes.end()};
391+
392+    FlatSigningProvider provider;
393+    for (const std::string& prov_script_hex : hex_scripts) {
394+    std::vector<unsigned char> prov_script_bytes{ParseHex(prov_script_hex)};
    sipa commented at 7:52 pm on October 5, 2023:
    Indentation?
    achow101 commented at 7:55 pm on October 5, 2023:
    Fixed
  9. achow101 force-pushed on Oct 5, 2023
  10. DrahtBot added the label CI failed on Oct 5, 2023
  11. in src/script/descriptor.cpp:1841 in 25a8a173ad outdated 
    1841+        bool ok = true;
1842         std::vector<std::unique_ptr<PubkeyProvider>> providers;
1843         for (size_t i = 1; i + 1 < data.size(); ++i) {
1844             CPubKey pubkey(data[i]);
1845-            providers.push_back(InferPubkey(pubkey, ctx, provider));
1846+            if(auto pubkey_provider = InferPubkey(pubkey, ctx, provider)) {
    sipa commented at 7:59 pm on October 5, 2023:
    Nit: space after if
    achow101 commented at 10:36 pm on October 5, 2023:
    Done
  12. in src/test/descriptor_tests.cpp:405 in 25a8a173ad outdated 
    400+        provider.pubkeys.emplace(prov_pubkey.GetID(), prov_pubkey);
401+    }
402+    for (const auto& [pubkey_hex, origin_str] : str_origins) {
403+        CPubKey origin_pubkey{ParseHex(pubkey_hex)};
404+
405+        using namespace spanparsing;
    sipa commented at 8:10 pm on October 5, 2023:

    The body of this loop looks like it’s duplicating code from existing key parsing code.

    Would it be possible to e.g. construct a [origin]pubkey string, pass that to ParsePubkey, and use the resulting FlatSigningProvider& out?

    achow101 commented at 10:37 pm on October 5, 2023:
    Not without a lot of refactoring that ends up exposing several descriptor parsing internals.
  13. in src/script/descriptor.cpp:1509 in 3e718b49e2 outdated 
    1453@@ -1454,8 +1454,10 @@ struct KeyParser {
1454         CPubKey pubkey(begin, end);
1455         if (pubkey.IsValidNonHybrid()) {
1456             Key key = m_keys.size();
1457-            m_keys.push_back(InferPubkey(pubkey, ParseScriptContext::P2WSH, *m_in));
1458-            return key;
1459+            if (auto pubkey_provider = InferPubkey(pubkey, ParseScriptContext::P2WSH, *m_in)) {
1460+                m_keys.push_back(std::move(pubkey_provider));
    furszy commented at 10:16 pm on October 5, 2023:

    tiny nit: Key key = m_keys.size(); can be inside the InferPubkey block.

    Same for FromPKHBytes().

    achow101 commented at 10:46 pm on October 6, 2023:
    Done
  14. in test/functional/wallet_migration.py:800 in 12f0d05775 outdated 
    795+        p2pkh_addr = key_to_p2pkh(hybrid_pubkey)
796+        p2pkh_addr_info = wallet.getaddressinfo(p2pkh_addr)
797+        assert_equal(p2pkh_addr_info["iswatchonly"], True)
798+        assert_equal(p2pkh_addr_info["ismine"], False) # Things involving hybrid pubkeys are not spendable
799+
800+        # Also import the p2pkh for the pubkey to make sure we don't migrate it
    furszy commented at 10:35 pm on October 5, 2023:
    s/p2pkh/p2wpkh
    achow101 commented at 10:46 pm on October 6, 2023:
    Done
  15. achow101 force-pushed on Oct 5, 2023
  16. DrahtBot removed the label CI failed on Oct 6, 2023
  17. in src/test/descriptor_tests.cpp:653 in 6f00af4bcb outdated 
    629@@ -582,6 +630,11 @@ BOOST_AUTO_TEST_CASE(descriptor_test)
630     // Same for hash256
631     Check("wsh(and_v(v:hash256(ae253ca2a54debcac7ecf414f6734f48c56421a08bb59182ff9f39a6fffdb588),pk(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc)))", "wsh(and_v(v:hash256(ae253ca2a54debcac7ecf414f6734f48c56421a08bb59182ff9f39a6fffdb588),pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL)))", "wsh(and_v(v:hash256(ae253ca2a54debcac7ecf414f6734f48c56421a08bb59182ff9f39a6fffdb588),pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL)))", SIGNABLE_FAILS, {{"0020cf62bf97baf977aec69cbc290c372899f913337a9093e8f066ab59b8657a365c"}}, OutputType::BECH32, /*op_desc_id=*/uint256S("8412ba3ac20ba3a30f81442d10d32e0468fa52814960d04e959bf84a9b813b88"), {{}}, /*spender_nlocktime=*/0, /*spender_nsequence=*/CTxIn::SEQUENCE_FINAL, {});
632     Check("wsh(and_v(v:hash256(ae253ca2a54debcac7ecf414f6734f48c56421a08bb59182ff9f39a6fffdb588),pk(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc)))", "wsh(and_v(v:hash256(ae253ca2a54debcac7ecf414f6734f48c56421a08bb59182ff9f39a6fffdb588),pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL)))", "wsh(and_v(v:hash256(ae253ca2a54debcac7ecf414f6734f48c56421a08bb59182ff9f39a6fffdb588),pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL)))", SIGNABLE, {{"0020cf62bf97baf977aec69cbc290c372899f913337a9093e8f066ab59b8657a365c"}}, OutputType::BECH32, /*op_desc_id=*/uint256S("8412ba3ac20ba3a30f81442d10d32e0468fa52814960d04e959bf84a9b813b88"), {{}}, /*spender_nlocktime=*/0, /*spender_nsequence=*/CTxIn::SEQUENCE_FINAL, {{ParseHex("ae253ca2a54debcac7ecf414f6734f48c56421a08bb59182ff9f39a6fffdb588"), ParseHex("000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f")}});
633+
634+    CheckInferDescriptor("a9141a31ad23bf49c247dd531a623c2ef57da3c400c587", "sh(pkh([deadbeef/0h/0h/0]03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd))", {"76a9149a1c78a507689f6f54b847ad1cef1e614ee23f1e88ac"}, {{"03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd", "deadbeef/0h/0h/0"}});
635+    CheckInferDescriptor("41069228de6902abb4f541791f6d7f925b10e2078ccb1298856e5ea5cc5fd667f930eac37a00cc07f9a91ef3c2d17bf7a17db04552ff90ac312a5b8b4caca6c97aa4ac", "raw(41069228de6902abb4f541791f6d7f925b10e2078ccb1298856e5ea5cc5fd667f930eac37a00cc07f9a91ef3c2d17bf7a17db04552ff90ac312a5b8b4caca6c97aa4ac)", {}, {{"069228de6902abb4f541791f6d7f925b10e2078ccb1298856e5ea5cc5fd667f930eac37a00cc07f9a91ef3c2d17bf7a17db04552ff90ac312a5b8b4caca6c97aa4", ""}});
636+    CheckInferDescriptor("76a91445ff7c2327866472639d507334a9a00119dfd32688ac", "addr(17P7ge56F2QcdHxxRBa2NyzmejFggPwBJ9)", {}, {{"069228de6902abb4f541791f6d7f925b10e2078ccb1298856e5ea5cc5fd667f930eac37a00cc07f9a91ef3c2d17bf7a17db04552ff90ac312a5b8b4caca6c97aa4", ""}});
637+    CheckInferDescriptor("001422e363a523947a110d9a9eb114820de183aca313", "addr(bc1qyt3k8ffrj3apzrv6n6c3fqsduxp6egcnk2r66j)", {}, {{"049228de6902abb4f541791f6d7f925b10e2078ccb1298856e5ea5cc5fd667f930eac37a00cc07f9a91ef3c2d17bf7a17db04552ff90ac312a5b8b4caca6c97aa4", ""}});
    furszy commented at 7:20 pm on October 6, 2023:

    For better maintainability, comments explaining what we are about to test on each case would be nice. Maybe:

    0// Infer valid sh(pkh(compressed_key))
1CheckInferDescriptor("a9141a31ad23bf49c247dd531a623c2ef57da3c400c587", "sh(pkh([deadbeef/0h/0h/0]03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd))", {"76a9149a1c78a507689f6f54b847ad1cef1e614ee23f1e88ac"}, {{"03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd", "deadbeef/0h/0h/0"}});
2// Only can watch an hybrid pk in a P2PK
3CheckInferDescriptor("41069228de6902abb4f541791f6d7f925b10e2078ccb1298856e5ea5cc5fd667f930eac37a00cc07f9a91ef3c2d17bf7a17db04552ff90ac312a5b8b4caca6c97aa4ac", "raw(41069228de6902abb4f541791f6d7f925b10e2078ccb1298856e5ea5cc5fd667f930eac37a00cc07f9a91ef3c2d17bf7a17db04552ff90ac312a5b8b4caca6c97aa4ac)", {}, {{"069228de6902abb4f541791f6d7f925b10e2078ccb1298856e5ea5cc5fd667f930eac37a00cc07f9a91ef3c2d17bf7a17db04552ff90ac312a5b8b4caca6c97aa4", ""}});
4// Only can watch an hybrid pk in a P2PKH
5CheckInferDescriptor("76a91445ff7c2327866472639d507334a9a00119dfd32688ac", "addr(17P7ge56F2QcdHxxRBa2NyzmejFggPwBJ9)", {}, {{"069228de6902abb4f541791f6d7f925b10e2078ccb1298856e5ea5cc5fd667f930eac37a00cc07f9a91ef3c2d17bf7a17db04552ff90ac312a5b8b4caca6c97aa4", ""}});
6// Only can watch an uncompressed pk in a P2WPKH
7CheckInferDescriptor("001422e363a523947a110d9a9eb114820de183aca313", "addr(bc1qyt3k8ffrj3apzrv6n6c3fqsduxp6egcnk2r66j)", {}, {{"049228de6902abb4f541791f6d7f925b10e2078ccb1298856e5ea5cc5fd667f930eac37a00cc07f9a91ef3c2d17bf7a17db04552ff90ac312a5b8b4caca6c97aa4", ""}});
    furszy commented at 7:56 pm on October 6, 2023:

    Could also verify the inference process for pkh(uncompressed) and pk(uncompressed):

    0// Infer valid pkh(uncompressed_key)
1CheckInferDescriptor("76a914a31725c74421fadc50d35520ab8751ed120af80588ac", "pkh(04c56fe4a92d401bcbf1b3dfbe4ac3dac5602ca155a3681497f02c1b9a733b92d704e2da6ec4162e4846af9236ef4171069ac8b7f8234a8405b6cadd96f34f5a31)", {}, {{"04c56fe4a92d401bcbf1b3dfbe4ac3dac5602ca155a3681497f02c1b9a733b92d704e2da6ec4162e4846af9236ef4171069ac8b7f8234a8405b6cadd96f34f5a31", ""}});
2// Infer valid pk(uncompressed_key)
3CheckInferDescriptor("4104032540df1d3c7070a8ab3a9cdd304dfc7fd1e6541369c53c4c3310b2537d91059afc8b8e7673eb812a32978dabb78c40f2e423f7757dca61d11838c7aeeb5220ac", "pk(04032540df1d3c7070a8ab3a9cdd304dfc7fd1e6541369c53c4c3310b2537d91059afc8b8e7673eb812a32978dabb78c40f2e423f7757dca61d11838c7aeeb5220)#59dvf0e2", {}, {{"04032540df1d3c7070a8ab3a9cdd304dfc7fd1e6541369c53c4c3310b2537d91059afc8b8e7673eb812a32978dabb78c40f2e423f7757dca61d11838c7aeeb5220", ""}});
    achow101 commented at 10:46 pm on October 6, 2023:
    Added some comments.
    achow101 commented at 10:46 pm on October 6, 2023:
    Done
  18. furszy commented at 7:57 pm on October 6, 2023: member
    Code review ACK 6f00af4bc, left few nits, nothing blocking.
  19. achow101 force-pushed on Oct 6, 2023
  20. achow101 force-pushed on Oct 6, 2023
  21. DrahtBot added the label Needs rebase on Oct 8, 2023
  22. achow101 force-pushed on Oct 8, 2023
  23. sipa commented at 6:34 pm on October 8, 2023: member
    utACK a49081f6cea603d354beef1edff480d4b034dafd
  24. DrahtBot requested review from furszy on Oct 8, 2023
  25. DrahtBot removed the label Needs rebase on Oct 8, 2023
  26. in src/script/descriptor.cpp:1503 in fb3c5f3e9f outdated 
    1490@@ -1491,13 +1491,17 @@ struct KeyParser {
1491         if (miniscript::IsTapscript(m_script_ctx) && end - begin == 32) {
1492             XOnlyPubKey pubkey;
1493             std::copy(begin, end, pubkey.begin());
1494-            m_keys.push_back(InferPubkey(pubkey.GetEvenCorrespondingCPubKey(), ParseContext(), *m_in));
1495+            if (auto pubkey_provider = InferPubkey(pubkey.GetEvenCorrespondingCPubKey(), ParseContext(), *m_in)) {
1496+                m_keys.push_back(std::move(pubkey_provider));
    Sjors commented at 7:09 am on October 9, 2023:

    fb3c5f3e9fb17e2313a6f2aec6db49f3e0da8614 why is return key outside the if context here, but in the below check it’s inside?

    Wouldn’t that cause it to return 0 instead of {}?

    achow101 commented at 6:07 pm on October 9, 2023:
    Fixed
  27. in src/script/descriptor.cpp:1883 in 62ca75bd99 outdated 
    1879@@ -1876,7 +1880,7 @@ std::unique_ptr<DescriptorImpl> InferScript(const CScript& script, ParseScriptCo
1880         CKeyID keyid(hash);
1881         CPubKey pubkey;
1882         if (provider.GetPubKey(keyid, pubkey)) {
1883-            if (auto pubkey_provider = InferPubkey(pubkey, ctx, provider)) {
1884+            if (auto pubkey_provider = InferPubkey(pubkey, ParseScriptContext::P2WPKH, provider)) {
    Sjors commented at 7:20 am on October 9, 2023:
    62ca75bd99b42f686ed1879481b856c882b40025: what’s wrong with passing ctx here?
    achow101 commented at 6:09 pm on October 9, 2023:
    The context for the pubkey inferring needs to be P2WPKH. ctx won’t ever be that, instead it will either be TOP or P2SH, both of which allow uncompressed pubkeys, but P2WPKH does not. So the context needs to be updated in order to enforce that.
  28. Sjors commented at 7:26 am on October 9, 2023: member

    Concept ACK

    Mostly happy with a49081f6cea603d354beef1edff480d4b034dafd

  29. DrahtBot requested review from Sjors on Oct 9, 2023
  30. Sjors commented at 7:27 am on October 9, 2023: member
    @DrahtBot you’re drunk, I just reviewed it.
  31. maflcko removed review request from Sjors on Oct 9, 2023
  32. maflcko commented at 10:21 am on October 9, 2023: member

    Thanks, fixed in https://github.com/maflcko/DrahtBot/commit/f8d7c68cda2c9eb2cf147666f28c4bd96d735f81

    You’ll be re-requested to review at a later point in time.

  33. descriptors: Check result of InferPubkey 
    InferPubkey can return a nullptr, so check it's result before continuing
with creating the inferred descriptor.
    b7485f11ab
  34. descriptors: Move InferScript's pubkey validity checks to InferPubkey 37b9b73477
  35. test: Scripts with hybrid pubkeys are migrated to watchonly 
    Descriptors disallows hybrid pubkeys. Anything with hybrid pubkeys
should becomes a raw() descriptor that shows up in the watchonly wallet.
    f895f97014
  36. test: Unit test for inferring scripts with hybrid and uncompressed keys 74c77825e5
  37. achow101 force-pushed on Oct 9, 2023
  38. in src/script/descriptor.cpp:1896 in b7485f11ab outdated 
    1896+                ok = false;
1897+                break;
1898+            }
1899         }
1900-        return std::make_unique<MultisigDescriptor>((int)data[0][0], std::move(providers));
1901+        if (ok) return std::make_unique<MultisigDescriptor>((int)data[0][0], std::move(providers));
    furszy commented at 10:21 pm on October 9, 2023:

    just a coding style nit. No need to implement it if you don’t like it.

    0std::vector<std::unique_ptr<PubkeyProvider>> providers;
1for (size_t i = 1; i + 1 < data.size(); ++i) {
2   CPubKey pubkey(data[i]);
3   auto pubkey_provider = InferPubkey(pubkey, ctx, provider);
4   if (!pubkey_provider) break;
5   providers.push_back(std::move(pubkey_provider));
6}
7size_t expected_num = data.size() - 2; // First position is the min number of signers, and the last one is the op_checkmultisig
8if (providers.size() == expected_num) return std::make_unique<MultisigDescriptor>((int)data[0][0], std::move(providers));
  39. in test/functional/wallet_migration.py:828 in f895f97014 outdated 
    823+        for desc in watchonly_wallet.listdescriptors()["descriptors"]:
824+            if desc["desc"].startswith("raw(") or desc["desc"].startswith("addr("):
825+                continue
826+            assert False, "Hybrid pubkey watchonly wallet has more than just raw() and addr()"
827+
828+        wallet.unloadwallet()
    furszy commented at 10:32 pm on October 9, 2023:
    In f895f970: would be good to also unload the watchonly_wallet wallet.
  40. furszy commented at 10:33 pm on October 9, 2023: member
    ACK 74c77825
  41. DrahtBot requested review from sipa on Oct 9, 2023
  42. DrahtBot requested review from Sjors on Oct 9, 2023
  43. Sjors approved
  44. Sjors commented at 7:32 am on October 11, 2023: member
    utACK 74c77825e5ab68bfa575dad86444506c43ef6c06
  45. fanquake merged this on Oct 11, 2023
  46. fanquake closed this on Oct 11, 2023
  47. Frank-GER referenced this in commit 2a48d53f31 on Oct 13, 2023
  48. luke-jr referenced this in commit cc6d6f301b on Oct 18, 2023
  49. luke-jr referenced this in commit 549401a089 on Oct 18, 2023
  50. luke-jr referenced this in commit 3ca5d9b6ef on Oct 18, 2023
  51. luke-jr referenced this in commit 98208a813c on Oct 18, 2023
  52. bitcoin locked this on Oct 10, 2024


achow101 DrahtBot sipa furszy Sjors maflcko


sipa darosior

Labels
Descriptors

Milestone
26.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-21 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me