Skip to content

fix(static_tests): adjusted gasLimit to only allow gasLimit of up to 30mil as enforced in eip-7825 #1587

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

felix314159
Copy link
Collaborator

🗒️ Description

For now this is only for static_tests we have. Currently filling and executing to see if it all still works. Please thoroughly review this PR. These changes were done via a Python script that has no notion of what any of this means, next thing we should do is within our framework at fill check whether gasLimit is in allowed range

🔗 Related Issues

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

@winsvega
Copy link
Contributor

does test filling work without errors?

@jochem-brouwer
Copy link
Member

Shouldn't this just mark the tests which have a gas limit higher than 30M invalid/not available from the fork which includes EIP-7825? Merging this would also require explicit checks that the test itself does not depend on having more than 30M gas (one use case for this "huge" gas limit would be benchmarks/stress tests, especially for precompiles)

@winsvega
Copy link
Contributor

I think most of the test have high gas limit just in case and copied from template. if the test filling works we can cap it.

@winsvega winsvega self-requested a review May 12, 2025 12:44
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.

Here some fixes
felix314159#1

Need to fix all the failures. Basically some tests require 1 trillion gas because they test the memory alloc limits or call depths. or simply have too many combinations.

@marioevz
Copy link
Member

@winsvega @felix314159 #808 is merged, please rebase and stBadOpcode/opcDiffPlaces changes can now be removed.

@felix314159 felix314159 force-pushed the gaslimit-fixes-new branch from 69d0985 to 09630a1 Compare May 13, 2025 09:29
@felix314159
Copy link
Collaborator Author

After rebase I filled again with uv run fill ./tests/static --fill-static-tests --evm-bin=/home/$USER/Documents/evmone/build/bin/evmone-t8n -n auto -vv and now we are down to 36 failed tests:

* stCreateTest/createLargeResultFiller.yml
* stCreateTest/CREATE_FirstByte_loopFiller.yml
* stCreateTest/CreateOOGafterMaxCodesizeFiller.yml
* stShift/shiftCombinationsFiller.yml
* stSelfBalance/diffPlacesFiller.yml

@winsvega
Copy link
Contributor

winsvega commented May 14, 2025

I optimised 1 more test:
felix314159#2

Review:
stShift/shiftCombinationsFiller.yml
has too many combinations better to rewrite in .py

stCreateTest/CREATE_FirstByte_loopFiller.yml
this test eats too much gas. it can be optimized when converted .py

stCreateTest/CreateOOGafterMaxCodesizeFiller
have large count contract interactions, need to rewrite it for prague limiting number of contract interactions

stSelfBalance/diffPlacesFiller.yml
seems to be already covered by test scenarios (recently merged) the only scenario missing is rerun. need to implement a scenario batch for test scenarios. where. call operation. call somthing elese and [oog, revert, selfdestruct, return] call opeartion again. make sure operation is not affected. (call1 = call2)

here I open a tracker issue: #1594

optimise createLargeResult test
@spencer-tb
Copy link
Contributor

As these are yml files, I'm wondering if we could set the gas limit as an env var that is set during fill time. That way it can be dynamic and we don't need to change it again if the gas limit changes etc. Something like:

gasLimit: ${GAS_LIMIT}

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.

5 participants