-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Conversation
File
|
The commit 1d87d6e (as a parent of ec7ea83) contains errors. |
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.
LGTM,
The proposal provided reasonable motivation and clear specs about the input and output and tests to validate, gave gas cost and other spec decision in the rationale. Editorially, I think this PR is good enough to merge as a Draft.
Leaving technical feedback, such as the design choice of verifying only v
component, to the peer reviews by the client developers.
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 left a few comments.
- [P256Verify Benchmark Test Results](../assets/eip-7212/p256Verify_benchmark_test) | ||
- [Ecrecover Benchmark Test Results](../assets/eip-7212/ecrecover_benchmark_test) | ||
|
||
``` |
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.
|
||
## Rationale | ||
|
||
The "secp256r1" elliptic curve signatures consists of `v`, `r`, `s` components. Even if recovering the public key on the curve is possible, most of the applications are not generating `v` component of the signature and it causes an uncertainty of the result values. However, the signatures can be verified with only `r` - `s` values. In order to provide an exact and more compatible method, verification is preferred over recovery to propose in a precompiled. |
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 bit- the P256 precompile is intended to support hardware signatures
- most of the existing hardware signature mechanisms (Secure Enclave, Webauthn, etc) return only the (r,s) signature
It'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
style
Pros
- Matches ecrecover.
- Less calldata. On the surface, 1 word = 32 bytes less, but actual savings should be a bit more, since the
v
parameter is mostly 0 bytes and therefore cheaper.
Cons
- Users / libraries have to recover
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
style
Pros
- Better matches the hardware interface. Users/libraries don't have to recover
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.
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
and s
, so the verification is possible with inputting public key coordinates x
and y
.
We need v
value to recover public key without the x
and y
coordinates. The v
value can be found by above-mentioned methods, but let me explain my design choice:
- While we are making recovery with the
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. - Having to find the
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.
-1
|
@paulmillr thanks for the feedback. |
Hi, thanks for you comment! Moved it to the EM forum for discussion: https://ethereum-magicians.org/t/eip-7212-precompiled-for-secp256r1-curve-support/14789/5?u=ulerdogan |
Co-authored-by: Sam Wilson <[email protected]>
* Add EIP: Precompiled for secp256r1 Curve Support * Update EIP: Fix the date * Update: Update with the numbers and links * Fix: Lint errors fixed * Fix: Lint errors v2 fixed * Fix: Disallowed links are removed * Update: Fix the specification misrepresentations * Update: Change license link Co-authored-by: Sam Wilson <[email protected]> --------- Co-authored-by: Sam Wilson <[email protected]>
* Add EIP: Precompiled for secp256r1 Curve Support * Update EIP: Fix the date * Update: Update with the numbers and links * Fix: Lint errors fixed * Fix: Lint errors v2 fixed * Fix: Disallowed links are removed * Update: Fix the specification misrepresentations * Update: Change license link Co-authored-by: Sam Wilson <[email protected]> --------- Co-authored-by: Sam Wilson <[email protected]>
* Add EIP: Precompiled for secp256r1 Curve Support * Update EIP: Fix the date * Update: Update with the numbers and links * Fix: Lint errors fixed * Fix: Lint errors v2 fixed * Fix: Disallowed links are removed * Update: Fix the specification misrepresentations * Update: Change license link Co-authored-by: Sam Wilson <[email protected]> --------- Co-authored-by: Sam Wilson <[email protected]>
Add EIP: Precompiled for secp256r1 Curve Support
The post is about a new EIP which proposes the addition of a new precompiled contract to the EVM that allows signature verifications in the “secp256r1” elliptic curve by given parameters of message hash, r - s components of the signature, and x - y coordinates of the public key.