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 EIP: Precompiled for secp256r1 Curve Support #7212

Merged
merged 12 commits into from
Jun 29, 2023

Conversation

ulerdogan
Copy link
Contributor

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.

@ulerdogan ulerdogan requested a review from eth-bot as a code owner June 22, 2023 12:35
@github-actions github-actions bot added c-new Creates a brand new proposal s-draft This EIP is a Draft t-core labels Jun 22, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented Jun 22, 2023

File EIPS/eip-7212.md

Requires 1 more reviewers from @axic, @g11tech, @gcolvin, @lightclient, @Pandapip1, @SamWilsn

@eth-bot eth-bot added e-consensus Waiting on editor consensus e-review Waiting on editor to review labels Jun 22, 2023
@github-actions github-actions bot added the w-ci Waiting on CI to pass label Jun 22, 2023
@github-actions github-actions bot added w-ci Waiting on CI to pass and removed w-ci Waiting on CI to pass labels Jun 22, 2023
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Jun 22, 2023
@github-actions
Copy link

The commit 1d87d6e (as a parent of ec7ea83) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot added the w-ci Waiting on CI to pass label Jun 22, 2023
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Jun 22, 2023
Copy link
Contributor

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

Copy link
Contributor

@jwasinger jwasinger left a 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)

```
Copy link
Contributor

@jwasinger jwasinger Jun 23, 2023

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

EIPS/eip-7212.md Outdated Show resolved Hide resolved
EIPS/eip-7212.md Outdated Show resolved Hide resolved

## 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.
Copy link
Contributor

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.

Copy link

@dcposch dcposch Jun 23, 2023

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.

Copy link
Contributor

@xinbenlv xinbenlv Jun 23, 2023

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

Copy link
Contributor Author

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

@paulmillr
Copy link
Contributor

-1

  1. 256r1 is usually more vulnerable to timing attacks than stuff like 25519.
  2. 256r1 is not even recommended by nist at this point, 384r1 is.
  3. There are some rumors with regards to general security of r1 curves, it's unclear.
  4. Adding a new elliptic curve impl into ALL execution layer clients is not a trivial task. I don't think the feature is too useful for this.

@xinbenlv
Copy link
Contributor

@paulmillr thanks for the feedback.
Editor here: for any technical feedback like those you gave, they are recommended to be put in https://ethereum-magicians.org/t/eip-7212-precompiled-for-secp256r1-curve-support/14789 so when this PR is merged, the discussion can continue and also has a higher chance to be seen by other peer reviewers.

@ulerdogan
Copy link
Contributor Author

**paulmillr ** commented

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

EIPS/eip-7212.md Outdated Show resolved Hide resolved
SamWilsn
SamWilsn previously approved these changes Jun 27, 2023
@ulerdogan ulerdogan requested a review from SamWilsn June 27, 2023 17:46
@SamWilsn SamWilsn merged commit 64b7d76 into ethereum:master Jun 29, 2023
streamnft-tech pushed a commit to streamnft-tech/EIPs that referenced this pull request Oct 27, 2023
* 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]>
RaphaelHardFork pushed a commit to RaphaelHardFork/EIPs that referenced this pull request Jan 30, 2024
* 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]>
GAEAlimited pushed a commit to GAEAlimited/EIPs that referenced this pull request Jun 19, 2024
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-new Creates a brand new proposal e-consensus Waiting on editor consensus e-review Waiting on editor to review s-draft This EIP is a Draft t-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants