Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
crypto, refactor: add new KeyPair class #30051
crypto, refactor: add new KeyPair class #30051
Changes from all commits
f14900b
5d507a0
c39fd39
cebb08b
72a5822
ec973dd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
nice adding ec973dd
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.
nit: AFAIR, for a long time we preferred to store the return value in a
ret
boolean and assert only on that (see e.g.$ git grep ret.*secp256k1
vs$ git grep assert.*secp256k1
), but not sure if we still have a developer guideline like "don't use assert with side-effects" in place (I think we once had, and removed it, so this way seems to be fine.)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.
We've had something like that before: #30051 (comment)
But it was ambiguous, as to whether it can fail or not during execution as we can see here:
https://github.com/hebasto/bitcoin/blob/master/src/pubkey.cpp#L345-L350 or https://github.com/hebasto/bitcoin/blob/master/src/secp256k1/src/secp256k1.c#L685
But @josibake argued that they cannot fail at that point - if they do it's a bug, but we didn't want to ignore the return values either. So we ended up asserting, like we did in GetPubKey already.
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.
nit:
secp256k1_context_create
docs saysThere 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.
Ah, nice! Good catch. Will change if I end up retouching.
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.
If you do that, please change other usages as well
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.
Yeah, did a quick grep and it looks there is at least one other place in the fuzz tests where
_SIGN
is used, so this one is probably best as a follow up PR to replace all instances with_NONE
in the codebase and perhaps add a mention to the developer notes.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.
nit, if you have to retouch:
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.
I think this effectively only tests that given a certain private key, the same
XOnlyPubKey
objects result with both used methods (once via the secp256k1 keypair functions, once with.GetPubKey()
). So strictly speaking computing and comparing the tweak hashes on top of that isn't needed and could be removed, but no strong feelings about that, as it also doesn't hurt. (Seems like this was already discussed in #30051 (comment) ff., so feel free to ignore).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.
This was the result of a change that was reverted since, where it was important to test the exact before/after structure.