Skip to content

EIP-7623#8093

Merged
siladu merged 14 commits intobesu-eth:mainfrom
daniellehrner:feat/issue-7994/eip-7623
Jan 16, 2025
Merged

EIP-7623#8093
siladu merged 14 commits intobesu-eth:mainfrom
daniellehrner:feat/issue-7994/eip-7623

Conversation

@daniellehrner
Copy link
Copy Markdown
Contributor

@daniellehrner daniellehrner commented Jan 9, 2025 •
edited by siladu
Loading

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?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

  • unit tests: ./gradlew build
  • acceptance tests: ./gradlew acceptanceTest
  • integration tests: ./gradlew integrationTest
  • reference tests: ./gradlew ethereum:referenceTests:referenceTests

@daniellehrner
first draft for EIP-7623
8fab3cc 
Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
@daniellehrner
adding tests and javadoc comments
c6f0c3a 
Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
@daniellehrner
renamed evmGasUsed to baselineGas
2d2cfaf 
Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
@daniellehrner daniellehrner force-pushed the feat/issue-7994/eip-7623 branch from 348ad34 to 2d2cfaf Compare January 9, 2025 22:15
@daniellehrner
ignore Prague reference tests
58fb4e1 
Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
@daniellehrner
fix tests
1628235 
Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
siladu
siladu reviewed Jan 10, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@siladu siladu left a 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?

Comment thread evm/src/main/java/org/hyperledger/besu/evm/gascalculator/FrontierGasCalculator.java
*
* @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
Copy link
Copy Markdown
Contributor

@siladu siladu Jan 10, 2025

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)

Comment thread evm/src/main/java/org/hyperledger/besu/evm/gascalculator/FrontierGasCalculator.java
Comment on lines +157 to +159
if (cost == Long.MIN_VALUE || cost == Long.MAX_VALUE) {
return cost;
}
Copy link
Copy Markdown
Contributor

@siladu siladu Jan 10, 2025

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

Copy link
Copy Markdown
Contributor

@siladu siladu Jan 16, 2025 •
edited
Loading

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

Comment thread evm/src/main/java/org/hyperledger/besu/evm/gascalculator/PragueGasCalculator.java Outdated
@siladu siladu mentioned this pull request Jan 14, 2025
Closed
@siladu
Copy link
Copy Markdown
Contributor

siladu commented Jan 14, 2025

Do we need to do anything different to support EthEstimateGas, etc?

@siladu

This comment was marked as outdated.

Sign in to view
siladu and others added 6 commits January 15, 2025 15:00
@siladu
Merge branch 'main' into feat/issue-7994/eip-7623
fe1af59 
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
@siladu
Extract transactionFloorCost and compare with used gas after refund a…
2a46890 
…pplied

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
@siladu
unit tests and small refactor
368affb 
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
@siladu
Fix evmtool tests
a49b864 
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
@Gabriel-Trintinalia @siladu
Add support for movePrecompileToAddress in State Overrides (eth_call) (…
d9e770d 
…besu-eth#8115)

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
@siladu
Add PragueGasCalculator tests
74685b3 
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
@siladu siladu force-pushed the feat/issue-7994/eip-7623 branch from 9411a7e to 74685b3 Compare January 16, 2025 03:02
@siladu
Merge branch 'main' into feat/issue-7994/eip-7623
4052690 
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
macfarla
macfarla reviewed Jan 16, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@macfarla macfarla left a 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

@siladu
CHANGELOG
5762068 
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
@siladu
Merge branch 'main' into feat/issue-7994/eip-7623
00e0c8d 
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
@siladu siladu enabled auto-merge (squash) January 16, 2025 10:02
@siladu siladu merged commit 1e7f6f6 into besu-eth:main Jan 16, 2025
@daniellehrner daniellehrner deleted the feat/issue-7994/eip-7623 branch January 16, 2025 10:32
pullurib pushed a commit to pullurib/besu that referenced this pull request Feb 6, 2025
@daniellehrner @siladu
EIP-7623 (besu-eth#8093)
033da75 
Implements EIP-7623 (increase calldata cost)

Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Co-authored-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Bhanu Pulluri <bhanu.pulluri@kaleido.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@siladu siladu siladu left review comments

@macfarla macfarla macfarla approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Pectra devnet-5: EIP-7623: Increase calldata cost

4 participants

@daniellehrner @siladu @macfarla @Gabriel-Trintinalia