Skip to content

Commit

Permalink
Merge bitcoin#31590: descriptors: Try pubkeys of both parities when r…
Browse files Browse the repository at this point in the history 
…etrieving the private keys for an xonly pubkey in a descriptor

c0045e6 Add test for multipath miniscript expression (David Gumberg)
b4ac480 descriptor: Use InferXOnlyPubkey for miniscript XOnly pubkey from script (Ava Chow)
4c50c21 tests: Check ExpandPrivate matches for both parsed descriptors (Ava Chow)
092569e descriptor: Try the other parity in ConstPubkeyProvider::GetPrivKey() (Ava Chow)

Pull request description:

  When a `ConstPubkeyProvider` is xonly, the stored pubkey does not necessarily have the correct parity bit. `ToPrivateString()` is correctly handling this by looking up the keys for both parity bits, but `GetPrivKey` does not. This results in not finding the private key when it is actually available if its pubkey has the other parity bit value.

  To fix this, this key finding is refactored into `GetPrivKey()` so that its behavior is corrected, and `ToPrivateString()` is changed to use `GetPrivKey()` as well.

  Additionally, the descriptor test checks are updated to include a check for `ExpandPrivate()` to verify that both the parsed public and private descriptors produce `SigningProvider`s with the same contents.

  Fixes bitcoin#31589

ACKs for top commit:
  Pttn:
    ACK c0045e6
  davidgumberg:
    utACK bitcoin@c0045e6
  kevkevinpal:
    Concept and Code review ACK [c0045e6](bitcoin@c0045e6)
  furszy:
    ACK c0045e6
  theStack:
    re-ACK c0045e6
  rkrux:
    Concept ACK c0045e6

Tree-SHA512: 3dcf2a802b996e0680a3f819075e5a689eb22e484c81ea79b40ec04197ee4ba3f6b9c87c45dfe8a847c9b805b2fd0fad77ffb92a93e65dc3aad74d69d9e3d97f
  • Loading branch information
