wallet: Expand MuSig test coverage and follow-ups

fjahr commented at 0:02 am on October 16, 2025: contributor

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.

DrahtBot added the label Wallet on Oct 16, 2025

DrahtBot commented at 0:02 am on October 16, 2025: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux
Concept ACK	achow101

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

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”

Miniscript pattern or Multipath pattern?

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_case now.

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_test func so that wallet creation and expectation calculations are not together.
Because of the above separation, extracted create_wallets_and_keys_from_pattern func to be used in both success and failure cases.
Extracted construct_and_import_musig_descriptor_in_wallets func to be used in both success and failure cases.
Split the test_musig_failure_modes func into 4 funcs that have a similar func signature like the do_test func 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.

  0diff --git a/test/functional/wallet_musig.py b/test/functional/wallet_musig.py
  1index 55129b1676..64e1e40259 100755
  2--- a/test/functional/wallet_musig.py
  3+++ b/test/functional/wallet_musig.py
  4@@ -28,43 +28,52 @@ class WalletMuSigTest(BitcoinTestFramework):
  5     def skip_test_if_missing_module(self):
  6         self.skip_if_no_wallet()
  7 
  8-    def setup_musig_scenario(self, num_wallets=3):
  9-        # Miniscript pattern based on number of wallets
 10-        placeholders = ",".join(f"${i}/<{i};{i+1}>/*" for i in range(num_wallets))
 11-        pattern = f"rawtr(musig({placeholders}))"
 12-
 13+    # Create wallets and extract keys
 14+    def create_wallets_and_keys_from_pattern(self, pat):
 15         wallets = []
 16         keys = []
 17 
 18-        # Create wallets and extract keys
 19-        for i in range(num_wallets):
 20-            wallet_name = f"musig_{self.wallet_num}"
 21-            self.wallet_num += 1
 22-            self.nodes[0].createwallet(wallet_name)
 23-            wallet = self.nodes[0].get_wallet_rpc(wallet_name)
 24-            wallets.append(wallet)
 25-
 26-            for priv_desc in wallet.listdescriptors(True)["descriptors"]:
 27-                desc = priv_desc["desc"]
 28-                if not desc.startswith("tr("):
 29-                    continue
 30-                privkey = PRIVKEY_RE.search(desc).group(1)
 31-                break
 32-            for pub_desc in wallet.listdescriptors()["descriptors"]:
 33-                desc = pub_desc["desc"]
 34-                if not desc.startswith("tr("):
 35+        for musig in MUSIG_RE.findall(pat):
 36+            for placeholder in PLACEHOLDER_RE.findall(musig):
 37+                wallet_index = int(placeholder[1:])
 38+                if wallet_index < len(wallets):
 39                     continue
 40-                pubkey = PUBKEY_RE.search(desc).group(1)
 41-                privkey += ORIGIN_PATH_RE.search(pubkey).group(1)
 42-                break
 43-            keys.append((privkey, pubkey))
 44 
 45-        # Construct and import descriptors
 46+                wallet_name = f"musig_{self.wallet_num}"
 47+                self.wallet_num += 1
 48+                self.nodes[0].createwallet(wallet_name)
 49+                wallet = self.nodes[0].get_wallet_rpc(wallet_name)
 50+                wallets.append(wallet)
 51+
 52+                for priv_desc in wallet.listdescriptors(True)["descriptors"]:
 53+                    desc = priv_desc["desc"]
 54+                    if not desc.startswith("tr("):
 55+                        continue
 56+                    privkey = PRIVKEY_RE.search(desc).group(1)
 57+                    break
 58+                for pub_desc in wallet.listdescriptors()["descriptors"]:
 59+                    desc = pub_desc["desc"]
 60+                    if not desc.startswith("tr("):
 61+                        continue
 62+                    pubkey = PUBKEY_RE.search(desc).group(1)
 63+                    # Since the pubkey is derived from the private key that we have, we need
 64+                    # to extract and insert the origin path from the pubkey as well.
 65+                    privkey += ORIGIN_PATH_RE.search(pubkey).group(1)
 66+                    break
 67+                keys.append((privkey, pubkey))
 68+
 69+        return wallets, keys
 70+
 71+    # Construct and import each wallet's musig descriptor that
 72+    # contains the private key from that wallet and pubkeys of the others
 73+    def construct_and_import_musig_descriptor_in_wallets(self, pat, wallets, keys, only_one_musig_wallet=False):
 74         for i, wallet in enumerate(wallets):
 75-            desc = pattern
 76+            if only_one_musig_wallet and i > 0:
 77+                continue
 78+            desc = pat
 79             for j, (priv, pub) in enumerate(keys):
 80                 if j == i:
 81-                    desc = desc.replace(f"${j}", priv)
 82+                    desc = desc.replace(f"${i}", priv)
 83                 else:
 84                     desc = desc.replace(f"${j}", pub)
 85 
 86@@ -78,6 +87,10 @@ class WalletMuSigTest(BitcoinTestFramework):
 87             for r in res:
 88                 assert_equal(r["success"], True)
 89 
 90+    def setup_musig_scenario(self, pat):
 91+        wallets, keys = self.create_wallets_and_keys_from_pattern(pat)
 92+        self.construct_and_import_musig_descriptor_in_wallets(pat, wallets, keys, only_one_musig_wallet=False)
 93+
 94         # Fund address
 95         addr = wallets[0].getnewaddress(address_type="bech32m")
 96         for wallet in wallets[1:]:
 97@@ -97,11 +110,9 @@ class WalletMuSigTest(BitcoinTestFramework):
 98 
 99         return wallets, psbt
100 
101-    def test_musig_failure_modes(self):
102-        """Test various MuSig2 failure scenarios"""
103-
104-        self.log.info("Testing missing participant nonce")
105-        wallets, psbt = self.setup_musig_scenario(num_wallets=3)
106+    def test_failure_case_1(self, comment, pat):
107+        self.log.info(f"Testing {comment}")
108+        wallets, psbt = self.setup_musig_scenario(pat)
109 
110         # Only 2 out of 3 participants provide nonces
111         nonce_psbts = []
112@@ -121,8 +132,9 @@ class WalletMuSigTest(BitcoinTestFramework):
113             # There are still only two nonces
114             assert_equal(len(dec["inputs"][0].get("musig2_pubnonces", [])), 2)
115 
116-        self.log.info("Testing insufficient partial signatures")
117-        wallets, psbt = self.setup_musig_scenario(num_wallets=3)
118+    def test_failure_case_2(self, comment, pat):
119+        self.log.info(f"Testing {comment}")
120+        wallets, psbt = self.setup_musig_scenario(pat)
121         nonce_psbts = [w.walletprocesspsbt(psbt=psbt)["psbt"] for w in wallets]
122         comb_nonce_psbt = self.nodes[0].combinepsbt(nonce_psbts)
123 
124@@ -142,30 +154,9 @@ class WalletMuSigTest(BitcoinTestFramework):
125         dec = self.nodes[0].decodepsbt(comb_psig_psbt)
126         assert_equal(len(dec["inputs"][0]["musig2_partial_sigs"]), 2)
127 
128-        self.log.info("Testing mismatched key order")
129-        wallets = []
130-        keys = []
131-        for i in range(2):
132-            wallet_name = f"musig_{self.wallet_num}"
133-            self.wallet_num += 1
134-            self.nodes[0].createwallet(wallet_name)
135-            wallet = self.nodes[0].get_wallet_rpc(wallet_name)
136-            wallets.append(wallet)
137-
138-            for priv_desc in wallet.listdescriptors(True)["descriptors"]:
139-                desc = priv_desc["desc"]
140-                if not desc.startswith("tr("):
141-                    continue
142-                privkey = PRIVKEY_RE.search(desc).group(1)
143-                break
144-            for pub_desc in wallet.listdescriptors()["descriptors"]:
145-                desc = pub_desc["desc"]
146-                if not desc.startswith("tr("):
147-                    continue
148-                pubkey = PUBKEY_RE.search(desc).group(1)
149-                privkey += ORIGIN_PATH_RE.search(pubkey).group(1)
150-                break
151-            keys.append((privkey, pubkey))
152+    def test_failure_case_3(self, comment, pat):
153+        self.log.info(f"Testing {comment}")
154+        wallets, keys = self.create_wallets_and_keys_from_pattern(pat)
155 
156         desc0 = f"rawtr(musig({keys[0][0]},{keys[1][1]}))"
157         wallets[0].importdescriptors([{
158@@ -187,8 +178,9 @@ class WalletMuSigTest(BitcoinTestFramework):
159         addr1 = wallets[1].getnewaddress(address_type="bech32m")
160         assert addr0 != addr1
161 
162-        self.log.info("Testing finalize without partial sigs")
163-        wallets, psbt = self.setup_musig_scenario(num_wallets=2)
164+    def test_failure_case_4(self, comment, pat):
165+        self.log.info(f"Testing {comment}")
166+        wallets, psbt = self.setup_musig_scenario(pat)
167         nonce_psbts = [w.walletprocesspsbt(psbt=psbt)["psbt"] for w in wallets]
168         comb_nonce_psbt = self.nodes[0].combinepsbt(nonce_psbts)
169 
170@@ -199,17 +191,14 @@ class WalletMuSigTest(BitcoinTestFramework):
171         assert "musig2_pubnonces" in dec["inputs"][0]
172         assert "musig2_partial_sigs" not in dec["inputs"][0]
173 
174-
175     def do_test(self, comment, pattern, sighash_type=None, scriptpath=False, nosign_wallets=None, only_one_musig_wallet=False):
176         self.log.info(f"Testing {comment}")
177         has_internal = MULTIPATH_TWO_RE.search(pattern) is not None
178 
179-        wallets = []
180-        keys = []
181-
182         pat = pattern.replace("$H", H_POINT)
183+        wallets, keys = self.create_wallets_and_keys_from_pattern(pat)
184+        self.construct_and_import_musig_descriptor_in_wallets(pat, wallets, keys, only_one_musig_wallet)
185 
186-        # Figure out how many wallets are needed and create them
187         expected_pubnonces = 0
188         expected_partial_sigs = 0
189         for musig in MUSIG_RE.findall(pat):
190@@ -224,54 +213,9 @@ class WalletMuSigTest(BitcoinTestFramework):
191                     musig_partial_sigs += 1
192                 if wallet_index < len(wallets):
193                     continue
194-                wallet_name = f"musig_{self.wallet_num}"
195-                self.wallet_num += 1
196-                self.nodes[0].createwallet(wallet_name)
197-                wallet = self.nodes[0].get_wallet_rpc(wallet_name)
198-                wallets.append(wallet)
199-
200-                for priv_desc in wallet.listdescriptors(True)["descriptors"]:
201-                    desc = priv_desc["desc"]
202-                    if not desc.startswith("tr("):
203-                        continue
204-                    privkey = PRIVKEY_RE.search(desc).group(1)
205-                    break
206-                for pub_desc in wallet.listdescriptors()["descriptors"]:
207-                    desc = pub_desc["desc"]
208-                    if not desc.startswith("tr("):
209-                        continue
210-                    pubkey = PUBKEY_RE.search(desc).group(1)
211-                    # Since the pubkey is derived from the private key that we have, we need
212-                    # to extract and insert the origin path from the pubkey as well.
213-                    privkey += ORIGIN_PATH_RE.search(pubkey).group(1)
214-                    break
215-                keys.append((privkey, pubkey))
216             if musig_partial_sigs is not None:
217                 expected_partial_sigs += musig_partial_sigs
218 
219-        # Construct and import each wallet's musig descriptor that
220-        # contains the private key from that wallet and pubkeys of the others
221-        for i, wallet in enumerate(wallets):
222-            if only_one_musig_wallet and i > 0:
223-                continue
224-            desc = pat
225-            import_descs = []
226-            for j, (priv, pub) in enumerate(keys):
227-                if j == i:
228-                    desc = desc.replace(f"${i}", priv)
229-                else:
230-                    desc = desc.replace(f"${j}", pub)
231-
232-            import_descs.append({
233-                "desc": descsum_create(desc),
234-                "active": True,
235-                "timestamp": "now",
236-            })
237-
238-            res = wallet.importdescriptors(import_descs)
239-            for r in res:
240-                assert_equal(r["success"], True)
241-
242         # Check that the wallets agree on the same musig address
243         addr = None
244         change_addr = None
245@@ -393,6 +337,11 @@ class WalletMuSigTest(BitcoinTestFramework):
246     def run_test(self):
247         self.def_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name)
248 
249         self.do_test("rawtr(musig(keys/*))", "rawtr(musig($0/<0;1>/*,$1/<1;2>/*,$2/<2;3>/*))")
250         self.do_test("rawtr(musig(keys/*)) with ALL|ANYONECANPAY", "rawtr(musig($0/<0;1>/*,$1/<1;2>/*,$2/<2;3>/*))", "ALL|ANYONECANPAY")
251         self.do_test("tr(musig(keys/*)) no multipath", "tr(musig($0/0/*,$1/1/*,$2/2/*))")
252@@ -410,8 +359,10 @@ class WalletMuSigTest(BitcoinTestFramework):
253         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])
254 
255         # Run failure mode tests after happy path tests to avoid wallet name conflicts
256-        self.test_musig_failure_modes()
257-
258+        self.test_failure_case_1("missing participant nonce", "tr(musig($0/<0;1>/*,$1/<1;2>/*,$2/<2;3>/*))")
259+        self.test_failure_case_2("insufficient partial signatures", "rawtr(musig($0/<0;1>/*,$1/<1;2>/*,$2/<2;3>/*))")
260+        self.test_failure_case_3("mismatched key order", "tr(musig($0/<0;1>/*,$1/<1;2>/*))")
261+        self.test_failure_case_4("finalize without partial sigs", "rawtr(musig($0/<0;1>/*,$1/<1;2>/*))")
262 
263 if __name__ == '__main__':
264     WalletMuSigTest(__file__).main()

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_WALLETS in 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: contributor

Concept 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: member

Concept ACK

musig: Move MUSIG_CHAINCODE to musig.cpp

This is the only place where it is used.

0f7f0692ca

sign: Remove duplicate sigversion check

Also improves documentation in the SignMuSig2 function.

e755614be5

musig: Check session id reuse

Prevent saving another secnonce to the same session id since this might make nonce reuse possible.

c9519c260b

fjahr force-pushed on Nov 24, 2025

fjahr commented at 5:59 pm on November 24, 2025: contributor

Addressed the feedback from @rkrux , thanks for the review!

in 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#L658

This test case is passing because desc0 and desc1 are not successfully imported, and instead an error is thrown from the importdescriptors call 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.

02025-11-25T08:25:08.970028Z TestFramework (INFO): Testing mismatched key order
1[{'success': False,
2  'error': {'code': -8, 'message': 'Active descriptors must be ranged'}}]

Upon successfully importing the descriptors after the below diff, the test fails.

 0diff --git a/test/functional/wallet_musig.py b/test/functional/wallet_musig.py
 1index 0323d488b5..9d6513c703 100755
 2--- a/test/functional/wallet_musig.py
 3+++ b/test/functional/wallet_musig.py
 4@@ -158,7 +158,7 @@ class WalletMuSigTest(BitcoinTestFramework):
 5         self.log.info(f"Testing {comment}")
 6         wallets, keys = self.create_wallets_and_keys_from_pattern(pat)
 7 
 8-        desc0 = f"rawtr(musig({keys[0][0]},{keys[1][1]}))"
 9+        desc0 = f"rawtr(musig({keys[0][0]},{keys[1][1]})/*)"
