From d24a4ffe30defee35c4285ff16865e434a437caa Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Tue, 8 Apr 2025 19:00:55 -0700 Subject: [PATCH] Fix builder API electra json response (#7285) #7277 Implement `ForkVersionDeserialize` for `ExecutionPayloadAndBlobs` so we get fork hinting when deserializing --- Cargo.lock | 2 + common/eth2/Cargo.toml | 2 + common/eth2/src/types.rs | 137 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 138 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 746cac9c14..d1ceb2dbaf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2615,6 +2615,7 @@ dependencies = [ "mediatype", "pretty_reqwest_error", "proto_array", + "rand 0.8.5", "reqwest", "reqwest-eventsource", "sensitive_url", @@ -2623,6 +2624,7 @@ dependencies = [ "slashing_protection", "ssz_types", "store", + "test_random_derive", "tokio", "types", "zeroize", diff --git a/common/eth2/Cargo.toml b/common/eth2/Cargo.toml index a1bc9d025b..a39a58ac14 100644 --- a/common/eth2/Cargo.toml +++ b/common/eth2/Cargo.toml @@ -18,6 +18,7 @@ lighthouse_network = { workspace = true } mediatype = "0.19.13" pretty_reqwest_error = { workspace = true } proto_array = { workspace = true } +rand = { workspace = true } reqwest = { workspace = true } reqwest-eventsource = "0.5.0" sensitive_url = { workspace = true } @@ -26,6 +27,7 @@ serde_json = { workspace = true } slashing_protection = { workspace = true } ssz_types = { workspace = true } store = { workspace = true } +test_random_derive = { path = "../../common/test_random_derive" } types = { workspace = true } zeroize = { workspace = true } diff --git a/common/eth2/src/types.rs b/common/eth2/src/types.rs index dd4f5437ae..66b4b7ea54 100644 --- a/common/eth2/src/types.rs +++ b/common/eth2/src/types.rs @@ -16,7 +16,9 @@ use std::fmt::{self, Display}; use std::str::FromStr; use std::sync::Arc; use std::time::Duration; +use test_random_derive::TestRandom; use types::beacon_block_body::KzgCommitments; +use types::test_utils::TestRandom; pub use types::*; #[cfg(feature = "lighthouse")] @@ -2017,11 +2019,11 @@ impl ForkVersionDeserialize for FullPayloadContents { fork_name: ForkName, ) -> Result { if fork_name.deneb_enabled() { - serde_json::from_value(value) + ExecutionPayloadAndBlobs::deserialize_by_fork::<'de, D>(value, fork_name) .map(Self::PayloadAndBlobs) .map_err(serde::de::Error::custom) } else if fork_name.bellatrix_enabled() { - serde_json::from_value(value) + ExecutionPayload::deserialize_by_fork::<'de, D>(value, fork_name) .map(Self::Payload) .map_err(serde::de::Error::custom) } else { @@ -2039,6 +2041,28 @@ pub struct ExecutionPayloadAndBlobs { pub blobs_bundle: BlobsBundle, } +impl ForkVersionDeserialize for ExecutionPayloadAndBlobs { + fn deserialize_by_fork<'de, D: Deserializer<'de>>( + value: Value, + fork_name: ForkName, + ) -> Result { + #[derive(Deserialize)] + #[serde(bound = "E: EthSpec")] + struct Helper { + execution_payload: serde_json::Value, + blobs_bundle: BlobsBundle, + } + let helper: Helper = serde_json::from_value(value).map_err(serde::de::Error::custom)?; + Ok(Self { + execution_payload: ExecutionPayload::deserialize_by_fork::<'de, D>( + helper.execution_payload, + fork_name, + )?, + blobs_bundle: helper.blobs_bundle, + }) + } +} + impl ForkVersionDecode for ExecutionPayloadAndBlobs { fn from_ssz_bytes_by_fork(bytes: &[u8], fork_name: ForkName) -> Result { let mut builder = ssz::SszDecoderBuilder::new(bytes); @@ -2069,7 +2093,7 @@ pub enum ContentType { Ssz, } -#[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize, Encode, Decode)] +#[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize, Encode, Decode, TestRandom)] #[serde(bound = "E: EthSpec")] pub struct BlobsBundle { pub commitments: KzgCommitments, @@ -2080,6 +2104,10 @@ pub struct BlobsBundle { #[cfg(test)] mod test { + use std::fmt::Debug; + + use types::test_utils::{SeedableRng, TestRandom, XorShiftRng}; + use super::*; #[test] @@ -2093,4 +2121,107 @@ mod test { let y: ValidatorId = serde_json::from_str(pubkey_str).unwrap(); assert_eq!(serde_json::to_string(&y).unwrap(), pubkey_str); } + + #[test] + fn test_execution_payload_execution_payload_deserialize_by_fork() { + let rng = &mut XorShiftRng::from_seed([42; 16]); + + let payloads = [ + ExecutionPayload::Bellatrix( + ExecutionPayloadBellatrix::::random_for_test(rng), + ), + ExecutionPayload::Capella(ExecutionPayloadCapella::::random_for_test( + rng, + )), + ExecutionPayload::Deneb(ExecutionPayloadDeneb::::random_for_test( + rng, + )), + ExecutionPayload::Electra(ExecutionPayloadElectra::::random_for_test( + rng, + )), + ExecutionPayload::Fulu(ExecutionPayloadFulu::::random_for_test(rng)), + ]; + let merged_forks = &ForkName::list_all()[2..]; + assert_eq!( + payloads.len(), + merged_forks.len(), + "we should test every known fork; add new fork variant to payloads above" + ); + + for (payload, &fork_name) in payloads.into_iter().zip(merged_forks) { + assert_eq!(payload.fork_name(), fork_name); + let payload_str = serde_json::to_string(&payload).unwrap(); + let mut de = serde_json::Deserializer::from_str(&payload_str); + generic_deserialize_by_fork(&mut de, payload, fork_name); + } + } + + #[test] + fn test_execution_payload_and_blobs_deserialize_by_fork() { + let rng = &mut XorShiftRng::from_seed([42; 16]); + + let payloads = [ + { + let execution_payload = + ExecutionPayload::Deneb( + ExecutionPayloadDeneb::::random_for_test(rng), + ); + let blobs_bundle = BlobsBundle::random_for_test(rng); + ExecutionPayloadAndBlobs { + execution_payload, + blobs_bundle, + } + }, + { + let execution_payload = + ExecutionPayload::Electra( + ExecutionPayloadElectra::::random_for_test(rng), + ); + let blobs_bundle = BlobsBundle::random_for_test(rng); + ExecutionPayloadAndBlobs { + execution_payload, + blobs_bundle, + } + }, + { + let execution_payload = + ExecutionPayload::Fulu( + ExecutionPayloadFulu::::random_for_test(rng), + ); + let blobs_bundle = BlobsBundle::random_for_test(rng); + ExecutionPayloadAndBlobs { + execution_payload, + blobs_bundle, + } + }, + ]; + let blob_forks = &ForkName::list_all()[4..]; + + assert_eq!( + payloads.len(), + blob_forks.len(), + "we should test every known fork; add new fork variant to payloads above" + ); + + for (payload, &fork_name) in payloads.into_iter().zip(blob_forks) { + assert_eq!(payload.execution_payload.fork_name(), fork_name); + let payload_str = serde_json::to_string(&payload).unwrap(); + let mut de = serde_json::Deserializer::from_str(&payload_str); + generic_deserialize_by_fork(&mut de, payload, fork_name); + } + } + + fn generic_deserialize_by_fork< + 'de, + D: Deserializer<'de>, + O: ForkVersionDeserialize + PartialEq + Debug, + >( + deserializer: D, + original: O, + fork_name: ForkName, + ) { + let val = Value::deserialize(deserializer).unwrap(); + let roundtrip = O::deserialize_by_fork::<'de, D>(val, fork_name).unwrap(); + assert_eq!(original, roundtrip); + } }