From 0ce058835a92e68ae2546dce89f2f8fb13cb0de5 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Fri, 1 May 2026 10:16:06 +0200 Subject: [PATCH] Address review comments - data_availability_checker.rs: use !gloas_enabled() instead of < ForkName::Gloas (jimmygchen, dapplion). - beacon_chain.rs: get_data_columns checks data_availability_checker first, then pending_payload_cache (dapplion). - pending_components.rs: merge_data_columns drops the unused Result return (jimmygchen). num_completed_columns uses filter() instead of filter_map (jimmygchen). - pending_column.rs: TODO marker on the hard-coded Gloas variant in try_to_sidecar (jimmygchen). - pending_payload_cache/mod.rs: gloas_spec test helper collapsed to ForkName::Gloas.make_genesis_spec(E::default_spec()) (jimmygchen). - gossip_methods.rs / sync/manager.rs: replace UnknownBlockHashFromAttestation fallback with TODO(gloas) for proper Gloas lookup sync (dapplion). --- beacon_node/beacon_chain/src/beacon_chain.rs | 4 ++-- .../beacon_chain/src/data_availability_checker.rs | 4 ++-- .../beacon_chain/src/pending_payload_cache/mod.rs | 15 +++------------ .../src/pending_payload_cache/pending_column.rs | 2 ++ .../pending_payload_cache/pending_components.rs | 6 ++---- .../network_beacon_processor/gossip_methods.rs | 10 +++++----- beacon_node/network/src/sync/manager.rs | 4 ++++ 7 files changed, 20 insertions(+), 25 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 0a48a3b168..30beefb8ce 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -1187,9 +1187,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/pending_payload_cache/mod.rs b/beacon_node/beacon_chain/src/pending_payload_cache/mod.rs index b056eb1af9..58a276b179 100644 --- a/beacon_node/beacon_chain/src/pending_payload_cache/mod.rs +++ b/beacon_node/beacon_chain/src/pending_payload_cache/mod.rs @@ -185,7 +185,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 @@ -423,14 +422,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| { @@ -623,15 +622,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 0844bbf0f9..6fd4428f56 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 @@ -59,6 +59,8 @@ impl PendingColumn { kzg_proofs.push(*proof); } + // 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). Some(Arc::new(DataColumnSidecar::Gloas(DataColumnSidecarGloas { index, // TODO(gloas): this should not error, but we need to catch it 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 0516afd4e9..b9679a12a3 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 @@ -78,7 +78,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(); @@ -98,8 +98,6 @@ impl PendingComponents { col.insert(cell_idx, cell, proof); } } - - Ok(()) } // TODO(gloas): merge partial columns @@ -115,7 +113,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); }