API cleanup with respect to contexts

[real-or-random]

• Documents the changes made to contexts that came with the transition to only static tables
• Rename the static context
• Expose the self tests for users of the static context

These are user-visible changes, so this is also a testbed for the changelog. I refrained from linking to the PR as the template proposed for multiple reasons:

• Including the PR title is not in line with the suggestions in https://keepachangelog.com/en/1.0.0/ What they do instead is really just describe what parts in the API have changed, and I tend to agree that this is better for the user. The PR title can be rather technical or saying not too much, and a PR can correspond to multiple changes (for example this PR).
• I'd be open to Include a link to the PR but I feel it's a little bit annoying because you get the PR number only after creating the PR... Also I think the changelog is addressed to our users, not our devs. It should be just a log of user-visible changes (syntax and visible API behavior), so we anyway shouldn't include every PR.

A normal strategy would be to create a release first, then make the API changes, but in this case, this would be somewhat confusing. The important thing here is the changes made to context flags and these are already for quite some time, so it makes sense to document these even for the initial release.

[apoelstra]

Thanks for this! It's super helpful that all the recent context changes are now visible in the docs.

[real-or-random]

Now that you mention the tests, I realize that many of the tests still use a lot of variants of the contexts, though we could simply check if all (non-static) contexts are equal (using memcpy). Let me add this.

But what about all the test code? What we currently have is not wrong, it's just not efficient because we may run the same tests multiple tests. Should I simplify it? If yes, I don't mind doing it here but it could also be done in a separate PR.

[sipa]

I find the explanation of context flags a bit confusing now.

The concept of "initialized for signing" still exists, and is referred to in API calls and the lack of it is mentioned for the static contexts, but at the same time, all flags that indicate verification/signing/... have lost their meaning. So what does "initialized for signing mean"? Is it just the static context that doesn't have it, while all others do? Or is it somehow still linked to the flags passed at creation time, despite those being deprecated?

