From 7e6df3ae6444f53bf3501e7774facba27edea9cc Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Thu, 12 Dec 2024 17:46:25 -0800 Subject: [PATCH 1/2] Exclude empty requests and add back prefix --- .../src/engine_api/json_structures.rs | 74 ++++++++++++++----- consensus/types/src/execution_requests.rs | 41 ++++++++-- consensus/types/src/lib.rs | 2 +- 3 files changed, 91 insertions(+), 26 deletions(-) diff --git a/beacon_node/execution_layer/src/engine_api/json_structures.rs b/beacon_node/execution_layer/src/engine_api/json_structures.rs index 1c6639804e3..f78c84f68f1 100644 --- a/beacon_node/execution_layer/src/engine_api/json_structures.rs +++ b/beacon_node/execution_layer/src/engine_api/json_structures.rs @@ -7,7 +7,7 @@ use superstruct::superstruct; use types::beacon_block_body::KzgCommitments; use types::blob_sidecar::BlobsList; use types::execution_requests::{ - ConsolidationRequests, DepositRequests, RequestPrefix, WithdrawalRequests, + ConsolidationRequests, DepositRequests, RequestType, WithdrawalRequests, }; use types::{Blob, FixedVector, KzgProof, Unsigned}; @@ -341,6 +341,15 @@ impl From> for ExecutionPayload { } } +#[derive(Debug, Clone)] +pub enum RequestsError { + InvalidHex(hex::FromHexError), + EmptyRequest(usize), + InvalidOrdering, + InvalidPrefix(u8), + DecodeError(String), +} + /// Format of `ExecutionRequests` received over the engine api. /// /// Array of ssz-encoded requests list encoded as hex bytes. @@ -355,33 +364,62 @@ impl From> for ExecutionPayload { pub struct JsonExecutionRequests(pub Vec); impl TryFrom for ExecutionRequests { - type Error = String; + type Error = RequestsError; fn try_from(value: JsonExecutionRequests) -> Result { let mut requests = ExecutionRequests::default(); - + let mut prev_prefix: Option = None; for (i, request) in value.0.into_iter().enumerate() { // hex string let decoded_bytes = hex::decode(request.strip_prefix("0x").unwrap_or(&request)) - .map_err(|e| format!("Invalid hex {:?}", e))?; - match RequestPrefix::from_prefix(i as u8) { - Some(RequestPrefix::Deposit) => { - requests.deposits = DepositRequests::::from_ssz_bytes(&decoded_bytes) - .map_err(|e| format!("Failed to decode DepositRequest from EL: {:?}", e))?; + .map_err(RequestsError::InvalidHex)?; + + // The first byte of each element is the `request_type` and the remaining bytes are the `request_data`. + // Elements with empty `request_data` **MUST** be excluded from the list. + let Some((prefix_byte, request_bytes)) = decoded_bytes.split_first() else { + return Err(RequestsError::EmptyRequest(i)); + }; + if request_bytes.is_empty() { + return Err(RequestsError::EmptyRequest(i)); + } + // Elements of the list **MUST** be ordered by `request_type` in ascending order + let current_prefix = RequestType::from_prefix(*prefix_byte) + .ok_or(RequestsError::InvalidPrefix(*prefix_byte))?; + if let Some(prev) = prev_prefix { + if prev.to_prefix() >= current_prefix.to_prefix() { + return Err(RequestsError::InvalidOrdering); } - Some(RequestPrefix::Withdrawal) => { - requests.withdrawals = WithdrawalRequests::::from_ssz_bytes(&decoded_bytes) + } + prev_prefix = Some(current_prefix); + + match current_prefix { + RequestType::Deposit => { + requests.deposits = DepositRequests::::from_ssz_bytes(request_bytes) .map_err(|e| { - format!("Failed to decode WithdrawalRequest from EL: {:?}", e) + RequestsError::DecodeError(format!( + "Failed to decode DepositRequest from EL: {:?}", + e + )) })?; } - Some(RequestPrefix::Consolidation) => { + RequestType::Withdrawal => { + requests.withdrawals = WithdrawalRequests::::from_ssz_bytes(request_bytes) + .map_err(|e| { + RequestsError::DecodeError(format!( + "Failed to decode WithdrawalRequest from EL: {:?}", + e + )) + })?; + } + RequestType::Consolidation => { requests.consolidations = - ConsolidationRequests::::from_ssz_bytes(&decoded_bytes).map_err( - |e| format!("Failed to decode ConsolidationRequest from EL: {:?}", e), - )?; + ConsolidationRequests::::from_ssz_bytes(request_bytes).map_err(|e| { + RequestsError::DecodeError(format!( + "Failed to decode ConsolidationRequest from EL: {:?}", + e + )) + })?; } - None => return Err("Empty requests string".to_string()), } } Ok(requests) @@ -448,7 +486,9 @@ impl TryFrom> for GetPayloadResponse { block_value: response.block_value, blobs_bundle: response.blobs_bundle.into(), should_override_builder: response.should_override_builder, - requests: response.execution_requests.try_into()?, + requests: response.execution_requests.try_into().map_err(|e| { + format!("Failed to convert json to execution requests : {:?}", e) + })?, })) } } diff --git a/consensus/types/src/execution_requests.rs b/consensus/types/src/execution_requests.rs index 96a39054207..0f502a208c9 100644 --- a/consensus/types/src/execution_requests.rs +++ b/consensus/types/src/execution_requests.rs @@ -43,10 +43,29 @@ impl ExecutionRequests { /// Returns the encoding according to EIP-7685 to send /// to the execution layer over the engine api. pub fn get_execution_requests_list(&self) -> Vec { - let deposit_bytes = Bytes::from(self.deposits.as_ssz_bytes()); - let withdrawal_bytes = Bytes::from(self.withdrawals.as_ssz_bytes()); - let consolidation_bytes = Bytes::from(self.consolidations.as_ssz_bytes()); - vec![deposit_bytes, withdrawal_bytes, consolidation_bytes] + let mut requests_list = Vec::new(); + if !self.deposits.is_empty() { + requests_list.push(Bytes::from_iter( + [RequestType::Deposit.to_prefix()] + .into_iter() + .chain(self.deposits.as_ssz_bytes()), + )); + } + if !self.withdrawals.is_empty() { + requests_list.push(Bytes::from_iter( + [RequestType::Withdrawal.to_prefix()] + .into_iter() + .chain(self.withdrawals.as_ssz_bytes()), + )); + } + if !self.consolidations.is_empty() { + requests_list.push(Bytes::from_iter( + [RequestType::Consolidation.to_prefix()] + .into_iter() + .chain(self.consolidations.as_ssz_bytes()), + )); + } + requests_list } /// Generate the execution layer `requests_hash` based on EIP-7685. @@ -55,9 +74,8 @@ impl ExecutionRequests { pub fn requests_hash(&self) -> Hash256 { let mut hasher = DynamicContext::new(); - for (i, request) in self.get_execution_requests_list().iter().enumerate() { + for request in self.get_execution_requests_list().iter() { let mut request_hasher = DynamicContext::new(); - request_hasher.update(&[i as u8]); request_hasher.update(request); let request_hash = request_hasher.finalize(); @@ -70,13 +88,13 @@ impl ExecutionRequests { /// This is used to index into the `execution_requests` array. #[derive(Debug, Copy, Clone)] -pub enum RequestPrefix { +pub enum RequestType { Deposit, Withdrawal, Consolidation, } -impl RequestPrefix { +impl RequestType { pub fn from_prefix(prefix: u8) -> Option { match prefix { 0 => Some(Self::Deposit), @@ -85,6 +103,13 @@ impl RequestPrefix { _ => None, } } + pub fn to_prefix(&self) -> u8 { + match self { + Self::Deposit => 0, + Self::Withdrawal => 1, + Self::Consolidation => 2, + } + } } #[cfg(test)] diff --git a/consensus/types/src/lib.rs b/consensus/types/src/lib.rs index dd304c6296c..612b64cf891 100644 --- a/consensus/types/src/lib.rs +++ b/consensus/types/src/lib.rs @@ -170,7 +170,7 @@ pub use crate::execution_payload_header::{ ExecutionPayloadHeaderDeneb, ExecutionPayloadHeaderElectra, ExecutionPayloadHeaderRef, ExecutionPayloadHeaderRefMut, }; -pub use crate::execution_requests::{ExecutionRequests, RequestPrefix}; +pub use crate::execution_requests::{ExecutionRequests, RequestType}; pub use crate::fork::Fork; pub use crate::fork_context::ForkContext; pub use crate::fork_data::ForkData; From 0b85d8632563bc3ddcc7b34bef599ad8e2db8428 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Fri, 3 Jan 2025 18:25:54 -0800 Subject: [PATCH 2/2] cleanup --- .../src/engine_api/json_structures.rs | 13 ++++--------- consensus/types/src/execution_requests.rs | 12 ++++++------ 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/beacon_node/execution_layer/src/engine_api/json_structures.rs b/beacon_node/execution_layer/src/engine_api/json_structures.rs index f78c84f68f1..cc777a62411 100644 --- a/beacon_node/execution_layer/src/engine_api/json_structures.rs +++ b/beacon_node/execution_layer/src/engine_api/json_structures.rs @@ -352,13 +352,8 @@ pub enum RequestsError { /// Format of `ExecutionRequests` received over the engine api. /// -/// Array of ssz-encoded requests list encoded as hex bytes. -/// The prefix of the request type is used to index into the array. -/// -/// For e.g. [0xab, 0xcd, 0xef] -/// Here, 0xab are the deposits bytes (prefix and index == 0) -/// 0xcd are the withdrawals bytes (prefix and index == 1) -/// 0xef are the consolidations bytes (prefix and index == 2) +/// Array of ssz-encoded requests list encoded as hex bytes prefixed +/// with a `RequestType` #[derive(Debug, Default, Clone, PartialEq, Serialize, Deserialize)] #[serde(transparent)] pub struct JsonExecutionRequests(pub Vec); @@ -383,10 +378,10 @@ impl TryFrom for ExecutionRequests { return Err(RequestsError::EmptyRequest(i)); } // Elements of the list **MUST** be ordered by `request_type` in ascending order - let current_prefix = RequestType::from_prefix(*prefix_byte) + let current_prefix = RequestType::from_u8(*prefix_byte) .ok_or(RequestsError::InvalidPrefix(*prefix_byte))?; if let Some(prev) = prev_prefix { - if prev.to_prefix() >= current_prefix.to_prefix() { + if prev.to_u8() >= current_prefix.to_u8() { return Err(RequestsError::InvalidOrdering); } } diff --git a/consensus/types/src/execution_requests.rs b/consensus/types/src/execution_requests.rs index 0f502a208c9..223c6444ccf 100644 --- a/consensus/types/src/execution_requests.rs +++ b/consensus/types/src/execution_requests.rs @@ -46,21 +46,21 @@ impl ExecutionRequests { let mut requests_list = Vec::new(); if !self.deposits.is_empty() { requests_list.push(Bytes::from_iter( - [RequestType::Deposit.to_prefix()] + [RequestType::Deposit.to_u8()] .into_iter() .chain(self.deposits.as_ssz_bytes()), )); } if !self.withdrawals.is_empty() { requests_list.push(Bytes::from_iter( - [RequestType::Withdrawal.to_prefix()] + [RequestType::Withdrawal.to_u8()] .into_iter() .chain(self.withdrawals.as_ssz_bytes()), )); } if !self.consolidations.is_empty() { requests_list.push(Bytes::from_iter( - [RequestType::Consolidation.to_prefix()] + [RequestType::Consolidation.to_u8()] .into_iter() .chain(self.consolidations.as_ssz_bytes()), )); @@ -86,7 +86,7 @@ impl ExecutionRequests { } } -/// This is used to index into the `execution_requests` array. +/// The prefix types for `ExecutionRequest` objects. #[derive(Debug, Copy, Clone)] pub enum RequestType { Deposit, @@ -95,7 +95,7 @@ pub enum RequestType { } impl RequestType { - pub fn from_prefix(prefix: u8) -> Option { + pub fn from_u8(prefix: u8) -> Option { match prefix { 0 => Some(Self::Deposit), 1 => Some(Self::Withdrawal), @@ -103,7 +103,7 @@ impl RequestType { _ => None, } } - pub fn to_prefix(&self) -> u8 { + pub fn to_u8(&self) -> u8 { match self { Self::Deposit => 0, Self::Withdrawal => 1,