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

Reducing context arguments in taproot-related functions #343

Open
dr-orlovsky opened this issue Nov 25, 2021 · 10 comments
Open

Reducing context arguments in taproot-related functions #343

dr-orlovsky opened this issue Nov 25, 2021 · 10 comments

Comments

@dr-orlovsky
Copy link
Contributor

Introduction of Taproot in rust-bitcoin has polluted a lot of existing APIs with a need to provide Secp context objects all around, mostly because of key tweaking. Upstream to rust-bitcoin it becomes even more painful; everything gets contaminated with these context generic parameters.

I am willing to work on a PR here to get rid of these context parameters wherever possible - first for all key tweaking functions and xonlypubley from seckey generation. But I do not understand the logic where we need verify, sign or static context. Where I get get an insight on this matter?

@real-or-random
Copy link
Collaborator

real-or-random commented Nov 26, 2021

I am willing to work on a PR here to get rid of these context parameters wherever possible - first for all key tweaking functions and xonlypubley from seckey generation. But I do not understand the logic where we need verify, sign or static context. Where I get get an insight on this matter?

Indeed, the context names are misleading.

So "verify" actually should be called "ecmult", should be called "ecmult_gen". The short explanation is that "ecmult" is for EC multiplications with only public data (e.g., signature verification) and the latter is for EC multiplications with secret data (e.g., signing, key generation). There's one exception: ECDH does not need a special context. For a longer explanation, see https://bitcoin.stackexchange.com/a/103061.

The static context is neither "verify" nor "sign", so you can use it only for operations which do not need EC mults (or do only ECDH).

edit: When in doubt, the API docs of secp256k1 should always say what kind of context is required.


What I described so far was the state of affairs for years. Until recently, when bitcoin-core/secp256k1#956 made verify contexts superficial. In fact now every context is implicitly a verify context, no matter if it has been created as such or not. So the API docs haven't changed so far and they still say you need a verify context for some operations, so we currently do not guarantee that behavior. But this will probably change in the future. If there's an interested in simplifying the API here, we could of course try to come to a decision upstream and document the changes in the API docs.

Further down the road, we'll probably have an entirely new context API (bitcoin-core/secp256k1#780) but I think it's too early to care about this right now.

@dr-orlovsky
Copy link
Contributor Author

Thank you for the very detailed explanations. Yeah, the naming is not obvious.

Do I get it right that before upstream changes nothing can be done here at rust level?

@real-or-random
Copy link
Collaborator

Do I get it right that before upstream changes nothing can be done here at rust level?

I don't see a reason why upstream would not make the changes but I think we should at least bring it up first.

@apoelstra
Copy link
Member

I think we could update our copy of upstream and remove the context objects from the taproot functionality, since it is very new anyway.

I would also like to remove the contexts from as many other things as possible, but that will be an irritating breaking change so we want to do it in its own major rev.

@dr-orlovsky
Copy link
Contributor Author

Looks like with the last secp256k1 release(s) this issue has been solved?

@apoelstra
Copy link
Member

I don't think so -- we didn't reflect all the upstream context changes in rust-secp (in upstream I guess we basically never need a context object, but in rust-secp you still do) because we weren't sure how we wanted to expose things in rust-secp, and didn't want to slow down our Taproot releases with additional API flux.

@real-or-random
Copy link
Collaborator

@dr-orlovsky @apoelstra Nothing substantial has changed in the upstream API since I wrote #343 (comment).

I guess you're referring to bitcoin-core/secp256k1#988. That change made all contexts signing contexts (except the static no_precomp context), no matter which flags you pass when creating the context. But contexts still exist as an explicit data structure in upstream secp256k1, e.g., to hold data related to blinding when performing ecmult_gen computations. That means we'll also still need them here as explicit data structure.

@apoelstra
Copy link
Member

@real-or-random right, I misspoke. What I meant by "upstream API changes" is that in rust-secp, we could elide all the context objects in our API and simply create zero-cost ones for upstream, internal to our code. The "API change" is that this is now zero-cost.

@elichai
Copy link
Member

elichai commented Jan 13, 2022

@dr-orlovsky @apoelstra Nothing substantial has changed in the upstream API since I wrote #343 (comment).

I guess you're referring to bitcoin-core/secp256k1#988. That change made all contexts signing contexts (except the static no_precomp context), no matter which flags you pass when creating the context. But contexts still exist as an explicit data structure in upstream secp256k1, e.g., to hold data related to blinding when performing ecmult_gen computations. That means we'll also still need them here as explicit data structure.

I think that now more of parts of the API can accept the noprecomp static context, no?

@real-or-random
Copy link
Collaborator

I think that now more of parts of the API can accept the noprecomp static context, no?

Yeah, the rust-secp methods that need a verification context but no signing context could now be changed not to take any context (and use the noprecomp context under the hood). I'm just not sure if it's a good idea to make this change right now, at least as long as upstream still has verification contexts. Maybe we'll get rid of them upstream but maybe not.

@real-or-random right, I misspoke. What I meant by "upstream API changes" is that in rust-secp, we could elide all the context objects in our API and simply create zero-cost ones for upstream, internal to our code. The "API change" is that this is now zero-cost.

Ok I see. That's a nice goal indeed. We can do a lot more with Rust built-ins than with C89... But we really need to look at the details. Context creation is fast now but it's not zero cost: It needs randomness and runs a PRG to init the blinding data and it performs a self-test. If we can handle this cleanly in the internal library code, including proper mutual exclusion when trying to rerandomize the context, that will be nice. Note though that in the future, signing itself may also update the context, and it's not really clear what contexts will evolve to in upstream (and when changes will happen), see mostly the recent discussion in bitcoin-core/secp256k1#780.

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

No branches or pull requests

4 participants