From b350f18fc814c08bd9b56f1376fd1bfd4030a00b Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 2 Aug 2024 15:53:11 +1000 Subject: [PATCH 1/3] Delete legacy payload reconstruction --- .../beacon_chain/src/bellatrix_readiness.rs | 45 ----- beacon_node/execution_layer/src/engine_api.rs | 184 ----------------- .../execution_layer/src/engine_api/http.rs | 48 ----- .../src/engine_api/json_structures.rs | 27 +++ beacon_node/execution_layer/src/lib.rs | 185 +----------------- beacon_node/execution_layer/src/metrics.rs | 7 - .../test_utils/execution_block_generator.rs | 44 ++--- .../src/test_utils/handle_rpc.rs | 62 ++---- .../src/test_rig.rs | 10 +- 9 files changed, 63 insertions(+), 549 deletions(-) diff --git a/beacon_node/beacon_chain/src/bellatrix_readiness.rs b/beacon_node/beacon_chain/src/bellatrix_readiness.rs index 60b1abaf098..7b3b3f728de 100644 --- a/beacon_node/beacon_chain/src/bellatrix_readiness.rs +++ b/beacon_node/beacon_chain/src/bellatrix_readiness.rs @@ -4,7 +4,6 @@ use crate::{BeaconChain, BeaconChainError as Error, BeaconChainTypes}; use execution_layer::BlockByNumberQuery; use serde::{Deserialize, Serialize, Serializer}; -use slog::debug; use std::fmt; use std::fmt::Write; use types::*; @@ -199,7 +198,6 @@ impl BeaconChain { else { return Ok(GenesisExecutionPayloadStatus::Irrelevant); }; - let fork = self.spec.fork_name_at_epoch(Epoch::new(0)); let execution_layer = self .execution_layer @@ -222,49 +220,6 @@ impl BeaconChain { }); } - // Double-check the block by reconstructing it. - let execution_payload = execution_layer - .get_payload_by_hash_legacy(exec_block_hash, fork) - .await - .map_err(|e| Error::ExecutionLayerGetBlockByHashFailed(Box::new(e)))? - .ok_or(Error::BlockHashMissingFromExecutionLayer(exec_block_hash))?; - - // Verify payload integrity. - let header_from_payload = ExecutionPayloadHeader::from(execution_payload.to_ref()); - - let got_transactions_root = header_from_payload.transactions_root(); - let expected_transactions_root = latest_execution_payload_header.transactions_root(); - let got_withdrawals_root = header_from_payload.withdrawals_root().ok(); - let expected_withdrawals_root = latest_execution_payload_header.withdrawals_root().ok(); - - if got_transactions_root != expected_transactions_root { - return Ok(GenesisExecutionPayloadStatus::TransactionsRootMismatch { - got: got_transactions_root, - expected: expected_transactions_root, - }); - } - - if let Some(expected) = expected_withdrawals_root { - if let Some(got) = got_withdrawals_root { - if got != expected { - return Ok(GenesisExecutionPayloadStatus::WithdrawalsRootMismatch { - got, - expected, - }); - } - } - } - - if header_from_payload.to_ref() != latest_execution_payload_header { - debug!( - self.log, - "Genesis execution payload reconstruction failure"; - "consensus_node_header" => ?latest_execution_payload_header, - "execution_node_header" => ?header_from_payload - ); - return Ok(GenesisExecutionPayloadStatus::OtherMismatch); - } - Ok(GenesisExecutionPayloadStatus::Correct(exec_block_hash)) } } diff --git a/beacon_node/execution_layer/src/engine_api.rs b/beacon_node/execution_layer/src/engine_api.rs index 6a56a5d076f..2e5d11c71e4 100644 --- a/beacon_node/execution_layer/src/engine_api.rs +++ b/beacon_node/execution_layer/src/engine_api.rs @@ -10,9 +10,6 @@ use eth2::types::{ BlobsBundle, SsePayloadAttributes, SsePayloadAttributesV1, SsePayloadAttributesV2, SsePayloadAttributesV3, }; -use ethers_core::types::Transaction; -use ethers_core::utils::rlp; -use ethers_core::utils::rlp::{Decodable, Rlp}; use http::deposit_methods::RpcError; pub use json_structures::{JsonWithdrawal, TransitionConfigurationV1}; use pretty_reqwest_error::PrettyReqwestError; @@ -42,8 +39,6 @@ pub use new_payload_request::{ NewPayloadRequestDeneb, NewPayloadRequestElectra, }; -use self::json_structures::{JsonDepositRequest, JsonWithdrawalRequest}; - pub const LATEST_TAG: &str = "latest"; pub type PayloadId = [u8; 8]; @@ -73,7 +68,6 @@ pub enum Error { RequiredMethodUnsupported(&'static str), UnsupportedForkVariant(String), InvalidClientVersion(String), - RlpDecoderError(rlp::DecoderError), } impl From for Error { @@ -107,12 +101,6 @@ impl From for Error { } } -impl From for Error { - fn from(e: rlp::DecoderError) -> Self { - Error::RlpDecoderError(e) - } -} - impl From for Error { fn from(e: ssz_types::Error) -> Self { Error::SszError(e) @@ -158,178 +146,6 @@ pub struct ExecutionBlock { pub timestamp: u64, } -/// Representation of an execution block with enough detail to reconstruct a payload. -#[superstruct( - variants(Bellatrix, Capella, Deneb, Electra), - variant_attributes( - derive(Clone, Debug, PartialEq, Serialize, Deserialize,), - serde(bound = "E: EthSpec", rename_all = "camelCase"), - ), - cast_error(ty = "Error", expr = "Error::IncorrectStateVariant"), - partial_getter_error(ty = "Error", expr = "Error::IncorrectStateVariant") -)] -#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] -#[serde(bound = "E: EthSpec", rename_all = "camelCase", untagged)] -pub struct ExecutionBlockWithTransactions { - pub parent_hash: ExecutionBlockHash, - #[serde(alias = "miner")] - pub fee_recipient: Address, - pub state_root: Hash256, - pub receipts_root: Hash256, - #[serde(with = "ssz_types::serde_utils::hex_fixed_vec")] - pub logs_bloom: FixedVector, - #[serde(alias = "mixHash")] - pub prev_randao: Hash256, - #[serde(rename = "number", with = "serde_utils::u64_hex_be")] - pub block_number: u64, - #[serde(with = "serde_utils::u64_hex_be")] - pub gas_limit: u64, - #[serde(with = "serde_utils::u64_hex_be")] - pub gas_used: u64, - #[serde(with = "serde_utils::u64_hex_be")] - pub timestamp: u64, - #[serde(with = "ssz_types::serde_utils::hex_var_list")] - pub extra_data: VariableList, - pub base_fee_per_gas: Uint256, - #[serde(rename = "hash")] - pub block_hash: ExecutionBlockHash, - pub transactions: Vec, - #[superstruct(only(Capella, Deneb, Electra))] - pub withdrawals: Vec, - #[superstruct(only(Deneb, Electra))] - #[serde(with = "serde_utils::u64_hex_be")] - pub blob_gas_used: u64, - #[superstruct(only(Deneb, Electra))] - #[serde(with = "serde_utils::u64_hex_be")] - pub excess_blob_gas: u64, - #[superstruct(only(Electra))] - pub deposit_requests: Vec, - #[superstruct(only(Electra))] - pub withdrawal_requests: Vec, -} - -impl TryFrom> for ExecutionBlockWithTransactions { - type Error = Error; - - fn try_from(payload: ExecutionPayload) -> Result { - let json_payload = match payload { - ExecutionPayload::Bellatrix(block) => { - Self::Bellatrix(ExecutionBlockWithTransactionsBellatrix { - parent_hash: block.parent_hash, - fee_recipient: block.fee_recipient, - state_root: block.state_root, - receipts_root: block.receipts_root, - logs_bloom: block.logs_bloom, - prev_randao: block.prev_randao, - block_number: block.block_number, - gas_limit: block.gas_limit, - gas_used: block.gas_used, - timestamp: block.timestamp, - extra_data: block.extra_data, - base_fee_per_gas: block.base_fee_per_gas, - block_hash: block.block_hash, - transactions: block - .transactions - .iter() - .map(|tx| Transaction::decode(&Rlp::new(tx))) - .collect::, _>>()?, - }) - } - ExecutionPayload::Capella(block) => { - Self::Capella(ExecutionBlockWithTransactionsCapella { - parent_hash: block.parent_hash, - fee_recipient: block.fee_recipient, - state_root: block.state_root, - receipts_root: block.receipts_root, - logs_bloom: block.logs_bloom, - prev_randao: block.prev_randao, - block_number: block.block_number, - gas_limit: block.gas_limit, - gas_used: block.gas_used, - timestamp: block.timestamp, - extra_data: block.extra_data, - base_fee_per_gas: block.base_fee_per_gas, - block_hash: block.block_hash, - transactions: block - .transactions - .iter() - .map(|tx| Transaction::decode(&Rlp::new(tx))) - .collect::, _>>()?, - withdrawals: Vec::from(block.withdrawals) - .into_iter() - .map(|withdrawal| withdrawal.into()) - .collect(), - }) - } - ExecutionPayload::Deneb(block) => Self::Deneb(ExecutionBlockWithTransactionsDeneb { - parent_hash: block.parent_hash, - fee_recipient: block.fee_recipient, - state_root: block.state_root, - receipts_root: block.receipts_root, - logs_bloom: block.logs_bloom, - prev_randao: block.prev_randao, - block_number: block.block_number, - gas_limit: block.gas_limit, - gas_used: block.gas_used, - timestamp: block.timestamp, - extra_data: block.extra_data, - base_fee_per_gas: block.base_fee_per_gas, - block_hash: block.block_hash, - transactions: block - .transactions - .iter() - .map(|tx| Transaction::decode(&Rlp::new(tx))) - .collect::, _>>()?, - withdrawals: Vec::from(block.withdrawals) - .into_iter() - .map(|withdrawal| withdrawal.into()) - .collect(), - blob_gas_used: block.blob_gas_used, - excess_blob_gas: block.excess_blob_gas, - }), - ExecutionPayload::Electra(block) => { - Self::Electra(ExecutionBlockWithTransactionsElectra { - parent_hash: block.parent_hash, - fee_recipient: block.fee_recipient, - state_root: block.state_root, - receipts_root: block.receipts_root, - logs_bloom: block.logs_bloom, - prev_randao: block.prev_randao, - block_number: block.block_number, - gas_limit: block.gas_limit, - gas_used: block.gas_used, - timestamp: block.timestamp, - extra_data: block.extra_data, - base_fee_per_gas: block.base_fee_per_gas, - block_hash: block.block_hash, - transactions: block - .transactions - .iter() - .map(|tx| Transaction::decode(&Rlp::new(tx))) - .collect::, _>>()?, - withdrawals: Vec::from(block.withdrawals) - .into_iter() - .map(|withdrawal| withdrawal.into()) - .collect(), - blob_gas_used: block.blob_gas_used, - excess_blob_gas: block.excess_blob_gas, - deposit_requests: block - .deposit_requests - .into_iter() - .map(|deposit| deposit.into()) - .collect(), - withdrawal_requests: block - .withdrawal_requests - .into_iter() - .map(|withdrawal| withdrawal.into()) - .collect(), - }) - } - }; - Ok(json_payload) - } -} - #[superstruct( variants(V1, V2, V3), variant_attributes(derive(Clone, Debug, Eq, Hash, PartialEq),), diff --git a/beacon_node/execution_layer/src/engine_api/http.rs b/beacon_node/execution_layer/src/engine_api/http.rs index ecaf9c6c23e..beb247c23cd 100644 --- a/beacon_node/execution_layer/src/engine_api/http.rs +++ b/beacon_node/execution_layer/src/engine_api/http.rs @@ -730,54 +730,6 @@ impl HttpJsonRpc { .await } - pub async fn get_block_by_hash_with_txns( - &self, - block_hash: ExecutionBlockHash, - fork: ForkName, - ) -> Result>, Error> { - let params = json!([block_hash, true]); - Ok(Some(match fork { - ForkName::Bellatrix => ExecutionBlockWithTransactions::Bellatrix( - self.rpc_request( - ETH_GET_BLOCK_BY_HASH, - params, - ETH_GET_BLOCK_BY_HASH_TIMEOUT * self.execution_timeout_multiplier, - ) - .await?, - ), - ForkName::Capella => ExecutionBlockWithTransactions::Capella( - self.rpc_request( - ETH_GET_BLOCK_BY_HASH, - params, - ETH_GET_BLOCK_BY_HASH_TIMEOUT * self.execution_timeout_multiplier, - ) - .await?, - ), - ForkName::Deneb => ExecutionBlockWithTransactions::Deneb( - self.rpc_request( - ETH_GET_BLOCK_BY_HASH, - params, - ETH_GET_BLOCK_BY_HASH_TIMEOUT * self.execution_timeout_multiplier, - ) - .await?, - ), - ForkName::Electra => ExecutionBlockWithTransactions::Electra( - self.rpc_request( - ETH_GET_BLOCK_BY_HASH, - params, - ETH_GET_BLOCK_BY_HASH_TIMEOUT * self.execution_timeout_multiplier, - ) - .await?, - ), - ForkName::Base | ForkName::Altair => { - return Err(Error::UnsupportedForkVariant(format!( - "called get_block_by_hash_with_txns with fork {:?}", - fork - ))) - } - })) - } - pub async fn new_payload_v1( &self, execution_payload: ExecutionPayload, 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 f654ba4a0ea..d58df63786c 100644 --- a/beacon_node/execution_layer/src/engine_api/json_structures.rs +++ b/beacon_node/execution_layer/src/engine_api/json_structures.rs @@ -749,6 +749,33 @@ pub struct JsonExecutionPayloadBodyV1 { 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::>(), + ) + }), + 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::>(), + ) + }), + } + } +} + impl From> for ExecutionPayloadBodyV1 { fn from(value: JsonExecutionPayloadBodyV1) -> Self { Self { diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index eaa739d7a5d..5fcecde5781 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -1807,7 +1807,6 @@ impl ExecutionLayer { header: &ExecutionPayloadHeader, fork: ForkName, ) -> Result>, Error> { - let hash = header.block_hash(); let block_number = header.block_number(); // Handle default payload body. @@ -1841,8 +1840,8 @@ impl ExecutionLayer { }) .transpose() } else { - // Fall back to eth_blockByHash. - self.get_payload_by_hash_legacy(hash, fork).await + // FIXME(sproul): return an error here? + Ok(None) } } @@ -1857,186 +1856,6 @@ impl ExecutionLayer { .map_err(Error::EngineError) } - pub async fn get_payload_by_hash_legacy( - &self, - hash: ExecutionBlockHash, - fork: ForkName, - ) -> Result>, Error> { - self.engine() - .request(|engine| async move { - self.get_payload_by_hash_from_engine(engine, hash, fork) - .await - }) - .await - .map_err(Box::new) - .map_err(Error::EngineError) - } - - async fn get_payload_by_hash_from_engine( - &self, - engine: &Engine, - hash: ExecutionBlockHash, - fork: ForkName, - ) -> Result>, ApiError> { - let _timer = metrics::start_timer(&metrics::EXECUTION_LAYER_GET_PAYLOAD_BY_BLOCK_HASH); - - if hash == ExecutionBlockHash::zero() { - return match fork { - ForkName::Bellatrix => Ok(Some(ExecutionPayloadBellatrix::default().into())), - ForkName::Capella => Ok(Some(ExecutionPayloadCapella::default().into())), - ForkName::Deneb => Ok(Some(ExecutionPayloadDeneb::default().into())), - ForkName::Electra => Ok(Some(ExecutionPayloadElectra::default().into())), - ForkName::Base | ForkName::Altair => Err(ApiError::UnsupportedForkVariant( - format!("called get_payload_by_hash_from_engine with {}", fork), - )), - }; - } - - let Some(block) = engine - .api - .get_block_by_hash_with_txns::(hash, fork) - .await? - else { - return Ok(None); - }; - - let convert_transactions = |transactions: Vec| { - VariableList::new( - transactions - .into_iter() - .map(|tx| VariableList::new(tx.rlp().to_vec())) - .collect::, ssz_types::Error>>()?, - ) - .map_err(ApiError::SszError) - }; - - let payload = match block { - ExecutionBlockWithTransactions::Bellatrix(bellatrix_block) => { - ExecutionPayload::Bellatrix(ExecutionPayloadBellatrix { - parent_hash: bellatrix_block.parent_hash, - fee_recipient: bellatrix_block.fee_recipient, - state_root: bellatrix_block.state_root, - receipts_root: bellatrix_block.receipts_root, - logs_bloom: bellatrix_block.logs_bloom, - prev_randao: bellatrix_block.prev_randao, - block_number: bellatrix_block.block_number, - gas_limit: bellatrix_block.gas_limit, - gas_used: bellatrix_block.gas_used, - timestamp: bellatrix_block.timestamp, - extra_data: bellatrix_block.extra_data, - base_fee_per_gas: bellatrix_block.base_fee_per_gas, - block_hash: bellatrix_block.block_hash, - transactions: convert_transactions(bellatrix_block.transactions)?, - }) - } - ExecutionBlockWithTransactions::Capella(capella_block) => { - let withdrawals = VariableList::new( - capella_block - .withdrawals - .into_iter() - .map(Into::into) - .collect(), - ) - .map_err(ApiError::DeserializeWithdrawals)?; - ExecutionPayload::Capella(ExecutionPayloadCapella { - parent_hash: capella_block.parent_hash, - fee_recipient: capella_block.fee_recipient, - state_root: capella_block.state_root, - receipts_root: capella_block.receipts_root, - logs_bloom: capella_block.logs_bloom, - prev_randao: capella_block.prev_randao, - block_number: capella_block.block_number, - gas_limit: capella_block.gas_limit, - gas_used: capella_block.gas_used, - timestamp: capella_block.timestamp, - extra_data: capella_block.extra_data, - base_fee_per_gas: capella_block.base_fee_per_gas, - block_hash: capella_block.block_hash, - transactions: convert_transactions(capella_block.transactions)?, - withdrawals, - }) - } - ExecutionBlockWithTransactions::Deneb(deneb_block) => { - let withdrawals = VariableList::new( - deneb_block - .withdrawals - .into_iter() - .map(Into::into) - .collect(), - ) - .map_err(ApiError::DeserializeWithdrawals)?; - ExecutionPayload::Deneb(ExecutionPayloadDeneb { - parent_hash: deneb_block.parent_hash, - fee_recipient: deneb_block.fee_recipient, - state_root: deneb_block.state_root, - receipts_root: deneb_block.receipts_root, - logs_bloom: deneb_block.logs_bloom, - prev_randao: deneb_block.prev_randao, - block_number: deneb_block.block_number, - gas_limit: deneb_block.gas_limit, - gas_used: deneb_block.gas_used, - timestamp: deneb_block.timestamp, - extra_data: deneb_block.extra_data, - base_fee_per_gas: deneb_block.base_fee_per_gas, - block_hash: deneb_block.block_hash, - transactions: convert_transactions(deneb_block.transactions)?, - withdrawals, - blob_gas_used: deneb_block.blob_gas_used, - excess_blob_gas: deneb_block.excess_blob_gas, - }) - } - ExecutionBlockWithTransactions::Electra(electra_block) => { - let withdrawals = VariableList::new( - electra_block - .withdrawals - .into_iter() - .map(Into::into) - .collect(), - ) - .map_err(ApiError::DeserializeWithdrawals)?; - let deposit_requests = VariableList::new( - electra_block - .deposit_requests - .into_iter() - .map(Into::into) - .collect(), - ) - .map_err(ApiError::DeserializeDepositRequests)?; - let withdrawal_requests = VariableList::new( - electra_block - .withdrawal_requests - .into_iter() - .map(Into::into) - .collect(), - ) - .map_err(ApiError::DeserializeWithdrawalRequests)?; - ExecutionPayload::Electra(ExecutionPayloadElectra { - parent_hash: electra_block.parent_hash, - fee_recipient: electra_block.fee_recipient, - state_root: electra_block.state_root, - receipts_root: electra_block.receipts_root, - logs_bloom: electra_block.logs_bloom, - prev_randao: electra_block.prev_randao, - block_number: electra_block.block_number, - gas_limit: electra_block.gas_limit, - gas_used: electra_block.gas_used, - timestamp: electra_block.timestamp, - extra_data: electra_block.extra_data, - base_fee_per_gas: electra_block.base_fee_per_gas, - block_hash: electra_block.block_hash, - transactions: convert_transactions(electra_block.transactions)?, - withdrawals, - blob_gas_used: electra_block.blob_gas_used, - excess_blob_gas: electra_block.excess_blob_gas, - deposit_requests, - withdrawal_requests, - }) - } - }; - - Ok(Some(payload)) - } - pub async fn propose_blinded_beacon_block( &self, block_root: Hash256, diff --git a/beacon_node/execution_layer/src/metrics.rs b/beacon_node/execution_layer/src/metrics.rs index c3da449535c..184031af4d0 100644 --- a/beacon_node/execution_layer/src/metrics.rs +++ b/beacon_node/execution_layer/src/metrics.rs @@ -54,13 +54,6 @@ pub static EXECUTION_LAYER_PRE_PREPARED_PAYLOAD_ID: LazyLock> = - LazyLock::new(|| { - try_create_histogram( - "execution_layer_get_payload_by_block_hash_time", - "Time to reconstruct a payload from the EE using eth_getBlockByHash", - ) - }); pub static EXECUTION_LAYER_GET_PAYLOAD_BODIES_BY_RANGE: LazyLock> = LazyLock::new(|| { try_create_histogram( 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 8619e24a238..8a6d5cd85cb 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 @@ -1,14 +1,11 @@ -use crate::engines::ForkchoiceState; -use crate::EthersTransaction; -use crate::{ - engine_api::{ - json_structures::{ - JsonForkchoiceUpdatedV1Response, JsonPayloadStatusV1, JsonPayloadStatusV1Status, - }, - ExecutionBlock, PayloadAttributes, PayloadId, PayloadStatusV1, PayloadStatusV1Status, +use crate::engine_api::{ + json_structures::{ + JsonForkchoiceUpdatedV1Response, JsonPayloadStatusV1, JsonPayloadStatusV1Status, }, - ExecutionBlockWithTransactions, + ExecutionBlock, PayloadAttributes, PayloadId, PayloadStatusV1, PayloadStatusV1Status, }; +use crate::engines::ForkchoiceState; +use crate::EthersTransaction; use eth2::types::BlobsBundle; use kzg::{Kzg, KzgCommitment, KzgProof}; use parking_lot::Mutex; @@ -88,17 +85,13 @@ impl Block { } } - pub fn as_execution_block_with_tx(&self) -> Option> { + pub fn as_execution_payload(&self) -> Option> { match self { - Block::PoS(payload) => Some(payload.clone().try_into().unwrap()), - Block::PoW(block) => Some( - ExecutionPayload::Bellatrix(ExecutionPayloadBellatrix { - block_hash: block.block_hash, - ..Default::default() - }) - .try_into() - .unwrap(), - ), + Block::PoS(payload) => Some(payload.clone()), + Block::PoW(block) => Some(ExecutionPayload::Bellatrix(ExecutionPayloadBellatrix { + block_hash: block.block_hash, + ..Default::default() + })), } } } @@ -252,20 +245,17 @@ impl ExecutionBlockGenerator { .map(|block| block.as_execution_block(self.terminal_total_difficulty)) } - pub fn execution_block_with_txs_by_hash( + pub fn execution_payload_by_hash( &self, hash: ExecutionBlockHash, - ) -> Option> { + ) -> Option> { self.block_by_hash(hash) - .and_then(|block| block.as_execution_block_with_tx()) + .and_then(|block| block.as_execution_payload()) } - pub fn execution_block_with_txs_by_number( - &self, - number: u64, - ) -> Option> { + pub fn execution_payload_by_number(&self, number: u64) -> Option> { self.block_by_number(number) - .and_then(|block| block.as_execution_block_with_tx()) + .and_then(|block| block.as_execution_payload()) } pub fn move_to_block_prior_to_terminal_block(&mut self) -> Result<(), String> { 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 0dc7a7759c5..71587b97c55 100644 --- a/beacon_node/execution_layer/src/test_utils/handle_rpc.rs +++ b/beacon_node/execution_layer/src/test_utils/handle_rpc.rs @@ -83,12 +83,10 @@ pub async fn handle_rpc( .ok_or_else(|| "missing/invalid params[1] value".to_string()) .map_err(|s| (s, BAD_PARAMS_ERROR_CODE))?; if full_tx { - Ok(serde_json::to_value( - ctx.execution_block_generator - .read() - .execution_block_with_txs_by_hash(hash), - ) - .unwrap()) + Err(( + "full_tx support has been removed".to_string(), + BAD_PARAMS_ERROR_CODE, + )) } else { Ok(serde_json::to_value( ctx.execution_block_generator @@ -556,48 +554,20 @@ pub async fn handle_rpc( let mut response = vec![]; for block_num in start..(start + count) { - let maybe_block = ctx + let maybe_payload = ctx .execution_block_generator .read() - .execution_block_with_txs_by_number(block_num); - - match maybe_block { - Some(block) => { - let transactions = Transactions::::new( - block - .transactions() - .iter() - .map(|transaction| VariableList::new(transaction.rlp().to_vec())) - .collect::>() - .map_err(|e| { - ( - format!("failed to deserialize transaction: {:?}", e), - GENERIC_ERROR_CODE, - ) - })?, - ) - .map_err(|e| { - ( - format!("failed to deserialize transactions: {:?}", e), - GENERIC_ERROR_CODE, - ) - })?; - - response.push(Some(JsonExecutionPayloadBodyV1:: { - transactions, - withdrawals: block - .withdrawals() - .ok() - .map(|withdrawals| VariableList::from(withdrawals.clone())), - deposit_requests: block.deposit_requests().ok().map( - |deposit_requests| VariableList::from(deposit_requests.clone()), - ), - withdrawal_requests: block.withdrawal_requests().ok().map( - |withdrawal_requests| { - VariableList::from(withdrawal_requests.clone()) - }, - ), - })); + .execution_payload_by_number(block_num); + + match maybe_payload { + Some(payload) => { + let payload_body = ExecutionPayloadBodyV1 { + transactions: payload.transactions().clone(), + withdrawals: payload.withdrawals().ok().cloned(), + deposit_requests: payload.deposit_requests().ok().cloned(), + withdrawal_requests: payload.withdrawal_requests().ok().cloned(), + }; + response.push(Some(JsonExecutionPayloadBodyV1::::from(payload_body))); } None => response.push(None), } diff --git a/testing/execution_engine_integration/src/test_rig.rs b/testing/execution_engine_integration/src/test_rig.rs index c7d5e704524..57723bbdd0f 100644 --- a/testing/execution_engine_integration/src/test_rig.rs +++ b/testing/execution_engine_integration/src/test_rig.rs @@ -649,15 +649,7 @@ async fn check_payload_reconstruction( ee: &ExecutionPair, payload: &ExecutionPayload, ) { - // check via legacy eth_getBlockByHash - let reconstructed = ee - .execution_layer - .get_payload_by_hash_legacy(payload.block_hash(), payload.fork_name()) - .await - .unwrap() - .unwrap(); - assert_eq!(reconstructed, *payload); - // also check via payload bodies method + // check via payload bodies method let capabilities = ee .execution_layer .get_engine_capabilities(None) From fff92d65fc3ddb8c3437a4a4d86b34cf108f1580 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 2 Aug 2024 20:12:17 +1000 Subject: [PATCH 2/3] Delete unneeded failing test --- .../beacon_chain/src/beacon_block_streamer.rs | 143 ------------------ 1 file changed, 143 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_block_streamer.rs b/beacon_node/beacon_chain/src/beacon_block_streamer.rs index d63a3ee7ead..bef794b5d56 100644 --- a/beacon_node/beacon_chain/src/beacon_block_streamer.rs +++ b/beacon_node/beacon_chain/src/beacon_block_streamer.rs @@ -862,147 +862,4 @@ mod tests { } } } - - #[tokio::test] - async fn check_fallback_altair_to_electra() { - let slots_per_epoch = MinimalEthSpec::slots_per_epoch() as usize; - let num_epochs = 10; - let bellatrix_fork_epoch = 2usize; - let capella_fork_epoch = 4usize; - let deneb_fork_epoch = 6usize; - let electra_fork_epoch = 8usize; - let num_blocks_produced = num_epochs * slots_per_epoch; - - let mut spec = test_spec::(); - spec.altair_fork_epoch = Some(Epoch::new(0)); - spec.bellatrix_fork_epoch = Some(Epoch::new(bellatrix_fork_epoch as u64)); - spec.capella_fork_epoch = Some(Epoch::new(capella_fork_epoch as u64)); - spec.deneb_fork_epoch = Some(Epoch::new(deneb_fork_epoch as u64)); - spec.electra_fork_epoch = Some(Epoch::new(electra_fork_epoch as u64)); - - let harness = get_harness(VALIDATOR_COUNT, spec); - - // modify execution engine so it doesn't support engine_payloadBodiesBy* methods - let mock_execution_layer = harness.mock_execution_layer.as_ref().unwrap(); - mock_execution_layer - .server - .set_engine_capabilities(EngineCapabilities { - get_payload_bodies_by_hash_v1: false, - get_payload_bodies_by_range_v1: false, - ..DEFAULT_ENGINE_CAPABILITIES - }); - // refresh capabilities cache - harness - .chain - .execution_layer - .as_ref() - .unwrap() - .get_engine_capabilities(Some(Duration::ZERO)) - .await - .unwrap(); - - // go to bellatrix fork - harness - .extend_slots(bellatrix_fork_epoch * slots_per_epoch) - .await; - // extend half an epoch - harness.extend_slots(slots_per_epoch / 2).await; - // trigger merge - harness - .execution_block_generator() - .move_to_terminal_block() - .expect("should move to terminal block"); - let timestamp = harness.get_timestamp_at_slot() + harness.spec.seconds_per_slot; - harness - .execution_block_generator() - .modify_last_block(|block| { - if let Block::PoW(terminal_block) = block { - terminal_block.timestamp = timestamp; - } - }); - // finish out merge epoch - harness.extend_slots(slots_per_epoch / 2).await; - // finish rest of epochs - harness - .extend_slots((num_epochs - 1 - bellatrix_fork_epoch) * slots_per_epoch) - .await; - - let head = harness.chain.head_snapshot(); - let state = &head.beacon_state; - - assert_eq!( - state.slot(), - Slot::new(num_blocks_produced as u64), - "head should be at the current slot" - ); - assert_eq!( - state.current_epoch(), - num_blocks_produced as u64 / MinimalEthSpec::slots_per_epoch(), - "head should be at the expected epoch" - ); - assert_eq!( - state.current_justified_checkpoint().epoch, - state.current_epoch() - 1, - "the head should be justified one behind the current epoch" - ); - assert_eq!( - state.finalized_checkpoint().epoch, - state.current_epoch() - 2, - "the head should be finalized two behind the current epoch" - ); - - let block_roots: Vec = harness - .chain - .forwards_iter_block_roots(Slot::new(0)) - .expect("should get iter") - .map(Result::unwrap) - .map(|(root, _)| root) - .collect(); - - let mut expected_blocks = vec![]; - // get all blocks the old fashioned way - for root in &block_roots { - let block = harness - .chain - .get_block(root) - .await - .expect("should get block") - .expect("block should exist"); - expected_blocks.push(block); - } - - for epoch in 0..num_epochs { - let start = epoch * slots_per_epoch; - let mut epoch_roots = vec![Hash256::zero(); slots_per_epoch]; - epoch_roots[..].clone_from_slice(&block_roots[start..(start + slots_per_epoch)]); - let streamer = BeaconBlockStreamer::new(&harness.chain, CheckCaches::No) - .expect("should create streamer"); - let (block_tx, mut block_rx) = mpsc::unbounded_channel(); - streamer.stream(epoch_roots.clone(), block_tx).await; - - for (i, expected_root) in epoch_roots.into_iter().enumerate() { - let (found_root, found_block_result) = - block_rx.recv().await.expect("should get block"); - - assert_eq!( - found_root, expected_root, - "expected block root should match" - ); - match found_block_result.as_ref() { - Ok(maybe_block) => { - let found_block = maybe_block.clone().expect("should have a block"); - let expected_block = expected_blocks - .get(start + i) - .expect("should get expected block"); - assert_eq!( - found_block.as_ref(), - expected_block, - "expected block should match found block" - ); - } - Err(e) => panic!("Error retrieving block {}: {:?}", expected_root, e), - } - } - } - } } From 5c49ae4e925fd160b2bdf949fd48c7185a2d76a3 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Thu, 5 Sep 2024 13:55:10 +1000 Subject: [PATCH 3/3] Cleanups --- beacon_node/execution_layer/src/lib.rs | 8 ++- .../src/test_utils/handle_rpc.rs | 64 ++++++------------- 2 files changed, 24 insertions(+), 48 deletions(-) diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index 3235f1016a1..648963a320e 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -144,6 +144,7 @@ pub enum Error { payload: ExecutionBlockHash, transactions_root: Hash256, }, + PayloadBodiesByRangeNotSupported, InvalidJWTSecret(String), InvalidForkForPayload, InvalidPayloadBody(String), @@ -1822,7 +1823,9 @@ 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 { + if capabilities.get_payload_bodies_by_range_v1 + || capabilities.get_payload_bodies_by_range_v2 + { let mut payload_bodies = self.get_payload_bodies_by_range(block_number, 1).await?; if payload_bodies.len() != 1 { @@ -1837,8 +1840,7 @@ impl ExecutionLayer { }) .transpose() } else { - // FIXME(sproul): return an error here? - Ok(None) + Err(Error::PayloadBodiesByRangeNotSupported) } } 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 6dc82dd08b9..f36cb9797d3 100644 --- a/beacon_node/execution_layer/src/test_utils/handle_rpc.rs +++ b/beacon_node/execution_layer/src/test_utils/handle_rpc.rs @@ -561,30 +561,17 @@ pub async fn handle_rpc( match maybe_payload { Some(payload) => { - // FIXME(sproul): remove electra support here? - let json_payload_body = if payload.fork_name().electra_enabled() { - 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(), - }; - JsonExecutionPayloadBody::V2(JsonExecutionPayloadBodyV2::::from( - payload_body, - )) - } else { - let payload_body = ExecutionPayloadBodyV1 { - transactions: payload.transactions().clone(), - withdrawals: payload.withdrawals().ok().cloned(), - }; - JsonExecutionPayloadBody::V1(JsonExecutionPayloadBodyV1::::from( - payload_body, - )) + assert!( + !payload.fork_name().electra_enabled(), + "payload bodies V1 is not supported for Electra blocks" + ); + let payload_body = 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), @@ -618,29 +605,16 @@ pub async fn handle_rpc( // deposit_requests // withdrawal_requests // consolidation_requests - let json_payload_body = if payload.fork_name().electra_enabled() { - 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(), - }; - JsonExecutionPayloadBody::V2(JsonExecutionPayloadBodyV2::::from( - payload_body, - )) - } else { - let payload_body = ExecutionPayloadBodyV1 { - transactions: payload.transactions().clone(), - withdrawals: payload.withdrawals().ok().cloned(), - }; - JsonExecutionPayloadBody::V1(JsonExecutionPayloadBodyV1::::from( - payload_body, - )) + 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), + ); response.push(Some(json_payload_body)); } None => response.push(None),