10         wallets[0].importdescriptors([{
11             "desc": descsum_create(desc0),
12             "active": True,
13@@ -166,7 +166,7 @@ class WalletMuSigTest(BitcoinTestFramework):
14         }])
15 
16         # Reverse order to desc0
17-        desc1 = f"rawtr(musig({keys[1][0]},{keys[0][1]}))"
18+        desc1 = f"rawtr(musig({keys[1][0]},{keys[0][1]})/*)"
19         wallets[1].importdescriptors([{
20             "desc": descsum_create(desc1),
21             "active": True,

 02025-11-25T08:36:43.985277Z TestFramework (ERROR): Unexpected exception
 1Traceback (most recent call last):
 2  File "/Users/rkrux/projects/bitcoin/test/functional/test_framework/test_framework.py", line 142, in main
 3    self.run_test()
 4    ~~~~~~~~~~~~~^^
 5  File "/Users/rkrux/projects/bitcoin/./build/test/functional/wallet_musig.py", line 358, in run_test
 6    self.test_failure_case_3("mismatched key order", "tr(musig($0/<0;1>/*,$1/<1;2>/*))")
 7    ~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 8  File "/Users/rkrux/projects/bitcoin/./build/test/functional/wallet_musig.py", line 179, in test_failure_case_3
 9    assert addr0 != addr1
10           ^^^^^^^^^^^^^^
11AssertionError

I 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_requested

rkrux commented at 8:45 am on November 25, 2025: contributor

Thanks for accepting the refactoring suggestions!

test: 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>

217dbbbb5e

fjahr force-pushed on Nov 25, 2025

rkrux approved

rkrux commented at 11:25 am on November 26, 2025: contributor

lgtm ACK 217dbbb

Thanks for accepting all the suggestions.

DrahtBot requested review from achow101 on Nov 26, 2025

rkrux commented at 11:31 am on November 26, 2025: contributor

In PR description:

0- primarily adds test coverage for some of the most prominent failure cases in the first commit.
1+ 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: contributor

In PR description:

0- primarily adds test coverage for some of the most prominent failure cases in the first commit.
1+ primarily adds test coverage for some of the most prominent failure cases in the last commit.

Fixed as well

wallet: Expand MuSig test coverage and follow-ups #33636

Code Coverage & Benchmarks

Reviews