-
Notifications
You must be signed in to change notification settings - Fork 849
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-6110: Add deposit validation and apis #5295
Conversation
Signed-off-by: Navie Chan <[email protected]>
|
Signed-off-by: Navie Chan <[email protected]>
Signed-off-by: Navie Chan <[email protected]>
Signed-off-by: Navie Chan <[email protected]>
Signed-off-by: Navie Chan <[email protected]>
Signed-off-by: Navie Chan <[email protected]>
Signed-off-by: Navie Chan <[email protected]>
Signed-off-by: Navie Chan <[email protected]>
Signed-off-by: Navie Chan <[email protected]>
Signed-off-by: navie <[email protected]>
Signed-off-by: navie <[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.
Did a first pass looks pretty straight forward to me. The deposit parsing and validation is simple and easy to understand. Some initial comments, mostly naming.
Next pass I will review against the EIP and execution-api spec in more detail.
...ptance-tests/tests/src/test/resources/jsonrpc/engine/test-cases/12_shanghai_get_payload.json
Outdated
Show resolved
Hide resolved
...tance-tests/tests/src/test/resources/jsonrpc/engine/test-cases/18_v6110_prepare_payload.json
Outdated
Show resolved
Hide resolved
datatypes/src/main/java/org/hyperledger/besu/datatypes/Address.java
Outdated
Show resolved
Hide resolved
datatypes/src/main/java/org/hyperledger/besu/datatypes/Address.java
Outdated
Show resolved
Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/DepositsValidator.java
Outdated
Show resolved
Hide resolved
ethereum/core/src/test/java/org/hyperledger/besu/ethereum/mainnet/DepositsValidatorTest.java
Outdated
Show resolved
Hide resolved
...rg/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineGetPayloadV2Test.java
Show resolved
Hide resolved
Signed-off-by: navie <[email protected]>
Signed-off-by: navie <[email protected]>
Signed-off-by: navie <[email protected]>
Signed-off-by: navie <[email protected]>
To accurately gauge the impact of this PR on Besu's performance, particularly regarding block processing, the most effective approach is to deploy it on a dedicated environment with real data. It's preferable to use an environment with many deposits, and then profile the node to observe any impact on performance. In the meantime, I've identified two pertinent methods to assess performance by conducting microbenchmarking with JMH
During block processing, both methods are invoked, with DepositDecoder.decodeFromLog being called iteratively within a loop that depends on the quantity of receipts and logs per receipt present in the block Block processing
Block creation
The microbenchmarking of both methods (Keccak256 was added to enable a comparison point)
If I'm not wrong, BodyValidation.depositRoots is called twice during block processing, and it takes 1/4 ms to execute, I think we can save one execution but even with both executions, the overhead is acceptable. DepositDecoder.decodeFromLog overhead will depends on the number of receipts per block and the number of logs per receipt. If there're for example 1000 logs to process, this represents 13 ms overhead for block processing and block creation. I started also a node on mainnet with PR to test against main on current fork (previous commit of this PR 795b7c4), I don't see any performance regression on block processing or FCU From what I gathered, I don't see a blocker to merge this PR in termes of performance |
I've started a regression test, fresh X_CHECKPOINT sync of 3 BONSAI nodes: sepolia, goerli, mainnet. |
This has passed regression: all nodes got in sync and have stayed in sync. No errors in the logs. |
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.
Looking very close now!
Couple of minor things, main one is a potentially missing depositContractAddress filter.
Also can you add a changelog entry please :)
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/DepositContract.java
Outdated
Show resolved
Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/DepositDecoder.java
Outdated
Show resolved
Hide resolved
config/src/main/java/org/hyperledger/besu/config/JsonGenesisConfigOptions.java
Show resolved
Hide resolved
...hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/DepositsValidatorProvider.java
Outdated
Show resolved
Hide resolved
...rg/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineGetPayloadV2Test.java
Outdated
Show resolved
Hide resolved
...creation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/AbstractBlockCreator.java
Outdated
Show resolved
Hide resolved
ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/encoding/DepositDecoderTest.java
Outdated
Show resolved
Hide resolved
ethereum/core/src/test/java/org/hyperledger/besu/ethereum/mainnet/DepositsValidatorTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Navie Chan <[email protected]>
Signed-off-by: Navie Chan <[email protected]>
...creation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/AbstractBlockCreator.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Navie Chan <[email protected]>
Signed-off-by: Navie Chan <[email protected]>
Signed-off-by: Simon Dudley <[email protected]>
Signed-off-by: Simon Dudley <[email protected]>
Signed-off-by: navie <[email protected]>
Signed-off-by: navie <[email protected]>
Remove unnecessary Spy annotations that were causing issues with the test setup code. Tidy up NPE Signed-off-by: Simon Dudley <[email protected]>
Signed-off-by: Simon Dudley <[email protected]>
Signed-off-by: Simon Dudley <[email protected]>
Extract findDepositsFromReceipts for testability Signed-off-by: Simon Dudley <[email protected]>
Signed-off-by: Simon Dudley <[email protected]>
Reran some regression tests (deposits disabled) on latest changes with a local interop and sepolia sync ✅ |
Looks fine to me |
Thanks @jframe. I will hold off merging until we are sure main branch is clear: there are some issues on main found during the 23.4.2 soak. |
Completes the implementation of EIP-6110 * Decode and extract deposit from transaction receipt log * Introduce depositContractAddress field in genesis file * Validate deposits in a block against the transaction receipt logs * Update engine_newPayloadV2 and engine_getPayloadV2 according to the spec. Some of the non-functional components were partially implemented in the previous PR Signed-off-by: naviechan <[email protected]> Signed-off-by: Simon Dudley <[email protected]> Co-authored-by: Simon Dudley <[email protected]>
Completes the implementation of EIP-6110 * Decode and extract deposit from transaction receipt log * Introduce depositContractAddress field in genesis file * Validate deposits in a block against the transaction receipt logs * Update engine_newPayloadV2 and engine_getPayloadV2 according to the spec. Some of the non-functional components were partially implemented in the previous PR Signed-off-by: naviechan <[email protected]> Signed-off-by: Simon Dudley <[email protected]> Co-authored-by: Simon Dudley <[email protected]>
This reverts commit ea8fbc2.
Completes the implementation of EIP-6110 * Decode and extract deposit from transaction receipt log * Introduce depositContractAddress field in genesis file * Validate deposits in a block against the transaction receipt logs * Update engine_newPayloadV2 and engine_getPayloadV2 according to the spec. Some of the non-functional components were partially implemented in the previous PR Signed-off-by: naviechan <[email protected]> Signed-off-by: Simon Dudley <[email protected]> Co-authored-by: Simon Dudley <[email protected]>
Completes the implementation of EIP-6110 * Decode and extract deposit from transaction receipt log * Introduce depositContractAddress field in genesis file * Validate deposits in a block against the transaction receipt logs * Update engine_newPayloadV2 and engine_getPayloadV2 according to the spec. Some of the non-functional components were partially implemented in the previous PR Signed-off-by: naviechan <[email protected]> Signed-off-by: Simon Dudley <[email protected]> Co-authored-by: Simon Dudley <[email protected]>
PR description
This PR is the continuation of #5055. It is the final PR for EIP-6110. The following are the main features introduced:
depositContractAddress
field in genesis fileengine_newPayloadV2
andengine_getPayloadV2
according to the spec. Some of the non-functional components were partially implemented in the previous PRFixed Issue(s)
#5004
Testing