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 first commit.

The following commits address a few left-over nit comments that didn’t make it in before merge.

test: Add musig failure scenarios c7c9dc2d86

test: Change non-constant variable to lower case 1c57aabd1e

musig: Move MUSIG_CHAINCODE to musig.cpp

This is the only place where it is used.

c2bb4da19c

sign: Remove duplicate sigversion check

Also improves documentation in the SignMuSig2 function.

22e42da8e6

musig: Check session id reuse

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

73a5d93153

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
Concept ACK	rkrux, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

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?

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

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.

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()

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

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

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

Code Coverage & Benchmarks

Reviews