fix(core/vm): triple modexp cost post-cancun #32231#2319
fix(core/vm): triple modexp cost post-cancun #32231#2319gzliudan merged 1 commit intoXinFinOrg:dev-upgradefrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
gzliudan
requested review from
AnilChinchawale,
anunay-xin,
benjamin202410,
liam-lai and
wanwiset25
There was a problem hiding this comment.
Pull request overview
Adjusts the gas pricing logic for the ModExp (big modular exponentiation) precompile to apply a different divisor rule when the eip7883 repricing mode is enabled, aligning pricing with the intended post-fork behavior described in ethereum#32231.
Changes:
- In
bigModExp.RequiredGas, skips theGQUADDIVISORdivision-by-3 step wheneip7883is enabled (effectively increasing gas vs the prior behavior in that mode).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 2. Different divisor (`GQUADDIVISOR`) (3) | ||
| gas.Div(gas, big3) | ||
| if !c.eip7883 { | ||
| gas.Div(gas, big3) | ||
| } |
There was a problem hiding this comment.
The inline comment says the divisor (GQUADDIVISOR) is 3, but with the new conditional the divisor becomes 1 when eip7883 is enabled (division is skipped). Please update the comment (and/or factor the divisor into a named variable) so the documented formula matches the implemented behavior for both modes.
| if !c.eip7883 { | ||
| gas.Div(gas, big3) | ||
| } |
There was a problem hiding this comment.
This change only takes effect when c.eip7883 is true. In this repo, eip7883 is currently only enabled in PrecompiledContractsOsaka, while the network constants schedule Cancun but keep Osaka at math.MaxInt64 (unscheduled). If the intent is to fix modexp pricing post-Cancun (per PR title), this condition will never be hit on mainnet/testnet; consider wiring the eip7883 mode to the Cancun (or intended) fork rules instead of Osaka-only.
| if !c.eip7883 { | ||
| gas.Div(gas, big3) | ||
| } |
There was a problem hiding this comment.
The PR changes modexp gas accounting, but the existing precompile tests don't assert RequiredGas (they only run with whatever gas RequiredGas returns). Please add/extend unit tests to assert the expected RequiredGas for at least a few representative vectors (e.g., reuse the Gas field already present in core/vm/testdata/precompiles/modexp_eip7883.json) so this repricing can’t regress silently.
Proposed changes
Ref: ethereum#32231
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which parts of the codebase does this PR touch?
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that