descriptors: MuSig2

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

Implements parsing of BIP 390 musig() descriptors.

Depends on #31243

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

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:

#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)
#29491 ([EXPERIMENTAL] Schnorr batch verification for blocks by fjahr)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

In descriptor.cpp comment “passthrough to it’s GetPubKey”: change “it’s” to “its”.
No other typos affecting comprehension were found.

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

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.

225b3adbf5

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

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 on the left side of the
splits, i.e. it appears at the end of the splits, except for the last
one.

4a1eeee27a

descriptors: Add PubkeyProvider::IsBIP32() 26c25fed91

build: Enable secp256k1 musig module 2d4f2f639f

sign: Add GetAggregateParticipantPubkeys to SigningProvider 7da3e7bdd0

Add MuSig2 Keyagg Cache class and functions

- MuSig2KeyAggCache contains a MuSig2KeyAggCacheImpl which has the
  secp256ke_musig_keyagg_cache object to avoid having to link libsecp256k1
  everywhere.
- GetMuSig2KeyAggCache creates the MuSig2KeyAggCache from a
  std::vector<CPubKey>
- GetCPubKeyFromMuSig2KeyAggCache creates a CPubKey from a cache created
  with GetMuSig2KeyAggCache
- MuSig2AggregatePubKeys does the two above functions together.

2e6dcdbc80

descriptor: Add MuSigPubkeyProvider b70531cac9

descriptor: Parse musig() key expressions 46f1a8257c

tests: Test musig() parsing e7c67ec86d

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;

in src/musig.h:10 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>

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

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

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.

descriptors: MuSig2 #31244

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)