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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32724.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task lint: https://github.com/bitcoin/bitcoin/runs/43856471515</sub>
<sub>LLM reason (✨ experimental): The CI failure is caused by a trailing whitespace error detected in src/test/musig2.cpp.</sub>
<details><summary>Hints</summary>
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.
</details>
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%
<details> <summary>Unkilled mutants for src/script/descriptor.cpp and src/musig.cpp</summary>
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;
}
--------------
</details>
note: It only generated mutants based on git diff - code touched by this PR.
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.
The CI error is related to the wallet migration test. https://github.com/bitcoin/bitcoin/issues/33096
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.
ACK fa65a57a826192ffae48f62f10c216068e8236f4
I (lightly) checked that the vectors match BIP 328 and that the test fails if I sabotage the input data.
84 | + CExtPubKey extpub; 85 | + extpub.nDepth = 0; 86 | + std::fill(std::begin(extpub.vchFingerprint), std::end(extpub.vchFingerprint), 0); 87 | + extpub.nChild = 0; 88 | + extpub.pubkey = m_aggregate_pubkey.value(); 89 | + extpub.chaincode = MUSIG_CHAINCODE;
There's a utility function in the master branch that can be used instead of hardcoding.
--- a/src/test/musig2_tests.cpp
+++ b/src/test/musig2_tests.cpp
@@ -81,12 +81,7 @@ BOOST_AUTO_TEST_CASE(bip328)
BOOST_CHECK_MESSAGE(combined_keys == test.expected_aggregate_pubkey, "Test vector " << i << ": Aggregate pubkey mismatch");
// Create extended public key
- CExtPubKey extpub;
- extpub.nDepth = 0;
- std::fill(std::begin(extpub.vchFingerprint), std::end(extpub.vchFingerprint), 0);
- extpub.nChild = 0;
- extpub.pubkey = m_aggregate_pubkey.value();
- extpub.chaincode = MUSIG_CHAINCODE;
+ CExtPubKey extpub = CreateMuSig2SyntheticXpub(m_aggregate_pubkey.value());
// Check xpub
std::string xpub = EncodeExtPubKey(extpub);
Please address this comment.
17 | +namespace { 18 | + 19 | +struct BIP328TestVector { 20 | + std::vector<std::string> pubkeys; 21 | + std::string expected_aggregate_pubkey; 22 | + std::string expected_xpub;
Nit: s/expected_xpub/expected_aggregate_xpub
- std::string expected_xpub;
+ std::string expected_aggregate_xpub;
but having more detailed tests for the MuSig2 class itself improves test reporting/coverage.
There isn't a MuSig2 class per-se.
Concept ACK fa65a57a826192ffae48f62f10c216068e8236f4
0 | @@ -0,0 +1,119 @@ 1 | +// Copyright (c) 2024-present The Bitcoin Core developers
nit: this was not written in 2024.
59 | @@ -60,6 +60,7 @@ add_executable(test_bitcoin 60 | miniscript_tests.cpp 61 | minisketch_tests.cpp 62 | multisig_tests.cpp 63 | + musig2_tests.cpp
This file specifically tests bip 328, not all of musig2. I think this is misnamed, especially as I think it is unlikely there will be any more specific musig2 tests since it is largely covered by the descriptor tests.
Comments addressed. Thanks for the review.
lgtm ACK a3c71c7
git range-diff fa65a57...a3c71c7
ACK a3c71c720158fd024d072ad32e78a3b353de0723