refactor, key: move `CreateMuSig2{Nonce,PartialSig}` functions to `musig.{h,cpp}` module

theStack commented at 11:45 pm on January 7, 2026: contributor

This PR is a follow-up of #29675, see #29675 (review). It moves all MuSig2 functions that currently live in CKey and call secp256k1 musig module API functions (i.e. secp256k1_musig_...) to the musig.{h,cpp} module, as this seems to be a better place. One small drawback of doing this is that the secp256k1_context_signing object needs to be accessed from the outside; this is currently solved by changing the linkage from static to extern to expose the symbol globally [1]. Happy to add an access function if that is preferred instead.

As the patch is mostly move-only, it can be best reviewed via the git option --color-moved=dimmed-zebra

[1] note that the same approach is used in the Silent Payments sending PR #28201, though having a TODO comment added, see https://github.com/bitcoin/bitcoin/pull/28201/commits/a4ebd7da5aaaa567fa9c613666513341f2aea241#diff-e5cbd40011e760f175b82d7ec44ad512bdb3d9108068b660642204da9c9187b1R25

refactor, key: move `CreateMuSig2Nonce` to `musig.{h,cpp}` module

Nonce creation is mainly derived by randomness, and the secret
key merely serves as (optional) additional data for increasing
misuse-resistance, rather than being a central part that would
justify an own CKey method, so move it to the musig.cpp module.

Can be reviewed via the git option `--color-moved=dimmed-zebra`.

138c6339ef

DrahtBot commented at 11:45 pm on January 7, 2026: 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/34225.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	w0xlt
Concept ACK	rkrux

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

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

[secp256k1_musig_nonce_gen(GetSecp256k1SignContext(), secnonce.Get(), &pubnonce, rand.data(), UCharCast(our_seckey.begin()), &pubkey, sighash.data(), &keyagg_cache, nullptr)] in src/musig.cpp

(No other added lines contain third-or-later positional literal arguments.)

2026-01-16

refactor, key: move `CreateMuSig2PartialSig` to `musig.{h,cpp}` module

Compared to `CreateMuSig2Nonce`, creating a partial signature
has a stronger link to the secret key used, but for consistency
reasons it still makes sense to move all functionality that call
the secp256k1 musig API functions to the `musig.{h,cpp}` module
for consistency.

Can be reviewed via the git option `--color-moved=dimmed-zebra`.

25709aaa79

theStack force-pushed on Jan 8, 2026

DrahtBot added the label CI failed on Jan 8, 2026

DrahtBot commented at 1:15 am on January 8, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/20800284605/job/59743518329 LLM reason (✨ experimental): Lint failure: a new circular dependency (key -> musig -> key) detected by the Python linter, causing all_python_linters to fail.

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.

w0xlt commented at 2:03 am on January 8, 2026: contributor

Approach ACK

DrahtBot removed the label CI failed on Jan 8, 2026

rkrux commented at 2:43 pm on January 8, 2026: contributor

Concept ACK 25709aaa790c05e647457bfce64571ca5495ceeb

I do prefer not seeing the implementation of MuSig2 functions outside the musig module. If I build without the extern keyword, then I see the following warnings but the build doesn’t fail.

 0[ 74%] Linking CXX executable ../bin/bitcoin-wallet
 1ld: warning: duplicate symbol '_secp256k1_context_sign' in:
 2    ../lib/libbitcoin_common.a[29](key.cpp.o)
 3    ../lib/libbitcoin_common.a[32](musig.cpp.o)
 4[ 74%] Built target bitcoin-util
 5[ 74%] Built target bitcoin
 6ld: warning: duplicate symbol '_secp256k1_context_sign' in:
 7    ../lib/libbitcoin_common.a[29](key.cpp.o)
 8    ../lib/libbitcoin_common.a[32](musig.cpp.o)
 9[ 74%] Built target bitcoin-cli
10[ 74%] Built target bitcoin-tx
11ld: warning: duplicate symbol '_secp256k1_context_sign' in:
12    ../lib/libbitcoin_common.a[29](key.cpp.o)
13    ../lib/libbitcoin_common.a[32](musig.cpp.o)
14ld: warning: duplicate symbol '_secp256k1_context_sign' in:
15    ../lib/libbitcoin_common.a[29](key.cpp.o)
16    ../lib/libbitcoin_common.a[32](musig.cpp.o)
17[ 75%] Built target bitcoin-wallet
18[ 75%] Built target bitcoind
19[ 75%] Built target bitcoin-node
20[ 75%] Linking CXX executable ../../bin/test_bitcoin
21ld: warning: ignoring duplicate libraries: '../../lib/libbitcoin_consensus.a', '../secp256k1/lib/libsecp256k1.a'
22ld: warning: duplicate symbol '_secp256k1_context_sign' in:
23    ../../lib/libbitcoin_common.a[29](key.cpp.o)
24    ../../lib/libbitcoin_common.a[32](musig.cpp.o)
25[101%] Built target test_bitcoin

Wondering if using the accessor function would make it more conspicuous that we must use the variable from the key module instead of creating another one.

w0xlt commented at 8:34 pm on January 14, 2026: contributor

