From 1cde1ca69617cf51b8fa377ea0c07732e388aafb Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Mon, 25 Sep 2023 14:58:54 -0400 Subject: [PATCH 1/6] split try_into_sealed_block into multiple methods --- .../rpc-types-compat/src/engine/payload.rs | 54 ++++++++++++++----- 1 file changed, 41 insertions(+), 13 deletions(-) diff --git a/crates/rpc/rpc-types-compat/src/engine/payload.rs b/crates/rpc/rpc-types-compat/src/engine/payload.rs index 4158e9990188..0e55b6833f14 100644 --- a/crates/rpc/rpc-types-compat/src/engine/payload.rs +++ b/crates/rpc/rpc-types-compat/src/engine/payload.rs @@ -257,19 +257,14 @@ pub fn convert_block_to_payload_input_v2(value: SealedBlock) -> ExecutionPayload } } -/// Tries to create a new block from the given payload and optional parent beacon block root. -/// Perform additional validation of `extra_data` and `base_fee_per_gas` fields. +/// Tries to create a new block (without a block hash) from the given payload and optional parent +/// beacon block root. +/// Performs additional validation of `extra_data` and `base_fee_per_gas` fields. /// /// NOTE: The log bloom is assumed to be validated during serialization. -/// NOTE: Empty ommers, nonce and difficulty values are validated upon computing block hash and -/// comparing the value with `payload.block_hash`. /// /// See -pub fn try_into_sealed_block( - value: ExecutionPayload, - parent_beacon_block_root: Option, -) -> Result { - let block_hash = value.block_hash(); +pub fn try_into_block(value: ExecutionPaylod, parent_beacon_block_root: Option) -> Result { let mut base_payload = match value { ExecutionPayload::V1(payload) => try_payload_v1_to_block(payload)?, ExecutionPayload::V2(payload) => try_payload_v2_to_block(payload)?, @@ -278,12 +273,45 @@ pub fn try_into_sealed_block( base_payload.header.parent_beacon_block_root = parent_beacon_block_root; - let payload = base_payload.seal_slow(); + Ok(base_payload) +} + +/// Tries to create a new block from the given payload and optional parent beacon block root. +/// +/// NOTE: Empty ommers, nonce and difficulty values are validated upon computing block hash and +/// comparing the value with `payload.block_hash`. +/// +/// Uses [try_into_block] to convert from the [ExecutionPayload] to [Block] and seals the block +/// with its hash. +/// +/// Uses [validate_block_hash] to validate the payload block hash and ultimately return the +/// [SealedBlock]. +pub fn try_into_sealed_block( + payload: ExecutionPayload, + parent_beacon_block_root: Option, +) -> Result { + let block_hash = payload.block_hash(); + let base_payload = try_into_block(payload, parent_beacon_block_root)?; + + // validate block hash and return + validate_block_hash(block_hash, base_payload) +} - if block_hash != payload.hash() { - return Err(PayloadError::BlockHash { execution: payload.hash(), consensus: block_hash }) +/// Takes the expected block hash and [Block], validating the block and converting it into a +/// [SealedBlock]. +/// +/// If the provided block hash does not match the block hash computed from the provided block, this +/// returns [PayloadError::BlockHash]. +pub fn validate_block_hash( + expected_block_hash: H256, + block: Block, +) -> Result { + let sealed_block = block.seal_slow(); + if expected_block_hash != sealed_block.hash() { + return Err(PayloadError::BlockHash { execution: sealed_block.hash(), consensus: expected_block_hash }) } - Ok(payload) + + Ok(sealed_block) } /// Converts [Withdrawal] to [reth_rpc_types::engine::payload::Withdrawal] From 28f3409f3ff3f06988e87ab5cdc8fa489b1d39ae Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Tue, 26 Sep 2023 00:10:31 -0400 Subject: [PATCH 2/6] fix: ensure no blob transactions in payloads pre-cancun --- crates/consensus/beacon/src/engine/error.rs | 3 ++ crates/consensus/beacon/src/engine/mod.rs | 40 ++++++++++++++----- crates/rpc/rpc-engine-api/src/error.rs | 1 + .../rpc-types-compat/src/engine/payload.rs | 10 ++++- 4 files changed, 43 insertions(+), 11 deletions(-) diff --git a/crates/consensus/beacon/src/engine/error.rs b/crates/consensus/beacon/src/engine/error.rs index 29db2c143d7d..d5f19b536343 100644 --- a/crates/consensus/beacon/src/engine/error.rs +++ b/crates/consensus/beacon/src/engine/error.rs @@ -80,6 +80,9 @@ pub enum BeaconOnNewPayloadError { /// Thrown when the engine task is unavailable/stopped. #[error("beacon consensus engine task stopped")] EngineUnavailable, + /// Thrown when a block has blob transactions, but is not after the Cancun fork. + #[error("block has blob transactions, but is not after the Cancun fork")] + PreCancunBlockWithBlobTransactions, /// An internal error occurred, not necessarily related to the payload. #[error(transparent)] Internal(Box), diff --git a/crates/consensus/beacon/src/engine/mod.rs b/crates/consensus/beacon/src/engine/mod.rs index b2df319b309c..32550b4256be 100644 --- a/crates/consensus/beacon/src/engine/mod.rs +++ b/crates/consensus/beacon/src/engine/mod.rs @@ -23,7 +23,7 @@ use reth_interfaces::{ use reth_payload_builder::{PayloadBuilderAttributes, PayloadBuilderHandle}; use reth_primitives::{ constants::EPOCH_SLOTS, listener::EventListeners, stage::StageId, BlockNumHash, BlockNumber, - Head, Header, SealedBlock, SealedHeader, H256, U256, + ChainSpec, Head, Header, SealedBlock, SealedHeader, H256, U256, }; use reth_provider::{ BlockIdReader, BlockReader, BlockSource, CanonChainTracker, ChainSpecProvider, ProviderError, @@ -33,7 +33,7 @@ use reth_rpc_types::engine::{ CancunPayloadFields, ExecutionPayload, PayloadAttributes, PayloadError, PayloadStatus, PayloadStatusEnum, PayloadValidationError, }; -use reth_rpc_types_compat::engine::payload::try_into_sealed_block; +use reth_rpc_types_compat::payload::{try_into_block, validate_block_hash}; use reth_stages::{ControlFlow, Pipeline, PipelineError}; use reth_tasks::TaskSpawner; use std::{ @@ -1079,7 +1079,7 @@ where payload: ExecutionPayload, cancun_fields: Option, ) -> Result { - let block = match self.ensure_well_formed_payload(payload, cancun_fields) { + let block = match self.ensure_well_formed_payload(payload, cancun_fields)? { Ok(block) => block, Err(status) => return Ok(status), }; @@ -1143,18 +1143,24 @@ where /// - incorrect hash /// - the versioned hashes passed with the payload do not exactly match transaction /// versioned hashes + /// - the block does not contain blob transactions if it is pre-cancun fn ensure_well_formed_payload( &self, payload: ExecutionPayload, cancun_fields: Option, - ) -> Result { + ) -> Result, BeaconOnNewPayloadError> { let parent_hash = payload.parent_hash(); - let block = match try_into_sealed_block( + + let block_hash = payload.block_hash(); + let block = match try_into_block( payload, cancun_fields.as_ref().map(|fields| fields.parent_beacon_block_root), - ) { + ) + .and_then(|block| validate_block_hash(block_hash, block)) + { Ok(block) => block, Err(error) => { + // TODO: need to fix this to use the correct type error!(target: "consensus::engine", ?error, "Invalid payload"); let mut latest_valid_hash = None; @@ -1166,10 +1172,17 @@ where } let status = PayloadStatusEnum::from(error); - return Err(PayloadStatus::new(status, latest_valid_hash)) + return Ok(Err(PayloadStatus::new(status, latest_valid_hash))) } }; + // make sure there are no blob transactions in the payload if it is pre-cancun + if !self.chain_spec().is_cancun_activated_at_timestamp(block.timestamp) { + if block.blob_transactions().len() > 0 { + return Err(BeaconOnNewPayloadError::PreCancunBlockWithBlobTransactions) + } + } + let block_versioned_hashes = block .blob_transactions() .iter() @@ -1177,9 +1190,18 @@ where .flatten() .collect::>(); - self.validate_versioned_hashes(parent_hash, block_versioned_hashes, cancun_fields)?; + if let Err(status) = + self.validate_versioned_hashes(parent_hash, block_versioned_hashes, cancun_fields) + { + return Ok(Err(status)) + } + + Ok(Ok(block)) + } - Ok(block) + /// Returns the currently configured [ChainSpec]. + fn chain_spec(&self) -> Arc { + self.blockchain.chain_spec() } /// Validates that the versioned hashes in the block match the versioned hashes passed in the diff --git a/crates/rpc/rpc-engine-api/src/error.rs b/crates/rpc/rpc-engine-api/src/error.rs index a5bf6b111d3b..d42add66d253 100644 --- a/crates/rpc/rpc-engine-api/src/error.rs +++ b/crates/rpc/rpc-engine-api/src/error.rs @@ -112,6 +112,7 @@ impl From for jsonrpsee_types::error::ErrorObject<'static> { }, EngineApiError::NewPayload(ref err) => match err { BeaconOnNewPayloadError::Internal(_) | + BeaconOnNewPayloadError::PreCancunBlockWithBlobTransactions => INVALID_PARAMS_CODE, BeaconOnNewPayloadError::EngineUnavailable => INTERNAL_ERROR_CODE, }, // Any other server error diff --git a/crates/rpc/rpc-types-compat/src/engine/payload.rs b/crates/rpc/rpc-types-compat/src/engine/payload.rs index 0e55b6833f14..e47f6ea2908c 100644 --- a/crates/rpc/rpc-types-compat/src/engine/payload.rs +++ b/crates/rpc/rpc-types-compat/src/engine/payload.rs @@ -264,7 +264,10 @@ pub fn convert_block_to_payload_input_v2(value: SealedBlock) -> ExecutionPayload /// NOTE: The log bloom is assumed to be validated during serialization. /// /// See -pub fn try_into_block(value: ExecutionPaylod, parent_beacon_block_root: Option) -> Result { +pub fn try_into_block( + value: ExecutionPayload, + parent_beacon_block_root: Option, +) -> Result { let mut base_payload = match value { ExecutionPayload::V1(payload) => try_payload_v1_to_block(payload)?, ExecutionPayload::V2(payload) => try_payload_v2_to_block(payload)?, @@ -308,7 +311,10 @@ pub fn validate_block_hash( ) -> Result { let sealed_block = block.seal_slow(); if expected_block_hash != sealed_block.hash() { - return Err(PayloadError::BlockHash { execution: sealed_block.hash(), consensus: expected_block_hash }) + return Err(PayloadError::BlockHash { + execution: sealed_block.hash(), + consensus: expected_block_hash, + }) } Ok(sealed_block) From 1aaf782733174ce5550bdd68357fc372eb5e8feb Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Tue, 26 Sep 2023 00:22:46 -0400 Subject: [PATCH 3/6] fix return order --- crates/consensus/beacon/src/engine/mod.rs | 29 ++++++++++++++--------- crates/primitives/src/block.rs | 5 ++++ 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/crates/consensus/beacon/src/engine/mod.rs b/crates/consensus/beacon/src/engine/mod.rs index 32550b4256be..65eaf54ace87 100644 --- a/crates/consensus/beacon/src/engine/mod.rs +++ b/crates/consensus/beacon/src/engine/mod.rs @@ -1152,12 +1152,26 @@ where let parent_hash = payload.parent_hash(); let block_hash = payload.block_hash(); - let block = match try_into_block( + let block_res = match try_into_block( payload, cancun_fields.as_ref().map(|fields| fields.parent_beacon_block_root), - ) - .and_then(|block| validate_block_hash(block_hash, block)) - { + ) { + Ok(block) => { + // make sure there are no blob transactions in the payload if it is pre-cancun + // we perform this check before validating the block hash because INVALID_PARAMS + // must be returned over an INVALID response. + if !self.chain_spec().is_cancun_activated_at_timestamp(block.timestamp) { + if block.blob_transactions().len() > 0 { + return Err(BeaconOnNewPayloadError::PreCancunBlockWithBlobTransactions) + } + } + + validate_block_hash(block_hash, block) + } + Err(error) => Err(error), + }; + + let block = match block_res { Ok(block) => block, Err(error) => { // TODO: need to fix this to use the correct type @@ -1176,13 +1190,6 @@ where } }; - // make sure there are no blob transactions in the payload if it is pre-cancun - if !self.chain_spec().is_cancun_activated_at_timestamp(block.timestamp) { - if block.blob_transactions().len() > 0 { - return Err(BeaconOnNewPayloadError::PreCancunBlockWithBlobTransactions) - } - } - let block_versioned_hashes = block .blob_transactions() .iter() diff --git a/crates/primitives/src/block.rs b/crates/primitives/src/block.rs index 199b088d06a1..172d5f64bc8e 100644 --- a/crates/primitives/src/block.rs +++ b/crates/primitives/src/block.rs @@ -60,6 +60,11 @@ impl Block { BlockWithSenders { block: self, senders } } + /// Returns only the blob transactions, if any, from the block body. + pub fn blob_transactions(&self) -> Vec<&TransactionSigned> { + self.body.iter().filter(|tx| tx.is_eip4844()).collect() + } + /// Calculates a heuristic for the in-memory size of the [Block]. #[inline] pub fn size(&self) -> usize { From c34b270f4d750c43dc8b03afd2fd9549e98315ba Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Tue, 26 Sep 2023 01:59:54 -0400 Subject: [PATCH 4/6] improve blob transaction check --- crates/consensus/beacon/src/engine/mod.rs | 8 ++++---- crates/primitives/src/block.rs | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/crates/consensus/beacon/src/engine/mod.rs b/crates/consensus/beacon/src/engine/mod.rs index 65eaf54ace87..d126f3e7cc6a 100644 --- a/crates/consensus/beacon/src/engine/mod.rs +++ b/crates/consensus/beacon/src/engine/mod.rs @@ -1160,10 +1160,10 @@ where // make sure there are no blob transactions in the payload if it is pre-cancun // we perform this check before validating the block hash because INVALID_PARAMS // must be returned over an INVALID response. - if !self.chain_spec().is_cancun_activated_at_timestamp(block.timestamp) { - if block.blob_transactions().len() > 0 { - return Err(BeaconOnNewPayloadError::PreCancunBlockWithBlobTransactions) - } + if !self.chain_spec().is_cancun_activated_at_timestamp(block.timestamp) && + block.has_blob_transactions() + { + return Err(BeaconOnNewPayloadError::PreCancunBlockWithBlobTransactions) } validate_block_hash(block_hash, block) diff --git a/crates/primitives/src/block.rs b/crates/primitives/src/block.rs index 172d5f64bc8e..c9be7194adea 100644 --- a/crates/primitives/src/block.rs +++ b/crates/primitives/src/block.rs @@ -60,9 +60,9 @@ impl Block { BlockWithSenders { block: self, senders } } - /// Returns only the blob transactions, if any, from the block body. - pub fn blob_transactions(&self) -> Vec<&TransactionSigned> { - self.body.iter().filter(|tx| tx.is_eip4844()).collect() + /// Returns whether or not the block contains any blob transactions. + pub fn has_blob_transactions(&self) -> bool { + self.body.iter().any(|tx| tx.is_eip4844()) } /// Calculates a heuristic for the in-memory size of the [Block]. From 0e48f72d2cbd0d4d4be369379c09822e36dbdee4 Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Tue, 26 Sep 2023 10:49:52 -0400 Subject: [PATCH 5/6] remove outdated comment --- crates/consensus/beacon/src/engine/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/consensus/beacon/src/engine/mod.rs b/crates/consensus/beacon/src/engine/mod.rs index d126f3e7cc6a..3501042a2e80 100644 --- a/crates/consensus/beacon/src/engine/mod.rs +++ b/crates/consensus/beacon/src/engine/mod.rs @@ -1174,7 +1174,6 @@ where let block = match block_res { Ok(block) => block, Err(error) => { - // TODO: need to fix this to use the correct type error!(target: "consensus::engine", ?error, "Invalid payload"); let mut latest_valid_hash = None; From ad8e4cc48c524194e5c0bb6bd0b774305e94e00e Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Tue, 26 Sep 2023 14:13:36 -0400 Subject: [PATCH 6/6] fix active_at_timestamp --- crates/consensus/beacon/src/engine/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/consensus/beacon/src/engine/mod.rs b/crates/consensus/beacon/src/engine/mod.rs index 3501042a2e80..013fe3aac4e5 100644 --- a/crates/consensus/beacon/src/engine/mod.rs +++ b/crates/consensus/beacon/src/engine/mod.rs @@ -1160,7 +1160,7 @@ where // make sure there are no blob transactions in the payload if it is pre-cancun // we perform this check before validating the block hash because INVALID_PARAMS // must be returned over an INVALID response. - if !self.chain_spec().is_cancun_activated_at_timestamp(block.timestamp) && + if !self.chain_spec().is_cancun_active_at_timestamp(block.timestamp) && block.has_blob_transactions() { return Err(BeaconOnNewPayloadError::PreCancunBlockWithBlobTransactions)