key: use static context for libsecp256k1 calls where applicable #33399

1. 
   theStack commented at 11:48 pm on September 15, 2025: contributor

   The dynamically created signing context for libsecp256k1 calls is only needed for functions that involve generator point multiplication with a secret key, i.e. different variants of public key creation and signing. The API docs hint to those by stating “(not secp256k1_context_static)” for the context parameter. In our case that applies to the following calls:

   • secp256k1_ec_pubkey_create
   • secp256k1_keypair_create
   • secp256k1_ellswift_create
   • secp256k1_ecdsa_sign
   • secp256k1_ecdsa_sign_recoverable
   • secp256k1_schnorrsig_sign32
   • ec_seckey_export_der (not a direct secp256k1 function, but calls secp256k1_ec_pubkey_create inside)

   For all the other secp256k1 calls we can simply use the static context. This is done for consistency to other calls that already use secp256k1_context_static, and also to reduce dependencies on the global signing context variable. Looked closer at this in the course of reviewing #29675, where some functions used the signing context that didn’t need to, avoiding a move to another module (see #29675 (review)).

2. 
   DrahtBot commented at 11:48 pm on September 15, 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/33399.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	Eunovo, furszy, rkrux
   Concept ACK	real-or-random, Raimo33

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

3. 
   furszy commented at 0:49 am on September 16, 2025: member

   Concept ACK
4. in src/test/fuzz/secp256k1_ec_seckey_import_export_der.cpp:20 in 455ff6c42e outdated

   19@@ -20,7 +20,7 @@ FUZZ_TARGET(secp256k1_ec_seckey_import_export_der)
   20     secp256k1_context* secp256k1_context_sign = secp256k1_context_create(SECP256K1_CONTEXT_NONE);
   

   ---

   ---

   

   l0rinc commented at 1:18 am on September 16, 2025:

   if we still need this, we can move it to the scope where it’s used (with the secp256k1_context_destroy call)

   ---

   

   theStack commented at 8:00 pm on September 16, 2025:

   Makes sense yes, done. Packed the scope reduction in the same commit as I think the diff is still digestible, but happy to divide it into two commits if that’s preferable for reviewers.
5. in src/test/fuzz/secp256k1_ec_seckey_import_export_der.cpp:30 in 455ff6c42e outdated

   29@@ -30,7 +30,7 @@ FUZZ_TARGET(secp256k1_ec_seckey_import_export_der)
   30         const bool exported = ec_seckey_export_der(secp256k1_context_sign, seckey.data(), &seckeylen, key32.data(), compressed);
   

   ---

   ---

   

   l0rinc commented at 1:38 am on September 16, 2025:

   Not my area of expertise, but is secp256k1_context_sign even the correct name here, wasn’t that deprecated in favor of SECP256K1_CONTEXT_NONE?
6. 
   l0rinc commented at 1:40 am on September 16, 2025: contributor

   Thanks for fixing these. Is there a way to separate these two usages by type somehow - I don’t claim to fully understand when the state is needed and when a static can be used. And would it make sense to rename the old secp256k1_context_sign references while we’re here?

   nit: typo in description for the context paramter

7. 
   fanquake commented at 7:17 am on September 16, 2025: member

   cc @jonasnick @real-or-random
8. 
   real-or-random commented at 7:34 am on September 16, 2025: contributor

   I don’t claim to fully understand when the state is needed and when a static can be used.

   The simple answer is that the static context can be used unless the API docs for the function in question say that it cannot be used. ;)

   A better answer is that the full context is needed whenever the generator point G is multiplied by a secret scalar. Or, roughly speaking, whenever a secret key is involved, i.e., key generation or signing. The exceptions to this rule are the computation of the shared secret in ECDH and EllSwift.

   And would it make sense to rename the old secp256k1_context_sign references while we’re here?

   I’m not sure. SECP256K1_CONTEXT_SIGN was deprecated, but secp256k1_context_sign still expresses that this is the context used for signing. On the other hand, that’s imprecise. It’s for signing and for key generation. So perhaps a better name will be secp256k1_context_full, or simply secp256k1_context, just to distinguish it from the static one.

9. 
   real-or-random commented at 7:35 am on September 16, 2025: contributor

   Concept ACK
10. 
    Raimo33 commented at 2:55 pm on September 16, 2025: none

    Concept ACK
11. key: use static context for libsecp256k1 calls where applicable

    The dynamically created signing context for libsecp256k1 calls is only
    needed for functions that involve generator point multiplication with a
    secret key, i.e. different variants of public key creation and signing.
    The API docs hint to this by stating "not secp256k1_context_static" for
    the context parameter. In our case that applies to the following calls:
    - `secp256k1_ec_pubkey_create`
    - `secp256k1_keypair_create`
    - `secp256k1_ellswift_create`
    - `secp256k1_ecdsa_sign`
    - `secp256k1_ecdsa_sign_recoverable`
    - `secp256k1_schnorrsig_sign32`
    - `ec_seckey_export_der` (not a direct secp256k1 function, but calls
      `secp256k1_ec_pubkey_create` inside)
    
    For all the other secp256k1 calls we can simply use the static context.

    1ff9e92948
12. theStack force-pushed on Sep 16, 2025
13. 
    theStack commented at 8:00 pm on September 16, 2025: contributor

    Thanks for the reviews, fixed the typo in commit message and PR description (s/paramter/parameter/) and reduced the scope of secp256k1_context_sign in the fuzz test as suggested (h/t @l0rinc). Didn’t do any renaming of the context object, as I haven’t stumbled upon a name yet where I was convinced that it’s much better (I agree with @real-or-random though that’s it’s imprecise).
14. 
    Eunovo commented at 2:04 pm on September 23, 2025: contributor

    ACK https://github.com/bitcoin/bitcoin/pull/33399/commits/1ff9e929489e625a603e8755b8efe849feda1f16

    I verified that all the secp256k1_context_sign that were replaced in this commit were not used (apart from the occasional VERIFY_CHECK(ctx != null)). I looked through the secp256k1 code of each function to confirm that they do not use the dynamic context, so it is safe to replace them with secp256k1_context_static.

    I also confirmed that the changed code in key.cpp is covered by tests, see: https://maflcko.github.io/b-c-cov/total.coverage/src/key.cpp.gcov.html

15. DrahtBot requested review from furszy on Sep 23, 2025
16. DrahtBot requested review from real-or-random on Sep 23, 2025
17. 
    furszy commented at 3:30 pm on September 24, 2025: member

    ACK 1ff9e929489e625a603e8755b8efe849feda1f16
18. 
    rkrux commented at 12:50 pm on September 25, 2025: contributor

    crACK 1ff9e929489e625a603e8755b8efe849feda1f16

    I have limited exposure to this area. I agree with the intent of using the static context in more places where the non-static one is not really required.

    I checked the implementation of the functions that have the static context passed now, and almost of them don’t seem to use the passed context besides for the not-null checks. The secp256k1_declassify function internally called via secp256k1_keypair_xonly_pub does seem to use the context but only to expect the corresponding property to be 0 that the static context should satisfy.

    Also verified that this PR doesn’t use the static context in those functions that have the (not secp256k1_context_static) note in their docs.

Contributors
theStack DrahtBot furszy l0rinc fanquake real-or-random Raimo33 Eunovo rkrux

Review Requested
real-or-random