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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32724.
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.
No conflicts as of last run.
🚧 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.
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.
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%
0mutation-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"
1* 223 MUTANTS *
2...
3MUTATION SCORE: 90.13%
4
5Surviving mutants:
6diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.186.cpp
7index 8cb78327f7..0f8840c015 100644
8--- a/src/script/descriptor.cpp
9+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.186.cpp
10@@ -1910,7 +1910,7 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkey(uint32_t& key_exp_index
11 vec.emplace_back(vec.at(0)->Clone());
12 }
13 } else if (vec.size() != length) {
14- return false;
15+ return true;
16 }
17 }
18 return true;
19
20--------------
21diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.218.cpp
22index 8cb78327f7..ca626ff150 100644
23--- a/src/script/descriptor.cpp
24+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.218.cpp
25@@ -2067,7 +2067,7 @@ struct KeyParser {
26 {
27 assert(m_out);
28 Key key = m_keys.size();
29- uint32_t exp_index = m_offset + key;
30+ uint32_t exp_index = m_offset - key;
31 auto pk = ParsePubkey(exp_index, {&*begin, &*end}, ParseContext(), *m_out, m_key_parsing_error);
32 if (pk.empty()) return {};
33 m_keys.emplace_back(std::move(pk));
34
35--------------
36diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.22.cpp
37index 8cb78327f7..53c1a41269 100644
38--- a/src/script/descriptor.cpp
39+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.22.cpp
40@@ -670,7 +670,7 @@ public:
41 // Use a dummy signing provider as private keys do not exist for the aggregate pubkey
42 FlatSigningProvider dummy;
43 std::optional<CPubKey> pub = m_aggregate_provider->GetPubKey(pos, dummy, out, read_cache, write_cache);
44- if (!pub) return std::nullopt;
45+ if (1==0) return std::nullopt;
46 pubout = *pub;
47 out.aggregate_pubkeys.emplace(m_aggregate_pubkey.value(), pubkeys);
48 } else {
49
50--------------
51diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.23.cpp
52index 8cb78327f7..b47540ebc5 100644
53--- a/src/script/descriptor.cpp
54+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.23.cpp
55@@ -670,7 +670,7 @@ public:
56 // Use a dummy signing provider as private keys do not exist for the aggregate pubkey
57 FlatSigningProvider dummy;
58 std::optional<CPubKey> pub = m_aggregate_provider->GetPubKey(pos, dummy, out, read_cache, write_cache);
59- if (!pub) return std::nullopt;
60+
61 pubout = *pub;
62 out.aggregate_pubkeys.emplace(m_aggregate_pubkey.value(), pubkeys);
63 } else {
64
65--------------
66diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.221.cpp
67index 8cb78327f7..96bc9cd33c 100644
68--- a/src/script/descriptor.cpp
69+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.221.cpp
70@@ -2067,7 +2067,7 @@ struct KeyParser {
71 {
72 assert(m_out);
73 Key key = m_keys.size();
74- uint32_t exp_index = m_offset + key;
75+ uint32_t exp_index = (m_offset + key) + 1;
76 auto pk = ParsePubkey(exp_index, {&*begin, &*end}, ParseContext(), *m_out, m_key_parsing_error);
77 if (pk.empty()) return {};
78 m_keys.emplace_back(std::move(pk));
79
80--------------
81diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.209.cpp
82index 8cb78327f7..471ea17a83 100644
83--- a/src/script/descriptor.cpp
84+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.209.cpp
85@@ -1940,7 +1940,7 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkey(uint32_t& key_exp_index
86 // Final MuSigPubkeyProvider use participant pubkey providers at each multipath position, and the first (and only) path
87 emplace_final_provider(i, 0);
88 }
89- } else if (paths.size() > 1) {
90+ } else if (paths.size() >= 1) {
91 // All key provider vectors should be length 1. Clone them until they have the same length as paths
92 if (!clone_providers(paths.size())) {
93 error = "musig(): Multipath derivation path with multipath participants is disallowed"; // This error is unreachable due to earlier check
94
95--------------
96diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.27.cpp
97index 8cb78327f7..1f3b5320c3 100644
98--- a/src/script/descriptor.cpp
99+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.27.cpp
100@@ -677,7 +677,7 @@ public:
101 if (!Assume(m_ranged_participants)) return std::nullopt;
102 // Compute aggregate key from derived participants
103 std::optional<CPubKey> aggregate_pubkey = MuSig2AggregatePubkeys(pubkeys);
104- if (!aggregate_pubkey) return std::nullopt;
105+
106 pubout = *aggregate_pubkey;
107
108 KeyOriginInfo info;
109
110--------------
111diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.26.cpp
112index 8cb78327f7..917be613cd 100644
113--- a/src/script/descriptor.cpp
114+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.26.cpp
115@@ -677,7 +677,7 @@ public:
116 if (!Assume(m_ranged_participants)) return std::nullopt;
117 // Compute aggregate key from derived participants
118 std::optional<CPubKey> aggregate_pubkey = MuSig2AggregatePubkeys(pubkeys);
119- if (!aggregate_pubkey) return std::nullopt;
120+ if (1==0) return std::nullopt;
121 pubout = *aggregate_pubkey;
122
123 KeyOriginInfo info;
124
125--------------
126diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.24.cpp
127index 8cb78327f7..8a4961d14f 100644
128--- a/src/script/descriptor.cpp
129+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.24.cpp
130@@ -674,7 +674,7 @@ public:
131 pubout = *pub;
132 out.aggregate_pubkeys.emplace(m_aggregate_pubkey.value(), pubkeys);
133 } else {
134- if (!Assume(m_ranged_participants)) return std::nullopt;
135+
136 // Compute aggregate key from derived participants
137 std::optional<CPubKey> aggregate_pubkey = MuSig2AggregatePubkeys(pubkeys);
138 if (!aggregate_pubkey) return std::nullopt;
139
140--------------
141diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.18.cpp
142index 8cb78327f7..bc2436a7cf 100644
143--- a/src/script/descriptor.cpp
144+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.18.cpp
145@@ -658,7 +658,7 @@ public:
146 std::vector<CPubKey> pubkeys;
147 for (const auto& prov : m_participants) {
148 std::optional<CPubKey> pub = prov->GetPubKey(pos, arg, out, read_cache, write_cache);
149- if (!pub) return std::nullopt;
150+
151 pubkeys.emplace_back(*pub);
152 }
153 std::sort(pubkeys.begin(), pubkeys.end());
154
155--------------
156diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.31.cpp
157index 8cb78327f7..b448f978e8 100644
158--- a/src/script/descriptor.cpp
159+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.31.cpp
160@@ -688,7 +688,7 @@ public:
161 out.aggregate_pubkeys.emplace(pubout, pubkeys);
162 }
163
164- if (!Assume(pubout.IsValid())) return std::nullopt;
165+
166 return pubout;
167 }
168 bool IsRange() const override { return IsRangedDerivation() || m_ranged_participants; }
169
170--------------
171diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.86.cpp
172index 8cb78327f7..73e12ab35e 100644
173--- a/src/script/descriptor.cpp
174+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.86.cpp
175@@ -730,7 +730,7 @@ public:
176 if (IsRangedDerivation()) {
177 out += "/*";
178 }
179- if (!any_privkeys) out.clear();
180+ if (1==0) out.clear();
181 return any_privkeys;
182 }
183 bool ToNormalizedString(const SigningProvider& arg, std::string& out, const DescriptorCache* cache = nullptr) const override
184
185--------------
186diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.99.cpp
187index 8cb78327f7..e1cff262a0 100644
188--- a/src/script/descriptor.cpp
189+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.99.cpp
190@@ -741,7 +741,7 @@ public:
191 if (i) out += ",";
192 std::string tmp;
193 if (!pubkey->ToNormalizedString(arg, tmp)) {
194- return false;
195+ return true;
196 }
197 out += tmp;
198 }
199
200--------------
201diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.117.cpp
202index 8cb78327f7..2ef8b39b2d 100644
203--- a/src/script/descriptor.cpp
204+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.117.cpp
205@@ -789,7 +789,7 @@ public:
206 bool IsBIP32() const override
207 {
208 // musig() can only be a BIP 32 key if all participants are bip32 too
209- return std::all_of(m_participants.begin(), m_participants.end(), [](const auto& pubkey) { return pubkey->IsBIP32(); });
210+ return std::any_of(m_participants.begin(), m_participants.end(), [](const auto& pubkey) { return pubkey->IsBIP32(); });
211 }
212 };
213
214
215--------------
216diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.165.cpp
217index 8cb78327f7..200cabd7a6 100644
218--- a/src/script/descriptor.cpp
219+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.165.cpp
220@@ -1892,7 +1892,7 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkey(uint32_t& key_exp_index
221 error = "musig(): Cannot have hardened child derivation";
222 return {};
223 }
224- bool dummy = false;
225+ bool dummy = true;
226 if (!ParseKeyPath(deriv_split, paths, dummy, error, /*allow_multipath=*/true, /*allow_hardened=*/false)) {
227 error = "musig(): " + error;
228 return {};
229
230--------------
231diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.17.cpp
232index 8cb78327f7..20f835caf8 100644
233--- a/src/script/descriptor.cpp
234+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.17.cpp
235@@ -658,7 +658,7 @@ public:
236 std::vector<CPubKey> pubkeys;
237 for (const auto& prov : m_participants) {
238 std::optional<CPubKey> pub = prov->GetPubKey(pos, arg, out, read_cache, write_cache);
239- if (!pub) return std::nullopt;
240+ if (1==0) return std::nullopt;
241 pubkeys.emplace_back(*pub);
242 }
243 std::sort(pubkeys.begin(), pubkeys.end());
244
245--------------
246diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.3.cpp
247index 8cb78327f7..93e335fc45 100644
248--- a/src/script/descriptor.cpp
249+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.3.cpp
250@@ -608,7 +608,7 @@ public:
251 m_participants(std::move(providers)),
252 m_path(std::move(path)),
253 m_derive(derive),
254- m_ranged_participants(std::any_of(m_participants.begin(), m_participants.end(), [](const auto& pubkey) { return pubkey->IsRange(); }))
255+ m_ranged_participants(std::all_of(m_participants.begin(), m_participants.end(), [](const auto& pubkey) { return pubkey->IsRange(); }))
256 {
257 if (!Assume(!(m_ranged_participants && IsRangedDerivation()))) {
258 throw std::runtime_error("musig(): Cannot have both ranged participants and ranged derivation");
259
260--------------
261diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.12.cpp
262index 8cb78327f7..b4a54fb541 100644
263--- a/src/script/descriptor.cpp
264+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.12.cpp
265@@ -648,7 +648,7 @@ public:
266 extpub.chaincode = MUSIG_CHAINCODE;
267 extpub.pubkey = m_aggregate_pubkey.value();
268
269- m_aggregate_provider = std::make_unique<BIP32PubkeyProvider>(m_expr_index, extpub, m_path, m_derive, /*apostrophe=*/false);
270+ m_aggregate_provider = std::make_unique<BIP32PubkeyProvider>(m_expr_index, extpub, m_path, m_derive, /*apostrophe=*/true);
271 } else {
272 m_aggregate_provider = std::make_unique<ConstPubkeyProvider>(m_expr_index, m_aggregate_pubkey.value(), /*xonly=*/false);
273 }
274
275--------------
276diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.13.cpp
277index 8cb78327f7..72e90a79de 100644
278--- a/src/script/descriptor.cpp
279+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.13.cpp
280@@ -650,7 +650,7 @@ public:
281
282 m_aggregate_provider = std::make_unique<BIP32PubkeyProvider>(m_expr_index, extpub, m_path, m_derive, /*apostrophe=*/false);
283 } else {
284- m_aggregate_provider = std::make_unique<ConstPubkeyProvider>(m_expr_index, m_aggregate_pubkey.value(), /*xonly=*/false);
285+ m_aggregate_provider = std::make_unique<ConstPubkeyProvider>(m_expr_index, m_aggregate_pubkey.value(), /*xonly=*/true);
286 }
287 }
288
289
290--------------
291diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.177.cpp
292index 8cb78327f7..363403a0c2 100644
293--- a/src/script/descriptor.cpp
294+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.177.cpp
295@@ -1906,7 +1906,7 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkey(uint32_t& key_exp_index
296 const auto& clone_providers = [&providers](size_t length) -> bool {
297 for (auto& vec : providers) {
298 if (vec.size() == 1) {
299- for (size_t i = 1; i < length; ++i) {
300+ for (size_t i = 1; i < length; --i) {
301 vec.emplace_back(vec.at(0)->Clone());
302 }
303 } else if (vec.size() != length) {
304
305--------------
306diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.175.cpp
307index 8cb78327f7..3bee51264c 100644
308--- a/src/script/descriptor.cpp
309+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.175.cpp
310@@ -1906,7 +1906,7 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkey(uint32_t& key_exp_index
311 const auto& clone_providers = [&providers](size_t length) -> bool {
312 for (auto& vec : providers) {
313 if (vec.size() == 1) {
314- for (size_t i = 1; i < length; ++i) {
315+ for (size_t i = 1; i <= length; ++i) {
316 vec.emplace_back(vec.at(0)->Clone());
317 }
318 } else if (vec.size() != length) {
319
320--------------
321diff --git a/src/script/descriptor.cpp b/muts-pr-32724-descriptor-cpp/descriptor.mutant.11.cpp
322index 8cb78327f7..9594e83929 100644
323--- a/src/script/descriptor.cpp
324+++ b/muts-pr-32724-descriptor-cpp/descriptor.mutant.11.cpp
325@@ -636,7 +636,7 @@ public:
326
327 // Aggregate the pubkey
328 m_aggregate_pubkey = MuSig2AggregatePubkeys(pubkeys);
329- if (!Assume(m_aggregate_pubkey.has_value())) return std::nullopt;
330+
331
332 // Make our pubkey provider
333 if (IsRangedDerivation() || !m_path.empty()) {
334
335--------------
336
337```diff
338➜ 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"
339...
340MUTATION SCORE: 80.0%
341
342Surviving mutants:
343diff --git a/src/musig.cpp b/muts-pr-32724-musig-cpp/musig.mutant.2.cpp
344index b332954312..8d60925399 100644
345--- a/src/musig.cpp
346+++ b/muts-pr-32724-musig-cpp/musig.mutant.2.cpp
347@@ -13,7 +13,7 @@ bool GetMuSig2KeyAggCache(const std::vector<CPubKey>& pubkeys, secp256k1_musig_k
348 std::vector<const secp256k1_pubkey*> pubkey_ptrs;
349 for (const CPubKey& pubkey : pubkeys) {
350 if (!secp256k1_ec_pubkey_parse(secp256k1_context_static, &secp_pubkeys.emplace_back(), pubkey.data(), pubkey.size())) {
351- return false;
352+ return true;
353 }
354 }
355 pubkey_ptrs.reserve(secp_pubkeys.size());
356
357--------------
358diff --git a/src/musig.cpp b/muts-pr-32724-musig-cpp/musig.mutant.5.cpp
359index b332954312..219f54aa14 100644
360--- a/src/musig.cpp
361+++ b/muts-pr-32724-musig-cpp/musig.mutant.5.cpp
362@@ -23,7 +23,7 @@ bool GetMuSig2KeyAggCache(const std::vector<CPubKey>& pubkeys, secp256k1_musig_k
363
364 // Aggregate the pubkey
365 if (!secp256k1_musig_pubkey_agg(secp256k1_context_static, nullptr, &keyagg_cache, pubkey_ptrs.data(), pubkey_ptrs.size())) {
366- return false;
367+ return true;
368 }
369 return true;
370 }
371--------------
note: It only generated mutants based on git diff - code touched by this PR.
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.
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.
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.