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, 2025
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-02 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me
