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.

Sign up for GitHub

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

Jump to bottom

LFVM: test EIP2929 warm/cold acceses charging #808

Merged
merged 7 commits into from
Sep 27, 2024
Merged

LFVM: test EIP2929 warm/cold acceses charging #808

merged 7 commits into from
Sep 27, 2024

Conversation

facuMH
Copy link
Contributor

@facuMH facuMH commented Sep 25, 2024 •
edited
Loading

Part of #751

This PR fixes and tests many of the changes introduced in EIP-2929 (https://eips.ethereum.org/EIPS/eip-2929) :

  • Fixes static costs of BALANCE, EXTCODECOPY, EXTCODESIZE and EXTCODEHASH to zero. All gas for these operations should be dynamically calculated.
  • removes gasEip2929AccountCheck because it was duplicated functionality from getAccessCost, which is already tested.
  • adds test checking for the extra charging of warm/cold accesses in Berlin and after, in the relevant opeations.

NOTE: SelfDestruct is a special case and has its own pre-existing tests.

  • ct

@facuMH facuMH added this to the Release Ready LFVM milestone Sep 25, 2024
@facuMH facuMH self-assigned this Sep 25, 2024
@LuisPH3 LuisPH3 mentioned this pull request Sep 25, 2024
Merged
LuisPH3
LuisPH3 requested changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@LuisPH3 LuisPH3 left a comment

Choose a reason for hiding this comment

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

I like the tests, they seem targeting what we listed.
Nevertheless have a look at the following comments:

go/interpreter/lfvm/instructions_test.go Outdated Show resolved Hide resolved
go/interpreter/lfvm/instructions_test.go Outdated Show resolved Hide resolved
go/interpreter/lfvm/instructions_test.go Outdated Show resolved Hide resolved
go/interpreter/lfvm/instructions.go Outdated Show resolved Hide resolved
@facuMH facuMH requested a review from LuisPH3 September 25, 2024 15:23
LuisPH3
LuisPH3 previously approved these changes Sep 26, 2024
View reviewed changes
@facuMH facuMH removed the request for review from HerbertJordan September 26, 2024 10:03
LuisPH3
LuisPH3 requested changes Sep 26, 2024
View reviewed changes
Copy link
Contributor

@LuisPH3 LuisPH3 left a comment

Choose a reason for hiding this comment

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

Later changes made comments difficult to read

go/interpreter/lfvm/instructions_test.go Show resolved Hide resolved
go/interpreter/lfvm/instructions_test.go Outdated Show resolved Hide resolved
HerbertJordan
HerbertJordan previously approved these changes Sep 26, 2024
View reviewed changes
go/interpreter/lfvm/instructions.go Outdated Show resolved Hide resolved
go/interpreter/lfvm/instructions.go Show resolved Hide resolved
go/interpreter/lfvm/instructions_test.go Outdated Show resolved Hide resolved
go/interpreter/lfvm/instructions_test.go Outdated Show resolved Hide resolved
go/interpreter/lfvm/instructions_test.go Outdated Show resolved Hide resolved
go/interpreter/lfvm/instructions_test.go Outdated Show resolved Hide resolved
@facuMH facuMH force-pushed the facundo/lfvm-test-eip2929 branch 2 times, most recently from ab22dd8 to 86f42e5 Compare September 27, 2024 07:53
@facuMH facuMH merged commit e2e5ac5 into main Sep 27, 2024
5 checks passed
@facuMH facuMH deleted the facundo/lfvm-test-eip2929 branch September 27, 2024 09:44
@LuisPH3
Copy link
Contributor

LuisPH3 commented Oct 17, 2024

part of #690

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LuisPH3 LuisPH3 LuisPH3 approved these changes

@HerbertJordan HerbertJordan HerbertJordan left review comments

@simonlechner simonlechner Awaiting requested review from simonlechner

Assignees

@facuMH facuMH

Labels
None yet
Projects
None yet
Milestone
Release Ready LFVM
Development

Successfully merging this pull request may close these issues.
4 participants
@facuMH @LuisPH3 @HerbertJordan @simonlechner