`secp256k1_context_static` should be a `const` variable [edited]

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.

real-or-random added the label feature on Nov 20, 2024

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?

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)

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?

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)

real-or-random commented at 9:17 pm on November 22, 2024: contributor

Oh, indeed. @purpleKarrot Can you check if this solves your problem?

purpleKarrot commented at 9:32 pm on November 22, 2024: none

Yes. I confirm this fixes the problem: https://github.com/bitcoin-core/secp256k1/commit/5f6baccf64dbaf820e8f5147cf137ac4a12021f8

real-or-random commented at 10:34 pm on November 22, 2024: contributor

Great, are you willing to open a PR?

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

real-or-random commented at 5:46 pm on November 25, 2024: contributor

I took the liberty to edit the title of the issue.

real-or-random removed the label feature on Nov 25, 2024

real-or-random added the label refactor/smell on Nov 25, 2024

secp256k1_context_static should be a const variable [edited] #1637

`secp256k1_context_static` should be a `const` variable [edited] #1637