Skip to content
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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

EIP-7667: Update gas costs #6932

wants to merge 14 commits into from

Conversation

yerke26
Copy link
Contributor

@yerke26 yerke26 commented Apr 18, 2024

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?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

Optional. Remove if not applicable.

Documentation

Requires documentation update

  • Yes
  • No

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

  • Yes
  • No

If yes, fill in the details here. Remove if not applicable.

Remarks

Optional. Remove if not applicable.

Copy link
Member

@LukaszRozmej LukaszRozmej left a 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.

@yerke26 yerke26 marked this pull request as draft April 18, 2024 19:43
Copy link
Member

@LukaszRozmej LukaszRozmej left a 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.

Copy link
Contributor

@tanishqjasoria tanishqjasoria left a 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

src/Nethermind/Nethermind.Specs/Forks/17_Cancun.cs Outdated Show resolved Hide resolved
Copy link
Member

@LukaszRozmej LukaszRozmej left a 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

@yerke26 yerke26 marked this pull request as ready for review April 23, 2024 12:14
Copy link
Contributor

@tanishqjasoria tanishqjasoria left a 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
Copy link
Contributor

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()
Copy link
Contributor

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

Comment on lines 66 to 82
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));
Copy link
Contributor

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.

Copy link
Contributor

@tanishqjasoria tanishqjasoria left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants