Skip to content

Fix to expmod precompile gas calculation #1176

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

Merged
merged 1 commit into from
Apr 15, 2025
Merged

Conversation

chfast
Copy link
Member

@chfast chfast commented Mar 20, 2025

Fix and refactor gas cost calculation for the expmod precompile.
The issue was that some computations could overflow and the result
was incorrect.

This was found by a fuzzer so some test vectors are included.

Copy link

codecov bot commented Mar 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.89%. Comparing base (4977c94) to head (f322609).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1176   +/-   ##
=======================================
  Coverage   94.89%   94.89%           
=======================================
  Files         171      171           
  Lines       19676    19687   +11     
=======================================
+ Hits        18671    18682   +11     
  Misses       1005     1005           
Flag Coverage Δ
eof_execution_spec_tests 2.53% <0.00%> (-0.01%) ⬇️
ethereum_tests 22.37% <83.72%> (+<0.01%) ⬆️
ethereum_tests_silkpre 17.86% <77.77%> (-0.02%) ⬇️
execution_spec_tests 19.57% <76.74%> (+<0.01%) ⬆️
unittests 92.61% <88.37%> (+0.17%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
test/state/precompiles.cpp 98.99% <100.00%> (+0.01%) ⬆️
test/unittests/precompiles_expmod_test.cpp 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@chfast chfast force-pushed the precompiles/fix_expmod_gas branch 2 times, most recently from 96674ed to e5a4ddb Compare March 21, 2025 15:40
@chfast chfast added bug Something isn't working precompiles Related to EVM precompiles labels Mar 21, 2025
@chfast chfast force-pushed the precompiles/fix_expmod_gas branch from e165b28 to 7b03fc1 Compare March 25, 2025 14:07
@chfast chfast requested review from rodiazet, gumb0 and yperbasis March 25, 2025 14:08
@chfast chfast force-pushed the precompiles/fix_expmod_gas branch 2 times, most recently from 9bf56bc to 4dd801d Compare April 8, 2025 12:21
@chfast chfast marked this pull request as ready for review April 8, 2025 12:23
@chfast chfast force-pushed the precompiles/fix_expmod_gas branch 2 times, most recently from 992144c to 45b766f Compare April 14, 2025 08:51
Copy link
Contributor

@rodiazet rodiazet left a comment

Choose a reason for hiding this comment

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

Ok overall, One comment added.

const auto base_len = be::unsafe::load<uint256>(&input_header[0]);
const auto exp_len = be::unsafe::load<uint256>(&input_header[32]);
const auto mod_len = be::unsafe::load<uint256>(&input_header[64]);
const auto adjusted_len = [input](size_t offset, uint32_t len) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's valid only for calculation of adjusted exp len. Should we call it adjusted_exp_len?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added prefix calc_ for helper function to distinguish it from values. And renamed this to calc_adjusted_exp_len().

Fix and refactor gas cost calculation for the expmod precompile.
The issue was that some computations could overflow and the result
was incorrect.

This was found by a fuzzer so some test vectors are included.
@chfast chfast force-pushed the precompiles/fix_expmod_gas branch from 45b766f to f322609 Compare April 15, 2025 10:09
@chfast chfast merged commit 1d574cf into master Apr 15, 2025
25 checks passed
@chfast chfast deleted the precompiles/fix_expmod_gas branch April 15, 2025 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working precompiles Related to EVM precompiles
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants