From 9d46ab48637b96ef5b32fb013a5d920e793659cf Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Fri, 18 Aug 2023 14:29:53 -0400 Subject: [PATCH] feat: validate engine cancun fields based on method version (#4256) --- crates/rpc/rpc-engine-api/src/engine_api.rs | 112 ++++++++++++++++---- crates/rpc/rpc-engine-api/src/error.rs | 17 +++ crates/rpc/rpc-engine-api/src/lib.rs | 3 + crates/rpc/rpc-engine-api/src/payload.rs | 56 ++++++++++ 4 files changed, 166 insertions(+), 22 deletions(-) create mode 100644 crates/rpc/rpc-engine-api/src/payload.rs diff --git a/crates/rpc/rpc-engine-api/src/engine_api.rs b/crates/rpc/rpc-engine-api/src/engine_api.rs index 56ddc71cdf..057b3954ee 100644 --- a/crates/rpc/rpc-engine-api/src/engine_api.rs +++ b/crates/rpc/rpc-engine-api/src/engine_api.rs @@ -1,4 +1,6 @@ -use crate::{EngineApiError, EngineApiMessageVersion, EngineApiResult}; +use crate::{ + payload::PayloadOrAttributes, EngineApiError, EngineApiMessageVersion, EngineApiResult, +}; use async_trait::async_trait; use jsonrpsee_core::RpcResult; use reth_beacon_consensus::BeaconConsensusEngineHandle; @@ -69,10 +71,9 @@ where &self, payload: ExecutionPayload, ) -> EngineApiResult { - self.validate_withdrawals_presence( + self.validate_version_specific_fields( EngineApiMessageVersion::V1, - payload.timestamp.as_u64(), - payload.withdrawals.is_some(), + PayloadOrAttributes::from_execution_payload(&payload, None), )?; Ok(self.inner.beacon_consensus.new_payload(payload).await?) } @@ -82,14 +83,29 @@ where &self, payload: ExecutionPayload, ) -> EngineApiResult { - self.validate_withdrawals_presence( + self.validate_version_specific_fields( EngineApiMessageVersion::V2, - payload.timestamp.as_u64(), - payload.withdrawals.is_some(), + PayloadOrAttributes::from_execution_payload(&payload, None), )?; Ok(self.inner.beacon_consensus.new_payload(payload).await?) } + /// See also + pub async fn new_payload_v3( + &self, + payload: ExecutionPayload, + _versioned_hashes: Vec, + parent_beacon_block_root: H256, + ) -> EngineApiResult { + self.validate_version_specific_fields( + EngineApiMessageVersion::V3, + PayloadOrAttributes::from_execution_payload(&payload, Some(parent_beacon_block_root)), + )?; + + // TODO: validate versioned hashes and figure out what to do with parent_beacon_block_root + Ok(self.inner.beacon_consensus.new_payload(payload).await?) + } + /// Sends a message to the beacon consensus engine to update the fork choice _without_ /// withdrawals. /// @@ -102,11 +118,7 @@ where payload_attrs: Option, ) -> EngineApiResult { if let Some(ref attrs) = payload_attrs { - self.validate_withdrawals_presence( - EngineApiMessageVersion::V1, - attrs.timestamp.as_u64(), - attrs.withdrawals.is_some(), - )?; + self.validate_version_specific_fields(EngineApiMessageVersion::V1, attrs.into())?; } Ok(self.inner.beacon_consensus.fork_choice_updated(state, payload_attrs).await?) } @@ -121,11 +133,7 @@ where payload_attrs: Option, ) -> EngineApiResult { if let Some(ref attrs) = payload_attrs { - self.validate_withdrawals_presence( - EngineApiMessageVersion::V2, - attrs.timestamp.as_u64(), - attrs.withdrawals.is_some(), - )?; + self.validate_version_specific_fields(EngineApiMessageVersion::V2, attrs.into())?; } Ok(self.inner.beacon_consensus.fork_choice_updated(state, payload_attrs).await?) } @@ -140,11 +148,7 @@ where payload_attrs: Option, ) -> EngineApiResult { if let Some(ref attrs) = payload_attrs { - self.validate_withdrawals_presence( - EngineApiMessageVersion::V3, - attrs.timestamp.as_u64(), - attrs.withdrawals.is_some(), - )?; + self.validate_version_specific_fields(EngineApiMessageVersion::V3, attrs.into())?; } Ok(self.inner.beacon_consensus.fork_choice_updated(state, payload_attrs).await?) @@ -353,6 +357,70 @@ where Ok(()) } + + /// Validate the presence of the `parentBeaconBlockRoot` field according to the payload + /// timestamp. + /// + /// After Cancun, `parentBeaconBlockRoot` field must be [Some]. + /// Before Cancun, `parentBeaconBlockRoot` field must be [None]. + /// + /// If the payload attribute's timestamp is before the Cancun fork and the engine API message + /// version is V3, then this will return [EngineApiError::UnsupportedFork]. + /// + /// If the engine API message version is V1 or V2, and the payload attribute's timestamp is + /// post-Cancun, then this will return [EngineApiError::NoParentBeaconBlockRootPostCancun]. + /// + /// Implements the following Engine API spec rule: + /// + /// * Client software MUST return `-38005: Unsupported fork` error if the timestamp of the + /// payload does not fall within the time frame of the Cancun fork. + fn validate_parent_beacon_block_root_presence( + &self, + version: EngineApiMessageVersion, + timestamp: u64, + has_parent_beacon_block_root: bool, + ) -> EngineApiResult<()> { + let is_cancun = self.inner.chain_spec.fork(Hardfork::Cancun).active_at_timestamp(timestamp); + + match version { + EngineApiMessageVersion::V1 | EngineApiMessageVersion::V2 => { + if has_parent_beacon_block_root { + return Err(EngineApiError::ParentBeaconBlockRootNotSupportedBeforeV3) + } + if is_cancun { + return Err(EngineApiError::NoParentBeaconBlockRootPostCancun) + } + } + EngineApiMessageVersion::V3 => { + if !is_cancun { + return Err(EngineApiError::UnsupportedFork) + } else if !has_parent_beacon_block_root { + return Err(EngineApiError::NoParentBeaconBlockRootPostCancun) + } + } + }; + + Ok(()) + } + + /// Validates the presence or exclusion of fork-specific fields based on the payload attributes + /// and the message version. + fn validate_version_specific_fields( + &self, + version: EngineApiMessageVersion, + payload_or_attrs: PayloadOrAttributes<'_>, + ) -> EngineApiResult<()> { + self.validate_withdrawals_presence( + version, + payload_or_attrs.timestamp(), + payload_or_attrs.withdrawals().is_some(), + )?; + self.validate_parent_beacon_block_root_presence( + version, + payload_or_attrs.timestamp(), + payload_or_attrs.parent_beacon_block_root().is_some(), + ) + } } #[async_trait] diff --git a/crates/rpc/rpc-engine-api/src/error.rs b/crates/rpc/rpc-engine-api/src/error.rs index 26c29916c3..a5bf6b111d 100644 --- a/crates/rpc/rpc-engine-api/src/error.rs +++ b/crates/rpc/rpc-engine-api/src/error.rs @@ -7,6 +7,8 @@ use thiserror::Error; /// The Engine API result type pub type EngineApiResult = Result; +/// Payload unsupported fork code. +pub const UNSUPPORTED_FORK_CODE: i32 = -38005; /// Payload unknown error code. pub const UNKNOWN_PAYLOAD_CODE: i32 = -38001; /// Request too large error code. @@ -34,6 +36,10 @@ pub enum EngineApiError { /// requested number of items count: u64, }, + /// Thrown if `PayloadAttributes` provided in engine_forkchoiceUpdated before V3 contains a + /// parent beacon block root + #[error("parent beacon block root not supported before V3")] + ParentBeaconBlockRootNotSupportedBeforeV3, /// Thrown if engine_forkchoiceUpdatedV1 contains withdrawals #[error("withdrawals not supported in V1")] WithdrawalsNotSupportedInV1, @@ -43,6 +49,14 @@ pub enum EngineApiError { /// Thrown if engine_forkchoiceUpdated contains withdrawals before Shanghai #[error("withdrawals pre-shanghai")] HasWithdrawalsPreShanghai, + /// Thrown if the `PayloadAttributes` provided in engine_forkchoiceUpdated contains no parent + /// beacon block root after Cancun + #[error("no parent beacon block root post-cancun")] + NoParentBeaconBlockRootPostCancun, + /// Thrown if `PayloadAttributes` were provided with a timestamp, but the version of the engine + /// method called is meant for a fork that occurs after the provided timestamp. + #[error("unsupported fork")] + UnsupportedFork, /// Terminal total difficulty mismatch during transition configuration exchange. #[error( "Invalid transition terminal total difficulty. Execution: {execution}. Consensus: {consensus}" @@ -82,10 +96,13 @@ impl From for jsonrpsee_types::error::ErrorObject<'static> { let code = match error { EngineApiError::InvalidBodiesRange { .. } | EngineApiError::WithdrawalsNotSupportedInV1 | + EngineApiError::ParentBeaconBlockRootNotSupportedBeforeV3 | + EngineApiError::NoParentBeaconBlockRootPostCancun | EngineApiError::NoWithdrawalsPostShanghai | EngineApiError::HasWithdrawalsPreShanghai => INVALID_PARAMS_CODE, EngineApiError::UnknownPayload => UNKNOWN_PAYLOAD_CODE, EngineApiError::PayloadRequestTooLarge { .. } => REQUEST_TOO_LARGE_CODE, + EngineApiError::UnsupportedFork => UNSUPPORTED_FORK_CODE, // Error responses from the consensus engine EngineApiError::ForkChoiceUpdate(ref err) => match err { diff --git a/crates/rpc/rpc-engine-api/src/lib.rs b/crates/rpc/rpc-engine-api/src/lib.rs index ba36670c5a..92631faa10 100644 --- a/crates/rpc/rpc-engine-api/src/lib.rs +++ b/crates/rpc/rpc-engine-api/src/lib.rs @@ -20,6 +20,9 @@ mod engine_api; /// The Engine API message type. mod message; +/// An type representing either an execution payload or payload attributes. +mod payload; + /// Engine API error. mod error; diff --git a/crates/rpc/rpc-engine-api/src/payload.rs b/crates/rpc/rpc-engine-api/src/payload.rs new file mode 100644 index 0000000000..95db05a3f7 --- /dev/null +++ b/crates/rpc/rpc-engine-api/src/payload.rs @@ -0,0 +1,56 @@ +use reth_primitives::{Withdrawal, H256}; +use reth_rpc_types::engine::{ExecutionPayload, PayloadAttributes}; + +/// Either an [ExecutionPayload] or a [PayloadAttributes]. +pub(crate) enum PayloadOrAttributes<'a> { + /// An [ExecutionPayload] and optional parent beacon block root. + ExecutionPayload { + /// The inner execution payload + payload: &'a ExecutionPayload, + /// The parent beacon block root + parent_beacon_block_root: Option, + }, + /// A [PayloadAttributes]. + PayloadAttributes(&'a PayloadAttributes), +} + +impl<'a> PayloadOrAttributes<'a> { + /// Construct a [PayloadOrAttributes] from an [ExecutionPayload] and optional parent beacon + /// block root. + pub(crate) fn from_execution_payload( + payload: &'a ExecutionPayload, + parent_beacon_block_root: Option, + ) -> Self { + Self::ExecutionPayload { payload, parent_beacon_block_root } + } + + /// Return the withdrawals for the payload or attributes. + pub(crate) fn withdrawals(&self) -> &Option> { + match self { + Self::ExecutionPayload { payload, .. } => &payload.withdrawals, + Self::PayloadAttributes(attributes) => &attributes.withdrawals, + } + } + + /// Return the timestamp for the payload or attributes. + pub(crate) fn timestamp(&self) -> u64 { + match self { + Self::ExecutionPayload { payload, .. } => payload.timestamp.as_u64(), + Self::PayloadAttributes(attributes) => attributes.timestamp.as_u64(), + } + } + + /// Return the parent beacon block root for the payload or attributes. + pub(crate) fn parent_beacon_block_root(&self) -> Option { + match self { + Self::ExecutionPayload { parent_beacon_block_root, .. } => *parent_beacon_block_root, + Self::PayloadAttributes(attributes) => attributes.parent_beacon_block_root, + } + } +} + +impl<'a> From<&'a PayloadAttributes> for PayloadOrAttributes<'a> { + fn from(attributes: &'a PayloadAttributes) -> Self { + Self::PayloadAttributes(attributes) + } +}