From 185646acb2ddcab08d0c2896e675e9bf49be1314 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 19 Jan 2024 12:56:30 +1100 Subject: [PATCH] Fix PublishBlockRequest SSZ decoding (#5078) * Fix PublishBlockRequest SSZ decoding * Send header for block v1 ssz client * Delete unused function --- beacon_node/http_api/src/lib.rs | 12 +++++-- .../tests/broadcast_validation_tests.rs | 10 +++--- common/eth2/src/lib.rs | 31 +++++------------ common/eth2/src/types.rs | 34 +++++++------------ consensus/types/src/signed_beacon_block.rs | 10 ++++++ 5 files changed, 47 insertions(+), 50 deletions(-) diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index f1d7769e896..02ea9518c69 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -45,7 +45,7 @@ use eth2::types::{ PublishBlockRequest, ValidatorBalancesRequestBody, ValidatorId, ValidatorStatus, ValidatorsRequestBody, }; -use eth2::{CONTENT_TYPE_HEADER, SSZ_CONTENT_TYPE_HEADER}; +use eth2::{CONSENSUS_VERSION_HEADER, CONTENT_TYPE_HEADER, SSZ_CONTENT_TYPE_HEADER}; use lighthouse_network::{types::SyncState, EnrExt, NetworkGlobals, PeerId, PubsubMessage}; use lighthouse_version::version_with_platform; use logging::SSELoggingComponents; @@ -1249,6 +1249,8 @@ pub fn serve( /* * beacon/blocks */ + let consensus_version_header_filter = + warp::header::header::(CONSENSUS_VERSION_HEADER); // POST beacon/blocks let post_beacon_blocks = eth_v1 @@ -1286,12 +1288,14 @@ pub fn serve( .and(warp::path("blocks")) .and(warp::path::end()) .and(warp::body::bytes()) + .and(consensus_version_header_filter) .and(task_spawner_filter.clone()) .and(chain_filter.clone()) .and(network_tx_filter.clone()) .and(log_filter.clone()) .then( move |block_bytes: Bytes, + consensus_version: ForkName, task_spawner: TaskSpawner, chain: Arc>, network_tx: UnboundedSender>, @@ -1299,7 +1303,7 @@ pub fn serve( task_spawner.spawn_async_with_rejection(Priority::P0, async move { let block_contents = PublishBlockRequest::::from_ssz_bytes( &block_bytes, - &chain.spec, + consensus_version, ) .map_err(|e| { warp_utils::reject::custom_bad_request(format!("invalid SSZ: {e:?}")) @@ -1356,6 +1360,7 @@ pub fn serve( .and(warp::query::()) .and(warp::path::end()) .and(warp::body::bytes()) + .and(consensus_version_header_filter) .and(task_spawner_filter.clone()) .and(chain_filter.clone()) .and(network_tx_filter.clone()) @@ -1363,6 +1368,7 @@ pub fn serve( .then( move |validation_level: api_types::BroadcastValidationQuery, block_bytes: Bytes, + consensus_version: ForkName, task_spawner: TaskSpawner, chain: Arc>, network_tx: UnboundedSender>, @@ -1370,7 +1376,7 @@ pub fn serve( task_spawner.spawn_async_with_rejection(Priority::P0, async move { let block_contents = PublishBlockRequest::::from_ssz_bytes( &block_bytes, - &chain.spec, + consensus_version, ) .map_err(|e| { warp_utils::reject::custom_bad_request(format!("invalid SSZ: {e:?}")) diff --git a/beacon_node/http_api/tests/broadcast_validation_tests.rs b/beacon_node/http_api/tests/broadcast_validation_tests.rs index eb39bdd1153..2d42371a7c9 100644 --- a/beacon_node/http_api/tests/broadcast_validation_tests.rs +++ b/beacon_node/http_api/tests/broadcast_validation_tests.rs @@ -2,17 +2,16 @@ use beacon_chain::{ test_utils::{AttestationStrategy, BlockStrategy}, GossipVerifiedBlock, IntoGossipVerifiedBlockContents, }; +use eth2::reqwest::StatusCode; use eth2::types::{BroadcastValidation, PublishBlockRequest, SignedBeaconBlock}; use http_api::test_utils::InteractiveTester; use http_api::{publish_blinded_block, publish_block, reconstruct_block, ProvenancedBlock}; use std::sync::Arc; use tree_hash::TreeHash; -use types::{Hash256, MainnetEthSpec, Slot}; +use types::{Epoch, EthSpec, ForkName, Hash256, MainnetEthSpec, Slot}; use warp::Rejection; use warp_utils::reject::CustomBadRequest; -use eth2::reqwest::StatusCode; - type E = MainnetEthSpec; /* @@ -190,7 +189,10 @@ pub async fn gossip_full_pass_ssz() { // `validator_count // 32`. let validator_count = 64; let num_initial: u64 = 31; - let tester = InteractiveTester::::new(None, validator_count).await; + // Deneb epoch set ahead of block slot, to test fork-based decoding + let mut spec = ForkName::Capella.make_genesis_spec(MainnetEthSpec::default_spec()); + spec.deneb_fork_epoch = Some(Epoch::new(4)); + let tester = InteractiveTester::::new(Some(spec), validator_count).await; // Create some chain depth. tester.harness.advance_slot(); diff --git a/common/eth2/src/lib.rs b/common/eth2/src/lib.rs index 9c8edc4bdbb..bed5044b4be 100644 --- a/common/eth2/src/lib.rs +++ b/common/eth2/src/lib.rs @@ -377,25 +377,6 @@ impl BeaconNodeHttpClient { ok_or_error(response).await } - /// Generic POST function supporting arbitrary responses and timeouts. - async fn post_generic_with_ssz_body, U: IntoUrl>( - &self, - url: U, - body: T, - timeout: Option, - ) -> Result { - let mut builder = self.client.post(url); - if let Some(timeout) = timeout { - builder = builder.timeout(timeout); - } - let response = builder - .header("Content-Type", "application/octet-stream") - .body(body) - .send() - .await?; - ok_or_error(response).await - } - /// Generic POST function supporting arbitrary responses and timeouts. async fn post_generic_with_consensus_version( &self, @@ -866,10 +847,11 @@ impl BeaconNodeHttpClient { .push("beacon") .push("blocks"); - self.post_generic_with_ssz_body( + self.post_generic_with_consensus_version_and_ssz_body( path, block_contents.as_ssz_bytes(), Some(self.timeouts.proposal), + block_contents.signed_block().fork_name_unchecked(), ) .await?; @@ -910,8 +892,13 @@ impl BeaconNodeHttpClient { .push("beacon") .push("blinded_blocks"); - self.post_generic_with_ssz_body(path, block.as_ssz_bytes(), Some(self.timeouts.proposal)) - .await?; + self.post_generic_with_consensus_version_and_ssz_body( + path, + block.as_ssz_bytes(), + Some(self.timeouts.proposal), + block.fork_name_unchecked(), + ) + .await?; Ok(()) } diff --git a/common/eth2/src/types.rs b/common/eth2/src/types.rs index 98074615b8d..3e8d0c60795 100644 --- a/common/eth2/src/types.rs +++ b/common/eth2/src/types.rs @@ -1487,7 +1487,7 @@ mod tests { .expect("should convert into signed block contents"); let decoded: PublishBlockRequest = - PublishBlockRequest::from_ssz_bytes(&block.as_ssz_bytes(), &spec) + PublishBlockRequest::from_ssz_bytes(&block.as_ssz_bytes(), ForkName::Capella) .expect("should decode Block"); assert!(matches!(decoded, PublishBlockRequest::Block(_))); } @@ -1505,9 +1505,11 @@ mod tests { let kzg_proofs = KzgProofs::::from(vec![KzgProof::empty()]); let signed_block_contents = PublishBlockRequest::new(block, Some((kzg_proofs, blobs))); - let decoded: PublishBlockRequest = - PublishBlockRequest::from_ssz_bytes(&signed_block_contents.as_ssz_bytes(), &spec) - .expect("should decode BlockAndBlobSidecars"); + let decoded: PublishBlockRequest = PublishBlockRequest::from_ssz_bytes( + &signed_block_contents.as_ssz_bytes(), + ForkName::Deneb, + ) + .expect("should decode BlockAndBlobSidecars"); assert!(matches!(decoded, PublishBlockRequest::BlockContents(_))); } } @@ -1746,22 +1748,11 @@ impl PublishBlockRequest { } } - /// SSZ decode with fork variant determined by slot. - pub fn from_ssz_bytes(bytes: &[u8], spec: &ChainSpec) -> Result { - let slot_len = ::ssz_fixed_len(); - let slot_bytes = bytes - .get(0..slot_len) - .ok_or(DecodeError::InvalidByteLength { - len: bytes.len(), - expected: slot_len, - })?; - - let slot = Slot::from_ssz_bytes(slot_bytes)?; - let fork_at_slot = spec.fork_name_at_slot::(slot); - - match fork_at_slot { + /// SSZ decode with fork variant determined by `fork_name`. + pub fn from_ssz_bytes(bytes: &[u8], fork_name: ForkName) -> Result { + match fork_name { ForkName::Base | ForkName::Altair | ForkName::Merge | ForkName::Capella => { - SignedBeaconBlock::from_ssz_bytes(bytes, spec) + SignedBeaconBlock::from_ssz_bytes_for_fork(bytes, fork_name) .map(|block| PublishBlockRequest::Block(block)) } ForkName::Deneb => { @@ -1771,8 +1762,9 @@ impl PublishBlockRequest { builder.register_type::>()?; let mut decoder = builder.build()?; - let block = decoder - .decode_next_with(|bytes| SignedBeaconBlock::from_ssz_bytes(bytes, spec))?; + let block = decoder.decode_next_with(|bytes| { + SignedBeaconBlock::from_ssz_bytes_for_fork(bytes, fork_name) + })?; let kzg_proofs = decoder.decode_next()?; let blobs = decoder.decode_next()?; Ok(PublishBlockRequest::new(block, Some((kzg_proofs, blobs)))) diff --git a/consensus/types/src/signed_beacon_block.rs b/consensus/types/src/signed_beacon_block.rs index 24b5e27c8c3..37304de1f1b 100644 --- a/consensus/types/src/signed_beacon_block.rs +++ b/consensus/types/src/signed_beacon_block.rs @@ -104,6 +104,16 @@ impl> SignedBeaconBlock Self::from_ssz_bytes_with(bytes, |bytes| BeaconBlock::from_ssz_bytes(bytes, spec)) } + /// SSZ decode with explicit fork variant. + pub fn from_ssz_bytes_for_fork( + bytes: &[u8], + fork_name: ForkName, + ) -> Result { + Self::from_ssz_bytes_with(bytes, |bytes| { + BeaconBlock::from_ssz_bytes_for_fork(bytes, fork_name) + }) + } + /// SSZ decode which attempts to decode all variants (slow). pub fn any_from_ssz_bytes(bytes: &[u8]) -> Result { Self::from_ssz_bytes_with(bytes, BeaconBlock::any_from_ssz_bytes)