sentry: creating a new secp256k1 context for every operation is not efficient

[canepat]

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.

Bytes sign(ByteView data_hash, ByteView private_key) {
    SecP256K1Context ctx{/* allow_verify = */ false, /* allow_sign = */ true};
    secp256k1_ecdsa_signature signature;
    bool ok = ctx.sign(&signature, data_hash, private_key);
    if (!ok) {
        throw std::runtime_error("ecdsa_signature::sign failed");
    }

    return ctx.serialize_signature(&signature);
}

[chfast]

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:

1. make SecP256K1Context singleton,
2. create global SecP256K1Context instance,
3. create SecP256K1Context in sentry main() and pass everywhere as an argument.

[chfast]

Option 2 is probably the quickest. I can make a PR if you this is good start.

[canepat]

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?

[battlmonstr]

@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.

[chfast]

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".

[battlmonstr]

@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.