From 39f8327f734e62fbe4d49c574515a17998d88852 Mon Sep 17 00:00:00 2001 From: ethDreamer <37123614+ethDreamer@users.noreply.github.com> Date: Fri, 10 Feb 2023 08:49:25 -0600 Subject: [PATCH] Properly Deserialize ForkVersionedResponses (#3944) * Move ForkVersionedResponse to consensus/types * Properly Deserialize ForkVersionedResponses * Elide Types in from_value Calls * Added Tests for ForkVersionedResponse Deserialize * Address Sean's Comments & Make Less Restrictive * Utilize `map_fork_name!` --- beacon_node/execution_layer/src/lib.rs | 11 +- beacon_node/http_api/src/version.rs | 8 +- common/eth2/src/lib.rs | 63 +------- common/eth2/src/types.rs | 39 +++-- consensus/types/src/beacon_block.rs | 18 +++ consensus/types/src/beacon_state.rs | 16 ++ consensus/types/src/builder_bid.rs | 58 +++++++- consensus/types/src/execution_payload.rs | 23 +++ .../types/src/execution_payload_header.rs | 26 ++++ .../types/src/fork_versioned_response.rs | 138 ++++++++++++++++++ consensus/types/src/lib.rs | 4 + consensus/types/src/signed_beacon_block.rs | 18 +++ 12 files changed, 335 insertions(+), 87 deletions(-) create mode 100644 consensus/types/src/fork_versioned_response.rs diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index 752fc8f681..2337776c4f 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -13,7 +13,7 @@ pub use engine_api::*; pub use engine_api::{http, http::deposit_methods, http::HttpJsonRpc}; use engines::{Engine, EngineError}; pub use engines::{EngineState, ForkchoiceState}; -use eth2::types::{builder_bid::SignedBuilderBid, ForkVersionedResponse}; +use eth2::types::builder_bid::SignedBuilderBid; use fork_choice::ForkchoiceUpdateParameters; use lru::LruCache; use payload_status::process_payload_status; @@ -38,11 +38,10 @@ use tokio::{ use tokio_stream::wrappers::WatchStream; use types::{AbstractExecPayload, BeaconStateError, Blob, ExecPayload, KzgCommitment}; use types::{ - BlindedPayload, BlockType, ChainSpec, Epoch, ExecutionBlockHash, ForkName, - ProposerPreparationData, PublicKeyBytes, Signature, SignedBeaconBlock, Slot, Uint256, -}; -use types::{ - ExecutionPayload, ExecutionPayloadCapella, ExecutionPayloadEip4844, ExecutionPayloadMerge, + BlindedPayload, BlockType, ChainSpec, Epoch, ExecutionBlockHash, ExecutionPayload, + ExecutionPayloadCapella, ExecutionPayloadEip4844, ExecutionPayloadMerge, ForkName, + ForkVersionedResponse, ProposerPreparationData, PublicKeyBytes, Signature, SignedBeaconBlock, + Slot, Uint256, }; mod block_hash; diff --git a/beacon_node/http_api/src/version.rs b/beacon_node/http_api/src/version.rs index 87ba3a4663..30f475e689 100644 --- a/beacon_node/http_api/src/version.rs +++ b/beacon_node/http_api/src/version.rs @@ -1,9 +1,9 @@ -use crate::api_types::{ - EndpointVersion, ExecutionOptimisticForkVersionedResponse, ForkVersionedResponse, -}; +use crate::api_types::EndpointVersion; use eth2::CONSENSUS_VERSION_HEADER; use serde::Serialize; -use types::{ForkName, InconsistentFork}; +use types::{ + ExecutionOptimisticForkVersionedResponse, ForkName, ForkVersionedResponse, InconsistentFork, +}; use warp::reply::{self, Reply, WithHeader}; pub const V1: EndpointVersion = EndpointVersion(1); diff --git a/common/eth2/src/lib.rs b/common/eth2/src/lib.rs index 9c4248d4d9..4a3114e432 100644 --- a/common/eth2/src/lib.rs +++ b/common/eth2/src/lib.rs @@ -14,9 +14,8 @@ pub mod lighthouse_vc; pub mod mixin; pub mod types; -use self::mixin::{RequestAccept, ResponseForkName, ResponseOptional}; +use self::mixin::{RequestAccept, ResponseOptional}; use self::types::{Error as ResponseError, *}; -use ::types::map_fork_name_with; use futures::Stream; use futures_util::StreamExt; use lighthouse_network::PeerId; @@ -683,35 +682,7 @@ impl BeaconNodeHttpClient { None => return Ok(None), }; - // If present, use the fork provided in the headers to decode the block. Gracefully handle - // missing and malformed fork names by falling back to regular deserialisation. - let (block, version, execution_optimistic) = match response.fork_name_from_header() { - Ok(Some(fork_name)) => { - let (data, (version, execution_optimistic)) = - map_fork_name_with!(fork_name, SignedBeaconBlock, { - let ExecutionOptimisticForkVersionedResponse { - version, - execution_optimistic, - data, - } = response.json().await?; - (data, (version, execution_optimistic)) - }); - (data, version, execution_optimistic) - } - Ok(None) | Err(_) => { - let ExecutionOptimisticForkVersionedResponse { - version, - execution_optimistic, - data, - } = response.json().await?; - (data, version, execution_optimistic) - } - }; - Ok(Some(ExecutionOptimisticForkVersionedResponse { - version, - execution_optimistic, - data: block, - })) + Ok(Some(response.json().await?)) } /// `GET v1/beacon/blinded_blocks/{block_id}` @@ -728,35 +699,7 @@ impl BeaconNodeHttpClient { None => return Ok(None), }; - // If present, use the fork provided in the headers to decode the block. Gracefully handle - // missing and malformed fork names by falling back to regular deserialisation. - let (block, version, execution_optimistic) = match response.fork_name_from_header() { - Ok(Some(fork_name)) => { - let (data, (version, execution_optimistic)) = - map_fork_name_with!(fork_name, SignedBlindedBeaconBlock, { - let ExecutionOptimisticForkVersionedResponse { - version, - execution_optimistic, - data, - } = response.json().await?; - (data, (version, execution_optimistic)) - }); - (data, version, execution_optimistic) - } - Ok(None) | Err(_) => { - let ExecutionOptimisticForkVersionedResponse { - version, - execution_optimistic, - data, - } = response.json().await?; - (data, version, execution_optimistic) - } - }; - Ok(Some(ExecutionOptimisticForkVersionedResponse { - version, - execution_optimistic, - data: block, - })) + Ok(Some(response.json().await?)) } /// `GET v1/beacon/blocks` (LEGACY) diff --git a/common/eth2/src/types.rs b/common/eth2/src/types.rs index 4b7ae5539a..36a690911b 100644 --- a/common/eth2/src/types.rs +++ b/common/eth2/src/types.rs @@ -236,21 +236,6 @@ impl<'a, T: Serialize> From<&'a T> for GenericResponseRef<'a, T> { } } -#[derive(Debug, PartialEq, Clone, Serialize, Deserialize)] -pub struct ExecutionOptimisticForkVersionedResponse { - #[serde(skip_serializing_if = "Option::is_none")] - pub version: Option, - pub execution_optimistic: Option, - pub data: T, -} - -#[derive(Debug, PartialEq, Clone, Serialize, Deserialize)] -pub struct ForkVersionedResponse { - #[serde(skip_serializing_if = "Option::is_none")] - pub version: Option, - pub data: T, -} - #[derive(Debug, Clone, Copy, PartialEq, Serialize, Deserialize)] pub struct RootData { pub root: Hash256, @@ -1128,6 +1113,30 @@ pub struct BlocksAndBlobs> { pub kzg_aggregate_proof: KzgProof, } +impl> ForkVersionDeserialize + for BlocksAndBlobs +{ + fn deserialize_by_fork<'de, D: serde::Deserializer<'de>>( + value: serde_json::value::Value, + fork_name: ForkName, + ) -> Result { + #[derive(Deserialize)] + #[serde(bound = "T: EthSpec")] + struct Helper { + block: serde_json::Value, + blobs: Vec>, + kzg_aggregate_proof: KzgProof, + } + let helper: Helper = serde_json::from_value(value).map_err(serde::de::Error::custom)?; + + Ok(Self { + block: BeaconBlock::deserialize_by_fork::<'de, D>(helper.block, fork_name)?, + blobs: helper.blobs, + kzg_aggregate_proof: helper.kzg_aggregate_proof, + }) + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/consensus/types/src/beacon_block.rs b/consensus/types/src/beacon_block.rs index f7b9790b4d..f960b21178 100644 --- a/consensus/types/src/beacon_block.rs +++ b/consensus/types/src/beacon_block.rs @@ -685,6 +685,24 @@ impl From>> } } +impl> ForkVersionDeserialize + for BeaconBlock +{ + fn deserialize_by_fork<'de, D: serde::Deserializer<'de>>( + value: serde_json::value::Value, + fork_name: ForkName, + ) -> Result { + Ok(map_fork_name!( + fork_name, + Self, + serde_json::from_value(value).map_err(|e| serde::de::Error::custom(format!( + "BeaconBlock failed to deserialize: {:?}", + e + )))? + )) + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index b44c14ded5..0b07ce4958 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -1853,3 +1853,19 @@ impl CompareFields for BeaconState { } } } + +impl ForkVersionDeserialize for BeaconState { + fn deserialize_by_fork<'de, D: serde::Deserializer<'de>>( + value: serde_json::value::Value, + fork_name: ForkName, + ) -> Result { + Ok(map_fork_name!( + fork_name, + Self, + serde_json::from_value(value).map_err(|e| serde::de::Error::custom(format!( + "BeaconState failed to deserialize: {:?}", + e + )))? + )) + } +} diff --git a/consensus/types/src/builder_bid.rs b/consensus/types/src/builder_bid.rs index 818ec52b81..e922e81c70 100644 --- a/consensus/types/src/builder_bid.rs +++ b/consensus/types/src/builder_bid.rs @@ -1,6 +1,6 @@ use crate::{ - AbstractExecPayload, ChainSpec, EthSpec, ExecPayload, ExecutionPayloadHeader, SignedRoot, - Uint256, + AbstractExecPayload, ChainSpec, EthSpec, ExecPayload, ExecutionPayloadHeader, ForkName, + ForkVersionDeserialize, SignedRoot, Uint256, }; use bls::PublicKeyBytes; use bls::Signature; @@ -34,6 +34,60 @@ pub struct SignedBuilderBid> { pub signature: Signature, } +impl> ForkVersionDeserialize + for BuilderBid +{ + fn deserialize_by_fork<'de, D: serde::Deserializer<'de>>( + value: serde_json::value::Value, + fork_name: ForkName, + ) -> Result { + let convert_err = |_| { + serde::de::Error::custom( + "BuilderBid failed to deserialize: unable to convert payload header to payload", + ) + }; + + #[derive(Deserialize)] + struct Helper { + header: serde_json::Value, + #[serde(with = "eth2_serde_utils::quoted_u256")] + value: Uint256, + pubkey: PublicKeyBytes, + } + let helper: Helper = serde_json::from_value(value).map_err(serde::de::Error::custom)?; + let payload_header = + ExecutionPayloadHeader::deserialize_by_fork::<'de, D>(helper.header, fork_name)?; + + Ok(Self { + header: Payload::try_from(payload_header).map_err(convert_err)?, + value: helper.value, + pubkey: helper.pubkey, + _phantom_data: Default::default(), + }) + } +} + +impl> ForkVersionDeserialize + for SignedBuilderBid +{ + fn deserialize_by_fork<'de, D: serde::Deserializer<'de>>( + value: serde_json::value::Value, + fork_name: ForkName, + ) -> Result { + #[derive(Deserialize)] + struct Helper { + pub message: serde_json::Value, + pub signature: Signature, + } + let helper: Helper = serde_json::from_value(value).map_err(serde::de::Error::custom)?; + + Ok(Self { + message: BuilderBid::deserialize_by_fork::<'de, D>(helper.message, fork_name)?, + signature: helper.signature, + }) + } +} + struct BlindedPayloadAsHeader(PhantomData); impl> SerializeAs for BlindedPayloadAsHeader { diff --git a/consensus/types/src/execution_payload.rs b/consensus/types/src/execution_payload.rs index 1721960f8b..16b2778355 100644 --- a/consensus/types/src/execution_payload.rs +++ b/consensus/types/src/execution_payload.rs @@ -146,3 +146,26 @@ impl ExecutionPayload { + (T::max_withdrawals_per_payload() * ::ssz_fixed_len()) } } + +impl ForkVersionDeserialize for ExecutionPayload { + fn deserialize_by_fork<'de, D: serde::Deserializer<'de>>( + value: serde_json::value::Value, + fork_name: ForkName, + ) -> Result { + let convert_err = |e| { + serde::de::Error::custom(format!("ExecutionPayload failed to deserialize: {:?}", e)) + }; + + Ok(match fork_name { + ForkName::Merge => Self::Merge(serde_json::from_value(value).map_err(convert_err)?), + ForkName::Capella => Self::Capella(serde_json::from_value(value).map_err(convert_err)?), + ForkName::Eip4844 => Self::Eip4844(serde_json::from_value(value).map_err(convert_err)?), + ForkName::Base | ForkName::Altair => { + return Err(serde::de::Error::custom(format!( + "ExecutionPayload failed to deserialize: unsupported fork '{}'", + fork_name + ))); + } + }) + } +} diff --git a/consensus/types/src/execution_payload_header.rs b/consensus/types/src/execution_payload_header.rs index 42e44ed739..695c0cfdf4 100644 --- a/consensus/types/src/execution_payload_header.rs +++ b/consensus/types/src/execution_payload_header.rs @@ -282,3 +282,29 @@ impl TryFrom> for ExecutionPayloadHeaderEi } } } + +impl ForkVersionDeserialize for ExecutionPayloadHeader { + fn deserialize_by_fork<'de, D: serde::Deserializer<'de>>( + value: serde_json::value::Value, + fork_name: ForkName, + ) -> Result { + let convert_err = |e| { + serde::de::Error::custom(format!( + "ExecutionPayloadHeader failed to deserialize: {:?}", + e + )) + }; + + Ok(match fork_name { + ForkName::Merge => Self::Merge(serde_json::from_value(value).map_err(convert_err)?), + ForkName::Capella => Self::Capella(serde_json::from_value(value).map_err(convert_err)?), + ForkName::Eip4844 => Self::Eip4844(serde_json::from_value(value).map_err(convert_err)?), + ForkName::Base | ForkName::Altair => { + return Err(serde::de::Error::custom(format!( + "ExecutionPayloadHeader failed to deserialize: unsupported fork '{}'", + fork_name + ))); + } + }) + } +} diff --git a/consensus/types/src/fork_versioned_response.rs b/consensus/types/src/fork_versioned_response.rs new file mode 100644 index 0000000000..07ff40b27e --- /dev/null +++ b/consensus/types/src/fork_versioned_response.rs @@ -0,0 +1,138 @@ +use crate::ForkName; +use serde::de::DeserializeOwned; +use serde::{Deserialize, Deserializer, Serialize}; +use serde_json::value::Value; +use std::sync::Arc; + +// Deserialize is only implemented for types that implement ForkVersionDeserialize +#[derive(Debug, PartialEq, Clone, Serialize)] +pub struct ExecutionOptimisticForkVersionedResponse { + #[serde(skip_serializing_if = "Option::is_none")] + pub version: Option, + pub execution_optimistic: Option, + pub data: T, +} + +impl<'de, F> serde::Deserialize<'de> for ExecutionOptimisticForkVersionedResponse +where + F: ForkVersionDeserialize, +{ + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + #[derive(Deserialize)] + struct Helper { + version: Option, + execution_optimistic: Option, + data: serde_json::Value, + } + + let helper = Helper::deserialize(deserializer)?; + let data = match helper.version { + Some(fork_name) => F::deserialize_by_fork::<'de, D>(helper.data, fork_name)?, + None => serde_json::from_value(helper.data).map_err(serde::de::Error::custom)?, + }; + + Ok(ExecutionOptimisticForkVersionedResponse { + version: helper.version, + execution_optimistic: helper.execution_optimistic, + data, + }) + } +} + +pub trait ForkVersionDeserialize: Sized + DeserializeOwned { + fn deserialize_by_fork<'de, D: Deserializer<'de>>( + value: Value, + fork_name: ForkName, + ) -> Result; +} + +// Deserialize is only implemented for types that implement ForkVersionDeserialize +#[derive(Debug, PartialEq, Clone, Serialize)] +pub struct ForkVersionedResponse { + #[serde(skip_serializing_if = "Option::is_none")] + pub version: Option, + pub data: T, +} + +impl<'de, F> serde::Deserialize<'de> for ForkVersionedResponse +where + F: ForkVersionDeserialize, +{ + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + #[derive(Deserialize)] + struct Helper { + version: Option, + data: serde_json::Value, + } + + let helper = Helper::deserialize(deserializer)?; + let data = match helper.version { + Some(fork_name) => F::deserialize_by_fork::<'de, D>(helper.data, fork_name)?, + None => serde_json::from_value(helper.data).map_err(serde::de::Error::custom)?, + }; + + Ok(ForkVersionedResponse { + version: helper.version, + data, + }) + } +} + +impl ForkVersionDeserialize for Arc { + fn deserialize_by_fork<'de, D: Deserializer<'de>>( + value: Value, + fork_name: ForkName, + ) -> Result { + Ok(Arc::new(F::deserialize_by_fork::<'de, D>( + value, fork_name, + )?)) + } +} + +#[cfg(test)] +mod fork_version_response_tests { + use crate::{ + ExecutionPayload, ExecutionPayloadMerge, ForkName, ForkVersionedResponse, MainnetEthSpec, + }; + use serde_json::json; + + #[test] + fn fork_versioned_response_deserialize_correct_fork() { + type E = MainnetEthSpec; + + let response_json = + serde_json::to_string(&json!(ForkVersionedResponse::> { + version: Some(ForkName::Merge), + data: ExecutionPayload::Merge(ExecutionPayloadMerge::default()), + })) + .unwrap(); + + let result: Result>, _> = + serde_json::from_str(&response_json); + + assert!(result.is_ok()); + } + + #[test] + fn fork_versioned_response_deserialize_incorrect_fork() { + type E = MainnetEthSpec; + + let response_json = + serde_json::to_string(&json!(ForkVersionedResponse::> { + version: Some(ForkName::Capella), + data: ExecutionPayload::Merge(ExecutionPayloadMerge::default()), + })) + .unwrap(); + + let result: Result>, _> = + serde_json::from_str(&response_json); + + assert!(result.is_err()); + } +} diff --git a/consensus/types/src/lib.rs b/consensus/types/src/lib.rs index 7bb0045e24..8d9156ff5d 100644 --- a/consensus/types/src/lib.rs +++ b/consensus/types/src/lib.rs @@ -46,6 +46,7 @@ pub mod execution_payload_header; pub mod fork; pub mod fork_data; pub mod fork_name; +pub mod fork_versioned_response; pub mod free_attestation; pub mod graffiti; pub mod historical_batch; @@ -150,6 +151,9 @@ pub use crate::fork::Fork; pub use crate::fork_context::ForkContext; pub use crate::fork_data::ForkData; pub use crate::fork_name::{ForkName, InconsistentFork}; +pub use crate::fork_versioned_response::{ + ExecutionOptimisticForkVersionedResponse, ForkVersionDeserialize, ForkVersionedResponse, +}; pub use crate::free_attestation::FreeAttestation; pub use crate::graffiti::{Graffiti, GRAFFITI_BYTES_LEN}; pub use crate::historical_batch::HistoricalBatch; diff --git a/consensus/types/src/signed_beacon_block.rs b/consensus/types/src/signed_beacon_block.rs index cd6cd5cb9e..70fb28fbe7 100644 --- a/consensus/types/src/signed_beacon_block.rs +++ b/consensus/types/src/signed_beacon_block.rs @@ -487,6 +487,24 @@ impl SignedBeaconBlock { } } +impl> ForkVersionDeserialize + for SignedBeaconBlock +{ + fn deserialize_by_fork<'de, D: serde::Deserializer<'de>>( + value: serde_json::value::Value, + fork_name: ForkName, + ) -> Result { + Ok(map_fork_name!( + fork_name, + Self, + serde_json::from_value(value).map_err(|e| serde::de::Error::custom(format!( + "SignedBeaconBlock failed to deserialize: {:?}", + e + )))? + )) + } +} + #[cfg(test)] mod test { use super::*;