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
Add EIP: Precompiled for secp256r1 Curve Support #7212
Add EIP: Precompiled for secp256r1 Curve Support #7212
Changes from 8 commits
a25412f
b3f1972
fb90cf4
6c775ee
077d635
1d87d6e
fbaaa02
1d9a85c
823c8ac
d5fb2f5
a265902
15f7137
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.
It would be nice to clarify exactly what we gain from requiring
x
/y
as inputs. It's not clear to me from the explanation here.And requiring them as inputs requires substantial added call data.
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 I understand correctly,
v
is a sign bitIt's easy to recover
v
-- you try the two possible values, and see which one returns your public key.So the comparison is something like
v, r, s
stylePros
v
parameter is mostly 0 bytes and therefore cheaper.Cons
v
. This is a bit awkward. Instead of just calling (hardware signing module), it has to call (hardware signing module) > (attempt verification with + v) > (attempt verification with - v). I am not a cryptographer... maybe there's a better way.v, r, x, y
stylePros
v
, they just pass a pubkey (x,y) directly from the hardware enclave.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.
FYI ERC-2098 by @ricmoo for might be relevant in the same spirit where user / library recovers
v
for secp256k1.This is provided to you as a reference and you can gauge the preference by usage / adoption for designing the precompiles
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.
Thanks @dcposch for the explanation of the logic.
Yes, the mechanisms using the curve are returning only
r
ands
, so the verification is possible with inputting public key coordinatesx
andy
.We need
v
value to recover public key without thex
andy
coordinates. Thev
value can be found by above-mentioned methods, but let me explain my design choice:ecrecover
, we can reach the public address of the EOA accounts, so it can be directly used in the smart contracts. Unlikely, recovering thesecp256r1
public key does not match any default stored types, and we still need to store the account public key.v
value in the implementation part of the signature creates complexity on the application side. So, I didn't want to bring this complexity for the applications. Still, I would love to reassess and edit the EIP to implement recovery after discussion.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.
IMO it's uneccessary to put system-specific benchmark results in the EIP itself.
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.
Hi, thanks for your comments! How do the benchmarks be displayed? I think that it's important to propose gas costs for the precompiled contract. I thought if I am comparing two results to propose gas cost in the same system would be beneficial. Should I include more systems benchmark results or completely remove it?
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.
IMHO The benchmark is being supplied as assets, I personally think it's a good supporting evidence for rationale like the gas cost choice. Hence I think it's good to be there. My two cents. Others might disagree.