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

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. For accessing the secp256k1_context_signing object from the outside, a new function GetSecp256k1SignContext is added in the third commit.

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

<!--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/34225.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

<!--85328a0da195eb286784d51f73fa0af9-->

🚧 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.}

<details><summary>Hints</summary>

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.

</details>

Approach ACK

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.

[ 74%] Linking CXX executable ../bin/bitcoin-wallet
ld: warning: duplicate symbol '_secp256k1_context_sign' in:
    ../lib/libbitcoin_common.a[29](key.cpp.o)
    ../lib/libbitcoin_common.a[32](musig.cpp.o)
[ 74%] Built target bitcoin-util
[ 74%] Built target bitcoin
ld: warning: duplicate symbol '_secp256k1_context_sign' in:
    ../lib/libbitcoin_common.a[29](key.cpp.o)
    ../lib/libbitcoin_common.a[32](musig.cpp.o)
[ 74%] Built target bitcoin-cli
[ 74%] Built target bitcoin-tx
ld: warning: duplicate symbol '_secp256k1_context_sign' in:
    ../lib/libbitcoin_common.a[29](key.cpp.o)
    ../lib/libbitcoin_common.a[32](musig.cpp.o)
ld: warning: duplicate symbol '_secp256k1_context_sign' in:
    ../lib/libbitcoin_common.a[29](key.cpp.o)
    ../lib/libbitcoin_common.a[32](musig.cpp.o)
[ 75%] Built target bitcoin-wallet
[ 75%] Built target bitcoind
[ 75%] Built target bitcoin-node
[ 75%] Linking CXX executable ../../bin/test_bitcoin
ld: warning: ignoring duplicate libraries: '../../lib/libbitcoin_consensus.a', '../secp256k1/lib/libsecp256k1.a'
ld: warning: duplicate symbol '_secp256k1_context_sign' in:
    ../../lib/libbitcoin_common.a[29](key.cpp.o)
    ../../lib/libbitcoin_common.a[32](musig.cpp.o)
[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.

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

diff --git a/src/key.cpp b/src/key.cpp
index 636d8310cd..2de5efb335 100644
--- a/src/key.cpp
+++ b/src/key.cpp
@@ -445,6 +445,11 @@ bool ECC_InitSanityCheck() {
     return key.VerifyPubKey(pubkey);
 }
 
+secp256k1_context* GetSecp256k1SignContext()
+{
+    return secp256k1_context_sign;
+}
+
 /** Initialize the elliptic curve support. May not be called twice without calling ECC_Stop first. */
 static void ECC_Start() {
     assert(secp256k1_context_sign == nullptr);
diff --git a/src/key.h b/src/key.h
index 22f96880b7..e8647a853f 100644
--- a/src/key.h
+++ b/src/key.h
@@ -15,6 +15,8 @@
 #include <stdexcept>
 #include <vector>
 
+struct secp256k1_context_struct;
+typedef struct secp256k1_context_struct secp256k1_context;
 
 /**
  * CPrivKey is a serialized private key, with all parameters included
@@ -311,6 +313,9 @@ private:
 /** Check that required EC support is available at runtime. */
 bool ECC_InitSanityCheck();
 
+/** Access the secp256k1 context used for signing. */
+secp256k1_context* GetSecp256k1SignContext();
+
 /**
  * RAII class initializing and deinitializing global state for elliptic curve support.
  * Only one instance may be initialized at a time.
diff --git a/src/musig.cpp b/src/musig.cpp
index f4a8e16608..9a1b34421c 100644
--- a/src/musig.cpp
+++ b/src/musig.cpp
@@ -9,8 +9,6 @@
 
 #include <secp256k1_musig.h>
 
-extern secp256k1_context* secp256k1_context_sign;
-
 //! MuSig2 chaincode as defined by BIP 328
 using namespace util::hex_literals;
 constexpr uint256 MUSIG_CHAINCODE{
@@ -149,7 +147,7 @@ std::vector<uint8_t> CreateMuSig2Nonce(MuSig2SecNonce& secnonce, const uint256&
 
     // Generate nonce
     secp256k1_musig_pubnonce pubnonce;
-    if (!secp256k1_musig_nonce_gen(secp256k1_context_sign, secnonce.Get(), &pubnonce, rand.data(), UCharCast(our_seckey.begin()), &pubkey, sighash.data(), &keyagg_cache, nullptr)) {
+    if (!secp256k1_musig_nonce_gen(GetSecp256k1SignContext(), secnonce.Get(), &pubnonce, rand.data(), UCharCast(our_seckey.begin()), &pubkey, sighash.data(), &keyagg_cache, nullptr)) {
         return {};
     }
 
@@ -166,7 +164,7 @@ std::vector<uint8_t> CreateMuSig2Nonce(MuSig2SecNonce& secnonce, const uint256&
 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)
 {
     secp256k1_keypair keypair;
-    if (!secp256k1_keypair_create(secp256k1_context_sign, &keypair, UCharCast(our_seckey.begin()))) return std::nullopt;
+    if (!secp256k1_keypair_create(GetSecp256k1SignContext(), &keypair, UCharCast(our_seckey.begin()))) return std::nullopt;
 
     // Get the keyagg cache and aggregate pubkey
     secp256k1_musig_keyagg_cache keyagg_cache;

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.

ACK 0bff8ffcc09a740fa99ec46d47faad76b2ff2814

@rkrux can you give this another look?

Taking another look - the last sentence in the first paragraph of the PR description is outdated and needs to be updated.

Taking another look - the last sentence in the first paragraph of the PR description is outdated and needs to be updated.

Updated the PR description accordingly, and also removed the reference to #28201, since that PR is closed now.

Code review 0bff8ffcc09a740fa99ec46d47faad76b2ff2814

Rebased on master and addressed the review comments by @rkrux (https://github.com/bitcoin/bitcoin/pull/34225#discussion_r3234702516, #34225 (review)). Thanks!

reACK 8ba5f68b1df99350aa037a644041034cf46842dc

lgtm ACK 8ba5f68b1df99350aa037a644041034cf46842dc

This is a helpful refactor because there's a lot of Musig2 specific code in these functions that I do like seeing in the musig* files. Adding the accessor function for the signing context should make it easy for finding its usages outside the key.cpp file.

Lastly, both the key* and musig* files are at the same level in the src directory, so I don't think this refactor takes the signing context too far away. Maybe in future a case can be made to move both the key* and musig* files within the script dir so that their presence is localised to signing code but that would require analysing the current usages of CKey et al.

ACK 8ba5f68b1df99350aa037a644041034cf46842dc

ACK 8ba5f68b1df99350aa037a644041034cf46842dc