Skip to content

feat(tests): remove Blake2b max rounds static test#1941

Merged
marioevz merged 1 commit intoethereum:mainfrom
ipsilon:no_blake_max_rounds
Jul 24, 2025
Merged

feat(tests): remove Blake2b max rounds static test#1941
marioevz merged 1 commit intoethereum:mainfrom
ipsilon:no_blake_max_rounds

Conversation

@chfast
Copy link
Copy Markdown
Member

@chfast chfast commented Jul 23, 2025

🗒️ Description

This test brings no additional coverage and with 10G gas limit is very annoying. It is also included in the regular releases so on the Client side you have to filter it out because it runs to long.

There are also benchmarks for Blake2b, so if you think a case with very high number of rounds is desirable, I can add one more case to https://github.com/ethereum/execution-spec-tests/blob/main/tests/benchmark/test_worst_compute.py#L995.

🔗 Related Issues or PRs

N/A.

✅ 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
Copy Markdown
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

Yes, I agree, this case would make sense if we didn't constrain the transaction gas limit on Osaka, but now it's kinda meaningless.

We should add a test consumes the full block gas limit using many transactions of which each uses the most amount of rounds that fit in the new ~16M gas limit of the transaction.

@marioevz
Copy link
Copy Markdown
Member

I would even see the point of removing the entire stTimeConsuming folder now, these tests are impossible to fill in Osaka anyway, so they are pretty much useless now.

I made an issue to track the porting of them to benchmark tests here: ethereum/execution-specs#1572

What do you think @chfast ?

@marioevz
Copy link
Copy Markdown
Member

Merging this now, and will reconsider removing stTimeConsuming in a follow up PR.

@marioevz marioevz merged commit f41c57e into ethereum:main Jul 24, 2025
16 checks passed
@marioevz marioevz deleted the no_blake_max_rounds branch July 24, 2025 17:23
@chfast
Copy link
Copy Markdown
Member Author

chfast commented Jul 24, 2025

I would even see the point of removing the entire stTimeConsuming folder now, these tests are impossible to fill in Osaka anyway, so they are pretty much useless now.

I made an issue to track the porting of them to benchmark tests here: ethereum/execution-specs#1572

What do you think @chfast ?

I think the 50000x SHA256 test can also be removed easily because it brings not much new coverage.
The benchmarks for blake2b and sha256 are not perfect at the moment, but this is a separate concern.

Finally, the "sstore combinations" as the exhaustive test for one of the storage net metering EIPs. I don't think it has good replacement currently, but probably should be ported to pytest.

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.

2 participants