feat: EIP-7951 for ECDSA on P-256 curve #1649
Conversation
yelhousni
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR implements the P256Verify precompile (EIP-7951) for ECDSA signature verification over the secp256r1 (P-256) elliptic curve. The implementation provides a gnark circuit for verifying ECDSA signatures at the EVM precompile address 0x100.
Key changes:
- Adds
P256Verifyfunction implementing EIP-7951 using gnark's emulated P256 curve and ECDSA signature verification - Includes comprehensive test coverage with both basic functional tests and Wycheproof test vectors
- Provides 60+ test vectors from the Wycheproof project covering edge cases and malleability scenarios
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| std/evmprecompiles/256-p256verify.go | Core implementation of P256Verify precompile using emulated ECDSA signature verification |
| std/evmprecompiles/256-p256verify_test.go | Test suite with basic circuit tests and EIP vector validation |
| std/evmprecompiles/test_vectors/p256verify_vectors_clean.json | Wycheproof test vectors for comprehensive edge case coverage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "encoding/hex" | ||
| "encoding/json" | ||
| "fmt" | ||
| "io/ioutil" |
There was a problem hiding this comment.
The io/ioutil package is deprecated since Go 1.16. Replace ioutil.ReadFile with os.ReadFile and update the import to use "os" instead of "io/ioutil".
| "io/ioutil" |
| panic(err) | ||
| } | ||
| if len(raw) != 160 { | ||
| return nil, nil, nil, nil, nil |
There was a problem hiding this comment.
The function splitInput160 silently returns nil values when the input length is not 160 bytes. This could lead to nil pointer dereferences on line 97-101 where the returned values are dereferenced. Consider returning an error or panicking with a descriptive message if the length check fails.
| return nil, nil, nil, nil, nil | |
| panic(fmt.Sprintf("splitInput160: input length is %d bytes, expected 160", len(raw))) |
| // 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 |
There was a problem hiding this comment.
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.
| // 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 | |
| // The following checks are performed by external systems in the zkEVM context (arithmetization/ECDATA). | |
| // For general-purpose use outside zkEVM, 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 a valid point on the P-256 curve, | |
| // 5. (qx, qy) ≠ (0,0). |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| t.Run(fmt.Sprintf("vector_%03d_%s", i, v.Name), func(t *testing.T) { | ||
| err := test.IsSolved(&circuit, &witness, ecc.BN254.ScalarField()) | ||
| assert.NoError(err) | ||
| }) |
There was a problem hiding this comment.
Bug: Assert Context Breaks Subtest Isolation
The assert helper is created with the outer test context but used inside t.Run subtests. When assert.NoError(err) fails, it reports to the parent test rather than the subtest, causing the entire test function to stop instead of just failing the specific subtest. This prevents other test vectors from running and makes it difficult to identify which specific vector failed. The assertion should use the subtest's t parameter instead.
Description
This PR implements a circuit corresponding to https://eips.ethereum.org/EIPS/eip-7951 alongside test against Consensys/gnark-crypto#767 and Wycheproof test vectors (https://eips.ethereum.org/assets/eip-7951/test-vectors.json).
Needs Consensys/gnark-crypto#767 to be merged first.
Type of change
How has this been tested?
Tests against gnark-crypto pass but against Wycheproof some edge cases fail because they are checked at the arithmetization level not the gnark circuit level. Currently,
p256verify_vectors_clean.jsoncontains some data that passes gnark circuit test andp256verify_vectors.jsonis the entire data.How has this been benchmarked?
In a BN254 circuit:
Checklist:
golangci-lintdoes not output errors locallyNote
Implements a P‑256 (secp256r1) ECDSA verification circuit and adds comprehensive tests with Wycheproof and gnark‑crypto vectors.
std/evmprecompiles/256-p256verify.goimplementingP256Verify(ECDSA verification oversecp256r1/P-256) using gnarkecdsaandsw_emulatedparams.std/evmprecompiles/256-p256verify_test.gowith unit tests: local keygen/sign/verify and vector-driven tests.std/evmprecompiles/test_vectors/p256verify_vectors.json(full) andstd/evmprecompiles/test_vectors/p256verify_vectors_clean.json(subset passing circuit).Written by Cursor Bugbot for commit 3bc2819. This will update automatically on new commits. Configure here.