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

Add serde impl for KeyPair #379

Merged
merged 1 commit into from
Jan 19, 2022

Conversation

elsirion
Copy link
Contributor

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.

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.
Copy link
Member

@apoelstra apoelstra left a 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.

Copy link
Member

@elichai elichai left a 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

@apoelstra
Copy link
Member

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..

@apoelstra apoelstra merged commit 5445bc3 into rust-bitcoin:master Jan 19, 2022
@real-or-random
Copy link
Collaborator

real-or-random commented Jan 19, 2022

we can remove that feature gate next time we bump upstream, so even the precomp has signing context in it AFAIU

The no_precomp context is not a signing context on upstream master. The reason is that you can't possibly randomize it, so it's always "less secure". Making this a "less secure" signing context is one of the things that needs to be talked about in upstream. So far, we refrained from doing so simply because this is an API change that can be talked about separately.

In fact, no_precomp (plus its clones) is the only remaining way to obtain a non-signing context.

I edited the upstream issue bitcoin-core/secp256k1#1065 to include this as a task.

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 this pull request may close these issues.

None yet

4 participants