@fanquake
fanquake committed Jan 21, 2025
2 parents 4601b7c + c0045e6 commit d7f56cc
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 11 deletions.
15 changes: 4 additions & 11 deletions src/script/descriptor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -312,15 +312,7 @@ class ConstPubkeyProvider final : public PubkeyProvider
bool ToPrivateString(const SigningProvider& arg, std::string& ret) const override
{
CKey key;
if (m_xonly) {
for (const auto& keyid : XOnlyPubKey(m_pubkey).GetKeyIDs()) {
arg.GetKey(keyid, key);
if (key.IsValid()) break;
}
} else {
arg.GetKey(m_pubkey.GetID(), key);
}
if (!key.IsValid()) return false;
if (!GetPrivKey(/*pos=*/0, arg, key)) return false;
ret = EncodeSecret(key);
return true;
}
Expand All @@ -331,7 +323,8 @@ class ConstPubkeyProvider final : public PubkeyProvider
}
bool GetPrivKey(int pos, const SigningProvider& arg, CKey& key) const override
{
return arg.GetKey(m_pubkey.GetID(), key);
return m_xonly ? arg.GetKeyByXOnly(XOnlyPubKey(m_pubkey), key) :
arg.GetKey(m_pubkey.GetID(), key);
}
std::optional<CPubKey> GetRootPubKey() const override
{
Expand Down Expand Up @@ -1716,7 +1709,7 @@ struct KeyParser {
if (miniscript::IsTapscript(m_script_ctx) && end - begin == 32) {
XOnlyPubKey pubkey;
std::copy(begin, end, pubkey.begin());
if (auto pubkey_provider = InferPubkey(pubkey.GetEvenCorrespondingCPubKey(), ParseContext(), *m_in)) {
if (auto pubkey_provider = InferXOnlyPubkey(pubkey, ParseContext(), *m_in)) {
m_keys.emplace_back();
m_keys.back().push_back(std::move(pubkey_provider));
return key;
Expand Down
2 changes: 2 additions & 0 deletions src/script/signingprovider.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,8 @@ class TaprootBuilder
std::vector<std::tuple<uint8_t, uint8_t, std::vector<unsigned char>>> GetTreeTuples() const;
/** Returns true if there are any tapscripts */
bool HasScripts() const { return !m_branch.empty(); }

bool operator==(const TaprootBuilder& other) const { return GetTreeTuples() == other.GetTreeTuples(); }
};

/** Given a TaprootSpendData and the output key, reconstruct its script tree.
Expand Down
43 changes: 43 additions & 0 deletions src/test/descriptor_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,15 @@ bool EqualDescriptor(std::string a, std::string b)
return a == b;
}

bool EqualSigningProviders(const FlatSigningProvider& a, const FlatSigningProvider& b)
{
return a.scripts == b.scripts
&& a.pubkeys == b.pubkeys
&& a.origins == b.origins
&& a.keys == b.keys
&& a.tr_trees == b.tr_trees;
}

std::string UseHInsteadOfApostrophe(const std::string& desc)
{
std::string ret = desc;
Expand Down Expand Up @@ -214,6 +223,15 @@ void DoCheck(std::string prv, std::string pub, const std::string& norm_pub, int
BOOST_CHECK_MESSAGE(EqualDescriptor(prv, prv1), "Private ser: " + prv1 + " Private desc: " + prv);
}
BOOST_CHECK(!parse_pub->ToPrivateString(keys_pub, prv1));

// Check that both can ExpandPrivate and get the same SigningProviders
FlatSigningProvider priv_prov;
parse_priv->ExpandPrivate(0, keys_priv, priv_prov);

FlatSigningProvider pub_prov;
parse_pub->ExpandPrivate(0, keys_priv, pub_prov);

BOOST_CHECK_MESSAGE(EqualSigningProviders(priv_prov, pub_prov), "Private desc: " + prv + " Pub desc: " + pub);
}

// Check that private can produce the normalized descriptors
Expand Down Expand Up @@ -808,6 +826,31 @@ BOOST_AUTO_TEST_CASE(descriptor_test)
{{0x80000000UL + 48, 0x80000000UL + 1, 0x80000000UL, 0x80000000UL + 2, 1, 0}, {0x80000000UL + 48, 0x80000000UL + 1, 0x80000000UL, 0x80000000UL + 2, 1, 1}, {0x80000000UL + 48, 0x80000000UL + 1, 0x80000000UL, 0x80000000UL + 2, 1, 2}},
}
);
CheckMultipath("tr(xprv9yYge4PS54XkYT9KiLfCRwc8Jeuz8DucxQGtuEecJZYhKNiqbPxYHTPzXtskmzWBqdqkRAGsghNmZzNsfU2wstaB3XjDQFPv567aQSSuPyo,l:pk(xprvA1ADjaN8H3HGnZSmt4VF7YdWoV9oNq8jhqhurxsrYycBAFK555cECoaY22KWt6BTRNLuvobW5VQTF89PN3iA485LAg7epazevPyjCa4xTzd/<2;3>))",
"tr(xpub6CY33ZvKuS63kwDnpNCCo5YrrgkUXgdUKdCVhd4Dru5gCB3z8wGnqFiUP98Za5pYSYF5KmvBHTY3Ra8FAJGggzBjuHS69WzN8gscPupuZwK,l:pk(xpub6E9a95u27Qqa13XEz62FUgaFMWzHnHrb54dWfMHU7K9A33eDccvUkbu1sHYoByHAgJdR326rWqn9pGZgZHz1afDprW5gGwS4gUX8Ri6aGPZ/<2;3>))",
{
"tr(xprv9yYge4PS54XkYT9KiLfCRwc8Jeuz8DucxQGtuEecJZYhKNiqbPxYHTPzXtskmzWBqdqkRAGsghNmZzNsfU2wstaB3XjDQFPv567aQSSuPyo,l:pk(xprvA1ADjaN8H3HGnZSmt4VF7YdWoV9oNq8jhqhurxsrYycBAFK555cECoaY22KWt6BTRNLuvobW5VQTF89PN3iA485LAg7epazevPyjCa4xTzd/2))",
"tr(xprv9yYge4PS54XkYT9KiLfCRwc8Jeuz8DucxQGtuEecJZYhKNiqbPxYHTPzXtskmzWBqdqkRAGsghNmZzNsfU2wstaB3XjDQFPv567aQSSuPyo,l:pk(xprvA1ADjaN8H3HGnZSmt4VF7YdWoV9oNq8jhqhurxsrYycBAFK555cECoaY22KWt6BTRNLuvobW5VQTF89PN3iA485LAg7epazevPyjCa4xTzd/3))",
},
{
"tr(xpub6CY33ZvKuS63kwDnpNCCo5YrrgkUXgdUKdCVhd4Dru5gCB3z8wGnqFiUP98Za5pYSYF5KmvBHTY3Ra8FAJGggzBjuHS69WzN8gscPupuZwK,l:pk(xpub6E9a95u27Qqa13XEz62FUgaFMWzHnHrb54dWfMHU7K9A33eDccvUkbu1sHYoByHAgJdR326rWqn9pGZgZHz1afDprW5gGwS4gUX8Ri6aGPZ/2))",
"tr(xpub6CY33ZvKuS63kwDnpNCCo5YrrgkUXgdUKdCVhd4Dru5gCB3z8wGnqFiUP98Za5pYSYF5KmvBHTY3Ra8FAJGggzBjuHS69WzN8gscPupuZwK,l:pk(xpub6E9a95u27Qqa13XEz62FUgaFMWzHnHrb54dWfMHU7K9A33eDccvUkbu1sHYoByHAgJdR326rWqn9pGZgZHz1afDprW5gGwS4gUX8Ri6aGPZ/3))",
},
{
"tr(xpub6CY33ZvKuS63kwDnpNCCo5YrrgkUXgdUKdCVhd4Dru5gCB3z8wGnqFiUP98Za5pYSYF5KmvBHTY3Ra8FAJGggzBjuHS69WzN8gscPupuZwK,l:pk(xpub6E9a95u27Qqa13XEz62FUgaFMWzHnHrb54dWfMHU7K9A33eDccvUkbu1sHYoByHAgJdR326rWqn9pGZgZHz1afDprW5gGwS4gUX8Ri6aGPZ/2))",
"tr(xpub6CY33ZvKuS63kwDnpNCCo5YrrgkUXgdUKdCVhd4Dru5gCB3z8wGnqFiUP98Za5pYSYF5KmvBHTY3Ra8FAJGggzBjuHS69WzN8gscPupuZwK,l:pk(xpub6E9a95u27Qqa13XEz62FUgaFMWzHnHrb54dWfMHU7K9A33eDccvUkbu1sHYoByHAgJdR326rWqn9pGZgZHz1afDprW5gGwS4gUX8Ri6aGPZ/3))",
},
XONLY_KEYS,
{
{{"512094cb097990da64eebbad7b979b1326f3cbe356357abf4deb4c4ff80c7acbe902"}},
{{"5120f091450b88c606f5cbc3f0cebe89e00bc5dd27f92e22f54da06439bc0c401f41"}},
},
OutputType::BECH32M,
{
{{2}, {}},
{{3}, {}},
}
);
CheckUnparsable("pkh(xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/<0;1>/<2;3>)", "pkh(xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/<0;1>/<2;3>)", "pkh(): Multiple multipath key path specifiers found");
CheckUnparsable("pkh([deadbeef/<0;1>]xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/0)", "pkh([deadbeef/<0;1>]xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/0)", "pkh(): Key path value \'<0;1>\' specifies multipath in a section where multipath is not allowed");
CheckUnparsable("tr(xprv9s21ZrQH143K2Zu2kTVKcQi9nKhfgJUkYqG73wXsHuhATm1wkt6kcSZeTYEw2PL7krZtJopEYDvBdYWdAai3n3TWUTCVfHvPHqTYJv7smYe/6/*,{pk(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc/<1;2;3>/0/*),pk(xprv9s21ZrQH143K3jUwNHoqQNrtzJnJmx4Yup8NkNLdVQCymYbPbJXnPhwkfTfxZfptcs3rLAPUXS39oDLgrNKQGwbGsEmJJ8BU3RzQuvShEG4/0/0/<3;4>/*)})", "tr(xpub6B4sSbNr8XFYXqqKB7PeUemqgEaVtCLjgd5Lf2VYtezSHozC7ffCvVNCyu9TCgHntRQdimjV3tHbxmNfocxtuh6saNtZEw91gjXLRhQ3Yar/6/*,{pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/<1;2;3>/0/*),pk(xpub6AhFhZJJGt9YB8i85RfrJ8jT3T2FF5EejDCXqXfm1DAczFEXkk8HD3CXTg2TmKM8wTbSnSw3wPg5JuyLitUrpRmkjn2BQXyZnqJx16AGy94/0/0/<3;4>/*)})", "tr(): Multipath subscripts have mismatched lengths");
Expand Down

0 comments on commit d7f56cc

Please sign in to comment.