-
Notifications
You must be signed in to change notification settings - Fork 191
feat(tests): add EIP-7951 for secp256r1 curve #1670
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
Conversation
spencer-tb
left a comment
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.
Really nice! Just wondering where you got the vectors from? Are they the same as these and if so do we know how they are generated: https://github.com/google/wycheproof/blob/master/testvectors/ecdsa_secp256r1_sha256_test.json
I think in a future PR we should add some additional edge cases manually. As for BLS (EIP-2537) we initially relied heavily on the external test vectors and missed some important coverage points.
Although the curve itself is extremely well tested the point of these manual cases would be to find divergence between the libraries that clients use or how they integrate the library (or simply a client implementation). Some cases I can think of just now:
- Zero signature component,
s=0should trigger divide by zero on libraries,r=0too. - All possible point at infinity input cases,
(x, y) = (0, 0)is the obvious case. - Some/all point at infinity output cases, I think when the hash is 0.
- Boundary conditions, that aim to trigger overflows. Cases where
x, y >= porr, s >= n, or where they are close top-1,n-1. The RIP follows NIST so1 <= s <= n-1, but all ethereum signatures thus far ares <= n/2, adding a case for this would be nice. - Input length or encoding cases, i.e < 160 bytes, > 160 bytes, 31 byte components etc.
tests/osaka/eip7212_secp256r1_precompiles/test_secp256r1_precompile_module.py
Outdated
Show resolved
Hide resolved
|
@spencer-tb Thanks for your reply The test vectors are from the Wycheproof repository. We’ve extracted and converted them into a format compatible with our test framework at These test cases are widely used across various Layer 2 implementations. We’ve also reviewed the testing strategies and infrastructure from them, and here’s a short summary of our findings. https://hackmd.io/@3uGN6yZhRLWqNJalNI1kig/BkavbMd-gl Regarding your second point, I agree that adding more test cases is important. Although the current implementation passes all tests, I’ve identified a few edge cases that should be covered — for instance, when the public key point (x, y) is not actually on the curve. |
e9aef27 to
42824e9
Compare
marioevz
left a comment
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.
Awesome work! I've left a couple of comments to be resolved before proceeding 👍
tests/osaka/eip7212_secp256r1_precompiles/test_p256verify_as_set_code_delegated_addr.py
Outdated
Show resolved
Hide resolved
tests/osaka/eip7212_secp256r1_precompiles/test_secp256r1_precompile_module.py
Outdated
Show resolved
Hide resolved
tests/osaka/eip7212_secp256r1_precompiles/test_secp256r1_precompile_module.py
Outdated
Show resolved
Hide resolved
bf73eb7 to
a1125c8
Compare
|
Amazing! Looks good from myside! Could we get these 2 comments in:
And maybe update the PR description/name to be 7951, stating that its based on 7212? |
a1125c8 to
d1167b5
Compare
|
Last couple items from myside! :D Could we rename:
Then add a line to the changelog here: |
|
Rebasing onto main will fix the evmone coverage report fail in ci :) |
|
@spencer-tb I've rebased to main branch, but the evm one ci is still failing, anything I should take a look? |
a887bb6 to
bcff1b9
Compare
This reverts commit 44a62c1.
c73be2d to
30d9cde
Compare
marioevz
left a comment
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.
Awesome work, thanks! 🎉
* feat: configure test env for osaka * feat: add tests for rip-7212 to support secp256r1 * test: add gas requirement and call type test * test(P256VERIFY): add fork transition tests * test: use precompile as set code delegated address * test: add precompile as tx entry point * refactor: add eip checklist marker * refactor: update rip7212 to eip7951 * feat: add curve params and negative tests * refactor: update precompile as set code delegated address * fix: update bounded value to include precompile * doc: add comments for external test vectors * docs: update changelog * refactor: rename filename * fix: update frontier configuration * Revert "fix: update frontier configuration" This reverts commit 44a62c1. * refactor: improve precompile address handling in tests * fix: tox --------- Co-authored-by: Mario Vega <[email protected]>
🗒️ Description
This PR implements EIP-7951 precompile to support
secp256r1curve (based on RIP-7212).Running the test locally
Since the current EELS repository does not support this functionality, I submitted an EELS PR to implement the
P256VERIFYprecompile. This precompile relies on cryptographic operations not currently supported by the default Python packages, so two additional packages are required:cryptographypycryptodomeAdditionally the EELS resolver must have these 2 packages added its
setup.cfg.To test this PR locally, follow the steps below.
pyproject.toml:Source of the test vector
In this PR, we convert the Wycheproof dataset into secp256r1_test.json and import the test cases for validation.
While this approach is commonly used across various Layer 2 implementations (see analysis here), adding additional test cases is still necessary to ensure broader coverage.
Implementation Note
In both EIP-7951 & RIP-7212 we could not depend on the output data length to determine whether the low-level call is successful, this might be different from the behavior of
BLSprecompile (EIP-2537).Consider the following two scenarios:
🔗 Related Issues
ethereum/execution-specs#1249
✅ Checklist