-
Notifications
You must be signed in to change notification settings - Fork 264
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
Add serde impl for KeyPair #379
Add serde impl for KeyPair #379
Conversation
40a7c43
to
c16582e
Compare
The impl is added as a module instead of being a direct implementation since it uses the global context and users should be aware that.
c16582e
to
1877e4d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 1877e4d
It's a little unfortunate that serde depends on the global context, and also that deserialization is so very slow (involving a const-time EC mult), but I think this is alright. Or at least, hard to do better.
Would like at least one more ACK before mergign.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 1877e4d
we can remove that feature gate next time we bump upstream, so even the precomp has signing context in it AFAIU
SGTM -- though bear in mind that it's not guaranteed for upstream to get rid of the context object for signing (or even make it "free") because of randomization. Though it may be cheap enough, next to the ec-mult, that we can just generate one inside the serde code.. |
The In fact, I edited the upstream issue bitcoin-core/secp256k1#1065 to include this as a task. |
The impl is added as a module instead of being a direct implementation since it uses the global context and users should be aware that.