-
Notifications
You must be signed in to change notification settings - Fork 510
eip-7883 implementation #8489
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
base: master
Are you sure you want to change the base?
eip-7883 implementation #8489
Conversation
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.
Could you add tests?
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.
Would be good to test base/modulo gas bump and exp gas bump. Right now only an increase of minimal gas is tested.
Structure of input to DataGasCost
is:
1st 32 bytes: base length
2nd 32 bytes: exp length
3rd 32 bytes: modulo length
We need test cases with:
- base length <= 32 and >32
- exp length <= 32 and >32
- modulo length <= 32 and >32
Also, calling Run
with 96 zero bytes doesn't make much sense, as it indicates the total data length is zero - meaning there's no actual data and no computation to perform, so there's nothing to throw.
Added tests for all cases |
bool overflow = UInt256.MultiplyOverflow(complexity, iterationCount, out UInt256 result); | ||
result /= 3; | ||
return result > long.MaxValue || overflow ? long.MaxValue : Math.Max(200L, (long)result); | ||
return result > long.MaxValue || overflow ? long.MaxValue : Math.Max(releaseSpec.IsEip7883Enabled ? 500L : 200L, (long)result); |
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.
should we create constants in GasCostOf for these values?
@@ -209,7 +210,8 @@ private static UInt256 CalculateIterationCount(UInt256 exponentLength, UInt256 e | |||
bitLength--; | |||
} | |||
|
|||
bool overflow = UInt256.MultiplyOverflow((exponentLength - 32), 8, out UInt256 multiplicationResult); | |||
var multiplier = (UInt256)(isEip7883Enabled ? 16 : 8); |
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.
should we create constants in GasCostOf for these values?
{ | ||
UInt256 maxLength = UInt256.Max(baseLength, modulusLength); | ||
UInt256.Mod(maxLength, 8, out UInt256 mod8); | ||
UInt256 words = (maxLength / 8) + ((mod8.IsZero) ? UInt256.Zero : UInt256.One); | ||
return words * words; | ||
return maxLength > 32 && isEip7883Enabled ? 2 * words * words : words * words; |
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.
should we create constants in GasCostOf for these values? (the 2x multiplier)
Eip-7883 implementation
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Optional. Remove if not applicable.
Documentation
Requires documentation update
If yes, link the PR to the docs update or the issue with the details labeled
docs
. Remove if not applicable.Requires explanation in Release Notes
If yes, fill in the details here. Remove if not applicable.
Remarks
Optional. Remove if not applicable.