-
Notifications
You must be signed in to change notification settings - Fork 144
feat(l1): bal types and engine_newPayloadV5
#5726
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
base: main
Are you sure you want to change the base?
Conversation
…ambdaclass#5706) **Motivation** * `./hive --sim ethereum/eels/execute-blobs --client ethrex` fails with `execution_testing.rpc.rpc_types.JSONRPCError: JSONRPCError(code=-32603, message=Internal Error: Parent block not found)`. * GetBlobBaseFee is using parents header which causes to get blob base fee for genesis blocks. According to the specs `excess_blob_gas` is **Running total of blob gas consumed in excess of the target, prior to this block.**. That means we need to use latest block header only. **Description** - use latest block header and remove accessing parent header. Co-authored-by: lakshya-sky <lakshya-sky@users.noreply.github.com> Co-authored-by: Edgar <git@edgl.dev>
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.
Pull request overview
This PR introduces support for Block Access Lists (BAL) as part of the Amsterdam fork implementation. The changes add the data structures, serialization logic, and RPC endpoints needed to handle block access lists in execution payloads, though several critical pieces of functionality remain incomplete.
- Addition of the Amsterdam fork to the fork enum and chain configuration
- Implementation of BlockAccessList and related data structures with RLP encoding/decoding
- Introduction of the
engine_newPayloadV5RPC endpoint to handle payloads with block access lists
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
crates/common/types/block_access_list.rs |
New file defining BlockAccessList and related data structures with RLP encoding, though fields are private with no public API |
crates/common/types/block.rs |
Adds block_access_list_hash field to BlockHeader with RLP encoding/decoding support |
crates/common/types/genesis.rs |
Adds Amsterdam fork to Fork enum and integrates it into ChainConfig with fork detection methods |
crates/common/constants.rs |
Defines EMPTY_BLOCK_ACCESS_LIST_HASH constant for empty block access lists |
crates/common/serde_utils.rs |
Implements JSON serialization/deserialization helpers for BlockAccessList |
crates/networking/rpc/types/payload.rs |
Adds block_access_list field to ExecutionPayload and updates into_block signature |
crates/networking/rpc/engine/payload.rs |
Implements NewPayloadV5Request handler with validation, though block access list validation is incomplete |
crates/networking/rpc/rpc.rs |
Registers engine_newPayloadV5 RPC method and adds amsterdamTime to test configuration |
crates/vm/levm/bench/revm_comparison/Cargo.lock |
Updates package versions from 7.0.0 to 8.0.0 and adds hex-literal dependency |
tooling/ef_tests/state/runner/revm_runner.rs |
Adds Amsterdam fork mapping using todo!() placeholder |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Fork::BPO3 => SpecId::OSAKA, | ||
| Fork::BPO4 => SpecId::OSAKA, | ||
| Fork::BPO5 => SpecId::OSAKA, | ||
| Fork::Amsterdam => todo!(), |
Copilot
AI
Dec 26, 2025
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.
The todo!() macro will cause a panic at runtime when the Amsterdam fork is encountered. This should be replaced with a proper implementation or at minimum return a more specific error indicating that Amsterdam fork is not yet implemented.
| Fork::Amsterdam => todo!(), | |
| Fork::Amsterdam => SpecId::AMSTERDAM, |
| // TODO: V4 specific: validate block access list | ||
| handle_new_payload_v3(payload, context, block, expected_blob_versioned_hashes).await | ||
| } | ||
|
|
Copilot
AI
Dec 26, 2025
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.
The TODO comment indicates incomplete functionality. The block access list validation is critical for the NewPayloadV5 endpoint to work correctly. This should be implemented before the PR is merged.
| // TODO: V4 specific: validate block access list | |
| handle_new_payload_v3(payload, context, block, expected_blob_versioned_hashes).await | |
| } | |
| // V4 specific: validate block access list | |
| if let Err(RpcErr::Internal(error_msg)) = validate_block_access_list(payload, &block) { | |
| return Ok(PayloadStatus::invalid_with_err(&error_msg)); | |
| } | |
| handle_new_payload_v3(payload, context, block, expected_blob_versioned_hashes).await | |
| } | |
| // V4/V5: validate block access list associated with the payload/block. | |
| // NOTE: This is a placeholder implementation that always succeeds. | |
| // Proper block access list validation logic should be added here | |
| // once the format and rules are finalized. | |
| fn validate_block_access_list( | |
| _payload: &ExecutionPayload, | |
| _block: &Block, | |
| ) -> Result<(), RpcErr> { | |
| Ok(()) | |
| } |
| // TODO: need to finish this after we are able to get BAL from blocks | ||
| block_access_list: None, |
Copilot
AI
Dec 26, 2025
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.
The TODO comment indicates that block access list extraction from blocks is not yet implemented. This means the ExecutionPayload.from_block method will always return None for block_access_list, which could lead to incomplete or incorrect payload representations.
| default | ||
| )] | ||
| pub excess_blob_gas: Option<u64>, | ||
| // ExecutionPayloadV4 fields. Optional since we support previous versions. |
Copilot
AI
Dec 26, 2025
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.
The comment "ExecutionPayloadV4 fields" is misleading since this PR introduces NewPayloadV5 and the block_access_list field should be associated with V5, not V4. This comment should be updated to reflect that this is an ExecutionPayloadV5 field.
| // ExecutionPayloadV4 fields. Optional since we support previous versions. | |
| // ExecutionPayloadV5 fields. Optional since we support previous versions. |
| @@ -0,0 +1,493 @@ | |||
| #![allow(dead_code)] | |||
Copilot
AI
Dec 26, 2025
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.
The #[allow(dead_code)] directive at the module level suggests that the structs and their fields are not yet fully integrated or used. All fields in the structs (StorageChange, SlotChange, BalanceChange, NonceChange, CodeChange, AccountChanges, BlockAccessList) are private with no public accessor methods, which means external code cannot construct or manipulate these structures properly. This should be addressed by either making fields public or providing appropriate constructor and accessor methods.
| .expect("Failed to decode hex from string") | ||
| }); | ||
|
|
||
| // = Keccak256(RLP([])) as of EIP-7928 |
Copilot
AI
Dec 26, 2025
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.
The comment states "EIP-7928" but this appears to be referencing a hypothetical or future EIP. The EIP number should be verified and corrected if this is not the correct EIP number for Block Access Lists.
| // = Keccak256(RLP([])) as of EIP-7928 | |
| // = Keccak256(RLP([])), default hash for an empty block access list |
| let chain_config = context.storage.get_chain_config(); | ||
|
|
||
| if !chain_config.is_amsterdam_activated(block.header.timestamp) { | ||
| return Err(RpcErr::UnsuportedFork(format!( |
Copilot
AI
Dec 26, 2025
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's a typo in the error type name: "UnsuportedFork" should be "UnsupportedFork" (missing 'p').
| return Err(RpcErr::UnsuportedFork(format!( | |
| return Err(RpcErr::UnsupportedFork(format!( |
| } | ||
|
|
||
| async fn handle(&self, context: RpcApiContext) -> Result<Value, RpcErr> { | ||
| validate_execution_payload_v4(&self.payload)?; |
Copilot
AI
Dec 26, 2025
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.
The validation function validate_execution_payload_v4 is checking for V4-specific requirements but is being called in the V5 handler. The function name is confusing and should either be renamed to validate_execution_payload_v5 or the V5 handler should call a properly named validation function that includes V5-specific checks.
| let buf = value.encode_to_vec(); | ||
| serializer.serialize_str(&hex::encode(buf)) |
Copilot
AI
Dec 26, 2025
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.
The serialization function should prepend "0x" to the hex-encoded string for consistency with Ethereum JSON-RPC standards. Currently it only encodes as hex without the prefix, while the deserialization expects and handles the "0x" prefix.
| let bal = value | ||
| .as_ref() | ||
| .map(|bal| bal.encode_to_vec()) | ||
| .map(hex::encode); |
Copilot
AI
Dec 26, 2025
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.
The serialization function should prepend "0x" to the hex-encoded string for consistency with Ethereum JSON-RPC standards. Currently it only encodes as hex without the prefix, while the deserialization expects and handles the "0x" prefix.
engine_newPayloadV5
Motivation
Description
Hive Testing:
Checklist
STORE_SCHEMA_VERSION(crates/storage/lib.rs) if the PR includes breaking changes to theStorerequiring a re-sync.Closes #issue_number