Add an optional global, static context

[sgeisler]

Add an optional global, static context to avoid creating new ones over and over again when there's no option to pass a context. According to my understanding the context is immutable, doesn't do any locking internally and is basically just a bunch of lookup-tables.

As this might not be useful for everyone, introduces a dependency and would bump the minimum rust version to 1.27.2 I hid the functionality behind the new global-context feature. While these limitations are a bit annoying I still think this crate is the right place for something like that (instead of a bunch of other crates doing the same, leading to a bunch of redundant, static contexts).

[apoelstra]

I wonder if it's possible to use the precomp context from the C code so we don't even need a lazy_static. May not be worth the added complexity.

Concept ack since it's feature-gated.

[elichai]

I wonder if it's possible to use the precomp context from the C code so we don't even need a lazy_static. May not be worth the added complexity.

Concept ack since it's feature-gated.

FWIW We could easily implement something like this ourself if we want

pub struct SECP256K1 {
    __private: (),
}
pub static SECP256K1: SECP256K1 = SECP256K1 { __private: () };

impl Deref for SECP256K1 {
    type Target = Secp256k1;

    fn deref(&self) -> &Self::Target {
        static ONCE: Once = Once::new();
        static mut CONTEXT: Option<Secp256k1> = None;
        ONCE.call_once(|| unsafe {
            CONTEXT = Some(Secp256k1::new());
        });
        unsafe { CONTEXT.as_ref().unwrap() }
    }
}

[sgeisler]

Yeah, but idk if copying unsafe code into the repo would be so much better. lazy-static is a well known crate (in rust lang nursery) and many applications may already have it as a dependency anyway. In that case us having the code copied in would increase review effort.

EDIT: I still have to look into Andrew's proposal. Haven't worked with plain secp256k1 so far.

[apoelstra]

Heh, that's pretty neat @elichai

I don't think the review burden of this copied code is very high @sgeisler ... it took me about a minute to read the 20 LOC snippet that Elichai posted and confirm its safety. I don't think I could even obtain the source of lazy_static in less time. The unsafe code looks like a pretty standard use of Once.

[real-or-random]

I don't mind lazy_static but @elichai's code is nice, too.

Using the precomputed context (= using hardcoded values computed at compile time) will also speed up context creation. But I tend to think this is orthogonal to this PR. In upstream, you can choose at compile them whether you want the precomputed context or not (faster context creation but larger binary). If we want the precomputed context here, this should be feature-gated, too. And depending on whether the feature is enabled, the code of this PR could give a either a precomputed context (if enabled) or a lazily-evaluated global context (as currently).

Does this make sense?

[elichai]

Using the precomputed context (= using hardcoded values computed at compile time) will also speed up context creation. But I tend to think this is orthogonal to this PR. In upstream, you can choose at compile them whether you want the precomputed context or not (faster context creation but larger binary). If we want the precomputed context here, this should be feature-gated, too. And depending on whether the feature is enabled, the code of this PR could give a either a precomputed context (if enabled) or a lazily-evaluated global context (as currently).

Does this make sense?

The nice thing about a precomputed one is allowing it without std.

[elichai]

Yeah, but idk if copying unsafe code into the repo would be so much better. lazy-static is a well known crate (in rust lang nursery) and many applications may already have it as a dependency anyway. In that case us having the code copied in would increase review effort.

I think the biggest difference between the example I've showed and lazy_static is that lazy_static also supports no-std and mine requires Once which is only available under std and not core (because std::once::Once uses thread::park while lazy_static uses spin::Once which is a spinlock)

But here Secp256k1::new() is already feature gated on std.

(I personally don't mind lazy_static, because it's in the rust-lang nursery and really commonly used, but it's always fun to implement things yourself :) )

[apoelstra]

I also don't mind lazy-static, FWIW. But I think if we can replace it with 18 LOC we ought to.

[real-or-random]

The nice thing about a precomputed one is allowing it without std.

I don't understand. Isn't it nice in either case?

[sgeisler]

I guess I should still feature gate it even when using @elichai's code?

[sgeisler]

I think I addressed your concerns 😃

[real-or-random]

Actually, does this now mean that the user has no change to call randomize on this? Do we need to wrap this in a RwLock?

edit: maybe this should then be discussed with #225 in mind.

[sgeisler]

How often do you actually need to re-randomize the context? Would it be sufficient to do so once at the beginning? Then it would be perfectly compatible with #225. If it's needed every n sign operations for fairly low values of n then maybe we want to think about a solution. But then I'd argue for using thread_local to avoid locking/synchronization headaches.

The secp256k1 docs don't specify that it has to be done repeadetly. This reads a bit like "wahatever" 🤷‍♂️

Regarding randomization, either do it once at creation time (in which case
you do not need any locking for the other calls), or use a read-write lock.

So I'm not sure if it's actually a problem.

[real-or-random]

Well yeah, sidechannel protection is always a little bit of "whatever". :D The idea is to run that once in the beginning. Doing it regularly won't hurt but I don't even immediately see how it would be better than running it only once.

So yes, then we can simply run this once when we create the context, and we should do this because otherwise the user won't be able to do it. But this means that we need std/rand as discussed in #225.

[sgeisler]

I mean, if #225 is implemented we wouldn't need to change anything in this PR. If std/rand is available it's done, otherwise it isn't. So maybe we want to block this on #225 to not encourage less-secure use in the meantime? Or would you propose to add opportunistic randomization here and later remove it once it's included in Secp256k1::new() (#225 looks like a easy to fix issue if everyone agrees)?

[real-or-random]

Hm, I fear it's more complicated due to the API stuff I discussed in #225.

The current solution here depends on std. #225 depends on std too but additionally on rand. So think the conservative thing is indeed to solve #225 first, even though this requires some thought and delays this PR here. Then I think we should this thing here dependent on rand. If you don't want rand or std, you need to deal with the context on your own. I believe that's acceptable.

[elichai]

I think conditioning this on std+rand and doing ctx.randomize(&mut rand::thread_rng()) is the best way to try and get this forward.
as the users of this global context probably don't mind binary size / memory usage too much.

[sgeisler]

Ok, I've added the randomization and will leave a comment in #225 once it is merged so it can be removed once #225 lands.

[elichai]

I'm fine with this like that.
This isn't optimal, but I think it's still better than what people do in practice (create multiple contexts, never randomize any of them, etc. )

[real-or-random]

We should keep this discussion in mind then. bitcoin-core/secp256k1#780 but I guess this should not stop us from making progress here.

[apoelstra]

I'm curious on a high level when "there is no option to pass a context". I have never written code that creates multiple contexts, nor have I ever wanted a global context, and I appreciate the documentation aspect of having functions explicitly call for context capabilities.

[apoelstra]

utACK, I like how this is implemented

[apoelstra]

I'm merging this because it's feature-gated and anyway it's frequently requested and I think it's useful for small one-off programs.