This patch could implement an accessor function as suggested by @rkrux .

 0diff --git a/src/key.cpp b/src/key.cpp
 1index 636d8310cd..2de5efb335 100644
 2--- a/src/key.cpp
 3+++ b/src/key.cpp
 4@@ -445,6 +445,11 @@ bool ECC_InitSanityCheck() {
 5     return key.VerifyPubKey(pubkey);
 6 }
 7 
 8+secp256k1_context* GetSecp256k1SignContext()
 9+{
10+    return secp256k1_context_sign;
11+}
12+
13 /** Initialize the elliptic curve support. May not be called twice without calling ECC_Stop first. */
14 static void ECC_Start() {
15     assert(secp256k1_context_sign == nullptr);
16diff --git a/src/key.h b/src/key.h
17index 22f96880b7..e8647a853f 100644
18--- a/src/key.h
19+++ b/src/key.h
20@@ -15,6 +15,8 @@
21 #include <stdexcept>
22 #include <vector>
23 
24+struct secp256k1_context_struct;
25+typedef struct secp256k1_context_struct secp256k1_context;
26 
27 /**
28  * CPrivKey is a serialized private key, with all parameters included
29@@ -311,6 +313,9 @@ private:
30 /** Check that required EC support is available at runtime. */
31 bool ECC_InitSanityCheck();
32 
33+/** Access the secp256k1 context used for signing. */
34+secp256k1_context* GetSecp256k1SignContext();
35+
36 /**
37  * RAII class initializing and deinitializing global state for elliptic curve support.
38  * Only one instance may be initialized at a time.
39diff --git a/src/musig.cpp b/src/musig.cpp
40index f4a8e16608..9a1b34421c 100644
41--- a/src/musig.cpp
42+++ b/src/musig.cpp
43@@ -9,8 +9,6 @@
44 
45 #include <secp256k1_musig.h>
46 
47-extern secp256k1_context* secp256k1_context_sign;
48-
49 //! MuSig2 chaincode as defined by BIP 328
50 using namespace util::hex_literals;
51 constexpr uint256 MUSIG_CHAINCODE{
52@@ -149,7 +147,7 @@ std::vector<uint8_t> CreateMuSig2Nonce(MuSig2SecNonce& secnonce, const uint256&
53 
54     // Generate nonce
55     secp256k1_musig_pubnonce pubnonce;
56-    if (!secp256k1_musig_nonce_gen(secp256k1_context_sign, secnonce.Get(), &pubnonce, rand.data(), UCharCast(our_seckey.begin()), &pubkey, sighash.data(), &keyagg_cache, nullptr)) {
57+    if (!secp256k1_musig_nonce_gen(GetSecp256k1SignContext(), secnonce.Get(), &pubnonce, rand.data(), UCharCast(our_seckey.begin()), &pubkey, sighash.data(), &keyagg_cache, nullptr)) {
58         return {};
59     }
60 
61@@ -166,7 +164,7 @@ std::vector<uint8_t> CreateMuSig2Nonce(MuSig2SecNonce& secnonce, const uint256&
62 std::optional<uint256> CreateMuSig2PartialSig(const uint256& sighash, const CKey& our_seckey, const CPubKey& aggregate_pubkey, const std::vector<CPubKey>& pubkeys, const std::map<CPubKey, std::vector<uint8_t>>& pubnonces, MuSig2SecNonce& secnonce, const std::vector<std::pair<uint256, bool>>& tweaks)
63 {
64     secp256k1_keypair keypair;
65-    if (!secp256k1_keypair_create(secp256k1_context_sign, &keypair, UCharCast(our_seckey.begin()))) return std::nullopt;
66+    if (!secp256k1_keypair_create(GetSecp256k1SignContext(), &keypair, UCharCast(our_seckey.begin()))) return std::nullopt;
67 
68     // Get the keyagg cache and aggregate pubkey
69     secp256k1_musig_keyagg_cache keyagg_cache;

key: add `GetSecp256k1SignContext` access function, use in musig.cpp 0bff8ffcc0

theStack commented at 5:02 pm on January 16, 2026: contributor

Thanks for the reviews, added an access function with the patch from @w0xlt. It’s a separate commit at the end so it could be still removed easily if (for whatever reason) it turns out we still prefer the “extern” variant. Happy to rearrange commits though.

If I build without the extern keyword, then I see the following warnings but the build doesn’t fail.

Didn’t check, but I strongly assume that without the extern keyword, the musig tests would fail (or crash) at run-time, as then the sign context within the musig module is a different object, which hasn’t been initialized.

w0xlt commented at 2:19 am on January 23, 2026: contributor

ACK 0bff8ffcc09a740fa99ec46d47faad76b2ff2814

DrahtBot requested review from rkrux on Jan 23, 2026

refactor, key: move CreateMuSig2{Nonce,PartialSig} functions to musig.{h,cpp} module #34225

Code Coverage & Benchmarks

Reviews

LLM Linter (✨ experimental)

refactor, key: move `CreateMuSig2{Nonce,PartialSig}` functions to `musig.{h,cpp}` module #34225