This is a follow-up to #29675 and primarily adds test coverage for some of the most prominent failure cases in the last commit.
The following commits address a few left-over nit comments that didn't make it in before merge.
wallet: Expand MuSig test coverage and follow-ups #33636
pull fjahr wants to merge 4 commits into bitcoin:master from fjahr:202510-musig-test changing 5 files +147 −54-
fjahr commented at 12:02 AM on October 16, 2025: contributor
- DrahtBot added the label Wallet on Oct 16, 2025
-
DrahtBot commented at 12:02 AM on October 16, 2025: contributor
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
Code Coverage & Benchmarks
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33636.
<!--021abf342d371248e50ceaed478a90ca-->
Reviews
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
- fanquake added this to the milestone 31.0 on Oct 16, 2025
-
in test/functional/wallet_musig.py:32 in c7c9dc2d86 outdated
27 | @@ -28,6 +28,178 @@ def set_test_params(self): 28 | def skip_test_if_missing_module(self): 29 | self.skip_if_no_wallet() 30 | 31 | + def setup_musig_scenario(self, num_wallets=3): 32 | + # Miniscript pattern based on number of wallets
rkrux commented at 12:32 PM on October 17, 2025:In c7c9dc2d8634bc79e76783be113f8d42cee63c6e "test: Add musig failure scenarios"
Miniscriptpattern orMultipathpattern?
fjahr commented at 5:55 PM on November 24, 2025:I guess that might be more informative but the comment is just removed now
in test/functional/wallet_musig.py:202 in c7c9dc2d86 outdated
197 | + 198 | + dec = self.nodes[0].decodepsbt(comb_nonce_psbt) 199 | + assert "musig2_pubnonces" in dec["inputs"][0] 200 | + assert "musig2_partial_sigs" not in dec["inputs"][0] 201 | + 202 | +
rkrux commented at 1:12 PM on October 17, 2025:In c7c9dc2d8634bc79e76783be113f8d42cee63c6e "test: Add musig failure scenarios"
Nit: extra blank line
fjahr commented at 5:56 PM on November 24, 2025:Fixed
in test/functional/wallet_musig.py:203 in c7c9dc2d86 outdated
198 | + dec = self.nodes[0].decodepsbt(comb_nonce_psbt) 199 | + assert "musig2_pubnonces" in dec["inputs"][0] 200 | + assert "musig2_partial_sigs" not in dec["inputs"][0] 201 | + 202 | + 203 | def do_test(self, comment, pattern, sighash_type=None, scriptpath=False, nosign_wallets=None, only_one_musig_wallet=False):
rkrux commented at 1:16 PM on October 17, 2025:In https://github.com/bitcoin/bitcoin/commit/c7c9dc2d8634bc79e76783be113f8d42cee63c6e "test: Add musig failure scenarios"
Nit: can rename this function to
test_success_casenow.
fjahr commented at 5:56 PM on November 24, 2025:Done
in test/functional/wallet_musig.py:1 in c7c9dc2d86 outdated
rkrux commented at 2:30 PM on October 17, 2025:In https://github.com/bitcoin/bitcoin/commit/c7c9dc2d8634bc79e76783be113f8d42cee63c6e "test: Add musig failure scenarios"
I noticed some duplication in both the success and failure cases and I tried to refactor this file based on the following points. Please find full diff below and consider extracting out commonalities.
- Separated the loop in the
do_testfunc so that wallet creation and expectation calculations are not together. - Because of the above separation, extracted
create_wallets_and_keys_from_patternfunc to be used in both success and failure cases. - Extracted
construct_and_import_musig_descriptor_in_walletsfunc to be used in both success and failure cases. - Split the
test_musig_failure_modesfunc into 4 funcs that have a similar func signature like thedo_testfunc that takes in a comment and pattern to work with. Sort of a nit but I like all the test cases to have a similar format - easier on the eyes of the reader.
<details open> <summary>Full diff that works on my system</summary>
diff --git a/test/functional/wallet_musig.py b/test/functional/wallet_musig.py index 55129b1676..64e1e40259 100755 --- a/test/functional/wallet_musig.py +++ b/test/functional/wallet_musig.py @@ -28,43 +28,52 @@ class WalletMuSigTest(BitcoinTestFramework): def skip_test_if_missing_module(self): self.skip_if_no_wallet() - def setup_musig_scenario(self, num_wallets=3): - # Miniscript pattern based on number of wallets - placeholders = ",".join(f"${i}/<{i};{i+1}>/*" for i in range(num_wallets)) - pattern = f"rawtr(musig({placeholders}))" - + # Create wallets and extract keys + def create_wallets_and_keys_from_pattern(self, pat): wallets = [] keys = [] - # Create wallets and extract keys - for i in range(num_wallets): - wallet_name = f"musig_{self.wallet_num}" - self.wallet_num += 1 - self.nodes[0].createwallet(wallet_name) - wallet = self.nodes[0].get_wallet_rpc(wallet_name) - wallets.append(wallet) - - for priv_desc in wallet.listdescriptors(True)["descriptors"]: - desc = priv_desc["desc"] - if not desc.startswith("tr("): - continue - privkey = PRIVKEY_RE.search(desc).group(1) - break - for pub_desc in wallet.listdescriptors()["descriptors"]: - desc = pub_desc["desc"] - if not desc.startswith("tr("): + for musig in MUSIG_RE.findall(pat): + for placeholder in PLACEHOLDER_RE.findall(musig): + wallet_index = int(placeholder[1:]) + if wallet_index < len(wallets): continue - pubkey = PUBKEY_RE.search(desc).group(1) - privkey += ORIGIN_PATH_RE.search(pubkey).group(1) - break - keys.append((privkey, pubkey)) - # Construct and import descriptors + wallet_name = f"musig_{self.wallet_num}" + self.wallet_num += 1 + self.nodes[0].createwallet(wallet_name) + wallet = self.nodes[0].get_wallet_rpc(wallet_name) + wallets.append(wallet) + + for priv_desc in wallet.listdescriptors(True)["descriptors"]: + desc = priv_desc["desc"] + if not desc.startswith("tr("): + continue + privkey = PRIVKEY_RE.search(desc).group(1) + break + for pub_desc in wallet.listdescriptors()["descriptors"]: + desc = pub_desc["desc"] + if not desc.startswith("tr("): + continue + pubkey = PUBKEY_RE.search(desc).group(1) + # Since the pubkey is derived from the private key that we have, we need + # to extract and insert the origin path from the pubkey as well. + privkey += ORIGIN_PATH_RE.search(pubkey).group(1) + break + keys.append((privkey, pubkey)) + + return wallets, keys + + # Construct and import each wallet's musig descriptor that + # contains the private key from that wallet and pubkeys of the others + def construct_and_import_musig_descriptor_in_wallets(self, pat, wallets, keys, only_one_musig_wallet=False): for i, wallet in enumerate(wallets): - desc = pattern + if only_one_musig_wallet and i > 0: + continue + desc = pat for j, (priv, pub) in enumerate(keys): if j == i: - desc = desc.replace(f"${j}", priv) + desc = desc.replace(f"${i}", priv) else: desc = desc.replace(f"${j}", pub) @@ -78,6 +87,10 @@ class WalletMuSigTest(BitcoinTestFramework): for r in res: assert_equal(r["success"], True) + def setup_musig_scenario(self, pat): + wallets, keys = self.create_wallets_and_keys_from_pattern(pat) + self.construct_and_import_musig_descriptor_in_wallets(pat, wallets, keys, only_one_musig_wallet=False) + # Fund address addr = wallets[0].getnewaddress(address_type="bech32m") for wallet in wallets[1:]: @@ -97,11 +110,9 @@ class WalletMuSigTest(BitcoinTestFramework): return wallets, psbt - def test_musig_failure_modes(self): - """Test various MuSig2 failure scenarios""" - - self.log.info("Testing missing participant nonce") - wallets, psbt = self.setup_musig_scenario(num_wallets=3) + def test_failure_case_1(self, comment, pat): + self.log.info(f"Testing {comment}") + wallets, psbt = self.setup_musig_scenario(pat) # Only 2 out of 3 participants provide nonces nonce_psbts = [] @@ -121,8 +132,9 @@ class WalletMuSigTest(BitcoinTestFramework): # There are still only two nonces assert_equal(len(dec["inputs"][0].get("musig2_pubnonces", [])), 2) - self.log.info("Testing insufficient partial signatures") - wallets, psbt = self.setup_musig_scenario(num_wallets=3) + def test_failure_case_2(self, comment, pat): + self.log.info(f"Testing {comment}") + wallets, psbt = self.setup_musig_scenario(pat) nonce_psbts = [w.walletprocesspsbt(psbt=psbt)["psbt"] for w in wallets] comb_nonce_psbt = self.nodes[0].combinepsbt(nonce_psbts) @@ -142,30 +154,9 @@ class WalletMuSigTest(BitcoinTestFramework): dec = self.nodes[0].decodepsbt(comb_psig_psbt) assert_equal(len(dec["inputs"][0]["musig2_partial_sigs"]), 2) - self.log.info("Testing mismatched key order") - wallets = [] - keys = [] - for i in range(2): - wallet_name = f"musig_{self.wallet_num}" - self.wallet_num += 1 - self.nodes[0].createwallet(wallet_name) - wallet = self.nodes[0].get_wallet_rpc(wallet_name) - wallets.append(wallet) - - for priv_desc in wallet.listdescriptors(True)["descriptors"]: - desc = priv_desc["desc"] - if not desc.startswith("tr("): - continue - privkey = PRIVKEY_RE.search(desc).group(1) - break - for pub_desc in wallet.listdescriptors()["descriptors"]: - desc = pub_desc["desc"] - if not desc.startswith("tr("): - continue - pubkey = PUBKEY_RE.search(desc).group(1) - privkey += ORIGIN_PATH_RE.search(pubkey).group(1) - break - keys.append((privkey, pubkey)) + def test_failure_case_3(self, comment, pat): + self.log.info(f"Testing {comment}") + wallets, keys = self.create_wallets_and_keys_from_pattern(pat) desc0 = f"rawtr(musig({keys[0][0]},{keys[1][1]}))" wallets[0].importdescriptors([{ @@ -187,8 +178,9 @@ class WalletMuSigTest(BitcoinTestFramework): addr1 = wallets[1].getnewaddress(address_type="bech32m") assert addr0 != addr1 - self.log.info("Testing finalize without partial sigs") - wallets, psbt = self.setup_musig_scenario(num_wallets=2) + def test_failure_case_4(self, comment, pat): + self.log.info(f"Testing {comment}") + wallets, psbt = self.setup_musig_scenario(pat) nonce_psbts = [w.walletprocesspsbt(psbt=psbt)["psbt"] for w in wallets] comb_nonce_psbt = self.nodes[0].combinepsbt(nonce_psbts) @@ -199,17 +191,14 @@ class WalletMuSigTest(BitcoinTestFramework): assert "musig2_pubnonces" in dec["inputs"][0] assert "musig2_partial_sigs" not in dec["inputs"][0] - def do_test(self, comment, pattern, sighash_type=None, scriptpath=False, nosign_wallets=None, only_one_musig_wallet=False): self.log.info(f"Testing {comment}") has_internal = MULTIPATH_TWO_RE.search(pattern) is not None - wallets = [] - keys = [] - pat = pattern.replace("$H", H_POINT) + wallets, keys = self.create_wallets_and_keys_from_pattern(pat) + self.construct_and_import_musig_descriptor_in_wallets(pat, wallets, keys, only_one_musig_wallet) - # Figure out how many wallets are needed and create them expected_pubnonces = 0 expected_partial_sigs = 0 for musig in MUSIG_RE.findall(pat): @@ -224,54 +213,9 @@ class WalletMuSigTest(BitcoinTestFramework): musig_partial_sigs += 1 if wallet_index < len(wallets): continue - wallet_name = f"musig_{self.wallet_num}" - self.wallet_num += 1 - self.nodes[0].createwallet(wallet_name) - wallet = self.nodes[0].get_wallet_rpc(wallet_name) - wallets.append(wallet) - - for priv_desc in wallet.listdescriptors(True)["descriptors"]: - desc = priv_desc["desc"] - if not desc.startswith("tr("): - continue - privkey = PRIVKEY_RE.search(desc).group(1) - break - for pub_desc in wallet.listdescriptors()["descriptors"]: - desc = pub_desc["desc"] - if not desc.startswith("tr("): - continue - pubkey = PUBKEY_RE.search(desc).group(1) - # Since the pubkey is derived from the private key that we have, we need - # to extract and insert the origin path from the pubkey as well. - privkey += ORIGIN_PATH_RE.search(pubkey).group(1) - break - keys.append((privkey, pubkey)) if musig_partial_sigs is not None: expected_partial_sigs += musig_partial_sigs - # Construct and import each wallet's musig descriptor that - # contains the private key from that wallet and pubkeys of the others - for i, wallet in enumerate(wallets): - if only_one_musig_wallet and i > 0: - continue - desc = pat - import_descs = [] - for j, (priv, pub) in enumerate(keys): - if j == i: - desc = desc.replace(f"${i}", priv) - else: - desc = desc.replace(f"${j}", pub) - - import_descs.append({ - "desc": descsum_create(desc), - "active": True, - "timestamp": "now", - }) - - res = wallet.importdescriptors(import_descs) - for r in res: - assert_equal(r["success"], True) - # Check that the wallets agree on the same musig address addr = None change_addr = None @@ -393,6 +337,11 @@ class WalletMuSigTest(BitcoinTestFramework): def run_test(self): self.def_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name) self.do_test("rawtr(musig(keys/*))", "rawtr(musig($0/<0;1>/*,$1/<1;2>/*,$2/<2;3>/*))") self.do_test("rawtr(musig(keys/*)) with ALL|ANYONECANPAY", "rawtr(musig($0/<0;1>/*,$1/<1;2>/*,$2/<2;3>/*))", "ALL|ANYONECANPAY") self.do_test("tr(musig(keys/*)) no multipath", "tr(musig($0/0/*,$1/1/*,$2/2/*))") @@ -410,8 +359,10 @@ class WalletMuSigTest(BitcoinTestFramework): self.do_test("tr(musig/*,{pk(partial keys diff musig-1/*),pk(partial keys diff musig-2/*)})} script-path", "tr(musig($0,$1,$2)/<3;4>/*,{pk(musig($0,$1)/<5;6>/*),pk(musig($1,$2)/7/*)})", scriptpath=True, nosign_wallets=[0]) # Run failure mode tests after happy path tests to avoid wallet name conflicts - self.test_musig_failure_modes() - + self.test_failure_case_1("missing participant nonce", "tr(musig($0/<0;1>/*,$1/<1;2>/*,$2/<2;3>/*))") + self.test_failure_case_2("insufficient partial signatures", "rawtr(musig($0/<0;1>/*,$1/<1;2>/*,$2/<2;3>/*))") + self.test_failure_case_3("mismatched key order", "tr(musig($0/<0;1>/*,$1/<1;2>/*))") + self.test_failure_case_4("finalize without partial sigs", "rawtr(musig($0/<0;1>/*,$1/<1;2>/*))") if __name__ == '__main__': WalletMuSigTest(__file__).main()</details>
fjahr commented at 5:57 PM on November 24, 2025:Yeah, that looks very good to me, I took this almost exactly as is, squashed the
NUM_WALLETSin there because these lines are now touched as well and made you co-author of that commit. Thanks a lot!in test/functional/wallet_musig.py:412 in c7c9dc2d86 outdated
408 | @@ -237,6 +409,9 @@ def run_test(self): 409 | self.do_test("tr(musig/*,{pk(partial keys diff musig-1/*),pk(partial keys diff musig-2/*)})}", "tr(musig($0,$1,$2)/<3;4>/*,{pk(musig($0,$1)/<5;6>/*),pk(musig($1,$2)/7/*)})") 410 | self.do_test("tr(musig/*,{pk(partial keys diff musig-1/*),pk(partial keys diff musig-2/*)})} script-path", "tr(musig($0,$1,$2)/<3;4>/*,{pk(musig($0,$1)/<5;6>/*),pk(musig($1,$2)/7/*)})", scriptpath=True, nosign_wallets=[0]) 411 | 412 | + # Run failure mode tests after happy path tests to avoid wallet name conflicts
rkrux commented at 2:31 PM on October 17, 2025:In https://github.com/bitcoin/bitcoin/commit/c7c9dc2d8634bc79e76783be113f8d42cee63c6e "test: Add musig failure scenarios"
I don't think this comment is necessary(?) because the tests pass even if the failure cases run first
fjahr commented at 5:58 PM on November 24, 2025:It's gone, probably left-over from an earlier version of the code where this was still the case
rkrux commented at 2:33 PM on October 17, 2025: contributorConcept ACK 73a5d9315340942f030e6586c640b9240498f8ac
Adds more functional tests that are preferred. I also like the last commit 73a5d9315340942f030e6586c640b9240498f8ac quite a lot, thanks for adding it.
achow101 commented at 11:36 PM on November 19, 2025: memberConcept ACK
0f7f0692camusig: Move MUSIG_CHAINCODE to musig.cpp
This is the only place where it is used.
e755614be5sign: Remove duplicate sigversion check
Also improves documentation in the SignMuSig2 function.
c9519c260bmusig: Check session id reuse
Prevent saving another secnonce to the same session id since this might make nonce reuse possible.
fjahr force-pushed on Nov 24, 2025in test/functional/wallet_musig.py:179 in b7215893e5
181 | + }]) 182 | + 183 | + # Addresses should be different due to different key ordering 184 | + addr0 = wallets[0].getnewaddress(address_type="bech32m") 185 | + addr1 = wallets[1].getnewaddress(address_type="bech32m") 186 | + assert addr0 != addr1
rkrux commented at 8:41 AM on November 25, 2025:In b7215893e566dec8a085c08e722aa61ec5a2f5a0 "test: Add musig failure scenarios"
The
self.test_failure_case_3("mismatched key order", "tr(musig($0/<0;1>/*,$1/<1;2>/*))")case seems incorrect to me because the participant pubkeys are sorted before generating the pubkey (and the address) that should result in both the wallets having same address. https://github.com/bitcoin/bitcoin/blob/238c1c8933b1f7479a9bca2b7cb207d26151c39d/src/script/descriptor.cpp#L658This test case is passing because
desc0anddesc1are not successfully imported, and instead an error is thrown from theimportdescriptorscall which is unchecked. I believe the last address assertion is on the ones generated by the non-musig taproot descriptors of the two wallets, which anyway would be different.2025-11-25T08:25:08.970028Z TestFramework (INFO): Testing mismatched key order [{'success': False, 'error': {'code': -8, 'message': 'Active descriptors must be ranged'}}]Upon successfully importing the descriptors after the below diff, the test fails.
diff --git a/test/functional/wallet_musig.py b/test/functional/wallet_musig.py index 0323d488b5..9d6513c703 100755 --- a/test/functional/wallet_musig.py +++ b/test/functional/wallet_musig.py @@ -158,7 +158,7 @@ class WalletMuSigTest(BitcoinTestFramework): self.log.info(f"Testing {comment}") wallets, keys = self.create_wallets_and_keys_from_pattern(pat) - desc0 = f"rawtr(musig({keys[0][0]},{keys[1][1]}))" + desc0 = f"rawtr(musig({keys[0][0]},{keys[1][1]})/*)" wallets[0].importdescriptors([{ "desc": descsum_create(desc0), "active": True, @@ -166,7 +166,7 @@ class WalletMuSigTest(BitcoinTestFramework): }]) # Reverse order to desc0 - desc1 = f"rawtr(musig({keys[1][0]},{keys[0][1]}))" + desc1 = f"rawtr(musig({keys[1][0]},{keys[0][1]})/*)" wallets[1].importdescriptors([{ "desc": descsum_create(desc1), "active": True,2025-11-25T08:36:43.985277Z TestFramework (ERROR): Unexpected exception Traceback (most recent call last): File "/Users/rkrux/projects/bitcoin/test/functional/test_framework/test_framework.py", line 142, in main self.run_test() ~~~~~~~~~~~~~^^ File "/Users/rkrux/projects/bitcoin/./build/test/functional/wallet_musig.py", line 358, in run_test self.test_failure_case_3("mismatched key order", "tr(musig($0/<0;1>/*,$1/<1;2>/*))") ~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/rkrux/projects/bitcoin/./build/test/functional/wallet_musig.py", line 179, in test_failure_case_3 assert addr0 != addr1 ^^^^^^^^^^^^^^ AssertionErrorI believe this test case should be removed.
fjahr commented at 11:22 PM on November 25, 2025:Right, I did have a wrong understanding here. I guess I didn't review the musig descriptors PR and I went by my assumptions, but I have looked at the PR again and at BIP390 and it seems like hitting this error should be impossible. Thanks!
rkrux changes_requestedrkrux commented at 8:45 AM on November 25, 2025: contributorThanks for accepting the refactoring suggestions!
217dbbbb5etest: Add musig failure scenarios
Also changes the the non-constant variable NUM_WALLETS to lower case and refactors the success case scenarios to reuse existing code. Co-authored-by: rkrux <rkrux.connect@gmail.com>
fjahr force-pushed on Nov 25, 2025rkrux approvedrkrux commented at 11:25 AM on November 26, 2025: contributorlgtm ACK 217dbbb
Thanks for accepting all the suggestions.
DrahtBot requested review from achow101 on Nov 26, 2025rkrux commented at 11:31 AM on November 26, 2025: contributorIn PR description:
- primarily adds test coverage for some of the most prominent failure cases in the first commit. + primarily adds test coverage for some of the most prominent failure cases in the last commit.fjahr commented at 2:26 PM on November 26, 2025: contributorIn PR description:
- primarily adds test coverage for some of the most prominent failure cases in the first commit. + primarily adds test coverage for some of the most prominent failure cases in the last commit.Fixed as well
achow101 commented at 1:04 AM on December 23, 2025: memberACK 217dbbbb5e38ab582ee0b3ef37fe9e99d887d7c8
achow101 merged this on Dec 23, 2025achow101 closed this on Dec 23, 2025sedited referenced this in commit f8db928648 on Dec 27, 2025joshdoman referenced this in commit 57661ceac9 on Dec 27, 2025stickies-v referenced this in commit 9fa736f8e4 on Jan 22, 2026stringintech referenced this in commit f651a10526 on Feb 1, 2026stiliyana94 commented at 5:55 PM on May 12, 2026: nonesrc/musig.h
src/script/sign.cpp
src/script/signingprovider.cpp
test/functional/wallet_musig.py
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: 2026-05-20 06:52 UTC
