diff --git a/Cargo.lock b/Cargo.lock index d4a531d26d..5d200768cd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3600,6 +3600,7 @@ version = "0.1.0" dependencies = [ "beacon_chain", "bls", + "criterion", "ethereum_ssz", "ethereum_ssz_derive", "fixed_bytes", diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 231865b1ca..0d8d2560de 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -16,7 +16,7 @@ use crate::block_verification_types::{ }; pub use crate::canonical_head::CanonicalHead; use crate::chain_config::ChainConfig; -use crate::custody_context::CustodyContextSsz; +use crate::custody_context::{CustodyContext, CustodyContextSsz}; use crate::data_availability_checker::{ Availability as BlockAvailability, AvailabilityCheckError, AvailableBlock, AvailableBlockData, DataColumnReconstructionResult as DataColumnReconstructionResultV1, @@ -500,6 +500,8 @@ pub struct BeaconChain { pub validator_monitor: RwLock>, /// The slot at which blocks are downloaded back to. pub genesis_backfill_slot: Slot, + /// Contains all the information the node requires to calculate which columns to custody + pub custody_context: Arc>, /// Provides KZG verification and temporary storage for pre-Gloas blocks and blobs. pub data_availability_checker: Arc>, /// Provides KZG verification and temporary storage for post-Gloas payload envelopes. @@ -682,11 +684,7 @@ impl BeaconChain { return Ok(()); } - let custody_context: CustodyContextSsz = self - .data_availability_checker - .custody_context() - .as_ref() - .into(); + let custody_context: CustodyContextSsz = self.custody_context.as_ref().into(); // Pattern match to avoid accidentally missing fields and to ignore deprecated fields. let CustodyContextSsz { @@ -3318,8 +3316,9 @@ impl BeaconChain { ); // Check if we have custody of this column - let sampling_columns = - self.sampling_columns_for_epoch(slot.epoch(T::EthSpec::slots_per_epoch())); + let sampling_columns = self + .custody_context + .sampling_columns_for_epoch(slot.epoch(T::EthSpec::slots_per_epoch())); let verified_partial = if sampling_columns.contains(&partial.index) { KzgVerifiedCustodyPartialDataColumn::from_asserted_custody(verified_partial) } else { @@ -3981,7 +3980,7 @@ impl BeaconChain { )?; let availability = self .data_availability_checker - .put_rpc_blobs(block_root, blobs) + .put_rpc_blobs(block_root, blobs, &self.slot_clock) .map_err(BlockError::from)?; self.process_availability(slot, availability, || Ok(())) @@ -7180,25 +7179,6 @@ impl BeaconChain { }) } - /// The data availability boundary for custodying columns. It will just be the - /// regular data availability boundary unless we are near the Fulu fork epoch. - pub fn column_data_availability_boundary(&self) -> Option { - match self.data_availability_boundary() { - Some(da_boundary_epoch) => { - if let Some(fulu_fork_epoch) = self.spec.fulu_fork_epoch { - if da_boundary_epoch < fulu_fork_epoch { - Some(fulu_fork_epoch) - } else { - Some(da_boundary_epoch) - } - } else { - None // Fulu hasn't been enabled - } - } - None => None, // Deneb hasn't been enabled - } - } - /// Safely update data column custody info by ensuring that: /// - cgc values at the updated epoch and the earliest custodied column epoch are equal /// - we are only decrementing the earliest custodied data column epoch by one epoch @@ -7216,14 +7196,12 @@ impl BeaconChain { } let cgc_at_effective_epoch = self - .data_availability_checker - .custody_context() - .custody_group_count_at_epoch(effective_epoch, &self.spec); + .custody_context + .custody_group_count_at_epoch(effective_epoch); let cgc_at_earliest_data_colum_epoch = self - .data_availability_checker - .custody_context() - .custody_group_count_at_epoch(earliest_data_column_epoch, &self.spec); + .custody_context + .custody_group_count_at_epoch(earliest_data_column_epoch); let can_update_data_column_custody_info = cgc_at_effective_epoch == cgc_at_earliest_data_colum_epoch @@ -7250,16 +7228,16 @@ impl BeaconChain { /// Compare columns custodied for `epoch` versus columns custodied for the head of the chain /// and return any column indices that are missing. pub fn get_missing_columns_for_epoch(&self, epoch: Epoch) -> HashSet { - let custody_context = self.data_availability_checker.custody_context(); - - let columns_required = custody_context - .custody_columns_for_epoch(None, &self.spec) + let columns_required = self + .custody_context + .custody_columns_for_epoch(None) .iter() .cloned() .collect::>(); - let current_columns_at_epoch = custody_context - .custody_columns_for_epoch(Some(epoch), &self.spec) + let current_columns_at_epoch = self + .custody_context + .custody_columns_for_epoch(Some(epoch)) .iter() .cloned() .collect::>(); @@ -7270,24 +7248,6 @@ impl BeaconChain { .collect::>() } - /// The da boundary for custodying columns. It will just be the DA boundary unless we are near the Fulu fork epoch. - pub fn get_column_da_boundary(&self) -> Option { - match self.data_availability_boundary() { - Some(da_boundary_epoch) => { - if let Some(fulu_fork_epoch) = self.spec.fulu_fork_epoch { - if da_boundary_epoch < fulu_fork_epoch { - Some(fulu_fork_epoch) - } else { - Some(da_boundary_epoch) - } - } else { - None - } - } - None => None, // If no DA boundary set, dont try to custody backfill - } - } - /// This method serves to get a sense of the current chain health. It is used in block proposal /// to determine whether we should outsource payload production duties. /// @@ -7490,30 +7450,6 @@ impl BeaconChain { gossip_attested || block_attested || aggregated || produced_block } - /// The epoch at which we require a data availability check in block processing. - /// `None` if the `Deneb` fork is disabled. - pub fn data_availability_boundary(&self) -> Option { - self.data_availability_checker.data_availability_boundary() - } - - /// Returns true if epoch is within the data availability boundary - pub fn da_check_required_for_epoch(&self, epoch: Epoch) -> bool { - self.data_availability_checker - .da_check_required_for_epoch(epoch) - } - - /// Returns true if we should fetch blobs for this block - pub fn should_fetch_blobs(&self, block_epoch: Epoch) -> bool { - self.da_check_required_for_epoch(block_epoch) - && !self.spec.is_peer_das_enabled_for_epoch(block_epoch) - } - - /// Returns true if we should fetch custody columns for this block - pub fn should_fetch_custody_columns(&self, block_epoch: Epoch) -> bool { - self.da_check_required_for_epoch(block_epoch) - && self.spec.is_peer_das_enabled_for_epoch(block_epoch) - } - /// Gets the `LightClientBootstrap` object for a requested block root. /// /// Returns `None` when the state or block is not found in the database. @@ -7552,7 +7488,7 @@ impl BeaconChain { Some(StoreOp::PutBlobs(block_root, blobs)) } AvailableBlockData::DataColumns(mut data_columns) => { - let columns_to_custody = self.custody_columns_for_epoch(Some( + let columns_to_custody = self.custody_context.custody_columns_for_epoch(Some( block_slot.epoch(T::EthSpec::slots_per_epoch()), )); // Supernodes need to persist all sampled custody columns @@ -7598,25 +7534,6 @@ impl BeaconChain { roots.reverse(); roots } - - /// Returns a list of column indices that should be sampled for a given epoch. - /// Used for data availability sampling in PeerDAS. - pub fn sampling_columns_for_epoch(&self, epoch: Epoch) -> &[ColumnIndex] { - self.data_availability_checker - .custody_context() - .sampling_columns_for_epoch(epoch, &self.spec) - } - - /// Returns a list of column indices that the node is expected to custody for a given epoch. - /// i.e. the node must have validated and persisted the column samples and should be able to - /// serve them to peers. - /// - /// If epoch is `None`, this function computes the custody columns at head. - pub fn custody_columns_for_epoch(&self, epoch_opt: Option) -> &[ColumnIndex] { - self.data_availability_checker - .custody_context() - .custody_columns_for_epoch(epoch_opt, &self.spec) - } } impl Drop for BeaconChain { diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 6b1ac3b033..0de9a5cdb1 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -1246,8 +1246,7 @@ impl SignatureVerifiedBlock { AvailableBlock::new( block, AvailableBlockData::NoData, - &chain.data_availability_checker, - chain.spec.clone(), + &chain.custody_context, ) .map_err(BlockError::AvailabilityCheck)?, ) diff --git a/beacon_node/beacon_chain/src/block_verification_types.rs b/beacon_node/beacon_chain/src/block_verification_types.rs index 18e95f58f3..51feb12a69 100644 --- a/beacon_node/beacon_chain/src/block_verification_types.rs +++ b/beacon_node/beacon_chain/src/block_verification_types.rs @@ -1,18 +1,18 @@ -use crate::data_availability_checker::{AvailabilityCheckError, DataAvailabilityChecker}; +use crate::data_availability_checker::AvailabilityCheckError; pub use crate::data_availability_checker::{ AvailableBlock, AvailableBlockData, MaybeAvailableBlock, }; use crate::payload_envelope_verification::AvailableEnvelope; use crate::payload_envelope_verification::gossip_verified_envelope::verify_envelope_consistency; -use crate::{BeaconChainTypes, PayloadVerificationOutcome}; +use crate::{BeaconChainTypes, CustodyContext, PayloadVerificationOutcome}; use state_processing::ConsensusContext; use std::fmt::{Debug, Formatter}; use std::hash::{Hash, Hasher}; use std::sync::Arc; use types::data::BlobIdentifier; use types::{ - BeaconBlockRef, BeaconState, BlindedPayload, ChainSpec, Epoch, EthSpec, Hash256, - SignedBeaconBlock, SignedBeaconBlockHeader, Slot, + BeaconBlockRef, BeaconState, BlindedPayload, Epoch, EthSpec, Hash256, SignedBeaconBlock, + SignedBeaconBlockHeader, Slot, }; /// A wrapper around a `SignedBeaconBlock`. This varaint is constructed @@ -118,8 +118,7 @@ impl RangeSyncBlock { pub fn new( block: Arc>, block_data: AvailableBlockData, - da_checker: &DataAvailabilityChecker, - spec: Arc, + custody_context: &CustodyContext, ) -> Result where T: BeaconChainTypes, @@ -127,7 +126,7 @@ impl RangeSyncBlock { if block.fork_name_unchecked().gloas_enabled() { return Err(AvailabilityCheckError::InvalidVariant); } - let available_block = AvailableBlock::new(block, block_data, da_checker, spec)?; + let available_block = AvailableBlock::new(block, block_data, custody_context)?; Ok(Self::Base(available_block)) } diff --git a/beacon_node/beacon_chain/src/builder.rs b/beacon_node/beacon_chain/src/builder.rs index c35167ea53..0ddc0a50f2 100644 --- a/beacon_node/beacon_chain/src/builder.rs +++ b/beacon_node/beacon_chain/src/builder.rs @@ -974,14 +974,18 @@ where self.node_custody_type, head_epoch, ordered_custody_column_indices, - &self.spec, + slot_clock.clone(), + complete_blob_backfill, + self.spec.clone(), ) } else { ( CustodyContext::new( self.node_custody_type, ordered_custody_column_indices, - &self.spec, + slot_clock.clone(), + complete_blob_backfill, + self.spec.clone(), ), None, ) @@ -1059,10 +1063,9 @@ where slasher: self.slasher.clone(), validator_monitor: RwLock::new(validator_monitor), genesis_backfill_slot, + custody_context: custody_context.clone(), data_availability_checker: Arc::new( DataAvailabilityChecker::new( - complete_blob_backfill, - slot_clock.clone(), self.kzg.clone(), custody_context.clone(), self.spec.clone(), @@ -1072,12 +1075,8 @@ where .map_err(|e| format!("Error initializing DataAvailabilityChecker: {:?}", e))?, ), pending_payload_cache: Arc::new( - PendingPayloadCache::new( - self.kzg.clone(), - custody_context.clone(), - self.spec.clone(), - ) - .map_err(|e| format!("Error initializing PendingPayloadCache: {:?}", e))?, + PendingPayloadCache::new(self.kzg.clone(), custody_context, self.spec.clone()) + .map_err(|e| format!("Error initializing PendingPayloadCache: {:?}", e))?, ), kzg: self.kzg.clone(), rng: Arc::new(Mutex::new(rng)), @@ -1148,7 +1147,9 @@ where } // Prune blobs older than the blob data availability boundary in the background. - if let Some(data_availability_boundary) = beacon_chain.data_availability_boundary() { + if let Some(data_availability_boundary) = + beacon_chain.custody_context.data_availability_boundary() + { beacon_chain .store_migrator .process_prune_blobs(data_availability_boundary); diff --git a/beacon_node/beacon_chain/src/canonical_head.rs b/beacon_node/beacon_chain/src/canonical_head.rs index 4e32008d52..e22ea64da8 100644 --- a/beacon_node/beacon_chain/src/canonical_head.rs +++ b/beacon_node/beacon_chain/src/canonical_head.rs @@ -1124,7 +1124,8 @@ impl BeaconChain { )?; // Prune blobs in the background. - if let Some(data_availability_boundary) = self.data_availability_boundary() { + if let Some(data_availability_boundary) = self.custody_context.data_availability_boundary() + { self.store_migrator .process_prune_blobs(data_availability_boundary); } diff --git a/beacon_node/beacon_chain/src/custody_context.rs b/beacon_node/beacon_chain/src/custody_context.rs index 72f62db1b4..4a1dfdbe71 100644 --- a/beacon_node/beacon_chain/src/custody_context.rs +++ b/beacon_node/beacon_chain/src/custody_context.rs @@ -1,13 +1,18 @@ +use crate::BeaconChainTypes; +use educe::Educe; use parking_lot::RwLock; use serde::{Deserialize, Serialize}; +use slot_clock::SlotClock; use ssz_derive::{Decode, Encode}; -use std::marker::PhantomData; use std::{ collections::{BTreeMap, HashMap}, + sync::Arc, sync::atomic::{AtomicU64, Ordering}, }; use tracing::{debug, warn}; -use types::{ChainSpec, ColumnIndex, Epoch, EthSpec, Slot}; +use types::{ + ChainSpec, ColumnIndex, Epoch, EthSpec, SignedBeaconBlock, SignedExecutionPayloadBid, Slot, +}; /// A delay before making the CGC change effective to the data availability checker. pub const CUSTODY_CHANGE_DA_EFFECTIVE_DELAY_SECONDS: u64 = 30; @@ -236,8 +241,9 @@ impl NodeCustodyType { /// Contains all the information the node requires to calculate the /// number of columns to be custodied when checking for DA. -#[derive(Debug)] -pub struct CustodyContext { +#[derive(Educe)] +#[educe(Debug(bound(T: BeaconChainTypes)))] +pub struct CustodyContext { /// The Number of custody groups required based on the number of validators /// that is attached to this node. /// @@ -250,10 +256,15 @@ pub struct CustodyContext { /// Stores an immutable, ordered list of all data column indices as determined by the node's NodeID /// on startup. This used to determine the node's custody columns. ordered_custody_column_indices: Vec, - _phantom_data: PhantomData, + #[educe(Debug(ignore))] + slot_clock: T::SlotClock, + /// backfill blobs and data columns beyond the data availability window. + complete_blob_backfill: bool, + #[educe(Debug(ignore))] + spec: Arc, } -impl CustodyContext { +impl CustodyContext { /// Create a new custody default custody context object when no persisted object /// exists. /// @@ -261,9 +272,11 @@ impl CustodyContext { pub fn new( node_custody_type: NodeCustodyType, ordered_custody_column_indices: Vec, - spec: &ChainSpec, + slot_clock: T::SlotClock, + complete_blob_backfill: bool, + spec: Arc, ) -> Self { - let cgc_override = node_custody_type.get_custody_count_override(spec); + let cgc_override = node_custody_type.get_custody_count_override(&spec); // If there's no override, we initialise `validator_custody_count` to 0. This has been the // existing behaviour and we maintain this for now to avoid a semantic schema change until // a later release. @@ -271,7 +284,9 @@ impl CustodyContext { validator_custody_count: AtomicU64::new(cgc_override.unwrap_or(0)), validator_registrations: RwLock::new(ValidatorRegistrations::new(cgc_override)), ordered_custody_column_indices, - _phantom_data: PhantomData, + slot_clock, + complete_blob_backfill, + spec, } } @@ -294,7 +309,9 @@ impl CustodyContext { node_custody_type: NodeCustodyType, head_epoch: Epoch, ordered_custody_column_indices: Vec, - spec: &ChainSpec, + slot_clock: T::SlotClock, + complete_blob_backfill: bool, + spec: Arc, ) -> (Self, Option) { let CustodyContextSsz { mut validator_custody_at_head, @@ -304,7 +321,7 @@ impl CustodyContext { let mut custody_count_changed = None; - if let Some(cgc_from_cli) = node_custody_type.get_custody_count_override(spec) { + if let Some(cgc_from_cli) = node_custody_type.get_custody_count_override(&spec) { debug!( ?node_custody_type, persisted_custody_count = validator_custody_at_head, @@ -360,7 +377,9 @@ impl CustodyContext { .collect(), }), ordered_custody_column_indices, - _phantom_data: PhantomData, + slot_clock, + complete_blob_backfill, + spec, }; (custody_context, custody_count_changed) @@ -376,13 +395,15 @@ impl CustodyContext { &self, validators_and_balance: ValidatorsAndBalances, current_slot: Slot, - spec: &ChainSpec, ) -> Option { let Some((effective_epoch, new_validator_custody)) = self .validator_registrations .write() - .register_validators::(validators_and_balance, current_slot, spec) - else { + .register_validators::( + validators_and_balance, + current_slot, + &self.spec, + ) else { return None; }; @@ -397,7 +418,7 @@ impl CustodyContext { self.validator_custody_count .store(new_validator_custody, Ordering::Relaxed); - let updated_cgc = self.custody_group_count_at_head(spec); + let updated_cgc = self.custody_group_count_at_head(); // Send the message to network only if there are more columns subnets to subscribe to if updated_cgc > current_cgc { debug!( @@ -407,7 +428,7 @@ impl CustodyContext { return Some(CustodyCountChanged { new_custody_group_count: updated_cgc, old_custody_group_count: current_cgc, - sampling_count: self.num_of_custody_groups_to_sample(effective_epoch, spec), + sampling_count: self.num_of_custody_groups_to_sample(effective_epoch), effective_epoch, }); } @@ -419,14 +440,14 @@ impl CustodyContext { /// This function is used to determine the custody group count at head ONLY. /// Do NOT use this directly for data availability check, use `self.sampling_size` instead as /// CGC can change over epochs. - pub fn custody_group_count_at_head(&self, spec: &ChainSpec) -> u64 { + pub fn custody_group_count_at_head(&self) -> u64 { let validator_custody_count_at_head = self.validator_custody_count.load(Ordering::Relaxed); // If there are no validators, return the minimum custody_requirement if validator_custody_count_at_head > 0 { validator_custody_count_at_head } else { - spec.custody_requirement + self.spec.custody_requirement } } @@ -436,33 +457,35 @@ impl CustodyContext { /// minimum sampling size which may exceed the custody group count (CGC). /// /// See also: [`Self::num_of_custody_groups_to_sample`]. - pub fn custody_group_count_at_epoch(&self, epoch: Epoch, spec: &ChainSpec) -> u64 { + pub fn custody_group_count_at_epoch(&self, epoch: Epoch) -> u64 { self.validator_registrations .read() .custody_requirement_at_epoch(epoch) - .unwrap_or(spec.custody_requirement) + .unwrap_or(self.spec.custody_requirement) } /// Returns the count of custody groups this node must _sample_ for a block at `epoch` to import. - pub fn num_of_custody_groups_to_sample(&self, epoch: Epoch, spec: &ChainSpec) -> u64 { - let custody_group_count = self.custody_group_count_at_epoch(epoch, spec); - spec.sampling_size_custody_groups(custody_group_count) + pub fn num_of_custody_groups_to_sample(&self, epoch: Epoch) -> u64 { + let custody_group_count = self.custody_group_count_at_epoch(epoch); + self.spec + .sampling_size_custody_groups(custody_group_count) .expect("should compute node sampling size from valid chain spec") } /// Returns the count of columns this node must _sample_ for a block at `epoch` to import. - pub fn num_of_data_columns_to_sample(&self, epoch: Epoch, spec: &ChainSpec) -> usize { - let custody_group_count = self.custody_group_count_at_epoch(epoch, spec); - spec.sampling_size_columns::(custody_group_count) + pub fn num_of_data_columns_to_sample(&self, epoch: Epoch) -> usize { + let custody_group_count = self.custody_group_count_at_epoch(epoch); + self.spec + .sampling_size_columns::(custody_group_count) .expect("should compute node sampling size from valid chain spec") } /// Returns whether the node should attempt reconstruction at a given epoch. - pub fn should_attempt_reconstruction(&self, epoch: Epoch, spec: &ChainSpec) -> bool { - let min_columns_for_reconstruction = E::number_of_columns() / 2; + pub fn should_attempt_reconstruction(&self, epoch: Epoch) -> bool { + let min_columns_for_reconstruction = T::EthSpec::number_of_columns() / 2; // performing reconstruction is not necessary if sampling column count is exactly 50%, // because the node doesn't need the remaining columns. - self.num_of_data_columns_to_sample(epoch, spec) > min_columns_for_reconstruction + self.num_of_data_columns_to_sample(epoch) > min_columns_for_reconstruction } /// Returns the ordered list of column indices that should be sampled for data availability checking at the given epoch. @@ -473,8 +496,8 @@ impl CustodyContext { /// /// # Returns /// A slice of ordered column indices that should be sampled for this epoch based on the node's custody configuration - pub fn sampling_columns_for_epoch(&self, epoch: Epoch, spec: &ChainSpec) -> &[ColumnIndex] { - let num_of_columns_to_sample = self.num_of_data_columns_to_sample(epoch, spec); + pub fn sampling_columns_for_epoch(&self, epoch: Epoch) -> &[ColumnIndex] { + let num_of_columns_to_sample = self.num_of_data_columns_to_sample(epoch); &self.ordered_custody_column_indices[..num_of_columns_to_sample] } @@ -491,19 +514,15 @@ impl CustodyContext { /// /// # Returns /// A slice of ordered custody column indices for this epoch based on the node's custody configuration - pub fn custody_columns_for_epoch( - &self, - epoch_opt: Option, - spec: &ChainSpec, - ) -> &[ColumnIndex] { + pub fn custody_columns_for_epoch(&self, epoch_opt: Option) -> &[ColumnIndex] { let custody_group_count = if let Some(epoch) = epoch_opt { - self.custody_group_count_at_epoch(epoch, spec) as usize + self.custody_group_count_at_epoch(epoch) as usize } else { - self.custody_group_count_at_head(spec) as usize + self.custody_group_count_at_head() as usize }; // This is an unnecessary conversion for spec compliance, basically just multiplying by 1. - let columns_per_custody_group = spec.data_columns_per_group::() as usize; + let columns_per_custody_group = self.spec.data_columns_per_group::() as usize; let custody_column_count = columns_per_custody_group * custody_group_count; &self.ordered_custody_column_indices[..custody_column_count] @@ -528,6 +547,61 @@ impl CustodyContext { .write() .reset_validator_custody_requirements(effective_epoch); } + + /// The epoch at which we require a data availability check in block processing. + /// `None` if the `Deneb` fork is disabled. + pub fn data_availability_boundary(&self) -> Option { + let fork_epoch = self.spec.deneb_fork_epoch?; + + if self.complete_blob_backfill { + Some(fork_epoch) + } else { + let current_epoch = self.slot_clock.now()?.epoch(T::EthSpec::slots_per_epoch()); + self.spec + .min_epoch_data_availability_boundary(current_epoch) + } + } + + /// Returns true if the given epoch lies within the da boundary and false otherwise. + pub fn da_check_required_for_epoch(&self, block_epoch: Epoch) -> bool { + self.data_availability_boundary() + .is_some_and(|da_epoch| block_epoch >= da_epoch) + } + + /// If the epoch is from prior to the data availability boundary, no blobs are required. + pub fn blobs_required_for_epoch(&self, epoch: Epoch) -> bool { + self.da_check_required_for_epoch(epoch) && !self.spec.is_peer_das_enabled_for_epoch(epoch) + } + + /// If the epoch is from prior to the data availability boundary, no data columns are required. + pub fn data_columns_required_for_epoch(&self, epoch: Epoch) -> bool { + self.da_check_required_for_epoch(epoch) && self.spec.is_peer_das_enabled_for_epoch(epoch) + } + + /// See `Self::blobs_required_for_epoch` + pub fn blobs_required_for_block(&self, block: &SignedBeaconBlock) -> bool { + block.num_expected_blobs() > 0 && self.blobs_required_for_epoch(block.epoch()) + } + + /// See `Self::data_columns_required_for_epoch` + pub fn data_columns_required_for_block(&self, block: &SignedBeaconBlock) -> bool { + block.num_expected_blobs() > 0 && self.data_columns_required_for_epoch(block.epoch()) + } + + pub fn data_columns_required_for_bid( + &self, + bid: &SignedExecutionPayloadBid, + ) -> bool { + bid.num_blobs_expected() > 0 && self.data_columns_required_for_epoch(bid.epoch()) + } + + /// The data availability boundary for custodying columns. It will just be the + /// regular data availability boundary unless we are near the Fulu fork epoch. + pub fn column_data_availability_boundary(&self) -> Option { + let da_boundary = self.data_availability_boundary()?; + let fulu_epoch = self.spec.fulu_fork_epoch?; + Some(da_boundary.max(fulu_epoch)) + } } /// Indicates that the custody group count (CGC) has increased. @@ -553,8 +627,8 @@ pub struct CustodyContextSsz { pub epoch_validator_custody_requirements: Vec<(Epoch, u64)>, } -impl From<&CustodyContext> for CustodyContextSsz { - fn from(context: &CustodyContext) -> Self { +impl From<&CustodyContext> for CustodyContextSsz { + fn from(context: &CustodyContext) -> Self { CustodyContextSsz { validator_custody_at_head: context.validator_custody_count.load(Ordering::Relaxed), // This field is deprecated and has no effect @@ -573,16 +647,27 @@ impl From<&CustodyContext> for CustodyContextSsz { #[cfg(test)] mod tests { use super::*; - use crate::test_utils::generate_data_column_indices_rand_order; + use crate::test_utils::{EphemeralHarnessType, generate_data_column_indices_rand_order}; + use slot_clock::{SlotClock, TestingSlotClock}; + use std::time::Duration; use types::MainnetEthSpec; type E = MainnetEthSpec; + type T = EphemeralHarnessType; + + fn testing_slot_clock(spec: &ChainSpec) -> TestingSlotClock { + TestingSlotClock::new( + Slot::new(0), + Duration::from_secs(0), + spec.get_slot_duration(), + ) + } fn setup_custody_context( - spec: &ChainSpec, + spec: Arc, head_epoch: Epoch, epoch_and_cgc_tuples: Vec<(Epoch, u64)>, - ) -> CustodyContext { + ) -> CustodyContext { let cgc_at_head = epoch_and_cgc_tuples.last().unwrap().1; let ssz_context = CustodyContextSsz { validator_custody_at_head: cgc_at_head, @@ -590,11 +675,14 @@ mod tests { epoch_validator_custody_requirements: epoch_and_cgc_tuples, }; - let (custody_context, _) = CustodyContext::::new_from_persisted_custody_context( + let complete_blob_backfill = false; + let (custody_context, _) = CustodyContext::::new_from_persisted_custody_context( ssz_context, NodeCustodyType::Fullnode, head_epoch, generate_data_column_indices_rand_order::(), + testing_slot_clock(&spec), + complete_blob_backfill, spec, ); @@ -602,7 +690,7 @@ mod tests { } fn complete_backfill_for_epochs( - custody_context: &CustodyContext, + custody_context: &CustodyContext, start_epoch: Epoch, end_epoch: Epoch, expected_cgc: u64, @@ -623,26 +711,29 @@ mod tests { target_node_custody_type: NodeCustodyType, expected_new_cgc: u64, head_epoch: Epoch, - spec: &ChainSpec, + spec: Arc, ) { let ssz_context = CustodyContextSsz { validator_custody_at_head: persisted_cgc, persisted_is_supernode: false, epoch_validator_custody_requirements: vec![(Epoch::new(0), persisted_cgc)], }; + let complete_blob_backfill = false; let (custody_context, custody_count_changed) = - CustodyContext::::new_from_persisted_custody_context( + CustodyContext::::new_from_persisted_custody_context( ssz_context, target_node_custody_type, head_epoch, generate_data_column_indices_rand_order::(), - spec, + testing_slot_clock(&spec), + complete_blob_backfill, + spec.clone(), ); // Verify CGC increased assert_eq!( - custody_context.custody_group_count_at_head(spec), + custody_context.custody_group_count_at_head(), expected_new_cgc, "cgc should increase from {} to {}", persisted_cgc, @@ -675,13 +766,13 @@ mod tests { // Verify custody_group_count_at_epoch returns correct values assert_eq!( - custody_context.custody_group_count_at_epoch(head_epoch, spec), + custody_context.custody_group_count_at_epoch(head_epoch), persisted_cgc, "current epoch should still use old cgc ({})", persisted_cgc ); assert_eq!( - custody_context.custody_group_count_at_epoch(head_epoch + 1, spec), + custody_context.custody_group_count_at_epoch(head_epoch + 1), expected_new_cgc, "next epoch should use new cgc ({})", expected_new_cgc @@ -694,26 +785,29 @@ mod tests { persisted_cgc: u64, target_node_custody_type: NodeCustodyType, head_epoch: Epoch, - spec: &ChainSpec, + spec: Arc, ) { let ssz_context = CustodyContextSsz { validator_custody_at_head: persisted_cgc, persisted_is_supernode: false, epoch_validator_custody_requirements: vec![(Epoch::new(0), persisted_cgc)], }; + let complete_blob_backfill = false; let (custody_context, custody_count_changed) = - CustodyContext::::new_from_persisted_custody_context( + CustodyContext::::new_from_persisted_custody_context( ssz_context, target_node_custody_type, head_epoch, generate_data_column_indices_rand_order::(), - spec, + testing_slot_clock(&spec), + complete_blob_backfill, + spec.clone(), ); // Verify CGC stays at persisted value (no reduction) assert_eq!( - custody_context.custody_group_count_at_head(spec), + custody_context.custody_group_count_at_head(), persisted_cgc, "cgc should remain at {} (reduction not supported)", persisted_cgc @@ -728,66 +822,78 @@ mod tests { #[test] fn no_validators_supernode_default() { - let spec = E::default_spec(); - let custody_context = CustodyContext::::new( + let spec = Arc::new(E::default_spec()); + let complete_blob_backfill = false; + let custody_context = CustodyContext::::new( NodeCustodyType::Supernode, generate_data_column_indices_rand_order::(), - &spec, + testing_slot_clock(&spec), + complete_blob_backfill, + spec.clone(), ); assert_eq!( - custody_context.custody_group_count_at_head(&spec), + custody_context.custody_group_count_at_head(), spec.number_of_custody_groups ); assert_eq!( - custody_context.num_of_custody_groups_to_sample(Epoch::new(0), &spec), + custody_context.num_of_custody_groups_to_sample(Epoch::new(0)), spec.number_of_custody_groups ); } #[test] fn no_validators_semi_supernode_default() { - let spec = E::default_spec(); - let custody_context = CustodyContext::::new( + let spec = Arc::new(E::default_spec()); + let complete_blob_backfill = false; + let custody_context = CustodyContext::::new( NodeCustodyType::SemiSupernode, generate_data_column_indices_rand_order::(), - &spec, + testing_slot_clock(&spec), + complete_blob_backfill, + spec.clone(), ); assert_eq!( - custody_context.custody_group_count_at_head(&spec), + custody_context.custody_group_count_at_head(), spec.number_of_custody_groups / 2 ); assert_eq!( - custody_context.num_of_custody_groups_to_sample(Epoch::new(0), &spec), + custody_context.num_of_custody_groups_to_sample(Epoch::new(0)), spec.number_of_custody_groups / 2 ); } #[test] fn no_validators_fullnode_default() { - let spec = E::default_spec(); - let custody_context = CustodyContext::::new( + let spec = Arc::new(E::default_spec()); + let complete_blob_backfill = false; + let custody_context = CustodyContext::::new( NodeCustodyType::Fullnode, generate_data_column_indices_rand_order::(), - &spec, + testing_slot_clock(&spec), + complete_blob_backfill, + spec.clone(), ); assert_eq!( - custody_context.custody_group_count_at_head(&spec), + custody_context.custody_group_count_at_head(), spec.custody_requirement, "head custody count should be minimum spec custody requirement" ); assert_eq!( - custody_context.num_of_custody_groups_to_sample(Epoch::new(0), &spec), + custody_context.num_of_custody_groups_to_sample(Epoch::new(0)), spec.samples_per_slot ); } #[test] fn register_single_validator_should_update_cgc() { - let spec = E::default_spec(); - let custody_context = CustodyContext::::new( + let spec = Arc::new(E::default_spec()); + let complete_blob_backfill = false; + let custody_context = CustodyContext::::new( NodeCustodyType::Fullnode, generate_data_column_indices_rand_order::(), - &spec, + testing_slot_clock(&spec), + complete_blob_backfill, + spec.clone(), ); let bal_per_additional_group = spec.balance_per_additional_custody_group; let min_val_custody_requirement = spec.validator_custody_requirement; @@ -802,20 +908,22 @@ mod tests { (vec![(0, 10 * bal_per_additional_group)], Some(10)), ]; - register_validators_and_assert_cgc::( + register_validators_and_assert_cgc::( &custody_context, validators_and_expected_cgc_change, - &spec, ); } #[test] fn register_multiple_validators_should_update_cgc() { - let spec = E::default_spec(); - let custody_context = CustodyContext::::new( + let spec = Arc::new(E::default_spec()); + let complete_blob_backfill = false; + let custody_context = CustodyContext::::new( NodeCustodyType::Fullnode, generate_data_column_indices_rand_order::(), - &spec, + testing_slot_clock(&spec), + complete_blob_backfill, + spec.clone(), ); let bal_per_additional_group = spec.balance_per_additional_custody_group; let min_val_custody_requirement = spec.validator_custody_requirement; @@ -843,20 +951,19 @@ mod tests { ), ]; - register_validators_and_assert_cgc::( - &custody_context, - validators_and_expected_cgc, - &spec, - ); + register_validators_and_assert_cgc::(&custody_context, validators_and_expected_cgc); } #[test] fn register_validators_should_not_update_cgc_for_supernode() { - let spec = E::default_spec(); - let custody_context = CustodyContext::::new( + let spec = Arc::new(E::default_spec()); + let complete_blob_backfill = false; + let custody_context = CustodyContext::::new( NodeCustodyType::Supernode, generate_data_column_indices_rand_order::(), - &spec, + testing_slot_clock(&spec), + complete_blob_backfill, + spec.clone(), ); let bal_per_additional_group = spec.balance_per_additional_custody_group; @@ -880,30 +987,28 @@ mod tests { ), ]; - register_validators_and_assert_cgc::( - &custody_context, - validators_and_expected_cgc, - &spec, - ); + register_validators_and_assert_cgc::(&custody_context, validators_and_expected_cgc); let current_epoch = Epoch::new(2); assert_eq!( - custody_context.num_of_custody_groups_to_sample(current_epoch, &spec), + custody_context.num_of_custody_groups_to_sample(current_epoch), spec.number_of_custody_groups ); } #[test] fn cgc_change_should_be_effective_to_sampling_after_delay() { - let spec = E::default_spec(); - let custody_context = CustodyContext::::new( + let spec = Arc::new(E::default_spec()); + let complete_blob_backfill = false; + let custody_context = CustodyContext::::new( NodeCustodyType::Fullnode, generate_data_column_indices_rand_order::(), - &spec, + testing_slot_clock(&spec), + complete_blob_backfill, + spec.clone(), ); let current_slot = Slot::new(10); let current_epoch = current_slot.epoch(E::slots_per_epoch()); - let default_sampling_size = - custody_context.num_of_custody_groups_to_sample(current_epoch, &spec); + let default_sampling_size = custody_context.num_of_custody_groups_to_sample(current_epoch); let validator_custody_units = 10; let _cgc_changed = custody_context.register_validators( @@ -912,28 +1017,30 @@ mod tests { validator_custody_units * spec.balance_per_additional_custody_group, )], current_slot, - &spec, ); // CGC update is not applied for `current_epoch`. assert_eq!( - custody_context.num_of_custody_groups_to_sample(current_epoch, &spec), + custody_context.num_of_custody_groups_to_sample(current_epoch), default_sampling_size ); // CGC update is applied for the next epoch. assert_eq!( - custody_context.num_of_custody_groups_to_sample(current_epoch + 1, &spec), + custody_context.num_of_custody_groups_to_sample(current_epoch + 1), validator_custody_units ); } #[test] fn validator_dropped_after_no_registrations_within_expiry_should_not_reduce_cgc() { - let spec = E::default_spec(); - let custody_context = CustodyContext::::new( + let spec = Arc::new(E::default_spec()); + let complete_blob_backfill = false; + let custody_context = CustodyContext::::new( NodeCustodyType::Fullnode, generate_data_column_indices_rand_order::(), - &spec, + testing_slot_clock(&spec), + complete_blob_backfill, + spec.clone(), ); let current_slot = Slot::new(10); let val_custody_units_1 = 10; @@ -952,7 +1059,6 @@ mod tests { ), ], current_slot, - &spec, ); // WHEN val_1 re-registered, but val_2 did not re-register after `VALIDATOR_REGISTRATION_EXPIRY_SLOTS + 1` slots @@ -962,24 +1068,26 @@ mod tests { val_custody_units_1 * spec.balance_per_additional_custody_group, )], current_slot + VALIDATOR_REGISTRATION_EXPIRY_SLOTS + 1, - &spec, ); // THEN the reduction from dropping val_2 balance should NOT result in a CGC reduction assert!(cgc_changed_opt.is_none(), "CGC should remain unchanged"); assert_eq!( - custody_context.custody_group_count_at_head(&spec), + custody_context.custody_group_count_at_head(), val_custody_units_1 + val_custody_units_2 ) } #[test] fn validator_dropped_after_no_registrations_within_expiry() { - let spec = E::default_spec(); - let custody_context = CustodyContext::::new( + let spec = Arc::new(E::default_spec()); + let complete_blob_backfill = false; + let custody_context = CustodyContext::::new( NodeCustodyType::Fullnode, generate_data_column_indices_rand_order::(), - &spec, + testing_slot_clock(&spec), + complete_blob_backfill, + spec.clone(), ); let current_slot = Slot::new(10); let val_custody_units_1 = 10; @@ -999,7 +1107,6 @@ mod tests { ), ], current_slot, - &spec, ); // WHEN val_1 and val_3 registered, but val_3 did not re-register after `VALIDATOR_REGISTRATION_EXPIRY_SLOTS + 1` slots @@ -1015,7 +1122,6 @@ mod tests { ), ], current_slot + VALIDATOR_REGISTRATION_EXPIRY_SLOTS + 1, - &spec, ); // THEN CGC should increase, BUT val_2 balance should NOT be included in CGC @@ -1028,10 +1134,9 @@ mod tests { } /// Update the validator every epoch and assert cgc against expected values. - fn register_validators_and_assert_cgc( - custody_context: &CustodyContext, + fn register_validators_and_assert_cgc( + custody_context: &CustodyContext, validators_and_expected_cgc_changed: Vec<(ValidatorsAndBalances, Option)>, - spec: &ChainSpec, ) { for (idx, (validators_and_balance, expected_cgc_change)) in validators_and_expected_cgc_changed.into_iter().enumerate() @@ -1040,8 +1145,7 @@ mod tests { let updated_custody_count_opt = custody_context .register_validators( validators_and_balance, - epoch.start_slot(E::slots_per_epoch()), - spec, + epoch.start_slot(T::EthSpec::slots_per_epoch()), ) .map(|c| c.new_custody_group_count); @@ -1051,44 +1155,53 @@ mod tests { #[test] fn custody_columns_for_epoch_no_validators_fullnode() { - let spec = E::default_spec(); + let spec = Arc::new(E::default_spec()); + let complete_blob_backfill = false; let ordered_custody_column_indices = generate_data_column_indices_rand_order::(); - let custody_context = CustodyContext::::new( + let custody_context = CustodyContext::::new( NodeCustodyType::Fullnode, ordered_custody_column_indices, - &spec, + testing_slot_clock(&spec), + complete_blob_backfill, + spec.clone(), ); assert_eq!( - custody_context.custody_columns_for_epoch(None, &spec).len(), + custody_context.custody_columns_for_epoch(None).len(), spec.custody_requirement as usize ); } #[test] fn custody_columns_for_epoch_no_validators_supernode() { - let spec = E::default_spec(); + let spec = Arc::new(E::default_spec()); + let complete_blob_backfill = false; let ordered_custody_column_indices = generate_data_column_indices_rand_order::(); - let custody_context = CustodyContext::::new( + let custody_context = CustodyContext::::new( NodeCustodyType::Supernode, ordered_custody_column_indices, - &spec, + testing_slot_clock(&spec), + complete_blob_backfill, + spec.clone(), ); assert_eq!( - custody_context.custody_columns_for_epoch(None, &spec).len(), + custody_context.custody_columns_for_epoch(None).len(), spec.number_of_custody_groups as usize ); } #[test] fn custody_columns_for_epoch_with_validators_should_match_cgc() { - let spec = E::default_spec(); + let spec = Arc::new(E::default_spec()); + let complete_blob_backfill = false; let ordered_custody_column_indices = generate_data_column_indices_rand_order::(); - let custody_context = CustodyContext::::new( + let custody_context = CustodyContext::::new( NodeCustodyType::Fullnode, ordered_custody_column_indices, - &spec, + testing_slot_clock(&spec), + complete_blob_backfill, + spec.clone(), ); let val_custody_units = 10; @@ -1098,30 +1211,32 @@ mod tests { val_custody_units * spec.balance_per_additional_custody_group, )], Slot::new(10), - &spec, ); assert_eq!( - custody_context.custody_columns_for_epoch(None, &spec).len(), + custody_context.custody_columns_for_epoch(None).len(), val_custody_units as usize ); } #[test] fn custody_columns_for_epoch_specific_epoch_uses_epoch_cgc() { - let spec = E::default_spec(); + let spec = Arc::new(E::default_spec()); + let complete_blob_backfill = false; let ordered_custody_column_indices = generate_data_column_indices_rand_order::(); - let custody_context = CustodyContext::::new( + let custody_context = CustodyContext::::new( NodeCustodyType::Fullnode, ordered_custody_column_indices, - &spec, + testing_slot_clock(&spec), + complete_blob_backfill, + spec.clone(), ); let test_epoch = Epoch::new(5); - let expected_cgc = custody_context.custody_group_count_at_epoch(test_epoch, &spec); + let expected_cgc = custody_context.custody_group_count_at_epoch(test_epoch); assert_eq!( custody_context - .custody_columns_for_epoch(Some(test_epoch), &spec) + .custody_columns_for_epoch(Some(test_epoch)) .len(), expected_cgc as usize ); @@ -1129,23 +1244,26 @@ mod tests { #[test] fn restore_from_persisted_fullnode_no_validators() { - let spec = E::default_spec(); + let spec = Arc::new(E::default_spec()); + let complete_blob_backfill = false; let ssz_context = CustodyContextSsz { validator_custody_at_head: 0, // no validators persisted_is_supernode: false, epoch_validator_custody_requirements: vec![], }; - let (custody_context, _) = CustodyContext::::new_from_persisted_custody_context( + let (custody_context, _) = CustodyContext::::new_from_persisted_custody_context( ssz_context, NodeCustodyType::Fullnode, Epoch::new(0), generate_data_column_indices_rand_order::(), - &spec, + testing_slot_clock(&spec), + complete_blob_backfill, + spec.clone(), ); assert_eq!( - custody_context.custody_group_count_at_head(&spec), + custody_context.custody_group_count_at_head(), spec.custody_requirement, "restored custody group count should match fullnode default" ); @@ -1155,7 +1273,7 @@ mod tests { /// CGC should increase and trigger backfill via CustodyCountChanged. #[test] fn restore_fullnode_then_switch_to_supernode_increases_cgc() { - let spec = E::default_spec(); + let spec = Arc::new(E::default_spec()); let head_epoch = Epoch::new(10); let supernode_cgc = spec.number_of_custody_groups; @@ -1164,7 +1282,7 @@ mod tests { NodeCustodyType::Supernode, supernode_cgc, head_epoch, - &spec, + spec, ); } @@ -1172,17 +1290,20 @@ mod tests { /// Semi-supernode can exceed 64 when validator effective balance increases CGC. #[test] fn restore_semi_supernode_with_validators_can_exceed_64() { - let spec = E::default_spec(); + let spec = Arc::new(E::default_spec()); + let complete_blob_backfill = false; let semi_supernode_cgc = spec.number_of_custody_groups / 2; // 64 - let custody_context = CustodyContext::::new( + let custody_context = CustodyContext::::new( NodeCustodyType::SemiSupernode, generate_data_column_indices_rand_order::(), - &spec, + testing_slot_clock(&spec), + complete_blob_backfill, + spec.clone(), ); // Verify initial CGC is 64 (semi-supernode) assert_eq!( - custody_context.custody_group_count_at_head(&spec), + custody_context.custody_group_count_at_head(), semi_supernode_cgc, "initial cgc should be 64" ); @@ -1196,7 +1317,6 @@ mod tests { validator_custody_units * spec.balance_per_additional_custody_group, )], current_slot, - &spec, ); // Verify CGC increased from 64 to 70 @@ -1216,7 +1336,7 @@ mod tests { // Verify the custody context reflects the new CGC assert_eq!( - custody_context.custody_group_count_at_head(&spec), + custody_context.custody_group_count_at_head(), validator_custody_units, "custody_group_count_at_head should be 70" ); @@ -1226,14 +1346,14 @@ mod tests { /// CGC reduction is not supported - persisted value is retained. #[test] fn restore_supernode_then_switch_to_fullnode_uses_persisted() { - let spec = E::default_spec(); + let spec = Arc::new(E::default_spec()); let supernode_cgc = spec.number_of_custody_groups; assert_custody_type_switch_unchanged_cgc( supernode_cgc, NodeCustodyType::Fullnode, Epoch::new(0), - &spec, + spec, ); } @@ -1241,7 +1361,7 @@ mod tests { /// CGC reduction is not supported - persisted value is retained. #[test] fn restore_supernode_then_switch_to_semi_supernode_keeps_supernode_cgc() { - let spec = E::default_spec(); + let spec = Arc::new(E::default_spec()); let supernode_cgc = spec.number_of_custody_groups; let head_epoch = Epoch::new(10); @@ -1249,7 +1369,7 @@ mod tests { supernode_cgc, NodeCustodyType::SemiSupernode, head_epoch, - &spec, + spec, ); } @@ -1257,7 +1377,7 @@ mod tests { /// CGC should increase and trigger backfill via CustodyCountChanged. #[test] fn restore_fullnode_with_validators_then_switch_to_semi_supernode() { - let spec = E::default_spec(); + let spec = Arc::new(E::default_spec()); let persisted_cgc = 32u64; let semi_supernode_cgc = spec.number_of_custody_groups / 2; let head_epoch = Epoch::new(10); @@ -1267,7 +1387,7 @@ mod tests { NodeCustodyType::SemiSupernode, semi_supernode_cgc, head_epoch, - &spec, + spec, ); } @@ -1275,7 +1395,7 @@ mod tests { /// CGC should increase and trigger backfill via CustodyCountChanged. #[test] fn restore_semi_supernode_then_switch_to_supernode() { - let spec = E::default_spec(); + let spec = Arc::new(E::default_spec()); let semi_supernode_cgc = spec.number_of_custody_groups / 2; let supernode_cgc = spec.number_of_custody_groups; let head_epoch = Epoch::new(10); @@ -1285,7 +1405,7 @@ mod tests { NodeCustodyType::Supernode, supernode_cgc, head_epoch, - &spec, + spec, ); } @@ -1293,7 +1413,7 @@ mod tests { /// CGC should increase and trigger backfill via CustodyCountChanged. #[test] fn restore_with_cli_flag_increases_cgc_from_nonzero() { - let spec = E::default_spec(); + let spec = Arc::new(E::default_spec()); let persisted_cgc = 32u64; let supernode_cgc = spec.number_of_custody_groups; let head_epoch = Epoch::new(10); @@ -1303,13 +1423,13 @@ mod tests { NodeCustodyType::Supernode, supernode_cgc, head_epoch, - &spec, + spec, ); } #[test] fn restore_with_validator_custody_history_across_epochs() { - let spec = E::default_spec(); + let spec = Arc::new(E::default_spec()); let initial_cgc = 8u64; let increased_cgc = 16u64; let final_cgc = 32u64; @@ -1324,45 +1444,45 @@ mod tests { ], }; - let (custody_context, _) = CustodyContext::::new_from_persisted_custody_context( + let complete_blob_backfill = false; + let (custody_context, _) = CustodyContext::::new_from_persisted_custody_context( ssz_context, NodeCustodyType::Fullnode, Epoch::new(20), generate_data_column_indices_rand_order::(), - &spec, + testing_slot_clock(&spec), + complete_blob_backfill, + spec.clone(), ); // Verify head uses latest value - assert_eq!( - custody_context.custody_group_count_at_head(&spec), - final_cgc - ); + assert_eq!(custody_context.custody_group_count_at_head(), final_cgc); // Verify historical epoch lookups work correctly assert_eq!( - custody_context.custody_group_count_at_epoch(Epoch::new(5), &spec), + custody_context.custody_group_count_at_epoch(Epoch::new(5)), initial_cgc, "epoch 5 should use initial cgc" ); assert_eq!( - custody_context.custody_group_count_at_epoch(Epoch::new(15), &spec), + custody_context.custody_group_count_at_epoch(Epoch::new(15)), increased_cgc, "epoch 15 should use increased cgc" ); assert_eq!( - custody_context.custody_group_count_at_epoch(Epoch::new(25), &spec), + custody_context.custody_group_count_at_epoch(Epoch::new(25)), final_cgc, "epoch 25 should use final cgc" ); // Verify sampling size calculation uses correct historical values assert_eq!( - custody_context.num_of_custody_groups_to_sample(Epoch::new(5), &spec), + custody_context.num_of_custody_groups_to_sample(Epoch::new(5)), spec.samples_per_slot, "sampling at epoch 5 should use spec minimum since cgc is at minimum" ); assert_eq!( - custody_context.num_of_custody_groups_to_sample(Epoch::new(25), &spec), + custody_context.num_of_custody_groups_to_sample(Epoch::new(25)), final_cgc, "sampling at epoch 25 should match final cgc" ); @@ -1370,16 +1490,16 @@ mod tests { #[test] fn backfill_single_cgc_increase_updates_past_epochs() { - let spec = E::default_spec(); + let spec = Arc::new(E::default_spec()); let final_cgc = 32u64; let default_cgc = spec.custody_requirement; // Setup: Node restart after validators were registered, causing CGC increase to 32 at epoch 20 let head_epoch = Epoch::new(20); let epoch_and_cgc_tuples = vec![(head_epoch, final_cgc)]; - let custody_context = setup_custody_context(&spec, head_epoch, epoch_and_cgc_tuples); + let custody_context = setup_custody_context(spec.clone(), head_epoch, epoch_and_cgc_tuples); assert_eq!( - custody_context.custody_group_count_at_epoch(Epoch::new(15), &spec), + custody_context.custody_group_count_at_epoch(Epoch::new(15)), default_cgc, ); @@ -1388,26 +1508,26 @@ mod tests { // After backfilling to epoch 15, it should use latest CGC (32) assert_eq!( - custody_context.custody_group_count_at_epoch(Epoch::new(15), &spec), + custody_context.custody_group_count_at_epoch(Epoch::new(15)), final_cgc, ); assert_eq!( custody_context - .custody_columns_for_epoch(Some(Epoch::new(15)), &spec) + .custody_columns_for_epoch(Some(Epoch::new(15))) .len(), final_cgc as usize, ); // Prior epoch should still return the original CGC assert_eq!( - custody_context.custody_group_count_at_epoch(Epoch::new(14), &spec), + custody_context.custody_group_count_at_epoch(Epoch::new(14)), default_cgc, ); } #[test] fn backfill_with_multiple_cgc_increases_prunes_map_correctly() { - let spec = E::default_spec(); + let spec = Arc::new(E::default_spec()); let initial_cgc = 8u64; let mid_cgc = 16u64; let final_cgc = 32u64; @@ -1419,7 +1539,7 @@ mod tests { (Epoch::new(10), mid_cgc), (head_epoch, final_cgc), ]; - let custody_context = setup_custody_context(&spec, head_epoch, epoch_and_cgc_tuples); + let custody_context = setup_custody_context(spec.clone(), head_epoch, epoch_and_cgc_tuples); // Backfill to epoch 15 (between the two CGC increases) complete_backfill_for_epochs(&custody_context, Epoch::new(20), Epoch::new(15), final_cgc); @@ -1427,7 +1547,7 @@ mod tests { // Verify epochs 15 - 20 return latest CGC (32) for epoch in 15..=20 { assert_eq!( - custody_context.custody_group_count_at_epoch(Epoch::new(epoch), &spec), + custody_context.custody_group_count_at_epoch(Epoch::new(epoch)), final_cgc, ); } @@ -1435,7 +1555,7 @@ mod tests { // Verify epochs 10-14 still return mid_cgc (16) for epoch in 10..14 { assert_eq!( - custody_context.custody_group_count_at_epoch(Epoch::new(epoch), &spec), + custody_context.custody_group_count_at_epoch(Epoch::new(epoch)), mid_cgc, ); } @@ -1443,7 +1563,7 @@ mod tests { #[test] fn attempt_backfill_with_invalid_cgc() { - let spec = E::default_spec(); + let spec = Arc::new(E::default_spec()); let initial_cgc = 8u64; let mid_cgc = 16u64; let final_cgc = 32u64; @@ -1455,7 +1575,7 @@ mod tests { (Epoch::new(10), mid_cgc), (head_epoch, final_cgc), ]; - let custody_context = setup_custody_context(&spec, head_epoch, epoch_and_cgc_tuples); + let custody_context = setup_custody_context(spec.clone(), head_epoch, epoch_and_cgc_tuples); // Backfill to epoch 15 (between the two CGC increases) complete_backfill_for_epochs(&custody_context, Epoch::new(20), Epoch::new(15), final_cgc); @@ -1463,7 +1583,7 @@ mod tests { // Verify epochs 15 - 20 return latest CGC (32) for epoch in 15..=20 { assert_eq!( - custody_context.custody_group_count_at_epoch(Epoch::new(epoch), &spec), + custody_context.custody_group_count_at_epoch(Epoch::new(epoch)), final_cgc, ); } @@ -1479,7 +1599,7 @@ mod tests { // Verify epochs 15 - 20 still return latest CGC (32) for epoch in 15..=20 { assert_eq!( - custody_context.custody_group_count_at_epoch(Epoch::new(epoch), &spec), + custody_context.custody_group_count_at_epoch(Epoch::new(epoch)), final_cgc, ); } @@ -1487,7 +1607,7 @@ mod tests { // Verify epochs 10-14 still return mid_cgc (16) for epoch in 10..14 { assert_eq!( - custody_context.custody_group_count_at_epoch(Epoch::new(epoch), &spec), + custody_context.custody_group_count_at_epoch(Epoch::new(epoch)), mid_cgc, ); } @@ -1495,7 +1615,7 @@ mod tests { #[test] fn reset_validator_custody_requirements() { - let spec = E::default_spec(); + let spec = Arc::new(E::default_spec()); let minimum_cgc = 4u64; let initial_cgc = 8u64; let mid_cgc = 16u64; @@ -1508,7 +1628,7 @@ mod tests { (Epoch::new(10), mid_cgc), (head_epoch, final_cgc), ]; - let custody_context = setup_custody_context(&spec, head_epoch, epoch_and_cgc_tuples); + let custody_context = setup_custody_context(spec.clone(), head_epoch, epoch_and_cgc_tuples); // Backfill from epoch 20 to 9 complete_backfill_for_epochs(&custody_context, Epoch::new(20), Epoch::new(9), final_cgc); @@ -1519,14 +1639,14 @@ mod tests { // Verify epochs 0 - 19 return the minimum cgc requirement because of the validator custody requirement reset for epoch in 0..=19 { assert_eq!( - custody_context.custody_group_count_at_epoch(Epoch::new(epoch), &spec), + custody_context.custody_group_count_at_epoch(Epoch::new(epoch)), minimum_cgc, ); } // Verify epoch 20 returns a CGC of 32 assert_eq!( - custody_context.custody_group_count_at_epoch(head_epoch, &spec), + custody_context.custody_group_count_at_epoch(head_epoch), final_cgc ); @@ -1536,7 +1656,7 @@ mod tests { // Verify epochs 0 - 20 return the final cgc requirements for epoch in 0..=20 { assert_eq!( - custody_context.custody_group_count_at_epoch(Epoch::new(epoch), &spec), + custody_context.custody_group_count_at_epoch(Epoch::new(epoch)), final_cgc, ); } diff --git a/beacon_node/beacon_chain/src/data_availability_checker.rs b/beacon_node/beacon_chain/src/data_availability_checker.rs index e559dc7689..d129072ff9 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker.rs @@ -18,7 +18,7 @@ use tracing::{debug, error, instrument}; use types::data::{BlobIdentifier, FixedBlobSidecarList, PartialDataColumn}; use types::{ BlobSidecar, BlobSidecarList, BlockImportSource, ChainSpec, DataColumnSidecar, - DataColumnSidecarList, Epoch, EthSpec, Hash256, PartialDataColumnSidecarError, + DataColumnSidecarList, EthSpec, Hash256, PartialDataColumnSidecarError, PartialDataColumnSidecarRef, SignedBeaconBlock, Slot, }; @@ -75,12 +75,9 @@ const OVERFLOW_LRU_CAPACITY: usize = 32; /// proposer. Having a capacity > 1 is an optimization to prevent sync lookup from having re-fetch /// data during moments of unstable network conditions. pub struct DataAvailabilityChecker { - complete_blob_backfill: bool, availability_cache: Arc>, partial_assembler: Option>>, - slot_clock: T::SlotClock, kzg: Arc, - custody_context: Arc>, spec: Arc, } @@ -115,10 +112,8 @@ impl Debug for Availability { impl DataAvailabilityChecker { pub fn new( - complete_blob_backfill: bool, - slot_clock: T::SlotClock, kzg: Arc, - custody_context: Arc>, + custody_context: Arc>, spec: Arc, enable_partial_columns: bool, disable_get_blobs: bool, @@ -137,18 +132,15 @@ impl DataAvailabilityChecker { None }; Ok(Self { - complete_blob_backfill, partial_assembler, availability_cache: Arc::new(inner), - slot_clock, kzg, - custody_context, spec, }) } - pub fn custody_context(&self) -> &Arc> { - &self.custody_context + fn custody_context(&self) -> &Arc> { + self.availability_cache.custody_context() } pub fn partial_assembler(&self) -> Option<&Arc>> { @@ -310,9 +302,9 @@ impl DataAvailabilityChecker { &self, block_root: Hash256, blobs: FixedBlobSidecarList, + slot_clock: &T::SlotClock, ) -> Result, AvailabilityCheckError> { - let seen_timestamp = self - .slot_clock + let seen_timestamp = slot_clock .now_duration() .ok_or(AvailabilityCheckError::SlotClockError)?; @@ -350,9 +342,7 @@ impl DataAvailabilityChecker { // not be yet effective for data availability check, as CGC changes are only effecive from // a new epoch. let epoch = slot.epoch(T::EthSpec::slots_per_epoch()); - let sampling_columns = self - .custody_context - .sampling_columns_for_epoch(epoch, &self.spec); + let sampling_columns = self.custody_context().sampling_columns_for_epoch(epoch); let verified_custody_columns = kzg_verified_columns .into_iter() .filter(|col| sampling_columns.contains(&col.index())) @@ -390,9 +380,7 @@ impl DataAvailabilityChecker { data_columns: I, ) -> Result, AvailabilityCheckError> { let epoch = slot.epoch(T::EthSpec::slots_per_epoch()); - let sampling_columns = self - .custody_context - .sampling_columns_for_epoch(epoch, &self.spec); + let sampling_columns = self.custody_context().sampling_columns_for_epoch(epoch); let custody_columns = data_columns .into_iter() .filter(|col| sampling_columns.contains(&col.index())) @@ -507,59 +495,6 @@ impl DataAvailabilityChecker { Ok(()) } - /// Determines the blob requirements for a block. If the block is pre-deneb, no blobs are required. - /// If the epoch is from prior to the data availability boundary, no blobs are required. - pub fn blobs_required_for_epoch(&self, epoch: Epoch) -> bool { - self.da_check_required_for_epoch(epoch) && !self.spec.is_peer_das_enabled_for_epoch(epoch) - } - - /// Determines the data column requirements for an epoch. - /// - If the epoch is pre-peerdas, no data columns are required. - /// - If the epoch is from prior to the data availability boundary, no data columns are required. - pub fn data_columns_required_for_epoch(&self, epoch: Epoch) -> bool { - self.da_check_required_for_epoch(epoch) && self.spec.is_peer_das_enabled_for_epoch(epoch) - } - - /// See `Self::blobs_required_for_epoch` - fn blobs_required_for_block(&self, block: &SignedBeaconBlock) -> bool { - block.num_expected_blobs() > 0 && self.blobs_required_for_epoch(block.epoch()) - } - - /// See `Self::data_columns_required_for_epoch` - fn data_columns_required_for_block(&self, block: &SignedBeaconBlock) -> bool { - block.num_expected_blobs() > 0 && self.data_columns_required_for_epoch(block.epoch()) - } - - /// The epoch at which we require a data availability check in block processing. - /// `None` if the `Deneb` fork is disabled. - pub fn data_availability_boundary(&self) -> Option { - let fork_epoch = self.spec.deneb_fork_epoch?; - - if self.complete_blob_backfill { - Some(fork_epoch) - } else { - let current_epoch = self.slot_clock.now()?.epoch(T::EthSpec::slots_per_epoch()); - self.spec - .min_epoch_data_availability_boundary(current_epoch) - } - } - - /// Returns true if the given epoch lies within the da boundary and false otherwise. - pub fn da_check_required_for_epoch(&self, block_epoch: Epoch) -> bool { - self.data_availability_boundary() - .is_some_and(|da_epoch| block_epoch >= da_epoch) - } - - /// Returns `true` if the current epoch is greater than or equal to the `Deneb` epoch. - pub fn is_deneb(&self) -> bool { - self.slot_clock.now().is_some_and(|slot| { - self.spec.deneb_fork_epoch.is_some_and(|deneb_epoch| { - let now_epoch = slot.epoch(T::EthSpec::slots_per_epoch()); - now_epoch >= deneb_epoch - }) - }) - } - /// Collects metrics from the data availability checker. pub fn metrics(&self) -> DataAvailabilityCheckerMetrics { DataAvailabilityCheckerMetrics { @@ -629,7 +564,7 @@ impl DataAvailabilityChecker { let columns_to_sample = self .custody_context() - .sampling_columns_for_epoch(slot.epoch(T::EthSpec::slots_per_epoch()), &self.spec); + .sampling_columns_for_epoch(slot.epoch(T::EthSpec::slots_per_epoch())); // We only need to import and publish columns that we need to sample // and columns that we haven't already received @@ -886,15 +821,14 @@ impl AvailableBlock { pub fn new( block: Arc>, block_data: AvailableBlockData, - da_checker: &DataAvailabilityChecker, - spec: Arc, + custody_context: &CustodyContext, ) -> Result where T: BeaconChainTypes, { // Ensure block availability - let blobs_required = da_checker.blobs_required_for_block(&block); - let columns_required = da_checker.data_columns_required_for_block(&block); + let blobs_required = custody_context.blobs_required_for_block(&block); + let columns_required = custody_context.data_columns_required_for_block(&block); match &block_data { AvailableBlockData::NoData => { @@ -935,9 +869,8 @@ impl AvailableBlock { return Err(AvailabilityCheckError::InvalidAvailableBlockData); } - let mut column_indices = da_checker - .custody_context - .sampling_columns_for_epoch(block.epoch(), &spec) + let mut column_indices = custody_context + .sampling_columns_for_epoch(block.epoch()) .iter() .collect::>(); @@ -1081,7 +1014,8 @@ mod test { use std::time::Duration; use types::data::DataColumn; use types::{ - ChainSpec, ColumnIndex, DataColumnSidecarFulu, EthSpec, ForkName, MainnetEthSpec, Slot, + ChainSpec, ColumnIndex, DataColumnSidecarFulu, Epoch, EthSpec, ForkName, MainnetEthSpec, + Slot, }; type E = MainnetEthSpec; @@ -1096,7 +1030,7 @@ mod test { let mut u = types::test_utils::test_unstructured(); let da_checker = new_da_checker(spec.clone()); - let custody_context = &da_checker.custody_context; + let custody_context = da_checker.custody_context(); // GIVEN a single 32 ETH validator is attached slot 0 let epoch = Epoch::new(0); @@ -1104,10 +1038,9 @@ mod test { custody_context.register_validators( vec![(validator_0, 32_000_000_000)], epoch.start_slot(E::slots_per_epoch()), - &spec, ); assert_eq!( - custody_context.num_of_data_columns_to_sample(epoch, &spec), + custody_context.num_of_data_columns_to_sample(epoch), spec.validator_custody_requirement as usize, "sampling size should be the minimal custody requirement == 8" ); @@ -1115,11 +1048,8 @@ mod test { // WHEN additional attached validators result in a CGC increase to 10 at the end slot of the same epoch let validator_1 = 1; let cgc_change_slot = epoch.end_slot(E::slots_per_epoch()); - custody_context.register_validators( - vec![(validator_1, 32_000_000_000 * 9)], - cgc_change_slot, - &spec, - ); + custody_context + .register_validators(vec![(validator_1, 32_000_000_000 * 9)], cgc_change_slot); // AND custody columns (8) and any new extra columns (2) are received via RPC responses. // NOTE: block lookup uses the **latest** CGC (10) instead of the effective CGC (8) as the slot is unknown. let (_, data_columns) = generate_rand_block_and_data_columns::( @@ -1134,7 +1064,7 @@ mod test { // The CGC change becomes effective after CUSTODY_CHANGE_DA_EFFECTIVE_DELAY_SECONDS, // which is typically epoch 2+ for MinimalEthSpec. let future_epoch = Epoch::new(10); // Far enough in the future to have the CGC change effective - let requested_columns = custody_context.sampling_columns_for_epoch(future_epoch, &spec); + let requested_columns = custody_context.sampling_columns_for_epoch(future_epoch); assert_eq!( requested_columns.len(), 10, @@ -1152,7 +1082,7 @@ mod test { .expect("should put rpc custody columns"); // THEN the sampling size for the end slot of the same epoch remains unchanged - let sampling_columns = custody_context.sampling_columns_for_epoch(epoch, &spec); + let sampling_columns = custody_context.sampling_columns_for_epoch(epoch); assert_eq!( sampling_columns.len(), spec.validator_custody_requirement as usize // 8 @@ -1183,7 +1113,7 @@ mod test { let mut u = types::test_utils::test_unstructured(); let da_checker = new_da_checker(spec.clone()); - let custody_context = &da_checker.custody_context; + let custody_context = da_checker.custody_context(); // GIVEN a single 32 ETH validator is attached slot 0 let epoch = Epoch::new(0); @@ -1191,10 +1121,9 @@ mod test { custody_context.register_validators( vec![(validator_0, 32_000_000_000)], epoch.start_slot(E::slots_per_epoch()), - &spec, ); assert_eq!( - custody_context.num_of_data_columns_to_sample(epoch, &spec), + custody_context.num_of_data_columns_to_sample(epoch), spec.validator_custody_requirement as usize, "sampling size should be the minimal custody requirement == 8" ); @@ -1202,11 +1131,8 @@ mod test { // WHEN additional attached validators result in a CGC increase to 10 at the end slot of the same epoch let validator_1 = 1; let cgc_change_slot = epoch.end_slot(E::slots_per_epoch()); - custody_context.register_validators( - vec![(validator_1, 32_000_000_000 * 9)], - cgc_change_slot, - &spec, - ); + custody_context + .register_validators(vec![(validator_1, 32_000_000_000 * 9)], cgc_change_slot); // AND custody columns (8) and any new extra columns (2) are received via gossip. // NOTE: CGC updates results in new topics subscriptions immediately, and extra columns may start to // arrive via gossip. @@ -1222,7 +1148,7 @@ mod test { // The CGC change becomes effective after CUSTODY_CHANGE_DA_EFFECTIVE_DELAY_SECONDS, // which is typically epoch 2+ for MinimalEthSpec. let future_epoch = Epoch::new(10); // Far enough in the future to have the CGC change effective - let requested_columns = custody_context.sampling_columns_for_epoch(future_epoch, &spec); + let requested_columns = custody_context.sampling_columns_for_epoch(future_epoch); assert_eq!( requested_columns.len(), 10, @@ -1238,7 +1164,7 @@ mod test { .expect("should put gossip custody columns"); // THEN the sampling size for the end slot of the same epoch remains unchanged - let sampling_columns = custody_context.sampling_columns_for_epoch(epoch, &spec); + let sampling_columns = custody_context.sampling_columns_for_epoch(epoch); assert_eq!( sampling_columns.len(), spec.validator_custody_requirement as usize // 8 @@ -1305,8 +1231,7 @@ mod test { }; let block_data = AvailableBlockData::new_with_data_columns(custody_columns); - let da_checker = Arc::new(new_da_checker(spec.clone())); - RangeSyncBlock::new(Arc::new(block), block_data, &da_checker, spec.clone()) + RangeSyncBlock::new(Arc::new(block), block_data, da_checker.custody_context()) .expect("should create RPC block with custody columns") }) .collect::>(); @@ -1331,16 +1256,15 @@ mod test { let mut u = types::test_utils::test_unstructured(); let da_checker = new_da_checker(spec.clone()); - let custody_context = &da_checker.custody_context; + let custody_context = da_checker.custody_context(); // Set custody requirement to 65 columns (enough to trigger reconstruction) let epoch = Epoch::new(1); custody_context.register_validators( vec![(0, 2_048_000_000_000), (1, 32_000_000_000)], // 64 + 1 Slot::new(0), - &spec, ); - let sampling_requirement = custody_context.num_of_data_columns_to_sample(epoch, &spec); + let sampling_requirement = custody_context.num_of_data_columns_to_sample(epoch); assert_eq!( sampling_requirement, 65, "sampling requirement should be 65" @@ -1362,7 +1286,7 @@ mod test { // Add 64 columns to the da checker (enough to be able to reconstruct) // Order by all_column_indices_ordered, then take first 64 - let custody_columns = custody_context.sampling_columns_for_epoch(epoch, &spec); + let custody_columns = custody_context.sampling_columns_for_epoch(epoch); let custody_columns = custody_columns .iter() .filter_map(|&col_idx| data_columns.iter().find(|d| *d.index() == col_idx).cloned()) @@ -1400,7 +1324,7 @@ mod test { ); // Only the columns required for custody (65) should be imported into the cache - let sampling_columns = custody_context.sampling_columns_for_epoch(epoch, &spec); + let sampling_columns = custody_context.sampling_columns_for_epoch(epoch); let actual_cached: HashSet = da_checker .cached_data_column_indexes(&block_root) .expect("should have cached data columns") @@ -1421,21 +1345,15 @@ mod test { ); let kzg = get_kzg(&spec); let ordered_custody_column_indices = generate_data_column_indices_rand_order::(); + let complete_blob_backfill = false; let custody_context = Arc::new(CustodyContext::new( NodeCustodyType::Fullnode, ordered_custody_column_indices, - &spec, - )); - let complete_blob_backfill = false; - DataAvailabilityChecker::new( - complete_blob_backfill, slot_clock, - kzg, - custody_context, - spec, - true, - false, - ) - .expect("should initialise data availability checker") + complete_blob_backfill, + spec.clone(), + )); + DataAvailabilityChecker::new(kzg, custody_context, spec, true, false) + .expect("should initialise data availability checker") } } 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 47740cdf5e..dc59d614df 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 @@ -349,7 +349,7 @@ impl PendingComponents { pub struct DataAvailabilityCheckerInner { /// Contains all the data we keep in memory, protected by an RwLock critical: RwLock>>, - custody_context: Arc>, + custody_context: Arc>, spec: Arc, } @@ -365,7 +365,7 @@ pub(crate) enum ReconstructColumnsDecision { impl DataAvailabilityCheckerInner { pub fn new( capacity: usize, - custody_context: Arc>, + custody_context: Arc>, spec: Arc, ) -> Result { Ok(Self { @@ -434,6 +434,10 @@ impl DataAvailabilityCheckerInner { f(self.critical.read().peek(block_root)) } + pub fn custody_context(&self) -> &Arc> { + &self.custody_context + } + /// Puts the KZG verified blobs into the availability cache as pending components. pub fn put_kzg_verified_blobs>>( &self, @@ -500,9 +504,7 @@ impl DataAvailabilityCheckerInner { pending_components.merge_data_columns(kzg_verified_data_columns) })?; - let num_expected_columns = self - .custody_context - .num_of_data_columns_to_sample(epoch, &self.spec); + let num_expected_columns = self.custody_context.num_of_data_columns_to_sample(epoch); pending_components.span.in_scope(|| { debug!( @@ -606,9 +608,7 @@ impl DataAvailabilityCheckerInner { }; let total_column_count = T::EthSpec::number_of_columns(); - let sampling_column_count = self - .custody_context - .num_of_data_columns_to_sample(epoch, &self.spec); + let sampling_column_count = self.custody_context.num_of_data_columns_to_sample(epoch); let received_column_count = pending_components.verified_data_columns.len(); if pending_components.reconstruction_started { @@ -709,9 +709,7 @@ impl DataAvailabilityCheckerInner { fn get_num_expected_columns(&self, epoch: Epoch) -> Option { if self.spec.is_peer_das_enabled_for_epoch(epoch) { - let num_of_column_samples = self - .custody_context - .num_of_data_columns_to_sample(epoch, &self.spec); + let num_of_column_samples = self.custody_context.num_of_data_columns_to_sample(epoch); Some(num_of_column_samples) } else { None @@ -760,6 +758,7 @@ mod test { }; use fork_choice::PayloadVerificationStatus; use logging::create_test_tracing_subscriber; + use slot_clock::TestingSlotClock; use state_processing::ConsensusContext; use store::{HotColdDB, ItemStore, StoreConfig, database::interface::BeaconNodeBackend}; use tempfile::{TempDir, tempdir}; @@ -922,19 +921,25 @@ mod test { HotStore = BeaconNodeBackend, ColdStore = BeaconNodeBackend, EthSpec = E, + SlotClock = TestingSlotClock, >, { create_test_tracing_subscriber(); let chain_db_path = tempdir().expect("should get temp dir"); let harness = get_fulu_chain(&chain_db_path).await; let spec = harness.spec.clone(); + let complete_blob_backfill = false; + let slot_clock = harness.chain.slot_clock.clone(); + let custody_context = Arc::new(CustodyContext::new( NodeCustodyType::Fullnode, generate_data_column_indices_rand_order::(), - &spec, + slot_clock, + complete_blob_backfill, + spec.clone(), )); let cache = Arc::new( - DataAvailabilityCheckerInner::::new(capacity, custody_context, spec.clone()) + DataAvailabilityCheckerInner::::new(capacity, custody_context, spec) .expect("should create cache"), ); (harness, cache, chain_db_path) @@ -952,9 +957,7 @@ mod test { let epoch = pending_block.block.epoch(); let num_blobs_expected = pending_block.num_blobs_expected(); - let columns_expected = cache - .custody_context - .num_of_data_columns_to_sample(epoch, &harness.spec); + let columns_expected = cache.custody_context.num_of_data_columns_to_sample(epoch); // All columns are returned from availability_pending_block (E::number_of_columns()) // but we only need custody columns @@ -994,9 +997,7 @@ mod test { } // Get sampling column indices for this epoch - let sampling_column_indices = cache - .custody_context - .sampling_columns_for_epoch(epoch, &harness.spec); + let sampling_column_indices = cache.custody_context.sampling_columns_for_epoch(epoch); // Filter to only sampling columns let sampling_columns: Vec<_> = columns @@ -1032,9 +1033,7 @@ mod test { let root = pending_block.import_data.block_root; // Get sampling column indices for this epoch - let sampling_column_indices = cache - .custody_context - .sampling_columns_for_epoch(epoch, &harness.spec); + let sampling_column_indices = cache.custody_context.sampling_columns_for_epoch(epoch); // Filter to only sampling columns let sampling_columns: Vec<_> = columns diff --git a/beacon_node/beacon_chain/src/historical_data_columns.rs b/beacon_node/beacon_chain/src/historical_data_columns.rs index d6977d9985..1089c801ff 100644 --- a/beacon_node/beacon_chain/src/historical_data_columns.rs +++ b/beacon_node/beacon_chain/src/historical_data_columns.rs @@ -131,8 +131,7 @@ impl BeaconChain { ); } - self.data_availability_checker - .custody_context() + self.custody_context .update_and_backfill_custody_count_at_epoch(epoch, expected_cgc); self.safely_backfill_data_column_custody_info(epoch) diff --git a/beacon_node/beacon_chain/src/invariants.rs b/beacon_node/beacon_chain/src/invariants.rs index b365f37a0a..369b968e78 100644 --- a/beacon_node/beacon_chain/src/invariants.rs +++ b/beacon_node/beacon_chain/src/invariants.rs @@ -29,14 +29,12 @@ impl BeaconChain { .collect() }; - let custody_context = self.data_availability_checker.custody_context(); + let custody_context = self.custody_context.clone(); let ctx = InvariantContext { fork_choice_blocks, state_cache_roots: self.store.state_cache.lock().state_roots(), - custody_columns: custody_context - .custody_columns_for_epoch(None, &self.spec) - .to_vec(), + custody_columns: custody_context.custody_columns_for_epoch(None).to_vec(), pubkey_cache_pubkeys: { let cache = self.validator_pubkey_cache.read(); (0..cache.len()) diff --git a/beacon_node/beacon_chain/src/payload_envelope_verification/mod.rs b/beacon_node/beacon_chain/src/payload_envelope_verification/mod.rs index a0d34949c6..1dc8418420 100644 --- a/beacon_node/beacon_chain/src/payload_envelope_verification/mod.rs +++ b/beacon_node/beacon_chain/src/payload_envelope_verification/mod.rs @@ -17,20 +17,21 @@ //! ExecutedEnvelope //! //! ``` - +use crate::data_availability_checker::AvailabilityCheckError; +use crate::{ + BeaconChainError, BeaconChainTypes, BeaconStore, BlockError, CustodyContext, + ExecutionPayloadError, PayloadVerificationError, PayloadVerificationOutcome, +}; use state_processing::envelope_processing::EnvelopeProcessingError; +use std::collections::HashSet; use std::sync::Arc; use store::Error as DBError; use strum::AsRefStr; -use tracing::instrument; +use tracing::{instrument, warn}; use types::{ BeaconState, BeaconStateError, DataColumnSidecarList, EthSpec, ExecutionBlockHash, - ExecutionPayloadEnvelope, Hash256, SignedExecutionPayloadEnvelope, Slot, -}; - -use crate::{ - BeaconChainError, BeaconChainTypes, BeaconStore, BlockError, ExecutionPayloadError, - PayloadVerificationError, PayloadVerificationOutcome, + ExecutionPayloadEnvelope, Hash256, SignedExecutionPayloadBid, SignedExecutionPayloadEnvelope, + Slot, }; pub mod execution_pending_envelope; @@ -47,11 +48,76 @@ pub struct AvailableEnvelope { } impl AvailableEnvelope { - pub fn new( + /// Constructs an `AvailableEnvelope` from an envelope and custody column data. + /// + /// This function validates that: + /// - All required custody columns are present + /// + /// If more columns are provided than necessary, a warning is logged and the extra + /// columns are filtered out of the list. + /// + /// Returns `AvailabilityCheckError` if: + /// - `MissingCustodyColumns`: Required custody columns are missing or incomplete + pub fn new( envelope: Arc>, columns: DataColumnSidecarList, - ) -> Self { - Self { envelope, columns } + bid: &SignedExecutionPayloadBid, + custody_context: &CustodyContext, + ) -> Result + where + T: BeaconChainTypes, + { + if custody_context.data_columns_required_for_bid(bid) { + let columns_expected = custody_context.num_of_data_columns_to_sample(bid.epoch()); + + // Get required custody column indices + let required_indices = custody_context + .sampling_columns_for_epoch(bid.epoch()) + .iter() + .copied() + .collect::>(); + + // Filter to only the columns we need (deduplicates if there are duplicates) + let mut filtered_columns = Vec::new(); + let mut seen_indices = HashSet::new(); + let num_provided_columns = columns.len(); + for column in columns { + if required_indices.contains(column.index()) && seen_indices.insert(*column.index()) + { + filtered_columns.push(column); + } + } + + // Check if we have all required columns + if filtered_columns.len() != columns_expected { + return Err(AvailabilityCheckError::MissingCustodyColumns); + } + + if num_provided_columns != filtered_columns.len() { + warn!( + message = "More columns provided than expected", + envelope = %envelope.message.payload.block_hash, + num_provided_columns = %num_provided_columns, + columns_expected = %columns_expected, + ); + } + + Ok(Self { + envelope, + columns: filtered_columns, + }) + } else if columns.is_empty() { + Ok(Self { envelope, columns }) + } else { + warn!( + message = "Custody columns provided for envelope that does not require them", + envelope = %envelope.message.payload.block_hash, + ); + Ok(Self { + envelope, + columns: vec![], + }) + } } pub fn envelope(&self) -> &Arc> { diff --git a/beacon_node/beacon_chain/src/pending_payload_cache/mod.rs b/beacon_node/beacon_chain/src/pending_payload_cache/mod.rs index 81af4133f6..0b0159be34 100644 --- a/beacon_node/beacon_chain/src/pending_payload_cache/mod.rs +++ b/beacon_node/beacon_chain/src/pending_payload_cache/mod.rs @@ -92,14 +92,14 @@ pub struct PendingPayloadCache { /// Contains all the data we keep in memory, protected by an RwLock availability_cache: RwLock>>, kzg: Arc, - custody_context: Arc>, + custody_context: Arc>, spec: Arc, } impl PendingPayloadCache { pub fn new( kzg: Arc, - custody_context: Arc>, + custody_context: Arc>, spec: Arc, ) -> Result { Ok(Self { @@ -110,7 +110,7 @@ impl PendingPayloadCache { }) } - pub fn custody_context(&self) -> &Arc> { + pub fn custody_context(&self) -> &Arc> { &self.custody_context } @@ -174,7 +174,6 @@ impl PendingPayloadCache { &self, executed_envelope: AvailabilityPendingExecutedEnvelope, ) -> Result, AvailabilityCheckError> { - let epoch = executed_envelope.envelope.epoch(); let beacon_block_root = executed_envelope.envelope.beacon_block_root(); let bid = self .get_bid(&beacon_block_root) @@ -185,19 +184,15 @@ impl PendingPayloadCache { pending_components.insert_executed_payload_envelope(executed_envelope); })?; - let num_expected_columns = self - .custody_context - .num_of_data_columns_to_sample(epoch, &self.spec); - pending_components.span.in_scope(|| { debug!( component = "executed envelope", - status = pending_components.status_str(num_expected_columns), + status = pending_components.status_str(&self.custody_context), "Component added to data availability checker" ); }); - self.check_availability(beacon_block_root, pending_components, num_expected_columns) + self.check_availability(beacon_block_root, pending_components) } /// Inserts a bid into the pending payload cache. @@ -228,9 +223,7 @@ impl PendingPayloadCache { .map_err(AvailabilityCheckError::InvalidColumn)?; let epoch = bid.message.slot.epoch(T::EthSpec::slots_per_epoch()); - let sampling_columns = self - .custody_context - .sampling_columns_for_epoch(epoch, &self.spec); + let sampling_columns = self.custody_context.sampling_columns_for_epoch(epoch); let verified_custody_columns = kzg_verified_columns .into_iter() .filter(|col| sampling_columns.contains(&col.index())) @@ -252,9 +245,7 @@ impl PendingPayloadCache { .get_bid(&block_root) .ok_or(AvailabilityCheckError::MissingBid(block_root))?; let epoch = bid.message.slot.epoch(T::EthSpec::slots_per_epoch()); - let sampling_columns = self - .custody_context - .sampling_columns_for_epoch(epoch, &self.spec); + let sampling_columns = self.custody_context.sampling_columns_for_epoch(epoch); let custody_columns = data_columns .into_iter() .filter(|col| sampling_columns.contains(&col.index())) @@ -280,21 +271,15 @@ impl PendingPayloadCache { pending_components.merge_data_columns(kzg_verified_data_columns) })?; - let epoch = bid.message.slot.epoch(T::EthSpec::slots_per_epoch()); - - let num_expected_columns = self - .custody_context - .num_of_data_columns_to_sample(epoch, &self.spec); - pending_components.span.in_scope(|| { debug!( component = "data_columns", - status = pending_components.status_str(num_expected_columns), + status = pending_components.status_str(&self.custody_context), "Component added to data availability checker" ); }); - self.check_availability(block_root, pending_components, num_expected_columns) + self.check_availability(block_root, pending_components) } #[instrument(skip_all, level = "debug")] @@ -340,7 +325,7 @@ impl PendingPayloadCache { let slot = bid.message.slot; let columns_to_sample = self .custody_context() - .sampling_columns_for_epoch(slot.epoch(T::EthSpec::slots_per_epoch()), &self.spec); + .sampling_columns_for_epoch(slot.epoch(T::EthSpec::slots_per_epoch())); let data_columns_to_import_and_publish = all_data_columns .into_iter() @@ -388,9 +373,10 @@ impl PendingPayloadCache { &self, block_root: Hash256, pending_components: MappedRwLockReadGuard<'_, PendingComponents>, - num_expected_columns: usize, ) -> Result, AvailabilityCheckError> { - if let Some(available_envelope) = pending_components.make_available(num_expected_columns)? { + if let Some(available_envelope) = + pending_components.make_available(&self.custody_context)? + { // Explicitly drop read lock before acquiring write lock drop(pending_components); if let Some(components) = self.availability_cache.write().get_mut(&block_root) { @@ -458,9 +444,7 @@ impl PendingPayloadCache { let epoch = pending_components.bid.epoch(); let total_column_count = T::EthSpec::number_of_columns(); - let sampling_column_count = self - .custody_context - .num_of_data_columns_to_sample(epoch, &self.spec); + let sampling_column_count = self.custody_context.num_of_data_columns_to_sample(epoch); if pending_components.reconstruction_started { return ReconstructColumnsDecision::No("already started"); @@ -516,10 +500,11 @@ mod data_availability_checker_tests { }; use fork_choice::PayloadVerificationStatus; use logging::create_test_tracing_subscriber; + use slot_clock::{SlotClock, TestingSlotClock}; + use std::time::Duration; use types::test_utils::test_unstructured; use types::{ - ExecutionPayloadEnvelope, ExecutionPayloadGloas, ExecutionRequestsGloas, ForkName, - MinimalEthSpec, SignedExecutionPayloadEnvelope, + ExecutionPayloadEnvelope, ExecutionPayloadGloas, ExecutionRequestsGloas, ForkName, MinimalEthSpec, SignedExecutionPayloadEnvelope, Slot }; type E = MinimalEthSpec; @@ -541,10 +526,18 @@ mod data_availability_checker_tests { create_test_tracing_subscriber(); let spec = Arc::new(ForkName::Gloas.make_genesis_spec(E::default_spec())); let kzg = get_kzg(&spec); - let custody_context = Arc::new(CustodyContext::::new( + let slot_clock = TestingSlotClock::new( + Slot::new(0), + Duration::from_secs(0), + spec.get_slot_duration(), + ); + let complete_blob_backfill = false; + let custody_context = Arc::new(CustodyContext::::new( node_custody, generate_data_column_indices_rand_order::(), - &spec, + slot_clock, + complete_blob_backfill, + spec.clone(), )); let cache = Arc::new( PendingPayloadCache::::new(kzg, custody_context, spec.clone()) @@ -567,9 +560,7 @@ mod data_availability_checker_tests { cache.insert_bid(block_root, bid.clone()); let epoch = bid.message.slot.epoch(E::slots_per_epoch()); - let sampling = cache - .custody_context() - .sampling_columns_for_epoch(epoch, &cache.spec); + let sampling = cache.custody_context().sampling_columns_for_epoch(epoch); let custody = columns .into_iter() .filter(|c| sampling.contains(c.index())) diff --git a/beacon_node/beacon_chain/src/pending_payload_cache/pending_components.rs b/beacon_node/beacon_chain/src/pending_payload_cache/pending_components.rs index e7b9009577..dac0180be8 100644 --- a/beacon_node/beacon_chain/src/pending_payload_cache/pending_components.rs +++ b/beacon_node/beacon_chain/src/pending_payload_cache/pending_components.rs @@ -1,3 +1,5 @@ +use crate::beacon_chain::BeaconChainTypes; +use crate::custody_context::CustodyContext; use crate::data_availability_checker::AvailabilityCheckError; use crate::data_column_verification::KzgVerifiedCustodyDataColumn; use crate::payload_envelope_verification::AvailabilityPendingExecutedEnvelope; @@ -27,8 +29,15 @@ pub struct PendingComponents { } impl PendingComponents { - pub fn num_blobs_expected(&self) -> usize { - self.bid.message.blob_kzg_commitments.len() + pub fn num_columns_required(&self, custody_context: &CustodyContext) -> usize + where + T: BeaconChainTypes, + { + if custody_context.data_columns_required_for_bid(&self.bid) { + custody_context.num_of_data_columns_to_sample(self.bid.epoch()) + } else { + 0 + } } /// Returns columns that have all cells present. @@ -59,7 +68,7 @@ impl PendingComponents { &mut self, kzg_verified_data_columns: &[KzgVerifiedCustodyDataColumn], ) { - let num_blobs_expected = self.num_blobs_expected(); + let num_blobs_expected = self.bid.num_blobs_expected(); for data_column in kzg_verified_data_columns { let data_column = data_column.as_data_column(); // The Vec-backed `PendingColumn` keys cells by index, so we have to allocate up to @@ -95,10 +104,13 @@ impl PendingComponents { } /// Returns `Some` if the envelope and all required data columns have been received. - pub fn make_available( + pub fn make_available( &self, - num_expected_columns: usize, - ) -> Result>, AvailabilityCheckError> { + custody_context: &CustodyContext, + ) -> Result>, AvailabilityCheckError> + where + T: BeaconChainTypes, + { // Check if the payload has been received and executed let Some(envelope) = &self.envelope else { return Ok(None); @@ -110,17 +122,24 @@ impl PendingComponents { payload_verification_outcome, } = envelope; - let columns = if self.num_blobs_expected() == 0 { - self.span.in_scope(|| { - debug!("Bid has no blobs, data is available"); - }); + let num_columns_required = self.num_columns_required(custody_context); + let columns = if num_columns_required == 0 { + if self.bid.num_blobs_expected() == 0 { + self.span.in_scope(|| { + debug!("Bid has no blobs, data is available"); + }); + } else { + self.span.in_scope(|| { + debug!("No data columns required for this epoch"); + }); + } vec![] } else { let columns = self.get_cached_data_columns(); - match columns.len().cmp(&num_expected_columns) { + match columns.len().cmp(&num_columns_required) { Ordering::Greater => { return Err(AvailabilityCheckError::Unexpected(format!( - "too many columns: got {} expected {num_expected_columns}", + "too many columns: got {} expected {num_columns_required}", columns.len() ))); } @@ -137,7 +156,8 @@ impl PendingComponents { } }; - let available_envelope = AvailableEnvelope::new(envelope.clone(), columns); + let available_envelope = + AvailableEnvelope::new(envelope.clone(), columns, &self.bid, custody_context)?; Ok(Some(AvailableExecutedEnvelope { envelope: available_envelope, @@ -160,12 +180,16 @@ impl PendingComponents { } } - pub fn status_str(&self, num_expected_columns: usize) -> String { + pub fn status_str(&self, custody_context: &CustodyContext) -> String + where + T: BeaconChainTypes, + { + let num_columns_required = self.num_columns_required(custody_context); format!( "envelope {}, data_columns {}/{}", self.envelope.is_some(), self.num_completed_columns(), - num_expected_columns + num_columns_required ) } } diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index deaae6cba5..ce51a82cec 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -226,29 +226,30 @@ pub fn test_da_checker( spec: Arc, node_custody_type: NodeCustodyType, ) -> DataAvailabilityChecker> { + let kzg = get_kzg(&spec); + let custody_context = test_custody_context(node_custody_type, spec.clone()); + DataAvailabilityChecker::new(kzg, custody_context, spec, true, false) + .expect("should initialise data availability checker") +} + +pub fn test_custody_context( + node_custody_type: NodeCustodyType, + spec: Arc, +) -> Arc>> { + let ordered_custody_column_indices = generate_data_column_indices_rand_order::(); + let complete_blob_backfill = false; let slot_clock = TestingSlotClock::new( Slot::new(0), Duration::from_secs(0), spec.get_slot_duration(), ); - let kzg = get_kzg(&spec); - let ordered_custody_column_indices = generate_data_column_indices_rand_order::(); - let custody_context = Arc::new(CustodyContext::new( + Arc::new(CustodyContext::new( node_custody_type, ordered_custody_column_indices, - &spec, - )); - let complete_blob_backfill = false; - DataAvailabilityChecker::new( - complete_blob_backfill, slot_clock, - kzg, - custody_context, + complete_blob_backfill, spec, - true, - false, - ) - .expect("should initialise data availability checker") + )) } pub struct Builder { @@ -3163,26 +3164,28 @@ where block: Arc>, ) -> RangeSyncBlock { let block_root = block_root.unwrap_or_else(|| get_block_root(&block)); - let is_gloas = block.fork_name_unchecked().gloas_enabled(); // For Gloas, kzg commitments live in the bid (`signed_execution_payload_bid`), so the // body's `blob_kzg_commitments()` accessor returns Err. `num_expected_blobs` already // handles both shapes. let has_blobs = block.num_expected_blobs() > 0; if !has_blobs { - return if is_gloas { + return if let Ok(bid) = block.message().body().signed_execution_payload_bid() { let envelope = self .chain .get_payload_envelope(&block_root) .unwrap() .map(Arc::new) - .map(|envelope| AvailableEnvelope::new(envelope, vec![])); + .map(|envelope| { + AvailableEnvelope::new(envelope, vec![], bid, &self.chain.custody_context) + }) + .transpose() + .unwrap(); RangeSyncBlock::new_gloas(block, envelope).unwrap() } else { RangeSyncBlock::new( block, AvailableBlockData::NoData, - &self.chain.data_availability_checker, - self.chain.spec.clone(), + &self.chain.custody_context, ) .unwrap() }; @@ -3197,23 +3200,26 @@ where .unwrap() .unwrap(); let custody_columns = columns.into_iter().collect::>(); - if is_gloas { + if let Ok(bid) = block.message().body().signed_execution_payload_bid() { let envelope = self .chain .get_payload_envelope(&block_root) .unwrap() .map(Arc::new) - .map(|envelope| AvailableEnvelope::new(envelope, custody_columns)); + .map(|envelope| { + AvailableEnvelope::new( + envelope, + custody_columns, + bid, + &self.chain.custody_context, + ) + }) + .transpose() + .unwrap(); RangeSyncBlock::new_gloas(block, envelope).unwrap() } else { let block_data = AvailableBlockData::new_with_data_columns(custody_columns); - RangeSyncBlock::new( - block, - block_data, - &self.chain.data_availability_checker, - self.chain.spec.clone(), - ) - .unwrap() + RangeSyncBlock::new(block, block_data, &self.chain.custody_context).unwrap() } } else { let blobs = self.chain.get_blobs(&block_root).unwrap().blobs(); @@ -3223,13 +3229,7 @@ where AvailableBlockData::NoData }; - RangeSyncBlock::new( - block, - block_data, - &self.chain.data_availability_checker, - self.chain.spec.clone(), - ) - .unwrap() + RangeSyncBlock::new(block, block_data, &self.chain.custody_context).unwrap() } } @@ -3239,7 +3239,7 @@ where block: Arc>>, blob_items: Option<(KzgProofs, BlobsList)>, ) -> Result, BlockError> { - if block.fork_name_unchecked().gloas_enabled() { + if let Ok(bid) = block.message().body().signed_execution_payload_bid() { let columns = blob_items .map(|_| generate_data_column_sidecars_from_block(&block, &self.spec)) .unwrap_or_default(); @@ -3248,13 +3248,17 @@ where .get_payload_envelope(&block.canonical_root()) .map_err(|e| BlockError::BeaconChainError(Box::new(e)))? .map(Arc::new) - .map(|envelope| AvailableEnvelope::new(envelope, columns)); + .map(|envelope| { + AvailableEnvelope::new(envelope, columns, bid, &self.chain.custody_context) + }) + .transpose() + .unwrap(); return RangeSyncBlock::new_gloas(block, envelope).map_err(BlockError::InternalError); } Ok(if self.spec.is_peer_das_enabled_for_epoch(block.epoch()) { let epoch = block.slot().epoch(E::slots_per_epoch()); - let sampling_columns = self.chain.sampling_columns_for_epoch(epoch); + let sampling_columns = self.chain.custody_context.sampling_columns_for_epoch(epoch); if blob_items.is_some_and(|(kzg_proofs, _)| !kzg_proofs.is_empty()) { // Note: this method ignores the actual custody columns and just take the first @@ -3265,18 +3269,12 @@ where .filter(|d| sampling_columns.contains(d.index())) .collect::>(); let block_data = AvailableBlockData::new_with_data_columns(columns); - RangeSyncBlock::new( - block, - block_data, - &self.chain.data_availability_checker, - self.chain.spec.clone(), - )? + RangeSyncBlock::new(block, block_data, &self.chain.custody_context)? } else { RangeSyncBlock::new( block, AvailableBlockData::NoData, - &self.chain.data_availability_checker, - self.chain.spec.clone(), + &self.chain.custody_context, )? } } else { @@ -3292,12 +3290,7 @@ where AvailableBlockData::NoData }; - RangeSyncBlock::new( - block, - block_data, - &self.chain.data_availability_checker, - self.chain.spec.clone(), - )? + RangeSyncBlock::new(block, block_data, &self.chain.custody_context)? }) } @@ -4007,6 +4000,7 @@ where let custody_columns = custody_columns_opt.unwrap_or_else(|| { let epoch = block.slot().epoch(E::slots_per_epoch()); self.chain + .custody_context .sampling_columns_for_epoch(epoch) .iter() .copied() diff --git a/beacon_node/beacon_chain/tests/block_verification.rs b/beacon_node/beacon_chain/tests/block_verification.rs index 94d4b3b9da..3269400c42 100644 --- a/beacon_node/beacon_chain/tests/block_verification.rs +++ b/beacon_node/beacon_chain/tests/block_verification.rs @@ -1,6 +1,7 @@ #![cfg(not(debug_assertions))] // TODO(gloas) we probably need similar test for payload envelope verification use beacon_chain::block_verification_types::{AsBlock, ExecutedBlock, LookupBlock, RangeSyncBlock}; +use beacon_chain::custody_context::CustodyContext; use beacon_chain::data_availability_checker::{AvailabilityCheckError, AvailableBlockData}; use beacon_chain::data_column_verification::CustodyDataColumn; use beacon_chain::payload_envelope_verification::AvailableEnvelope; @@ -10,7 +11,7 @@ use beacon_chain::{ custody_context::NodeCustodyType, test_utils::{ AttestationStrategy, BeaconChainHarness, BlockStrategy, EphemeralHarnessType, - MakeAttestationOptions, test_spec, + MakeAttestationOptions, generate_data_column_sidecars_from_block, test_spec, }, }; use beacon_chain::{ @@ -178,7 +179,7 @@ fn build_range_sync_block( where T: BeaconChainTypes, { - if block.fork_name_unchecked().gloas_enabled() { + if let Ok(bid) = block.message().body().signed_execution_payload_bid() { let columns = match data_sidecars { Some(DataSidecars::DataColumns(columns)) => columns .iter() @@ -186,20 +187,17 @@ where .collect::>(), Some(DataSidecars::Blobs(_)) | None => vec![], }; - let envelope = execution_envelope.map(|envelope| AvailableEnvelope::new(envelope, columns)); + let envelope = execution_envelope + .map(|envelope| AvailableEnvelope::new(envelope, columns, bid, &chain.custody_context)) + .transpose() + .unwrap(); return RangeSyncBlock::new_gloas(block, envelope).unwrap(); } match data_sidecars { Some(DataSidecars::Blobs(blobs)) => { let block_data = AvailableBlockData::new_with_blobs(blobs.clone()); - RangeSyncBlock::new( - block, - block_data, - &chain.data_availability_checker, - chain.spec.clone(), - ) - .unwrap() + RangeSyncBlock::new(block, block_data, &chain.custody_context).unwrap() } Some(DataSidecars::DataColumns(columns)) => { let block_data = AvailableBlockData::new_with_data_columns( @@ -208,21 +206,11 @@ where .map(|c| c.as_data_column().clone()) .collect::>(), ); - RangeSyncBlock::new( - block, - block_data, - &chain.data_availability_checker, - chain.spec.clone(), - ) - .unwrap() + RangeSyncBlock::new(block, block_data, &chain.custody_context).unwrap() + } + None => { + RangeSyncBlock::new(block, AvailableBlockData::NoData, &chain.custody_context).unwrap() } - None => RangeSyncBlock::new( - block, - AvailableBlockData::NoData, - &chain.data_availability_checker, - chain.spec.clone(), - ) - .unwrap(), } } @@ -522,8 +510,7 @@ async fn chain_segment_non_linear_parent_roots() { RangeSyncBlock::new( mutated_block, blocks[3].block_data().clone(), - &harness.chain.data_availability_checker, - harness.spec.clone(), + &harness.chain.custody_context, ) .unwrap() }; @@ -567,8 +554,7 @@ async fn chain_segment_non_linear_slots() { RangeSyncBlock::new( mutated_block, blocks[3].block_data().clone(), - &harness.chain.data_availability_checker, - harness.spec.clone(), + &harness.chain.custody_context, ) .unwrap() }; @@ -602,8 +588,7 @@ async fn chain_segment_non_linear_slots() { RangeSyncBlock::new( mutated_block, blocks[3].block_data().clone(), - &harness.chain.data_availability_checker, - harness.chain.spec.clone(), + &harness.chain.custody_context, ) .unwrap() }; @@ -1809,8 +1794,7 @@ async fn add_base_block_to_altair_chain() { let base_range_sync_block = RangeSyncBlock::new( Arc::new(base_block.clone()), AvailableBlockData::NoData, - &harness.chain.data_availability_checker, - harness.spec.clone(), + &harness.chain.custody_context, ) .unwrap(); assert!(matches!( @@ -1840,8 +1824,7 @@ async fn add_base_block_to_altair_chain() { RangeSyncBlock::new( Arc::new(base_block), AvailableBlockData::NoData, - &harness.chain.data_availability_checker, - harness.spec.clone() + &harness.chain.custody_context, ) .unwrap() ], @@ -1985,8 +1968,7 @@ async fn add_altair_block_to_base_chain() { RangeSyncBlock::new( Arc::new(altair_block), AvailableBlockData::NoData, - &harness.chain.data_availability_checker, - harness.spec.clone() + &harness.chain.custody_context, ) .unwrap() ], @@ -2205,8 +2187,7 @@ async fn import_duplicate_block_unrealized_justification() { RangeSyncBlock::new( block.clone(), AvailableBlockData::NoData, - &harness.chain.data_availability_checker, - harness.spec.clone(), + &harness.chain.custody_context, ) .unwrap() }; @@ -2284,8 +2265,12 @@ async fn import_execution_pending_block( } } -async fn make_gloas_range_sync_block_inputs() --> Option<(Arc>, SignedExecutionPayloadEnvelope)> { +async fn make_gloas_range_sync_block_inputs() -> Option<( + Arc>, + SignedExecutionPayloadEnvelope, + Arc>>, + DataColumnSidecarList, +)> { let spec = test_spec::(); if !spec.fork_name_at_slot::(Slot::new(1)).gloas_enabled() { return None; @@ -2304,16 +2289,37 @@ async fn make_gloas_range_sync_block_inputs() let state = harness.get_current_state(); let slot = harness.get_current_slot(); let ((block, _), envelope, _) = harness.make_block_with_envelope(state, slot).await; - Some((block, envelope.expect("gloas block should have envelope"))) + let custody_context = harness.chain.custody_context.clone(); + let columns = generate_data_column_sidecars_from_block(&block, &harness.chain.spec) + .into_iter() + .filter(|column| { + custody_context + .sampling_columns_for_epoch(block.epoch()) + .contains(column.index()) + }) + .collect(); + Some(( + block, + envelope.expect("gloas block should have envelope"), + custody_context, + columns, + )) } #[tokio::test] async fn range_sync_block_new_gloas_accepts_matching_envelope() { - let Some((block, envelope)) = make_gloas_range_sync_block_inputs().await else { + let Some((block, envelope, custody_context, columns)) = + make_gloas_range_sync_block_inputs().await + else { return; }; - - let available_envelope = AvailableEnvelope::new(Arc::new(envelope), vec![]); + let bid = block + .message() + .body() + .signed_execution_payload_bid() + .unwrap(); + let available_envelope = + AvailableEnvelope::new(Arc::new(envelope), columns, bid, &custody_context).unwrap(); let result = RangeSyncBlock::new_gloas(block, Some(available_envelope)); assert!( @@ -2325,7 +2331,7 @@ async fn range_sync_block_new_gloas_accepts_matching_envelope() { #[tokio::test] async fn range_sync_block_new_gloas_allows_missing_envelope() { - let Some((block, _)) = make_gloas_range_sync_block_inputs().await else { + let Some((block, _, _, _)) = make_gloas_range_sync_block_inputs().await else { return; }; @@ -2340,12 +2346,20 @@ async fn range_sync_block_new_gloas_allows_missing_envelope() { #[tokio::test] async fn range_sync_block_new_gloas_rejects_slot_mismatch() { - let Some((block, mut envelope)) = make_gloas_range_sync_block_inputs().await else { + let Some((block, mut envelope, custody_context, columns)) = + make_gloas_range_sync_block_inputs().await + else { return; }; envelope.message.payload.slot_number += 1; - let available_envelope = AvailableEnvelope::new(Arc::new(envelope), vec![]); + let bid = block + .message() + .body() + .signed_execution_payload_bid() + .unwrap(); + let available_envelope = + AvailableEnvelope::new(Arc::new(envelope), columns, bid, &custody_context).unwrap(); let result = RangeSyncBlock::new_gloas(block, Some(available_envelope)); assert!( @@ -2357,12 +2371,20 @@ async fn range_sync_block_new_gloas_rejects_slot_mismatch() { #[tokio::test] async fn range_sync_block_new_gloas_rejects_builder_index_mismatch() { - let Some((block, mut envelope)) = make_gloas_range_sync_block_inputs().await else { + let Some((block, mut envelope, custody_context, columns)) = + make_gloas_range_sync_block_inputs().await + else { return; }; envelope.message.builder_index += 1; - let available_envelope = AvailableEnvelope::new(Arc::new(envelope), vec![]); + let bid = block + .message() + .body() + .signed_execution_payload_bid() + .unwrap(); + let available_envelope = + AvailableEnvelope::new(Arc::new(envelope), columns, bid, &custody_context).unwrap(); let result = RangeSyncBlock::new_gloas(block, Some(available_envelope)); assert!( @@ -2374,12 +2396,20 @@ async fn range_sync_block_new_gloas_rejects_builder_index_mismatch() { #[tokio::test] async fn range_sync_block_new_gloas_rejects_block_hash_mismatch() { - let Some((block, mut envelope)) = make_gloas_range_sync_block_inputs().await else { + let Some((block, mut envelope, custody_context, columns)) = + make_gloas_range_sync_block_inputs().await + else { return; }; envelope.message.payload.block_hash = ExecutionBlockHash::repeat_byte(0x22); - let available_envelope = AvailableEnvelope::new(Arc::new(envelope), vec![]); + let bid = block + .message() + .body() + .signed_execution_payload_bid() + .unwrap(); + let available_envelope = + AvailableEnvelope::new(Arc::new(envelope), columns, bid, &custody_context).unwrap(); let result = RangeSyncBlock::new_gloas(block, Some(available_envelope)); assert!( @@ -2442,12 +2472,8 @@ async fn range_sync_block_construction_fails_with_wrong_blob_count() { let block_data = AvailableBlockData::new_with_blobs(wrong_blobs); // Try to create RpcBlock with wrong blob count - let result = RangeSyncBlock::new( - Arc::new(block), - block_data, - &harness.chain.data_availability_checker, - harness.chain.spec.clone(), - ); + let result = + RangeSyncBlock::new(Arc::new(block), block_data, &harness.chain.custody_context); // Should fail with MissingBlobs assert!( @@ -2523,8 +2549,7 @@ async fn range_sync_block_rejects_missing_custody_columns() { let result = RangeSyncBlock::new( Arc::new(block), block_data, - &harness.chain.data_availability_checker, - harness.chain.spec.clone(), + &harness.chain.custody_context, ); // Should fail with MissingCustodyColumns @@ -2599,6 +2624,7 @@ async fn rpc_block_allows_construction_past_da_boundary() { // Now verify the block is past the DA boundary let da_boundary = harness .chain + .custody_context .data_availability_boundary() .expect("DA boundary should be set"); assert!( @@ -2613,8 +2639,7 @@ async fn rpc_block_allows_construction_past_da_boundary() { let result = RangeSyncBlock::new( Arc::new(block), AvailableBlockData::NoData, - &harness.chain.data_availability_checker, - harness.chain.spec.clone(), + &harness.chain.custody_context, ); assert!( diff --git a/beacon_node/beacon_chain/tests/events.rs b/beacon_node/beacon_chain/tests/events.rs index 9f0b3675f3..537e8f40ee 100644 --- a/beacon_node/beacon_chain/tests/events.rs +++ b/beacon_node/beacon_chain/tests/events.rs @@ -43,7 +43,10 @@ async fn data_column_sidecar_event_on_process_gossip_data_column() { let mut random_sidecar = DataColumnSidecarGloas::arbitrary(&mut u).unwrap(); let epoch = slot.epoch(E::slots_per_epoch()); random_sidecar.slot = slot; - random_sidecar.index = harness.chain.sampling_columns_for_epoch(epoch)[0]; + random_sidecar.index = harness + .chain + .custody_context + .sampling_columns_for_epoch(epoch)[0]; // For gloas, the bid must be known, e.g. in the pending payload cache let mut bid = SignedExecutionPayloadBid::::empty(); @@ -58,7 +61,10 @@ async fn data_column_sidecar_event_on_process_gossip_data_column() { let mut random_sidecar = DataColumnSidecarFulu::arbitrary(&mut u).unwrap(); let epoch = slot.epoch(E::slots_per_epoch()); random_sidecar.signed_block_header.message.slot = slot; - random_sidecar.index = harness.chain.sampling_columns_for_epoch(epoch)[0]; + random_sidecar.index = harness + .chain + .custody_context + .sampling_columns_for_epoch(epoch)[0]; DataColumnSidecar::Fulu(random_sidecar) } }; diff --git a/beacon_node/beacon_chain/tests/store_tests.rs b/beacon_node/beacon_chain/tests/store_tests.rs index 1d7a5995de..7eb040fc71 100644 --- a/beacon_node/beacon_chain/tests/store_tests.rs +++ b/beacon_node/beacon_chain/tests/store_tests.rs @@ -3322,13 +3322,8 @@ async fn weak_subjectivity_sync_test( let (_, block, data) = clone_block(&available_blocks[0]).deconstruct(); let mut corrupt_block = (*block).clone(); *corrupt_block.signature_mut() = Signature::empty(); - AvailableBlock::new( - Arc::new(corrupt_block), - data, - &beacon_chain.data_availability_checker, - Arc::new(spec), - ) - .expect("available block") + AvailableBlock::new(Arc::new(corrupt_block), data, &beacon_chain.custody_context) + .expect("available block") }; // Importing the invalid batch should error. @@ -4878,7 +4873,10 @@ async fn test_column_da_boundary() { // The column da boundary should be the fulu fork epoch assert_eq!( - harness.chain.column_data_availability_boundary(), + harness + .chain + .custody_context + .column_data_availability_boundary(), Some(fulu_fork_epoch) ); } @@ -5293,6 +5291,7 @@ async fn test_custody_column_filtering_regular_node() { // Get custody columns for this epoch - regular nodes only store a subset let expected_custody_columns: HashSet<_> = harness .chain + .custody_context .custody_columns_for_epoch(Some(current_slot.epoch(E::slots_per_epoch()))) .iter() .copied() @@ -5374,8 +5373,6 @@ async fn test_missing_columns_after_cgc_change() { return; } - let custody_context = harness.chain.data_availability_checker.custody_context(); - harness.advance_slot(); harness .extend_chain( @@ -5397,7 +5394,10 @@ async fn test_missing_columns_after_cgc_change() { let epoch_after_increase = Epoch::new(num_epochs_before_increase + 2); let cgc_change_slot = epoch_before_increase.end_slot(E::slots_per_epoch()); - custody_context.register_validators(vec![(1, 32_000_000_000 * 9)], cgc_change_slot, &spec); + harness + .chain + .custody_context + .register_validators(vec![(1, 32_000_000_000 * 9)], cgc_change_slot); harness.advance_slot(); harness @@ -5444,8 +5444,6 @@ async fn test_safely_backfill_data_column_custody_info() { return; } - let custody_context = harness.chain.data_availability_checker.custody_context(); - harness.advance_slot(); harness .extend_chain( @@ -5461,7 +5459,10 @@ async fn test_safely_backfill_data_column_custody_info() { let cgc_change_slot = epoch_before_increase.end_slot(E::slots_per_epoch()); - custody_context.register_validators(vec![(1, 32_000_000_000 * 16)], cgc_change_slot, &spec); + harness + .chain + .custody_context + .register_validators(vec![(1, 32_000_000_000 * 16)], cgc_change_slot); let epoch_after_increase = (cgc_change_slot + effective_delay_slots).epoch(E::slots_per_epoch()); diff --git a/beacon_node/client/src/notifier.rs b/beacon_node/client/src/notifier.rs index bdb4228765..7b7c7beb9b 100644 --- a/beacon_node/client/src/notifier.rs +++ b/beacon_node/client/src/notifier.rs @@ -171,7 +171,9 @@ pub fn spawn_notifier( Ok(data_column_custody_info) => { if let Some(earliest_data_column_slot) = data_column_custody_info .and_then(|info| info.earliest_data_column_slot) - && let Some(da_boundary) = beacon_chain.get_column_da_boundary() + && let Some(da_boundary) = beacon_chain + .custody_context + .column_data_availability_boundary() { sync_distance = earliest_data_column_slot.saturating_sub( da_boundary.start_slot(T::EthSpec::slots_per_epoch()), @@ -295,7 +297,9 @@ pub fn spawn_notifier( let speed = speedo.slots_per_second(); let display_speed = speed.is_some_and(|speed| speed != 0.0); let est_time_in_secs = if let (Some(da_boundary_epoch), Some(original_slot)) = ( - beacon_chain.get_column_da_boundary(), + beacon_chain + .custody_context + .column_data_availability_boundary(), original_earliest_data_column_slot, ) { let target = original_slot.saturating_sub( diff --git a/beacon_node/http_api/src/beacon/execution_payload_envelopes.rs b/beacon_node/http_api/src/beacon/execution_payload_envelopes.rs index d058f66001..456d371c71 100644 --- a/beacon_node/http_api/src/beacon/execution_payload_envelopes.rs +++ b/beacon_node/http_api/src/beacon/execution_payload_envelopes.rs @@ -200,7 +200,7 @@ pub async fn publish_execution_payload_envelope( } let epoch = slot.epoch(T::EthSpec::slots_per_epoch()); - let sampling_column_indices = chain.sampling_columns_for_epoch(epoch); + let sampling_column_indices = chain.custody_context.sampling_columns_for_epoch(epoch); let sampling_columns = gossip_verified_columns .into_iter() .filter(|col| sampling_column_indices.contains(&col.index())) diff --git a/beacon_node/http_api/src/block_id.rs b/beacon_node/http_api/src/block_id.rs index 50d5c8d165..fe34863b77 100644 --- a/beacon_node/http_api/src/block_id.rs +++ b/beacon_node/http_api/src/block_id.rs @@ -602,7 +602,9 @@ mod tests { "precondition: test block must not be imported into fork choice yet" ); - let sampling_columns = chain.sampling_columns_for_epoch(block.epoch()); + let sampling_columns = chain + .custody_context + .sampling_columns_for_epoch(block.epoch()); let data_columns = generate_data_column_sidecars_from_block(&block, &chain.spec) .into_iter() .filter(|column| sampling_columns.contains(column.index())) @@ -615,8 +617,7 @@ mod tests { let available_block = AvailableBlock::new( block.clone(), AvailableBlockData::new_with_data_columns(data_columns), - &chain.data_availability_checker, - chain.spec.clone(), + &chain.custody_context, ) .unwrap(); diff --git a/beacon_node/http_api/src/custody.rs b/beacon_node/http_api/src/custody.rs index a43b55ceca..21004beb72 100644 --- a/beacon_node/http_api/src/custody.rs +++ b/beacon_node/http_api/src/custody.rs @@ -17,6 +17,7 @@ pub fn info( .map_err(|e| custom_server_error(format!("error reading DataColumnCustodyInfo: {e:?}")))?; let column_data_availability_boundary = chain + .custody_context .column_data_availability_boundary() .ok_or_else(|| custom_server_error("unreachable: Fulu should be enabled".to_string()))?; @@ -38,12 +39,13 @@ pub fn info( // Compute the custody columns and the CGC *at the earliest custodied slot*. The node might // have some columns prior to this, but this value is the most up-to-date view of the data the // node is custodying. - let custody_context = chain.data_availability_checker.custody_context(); - let custody_columns = custody_context - .custody_columns_for_epoch(Some(earliest_custodied_data_column_epoch), &chain.spec) + let custody_columns = chain + .custody_context + .custody_columns_for_epoch(Some(earliest_custodied_data_column_epoch)) .to_vec(); - let custody_group_count = custody_context - .custody_group_count_at_epoch(earliest_custodied_data_column_epoch, &chain.spec); + let custody_group_count = chain + .custody_context + .custody_group_count_at_epoch(earliest_custodied_data_column_epoch); Ok(CustodyInfo { earliest_custodied_data_column_slot, diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 7c0959acb9..8e31e88ff8 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -3179,10 +3179,11 @@ pub fn serve( .head_slot() .epoch(T::EthSpec::slots_per_epoch()) + 1; - let custody_context = chain.data_availability_checker.custody_context(); // Reset validator custody requirements to `effective_epoch` with the latest // cgc requiremnets. - custody_context.reset_validator_custody_requirements(effective_epoch); + chain + .custody_context + .reset_validator_custody_requirements(effective_epoch); // Update `DataColumnCustodyInfo` to reflect the custody change. chain.update_data_column_custody_info(Some( effective_epoch.start_slot(T::EthSpec::slots_per_epoch()), diff --git a/beacon_node/http_api/src/publish_blocks.rs b/beacon_node/http_api/src/publish_blocks.rs index e3e9839b2d..41bb515a2b 100644 --- a/beacon_node/http_api/src/publish_blocks.rs +++ b/beacon_node/http_api/src/publish_blocks.rs @@ -217,7 +217,7 @@ pub async fn publish_block>( warp_utils::reject::custom_server_error("unable to publish data column sidecars".into()) })?; let epoch = block.slot().epoch(T::EthSpec::slots_per_epoch()); - let sampling_columns_indices = chain.sampling_columns_for_epoch(epoch); + let sampling_columns_indices = chain.custody_context.sampling_columns_for_epoch(epoch); let sampling_columns = gossip_verified_columns .into_iter() .filter(|data_column| sampling_columns_indices.contains(&data_column.index())) diff --git a/beacon_node/http_api/src/validator/mod.rs b/beacon_node/http_api/src/validator/mod.rs index ee699b3adc..7f3d1cd721 100644 --- a/beacon_node/http_api/src/validator/mod.rs +++ b/beacon_node/http_api/src/validator/mod.rs @@ -855,9 +855,8 @@ pub fn post_validator_prepare_beacon_proposer( let current_slot = chain.slot().map_err(warp_utils::reject::unhandled_error)?; if let Some(cgc_change) = chain - .data_availability_checker - .custody_context() - .register_validators(validators_and_balances, current_slot, &chain.spec) + .custody_context + .register_validators(validators_and_balances, current_slot) { chain.update_data_column_custody_info(Some( cgc_change diff --git a/beacon_node/http_api/tests/broadcast_validation_tests.rs b/beacon_node/http_api/tests/broadcast_validation_tests.rs index 98629a1c5e..6d80344e94 100644 --- a/beacon_node/http_api/tests/broadcast_validation_tests.rs +++ b/beacon_node/http_api/tests/broadcast_validation_tests.rs @@ -2036,6 +2036,7 @@ fn get_custody_columns(tester: &InteractiveTester, slot: Slot) -> HashSet(slot_b) - .gloas_enabled(); - let reveal_block_b_payload = is_gloas && !should_re_org; - let (block_b, block_b_envelope, mut state_b) = if is_gloas { - let (block_b, block_b_envelope, state_b) = harness - .make_block_with_envelope_on(state_a.clone(), slot_b, head_parent_payload_status) - .await; - let block_b_envelope = if reveal_block_b_payload { - block_b_envelope - } else { - None - }; - (block_b, block_b_envelope, state_b) + let reveal_block_b_payload = !should_re_org; + let (block_b, block_b_envelope, mut state_b) = harness + .make_block_with_envelope_on(state_a.clone(), slot_b, head_parent_payload_status) + .await; + let block_b_envelope = if reveal_block_b_payload { + block_b_envelope } else { - let (block_b, state_b) = harness.make_block(state_a.clone(), slot_b).await; - (block_b, None, state_b) + None }; let state_b_root = state_b.canonical_root().unwrap(); let block_b_root = block_b.0.canonical_root(); @@ -735,13 +723,8 @@ pub async fn proposer_boost_re_org_test( let randao_reveal = harness .sign_randao_reveal(&state_b, proposer_index, slot_c) .into(); - let is_gloas = harness - .chain - .spec - .fork_name_at_slot::(slot_c) - .gloas_enabled(); - let (block_c, block_c_blobs) = if is_gloas { + let (block_c, block_c_blobs) = { let (response, _) = tester .client .get_validator_blocks_v4::(slot_c, &randao_reveal, None, None, None, None) @@ -751,84 +734,51 @@ pub async fn proposer_boost_re_org_test( Arc::new(harness.sign_beacon_block(response.data, &state_b)), None, ) - } else { - let (unsigned_block_type, _) = tester - .client - .get_validator_blocks_v3::(slot_c, &randao_reveal, None, None, None) - .await - .unwrap(); - - let (unsigned_block_c, block_c_blobs) = match unsigned_block_type.data { - ProduceBlockV3Response::Full(unsigned_block_contents_c) => { - unsigned_block_contents_c.deconstruct() - } - ProduceBlockV3Response::Blinded(_) => { - panic!("Should not be a blinded block"); - } - }; - ( - Arc::new(harness.sign_beacon_block(unsigned_block_c, &state_b)), - block_c_blobs, - ) }; // Post-Gloas the execution payload is decoupled from the beacon block: the payload hash // lives in the execution payload bid, and the payload timestamp is derived from the slot. let exec_block_hash = |block: BeaconBlockRef| -> ExecutionBlockHash { - if is_gloas { - block - .body() - .signed_execution_payload_bid() - .unwrap() - .message - .block_hash - } else { - block.execution_payload().unwrap().block_hash() - } + block + .body() + .signed_execution_payload_bid() + .unwrap() + .message + .block_hash }; let exec_parent_hash = |block: BeaconBlockRef| -> ExecutionBlockHash { - if is_gloas { - block - .body() - .signed_execution_payload_bid() - .unwrap() - .message - .parent_block_hash - } else { - block.execution_payload().unwrap().parent_hash() - } + block + .body() + .signed_execution_payload_bid() + .unwrap() + .message + .parent_block_hash }; let block_a_exec_hash = exec_block_hash(block_a.0.message()); let block_b_exec_hash = exec_block_hash(block_b.0.message()); - if is_gloas { - assert_eq!( - block_b.0.is_parent_block_full(block_a_exec_hash), - head_parent_payload_status == PayloadStatus::Full - ); - } + assert_eq!( + block_b.0.is_parent_block_full(block_a_exec_hash), + head_parent_payload_status == PayloadStatus::Full + ); if should_re_org { // Block C should build on A. assert_eq!(block_c.parent_root(), Hash256::from(block_a_root)); - if is_gloas { - assert_eq!( - block_c.is_parent_block_full(block_a_exec_hash), - expected_parent_payload_status == PayloadStatus::Full - ); - } + assert_eq!( + block_c.is_parent_block_full(block_a_exec_hash), + expected_parent_payload_status == PayloadStatus::Full + ); } else { // Block C should build on B. assert_eq!(block_c.parent_root(), block_b_root); - if is_gloas { - assert_eq!( - block_c.is_parent_block_full(block_b_exec_hash), - expected_parent_payload_status == PayloadStatus::Full - ); - } + assert_eq!( + block_c.is_parent_block_full(block_b_exec_hash), + expected_parent_payload_status == PayloadStatus::Full + ); } // Applying block C should cause it to become head regardless (re-org or continuation). @@ -844,11 +794,7 @@ pub async fn proposer_boost_re_org_test( // Check the fork choice updates that were sent. let forkchoice_updates = forkchoice_updates.lock(); - let block_c_timestamp = if is_gloas { - harness.chain.slot_clock.start_of(slot_c).unwrap().as_secs() - } else { - block_c.message().execution_payload().unwrap().timestamp() - }; + let block_c_timestamp = harness.chain.slot_clock.start_of(slot_c).unwrap().as_secs(); // If we re-orged then no fork choice update for B should have been sent. assert_eq!( @@ -872,8 +818,8 @@ pub async fn proposer_boost_re_org_test( .first_update_with_payload_attributes( c_parent_hash, block_c_timestamp, - is_gloas.then(|| block_c.parent_root()), - is_gloas.then(|| slot_c.as_u64()), + Some(block_c.parent_root()), + Some(slot_c.as_u64()), ) .unwrap(); let payload_attribs = first_update.payload_attributes.as_ref().unwrap(); @@ -887,12 +833,10 @@ pub async fn proposer_boost_re_org_test( } else { state_b.clone() }; - let expected_withdrawals = if is_gloas - && matches!( - expected_first_update_lookahead, - ExpectedFirstUpdateLookahead::BlockProduction - ) - && expected_parent_payload_status == PayloadStatus::Empty + let expected_withdrawals = if matches!( + expected_first_update_lookahead, + ExpectedFirstUpdateLookahead::BlockProduction + ) && expected_parent_payload_status == PayloadStatus::Empty { parent_state_advanced .payload_expected_withdrawals() diff --git a/beacon_node/http_api/tests/interactive_tests.rs b/beacon_node/http_api/tests/interactive_tests.rs index d2a28da0f5..437da7d4ea 100644 --- a/beacon_node/http_api/tests/interactive_tests.rs +++ b/beacon_node/http_api/tests/interactive_tests.rs @@ -1262,8 +1262,7 @@ async fn lighthouse_restart_custody_backfill() { let max_cgc = spec.number_of_custody_groups; let num_blocks = 2 * E::slots_per_epoch(); - - let custody_context = harness.chain.data_availability_checker.custody_context(); + let custody_context = &harness.chain.custody_context; harness.advance_slot(); harness @@ -1276,7 +1275,7 @@ async fn lighthouse_restart_custody_backfill() { ) .await; - let cgc_at_head = custody_context.custody_group_count_at_head(spec); + let cgc_at_head = custody_context.custody_group_count_at_head(); let earliest_data_column_epoch = harness.chain.earliest_custodied_data_column_epoch(); assert_eq!(cgc_at_head, max_cgc); @@ -1286,9 +1285,9 @@ async fn lighthouse_restart_custody_backfill() { .update_and_backfill_custody_count_at_epoch(harness.chain.epoch().unwrap(), cgc_at_head); client.post_lighthouse_custody_backfill().await.unwrap(); - let cgc_at_head = custody_context.custody_group_count_at_head(spec); + let cgc_at_head = custody_context.custody_group_count_at_head(); let cgc_at_previous_epoch = - custody_context.custody_group_count_at_epoch(harness.chain.epoch().unwrap() - 1, spec); + custody_context.custody_group_count_at_epoch(harness.chain.epoch().unwrap() - 1); let earliest_data_column_epoch = harness.chain.earliest_custodied_data_column_epoch(); // `DataColumnCustodyInfo` should have been updated to the head epoch diff --git a/beacon_node/network/src/metrics.rs b/beacon_node/network/src/metrics.rs index 1a664662df..07e6d7fdb2 100644 --- a/beacon_node/network/src/metrics.rs +++ b/beacon_node/network/src/metrics.rs @@ -665,22 +665,6 @@ pub static BEACON_BLOB_DELAY_FULL_VERIFICATION: LazyLock> = Laz ) }); -pub static BEACON_BLOB_RPC_SLOT_START_DELAY_TIME: LazyLock> = LazyLock::new( - || { - try_create_histogram_with_buckets( - "beacon_blob_rpc_slot_start_delay_time", - "Duration between when a blob is received over rpc and the start of the slot it belongs to.", - // Create a custom bucket list for greater granularity in block delay - Ok(vec![ - 0.1, 0.2, 0.3, 0.4, 0.5, 0.75, 1.0, 1.25, 1.5, 1.75, 2.0, 2.5, 3.0, 3.5, 4.0, 5.0, - 6.0, 7.0, 8.0, 9.0, 10.0, 15.0, 20.0, - ]), // NOTE: Previous values, which we may want to switch back to. - // [0.1, 0.2, 0.5, 1, 2, 5, 10, 20, 50] - //decimal_buckets(-1,2) - ) - }, -); - /* * Light client update reprocessing queue metrics. */ diff --git a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs index 451ef978e5..7957b56cbf 100644 --- a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs @@ -1409,12 +1409,8 @@ impl NetworkBeaconProcessor { async fn check_reconstruction_trigger(self: &Arc, slot: Slot, block_root: &Hash256) { if self .chain - .data_availability_checker - .custody_context() - .should_attempt_reconstruction( - slot.epoch(T::EthSpec::slots_per_epoch()), - &self.chain.spec, - ) + .custody_context + .should_attempt_reconstruction(slot.epoch(T::EthSpec::slots_per_epoch())) { // Instead of triggering reconstruction immediately, schedule it to be run. If // another column arrives, it either completes availability or pushes diff --git a/beacon_node/network/src/network_beacon_processor/mod.rs b/beacon_node/network/src/network_beacon_processor/mod.rs index c5023ed5f4..8066d8c689 100644 --- a/beacon_node/network/src/network_beacon_processor/mod.rs +++ b/beacon_node/network/src/network_beacon_processor/mod.rs @@ -528,15 +528,11 @@ impl NetworkBeaconProcessor { self: &Arc, block_root: Hash256, block: LookupBlock, - seen_timestamp: Duration, process_type: BlockProcessType, ) -> Result<(), Error> { - let process_fn = self.clone().generate_lookup_beacon_block_process_fn( - block_root, - block, - seen_timestamp, - process_type, - ); + let process_fn = + self.clone() + .generate_lookup_beacon_block_process_fn(block_root, block, process_type); self.try_send(BeaconWorkEvent { drop_during_sync: false, work: Work::RpcBlock { @@ -552,14 +548,13 @@ impl NetworkBeaconProcessor { self: &Arc, block_root: Hash256, envelope: Arc>, - seen_timestamp: Duration, process_type: BlockProcessType, ) -> Result<(), Error> { let s = self.clone(); self.try_send(BeaconWorkEvent { drop_during_sync: false, work: Work::RpcEnvelope(Box::pin(async move { - s.process_lookup_envelope(block_root, envelope, seen_timestamp, process_type) + s.process_lookup_envelope(block_root, envelope, process_type) .await; })), }) @@ -571,20 +566,14 @@ impl NetworkBeaconProcessor { self: &Arc, block_root: Hash256, custody_columns: DataColumnSidecarList, - seen_timestamp: Duration, process_type: BlockProcessType, ) -> Result<(), Error> { let s = self.clone(); self.try_send(BeaconWorkEvent { drop_during_sync: false, work: Work::RpcCustodyColumn(Box::pin(async move { - s.process_rpc_custody_columns( - block_root, - custody_columns, - seen_timestamp, - process_type, - ) - .await; + s.process_rpc_custody_columns(block_root, custody_columns, process_type) + .await; })), }) } @@ -921,7 +910,7 @@ impl NetworkBeaconProcessor { return; } let epoch = header.slot().epoch(T::EthSpec::slots_per_epoch()); - let custody_columns = self.chain.sampling_columns_for_epoch(epoch); + let custody_columns = self.chain.custody_context.sampling_columns_for_epoch(epoch); let self_cloned = self.clone(); let publish_fn = move |columns: Vec>| { if publish_blobs { @@ -996,7 +985,7 @@ impl NetworkBeaconProcessor { return; }; let epoch = header.slot().epoch(T::EthSpec::slots_per_epoch()); - let custody_columns = self.chain.sampling_columns_for_epoch(epoch); + let custody_columns = self.chain.custody_context.sampling_columns_for_epoch(epoch); let columns = assembler.get_columns_and_mark_as_local_fetched(block_root, &header); let mut present_indices: HashSet = HashSet::with_capacity(columns.len()); diff --git a/beacon_node/network/src/network_beacon_processor/rpc_methods.rs b/beacon_node/network/src/network_beacon_processor/rpc_methods.rs index 37a6f3779a..fc451ebc05 100644 --- a/beacon_node/network/src/network_beacon_processor/rpc_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/rpc_methods.rs @@ -791,7 +791,7 @@ impl NetworkBeaconProcessor { ) -> Result<(), (RpcErrorResponse, &'static str)> { let mut send_data_column_count = 0; // Only attempt lookups for columns the node has advertised and is responsible for maintaining custody of. - let available_columns = self.chain.custody_columns_for_epoch(None); + let available_columns = self.chain.custody_context.custody_columns_for_epoch(None); for data_column_ids_by_root in request.data_column_ids.as_slice() { let indices_to_retrieve = data_column_ids_by_root @@ -1595,13 +1595,14 @@ impl NetworkBeaconProcessor { req.count }; - let data_availability_boundary_slot = match self.chain.data_availability_boundary() { - Some(boundary) => boundary.start_slot(T::EthSpec::slots_per_epoch()), - None => { - debug!("Deneb fork is disabled"); - return Err((RpcErrorResponse::InvalidRequest, "Deneb fork is disabled")); - } - }; + let data_availability_boundary_slot = + match self.chain.custody_context.data_availability_boundary() { + Some(boundary) => boundary.start_slot(T::EthSpec::slots_per_epoch()), + None => { + debug!("Deneb fork is disabled"); + return Err((RpcErrorResponse::InvalidRequest, "Deneb fork is disabled")); + } + }; let oldest_blob_slot = self .chain @@ -1745,14 +1746,17 @@ impl NetworkBeaconProcessor { let request_start_slot = Slot::from(req.start_slot); - let column_data_availability_boundary_slot = - match self.chain.column_data_availability_boundary() { - Some(boundary) => boundary.start_slot(T::EthSpec::slots_per_epoch()), - None => { - debug!("Fulu fork is disabled"); - return Err((RpcErrorResponse::InvalidRequest, "Fulu fork is disabled")); - } - }; + let column_data_availability_boundary_slot = match self + .chain + .custody_context + .column_data_availability_boundary() + { + Some(boundary) => boundary.start_slot(T::EthSpec::slots_per_epoch()), + None => { + debug!("Fulu fork is disabled"); + return Err((RpcErrorResponse::InvalidRequest, "Fulu fork is disabled")); + } + }; let earliest_custodied_data_column_slot = match self.chain.earliest_custodied_data_column_epoch() { @@ -1798,6 +1802,7 @@ impl NetworkBeaconProcessor { let request_start_epoch = request_start_slot.epoch(T::EthSpec::slots_per_epoch()); let available_columns = self .chain + .custody_context .custody_columns_for_epoch(Some(request_start_epoch)); let indices_to_retrieve = req @@ -1916,9 +1921,8 @@ impl NetworkBeaconProcessor { let non_custody_indices = { let custody_columns = self .chain - .data_availability_checker - .custody_context() - .custody_columns_for_epoch(epoch_opt, &self.chain.spec); + .custody_context + .custody_columns_for_epoch(epoch_opt); requested_indices .iter() .filter(|subnet_id| !custody_columns.contains(subnet_id)) diff --git a/beacon_node/network/src/network_beacon_processor/sync_methods.rs b/beacon_node/network/src/network_beacon_processor/sync_methods.rs index b9e07743eb..c376f1844b 100644 --- a/beacon_node/network/src/network_beacon_processor/sync_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/sync_methods.rs @@ -14,7 +14,7 @@ use beacon_chain::data_availability_checker::{ use beacon_chain::historical_data_columns::HistoricalDataColumnError; use beacon_chain::{ AvailabilityProcessingStatus, BeaconChainTypes, BlockError, ChainSegmentResult, - HistoricalBlockError, NotifyExecutionLayer, validator_monitor::get_slot_delay_ms, + HistoricalBlockError, NotifyExecutionLayer, }; use beacon_processor::{ AsyncFn, BlockingFn, DuplicateCache, @@ -26,7 +26,6 @@ use lighthouse_network::PeerId; use lighthouse_network::service::api_types::CustodyBackfillBatchId; use logging::crit; use std::sync::Arc; -use std::time::Duration; use tracing::{debug, debug_span, error, info, instrument, warn}; use types::{BlockImportSource, DataColumnSidecarList, Epoch, ExecutionBlockHash, Hash256}; @@ -56,19 +55,12 @@ impl NetworkBeaconProcessor { self: Arc, block_root: Hash256, block: LookupBlock, - seen_timestamp: Duration, process_type: BlockProcessType, ) -> AsyncFn { let process_fn = async move { let duplicate_cache = self.duplicate_cache.clone(); - self.process_lookup_block( - block_root, - block, - seen_timestamp, - process_type, - duplicate_cache, - ) - .await; + self.process_lookup_block(block_root, block, process_type, duplicate_cache) + .await; }; Box::pin(process_fn) } @@ -78,14 +70,12 @@ impl NetworkBeaconProcessor { self: Arc, block_root: Hash256, block: LookupBlock, - seen_timestamp: Duration, process_type: BlockProcessType, ) -> (AsyncFn, BlockingFn) { // An async closure which will import the block. let process_fn = self.clone().generate_lookup_beacon_block_process_fn( block_root, block, - seen_timestamp, process_type.clone(), ); // A closure which will ignore the block. @@ -119,7 +109,6 @@ impl NetworkBeaconProcessor { self: Arc>, block_root: Hash256, block: LookupBlock, - seen_timestamp: Duration, process_type: BlockProcessType, duplicate_cache: DuplicateCache, ) { @@ -133,12 +122,9 @@ impl NetworkBeaconProcessor { ); // Send message to work reprocess queue to retry the block - let (process_fn, ignore_fn) = self.clone().generate_lookup_beacon_block_fns( - block_root, - block, - seen_timestamp, - process_type, - ); + let (process_fn, ignore_fn) = + self.clone() + .generate_lookup_beacon_block_fns(block_root, block, process_type); let reprocess_msg = ReprocessQueueMessage::RpcBlock(QueuedRpcBlock { beacon_block_root: block_root, process_fn, @@ -206,13 +192,6 @@ impl NetworkBeaconProcessor { "Failed to inform block import" ); }; - self.chain.block_times_cache.write().set_time_observed( - *hash, - *slot, - seen_timestamp, - None, - None, - ); self.chain.recompute_head_at_current_slot().await; } @@ -254,7 +233,6 @@ impl NetworkBeaconProcessor { self: Arc>, block_root: Hash256, custody_columns: DataColumnSidecarList, - seen_timestamp: Duration, process_type: BlockProcessType, ) { // custody_columns must always have at least one element @@ -262,13 +240,6 @@ impl NetworkBeaconProcessor { return; }; - if let Ok(current_slot) = self.chain.slot() - && current_slot == slot - { - let delay = get_slot_delay_ms(seen_timestamp, slot, &self.chain.slot_clock); - metrics::observe_duration(&metrics::BEACON_BLOB_RPC_SLOT_START_DELAY_TIME, delay); - } - let mut indices = custody_columns .iter() .map(|d| *d.index()) @@ -332,7 +303,6 @@ impl NetworkBeaconProcessor { self: Arc>, block_root: Hash256, envelope: Arc>, - _seen_timestamp: Duration, process_type: BlockProcessType, ) { debug!( diff --git a/beacon_node/network/src/network_beacon_processor/tests.rs b/beacon_node/network/src/network_beacon_processor/tests.rs index c3cf1b8e1f..f5c557a130 100644 --- a/beacon_node/network/src/network_beacon_processor/tests.rs +++ b/beacon_node/network/src/network_beacon_processor/tests.rs @@ -340,7 +340,7 @@ impl TestRig { if chain.spec.is_peer_das_enabled_for_epoch(block.epoch()) { let kzg = get_kzg(&chain.spec); let epoch = block.slot().epoch(E::slots_per_epoch()); - let sampling_indices = chain.sampling_columns_for_epoch(epoch); + let sampling_indices = chain.custody_context.sampling_columns_for_epoch(epoch); let custody_columns: DataColumnSidecarList = blobs_to_data_column_sidecars( &blobs.iter().collect_vec(), kzg_proofs.clone().into_iter().collect_vec(), @@ -422,7 +422,6 @@ impl TestRig { .send_lookup_beacon_block( block_root, LookupBlock::new(self.next_block.clone()), - std::time::Duration::default(), BlockProcessType::SingleBlock { id: 0 }, ) .unwrap(); @@ -434,7 +433,6 @@ impl TestRig { .send_lookup_beacon_block( block_root, LookupBlock::new(self.next_block.clone()), - std::time::Duration::default(), BlockProcessType::SingleBlock { id: 1 }, ) .unwrap(); @@ -446,7 +444,6 @@ impl TestRig { .send_rpc_custody_columns( self.next_block.canonical_root(), data_columns, - Duration::default(), BlockProcessType::SingleCustodyColumn(1), ) .unwrap(); @@ -1839,6 +1836,7 @@ async fn test_data_columns_by_range_request_only_returns_requested_columns() { let all_custody_columns = rig .chain + .custody_context .sampling_columns_for_epoch(rig.chain.epoch().unwrap()); let available_columns: Vec = all_custody_columns.to_vec(); @@ -1900,7 +1898,10 @@ async fn test_data_columns_by_range_no_duplicates_with_skip_slots() { let skip_slots: HashSet = [5, 6].into_iter().collect(); let mut rig = TestRig::new_with_skip_slots(128, &skip_slots).await; - let all_custody_columns = rig.chain.custody_columns_for_epoch(Some(Epoch::new(0))); + let all_custody_columns = rig + .chain + .custody_context + .custody_columns_for_epoch(Some(Epoch::new(0))); let requested_column = vec![all_custody_columns[0]]; // Request a range that spans the skip slots (slots 0 through 9). diff --git a/beacon_node/network/src/router.rs b/beacon_node/network/src/router.rs index 315ec9387d..d83068e337 100644 --- a/beacon_node/network/src/router.rs +++ b/beacon_node/network/src/router.rs @@ -659,7 +659,6 @@ impl Router { peer_id, sync_request_id, beacon_block, - seen_timestamp: self.chain.slot_clock.now_duration().unwrap_or_default(), }); } @@ -679,7 +678,6 @@ impl Router { peer_id, sync_request_id, blob_sidecar, - seen_timestamp: self.chain.slot_clock.now_duration().unwrap_or_default(), }); } else { crit!("All blobs by range responses should belong to sync"); @@ -716,7 +714,6 @@ impl Router { peer_id, sync_request_id, beacon_block, - seen_timestamp: self.chain.slot_clock.now_duration().unwrap_or_default(), }); } @@ -750,7 +747,6 @@ impl Router { sync_request_id, peer_id, data_column, - seen_timestamp: self.chain.slot_clock.now_duration().unwrap_or_default(), }); } @@ -770,7 +766,6 @@ impl Router { peer_id, sync_request_id, data_column, - seen_timestamp: self.chain.slot_clock.now_duration().unwrap_or_default(), }); } else { crit!("All data columns by range responses should belong to sync"); @@ -796,7 +791,6 @@ impl Router { sync_request_id, peer_id, envelope, - seen_timestamp: self.chain.slot_clock.now_duration().unwrap_or_default(), }); } @@ -819,7 +813,6 @@ impl Router { sync_request_id, peer_id, envelope, - seen_timestamp: self.chain.slot_clock.now_duration().unwrap_or_default(), }); } diff --git a/beacon_node/network/src/service.rs b/beacon_node/network/src/service.rs index ba4aada352..cef3d5859e 100644 --- a/beacon_node/network/src/service.rs +++ b/beacon_node/network/src/service.rs @@ -294,10 +294,7 @@ impl NetworkService { let (mut libp2p, network_globals) = Network::new( executor.clone(), service_context, - beacon_chain - .data_availability_checker - .custody_context() - .custody_group_count_at_head(&beacon_chain.spec), + beacon_chain.custody_context.custody_group_count_at_head(), local_keypair, ) .await?; diff --git a/beacon_node/network/src/sync/backfill_sync/mod.rs b/beacon_node/network/src/sync/backfill_sync/mod.rs index c8bb17243e..edf358976a 100644 --- a/beacon_node/network/src/sync/backfill_sync/mod.rs +++ b/beacon_node/network/src/sync/backfill_sync/mod.rs @@ -352,11 +352,14 @@ impl BackFillSync { debug!(?batch_id, msg, "Blob peer failure"); } CouplingError::EnvelopePeerFailure(msg) => { - debug!(?batch_id, msg, "Envelope peer failure"); + debug!(?batch_id, ?msg, "Envelope peer failure"); } CouplingError::InternalError(msg) => { error!(?batch_id, msg, "Block components coupling internal error"); } + CouplingError::AvailabilityCheckError(err) => { + error!(?batch_id, ?err, "Availability check error"); + } } } // A batch could be retried without the peer failing the request (disconnecting/ diff --git a/beacon_node/network/src/sync/block_lookups/mod.rs b/beacon_node/network/src/sync/block_lookups/mod.rs index d403382e9e..ef9807f037 100644 --- a/beacon_node/network/src/sync/block_lookups/mod.rs +++ b/beacon_node/network/src/sync/block_lookups/mod.rs @@ -114,6 +114,8 @@ pub(crate) struct BlockLookupSummary { pub block_root: Hash256, /// List of peers that claim to have imported this set of block components. pub peers: Vec, + /// Whether the lookup expects some future event to make progress. + pub is_awaiting_event: bool, } impl BlockLookups { @@ -150,6 +152,7 @@ impl BlockLookups { id: *id, block_root: l.block_root(), peers: l.all_peers(), + is_awaiting_event: l.is_awaiting_event(), }) .collect() } 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 f03eed1638..09ffa50f9b 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 @@ -355,12 +355,16 @@ impl SingleBlockLookup { self.awaiting_parent.is_some() || self.block_request.state.is_awaiting_event() || match &self.data_request { - DataRequest::WaitingForBlock => true, + // Not awaiting an event itself; it's blocked on the block request, already covered + // by the `block_request` term above. Returning `true` kept a peerless lookup parked + // in `AwaitingDownload` from being pruned, so it got stuck. + DataRequest::WaitingForBlock => false, DataRequest::Request { state, .. } => state.is_awaiting_event(), DataRequest::NoData => false, } || match &self.payload_request { - PayloadRequest::WaitingForBlock => true, + // See `data_request` above: not awaiting an event itself, the block request covers it. + PayloadRequest::WaitingForBlock => false, PayloadRequest::Request { state, .. } => state.is_awaiting_event(), PayloadRequest::PreGloas => false, } @@ -381,7 +385,7 @@ impl SingleBlockLookup { if self.awaiting_parent.is_none() && let Some(data) = self.block_request.state.maybe_start_processing() { - cx.send_block_for_processing(self.id, self.block_root, data.value, data.seen_timestamp) + cx.send_block_for_processing(self.id, self.block_root, data.value) .map_err(LookupRequestError::SendFailedProcessor)?; } @@ -390,12 +394,11 @@ impl SingleBlockLookup { match &mut self.data_request { DataRequest::WaitingForBlock => { if let Some(block) = self.block_request.state.peek_downloaded_data() { - let block_epoch = block - .slot() - .epoch(::EthSpec::slots_per_epoch()); - self.data_request = if block.num_expected_blobs() == 0 { - DataRequest::NoData - } else if cx.chain.should_fetch_custody_columns(block_epoch) { + self.data_request = if cx + .chain + .custody_context + .data_columns_required_for_block(block) + { DataRequest::Request { slot: block.slot(), peers: self.get_data_peers(block.payload_bid_block_hash().ok()), @@ -422,7 +425,6 @@ impl SingleBlockLookup { self.id, self.block_root, data.value, - data.seen_timestamp, BlockProcessType::SingleCustodyColumn(self.id), ) .map_err(LookupRequestError::SendFailedProcessor)?; @@ -463,7 +465,6 @@ impl SingleBlockLookup { cx.send_payload_for_processing( self.block_root, data.value, - data.seen_timestamp, BlockProcessType::SinglePayloadEnvelope(self.id), ) .map_err(LookupRequestError::SendFailedProcessor)?; @@ -711,17 +712,12 @@ impl SingleBlockLookup { #[derive(Debug, Clone)] pub struct DownloadResult { pub value: T, - pub seen_timestamp: Duration, pub peer_group: PeerGroup, } impl DownloadResult { - pub fn new(value: T, peer_group: PeerGroup, seen_timestamp: Duration) -> Self { - Self { - value, - seen_timestamp, - peer_group, - } + pub fn new(value: T, peer_group: PeerGroup) -> Self { + Self { value, peer_group } } } diff --git a/beacon_node/network/src/sync/block_sidecar_coupling.rs b/beacon_node/network/src/sync/block_sidecar_coupling.rs index 93c0699ded..001fabb704 100644 --- a/beacon_node/network/src/sync/block_sidecar_coupling.rs +++ b/beacon_node/network/src/sync/block_sidecar_coupling.rs @@ -1,8 +1,9 @@ +use beacon_chain::data_availability_checker::AvailabilityCheckError; use beacon_chain::payload_envelope_verification::AvailableEnvelope; use beacon_chain::{ BeaconChainTypes, block_verification_types::{AvailableBlockData, RangeSyncBlock}, - data_availability_checker::DataAvailabilityChecker, + custody_context::CustodyContext, data_column_verification::CustodyDataColumn, get_block_root, }; @@ -81,6 +82,13 @@ pub enum CouplingError { }, BlobPeerFailure(String), EnvelopePeerFailure(String), + AvailabilityCheckError(AvailabilityCheckError), +} + +impl From for CouplingError { + fn from(e: AvailabilityCheckError) -> Self { + CouplingError::AvailabilityCheckError(e) + } } impl RangeBlockComponentsRequest { @@ -221,7 +229,7 @@ impl RangeBlockComponentsRequest { /// Returns `Some(Err(_))` if there are issues coupling blocks with their data. pub fn responses( &mut self, - da_checker: Arc>, + custody_context: &CustodyContext, spec: Arc, ) -> Option>, CouplingError>> where @@ -241,7 +249,7 @@ impl RangeBlockComponentsRequest { RangeBlockDataRequest::NoData => Some(Self::responses_with_blobs( blocks.to_vec(), vec![], - da_checker, + custody_context, spec, )), RangeBlockDataRequest::Blobs(request) => { @@ -251,7 +259,7 @@ impl RangeBlockComponentsRequest { Some(Self::responses_with_blobs( blocks.to_vec(), blobs.to_vec(), - da_checker, + custody_context, spec, )) } @@ -294,8 +302,7 @@ impl RangeBlockComponentsRequest { column_to_peer_id, expected_custody_columns, *attempt, - da_checker, - spec, + custody_context, payload_envelopes, ); @@ -321,7 +328,7 @@ impl RangeBlockComponentsRequest { fn responses_with_blobs( blocks: Vec>>, blobs: Vec>>, - da_checker: Arc>, + custody_context: &CustodyContext, spec: Arc, ) -> Result>, CouplingError> where @@ -370,7 +377,7 @@ impl RangeBlockComponentsRequest { })?; let block_data = AvailableBlockData::new_with_blobs(blobs); responses.push( - RangeSyncBlock::new(block, block_data, &da_checker, spec.clone()) + RangeSyncBlock::new(block, block_data, custody_context) .map_err(|e| CouplingError::BlobPeerFailure(format!("{e:?}")))?, ) } @@ -394,8 +401,7 @@ impl RangeBlockComponentsRequest { column_to_peer: HashMap, expects_custody_columns: &[ColumnIndex], attempt: usize, - da_checker: Arc>, - spec: Arc, + custody_context: &CustodyContext, payload_envelopes: Option>>>, ) -> Result>, CouplingError> where @@ -499,17 +505,24 @@ impl RangeBlockComponentsRequest { envelopes_by_block_root.as_mut() { let envelope = envelopes_by_block_root.remove(&block_root); - let available_envelope = - envelope.map(|env| AvailableEnvelope::new(env, custody_columns)); + let bid = block + .message() + .body() + .signed_execution_payload_bid() + // this really should never fail + .map_err(|_| AvailabilityCheckError::MissingBid(block_root))?; + let available_envelope = envelope + .map(|env| AvailableEnvelope::new(env, custody_columns, bid, custody_context)) + .transpose()?; RangeSyncBlock::new_gloas(block, available_envelope) .map_err(CouplingError::EnvelopePeerFailure)? } else if custody_columns.is_empty() { - RangeSyncBlock::new(block, AvailableBlockData::NoData, &da_checker, spec.clone()) + RangeSyncBlock::new(block, AvailableBlockData::NoData, custody_context) .map_err(|e| CouplingError::InternalError(format!("{:?}", e)))? } else { let block_data = AvailableBlockData::new_with_data_columns(custody_columns); - RangeSyncBlock::new(block, block_data, &da_checker, spec.clone()) + RangeSyncBlock::new(block, block_data, custody_context) .map_err(|e| CouplingError::InternalError(format!("{:?}", e)))? }; range_sync_blocks.push(range_sync_block); @@ -562,11 +575,10 @@ mod tests { use super::RangeBlockComponentsRequest; use beacon_chain::block_verification_types::RangeSyncBlock; - use beacon_chain::custody_context::NodeCustodyType; - use beacon_chain::data_availability_checker::DataAvailabilityChecker; + use beacon_chain::custody_context::{CustodyContext, NodeCustodyType}; use beacon_chain::test_utils::{ EphemeralHarnessType, NumBlobs, generate_rand_block_and_blobs, - generate_rand_block_and_data_columns, test_da_checker, test_spec, + generate_rand_block_and_data_columns, test_custody_context, test_spec, }; use bls::Signature; use lighthouse_network::{ @@ -737,8 +749,8 @@ mod tests { fn is_finished(info: &mut RangeBlockComponentsRequest) -> bool { let spec = Arc::new(test_spec::()); - let da_checker = Arc::new(test_da_checker(spec.clone(), NodeCustodyType::Fullnode)); - info.responses(da_checker, spec).is_some() + let custody_context = test_custody_context(NodeCustodyType::Fullnode, spec.clone()); + info.responses(&custody_context, spec).is_some() } fn gloas_spec() -> ChainSpec { @@ -826,7 +838,7 @@ mod tests { #[allow(clippy::type_complexity)] struct GloasSetup { info: RangeBlockComponentsRequest, - da_checker: Arc>>, + custody_context: Arc>>, spec: Arc, blocks: Vec<( Arc>, @@ -841,10 +853,9 @@ mod tests { /// ready for the per-test payload-envelope step. fn setup_gloas_coupling(count: usize) -> GloasSetup { let spec = Arc::new(gloas_spec()); - let da_checker = Arc::new(test_da_checker(spec.clone(), NodeCustodyType::Fullnode)); - let expected_custody_columns = da_checker - .custody_context() - .sampling_columns_for_epoch(Epoch::new(0), &spec) + let custody_context = test_custody_context(NodeCustodyType::Fullnode, spec.clone()); + let expected_custody_columns = custody_context + .sampling_columns_for_epoch(Epoch::new(0)) .to_vec(); let blocks = make_gloas_blocks_and_columns(count, &spec); @@ -886,7 +897,7 @@ mod tests { GloasSetup { info, - da_checker, + custody_context, spec, blocks, payloads_req_id, @@ -900,7 +911,7 @@ mod tests { // Gloas each block still couples to its (empty-column) payload envelope, so the envelope // request is driven too. let spec = Arc::new(custody_test_spec()); - let da_checker = Arc::new(test_da_checker(spec.clone(), NodeCustodyType::Fullnode)); + let custody_context = test_custody_context(NodeCustodyType::Fullnode, spec.clone()); let blocks = make_blocks_and_columns(4, NumBlobs::None, &spec); let components_id = components_id(); @@ -921,7 +932,7 @@ mod tests { .unwrap(); add_envelopes_if_gloas(&mut info, payloads_req_id, &blocks); - let responses = info.responses(da_checker, spec).unwrap().unwrap(); + let responses = info.responses(&custody_context, spec).unwrap().unwrap(); assert_custody_columns_coupled(&responses, blocks.len(), 0); } @@ -960,19 +971,18 @@ mod tests { // FORK_NAME. spec.fulu_fork_epoch = None; let spec = Arc::new(spec); - let da_checker = Arc::new(test_da_checker(spec.clone(), NodeCustodyType::Fullnode)); + let custody_context = test_custody_context(NodeCustodyType::Fullnode, spec.clone()); // Blobs are no longer required for availability, so the response succeeds without them. - let result = info.responses(da_checker, spec).unwrap(); + let result = info.responses(&custody_context, spec).unwrap(); assert!(result.is_ok()) } #[test] fn rpc_block_with_custody_columns() { let spec = Arc::new(custody_test_spec()); - let da_checker = Arc::new(test_da_checker(spec.clone(), NodeCustodyType::Fullnode)); - let expects_custody_columns = da_checker - .custody_context() - .sampling_columns_for_epoch(Epoch::new(0), &spec) + let custody_context = test_custody_context(NodeCustodyType::Fullnode, spec.clone()); + let expects_custody_columns = custody_context + .sampling_columns_for_epoch(Epoch::new(0)) .to_vec(); let blocks = make_blocks_and_columns(4, NumBlobs::Number(1), &spec); @@ -1038,17 +1048,16 @@ mod tests { add_envelopes_if_gloas(&mut info, payloads_req_id, &blocks); // All completed construct response - let responses = info.responses(da_checker, spec).unwrap().unwrap(); + let responses = info.responses(&custody_context, spec).unwrap().unwrap(); assert_custody_columns_coupled(&responses, blocks.len(), expects_custody_columns.len()); } #[test] fn rpc_block_with_custody_columns_batched() { let spec = Arc::new(custody_test_spec()); - let da_checker = Arc::new(test_da_checker(spec.clone(), NodeCustodyType::Fullnode)); - let expected_sampling_columns = da_checker - .custody_context() - .sampling_columns_for_epoch(Epoch::new(0), &spec) + let custody_context = test_custody_context(NodeCustodyType::Fullnode, spec.clone()); + let expected_sampling_columns = custody_context + .sampling_columns_for_epoch(Epoch::new(0)) .to_vec(); // Split sampling columns into two batches let mid = expected_sampling_columns.len() / 2; @@ -1126,7 +1135,7 @@ mod tests { add_envelopes_if_gloas(&mut info, payloads_req_id, &blocks); // All completed construct response - let responses = info.responses(da_checker, spec).unwrap().unwrap(); + let responses = info.responses(&custody_context, spec).unwrap().unwrap(); assert_custody_columns_coupled(&responses, blocks.len(), expected_sampling_columns.len()); } @@ -1134,20 +1143,20 @@ mod tests { fn gloas_payload_envelopes_must_complete_before_responses() { let GloasSetup { mut info, - da_checker, + custody_context, spec, .. } = setup_gloas_coupling(2); // No payload envelopes added yet, so the request must not be complete. - assert!(info.responses(da_checker, spec).is_none()); + assert!(info.responses(&custody_context, spec).is_none()); } #[test] fn gloas_payload_envelopes_are_coupled_by_block_root() { let GloasSetup { mut info, - da_checker, + custody_context, spec, blocks, payloads_req_id, @@ -1165,7 +1174,7 @@ mod tests { ) .unwrap(); - let responses = info.responses(da_checker, spec).unwrap().unwrap(); + let responses = info.responses(&custody_context, spec).unwrap().unwrap(); assert_eq!(responses.len(), blocks.len()); for response in responses { match response { @@ -1188,7 +1197,7 @@ mod tests { fn gloas_payload_envelopes_allow_missing_envelopes() { let GloasSetup { mut info, - da_checker, + custody_context, spec, blocks, payloads_req_id, @@ -1199,7 +1208,7 @@ mod tests { info.add_payload_envelopes(payloads_req_id, vec![blocks[0].2.clone()]) .unwrap(); - let responses = info.responses(da_checker, spec).unwrap().unwrap(); + let responses = info.responses(&custody_context, spec).unwrap().unwrap(); let count_with = |with_envelope: bool| { responses .iter() @@ -1216,7 +1225,7 @@ mod tests { fn gloas_payload_envelope_mismatch_fails_coupling() { let GloasSetup { mut info, - da_checker, + custody_context, spec, blocks, payloads_req_id, @@ -1228,7 +1237,7 @@ mod tests { info.add_payload_envelopes(payloads_req_id, vec![Arc::new(bad_envelope)]) .unwrap(); - let result = info.responses(da_checker, spec).unwrap(); + let result = info.responses(&custody_context, spec).unwrap(); assert!( matches!( result, @@ -1243,10 +1252,9 @@ mod tests { fn missing_custody_columns_from_faulty_peers() { // GIVEN: A request expecting sampling columns from multiple peers let spec = Arc::new(custody_test_spec()); - let da_checker = Arc::new(test_da_checker(spec.clone(), NodeCustodyType::Fullnode)); - let expected_sampling_columns = da_checker - .custody_context() - .sampling_columns_for_epoch(Epoch::new(0), &spec) + let custody_context = test_custody_context(NodeCustodyType::Fullnode, spec.clone()); + let expected_sampling_columns = custody_context + .sampling_columns_for_epoch(Epoch::new(0)) .to_vec(); let blocks = make_blocks_and_columns(2, NumBlobs::Number(1), &spec); @@ -1309,7 +1317,7 @@ mod tests { } // WHEN: Attempting to construct RPC blocks - let result = info.responses(da_checker, spec).unwrap(); + let result = info.responses(&custody_context, spec).unwrap(); // THEN: Should fail with PeerFailure identifying the faulty peers assert!(result.is_err()); @@ -1337,10 +1345,9 @@ mod tests { fn retry_logic_after_peer_failures() { // GIVEN: A request expecting sampling columns where some peers initially fail let spec = Arc::new(custody_test_spec()); - let da_checker = Arc::new(test_da_checker(spec.clone(), NodeCustodyType::Fullnode)); - let expected_sampling_columns = da_checker - .custody_context() - .sampling_columns_for_epoch(Epoch::new(0), &spec) + let custody_context = test_custody_context(NodeCustodyType::Fullnode, spec.clone()); + let expected_sampling_columns = custody_context + .sampling_columns_for_epoch(Epoch::new(0)) .to_vec(); let blocks = make_blocks_and_columns(2, NumBlobs::Number(1), &spec); @@ -1402,7 +1409,7 @@ mod tests { let result: Result< Vec>, crate::sync::block_sidecar_coupling::CouplingError, - > = info.responses(da_checker.clone(), spec.clone()).unwrap(); + > = info.responses(&custody_context, spec.clone()).unwrap(); assert!(result.is_err()); // AND: We retry with a new peer for the failed columns @@ -1433,7 +1440,7 @@ mod tests { .unwrap(); // WHEN: Attempting to get responses again - let result = info.responses(da_checker, spec).unwrap(); + let result = info.responses(&custody_context, spec).unwrap(); // THEN: Should succeed with complete RangeSync blocks assert!(result.is_ok()); @@ -1445,10 +1452,9 @@ mod tests { fn max_retries_exceeded_behavior() { // GIVEN: A request where peers consistently fail to provide required columns let spec = Arc::new(custody_test_spec()); - let da_checker = Arc::new(test_da_checker(spec.clone(), NodeCustodyType::Fullnode)); - let expected_sampling_columns = da_checker - .custody_context() - .sampling_columns_for_epoch(Epoch::new(0), &spec) + let custody_context = test_custody_context(NodeCustodyType::Fullnode, spec.clone()); + let expected_sampling_columns = custody_context + .sampling_columns_for_epoch(Epoch::new(0)) .to_vec(); let blocks = make_blocks_and_columns(1, NumBlobs::Number(1), &spec); @@ -1509,7 +1515,7 @@ mod tests { // WHEN: Multiple retry attempts are made (up to max retries) for _ in 0..MAX_COLUMN_RETRIES { - let result = info.responses(da_checker.clone(), spec.clone()).unwrap(); + let result = info.responses(&custody_context, spec.clone()).unwrap(); assert!(result.is_err()); if let Err(super::CouplingError::DataColumnPeerFailure { @@ -1522,7 +1528,7 @@ mod tests { } // AND: One final attempt after exceeding max retries - let result = info.responses(da_checker, spec).unwrap(); + let result = info.responses(&custody_context, spec).unwrap(); // THEN: Should fail with exceeded_retries = true assert!(result.is_err()); diff --git a/beacon_node/network/src/sync/custody_backfill_sync/mod.rs b/beacon_node/network/src/sync/custody_backfill_sync/mod.rs index c85610613c..2874bbebf1 100644 --- a/beacon_node/network/src/sync/custody_backfill_sync/mod.rs +++ b/beacon_node/network/src/sync/custody_backfill_sync/mod.rs @@ -162,7 +162,11 @@ impl CustodyBackFillSync { /// - The earliest data column epoch's custodied columns != previous epoch's custodied columns /// - The earliest data column epoch is a finalied epoch pub fn should_start_custody_backfill_sync(&mut self) -> bool { - let Some(da_boundary_epoch) = self.beacon_chain.get_column_da_boundary() else { + let Some(da_boundary_epoch) = self + .beacon_chain + .custody_context + .column_data_availability_boundary() + else { return false; }; @@ -220,9 +224,8 @@ impl CustodyBackFillSync { fn restart_if_required(&mut self) -> bool { let cgc_at_head = self .beacon_chain - .data_availability_checker - .custody_context() - .custody_group_count_at_head(&self.beacon_chain.spec); + .custody_context + .custody_group_count_at_head(); if cgc_at_head != self.cgc { self.restart_sync(); @@ -290,7 +293,11 @@ impl CustodyBackFillSync { } } - let Some(column_da_boundary) = self.beacon_chain.get_column_da_boundary() else { + let Some(column_da_boundary) = self + .beacon_chain + .custody_context + .column_data_availability_boundary() + else { return Ok(SyncStart::NotSyncing); }; @@ -309,9 +316,8 @@ impl CustodyBackFillSync { fn set_cgc(&mut self) { self.cgc = self .beacon_chain - .data_availability_checker - .custody_context() - .custody_group_count_at_head(&self.beacon_chain.spec); + .custody_context + .custody_group_count_at_head(); } fn set_start_epoch(&mut self) { @@ -379,9 +385,10 @@ impl CustodyBackFillSync { /// Creates the next required batch from the chain. If there are no more batches required, /// `None` is returned. fn include_next_batch(&mut self) -> Option { - let Some(column_da_boundary) = self.beacon_chain.get_column_da_boundary() else { - return None; - }; + let column_da_boundary = self + .beacon_chain + .custody_context + .column_data_availability_boundary()?; // Skip all batches (Epochs) that don't have missing columns. for epoch in Epoch::range_inclusive_rev(self.to_be_downloaded, column_da_boundary) { @@ -715,7 +722,11 @@ impl CustodyBackFillSync { self.advance_custody_backfill_sync(batch_id); - let Some(column_da_boundary) = self.beacon_chain.get_column_da_boundary() else { + let Some(column_da_boundary) = self + .beacon_chain + .custody_context + .column_data_availability_boundary() + else { return Err(CustodyBackfillError::InvalidSyncState( "Can't calculate column data availability boundary".to_string(), )); @@ -889,7 +900,11 @@ impl CustodyBackFillSync { /// /// The `validating_epoch` must align with batch boundaries. fn advance_custody_backfill_sync(&mut self, validating_epoch: Epoch) { - let Some(column_da_boundary) = self.beacon_chain.get_column_da_boundary() else { + let Some(column_da_boundary) = self + .beacon_chain + .custody_context + .column_data_availability_boundary() + else { return; }; // make sure this epoch produces an advancement, unless its at the column DA boundary @@ -986,7 +1001,11 @@ impl CustodyBackFillSync { return false; }; - let Some(column_da_boundary) = self.beacon_chain.get_column_da_boundary() else { + let Some(column_da_boundary) = self + .beacon_chain + .custody_context + .column_data_availability_boundary() + else { return false; }; @@ -997,7 +1016,11 @@ impl CustodyBackFillSync { /// Checks if custody backfill would complete by syncing to `start_epoch`. fn would_complete(&self, start_epoch: Epoch) -> bool { - let Some(column_da_boundary) = self.beacon_chain.get_column_da_boundary() else { + let Some(column_da_boundary) = self + .beacon_chain + .custody_context + .column_data_availability_boundary() + else { return false; }; start_epoch <= column_da_boundary diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index 3282f7f083..b9bea21b8c 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -113,7 +113,6 @@ pub enum SyncMessage { sync_request_id: SyncRequestId, peer_id: PeerId, beacon_block: Option>>, - seen_timestamp: Duration, }, /// A blob has been received from the RPC. @@ -121,7 +120,6 @@ pub enum SyncMessage { sync_request_id: SyncRequestId, peer_id: PeerId, blob_sidecar: Option>>, - seen_timestamp: Duration, }, /// A data columns has been received from the RPC @@ -129,7 +127,6 @@ pub enum SyncMessage { sync_request_id: SyncRequestId, peer_id: PeerId, data_column: Option>>, - seen_timestamp: Duration, }, /// A payload envelope has been received from the RPC. @@ -137,7 +134,6 @@ pub enum SyncMessage { sync_request_id: SyncRequestId, peer_id: PeerId, envelope: Option>>, - seen_timestamp: Duration, }, /// A block with an unknown parent has been received. @@ -835,35 +831,24 @@ impl SyncManager { sync_request_id, peer_id, beacon_block, - seen_timestamp, } => { - self.rpc_block_received(sync_request_id, peer_id, beacon_block, seen_timestamp); + self.rpc_block_received(sync_request_id, peer_id, beacon_block); } SyncMessage::RpcBlob { sync_request_id, peer_id, blob_sidecar, - seen_timestamp, - } => self.rpc_blob_received(sync_request_id, peer_id, blob_sidecar, seen_timestamp), + } => self.rpc_blob_received(sync_request_id, peer_id, blob_sidecar), SyncMessage::RpcDataColumn { sync_request_id, peer_id, data_column, - seen_timestamp, - } => { - self.rpc_data_column_received(sync_request_id, peer_id, data_column, seen_timestamp) - } + } => self.rpc_data_column_received(sync_request_id, peer_id, data_column), SyncMessage::RpcPayloadEnvelope { sync_request_id, peer_id, envelope, - seen_timestamp, - } => self.rpc_payload_envelope_received( - sync_request_id, - peer_id, - envelope, - seen_timestamp, - ), + } => self.rpc_payload_envelope_received(sync_request_id, peer_id, envelope), SyncMessage::UnknownParentBlock(peer_id, block, block_root) => { let block_slot = block.slot(); let parent_root = block.parent_root(); @@ -877,7 +862,6 @@ impl SyncManager { block_slot, BlockComponent::Block(DownloadResult { value: block.block_cloned(), - seen_timestamp: self.chain.slot_clock.now_duration().unwrap_or_default(), peer_group: PeerGroup::from_single(peer_id), }), ); @@ -1122,19 +1106,14 @@ impl SyncManager { sync_request_id: SyncRequestId, peer_id: PeerId, block: Option>>, - seen_timestamp: Duration, ) { match sync_request_id { - SyncRequestId::SingleBlock { id } => self.on_single_block_response( - id, - peer_id, - RpcEvent::from_chunk(block, seen_timestamp), - ), - SyncRequestId::BlocksByRange(id) => self.on_blocks_by_range_response( - id, - peer_id, - RpcEvent::from_chunk(block, seen_timestamp), - ), + SyncRequestId::SingleBlock { id } => { + self.on_single_block_response(id, peer_id, RpcEvent::from_chunk(block)) + } + SyncRequestId::BlocksByRange(id) => { + self.on_blocks_by_range_response(id, peer_id, RpcEvent::from_chunk(block)) + } _ => { crit!(%peer_id, "bad request id for block"); } @@ -1150,9 +1129,7 @@ impl SyncManager { if let Some(resp) = self.network.on_single_block_response(id, peer_id, block) { self.block_lookups.on_block_download_response( id, - resp.map(|(value, seen_timestamp)| { - DownloadResult::new(value, PeerGroup::from_single(peer_id), seen_timestamp) - }), + resp.map(|value| DownloadResult::new(value, PeerGroup::from_single(peer_id))), &mut self.network, ) } @@ -1163,14 +1140,11 @@ impl SyncManager { sync_request_id: SyncRequestId, peer_id: PeerId, blob: Option>>, - seen_timestamp: Duration, ) { match sync_request_id { - SyncRequestId::BlobsByRange(id) => self.on_blobs_by_range_response( - id, - peer_id, - RpcEvent::from_chunk(blob, seen_timestamp), - ), + SyncRequestId::BlobsByRange(id) => { + self.on_blobs_by_range_response(id, peer_id, RpcEvent::from_chunk(blob)) + } _ => { crit!(%peer_id, "bad request id for blob"); } @@ -1182,20 +1156,15 @@ impl SyncManager { sync_request_id: SyncRequestId, peer_id: PeerId, envelope: Option>>, - seen_timestamp: Duration, ) { match sync_request_id { SyncRequestId::SinglePayloadEnvelope { id } => self - .on_single_payload_envelope_response( - id, - peer_id, - RpcEvent::from_chunk(envelope, seen_timestamp), - ), + .on_single_payload_envelope_response(id, peer_id, RpcEvent::from_chunk(envelope)), SyncRequestId::PayloadEnvelopesByRange(req_id) => { self.on_payload_envelopes_by_range_response( req_id, peer_id, - RpcEvent::from_chunk(envelope, seen_timestamp), + RpcEvent::from_chunk(envelope), ); } _ => { @@ -1209,21 +1178,20 @@ impl SyncManager { sync_request_id: SyncRequestId, peer_id: PeerId, data_column: Option>>, - seen_timestamp: Duration, ) { match sync_request_id { SyncRequestId::DataColumnsByRoot(req_id) => { self.on_data_columns_by_root_response( req_id, peer_id, - RpcEvent::from_chunk(data_column, seen_timestamp), + RpcEvent::from_chunk(data_column), ); } SyncRequestId::DataColumnsByRange(req_id) => { self.on_data_columns_by_range_response( req_id, peer_id, - RpcEvent::from_chunk(data_column, seen_timestamp), + RpcEvent::from_chunk(data_column), ); } _ => { @@ -1244,9 +1212,7 @@ impl SyncManager { { self.block_lookups.on_payload_download_response( id, - resp.map(|(value, seen_timestamp)| { - DownloadResult::new(value, PeerGroup::from_single(peer_id), seen_timestamp) - }), + resp.map(|value| DownloadResult::new(value, PeerGroup::from_single(peer_id))), &mut self.network, ) } diff --git a/beacon_node/network/src/sync/network_context.rs b/beacon_node/network/src/sync/network_context.rs index 0db172a21a..100be9f4d7 100644 --- a/beacon_node/network/src/sync/network_context.rs +++ b/beacon_node/network/src/sync/network_context.rs @@ -52,7 +52,6 @@ use std::collections::hash_map::Entry; use std::collections::{HashMap, HashSet}; use std::fmt::Debug; use std::sync::Arc; -use std::time::Duration; #[cfg(test)] use task_executor::TaskExecutor; use tokio::sync::mpsc; @@ -83,22 +82,21 @@ pub const MAX_COLUMN_RETRIES: usize = 3; #[derive(Debug)] pub enum RpcEvent { StreamTermination, - Response(T, Duration), + Response(T), RPCError(RPCError), } impl RpcEvent { - pub fn from_chunk(chunk: Option, seen_timestamp: Duration) -> Self { + pub fn from_chunk(chunk: Option) -> Self { match chunk { - Some(item) => RpcEvent::Response(item, seen_timestamp), + Some(item) => RpcEvent::Response(item), None => RpcEvent::StreamTermination, } } } -pub type RpcResponseResult = Result<(T, Duration), RpcResponseError>; +pub type RpcResponseResult = Result; -/// Duration = latest seen timestamp of all received data columns pub type CustodyByRootResult = Result>, RpcResponseError>; @@ -591,6 +589,7 @@ impl SyncNetworkContext { let epoch = Slot::new(*request.start_slot()).epoch(T::EthSpec::slots_per_epoch()); let column_indexes = self .chain + .custody_context .sampling_columns_for_epoch(epoch) .iter() .cloned() @@ -696,7 +695,10 @@ impl SyncNetworkContext { data_column_requests.map(|data_column_requests| { ( data_column_requests, - self.chain.sampling_columns_for_epoch(epoch).to_vec(), + self.chain + .custody_context + .sampling_columns_for_epoch(epoch) + .to_vec(), ) }), payloads_req_id, @@ -776,14 +778,14 @@ impl SyncNetworkContext { if let Err(e) = { let request = entry.get_mut(); match range_block_component { - RangeBlockComponent::Block(req_id, resp) => resp.and_then(|(blocks, _)| { + RangeBlockComponent::Block(req_id, resp) => resp.and_then(|blocks| { request.add_blocks(req_id, blocks).map_err(|e| { RpcResponseError::BlockComponentCouplingError(CouplingError::InternalError( e, )) }) }), - RangeBlockComponent::Blob(req_id, resp) => resp.and_then(|(blobs, _)| { + RangeBlockComponent::Blob(req_id, resp) => resp.and_then(|blobs| { request.add_blobs(req_id, blobs).map_err(|e| { RpcResponseError::BlockComponentCouplingError(CouplingError::InternalError( e, @@ -791,7 +793,7 @@ impl SyncNetworkContext { }) }), RangeBlockComponent::CustodyColumns(req_id, resp) => { - resp.and_then(|(custody_columns, _)| { + resp.and_then(|custody_columns| { request .add_custody_columns(req_id, custody_columns) .map_err(|e| { @@ -801,17 +803,15 @@ impl SyncNetworkContext { }) }) } - RangeBlockComponent::PayloadEnvelope(req_id, resp) => { - resp.and_then(|(envelopes, _)| { - request - .add_payload_envelopes(req_id, envelopes) - .map_err(|e| { - RpcResponseError::BlockComponentCouplingError( - CouplingError::InternalError(e), - ) - }) - }) - } + RangeBlockComponent::PayloadEnvelope(req_id, resp) => resp.and_then(|envelopes| { + request + .add_payload_envelopes(req_id, envelopes) + .map_err(|e| { + RpcResponseError::BlockComponentCouplingError( + CouplingError::InternalError(e), + ) + }) + }), } } { entry.remove(); @@ -819,10 +819,9 @@ impl SyncNetworkContext { } let range_req = entry.get_mut(); - if let Some(blocks_result) = range_req.responses( - self.chain.data_availability_checker.clone(), - self.chain.spec.clone(), - ) { + if let Some(blocks_result) = + range_req.responses(&self.chain.custody_context, self.chain.spec.clone()) + { if let Err(CouplingError::DataColumnPeerFailure { error, faulty_peers: _, @@ -1112,6 +1111,7 @@ impl SyncNetworkContext { // Include only the blob indexes not yet imported (received through gossip) let mut custody_indexes_to_fetch = self .chain + .custody_context .sampling_columns_for_epoch(block_slot.epoch(T::EthSpec::slots_per_epoch())) .iter() .copied() @@ -1428,15 +1428,11 @@ impl SyncNetworkContext { ByRangeRequestType::BlocksAndEnvelopesAndColumns } else if self .chain - .data_availability_checker + .custody_context .data_columns_required_for_epoch(epoch) { ByRangeRequestType::BlocksAndColumns - } else if self - .chain - .data_availability_checker - .blobs_required_for_epoch(epoch) - { + } else if self.chain.custody_context.blobs_required_for_epoch(epoch) { ByRangeRequestType::BlocksAndBlobs } else { ByRangeRequestType::Blocks @@ -1479,11 +1475,11 @@ impl SyncNetworkContext { ) -> Option>>> { let resp = self.blocks_by_root_requests.on_response(id, rpc_event); let resp = resp.map(|res| { - res.and_then(|(mut blocks, seen_timestamp)| { + res.and_then(|mut blocks| { // Enforce that exactly one chunk = one block is returned. ReqResp behavior limits the // response count to at most 1. match blocks.pop() { - Some(block) => Ok((block, seen_timestamp)), + Some(block) => Ok(block), // Should never happen, `blocks_by_root_requests` enforces that we receive at least // 1 chunk. None => Err(LookupVerifyError::NotEnoughResponsesReturned { actual: 0 }.into()), @@ -1503,9 +1499,9 @@ impl SyncNetworkContext { .payload_envelopes_by_root_requests .on_response(id, rpc_event); let resp = resp.map(|res| { - res.and_then(|(mut envelopes, seen_timestamp)| { + res.and_then(|mut envelopes| { match envelopes.pop() { - Some(envelope) => Ok((envelope, seen_timestamp)), + Some(envelope) => Ok(envelope), // Should never happen, we enforce at least 1 chunk. None => Err(LookupVerifyError::NotEnoughResponsesReturned { actual: 0 }.into()), } @@ -1646,7 +1642,6 @@ impl SyncNetworkContext { id: Id, block_root: Hash256, block: Arc>, - seen_timestamp: Duration, ) -> Result<(), SendErrorProcessor> { let beacon_processor = self .beacon_processor_if_enabled() @@ -1661,7 +1656,6 @@ impl SyncNetworkContext { .send_lookup_beacon_block( block_root, lookup_block, - seen_timestamp, BlockProcessType::SingleBlock { id }, ) .map_err(|e| { @@ -1677,7 +1671,6 @@ impl SyncNetworkContext { &self, block_root: Hash256, envelope: Arc>, - seen_timestamp: Duration, process_type: BlockProcessType, ) -> Result<(), SendErrorProcessor> { let beacon_processor = self @@ -1691,7 +1684,7 @@ impl SyncNetworkContext { ); beacon_processor - .send_lookup_envelope(block_root, envelope, seen_timestamp, process_type) + .send_lookup_envelope(block_root, envelope, process_type) .map_err(|e| { error!( error = ?e, @@ -1706,7 +1699,6 @@ impl SyncNetworkContext { _id: Id, block_root: Hash256, custody_columns: DataColumnSidecarList, - seen_timestamp: Duration, process_type: BlockProcessType, ) -> Result<(), SendErrorProcessor> { let beacon_processor = self @@ -1720,7 +1712,7 @@ impl SyncNetworkContext { ); beacon_processor - .send_rpc_custody_columns(block_root, custody_columns, seen_timestamp, process_type) + .send_rpc_custody_columns(block_root, custody_columns, process_type) .map_err(|e| { error!( error = ?e, @@ -1742,6 +1734,7 @@ impl SyncNetworkContext { let columns_by_range_peers_to_request = { let column_indexes = self .chain + .custody_context .sampling_columns_for_epoch(batch_id.epoch) .iter() .cloned() @@ -1806,7 +1799,7 @@ impl SyncNetworkContext { if let Err(e) = { let request = entry.get_mut(); - data_columns.and_then(|(data_columns, _)| { + data_columns.and_then(|data_columns| { request .add_custody_columns(req_id, data_columns.clone()) .map_err(|e| { diff --git a/beacon_node/network/src/sync/network_context/custody.rs b/beacon_node/network/src/sync/network_context/custody.rs index 3dd3683c42..29cb0a22e5 100644 --- a/beacon_node/network/src/sync/network_context/custody.rs +++ b/beacon_node/network/src/sync/network_context/custody.rs @@ -7,7 +7,6 @@ use fnv::FnvHashMap; use lighthouse_network::PeerId; use lighthouse_network::service::api_types::{CustodyId, DataColumnsByRootRequester}; use parking_lot::RwLock; -use slot_clock::SlotClock; use std::collections::HashSet; use std::hash::{BuildHasher, RandomState}; use std::time::{Duration, Instant}; @@ -119,7 +118,7 @@ impl ActiveCustodyRequest { let _guard = batch_request.span.clone().entered(); match resp { - Ok((data_columns, seen_timestamp)) => { + Ok(data_columns) => { debug!( block_root = ?self.block_root, %req_id, @@ -144,12 +143,7 @@ impl ActiveCustodyRequest { .ok_or(Error::BadState("unknown column_index".to_owned()))?; if let Some(data_column) = data_columns.remove(column_index) { - column_request.on_download_success( - req_id, - peer_id, - data_column, - seen_timestamp, - )?; + column_request.on_download_success(req_id, peer_id, data_column)?; } else { // Peer does not have the requested data. // TODO(das) do not consider this case a success. We know for sure the block has @@ -213,30 +207,20 @@ impl ActiveCustodyRequest { if completed_requests >= total_requests { // All requests have completed successfully. let mut peers = HashMap::>::new(); - let mut seen_timestamps = vec![]; let columns = std::mem::take(&mut self.column_requests) .into_values() .map(|request| { - let (peer, data_column, seen_timestamp) = request.complete()?; + let (peer, data_column) = request.complete()?; peers .entry(peer) .or_default() .push(*data_column.index() as usize); - seen_timestamps.push(seen_timestamp); Ok(data_column) }) .collect::, _>>()?; let peer_group = PeerGroup::from_set(peers); - let max_seen_timestamp = seen_timestamps - .into_iter() - .max() - .unwrap_or_else(|| cx.chain.slot_clock.now_duration().unwrap_or_default()); - return Ok(Some(DownloadResult::new( - columns, - peer_group, - max_seen_timestamp, - ))); + return Ok(Some(DownloadResult::new(columns, peer_group))); } let data_columns_by_root_per_peer = @@ -416,7 +400,7 @@ struct ColumnRequest { enum Status { NotStarted(Instant), Downloading(DataColumnsByRootRequestId), - Downloaded(PeerId, Arc>, Duration), + Downloaded(PeerId, Arc>), } impl ColumnRequest { @@ -485,7 +469,6 @@ impl ColumnRequest { req_id: DataColumnsByRootRequestId, peer_id: PeerId, data_column: Arc>, - seen_timestamp: Duration, ) -> Result<(), Error> { match &self.status { Status::Downloading(expected_req_id) => { @@ -495,7 +478,7 @@ impl ColumnRequest { req_id, }); } - self.status = Status::Downloaded(peer_id, data_column, seen_timestamp); + self.status = Status::Downloaded(peer_id, data_column); Ok(()) } other => Err(Error::BadState(format!( @@ -504,11 +487,9 @@ impl ColumnRequest { } } - fn complete(self) -> Result<(PeerId, Arc>, Duration), Error> { + fn complete(self) -> Result<(PeerId, Arc>), Error> { match self.status { - Status::Downloaded(peer_id, data_column, seen_timestamp) => { - Ok((peer_id, data_column, seen_timestamp)) - } + Status::Downloaded(peer_id, data_column) => Ok((peer_id, data_column)), other => Err(Error::BadState(format!( "bad state complete expected Downloaded got {other:?}" ))), diff --git a/beacon_node/network/src/sync/network_context/requests.rs b/beacon_node/network/src/sync/network_context/requests.rs index cc74785098..b340064746 100644 --- a/beacon_node/network/src/sync/network_context/requests.rs +++ b/beacon_node/network/src/sync/network_context/requests.rs @@ -3,7 +3,6 @@ use std::{collections::hash_map::Entry, hash::Hash}; use fnv::FnvHashMap; use lighthouse_network::PeerId; -use slot_clock::timestamp_now; use strum::IntoStaticStr; use tracing::{Span, debug}; use types::{Hash256, Slot}; @@ -122,7 +121,7 @@ impl ActiveReque let result = match rpc_event { // Handler of a success ReqResp chunk. Adds the item to the request accumulator. // `ActiveRequestItems` validates the item before appending to its internal state. - RpcEvent::Response(item, seen_timestamp) => { + RpcEvent::Response(item) => { let request = &mut entry.get_mut(); let _guard = request.span.clone().entered(); match &mut request.state { @@ -133,7 +132,7 @@ impl ActiveReque Ok(true) => { let items = items.consume(); request.state = State::CompletedEarly; - Some(Ok((items, seen_timestamp, request.start_instant.elapsed()))) + Some(Ok((items, request.start_instant.elapsed()))) } // Received item, but we are still expecting more Ok(false) => None, @@ -170,11 +169,7 @@ impl ActiveReque } .into())) } else { - Some(Ok(( - items.consume(), - timestamp_now(), - request.start_instant.elapsed(), - ))) + Some(Ok((items.consume(), request.start_instant.elapsed()))) } } // Items already returned, ignore stream termination @@ -202,7 +197,7 @@ impl ActiveReque }; result.map(|result| match result { - Ok((items, seen_timestamp, duration)) => { + Ok((items, duration)) => { metrics::inc_counter_vec(&metrics::SYNC_RPC_REQUEST_SUCCESSES, &[self.name]); metrics::observe_timer_vec(&metrics::SYNC_RPC_REQUEST_TIME, &[self.name], duration); debug!( @@ -212,7 +207,7 @@ impl ActiveReque "Sync RPC request completed" ); - Ok((items, seen_timestamp)) + Ok(items) } Err(e) => { let err_str: &'static str = match &e { diff --git a/beacon_node/network/src/sync/range_sync/chain.rs b/beacon_node/network/src/sync/range_sync/chain.rs index 6292388339..e82bfa8a1a 100644 --- a/beacon_node/network/src/sync/range_sync/chain.rs +++ b/beacon_node/network/src/sync/range_sync/chain.rs @@ -958,6 +958,9 @@ impl SyncingChain { CouplingError::InternalError(msg) => { error!(?batch_id, msg, "Block components coupling internal error"); } + CouplingError::AvailabilityCheckError(err) => { + error!(?batch_id, ?err, "Availability check error"); + } } } // A batch could be retried without the peer failing the request (disconnecting/ diff --git a/beacon_node/network/src/sync/tests/lookups.rs b/beacon_node/network/src/sync/tests/lookups.rs index deaf421e39..f9afb910d9 100644 --- a/beacon_node/network/src/sync/tests/lookups.rs +++ b/beacon_node/network/src/sync/tests/lookups.rs @@ -43,8 +43,6 @@ use types::{ SignedExecutionPayloadEnvelope, Slot, }; -const D: Duration = Duration::new(0, 0); - /// Extract the Gloas payload envelope (if any) carried by a stored `RangeSyncBlock`. fn envelope_of(block: &RangeSyncBlock) -> Option>> { match block { @@ -886,14 +884,12 @@ impl TestRig { sync_request_id, peer_id, beacon_block: Some(block.clone()), - seen_timestamp: D, }); } self.push_sync_message(SyncMessage::RpcBlock { sync_request_id, peer_id, beacon_block: None, - seen_timestamp: D, }); } @@ -917,14 +913,12 @@ impl TestRig { sync_request_id, peer_id, blob_sidecar: Some(blob.clone()), - seen_timestamp: D, }); } self.push_sync_message(SyncMessage::RpcBlob { sync_request_id, peer_id, blob_sidecar: None, - seen_timestamp: D, }); } @@ -953,14 +947,12 @@ impl TestRig { sync_request_id, peer_id, data_column: Some(column.clone()), - seen_timestamp: D, }); } self.push_sync_message(SyncMessage::RpcDataColumn { sync_request_id, peer_id, data_column: None, - seen_timestamp: D, }); } @@ -979,14 +971,12 @@ impl TestRig { sync_request_id, peer_id, envelope: envelope.clone(), - seen_timestamp: D, }); // Stream termination self.push_sync_message(SyncMessage::RpcPayloadEnvelope { sync_request_id, peer_id, envelope: None, - seen_timestamp: D, }); } @@ -1006,7 +996,6 @@ impl TestRig { sync_request_id, peer_id, envelope: Some(envelope.clone()), - seen_timestamp: D, }); } // Stream termination @@ -1014,7 +1003,6 @@ impl TestRig { sync_request_id, peer_id, envelope: None, - seen_timestamp: D, }); } @@ -1294,30 +1282,34 @@ impl TestRig { ) { let block_root = block.canonical_root(); let block_slot = block.slot(); - let range_sync_block = if block.fork_name_unchecked().gloas_enabled() { - // Gloas carries data columns in the payload envelope, not in `block_data`. - let envelope = self - .network_blocks_by_root - .get(&block_root) - .and_then(envelope_of) - .map(|envelope| AvailableEnvelope::new(envelope, columns.unwrap_or_default())); - RangeSyncBlock::new_gloas(block, envelope).unwrap() - } else { - let block_data = if let Some(columns) = columns { - AvailableBlockData::new_with_data_columns(columns) - } else if let Some(blobs) = blobs { - AvailableBlockData::new_with_blobs(blobs) + let range_sync_block = + if let Ok(bid) = block.message().body().signed_execution_payload_bid() { + // Gloas carries data columns in the payload envelope, not in `block_data`. + let envelope = self + .network_blocks_by_root + .get(&block_root) + .and_then(envelope_of) + .map(|envelope| { + AvailableEnvelope::new( + envelope, + columns.unwrap_or_default(), + bid, + &self.harness.chain.custody_context, + ) + }) + .transpose() + .unwrap(); + RangeSyncBlock::new_gloas(block, envelope).unwrap() } else { - AvailableBlockData::NoData + let block_data = if let Some(columns) = columns { + AvailableBlockData::new_with_data_columns(columns) + } else if let Some(blobs) = blobs { + AvailableBlockData::new_with_blobs(blobs) + } else { + AvailableBlockData::NoData + }; + RangeSyncBlock::new(block, block_data, &self.harness.chain.custody_context).unwrap() }; - RangeSyncBlock::new( - block, - block_data, - &self.harness.chain.data_availability_checker, - self.harness.chain.spec.clone(), - ) - .unwrap() - }; self.network_blocks_by_slot .insert(block_slot, range_sync_block.clone()); self.network_blocks_by_root @@ -1634,9 +1626,8 @@ impl TestRig { fn custody_columns(&self) -> &[ColumnIndex] { self.harness .chain - .data_availability_checker - .custody_context() - .custody_columns_for_epoch(None, &self.harness.spec) + .custody_context + .custody_columns_for_epoch(None) } // Test setup @@ -2419,6 +2410,21 @@ async fn peer_disconnected_then_rpc_error(depth: usize) { r.assert_single_lookups_count(1); } +#[tokio::test] +/// A lookup that loses its only peer while still waiting to download the block must not report +/// itself as awaiting an event, else `drop_lookups_without_peers` skips it and it gets stuck. +/// Regression for the "Notify the devs a sync lookup is stuck" report. +async fn peerless_lookup_awaiting_download_is_not_awaiting_event() { + let mut r = TestRig::default(); + r.build_chain_and_trigger_last_block(1).await; + r.disconnect_all_peers(); + r.simulate(SimulateConfig::new().return_rpc_error(RPCError::Disconnected)) + .await; + let lookup = &r.active_single_lookups()[0]; + assert_eq!(lookup.peers.len(), 0); + assert!(!lookup.is_awaiting_event); +} + #[tokio::test] /// Assert that when creating multiple lookups their parent-child relation is discovered and we add /// peers recursively from child to parent. diff --git a/consensus/fork_choice/Cargo.toml b/consensus/fork_choice/Cargo.toml index df47a5c9d1..03dace1f9f 100644 --- a/consensus/fork_choice/Cargo.toml +++ b/consensus/fork_choice/Cargo.toml @@ -20,5 +20,10 @@ types = { workspace = true } [dev-dependencies] beacon_chain = { workspace = true } bls = { workspace = true } +criterion = { workspace = true } store = { workspace = true } tokio = { workspace = true } + +[[bench]] +name = "benches" +harness = false diff --git a/consensus/fork_choice/benches/benches.rs b/consensus/fork_choice/benches/benches.rs new file mode 100644 index 0000000000..bf01b4c346 --- /dev/null +++ b/consensus/fork_choice/benches/benches.rs @@ -0,0 +1,62 @@ +use criterion::{BenchmarkId, Criterion, criterion_group, criterion_main}; +use fork_choice::{QueuedAttestation, dequeue_attestations}; +use std::collections::BTreeMap; +use types::{Epoch, Hash256, Slot}; + +fn att(slot: Slot) -> QueuedAttestation { + QueuedAttestation { + slot, + attesting_indices: vec![], + block_root: Hash256::ZERO, + target_epoch: Epoch::new(0), + payload_present: false, + } +} + +// Anticipated steady-state workload on mainnet: ~94k attestations spread over a small number of slots, then +// many iterations of dequeue (one slot's worth) + enqueue (one slot's worth for a future slot). +// The queue stays at a constant size, exercising the per-slot dequeue cost. +const NUM_ATTESTATIONS: usize = 1_500_000 / 16; +const UNIQUE_SLOTS: usize = 2; +const NUM_ITERATIONS: usize = 64; +const PER_SLOT: usize = NUM_ATTESTATIONS / UNIQUE_SLOTS; + +fn build_queue() -> BTreeMap> { + let mut queue: BTreeMap> = BTreeMap::new(); + for i in 0..NUM_ATTESTATIONS { + let slot = Slot::from(i / PER_SLOT); + queue.entry(slot).or_default().push(att(slot)); + } + queue +} + +fn all_benches(c: &mut Criterion) { + let initial = build_queue(); + + c.bench_with_input( + BenchmarkId::new("dequeue_attestations", NUM_ATTESTATIONS), + &initial, + |b, initial| { + b.iter(|| { + let mut queue = initial.clone(); + for i in 1..=NUM_ITERATIONS { + let dequeued = dequeue_attestations(Slot::from(i), &mut queue); + let dequeued_count: usize = dequeued.values().map(Vec::len).sum(); + assert_eq!(dequeued_count, PER_SLOT); + + let next_slot = Slot::from(UNIQUE_SLOTS + i - 1); + queue + .entry(next_slot) + .or_default() + .extend(std::iter::repeat_with(|| att(next_slot)).take(PER_SLOT)); + + let total: usize = queue.values().map(Vec::len).sum(); + assert_eq!(total, NUM_ATTESTATIONS); + } + }) + }, + ); +} + +criterion_group!(benches, all_benches); +criterion_main!(benches); diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 452d379d30..2fa36d721b 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -11,7 +11,7 @@ use state_processing::{ per_block_processing::errors::AttesterSlashingValidationError, per_epoch_processing, }; use std::cmp::Ordering; -use std::collections::BTreeSet; +use std::collections::{BTreeMap, BTreeSet}; use std::marker::PhantomData; use std::time::Duration; use superstruct::superstruct; @@ -285,12 +285,12 @@ fn compute_start_slot_at_epoch(epoch: Epoch) -> Slot { /// information about the attestation. #[derive(Clone, PartialEq, Encode, Decode)] pub struct QueuedAttestation { - slot: Slot, - attesting_indices: Vec, - block_root: Hash256, - target_epoch: Epoch, + pub slot: Slot, + pub attesting_indices: Vec, + pub block_root: Hash256, + pub target_epoch: Epoch, /// Per Gloas spec: `payload_present = attestation.data.index == 1`. - payload_present: bool, + pub payload_present: bool, } /// Legacy queued attestation without payload_present (pre-Gloas, schema V28). @@ -314,25 +314,22 @@ impl<'a, E: EthSpec> From> for QueuedAttestation { } } -/// Returns all values in `self.queued_attestations` that have a slot that is earlier than the -/// current slot. Also removes those values from `self.queued_attestations`. -fn dequeue_attestations( +/// Returns all attestations in `queued_attestations` with a slot earlier than the current slot, +/// removing them from the queue. +pub fn dequeue_attestations( current_slot: Slot, - queued_attestations: &mut Vec, -) -> Vec { - let remaining = queued_attestations.split_off( - queued_attestations - .iter() - .position(|a| a.slot >= current_slot) - .unwrap_or(queued_attestations.len()), - ); + queued_attestations: &mut BTreeMap>, +) -> BTreeMap> { + let remaining = queued_attestations.split_off(¤t_slot); + let due = std::mem::replace(queued_attestations, remaining); + let dequeued_count: usize = due.values().map(Vec::len).sum(); metrics::inc_counter_by( &metrics::FORK_CHOICE_DEQUEUED_ATTESTATIONS, - queued_attestations.len() as u64, + dequeued_count as u64, ); - std::mem::replace(queued_attestations, remaining) + due } /// Denotes whether an attestation we are processing was received from a block or from gossip. @@ -377,8 +374,9 @@ pub struct ForkChoice { fc_store: T, /// The underlying representation of the block DAG. proto_array: ProtoArrayForkChoice, - /// Attestations that arrived at the current slot and must be queued for later processing. - queued_attestations: Vec, + /// Attestations that arrived at the current slot and must be queued for later processing, + /// keyed by their slot. + queued_attestations: BTreeMap>, /// Stores a cache of the values required to be sent to the execution layer. forkchoice_update_parameters: ForkchoiceUpdateParameters, _phantom: PhantomData, @@ -475,7 +473,7 @@ where let mut fork_choice = Self { fc_store, proto_array, - queued_attestations: vec![], + queued_attestations: BTreeMap::new(), // This will be updated during the next call to `Self::get_head`. forkchoice_update_parameters: ForkchoiceUpdateParameters { head_hash: None, @@ -1367,8 +1365,11 @@ where // Attestations can only affect the fork choice of subsequent slots. // Delay consideration in the fork choice until their slot is in the past. // ``` + let queued_attestation = QueuedAttestation::from(attestation); self.queued_attestations - .push(QueuedAttestation::from(attestation)); + .entry(queued_attestation.slot) + .or_default() + .push(queued_attestation); } Ok(()) @@ -1560,10 +1561,11 @@ where /// Processes and removes from the queue any queued attestations which may now be eligible for /// processing due to the slot clock incrementing. fn process_attestation_queue(&mut self) -> Result<(), Error> { - for attestation in dequeue_attestations( + let dequeued = dequeue_attestations( self.fc_store.get_current_slot(), &mut self.queued_attestations, - ) { + ); + for attestation in dequeued.into_values().flatten() { for validator_index in attestation.attesting_indices.iter() { self.proto_array.process_attestation( *validator_index as usize, @@ -1826,8 +1828,8 @@ where &self.fc_store } - /// Returns a reference to the currently queued attestations. - pub fn queued_attestations(&self) -> &[QueuedAttestation] { + /// Returns a reference to the currently queued attestations, keyed by slot. + pub fn queued_attestations(&self) -> &BTreeMap> { &self.queued_attestations } @@ -1912,7 +1914,7 @@ where let mut fork_choice = Self { fc_store, proto_array, - queued_attestations: vec![], + queued_attestations: BTreeMap::new(), // Will be updated in the following call to `Self::get_head`. forkchoice_update_parameters: ForkchoiceUpdateParameters { head_hash: None, @@ -2025,20 +2027,33 @@ mod tests { } } - fn get_queued_attestations() -> Vec { - (1..4) - .map(|i| QueuedAttestation { - slot: Slot::new(i), + fn queue_from_slots( + slots: impl IntoIterator, + ) -> BTreeMap> { + let mut queued: BTreeMap> = BTreeMap::new(); + for i in slots { + let slot = Slot::new(i); + queued.entry(slot).or_default().push(QueuedAttestation { + slot, attesting_indices: vec![], block_root: Hash256::zero(), target_epoch: Epoch::new(0), payload_present: false, - }) - .collect() + }); + } + queued } - fn get_slots(queued_attestations: &[QueuedAttestation]) -> Vec { - queued_attestations.iter().map(|a| a.slot.into()).collect() + fn get_queued_attestations() -> BTreeMap> { + queue_from_slots(1..4) + } + + fn get_slots(queued_attestations: &BTreeMap>) -> Vec { + queued_attestations + .values() + .flatten() + .map(|a| a.slot.into()) + .collect() } fn test_queued_attestations(current_time: Slot) -> (Vec, Vec) { @@ -2070,4 +2085,25 @@ mod tests { assert!(queued.is_empty()); assert_eq!(dequeued, vec![1, 2, 3]); } + + #[test] + fn dequeue_attestations_out_of_order() { + // A future-slot vote enqueued before a vote that becomes due sooner must not block the + // due vote from being released. + let mut queued = queue_from_slots([4, 3]); + + // At slot 4, the slot-3 vote is due (3 < 4) and must be released. + let dequeued = dequeue_attestations(Slot::new(4), &mut queued); + + assert_eq!( + get_slots(&dequeued), + vec![3], + "slot-3 vote must be dequeued at slot 4" + ); + assert_eq!( + get_slots(&queued), + vec![4], + "only the not-yet-due slot-4 vote should remain" + ); + } } diff --git a/consensus/fork_choice/src/lib.rs b/consensus/fork_choice/src/lib.rs index dcc499547b..453926fbee 100644 --- a/consensus/fork_choice/src/lib.rs +++ b/consensus/fork_choice/src/lib.rs @@ -6,7 +6,7 @@ pub use crate::fork_choice::{ AttestationFromBlock, Error, ForkChoice, ForkChoiceView, ForkchoiceUpdateParameters, InvalidAttestation, InvalidBlock, InvalidPayloadAttestation, ParentImportStatus, PayloadVerificationStatus, PersistedForkChoice, PersistedForkChoiceV28, PersistedForkChoiceV29, - QueuedAttestation, ResetPayloadStatuses, + QueuedAttestation, ResetPayloadStatuses, dequeue_attestations, }; pub use fork_choice_store::ForkChoiceStore; pub use proto_array::{ diff --git a/consensus/fork_choice/src/metrics.rs b/consensus/fork_choice/src/metrics.rs index b5cda2f587..2ad18fc1ed 100644 --- a/consensus/fork_choice/src/metrics.rs +++ b/consensus/fork_choice/src/metrics.rs @@ -49,7 +49,11 @@ pub static FORK_CHOICE_ON_ATTESTER_SLASHING_TIMES: LazyLock> = pub fn scrape_for_metrics, E: EthSpec>(fork_choice: &ForkChoice) { set_gauge( &FORK_CHOICE_QUEUED_ATTESTATIONS, - fork_choice.queued_attestations().len() as i64, + fork_choice + .queued_attestations() + .values() + .map(Vec::len) + .sum::() as i64, ); set_gauge( &FORK_CHOICE_NODES, diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index 02229e6f33..9b3f5d8857 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -149,13 +149,17 @@ impl ForkChoiceTest { .fork_choice_write_lock() .update_time(self.harness.chain.slot().unwrap()) .unwrap(); - func( - self.harness - .chain - .canonical_head - .fork_choice_read_lock() - .queued_attestations(), - ); + let queued = self + .harness + .chain + .canonical_head + .fork_choice_read_lock() + .queued_attestations() + .values() + .flatten() + .cloned() + .collect::>(); + func(&queued); self } @@ -415,6 +419,24 @@ impl ForkChoiceTest { async fn apply_attestation_to_chain( self, delay: MutationDelay, + mutation_func: F, + comparison_func: G, + ) -> Self + where + F: FnMut(&mut IndexedAttestation, &BeaconChain>), + G: FnMut(Result<(), BeaconChainError>), + { + self.apply_nth_attestation_to_chain(0, delay, mutation_func, comparison_func) + .await + } + + /// Like `apply_attestation_to_chain`, but attests with the validator at + /// `validator_index_in_committee` within the committee. Lets a test enqueue multiple distinct + /// votes for the same slot without tripping `PriorAttestationKnown`. + async fn apply_nth_attestation_to_chain( + self, + validator_index_in_committee: usize, + delay: MutationDelay, mut mutation_func: F, mut comparison_func: G, ) -> Self @@ -431,18 +453,16 @@ impl ForkChoiceTest { .produce_unaggregated_attestation(current_slot, 0) .expect("should not error while producing attestation"); - let validator_committee_index = 0; + // For these tests we always use committee index 0, which also matches the "dummy" committee + // index used post-Electra. + let committee_index = 0; + let validator_index = *head .beacon_state - .get_beacon_committee( - current_slot, - attestation - .committee_index() - .expect("should get committee index"), - ) + .get_beacon_committee(current_slot, committee_index) .expect("should get committees") .committee - .get(validator_committee_index) + .get(validator_index_in_committee) .expect("there should be an attesting validator"); let committee_count = head @@ -452,7 +472,7 @@ impl ForkChoiceTest { let subnet_id = SubnetId::compute_subnet::( current_slot, - 0, + committee_index, committee_count, &self.harness.chain.spec, ) @@ -463,7 +483,7 @@ impl ForkChoiceTest { attestation .sign( &validator_sk, - validator_committee_index, + committee_index as usize, &head.beacon_state.fork(), self.harness.chain.genesis_validators_root, &self.harness.chain.spec, @@ -472,7 +492,7 @@ impl ForkChoiceTest { let single_attestation = SingleAttestation { attester_index: validator_index as u64, - committee_index: validator_committee_index as u64, + committee_index, data: attestation.data().clone(), signature: attestation.signature().clone(), }; @@ -1039,6 +1059,100 @@ async fn invalid_attestation_delayed_slot() { .inspect_queued_attestations(|queue| assert_eq!(queue.len(), 0)); } +/// Regression test for dequeuing when votes for two different future slots are queued. +/// +/// With votes queued for consecutive slots, advancing the clock past the earlier one must release +/// only that vote and leave the later one queued until its own slot is in the past. +#[tokio::test] +async fn dequeue_attestations_consecutive_slot_divergence() { + ForkChoiceTest::new() + .apply_blocks_without_new_attestations(1) + .await + .inspect_queued_attestations(|queue| assert_eq!(queue.len(), 0)) + // Queue a vote for `slot + 2`. + .apply_nth_attestation_to_chain( + 0, + MutationDelay::NoDelay, + |attestation, _| { + let slot = attestation.data().slot; + attestation.data_mut().slot = slot + 2; + }, + |result| assert!(result.is_ok()), + ) + .await + // Queue a vote for `slot + 1`, which becomes due sooner. + // A different committee position avoids `PriorAttestationKnown`. + .apply_nth_attestation_to_chain( + 1, + MutationDelay::NoDelay, + |attestation, _| { + let slot = attestation.data().slot; + attestation.data_mut().slot = slot + 1; + }, + |result| assert!(result.is_ok()), + ) + .await + .inspect_queued_attestations(|queue| assert_eq!(queue.len(), 2)) + // Advance so the slot+1 vote is due (in the past) but the slot+2 vote is not yet. + .skip_slots(2) + .inspect_queued_attestations(|queue| { + assert_eq!( + queue.len(), + 1, + "only the due slot+1 vote should be dequeued" + ); + assert_eq!( + queue[0].slot, + Slot::new(3), + "the surviving vote must be the not-yet-due slot+2 vote" + ); + }); +} + +/// Companion to `dequeue_attestations_consecutive_slot_divergence`: votes for two different slots +/// are queued, but the clock is advanced far enough that *both* are due at dequeue time. +/// +/// When every queued vote is in the past, the whole queue drains in a single dequeue. +#[tokio::test] +async fn dequeue_attestations_conciliation() { + ForkChoiceTest::new() + .apply_blocks_without_new_attestations(1) + .await + .inspect_queued_attestations(|queue| assert_eq!(queue.len(), 0)) + // Queue a vote for `slot + 2`. + .apply_nth_attestation_to_chain( + 0, + MutationDelay::NoDelay, + |attestation, _| { + let slot = attestation.data().slot; + attestation.data_mut().slot = slot + 2; + }, + |result| assert!(result.is_ok()), + ) + .await + // Queue a vote for `slot + 1`. + .apply_nth_attestation_to_chain( + 1, + MutationDelay::NoDelay, + |attestation, _| { + let slot = attestation.data().slot; + attestation.data_mut().slot = slot + 1; + }, + |result| assert!(result.is_ok()), + ) + .await + .inspect_queued_attestations(|queue| assert_eq!(queue.len(), 2)) + // Advance past both votes (to slot + 3) so the whole queue drains. + .skip_slots(3) + .inspect_queued_attestations(|queue| { + assert_eq!( + queue.len(), + 0, + "all votes are due, so the entire queue must drain" + ); + }); +} + /// Tests that the correct target root is used when the attested-to block is in a prior epoch to /// the attestation. #[tokio::test] diff --git a/consensus/types/src/execution/signed_execution_payload_bid.rs b/consensus/types/src/execution/signed_execution_payload_bid.rs index 2ad6dcea1a..7dd6a88a1a 100644 --- a/consensus/types/src/execution/signed_execution_payload_bid.rs +++ b/consensus/types/src/execution/signed_execution_payload_bid.rs @@ -37,6 +37,10 @@ impl SignedExecutionPayloadBid { signature: Signature::empty(), } } + + pub fn num_blobs_expected(&self) -> usize { + self.message.blob_kzg_commitments.len() + } } #[cfg(test)]