Skip to content

Conversation

bshastry
Copy link
Contributor

@bshastry bshastry commented Sep 8, 2025

🗒️ Description

This PR adds additional zero edge cases to the ModExp (EIP-7883) gas cost tests. These new test cases improve coverage by testing various combinations of empty and zero-valued inputs for base, exponent, and modulus parameters.

The changes specifically add test cases Z1-Z7 and Z11-Z15 to test edge cases where:

  • Empty strings are used for base, exponent, or modulus
  • Large zero-filled inputs (1024 bytes) are tested
  • Combinations of empty and zero values are explored
  • Edge cases with single non-zero bytes at the end of otherwise zero-filled inputs

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

@spencer-tb
Copy link
Contributor

cc-ing @LouisTsai-Csie

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.

Thanks a lot! Could you please help update the table below:

There is a script that might help: #1929 (comment)

@bshastry
Copy link
Contributor Author

bshastry commented Sep 9, 2025

Thanks a lot! Could you please help update the table below:

There is a script that might help: #1929 (comment)

Thank you for your review. I have updated the table. The coverage framework in the table is very good. I just complemented it with some edge cases wrt implementation (spec coverage is excellent).

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.

No problem from my side! Let's wait for final review and get merge. cc @spencer-tb

@LouisTsai-Csie
Copy link
Collaborator

@bshastry I am currently working on some refactoring PR for eip7823 & eip7883, it will give you an overview of test coverage like this.

Additionally, I update the testing infra in the PR for eip7823, to ensure all the output and gas usage is correct. Please feel free to add more cases to test the upper bound.

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.

Amazing!! Thanks 🫡

@spencer-tb spencer-tb added scope:tests Scope: Changes EL client test cases in `./tests` type:feat type: Feature labels Sep 9, 2025
@spencer-tb spencer-tb changed the title test(eip7883): Add zero edge cases for ModExp gas cost tests feat(tests): add zero edge cases for modexp eip7883 gas cost tests Sep 9, 2025
@spencer-tb spencer-tb merged commit b1441c9 into ethereum:main Sep 9, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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