-
Notifications
You must be signed in to change notification settings - Fork 493
feat: EIP-7951 for ECDSA on P-256 curve #1649
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
Open
yelhousni
wants to merge
10
commits into
master
Choose a base branch
from
feat/eip-7951
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+6,328
−22
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
265d08f
feat: eip-7951 ecdsa p256
yelhousni 847ba74
docs: add comments P256Verify
yelhousni 3bc2819
test: add test vectors
yelhousni 3f6b94b
Merge branch 'master' into feat/eip-7951
ivokub 72ad80d
chore: use non-deprecated method for reading file
ivokub 5459acd
feat: implement unified versions of double, triple and doubleAndAdd
ivokub 465ba29
feat: use generic methods in scalarmul on switch
ivokub 7931f3e
refactor: direct implementation of ecdsa verification precompile
ivokub 2423768
test: implement utility functions for mocking arithmetization
ivokub 259be59
test: mock arithmetization and use full test vector set
ivokub File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| package evmprecompiles | ||
|
|
||
| import ( | ||
| "github.com/consensys/gnark/frontend" | ||
| "github.com/consensys/gnark/std/algebra/algopts" | ||
| "github.com/consensys/gnark/std/algebra/emulated/sw_emulated" | ||
| "github.com/consensys/gnark/std/math/emulated" | ||
| ) | ||
|
|
||
| // P256Verify implements [P256Verify] precompile contract at address 0x100. | ||
| // | ||
| // This circuit performs ECDSA signature verification over the secp256r1 | ||
| // elliptic curve (also known as P-256 or prime256v1). | ||
| // | ||
| // [P256Verify]: https://eips.ethereum.org/EIPS/eip-7951 | ||
| func P256Verify(api frontend.API, | ||
| msgHash *emulated.Element[emulated.P256Fr], | ||
| r, s *emulated.Element[emulated.P256Fr], | ||
| qx, qy *emulated.Element[emulated.P256Fp], | ||
| ) frontend.Variable { | ||
| // XXX: I think it is more efficient to just compute JointScalarMul here -- we don't need to do range checks. | ||
| // XXX: should we also explicitly check that the recovered point is not infinity? It is implicit anyway as we never receive `r==0` here (because of arithmetization checks), | ||
| // but I think we should at least mention it? | ||
| // XXX: and I think we also cannot directly check as the IsValid method checks that the r and r' are equal bitwise, but the EIP defines the check modulo n. | ||
|
|
||
| // Input validation: | ||
| // 1. input_length == 160 ==> checked by the arithmetization | ||
| // 2. 0 < r < n and 0 < s < n ==> checked by the arithmetization/ECDATA and enforced in `IsValid()` | ||
| // 3. 0 ≤ qx < p and 0 ≤ qy < p ==> checked by the arithmetization/ECDATA | ||
| // 4. (qx, qy) is a valid point on the curve P256 ==> checked by the arithmetization/ECDATA | ||
| // 5. (qx, qy) is not (0,0) ==> checked by the arithmetization/ECDATA | ||
|
|
||
| // we currently implement signature verification directly to avoid cases | ||
| // which the ECDSA gadget does not handle: | ||
| // * we don't need to perform range checks on r and s as they are done by the arithmetization | ||
| // * instead of two divs we compute an inverse and do two multiplications | ||
| // * we perform modular equality check instead of bitwise equality check | ||
| curve, err := sw_emulated.New[emulated.P256Fp, emulated.P256Fr](api, sw_emulated.GetP256Params()) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| scalarApi, err := emulated.NewField[emulated.P256Fr](api) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| baseApi, err := emulated.NewField[emulated.P256Fp](api) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| // we don't perform range checks on r and s as they are done by the arithmetization | ||
| sinv := scalarApi.Inverse(s) | ||
| msInv := scalarApi.Mul(msgHash, sinv) | ||
| rsInv := scalarApi.Mul(r, sinv) | ||
| msInvG := curve.ScalarMulBase(msInv, algopts.WithCompleteArithmetic()) | ||
| PK := sw_emulated.AffinePoint[emulated.P256Fp]{X: *qx, Y: *qy} | ||
| rsInvQ := curve.ScalarMul(&PK, rsInv, algopts.WithCompleteArithmetic()) | ||
| Rprime := curve.AddUnified(msInvG, rsInvQ) | ||
|
|
||
| ResIsInfinity := api.And( | ||
| baseApi.IsZero(&Rprime.X), | ||
| baseApi.IsZero(&Rprime.Y), | ||
| ) | ||
| // we need to perform modular equality check, but r and Rx are in different fields. We manually | ||
| // enforce them to be in the same field by doing limbwise conversion. | ||
| Rx := baseApi.ReduceStrict(&Rprime.X) | ||
| RxInFr := scalarApi.NewElement(Rx.Limbs) | ||
|
|
||
| // we don't have IsEqual method, so we do it through a diff | ||
| diffRxR := scalarApi.Sub(RxInFr, r) | ||
| isEqual := scalarApi.IsZero(diffRxR) | ||
|
|
||
| res := api.And( | ||
| api.Sub(1, ResIsInfinity), // signature is invalid if R' is infinity | ||
| isEqual, // r == R'.X mod n | ||
| ) | ||
| return res | ||
|
|
||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
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.
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.
The comments reference "arithmetization" and "ECDATA" which are zkEVM-specific implementation details. For a general-purpose gnark circuit, these validations are not performed by external systems. The comments should clarify that when used outside a zkEVM context, the caller must ensure: (1) input length is 160 bytes, (2) 0 < r < n and 0 < s < n, (3) 0 ≤ qx < p and 0 ≤ qy < p, (4) (qx, qy) is on the P-256 curve, and (5) (qx, qy) ≠ (0,0). Alternatively, consider adding explicit validation within the circuit for general-purpose use.