Skip to content

Conversation

w0xlt
Copy link
Contributor

@w0xlt w0xlt commented Jun 11, 2025

Built on #31244

This PR adds explicit tests for Bitcoin Core's MuSig2 interface.

Any issues in musig2.{cpp,h} will likely also be caught by the descriptor tests, but having more detailed tests for the MuSig2 class itself improves test reporting/coverage.

It uses BIP 328 test vectors.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 11, 2025

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/32724.

Reviews

See the guideline for information on the review process.

Type Reviewers
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

No conflicts as of last run.

@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/43856471515
LLM reason (✨ experimental): The CI failure is caused by a trailing whitespace error detected in src/test/musig2.cpp.

Hints

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.

@rkrux
Copy link
Contributor

rkrux commented Jun 12, 2025

I feel this PR should be in draft until #31244 is merged as all the commits here are from that PR except the last one.

Though the last commit can be reviewed separately.

@brunoerg
Copy link
Contributor

Mutation testing report for #32724

223 mutants for src/script/descriptor.cpp - mutation score: 90.13%
10 mutants for src/musig.cpp - mutation score: 80.0%

Unkilled mutants for src/script/descriptor.cpp and src/musig.cpp
mutation-core analyze -f="muts-pr-32724-descriptor-cpp" -c="cmake --build build -j 5 && ./build/bin/test_bitcoin --run_test=musig2_tests && ./build/bin/test_bitcoin --run_test=descriptor_tests"
* 223 MUTANTS *
...
MUTATION SCORE: 90.13%

Surviving mutants:
diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.186.cpp
index 8cb78327f7..0f8840c015 100644
--- a/src/script/descriptor.cpp
+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.186.cpp
@@ -1910,7 +1910,7 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkey(uint32_t& key_exp_index
                         vec.emplace_back(vec.at(0)->Clone());
                     }
                 } else if (vec.size() != length) {
-                    return false;
+                    return true;
                 }
             }
             return true;

