secp256k1_context_static should be a const variable [edited] #1637

issue purpleKarrot openend this issue on November 20, 2024
  1. purpleKarrot commented at 1:51 pm on November 20, 2024: none

    Some language bindings have issues with exported variables. The static context is currently exported as a variable:

    0SECP256K1_API const secp256k1_context *secp256k1_context_static;
    

    which is implemented as:

    0const secp256k1_context *secp256k1_context_static = &secp256k1_context_static_;
    

    A more portable way would be to provide it through a function:

    0SECP256K1_API const secp256k1_context *secp256k1_context_static(void);
    

    with implementation:

    0const secp256k1_context *secp256k1_context_static(void)
    1{
    2    return &secp256k1_context_static_;
    3}
    

    PS: It is clear that changing the symbol into a function is a breaking change and therefore off the table. However, please consider adding a new symbol with the proposed implementation.

  2. real-or-random added the label feature on Nov 20, 2024
  3. real-or-random commented at 3:32 pm on November 20, 2024: contributor

    Some language bindings have issues with exported variables.

    Can you elaborate a bit? Which bindings and what are the issues?

  4. purpleKarrot commented at 7:25 pm on November 20, 2024: none

    In Swift, when secp256k1_context_static is passed to a function, the compiler complains:

    Reference to var ‘secp256k1_context_static’ is not concurrency-safe because it involves shared mutable state

    This is definitely a false positive, because the instance is const. The workaround is to define a custom function in the ffi wrapper: https://github.com/swift-bitcoin/swift-bitcoin/blob/develop/src/ecc-helper/ecdsa.c#L6-L8

    I remember having had issues with exported variables in the past with other languages. I don’t remember the exact details. It may have been with Javascript, Go, Lua, or Matlab. It is less of an issue when the variable is of a builtin type, which is the case here with a pointer.

    While it is easily possible to find work-arounds for any ffi, providing functions only is just more intuitive. Consider this Python:

     0>>> from ctypes import CDLL
     1>>> lib = CDLL("libsecp256k1.5.dylib")
     2>>> lib
     3<CDLL 'libsecp256k1.5.dylib', handle 6d672be0 at 0x102b3a430>
     4>>> lib.secp256k1_selftest
     5<_FuncPtr object at 0x102b83340>
     6>>> lib.secp256k1_selftest()
     7-1486072369
     8>>> lib.secp256k1_context_static
     9<_FuncPtr object at 0x102b83a00>   # <--- orly?
    10>>> lib.secp256k1_context_static()
    11fish: Job 1, 'python3' terminated by signal SIGBUS (Misaligned address error)
    
  5. real-or-random commented at 12:15 pm on November 21, 2024: contributor

    In Swift, when secp256k1_context_static is passed to a function, the compiler complains:

    Reference to var ‘secp256k1_context_static’ is not concurrency-safe because it involves shared mutable state

    This is definitely a false positive, because the instance is const.

    Not sure. A pointer to const is a promise not to mutate through the pointer, but that doesn’t mean that the object pointed to cannot be mutated at all (e.g., there could exist a pointer to non-const). In this case, secp256k1_context_static_ is indeed static const, but I assume this is (in general) not obvious if you just see the shared library. I mean, the identifier secp256k1_context_static_ doesn’t even make sense at this level of abstraction.

    Anyway, I agree that if Swift’s FFI is happy with a function that returns a pointer to const, then it should also be happy with a variable of the same type…


    I don’t think it hurts to add something like this:

    0/** Return secp256k1_get_context_static. */
    1const secp256k1_context* secp256k1_get_context_static(void);
    2
    3const secp256k1_context* secp256k1_get_context_static(void) {
    4    return secp256k1_context_static;
    5}
    

    But then we’d need to do the same for all the other exported variables (and add a rule to CONTRIBUTING.md). I’m not entirely convinced that the best place to do this in libsecp256k1. Shouldn’t it be the job of bindings to provide a wrapper if necessary?

  6. sipa commented at 3:33 am on November 22, 2024: contributor

    The Swift FFI is right, actually.

    The variable secp256k1_context_static is a mutable global variable (it points to a constant object, but the variable itself isn’t constant).

    I think we should change the definition from

    0const secp256k1_context *secp256k1_context_static = &secp256k1_context_static_;
    

    to:

    0const secp256k1_context * const secp256k1_context_static = &secp256k1_context_static_;
    

    (and similarly in the .h file)

  7. real-or-random commented at 9:17 pm on November 22, 2024: contributor
    Oh, indeed. @purpleKarrot Can you check if this solves your problem?
  8. purpleKarrot commented at 9:32 pm on November 22, 2024: none
  9. real-or-random commented at 10:34 pm on November 22, 2024: contributor
    Great, are you willing to open a PR?
  10. real-or-random renamed this:
    feature request: export static context through a function
    `secp256k1_context_static` should be a `const` variable [edited]
    on Nov 25, 2024
  11. real-or-random commented at 5:46 pm on November 25, 2024: contributor
    I took the liberty to edit the title of the issue.
  12. real-or-random removed the label feature on Nov 25, 2024
  13. real-or-random added the label refactor/smell on Nov 25, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-03 21:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me