(just playing the devil's advocate here, trying to show the reasoning someone going through the .h files may follow)

[real-or-random]

@sipa I agree. It will be probably confusing in particular for new users who don't know the history. Do you have a suggestion? Call them "full context" and "static context" or similar?

[sipa]

@real-or-random Actually, in what way is the static context not usable for signing operations?

[real-or-random]

@real-or-random Actually, in what way is the static context not usable for signing operations?

Oh, we could make it usable for signing. (At the moment the relevant parts of the object are simply not initialized but that will be easy to fix, in particular now that https://github.com/bitcoin-core/secp256k1/pull/1120 has been merged.)

The actual question is whether we would like to make it usable for signing. The static context cannot be randomized, so you'll never get protection by random blinding. I think allowing signing would not be unreasonable (and simplify the API and the docs a lot). But I'm not sure if we want to encourage this mode of usage. I tend to the opposite: Now that dynamic context creation is fast and doesn't need a lot of memory, we should encourage people to use that route.

With the current API, you could argue that anyway a lot of users are probably never adding randomness, so it won't make a lot of difference. But I think when evaluating this, we should keep in mind that a future context API will hopefully make it easier to randomize, e.g., context creation should directly accept a seed, and some of the (standard) signing API calls should rerandomize the context etc...

(This was one of the questions in https://github.com/bitcoin-core/secp256k1/pull/988#issuecomment-938650194, and there I decided to not change the API because we could always do it later.)

[sipa]

@real-or-random Hard question. On the one hand, yes, I think we do want to encourage using properly-(re)randomized signing contexts to get maximal blinding protection. But on the other hand, I also don't think we should go as far as saying that one cannot use the signing API without forcing randomization; I could imagine platforms where randomness may actually be unavailable at signing time.

[apoelstra]

In particular, in rust-wasm it is very difficult to get randomness (there are definitely ways to get randomness in JS but for some reason these are not exposed in WASM and the standard Rust randomness crate will not work).

[real-or-random]

On the one hand, yes, I think we do want to encourage using properly-(re)randomized signing contexts to get maximal blinding protection. But on the other hand, I also don't think we should go as far as saying that one cannot use the signing API without forcing randomization;

Okay, then the current API does exactly this. You are not forced to call context_randomize but you don't get a convenience bonus from the static context.

edit: I can try to rewrite this with better names and make sure the static context is a special case. Then some of the function descriptions would they're not compatible with the static context; I'm sure there's a brief way to say this.

[sipa]

I feel it would be simpler to do make the static context usable for signing functions, and then remove the "initialized for signing" and "initialized for verification" conditions in the API documentation everywhere.

Even if we don't do that, the notion of "initialized for verification" is not defined anywhere anymore, and not relevant, so that seems like it should certainly disappear.

[jonasnick]

I don't think "initialized for signing" is very confusing (it would be clearer if the flag was explicitly mentioned). The flag is not deprecated after all. Also the static context explicitly mentions that it's not initialized for signing. However, if it's not too complicated to implement, sipa's approach (which involves deprecating the signing flag?) seems to be a significant simplification of the initial release API.

[real-or-random]

Even if we don't do that, the notion of "initialized for verification" is not defined anywhere anymore, and not relevant, so that seems like it should certainly disappear.

The PR (even in its current form) already removes all "initialized for verification". Or are you saying I missed some?

However, if it's not too complicated to implement, sipa's approach (which involves deprecating the signing flag?) seems to be a significant simplification of the initial release API.

Making the static context usable for signing will be rather simply to implement under the hood. And I don't think this change to the static context would affect the context creation flags. Even with the current PR, the flags are simply ignored. As all contexts behave like the SIGN flag did in the past, I'd still say that the right way to document this is to deprecate all flags expect SIGN.

@jonasnick What's your opinion on encouraging randomization vs keeping the API simple?

[sipa]

The PR (even in its current form) already removes all "initialized for verification". Or are you saying I missed some?

I'm confused why I said that. I must have been looking at the wrong branch... Please disregard.

[jonasnick]

@real-or-random

As all contexts behave like the SIGN flag did in the past, I'd still say that the right way to document this is to deprecate all flags expect SIGN.

When I pondered whether sipa's approach would involve deprecating SIGN I had thought that the SIGN flag is still necessary for signing. But that's not the case. So there's only a minor reason to deprecate SIGN:

With your current PR, the only reason the SIGN flag exists is to distinguish between static and dynamic contexts as far as I can see. If we allow signing with static contexts, the SIGN flag lost its meaning entirely since all contexts are SIGN contexts. Therefore, we could deprecate all flags except NONE, which would be the default flag that we'd most likely choose if we started with a clean slate. But effectively it's just a different name.

What's your opinion on encouraging randomization vs keeping the API simple?

I'm slightly in favor of allowing secret key operations with the static context, because it allows removing the "initialized for signing" terminology which is somewhat confusing.

[sipa]

With your current PR, the only reason the SIGN flag exists is to distinguish between static and dynamic contexts as far as I can see.

Not even. There is no way to construct a context which doesn't have signing capability. It's just that there is one special context precreated which cannot. That doesn't need a notion of a signing flag, just an exception ("this context can't be used for signing operations"). Referring to that as "initialized for signing" is confusing to me, as it makes it sound like there is some step ("initialization") the user has to do which is missing. It's more a capability than initialization.

[real-or-random]

I'm still slightly in favor of the more conservative option that keeps the semantics as is but I think it could be convinced otherwise.

Referring to that as "initialized for signing" is confusing to me, as it makes it sound like there is some step ("initialization") the user has to do which is missing. It's more a capability than initialization.

Indeed. I just talked to Jonas somewhere else and I decided to give rewriting the docs a try. So I'll update the PR to avoid the "init" terminology and then we can see if we like it or if we want to get rid of the distinction between static/dynamic entirely.

[real-or-random]

I added some WIP commits to demonstrate how another version would look like. This now tries to avoid the "historical" terminology entirely. As a consequence, this also introduces the new flag SECP256K1_CONTEXT_DEFAULT (name inspired by https://github.com/bitcoin-core/secp256k1/issues/559 and up to bikeshedding of course).

WIP because

• [x] the actual flags are not yet updated
• [x] this does not touch all occurrences of "initialized for signing"
• [x] and this will need some paragraph formatting.
• [x] examples need to be updated

I can take care of all of this once know which version we want to have.

Please have a look and tell me if you prefer this new version (maybe with some changes) or instead allowing secret key ops with the static context. I still lean towards disallowing secret key ops with the static context because allowing it would create an incentive to skip randomization:

On the one hand, yes, I think we do want to encourage using properly-(re)randomized signing contexts to get maximal blinding protection. But on the other hand, I also don't think we should go as far as saying that one cannot use the signing API without forcing randomization;

Okay, then the current API does exactly this. You are not forced to call context_randomize but you don't get a convenience bonus from the static context.

[real-or-random]

 *  Args:    ctx:       pointer to a context object, configured with flag SECP256K1_CONTEXT_FOO (not secp256k1_context_static).

Yeah, that's roughly what I had in mind. Maybe s/configured/created/

[real-or-random]

Note: This approach got Concept ACKed at the IRC meeting two weeks ago, so it's currently up to me to unWIP the PR.

[real-or-random]

Ready for review.

This probably could have been split into multiple PRs in hindsight but I think it's still manageable.

One open question (that can be resolved after this PR) is whether and how we want to tidy the tests... At the moment they still use deprecated flags like SECP256K1_CONTEXT_NONE.

[real-or-random]

Here's something that came up when reviewing #1168, where @jonasnick changes the tests to use secp256k1_context_create(SECP256K1_CONTEXT_DECLASSIFY)

I was wondering if this should be SECP256K1_CONTEXT_DEFAULT | SECP256K1_CONTEXT_DECLASSIFY? I mean... it won't matter for the tests. But assume we'll add a new flag SECP256K1_CONTEXT_FANCY_FEATURE?

So far, the flags have really been bitflags, so that you can bitwise-OR them. And so far, SECP256K1_CONTEXT_NONE has been the neutral element in this algebraic group.

After this PR, if you want fancy features, should you ask for SECP256K1_CONTEXT_DEFAULT | SECP256K1_CONTEXT_FANCY_FEATURE? Or just SECP256K1_CONTEXT_FANCY_FEATURE (which has a different value since SECP256K1_CONTEXT_DEFAULT != SECP256K1_FLAGS_TYPE_CONTEXT -- the "SIGN bit" (1 << 9) is set).

I think in the end it won't really matter -- as long as we promise to ignore the value of the "SIGN bit".

But I wonder:

• Should DEFAULT in fact be an alias to (the existing value of) NONE? I didn't do that due to ABI compatibility: If the user has a new header but an old binary, then they'll get an actual NONE context... I'm not sure if this matters at all but making DEFAULT an alias to SIGN seems slightly cleaner.
• Does "DEFAULT" make sufficiently clear that it is supposed to be the neutral element of a group (which it is if we'll ignore the value of the SIGN bit)? Maybe NONE expressed this better, and now I realize that @jonasnick suggested this above: "Therefore, we could deprecate all flags except NONE, which would be the default flag that we'd most likely choose if we started with a clean slate. But effectively it's just a different name."

So now that I've been writing this, I think slightly cleaner solution will be to

• set the SIGN bit with value 1 as part of SECP256K1_FLAGS_TYPE_CONTEXT instead (just to ensure the mentioned ABI compatibility)
• call the one non-deprecated flag NONE (instead of DEFAULT) and set no "feature" bits at all.

This is indeed a new clean slate. And it reserves the DEFAULT name flag for future use. (Maybe in the future, we want introduce a feature that should be enabled by "default", and we could rather use "DEFAULT" to mean "give me whatever the maintainers think should be the default" instead of "enable no features".)

Opinions? I'll think I'll make that change this if noone objects.

[jonasnick]

I'd be in favor of calling the non-deprecated flag NONE and keep it being the neutral element wrt | as you're suggesting. When I wrote the code you reference I had to stop for a moment and asked myself the exact question that you mentioned.

[real-or-random]

Ok, I changed the _DEFAULT back to _NONE. But after talking to @jonasnick , I realized that I don't need to fiddle with SECP256K1_FLAGS_TYPE_CONTEXT. The behavior before this PR was perfectly fine, it's simply not properly documented. So the only thing that this PR now does (with respect to context flags) is to fix the docs.

[sipa]

utACK 4386a2306c2b8cf9ad3040d8010e4295f6f01490

There's a typo left, but if this is going to need a follow-up anyway, that can be done then too.