--------------
diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.218.cpp
index 8cb78327f7..ca626ff150 100644
--- a/src/script/descriptor.cpp
+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.218.cpp
@@ -2067,7 +2067,7 @@ struct KeyParser {
     {
         assert(m_out);
         Key key = m_keys.size();
-        uint32_t exp_index = m_offset + key;
+        uint32_t exp_index = m_offset - key;
         auto pk = ParsePubkey(exp_index, {&*begin, &*end}, ParseContext(), *m_out, m_key_parsing_error);
         if (pk.empty()) return {};
         m_keys.emplace_back(std::move(pk));

--------------
diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.22.cpp
index 8cb78327f7..53c1a41269 100644
--- a/src/script/descriptor.cpp
+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.22.cpp
@@ -670,7 +670,7 @@ public:
             // Use a dummy signing provider as private keys do not exist for the aggregate pubkey
             FlatSigningProvider dummy;
             std::optional<CPubKey> pub = m_aggregate_provider->GetPubKey(pos, dummy, out, read_cache, write_cache);
-            if (!pub) return std::nullopt;
+            if (1==0) return std::nullopt;
             pubout = *pub;
             out.aggregate_pubkeys.emplace(m_aggregate_pubkey.value(), pubkeys);
         } else {

--------------
diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.23.cpp
index 8cb78327f7..b47540ebc5 100644
--- a/src/script/descriptor.cpp
+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.23.cpp
@@ -670,7 +670,7 @@ public:
             // Use a dummy signing provider as private keys do not exist for the aggregate pubkey
             FlatSigningProvider dummy;
             std::optional<CPubKey> pub = m_aggregate_provider->GetPubKey(pos, dummy, out, read_cache, write_cache);
-            if (!pub) return std::nullopt;
+
             pubout = *pub;
             out.aggregate_pubkeys.emplace(m_aggregate_pubkey.value(), pubkeys);
         } else {

--------------
diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.221.cpp
index 8cb78327f7..96bc9cd33c 100644
--- a/src/script/descriptor.cpp
+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.221.cpp
@@ -2067,7 +2067,7 @@ struct KeyParser {
     {
         assert(m_out);
         Key key = m_keys.size();
-        uint32_t exp_index = m_offset + key;
+        uint32_t exp_index = (m_offset + key) + 1;
         auto pk = ParsePubkey(exp_index, {&*begin, &*end}, ParseContext(), *m_out, m_key_parsing_error);
         if (pk.empty()) return {};
         m_keys.emplace_back(std::move(pk));

--------------
diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.209.cpp
index 8cb78327f7..471ea17a83 100644
--- a/src/script/descriptor.cpp
+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.209.cpp
@@ -1940,7 +1940,7 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkey(uint32_t& key_exp_index
                 // Final MuSigPubkeyProvider use participant pubkey providers at each multipath position, and the first (and only) path
                 emplace_final_provider(i, 0);
             }
-        } else if (paths.size() > 1) {
+        } else if (paths.size() >= 1) {
             // All key provider vectors should be length 1. Clone them until they have the same length as paths
             if (!clone_providers(paths.size())) {
                 error = "musig(): Multipath derivation path with multipath participants is disallowed"; // This error is unreachable due to earlier check

--------------
diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.27.cpp
index 8cb78327f7..1f3b5320c3 100644
--- a/src/script/descriptor.cpp
+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.27.cpp
@@ -677,7 +677,7 @@ public:
             if (!Assume(m_ranged_participants)) return std::nullopt;
             // Compute aggregate key from derived participants
             std::optional<CPubKey> aggregate_pubkey = MuSig2AggregatePubkeys(pubkeys);
-            if (!aggregate_pubkey) return std::nullopt;
+
             pubout = *aggregate_pubkey;

             KeyOriginInfo info;

--------------
diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.26.cpp
index 8cb78327f7..917be613cd 100644
--- a/src/script/descriptor.cpp
+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.26.cpp
@@ -677,7 +677,7 @@ public:
             if (!Assume(m_ranged_participants)) return std::nullopt;
             // Compute aggregate key from derived participants
             std::optional<CPubKey> aggregate_pubkey = MuSig2AggregatePubkeys(pubkeys);
-            if (!aggregate_pubkey) return std::nullopt;
+            if (1==0) return std::nullopt;
             pubout = *aggregate_pubkey;

             KeyOriginInfo info;

--------------
diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.24.cpp
index 8cb78327f7..8a4961d14f 100644
--- a/src/script/descriptor.cpp
+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.24.cpp
@@ -674,7 +674,7 @@ public:
             pubout = *pub;
             out.aggregate_pubkeys.emplace(m_aggregate_pubkey.value(), pubkeys);
         } else {
-            if (!Assume(m_ranged_participants)) return std::nullopt;
+
             // Compute aggregate key from derived participants
             std::optional<CPubKey> aggregate_pubkey = MuSig2AggregatePubkeys(pubkeys);
             if (!aggregate_pubkey) return std::nullopt;

--------------
diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.18.cpp
index 8cb78327f7..bc2436a7cf 100644
--- a/src/script/descriptor.cpp
+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.18.cpp
@@ -658,7 +658,7 @@ public:
         std::vector<CPubKey> pubkeys;
         for (const auto& prov : m_participants) {
             std::optional<CPubKey> pub = prov->GetPubKey(pos, arg, out, read_cache, write_cache);
-            if (!pub) return std::nullopt;
+
             pubkeys.emplace_back(*pub);
         }
         std::sort(pubkeys.begin(), pubkeys.end());

--------------
diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.31.cpp
index 8cb78327f7..b448f978e8 100644
--- a/src/script/descriptor.cpp
+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.31.cpp
@@ -688,7 +688,7 @@ public:
             out.aggregate_pubkeys.emplace(pubout, pubkeys);
         }

-        if (!Assume(pubout.IsValid())) return std::nullopt;
+
         return pubout;
     }
     bool IsRange() const override { return IsRangedDerivation() || m_ranged_participants; }

--------------
diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.86.cpp
index 8cb78327f7..73e12ab35e 100644
--- a/src/script/descriptor.cpp
+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.86.cpp
@@ -730,7 +730,7 @@ public:
         if (IsRangedDerivation()) {
             out += "/*";
         }
-        if (!any_privkeys) out.clear();
+        if (1==0) out.clear();
         return any_privkeys;
     }
     bool ToNormalizedString(const SigningProvider& arg, std::string& out, const DescriptorCache* cache = nullptr) const override

--------------
diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.99.cpp
index 8cb78327f7..e1cff262a0 100644
--- a/src/script/descriptor.cpp
+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.99.cpp
@@ -741,7 +741,7 @@ public:
             if (i) out += ",";
             std::string tmp;
             if (!pubkey->ToNormalizedString(arg, tmp)) {
-                return false;
+                return true;
             }
             out += tmp;
         }

--------------
diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.117.cpp
index 8cb78327f7..2ef8b39b2d 100644
--- a/src/script/descriptor.cpp
+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.117.cpp
@@ -789,7 +789,7 @@ public:
     bool IsBIP32() const override
     {
         // musig() can only be a BIP 32 key if all participants are bip32 too
-        return std::all_of(m_participants.begin(), m_participants.end(), [](const auto& pubkey) { return pubkey->IsBIP32(); });
+        return std::any_of(m_participants.begin(), m_participants.end(), [](const auto& pubkey) { return pubkey->IsBIP32(); });
     }
 };


