-
Notifications
You must be signed in to change notification settings - Fork 877
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-7623 #8093
EIP-7623 #8093
Conversation
Signed-off-by: Daniel Lehrner <[email protected]>
Signed-off-by: Daniel Lehrner <[email protected]>
Signed-off-by: Daniel Lehrner <[email protected]>
348ad34
to
2d2cfaf
Compare
Signed-off-by: Daniel Lehrner <[email protected]>
Signed-off-by: Daniel Lehrner <[email protected]>
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 there's a bug in when the floor gets applied for tx processing, see comments.
I like the way you structured the refactor 👍
Perhaps not a functional issue, but I'm a little confused by the Long boundary checks:
- Their use appears inconsistent which makes me wonder if one or more of them were left in by accident?
- I understand checking MAX_VALUE before an add (although doesn't clampedAdd already handle this?) but I don't understand why we return early if the intermediate cost == MIN_VALUE. In the intermediate calculations, won't the subsequent addition usually mean cost > MIN_VALUE?
* | ||
* @param payload the call data payload | ||
* @param isContractCreation whether the transaction is a contract creation | ||
* @param baselineGas how much gas is used by access lists and code delegations |
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.
nit: baselineGas
isn't the most descriptive name but this is a hard one to name. IMO we should try to match the spec with our names (which is still in flux so good luck 😆)
cc. @jflo as just noticed you had the opposite opinion 😅 #7997 (comment)
if (cost == Long.MIN_VALUE || cost == Long.MAX_VALUE) { | ||
return cost; | ||
} |
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.
Why do we do check MIN_VALUE and return here, potentially bypassing adding more to the cost below?
To me, this doesn't appear to be equivalent behaviour to https://github.com/hyperledger/besu/blob/2aadbfcb0a6c22734ef3ae8cc2ae3823512c5419/evm/src/main/java/org/hyperledger/besu/evm/gascalculator/FrontierGasCalculator.java
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.
@daniellehrner this question is still outstanding but I don't think it should block the PR
evm/src/main/java/org/hyperledger/besu/evm/gascalculator/PragueGasCalculator.java
Outdated
Show resolved
Hide resolved
Do we need to do anything different to support EthEstimateGas, etc? |
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: Simon Dudley <[email protected]>
…pplied Signed-off-by: Simon Dudley <[email protected]>
Signed-off-by: Simon Dudley <[email protected]>
Signed-off-by: Simon Dudley <[email protected]>
…hyperledger#8115) Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Simon Dudley <[email protected]>
9411a7e
to
74685b3
Compare
Signed-off-by: Simon Dudley <[email protected]>
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.
think it looks good. prob needs a changelog entry
Signed-off-by: Simon Dudley <[email protected]>
Signed-off-by: Simon Dudley <[email protected]>
PR description
Implements EIP-7623 (calldata cost increase)
https://eips.ethereum.org/EIPS/eip-7623
EELS:
https://github.com/ethereum/execution-specs/blob/devnets/prague/5/src/ethereum/prague/fork.py
EEST:
Nearly all execution-spec-tests are passing except for some that involve 7702, which might be fixed by #8118
Fixed Issue(s)
fixes #7994
Thanks for sending a pull request! Have you done the following?
doc-change-required
label to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew build
./gradlew acceptanceTest
./gradlew integrationTest
./gradlew ethereum:referenceTests:referenceTests