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

LFVM: move addressInAccessList to getAccessCost #730

Merged
merged 8 commits into from
Sep 16, 2024

Conversation

facuMH
Copy link
Contributor

@facuMH facuMH commented Sep 4, 2024

This PR removes the function addressInAccessList and reworks its functionality into getAccessCost. It also adds comprehensive documentation for the values used, and a test for the different cases.

Note: static call prices after berlin should be 0, and warm cost should be applied in dynamic cost calcualtion (https://eips.ethereum.org/EIPS/eip-2929). This changes are also applied in this PR.

@facuMH facuMH added this to the Release Ready LFVM milestone Sep 4, 2024
@facuMH facuMH self-assigned this Sep 4, 2024
@facuMH facuMH force-pushed the facundo/lfvm-gas-address-in-list branch from cb264ee to e343d36 Compare September 4, 2024 14:05
LuisPH3
LuisPH3 previously approved these changes Sep 5, 2024
go/interpreter/lfvm/gas_test.go Outdated Show resolved Hide resolved
go/interpreter/lfvm/gas_test.go Outdated Show resolved Hide resolved
@facuMH facuMH force-pushed the facundo/lfvm-gas-address-in-list branch 2 times, most recently from 96fd6ca to 6cf6dd6 Compare September 5, 2024 09:37
@facuMH facuMH requested a review from LuisPH3 September 5, 2024 09:37
simonlechner
simonlechner previously approved these changes Sep 5, 2024
Copy link
Contributor

@simonlechner simonlechner 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, just one remark regarding readability.

go/interpreter/lfvm/gas_test.go Outdated Show resolved Hide resolved
LuisPH3
LuisPH3 previously approved these changes Sep 5, 2024
@facuMH facuMH dismissed stale reviews from LuisPH3 and simonlechner via cc7d19e September 6, 2024 12:03
@facuMH facuMH force-pushed the facundo/lfvm-gas-address-in-list branch 2 times, most recently from cc7d19e to f1c0567 Compare September 6, 2024 12:03
@facuMH facuMH force-pushed the facundo/lfvm-gas-address-in-list branch from f1c0567 to 7cfbdf6 Compare September 6, 2024 12:18
@facuMH facuMH changed the title LFVM: test for addressInAccessList LFVM: test for calculateAccessCost Sep 6, 2024
Copy link
Collaborator

@HerbertJordan HerbertJordan left a comment

Choose a reason for hiding this comment

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

I would say inlining this function into genericCall is the best way forward.

go/interpreter/lfvm/gas.go Outdated Show resolved Hide resolved
go/interpreter/lfvm/gas_test.go Outdated Show resolved Hide resolved
go/interpreter/lfvm/gas_test.go Outdated Show resolved Hide resolved
go/interpreter/lfvm/gas_test.go Outdated Show resolved Hide resolved
@facuMH facuMH force-pushed the facundo/lfvm-gas-address-in-list branch from 7cfbdf6 to 2c6e0e6 Compare September 6, 2024 14:12
@facuMH facuMH changed the title LFVM: test for calculateAccessCost LFVM: move addressInAccessList into genericCall Sep 6, 2024
go/interpreter/lfvm/instructions.go Outdated Show resolved Hide resolved
@LuisPH3
Copy link
Contributor

LuisPH3 commented Sep 9, 2024

Relates to #616

LuisPH3
LuisPH3 previously approved these changes Sep 9, 2024
go/interpreter/lfvm/instructions.go Show resolved Hide resolved
go/interpreter/lfvm/instructions.go Show resolved Hide resolved
Copy link
Collaborator

@HerbertJordan HerbertJordan 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 how this PR has simplified the gas cost handling and aligned it with the online reference specification. Will approve when the remaining remarks are covered.

go/interpreter/lfvm/instructions.go Outdated Show resolved Hide resolved
go/interpreter/lfvm/instructions_test.go Show resolved Hide resolved
HerbertJordan
HerbertJordan previously approved these changes Sep 13, 2024
@facuMH facuMH merged commit dd9c7d6 into main Sep 16, 2024
5 checks passed
@facuMH facuMH deleted the facundo/lfvm-gas-address-in-list branch September 16, 2024 07:10
facuMH added a commit that referenced this pull request Sep 16, 2024
* add gas selfdestruct test

* remove selfdestruct gas functions. integrate them into operation func

* improve constants comments and remove unused gas constants

* make functions for self destruction costs and tests them

* add test for all selfdestruct dynamic costs

* changes requested in review. add and use getAccessCost from #730

* fix typo and rephrase revision check

* fix comment phrasing
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.

4 participants