From 6eee7488ed4898e2f792d683c22f0568ab6699cb Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Thu, 12 Sep 2024 17:37:19 -0700 Subject: [PATCH 1/7] Change request json types --- .../execution_layer/src/engine_api/http.rs | 53 +- .../src/engine_api/json_structures.rs | 473 +++++++++--------- .../src/test_utils/handle_rpc.rs | 2 +- consensus/types/src/consolidation_request.rs | 13 + consensus/types/src/deposit_request.rs | 15 + consensus/types/src/withdrawal_request.rs | 13 + 6 files changed, 331 insertions(+), 238 deletions(-) diff --git a/beacon_node/execution_layer/src/engine_api/http.rs b/beacon_node/execution_layer/src/engine_api/http.rs index c497a4a7254..4a14c304310 100644 --- a/beacon_node/execution_layer/src/engine_api/http.rs +++ b/beacon_node/execution_layer/src/engine_api/http.rs @@ -849,7 +849,9 @@ impl HttpJsonRpc { ENGINE_GET_PAYLOAD_TIMEOUT * self.execution_timeout_multiplier, ) .await?; - Ok(JsonGetPayloadResponse::V1(response).into()) + JsonGetPayloadResponse::V1(response) + .try_into() + .map_err(Error::BadResponse) } ForkName::Capella => { let response: JsonGetPayloadResponseV2 = self @@ -859,7 +861,9 @@ impl HttpJsonRpc { ENGINE_GET_PAYLOAD_TIMEOUT * self.execution_timeout_multiplier, ) .await?; - Ok(JsonGetPayloadResponse::V2(response).into()) + JsonGetPayloadResponse::V2(response) + .try_into() + .map_err(Error::BadResponse) } ForkName::Base | ForkName::Altair | ForkName::Deneb | ForkName::Electra => Err( Error::UnsupportedForkVariant(format!("called get_payload_v2 with {}", fork_name)), @@ -883,7 +887,9 @@ impl HttpJsonRpc { ENGINE_GET_PAYLOAD_TIMEOUT * self.execution_timeout_multiplier, ) .await?; - Ok(JsonGetPayloadResponse::V3(response).into()) + JsonGetPayloadResponse::V3(response) + .try_into() + .map_err(Error::BadResponse) } ForkName::Base | ForkName::Altair @@ -912,7 +918,9 @@ impl HttpJsonRpc { ENGINE_GET_PAYLOAD_TIMEOUT * self.execution_timeout_multiplier, ) .await?; - Ok(JsonGetPayloadResponse::V4(response).into()) + JsonGetPayloadResponse::V4(response) + .try_into() + .map_err(Error::BadResponse) } ForkName::Base | ForkName::Altair @@ -1004,7 +1012,7 @@ impl HttpJsonRpc { Ok(response .into_iter() - .map(|opt_json| opt_json.map(|v1| JsonExecutionPayloadBody::V1(v1).into())) + .map(|opt_json| opt_json.map(|v1| ExecutionPayloadBody::V1(v1.into()))) .collect()) } @@ -1022,10 +1030,19 @@ impl HttpJsonRpc { ) .await?; - Ok(response - .into_iter() - .map(|opt_json| opt_json.map(|v2| JsonExecutionPayloadBody::V2(v2).into())) - .collect()) + let mut responses = vec![]; + for resp in response.into_iter() { + if let Some(v2) = resp { + // Try decoding requests and return error if it fails + let body = + ExecutionPayloadBody::V2(v2.try_into().map_err(|e| Error::BadResponse(e))?); + responses.push(Some(body)); + } else { + responses.push(None); + } + } + + Ok(responses) } pub async fn get_payload_bodies_by_range_v1( @@ -1048,7 +1065,7 @@ impl HttpJsonRpc { Ok(response .into_iter() - .map(|opt_json| opt_json.map(|v1| JsonExecutionPayloadBody::V1(v1).into())) + .map(|opt_json| opt_json.map(|v1| ExecutionPayloadBody::V1(v1.into()))) .collect()) } @@ -1070,10 +1087,18 @@ impl HttpJsonRpc { ) .await?; - Ok(response - .into_iter() - .map(|opt_json| opt_json.map(|v2| JsonExecutionPayloadBody::V2(v2).into())) - .collect()) + let mut responses = vec![]; + for resp in response.into_iter() { + if let Some(v2) = resp { + // Try decoding requests and return error if it fails + let body = ExecutionPayloadBody::V2(v2.try_into().map_err(Error::BadResponse)?); + responses.push(Some(body)); + } else { + responses.push(None); + } + } + + Ok(responses) } pub async fn exchange_capabilities(&self) -> Result { 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 a05d584cfca..c6d5edea11f 100644 --- a/beacon_node/execution_layer/src/engine_api/json_structures.rs +++ b/beacon_node/execution_layer/src/engine_api/json_structures.rs @@ -1,6 +1,8 @@ use super::*; use alloy_rlp::RlpEncodable; +use bytes::{BufMut, Bytes, BytesMut}; use serde::{Deserialize, Serialize}; +use ssz::{Decode, Encode}; use strum::EnumString; use superstruct::superstruct; use types::beacon_block_body::KzgCommitments; @@ -105,13 +107,10 @@ pub struct JsonExecutionPayload { #[serde(with = "serde_utils::u64_hex_be")] pub excess_blob_gas: u64, #[superstruct(only(V4))] - pub deposit_requests: VariableList, - #[superstruct(only(V4))] - pub withdrawal_requests: - VariableList, - #[superstruct(only(V4))] - pub consolidation_requests: - VariableList, + // TODO(pawan): needs a hex encoded unbounded vec value serde transformation + // The transformation from JsonExecutionPayload to ExecutionPayload should fail + // if the length bounds are violated, but the json decoding should still pass + pub requests: Vec, } impl From> for JsonExecutionPayloadV1 { @@ -189,8 +188,66 @@ impl From> for JsonExecutionPayloadV3 { } } +#[derive(Debug, Copy, Clone)] +enum RequestPrefix { + Deposit, + Withdrawal, + Consolidation, +} + +impl RequestPrefix { + // TODO(pawan): get the final values from the spec + pub fn to_prefix(&self) -> u8 { + match self { + Self::Deposit => 0, + Self::Withdrawal => 1, + Self::Consolidation => 2, + } + } + + pub fn from_prefix(prefix: u8) -> Option { + match prefix { + 0 => Some(Self::Deposit), + 1 => Some(Self::Withdrawal), + 2 => Some(Self::Consolidation), + _ => None, + } + } +} + +fn prefix_and_bytes(request: &T, prefix: u8, max_size: usize) -> Bytes { + let mut bytes: Vec = Vec::with_capacity(max_size); + bytes.push(prefix); + bytes.append(&mut request.as_ssz_bytes()); + Bytes::from(bytes) +} + impl From> for JsonExecutionPayloadV4 { fn from(payload: ExecutionPayloadElectra) -> Self { + let deposit_requests = payload.deposit_requests.into_iter().map(|req| { + prefix_and_bytes( + &req, + RequestPrefix::Deposit.to_prefix(), + DepositRequest::max_size() + 1, + ) + }); + + let withdrawal_requests = payload.withdrawal_requests.into_iter().map(|req| { + prefix_and_bytes( + &req, + RequestPrefix::Withdrawal.to_prefix(), + WithdrawalRequest::max_size() + 1, + ) + }); + + let consolidation_requests = payload.consolidation_requests.into_iter().map(|req| { + prefix_and_bytes( + &req, + RequestPrefix::Consolidation.to_prefix(), + ConsolidationRequest::max_size() + 1, + ) + }); + JsonExecutionPayloadV4 { parent_hash: payload.parent_hash, fee_recipient: payload.fee_recipient, @@ -214,24 +271,10 @@ impl From> for JsonExecutionPayloadV4 .into(), blob_gas_used: payload.blob_gas_used, excess_blob_gas: payload.excess_blob_gas, - deposit_requests: payload - .deposit_requests - .into_iter() - .map(Into::into) - .collect::>() - .into(), - withdrawal_requests: payload - .withdrawal_requests - .into_iter() - .map(Into::into) - .collect::>() - .into(), - consolidation_requests: payload - .consolidation_requests - .into_iter() - .map(Into::into) - .collect::>() - .into(), + requests: deposit_requests + .chain(withdrawal_requests) + .chain(consolidation_requests) + .collect(), } } } @@ -323,9 +366,40 @@ impl From> for ExecutionPayloadDeneb { } } -impl From> for ExecutionPayloadElectra { - fn from(payload: JsonExecutionPayloadV4) -> Self { - ExecutionPayloadElectra { +impl TryFrom> for ExecutionPayloadElectra { + type Error = String; + + fn try_from(payload: JsonExecutionPayloadV4) -> Result { + let mut deposit_requests = Vec::with_capacity(E::max_deposit_requests_per_payload()); + let mut withdrawal_requests = Vec::with_capacity(E::max_withdrawal_requests_per_payload()); + let mut consolidation_requests = + Vec::with_capacity(E::max_consolidation_requests_per_payload()); + + // TODO(pawan): enforce ordering constraints here + for request in payload.requests.into_iter() { + if let Some((first, rest)) = request.split_first() { + match RequestPrefix::from_prefix(*first) { + Some(RequestPrefix::Deposit) => { + deposit_requests.push(DepositRequest::from_ssz_bytes(rest).map_err( + |e| format!("Failed to decode DepositRequest from EL: {:?}", e), + )?) + } + Some(RequestPrefix::Withdrawal) => { + withdrawal_requests.push(WithdrawalRequest::from_ssz_bytes(rest).map_err( + |e| format!("Failed to decode WithdrawalRequest from EL: {:?}", e), + )?) + } + Some(RequestPrefix::Consolidation) => consolidation_requests.push( + ConsolidationRequest::from_ssz_bytes(rest).map_err(|e| { + format!("Failed to decode ConsolidationRequest from EL: {:?}", e) + })?, + ), + None => return Err("Empty requests json".to_string()), + } + } + } + + Ok(ExecutionPayloadElectra { parent_hash: payload.parent_hash, fee_recipient: payload.fee_recipient, state_root: payload.state_root, @@ -348,35 +422,24 @@ impl From> for ExecutionPayloadElectra .into(), blob_gas_used: payload.blob_gas_used, excess_blob_gas: payload.excess_blob_gas, - deposit_requests: payload - .deposit_requests - .into_iter() - .map(Into::into) - .collect::>() - .into(), - withdrawal_requests: payload - .withdrawal_requests - .into_iter() - .map(Into::into) - .collect::>() - .into(), - consolidation_requests: payload - .consolidation_requests - .into_iter() - .map(Into::into) - .collect::>() - .into(), - } + deposit_requests: VariableList::new(deposit_requests) + .map_err(|_| "DepositRequests from EL exceeded length limits".to_string())?, + withdrawal_requests: VariableList::new(withdrawal_requests) + .map_err(|_| "WithdrawalRequests from EL exceeded length limits".to_string())?, + consolidation_requests: VariableList::new(consolidation_requests) + .map_err(|_| "ConsolidationRequests from EL exceeded length limits".to_string())?, + }) } } -impl From> for ExecutionPayload { - fn from(json_execution_payload: JsonExecutionPayload) -> Self { +impl TryFrom> for ExecutionPayload { + type Error = String; + fn try_from(json_execution_payload: JsonExecutionPayload) -> Result { match json_execution_payload { - JsonExecutionPayload::V1(payload) => ExecutionPayload::Bellatrix(payload.into()), - JsonExecutionPayload::V2(payload) => ExecutionPayload::Capella(payload.into()), - JsonExecutionPayload::V3(payload) => ExecutionPayload::Deneb(payload.into()), - JsonExecutionPayload::V4(payload) => ExecutionPayload::Electra(payload.into()), + JsonExecutionPayload::V1(payload) => Ok(ExecutionPayload::Bellatrix(payload.into())), + JsonExecutionPayload::V2(payload) => Ok(ExecutionPayload::Capella(payload.into())), + JsonExecutionPayload::V3(payload) => Ok(ExecutionPayload::Deneb(payload.into())), + JsonExecutionPayload::V4(payload) => Ok(ExecutionPayload::Electra(payload.try_into()?)), } } } @@ -409,36 +472,37 @@ pub struct JsonGetPayloadResponse { pub should_override_builder: bool, } -impl From> for GetPayloadResponse { - fn from(json_get_payload_response: JsonGetPayloadResponse) -> Self { +impl TryFrom> for GetPayloadResponse { + type Error = String; + fn try_from(json_get_payload_response: JsonGetPayloadResponse) -> Result { match json_get_payload_response { JsonGetPayloadResponse::V1(response) => { - GetPayloadResponse::Bellatrix(GetPayloadResponseBellatrix { + Ok(GetPayloadResponse::Bellatrix(GetPayloadResponseBellatrix { execution_payload: response.execution_payload.into(), block_value: response.block_value, - }) + })) } JsonGetPayloadResponse::V2(response) => { - GetPayloadResponse::Capella(GetPayloadResponseCapella { + Ok(GetPayloadResponse::Capella(GetPayloadResponseCapella { execution_payload: response.execution_payload.into(), block_value: response.block_value, - }) + })) } JsonGetPayloadResponse::V3(response) => { - GetPayloadResponse::Deneb(GetPayloadResponseDeneb { + Ok(GetPayloadResponse::Deneb(GetPayloadResponseDeneb { execution_payload: response.execution_payload.into(), block_value: response.block_value, blobs_bundle: response.blobs_bundle.into(), should_override_builder: response.should_override_builder, - }) + })) } JsonGetPayloadResponse::V4(response) => { - GetPayloadResponse::Electra(GetPayloadResponseElectra { - execution_payload: response.execution_payload.into(), + Ok(GetPayloadResponse::Electra(GetPayloadResponseElectra { + execution_payload: response.execution_payload.try_into()?, block_value: response.block_value, blobs_bundle: response.blobs_bundle.into(), should_override_builder: response.should_override_builder, - }) + })) } } } @@ -769,13 +833,7 @@ pub struct JsonExecutionPayloadBody { pub transactions: Transactions, pub withdrawals: Option>, #[superstruct(only(V2))] - pub deposit_requests: Option>, - #[superstruct(only(V2))] - pub withdrawal_requests: - Option>, - #[superstruct(only(V2))] - pub consolidation_requests: - Option>, + pub requests: Option>, } impl From> for JsonExecutionPayloadBodyV1 { @@ -794,84 +852,146 @@ impl From> for JsonExecutionPayloadBodyV1< } } -impl From> for JsonExecutionPayloadBodyV2 { - fn from(value: ExecutionPayloadBodyV2) -> Self { - Self { +impl From> for ExecutionPayloadBodyV1 { + fn from(value: JsonExecutionPayloadBodyV1) -> Self { + ExecutionPayloadBodyV1 { transactions: value.transactions, withdrawals: value.withdrawals.map(|json_withdrawals| { - VariableList::from( + Withdrawals::::from( json_withdrawals .into_iter() .map(Into::into) .collect::>(), ) }), - deposit_requests: value.deposit_requests.map(|receipts| { - VariableList::from(receipts.into_iter().map(Into::into).collect::>()) - }), - withdrawal_requests: value.withdrawal_requests.map(|withdrawal_requests| { - VariableList::from( - withdrawal_requests - .into_iter() - .map(Into::into) - .collect::>(), - ) - }), - consolidation_requests: value.consolidation_requests.map(|consolidation_requests| { + } + } +} + +impl From> for JsonExecutionPayloadBodyV2 { + fn from(value: ExecutionPayloadBodyV2) -> Self { + let mut requests = vec![]; + // TODO(pawan): make this more rusty + let mut flag = false; + if let Some(deposit_requests) = value.deposit_requests { + flag = true; + for req in deposit_requests.into_iter() { + requests.push(prefix_and_bytes( + &req, + RequestPrefix::Deposit.to_prefix(), + DepositRequest::max_size() + 1, + )); + } + } + + if let Some(withdrawal_requests) = value.withdrawal_requests { + flag = true; + for req in withdrawal_requests.into_iter() { + requests.push(prefix_and_bytes( + &req, + RequestPrefix::Withdrawal.to_prefix(), + WithdrawalRequest::max_size() + 1, + )); + } + } + + if let Some(consolidation_requests) = value.consolidation_requests { + flag = true; + for req in consolidation_requests.into_iter() { + requests.push(prefix_and_bytes( + &req, + RequestPrefix::Deposit.to_prefix(), + DepositRequest::max_size() + 1, + )); + } + } + + Self { + transactions: value.transactions, + withdrawals: value.withdrawals.map(|json_withdrawals| { VariableList::from( - consolidation_requests + json_withdrawals .into_iter() .map(Into::into) .collect::>(), ) }), + requests: flag.then(|| requests), } } } -impl From> for ExecutionPayloadBody { - fn from(value: JsonExecutionPayloadBody) -> Self { - match value { - JsonExecutionPayloadBody::V1(body_v1) => Self::V1(ExecutionPayloadBodyV1 { - transactions: body_v1.transactions, - withdrawals: body_v1.withdrawals.map(|json_withdrawals| { - Withdrawals::::from( - json_withdrawals - .into_iter() - .map(Into::into) - .collect::>(), - ) - }), - }), - JsonExecutionPayloadBody::V2(body_v2) => Self::V2(ExecutionPayloadBodyV2 { - transactions: body_v2.transactions, - withdrawals: body_v2.withdrawals.map(|json_withdrawals| { - Withdrawals::::from( - json_withdrawals - .into_iter() - .map(Into::into) - .collect::>(), - ) - }), - deposit_requests: body_v2.deposit_requests.map(|json_receipts| { - DepositRequests::::from( - json_receipts - .into_iter() - .map(Into::into) - .collect::>(), - ) - }), - withdrawal_requests: body_v2.withdrawal_requests.map(|json_withdrawal_requests| { - WithdrawalRequests::::from( - json_withdrawal_requests - .into_iter() - .map(Into::into) - .collect::>(), - ) - }), - consolidation_requests: body_v2.consolidation_requests, +impl TryFrom> for ExecutionPayloadBodyV2 { + type Error = String; + + fn try_from( + body_v2: JsonExecutionPayloadBodyV2, + ) -> Result, Self::Error> { + let (deposit_requests, withdrawal_requests, consolidation_requests) = + if let Some(requests) = body_v2.requests { + let mut deposit_requests = + Vec::with_capacity(E::max_deposit_requests_per_payload()); + let mut withdrawal_requests = + Vec::with_capacity(E::max_withdrawal_requests_per_payload()); + let mut consolidation_requests = + Vec::with_capacity(E::max_consolidation_requests_per_payload()); + + // TODO(pawan): enforce ordering constraints here + // Extract into separate function + for request in requests.into_iter() { + if let Some((first, rest)) = request.split_first() { + match RequestPrefix::from_prefix(*first) { + Some(RequestPrefix::Deposit) => deposit_requests.push( + DepositRequest::from_ssz_bytes(rest).map_err(|e| { + format!("Failed to decode DepositRequest from EL: {:?}", e) + })?, + ), + Some(RequestPrefix::Withdrawal) => withdrawal_requests.push( + WithdrawalRequest::from_ssz_bytes(rest).map_err(|e| { + format!("Failed to decode WithdrawalRequest from EL: {:?}", e) + })?, + ), + Some(RequestPrefix::Consolidation) => consolidation_requests.push( + ConsolidationRequest::from_ssz_bytes(rest).map_err(|e| { + format!( + "Failed to decode ConsolidationRequest from EL: {:?}", + e + ) + })?, + ), + None => return Err("Empty requests json".to_string()), + } + } + } + ( + Some(VariableList::new(deposit_requests).map_err(|_| { + "DepositRequests from EL exceeded length limits".to_string() + })?), + Some(VariableList::new(withdrawal_requests).map_err(|_| { + "WithdrawalRequests from EL exceeded length limits".to_string() + })?), + Some(VariableList::new(consolidation_requests).map_err(|_| { + "ConsolidationRequests from EL exceeded length limits".to_string() + })?), + ) + } else { + (None, None, None) + }; + + Ok(ExecutionPayloadBodyV2 { + transactions: body_v2.transactions, + withdrawals: body_v2.withdrawals.map(|json_withdrawals| { + Withdrawals::::from( + json_withdrawals + .into_iter() + .map(Into::into) + .collect::>(), + ) }), - } + deposit_requests, + withdrawal_requests, + consolidation_requests, + }) } } @@ -950,96 +1070,3 @@ impl TryFrom for ClientVersionV1 { }) } } - -#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] -#[serde(rename_all = "camelCase")] -pub struct JsonDepositRequest { - pub pubkey: PublicKeyBytes, - pub withdrawal_credentials: Hash256, - #[serde(with = "serde_utils::u64_hex_be")] - pub amount: u64, - pub signature: Signature, - #[serde(with = "serde_utils::u64_hex_be")] - pub index: u64, -} - -impl From for JsonDepositRequest { - fn from(deposit: DepositRequest) -> Self { - Self { - pubkey: deposit.pubkey, - withdrawal_credentials: deposit.withdrawal_credentials, - amount: deposit.amount, - signature: deposit.signature, - index: deposit.index, - } - } -} - -impl From for DepositRequest { - fn from(json_deposit: JsonDepositRequest) -> Self { - Self { - pubkey: json_deposit.pubkey, - withdrawal_credentials: json_deposit.withdrawal_credentials, - amount: json_deposit.amount, - signature: json_deposit.signature, - index: json_deposit.index, - } - } -} - -#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] -#[serde(rename_all = "camelCase")] -pub struct JsonWithdrawalRequest { - pub source_address: Address, - pub validator_pubkey: PublicKeyBytes, - #[serde(with = "serde_utils::u64_hex_be")] - pub amount: u64, -} - -impl From for JsonWithdrawalRequest { - fn from(withdrawal_request: WithdrawalRequest) -> Self { - Self { - source_address: withdrawal_request.source_address, - validator_pubkey: withdrawal_request.validator_pubkey, - amount: withdrawal_request.amount, - } - } -} - -impl From for WithdrawalRequest { - fn from(json_withdrawal_request: JsonWithdrawalRequest) -> Self { - Self { - source_address: json_withdrawal_request.source_address, - validator_pubkey: json_withdrawal_request.validator_pubkey, - amount: json_withdrawal_request.amount, - } - } -} - -#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] -#[serde(rename_all = "camelCase")] -pub struct JsonConsolidationRequest { - pub source_address: Address, - pub source_pubkey: PublicKeyBytes, - pub target_pubkey: PublicKeyBytes, -} - -impl From for JsonConsolidationRequest { - fn from(consolidation_request: ConsolidationRequest) -> Self { - Self { - source_address: consolidation_request.source_address, - source_pubkey: consolidation_request.source_pubkey, - target_pubkey: consolidation_request.target_pubkey, - } - } -} - -impl From for ConsolidationRequest { - fn from(json_consolidation_request: JsonConsolidationRequest) -> Self { - Self { - source_address: json_consolidation_request.source_address, - source_pubkey: json_consolidation_request.source_pubkey, - target_pubkey: json_consolidation_request.target_pubkey, - } - } -} diff --git a/beacon_node/execution_layer/src/test_utils/handle_rpc.rs b/beacon_node/execution_layer/src/test_utils/handle_rpc.rs index f36cb9797d3..6f5362a2ed8 100644 --- a/beacon_node/execution_layer/src/test_utils/handle_rpc.rs +++ b/beacon_node/execution_layer/src/test_utils/handle_rpc.rs @@ -247,7 +247,7 @@ pub async fn handle_rpc( Some( ctx.execution_block_generator .write() - .new_payload(request.into()), + .new_payload(request.try_into().unwrap()), ) } else { None diff --git a/consensus/types/src/consolidation_request.rs b/consensus/types/src/consolidation_request.rs index b21f34e7bba..e2df0bb9726 100644 --- a/consensus/types/src/consolidation_request.rs +++ b/consensus/types/src/consolidation_request.rs @@ -1,5 +1,6 @@ use crate::{test_utils::TestRandom, Address, PublicKeyBytes, SignedRoot}; use serde::{Deserialize, Serialize}; +use ssz::Encode; use ssz_derive::{Decode, Encode}; use test_random_derive::TestRandom; use tree_hash_derive::TreeHash; @@ -24,6 +25,18 @@ pub struct ConsolidationRequest { pub target_pubkey: PublicKeyBytes, } +impl ConsolidationRequest { + pub fn max_size() -> usize { + Self { + source_address: Address::repeat_byte(0), + source_pubkey: PublicKeyBytes::empty(), + target_pubkey: PublicKeyBytes::empty(), + } + .as_ssz_bytes() + .len() + } +} + impl SignedRoot for ConsolidationRequest {} #[cfg(test)] diff --git a/consensus/types/src/deposit_request.rs b/consensus/types/src/deposit_request.rs index f6ddf8b63a8..7af949fef3a 100644 --- a/consensus/types/src/deposit_request.rs +++ b/consensus/types/src/deposit_request.rs @@ -1,6 +1,7 @@ use crate::test_utils::TestRandom; use crate::{Hash256, PublicKeyBytes, Signature}; use serde::{Deserialize, Serialize}; +use ssz::Encode; use ssz_derive::{Decode, Encode}; use test_random_derive::TestRandom; use tree_hash_derive::TreeHash; @@ -29,6 +30,20 @@ pub struct DepositRequest { pub index: u64, } +impl DepositRequest { + pub fn max_size() -> usize { + Self { + pubkey: PublicKeyBytes::empty(), + withdrawal_credentials: Hash256::ZERO, + amount: 0, + signature: Signature::empty(), + index: 0, + } + .as_ssz_bytes() + .len() + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/consensus/types/src/withdrawal_request.rs b/consensus/types/src/withdrawal_request.rs index b6db0efb26d..1296426ac05 100644 --- a/consensus/types/src/withdrawal_request.rs +++ b/consensus/types/src/withdrawal_request.rs @@ -1,6 +1,7 @@ use crate::test_utils::TestRandom; use crate::{Address, PublicKeyBytes}; use serde::{Deserialize, Serialize}; +use ssz::Encode; use ssz_derive::{Decode, Encode}; use test_random_derive::TestRandom; use tree_hash_derive::TreeHash; @@ -27,6 +28,18 @@ pub struct WithdrawalRequest { pub amount: u64, } +impl WithdrawalRequest { + pub fn max_size() -> usize { + Self { + source_address: Address::repeat_byte(0), + validator_pubkey: PublicKeyBytes::empty(), + amount: 0, + } + .as_ssz_bytes() + .len() + } +} + #[cfg(test)] mod tests { use super::*; From 6c836e6ed44de005ca54dbf7b1e8d7fd0357d00e Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Thu, 19 Sep 2024 18:33:25 -0700 Subject: [PATCH 2/7] Move requests from payload to beacon block body --- consensus/types/src/beacon_block.rs | 2 + consensus/types/src/beacon_block_body.rs | 6 + consensus/types/src/execution_payload.rs | 19 +- .../types/src/execution_payload_header.rs | 12 - consensus/types/src/execution_requests.rs | 37 +++ consensus/types/src/lib.rs | 2 + consensus/types/src/payload.rs | 225 +----------------- consensus/types/src/signed_beacon_block.rs | 2 + 8 files changed, 57 insertions(+), 248 deletions(-) create mode 100644 consensus/types/src/execution_requests.rs diff --git a/consensus/types/src/beacon_block.rs b/consensus/types/src/beacon_block.rs index 4a6816c024d..a2983035138 100644 --- a/consensus/types/src/beacon_block.rs +++ b/consensus/types/src/beacon_block.rs @@ -670,6 +670,7 @@ impl> BeaconBlockElectra graffiti: Graffiti::default(), execution_payload: Payload::Electra::default(), blob_kzg_commitments: VariableList::empty(), + execution_requests: ExecutionRequests::default(), }, } } @@ -700,6 +701,7 @@ impl> EmptyBlock for BeaconBlockElec execution_payload: Payload::Electra::default(), bls_to_execution_changes: VariableList::empty(), blob_kzg_commitments: VariableList::empty(), + execution_requests: ExecutionRequests::default(), }, } } diff --git a/consensus/types/src/beacon_block_body.rs b/consensus/types/src/beacon_block_body.rs index 305ef105445..c81e7bcde93 100644 --- a/consensus/types/src/beacon_block_body.rs +++ b/consensus/types/src/beacon_block_body.rs @@ -114,6 +114,8 @@ pub struct BeaconBlockBody = FullPay VariableList, #[superstruct(only(Deneb, Electra))] pub blob_kzg_commitments: KzgCommitments, + #[superstruct(only(Electra))] + pub execution_requests: ExecutionRequests, #[superstruct(only(Base, Altair))] #[metastruct(exclude_from(fields))] #[ssz(skip_serializing, skip_deserializing)] @@ -662,6 +664,7 @@ impl From>> execution_payload: FullPayloadElectra { execution_payload }, bls_to_execution_changes, blob_kzg_commitments, + execution_requests, } = body; ( @@ -680,6 +683,7 @@ impl From>> }, bls_to_execution_changes, blob_kzg_commitments: blob_kzg_commitments.clone(), + execution_requests, }, Some(execution_payload), ) @@ -818,6 +822,7 @@ impl BeaconBlockBodyElectra> { execution_payload: FullPayloadElectra { execution_payload }, bls_to_execution_changes, blob_kzg_commitments, + execution_requests, } = self; BeaconBlockBodyElectra { @@ -835,6 +840,7 @@ impl BeaconBlockBodyElectra> { }, bls_to_execution_changes: bls_to_execution_changes.clone(), blob_kzg_commitments: blob_kzg_commitments.clone(), + execution_requests: execution_requests.clone(), } } } diff --git a/consensus/types/src/execution_payload.rs b/consensus/types/src/execution_payload.rs index 4d41d568308..913ad8d1715 100644 --- a/consensus/types/src/execution_payload.rs +++ b/consensus/types/src/execution_payload.rs @@ -13,12 +13,12 @@ pub type Transactions = VariableList< >; pub type Withdrawals = VariableList::MaxWithdrawalsPerPayload>; -pub type DepositRequests = - VariableList::MaxDepositRequestsPerPayload>; -pub type WithdrawalRequests = - VariableList::MaxWithdrawalRequestsPerPayload>; -pub type ConsolidationRequests = - VariableList::MaxConsolidationRequestsPerPayload>; +// pub type DepositRequests = +// VariableList::MaxDepositRequestsPerPayload>; +// pub type WithdrawalRequests = +// VariableList::MaxWithdrawalRequestsPerPayload>; +// pub type ConsolidationRequests = +// VariableList::MaxConsolidationRequestsPerPayload>; #[superstruct( variants(Bellatrix, Capella, Deneb, Electra), @@ -96,13 +96,6 @@ pub struct ExecutionPayload { #[superstruct(only(Deneb, Electra), partial_getter(copy))] #[serde(with = "serde_utils::quoted_u64")] pub excess_blob_gas: u64, - #[superstruct(only(Electra))] - pub deposit_requests: VariableList, - #[superstruct(only(Electra))] - pub withdrawal_requests: VariableList, - #[superstruct(only(Electra))] - pub consolidation_requests: - VariableList, } impl<'a, E: EthSpec> ExecutionPayloadRef<'a, E> { diff --git a/consensus/types/src/execution_payload_header.rs b/consensus/types/src/execution_payload_header.rs index 90dd8c54e21..e9690435f1f 100644 --- a/consensus/types/src/execution_payload_header.rs +++ b/consensus/types/src/execution_payload_header.rs @@ -86,12 +86,6 @@ pub struct ExecutionPayloadHeader { #[superstruct(only(Deneb, Electra), partial_getter(copy))] #[serde(with = "serde_utils::quoted_u64")] pub excess_blob_gas: u64, - #[superstruct(only(Electra), partial_getter(copy))] - pub deposit_requests_root: Hash256, - #[superstruct(only(Electra), partial_getter(copy))] - pub withdrawal_requests_root: Hash256, - #[superstruct(only(Electra), partial_getter(copy))] - pub consolidation_requests_root: Hash256, } impl ExecutionPayloadHeader { @@ -214,9 +208,6 @@ impl ExecutionPayloadHeaderDeneb { withdrawals_root: self.withdrawals_root, blob_gas_used: self.blob_gas_used, excess_blob_gas: self.excess_blob_gas, - deposit_requests_root: Hash256::zero(), - withdrawal_requests_root: Hash256::zero(), - consolidation_requests_root: Hash256::zero(), } } } @@ -308,9 +299,6 @@ impl<'a, E: EthSpec> From<&'a ExecutionPayloadElectra> for ExecutionPayloadHe withdrawals_root: payload.withdrawals.tree_hash_root(), blob_gas_used: payload.blob_gas_used, excess_blob_gas: payload.excess_blob_gas, - deposit_requests_root: payload.deposit_requests.tree_hash_root(), - withdrawal_requests_root: payload.withdrawal_requests.tree_hash_root(), - consolidation_requests_root: payload.consolidation_requests.tree_hash_root(), } } } diff --git a/consensus/types/src/execution_requests.rs b/consensus/types/src/execution_requests.rs new file mode 100644 index 00000000000..6a26e611ae0 --- /dev/null +++ b/consensus/types/src/execution_requests.rs @@ -0,0 +1,37 @@ +use crate::test_utils::TestRandom; +use crate::{ConsolidationRequest, DepositRequest, EthSpec, WithdrawalRequest}; +use derivative::Derivative; +use serde::{Deserialize, Serialize}; +use ssz_derive::{Decode, Encode}; +use ssz_types::VariableList; +use test_random_derive::TestRandom; +use tree_hash_derive::TreeHash; + +#[derive( + arbitrary::Arbitrary, + Debug, + Derivative, + Default, + Clone, + Serialize, + Deserialize, + Encode, + Decode, + TreeHash, + TestRandom, +)] +#[serde(bound = "E: EthSpec")] +#[arbitrary(bound = "E: EthSpec")] +#[derivative(PartialEq, Eq, Hash(bound = "E: EthSpec"))] +pub struct ExecutionRequests { + pub deposits: VariableList, + pub withdrawals: VariableList, + pub consolidations: VariableList, +} + +#[cfg(test)] +mod tests { + use super::*; + + ssz_and_tree_hash_tests!(ExecutionRequests); +} diff --git a/consensus/types/src/lib.rs b/consensus/types/src/lib.rs index 281a84d8592..e168199b98c 100644 --- a/consensus/types/src/lib.rs +++ b/consensus/types/src/lib.rs @@ -81,6 +81,7 @@ pub mod slot_epoch_macros; pub mod activation_queue; pub mod config_and_preset; pub mod execution_block_header; +pub mod execution_requests; pub mod fork_context; pub mod participation_flags; pub mod payload; @@ -169,6 +170,7 @@ pub use crate::execution_payload_header::{ ExecutionPayloadHeaderDeneb, ExecutionPayloadHeaderElectra, ExecutionPayloadHeaderRef, ExecutionPayloadHeaderRefMut, }; +pub use crate::execution_requests::ExecutionRequests; pub use crate::fork::Fork; pub use crate::fork_context::ForkContext; pub use crate::fork_data::ForkData; diff --git a/consensus/types/src/payload.rs b/consensus/types/src/payload.rs index cee8b8cc219..80a70c171f5 100644 --- a/consensus/types/src/payload.rs +++ b/consensus/types/src/payload.rs @@ -39,18 +39,6 @@ pub trait ExecPayload: Debug + Clone + PartialEq + Hash + TreeHash + /// fork-specific fields fn withdrawals_root(&self) -> Result; fn blob_gas_used(&self) -> Result; - fn withdrawal_requests( - &self, - ) -> Result>, Error>; - fn deposit_requests( - &self, - ) -> Result>, Error>; - fn consolidation_requests( - &self, - ) -> Result< - Option>, - Error, - >; /// Is this a default payload with 0x0 roots for transactions and withdrawals? fn is_default_with_zero_roots(&self) -> bool; @@ -290,51 +278,6 @@ impl ExecPayload for FullPayload { } } - fn withdrawal_requests( - &self, - ) -> Result>, Error> - { - match self { - FullPayload::Bellatrix(_) | FullPayload::Capella(_) | FullPayload::Deneb(_) => { - Err(Error::IncorrectStateVariant) - } - FullPayload::Electra(inner) => { - Ok(Some(inner.execution_payload.withdrawal_requests.clone())) - } - } - } - - fn deposit_requests( - &self, - ) -> Result>, Error> { - match self { - FullPayload::Bellatrix(_) | FullPayload::Capella(_) | FullPayload::Deneb(_) => { - Err(Error::IncorrectStateVariant) - } - FullPayload::Electra(inner) => { - Ok(Some(inner.execution_payload.deposit_requests.clone())) - } - } - } - - fn consolidation_requests( - &self, - ) -> Result< - Option< - VariableList::MaxConsolidationRequestsPerPayload>, - >, - Error, - > { - match self { - FullPayload::Bellatrix(_) | FullPayload::Capella(_) | FullPayload::Deneb(_) => { - Err(Error::IncorrectStateVariant) - } - FullPayload::Electra(inner) => { - Ok(Some(inner.execution_payload.consolidation_requests.clone())) - } - } - } - fn is_default_with_zero_roots<'a>(&'a self) -> bool { map_full_payload_ref!(&'a _, self.to_ref(), move |payload, cons| { cons(payload); @@ -467,51 +410,6 @@ impl<'b, E: EthSpec> ExecPayload for FullPayloadRef<'b, E> { } } - fn withdrawal_requests( - &self, - ) -> Result>, Error> - { - match self { - FullPayloadRef::Bellatrix(_) - | FullPayloadRef::Capella(_) - | FullPayloadRef::Deneb(_) => Err(Error::IncorrectStateVariant), - FullPayloadRef::Electra(inner) => { - Ok(Some(inner.execution_payload.withdrawal_requests.clone())) - } - } - } - - fn deposit_requests( - &self, - ) -> Result>, Error> { - match self { - FullPayloadRef::Bellatrix(_) - | FullPayloadRef::Capella(_) - | FullPayloadRef::Deneb(_) => Err(Error::IncorrectStateVariant), - FullPayloadRef::Electra(inner) => { - Ok(Some(inner.execution_payload.deposit_requests.clone())) - } - } - } - - fn consolidation_requests( - &self, - ) -> Result< - Option< - VariableList::MaxConsolidationRequestsPerPayload>, - >, - Error, - > { - match self { - FullPayloadRef::Bellatrix(_) - | FullPayloadRef::Capella(_) - | FullPayloadRef::Deneb(_) => Err(Error::IncorrectStateVariant), - FullPayloadRef::Electra(inner) => { - Ok(Some(inner.execution_payload.consolidation_requests.clone())) - } - } - } - fn is_default_with_zero_roots<'a>(&'a self) -> bool { map_full_payload_ref!(&'a _, self, move |payload, cons| { cons(payload); @@ -692,30 +590,6 @@ impl ExecPayload for BlindedPayload { } } - fn withdrawal_requests( - &self, - ) -> Result>, Error> - { - Ok(None) - } - - fn deposit_requests( - &self, - ) -> Result>, Error> { - Ok(None) - } - - fn consolidation_requests( - &self, - ) -> Result< - Option< - VariableList::MaxConsolidationRequestsPerPayload>, - >, - Error, - > { - Ok(None) - } - fn is_default_with_zero_roots(&self) -> bool { self.to_ref().is_default_with_zero_roots() } @@ -817,30 +691,6 @@ impl<'b, E: EthSpec> ExecPayload for BlindedPayloadRef<'b, E> { } } - fn withdrawal_requests( - &self, - ) -> Result>, Error> - { - Ok(None) - } - - fn deposit_requests( - &self, - ) -> Result>, Error> { - Ok(None) - } - - fn consolidation_requests( - &self, - ) -> Result< - Option< - VariableList::MaxConsolidationRequestsPerPayload>, - >, - Error, - > { - Ok(None) - } - fn is_default_with_zero_roots<'a>(&'a self) -> bool { map_blinded_payload_ref!(&'b _, self, move |payload, cons| { cons(payload); @@ -867,10 +717,7 @@ macro_rules! impl_exec_payload_common { $is_default_with_empty_roots:block, $f:block, $g:block, - $h:block, - $i:block, - $j:block, - $k:block) => { + $h:block) => { impl ExecPayload for $wrapper_type { fn block_type() -> BlockType { BlockType::$block_type_variant @@ -933,30 +780,6 @@ macro_rules! impl_exec_payload_common { let h = $h; h(self) } - - fn withdrawal_requests( - &self, - ) -> Result< - Option>, - Error, - > { - let i = $i; - i(self) - } - - fn deposit_requests( - &self, - ) -> Result>, Error> { - let j = $j; - j(self) - } - - fn consolidation_requests( - &self, - ) -> Result::MaxConsolidationRequestsPerPayload>>, Error> { - let k = $k; - k(self) - } } impl From<$wrapped_type> for $wrapper_type { @@ -1002,10 +825,7 @@ macro_rules! impl_exec_payload_for_fork { wrapper_ref_type.blob_gas_used() }; c - }, - { |_| { Ok(None) } }, - { |_| { Ok(None) } }, - { |_| { Ok(None) } } + } ); impl TryInto<$wrapper_type_header> for BlindedPayload { @@ -1092,47 +912,6 @@ macro_rules! impl_exec_payload_for_fork { wrapper_ref_type.blob_gas_used() }; c - }, - { - let c: for<'a> fn( - &'a $wrapper_type_full, - ) -> Result< - Option>, - Error, - > = |payload: &$wrapper_type_full| { - let wrapper_ref_type = FullPayloadRef::$fork_variant(&payload); - wrapper_ref_type.withdrawal_requests() - }; - c - }, - { - let c: for<'a> fn( - &'a $wrapper_type_full, - ) -> Result< - Option>, - Error, - > = |payload: &$wrapper_type_full| { - let wrapper_ref_type = FullPayloadRef::$fork_variant(&payload); - wrapper_ref_type.deposit_requests() - }; - c - }, - { - let c: for<'a> fn( - &'a $wrapper_type_full, - ) -> Result< - Option< - VariableList< - ConsolidationRequest, - ::MaxConsolidationRequestsPerPayload, - >, - >, - Error, - > = |payload: &$wrapper_type_full| { - let wrapper_ref_type = FullPayloadRef::$fork_variant(&payload); - wrapper_ref_type.consolidation_requests() - }; - c } ); diff --git a/consensus/types/src/signed_beacon_block.rs b/consensus/types/src/signed_beacon_block.rs index 4d3279a7f77..b52adcfe412 100644 --- a/consensus/types/src/signed_beacon_block.rs +++ b/consensus/types/src/signed_beacon_block.rs @@ -498,6 +498,7 @@ impl SignedBeaconBlockElectra> { execution_payload: BlindedPayloadElectra { .. }, bls_to_execution_changes, blob_kzg_commitments, + execution_requests, }, }, signature, @@ -521,6 +522,7 @@ impl SignedBeaconBlockElectra> { execution_payload: FullPayloadElectra { execution_payload }, bls_to_execution_changes, blob_kzg_commitments, + execution_requests, }, }, signature, From a582526cc24e41b09a5fb4c37a82415298132b14 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Tue, 24 Sep 2024 18:59:02 -0700 Subject: [PATCH 3/7] Refactor engine api --- .../beacon_chain/src/beacon_block_streamer.rs | 4 +- beacon_node/beacon_chain/src/beacon_chain.rs | 28 +- beacon_node/beacon_chain/src/errors.rs | 1 + beacon_node/execution_layer/src/engine_api.rs | 294 ++++++----------- .../execution_layer/src/engine_api/http.rs | 110 +------ .../src/engine_api/json_structures.rs | 298 +++++------------- beacon_node/execution_layer/src/lib.rs | 33 +- .../test_utils/execution_block_generator.rs | 4 - .../src/test_utils/handle_rpc.rs | 51 +-- .../src/test_utils/mock_builder.rs | 12 +- .../execution_layer/src/test_utils/mod.rs | 2 - .../process_operations.rs | 16 +- consensus/types/src/execution_payload.rs | 6 - consensus/types/src/execution_requests.rs | 13 +- 14 files changed, 247 insertions(+), 625 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_block_streamer.rs b/beacon_node/beacon_chain/src/beacon_block_streamer.rs index 198d7d61f09..b76dba88fd0 100644 --- a/beacon_node/beacon_chain/src/beacon_block_streamer.rs +++ b/beacon_node/beacon_chain/src/beacon_block_streamer.rs @@ -1,5 +1,5 @@ use crate::{metrics, BeaconChain, BeaconChainError, BeaconChainTypes, BlockProcessStatus}; -use execution_layer::{ExecutionLayer, ExecutionPayloadBody}; +use execution_layer::{ExecutionLayer, ExecutionPayloadBodyV1}; use slog::{crit, debug, error, Logger}; use std::collections::HashMap; use std::sync::Arc; @@ -57,7 +57,7 @@ struct BodiesByRange { struct BlockParts { blinded_block: Box>, header: Box>, - body: Option>>, + body: Option>>, } impl BlockParts { diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 13022b82699..5d7d7f0e06e 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -5553,10 +5553,15 @@ impl BeaconChain { ) } BeaconState::Deneb(_) => { - let (payload, kzg_commitments, maybe_blobs_and_proofs, execution_payload_value) = - block_contents - .ok_or(BlockProductionError::MissingExecutionPayload)? - .deconstruct(); + let ( + payload, + kzg_commitments, + maybe_blobs_and_proofs, + _maybe_requests, + execution_payload_value, + ) = block_contents + .ok_or(BlockProductionError::MissingExecutionPayload)? + .deconstruct(); ( BeaconBlock::Deneb(BeaconBlockDeneb { @@ -5591,10 +5596,15 @@ impl BeaconChain { ) } BeaconState::Electra(_) => { - let (payload, kzg_commitments, maybe_blobs_and_proofs, execution_payload_value) = - block_contents - .ok_or(BlockProductionError::MissingExecutionPayload)? - .deconstruct(); + let ( + payload, + kzg_commitments, + maybe_blobs_and_proofs, + maybe_requests, + execution_payload_value, + ) = block_contents + .ok_or(BlockProductionError::MissingExecutionPayload)? + .deconstruct(); ( BeaconBlock::Electra(BeaconBlockElectra { @@ -5619,6 +5629,8 @@ impl BeaconChain { bls_to_execution_changes: bls_to_execution_changes.into(), blob_kzg_commitments: kzg_commitments .ok_or(BlockProductionError::InvalidPayloadFork)?, + execution_requests: maybe_requests + .ok_or(BlockProductionError::MissingExecutionRequests)?, }, }), maybe_blobs_and_proofs, diff --git a/beacon_node/beacon_chain/src/errors.rs b/beacon_node/beacon_chain/src/errors.rs index 8a317ce7549..a26d7553163 100644 --- a/beacon_node/beacon_chain/src/errors.rs +++ b/beacon_node/beacon_chain/src/errors.rs @@ -294,6 +294,7 @@ pub enum BlockProductionError { InvalidBlockVariant(String), KzgError(kzg::Error), FailedToBuildBlobSidecars(String), + MissingExecutionRequests, } easy_from_to!(BlockProcessingError, BlockProductionError); diff --git a/beacon_node/execution_layer/src/engine_api.rs b/beacon_node/execution_layer/src/engine_api.rs index 8ba8ecfffbc..ab275e8b117 100644 --- a/beacon_node/execution_layer/src/engine_api.rs +++ b/beacon_node/execution_layer/src/engine_api.rs @@ -2,8 +2,7 @@ use crate::engines::ForkchoiceState; use crate::http::{ ENGINE_FORKCHOICE_UPDATED_V1, ENGINE_FORKCHOICE_UPDATED_V2, ENGINE_FORKCHOICE_UPDATED_V3, ENGINE_GET_CLIENT_VERSION_V1, ENGINE_GET_PAYLOAD_BODIES_BY_HASH_V1, - ENGINE_GET_PAYLOAD_BODIES_BY_HASH_V2, ENGINE_GET_PAYLOAD_BODIES_BY_RANGE_V1, - ENGINE_GET_PAYLOAD_BODIES_BY_RANGE_V2, ENGINE_GET_PAYLOAD_V1, ENGINE_GET_PAYLOAD_V2, + ENGINE_GET_PAYLOAD_BODIES_BY_RANGE_V1, ENGINE_GET_PAYLOAD_V1, ENGINE_GET_PAYLOAD_V2, ENGINE_GET_PAYLOAD_V3, ENGINE_GET_PAYLOAD_V4, ENGINE_NEW_PAYLOAD_V1, ENGINE_NEW_PAYLOAD_V2, ENGINE_NEW_PAYLOAD_V3, ENGINE_NEW_PAYLOAD_V4, }; @@ -18,7 +17,6 @@ use reqwest::StatusCode; use serde::{Deserialize, Serialize}; use strum::IntoStaticStr; use superstruct::superstruct; -use types::execution_payload::{ConsolidationRequests, DepositRequests, WithdrawalRequests}; pub use types::{ Address, BeaconBlockRef, ConsolidationRequest, EthSpec, ExecutionBlockHash, ExecutionPayload, ExecutionPayloadHeader, ExecutionPayloadRef, FixedVector, ForkName, Hash256, Transactions, @@ -26,7 +24,7 @@ pub use types::{ }; use types::{ ExecutionPayloadBellatrix, ExecutionPayloadCapella, ExecutionPayloadDeneb, - ExecutionPayloadElectra, KzgProofs, + ExecutionPayloadElectra, ExecutionRequests, KzgProofs, }; use types::{Graffiti, GRAFFITI_BYTES_LEN}; @@ -288,6 +286,8 @@ pub struct GetPayloadResponse { pub blobs_bundle: BlobsBundle, #[superstruct(only(Deneb, Electra), partial_getter(copy))] pub should_override_builder: bool, + #[superstruct(only(Electra))] + pub requests: ExecutionRequests, } impl GetPayloadResponse { @@ -321,7 +321,12 @@ impl From> for ExecutionPayload { } impl From> - for (ExecutionPayload, Uint256, Option>) + for ( + ExecutionPayload, + Uint256, + Option>, + Option>, + ) { fn from(response: GetPayloadResponse) -> Self { match response { @@ -329,21 +334,25 @@ impl From> ExecutionPayload::Bellatrix(inner.execution_payload), inner.block_value, None, + None, ), GetPayloadResponse::Capella(inner) => ( ExecutionPayload::Capella(inner.execution_payload), inner.block_value, None, + None, ), GetPayloadResponse::Deneb(inner) => ( ExecutionPayload::Deneb(inner.execution_payload), inner.block_value, Some(inner.blobs_bundle), + None, ), GetPayloadResponse::Electra(inner) => ( ExecutionPayload::Electra(inner.execution_payload), inner.block_value, Some(inner.blobs_bundle), + Some(inner.requests), ), } } @@ -360,106 +369,25 @@ impl GetPayloadResponse { } } -#[superstruct( - variants(V1, V2), - variant_attributes(derive(Clone, Debug),), - partial_getter_error(ty = "Error", expr = "Error::IncorrectStateVariant") -)] #[derive(Clone, Debug)] -pub struct ExecutionPayloadBody { +pub struct ExecutionPayloadBodyV1 { pub transactions: Transactions, pub withdrawals: Option>, - #[superstruct(only(V2))] - pub deposit_requests: Option>, - #[superstruct(only(V2))] - pub withdrawal_requests: Option>, - #[superstruct(only(V2))] - pub consolidation_requests: Option>, } -impl ExecutionPayloadBody { - #[allow(clippy::type_complexity)] - pub fn deconstruct( - self, - ) -> ( - Transactions, - Option>, - Option>, - Option>, - Option>, - ) { - match self { - ExecutionPayloadBody::V1(body) => { - (body.transactions, body.withdrawals, None, None, None) - } - ExecutionPayloadBody::V2(body) => ( - body.transactions, - body.withdrawals, - body.deposit_requests, - body.withdrawal_requests, - body.consolidation_requests, - ), - } - } +impl ExecutionPayloadBodyV1 { pub fn to_payload( self, header: ExecutionPayloadHeader, ) -> Result, String> { - let header_fork = header.fork_name_unchecked(); - match &self { - Self::V1(_) => { - if header_fork.electra_enabled() { + match header { + ExecutionPayloadHeader::Bellatrix(header) => { + if self.withdrawals.is_some() { return Err(format!( - "block {} is {} but response is ExecutionPayloadBodyV1. Does the EL support {}?", - header.block_hash(), - header_fork, - ENGINE_GET_PAYLOAD_BODIES_BY_HASH_V2, + "block {} is bellatrix but payload body has withdrawals", + header.block_hash )); } - } - Self::V2(_) => {} - } - - let ( - transactions, - withdrawals, - deposit_requests, - withdrawal_requests, - consolidation_requests, - ) = self.deconstruct(); - if !header_fork.capella_enabled() && withdrawals.is_some() { - return Err(format!( - "block {} is {} but payload body has withdrawals", - header.block_hash(), - header_fork - )); - } - if !header_fork.electra_enabled() { - if deposit_requests.is_some() { - return Err(format!( - "block {} is {} but payload body has deposit_requests", - header.block_hash(), - header_fork - )); - } - if withdrawal_requests.is_some() { - return Err(format!( - "block {} is {} but payload body has withdrawal_requests", - header.block_hash(), - header_fork - )); - } - if consolidation_requests.is_some() { - return Err(format!( - "block {} is {} but payload body has consolidation_requests", - header.block_hash(), - header_fork - )); - } - } - - match header { - ExecutionPayloadHeader::Bellatrix(header) => { Ok(ExecutionPayload::Bellatrix(ExecutionPayloadBellatrix { parent_hash: header.parent_hash, fee_recipient: header.fee_recipient, @@ -474,108 +402,90 @@ impl ExecutionPayloadBody { extra_data: header.extra_data, base_fee_per_gas: header.base_fee_per_gas, block_hash: header.block_hash, - transactions, + transactions: self.transactions, })) } ExecutionPayloadHeader::Capella(header) => { - let withdrawals = withdrawals.ok_or_else(|| { - format!( - "block {} is {} but payload body has withdrawals set to null", - header.block_hash, header_fork - ) - })?; - Ok(ExecutionPayload::Capella(ExecutionPayloadCapella { - parent_hash: header.parent_hash, - fee_recipient: header.fee_recipient, - state_root: header.state_root, - receipts_root: header.receipts_root, - logs_bloom: header.logs_bloom, - prev_randao: header.prev_randao, - block_number: header.block_number, - gas_limit: header.gas_limit, - gas_used: header.gas_used, - timestamp: header.timestamp, - extra_data: header.extra_data, - base_fee_per_gas: header.base_fee_per_gas, - block_hash: header.block_hash, - transactions, - withdrawals, - })) + if let Some(withdrawals) = self.withdrawals { + Ok(ExecutionPayload::Capella(ExecutionPayloadCapella { + parent_hash: header.parent_hash, + fee_recipient: header.fee_recipient, + state_root: header.state_root, + receipts_root: header.receipts_root, + logs_bloom: header.logs_bloom, + prev_randao: header.prev_randao, + block_number: header.block_number, + gas_limit: header.gas_limit, + gas_used: header.gas_used, + timestamp: header.timestamp, + extra_data: header.extra_data, + base_fee_per_gas: header.base_fee_per_gas, + block_hash: header.block_hash, + transactions: self.transactions, + withdrawals, + })) + } else { + Err(format!( + "block {} is capella but payload body doesn't have withdrawals", + header.block_hash + )) + } } ExecutionPayloadHeader::Deneb(header) => { - let withdrawals = withdrawals.ok_or_else(|| { - format!( - "block {} is {} but payload body has withdrawals set to null", - header.block_hash, header_fork - ) - })?; - Ok(ExecutionPayload::Deneb(ExecutionPayloadDeneb { - parent_hash: header.parent_hash, - fee_recipient: header.fee_recipient, - state_root: header.state_root, - receipts_root: header.receipts_root, - logs_bloom: header.logs_bloom, - prev_randao: header.prev_randao, - block_number: header.block_number, - gas_limit: header.gas_limit, - gas_used: header.gas_used, - timestamp: header.timestamp, - extra_data: header.extra_data, - base_fee_per_gas: header.base_fee_per_gas, - block_hash: header.block_hash, - transactions, - withdrawals, - blob_gas_used: header.blob_gas_used, - excess_blob_gas: header.excess_blob_gas, - })) + if let Some(withdrawals) = self.withdrawals { + Ok(ExecutionPayload::Deneb(ExecutionPayloadDeneb { + parent_hash: header.parent_hash, + fee_recipient: header.fee_recipient, + state_root: header.state_root, + receipts_root: header.receipts_root, + logs_bloom: header.logs_bloom, + prev_randao: header.prev_randao, + block_number: header.block_number, + gas_limit: header.gas_limit, + gas_used: header.gas_used, + timestamp: header.timestamp, + extra_data: header.extra_data, + base_fee_per_gas: header.base_fee_per_gas, + block_hash: header.block_hash, + transactions: self.transactions, + withdrawals, + blob_gas_used: header.blob_gas_used, + excess_blob_gas: header.excess_blob_gas, + })) + } else { + Err(format!( + "block {} is post capella but payload body doesn't have withdrawals", + header.block_hash + )) + } } ExecutionPayloadHeader::Electra(header) => { - let withdrawals = withdrawals.ok_or_else(|| { - format!( - "block {} is {} but payload body has withdrawals set to null", - header.block_hash, header_fork - ) - })?; - let deposit_requests = deposit_requests.ok_or_else(|| { - format!( - "block {} is {} but payload body has deposit_requests set to null", - header.block_hash, header_fork - ) - })?; - let withdrawal_requests = withdrawal_requests.ok_or_else(|| { - format!( - "block {} is {} but payload body has withdrawal_requests set to null", - header.block_hash, header_fork - ) - })?; - let consolidation_requests = consolidation_requests.ok_or_else(|| { - format!( - "block {} is {} but payload body has consolidation_requests set to null", - header.block_hash, header_fork - ) - })?; - Ok(ExecutionPayload::Electra(ExecutionPayloadElectra { - parent_hash: header.parent_hash, - fee_recipient: header.fee_recipient, - state_root: header.state_root, - receipts_root: header.receipts_root, - logs_bloom: header.logs_bloom, - prev_randao: header.prev_randao, - block_number: header.block_number, - gas_limit: header.gas_limit, - gas_used: header.gas_used, - timestamp: header.timestamp, - extra_data: header.extra_data, - base_fee_per_gas: header.base_fee_per_gas, - block_hash: header.block_hash, - transactions, - withdrawals, - blob_gas_used: header.blob_gas_used, - excess_blob_gas: header.excess_blob_gas, - deposit_requests, - withdrawal_requests, - consolidation_requests, - })) + if let Some(withdrawals) = self.withdrawals { + Ok(ExecutionPayload::Electra(ExecutionPayloadElectra { + parent_hash: header.parent_hash, + fee_recipient: header.fee_recipient, + state_root: header.state_root, + receipts_root: header.receipts_root, + logs_bloom: header.logs_bloom, + prev_randao: header.prev_randao, + block_number: header.block_number, + gas_limit: header.gas_limit, + gas_used: header.gas_used, + timestamp: header.timestamp, + extra_data: header.extra_data, + base_fee_per_gas: header.base_fee_per_gas, + block_hash: header.block_hash, + transactions: self.transactions, + withdrawals, + blob_gas_used: header.blob_gas_used, + excess_blob_gas: header.excess_blob_gas, + })) + } else { + Err(format!( + "block {} is post capella but payload body doesn't have withdrawals", + header.block_hash + )) + } } } } @@ -592,8 +502,6 @@ pub struct EngineCapabilities { pub forkchoice_updated_v3: bool, pub get_payload_bodies_by_hash_v1: bool, pub get_payload_bodies_by_range_v1: bool, - pub get_payload_bodies_by_hash_v2: bool, - pub get_payload_bodies_by_range_v2: bool, pub get_payload_v1: bool, pub get_payload_v2: bool, pub get_payload_v3: bool, @@ -631,12 +539,6 @@ impl EngineCapabilities { if self.get_payload_bodies_by_range_v1 { response.push(ENGINE_GET_PAYLOAD_BODIES_BY_RANGE_V1); } - if self.get_payload_bodies_by_hash_v2 { - response.push(ENGINE_GET_PAYLOAD_BODIES_BY_HASH_V2); - } - if self.get_payload_bodies_by_range_v2 { - response.push(ENGINE_GET_PAYLOAD_BODIES_BY_RANGE_V2); - } if self.get_payload_v1 { response.push(ENGINE_GET_PAYLOAD_V1); } diff --git a/beacon_node/execution_layer/src/engine_api/http.rs b/beacon_node/execution_layer/src/engine_api/http.rs index 4a14c304310..33a77f934b7 100644 --- a/beacon_node/execution_layer/src/engine_api/http.rs +++ b/beacon_node/execution_layer/src/engine_api/http.rs @@ -50,8 +50,6 @@ pub const ENGINE_FORKCHOICE_UPDATED_TIMEOUT: Duration = Duration::from_secs(8); pub const ENGINE_GET_PAYLOAD_BODIES_BY_HASH_V1: &str = "engine_getPayloadBodiesByHashV1"; pub const ENGINE_GET_PAYLOAD_BODIES_BY_RANGE_V1: &str = "engine_getPayloadBodiesByRangeV1"; -pub const ENGINE_GET_PAYLOAD_BODIES_BY_HASH_V2: &str = "engine_getPayloadBodiesByHashV2"; -pub const ENGINE_GET_PAYLOAD_BODIES_BY_RANGE_V2: &str = "engine_getPayloadBodiesByRangeV2"; pub const ENGINE_GET_PAYLOAD_BODIES_TIMEOUT: Duration = Duration::from_secs(10); pub const ENGINE_EXCHANGE_CAPABILITIES: &str = "engine_exchangeCapabilities"; @@ -80,8 +78,6 @@ pub static LIGHTHOUSE_CAPABILITIES: &[&str] = &[ ENGINE_FORKCHOICE_UPDATED_V3, ENGINE_GET_PAYLOAD_BODIES_BY_HASH_V1, ENGINE_GET_PAYLOAD_BODIES_BY_RANGE_V1, - ENGINE_GET_PAYLOAD_BODIES_BY_HASH_V2, - ENGINE_GET_PAYLOAD_BODIES_BY_RANGE_V2, ENGINE_GET_CLIENT_VERSION_V1, ]; @@ -999,7 +995,7 @@ impl HttpJsonRpc { pub async fn get_payload_bodies_by_hash_v1( &self, block_hashes: Vec, - ) -> Result>>, Error> { + ) -> Result>>, Error> { let params = json!([block_hashes]); let response: Vec>> = self @@ -1012,44 +1008,15 @@ impl HttpJsonRpc { Ok(response .into_iter() - .map(|opt_json| opt_json.map(|v1| ExecutionPayloadBody::V1(v1.into()))) + .map(|opt_json| opt_json.map(From::from)) .collect()) } - pub async fn get_payload_bodies_by_hash_v2( - &self, - block_hashes: Vec, - ) -> Result>>, Error> { - let params = json!([block_hashes]); - - let response: Vec>> = self - .rpc_request( - ENGINE_GET_PAYLOAD_BODIES_BY_HASH_V2, - params, - ENGINE_GET_PAYLOAD_BODIES_TIMEOUT * self.execution_timeout_multiplier, - ) - .await?; - - let mut responses = vec![]; - for resp in response.into_iter() { - if let Some(v2) = resp { - // Try decoding requests and return error if it fails - let body = - ExecutionPayloadBody::V2(v2.try_into().map_err(|e| Error::BadResponse(e))?); - responses.push(Some(body)); - } else { - responses.push(None); - } - } - - Ok(responses) - } - pub async fn get_payload_bodies_by_range_v1( &self, start: u64, count: u64, - ) -> Result>>, Error> { + ) -> Result>>, Error> { #[derive(Serialize)] #[serde(transparent)] struct Quantity(#[serde(with = "serde_utils::u64_hex_be")] u64); @@ -1065,42 +1032,10 @@ impl HttpJsonRpc { Ok(response .into_iter() - .map(|opt_json| opt_json.map(|v1| ExecutionPayloadBody::V1(v1.into()))) + .map(|opt_json| opt_json.map(From::from)) .collect()) } - pub async fn get_payload_bodies_by_range_v2( - &self, - start: u64, - count: u64, - ) -> Result>>, Error> { - #[derive(Serialize)] - #[serde(transparent)] - struct Quantity(#[serde(with = "serde_utils::u64_hex_be")] u64); - - let params = json!([Quantity(start), Quantity(count)]); - let response: Vec>> = self - .rpc_request( - ENGINE_GET_PAYLOAD_BODIES_BY_RANGE_V2, - params, - ENGINE_GET_PAYLOAD_BODIES_TIMEOUT * self.execution_timeout_multiplier, - ) - .await?; - - let mut responses = vec![]; - for resp in response.into_iter() { - if let Some(v2) = resp { - // Try decoding requests and return error if it fails - let body = ExecutionPayloadBody::V2(v2.try_into().map_err(Error::BadResponse)?); - responses.push(Some(body)); - } else { - responses.push(None); - } - } - - Ok(responses) - } - pub async fn exchange_capabilities(&self) -> Result { let params = json!([LIGHTHOUSE_CAPABILITIES]); @@ -1124,10 +1059,6 @@ impl HttpJsonRpc { .contains(ENGINE_GET_PAYLOAD_BODIES_BY_HASH_V1), get_payload_bodies_by_range_v1: capabilities .contains(ENGINE_GET_PAYLOAD_BODIES_BY_RANGE_V1), - get_payload_bodies_by_hash_v2: capabilities - .contains(ENGINE_GET_PAYLOAD_BODIES_BY_HASH_V2), - get_payload_bodies_by_range_v2: capabilities - .contains(ENGINE_GET_PAYLOAD_BODIES_BY_RANGE_V2), get_payload_v1: capabilities.contains(ENGINE_GET_PAYLOAD_V1), get_payload_v2: capabilities.contains(ENGINE_GET_PAYLOAD_V2), get_payload_v3: capabilities.contains(ENGINE_GET_PAYLOAD_V3), @@ -1303,39 +1234,6 @@ impl HttpJsonRpc { } } - pub async fn get_payload_bodies_by_hash( - &self, - block_hashes: Vec, - ) -> Result>>, Error> { - let engine_capabilities = self.get_engine_capabilities(None).await?; - if engine_capabilities.get_payload_bodies_by_hash_v2 { - self.get_payload_bodies_by_hash_v2(block_hashes).await - } else if engine_capabilities.get_payload_bodies_by_hash_v1 { - self.get_payload_bodies_by_hash_v1(block_hashes).await - } else { - Err(Error::RequiredMethodUnsupported( - "engine_getPayloadBodiesByHash", - )) - } - } - - pub async fn get_payload_bodies_by_range( - &self, - start: u64, - count: u64, - ) -> Result>>, Error> { - let engine_capabilities = self.get_engine_capabilities(None).await?; - if engine_capabilities.get_payload_bodies_by_range_v2 { - self.get_payload_bodies_by_range_v2(start, count).await - } else if engine_capabilities.get_payload_bodies_by_range_v1 { - self.get_payload_bodies_by_range_v1(start, count).await - } else { - Err(Error::RequiredMethodUnsupported( - "engine_getPayloadBodiesByRange", - )) - } - } - // automatically selects the latest version of // forkchoice_updated that the execution engine supports pub async fn forkchoice_updated( 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 c6d5edea11f..285c01c81b9 100644 --- a/beacon_node/execution_layer/src/engine_api/json_structures.rs +++ b/beacon_node/execution_layer/src/engine_api/json_structures.rs @@ -106,11 +106,6 @@ pub struct JsonExecutionPayload { #[superstruct(only(V3, V4))] #[serde(with = "serde_utils::u64_hex_be")] pub excess_blob_gas: u64, - #[superstruct(only(V4))] - // TODO(pawan): needs a hex encoded unbounded vec value serde transformation - // The transformation from JsonExecutionPayload to ExecutionPayload should fail - // if the length bounds are violated, but the json decoding should still pass - pub requests: Vec, } impl From> for JsonExecutionPayloadV1 { @@ -224,30 +219,6 @@ fn prefix_and_bytes(request: &T, prefix: u8, max_size: usize) -> Byte impl From> for JsonExecutionPayloadV4 { fn from(payload: ExecutionPayloadElectra) -> Self { - let deposit_requests = payload.deposit_requests.into_iter().map(|req| { - prefix_and_bytes( - &req, - RequestPrefix::Deposit.to_prefix(), - DepositRequest::max_size() + 1, - ) - }); - - let withdrawal_requests = payload.withdrawal_requests.into_iter().map(|req| { - prefix_and_bytes( - &req, - RequestPrefix::Withdrawal.to_prefix(), - WithdrawalRequest::max_size() + 1, - ) - }); - - let consolidation_requests = payload.consolidation_requests.into_iter().map(|req| { - prefix_and_bytes( - &req, - RequestPrefix::Consolidation.to_prefix(), - ConsolidationRequest::max_size() + 1, - ) - }); - JsonExecutionPayloadV4 { parent_hash: payload.parent_hash, fee_recipient: payload.fee_recipient, @@ -271,10 +242,6 @@ impl From> for JsonExecutionPayloadV4 .into(), blob_gas_used: payload.blob_gas_used, excess_blob_gas: payload.excess_blob_gas, - requests: deposit_requests - .chain(withdrawal_requests) - .chain(consolidation_requests) - .collect(), } } } @@ -366,40 +333,9 @@ impl From> for ExecutionPayloadDeneb { } } -impl TryFrom> for ExecutionPayloadElectra { - type Error = String; - - fn try_from(payload: JsonExecutionPayloadV4) -> Result { - let mut deposit_requests = Vec::with_capacity(E::max_deposit_requests_per_payload()); - let mut withdrawal_requests = Vec::with_capacity(E::max_withdrawal_requests_per_payload()); - let mut consolidation_requests = - Vec::with_capacity(E::max_consolidation_requests_per_payload()); - - // TODO(pawan): enforce ordering constraints here - for request in payload.requests.into_iter() { - if let Some((first, rest)) = request.split_first() { - match RequestPrefix::from_prefix(*first) { - Some(RequestPrefix::Deposit) => { - deposit_requests.push(DepositRequest::from_ssz_bytes(rest).map_err( - |e| format!("Failed to decode DepositRequest from EL: {:?}", e), - )?) - } - Some(RequestPrefix::Withdrawal) => { - withdrawal_requests.push(WithdrawalRequest::from_ssz_bytes(rest).map_err( - |e| format!("Failed to decode WithdrawalRequest from EL: {:?}", e), - )?) - } - Some(RequestPrefix::Consolidation) => consolidation_requests.push( - ConsolidationRequest::from_ssz_bytes(rest).map_err(|e| { - format!("Failed to decode ConsolidationRequest from EL: {:?}", e) - })?, - ), - None => return Err("Empty requests json".to_string()), - } - } - } - - Ok(ExecutionPayloadElectra { +impl From> for ExecutionPayloadElectra { + fn from(payload: JsonExecutionPayloadV4) -> Self { + ExecutionPayloadElectra { parent_hash: payload.parent_hash, fee_recipient: payload.fee_recipient, state_root: payload.state_root, @@ -422,25 +358,67 @@ impl TryFrom> for ExecutionPayloadElectra< .into(), blob_gas_used: payload.blob_gas_used, excess_blob_gas: payload.excess_blob_gas, - deposit_requests: VariableList::new(deposit_requests) - .map_err(|_| "DepositRequests from EL exceeded length limits".to_string())?, - withdrawal_requests: VariableList::new(withdrawal_requests) - .map_err(|_| "WithdrawalRequests from EL exceeded length limits".to_string())?, - consolidation_requests: VariableList::new(consolidation_requests) - .map_err(|_| "ConsolidationRequests from EL exceeded length limits".to_string())?, - }) + } } } -impl TryFrom> for ExecutionPayload { - type Error = String; - fn try_from(json_execution_payload: JsonExecutionPayload) -> Result { +impl From> for ExecutionPayload { + fn from(json_execution_payload: JsonExecutionPayload) -> Self { match json_execution_payload { - JsonExecutionPayload::V1(payload) => Ok(ExecutionPayload::Bellatrix(payload.into())), - JsonExecutionPayload::V2(payload) => Ok(ExecutionPayload::Capella(payload.into())), - JsonExecutionPayload::V3(payload) => Ok(ExecutionPayload::Deneb(payload.into())), - JsonExecutionPayload::V4(payload) => Ok(ExecutionPayload::Electra(payload.try_into()?)), + JsonExecutionPayload::V1(payload) => ExecutionPayload::Bellatrix(payload.into()), + JsonExecutionPayload::V2(payload) => ExecutionPayload::Capella(payload.into()), + JsonExecutionPayload::V3(payload) => ExecutionPayload::Deneb(payload.into()), + JsonExecutionPayload::V4(payload) => ExecutionPayload::Electra(payload.into()), + } + } +} + +#[derive(Debug, Default, Clone, PartialEq, Serialize, Deserialize)] +#[serde(transparent)] +/// TODO(pawan): https://github.com/ethereum/execution-apis/pull/587/ has a typed representation, but that's probably going to +/// be changed in https://github.com/ethereum/execution-apis/pull/577/ +pub struct JsonExecutionRequests(pub Vec); + +impl TryFrom for ExecutionRequests { + type Error = String; + + fn try_from(value: JsonExecutionRequests) -> Result { + let mut deposits = Vec::with_capacity(E::max_deposit_requests_per_payload()); + let mut withdrawals = Vec::with_capacity(E::max_withdrawal_requests_per_payload()); + let mut consolidations = Vec::with_capacity(E::max_consolidation_requests_per_payload()); + + // TODO(pawan): enforce ordering constraints here + for request in value.0.into_iter() { + if let Some((first, rest)) = request.split_first() { + match RequestPrefix::from_prefix(*first) { + Some(RequestPrefix::Deposit) => { + deposits.push(DepositRequest::from_ssz_bytes(rest).map_err(|e| { + format!("Failed to decode DepositRequest from EL: {:?}", e) + })?) + } + Some(RequestPrefix::Withdrawal) => { + withdrawals.push(WithdrawalRequest::from_ssz_bytes(rest).map_err(|e| { + format!("Failed to decode WithdrawalRequest from EL: {:?}", e) + })?) + } + Some(RequestPrefix::Consolidation) => { + consolidations.push(ConsolidationRequest::from_ssz_bytes(rest).map_err( + |e| format!("Failed to decode ConsolidationRequest from EL: {:?}", e), + )?) + } + None => return Err("Empty requests json".to_string()), + } + } } + + Ok(ExecutionRequests { + deposits: VariableList::new(deposits) + .map_err(|_| "DepositRequests from EL exceeded length limits".to_string())?, + withdrawals: VariableList::new(withdrawals) + .map_err(|_| "WithdrawalRequests from EL exceeded length limits".to_string())?, + consolidations: VariableList::new(consolidations) + .map_err(|_| "ConsolidationRequests from EL exceeded length limits".to_string())?, + }) } } @@ -470,6 +448,8 @@ pub struct JsonGetPayloadResponse { pub blobs_bundle: JsonBlobsBundleV1, #[superstruct(only(V3, V4))] pub should_override_builder: bool, + #[superstruct(only(V4))] + pub requests: JsonExecutionRequests, } impl TryFrom> for GetPayloadResponse { @@ -498,10 +478,11 @@ impl TryFrom> for GetPayloadResponse { } JsonGetPayloadResponse::V4(response) => { Ok(GetPayloadResponse::Electra(GetPayloadResponseElectra { - execution_payload: response.execution_payload.try_into()?, + execution_payload: response.execution_payload.into(), block_value: response.block_value, blobs_bundle: response.blobs_bundle.into(), should_override_builder: response.should_override_builder, + requests: response.requests.try_into()?, })) } } @@ -818,43 +799,17 @@ impl From for JsonForkchoiceUpdatedV1Response { } } -#[superstruct( - variants(V1, V2), - variant_attributes( - derive(Clone, Debug, Serialize, Deserialize), - serde(bound = "E: EthSpec", rename_all = "camelCase"), - ), - partial_getter_error(ty = "Error", expr = "Error::IncorrectStateVariant") -)] -#[derive(Clone, Debug, Serialize)] -#[serde(bound = "E: EthSpec", rename_all = "camelCase", untagged)] -pub struct JsonExecutionPayloadBody { +#[derive(Clone, Debug, Serialize, Deserialize)] +#[serde(bound = "E: EthSpec")] +pub struct JsonExecutionPayloadBodyV1 { #[serde(with = "ssz_types::serde_utils::list_of_hex_var_list")] pub transactions: Transactions, pub withdrawals: Option>, - #[superstruct(only(V2))] - pub requests: Option>, -} - -impl From> for JsonExecutionPayloadBodyV1 { - fn from(value: ExecutionPayloadBodyV1) -> Self { - Self { - transactions: value.transactions, - withdrawals: value.withdrawals.map(|json_withdrawals| { - VariableList::from( - json_withdrawals - .into_iter() - .map(Into::into) - .collect::>(), - ) - }), - } - } } impl From> for ExecutionPayloadBodyV1 { fn from(value: JsonExecutionPayloadBodyV1) -> Self { - ExecutionPayloadBodyV1 { + Self { transactions: value.transactions, withdrawals: value.withdrawals.map(|json_withdrawals| { Withdrawals::::from( @@ -868,133 +823,22 @@ impl From> for ExecutionPayloadBodyV1< } } -impl From> for JsonExecutionPayloadBodyV2 { - fn from(value: ExecutionPayloadBodyV2) -> Self { - let mut requests = vec![]; - // TODO(pawan): make this more rusty - let mut flag = false; - if let Some(deposit_requests) = value.deposit_requests { - flag = true; - for req in deposit_requests.into_iter() { - requests.push(prefix_and_bytes( - &req, - RequestPrefix::Deposit.to_prefix(), - DepositRequest::max_size() + 1, - )); - } - } - - if let Some(withdrawal_requests) = value.withdrawal_requests { - flag = true; - for req in withdrawal_requests.into_iter() { - requests.push(prefix_and_bytes( - &req, - RequestPrefix::Withdrawal.to_prefix(), - WithdrawalRequest::max_size() + 1, - )); - } - } - - if let Some(consolidation_requests) = value.consolidation_requests { - flag = true; - for req in consolidation_requests.into_iter() { - requests.push(prefix_and_bytes( - &req, - RequestPrefix::Deposit.to_prefix(), - DepositRequest::max_size() + 1, - )); - } - } - +impl From> for JsonExecutionPayloadBodyV1 { + fn from(value: ExecutionPayloadBodyV1) -> Self { Self { transactions: value.transactions, - withdrawals: value.withdrawals.map(|json_withdrawals| { + withdrawals: value.withdrawals.map(|withdrawals| { VariableList::from( - json_withdrawals + withdrawals .into_iter() .map(Into::into) .collect::>(), ) }), - requests: flag.then(|| requests), } } } -impl TryFrom> for ExecutionPayloadBodyV2 { - type Error = String; - - fn try_from( - body_v2: JsonExecutionPayloadBodyV2, - ) -> Result, Self::Error> { - let (deposit_requests, withdrawal_requests, consolidation_requests) = - if let Some(requests) = body_v2.requests { - let mut deposit_requests = - Vec::with_capacity(E::max_deposit_requests_per_payload()); - let mut withdrawal_requests = - Vec::with_capacity(E::max_withdrawal_requests_per_payload()); - let mut consolidation_requests = - Vec::with_capacity(E::max_consolidation_requests_per_payload()); - - // TODO(pawan): enforce ordering constraints here - // Extract into separate function - for request in requests.into_iter() { - if let Some((first, rest)) = request.split_first() { - match RequestPrefix::from_prefix(*first) { - Some(RequestPrefix::Deposit) => deposit_requests.push( - DepositRequest::from_ssz_bytes(rest).map_err(|e| { - format!("Failed to decode DepositRequest from EL: {:?}", e) - })?, - ), - Some(RequestPrefix::Withdrawal) => withdrawal_requests.push( - WithdrawalRequest::from_ssz_bytes(rest).map_err(|e| { - format!("Failed to decode WithdrawalRequest from EL: {:?}", e) - })?, - ), - Some(RequestPrefix::Consolidation) => consolidation_requests.push( - ConsolidationRequest::from_ssz_bytes(rest).map_err(|e| { - format!( - "Failed to decode ConsolidationRequest from EL: {:?}", - e - ) - })?, - ), - None => return Err("Empty requests json".to_string()), - } - } - } - ( - Some(VariableList::new(deposit_requests).map_err(|_| { - "DepositRequests from EL exceeded length limits".to_string() - })?), - Some(VariableList::new(withdrawal_requests).map_err(|_| { - "WithdrawalRequests from EL exceeded length limits".to_string() - })?), - Some(VariableList::new(consolidation_requests).map_err(|_| { - "ConsolidationRequests from EL exceeded length limits".to_string() - })?), - ) - } else { - (None, None, None) - }; - - Ok(ExecutionPayloadBodyV2 { - transactions: body_v2.transactions, - withdrawals: body_v2.withdrawals.map(|json_withdrawals| { - Withdrawals::::from( - json_withdrawals - .into_iter() - .map(Into::into) - .collect::>(), - ) - }), - deposit_requests, - withdrawal_requests, - consolidation_requests, - }) - } -} - #[derive(Clone, Copy, Debug, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct TransitionConfigurationV1 { diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index 648963a320e..393bee22689 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -48,7 +48,8 @@ use types::builder_bid::BuilderBid; use types::non_zero_usize::new_non_zero_usize; use types::payload::BlockProductionVersion; use types::{ - AbstractExecPayload, BlobsList, ExecutionPayloadDeneb, KzgProofs, SignedBlindedBeaconBlock, + AbstractExecPayload, BlobsList, ExecutionPayloadDeneb, ExecutionRequests, KzgProofs, + SignedBlindedBeaconBlock, }; use types::{ BeaconStateError, BlindedPayload, ChainSpec, Epoch, ExecPayload, ExecutionPayloadBellatrix, @@ -112,12 +113,15 @@ impl TryFrom> for ProvenancedPayload BlockProposalContents::PayloadAndBlobs { payload: ExecutionPayloadHeader::Electra(builder_bid.header).into(), block_value: builder_bid.value, kzg_commitments: builder_bid.blob_kzg_commitments, blobs_and_proofs: None, + // TODO(electra): update this with builder api returning the requests + requests: None, }, }; Ok(ProvenancedPayload::Builder( @@ -194,6 +198,8 @@ pub enum BlockProposalContents> { kzg_commitments: KzgCommitments, /// `None` for blinded `PayloadAndBlobs`. blobs_and_proofs: Option<(BlobsList, KzgProofs)>, + // TODO(pawan): this should probably be a separate variant/superstruct + requests: Option>, }, } @@ -214,11 +220,13 @@ impl From>> block_value, kzg_commitments, blobs_and_proofs: _, + requests, } => BlockProposalContents::PayloadAndBlobs { payload: payload.execution_payload().into(), block_value, kzg_commitments, blobs_and_proofs: None, + requests, }, } } @@ -230,13 +238,14 @@ impl> TryFrom> type Error = Error; fn try_from(response: GetPayloadResponse) -> Result { - let (execution_payload, block_value, maybe_bundle) = response.into(); + let (execution_payload, block_value, maybe_bundle, maybe_requests) = response.into(); match maybe_bundle { Some(bundle) => Ok(Self::PayloadAndBlobs { payload: execution_payload.into(), block_value, kzg_commitments: bundle.commitments, blobs_and_proofs: Some((bundle.blobs, bundle.proofs)), + requests: maybe_requests, }), None => Ok(Self::Payload { payload: execution_payload.into(), @@ -265,22 +274,25 @@ impl> BlockProposalContents>, Option<(BlobsList, KzgProofs)>, + Option>, Uint256, ) { match self { Self::Payload { payload, block_value, - } => (payload, None, None, block_value), + } => (payload, None, None, None, block_value), Self::PayloadAndBlobs { payload, block_value, kzg_commitments, blobs_and_proofs, + requests, } => ( payload, Some(kzg_commitments), blobs_and_proofs, + requests, block_value, ), } @@ -1772,10 +1784,10 @@ impl ExecutionLayer { pub async fn get_payload_bodies_by_hash( &self, hashes: Vec, - ) -> Result>>, Error> { + ) -> Result>>, Error> { self.engine() .request(|engine: &Engine| async move { - engine.api.get_payload_bodies_by_hash(hashes).await + engine.api.get_payload_bodies_by_hash_v1(hashes).await }) .await .map_err(Box::new) @@ -1786,11 +1798,14 @@ impl ExecutionLayer { &self, start: u64, count: u64, - ) -> Result>>, Error> { + ) -> Result>>, Error> { let _timer = metrics::start_timer(&metrics::EXECUTION_LAYER_GET_PAYLOAD_BODIES_BY_RANGE); self.engine() .request(|engine: &Engine| async move { - engine.api.get_payload_bodies_by_range(start, count).await + engine + .api + .get_payload_bodies_by_range_v1(start, count) + .await }) .await .map_err(Box::new) @@ -1823,9 +1838,7 @@ impl ExecutionLayer { // Use efficient payload bodies by range method if supported. let capabilities = self.get_engine_capabilities(None).await?; - if capabilities.get_payload_bodies_by_range_v1 - || capabilities.get_payload_bodies_by_range_v2 - { + if capabilities.get_payload_bodies_by_range_v1 { let mut payload_bodies = self.get_payload_bodies_by_range(block_number, 1).await?; if payload_bodies.len() != 1 { diff --git a/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs b/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs index 42f594fdf4b..4deb91e0567 100644 --- a/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs +++ b/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs @@ -652,10 +652,6 @@ impl ExecutionBlockGenerator { withdrawals: pa.withdrawals.clone().into(), blob_gas_used: 0, excess_blob_gas: 0, - // TODO(electra): consider how to test these fields below - deposit_requests: vec![].into(), - withdrawal_requests: vec![].into(), - consolidation_requests: vec![].into(), }), _ => unreachable!(), }, diff --git a/beacon_node/execution_layer/src/test_utils/handle_rpc.rs b/beacon_node/execution_layer/src/test_utils/handle_rpc.rs index 6f5362a2ed8..127460b19f9 100644 --- a/beacon_node/execution_layer/src/test_utils/handle_rpc.rs +++ b/beacon_node/execution_layer/src/test_utils/handle_rpc.rs @@ -373,6 +373,8 @@ pub async fn handle_rpc( ))? .into(), should_override_builder: false, + // TODO(pawan): add EL requests in mock el + requests: Default::default(), }) .unwrap() } @@ -565,56 +567,11 @@ pub async fn handle_rpc( !payload.fork_name().electra_enabled(), "payload bodies V1 is not supported for Electra blocks" ); - let payload_body = ExecutionPayloadBodyV1 { + let payload_body: ExecutionPayloadBodyV1 = ExecutionPayloadBodyV1 { transactions: payload.transactions().clone(), withdrawals: payload.withdrawals().ok().cloned(), }; - let json_payload_body = JsonExecutionPayloadBody::V1( - JsonExecutionPayloadBodyV1::::from(payload_body), - ); - response.push(Some(json_payload_body)); - } - None => response.push(None), - } - } - - Ok(serde_json::to_value(response).unwrap()) - } - ENGINE_GET_PAYLOAD_BODIES_BY_RANGE_V2 => { - #[derive(Deserialize)] - #[serde(transparent)] - struct Quantity(#[serde(with = "serde_utils::u64_hex_be")] pub u64); - - let start = get_param::(params, 0) - .map_err(|s| (s, BAD_PARAMS_ERROR_CODE))? - .0; - let count = get_param::(params, 1) - .map_err(|s| (s, BAD_PARAMS_ERROR_CODE))? - .0; - - let mut response = vec![]; - for block_num in start..(start + count) { - let maybe_payload = ctx - .execution_block_generator - .read() - .execution_payload_by_number(block_num); - - match maybe_payload { - Some(payload) => { - // TODO(electra): add testing for: - // deposit_requests - // withdrawal_requests - // consolidation_requests - let payload_body = ExecutionPayloadBodyV2 { - transactions: payload.transactions().clone(), - withdrawals: payload.withdrawals().ok().cloned(), - deposit_requests: payload.deposit_requests().ok().cloned(), - withdrawal_requests: payload.withdrawal_requests().ok().cloned(), - consolidation_requests: payload.consolidation_requests().ok().cloned(), - }; - let json_payload_body = JsonExecutionPayloadBody::V2( - JsonExecutionPayloadBodyV2::::from(payload_body), - ); + let json_payload_body = JsonExecutionPayloadBodyV1::from(payload_body); response.push(Some(json_payload_body)); } None => response.push(None), diff --git a/beacon_node/execution_layer/src/test_utils/mock_builder.rs b/beacon_node/execution_layer/src/test_utils/mock_builder.rs index 139ea069186..311551467e8 100644 --- a/beacon_node/execution_layer/src/test_utils/mock_builder.rs +++ b/beacon_node/execution_layer/src/test_utils/mock_builder.rs @@ -20,9 +20,9 @@ use types::builder_bid::{ }; use types::{ Address, BeaconState, ChainSpec, EthSpec, ExecPayload, ExecutionPayload, - ExecutionPayloadHeaderRefMut, FixedBytesExtended, ForkName, ForkVersionedResponse, Hash256, - PublicKeyBytes, Signature, SignedBlindedBeaconBlock, SignedRoot, - SignedValidatorRegistrationData, Slot, Uint256, + ExecutionPayloadHeaderRefMut, ExecutionRequests, FixedBytesExtended, ForkName, + ForkVersionedResponse, Hash256, PublicKeyBytes, Signature, SignedBlindedBeaconBlock, + SignedRoot, SignedValidatorRegistrationData, Slot, Uint256, }; use types::{ExecutionBlockHash, SecretKey}; use warp::{Filter, Rejection}; @@ -542,10 +542,11 @@ pub fn serve( let mut message = match payload_response_type { crate::GetPayloadResponseType::Full(payload_response) => { - let (payload, _block_value, maybe_blobs_bundle): ( + let (payload, _block_value, maybe_blobs_bundle, maybe_requests): ( ExecutionPayload, Uint256, Option>, + Option>, ) = payload_response.into(); match fork { @@ -593,10 +594,11 @@ pub fn serve( } } crate::GetPayloadResponseType::Blinded(payload_response) => { - let (payload, _block_value, maybe_blobs_bundle): ( + let (payload, _block_value, maybe_blobs_bundle, maybe_requests): ( ExecutionPayload, Uint256, Option>, + Option>, ) = payload_response.into(); match fork { ForkName::Electra => BuilderBid::Electra(BuilderBidElectra { diff --git a/beacon_node/execution_layer/src/test_utils/mod.rs b/beacon_node/execution_layer/src/test_utils/mod.rs index fe847ec3eda..be99b380543 100644 --- a/beacon_node/execution_layer/src/test_utils/mod.rs +++ b/beacon_node/execution_layer/src/test_utils/mod.rs @@ -47,9 +47,7 @@ pub const DEFAULT_ENGINE_CAPABILITIES: EngineCapabilities = EngineCapabilities { forkchoice_updated_v2: true, forkchoice_updated_v3: true, get_payload_bodies_by_hash_v1: true, - get_payload_bodies_by_hash_v2: true, get_payload_bodies_by_range_v1: true, - get_payload_bodies_by_range_v2: true, get_payload_v1: true, get_payload_v2: true, get_payload_v3: true, diff --git a/consensus/state_processing/src/per_block_processing/process_operations.rs b/consensus/state_processing/src/per_block_processing/process_operations.rs index 74166f67130..fb1c5c7eee2 100644 --- a/consensus/state_processing/src/per_block_processing/process_operations.rs +++ b/consensus/state_processing/src/per_block_processing/process_operations.rs @@ -40,15 +40,13 @@ pub fn process_operations>( if state.fork_name_unchecked().electra_enabled() { state.update_pubkey_cache()?; - if let Some(deposit_requests) = block_body.execution_payload()?.deposit_requests()? { - process_deposit_requests(state, &deposit_requests, spec)?; - } - if let Some(withdrawal_requests) = block_body.execution_payload()?.withdrawal_requests()? { - process_withdrawal_requests(state, &withdrawal_requests, spec)?; - } - if let Some(consolidations) = block_body.execution_payload()?.consolidation_requests()? { - process_consolidation_requests(state, &consolidations, spec)?; - } + process_deposit_requests(state, &block_body.execution_requests()?.deposits, spec)?; + process_withdrawal_requests(state, &block_body.execution_requests()?.withdrawals, spec)?; + process_consolidation_requests( + state, + &block_body.execution_requests()?.consolidations, + spec, + )?; } Ok(()) diff --git a/consensus/types/src/execution_payload.rs b/consensus/types/src/execution_payload.rs index 913ad8d1715..9f16b676a6a 100644 --- a/consensus/types/src/execution_payload.rs +++ b/consensus/types/src/execution_payload.rs @@ -13,12 +13,6 @@ pub type Transactions = VariableList< >; pub type Withdrawals = VariableList::MaxWithdrawalsPerPayload>; -// pub type DepositRequests = -// VariableList::MaxDepositRequestsPerPayload>; -// pub type WithdrawalRequests = -// VariableList::MaxWithdrawalRequestsPerPayload>; -// pub type ConsolidationRequests = -// VariableList::MaxConsolidationRequestsPerPayload>; #[superstruct( variants(Bellatrix, Capella, Deneb, Electra), diff --git a/consensus/types/src/execution_requests.rs b/consensus/types/src/execution_requests.rs index 6a26e611ae0..46f313f933f 100644 --- a/consensus/types/src/execution_requests.rs +++ b/consensus/types/src/execution_requests.rs @@ -7,6 +7,13 @@ use ssz_types::VariableList; use test_random_derive::TestRandom; use tree_hash_derive::TreeHash; +pub type DepositRequests = + VariableList::MaxDepositRequestsPerPayload>; +pub type WithdrawalRequests = + VariableList::MaxWithdrawalRequestsPerPayload>; +pub type ConsolidationRequests = + VariableList::MaxConsolidationRequestsPerPayload>; + #[derive( arbitrary::Arbitrary, Debug, @@ -24,9 +31,9 @@ use tree_hash_derive::TreeHash; #[arbitrary(bound = "E: EthSpec")] #[derivative(PartialEq, Eq, Hash(bound = "E: EthSpec"))] pub struct ExecutionRequests { - pub deposits: VariableList, - pub withdrawals: VariableList, - pub consolidations: VariableList, + pub deposits: DepositRequests, + pub withdrawals: WithdrawalRequests, + pub consolidations: ConsolidationRequests, } #[cfg(test)] From 9e614f1614f09a66b59003f1ab821b63d8f5e7ca Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Fri, 27 Sep 2024 16:40:53 -0700 Subject: [PATCH 4/7] Fix warnings --- .../src/engine_api/json_structures.rs | 29 +++---------------- .../src/test_utils/handle_rpc.rs | 2 +- .../src/test_utils/mock_builder.rs | 6 ++-- consensus/types/src/execution_requests.rs | 4 ++- 4 files changed, 12 insertions(+), 29 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 285c01c81b9..b0ff6f5236d 100644 --- a/beacon_node/execution_layer/src/engine_api/json_structures.rs +++ b/beacon_node/execution_layer/src/engine_api/json_structures.rs @@ -1,13 +1,13 @@ use super::*; use alloy_rlp::RlpEncodable; -use bytes::{BufMut, Bytes, BytesMut}; +use bytes::Bytes; use serde::{Deserialize, Serialize}; -use ssz::{Decode, Encode}; +use ssz::Decode; use strum::EnumString; use superstruct::superstruct; use types::beacon_block_body::KzgCommitments; use types::blob_sidecar::BlobsList; -use types::{DepositRequest, FixedVector, PublicKeyBytes, Signature, Unsigned, WithdrawalRequest}; +use types::{DepositRequest, FixedVector, Unsigned, WithdrawalRequest}; #[derive(Debug, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] @@ -191,15 +191,6 @@ enum RequestPrefix { } impl RequestPrefix { - // TODO(pawan): get the final values from the spec - pub fn to_prefix(&self) -> u8 { - match self { - Self::Deposit => 0, - Self::Withdrawal => 1, - Self::Consolidation => 2, - } - } - pub fn from_prefix(prefix: u8) -> Option { match prefix { 0 => Some(Self::Deposit), @@ -210,13 +201,6 @@ impl RequestPrefix { } } -fn prefix_and_bytes(request: &T, prefix: u8, max_size: usize) -> Bytes { - let mut bytes: Vec = Vec::with_capacity(max_size); - bytes.push(prefix); - bytes.append(&mut request.as_ssz_bytes()); - Bytes::from(bytes) -} - impl From> for JsonExecutionPayloadV4 { fn from(payload: ExecutionPayloadElectra) -> Self { JsonExecutionPayloadV4 { @@ -828,12 +812,7 @@ impl From> for JsonExecutionPayloadBodyV1< Self { transactions: value.transactions, withdrawals: value.withdrawals.map(|withdrawals| { - VariableList::from( - withdrawals - .into_iter() - .map(Into::into) - .collect::>(), - ) + VariableList::from(withdrawals.into_iter().map(Into::into).collect::>()) }), } } diff --git a/beacon_node/execution_layer/src/test_utils/handle_rpc.rs b/beacon_node/execution_layer/src/test_utils/handle_rpc.rs index 127460b19f9..8f8ac87b9a7 100644 --- a/beacon_node/execution_layer/src/test_utils/handle_rpc.rs +++ b/beacon_node/execution_layer/src/test_utils/handle_rpc.rs @@ -247,7 +247,7 @@ pub async fn handle_rpc( Some( ctx.execution_block_generator .write() - .new_payload(request.try_into().unwrap()), + .new_payload(request.into()), ) } else { None diff --git a/beacon_node/execution_layer/src/test_utils/mock_builder.rs b/beacon_node/execution_layer/src/test_utils/mock_builder.rs index 311551467e8..341daedbc8d 100644 --- a/beacon_node/execution_layer/src/test_utils/mock_builder.rs +++ b/beacon_node/execution_layer/src/test_utils/mock_builder.rs @@ -542,7 +542,8 @@ pub fn serve( let mut message = match payload_response_type { crate::GetPayloadResponseType::Full(payload_response) => { - let (payload, _block_value, maybe_blobs_bundle, maybe_requests): ( + #[allow(clippy::type_complexity)] + let (payload, _block_value, maybe_blobs_bundle, _maybe_requests): ( ExecutionPayload, Uint256, Option>, @@ -594,7 +595,8 @@ pub fn serve( } } crate::GetPayloadResponseType::Blinded(payload_response) => { - let (payload, _block_value, maybe_blobs_bundle, maybe_requests): ( + #[allow(clippy::type_complexity)] + let (payload, _block_value, maybe_blobs_bundle, _maybe_requests): ( ExecutionPayload, Uint256, Option>, diff --git a/consensus/types/src/execution_requests.rs b/consensus/types/src/execution_requests.rs index 46f313f933f..c0f6bac06d7 100644 --- a/consensus/types/src/execution_requests.rs +++ b/consensus/types/src/execution_requests.rs @@ -38,7 +38,9 @@ pub struct ExecutionRequests { #[cfg(test)] mod tests { + use crate::MainnetEthSpec; + use super::*; - ssz_and_tree_hash_tests!(ExecutionRequests); + ssz_and_tree_hash_tests!(ExecutionRequests); } From 9ecb16a1b1a5b962a9bf1c8841779e51be9bba7a Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Mon, 7 Oct 2024 12:31:22 -0700 Subject: [PATCH 5/7] Update engine api to latest --- Cargo.lock | 1 + beacon_node/execution_layer/Cargo.toml | 1 + .../execution_layer/src/engine_api/http.rs | 1 + .../src/engine_api/json_structures.rs | 67 +++++++++++-------- .../src/engine_api/new_payload_request.rs | 7 ++ 5 files changed, 48 insertions(+), 29 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3a063e7e0e7..ecbfd0cb8d1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3008,6 +3008,7 @@ dependencies = [ "sensitive_url", "serde", "serde_json", + "sha2 0.9.9", "slog", "slot_clock", "ssz_types", diff --git a/beacon_node/execution_layer/Cargo.toml b/beacon_node/execution_layer/Cargo.toml index 93d8086149d..843a7b83cb7 100644 --- a/beacon_node/execution_layer/Cargo.toml +++ b/beacon_node/execution_layer/Cargo.toml @@ -52,3 +52,4 @@ alloy-rlp = { workspace = true } alloy-consensus = { workspace = true } lighthouse_version = { workspace = true } fixed_bytes = { workspace = true } +sha2 = { workspace = true } diff --git a/beacon_node/execution_layer/src/engine_api/http.rs b/beacon_node/execution_layer/src/engine_api/http.rs index 33a77f934b7..e5ed2e640e1 100644 --- a/beacon_node/execution_layer/src/engine_api/http.rs +++ b/beacon_node/execution_layer/src/engine_api/http.rs @@ -793,6 +793,7 @@ impl HttpJsonRpc { JsonExecutionPayload::V4(new_payload_request_electra.execution_payload.clone().into()), new_payload_request_electra.versioned_hashes, new_payload_request_electra.parent_beacon_block_root, + new_payload_request_electra.execution_requests_hash, ]); let response: JsonPayloadStatusV1 = self 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 b0ff6f5236d..13aac62b7fc 100644 --- a/beacon_node/execution_layer/src/engine_api/json_structures.rs +++ b/beacon_node/execution_layer/src/engine_api/json_structures.rs @@ -1,13 +1,14 @@ use super::*; use alloy_rlp::RlpEncodable; -use bytes::Bytes; use serde::{Deserialize, Serialize}; -use ssz::Decode; +use sha2::{Digest, Sha256}; +use ssz::{Decode, Encode}; use strum::EnumString; use superstruct::superstruct; use types::beacon_block_body::KzgCommitments; use types::blob_sidecar::BlobsList; -use types::{DepositRequest, FixedVector, Unsigned, WithdrawalRequest}; +use types::execution_requests::{ConsolidationRequests, DepositRequests, WithdrawalRequests}; +use types::{FixedVector, Unsigned}; #[derive(Debug, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] @@ -201,6 +202,18 @@ impl RequestPrefix { } } +/// Computes the hash of the `ExecutionRequests` based on the specification +/// in EIP-7685. +pub fn compute_execution_requests_hash(request: &ExecutionRequests) -> Hash256 { + let depsoits_hash = Sha256::digest(&request.deposits.as_ssz_bytes()); + let withdrawals_hash = Sha256::digest(&request.withdrawals.as_ssz_bytes()); + let consolidation_hash = Sha256::digest(&request.consolidations.as_ssz_bytes()); + + Hash256::from_slice(&Sha256::digest( + &[depsoits_hash, withdrawals_hash, consolidation_hash].concat(), + )) +} + impl From> for JsonExecutionPayloadV4 { fn from(payload: ExecutionPayloadElectra) -> Self { JsonExecutionPayloadV4 { @@ -357,52 +370,48 @@ impl From> for ExecutionPayload { } } +/// Format of `ExecutionRequests` received over json rpc is +/// +/// Array of `prefix-type ++ ssz-encoded requests list` encoded as hex bytes. #[derive(Debug, Default, Clone, PartialEq, Serialize, Deserialize)] #[serde(transparent)] -/// TODO(pawan): https://github.com/ethereum/execution-apis/pull/587/ has a typed representation, but that's probably going to -/// be changed in https://github.com/ethereum/execution-apis/pull/577/ -pub struct JsonExecutionRequests(pub Vec); +pub struct JsonExecutionRequests(pub Vec); impl TryFrom for ExecutionRequests { type Error = String; fn try_from(value: JsonExecutionRequests) -> Result { - let mut deposits = Vec::with_capacity(E::max_deposit_requests_per_payload()); - let mut withdrawals = Vec::with_capacity(E::max_withdrawal_requests_per_payload()); - let mut consolidations = Vec::with_capacity(E::max_consolidation_requests_per_payload()); + let mut requests = ExecutionRequests::default(); - // TODO(pawan): enforce ordering constraints here for request in value.0.into_iter() { - if let Some((first, rest)) = request.split_first() { + // hex string + let decoded_bytes = hex::decode(request).map_err(|e| format!("Invalid hex {:?}", e))?; + if let Some((first, rest)) = decoded_bytes.split_first() { match RequestPrefix::from_prefix(*first) { Some(RequestPrefix::Deposit) => { - deposits.push(DepositRequest::from_ssz_bytes(rest).map_err(|e| { - format!("Failed to decode DepositRequest from EL: {:?}", e) - })?) + requests.deposits = + DepositRequests::::from_ssz_bytes(rest).map_err(|e| { + format!("Failed to decode DepositRequest from EL: {:?}", e) + })?; } Some(RequestPrefix::Withdrawal) => { - withdrawals.push(WithdrawalRequest::from_ssz_bytes(rest).map_err(|e| { - format!("Failed to decode WithdrawalRequest from EL: {:?}", e) - })?) + requests.withdrawals = WithdrawalRequests::::from_ssz_bytes(rest) + .map_err(|e| { + format!("Failed to decode WithdrawalRequest from EL: {:?}", e) + })?; } Some(RequestPrefix::Consolidation) => { - consolidations.push(ConsolidationRequest::from_ssz_bytes(rest).map_err( - |e| format!("Failed to decode ConsolidationRequest from EL: {:?}", e), - )?) + requests.consolidations = ConsolidationRequests::::from_ssz_bytes(rest) + .map_err(|e| { + format!("Failed to decode ConsolidationRequest from EL: {:?}", e) + })?; } - None => return Err("Empty requests json".to_string()), + None => return Err("Empty requests string".to_string()), } } } - Ok(ExecutionRequests { - deposits: VariableList::new(deposits) - .map_err(|_| "DepositRequests from EL exceeded length limits".to_string())?, - withdrawals: VariableList::new(withdrawals) - .map_err(|_| "WithdrawalRequests from EL exceeded length limits".to_string())?, - consolidations: VariableList::new(consolidations) - .map_err(|_| "ConsolidationRequests from EL exceeded length limits".to_string())?, - }) + Ok(requests) } } diff --git a/beacon_node/execution_layer/src/engine_api/new_payload_request.rs b/beacon_node/execution_layer/src/engine_api/new_payload_request.rs index 8d2e3d5ad06..13565ab67b9 100644 --- a/beacon_node/execution_layer/src/engine_api/new_payload_request.rs +++ b/beacon_node/execution_layer/src/engine_api/new_payload_request.rs @@ -12,6 +12,8 @@ use types::{ ExecutionPayloadElectra, }; +use super::json_structures::compute_execution_requests_hash; + #[superstruct( variants(Bellatrix, Capella, Deneb, Electra), variant_attributes(derive(Clone, Debug, PartialEq),), @@ -43,6 +45,8 @@ pub struct NewPayloadRequest<'block, E: EthSpec> { pub versioned_hashes: Vec, #[superstruct(only(Deneb, Electra))] pub parent_beacon_block_root: Hash256, + #[superstruct(only(Electra))] + pub execution_requests_hash: Hash256, } impl<'block, E: EthSpec> NewPayloadRequest<'block, E> { @@ -183,6 +187,9 @@ impl<'a, E: EthSpec> TryFrom> for NewPayloadRequest<'a, E> .map(kzg_commitment_to_versioned_hash) .collect(), parent_beacon_block_root: block_ref.parent_root, + execution_requests_hash: compute_execution_requests_hash( + &block_ref.body.execution_requests, + ), })), } } From a5ce9f6e7f130f01289ff83e00f3f35201e78494 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Wed, 9 Oct 2024 11:45:50 -0700 Subject: [PATCH 6/7] engine api changed..again --- .../execution_layer/src/engine_api/http.rs | 2 +- .../src/engine_api/json_structures.rs | 15 +-------------- .../src/engine_api/new_payload_request.rs | 12 ++++++------ consensus/types/src/execution_requests.rs | 13 +++++++++++++ 4 files changed, 21 insertions(+), 21 deletions(-) diff --git a/beacon_node/execution_layer/src/engine_api/http.rs b/beacon_node/execution_layer/src/engine_api/http.rs index e5ed2e640e1..960ee192322 100644 --- a/beacon_node/execution_layer/src/engine_api/http.rs +++ b/beacon_node/execution_layer/src/engine_api/http.rs @@ -793,7 +793,7 @@ impl HttpJsonRpc { JsonExecutionPayload::V4(new_payload_request_electra.execution_payload.clone().into()), new_payload_request_electra.versioned_hashes, new_payload_request_electra.parent_beacon_block_root, - new_payload_request_electra.execution_requests_hash, + new_payload_request_electra.execution_requests_list, ]); let response: JsonPayloadStatusV1 = self 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 13aac62b7fc..1eefe74383c 100644 --- a/beacon_node/execution_layer/src/engine_api/json_structures.rs +++ b/beacon_node/execution_layer/src/engine_api/json_structures.rs @@ -1,8 +1,7 @@ use super::*; use alloy_rlp::RlpEncodable; use serde::{Deserialize, Serialize}; -use sha2::{Digest, Sha256}; -use ssz::{Decode, Encode}; +use ssz::Decode; use strum::EnumString; use superstruct::superstruct; use types::beacon_block_body::KzgCommitments; @@ -202,18 +201,6 @@ impl RequestPrefix { } } -/// Computes the hash of the `ExecutionRequests` based on the specification -/// in EIP-7685. -pub fn compute_execution_requests_hash(request: &ExecutionRequests) -> Hash256 { - let depsoits_hash = Sha256::digest(&request.deposits.as_ssz_bytes()); - let withdrawals_hash = Sha256::digest(&request.withdrawals.as_ssz_bytes()); - let consolidation_hash = Sha256::digest(&request.consolidations.as_ssz_bytes()); - - Hash256::from_slice(&Sha256::digest( - &[depsoits_hash, withdrawals_hash, consolidation_hash].concat(), - )) -} - impl From> for JsonExecutionPayloadV4 { fn from(payload: ExecutionPayloadElectra) -> Self { JsonExecutionPayloadV4 { diff --git a/beacon_node/execution_layer/src/engine_api/new_payload_request.rs b/beacon_node/execution_layer/src/engine_api/new_payload_request.rs index 13565ab67b9..59e3d65abb6 100644 --- a/beacon_node/execution_layer/src/engine_api/new_payload_request.rs +++ b/beacon_node/execution_layer/src/engine_api/new_payload_request.rs @@ -1,6 +1,7 @@ use crate::{block_hash::calculate_execution_block_hash, metrics, Error}; use crate::versioned_hashes::verify_versioned_hashes; +use alloy_primitives::Bytes; use state_processing::per_block_processing::deneb::kzg_commitment_to_versioned_hash; use superstruct::superstruct; use types::{ @@ -12,8 +13,6 @@ use types::{ ExecutionPayloadElectra, }; -use super::json_structures::compute_execution_requests_hash; - #[superstruct( variants(Bellatrix, Capella, Deneb, Electra), variant_attributes(derive(Clone, Debug, PartialEq),), @@ -46,7 +45,7 @@ pub struct NewPayloadRequest<'block, E: EthSpec> { #[superstruct(only(Deneb, Electra))] pub parent_beacon_block_root: Hash256, #[superstruct(only(Electra))] - pub execution_requests_hash: Hash256, + pub execution_requests_list: Vec, } impl<'block, E: EthSpec> NewPayloadRequest<'block, E> { @@ -187,9 +186,10 @@ impl<'a, E: EthSpec> TryFrom> for NewPayloadRequest<'a, E> .map(kzg_commitment_to_versioned_hash) .collect(), parent_beacon_block_root: block_ref.parent_root, - execution_requests_hash: compute_execution_requests_hash( - &block_ref.body.execution_requests, - ), + execution_requests_list: block_ref + .body + .execution_requests + .get_execution_requests_list(), })), } } diff --git a/consensus/types/src/execution_requests.rs b/consensus/types/src/execution_requests.rs index c0f6bac06d7..778260dd841 100644 --- a/consensus/types/src/execution_requests.rs +++ b/consensus/types/src/execution_requests.rs @@ -1,7 +1,9 @@ use crate::test_utils::TestRandom; use crate::{ConsolidationRequest, DepositRequest, EthSpec, WithdrawalRequest}; +use alloy_primitives::Bytes; use derivative::Derivative; use serde::{Deserialize, Serialize}; +use ssz::Encode; use ssz_derive::{Decode, Encode}; use ssz_types::VariableList; use test_random_derive::TestRandom; @@ -36,6 +38,17 @@ pub struct ExecutionRequests { pub consolidations: ConsolidationRequests, } +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] + } +} + #[cfg(test)] mod tests { use crate::MainnetEthSpec; From b60f5cfae925583a3467f56c21a29fca7ac76635 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Wed, 9 Oct 2024 13:17:34 -0700 Subject: [PATCH 7/7] yet again --- .../src/engine_api/json_structures.rs | 88 ++++++++++--------- 1 file changed, 45 insertions(+), 43 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 1eefe74383c..128d96dd230 100644 --- a/beacon_node/execution_layer/src/engine_api/json_structures.rs +++ b/beacon_node/execution_layer/src/engine_api/json_structures.rs @@ -183,24 +183,6 @@ impl From> for JsonExecutionPayloadV3 { } } -#[derive(Debug, Copy, Clone)] -enum RequestPrefix { - Deposit, - Withdrawal, - Consolidation, -} - -impl RequestPrefix { - pub fn from_prefix(prefix: u8) -> Option { - match prefix { - 0 => Some(Self::Deposit), - 1 => Some(Self::Withdrawal), - 2 => Some(Self::Consolidation), - _ => None, - } - } -} - impl From> for JsonExecutionPayloadV4 { fn from(payload: ExecutionPayloadElectra) -> Self { JsonExecutionPayloadV4 { @@ -357,9 +339,34 @@ impl From> for ExecutionPayload { } } -/// Format of `ExecutionRequests` received over json rpc is +/// This is used to index into the `execution_requests` array. +#[derive(Debug, Copy, Clone)] +enum RequestPrefix { + Deposit, + Withdrawal, + Consolidation, +} + +impl RequestPrefix { + pub fn from_prefix(prefix: u8) -> Option { + match prefix { + 0 => Some(Self::Deposit), + 1 => Some(Self::Withdrawal), + 2 => Some(Self::Consolidation), + _ => None, + } + } +} + +/// 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. /// -/// Array of `prefix-type ++ ssz-encoded requests list` encoded as hex bytes. +/// 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) #[derive(Debug, Default, Clone, PartialEq, Serialize, Deserialize)] #[serde(transparent)] pub struct JsonExecutionRequests(pub Vec); @@ -370,34 +377,29 @@ impl TryFrom for ExecutionRequests { fn try_from(value: JsonExecutionRequests) -> Result { let mut requests = ExecutionRequests::default(); - for request in value.0.into_iter() { + for (i, request) in value.0.into_iter().enumerate() { // hex string let decoded_bytes = hex::decode(request).map_err(|e| format!("Invalid hex {:?}", e))?; - if let Some((first, rest)) = decoded_bytes.split_first() { - match RequestPrefix::from_prefix(*first) { - Some(RequestPrefix::Deposit) => { - requests.deposits = - DepositRequests::::from_ssz_bytes(rest).map_err(|e| { - format!("Failed to decode DepositRequest from EL: {:?}", e) - })?; - } - Some(RequestPrefix::Withdrawal) => { - requests.withdrawals = WithdrawalRequests::::from_ssz_bytes(rest) - .map_err(|e| { - format!("Failed to decode WithdrawalRequest from EL: {:?}", e) - })?; - } - Some(RequestPrefix::Consolidation) => { - requests.consolidations = ConsolidationRequests::::from_ssz_bytes(rest) - .map_err(|e| { - format!("Failed to decode ConsolidationRequest from EL: {:?}", e) - })?; - } - None => return Err("Empty requests string".to_string()), + 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))?; + } + Some(RequestPrefix::Withdrawal) => { + requests.withdrawals = WithdrawalRequests::::from_ssz_bytes(&decoded_bytes) + .map_err(|e| { + format!("Failed to decode WithdrawalRequest from EL: {:?}", e) + })?; } + Some(RequestPrefix::Consolidation) => { + requests.consolidations = + ConsolidationRequests::::from_ssz_bytes(&decoded_bytes).map_err( + |e| format!("Failed to decode ConsolidationRequest from EL: {:?}", e), + )?; + } + None => return Err("Empty requests string".to_string()), } } - Ok(requests) } }