--------------
diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.165.cpp
index 8cb78327f7..200cabd7a6 100644
--- a/src/script/descriptor.cpp
+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.165.cpp
@@ -1892,7 +1892,7 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkey(uint32_t& key_exp_index
                 error = "musig(): Cannot have hardened child derivation";
                 return {};
             }
-            bool dummy = false;
+            bool dummy = true;
             if (!ParseKeyPath(deriv_split, paths, dummy, error, /*allow_multipath=*/true, /*allow_hardened=*/false)) {
                 error = "musig(): " + error;
                 return {};

--------------
diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.17.cpp
index 8cb78327f7..20f835caf8 100644
--- a/src/script/descriptor.cpp
+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.17.cpp
@@ -658,7 +658,7 @@ public:
         std::vector<CPubKey> pubkeys;
         for (const auto& prov : m_participants) {
             std::optional<CPubKey> pub = prov->GetPubKey(pos, arg, out, read_cache, write_cache);
-            if (!pub) return std::nullopt;
+            if (1==0) return std::nullopt;
             pubkeys.emplace_back(*pub);
         }
         std::sort(pubkeys.begin(), pubkeys.end());

--------------
diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.3.cpp
index 8cb78327f7..93e335fc45 100644
--- a/src/script/descriptor.cpp
+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.3.cpp
@@ -608,7 +608,7 @@ public:
         m_participants(std::move(providers)),
         m_path(std::move(path)),
         m_derive(derive),
-        m_ranged_participants(std::any_of(m_participants.begin(), m_participants.end(), [](const auto& pubkey) { return pubkey->IsRange(); }))
+        m_ranged_participants(std::all_of(m_participants.begin(), m_participants.end(), [](const auto& pubkey) { return pubkey->IsRange(); }))
     {
         if (!Assume(!(m_ranged_participants && IsRangedDerivation()))) {
             throw std::runtime_error("musig(): Cannot have both ranged participants and ranged derivation");

--------------
diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.12.cpp
index 8cb78327f7..b4a54fb541 100644
--- a/src/script/descriptor.cpp
+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.12.cpp
@@ -648,7 +648,7 @@ public:
                 extpub.chaincode = MUSIG_CHAINCODE;
                 extpub.pubkey = m_aggregate_pubkey.value();

-                m_aggregate_provider = std::make_unique<BIP32PubkeyProvider>(m_expr_index, extpub, m_path, m_derive, /*apostrophe=*/false);
+                m_aggregate_provider = std::make_unique<BIP32PubkeyProvider>(m_expr_index, extpub, m_path, m_derive, /*apostrophe=*/true);
             } else {
                 m_aggregate_provider = std::make_unique<ConstPubkeyProvider>(m_expr_index, m_aggregate_pubkey.value(), /*xonly=*/false);
             }

--------------
diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.13.cpp
index 8cb78327f7..72e90a79de 100644
--- a/src/script/descriptor.cpp
+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.13.cpp
@@ -650,7 +650,7 @@ public:

                 m_aggregate_provider = std::make_unique<BIP32PubkeyProvider>(m_expr_index, extpub, m_path, m_derive, /*apostrophe=*/false);
             } else {
-                m_aggregate_provider = std::make_unique<ConstPubkeyProvider>(m_expr_index, m_aggregate_pubkey.value(), /*xonly=*/false);
+                m_aggregate_provider = std::make_unique<ConstPubkeyProvider>(m_expr_index, m_aggregate_pubkey.value(), /*xonly=*/true);
             }
         }


