-
Notifications
You must be signed in to change notification settings - Fork 376
feat: add mainnet tests for osaka #1806
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: forks/osaka
Are you sure you want to change the base?
feat: add mainnet tests for osaka #1806
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.
Thanks for starting on this! :)
Some comments on the existing tests below and ideas for the other EIPs.
I understand the logic for not adding tests for some of these EIPs but if we can I really think we should, mostly for completeness but also so we can sleep easy. It will additionally look better for announcing on twitter/x after the fork!
In another PR I think we should add mainnet marked tests for EIP-7954 and EIP-7918 (blob related EIPs).
EIP-7823
While we already have some validation from the EIP-7883 tests, I do think it would be good to add a test right at the 1024-byte boundary. We can re-use base_boundary:
pytest.param(
ModExpInput(
base=b"\x01" * Spec.MAX_LENGTH_BYTES,
exponent=b"\x00",
modulus=b"\x02",
),EIP-7939
I noted you stated not to add a tests for this but I think we should add at least one maybe 2 for sanity:
- One basic case with for example 8 leading zeros,
- One edge case with all zeros (256).
Something like the following cases:
@pytest.mark.parametrize(
"value,expected_clz",
[
pytest.param( 0x0100...0000,
8,
id="clz_8_leading_zeros",
),
pytest.param(
0x0000...0000,
256,
id="clz_all_zeros",
),
],
)EIP-7825
Worth considering for completeness, but I won't push too hard on this one.
Adding a simple state test tx on exactly the cap (2^24).
Please see test_gas_limit_cap_over: https://github.com/ethereum/execution-specs/blob/forks/osaka/tests/osaka/eip7825_transaction_gas_limit_cap/test_tx_gas_limit.py#L68
| ), | ||
| ], | ||
| ) | ||
| @pytest.mark.valid_at("Osaka") |
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 mark these tests as mainnet only like we do in: https://github.com/ethereum/execution-spec-tests/pull/1511/files#diff-a9add5727efb38c476ff59942c2a0732eb1f9dde4958dd3b573a36fe3444bf05R15
| @pytest.mark.valid_at("Osaka") | |
| @pytest.mark.valid_at("Osaka") | |
| @pytest.mark.mainnet |
We only want to fill/execute these specifically for mainnet validation.
| EIP-7883 ModExp gas cost increase tests. | ||
|
|
||
| Tests for ModExp gas cost increase in | ||
| [EIP-7883: ModExp Gas Cost Increase](https://eips.ethereum.org/EIPS/eip-7883). |
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.
Could we make it clear in the docstring that these are specifically for mainnet execute use only (fill is a sanity check).
| EIP-7883 ModExp gas cost increase tests. | |
| Tests for ModExp gas cost increase in | |
| [EIP-7883: ModExp Gas Cost Increase](https://eips.ethereum.org/EIPS/eip-7883). | |
| Mainnet Marked Checklist Tests for ModExp gas cost increase in | |
| [EIP-7883: ModExp Gas Cost Increase](https://eips.ethereum.org/EIPS/eip-7883). |
| @@ -0,0 +1,91 @@ | |||
| """ | |||
| Tests for [EIP-7951: Precompile for secp256r1 Curve Support](https://eips.ethereum.org/EIPS/eip-7951). | |||
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.
Similar here:
| Tests for [EIP-7951: Precompile for secp256r1 Curve Support](https://eips.ethereum.org/EIPS/eip-7951). | |
| Mainnet Marked Checklist Tests for [EIP-7951: Precompile for secp256r1 Curve Support](https://eips.ethereum.org/EIPS/eip-7951). |
| REFERENCE_SPEC_GIT_PATH = ref_spec_7951.git_path | ||
| REFERENCE_SPEC_VERSION = ref_spec_7951.version | ||
|
|
||
|
|
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.
If we add this here we can remove Osaka mark's below.
| pytestmark = [pytest.mark.valid_at("Osaka"), pytest.mark.mainnet] |
| ), | ||
| ], | ||
| ) | ||
| @pytest.mark.valid_at("Osaka") |
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.
| @pytest.mark.valid_at("Osaka") |
| pytest.param( | ||
| ModExpInput( | ||
| base="ff" * 32 + "fb", | ||
| exponent="02", | ||
| modulus="05", | ||
| declared_base_length=33, | ||
| declared_exponent_length=1, | ||
| declared_modulus_length=1, | ||
| ), | ||
| bytes([1]), | ||
| id="33-bytes-long-base", # higher cost than 32 bytes |
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 remove this test and add 1/2 extras. These both currently test the minimum gas cost increase of 200 to 500 from the EIP.
-
If we add
nagydani-1-pow0x10001from here we additionally check that the 2x complexity multiplier works,if max_length > 32: multiplication_complexity = 2 * words**2, from the spec. -
If we add
zero-exponent-64bytesfrom here we additionally test the 16x exponent multiplaction works,iteration_count = (16 * (exponent_length - 32)) + ((exponent & (2**256 - 1)).bit_length() - 1), from the spec.
Please see below for modexp equation snippets:
https://eips.ethereum.org/EIPS/eip-7883#specification
They shouldn't cost too much more.
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 link you have put shows that these two test do NOT both test the same thing. The 32 bytes base test is cheaper than the 33 bytes test. If you remove one of them you lose coverage of that case. But I agree that we can add another test we an even larger number, e.g. exp of size 100 bytes
| def test_invalid( | ||
| state_test: StateTestFiller, pre: Alloc, post: dict, tx: Transaction | ||
| ) -> None: | ||
| """ | ||
| Negative mainnet test for the P256VERIFY precompile. | ||
|
|
||
| The signature actually is a valid secp256k1 signature, | ||
| so this is an interesting test case. | ||
| """ | ||
| state_test(env=Environment(), pre=pre, post=post, tx=tx) |
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.
Nice. My understanding is that this is a valid tx but with an invalid signature?
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 is a valid ecdsa signature over a different curve, so the signature verification is expected to fail (but this is a good test because the other curve is also used by clients)
🗒️ Description
Adds tests for modexp cost increase and introduction of secp256r1 precompile.
Blob tests still missing, standalone script still has to be integrated into framework for that.
Partially solves #1714
🔗 Related Issues or PRs
N/A.
✅ Checklist
toxchecks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx tox -e statictype(scope):.mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.@ported_frommarker.Cute Animal Picture