Open canepat opened 1 month ago
First of all, I think node and sentry should have its own secp256k1 contexts. They differ in capabilities and sentry is additionally handling private keys (for that you can consider context randomization).
I think we have to pick on of the classic solutions:
SecP256K1Context
singleton,SecP256K1Context
instance,SecP256K1Context
in sentry main()
and pass everywhere as an argument.Option 2 is probably the quickest. I can make a PR if you this is good start.
Option 2 is probably the quickest. I can make a PR if you this is good start.
It's fine for me. @battlmonstr what do you think?
@chfast I prefer to not introduce a global/singleton if possible, but also not a fan of exposing this implementation detail and passing it as a parameter everywhere 🤷 . Note that we need at least 2 contexts: one with allow_verify=true, allow_sign=false
(default) and one with allow_verify=false, allow_sign=true
. They differ in capabilities.
Do you know if functions we need to call with secp256k1_context*
are thread-safe in respect to the context? If it is thread-safe then the simplest is to turn all usages in cpp files into static variables as in:
static SecP256K1Context ctx;
in this case it will be a small finite number of secp256k1_context_create
calls overall (while still keeping it a private implementation detail).
If it is not thread safe, we could maybe introduce a pool of contexts to reuse where SecP256K1Context constructor can take one if it is available or create and add one if not.
Reading the context is thread safe (parameter const secp256k1_context*
), creating/randomizing it is not (parameter secp256k1_context*
).
You can have single context for both capabilities allow_verify=true, allow_sign=true
. I don't think there are any benefits of having them separated.
static SecP256K1Context ctx;
Do you mean function-level static or "global" static variable?
I don't think the function-level static
is good because it will wrapped with a mutex.
The file-level is what I meant by "global instance".
@chfast yes, I meant function level static, or a class-level (a static member), but I'm fine with global level in .cpp files too. Out of curiosity, does a non-global static always imply a mutex? Would std::call_once
help or is it also a mutex inside or just an atomic?
If allow_verify=true, allow_sign=true
works, then perhaps the simplest is to make secp256k1_context
a static member of SecP256K1Context
initialized once with smth like std::call_once
? I'm not sure why I did it that way (have separated parameters). It would be a good simplification if it works in both sign and verify cases with having both options set to true
.
In sentry the secp256k1 is created for every operation which requires it (see the example below). This is not efficient, only single global context is enough.