--------------
diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.177.cpp
index 8cb78327f7..363403a0c2 100644
--- a/src/script/descriptor.cpp
+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.177.cpp
@@ -1906,7 +1906,7 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkey(uint32_t& key_exp_index
         const auto& clone_providers = [&providers](size_t length) -> bool {
             for (auto& vec : providers) {
                 if (vec.size() == 1) {
-                    for (size_t i = 1; i < length; ++i) {
+                    for (size_t i = 1; i < length; --i) {
                         vec.emplace_back(vec.at(0)->Clone());
                     }
                 } else if (vec.size() != length) {

--------------
diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.175.cpp
index 8cb78327f7..3bee51264c 100644
--- a/src/script/descriptor.cpp
+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.175.cpp
@@ -1906,7 +1906,7 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkey(uint32_t& key_exp_index
         const auto& clone_providers = [&providers](size_t length) -> bool {
             for (auto& vec : providers) {
                 if (vec.size() == 1) {
-                    for (size_t i = 1; i < length; ++i) {
+                    for (size_t i = 1; i <= length; ++i) {
                         vec.emplace_back(vec.at(0)->Clone());
                     }
                 } else if (vec.size() != length) {

--------------
diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.11.cpp
index 8cb78327f7..9594e83929 100644
--- a/src/script/descriptor.cpp
+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.11.cpp
@@ -636,7 +636,7 @@ public:

             // Aggregate the pubkey
             m_aggregate_pubkey = MuSig2AggregatePubkeys(pubkeys);
-            if (!Assume(m_aggregate_pubkey.has_value())) return std::nullopt;
+

             // Make our pubkey provider
             if (IsRangedDerivation() || !m_path.empty()) {

--------------

```diff
➜  bitcoin-core-dev git:(pr/32724) ✗ mutation-core analyze -f="muts-pr-32724-musig-cpp" -c="cmake --build build -j 5 && ./build/bin/test_bitcoin --run_test=musig2_tests && ./build/bin/test_bitcoin --run_test=descriptor_tests"
...
MUTATION SCORE: 80.0%

Surviving mutants:
diff --git a/src/musig.cpp b/muts-pr-32724-musig-cpp/musig.mutant.2.cpp
index b332954312..8d60925399 100644
--- a/src/musig.cpp
+++ b/muts-pr-32724-musig-cpp/musig.mutant.2.cpp
@@ -13,7 +13,7 @@ bool GetMuSig2KeyAggCache(const std::vector<CPubKey>& pubkeys, secp256k1_musig_k
     std::vector<const secp256k1_pubkey*> pubkey_ptrs;
     for (const CPubKey& pubkey : pubkeys) {
         if (!secp256k1_ec_pubkey_parse(secp256k1_context_static, &secp_pubkeys.emplace_back(), pubkey.data(), pubkey.size())) {
-            return false;
+            return true;
         }
     }
     pubkey_ptrs.reserve(secp_pubkeys.size());

--------------
diff --git a/src/musig.cpp b/muts-pr-32724-musig-cpp/musig.mutant.5.cpp
index b332954312..219f54aa14 100644
--- a/src/musig.cpp
+++ b/muts-pr-32724-musig-cpp/musig.mutant.5.cpp
@@ -23,7 +23,7 @@ bool GetMuSig2KeyAggCache(const std::vector<CPubKey>& pubkeys, secp256k1_musig_k

     // Aggregate the pubkey
     if (!secp256k1_musig_pubkey_agg(secp256k1_context_static, nullptr, &keyagg_cache, pubkey_ptrs.data(), pubkey_ptrs.size())) {
-        return false;
+        return true;
     }
     return true;
 }
--------------

note: It only generated mutants based on git diff - code touched by this PR.

@w0xlt
Copy link
Contributor Author

w0xlt commented Jul 31, 2025

Rebased and fixed musig.mutant.2.cpp.
It is impossible to reach the condition in musig.mutant.5.cpp with valid public keys. The condition detected by mutant 2 prevents sending invalid public keys to secp256k1_musig_pubkey_agg.
Even if I try to send P and -P, the MuSig2 protocol prevents key cancellation attacks.

@w0xlt
Copy link
Contributor Author

w0xlt commented Jul 31, 2025

The CI error is related to the wallet migration test.
#33096

@Sjors
Copy link
Member

Sjors commented Aug 1, 2025

This can be moved out of draft? (since the CI failure is unrelated)

Concept ACK

Just two test vectors? If there's more then it might make sense to put them in a JSON file.

@fanquake fanquake marked this pull request as ready for review August 1, 2025 16:02
@DrahtBot DrahtBot removed the CI failed label Aug 5, 2025
@w0xlt
Copy link
Contributor Author

w0xlt commented Aug 6, 2025

Just two test vectors?

Yes, they cover BIP 328 test vectors and the mutation detected by @brunoerg .
BIP 327 doesn't have any test vectors (even the musig2 secp256k1 library uses random keys in the test).
BIP 328 test vectors are particularly interesting because they test aggregate keys and xpubs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants