-
Notifications
You must be signed in to change notification settings - Fork 432
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
EIP-7667: Update gas costs #6932
base: master
Are you sure you want to change the base?
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.
We can't change cost on historical blocks, this needs to be decided dependent on EIP activation. Check ReleaseSpecExtensions
class.
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.
Need to add proper way of parsing/converting from json, but otherwise looks good.
I think BLAKE2 precompile cost is still missing.
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.
you can refer to changes in Nethermind.Specs
here to understand the changes that are needed for parsing json.
for BLAKE2 gas, update BLAKE2_GFROUND
cost.
ref: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-152.md
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.
Looks good, could use some 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.
we need some tests here - you can have a look at Evm.Tests/Eip152Test.cs
it will give you some idea on how to write some tests for this change
|
||
namespace Nethermind.Evm.Test; | ||
|
||
public class Eip7667Spec : Cancun |
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.
just rename this to Prague and move this to Nethermind.Specs/Forks
public class Eip7667Tests : VirtualMachineTestsBase | ||
{ | ||
[SetUp] | ||
public void SetUp() |
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.
you can remove this - the base class function will anyways be called
public void Keccak256OpDifference() | ||
{ | ||
byte[] code = Prepare.EvmCode | ||
.PushData(32) | ||
.PushData(0) | ||
.Op(Instruction.KECCAK256) | ||
.STOP() | ||
.Done; | ||
|
||
TestAllTracerWithOutput resultEipDisabled = Execute((ForkActivation)1, code); | ||
Setup(); | ||
TestAllTracerWithOutput resultEipEnabled = Execute((ForkActivation)2, code); | ||
|
||
long gasDifference = Eip7667Spec.Instance.GetSha3Cost() - Cancun.Instance.GetSha3Cost() | ||
+ Eip7667Spec.Instance.GetSha3WordCost() - Cancun.Instance.GetSha3WordCost(); | ||
|
||
Assert.That(resultEipEnabled.GasSpent - resultEipDisabled.GasSpent, Is.EqualTo(gasDifference)); |
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.
just have a look at how other test works regarding checking values before and after. implement this similarly. EIP3651Tests can be a good reference for that.
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
Fixes Closes Resolves #
#6924
Changes
Update gas cost for opcodes and precompiles mentioned in EIP-7667
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.