From 8c914c92b2197481952dfb9e2d9b64fbf439449c Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Mon, 18 Mar 2024 01:23:04 +0900 Subject: [PATCH 1/3] Remove DataAvailabilityView trait from ChildComponents --- .../src/data_availability_checker.rs | 35 +++++-------------- .../availability_view.rs | 33 ----------------- .../child_components.rs | 19 ++++++++-- .../network/src/sync/block_lookups/common.rs | 32 ++++++++++------- .../network/src/sync/block_lookups/mod.rs | 2 ++ .../src/sync/block_lookups/parent_lookup.rs | 4 +++ .../sync/block_lookups/single_block_lookup.rs | 24 ++++++++----- 7 files changed, 66 insertions(+), 83 deletions(-) diff --git a/beacon_node/beacon_chain/src/data_availability_checker.rs b/beacon_node/beacon_chain/src/data_availability_checker.rs index f906032ecd2..d7e7ad821a6 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker.rs @@ -15,6 +15,7 @@ pub use processing_cache::ProcessingComponents; use slasher::test_utils::E; use slog::{debug, error, Logger}; use slot_clock::SlotClock; +use ssz_types::FixedVector; use std::fmt; use std::fmt::Debug; use std::num::NonZeroUsize; @@ -114,10 +115,11 @@ impl DataAvailabilityChecker { /// /// If there's no block, all possible ids will be returned that don't exist in the given blobs. /// If there no blobs, all possible ids will be returned. - pub fn get_missing_blob_ids>( + pub fn get_missing_blob_ids( &self, block_root: Hash256, - availability_view: &V, + block: &Option>>, + blobs: &FixedVector, ::MaxBlobsPerBlock>, ) -> MissingBlobs { let Some(current_slot) = self.slot_clock.now_or_genesis() else { error!( @@ -130,10 +132,9 @@ impl DataAvailabilityChecker { let current_epoch = current_slot.epoch(T::EthSpec::slots_per_epoch()); if self.da_check_required_for_epoch(current_epoch) { - match availability_view.get_cached_block() { + match block { Some(cached_block) => { let block_commitments = cached_block.get_commitments(); - let blob_commitments = availability_view.get_cached_blobs(); let num_blobs_expected = block_commitments.len(); let mut blob_ids = Vec::with_capacity(num_blobs_expected); @@ -141,37 +142,17 @@ impl DataAvailabilityChecker { // Zip here will always limit the number of iterations to the size of // `block_commitment` because `blob_commitments` will always be populated // with `Option` values up to `MAX_BLOBS_PER_BLOCK`. - for (index, (block_commitment, blob_commitment_opt)) in block_commitments - .into_iter() - .zip(blob_commitments.iter()) - .enumerate() + for (index, (_, blob_commitment_opt)) in + block_commitments.into_iter().zip(blobs.iter()).enumerate() { // Always add a missing blob. - let Some(blob_commitment) = blob_commitment_opt else { + if blob_commitment_opt.is_none() { blob_ids.push(BlobIdentifier { block_root, index: index as u64, }); continue; }; - - let blob_commitment = *blob_commitment.get_commitment(); - - // Check for consistency, but this shouldn't happen, an availability view - // should guaruntee consistency. - if blob_commitment != block_commitment { - error!(self.log, - "Inconsistent availability view"; - "block_root" => ?block_root, - "block_commitment" => ?block_commitment, - "blob_commitment" => ?blob_commitment, - "index" => index - ); - blob_ids.push(BlobIdentifier { - block_root, - index: index as u64, - }); - } } MissingBlobs::KnownMissing(blob_ids) } diff --git a/beacon_node/beacon_chain/src/data_availability_checker/availability_view.rs b/beacon_node/beacon_chain/src/data_availability_checker/availability_view.rs index 65093db26bd..9111a8b0707 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker/availability_view.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker/availability_view.rs @@ -1,4 +1,3 @@ -use super::child_components::ChildComponents; use super::state_lru_cache::DietAvailabilityPendingExecutedBlock; use crate::blob_verification::KzgVerifiedBlob; use crate::block_verification_types::AsBlock; @@ -196,14 +195,6 @@ impl_availability_view!( verified_blobs ); -impl_availability_view!( - ChildComponents, - Arc>, - Arc>, - downloaded_block, - downloaded_blobs -); - pub trait GetCommitments { fn get_commitments(&self) -> KzgCommitments; } @@ -382,23 +373,6 @@ pub mod tests { (block.into(), blobs, invalid_blobs) } - type ChildComponentsSetup = ( - Arc>, - FixedVector>>, ::MaxBlobsPerBlock>, - FixedVector>>, ::MaxBlobsPerBlock>, - ); - - pub fn setup_child_components( - block: SignedBeaconBlock, - valid_blobs: FixedVector>>, ::MaxBlobsPerBlock>, - invalid_blobs: FixedVector>>, ::MaxBlobsPerBlock>, - ) -> ChildComponentsSetup { - let blobs = FixedVector::from(valid_blobs.into_iter().cloned().collect::>()); - let invalid_blobs = - FixedVector::from(invalid_blobs.into_iter().cloned().collect::>()); - (Arc::new(block), blobs, invalid_blobs) - } - pub fn assert_cache_consistent>(cache: V) { if let Some(cached_block) = cache.get_cached_block() { let cached_block_commitments = cached_block.get_commitments(); @@ -531,11 +505,4 @@ pub mod tests { verified_blobs, setup_pending_components ); - generate_tests!( - child_component_tests, - ChildComponents::, - downloaded_block, - downloaded_blobs, - setup_child_components - ); } diff --git a/beacon_node/beacon_chain/src/data_availability_checker/child_components.rs b/beacon_node/beacon_chain/src/data_availability_checker/child_components.rs index 028bf9d67c8..184dfc45001 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker/child_components.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker/child_components.rs @@ -1,9 +1,8 @@ use crate::block_verification_types::RpcBlock; -use crate::data_availability_checker::AvailabilityView; use bls::Hash256; use std::sync::Arc; use types::blob_sidecar::FixedBlobSidecarList; -use types::{EthSpec, SignedBeaconBlock}; +use types::{BlobSidecar, EthSpec, SignedBeaconBlock}; /// For requests triggered by an `UnknownBlockParent` or `UnknownBlobParent`, this struct /// is used to cache components as they are sent to the network service. We can't use the @@ -48,6 +47,22 @@ impl ChildComponents { cache } + pub fn merge_block(&mut self, block: Arc>) { + self.downloaded_block = Some(block); + } + + pub fn merge_blob(&mut self, blob: Arc>) { + if let Some(blob_ref) = self.downloaded_blobs.get_mut(blob.index as usize) { + *blob_ref = Some(blob); + } + } + + pub fn merge_blobs(&mut self, blobs: FixedBlobSidecarList) { + for blob in blobs.iter().flatten() { + self.merge_blob(blob.clone()); + } + } + pub fn clear_blobs(&mut self) { self.downloaded_blobs = FixedBlobSidecarList::default(); } diff --git a/beacon_node/network/src/sync/block_lookups/common.rs b/beacon_node/network/src/sync/block_lookups/common.rs index d989fbb3362..7844612624b 100644 --- a/beacon_node/network/src/sync/block_lookups/common.rs +++ b/beacon_node/network/src/sync/block_lookups/common.rs @@ -8,7 +8,7 @@ use crate::sync::block_lookups::{ use crate::sync::manager::{BlockProcessType, Id, SingleLookupReqId}; use crate::sync::network_context::SyncNetworkContext; use beacon_chain::block_verification_types::RpcBlock; -use beacon_chain::data_availability_checker::{AvailabilityView, ChildComponents}; +use beacon_chain::data_availability_checker::ChildComponents; use beacon_chain::{get_block_root, BeaconChainTypes}; use lighthouse_network::rpc::methods::BlobsByRootRequest; use lighthouse_network::rpc::BlocksByRootRequest; @@ -371,7 +371,7 @@ impl RequestState for BlobRequestState, peer_id: PeerId, ) -> Result>, LookupVerifyError> { @@ -380,18 +380,24 @@ impl RequestState for BlobRequestState= T::EthSpec::max_blobs_per_block() as u64 { - return Err(LookupVerifyError::InvalidIndex(blob.index)); - } - *self.blob_download_queue.index_mut(blob_index as usize) = Some(blob); - Ok(None) + return Err(LookupVerifyError::UnrequestedBlobId); + } + + blob.verify_blob_sidecar_inclusion_proof() + .map_err(|_| LookupVerifyError::InvalidInclusionProof)?; + if blob.block_root() != expected_block_root { + return Err(LookupVerifyError::UnrequestedHeader); + } + + // State should remain downloading until we receive the stream terminator. + self.requested_ids.remove(&received_id); + let blob_index = blob.index; + + if blob_index >= T::EthSpec::max_blobs_per_block() as u64 { + return Err(LookupVerifyError::InvalidIndex(blob.index)); } + *self.blob_download_queue.index_mut(blob_index as usize) = Some(blob); + Ok(None) } None => { self.state.state = State::Processing { peer_id }; diff --git a/beacon_node/network/src/sync/block_lookups/mod.rs b/beacon_node/network/src/sync/block_lookups/mod.rs index 62cdc4fa223..f0c2a35e828 100644 --- a/beacon_node/network/src/sync/block_lookups/mod.rs +++ b/beacon_node/network/src/sync/block_lookups/mod.rs @@ -541,6 +541,8 @@ impl BlockLookups { | ParentVerifyError::NotEnoughBlobsReturned | ParentVerifyError::ExtraBlocksReturned | ParentVerifyError::UnrequestedBlobId + | ParentVerifyError::InvalidInclusionProof + | ParentVerifyError::UnrequestedHeader | ParentVerifyError::ExtraBlobsReturned | ParentVerifyError::InvalidIndex(_) => { let e = e.into(); diff --git a/beacon_node/network/src/sync/block_lookups/parent_lookup.rs b/beacon_node/network/src/sync/block_lookups/parent_lookup.rs index 5c2e90b48c9..5608b418665 100644 --- a/beacon_node/network/src/sync/block_lookups/parent_lookup.rs +++ b/beacon_node/network/src/sync/block_lookups/parent_lookup.rs @@ -37,6 +37,8 @@ pub enum ParentVerifyError { NotEnoughBlobsReturned, ExtraBlocksReturned, UnrequestedBlobId, + InvalidInclusionProof, + UnrequestedHeader, ExtraBlobsReturned, InvalidIndex(u64), PreviousFailure { parent_root: Hash256 }, @@ -243,6 +245,8 @@ impl From for ParentVerifyError { E::NoBlockReturned => ParentVerifyError::NoBlockReturned, E::ExtraBlocksReturned => ParentVerifyError::ExtraBlocksReturned, E::UnrequestedBlobId => ParentVerifyError::UnrequestedBlobId, + E::InvalidInclusionProof => ParentVerifyError::InvalidInclusionProof, + E::UnrequestedHeader => ParentVerifyError::UnrequestedHeader, E::ExtraBlobsReturned => ParentVerifyError::ExtraBlobsReturned, E::InvalidIndex(index) => ParentVerifyError::InvalidIndex(index), E::NotEnoughBlobsReturned => ParentVerifyError::NotEnoughBlobsReturned, diff --git a/beacon_node/network/src/sync/block_lookups/single_block_lookup.rs b/beacon_node/network/src/sync/block_lookups/single_block_lookup.rs index 8c60621f1c7..c96c35ef300 100644 --- a/beacon_node/network/src/sync/block_lookups/single_block_lookup.rs +++ b/beacon_node/network/src/sync/block_lookups/single_block_lookup.rs @@ -3,10 +3,10 @@ use crate::sync::block_lookups::common::{Lookup, RequestState}; use crate::sync::block_lookups::Id; use crate::sync::network_context::SyncNetworkContext; use beacon_chain::block_verification_types::RpcBlock; +use beacon_chain::data_availability_checker::ChildComponents; use beacon_chain::data_availability_checker::{ AvailabilityCheckError, DataAvailabilityChecker, MissingBlobs, }; -use beacon_chain::data_availability_checker::{AvailabilityView, ChildComponents}; use beacon_chain::BeaconChainTypes; use lighthouse_network::PeerAction; use slog::{trace, Logger}; @@ -32,6 +32,8 @@ pub enum LookupVerifyError { NoBlockReturned, ExtraBlocksReturned, UnrequestedBlobId, + InvalidInclusionProof, + UnrequestedHeader, ExtraBlobsReturned, NotEnoughBlobsReturned, InvalidIndex(u64), @@ -247,7 +249,7 @@ impl SingleBlockLookup { /// Returns `true` if the block has already been downloaded. pub(crate) fn block_already_downloaded(&self) -> bool { if let Some(components) = self.child_components.as_ref() { - components.block_exists() + components.downloaded_block.is_some() } else { self.da_checker.has_block(&self.block_root()) } @@ -272,19 +274,25 @@ impl SingleBlockLookup { pub(crate) fn missing_blob_ids(&self) -> MissingBlobs { let block_root = self.block_root(); if let Some(components) = self.child_components.as_ref() { - self.da_checker.get_missing_blob_ids(block_root, components) + self.da_checker.get_missing_blob_ids( + block_root, + &components.downloaded_block, + &components.downloaded_blobs, + ) } else { - let Some(processing_availability_view) = - self.da_checker.get_processing_components(block_root) + let Some(processing_components) = self.da_checker.get_processing_components(block_root) else { return MissingBlobs::new_without_block(block_root, self.da_checker.is_deneb()); }; - self.da_checker - .get_missing_blob_ids(block_root, &processing_availability_view) + self.da_checker.get_missing_blob_ids( + block_root, + &processing_components.block, + &processing_components.blob_commitments, + ) } } - /// Penalizes a blob peer if it should have blobs but didn't return them to us. + /// Penalizes a blob peer if it should have blobs but didn't return them to us. pub fn penalize_blob_peer(&mut self, cx: &SyncNetworkContext) { if let Ok(blob_peer) = self.blob_request_state.state.processing_peer() { cx.report_peer( From a6c4c09589dd3e39df3fe0b1d7717eea6f941feb Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Tue, 26 Mar 2024 18:54:32 +0900 Subject: [PATCH 2/3] PR reviews --- .../src/data_availability_checker.rs | 26 ++++++----------- .../network/src/sync/block_lookups/common.rs | 28 ++++++++++--------- 2 files changed, 24 insertions(+), 30 deletions(-) diff --git a/beacon_node/beacon_chain/src/data_availability_checker.rs b/beacon_node/beacon_chain/src/data_availability_checker.rs index d7e7ad821a6..fe2d6e48258 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker.rs @@ -135,25 +135,17 @@ impl DataAvailabilityChecker { match block { Some(cached_block) => { let block_commitments = cached_block.get_commitments(); - - let num_blobs_expected = block_commitments.len(); - let mut blob_ids = Vec::with_capacity(num_blobs_expected); - - // Zip here will always limit the number of iterations to the size of - // `block_commitment` because `blob_commitments` will always be populated - // with `Option` values up to `MAX_BLOBS_PER_BLOCK`. - for (index, (_, blob_commitment_opt)) in - block_commitments.into_iter().zip(blobs.iter()).enumerate() - { - // Always add a missing blob. - if blob_commitment_opt.is_none() { - blob_ids.push(BlobIdentifier { + let blob_ids = blobs + .iter() + .take(block_commitments.len()) + .enumerate() + .filter_map(|(index, blob_commitment_opt)| { + blob_commitment_opt.is_none().then_some(BlobIdentifier { block_root, index: index as u64, - }); - continue; - }; - } + }) + }) + .collect(); MissingBlobs::KnownMissing(blob_ids) } None => { diff --git a/beacon_node/network/src/sync/block_lookups/common.rs b/beacon_node/network/src/sync/block_lookups/common.rs index 7844612624b..b7e0b7700b0 100644 --- a/beacon_node/network/src/sync/block_lookups/common.rs +++ b/beacon_node/network/src/sync/block_lookups/common.rs @@ -17,7 +17,7 @@ use std::ops::IndexMut; use std::sync::Arc; use std::time::Duration; use types::blob_sidecar::{BlobIdentifier, FixedBlobSidecarList}; -use types::{BlobSidecar, ChainSpec, EthSpec, Hash256, SignedBeaconBlock}; +use types::{BlobSidecar, ChainSpec, Hash256, SignedBeaconBlock}; #[derive(Debug, Copy, Clone)] pub enum ResponseType { @@ -378,24 +378,26 @@ impl RequestState for BlobRequestState { let received_id = blob.id(); - if !self.requested_ids.contains(&received_id) { - self.state.register_failure_downloading(); - return Err(LookupVerifyError::UnrequestedBlobId); - } - blob.verify_blob_sidecar_inclusion_proof() - .map_err(|_| LookupVerifyError::InvalidInclusionProof)?; - if blob.block_root() != expected_block_root { - return Err(LookupVerifyError::UnrequestedHeader); + if !self.requested_ids.contains(&received_id) { + Err(LookupVerifyError::UnrequestedBlobId) + } else if blob.verify_blob_sidecar_inclusion_proof().unwrap_or(false) { + Err(LookupVerifyError::InvalidInclusionProof) + } else if blob.block_root() != expected_block_root { + Err(LookupVerifyError::UnrequestedHeader) + } else { + Ok(()) } + .map_err(|e| { + self.state.register_failure_downloading(); + e + })?; // State should remain downloading until we receive the stream terminator. self.requested_ids.remove(&received_id); - let blob_index = blob.index; - if blob_index >= T::EthSpec::max_blobs_per_block() as u64 { - return Err(LookupVerifyError::InvalidIndex(blob.index)); - } + // The inclusion proof check above ensures `blob.index` is < MAX_BLOBS_PER_BLOCK + let blob_index = blob.index; *self.blob_download_queue.index_mut(blob_index as usize) = Some(blob); Ok(None) } From 22116348ac2021c3b5a1edc9a4591093de0af874 Mon Sep 17 00:00:00 2001 From: Lion - dapplion <35266934+dapplion@users.noreply.github.com> Date: Fri, 29 Mar 2024 11:21:00 +0900 Subject: [PATCH 3/3] Update beacon_node/network/src/sync/block_lookups/common.rs Co-authored-by: realbigsean --- beacon_node/network/src/sync/block_lookups/common.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beacon_node/network/src/sync/block_lookups/common.rs b/beacon_node/network/src/sync/block_lookups/common.rs index b7e0b7700b0..850c5ecfc64 100644 --- a/beacon_node/network/src/sync/block_lookups/common.rs +++ b/beacon_node/network/src/sync/block_lookups/common.rs @@ -381,7 +381,7 @@ impl RequestState for BlobRequestState