descriptors: MuSig2 #31244

pull achow101 wants to merge 12 commits into bitcoin:master from achow101:musig2-desc changing 14 files +752 −51
  1. achow101 commented at 6:10 pm on November 7, 2024: member

    Implements parsing of BIP 390 musig() descriptors.

    Split from #29675

  2. 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
    ACK w0xlt, rkrux, theStack
    Concept ACK 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)
    • #32745 (scripted-diff: Update DeriveType enum values to mention ranged derivations by rkrux)
    • #32724 (Musig2 tests by w0xlt)
    • #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.

  3. DrahtBot added the label Descriptors on Nov 7, 2024
  4. 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?

  5. 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.
    
  6. achow101 force-pushed on Dec 9, 2024
  7. achow101 commented at 10:34 pm on December 9, 2024: member
    Fixed the fuzz crashes and added those descriptors to the unit tests.
  8. 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.
    
  9. 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
    
  10. achow101 force-pushed on Dec 10, 2024
  11. achow101 commented at 5:16 pm on December 10, 2024: member
    Fixed those crashes too.
  12. 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.
    
  13. achow101 force-pushed on Dec 30, 2024
  14. 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.

  15. DrahtBot added the label CI failed on Dec 30, 2024
  16. achow101 force-pushed on Jan 3, 2025
  17. DrahtBot removed the label CI failed on Jan 3, 2025
  18. achow101 force-pushed on Jan 6, 2025
  19. DrahtBot added the label Needs rebase on Jan 21, 2025
  20. achow101 force-pushed on Jan 21, 2025
  21. DrahtBot removed the label Needs rebase on Jan 21, 2025
  22. DrahtBot added the label Needs rebase on Mar 20, 2025
  23. achow101 force-pushed on Apr 10, 2025
  24. DrahtBot added the label CI failed on Apr 10, 2025
  25. 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.

  26. achow101 force-pushed on Apr 10, 2025
  27. DrahtBot removed the label Needs rebase on Apr 10, 2025
  28. achow101 force-pushed on Apr 14, 2025
  29. achow101 force-pushed on Apr 14, 2025
  30. DrahtBot removed the label CI failed on Apr 14, 2025
  31. DrahtBot added the label Needs rebase on Apr 21, 2025
  32. achow101 force-pushed on Apr 21, 2025
  33. achow101 marked this as ready for review on Apr 21, 2025
  34. achow101 commented at 9:37 pm on April 21, 2025: member
    All dependencies have been merged, ready for review.
  35. DrahtBot removed the label Needs rebase on Apr 21, 2025
  36. 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
  37. 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
  38. 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
  39. 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
  40. 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.

  41. w0xlt commented at 9:26 pm on April 30, 2025: contributor
    Concept ACK.
  42. achow101 force-pushed on May 7, 2025
  43. 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.

  44. 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
  45. 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)
  46. 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.
  47. 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?
  48. 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
  49. 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
  50. 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”
  51. 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.
  52. 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.
  53. in src/script/descriptor.cpp:767 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
  54. 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.
  55. 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:

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

  57. achow101 force-pushed on May 16, 2025
  58. 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.
  59. 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.
  60. achow101 force-pushed on May 20, 2025
  61. 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
  62. 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
  63. achow101 force-pushed on May 21, 2025
  64. DrahtBot added the label CI failed on May 21, 2025
  65. in src/script/descriptor.cpp:781 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.
  66. in src/script/descriptor.cpp:714 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.
  67. in src/script/descriptor.cpp:624 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.
  68. Sjors commented at 10:08 am on May 22, 2025: member
    Some more questions about the MuSigPubkeyProvider introduced in 35db4f2dcfc3435e10935581ffa447ffe219cc1e.
  69. DrahtBot removed the label CI failed on May 23, 2025
  70. 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
  71. 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
  72. in src/script/descriptor.cpp:1660 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
  73. in src/script/descriptor.cpp:1815 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).
  74. achow101 force-pushed on May 23, 2025
  75. achow101 force-pushed on May 23, 2025
  76. achow101 force-pushed on May 26, 2025
  77. 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
  78. 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.
  79. 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.
  80. achow101 force-pushed on May 27, 2025
  81. theStack commented at 5:00 pm on June 1, 2025: contributor
    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): https://github.com/theStack/bitcoin/commit/f260c7ec06e8bbb7148877a9a9b7e96707c41fa1
  82. achow101 force-pushed on Jun 2, 2025
  83. achow101 commented at 8:13 pm on June 2, 2025: member

    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.

  84. in src/script/descriptor.cpp:591 in 16a88829e9 outdated
    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;
    


    theStack commented at 12:42 pm on June 3, 2025:

    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?


    achow101 commented at 9:14 pm on June 3, 2025:
    Done
  85. in src/script/descriptor.cpp:1837 in 9e3a1d05bf outdated
    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)) {
    


    theStack commented at 1:07 pm on June 3, 2025:

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

    achow101 commented at 9:14 pm on June 3, 2025:
    Done
  86. in src/script/descriptor.cpp:1879 in 9e3a1d05bf outdated
    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";
    


    theStack commented at 1:15 pm on June 3, 2025:

    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.


    achow101 commented at 9:14 pm on June 3, 2025:
    Done
  87. in src/script/descriptor.cpp:1860 in 9e3a1d05bf outdated
    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+            }
    


    theStack commented at 1:25 pm on June 3, 2025:
    in commit 9e3a1d05bf066df31a6e124638046e70669eb470: nit: A more thorough check would be to detect duplicates of actual resulting pubkeys, rather than only the expression strings (i.e. for a keypair (priv,pub), the expression musig(priv,pub) would now still be valid).

    achow101 commented at 8:43 pm on June 3, 2025:

    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.


    Sjors commented at 7:59 am on June 4, 2025:

    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.


    rkrux commented at 1:52 pm on June 4, 2025:

    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.


    achow101 commented at 3:59 pm on June 4, 2025:

    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.


    rkrux commented at 1:58 pm on June 5, 2025:

    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.


    achow101 commented at 6:54 pm on June 5, 2025:

    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.


    rkrux commented at 12:03 pm on June 25, 2025:

    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.

  88. achow101 force-pushed on Jun 3, 2025
  89. in src/script/descriptor.cpp:1817 in 8a2dc4662c outdated
    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()";
    


    rkrux commented at 1:38 pm on June 4, 2025:
    0- "musig() is only allowed in tr()"
    1+ "musig() is allowed in only tr() and rawtr()"
    

    achow101 commented at 9:31 pm on June 4, 2025:
    Done
  90. in src/script/descriptor.cpp:1877 in 8a2dc4662c outdated
    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";
    


    rkrux commented at 1:39 pm on June 4, 2025:
    0- musig(): musig() derivation requires all participants to be xpubs
    1+ musig(): derivation requires all participants to be xpubs or xprvs
    

    achow101 commented at 9:31 pm on June 4, 2025:
    Done
  91. in src/script/descriptor.cpp:1860 in 8a2dc4662c outdated
    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();
    


    rkrux commented at 1:43 pm on June 4, 2025:
    Bitwise operations on bools, sorry to be the one raising this multiple times. :)

    achow101 commented at 9:31 pm on June 4, 2025:
    Done
  92. in src/script/descriptor.cpp:1934 in 8a2dc4662c outdated
    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";
    


    rkrux commented at 2:00 pm on June 4, 2025:

    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?


    achow101 commented at 9:02 pm on June 4, 2025:
    Since multipath is similar to ranged expansion, I’ve applied the same restrictions that are on ranged to multipath as well. I suppose this should be mentioned in the BIP.

    rkrux commented at 1:12 pm on June 5, 2025:
    I raised a PR for it here: https://github.com/bitcoin/bips/pull/1866
  93. in src/script/descriptor.cpp:601 in 66f5405f5f outdated
    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+    }
    


    rkrux commented at 2:06 pm on June 4, 2025:
    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.

    achow101 commented at 9:31 pm on June 4, 2025:
    Done
  94. in src/script/descriptor.cpp:605 in 66f5405f5f outdated
    603+public:
    604+    MuSigPubkeyProvider(
    605+        uint32_t exp_index,
    606+        std::vector<std::unique_ptr<PubkeyProvider>> providers,
    607+        KeyPath path,
    608+        DeriveType derive
    


    rkrux commented at 2:08 pm on June 4, 2025:
    Can add 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.

    achow101 commented at 9:32 pm on June 4, 2025:
    Done
  95. in src/script/descriptor.cpp:668 in 66f5405f5f outdated
    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
    


    rkrux commented at 2:08 pm on June 4, 2025:
    0- Either way, we can passthrough to it's GetPubKey
    1+ Either way, we can passthrough to its GetPubKey
    

    achow101 commented at 9:32 pm on June 4, 2025:
    Done
  96. in src/script/descriptor.cpp:655 in 66f5405f5f outdated
    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+        }
    


    rkrux commented at 2:20 pm on June 4, 2025:
    Nit: The calculation of aggregate public key & its provider can be done in the constructor itself unless it’s anticipated that for some reason dummy MuSigPubkeyProviders will be used later.

    achow101 commented at 9:22 pm on June 4, 2025:
    It’s deferred to GetPubKey as the participants may involve hardened derivation which requires private keys that the constructor doesn’t have access to.

    rkrux commented at 2:19 pm on June 5, 2025:
    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.

  97. rkrux commented at 2:30 pm on June 4, 2025: contributor
    Partial code review 981b2abd79e3a7a4701c9d8794057b5523acb177 I just started and am still reviewing.
  98. achow101 force-pushed on Jun 4, 2025
  99. in src/script/descriptor.cpp:670 in 96af090b97 outdated
    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);
    


    rkrux commented at 3:15 pm on June 5, 2025:
    IIUC, I think we should explicitly pass a dummy signing provider here instead of 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.

    achow101 commented at 7:13 pm on June 5, 2025:
    Done
  100. in src/script/descriptor.cpp:684 in 96af090b97 outdated
    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);
    


    rkrux commented at 3:28 pm on June 5, 2025:

    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?


    achow101 commented at 7:02 pm on June 5, 2025:
    It’s disallowed. If there is derivation done on the aggregate at all, there cannot be ranged participants. This is not explicitly stated as disallowed in the BIP, but it is not stated as being allowed. It should be implied as the only form of musig() with aggregate derivation that the BIP describes explicitly disallows ranged participants.
  101. in src/script/descriptor.cpp:617 in 96af090b97 outdated
    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");
    


    rkrux commented at 3:31 pm on June 5, 2025:
    0- Cannot have hardened hardened derivation
    1+ Cannot have hardened derivation
    

    achow101 commented at 7:13 pm on June 5, 2025:
    Done
  102. in src/script/descriptor.cpp:616 in 96af090b97 outdated
    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)) {
    


    rkrux commented at 3:31 pm on June 5, 2025:
    Interesting, I now understand why it’s written in a double negation way.

    w0xlt commented at 6:52 pm on June 11, 2025:
    This way, the derivation type is equal to 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.
  103. in src/script/descriptor.cpp:676 in 96af090b97 outdated
    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
    


    rkrux commented at 3:46 pm on June 5, 2025:
    Aren’t the participant pubkeys already derived in the loop on line 659? Here it seems to only aggregate the derived keys.

    achow101 commented at 7:13 pm on June 5, 2025:
    Changed the comment.
  104. in src/script/descriptor.cpp:664 in 96af090b97 outdated
    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());
    


    rkrux commented at 6:46 pm on June 5, 2025:

    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.


    achow101 commented at 7:11 pm on June 5, 2025:
    Even with aggregate derivation, 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.

    rkrux commented at 1:04 pm on June 24, 2025:

    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.


    achow101 commented at 8:16 pm on June 24, 2025:
    Non-ranged participants generally won’t be rederived every time because of the read_cache.
  105. achow101 force-pushed on Jun 5, 2025
  106. in src/script/parsing.h:19 in 4b135f80d0 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.
    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);
    


    w0xlt commented at 11:26 pm on June 10, 2025:
    Maybe a test in src/test/util_tests.cpp in BOOST_AUTO_TEST_CASE(test_script_parsing) would be good for consistency’s sake.

    achow101 commented at 6:05 pm on June 11, 2025:
    Done
  107. w0xlt commented at 11:43 pm on June 10, 2025: contributor
    Perhaps the description of the commits 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.
  108. in src/script/signingprovider.h:164 in af27ffbce3 outdated
    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 {}; }
    


    w0xlt commented at 11:58 pm on June 10, 2025:

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

    achow101 commented at 6:05 pm on June 11, 2025:
    Changed to GetMuSig2ParticipantPubkeys
  109. in src/musig.cpp:25 in 18aa775230 outdated
    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())) {
    


    w0xlt commented at 0:06 am on June 11, 2025:
    I wonder if this function should have an output parameter for 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.

    achow101 commented at 5:58 pm on June 11, 2025:

    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.

  110. achow101 force-pushed on Jun 11, 2025
  111. script/parsing: Allow Const to not skip the found constant
    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.
    8811312571
  112. 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.
    
    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.
    12bc1d0b1e
  113. descriptors: Add PubkeyProvider::IsBIP32() 1894f97503
  114. build: Enable secp256k1 musig module fac0ee0bfc
  115. sign: Add GetMuSig2ParticipantPubkeys to SigningProvider 8ecea91bf2
  116. 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.
    d00d95437d
  117. achow101 force-pushed on Jun 11, 2025
  118. achow101 commented at 6:12 pm on June 11, 2025: member

    Perhaps the description of the commits script/parsing: Allow Const to not skip the found constant (4b135f8) and util/string: Allow Split to include the separator (b89a937) could explain why these changes are necessary for the musig2 descriptor.

    Done

  119. in src/script/descriptor.cpp:635 in f1b0fded41 outdated
    630+                if (!pubkey.has_value()) {
    631+                    return std::nullopt;
    632+                }
    633+                pubkeys.push_back(pubkey.value());
    634+            }
    635+            std::sort(pubkeys.begin(), pubkeys.end());
    


    w0xlt commented at 6:56 pm on June 11, 2025:
    Is std::sort(pubkeys.begin(), pubkeys.end()) equivalent to secp256k1_ec_pubkey_sort ?

    achow101 commented at 7:46 pm on June 11, 2025:
    Yes. They both essentially do memcmp on the serialized pubkey
  120. in src/script/descriptor.cpp:646 in adeeacb49f outdated
    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);
    


    w0xlt commented at 8:17 pm on June 11, 2025:
    nit: if you don’t want to hardcode vchFingerprint size: std::fill(std::begin(extpub.vchFingerprint), std::end(extpub.vchFingerprint), 0);
  121. DrahtBot requested review from theStack on Jun 11, 2025
  122. DrahtBot requested review from Sjors on Jun 11, 2025
  123. in src/script/signingprovider.h:218 in 8ecea91bf2 outdated
    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 */
    


    theStack commented at 10:40 pm on June 11, 2025:

    in commit 8ecea91bf296b8fae8b84c3dbf68d5703821cb79: comment nit

    0    std::map<CPubKey, std::vector<CPubKey>> aggregate_pubkeys; /** Map from MuSig2 aggregate pubkey to participant pubkeys */
    
  124. in src/script/descriptor.cpp:789 in adeeacb49f outdated
    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+    }
    


    theStack commented at 11:18 pm on June 11, 2025:
    in commit adeeacb49f11eec3dd31327cc33b094b7a5601d6: just a small note, since I didn’t notice it in previous review rounds: it seems this method contains dead code in practice for this class, as currently .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.

    achow101 commented at 6:51 pm on June 12, 2025:
    I prefer to leave it for correctness.
  125. theStack approved
  126. theStack commented at 11:59 pm on June 11, 2025: contributor
    Code-review ACK f1b0fded41223c9b5b92696a0cfa553a10f3d9bf (modulo the outstanding question on how to cope with duplicate participant pubkeys, as https://github.com/bitcoin/bips/pull/1867 isn’t merged yet; very likely the repeated key expression detection in 1e738242a8c642805b7cdbab51a21ee5a298bbab can be removed)
  127. rkrux referenced this in commit 2a064f0a7a on Jun 13, 2025
  128. rkrux referenced this in commit a11bcca960 on Jun 13, 2025
  129. achow101 force-pushed on Jun 17, 2025
  130. achow101 force-pushed on Jun 17, 2025
  131. achow101 commented at 2:15 am on June 17, 2025: member
    #29675 revealed a bug in ToNormalizedString which is fixed in the latest push.
  132. DrahtBot requested review from theStack on Jun 17, 2025
  133. in src/script/descriptor.cpp:1896 in c2e931653d outdated
    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)) {
    


    theStack commented at 2:47 pm on June 18, 2025:
    nit: I think a slightly cleaner approach to implement the “disallow hardened derivation” rule would be to not clutter the keypath parsing interface with another parameter, but check the resulting paths object here instead (with e.g. a new IsKeyPathHardened helper).

    achow101 commented at 9:47 pm on June 18, 2025:
    Done as suggested.
  134. theStack approved
  135. theStack commented at 2:50 pm on June 18, 2025: contributor
    re-ACK 1e205ca66c97295eca21f61dc309e8403814e802
  136. achow101 force-pushed on Jun 18, 2025
  137. achow101 commented at 9:47 pm on June 18, 2025: member
    The BIP change was merged, so dropped the duplicate check
  138. DrahtBot requested review from theStack on Jun 18, 2025
  139. DrahtBot added the label CI failed on Jun 19, 2025
  140. DrahtBot commented at 0:15 am on June 19, 2025: contributor

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

  141. achow101 force-pushed on Jun 19, 2025
  142. DrahtBot removed the label CI failed on Jun 19, 2025
  143. in src/script/descriptor.cpp:1945 in 677639d6dd outdated
    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
    


    theStack commented at 2:30 pm on June 20, 2025:
    0                // Final MuSigPubkeyProvider uses participant pubkey providers at each multipath position, and the first (and only) path
    

    achow101 commented at 7:33 pm on June 20, 2025:
    Done
  144. in src/script/descriptor.cpp:1959 in 677639d6dd outdated
    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
    


    theStack commented at 2:31 pm on June 20, 2025:
    0            // No multipath derivation, MuSigPubkeyProvider uses the first (and only) participant pubkey providers, and the first (and only) path
    

    achow101 commented at 7:33 pm on June 20, 2025:
    Done
  145. theStack approved
  146. theStack commented at 2:31 pm on June 20, 2025: contributor

    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.

  147. DrahtBot requested review from w0xlt on Jun 20, 2025
  148. achow101 force-pushed on Jun 20, 2025
  149. achow101 force-pushed on Jun 20, 2025
  150. DrahtBot added the label CI failed on Jun 20, 2025
  151. DrahtBot removed the label CI failed on Jun 20, 2025
  152. DrahtBot requested review from theStack on Jun 20, 2025
  153. theStack approved
  154. theStack commented at 11:33 am on June 21, 2025: contributor
    re-ACK cf6e4170fa0529c4c238b7f185fc708e94dec34f
  155. in src/script/descriptor.cpp:1881 in effe4fcc59 outdated
    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";
    


    rkrux commented at 12:53 pm on June 23, 2025:

    #31244 (review)

    I think this was done earlier but for some reason I don’t see xprvs mentioned now.


    achow101 commented at 8:43 pm on June 24, 2025:
    Fixed
  156. in src/script/descriptor.cpp:1823 in effe4fcc59 outdated
    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)) {
    


    rkrux commented at 1:02 pm on June 23, 2025:

    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.


    achow101 commented at 8:44 pm on June 24, 2025:
    Since this if uses ParsePubkey recursively, I prefer to have it be in ParsePubkey than adding a circular dependency between ParsePubkey and a ParseMusig.
  157. in src/script/descriptor.cpp:1803 in effe4fcc59 outdated
    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)
    


    rkrux commented at 1:07 pm on June 23, 2025:

    Nit: because this function early returns if any key path is hardened -

    0- IsKeyPathsHardened
    1+ IsAnyKeyPathHardened
    

    achow101 commented at 8:45 pm on June 24, 2025:
    Ended up dropping this funciton
  158. in src/script/descriptor.cpp:1892 in effe4fcc59 outdated
    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))) {
    


    rkrux commented at 1:08 pm on June 23, 2025:
    Preference to use a common utility function IsHardenedIndicator here that can also check for H internally as suggested in the other PR: #32788#pullrequestreview-2949824136

    achow101 commented at 8:45 pm on June 24, 2025:
    Ended up extracting this entire derive type parsing into a separate function.
  159. in src/script/descriptor.cpp:618 in 4039be3f9b outdated
    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+        }
    


    rkrux commented at 1:15 pm on June 23, 2025:
    At the cost of shilling PR #32745, this check in the 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).

    achow101 commented at 7:54 pm on June 24, 2025:
    Yes, that is a known and expected limitation. The purpose is to have cheap checks here as everything should already be checked by the parser. There is no way for these specific PubkeyProviders to be created outside of this file anyways.
  160. in src/script/descriptor.cpp:1883 in effe4fcc59 outdated
    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";
    


    rkrux commented at 1:37 pm on June 23, 2025:

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

  161. in src/script/descriptor.cpp:1855 in effe4fcc59 outdated
    1851+        while (expr.size()) {
    1852+            if (!first && !Const(",", expr)) {
    1853+                error = strprintf("musig(): expected ',', got '%c'", expr[0]);
    1854+                return {};
    1855+            }
    1856+            first = false;
    


    rkrux commented at 1:57 pm on June 23, 2025:

    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        }
    

    achow101 commented at 8:45 pm on June 24, 2025:
    Renamed
  162. in src/script/descriptor.cpp:1901 in effe4fcc59 outdated
    1900+                return {};
    1901+            }
    1902+            if (IsKeyPathsHardened(paths)) {
    1903+                error = "musig(): cannot have hardened derivation steps";
    1904+                return {};
    1905+            }
    


    rkrux commented at 2:31 pm on June 23, 2025:

    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.


    achow101 commented at 8:46 pm on June 24, 2025:
    Added a has_hardened output parameter.
  163. in src/script/descriptor.cpp:1950 in effe4fcc59 outdated
    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+            }
    


    rkrux commented at 3:11 pm on June 23, 2025:

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

    achow101 commented at 8:46 pm on June 24, 2025:
    I’ve changed it to an Assume. I don’t really like having asserts in modules like descriptors.
  164. in src/script/descriptor.cpp:1936 in effe4fcc59 outdated
    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) {
    


    rkrux commented at 3:28 pm on June 23, 2025:

    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.


    achow101 commented at 8:46 pm on June 24, 2025:
    Done, mostly
  165. in src/script/descriptor.cpp:1912 in effe4fcc59 outdated
    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) {
    


    rkrux commented at 3:34 pm on June 23, 2025:
    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.


    achow101 commented at 8:47 pm on June 24, 2025:
    Done, mostly
  166. rkrux commented at 3:51 pm on June 23, 2025: contributor

    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.

  167. in src/script/descriptor.cpp:722 in 4039be3f9b outdated
    721+            if (pubkey->ToPrivateString(arg, tmp)) {
    722+                any_privkeys = true;
    723+                out += tmp;
    724+            } else {
    725+                out += pubkey->ToString();
    726+            }
    


    rkrux commented at 10:44 am on June 24, 2025:
    Nice defaulting to using the public key incase private key is not present. This is a more forward facing solution that aligns with the resolution of the existing issue #32078.
  168. in src/script/descriptor.cpp:677 in 4039be3f9b outdated
    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;
    


    rkrux commented at 11:57 am on June 24, 2025:

    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”


    achow101 commented at 8:47 pm on June 24, 2025:
    Done
  169. in src/script/descriptor.cpp:687 in 4039be3f9b outdated
    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);
    


    rkrux commented at 1:57 pm on June 24, 2025:
    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.


    achow101 commented at 8:47 pm on June 24, 2025:
    Done
  170. rkrux commented at 2:03 pm on June 24, 2025: contributor
    I have finished reviewing the descriptor: Add MuSigPubkeyProvider commit - 4039be3f9b78c1ddb75c04f4a8002d36c4b05f79
  171. descriptor: Add MuSigPubkeyProvider 4af0dca096
  172. descriptors: Move DeriveType parsing into its own function 9473e9606c
  173. descriptor: Parse musig() key expressions a53924bee3
  174. tests: Test musig() parsing d576079ab4
  175. doc: Add musig() example 5fe7915c86
  176. achow101 force-pushed on Jun 24, 2025
  177. DrahtBot requested review from theStack on Jun 24, 2025
  178. in src/script/descriptor.cpp:1872 in a53924bee3 outdated
    1868+            key_exp_index++;
    1869+        }
    1870+        if (any_key_parsed) {
    1871+            error = "musig(): Must contain key expressions";
    1872+            return {};
    1873+        }
    


    rkrux commented at 9:23 am on June 25, 2025:
    Thanks for using a more elaborate name, using 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.
  179. in src/script/descriptor.cpp:1823 in a53924bee3 outdated
    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)) {
    


    rkrux commented at 9:28 am on June 25, 2025:

    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.

  180. in src/script/descriptor.cpp:1949 in a53924bee3 outdated
    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 {};
    


    rkrux commented at 9:33 am on June 25, 2025:
    Fine to use Assume but we still end up with an uncovered code block because it is unreachable.
  181. in src/test/descriptor_tests.cpp:1257 in d576079ab4 outdated
    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");
    


    rkrux commented at 9:40 am on June 25, 2025:

    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.

  182. in src/script/descriptor.cpp:1650 in a53924bee3 outdated
    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
    


    rkrux commented at 9:47 am on June 25, 2025:
    0+ * [@param](/bitcoin-bitcoin/contributor/param/)[out] has_hardened updated if hardened derivation is found
    
  183. in doc/descriptors.md:72 in 5fe7915c86
    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.
    


    rkrux commented at 11:19 am on June 25, 2025:

    The internal keys

    I assume it’s referring to the taproot internal key(s).

  184. in src/pubkey.cpp:200 in 5fe4c66462 outdated
    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
    


    rkrux commented at 12:00 pm on June 25, 2025:
    Review note: Besides the internal use within GetKeyIDs, I see this function is actually used in the Musig signing PR.
  185. rkrux approved
  186. rkrux commented at 12:00 pm on June 25, 2025: contributor

    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.

  187. theStack approved
  188. theStack commented at 2:23 pm on June 25, 2025: contributor

    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.


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-06-28 18:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me