-
Notifications
You must be signed in to change notification settings - Fork 6k
Update EIP-7883: Assume minimal base/mod length of 32 #9855
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
|
✅ All reviewers have approved. |
|
The benchmarks are available in ethereum/execution-spec-tests#1701. This change moves the worst case: from
|
|
Assuming minimal base/mod length of 32 is a great idea and worth to update an EIP and add it. It is covering some edge-case scenarios while having zero impact on common cases. Win-win. |
|
With minimal base/mod length of 32 and min price at 500, our worst case should be |
I restored the minimal cost of 500. The description and benchmark results are also updated. |
MarekM25
left a comment
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.
If we approve this we need to adjust test cases
https://eips.ethereum.org/EIPS/eip-7883#test-cases
Correct. But these test cases are not attached so I cannot do so. |
Here is the pricing changes from our side in EEST if that helps. |
gakonst
left a comment
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.
ACK from Reth side.
|
Makes sense to do it |
|
Can we get this PR merged in? |
lu-pinto
left a comment
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
eth-bot
left a comment
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.
All Reviewers Have Approved; Performing Automatic Merge...
Update the [EIP-7883 "ModExp Gas Cost Increase"](https://eips.ethereum.org/EIPS/eip-7883) to the latest revision: - ethereum/EIPs#9855 - ethereum/EIPs#9969
Update the [EIP-7883 "ModExp Gas Cost Increase"](https://eips.ethereum.org/EIPS/eip-7883) to the latest revision: - ethereum/EIPs#9855 - ethereum/EIPs#9969
From the performance analysis of the modexp precompile after EIP-7883, the increase minimal cost of 500 don't fully cover the worst case of small base/modulus. E.g. the case of the smallest base/modulus of 8 bytes the maximum exponent within the 500 gas cost budget has 880 bits. This case performs significantly worst than the average case (modulus 32 bytes).
We propose to eliminate the pathological cases of base/modulus smaller than 32 bytes by computing the gas cost as if they had 32 byte length. By the Mainnet usage data of the modexp, a modulus smaller than 32 bytes had never been used.
Additional context in the EIP discussion: https://ethereum-magicians.org/t/eip-7883-modexp-gas-cost-increase/22841/6.
The below graphs (of different zoom levels) visualize the proposed changes. They show:
You can notice how slowly rises the
EIP-7883 mod=8bcurve.