Skip to content

Conversation

acuarica
Copy link
Contributor

@acuarica acuarica commented Oct 4, 2025

πŸ—’οΈ Description

This PR fixes the storage slot of the actual value in the test.

In execute remote mode, it used NUMBER as the storage slot, but the expected value is stored in slot 1. This causes problems when running tests with execute remote where NUMBER cannot be ensured to be 1.

πŸ”— Related Issues or PRs

Fixes #2255.

βœ… 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.

Signed-off-by: Luis Mastrangelo <luis@swirldslabs.com>
@LouisTsai-Csie
Copy link
Collaborator

This fix resolves the issue in execute mode, but it will fail under the fill mode in our framework. Please take a look at the failing case test_blobbasefee_during_fork, where the block number is used as the storage key.

 pre-set storage just to make sure we detect the change
        code_caller_post_storage[block_number] = 0 if timestamp < 15_000 else 1

That is the reason for the failing CI, you could also run it locally via

uv run fill -v tests/cancun/eip7516_blobgasfee/test_blobgasfee_opcode.py::test_blobbasefee_during_fork --fork Osaka --clean

Signed-off-by: Luis Mastrangelo <luis@swirldslabs.com>
@acuarica
Copy link
Contributor Author

acuarica commented Oct 6, 2025

Hi @LouisTsai-Csie, thanks for taking a look. I've managed to solve the issue essentially by parameterizing the expected value slot. Happy to explore other solutions if required.

@acuarica
Copy link
Contributor Author

acuarica commented Oct 6, 2025

However, I had to skip this gas validation, otherwise the test will fail in execute mode

# Perform gas validation if required for benchmarking
# Ensures benchmark tests consume exactly the expected gas
if not self.skip_gas_used_validation and self.expected_benchmark_gas_used is not None:

Am I missing something?

@LouisTsai-Csie
Copy link
Collaborator

Gas usage validation is intended for benchmark mode (see docs).
You can temporarily use skip_gas_used_validation to bypass it, we’ll later limit this check to benchmark mode after this wip issue is done

@LouisTsai-Csie
Copy link
Collaborator

LouisTsai-Csie commented Oct 6, 2025

For the failing evmone CI issue, I checked the report and found an LBC. The root cause is the use of a different storage key. In the previous setup, Op.NUMBER was used to manage the storage slot, whereas the latest approach instead uses slot 1. As a result, evmone detects the discrepancy and reports it as an LBC. This is fine so nothing else needs to be changed.

Report data
Screenshot 2025-10-06 at 4 10 24β€―PM

LBC Found
Screenshot 2025-10-06 at 4 11 06β€―PM

@LouisTsai-Csie
Copy link
Collaborator

A recent merged PR might help you with the gas validation issue. For non benchmark mode, it would not apply the gas validation

@acuarica
Copy link
Contributor Author

acuarica commented Oct 6, 2025

Thanks for clarifying #2266 (comment), and in general for taking the time to review this.

Copy link
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.

Thanks for this! Just one suggestion that might simplify and fix the issues found in the previous review.

Signed-off-by: Luis Mastrangelo <luis@swirldslabs.com>
Copy link
Collaborator

@LouisTsai-Csie LouisTsai-Csie left a comment

Choose a reason for hiding this comment

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

The CI is failing but i think it's fine, it is due to the changed testing method.

Copy link
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.

LGTM, thanks!

Coverage CI issue is because we swapped opcodes, so it's ok to merge.

@marioevz marioevz merged commit c06786c into ethereum:main Oct 7, 2025
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment