Cache number of zero bytes in the payload in the tx #8488
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
d9c570e to
53c554e
Compare
There was a problem hiding this comment.
Pull Request Overview
This pull request implements caching for the number of zero bytes in the transaction payload by introducing a dedicated Payload class and updating gas calculation and transaction processing methods accordingly. Key changes include:
- Updating gas calculator methods (transactionIntrinsicGasCost and transactionFloorCost) to accept an explicit payloadZeroBytes parameter.
- Replacing direct use of Bytes payload in Transaction with a new Payload object that caches its zero byte count.
- Adjusting tests and API calls to use the new method signatures and improved payload handling.
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| evm/src/test/java/org/hyperledger/besu/evm/gascalculator/PragueGasCalculatorTest.java | Updated floor cost tests to pass an explicit payloadZeroBytes argument. |
| evm/src/test/java/org/hyperledger/besu/evm/gascalculator/FrontierGasCalculatorTest.java | Modified tests to use the new gas cost method signatures. |
| evm/src/main/java/org/hyperledger/besu/evm/gascalculator/PragueGasCalculator.java | Refactored transactionFloorCost to include payloadZeroBytes. |
| evm/src/main/java/org/hyperledger/besu/evm/gascalculator/GasCalculator.java | Updated interface signatures for intrinsic and floor gas cost methods. |
| evm/src/main/java/org/hyperledger/besu/evm/gascalculator/FrontierGasCalculator.java | Adjusted intrinsic gas cost calculations with the new transaction method signature. |
| ethereum/referencetests/.../TransactionTest.java | Updated tests to use the revised transactionIntrinsicGasCost method. |
| ethereum/evmtool/.../T8nExecutor.java and EvmToolCommand.java | Updated gas cost calls to pass the new parameters. |
| ethereum/core/.../Transaction.java | Replaced the raw payload with a Payload object and updated related methods. |
| ethereum/core/.../Payload.java | Introduced the Payload class for caching and computing zero byte counts. |
| ethereum/api/.../PluginEeaSendRawTransaction.java | Updated usage to create an unsigned transaction using the new payload handling. |
| datatypes/src/main/java/org/hyperledger/besu/datatypes/Transaction.java | Extended the Transaction interface to expose payload and zero byte count. |
| @@ -1296,7 +1297,7 @@ public Builder copiedFrom(final Transaction toCopy) { | |||
| this.to = toCopy.to; | |||
| this.value = toCopy.value; | |||
| this.signature = toCopy.signature; | |||
| this.payload = toCopy.payload; | |||
| this.payload = toCopy.payload.getPayload(); | |||
There was a problem hiding this comment.
Directly copying the payload bytes bypasses the caching mechanism provided by the Payload class. Consider preserving the Payload object by copying or reusing the entire Payload instance instead.
Suggested change
| this.payload = toCopy.payload.getPayload(); | |
| this.payload = toCopy.payload; |
Copilot uses AI. Check for mistakes.
Signed-off-by: daniellehrner <daniel.lehrner@consensys.net>
Signed-off-by: daniellehrner <daniel.lehrner@consensys.net>
Signed-off-by: daniellehrner <daniel.lehrner@consensys.net>
Signed-off-by: daniellehrner <daniel.lehrner@consensys.net>
53c554e to
a279d84
Compare
garyschulte
reviewed
Apr 2, 2025
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/Payload.java
Outdated
Show resolved
Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/Payload.java
Outdated
Show resolved
Hide resolved
Comment on lines
+64
to
+71
| private long computeZeroBytes() { | ||
| int zeros = 0; | ||
| for (int i = 0; i < payloadBytes.size(); i++) { | ||
| if (payloadBytes.get(i) == 0) { | ||
| zeros += 1; | ||
| } | ||
| } | ||
| return zeros; |
There was a problem hiding this comment.
I wonder if/how we can determine this is using simd under-the-hood. Non-blocking just curious. @ahamlat ?
Signed-off-by: daniellehrner <daniel.lehrner@consensys.net>
|
@garyschulte Do you think it's ready to merged with the latest changes? If yes, could you please approve? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
PR description
We need to count the zero bytes of the transaction payload to calculate the intrinsic gas cost. Right now we do this up to 3 times:
This PR caches the number of zero bytes in the tx object, which avoids recounting them and should be more efficient.
Thanks for sending a pull request! Have you done the following?
doc-change-requiredlabel to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew spotlessApply./gradlew build./gradlew acceptanceTest./gradlew integrationTest./gradlew ethereum:referenceTests:referenceTests