Skip to content

Conversation

LouisTsai-Csie
Copy link
Collaborator

@LouisTsai-Csie LouisTsai-Csie commented Jul 21, 2025

🗒️ Description

EIP-7883: ModExp Gas Cost Increase

This EIP changes the gas cost, so it falls under this category in the checklist. While this category is meant specifically for gas cost changes, I’ve added a broader set of scenarios, similar to what would be done for a new precompile, since the existing tests from Byzantium are incomplete.

Call contexts

  • CALL / DELEGATECALL / STATICCALL / CALLCODE
  • Transaction Entry-point
  • Initcode call: IMO this is unnecessary, since we still need to use *CALL to trigger the precompile in the create/create2 initcode, it is exactly the same testing scenario as the call context one.
  • Precompile as Set-code Delegated Address: please check this in eip-7702 test, it is not located in eip-7883 test.

Inputs

  • precompile/test/inputs/valid
  • precompile/test/inputs/valid/boundary
  • precompile/test/inputs/valid/crypto
  • precompile/test/inputs/all_zeros
  • precompile/test/inputs/max_values
  • precompile/test/inputs/invalid
  • precompile/test/inputs/invalid/crypto
  • precompile/test/inputs/invalid/corrupted

Value Transfer

  • Minimum Fee Precompile
  • No-Fee Precompile: Do not need this one, as Modexp is not no-fee precompile

Out-of-bounds checks

  • precompile/test/out_of_bounds/max
  • precompile/test/out_of_bounds/max_plus_one

Input Lengths

  • Zero-length Input
  • Static Required Input Length: The input length is dynamic
  • Dynamic Required Input Length

Gas usage

  • Constant Gas Cost: We do not need this one, as the gas cost is dynamic
  • Variable Gas Cost: See analysis below
  • Excessive Gas: Already have this in benchmark test

Fork transition

  • Pre-fork Block Call
  • Cold/Warm Precompile Address State: We do not need this one as the Modexp is not a new precompile.

🔗 Related Issues or PRs

Issue #1791, #1790, #1971

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

@LouisTsai-Csie LouisTsai-Csie changed the title refactor(eip7883): update vector input structure feat(tests): enhance eip7883 test coverage Jul 21, 2025
@LouisTsai-Csie LouisTsai-Csie force-pushed the enhance-eip7823-coverage branch from 016bf91 to 4d57f68 Compare July 21, 2025 07:45
@LouisTsai-Csie LouisTsai-Csie self-assigned this Jul 21, 2025
@LouisTsai-Csie LouisTsai-Csie added fork:osaka Osaka hardfork type:test Type: Add/refactor fw unit tests; no fw or el client test case changes labels Jul 22, 2025
@LouisTsai-Csie LouisTsai-Csie marked this pull request as ready for review July 22, 2025 15:09
@LouisTsai-Csie LouisTsai-Csie force-pushed the enhance-eip7823-coverage branch from 256b98c to ed33e8d Compare July 23, 2025 06:48
@LouisTsai-Csie LouisTsai-Csie force-pushed the enhance-eip7823-coverage branch 3 times, most recently from 5c7c640 to 87c8cac Compare July 30, 2025 04:03
@marioevz marioevz requested a review from spencer-tb July 31, 2025 00:12
@LouisTsai-Csie LouisTsai-Csie force-pushed the enhance-eip7823-coverage branch from fce1649 to 13ab385 Compare July 31, 2025 03:40
@LouisTsai-Csie
Copy link
Collaborator Author

I didn’t create a separate PR for issue #1971, as it depends on the infrastructure introduced in this PR.

There are 37 test cases in the legacy modexpFiller.json test. Most of them have been ported, but the following cases are still pending and require further analysis:

  • Case 2: Would parse a base length of 0, a modulus length of 32, and an exponent length of 2256 - 1, where the base is empty, the modulus is 2256 - 2 and the exponent is (2256 - 3) * 256(2**256 - 33) (yes, that's a really big number). It would then immediately fail, as it's not possible to provide enough gas to make that computation.
  • Case 28: base length 4TiB
  • Case 29: exp length 4TiB; returns 0 because mod is zero
  • Case 30: base and mod have zero-length. exp's length is 2^255. Since mod is zero, the result should be zero.
  • Case 36: the input found on 10 Oct. 2017 that overflows the gas calculation
  • Case 37: input found in July 2022, overflows the gas calculation
  1. Based on the description of case 2, 28, 29 and 30, they will fail as eip7623 introduces the upper bound for each field, we already have similar test in test_modexp_invalid_inputs.
  2. For case 36, 37, I am checking if it is valid after Fusaka.

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.

Thanks for working on this. Just some small comments for now.

Could you double check my understanding for my comment here:
#1929 (comment)

@LouisTsai-Csie LouisTsai-Csie force-pushed the enhance-eip7823-coverage branch from 6e7819f to ec39341 Compare August 12, 2025 07:48
@LouisTsai-Csie LouisTsai-Csie force-pushed the enhance-eip7823-coverage branch from c6b83d4 to 7bd5794 Compare August 21, 2025 08:17
@LouisTsai-Csie
Copy link
Collaborator Author

These test works as expected locally but CI fails. I've checked the error message, and it seems like all the tests are ignored?

@spencer-tb spencer-tb added scope:tests Scope: Changes EL client test cases in `./tests` type:feat type: Feature and removed type:test Type: Add/refactor fw unit tests; no fw or el client test case changes labels Aug 21, 2025
@spencer-tb
Copy link
Contributor

Validated coverage script in #2071.

@spencer-tb spencer-tb merged commit 4e754c4 into ethereum:main Aug 22, 2025
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fork:osaka Osaka hardfork scope:tests Scope: Changes EL client test cases in `./tests` type:feat type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants