Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Istanbul: EIP-2200 net gas metering for SSTORE #590
Istanbul: EIP-2200 net gas metering for SSTORE #590
Changes from 6 commits
f28c552
9a73667
c969e59
28abb65
45de15c
d6fac6e
0264f70
789c8ed
eb68617
d6a7e84
f2215c0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Hmm, the numbers in the test cases differ to what is listed in the EIP? 🤔
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.
Hm, these test cases have been added after I created the PR: ethereum/EIPs@01b66af
It seems the EIP document has also been modified. Will have to look into it
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.
The parameters in the EIP are also named very much differently than here (and in Go), did this also change? Or is this an intentional deviation to be in line with the old Constantinople naming?
If this changed we should likely do the extra round sigh on the Common library and change the parameter names (and eventually delete some?).
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.
So the test vectors that were recently added to the EIP are for an older version of the EIP which assumed cost of SLOAD to be 200 instead of 800. See comment: ethereum/EIPs#2200 (comment)
About the param names, as long as we're conforming to test cases it should be fine. I don't think every client uses the same param names. The EIP is also somewhat under-specified. I'm more confident in geth's code as that's what eventually makes it into mainnet. But we can wait until the EIP's PR is merged, hopefully by then things will be more clear.