-
Notifications
You must be signed in to change notification settings - Fork 144
feat(tests): add RIP-7212 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
base: main
Are you sure you want to change the base?
Conversation
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=0
should trigger divide by zero on libraries,r=0
too. - 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 >= p
orr, 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.
@@ -0,0 +1,4 @@ | |||
""" | |||
abstract: Tests [EIP-7212 EIP-7212: Precompiled for secp256r1 Curve Support](https://eips.ethereum.org/EIPS/eip-7212) | |||
Test cases for [EIP-7212 EIP-7212: Precompiled for secp256r1 Curve Support](https://eips.ethereum.org/EIPS/eip-7212)]. |
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.
This link doesn't seem to exist as this is our first RIP!
We could use the eip directory website instead or the github link:
Wdyt @danceratopz?
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 am not sure if the RIP will be transformed to EIP in the future, or a new EIP will be proposed for this precompile as discussed in the last ACD meeting.
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.
Finally can be updated it seems :)
|
||
@pytest.mark.parametrize( | ||
"input_data,expected_output,vector_gas_value", | ||
vectors_from_file("secp256r1_test.json"), |
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 could be nice to add a comment here stating where the vectors are from and/or how they are generated if applicable:
vectors_from_file("secp256r1_test.json"), | |
vectors_from_file("secp256r1_test.json"), | |
# From wycheproof... etc |
@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
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 👍
@@ -0,0 +1,4 @@ | |||
""" | |||
abstract: Tests [EIP-7212 EIP-7212: Precompiled for secp256r1 Curve Support](https://eips.ethereum.org/EIPS/eip-7212) | |||
Test cases for [EIP-7212 EIP-7212: Precompiled for secp256r1 Curve Support](https://eips.ethereum.org/EIPS/eip-7212)]. |
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.
Finally can be updated it seems :)
|
||
P256VERIFY = 0x100 | ||
""" | ||
return [Address(0x100)] + super(Osaka, cls).precompiles(block_number, timestamp) |
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.
We should verify that tests in https://github.com/ethereum/execution-spec-tests/tree/main/tests/frontier/precompiles are not susceptible to breaking with this change because I remember they might require all precompile addresses to be sequential.
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 are two tests involved: test_precompiles
and test_precompile_absence
.
In test_precompiles
, the UPPER_BOUND
is set to 0xff, so addresses above this (such as 0x100 like RIP-7212) are excluded from the test. However, even when we raise the upper bound to higher value, all test cases still pass.
As for test_precompile_absence
, it checks for the behavior of inactive (non-enabled) precompiles, so this case remains unaffected.
In summary, neither test performs a strict sequential check. But for test_precompiles
, we can consider whether or not to include the P256VERIFY
opcode.
|
||
@pytest.mark.with_all_call_opcodes | ||
@pytest.mark.with_all_precompiles | ||
@pytest.mark.eip_checklist("new_precompile/test/call_contexts/set_code") |
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 think we should rather add this new precompile to existing test in
execution-spec-tests/tests/prague/eip7702_set_code_tx/test_set_code_txs.py
Lines 2518 to 2570 in e5e3ebe
@pytest.mark.with_all_call_opcodes | |
@pytest.mark.with_all_precompiles | |
def test_set_code_to_precompile( | |
state_test: StateTestFiller, | |
pre: Alloc, | |
precompile: int, | |
call_opcode: Op, | |
): | |
""" | |
Test setting the code of an account to a pre-compile address and executing all call | |
opcodes. | |
""" | |
auth_signer = pre.fund_eoa(auth_account_start_balance) | |
value = 1 if call_opcode in {Op.CALL, Op.CALLCODE, Op.EXTCALL} else 0 | |
caller_code_storage = Storage() | |
caller_code = ( | |
Op.SSTORE( | |
caller_code_storage.store_next(call_return_code(opcode=call_opcode, success=True)), | |
call_opcode(address=auth_signer, value=value, gas=0), | |
) | |
+ Op.SSTORE(caller_code_storage.store_next(0), Op.RETURNDATASIZE) | |
+ Op.STOP | |
) | |
caller_code_address = pre.deploy_contract(caller_code, balance=value) | |
tx = Transaction( | |
sender=pre.fund_eoa(), | |
gas_limit=500_000, | |
to=caller_code_address, | |
authorization_list=[ | |
AuthorizationTuple( | |
address=Address(precompile), | |
nonce=0, | |
signer=auth_signer, | |
), | |
], | |
) | |
state_test( | |
env=Environment(), | |
pre=pre, | |
tx=tx, | |
post={ | |
auth_signer: Account( | |
nonce=1, | |
code=Spec.delegation_designation(Address(precompile)), | |
), | |
caller_code_address: Account( | |
storage=caller_code_storage, | |
), | |
}, | |
) |
And also take the opportunity to add the marker to that test as:
@pytest.mark.eip_checklist("new_precompile/test/call_contexts/set_code", eips=[7951])
@pytest.mark.eip_checklist("new_precompile/test/value_transfer/fee/over") | ||
@pytest.mark.eip_checklist("new_precompile/test/value_transfer/fee/under") |
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.
Should be:
@pytest.mark.eip_checklist("new_precompile/test/value_transfer/fee/over") | |
@pytest.mark.eip_checklist("new_precompile/test/value_transfer/fee/under") | |
@pytest.mark.eip_checklist("new_precompile/test/gas_usage/constant/exact") | |
@pytest.mark.eip_checklist("new_precompile/test/gas_usage/constant/oog") |
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.
For the first case (id="extra_gas"), it may not fall under the gas_usage/constant/exact
category, but there is no gas_usage/constant/over
case either.
For most of the test case, it consume the exact gas amount, so these two cases represents two special scenarios: more & less gas than requirement
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 think we should add an explicit exact
case here anyway just for the sake of completeness and readability, even though I understand it's implicitly checked in many other tests.
The over
case we can leave the case but unmarked because it's not part of the checklist (we could add it to the checklist later).
@pytest.fixture | ||
def sender(pre: Alloc) -> EOA: | ||
"""Sender of the transaction.""" | ||
return pre.fund_eoa(1_000_000_000_000_000_000) |
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.
return pre.fund_eoa(1_000_000_000_000_000_000) | |
return pre.fund_eoa() |
The default amount should be enough, otherwise we can take a look if any of the tests is using too much gas.
) | ||
@pytest.mark.parametrize("precompile_address", [Spec.P256VERIFY], ids=[""]) | ||
@pytest.mark.eip_checklist("new_precompile/test/call_contexts/normal") | ||
def test_valid(state_test: StateTestFiller, pre: Alloc, post: dict, tx: Transaction): |
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.
We need to add a negative test test_invalid
with all combinations in the EIP:
https://github.com/ethereum/EIPs/blob/d557584cbd0bbb88df52de7b43f0a35c2837a26f/EIPS/eip-7951.md?plain=1#L106-L110
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've updated the invalid test case, please let me know if there is anything more that needs to be added!
ecc5490
to
64451b4
Compare
bf73eb7
to
a1125c8
Compare
🗒️ Description
This PR implements the RIP-7212 precompile to support
secp256r1
curve.Running the test locally
Since the current EELS repository does not support this functionality, I submitted an EELS PR to implement the
P256VERIFY
precompile. This precompile relies on cryptographic operations not currently supported by the default Python packages, so two additional packages are required:cryptography
pycryptodome
To test this PR locally, follow the steps below:
Clone the Execution Specs Fork: Clone my execution-specs fork and check out the
eips/osaka/eip-7212
branch.Alternatively, apply the changes from the PR directly.
Clone and Modify the Resolver: Clone
ethereum-spec-evm-resolver
. Then modify thesetup.cfg
file to include the required packages:eels_resolutions.json
configuration like this:response_json = response.json()
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 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
BLS
precompile (EIP-2537).Consider the following two scenarios:
🔗 Related Issues
✅ Checklist
mkdocs serve
locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.