diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 91aefda40f..5253c85bbc 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -1186,9 +1186,9 @@ impl BeaconChain { indices: &[ColumnIndex], ) -> Result, Error> { let all_cached_columns_opt = self - .pending_payload_cache + .data_availability_checker .get_data_columns(block_root) - .or_else(|| self.data_availability_checker.get_data_columns(block_root)) + .or_else(|| self.pending_payload_cache.get_data_columns(block_root)) .or_else(|| self.early_attester_cache.get_data_columns(block_root)); if let Some(mut all_cached_columns) = all_cached_columns_opt { diff --git a/beacon_node/beacon_chain/src/data_availability_checker.rs b/beacon_node/beacon_chain/src/data_availability_checker.rs index bdf16ec52c..54f21b97e0 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker.rs @@ -21,7 +21,7 @@ use tracing::{debug, error, instrument}; use types::data::{BlobIdentifier, FixedBlobSidecarList, PartialDataColumn}; use types::{ BlobSidecar, BlobSidecarList, BlockImportSource, ChainSpec, DataColumnSidecar, - DataColumnSidecarList, Epoch, EthSpec, ForkName, Hash256, PartialDataColumnSidecarError, + DataColumnSidecarList, Epoch, EthSpec, Hash256, PartialDataColumnSidecarError, PartialDataColumnSidecarRef, SignedBeaconBlock, Slot, new_non_zero_usize, }; @@ -906,7 +906,7 @@ impl AvailableBlock { match &block_data { AvailableBlockData::NoData => { // For Gloas, DA is checked for the PayloadEnvelope, not for the block. - if block.fork_name_unchecked() < ForkName::Gloas { + if !block.fork_name_unchecked().gloas_enabled() { if columns_required { return Err(AvailabilityCheckError::MissingCustodyColumns); } else if blobs_required { diff --git a/beacon_node/beacon_chain/src/data_column_verification.rs b/beacon_node/beacon_chain/src/data_column_verification.rs index 2cc078871b..08c4bbd5fd 100644 --- a/beacon_node/beacon_chain/src/data_column_verification.rs +++ b/beacon_node/beacon_chain/src/data_column_verification.rs @@ -1096,6 +1096,12 @@ pub fn validate_data_column_sidecar_for_gossip_gloas< slot: column_slot, }, )?; + if bid.message.slot != column_slot { + return Err(GossipDataColumnError::BlockSlotMismatch { + block_slot: bid.message.slot, + data_column_slot: column_slot, + }); + } let kzg_commitments = &bid.message.blob_kzg_commitments; verify_data_column_sidecar_with_commitments_len( &data_column, 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 3aaf688602..fbe68c97b2 100644 --- a/beacon_node/beacon_chain/src/pending_payload_cache/mod.rs +++ b/beacon_node/beacon_chain/src/pending_payload_cache/mod.rs @@ -184,7 +184,6 @@ impl PendingPayloadCache { let pending_components = self.get_pending_components(beacon_block_root, bid, |pending_components| { pending_components.insert_executed_payload_envelope(executed_envelope); - Ok(()) })?; let num_expected_columns = self @@ -422,14 +421,14 @@ impl PendingPayloadCache { update_fn: F, ) -> Result>, AvailabilityCheckError> where - F: FnOnce(&mut PendingComponents) -> Result<(), AvailabilityCheckError>, + F: FnOnce(&mut PendingComponents), { let mut write_lock = self.availability_cache.write(); { let pending_components = write_lock .get_or_insert_mut(block_root, || PendingComponents::new(block_root, bid)); - update_fn(pending_components)? + update_fn(pending_components) } RwLockReadGuard::try_map(RwLockWriteGuard::downgrade(write_lock), |cache| { @@ -622,15 +621,7 @@ mod data_availability_checker_tests { const RNG_SEED: u64 = 0xDEADBEEF; fn gloas_spec() -> Arc { - let mut spec = E::default_spec(); - spec.altair_fork_epoch = Some(Epoch::new(0)); - spec.bellatrix_fork_epoch = Some(Epoch::new(0)); - spec.capella_fork_epoch = Some(Epoch::new(0)); - spec.deneb_fork_epoch = Some(Epoch::new(0)); - spec.electra_fork_epoch = Some(Epoch::new(0)); - spec.fulu_fork_epoch = Some(Epoch::new(0)); - spec.gloas_fork_epoch = Some(Epoch::new(0)); - Arc::new(spec) + Arc::new(ForkName::Gloas.make_genesis_spec(E::default_spec())) } fn get_store_with_spec( diff --git a/beacon_node/beacon_chain/src/pending_payload_cache/pending_column.rs b/beacon_node/beacon_chain/src/pending_payload_cache/pending_column.rs index db13bec574..22bb6d3620 100644 --- a/beacon_node/beacon_chain/src/pending_payload_cache/pending_column.rs +++ b/beacon_node/beacon_chain/src/pending_payload_cache/pending_column.rs @@ -8,12 +8,6 @@ pub struct PendingColumn { cells: Vec, KzgProof)>>, } -impl Default for PendingColumn { - fn default() -> Self { - Self { cells: Vec::new() } - } -} - impl PendingColumn { /// Allocate a `PendingColumn` whose `cells` vec has space for `blob_count` entries, all /// initialised to `None`. Required so that `insert(idx, ...)` can write into `cells[idx]`. @@ -42,36 +36,47 @@ impl PendingColumn { self.cells.len() == blob_count && self.cells.iter().all(|cell| cell.is_some()) } - pub fn try_to_sidecar( + /// Build a `DataColumnSidecar` from the cached cells. + /// + /// Caller MUST have checked `is_complete(blob_count)` first; this returns `Err` only on the + /// (currently theoretically impossible) `VariableList` size-bound failures, which we surface + /// as a typed error so the caller can log/metric it instead of silently producing nothing. + pub fn to_sidecar( &self, index: ColumnIndex, slot: Slot, beacon_block_root: Hash256, - blob_count: usize, - ) -> Option>> { - if self.cells.len() != blob_count { - return None; - } - - let mut column = Vec::with_capacity(blob_count); + ) -> Result>, PendingColumnError> { + let mut column = Vec::with_capacity(self.cells.len()); let mut kzg_proofs = Vec::with_capacity(self.cells.len()); for cell in self.cells.iter() { - let Some((cell, proof)) = cell else { - return None; - }; + let (cell, proof) = cell.as_ref().ok_or(PendingColumnError::IncompleteColumn)?; // TODO(gloas): we likely want to go and arc all cells column.push(cell.clone()); kzg_proofs.push(*proof); } - Some(Arc::new(DataColumnSidecar::Gloas(DataColumnSidecarGloas { + // TODO(gloas): this hard-codes the Gloas sidecar variant. Pass the fork in once + // post-Gloas variants are introduced (or move construction to a fork-aware helper). + Ok(Arc::new(DataColumnSidecar::Gloas(DataColumnSidecarGloas { index, - // TODO(gloas): this should not error, but we need to catch it - column: VariableList::try_from(column).ok()?, - kzg_proofs: VariableList::try_from(kzg_proofs).ok()?, + column: VariableList::try_from(column) + .map_err(|_| PendingColumnError::ColumnSizeExceedsBound)?, + kzg_proofs: VariableList::try_from(kzg_proofs) + .map_err(|_| PendingColumnError::ProofsSizeExceedsBound)?, slot, beacon_block_root, }))) } } + +/// Errors returned by [`PendingColumn::to_sidecar`]. `IncompleteColumn` should never fire if the +/// caller checks [`PendingColumn::is_complete`] first; the size-bound variants reflect spec-bound +/// invariants and should never fire in practice. +#[derive(Debug, Clone)] +pub enum PendingColumnError { + IncompleteColumn, + ColumnSizeExceedsBound, + ProofsSizeExceedsBound, +} 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 63c5f4dcda..9bf6eb6436 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 @@ -7,7 +7,7 @@ use crate::pending_payload_cache::pending_column::PendingColumn; use std::cmp::Ordering; use std::collections::HashMap; use std::sync::Arc; -use tracing::{Span, debug, debug_span}; +use tracing::{Span, debug, debug_span, error}; use types::DataColumnSidecar; use types::{ColumnIndex, EthSpec, Hash256, SignedExecutionPayloadBid}; @@ -30,18 +30,31 @@ impl PendingComponents { self.bid.message.blob_kzg_commitments.len() } - /// Returns the completed custody columns + /// Returns the completed custody columns. + /// + /// Skips columns that are not yet complete and logs an error if a complete column fails to + /// build (a spec-bound invariant would have to be violated; should never happen in practice). pub fn get_cached_data_columns(&self) -> Vec>> { + let blob_count = self.num_blobs_expected(); + let slot = self.bid.message.slot; + let block_root = self.block_root; self.verified_data_columns .iter() - .filter_map(|(col_idx, col)| { - col.try_to_sidecar( - *col_idx, - self.bid.message.slot, - self.block_root, - self.num_blobs_expected(), - ) - }) + .filter(|(_, col)| col.is_complete(blob_count)) + .filter_map( + |(col_idx, col)| match col.to_sidecar(*col_idx, slot, block_root) { + Ok(sidecar) => Some(sidecar), + Err(e) => { + error!( + ?e, + column_index = %col_idx, + ?block_root, + "Failed to build sidecar for complete column" + ); + None + } + }, + ) .collect() } @@ -60,7 +73,7 @@ impl PendingComponents { pub(crate) fn merge_data_columns( &mut self, kzg_verified_data_columns: &[KzgVerifiedCustodyDataColumn], - ) -> Result<(), AvailabilityCheckError> { + ) { let num_blobs_expected = self.num_blobs_expected(); for data_column in kzg_verified_data_columns { let data_column = data_column.as_data_column(); @@ -80,8 +93,6 @@ impl PendingComponents { col.insert(cell_idx, cell, proof); } } - - Ok(()) } // TODO(gloas): merge partial columns @@ -97,7 +108,7 @@ impl PendingComponents { pub fn num_completed_columns(&self) -> usize { self.verified_data_columns .values() - .filter_map(|col| col.is_complete(self.num_blobs_expected()).then_some(())) + .filter(|col| col.is_complete(self.num_blobs_expected())) .count() } 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 4e636060e0..b2b5960597 100644 --- a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs @@ -728,14 +728,14 @@ impl NetworkBeaconProcessor { .. } => { debug!( - action = "requesting block", + action = "ignoring", %unknown_block_root, "Unknown block root for column" ); - self.send_sync_message(SyncMessage::UnknownBlockHashFromAttestation( - peer_id, - unknown_block_root, - )); + // TODO(gloas): wire this into proper lookup sync. Sending + // `UnknownBlockHashFromAttestation` here is a Fulu-shaped fallback that + // mixes column processing with the attestation lookup path and is not + // the right primitive for Gloas column lookups. self.propagate_validation_result( message_id, peer_id, diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index 8eba8300cd..68d193ff50 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -906,6 +906,10 @@ impl SyncManager { ); } DataColumnSidecar::Gloas(_) => { + // TODO(gloas): proper lookup sync for Gloas. Routing into + // `handle_unknown_block_root` here mixes column processing with the + // single-block-lookup path; the Gloas column-arrives-before-block + // case wants its own queue/wakeup. debug!(%block_root, "Received unknown block data column message"); self.handle_unknown_block_root(peer_id, block_root); }