Skip to content

feat(tests) EIP-2929: Gas cost increase tests #1305

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

reedsa
Copy link
Contributor

@reedsa reedsa commented Mar 12, 2025

πŸ—’οΈ Description

Converted tests for gas cost increases from EIP-2929.

πŸ”— Related Issues

#972 (stPreCompiledContracts / stPreCompiledContracts2)

βœ… Checklist

  • All: Set appropriate labels for the changes.
  • All: Considered squashing commits to improve commit history.
  • All: Added an entry to CHANGELOG.md.
  • All: Considered updating the online docs in the ./docs/ directory.
  • Tests: All converted JSON/YML tests from ethereum/tests have been added to converted-ethereum-tests.txt.
  • Tests: A PR with removal of converted JSON/YML tests from ethereum/tests have been opened.
  • Tests: Included the type and version of evm t8n tool used to locally execute test cases: e.g., ref with commit hash or geth 1.13.1-stable-3f40e65.
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.

pytest.param(
{
"address": 0x08,
"value": 1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so looks like the parametrization in all this tests really is

adress, value, output size.

it can be one test with that many parametrizations

for each addesss
for each value
for each output size

and so on. if the test body does not change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had issues with parametrizing with expected values that depend on specific inputs. Ended up changing these to parametrize the inputs and created separate tests for the permutations of the expected output.

I noticed the expected gas cost for many cases are different from the original tests. I'm looking at those closely to figure out the actual values to use, there may be additional changes to the contract based on certain scenarios. I'll check dr to figure out the differences there.

Does the current approach seem reasonable? If so, I plan to add the remaining test cases that use CALLCODE, DELEGATECALL, etc.

@reedsa reedsa force-pushed the eip2929-gas-cost branch from 9335560 to b760963 Compare March 24, 2025 17:13
@reedsa reedsa added type:test Type: Add/refactor fw unit tests; no fw or el client test case changes type:feat type: Feature port Related to porting ethereum/tests to EEST and removed type:test Type: Add/refactor fw unit tests; no fw or el client test case changes labels Mar 24, 2025
@reedsa reedsa force-pushed the eip2929-gas-cost branch from b760963 to 82800a0 Compare March 24, 2025 19:10
@reedsa reedsa force-pushed the eip2929-gas-cost branch from 61e278e to bb83ec9 Compare March 24, 2025 20:01
@reedsa
Copy link
Contributor Author

reedsa commented Mar 26, 2025

@marioevz @winsvega I'm seeing some differences for the final gas cost in the failing tests. The original values seem to be used to account for the cost to create a trie. It's possible these tests have a different cost due to initial allocations. Should I update the expectations to reflect the actual costs that I'm seeing here?

expected_gas_cost_change = hex(0x0)
if not precompile_exists:
expected_gas_cost_change = hex(0x6B6C)
elif fork >= Cancun and address in ["0x1", "0x2", "0x3", "0x4", "0x5", "0x6", "0x7", "0x8"]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fork class iteslf has a list of precompiles defined in it. Use that function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic in each of these tests is set for only certain precompiles, for example the precompiles at 0x9 and 0xa for Cancun should expect to have no change in the cost of gas where the others will expect to have a cost difference of 0x61A8.

@winsvega
Copy link
Contributor

Ideally we compare the vmtrace, buf if the diff is not singnificant I assume it's because ported test uses fewer opcodes ?

Whats the diff?

@reedsa
Copy link
Contributor Author

reedsa commented Mar 26, 2025

Ideally we compare the vmtrace, buf if the diff is not singnificant I assume it's because ported test uses fewer opcodes ?

Whats the diff?

There are differences with the test setup, mainly because of the parameterization used in these new tests. The CALLDATALOAD opcodes are no longer used to determine different "cases" outlined in the original test. There are other operations used for creating separate contracts at specific addresses to test gas cost of accessing those, but those are not used in the cases I've implemented so far.

The cases implemented so far only capture cases for the CALL operation for precompiles, checking gas cost before and after in the Cancun and Prague forks.

A separate question I have is about the EIP-2929 in general. Currently the original tests capture cases in forks >= Cancun. Since this EIP was in the Berlin fork, should these tests actually be checking the change in cost within the fork before (Istanbul) and those witin Berlin?

@reedsa reedsa self-assigned this Mar 28, 2025
@reedsa reedsa requested a review from marioevz March 28, 2025 22:03
@spencer-tb spencer-tb added the scope:tests Scope: Changes EL client test cases in `./tests` label Apr 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
port Related to porting ethereum/tests to EEST scope:tests Scope: Changes EL client test cases in `./tests` type:feat type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants