Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide more ergonomic APIs using global-context #330

Closed
thomaseizinger opened this issue Sep 15, 2021 · 9 comments · Fixed by #392
Closed

Provide more ergonomic APIs using global-context #330

thomaseizinger opened this issue Sep 15, 2021 · 9 comments · Fixed by #392

Comments

@thomaseizinger
Copy link
Contributor

When the global-context feature is enabled, we could provide more ergonomic APIs:

impl SecretKey {
	pub fn sign(&self, msg: Message) -> Signature;
}

impl Signature {
	pub fn verify(&self, key: PublicKey, msg: Message) -> Result<(), Error>;
}

impl RecoverableSignature {
	pub fn recover(&self, msg: &Message) -> Result<PublicKey, Error>;
}

impl KeyPair {
	pub fn from_secret_key(&self, key &SecretKey) -> Result<Self, Error>;
}

We've been working with rust-secp256k1 directly a lot lately and found ourselves defaulting to the global context in practically all cases.

@real-or-random
Copy link
Collaborator

We'll most probably have static precomputation tables soon in upstream. And then the contexts upstream would need a redesign anyway because the only function that remains is randomization. In a sense, global-context is then the only thing allowed be upstream anyway.

I'm not sure if that means we should wait in this PR. Probably not. We could offer the API here, and if upstream moves towards the mentioned direction, we could simply get rid of the global-context feature.

@elichai
Copy link
Member

elichai commented Sep 15, 2021

@real-or-random I don't think you're correct. the context also includes the callback, so I don't think the upstream API will change much.

On the other hand, we provide our own callbacks, so we can use a static global context in everything except for signing (due to the randomization).

(Although I'd argue that upstream should remove the ability to change the callbacks in runtime but that's a discussion for an upstream issue)

@real-or-random
Copy link
Collaborator

(Although I'd argue that upstream should remove the ability to change the callbacks in runtime but that's a discussion for an upstream issue)

Indeed, this will need to be discussed. I'd probably also be in favor of this.

On the other hand, we provide our own callbacks, so we can use a static global context in everything except for signing (due to the randomization).

Yeah, and here we can randomize the global context, too (as we already do now!)

@thomaseizinger
Copy link
Contributor Author

I don't follow any upstream discussions but from what I understand, the proposed APIs would be welcomed because that is what upstream might move to anyway?

How would we support the no-std usecase then?

@real-or-random
Copy link
Collaborator

real-or-random commented Sep 16, 2021

I don't follow any upstream discussions but from what I understand, the proposed APIs would be welcomed because that is what upstream might move to anyway?

Indeed.

How would we support the no-std usecase then?

Oh you mean we currently have:

global-context = ["std", "rand-std", "global-context-less-secure"]
global-context-less-secure = []

And it may not be a good idea to remove global-context and global-context-less-secure because then there's no loud "less secure" that the user needs to select but with no-std they're still less secure?

Yeah that's an issue, we need to think about this...

edit: Okay, here's a very vague idea: Couldn't we make the context type itself loud? So there will be Context and ContextLessSecure that we could make global depending on std. And when you call randomize on ContextLessSecure, it gives you back a Context. (I haven't thought this through, this may be a bad idea or not even work at all, etc.)

@thomaseizinger
Copy link
Contributor Author

How would we support the no-std usecase then?

Oh you mean we currently have:

No, I meant how will upstream support a no-std usecase with memory constraints that are tighter than what the global-context uses by default?

Correct me if I am wrong but isn't the whole idea of having the context explicit that people can chose how to initialize it, thus providing flexility for no-std, no-allocator, etc? If upstream already provides APIs that imply the global context, this would become impossible?

And it may not be a good idea to remove global-context and global-context-less-secure because then there's no loud "less secure" that the user needs to select but with no-std they're still less secure?

Yeah that's an issue, we need to think about this...

edit: Okay, here's a very vague idea: Couldn't we make the context type itself loud? So there will be Context and ContextLessSecure that we could make global depending on std. And when you call randomize on ContextLessSecure, it gives you back a Context. (I haven't thought this through, this may be a bad idea or not even work at all, etc.)

Generally yes. Cargo's features play well with adding / removing code. Changing functionality of existing code (like randomizing or not randomizing) may lead to weird bugs. So ideally, there are two global contexts and they are activated by different feature flags.

@elichai
Copy link
Member

elichai commented Sep 16, 2021

Correct me if I am wrong but isn't the whole idea of having the context explicit that people can chose how to initialize it, thus providing flexility for no-std, no-allocator, etc? If upstream already provides APIs that imply the global context, this would become impossible?

The context will be static in the binary, and in a size that should work for almost all targets we know about.
I think upstream will end up providing 2 tables 1MB and 100Kb ones, and those should cover both ends of the spectrum (again remember these won't be stack allocated, but static in the binary, so should work for virtually any target).

If some user will actually present an even smaller target then we'll have to handle that, but I don't think that's probable.

as for fine details, secp256k1_context_no_precomp will probably be changed to something like secp256k1_context_no_randomization or something, that we will use in all non signing operations and remove the context need in those, and then we will provide a global context that is randomized once, or a user can create their own context that can be re-randomized whenever they want.

Maybe we could even hard code the size of that context such that it can be in stack instead of on the heap? (as it should be just a couple of function pointers and a small array)

@real-or-random
Copy link
Collaborator

Yes, the main point is that you can have this now in flash memory and not in RAM, and usually RAM is the issue here.

And if you really want to get rid of one of the two tables in flash, LTO should prune it (including the code that uses it). I'm not sure if this will work across C and Rust but if not, we can still provide no-sign or no-verify here to eliminate the parts you don't need.

@thomaseizinger
Copy link
Contributor Author

Okay, interesting! Thanks for the insight!

With this outlook, I agree with @real-or-random that going for the above suggestion APIs is the way to go. Once upstream lands the changes to context, we can simply drop the global-context feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants