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
Update EIP-4762: reworked gas schedule from interop #8550
Update EIP-4762: reworked gas schedule from interop #8550
Changes from 4 commits
a8cfb56
06b7248
acec5e6
2e609fd
7af9770
271a9e4
508f0ab
ea647e7
96e3281
29da746
65aafe5
9e5644f
e785fdf
427e408
285a2e7
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.
i think this is covered below?
https://github.com/ethereum/EIPs/pull/8550/files#diff-cd2aaa222faeb6081a41605386a509b3c4839285cd15f1b0ebe009318707914eR126
Check failure on line 63 in EIPS/eip-4762.md
GitHub Actions / Markdown Linter
Fenced code blocks should be surrounded by blank lines [Context: "```"]
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.
Shouldn't we also touch the
VERSION
thusBASIC_DATA_LEAF_KEY
?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.
TODO: add version
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.
i think this is also duplicated - https://github.com/ethereum/EIPs/pull/8550/files#diff-cd2aaa222faeb6081a41605386a509b3c4839285cd15f1b0ebe009318707914eL143
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.
[Edit: comment quoting code is not correct, I meant L173
(tx.target, 0, BASIC_DATA_LEAF_KEY)
]If we have a tx (non-value-bearing) targeting a precompile or system contract, we don't need to touch BASIC_DATA. (i.e: stateless client knows the code-size)
We can still argue that this rule should be avoided to avoid a border case. But considering we didn't do any similar simplification for precompiles/system contracts in other places in the EIP (i.e. we avoid adding in the witness for them as much as possible), then maybe not doing it here is actually adding a border case (we have to remember this is an exception), not removing it.
No strong opinions, just surfacing this.
cc @gballet @g11tech @gabrocheleau @tanishqjasoria @matkt
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.
I can't add this comment in the intended line because there wasn't a change there.
I'm referring to lines L218-L220 and L226.
The intention in both places to say "skip it" is for gas cost charging. But the thing being skipped is also adding it to the underlying access list that are used for the witness creation. If those things also skip adding them to the list, then I think the witness wouldn't be correct?
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.
but the adding of stuff in the access list is done in previous sections, whereas this is specific to the charging of costs.
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.
Want to use
*CALL
to also contemplateAUTHCALL
? (Also note a missing space afteror
)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 also clarify that the contract creation complete gas cost should be charged before adding the 1/64 gas after the creation is completed.
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.
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.
Should we update CODE_KECCAK_LEAF_KEY to
1
instead of leaving1
and2
empty?