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 <[email protected]>

.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: ""

crates/networking/rpc/engine/fork_choice.rs

@@ -25,8 +25,7 @@ pub struct ForkChoiceUpdatedV1 {
 
 impl RpcHandler for ForkChoiceUpdatedV1 {
     fn parse(params: &Option<Vec<Value>>) -> Result<Self, RpcErr> {
-        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<Vec<Value>>) -> Result<Self, RpcErr> {
-        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<ForkChoiceUpdatedV3> for RpcRequest {
 
 impl RpcHandler for ForkChoiceUpdatedV3 {
     fn parse(params: &Option<Vec<Value>>) -> Result<Self, RpcErr> {
-        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<Vec<Value>>,
+    is_v3: bool,
 ) -> Result<(ForkChoiceState, Option<PayloadAttributesV3>), 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(

crates/networking/rpc/engine/payload.rs

@@ -23,7 +23,8 @@ impl RpcHandler for NewPayloadV1Request {
     }
 
     fn handle(&self, context: RpcApiContext) -> Result<Value, RpcErr> {
-        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<Value, RpcErr> {
-        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<Value, RpcErr> {
-        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<Value, RpcErr> {
+        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<Value, RpcErr> {
+        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<Value, RpcErr> {
+        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<Vec<Value>>) -> Result<ExecutionPaylo
     serde_json::from_value(params[0].clone()).map_err(|_| RpcErr::WrongParam("payload".to_string()))
 }
 
+fn validate_execution_payload_v1(payload: &ExecutionPayload) -> 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<Value, RpcErr> {
     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<H256>,
@@ -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<bool>,
     context: RpcApiContext,
 ) -> Result<ExecutionPayloadResponse, RpcErr> {
-    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),

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")]