-
Notifications
You must be signed in to change notification settings - Fork 167
test(osaka): add edge case test vectors for EIP-7883 MODEXP gas calculation #1993
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
Conversation
Thanks for the PR! Iβve reviewed the CI failure, and it looks like the current gas calculation is incorrect. IMO this is related to an issue in our testing helper. Weβre refactoring eip-7883 tests in a separate PR to resolve the issue. Weβve also added some additional test cases for the gas formula that might be relevant, feel free to take a look here. |
Thank you for the pointers, I will check them out. In the meantime, I realized that the 7883 spec calculates iteration count slightly differently than execution-spec-tests/tests/osaka/eip7883_modexp_gas_increase/spec.py Lines 62 to 63 in 1b43e7a
Essentially, the spec says subtract 1 from exponent bit length whether the bit length is zero or not, but your impl interprets it as "do this only if bit length is positive". This would result in off-by-one iteration counts for large exponents (e.g., 512 byte) whose value is zero. Is this accurate and something that was agreed upon? |
Nice! That a bug in our spec I believe. Will be prioritizing this tmo. Do you have the code you used to generate these vectors? |
For more context here is the EELS spec we are using to fill the tests so there could be inconsistencies there: I see the same within the EELS spec: if bits_part > 0:
bits_part -= 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tests.
I get a lot of gas price mismatches, and some of the inputs do not totally make sense to me, could you please double check if the inputs of the values are actually intended?
If you want to test the vectors, make sure the And you can run the command for test: |
Thanks for the pointer. One more possibly minor issue in spec wording: From EIP 7883
does not make it explicit that we are to take the most significant word (first 32 bytes) of the exponent while computing the U256 masked bit length. My first interpretation was actually to compute the bit length of the last 32 bytes of the exponent. However EELS and clients consistently seem to use the first 32 byte logic. |
Thank for your your detailed review @marioevz and others! I have updated the test vectors accordingly. Hope they are okay. Here is the validation script and summary: Validation script: https://gist.github.com/bshastry/323aacbe95044a9fb31212ccfaf7ec2d |
Curious what the fill command actually does? Does it run the test input against multiple EL clients? I am asking because I got the expected output wrong in an earlier iteration of this PR but the fill command finished successfully leading me to believe the vectors were sound :-) |
Following up on this. It seems that this is acutally correct in EELS and the test framework, based on consensus. The existing gas calculations and tests against clients agree here too. Geth implementation: // Calculate the adjusted exponent length
var msb int
if bitlen := expHead.BitLen(); bitlen > 0 {
msb = bitlen - 1
} Nethermind implementation: uint bitLength = (uint)exponent.BitLen;
if (bitLength > 0)
{
bitLength--;
} The original EIP-198 additionally states this:
So we should consider updating the EIP-7883/2565 psuedocode to be more consitent with the original EIP-198 spec definition. I think in the EIP-2565 spec we should update: elif exponent_length <= 32: iteration_count = exponent.bit_length() - 1
elif exponent_length > 32: iteration_count = (8 * (exponent_length - 32)) + ((exponent & (2**256 - 1)).bit_length() - 1) To the following: elif exponent_length <= 32:
bit_length = exponent.bit_length()
iteration_count = bit_length - 1 if bit_length > 0 else 0
elif exponent_length > 32:
exponent_head_bit_length = (exponent & (2**256 - 1)).bit_length()
iteration_count = (8 * (exponent_length - 32)) + (exponent_head_bit_length - 1 if exponent_head_bit_length > 0 else 0) And for EIP-7883: elif exponent_length <= 32: iteration_count = exponent.bit_length() - 1
elif exponent_length > 32: iteration_count = (16 * (exponent_length - 32)) + ((exponent & (2**256 - 1)).bit_length() - 1) To the following: elif exponent_length <= 32:
bit_length = exponent.bit_length()
iteration_count = bit_length - 1 if bit_length > 0 else 0
elif exponent_length > 32:
exponent_head_bit_length = (exponent & (2**256 - 1)).bit_length()
iteration_count = (16 * (exponent_length - 32)) + (exponent_head_bit_length - 1 if exponent_head_bit_length > 0 else 0) |
The fill command only generates the test fixtures using a reference client implementation (EELS is the default for us). We have some checks within the test framework when generating the tests. That's all! These generated json fixtures must then be executed against an EL client. Consume engine via hive is the defacto standard we use. More context in the docs: https://eest.ethereum.org/main/running_tests/hive/common_options/ |
I've checked it on my PR and it works as expected now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! All filling fine from my side.
Nice! These caught an issue in Reth/Besu (Osaka/7883) with |
ποΈ Description
This PR adds 16 test vectors for EIP-7883 (MODEXP gas increase) to address gaps in edge case coverage. The existing test suite only covers standard cryptographic parameter sizes but misses important corner cases in the gas calculation formula that could lead to consensus differences between EVM implementations.
Key additions:
All gas values have been calculated using the exact formulas from spec.py to ensure accuracy.
π Related Issues or PRs
N/A.
β Checklist
https://eest.ethereum.org/main/getting_started/code_standards/ and
https://eest.ethereum.org/main/dev/precommit/:
uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlint
contri#commit-messages-issue-and-pr-titles - it will be used as the squash commit message and
should start type(scope):.
/ethereum/execution-spec-tests/blob/main/docs/CHANGELOG.md.
/ethereum/execution-spec-tests/blob/main/docs/ directory.
https://eest.ethereum.org/main/tests/ are correctly formatted.
/ethereum/execution-spec-tests/blob/main/docs/writing_tests/post_mortems.md to add an entry
the list.
/ethereum/execution-spec-tests/blob/main/tests/static have been assigned @ported_from marker.