Skip to content

Conversation

LouisTsai-Csie
Copy link
Collaborator

@LouisTsai-Csie LouisTsai-Csie commented Sep 16, 2025

🗒️ Description

Currently, we enforce verification of expected_benchmark_gas_used, which ensures that no invalid operation terminates execution prematurely. This PR relaxes that rule to give benchmark developers more flexibility.

For example, when benchmarks use while-loops or conditional comparisons, calculating the exact gas usage becomes difficult. In such cases, developers may end up spending more effort on tracking precise gas costs than on the test logic itself. Moreover, not all benchmark cases are intended to fully exhaust the block’s gas like it does in gas-limit-testing.

That said, this change does not mean future benchmarks can skip gas accounting altogether. This exception should only apply when gas usage is inherently hard to predict. By default, reviewers must still verify that expected_benchmark_gas_used is correctly set.

This is initially discussed in PR #2090, although PR created, it is still open to discussion!

🔗 Related Issues or PRs

PR #2090
Comment: #2090 (comment)

✅ Checklist

  • All: Ran fast tox checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:
    uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlint
  • All: PR title adheres to the repo standard - it will be used as the squash commit message and should start type(scope):.
  • All: Considered adding an entry to CHANGELOG.md.
  • All: Considered updating the online docs in the ./docs/ directory.
  • All: Set appropriate labels for the changes (only maintainers can apply labels).
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.
  • Tests: For PRs implementing a missed test case, update the post-mortem document to add an entry the list.
  • Ported Tests: All converted JSON/YML tests from ethereum/tests or tests/static have been assigned @ported_from marker.

Copy link
Contributor

@spencer-tb spencer-tb left a comment

Choose a reason for hiding this comment

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

Works from my side. From the call we had a couple weeks ago on the 1st of September with Guilliaume my expectation was that we'd make this change. LGTM!

cc-ing @marioevz for when he's back - happy to merge nonetheless!

@spencer-tb spencer-tb requested a review from fselmo September 16, 2025 13:11
@spencer-tb
Copy link
Contributor

Tagged @fselmo for another opinion - if all good I say we should merge.

Copy link
Collaborator

@fselmo fselmo left a comment

Choose a reason for hiding this comment

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

Looks good to me. We will have to be very diligent about checking the logic of the tests that set this. I added one nit to think about but this is not a blocker.

Copy link
Collaborator

@fselmo fselmo left a comment

Choose a reason for hiding this comment

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

lgtm!

@fselmo fselmo merged commit 50c6845 into ethereum:main Sep 18, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants