Skip to content
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

feat(forks,tests): Add EIP-7623 #1004

Merged
merged 12 commits into from
Dec 18, 2024
Merged

feat(forks,tests): Add EIP-7623 #1004

merged 12 commits into from
Dec 18, 2024

Conversation

marioevz
Copy link
Member

@marioevz marioevz commented Dec 7, 2024

🗒️ Description

Adds tests for EIP-7623: Increase calldata cost.

Framework changes

Modifies transaction_intrinsic_cost_calculator that is returned by the fork objects to account for the extra intrinsic cost that the EIP adds.

Also a new parameter is added to the function in order to return different values depending on how the function is used:

  • Return the minimum gas that the transaction requires to be included
  • Return the gas cost that is deducted before the transaction starts execution

Fix Existing Tests

Some tests were broken because they needed a transaction to start with a deterministic amount of gas and also with a given calldata length.

Solution to most of these issues was to add an access list so the intrinsic gas cost exceeded the new floor data cost.

🔗 Related Issues

None

✅ 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.

Copy link
Contributor

@winsvega winsvega left a comment

Choose a reason for hiding this comment

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

added some comments, quick review

this is a good eip for transaction tests. in tx tests we assume that intrinsic gas rules are still checked. so if tx deserialization is ok, but didn't pass intrinsic gas check such tx test is invalid tx.

by playing with transaction format in any way we want we can parametrize transaction fields in transaction test
not building env,pre,blocks

@marioevz
Copy link
Member Author

@reedsa @fselmo

@marioevz marioevz requested review from reedsa and fselmo December 10, 2024 14:53
@marioevz marioevz marked this pull request as ready for review December 11, 2024 00:03
@marioevz marioevz requested a review from danceratopz December 11, 2024 00:03
Copy link
Contributor

@reedsa reedsa left a comment

Choose a reason for hiding this comment

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

A small suggestion for docs style but otherwise looks great!

src/ethereum_test_forks/base_fork.py Outdated Show resolved Hide resolved
Copy link
Member

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

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

Looks really good. Just one minor nit on the helper function name :)

tests/prague/eip7623_increase_calldata_cost/helpers.py Outdated Show resolved Hide resolved
tests/prague/eip7623_increase_calldata_cost/conftest.py Outdated Show resolved Hide resolved
tests/prague/eip7623_increase_calldata_cost/conftest.py Outdated Show resolved Hide resolved
@marioevz marioevz merged commit 6446ed4 into main Dec 18, 2024
6 checks passed
@marioevz marioevz deleted the eip-7623 branch December 18, 2024 21:06
codeofcarson pushed a commit to codeofcarson/execution-spec-tests that referenced this pull request Jan 24, 2025
* feat(forks): Add EIP-7623 logic to gas calculation

* fix(tests): Broken tests due to gas cost change

* feat(forks): Add `transaction_data_floor_cost_calculator`

* new(tests): EIP-7623: Add transaction validity tests

* review comments

* new(tests): EIP-7623: Add gas consumption tests

* refactor(tests): EIP-7623: minor refactor

* Update src/ethereum_test_forks/base_fork.py

Co-authored-by: Stuart Reed <[email protected]>

* Update src/ethereum_test_forks/base_fork.py

* Apply suggestions from code review

Co-authored-by: danceratopz <[email protected]>

* fix(forks): tox

* docs: changelog

---------

Co-authored-by: Stuart Reed <[email protected]>
Co-authored-by: danceratopz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants