Implements parsing of BIP 390 musig()
descriptors.
Split from #29675
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31244.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
exportwatchonlywallet
RPC to export a watchonly version of a wallet by achow101)XOnlyPubKey::GetKeyIDs()
to return a pair of pubkeys by w0xlt)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.
0$ echo "cGsobXVzaWcoZGR9dXVlLzAwLylrKA==" | base64 --decode > mocked_descriptor_parse_1.crash
1$ FUZZ=mocked_descriptor_parse fuzz mocked_descriptor_parse_1.crash
2script/descriptor.cpp:1838 ParsePubkey: Assertion `Func("musig", expr)' failed.
3
4$ echo "dHIobXVzaWcoICAgICB0dXVzKG9sZGVwayhnZylnZ2dnZmdnKTwseigoKCgoKCgoKCgoKCgoKCgoKCgoKHN0KQ==" | base64 --decode > mocked_descriptor_parse_2.crash
5$ FUZZ=mocked_descriptor_parse fuzz mocked_descriptor_parse_2.crash
6script/descriptor.cpp:1833 ParsePubkey: Assertion `split.size() <= 2' failed.
0$ echo "dHIobXVzaWcoJTIyLzMzMmgpSigoKChhZGRyKEJjdXUp" | base64 --decode > mocked_descriptor_parse_3.crash
1$ FUZZ=mocked_descriptor_parse fuzz mocked_descriptor_parse_3.crash
2script/descriptor.cpp:632 GetPubKey: Assertion `pubkey.has_value()' failed.
0$ echo "dHIobXVzaWcoKS8wMDAwMTEp" | base64 --decode > descriptor_parse_1.crash
1$ FUZZ=descriptor_parse fuzz descriptor_parse_1.crash
2[libsecp256k1] illegal argument: pubkeys != NULL
0$ echo "dHIobXVzaWcoJWU5LzwwOzAwMDAwMzU7MDYwOzM7MDY+KS82Nik=" | base64 --decode > mocked_descriptor_parse_4.crash
1$ FUZZ=mocked_descriptor_parse fuzz mocked_descriptor_parse_4.crash
2fuzz: script/descriptor.cpp:1942: std::vector<std::unique_ptr<PubkeyProvider>> (anonymous namespace)::ParsePubkey(uint32_t &, const Span<const char> &, ParseScriptContext, FlatSigningProvider &, std::string &): Assertion `pub.size() == 1' failed.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/34992034826
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/40357481289
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
86@@ -82,13 +87,21 @@ bool FlatSigningProvider::GetTaprootBuilder(const XOnlyPubKey& output_key, Tapro
87 return LookupHelper(tr_trees, output_key, builder);
88 }
89
90+std::vector<CPubKey> FlatSigningProvider::GetAggregateParticipantPubkeys(const CPubKey& pubkey) const
91+{
92+ const auto& it = aggregate_pubkeys.find(pubkey);
93+ if (it == aggregate_pubkeys.end()) return {};
94+ return it->second;
nit: could use LookupHelper
here (for slightly better readability imho)
0 std::vector<CPubKey> participant_pubkeys;
1 LookupHelper(aggregate_pubkeys, pubkey, participant_pubkeys);
2 return participant_pubkeys;
5+#ifndef BITCOIN_MUSIG_H
6+#define BITCOIN_MUSIG_H
7+
8+#include <pubkey.h>
9+
10+#include <vector>
nit: should also include <optional>
, since it’s used for the return types below
0#include <optional>
1#include <vector>
598+ bool IsRangedParticipants() const
599+ {
600+ for (const auto& pubkey : m_participants) {
601+ if (pubkey->IsRange()) return true;
602+ }
603+ return false;
std::any_of
788+ // musig() can only be a BIP 32 key if all participants are bip32 too
789+ bool all_bip32 = true;
790+ for (const auto& pk : m_participants) {
791+ all_bip32 &= pk->IsBIP32();
792+ }
793+ return all_bip32;
std::all_of
Concept ACK
Seems like the commit message for 2e6dcdbc8055660a2e20ba81b62b7d26ae0ccb05 (“Add MuSig2 Keyagg Cache class and functions”) is out-of-sync, as there is no such class added and also the mentioned MuSig2KeyAggCacheImpl
doesn’t exist.
For the newly introduced parameters of the Const
and Split
functions (commits fe02d7cb237c00de6abe1776b3101342ffddf757 and 4a1eeee27a64c4be7293740f8fff839879b88d86), it would be nice to have unit test coverage, but this can be also done in a follow-up.
638+ // Make the synthetic xpub and construct the BIP32PubkeyProvider
639+ CExtPubKey extpub;
640+ extpub.nDepth = 0;
641+ std::memset(extpub.vchFingerprint, 0, 4);
642+ extpub.nChild = 0;
643+ extpub.chaincode.FromHex("6589e367712c6200e367717145cb322d76576bc3248959c474f9a602ca878086");
musig.h
.
11+#include <vector>
12+
13+struct secp256k1_musig_keyagg_cache;
14+
15+bool GetMuSig2KeyAggCache(const std::vector<CPubKey>& pubkeys, secp256k1_musig_keyagg_cache& keyagg_cache);
16+std::optional<CPubKey> GetCPubKeyFromMuSig2KeyAggCache(secp256k1_musig_keyagg_cache& cache);
285@@ -286,6 +286,7 @@ class XOnlyPubKey
286 * This is needed for key lookups since keys are indexed by CKeyID.
287 */
288 std::vector<CKeyID> GetKeyIDs() const;
289+ std::vector<CPubKey> GetCPubKeys() const;
In 225b3adbf53e4dfde32a1f798cde30cc41998e3c “XOnlyPubKey: Add GetCPubKeys”: maybe document:
0// Return both a version prefixed with 0x02, and one with 0x03.
The comment inside GetKeyIDs
could also be moved to the header.
12@@ -13,10 +13,10 @@ namespace script {
13
14 /** Parse a constant.
15 *
16- * If sp's initial part matches str, sp is updated to skip that part, and true is returned.
17+ * If sp's initial part matches str, sp is optionally updated to skip that part, and true is returned.
99@@ -100,18 +100,27 @@ void ReplaceAll(std::string& in_out, const std::string& search, const std::strin
100 *
101 * If sep does not occur in sp, a singleton with the entirety of sp is returned.
102 *
103+ * @param[in] include_sep Whether to include the separator at the end of the left side of the splits.
104+ *
105 * Note that this function does not care about braces, so splitting
106 * "foo(bar(1),2),3) on ',' will return {"foo(bar(1)", "2)", "3)"}.
107+ *
108+ * If include_sep == true, splitting "foo(bar(1),2),3) on ','
109+ * will return {"foo(bar(1),", "2),", "3)"}.
Also in https://github.com/bitcoin/bitcoin/commit/4a1eeee27a64c4be7293740f8fff839879b88d86, this would be a bit easier to read:
0* will return the following strings:
1* - foo(bar(1),
2* - 2),
3* - 3)
220@@ -221,6 +221,9 @@ struct PubkeyProvider
221
222 /** Make a deep copy of this PubkeyProvider */
223 virtual std::unique_ptr<PubkeyProvider> Clone() const = 0;
224+
225+ /** Whether this PubkeyProvider can be a BIP 32 extended key that can be derived from */
12+
13+struct secp256k1_musig_keyagg_cache;
14+
15+bool GetMuSig2KeyAggCache(const std::vector<CPubKey>& pubkeys, secp256k1_musig_keyagg_cache& keyagg_cache);
16+std::optional<CPubKey> GetCPubKeyFromMuSig2KeyAggCache(secp256k1_musig_keyagg_cache& cache);
17+std::optional<CPubKey> MuSig2AggregatePubkeys(const std::vector<CPubKey>& pubkeys);
MuSig2DeriveAggregatePubkey()
10+#include <optional>
11+#include <vector>
12+
13+struct secp256k1_musig_keyagg_cache;
14+
15+bool GetMuSig2KeyAggCache(const std::vector<CPubKey>& pubkeys, secp256k1_musig_keyagg_cache& keyagg_cache);
761+ // If there is not, then the participant privkeys will be included directly
762+ for (const auto& prov : m_participants) {
763+ prov->GetPrivKey(pos, arg, out);
764+ }
765+ }
766+ std::optional<CPubKey> GetRootPubKey() const override
754+ return true;
755+ }
756+
757+ void GetPrivKey(int pos, const SigningProvider& arg, FlatSigningProvider& out) const override
758+ {
759+ // Get the private keys for all participants
725+ } else {
726+ out += pubkey->ToString();
727+ }
728+ }
729+ out += ")";
730+ out += FormatHDKeypath(m_path, /*apostrophe=*/true);
Concept ACK
Reviewed up to 6b634754d2783ffe16570ad37dbcf9251a7efc24, mostly happy.
I didn’t thoroughly check that all test vectors from BIP390 are in the test, but did check a few.
Can you add an example to
doc/descriptors.md
?
This still seems useful (and this document should probably link back to the bip numbers, but that’s orthogonal).
Manual testing hint for other reviewers: you can use the deriveaddresses
RPC with some of the test vectors. Just use 00000000
as the checksum, it will tell you the right one, and then call it gain to see an address. Pass that address into validateaddress
and compare the scriptPubKey
to the one in the bip.
664+ // Either way, we can passthrough to it's GetPubKey
665+ std::optional<CPubKey> pub = m_aggregate_provider->GetPubKey(pos, arg, out, read_cache, write_cache);
666+ if (!pub) return std::nullopt;
667+ pubout = *pub;
668+ out.aggregate_pubkeys.emplace(m_aggregate_pubkey.value(), pubkeys);
669+ } else if (IsRangedParticipants()) {
in commit 048c95ea557cf81a8e55c5ffe9994cc9d8f3e039: could Assert
this condition instead, as otherwise the logic suggests that it’s possible that neither of the if-branches are executed (it’s not, IIUC; the only reason to not have a cached aggregate pubkey at this point is if we have ranged participants, so this condition is always true)
0 } else {
1 Assert(IsRangedParticipants());
Assume
. It’s occurred to me that Assert
in descriptor code is probably not the way to go, so I’ve changed the couple other Assert
to Assume
.
11@@ -12,6 +12,10 @@
12
13 struct secp256k1_musig_keyagg_cache;
14
15+//! MuSig2 chaincode as defined by BIP 328
16+//! uint256 will byteswap the hex
17+constexpr uint256 MUSIG_CHAINCODE{"6589e367712c6200e367717145cb322d76576bc3248959c474f9a602ca878086"};
in commit 048c95ea557cf81a8e55c5ffe9994cc9d8f3e039: not too fond of “reverse-hexstrings of byte-strings” creeping in, could do instead
0using namespace util::hex_literals;
1constexpr std::array<uint8_t, 32> MUSIG_CHAINCODE{"868087ca02a6f974c4598924c36b57762d32cb45717167e300622c7167e38965"_hex_u8};
and assign the constant to the uint256 instance via ... = uint256(MUSIG_CHAINCODE)
.
(but shouldn’t be a blocker, can be improved in a follow-up as well, if others are even as annoyed by that as me 😅 )
_hex_u8
can be used directly in uin256’s constructor with normal byte order, so used that.
We need to retrieve the even and odd compressed pubkeys for xonly
pubkeys, so add a function to do that. Also reuse it in GetKeyIDs.
15+//! MuSig2 chaincode as defined by BIP 328
16+//! uint256 will byteswap the hex
17+using namespace util::hex_literals;
18+constexpr uint256 MUSIG_CHAINCODE{"868087ca02a6f974c4598924c36b57762d32cb45717167e300622c7167e38965"_hex_u8};
19+
20+//! Create a secp56k1_musig_keyagg_cache from the pubkeys in their current order. This is necessary for most MuSig2 operations
782+ std::unique_ptr<PubkeyProvider> Clone() const override
783+ {
784+ std::vector<std::unique_ptr<PubkeyProvider>> providers;
785+ providers.reserve(m_participants.size());
786+ for (const std::unique_ptr<PubkeyProvider>& p : m_participants) {
787+ providers.emplace_back(p->Clone());
Clone()
? Maybe Check()
in descriptor_tests.cpp
should do this for all valid descriptors?
DescriptorImpl
and PubkeyProvider
classes; all of the unit tests start with a descriptor string that has to be parsed. The parsing is implemented in the next commit (because the PubkeyProvider
it uses needs to exist before parsing can construct the object), and tests for everything in the following commit.
715+ bool ToPrivateString(const SigningProvider& arg, std::string& out) const override
716+ {
717+ bool any_privkeys = false;
718+ out = "musig(";
719+ for (size_t i = 0; i < m_participants.size(); ++i) {
720+ const auto& pubkey = m_participants.at(i);
In 35db4f2dcfc3435e10935581ffa447ffe219cc1e “descriptor: Add MuSigPubkeyProvider”: could avoid old school for i
, make it slightly shorter and have the intention be more clear:
0--- a/src/script/descriptor.cpp
1+++ b/src/script/descriptor.cpp
2@@ -691,9 +691,9 @@ public:
3 std::string ToString(StringType type=StringType::PUBLIC) const override
4 {
5 std::string out = "musig(";
6- for (size_t i = 0; i < m_participants.size(); ++i) {
7- const auto& pubkey = m_participants.at(i);
8- if (i) out += ",";
9+ size_t pos{0};
10+ for (const auto& pubkey : m_participants) {
11+ if (pos++) out += ",";
12 std::string tmp;
Though it’s a not a huge improvement.
for i
is used here because we need the position.
613+ m_derive(derive)
614+ {}
615+
616+ std::optional<CPubKey> GetPubKey(int pos, const SigningProvider& arg, FlatSigningProvider& out, const DescriptorCache* read_cache = nullptr, DescriptorCache* write_cache = nullptr) const override
617+ {
618+ // If the participants are not ranged, we can compute and cache the aggregate pubkey by creating a PubkeyProvider for it
In 35db4f2dcfc3435e10935581ffa447ffe219cc1e “descriptor: Add MuSigPubkeyProvider”: just to check my own understanding, by ranged participants you mean the case of musig(alice/*, bob)
, i.e. derivation before aggregation?
I assume that if the participants are ranged, then you can’t also have a range after derivation, e.g. musig(alice/*, bob)/*
isn’t allowed. Where do we check this?
I assume that if the participants are ranged, then you can’t also have a range after derivation, e.g. musig(alice/*, bob)/* isn’t allowed. Where do we check this?
In the next commit which implements parsing. Enforcement of the descriptor constraints is usually at the parsing level as it is not possible create these classes outside of the descriptor module.
As an additional belt-and-suspenders and documentation, would a corresponding assertion (or Assume
) in the MuSig2PubkeyProvider ctor make sense? E.g. something like
0diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
1index 04ba832ffc..92dcf73755 100644
2--- a/src/script/descriptor.cpp
3+++ b/src/script/descriptor.cpp
4@@ -611,7 +611,10 @@ public:
5 m_participants(std::move(providers)),
6 m_path(std::move(path)),
7 m_derive(derive)
8- {}
9+ {
10+ /* if participants are already ranged, further derivation after pubkey aggregation isn't allowed */
11+ Assert(!(IsRangedParticipants() && IsRangedDerivation()));
12+ }
13
14 std::optional<CPubKey> GetPubKey(int pos, const SigningProvider& arg, FlatSigningProvider& out, const DescriptorCache* read_cache = nullptr, DescriptorCache* write_cache = nullptr) const override
15 {
Assume
.
MuSigPubkeyProvider
introduced in 35db4f2dcfc3435e10935581ffa447ffe219cc1e.
11@@ -12,6 +12,11 @@
12
13 struct secp256k1_musig_keyagg_cache;
14
15+//! MuSig2 chaincode as defined by BIP 328
16+//! uint256 will byteswap the hex
17+using namespace util::hex_literals;
18+constexpr uint256 MUSIG_CHAINCODE{"868087ca02a6f974c4598924c36b57762d32cb45717167e300622c7167e38965"_hex_u8};
in commit 35db4f2dcfc3435e10935581ffa447ffe219cc1e: the byteswap comment is obsolete now
0//! MuSig2 chaincode as defined by BIP 328
1using namespace util::hex_literals;
2constexpr uint256 MUSIG_CHAINCODE{"868087ca02a6f974c4598924c36b57762d32cb45717167e300622c7167e38965"_hex_u8};
632+ // Aggregate the pubkey
633+ m_aggregate_pubkey = MuSig2AggregatePubkeys(pubkeys);
634+ if (!Assume(m_aggregate_pubkey.has_value())) return std::nullopt;
635+
636+ // Make our pubkey provider
637+ if (m_derive != DeriveType::NO || !m_path.empty()) {
in commit 35db4f2dcfc3435e10935581ffa447ffe219cc1e: nit, could use the helper here:
0 if (IsRangedDerivation() || !m_path.empty()) {
1657@@ -1653,7 +1658,7 @@ std::optional<uint32_t> ParseKeyPathNum(std::span<const char> elem, bool& apostr
1658 * @param[in] allow_multipath Allows the parsed path to use the multipath specifier
1659 * @returns false if parsing failed
1660 **/
1661-[[nodiscard]] bool ParseKeyPath(const std::vector<std::span<const char>>& split, std::vector<KeyPath>& out, bool& apostrophe, std::string& error, bool allow_multipath)
1662+[[nodiscard]] bool ParseKeyPath(const std::vector<std::span<const char>>& split, std::vector<KeyPath>& out, bool& apostrophe, std::string& error, bool allow_multipath, bool allow_hardened = true)
1806@@ -1802,9 +1807,153 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkeyInner(uint32_t key_exp_i
1807 }
1808
1809 /** Parse a public key including origin information (if enabled). */
1810-std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkey(uint32_t key_exp_index, const std::span<const char>& sp, ParseScriptContext ctx, FlatSigningProvider& out, std::string& error)
1811+// NOLINTNEXTLINE(misc-no-recursion)
1812+std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkey(uint32_t& key_exp_index, const std::span<const char>& sp, ParseScriptContext ctx, FlatSigningProvider& out, std::string& error)
key_exp_index
parameter changed to pass by reference? Without that the unit tests fail, so it seems necessary, but I’m confused why this was not needed earlier already for similar constructs (e.g. for multi()
expressions where the value is also incremented in the key expression parsing loop). Probably I’m missing some basic descriptors parsing knowledge.
musig()
expressions are a key expression, which itself contains multiple key expressions. It needs to be able to increment key_exp_index
such that the caller will also know what the key_exp_index
has been incremented to. The easiest way to do this was a reference.
ParsePubkey
vs ParseScript
; for the latter the key_exp_index
argument was already passed by reference).
611+ m_participants(std::move(providers)),
612+ m_path(std::move(path)),
613+ m_derive(derive)
614+ {
615+ if (!Assume(!(IsRangedParticipants() && IsRangedDerivation()))) {
616+ throw std::runtime_error("musig(): Cannot have both ranged participatns and ranged derivation");
participatns -> participants [correct misspelling in error message]
705+ break;
706+ case StringType::COMPAT:
707+ tmp = pubkey->ToString(PubkeyProvider::StringType::COMPAT);
708+ break;
709+ }
710+ out += tmp;
in commit ff7476ec32f4dbee07bf04169e741e2aea5a9ab7: can’t this be simplified to
0 out += pubkey->ToString(type);
? It’s a bit confusing that there are two StringType
enum classes within different classes, (PubkeyProvider::StringType
and DescriptorImpl::StringType
), but the code here seems to deal only with the former. I guess whether the diff makes sense or not depends on if it’s possible in the future that the default arguments for pubkey providers ToString(...)
method is ever changed from StringType::PUBLIC
to something else. The unit tests still seem to pass at least.
According to BIP 390 “Repeated participant public keys are not allowed.”, but it seems there is currently no check preventing that? Example test case that passes but shouldn’t (IIUC): theStack@f260c7e
Good catch. Fixed and added a test.
586+{
587+private:
588+ //! PubkeyProvider for the participants
589+ const std::vector<std::unique_ptr<PubkeyProvider>> m_participants;
590+ //! Derivation path if this is ranged
591+ const KeyPath m_path;
in commit 16a88829e92c2676cae6b6e0acb75d2d26795a6d:
0 //! Derivation path
1 const KeyPath m_path;
as using non-ranged derivation is also possible (e.g. musig(...)/1
, we also have a test for that); the BIP 390 specification can be interpreted in a way that only ranged derivation is possible though (https://github.com/bitcoin/bips/blob/master/bip-0390.mediawiki#user-content-Specification), maybe something that could be improved there?
1827+ return {};
1828+ }
1829+ std::span<const char> sp_musig(split.at(0).begin(), split.at(0).end());
1830+
1831+ auto expr = Expr(sp_musig);
1832+ if (!Func("musig", expr)) {
in commit 9e3a1d05bf066df31a6e124638046e70669eb470: nit: the Expr
call seems to be a no-op here, considering that we already extracted the function expression with the parens-splitting above
0 std::span<const char> expr(split.at(0).begin(), split.at(0).end());
1 if (!Func("musig", expr)) {
1875+ // Parse any derivation
1876+ DeriveType deriv_type = DeriveType::NO;
1877+ std::vector<KeyPath> paths;
1878+ if (split.size() == 2 && Const("/", split.at(1), /*skip=*/false)) {
1879+ if (!all_bip32) {
1880+ error = "musig(): Ranged musig() requires all participants to be xpubs";
in commit 9e3a1d05bf066df31a6e124638046e70669eb470: should this be
0 if (!all_bip32) {
1 error = "musig(): musig() derivation requires all participants to be xpubs";
instead? Ranged would mean ending with /*
IIUC, which is not necessarily the case.
1855+ }
1856+ const auto& [_, inserted] = key_exprs.emplace(arg.begin(), arg.end());
1857+ if (!inserted) {
1858+ error = strprintf("musig(): Cannot have repeated participant key expressions");
1859+ return {};
1860+ }
(priv,pub)
, the expression musig(priv,pub)
would now still be valid).
Checking for duplicates is actually a bit more complicated, and I’m thinking now that it would probably be easier to allow duplicates.
There are some scenarios where I think it’s ambiguous whether they should be allowed. For example, would musig(xpub/1,xpub/*)
be allowed? The same pubkey would appear twice for exactly one index.
You could also be more strict and disallow any duplicate non-hardened path. Or even duplicate origin fingerprint.
The bigger problem may be that whatever we choose might be slightly different than what implementations do. So we should probably change the descriptor standard to pick an extreme on either side: allow duplicate keys or more thorowly prevent them.
There are some scenarios where I think it’s ambiguous whether they should be allowed.
“Repeated participant public keys are not allowed. The aggregate public key is produced by using the KeyAgg algorithm on all KEYs specified in the expression after performing all specified derivation.”
The current language^ in BIP 390 gives me an impression that the public key derived from the KEY expression should not be repeated that subsequently leads me to believe that the current check, which operates on string key expressions, is brittle.
So we should probably change the descriptor standard to pick an extreme on either side
Agree, picking a side will lead to certainty.
allow duplicate keys or more thorowly prevent them.
Yes, I’ve suggest to the mailing list that we should allow the duplicate keys. Still checking with cryptographers that that will be okay, particularly since PSBT doesn’t allow duplicate participants to use different pubnonces and partial sigs.
In BIP 327, the paragraph starting from here makes allows the case for having duplicate keys in the protocol. It’s also recommended to omit such checks.
In fact, applications are recommended to omit checks for duplicate individual public keys in order to simplify error handling.
The next paragraph, however, mentions a case where applications may choose to abort when duplicates are found. Since the base Musig2 BIP itself has a recommendation towards allowing duplicates, I don’t mind the parsing of descriptors allowing them as well though I don’t see a case where having duplicate keys would be valuable.
since PSBT doesn’t allow duplicate participants to use different pubnonces and partial sigs.
Good point, these 2 structs in PSBT would need to be updated: https://github.com/bitcoin/bitcoin/blob/ae024137bda9fe189f4e7ccf26dbaffd44cbbeb6/src/psbt.h#L272-L275 One way I see is to also store the index of the participant key in the expression to differentiate between the duplicates.
Good point, these 2 structs in PSBT would need to be updated:
I believe there won’t need to be any changes to PSBT. If a participant pubkey is duplicated, it is still valid to for each time the pubkey appears, to use the same pubnonce and the same partial sig for that nonce for each instance the pubkey appears. The only complication with this is if the duplicate pubkeys are different signers and produce different nonces and partial sigs as they may overwrite each other resulting a PSBT with a partial sig that does not match the pubn However, I’m pretty sure (and I did check with a cryptographer) that there cannot be any key leakage.
I see that I had misunderstood your previous comment, this^ looks fine to me.
The Sign algorithm must not be executed twice with the same secnonce. Otherwise, it is possible to extract the secret signing key from the two partial signatures output by the two executions of Sign.
This^ is the only precaution I read in BIP 327 related to reusing, and I don’t think anything in the above comment goes against this precaution. Specially since using the same partial sig doesn’t translate to signing again.
I have not yet thought through the consequences when PSBTs are shared between signers during the signing process, I believe that picture would be clearer to me in the next PR of Musig signing.
1813+
1814+ // musig cannot be nested inside of an origin
1815+ std::span<const char> span = sp;
1816+ if (Const("musig(", span, /*skip=*/false)) {
1817+ if (ctx != ParseScriptContext::P2TR) {
1818+ error = "musig() is only allowed in tr()";
0- "musig() is only allowed in tr()"
1+ "musig() is allowed in only tr() and rawtr()"
1873+ // Parse any derivation
1874+ DeriveType deriv_type = DeriveType::NO;
1875+ std::vector<KeyPath> paths;
1876+ if (split.size() == 2 && Const("/", split.at(1), /*skip=*/false)) {
1877+ if (!all_bip32) {
1878+ error = "musig(): musig() derivation requires all participants to be xpubs";
0- musig(): musig() derivation requires all participants to be xpubs
1+ musig(): derivation requires all participants to be xpubs or xprvs
1856+ error = strprintf("musig(): Cannot have repeated participant key expressions");
1857+ return {};
1858+ }
1859+
1860+ any_ranged |= pk.at(0)->IsRange();
1861+ all_bip32 &= pk.at(0)->IsBIP32();
1925+ }
1926+ ret.emplace_back(std::make_unique<MuSigPubkeyProvider>(key_exp_index, std::move(pubs), path, deriv_type));
1927+ };
1928+
1929+ if (max_providers_len > 1 && paths.size() > 1) {
1930+ error = "musig(): Cannot have multipath participant keys if musig() is also multipath";
Currently, BIP 390 doesn’t have a mention of multipath participant keys. As per BIP 389:
Descriptors that contain multiple Key Expressions that each have a /<NUM;NUM;…;NUM> must have tuples of exactly the same length so that they are derived in lockstep in the same way that /* paths in multiple Key expressions are handled.
By using this lockstep approach, I believe technically multipath derivations for musig can be done. Is it not allowed because it doesn’t seem as needed?
596+
597+ bool IsRangedDerivation() const { return m_derive != DeriveType::NO; }
598+ bool IsRangedParticipants() const
599+ {
600+ return std::any_of(m_participants.begin(), m_participants.end(), [](const auto& pubkey) { return pubkey->IsRange(); });
601+ }
IsRangedParticipants
is called multiple times in this class, can evaluate it once in the constructor itself and store the result in a const because I don’t suppose a MuSigPubkeyProvider
is updated with more m_participants
after creation.
603+public:
604+ MuSigPubkeyProvider(
605+ uint32_t exp_index,
606+ std::vector<std::unique_ptr<PubkeyProvider>> providers,
607+ KeyPath path,
608+ DeriveType derive
Assume
and throw if the derivation type is hardened, it’s not allowed because IIUC, m_derive
refers to the derivation type of this musig provider only.
663+ std::sort(pubkeys.begin(), pubkeys.end());
664+
665+ CPubKey pubout;
666+ if (m_aggregate_provider) {
667+ // When we have a cached aggregate key, we are either returning it or deriving from it
668+ // Either way, we can passthrough to it's GetPubKey
0- Either way, we can passthrough to it's GetPubKey
1+ Either way, we can passthrough to its GetPubKey
649+
650+ m_aggregate_provider = std::make_unique<BIP32PubkeyProvider>(m_expr_index, extpub, m_path, m_derive, /*apostrophe=*/false);
651+ } else {
652+ m_aggregate_provider = std::make_unique<ConstPubkeyProvider>(m_expr_index, m_aggregate_pubkey.value(), /*xonly=*/false);
653+ }
654+ }
GetPubKey
as the participants may involve hardened derivation which requires private keys that the constructor doesn’t have access to.
0prov->GetPubKey(0, arg,
Ah I see now the SigningProvider being passed here. I notice that const SigningProvider& arg
is quite prevalent in this file, but I don’t prefer calling the signing provider here just arg
- it’s easy to miss. Fine to stick with it in this PR, can be updated separately for all occurrences in one go if it makes sense to others as well.
665+
666+ CPubKey pubout;
667+ if (m_aggregate_provider) {
668+ // When we have a cached aggregate key, we are either returning it or deriving from it
669+ // Either way, we can passthrough to its GetPubKey
670+ std::optional<CPubKey> pub = m_aggregate_provider->GetPubKey(pos, arg, out, read_cache, write_cache);
arg
because there can’t be a hardened derivation at this level. Also, better to not pass the signing provider if we know for certain that it will not be used.
681+ KeyOriginInfo info;
682+ CKeyID keyid = aggregate_pubkey->GetID();
683+ std::copy(keyid.begin(), keyid.begin() + sizeof(info.fingerprint), info.fingerprint);
684+ out.origins.emplace(keyid, std::make_pair(*aggregate_pubkey, info));
685+ out.pubkeys.emplace(aggregate_pubkey->GetID(), *aggregate_pubkey);
686+ out.aggregate_pubkeys.emplace(pubout, pubkeys);
Slightly confused in this else
block: for the case of ranged participants and non ranged musig derivation that has a m_path
such as /0/1/2
, it doesn’t seem to do the derivation after getting the aggregate pubkey because I don’t see m_path
being used here like it is used in m_aggregate_provider
above.
I don’t see an explicit mention of this in BIP 390 as well, is this not a valid case to consider?
musig()
with aggregate derivation that the BIP describes explicitly disallows ranged participants.
612+ {
613+ if (!Assume(!(m_ranged_participants && IsRangedDerivation()))) {
614+ throw std::runtime_error("musig(): Cannot have both ranged participants and ranged derivation");
615+ }
616+ if (!Assume(m_derive != DeriveType::HARDENED)) {
617+ throw std::runtime_error("musig(): Cannot have hardened hardened derivation");
0- Cannot have hardened hardened derivation
1+ Cannot have hardened derivation
611+ m_ranged_participants(std::any_of(m_participants.begin(), m_participants.end(), [](const auto& pubkey) { return pubkey->IsRange(); }))
612+ {
613+ if (!Assume(!(m_ranged_participants && IsRangedDerivation()))) {
614+ throw std::runtime_error("musig(): Cannot have both ranged participants and ranged derivation");
615+ }
616+ if (!Assume(m_derive != DeriveType::HARDENED)) {
DeriveType::HARDENED
, the test/debug run will be terminated and a diagnostic message will be printed to the standard error stream.
In production, a runtime error will be generated.
671+ if (!pub) return std::nullopt;
672+ pubout = *pub;
673+ out.aggregate_pubkeys.emplace(m_aggregate_pubkey.value(), pubkeys);
674+ } else {
675+ if (!Assume(m_ranged_participants)) return std::nullopt;
676+ // Derive participants and compute new aggregate key
659+ for (const auto& prov : m_participants) {
660+ std::optional<CPubKey> pub = prov->GetPubKey(pos, arg, out, read_cache, write_cache);
661+ if (!pub) return std::nullopt;
662+ pubkeys.emplace_back(*pub);
663+ }
664+ std::sort(pubkeys.begin(), pubkeys.end());
I’m sensing that this block of derivation of participants’ keys should reside later in the else
block starting on line 674. Reason being that in case of non ranged participants that leads to the presence of m_aggregate_provider
, we don’t really need to derive the participants’ keys again and pos
is effectively unused.
After moving this block down there, either we can derive the keys in the if
block below with 0 pos
, or preferably cache the participants’ derived keys along with m_aggregate_provider
so that we don’t do this derivation again.
Though the current implementation gets rid of code duplication by having this block before the below if/else blocks, but it derives again the keys in case of non ranged participants and also passes the unneeded pos
argument, which makes the reader reason through it unnecessarily imho.
out
needs to be filled with participant pukey info. This loop is present in order to do that for both aggregate derivation and participant derivation.
I see, out
does need to be filled. In that case, there is an option to cache the dummy
signing provider on line 629 when the aggregate pubkey & provider are being created (and cached) in case of non ranged participants. This could later be FlatSigningProvider::Merge
’d with the out
signing provider on line 672 so that out
has information for both the participants and the aggregate.
The reason I find moving this snippet inside the else
block here appealing is that then we don’t need to derive the non ranged participant pubkeys every time GetPubKey
is called and I do like seeing 0 being passed as pos
in case of non ranged participants (ConstPubkeyProvider or DeriveType::NO BIP32PubkeyProvider) like done on line 629.
pubkeys
would also need to be cached in this case. Ok if you are not looking to do major changes in the PR at this stage.
read_cache
.
12@@ -13,10 +13,10 @@ namespace script {
13
14 /** Parse a constant.
15 *
16- * If sp's initial part matches str, sp is updated to skip that part, and true is returned.
17+ * If sp's initial part matches str, sp is optionally updated to skip that part, and true is returned.
18 * Otherwise sp is unmodified and false is returned.
19 */
20-bool Const(const std::string& str, std::span<const char>& sp);
21+bool Const(const std::string& str, std::span<const char>& sp, bool skip = true);
src/test/util_tests.cpp
in BOOST_AUTO_TEST_CASE(test_script_parsing)
would be good for consistency’s sake.
script/parsing: Allow Const to not skip the found constant
(https://github.com/bitcoin/bitcoin/pull/31244/commits/4b135f80d03c92fb522d1dcfd7d697aa0a4af626) and util/string: Allow Split to include the separator
(https://github.com/bitcoin/bitcoin/pull/31244/commits/b89a937225ed217556b38e000ee5d1bf0b5952aa) could explain why these changes are necessary for the musig2 descriptor.
160@@ -161,6 +161,7 @@ class SigningProvider
161 virtual bool GetKeyOrigin(const CKeyID& keyid, KeyOriginInfo& info) const { return false; }
162 virtual bool GetTaprootSpendData(const XOnlyPubKey& output_key, TaprootSpendData& spenddata) const { return false; }
163 virtual bool GetTaprootBuilder(const XOnlyPubKey& output_key, TaprootBuilder& builder) const { return false; }
164+ virtual std::vector<CPubKey> GetAggregateParticipantPubkeys(const CPubKey& pubkey) const { return {}; }
Without proper context, it may not be clear that this aggregation specifically refers to a MuSig2 aggregation. Perhaps a name change could make the purpose and use of the function clearer.
0 virtual std::vector<CPubKey> GetMuSig2AggregateParticipantPubkeys(const CPubKey& pubkey) const { return {}; }
GetMuSig2ParticipantPubkeys
20+ for (const secp256k1_pubkey& p : secp_pubkeys) {
21+ pubkey_ptrs.push_back(&p);
22+ }
23+
24+ // Aggregate the pubkey
25+ if (!secp256k1_musig_pubkey_agg(secp256k1_context_static, nullptr, &keyagg_cache, pubkey_ptrs.data(), pubkey_ptrs.size())) {
agg_pk
(the MuSig-aggregated x-only public key) for future use, instead of considering it null.
But yes, for now, there is no use.
That can be added if it is needed.
All of the MuSig2 operations rely on the keyagg cache, which is only produced by aggregating. That’s why this function has that as an output parameter.
When parsing a descriptor, it is useful to be able to check whether a
string begins with a substring without consuming that substring as
another function such as Func() will be used later which requires that
substring to be present at the beginning.
Specifically, for MuSig2, this modified Const will be used to determine
whether a an expression begins with "musig(" before a subsequent
Func("musig", ...) is used.
When splitting a string, sometimes the separator needs to be included.
Split will now optionally include the separator at the end of the left
side of the splits, i.e. it appears at the end of the splits, except
for the last one.
Specifically, for musig() descriptors, Split is used to separate a
musig() from any derivation path that follows it by splitting on the
closing parentheses. Since that parentheses is needed for Func() and
Expr(), Split() needs to preserve the end parentheses instead of
discarding it.
secp256k1 provides us secp256k1_musig_keyagg_cache objects which we are
used as part of session info and to get the aggregate pubkey. These
helper functions help us convert to/from the secp256k1 C objects into
the Bitcoin Core C++ objects.
630+ if (!pubkey.has_value()) {
631+ return std::nullopt;
632+ }
633+ pubkeys.push_back(pubkey.value());
634+ }
635+ std::sort(pubkeys.begin(), pubkeys.end());
std::sort(pubkeys.begin(), pubkeys.end())
equivalent to secp256k1_ec_pubkey_sort
?
641+ // Make our pubkey provider
642+ if (IsRangedDerivation() || !m_path.empty()) {
643+ // Make the synthetic xpub and construct the BIP32PubkeyProvider
644+ CExtPubKey extpub;
645+ extpub.nDepth = 0;
646+ std::memset(extpub.vchFingerprint, 0, 4);
std::fill(std::begin(extpub.vchFingerprint), std::end(extpub.vchFingerprint), 0);
214@@ -213,6 +215,7 @@ struct FlatSigningProvider final : public SigningProvider
215 std::map<CKeyID, std::pair<CPubKey, KeyOriginInfo>> origins;
216 std::map<CKeyID, CKey> keys;
217 std::map<XOnlyPubKey, TaprootBuilder> tr_trees; /** Map from output key to Taproot tree (which can then make the TaprootSpendData */
218+ std::map<CPubKey, std::vector<CPubKey>> aggregate_pubkeys; /** MuSig2 aggregate pubkeys */
in commit 8ecea91bf296b8fae8b84c3dbf68d5703821cb79: comment nit
0 std::map<CPubKey, std::vector<CPubKey>> aggregate_pubkeys; /** Map from MuSig2 aggregate pubkey to participant pubkeys */
788+ }
789+ bool IsBIP32() const override
790+ {
791+ // musig() can only be a BIP 32 key if all participants are bip32 too
792+ return std::all_of(m_participants.begin(), m_participants.end(), [](const auto& pubkey) { return pubkey->IsBIP32(); });
793+ }
.IsBIP32()
is only called for parsing participant pubkeys in musig(...)
expressions, and nesting those is not allowed. Not sure what to do with this information though, I guess it’s fine to keep as-is even if it stays very likely unused.
1892+ } else if (std::ranges::equal(deriv_split.back(), std::span{"*'"}.first(2)) || std::ranges::equal(deriv_split.back(), std::span{"*h"}.first(2))) {
1893+ error = "musig(): Cannot have hardened child derivation";
1894+ return {};
1895+ }
1896+ bool dummy = false;
1897+ if (!ParseKeyPath(deriv_split, paths, dummy, error, /*allow_multipath=*/true, /*allow_hardened=*/false)) {
paths
object here instead (with e.g. a new IsKeyPathHardened
helper).
🚧 At least one of the CI tasks failed.
Task tidy
: https://github.com/bitcoin/bitcoin/runs/44377129467
LLM reason (✨ experimental): The CI failure is caused by errors reported by clang-tidy, specifically an unused variable warning treated as an error in descriptor_tests.cpp.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
1941+ if (!clone_providers(max_providers_len)) {
1942+ error = strprintf("musig(): Multipath derivation paths have mismatched lengths");
1943+ return {};
1944+ }
1945+ for (size_t i = 0; i < max_providers_len; ++i) {
1946+ // Final MuSigPubkeyProvider use participant pubkey providers at each multipath position, and the first (and only) path
0 // Final MuSigPubkeyProvider uses participant pubkey providers at each multipath position, and the first (and only) path
1955+ for (size_t i = 0; i < paths.size(); ++i) {
1956+ // Final MuSigPubkeyProvider uses cloned participant pubkey providers, and the multipath derivation paths
1957+ emplace_final_provider(i, i);
1958+ }
1959+ } else {
1960+ // No multipath derivation MuSigPubkeyProvider uses the first (and only) participant pubkey providers, and the first (and only) path
0 // No multipath derivation, MuSigPubkeyProvider uses the first (and only) participant pubkey providers, and the first (and only) path
re-ACK 35a17e5a2e1394cc3f4f5a4f304f58528cb44151 :musical_note: :two:
Verified that since my previous ACK, the duplicate KEY expression check has been removed (as per BIP390 change https://github.com/bitcoin/bips/pull/1867), test cases were updated accordingly, and the alternative “disallow hardened derivation” suggestion was tackled. Just left two code comment nits below, feel free to ignore.
1877+ // Parse any derivation
1878+ DeriveType deriv_type = DeriveType::NO;
1879+ std::vector<KeyPath> paths;
1880+ if (split.size() == 2 && Const("/", split.at(1), /*skip=*/false)) {
1881+ if (!all_bip32) {
1882+ error = "musig(): derivation requires all participants to be xpubs";
I think this was done earlier but for some reason I don’t see xprvs
mentioned now.
1821+
1822+ using namespace script;
1823+
1824+ // musig cannot be nested inside of an origin
1825+ std::span<const char> span = sp;
1826+ if (Const("musig(", span, /*skip=*/false)) {
This might be an opinionated suggestion, feel free to ignore if you’re not convinced:
One of the reasons I find ParseScript
function difficult to read is because of its length and all the script types if
blocks present in one function. Though I don’t suppose this ParsePubkey
function will ever have that many types but if we are branching off right away at the start, then extracting out the MuSig parsing in a ParseMusigPubKey
function (or similar) might be easier on the eyes - that way the reader doesn’t have to keep this if
condition is a mental context while reading the Musig specific flow.
if
uses ParsePubkey
recursively, I prefer to have it be in ParsePubkey
than adding a circular dependency between ParsePubkey
and a ParseMusig
.
1799@@ -1799,10 +1800,168 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkeyInner(uint32_t key_exp_i
1800 return ret;
1801 }
1802
1803+static bool IsKeyPathsHardened(const std::vector<KeyPath>& paths)
Nit: because this function early returns if any key path is hardened -
0- IsKeyPathsHardened
1+ IsAnyKeyPathHardened
1888+ }
1889+ auto deriv_split = Split(split.at(1), '/');
1890+ if (std::ranges::equal(deriv_split.back(), std::span{"*"}.first(1))) {
1891+ deriv_split.pop_back();
1892+ deriv_type = DeriveType::UNHARDENED;
1893+ } else if (std::ranges::equal(deriv_split.back(), std::span{"*'"}.first(2)) || std::ranges::equal(deriv_split.back(), std::span{"*h"}.first(2))) {
613+ if (!Assume(!(m_ranged_participants && IsRangedDerivation()))) {
614+ throw std::runtime_error("musig(): Cannot have both ranged participants and ranged derivation");
615+ }
616+ if (!Assume(m_derive != DeriveType::HARDENED)) {
617+ throw std::runtime_error("musig(): Cannot have hardened derivation");
618+ }
if
condition doesn’t seem to cover all the ways the corresponding error can be thrown because the m_path
can possibly still have a hardened derivation (even though the parsing caller code has a check for it).
PubkeyProvider
s to be created outside of this file anyways.
1881+ if (!all_bip32) {
1882+ error = "musig(): derivation requires all participants to be xpubs";
1883+ return {};
1884+ }
1885+ if (any_ranged) {
1886+ error = "musig(): Cannot have ranged participant keys if musig() also has derivation";
musig(): Cannot have ranged participant keys if musig() also has derivation
+1, this is a much clearer error message that gets rid of the doubt I had in #31244 (review).
1851+ while (expr.size()) {
1852+ if (!first && !Const(",", expr)) {
1853+ error = strprintf("musig(): expected ',', got '%c'", expr[0]);
1854+ return {};
1855+ }
1856+ first = false;
Based on my understanding of the loop and the corresponding error thrown on line 1872, the first
here refers to no_key_parsed
bool that’s initialised to true
at the beginning and toggled to false
when one is (possibly) parsed.
I think explicit variable naming like suggested above would help to read the code. An alternative (my preference) would be to use an any_key_parsed
bool with inverse boolean values instead.
0 bool any_key_parsed = false;
1 size_t max_providers_len = 0;
2 while (expr.size()) {
3 if (any_key_parsed && !Const(",", expr)) {
4 error = strprintf("musig(): expected ',', got '%c'", expr[0]);
5 return {};
6 }
7 any_key_parsed = true;
0 if (!any_key_parsed) {
1 error = "musig(): Must contain key expressions";
2 return {};
3 }
1900+ return {};
1901+ }
1902+ if (IsKeyPathsHardened(paths)) {
1903+ error = "musig(): cannot have hardened derivation steps";
1904+ return {};
1905+ }
The ParseKeyPath
function updates the apostrophe
boolean (dummy
here) if an apostrophe’d hardened derivation is found. If this function also updated a hardened
boolean that it already checks for, then the following IsKeyPathsHardened
function, which iterates the paths again, would not have been required.
This can be done in a follow up if you don’t want to introduce this kind of refactoring of the common ParseKeyPath
function in this PR, which is already big enough.
has_hardened
output parameter.
1949+ } else if (paths.size() > 1) {
1950+ // All key provider vectors should be length 1. Clone them until they have the same length as paths
1951+ if (!clone_providers(paths.size())) {
1952+ error = "musig(): Multipath derivation path with multipath participants is disallowed"; // This error is unreachable due to earlier check
1953+ return {};
1954+ }
// All key provider vectors should be length 1.
// This error is unreachable due to earlier check
Instead of having uncovered code because it’s unreachable, can’t we replace the error setting with an assert instead?
0- if (!clone_providers(paths.size())) {
1- error = "musig(): Multipath derivation path with multipath participants is disallowed"; // This error is unreachable due to earlier check
2- return {};
3- }
4+ assert(max_providers_len == 1);
5+ clone_providers(paths.size());
or
0+ assert(clone_providers(paths.size()));
Assume
. I don’t really like having assert
s in modules like descriptors.
1932+ pubs.emplace_back(std::move(vec.at(vec_idx)));
1933+ }
1934+ ret.emplace_back(std::make_unique<MuSigPubkeyProvider>(key_exp_index, std::move(pubs), path, deriv_type));
1935+ };
1936+
1937+ if (max_providers_len > 1 && paths.size() > 1) {
It took me some time to understand how the 4 conditions starting from here are handled individually. I feel some verbose and contextualised naming could be helpful for the reader as most of them are concerned with multipaths.
0max_providers_len -> providers_max_multipaths
1paths -> derivation_multipaths
This or something similar would be helpful imho.
1908+ }
1909+
1910+ // Makes sure that all providers vectors in providers are the given length, or exactly length 1
1911+ // Length 1 vectors have the single provider cloned until it matches the given length.
1912+ const auto& clone_providers = [&providers](size_t length) -> bool {
1913+ for (auto& vec : providers) {
0providers -> pubkeys_providers
1vec -> pubkey_providers_mulltipath
Because std::vector<std::vector<std::unique_ptr<PubkeyProvider>>>
is a vector of vectors representing multiple pubkeys in the Musig expression each of which could have multipaths if vec.size() > 1
.
Applicable inside emplace_final_provider
function as well.
I feel the clone_providers
function is quite cool because IIUC it handles both the cases of expanding/cloning when either the pubkey provider multipath size is passed, or the musig derivation multipath size is passed.
Code review effe4fcc59bb2f84139bbaee064bfcb72da0baba
I have completed reviewing the descriptor: Parse musig() key expressions
commit by going through it in detail and have suggested few naming suggestions if they make sense.
I’m taking a final look at the descriptor: Add MuSigPubkeyProvider
commit and the open discussions. Thank you for addressing the comments previously.
721+ if (pubkey->ToPrivateString(arg, tmp)) {
722+ any_privkeys = true;
723+ out += tmp;
724+ } else {
725+ out += pubkey->ToString();
726+ }
672+ std::optional<CPubKey> pub = m_aggregate_provider->GetPubKey(pos, dummy, out, read_cache, write_cache);
673+ if (!pub) return std::nullopt;
674+ pubout = *pub;
675+ out.aggregate_pubkeys.emplace(m_aggregate_pubkey.value(), pubkeys);
676+ } else {
677+ if (!Assume(m_ranged_participants)) return std::nullopt;
Nit: can add an Assume
here for m_path
being empty keeping in mind this error.
“musig(): Cannot have ranged participant keys if musig() also has derivation”
682+
683+ KeyOriginInfo info;
684+ CKeyID keyid = aggregate_pubkey->GetID();
685+ std::copy(keyid.begin(), keyid.begin() + sizeof(info.fingerprint), info.fingerprint);
686+ out.origins.emplace(keyid, std::make_pair(*aggregate_pubkey, info));
687+ out.pubkeys.emplace(aggregate_pubkey->GetID(), *aggregate_pubkey);
0- KeyOriginInfo info;
1- CKeyID keyid = aggregate_pubkey->GetID();
2- std::copy(keyid.begin(), keyid.begin() + sizeof(info.fingerprint), info.fingerprint);
3- out.origins.emplace(keyid, std::make_pair(*aggregate_pubkey, info));
4- out.pubkeys.emplace(aggregate_pubkey->GetID(), *aggregate_pubkey);
5+ auto temp_aggregate_provider = std::make_unique<ConstPubkeyProvider>(m_expr_index, aggregate_pubkey.value(), /*xonly=*/false);
6+ FlatSigningProvider dummy;
7+ temp_aggregate_provider->GetPubKey(/*pos=*/0, dummy, out, read_cache, write_cache);
This passes the unit tests in my system.
musig(KEY, KEY, …, KEY)
I could notice that we effectively need a const aggregate provider in case of ranged participants.
FlatSigningProvider dummy;
- this would be common in both the if/else blocks and can be extracted out.
descriptor: Add MuSigPubkeyProvider
commit - 4039be3f9b78c1ddb75c04f4a8002d36c4b05f79
1868+ key_exp_index++;
1869+ }
1870+ if (any_key_parsed) {
1871+ error = "musig(): Must contain key expressions";
1872+ return {};
1873+ }
first
variable name here was not resonating with me.
This previous suggestion (https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2161706514) also suggested to inverse the boolean values, otherwise this block now reads “if any key is parsed, then set error”, which is not right.
I can fix this in follow-up though.
1819+
1820+ using namespace script;
1821+
1822+ // musig cannot be nested inside of an origin
1823+ std::span<const char> span = sp;
1824+ if (Const("musig(", span, /*skip=*/false)) {
span
variable is used only here, I tried to pass sp
instead and got this error.
0note: candidate function not viable: 2nd argument ('const std::span<const char>') would lose const qualifier
1 19 | bool Const(const std::string& str, std::span<const char>& sp, bool skip = true);
2 | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~
31 error generated.
I wish there was a way to inform the compiler that the 2nd argument here is not updated if skip
is false (which is indeed the case here), and we could have avoided creating a new variable just for this.
1945+ }
1946+ } else if (derivation_multipaths.size() > 1) {
1947+ // All key provider vectors should be length 1. Clone them until they have the same length as paths
1948+ if (!Assume(clone_providers(derivation_multipaths.size()))) {
1949+ error = "musig(): Multipath derivation path with multipath participants is disallowed"; // This error is unreachable due to earlier check
1950+ return {};
Assume
but we still end up with an uncovered code block because it is unreachable.
1252+ CheckUnparsable("tr(musig(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc/<0;1>,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y/<2;3>)/<3;4>)","tr(musig(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/<0;1>,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y/<2;3>)/<3;4>)", "tr(): musig(): Cannot have multipath participant keys if musig() is also multipath");
1253+ CheckUnparsable("tr(musig()/0)", "tr(musig()/0)", "tr(): musig(): Must contain key expressions");
1254+ CheckUnparsable("tr(musig(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc/*,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y/*)/0)","tr(musig(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/*,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y/*)/0)", "tr(): musig(): Cannot have ranged participant keys if musig() also has derivation");
1255+
1256+ // Fuzzer crash test cases
1257+ CheckUnparsable("pk(musig(dd}uue/00/)k(", "pk(musig(dd}uue/00/)k(", "Invalid musig() expression");
Interesting case fuzzer caught - the error message made me curious because it didn’t contain the pk
prefix and that made me debug the flow a bit. This case triggered the Miniscript parsing flow in ParseScript
function and the error is coming from this line in KeyParser
.
0auto pk = ParsePubkey(exp_index, {&*begin, &*end}, ParseContext(), *m_out, m_key_parsing_error);
I don’t think this requires any change in this PR.
1648@@ -1647,14 +1649,15 @@ std::optional<uint32_t> ParseKeyPathNum(std::span<const char> elem, bool& apostr
1649 * @param[in] allow_multipath Allows the parsed path to use the multipath specifier
1650 * @returns false if parsing failed
0+ * [@param](/bitcoin-bitcoin/contributor/param/)[out] has_hardened updated if hardened derivation is found
68@@ -69,6 +69,7 @@ Output descriptors currently support:
69 - `tr(c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5,sortedmulti_a(2,2f8bde4d1a07209355b4a7250a5c5128e88b84bddc619ab7cba8d569b240efe4,5cbdf0646e5db4eaa398f365f2ea7a0e3d419b7e0330e39ce92bddedcac4f9bc))` describes a P2TR output with the `c6...` x-only pubkey as internal key, and a single `multi_a` script that needs 2 signatures with 2 specified x-only keys, which will be sorted lexicographically.
70 - `wsh(sortedmulti(2,[6f53d49c/44h/1h/0h]tpubDDjsCRDQ9YzyaAq9rspCfq8RZFrWoBpYnLxK6sS2hS2yukqSczgcYiur8Scx4Hd5AZatxTuzMtJQJhchufv1FRFanLqUP7JHwusSSpfcEp2/0/*,[e6807791/44h/1h/0h]tpubDDAfvogaaAxaFJ6c15ht7Tq6ZmiqFYfrSmZsHu7tHXBgnjMZSHAeHSwhvjARNA6Qybon4ksPksjRbPDVp7yXA1KjTjSd5x18KHqbppnXP1s/0/*,[367c9cfa/44h/1h/0h]tpubDDtPnSgWYk8dDnaDwnof4ehcnjuL5VoUt1eW2MoAed1grPHuXPDnkX1fWMvXfcz3NqFxPbhqNZ3QBdYjLz2hABeM9Z2oqMR1Gt2HHYDoCgh/0/*))#av0kxgw0` describes a *2-of-3* multisig. For brevity, the internal "change" descriptor accompanying the above external "receiving" descriptor is not included here, but it typically differs only in the xpub derivation steps, ending in `/1/*` for change addresses.
71 - `wsh(thresh(4,pk([7258e4f9/44h/1h/0h]tpubDCZrkQoEU3845aFKUu9VQBYWZtrTwxMzcxnBwKFCYXHD6gEXvtFcxddCCLFsEwmxQaG15izcHxj48SXg1QS5FQGMBx5Ak6deXKPAL7wauBU/0/*),s:pk([c80b1469/44h/1h/0h]tpubDD3UwwHoNUF4F3Vi5PiUVTc3ji1uThuRfFyBexTSHoAcHuWW2z8qEE2YujegcLtgthr3wMp3ZauvNG9eT9xfJyxXCfNty8h6rDBYU8UU1qq/0/*),s:pk([4e5024fe/44h/1h/0h]tpubDDLrpPymPLSCJyCMLQdmcWxrAWwsqqssm5NdxT2WSdEBPSXNXxwbeKtsHAyXPpLkhUyKovtZgCi47QxVpw9iVkg95UUgeevyAqtJ9dqBqa1/0/*),s:pk([3b1d1ee9/44h/1h/0h]tpubDCmDTANBWPzf6d8Ap1J5Ku7J1Ay92MpHMrEV7M5muWxCrTBN1g5f1NPcjMEL6dJHxbvEKNZtYCdowaSTN81DAyLsmv6w6xjJHCQNkxrsrfu/0/*),sln:after(840000),sln:after(1050000),sln:after(1260000)))#k28080kv` describes a Miniscript multisig with spending policy: `thresh(4,pk(key_1),pk(key_2),pk(key_3),pk(key_4),after(t1),after(t2),after(t3))` that starts as 4-of-4 and "decays" to 3-of-4, 2-of-4, and finally 1-of-4 at each future halvening block height. For brevity, the internal "change" descriptor accompanying the above external "receiving" descriptor is not included here, but it typically differs only in the xpub derivation steps, ending in `/1/*` for change addresses.
72+- `tr(musig(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y)/0/*)` describes a MuSig2 multisig with key derivation. The internal keys are derived at `m/0/*` from the aggregate key computed from the 2 participants.
The internal keys
I assume it’s referring to the taproot internal key(s).
196@@ -197,20 +197,26 @@ constexpr XOnlyPubKey XOnlyPubKey::NUMS_H{
197 []() consteval { return XOnlyPubKey{"50929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0"_hex_u8}; }(),
198 };
199
200-std::vector<CKeyID> XOnlyPubKey::GetKeyIDs() const
201+std::vector<CPubKey> XOnlyPubKey::GetCPubKeys() const
GetKeyIDs
, I see this function is actually used in the Musig signing PR.
ACK 5fe7915c865a8e7c0a95ec376d0f1ff737f5d1c2
TYVM for patiently addressing all the comments in this big PR!
In the tests: Test musig() parsing
commit, I have looked at all the unit test vectors but I have only glanced through the changes in the DoCheck
function in the descriptor_tests
for now.
re-ACK 5fe7915c865a8e7c0a95ec376d0f1ff737f5d1c2 🎹
(as per $ git range-diff cf6e417...5fe7915
, changes were mostly tackling rkrux’ suggestions, which I agree are all improvements 👍 )
micro-nit: not worth it now to further invalidate acks, but fwiw, I’m still not a huge fan of cluttering the ParseKeyPath{,num}
functions with extra booleans to restrict/detect properties that can be trivially detected after and think the previously suggested approach (IsKeyPathsHardened
helper) was fine.