From e8a402aa5b3033782a5cf3379ff31800f4eea4ab Mon Sep 17 00:00:00 2001 From: Julian Ventura <43799596+JulianVentura@users.noreply.github.com> Date: Fri, 17 Jan 2025 17:39:25 -0300 Subject: [PATCH] fix(l1): fixes some withdrawals hive tests (#1713) **Motivation** There are some hive tests under `ethereum/engine` simulation that are failing due to some mismatches between the implementation of some engine RPC endpoints and their specification. **Description** This PR modifies some engine RPC endpoints validations, to comply with [shangai](https://github.com/ethereum/execution-apis/blob/main/src/engine/shanghai.md) and [cancun](https://github.com/ethereum/execution-apis/blob/main/src/engine/cancun.md) official ethereum engine specification. Those endpoints are: * `engine_newPayloadV1`, `engine_newPayloadV2`, `engine_newPayloadV3` * `engine_forkChoiceUpdatedV1`, `engine_forkChoiceUpdatedV2`, `engine_forkChoiceUpdatedV3` * `engine_getPayloadV1`, `engine_getPayloadV2`, `engine_getPayloadV3` This PR fixes some of the failing tests mentioned on #1586. Those tests are: * Withdrawals Fork on Block 2 (Paris) (ethrex) * Withdrawals Fork on Block 3 (Paris) (ethrex) If you want, you can run only those tests by using the following command: ```bash make run-hive SIMULATION=ethereum/engine TEST_PATTERN="engine-withdrawals/(Withdrawals Fork on Block 2|Withdrawals Fork on Block 3)" ``` --------- Co-authored-by: Julian Ventura --- .github/workflows/ci_l1.yaml | 2 +- crates/networking/rpc/engine/fork_choice.rs | 89 ++++++++-------- crates/networking/rpc/engine/payload.rs | 107 +++++++++++++++----- crates/networking/rpc/types/payload.rs | 2 +- 4 files changed, 130 insertions(+), 70 deletions(-) diff --git a/.github/workflows/ci_l1.yaml b/.github/workflows/ci_l1.yaml index bd66d6b4d2..e6518b03c0 100644 --- a/.github/workflows/ci_l1.yaml +++ b/.github/workflows/ci_l1.yaml @@ -179,7 +179,7 @@ jobs: test_pattern: "engine-api/RPC|Re-Org Back to Canonical Chain From Syncing Chain|Re-org to Previously Validated Sidechain Payload|Re-Org Back into Canonical Chain, Depth=5|Safe Re-Org|Transaction Re-Org|Inconsistent|Suggested Fee|PrevRandao|Fork ID|Unknown|Invalid PayloadAttributes|Bad Hash|Unique Payload ID|Re-Execute Payload|In-Order|Multiple New Payloads|Valid NewPayload|NewPayload with|Invalid NewPayload|Payload Build|Invalid NewPayload, Transaction|ParentHash equals|Build Payload|Invalid Missing Ancestor ReOrg" - name: "Engine withdrawal tests" simulation: ethereum/engine - test_pattern: "engine-withdrawals/engine-withdrawals test loader|GetPayloadV2 Block Value|Sync after 2 blocks - Withdrawals on Genesis|Max Initcode Size|Pre-Merge Fork Number > 0|Empty Withdrawals|Corrupted Block Hash Payload" + test_pattern: "engine-withdrawals/engine-withdrawals test loader|GetPayloadV2 Block Value|Sync after 2 blocks - Withdrawals on Genesis|Max Initcode Size|Pre-Merge Fork Number > 0|Empty Withdrawals|Corrupted Block Hash Payload|Withdrawals Fork on Block 2|Withdrawals Fork on Block 3" - name: "Sync" simulation: ethereum/sync test_pattern: "" diff --git a/crates/networking/rpc/engine/fork_choice.rs b/crates/networking/rpc/engine/fork_choice.rs index 42e1393c72..6f21734bae 100644 --- a/crates/networking/rpc/engine/fork_choice.rs +++ b/crates/networking/rpc/engine/fork_choice.rs @@ -25,8 +25,7 @@ pub struct ForkChoiceUpdatedV1 { impl RpcHandler for ForkChoiceUpdatedV1 { fn parse(params: &Option>) -> Result { - let (fork_choice_state, payload_attributes) = parse(params)?; - + let (fork_choice_state, payload_attributes) = parse(params, false)?; Ok(ForkChoiceUpdatedV1 { fork_choice_state, payload_attributes, @@ -37,11 +36,16 @@ impl RpcHandler for ForkChoiceUpdatedV1 { let (head_block_opt, mut response) = handle_forkchoice(&self.fork_choice_state, context.clone(), 1)?; if let (Some(head_block), Some(attributes)) = (head_block_opt, &self.payload_attributes) { - validate_v1(attributes, head_block)?; + let chain_config = context.storage.get_chain_config()?; + if chain_config.is_cancun_activated(attributes.timestamp) { + return Err(RpcErr::UnsuportedFork( + "forkChoiceV1 used to build Cancun payload".to_string(), + )); + } + validate_attributes_v1(attributes, &head_block)?; let payload_id = build_payload(attributes, context, &self.fork_choice_state, 1)?; response.set_id(payload_id); } - serde_json::to_value(response).map_err(|error| RpcErr::Internal(error.to_string())) } } @@ -54,8 +58,7 @@ pub struct ForkChoiceUpdatedV2 { impl RpcHandler for ForkChoiceUpdatedV2 { fn parse(params: &Option>) -> Result { - let (fork_choice_state, payload_attributes) = parse(params)?; - + let (fork_choice_state, payload_attributes) = parse(params, false)?; Ok(ForkChoiceUpdatedV2 { fork_choice_state, payload_attributes, @@ -66,11 +69,20 @@ impl RpcHandler for ForkChoiceUpdatedV2 { let (head_block_opt, mut response) = handle_forkchoice(&self.fork_choice_state, context.clone(), 2)?; if let (Some(head_block), Some(attributes)) = (head_block_opt, &self.payload_attributes) { - validate_v2(attributes, head_block, &context)?; + let chain_config = context.storage.get_chain_config()?; + if chain_config.is_cancun_activated(attributes.timestamp) { + return Err(RpcErr::UnsuportedFork( + "forkChoiceV2 used to build Cancun payload".to_string(), + )); + } else if chain_config.is_shanghai_activated(attributes.timestamp) { + validate_attributes_v2(attributes, &head_block)?; + } else { + // Behave as a v1 + validate_attributes_v1(attributes, &head_block)?; + } let payload_id = build_payload(attributes, context, &self.fork_choice_state, 2)?; response.set_id(payload_id); } - serde_json::to_value(response).map_err(|error| RpcErr::Internal(error.to_string())) } } @@ -96,8 +108,7 @@ impl From for RpcRequest { impl RpcHandler for ForkChoiceUpdatedV3 { fn parse(params: &Option>) -> Result { - let (fork_choice_state, payload_attributes) = parse(params)?; - + let (fork_choice_state, payload_attributes) = parse(params, true)?; Ok(ForkChoiceUpdatedV3 { fork_choice_state, payload_attributes, @@ -108,17 +119,17 @@ impl RpcHandler for ForkChoiceUpdatedV3 { let (head_block_opt, mut response) = handle_forkchoice(&self.fork_choice_state, context.clone(), 3)?; if let (Some(head_block), Some(attributes)) = (head_block_opt, &self.payload_attributes) { - validate_v3(attributes, head_block, &context)?; + validate_attributes_v3(attributes, &head_block, &context)?; let payload_id = build_payload(attributes, context, &self.fork_choice_state, 3)?; response.set_id(payload_id); } - serde_json::to_value(response).map_err(|error| RpcErr::Internal(error.to_string())) } } fn parse( params: &Option>, + is_v3: bool, ) -> Result<(ForkChoiceState, Option), RpcErr> { let params = params .as_ref() @@ -137,7 +148,13 @@ fn parse( None } }; - + if let Some(attr) = &payload_attributes { + if !is_v3 && attr.parent_beacon_block_root.is_some() { + return Err(RpcErr::InvalidPayloadAttributes( + "Attribute parent_beacon_block_root is non-null".to_string(), + )); + } + } Ok((forkchoice_state, payload_attributes)) } @@ -204,48 +221,40 @@ fn handle_forkchoice( } } -fn validate_v1(attributes: &PayloadAttributesV3, head_block: BlockHeader) -> Result<(), RpcErr> { +fn validate_attributes_v1( + attributes: &PayloadAttributesV3, + head_block: &BlockHeader, +) -> Result<(), RpcErr> { + if attributes.withdrawals.is_some() { + return Err(RpcErr::WrongParam("withdrawals".to_string())); + } validate_timestamp(attributes, head_block) } -fn validate_v2( +fn validate_attributes_v2( attributes: &PayloadAttributesV3, - head_block: BlockHeader, - context: &RpcApiContext, + head_block: &BlockHeader, ) -> Result<(), RpcErr> { - let chain_config = context.storage.get_chain_config()?; if attributes.withdrawals.is_none() { - return Err(RpcErr::WrongParam( - "forkChoiceV2 withdrawals is null".to_string(), - )); - } - if attributes.parent_beacon_block_root.is_some() { - return Err(RpcErr::InvalidPayloadAttributes( - "forkChoiceV2 with Beacon Root".to_string(), - )); - } - if !chain_config.is_shanghai_activated(attributes.timestamp) { - return Err(RpcErr::UnsuportedFork( - "forkChoiceV2 used to build pre-Shanghai payload".to_string(), - )); - } - if chain_config.is_cancun_activated(attributes.timestamp) { - return Err(RpcErr::UnsuportedFork( - "forkChoiceV2 used to build Cancun payload".to_string(), - )); + return Err(RpcErr::WrongParam("withdrawals".to_string())); } validate_timestamp(attributes, head_block) } -fn validate_v3( +fn validate_attributes_v3( attributes: &PayloadAttributesV3, - head_block: BlockHeader, + head_block: &BlockHeader, context: &RpcApiContext, ) -> Result<(), RpcErr> { let chain_config = context.storage.get_chain_config()?; + // Specification indicates this order of validations: + // https://github.com/ethereum/execution-apis/blob/main/src/engine/cancun.md#specification-1 + if attributes.withdrawals.is_none() { + return Err(RpcErr::InvalidPayloadAttributes("withdrawals".to_string())); + } if attributes.parent_beacon_block_root.is_none() { return Err(RpcErr::InvalidPayloadAttributes( - "Null Parent Beacon Root".to_string(), + "Attribute parent_beacon_block_root is null".to_string(), )); } if !chain_config.is_cancun_activated(attributes.timestamp) { @@ -258,7 +267,7 @@ fn validate_v3( fn validate_timestamp( attributes: &PayloadAttributesV3, - head_block: BlockHeader, + head_block: &BlockHeader, ) -> Result<(), RpcErr> { if attributes.timestamp <= head_block.timestamp { return Err(RpcErr::InvalidPayloadAttributes( diff --git a/crates/networking/rpc/engine/payload.rs b/crates/networking/rpc/engine/payload.rs index 08f80292f7..5207d9995d 100644 --- a/crates/networking/rpc/engine/payload.rs +++ b/crates/networking/rpc/engine/payload.rs @@ -23,7 +23,8 @@ impl RpcHandler for NewPayloadV1Request { } fn handle(&self, context: RpcApiContext) -> Result { - handle_new_payload_v1_v2(&self.payload, Fork::Paris, context) + validate_execution_payload_v1(&self.payload)?; + handle_new_payload_v1_v2(&self.payload, context) } } @@ -39,13 +40,15 @@ impl RpcHandler for NewPayloadV2Request { } fn handle(&self, context: RpcApiContext) -> Result { - if self.payload.withdrawals.is_none() { - Err(RpcErr::WrongParam( - "forkChoiceV2 withdrawals is null".to_string(), - )) + let chain_config = &context.storage.get_chain_config()?; + if chain_config.is_shanghai_activated(self.payload.timestamp) { + validate_execution_payload_v2(&self.payload)?; } else { - handle_new_payload_v1_v2(&self.payload, Fork::Shanghai, context) + // Behave as a v1 + validate_execution_payload_v1(&self.payload)?; } + + handle_new_payload_v1_v2(&self.payload, context) } } @@ -88,9 +91,9 @@ impl RpcHandler for NewPayloadV3Request { } fn handle(&self, context: RpcApiContext) -> Result { - validate_execution_payload_v3(&self.payload)?; let block = get_block_from_payload(&self.payload, Some(self.parent_beacon_block_root))?; validate_fork(&block, Fork::Cancun, &context)?; + validate_execution_payload_v3(&self.payload)?; let payload_status = { if let Err(RpcErr::Internal(error_msg)) = validate_block_hash(&self.payload, &block) { PayloadStatus::invalid_with_err(&error_msg) @@ -125,8 +128,12 @@ impl RpcHandler for GetPayloadV1Request { } fn handle(&self, context: RpcApiContext) -> Result { + let payload = get_payload(self.payload_id, &context)?; + // NOTE: This validation is actually not required to run Hive tests. Not sure if it's + // necessary + validate_payload_v1_v2(&payload.0, &context)?; let execution_payload_response = - build_execution_payload_response(self.payload_id, Fork::Paris, None, context)?; + build_execution_payload_response(self.payload_id, payload, None, context)?; serde_json::to_value(execution_payload_response.execution_payload) .map_err(|error| RpcErr::Internal(error.to_string())) } @@ -143,8 +150,10 @@ impl RpcHandler for GetPayloadV2Request { } fn handle(&self, context: RpcApiContext) -> Result { + let payload = get_payload(self.payload_id, &context)?; + validate_payload_v1_v2(&payload.0, &context)?; let execution_payload_response = - build_execution_payload_response(self.payload_id, Fork::Shanghai, None, context)?; + build_execution_payload_response(self.payload_id, payload, None, context)?; serde_json::to_value(execution_payload_response) .map_err(|error| RpcErr::Internal(error.to_string())) } @@ -171,8 +180,11 @@ impl RpcHandler for GetPayloadV3Request { } fn handle(&self, context: RpcApiContext) -> Result { + let payload = get_payload(self.payload_id, &context)?; + validate_fork(&payload.0, Fork::Cancun, &context)?; let execution_payload_response = - build_execution_payload_response(self.payload_id, Fork::Cancun, Some(false), context)?; + build_execution_payload_response(self.payload_id, payload, Some(false), context)?; + serde_json::to_value(execution_payload_response) .map_err(|error| RpcErr::Internal(error.to_string())) } @@ -188,13 +200,66 @@ fn parse_execution_payload(params: &Option>) -> Result Result<(), RpcErr> { + // Validate that only the required arguments are present + if payload.withdrawals.is_some() { + return Err(RpcErr::WrongParam("withdrawals".to_string())); + } + if payload.blob_gas_used.is_some() { + return Err(RpcErr::WrongParam("blob_gas_used".to_string())); + } + if payload.excess_blob_gas.is_some() { + return Err(RpcErr::WrongParam("excess_blob_gas".to_string())); + } + + Ok(()) +} + +fn validate_execution_payload_v2(payload: &ExecutionPayload) -> Result<(), RpcErr> { + // Validate that only the required arguments are present + if payload.withdrawals.is_none() { + return Err(RpcErr::WrongParam("withdrawals".to_string())); + } + if payload.blob_gas_used.is_some() { + return Err(RpcErr::WrongParam("blob_gas_used".to_string())); + } + if payload.excess_blob_gas.is_some() { + return Err(RpcErr::WrongParam("excess_blob_gas".to_string())); + } + + Ok(()) +} + +fn validate_execution_payload_v3(payload: &ExecutionPayload) -> Result<(), RpcErr> { + // Validate that only the required arguments are present + if payload.withdrawals.is_none() { + return Err(RpcErr::WrongParam("withdrawals".to_string())); + } + if payload.blob_gas_used.is_none() { + return Err(RpcErr::WrongParam("blob_gas_used".to_string())); + } + if payload.excess_blob_gas.is_none() { + return Err(RpcErr::WrongParam("excess_blob_gas".to_string())); + } + + Ok(()) +} + +fn validate_payload_v1_v2(block: &Block, context: &RpcApiContext) -> Result<(), RpcErr> { + let chain_config = &context.storage.get_chain_config()?; + if chain_config.is_cancun_activated(block.header.timestamp) { + return Err(RpcErr::UnsuportedFork( + "Cancun payload received".to_string(), + )); + } + Ok(()) +} + fn handle_new_payload_v1_v2( payload: &ExecutionPayload, - fork: Fork, context: RpcApiContext, ) -> Result { let block = get_block_from_payload(payload, None)?; - validate_fork(&block, fork, &context)?; let payload_status = { if let Err(RpcErr::Internal(error_msg)) = validate_block_hash(payload, &block) { PayloadStatus::invalid_with_err(&error_msg) @@ -205,16 +270,6 @@ fn handle_new_payload_v1_v2( serde_json::to_value(payload_status).map_err(|error| RpcErr::Internal(error.to_string())) } -fn validate_execution_payload_v3(payload: &ExecutionPayload) -> Result<(), RpcErr> { - if payload.excess_blob_gas.is_none() { - return Err(RpcErr::WrongParam("excess_blob_gas".to_string())); - } - if payload.blob_gas_used.is_none() { - return Err(RpcErr::WrongParam("blob_gas_used".to_string())); - } - Ok(()) -} - fn get_block_from_payload( payload: &ExecutionPayload, parent_beacon_block_root: Option, @@ -338,15 +393,11 @@ fn validate_fork(block: &Block, fork: Fork, context: &RpcApiContext) -> Result<( fn build_execution_payload_response( payload_id: u64, - fork: Fork, + payload: (Block, U256, BlobsBundle, bool), should_override_builder: Option, context: RpcApiContext, ) -> Result { - let (mut payload_block, block_value, blobs_bundle, completed) = - get_payload(payload_id, &context)?; - - validate_fork(&payload_block, fork, &context)?; - + let (mut payload_block, block_value, blobs_bundle, completed) = payload; if completed { Ok(ExecutionPayloadResponse { execution_payload: ExecutionPayload::from_block(payload_block), diff --git a/crates/networking/rpc/types/payload.rs b/crates/networking/rpc/types/payload.rs index 211e25190c..320c354025 100644 --- a/crates/networking/rpc/types/payload.rs +++ b/crates/networking/rpc/types/payload.rs @@ -27,7 +27,7 @@ pub struct ExecutionPayload { #[serde(with = "serde_utils::u64::hex_str")] gas_used: u64, #[serde(with = "serde_utils::u64::hex_str")] - timestamp: u64, + pub timestamp: u64, #[serde(with = "serde_utils::bytes")] extra_data: Bytes, #[serde(with = "serde_utils::u64::hex_str")]