descriptors: Disallow hybrid and uncompressed keys when inferring #28602

achow101 · 2023-10-05T18:01:50Z

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.

DrahtBot · 2023-10-05T18:01:52Z

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.

src/script/descriptor.cpp

src/test/descriptor_tests.cpp

src/script/descriptor.cpp

sipa · 2023-10-05T20:10:23Z

src/test/descriptor_tests.cpp

+    for (const auto& [pubkey_hex, origin_str] : str_origins) {
+        CPubKey origin_pubkey{ParseHex(pubkey_hex)};
+
+        using namespace spanparsing;


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?

Not without a lot of refactoring that ends up exposing several descriptor parsing internals.

furszy

Code review ACK 6f00af4, left few nits, nothing blocking.

src/script/descriptor.cpp

test/functional/wallet_migration.py

src/test/descriptor_tests.cpp

sipa · 2023-10-08T18:34:51Z

utACK a49081f

Sjors

Concept ACK

Mostly happy with a49081f

src/script/descriptor.cpp

Sjors · 2023-10-09T07:20:54Z

src/script/descriptor.cpp

@@ -1876,7 +1880,7 @@ std::unique_ptr<DescriptorImpl> InferScript(const CScript& script, ParseScriptCo
        CKeyID keyid(hash);
        CPubKey pubkey;
        if (provider.GetPubKey(keyid, pubkey)) {
-            if (auto pubkey_provider = InferPubkey(pubkey, ctx, provider)) {
+            if (auto pubkey_provider = InferPubkey(pubkey, ParseScriptContext::P2WPKH, provider)) {


62ca75b: what's wrong with passing ctx here?

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.

Sjors · 2023-10-09T07:27:41Z

@DrahtBot you're drunk, I just reviewed it.

maflcko · 2023-10-09T10:21:36Z

Thanks, fixed in maflcko/DrahtBot@f8d7c68

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

InferPubkey can return a nullptr, so check it's result before continuing with creating the inferred descriptor.

Descriptors disallows hybrid pubkeys. Anything with hybrid pubkeys should becomes a raw() descriptor that shows up in the watchonly wallet.

furszy

ACK 74c7782

furszy · 2023-10-09T22:21:04Z

src/script/descriptor.cpp

+        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));


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

std::vector<std::unique_ptr<PubkeyProvider>> providers; for (size_t i = 1; i + 1 < data.size(); ++i) { CPubKey pubkey(data[i]); auto pubkey_provider = InferPubkey(pubkey, ctx, provider); if (!pubkey_provider) break; providers.push_back(std::move(pubkey_provider)); } size_t expected_num = data.size() - 2; // First position is the min number of signers, and the last one is the op_checkmultisig if (providers.size() == expected_num) return std::make_unique<MultisigDescriptor>((int)data[0][0], std::move(providers));

furszy · 2023-10-09T22:32:04Z

test/functional/wallet_migration.py

+                continue
+            assert False, "Hybrid pubkey watchonly wallet has more than just raw() and addr()"
+
+        wallet.unloadwallet()


In f895f97:
would be good to also unload the watchonly_wallet wallet.

Sjors

utACK 74c7782

…ys when inferring

InferPubkey can return a nullptr, so check it's result before continuing with creating the inferred descriptor. Github-Pull: bitcoin#28602 Rebased-From: b7485f1

Github-Pull: bitcoin#28602 Rebased-From: 37b9b73

Descriptors disallows hybrid pubkeys. Anything with hybrid pubkeys should becomes a raw() descriptor that shows up in the watchonly wallet. Github-Pull: bitcoin#28602 Rebased-From: f895f97

Github-Pull: bitcoin#28602 Rebased-From: 74c7782

achow101 added this to the 26.0 milestone Oct 5, 2023

DrahtBot added the Descriptors label Oct 5, 2023

fanquake requested review from darosior and sipa October 5, 2023 18:04

sipa reviewed Oct 5, 2023

View reviewed changes

src/script/descriptor.cpp Outdated Show resolved Hide resolved

sipa reviewed Oct 5, 2023

View reviewed changes

src/test/descriptor_tests.cpp Outdated Show resolved Hide resolved

achow101 force-pushed the migrate-hybrid-keys branch from 6c80e82 to 25a8a17 Compare October 5, 2023 19:54

DrahtBot added the CI failed label Oct 5, 2023

sipa reviewed Oct 5, 2023

View reviewed changes

DrahtBot mentioned this pull request Oct 5, 2023

descriptors: Be able to specify change and receiving in a single descriptor string #22838

Merged

achow101 force-pushed the migrate-hybrid-keys branch from 25a8a17 to 6f00af4 Compare October 5, 2023 22:36

DrahtBot removed the CI failed label Oct 6, 2023

DrahtBot mentioned this pull request Oct 6, 2023

MiniTapscript: port Miniscript to Tapscript #27255

Merged

furszy reviewed Oct 6, 2023

View reviewed changes

src/script/descriptor.cpp Outdated Show resolved Hide resolved

test/functional/wallet_migration.py Outdated Show resolved Hide resolved

src/test/descriptor_tests.cpp Outdated Show resolved Hide resolved

src/test/descriptor_tests.cpp Show resolved Hide resolved

achow101 force-pushed the migrate-hybrid-keys branch 2 times, most recently from 2b95b11 to 9d639f0 Compare October 6, 2023 22:47

This was referenced Oct 7, 2023

wallet: Migrate entire address book entries to watchonly and solvables too #28610

Merged

wallet: Reload watchonly and solvables wallets after migration #28609

Merged

DrahtBot added the Needs rebase label Oct 8, 2023

achow101 force-pushed the migrate-hybrid-keys branch from 9d639f0 to a49081f Compare October 8, 2023 17:50

DrahtBot requested a review from furszy October 8, 2023 18:34

DrahtBot removed the Needs rebase label Oct 8, 2023

Sjors reviewed Oct 9, 2023

View reviewed changes

DrahtBot requested a review from Sjors October 9, 2023 07:26

maflcko removed the request for review from Sjors October 9, 2023 10:20

descriptors: Check result of InferPubkey

b7485f1

InferPubkey can return a nullptr, so check it's result before continuing with creating the inferred descriptor.

achow101 added 3 commits October 9, 2023 14:07

descriptors: Move InferScript's pubkey validity checks to InferPubkey

37b9b73

test: Scripts with hybrid pubkeys are migrated to watchonly

f895f97

Descriptors disallows hybrid pubkeys. Anything with hybrid pubkeys should becomes a raw() descriptor that shows up in the watchonly wallet.

test: Unit test for inferring scripts with hybrid and uncompressed keys

74c7782

achow101 force-pushed the migrate-hybrid-keys branch from a49081f to 74c7782 Compare October 9, 2023 18:07

furszy reviewed Oct 9, 2023

View reviewed changes

DrahtBot requested review from sipa and Sjors October 9, 2023 22:33

Sjors approved these changes Oct 11, 2023

View reviewed changes

fanquake merged commit 744157e into bitcoin:master Oct 11, 2023

Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Oct 13, 2023

Merge bitcoin#28602: descriptors: Disallow hybrid and uncompressed ke…

2a48d53

…ys when inferring

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Oct 18, 2023

descriptors: Move InferScript's pubkey validity checks to InferPubkey

549401a

Github-Pull: bitcoin#28602 Rebased-From: 37b9b73

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Oct 18, 2023

test: Unit test for inferring scripts with hybrid and uncompressed keys

98208a8

Github-Pull: bitcoin#28602 Rebased-From: 74c7782

luke-jr mentioned this pull request Oct 18, 2023

[25.x] descriptors: Disallow hybrid keys, and disallow uncompressed keys when inferring #28677

Closed

bitcoin locked and limited conversation to collaborators Oct 10, 2024

descriptors: Disallow hybrid and uncompressed keys when inferring #28602

descriptors: Disallow hybrid and uncompressed keys when inferring #28602

Uh oh!

Conversation

achow101 commented Oct 5, 2023

Uh oh!

DrahtBot commented Oct 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sipa Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

achow101 Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sipa commented Oct 8, 2023

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors Oct 9, 2023

Choose a reason for hiding this comment

Uh oh!

achow101 Oct 9, 2023

Choose a reason for hiding this comment

Uh oh!

Sjors commented Oct 9, 2023

Uh oh!

maflcko commented Oct 9, 2023

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

furszy Oct 9, 2023

Choose a reason for hiding this comment

Uh oh!

furszy Oct 9, 2023

Choose a reason for hiding this comment

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Oct 5, 2023 •

edited

Loading