Skip to content

tests(benchmark): expand worst case modexp benchmarks #1780

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

LouisTsai-Csie
Copy link
Collaborator

@LouisTsai-Csie LouisTsai-Csie commented Jun 20, 2025

🗒️ Description

The test cases are divided into two parts: the first tests the upper bound of modexp based on EIP-7823, and the second ports cases from Nethermind. For the latter one, I’ve added a reference to each individual case.

Script for converting the test case: https://gist.github.com/648207a8651d3319ea6dd06e9c977422.git

This PR ports the modexp test cases from Nethermind, you can find the corresponding code here.

I wrote a script to generate test cases, this is my process:

  1. I port all the test case from Nethermind's codebase to this text file.
  2. Then I use this script to create the correct format we use in EEST, please put your script and the text file in the same folder and run the script: python script.py
  3. It will create a file: output.txt and this is the result.

🔗 Related Issues

Issue #1453

✅ Checklist

  • All: Set appropriate labels for the changes.
  • Tests: Included the type and version of evm t8n tool used to locally execute test cases: e.g., ref with commit hash or geth 1.13.1-stable-3f40e65.
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.

@LouisTsai-Csie LouisTsai-Csie marked this pull request as draft June 20, 2025 14:55
@LouisTsai-Csie LouisTsai-Csie marked this pull request as ready for review June 23, 2025 12:30
@LouisTsai-Csie LouisTsai-Csie marked this pull request as draft June 24, 2025 20:13
@LouisTsai-Csie LouisTsai-Csie self-assigned this Jun 24, 2025
@LouisTsai-Csie LouisTsai-Csie added feature:zkevm scope:tests Scope: Changes EL client test cases in `./tests` labels Jun 24, 2025
@LouisTsai-Csie
Copy link
Collaborator Author

@kevaundray I’ve checked with the Nethermind team regarding the ported test. Could you help confirm whether we can change the ending from "00" to "02" for the even case?

Based on our previous discussion, using "00" as the ending might get stripped by calldata = bytes(mod_exp_input).rstrip(b"\x00"), which could cause issues.

Example:

calldata = bytes(mod_exp_input).rstrip(b"\x00")

@LouisTsai-Csie LouisTsai-Csie marked this pull request as ready for review June 27, 2025 20:49
@LouisTsai-Csie LouisTsai-Csie marked this pull request as draft July 1, 2025 14:16
@spencer-tb spencer-tb changed the title tests(zkevm): expand worst case modexp benchmarks tests(benchmark): expand worst case modexp benchmarks Jul 1, 2025
@marioevz
Copy link
Member

marioevz commented Jul 1, 2025

#1804 got merged! Please rebase the branch and move the tests to the appropriate folder.

@marioevz
Copy link
Member

@kevaundray I’ve checked with the Nethermind team regarding the ported test. Could you help confirm whether we can change the ending from "00" to "02" for the even case?

Based on our previous discussion, using "00" as the ending might get stripped by calldata = bytes(mod_exp_input).rstrip(b"\x00"), which could cause issues.

Example:

calldata = bytes(mod_exp_input).rstrip(b"\x00")

I don't think this is a problem because modexp is supposed to right-pad the input with zeros if the sum of the lengths provided is greater than the calldata input.

It might be better to rollback this change IMO.

@marioevz
Copy link
Member

I could not make the conversion script work or rather I don't know what the inputs should be, could you share more info on the input data?

@LouisTsai-Csie LouisTsai-Csie marked this pull request as ready for review July 22, 2025 08:39
@LouisTsai-Csie
Copy link
Collaborator Author

Thanks @marioevz, I've updated my script and the description, you could find the details in the PR description now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature:benchmark feature:zkevm scope:tests Scope: Changes EL client test cases in `./tests`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants