descriptors: MuSig2

achow101 commented at 6:10 pm on November 7, 2024: member

Implements parsing of BIP 390 musig() descriptors.

Split from #29675

DrahtBot commented at 6:10 pm on November 7, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31244.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	theStack, w0xlt, Sjors

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:

#bitcoin-core/gui/872 (Menu action to export a watchonly wallet by achow101)
#32489 (wallet: Add exportwatchonlywallet RPC to export a watchonly version of a wallet by achow101)
#32471 (wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key by Eunovo)
#32332 (refactor: Update XOnlyPubKey::GetKeyIDs() to return a pair of pubkeys by w0xlt)
#30243 (descriptors: taproot partial descriptors by Eunovo)
#29675 (wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys 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.

DrahtBot added the label Descriptors on Nov 7, 2024

Sjors commented at 1:54 pm on November 26, 2024: member

Can you add an example to doc/descriptors.md?

Did you mean BIP390 in the description?

dergoegge commented at 10:06 am on December 9, 2024: member

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.

achow101 force-pushed on Dec 9, 2024

achow101 commented at 10:34 pm on December 9, 2024: member

Fixed the fuzz crashes and added those descriptors to the unit tests.

dergoegge commented at 9:51 am on December 10, 2024: member

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.

dergoegge commented at 10:19 am on December 10, 2024: member

0$ echo "dHIobXVzaWcoKS8wMDAwMTEp" | base64 --decode > descriptor_parse_1.crash
1$ FUZZ=descriptor_parse fuzz descriptor_parse_1.crash
2[libsecp256k1] illegal argument: pubkeys != NULL

achow101 force-pushed on Dec 10, 2024

achow101 commented at 5:16 pm on December 10, 2024: member

Fixed those crashes too.

dergoegge commented at 7:41 pm on December 10, 2024: member

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.

achow101 force-pushed on Dec 30, 2024

DrahtBot commented at 7:46 pm on December 30, 2024: contributor

🚧 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.

DrahtBot added the label CI failed on Dec 30, 2024

achow101 force-pushed on Jan 3, 2025

DrahtBot removed the label CI failed on Jan 3, 2025

achow101 force-pushed on Jan 6, 2025

DrahtBot added the label Needs rebase on Jan 21, 2025

achow101 force-pushed on Jan 21, 2025

DrahtBot removed the label Needs rebase on Jan 21, 2025

DrahtBot added the label Needs rebase on Mar 20, 2025

achow101 force-pushed on Apr 10, 2025

DrahtBot added the label CI failed on Apr 10, 2025

DrahtBot commented at 10:07 pm on April 10, 2025: contributor

🚧 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.

achow101 force-pushed on Apr 10, 2025

DrahtBot removed the label Needs rebase on Apr 10, 2025

achow101 force-pushed on Apr 14, 2025

DrahtBot removed the label CI failed on Apr 14, 2025

DrahtBot added the label Needs rebase on Apr 21, 2025

achow101 force-pushed on Apr 21, 2025

achow101 marked this as ready for review on Apr 21, 2025

achow101 commented at 9:37 pm on April 21, 2025: member

All dependencies have been merged, ready for review.

DrahtBot removed the label Needs rebase on Apr 21, 2025

in src/script/signingprovider.cpp:94 in 7da3e7bdd0 outdated

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;

theStack commented at 2:20 pm on April 26, 2025:

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;

achow101 commented at 9:33 pm on May 7, 2025:

Done

in src/musig.h:11 in 2e6dcdbc80 outdated

 5+#ifndef BITCOIN_MUSIG_H
 6+#define BITCOIN_MUSIG_H
 7+
 8+#include <pubkey.h>
 9+
10+#include <vector>

theStack commented at 2:27 pm on April 26, 2025:

nit: should also include <optional>, since it’s used for the return types below

0#include <optional>
1#include <vector>

achow101 commented at 9:33 pm on May 7, 2025:

Done

in src/script/descriptor.cpp:603 in b70531cac9 outdated

598+    bool IsRangedParticipants() const
599+    {
600+        for (const auto& pubkey : m_participants) {
601+            if (pubkey->IsRange()) return true;
602+        }
603+        return false;

theStack commented at 2:29 pm on April 26, 2025:

nit: could turn this into a one-liner by using std::any_of

achow101 commented at 9:33 pm on May 7, 2025:

Done

in src/script/descriptor.cpp:793 in b70531cac9 outdated

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;

theStack commented at 2:30 pm on April 26, 2025:

nit: could turn this into a one-liner by using std::all_of

achow101 commented at 9:33 pm on May 7, 2025:

Done

theStack commented at 2:39 pm on April 26, 2025: contributor

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.

w0xlt commented at 9:26 pm on April 30, 2025: contributor

Concept ACK.

achow101 force-pushed on May 7, 2025

achow101 commented at 9:34 pm on May 7, 2025: member

Seems like the commit message for 2e6dcdb (“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.

Updated the commit message.

in src/script/descriptor.cpp:643 in 6b634754d2 outdated

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

theStack commented at 1:21 pm on May 15, 2025:

nit: could add a comment where this constant is coming from (https://github.com/bitcoin/bips/blob/master/bip-0328.mediawiki#cite_note-1)

Sjors commented at 1:43 pm on May 16, 2025:

This constant should probably live in musig.h.

achow101 commented at 7:08 pm on May 16, 2025:

Done, added to musig.h

in src/musig.h:22 in 895f0c5ca9 outdated

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

theStack commented at 6:40 pm on May 15, 2025:

note for other reviewers that might wonder: these two functions and the involved secp256k1_musig_keyagg_cache instance are not publicly used within this PR yet, but they will be in #29675 (or any other future PRs that get split up from that one)

in src/pubkey.h:292 in 225b3adbf5 outdated

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;

Sjors commented at 11:05 am on May 16, 2025:

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.

achow101 commented at 7:08 pm on May 16, 2025:

Updated the comments.

in src/script/parsing.h:16 in fe02d7cb23 outdated

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.

Sjors commented at 11:30 am on May 16, 2025:

In fe02d7cb237c00de6abe1776b3101342ffddf757 “script/parsing: Allow Const to not skip the found constant”: what does “sp” stand for anyway?

achow101 commented at 6:23 pm on May 16, 2025:

span?

in src/util/string.h:103 in 4a1eeee27a outdated

 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.

Sjors commented at 11:37 am on May 16, 2025:

In 4a1eeee27a64c4be7293740f8fff839879b88d86 “util/string: Allow Split to include the separator”: the commit message would be more clear if you use the same wording as in this comment, i.e. “include the separator at the end of the left side of the splits”.

achow101 commented at 7:08 pm on May 16, 2025:

Done

in src/util/string.h:109 in 4a1eeee27a outdated

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)"}.

Sjors commented at 11:42 am on May 16, 2025:

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)

achow101 commented at 7:08 pm on May 16, 2025:

Done

in src/script/descriptor.cpp:225 in 26c25fed91 outdated

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 */

Sjors commented at 11:45 am on May 16, 2025:

In 26c25fed919d2520f561a891236220d54880b079 “descriptors: Add PubkeyProvider::IsBIP32()”: “can be” a BIP 32 extended key or “is”? In the former case, an additional comment would be useful.

achow101 commented at 7:09 pm on May 16, 2025:

“is”

in src/musig.h:24 in 895f0c5ca9 outdated

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

Sjors commented at 11:54 am on May 16, 2025:

In 895f0c5ca9a12df843f2f7faff37c948046654ea “Add MuSig2 Keyagg Cache helper functions”: why is this plural? IIUC there’s one aggregate key that’s derived from the participant keys. If so, then maybe call this MuSig2DeriveAggregatePubkey()

achow101 commented at 6:34 pm on May 16, 2025:

“Aggregate” is used as an imperative verb here, so it is “(you,) aggregate (these) pubkeys”

Sjors commented at 7:14 am on May 19, 2025:

Ah, it’s a verb, then it makes sense.

in src/musig.h:20 in 895f0c5ca9 outdated

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

Sjors commented at 12:57 pm on May 16, 2025:

In 895f0c5ca9a12df843f2f7faff37c948046654ea “Add MuSig2 Keyagg Cache helper functions”: although we don’t have to copy-paste all libsecp documentation, I think it’s worth mentioning that this doesn’t sort pubkeys and the order matters.

achow101 commented at 7:09 pm on May 16, 2025:

Added comments.

in src/script/descriptor.cpp:768 in 6b634754d2 outdated

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

Sjors commented at 1:22 pm on May 16, 2025:

In 6b634754d2783ffe16570ad37dbcf9251a7efc24 “descriptor: Add MuSigPubkeyProvider”: could add a short comment to explain why this doesn’t exist

achow101 commented at 7:09 pm on May 16, 2025:

Done

in src/script/descriptor.cpp:759 in 6b634754d2 outdated

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

Sjors commented at 1:23 pm on May 16, 2025:

In 6b634754d2783ffe16570ad37dbcf9251a7efc24 “descriptor: Add MuSigPubkeyProvider”: do you mean “all or nothing” or “any that we have”?

achow101 commented at 7:09 pm on May 16, 2025:

any that we have.

in src/script/descriptor.cpp:730 in 6b634754d2 outdated

725+            } else {
726+                out += pubkey->ToString();
727+            }
728+        }
729+        out += ")";
730+        out += FormatHDKeypath(m_path, /*apostrophe=*/true);

Sjors commented at 1:25 pm on May 16, 2025:

In https://github.com/bitcoin/bitcoin/commit/6b634754d2783ffe16570ad37dbcf9251a7efc24 “descriptor: why apostrophe? 😢

achow101 commented at 6:46 pm on May 16, 2025:

It doesn’t actually matter, the parameter does nothing as you can’t do hardened derivation from the musig aggregate key.

Sjors commented at 7:16 am on May 19, 2025:

In that case it’s less confusing if you don’t pass this argument (as the default is false).

Sjors commented at 7:36 am on May 19, 2025:

Update: I see you already did that in 048c95ea55.

Sjors commented at 1:58 pm on May 16, 2025: member

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.

achow101 force-pushed on May 16, 2025

in src/script/descriptor.cpp:669 in 048c95ea55 outdated

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

theStack commented at 5:52 pm on May 19, 2025:

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

achow101 commented at 6:24 pm on May 20, 2025:

Done, with 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.

in src/musig.h:17 in 048c95ea55 outdated

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"};

theStack commented at 6:02 pm on May 19, 2025:

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 😅 )

Sjors commented at 6:41 pm on May 19, 2025:

Indeed it’s nice if we can avoid this, because although reviewing the reversal was easy, finding it later with search is tedious.

achow101 commented at 6:24 pm on May 20, 2025:

It looks like _hex_u8 can be used directly in uin256’s constructor with normal byte order, so used that.

achow101 force-pushed on May 20, 2025

XOnlyPubKey: Add GetCPubKeys

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.

5fe4c66462

script/parsing: Allow Const to not skip the found constant 4b135f80d0

util/string: Allow Split to include the separator

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.

b89a937225

descriptors: Add PubkeyProvider::IsBIP32() 81fe05d65f

build: Enable secp256k1 musig module fc1e8b63b6

sign: Add GetAggregateParticipantPubkeys to SigningProvider b9c4096beb

Add MuSig2 Keyagg Cache helper functions

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.

18aa775230

in src/musig.h:20 in 08d8a59597 outdated

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

DrahtBot commented at 9:37 am on May 21, 2025:

secp56k1 -> secp256k1 [typo in library name]

achow101 commented at 5:55 pm on May 21, 2025:

Done

achow101 force-pushed on May 21, 2025

DrahtBot added the label CI failed on May 21, 2025

in src/script/descriptor.cpp:782 in 35db4f2dcf outdated

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

Sjors commented at 9:13 am on May 22, 2025:

In 35db4f2dcfc3435e10935581ffa447ffe219cc1e “descriptor: Add MuSigPubkeyProvider”: no tests break if I delete this loop. What’s the typical use case for Clone()? Maybe Check() in descriptor_tests.cpp should do this for all valid descriptors?

achow101 commented at 6:09 pm on May 22, 2025:

It is used for multipath descriptors, tests of which are added 2 commits later. We do not (and cannot) directly test the actual 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.

Sjors commented at 7:45 am on May 23, 2025:

Just to clarify my initial comment: I can remove this loop in the final version of this PR without breaking any test.

achow101 commented at 7:19 pm on May 23, 2025:

Ah. I’ve added a test which exercises this code.

in src/script/descriptor.cpp:715 in 35db4f2dcf outdated

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

Sjors commented at 9:45 am on May 22, 2025:

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.

achow101 commented at 6:11 pm on May 22, 2025:

I find that harder to read. for i is used here because we need the position.

in src/script/descriptor.cpp:622 in 35db4f2dcf outdated

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

Sjors commented at 10:08 am on May 22, 2025:

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?

achow101 commented at 6:11 pm on May 22, 2025:

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.

Sjors commented at 7:48 am on May 23, 2025:

Being able to generate a (derived) descriptor without parsing is still on my wish list: #24003

But even in that case it could generate, convert to string and then re-parse as a sanity check.

theStack commented at 4:45 pm on May 26, 2025:

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     {

achow101 commented at 6:28 pm on May 26, 2025:

Added an Assume.

Sjors commented at 10:08 am on May 22, 2025: member

Some more questions about the MuSigPubkeyProvider introduced in 35db4f2dcfc3435e10935581ffa447ffe219cc1e.

DrahtBot removed the label CI failed on May 23, 2025

in src/musig.h:17 in 35db4f2dcf outdated

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};

theStack commented at 12:29 pm on May 23, 2025:

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};

achow101 commented at 6:27 pm on May 23, 2025:

Done

in src/script/descriptor.cpp:637 in 35db4f2dcf outdated

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

theStack commented at 12:32 pm on May 23, 2025:

in commit 35db4f2dcfc3435e10935581ffa447ffe219cc1e: nit, could use the helper here:

0            if (IsRangedDerivation() || !m_path.empty()) {

achow101 commented at 6:27 pm on May 23, 2025:

Done

in src/script/descriptor.cpp:1657 in 739f13986a outdated

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)

theStack commented at 12:47 pm on May 23, 2025:

in commit 739f13986a49d8d7d5568ebcdd88665ea8423d42: could add a doxygen comment for the newly introduced parameter

achow101 commented at 6:27 pm on May 23, 2025:

Done

in src/script/descriptor.cpp:1807 in 739f13986a outdated

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)

theStack commented at 1:20 pm on May 23, 2025:

Why is the 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.

achow101 commented at 6:27 pm on May 23, 2025:

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.

theStack commented at 4:50 pm on May 26, 2025:

That makes sense. I only realize now that I was talking about code paths in two different functions (ParsePubkey vs ParseScript; for the latter the key_exp_index argument was already passed by reference).

achow101 force-pushed on May 23, 2025

achow101 force-pushed on May 26, 2025

in src/script/descriptor.cpp:616 in e1475cbd0d outdated

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

maflcko commented at 12:54 pm on May 27, 2025:

participatns -> participants [correct misspelling in error message]

achow101 commented at 6:13 pm on May 27, 2025:

Fixed

in src/script/descriptor.cpp:710 in ff7476ec32 outdated

705+                    break;
706+                case StringType::COMPAT:
707+                    tmp = pubkey->ToString(PubkeyProvider::StringType::COMPAT);
708+                    break;
709+            }
710+            out += tmp;

theStack commented at 4:44 pm on May 27, 2025:

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.

achow101 commented at 6:13 pm on May 27, 2025:

Good point, done.

theStack commented at 5:26 pm on May 27, 2025: contributor

Reviewed thoroughly up to ff7476ec32f4dbee07bf04169e741e2aea5a9ab7, looks good to me (with one refactoring suggestion below). Still need to go again through the parsing code as there is a lot going on with all the conditions on what is allowed and what not w.r.t. derivations.

descriptor: Add MuSigPubkeyProvider 16a88829e9

descriptor: Parse musig() key expressions 87a3576c97

tests: Test musig() parsing c86f99ba32

doc: Add musig() example c30c297461

achow101 force-pushed on May 27, 2025

descriptors: MuSig2 #31244

Code Coverage & Benchmarks

Reviews

Conflicts