From ea4fcef11fdbc9c52a9f41f3f82f1336830fc396 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Wed, 20 Mar 2024 10:52:42 +0900 Subject: [PATCH] Resolve more todos --- .../src/data_availability_checker.rs | 57 ------------- .../overflow_lru_cache.rs | 41 +--------- .../src/peer_manager/peerdb/peer_info.rs | 1 - .../src/network_beacon_processor/mod.rs | 10 --- .../network/src/sync/block_lookups/mod.rs | 6 +- .../src/sync/block_lookups/parent_lookup.rs | 1 - .../sync/block_lookups/single_block_lookup.rs | 80 +++---------------- .../network/src/sync/network_context.rs | 16 ---- consensus/types/src/data_column_subnet_id.rs | 2 +- 9 files changed, 17 insertions(+), 197 deletions(-) diff --git a/beacon_node/beacon_chain/src/data_availability_checker.rs b/beacon_node/beacon_chain/src/data_availability_checker.rs index 0fc4c39a3f2..d3d3b2b4592 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker.rs @@ -211,63 +211,6 @@ impl DataAvailabilityChecker { } } - pub fn get_missing_data_column_ids( - &self, - block_root: Hash256, - block: &Option>>, - ) -> MissingDataColumns { - let Some(current_slot) = self.slot_clock.now_or_genesis() else { - error!( - self.log, - "Failed to read slot clock when checking for missing blob ids" - ); - return MissingDataColumns::NotRequired; - }; - - if !self.da_check_required_for_epoch(current_slot.epoch(T::EthSpec::slots_per_epoch())) { - return MissingDataColumns::NotRequired; - } - - // Compute ids for sampling - let mut sampling_ids = self - .compute_sampling_column_ids(block_root) - .into_iter() - .map(|index| BlobIdentifier { - block_root, - index: index as u64, - }) - .collect::>(); - - // Compute ids for custody - match block { - Some(cached_block) => { - sampling_ids.extend_from_slice( - &self - .compute_custody_column_ids(cached_block.slot()) - .into_iter() - .map(|index| BlobIdentifier { - block_root, - index: index as u64, - }) - .collect::>(), - ); - MissingDataColumns::KnownMissing(sampling_ids) - } - // TODO(das): without knowledge of the block's slot you can't know your custody - // requirements. When syncing a block via single block lookup, you need to fetch the - // block first, then fetch your custody columns - None => MissingDataColumns::KnownMissingIncomplete(sampling_ids), - } - } - - fn compute_sampling_column_ids(&self, block_root: Hash256) -> Vec { - todo!("Use local randomness to derive this block's sample column ids") - } - - fn compute_custody_column_ids(&self, slot: Slot) -> Vec { - todo!("Use local node ID and custody parameter to compute column ids for this slot"); - } - /// Get a blob from the availability cache. pub fn get_blob( &self, diff --git a/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs b/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs index 4b17403c870..e974b0b709b 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs @@ -28,7 +28,7 @@ //! the cache when they are accessed. use super::state_lru_cache::{DietAvailabilityPendingExecutedBlock, StateLRUCache}; -use super::{CustodyConfig, NodeIdRaw}; +use super::CustodyConfig; use crate::beacon_chain::BeaconStore; use crate::blob_verification::KzgVerifiedBlob; use crate::block_verification_types::{ @@ -47,7 +47,7 @@ use std::num::NonZeroUsize; use std::{collections::HashSet, sync::Arc}; use types::blob_sidecar::BlobIdentifier; use types::data_column_sidecar::{ColumnIndex, DataColumnIdentifier}; -use types::{BlobSidecar, ChainSpec, DataColumnSidecar, Epoch, EthSpec, ForkName, Hash256, Slot}; +use types::{BlobSidecar, ChainSpec, DataColumnSidecar, Epoch, EthSpec, Hash256, Slot}; /// This represents the components of a partially available block /// @@ -80,27 +80,6 @@ impl PendingComponents { .map(|b| b.as_block().message().slot()) } - /// Checks if a block exists in the cache. - /// - /// Returns: - /// - `true` if a block exists. - /// - `false` otherwise. - fn block_exists(&self) -> bool { - self.executed_block.is_some() - } - - /// Checks if a blob exists at the given index in the cache. - /// - /// Returns: - /// - `true` if a blob exists at the given index. - /// - `false` otherwise. - fn blob_exists(&self, blob_index: usize) -> bool { - self.verified_blobs - .get(blob_index) - .map(|b| b.is_some()) - .unwrap_or(false) - } - fn data_column_exists(&self, data_colum_index: ColumnIndex) -> bool { self.verified_data_columns .iter() @@ -108,22 +87,6 @@ impl PendingComponents { .is_some() } - /// Returns the number of blobs that are expected to be present. Returns `None` if we don't have a - /// block. - /// - /// This corresponds to the number of commitments that are present in a block. - fn num_expected_blobs(&self) -> Option { - self.executed_block.as_ref().map(|b| { - b.as_block() - .message() - .body() - .blob_kzg_commitments() - .cloned() - .unwrap_or_default() - .len() - }) - } - /// Returns the number of blobs that have been received and are stored in the cache. fn num_received_blobs(&self) -> usize { self.verified_blobs.iter().flatten().count() diff --git a/beacon_node/lighthouse_network/src/peer_manager/peerdb/peer_info.rs b/beacon_node/lighthouse_network/src/peer_manager/peerdb/peer_info.rs index c808c01498b..62b8820f5fb 100644 --- a/beacon_node/lighthouse_network/src/peer_manager/peerdb/peer_info.rs +++ b/beacon_node/lighthouse_network/src/peer_manager/peerdb/peer_info.rs @@ -3,7 +3,6 @@ use super::score::{PeerAction, Score, ScoreState}; use super::sync_status::SyncStatus; use crate::discovery::Eth2Enr; use crate::{rpc::MetaData, types::Subnet}; -use discv5::enr::NodeId; use discv5::Enr; use libp2p::core::multiaddr::{Multiaddr, Protocol}; use serde::{ diff --git a/beacon_node/network/src/network_beacon_processor/mod.rs b/beacon_node/network/src/network_beacon_processor/mod.rs index dea3d57ac5a..03e6a46d1cd 100644 --- a/beacon_node/network/src/network_beacon_processor/mod.rs +++ b/beacon_node/network/src/network_beacon_processor/mod.rs @@ -502,16 +502,6 @@ impl NetworkBeaconProcessor { }) } - pub fn sampling_completed(self: &Arc, block_root: Hash256) { - todo!("send work event to process sampling result"); - - // Sync handles these results - // self.send_sync_message(SyncMessage::SampleProcessed { - // id, - // result: result.into(), - // }); - } - /// Create a new work event to import `blocks` as a beacon chain segment. pub fn send_chain_segment( self: &Arc, diff --git a/beacon_node/network/src/sync/block_lookups/mod.rs b/beacon_node/network/src/sync/block_lookups/mod.rs index f1e255e8b78..dfa9582f509 100644 --- a/beacon_node/network/src/sync/block_lookups/mod.rs +++ b/beacon_node/network/src/sync/block_lookups/mod.rs @@ -11,12 +11,11 @@ pub use crate::sync::block_lookups::single_block_lookup::{ CachedChild, ColumnRequestState, LookupRequestError, }; use crate::sync::manager::{Id, SingleLookupReqId}; -use crate::sync::network_context::PeersByCustody; use beacon_chain::block_verification_types::{AsBlock, RpcBlock}; pub use beacon_chain::data_availability_checker::ChildComponents; use beacon_chain::data_availability_checker::{ compute_custody_requirements, AvailabilityCheckErrorCategory, CustodyConfig, - DataAvailabilityChecker, NodeIdRaw, + DataAvailabilityChecker, }; use beacon_chain::validator_monitor::timestamp_now; use beacon_chain::{AvailabilityProcessingStatus, BeaconChainTypes, BlockError}; @@ -36,7 +35,7 @@ use std::sync::Arc; use std::time::Duration; use store::Hash256; use types::blob_sidecar::FixedBlobSidecarList; -use types::{DataColumnSidecar, EthSpec, Slot}; +use types::{DataColumnSidecar, Slot}; pub mod common; mod parent_lookup; @@ -215,7 +214,6 @@ impl BlockLookups { child_components, peers, self.da_checker.clone(), - PeersByCustody::new(), cx.next_id(), ); 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 746c7d436eb..997ec1ace56 100644 --- a/beacon_node/network/src/sync/block_lookups/parent_lookup.rs +++ b/beacon_node/network/src/sync/block_lookups/parent_lookup.rs @@ -72,7 +72,6 @@ impl ParentLookup { Some(ChildComponents::empty(block_root)), &[peer_id], da_checker, - cx.peers_by_custody(), cx.next_id(), ); 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 8428d714d58..c6aeea88e35 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 @@ -1,7 +1,7 @@ use super::PeerId; use crate::sync::block_lookups::common::{Lookup, RequestState}; use crate::sync::block_lookups::Id; -use crate::sync::network_context::{PeersByCustody, SyncNetworkContext}; +use crate::sync::network_context::SyncNetworkContext; use beacon_chain::block_verification_types::RpcBlock; use beacon_chain::data_availability_checker::{ compute_custody_requirements, compute_sample_requirements, AvailabilityCheckError, @@ -70,30 +70,9 @@ impl SingleBlockLookup { child_components: Option>, peers: &[PeerId], da_checker: Arc>, - peer_by_custody: PeersByCustody, id: Id, ) -> Self { let is_deneb = da_checker.is_deneb(); - let missing_column_ids = missing_data_column_ids( - requested_block_root, - child_components.as_ref(), - da_checker.clone(), - ); - - let columns_request_state = missing_column_ids - .indices() - .iter() - .map(|index| { - ColumnRequestState::::new( - DataColumnIdentifier { - block_root: requested_block_root, - index: *index, - }, - // Initially no peers slot unknown - &vec![], - ) - }) - .collect::>(); Self { id, @@ -227,10 +206,16 @@ impl SingleBlockLookup { return CachedChild::DownloadIncomplete; }; - if !self.missing_blob_ids().is_empty() || !self.missing_data_column_ids().is_empty() { + if !self.missing_blob_ids().is_empty() { return CachedChild::DownloadIncomplete; } + // TODO(das): consider column custody and sampling. I don't want to duplicate logic, and + // it's odd that child component download has different paths than regular lookups. I + // feels like this can be simplified by unifying both paths. The whole child block + // business is regular download that should be delayed for processing until latter when + // all parents are known. + match RpcBlock::new_from_fixed( self.block_request_state.requested_block_root, block.clone(), @@ -380,11 +365,6 @@ impl SingleBlockLookup { self.blob_request_state.requested_ids = self.missing_blob_ids(); } - pub(crate) fn columns_already_downloaded(&mut self) -> bool { - // TODO(das) request_ids is not actually used now - todo!("deduplicate requests properly"); - } - /// If `child_components` is `Some`, we know block components won't hit the data /// availability cache, so we don't check its processing cache unless `child_components` /// is `None`. @@ -410,14 +390,6 @@ impl SingleBlockLookup { } } - pub(crate) fn missing_data_column_ids(&self) -> MissingDataColumns { - missing_data_column_ids( - self.block_root(), - self.child_components.as_ref(), - self.da_checker.clone(), - ) - } - /// 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() { @@ -459,19 +431,6 @@ pub enum ColumnsRequestState { }, } -fn missing_data_column_ids( - block_root: Hash256, - child_components: Option<&ChildComponents>, - da_checker: Arc>, -) -> MissingDataColumns { - if let Some(components) = child_components { - da_checker.get_missing_data_column_ids(block_root, &components.downloaded_block) - } else { - // TODO(das): same code as for missing_blob_ids? - todo!() - } -} - /// The state of the blob request component of a `SingleBlockLookup`. pub struct BlobRequestState { /// The latest picture of which blobs still need to be requested. This includes information @@ -759,15 +718,8 @@ mod tests { HotColdDB::open_ephemeral(StoreConfig::default(), ChainSpec::minimal(), log.clone()) .expect("store"); let da_checker = Arc::new( - DataAvailabilityChecker::new( - slot_clock, - None, - store.into(), - &log, - spec.clone(), - NODE_ID, - ) - .expect("data availability checker"), + DataAvailabilityChecker::new(slot_clock, None, store.into(), &log, spec.clone()) + .expect("data availability checker"), ); let mut sl = SingleBlockLookup::::new( block.canonical_root(), @@ -809,15 +761,8 @@ mod tests { .expect("store"); let da_checker = Arc::new( - DataAvailabilityChecker::new( - slot_clock, - None, - store.into(), - &log, - spec.clone(), - NODE_ID, - ) - .expect("data availability checker"), + DataAvailabilityChecker::new(slot_clock, None, store.into(), &log, spec.clone()) + .expect("data availability checker"), ); let mut sl = SingleBlockLookup::::new( @@ -825,7 +770,6 @@ mod tests { None, &[peer_id], da_checker, - PeersByCustody::new(), 1, ); for _ in 1..TestLookup2::MAX_ATTEMPTS { diff --git a/beacon_node/network/src/sync/network_context.rs b/beacon_node/network/src/sync/network_context.rs index a794bd43ce2..8742543d4e9 100644 --- a/beacon_node/network/src/sync/network_context.rs +++ b/beacon_node/network/src/sync/network_context.rs @@ -91,18 +91,6 @@ impl From>>> for BlockOrBlob { } } -pub struct PeersByCustody {} - -impl PeersByCustody { - pub fn new() -> Self { - Self {} - } - - pub fn custodial_peers(&self, slot: Slot, column_id: u64) -> Vec { - todo!(); - } -} - fn custody_config_from_peer_info(peer_info: &PeerInfo) -> Option { peer_info .node_id() @@ -134,10 +122,6 @@ impl SyncNetworkContext { &self.network_beacon_processor.network_globals } - pub fn peers_by_custody(&self) -> PeersByCustody { - todo!(); - } - pub fn get_custody_config_of_peer(&self, peer_id: &PeerId) -> Option { self.network_globals() .peers diff --git a/consensus/types/src/data_column_subnet_id.rs b/consensus/types/src/data_column_subnet_id.rs index fa343618b8f..ea95d7b9109 100644 --- a/consensus/types/src/data_column_subnet_id.rs +++ b/consensus/types/src/data_column_subnet_id.rs @@ -1,5 +1,5 @@ //! Identifies each data column subnet by an integer identifier. -use crate::{ChainSpec, EthSpec}; +use crate::EthSpec; use ethereum_types::U256; use safe_arith::{ArithError, SafeArith}; use serde::{Deserialize, Serialize};