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

@@ -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;

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);

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)

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<T> {
-    #[serde(skip_serializing_if = "Option::is_none")]
-    pub version: Option<ForkName>,
-    pub execution_optimistic: Option<bool>,
-    pub data: T,
-}
-
-#[derive(Debug, PartialEq, Clone, Serialize, Deserialize)]
-pub struct ForkVersionedResponse<T> {
-    #[serde(skip_serializing_if = "Option::is_none")]
-    pub version: Option<ForkName>,
-    pub data: T,
-}
-
 #[derive(Debug, Clone, Copy, PartialEq, Serialize, Deserialize)]
 pub struct RootData {
     pub root: Hash256,
@@ -1128,6 +1113,30 @@ pub struct BlocksAndBlobs<T: EthSpec, Payload: AbstractExecPayload<T>> {
     pub kzg_aggregate_proof: KzgProof,
 }
 
+impl<T: EthSpec, Payload: AbstractExecPayload<T>> ForkVersionDeserialize
+    for BlocksAndBlobs<T, Payload>
+{
+    fn deserialize_by_fork<'de, D: serde::Deserializer<'de>>(
+        value: serde_json::value::Value,
+        fork_name: ForkName,
+    ) -> Result<Self, D::Error> {
+        #[derive(Deserialize)]
+        #[serde(bound = "T: EthSpec")]
+        struct Helper<T: EthSpec> {
+            block: serde_json::Value,
+            blobs: Vec<Blob<T>>,
+            kzg_aggregate_proof: KzgProof,
+        }
+        let helper: Helper<T> = 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::*;

consensus/types/src/beacon_block.rs

@@ -685,6 +685,24 @@ impl<E: EthSpec> From<BeaconBlock<E, FullPayload<E>>>
     }
 }
 
+impl<T: EthSpec, Payload: AbstractExecPayload<T>> ForkVersionDeserialize
+    for BeaconBlock<T, Payload>
+{
+    fn deserialize_by_fork<'de, D: serde::Deserializer<'de>>(
+        value: serde_json::value::Value,
+        fork_name: ForkName,
+    ) -> Result<Self, D::Error> {
+        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::*;

consensus/types/src/beacon_state.rs

@@ -1853,3 +1853,19 @@ impl<T: EthSpec> CompareFields for BeaconState<T> {
         }
     }
 }
+
+impl<T: EthSpec> ForkVersionDeserialize for BeaconState<T> {
+    fn deserialize_by_fork<'de, D: serde::Deserializer<'de>>(
+        value: serde_json::value::Value,
+        fork_name: ForkName,
+    ) -> Result<Self, D::Error> {
+        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
+            )))?
+        ))
+    }
+}

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<E: EthSpec, Payload: AbstractExecPayload<E>> {
     pub signature: Signature,
 }
 
+impl<T: EthSpec, Payload: AbstractExecPayload<T>> ForkVersionDeserialize
+    for BuilderBid<T, Payload>
+{
+    fn deserialize_by_fork<'de, D: serde::Deserializer<'de>>(
+        value: serde_json::value::Value,
+        fork_name: ForkName,
+    ) -> Result<Self, D::Error> {
+        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<T: EthSpec, Payload: AbstractExecPayload<T>> ForkVersionDeserialize
+    for SignedBuilderBid<T, Payload>
+{
+    fn deserialize_by_fork<'de, D: serde::Deserializer<'de>>(
+        value: serde_json::value::Value,
+        fork_name: ForkName,
+    ) -> Result<Self, D::Error> {
+        #[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<E>(PhantomData<E>);
 
 impl<E: EthSpec, Payload: ExecPayload<E>> SerializeAs<Payload> for BlindedPayloadAsHeader<E> {

consensus/types/src/execution_payload.rs

@@ -146,3 +146,26 @@ impl<T: EthSpec> ExecutionPayload<T> {
             + (T::max_withdrawals_per_payload() * <Withdrawal as Encode>::ssz_fixed_len())
     }
 }
+
+impl<T: EthSpec> ForkVersionDeserialize for ExecutionPayload<T> {
+    fn deserialize_by_fork<'de, D: serde::Deserializer<'de>>(
+        value: serde_json::value::Value,
+        fork_name: ForkName,
+    ) -> Result<Self, D::Error> {
+        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
+                )));
+            }
+        })
+    }
+}

consensus/types/src/execution_payload_header.rs

@@ -282,3 +282,29 @@ impl<T: EthSpec> TryFrom<ExecutionPayloadHeader<T>> for ExecutionPayloadHeaderEi
         }
     }
 }
+
+impl<T: EthSpec> ForkVersionDeserialize for ExecutionPayloadHeader<T> {
+    fn deserialize_by_fork<'de, D: serde::Deserializer<'de>>(
+        value: serde_json::value::Value,
+        fork_name: ForkName,
+    ) -> Result<Self, D::Error> {
+        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
+                )));
+            }
+        })
+    }
+}

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<T> {
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub version: Option<ForkName>,
+    pub execution_optimistic: Option<bool>,
+    pub data: T,
+}
+
+impl<'de, F> serde::Deserialize<'de> for ExecutionOptimisticForkVersionedResponse<F>
+where
+    F: ForkVersionDeserialize,
+{
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: Deserializer<'de>,
+    {
+        #[derive(Deserialize)]
+        struct Helper {
+            version: Option<ForkName>,
+            execution_optimistic: Option<bool>,
+            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<Self, D::Error>;
+}
+
+// Deserialize is only implemented for types that implement ForkVersionDeserialize
+#[derive(Debug, PartialEq, Clone, Serialize)]
+pub struct ForkVersionedResponse<T> {
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub version: Option<ForkName>,
+    pub data: T,
+}
+
+impl<'de, F> serde::Deserialize<'de> for ForkVersionedResponse<F>
+where
+    F: ForkVersionDeserialize,
+{
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: Deserializer<'de>,
+    {
+        #[derive(Deserialize)]
+        struct Helper {
+            version: Option<ForkName>,
+            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<F: ForkVersionDeserialize> ForkVersionDeserialize for Arc<F> {
+    fn deserialize_by_fork<'de, D: Deserializer<'de>>(
+        value: Value,
+        fork_name: ForkName,
+    ) -> Result<Self, D::Error> {
+        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::<ExecutionPayload<E>> {
+                version: Some(ForkName::Merge),
+                data: ExecutionPayload::Merge(ExecutionPayloadMerge::default()),
+            }))
+            .unwrap();
+
+        let result: Result<ForkVersionedResponse<ExecutionPayload<E>>, _> =
+            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::<ExecutionPayload<E>> {
+                version: Some(ForkName::Capella),
+                data: ExecutionPayload::Merge(ExecutionPayloadMerge::default()),
+            }))
+            .unwrap();
+
+        let result: Result<ForkVersionedResponse<ExecutionPayload<E>>, _> =
+            serde_json::from_str(&response_json);
+
+        assert!(result.is_err());
+    }
+}

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;

consensus/types/src/signed_beacon_block.rs

@@ -487,6 +487,24 @@ impl<E: EthSpec> SignedBeaconBlock<E> {
     }
 }
 
+impl<E: EthSpec, Payload: AbstractExecPayload<E>> ForkVersionDeserialize
+    for SignedBeaconBlock<E, Payload>
+{
+    fn deserialize_by_fork<'de, D: serde::Deserializer<'de>>(
+        value: serde_json::value::Value,
+        fork_name: ForkName,
+    ) -> Result<Self, D::Error> {
+        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::*;