From dac8a6ec8d4c22446daf68294b852e0c3ca873f0 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Fri, 1 May 2026 03:46:10 +0200 Subject: [PATCH 1/3] Gloas: fix test failures (KZG verifier wiring, harness columns, WSS sync) Brings the FORK_NAME=gloas beacon_chain test suite from 31 failures to green: - v1 KZG batch verifier couldn't verify Gloas columns. Added verify_columns_against_block helper that picks commitments per fork (Fulu: inline on column; Gloas: signed_execution_payload_bid). - BeaconChainHarness::process_envelope didn't persist columns. Now mirrors what production does in import_available_execution_payload_envelope. - get_or_reconstruct_blobs returned an error for Gloas. Now short-circuits to Ok(None); WSS test copies columns from source to dest directly. - update_data_column_signed_header (block_verification tests) only handled Fulu shape. Added a Gloas branch that re-keys to canonical_root. - BlockError::EnvelopeBlockRootUnknown changed to tuple variant. - Removed duplicate process_payload_envelope_availability. --- beacon_node/beacon_chain/src/beacon_chain.rs | 20 +++++-- .../beacon_chain/src/block_verification.rs | 2 +- .../src/data_availability_checker.rs | 50 ++++++++++++---- beacon_node/beacon_chain/src/kzg_utils.rs | 19 ++++-- .../payload_envelope_verification/import.rs | 4 +- .../pending_payload_cache/pending_column.rs | 14 +++-- .../pending_components.rs | 6 +- beacon_node/beacon_chain/src/test_utils.rs | 46 ++++++++++++--- .../beacon_chain/tests/block_verification.rs | 40 +++++++++---- beacon_node/beacon_chain/tests/store_tests.rs | 58 ++++++++++++++++++- .../gossip_methods.rs | 2 +- 11 files changed, 205 insertions(+), 56 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index a7f289b3f1..0a48a3b168 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -1331,6 +1331,14 @@ impl BeaconChain { return Ok(None); }; + // Gloas removes the standalone `BlobSidecar` shape — KZG commitments live in the bid and + // there's no signed-block-header / inclusion-proof to populate a `BlobSidecar` from. The + // canonical data is the column sidecar set on disk; callers needing data for a Gloas + // block should consume columns directly via `get_data_columns`. + if block.fork_name_unchecked().gloas_enabled() { + return Ok(None); + } + if self.spec.is_peer_das_enabled_for_epoch(block.epoch()) { let fork_name = self.spec.fork_name_at_epoch(block.epoch()); if let Some(columns) = self.store.get_data_columns(block_root, fork_name)? { @@ -3442,7 +3450,7 @@ impl BeaconChain { .gloas_enabled() { let bid = load_gloas_payload_bid(block_root, self)? - .ok_or(BlockError::EnvelopeBlockRootUnknown { block_root })?; + .ok_or(BlockError::EnvelopeBlockRootUnknown(block_root))?; let availability = self .pending_payload_cache .put_kzg_verified_custody_data_columns( @@ -3680,7 +3688,7 @@ impl BeaconChain { if is_gloas { let bid = load_gloas_payload_bid(block_root, self)? - .ok_or(BlockError::EnvelopeBlockRootUnknown { block_root })?; + .ok_or(BlockError::EnvelopeBlockRootUnknown(block_root))?; let pending_payload_cache = self.pending_payload_cache.clone(); let result = self .task_executor @@ -4005,7 +4013,7 @@ impl BeaconChain { .gloas_enabled() { let bid = load_gloas_payload_bid(block_root, self)? - .ok_or(BlockError::EnvelopeBlockRootUnknown { block_root })?; + .ok_or(BlockError::EnvelopeBlockRootUnknown(block_root))?; let availability = self .pending_payload_cache .put_gossip_verified_data_columns(block_root, bid, data_columns)?; @@ -4111,7 +4119,7 @@ impl BeaconChain { .gloas_enabled() { let bid = load_gloas_payload_bid(block_root, self)? - .ok_or(BlockError::EnvelopeBlockRootUnknown { block_root })?; + .ok_or(BlockError::EnvelopeBlockRootUnknown(block_root))?; let availability = self .pending_payload_cache .put_kzg_verified_custody_data_columns(block_root, bid, &data_columns) @@ -4155,7 +4163,7 @@ impl BeaconChain { .gloas_enabled() { let bid = load_gloas_payload_bid(block_root, self)? - .ok_or(BlockError::EnvelopeBlockRootUnknown { block_root })?; + .ok_or(BlockError::EnvelopeBlockRootUnknown(block_root))?; let availability = self .pending_payload_cache .put_rpc_custody_columns(block_root, bid, custody_columns) @@ -7750,7 +7758,7 @@ impl BeaconChain { ) } - pub(crate) fn get_blobs_or_columns_store_op( + pub fn get_blobs_or_columns_store_op( &self, block_root: Hash256, block_slot: Slot, diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index eeee562e9c..24f971f736 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -287,7 +287,7 @@ pub enum BlockError { /// https://github.com/sigp/lighthouse/issues/4546 AvailabilityCheck(AvailabilityCheckError), /// The payload envelope's block root is unknown. - EnvelopeBlockRootUnknown { block_root: Hash256 }, + EnvelopeBlockRootUnknown(Hash256), /// Optimistic sync is not supported for Gloas payload envelopes. OptimisticSyncNotSupported { block_root: Hash256 }, /// A Blob with a slot after PeerDAS is received and is not required to be imported. diff --git a/beacon_node/beacon_chain/src/data_availability_checker.rs b/beacon_node/beacon_chain/src/data_availability_checker.rs index 928d7a1bad..bdf16ec52c 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker.rs @@ -33,6 +33,7 @@ use crate::data_column_verification::{ GossipVerifiedDataColumn, KzgVerifiedCustodyDataColumn, KzgVerifiedDataColumn, verify_kzg_for_data_column_list, }; +use crate::kzg_utils::validate_full_data_columns_with_commitments; use crate::metrics::{ KZG_DATA_COLUMN_RECONSTRUCTION_ATTEMPTS, KZG_DATA_COLUMN_RECONSTRUCTION_FAILURES, }; @@ -490,8 +491,7 @@ impl DataAvailabilityChecker { AvailableBlockData::Blobs(blobs) => verify_kzg_for_blob_list(blobs.iter(), &self.kzg) .map_err(AvailabilityCheckError::InvalidBlobs), AvailableBlockData::DataColumns(columns) => { - verify_kzg_for_data_column_list(columns.iter(), &self.kzg) - .map_err(AvailabilityCheckError::InvalidColumn) + verify_columns_against_block(&self.kzg, available_block.block(), columns) } } } @@ -504,13 +504,17 @@ impl DataAvailabilityChecker { available_blocks: &[AvailableBlock], ) -> Result<(), AvailabilityCheckError> { let mut all_blobs = Vec::new(); - let mut all_data_columns = Vec::new(); for available_block in available_blocks { - match available_block.data().to_owned() { + match available_block.data() { AvailableBlockData::NoData => {} - AvailableBlockData::Blobs(blobs) => all_blobs.extend(blobs), - AvailableBlockData::DataColumns(columns) => all_data_columns.extend(columns), + AvailableBlockData::Blobs(blobs) => all_blobs.extend(blobs.iter().cloned()), + AvailableBlockData::DataColumns(columns) => { + // Each block has its own commitments. For Gloas they live in the bid; for + // Fulu they live inline on the column. Verify per block and let the helper + // pick the right path. + verify_columns_against_block(&self.kzg, available_block.block(), columns)?; + } } } @@ -519,11 +523,6 @@ impl DataAvailabilityChecker { .map_err(AvailabilityCheckError::InvalidBlobs)?; } - if !all_data_columns.is_empty() { - verify_kzg_for_data_column_list(all_data_columns.iter(), &self.kzg) - .map_err(AvailabilityCheckError::InvalidColumn)?; - } - Ok(()) } @@ -679,6 +678,35 @@ impl DataAvailabilityChecker { } } +/// Verify a batch of data columns belonging to a single block, picking the right commitment +/// source for the block's fork (Fulu: inline on column; Gloas: from the embedded payload bid). +fn verify_columns_against_block( + kzg: &Kzg, + block: &SignedBeaconBlock, + columns: &[Arc>], +) -> Result<(), AvailabilityCheckError> { + if columns.is_empty() { + return Ok(()); + } + if block.fork_name_unchecked().gloas_enabled() { + let commitments = block + .message() + .body() + .signed_execution_payload_bid() + .map(|bid| bid.message.blob_kzg_commitments.clone()) + .map_err(|_| { + AvailabilityCheckError::Unexpected( + "Gloas block missing signed_execution_payload_bid".to_string(), + ) + })?; + validate_full_data_columns_with_commitments(kzg, columns.iter(), commitments.as_ref()) + .map_err(AvailabilityCheckError::InvalidColumn) + } else { + verify_kzg_for_data_column_list(columns.iter(), kzg) + .map_err(AvailabilityCheckError::InvalidColumn) + } +} + /// Helper struct to group data availability checker metrics. pub struct DataAvailabilityCheckerMetrics { pub block_cache_size: usize, diff --git a/beacon_node/beacon_chain/src/kzg_utils.rs b/beacon_node/beacon_chain/src/kzg_utils.rs index 5ad1cc115d..e1558f050b 100644 --- a/beacon_node/beacon_chain/src/kzg_utils.rs +++ b/beacon_node/beacon_chain/src/kzg_utils.rs @@ -676,12 +676,19 @@ pub fn reconstruct_blobs( let blob_indices: Vec = match blob_indices_opt { Some(indices) => indices.into_iter().map(|i| i as usize).collect(), None => { - // TODO(gloas): support blob reconstruction for Gloas - // https://github.com/sigp/lighthouse/issues/7413 - let num_of_blobs = first_data_column - .kzg_commitments() - .map_err(|_| "Gloas blob reconstruction not yet supported".to_string())? - .len(); + // Fulu columns carry commitments inline; Gloas columns don't, so fall back to + // the block's payload bid commitments. + let num_of_blobs = match first_data_column.kzg_commitments() { + Ok(commitments) => commitments.len(), + Err(_) => signed_block + .message() + .body() + .signed_execution_payload_bid() + .map(|bid| bid.message.blob_kzg_commitments.len()) + .map_err(|_| { + "Gloas blob reconstruction: block missing payload bid".to_string() + })?, + }; (0..num_of_blobs).collect() } }; diff --git a/beacon_node/beacon_chain/src/payload_envelope_verification/import.rs b/beacon_node/beacon_chain/src/payload_envelope_verification/import.rs index 10d97add1d..e7c900dcd7 100644 --- a/beacon_node/beacon_chain/src/payload_envelope_verification/import.rs +++ b/beacon_node/beacon_chain/src/payload_envelope_verification/import.rs @@ -163,7 +163,7 @@ impl BeaconChain { let slot = envelope.envelope.slot(); let block_root = envelope.block_root; let bid = load_gloas_payload_bid(block_root, self)? - .ok_or(BlockError::EnvelopeBlockRootUnknown { block_root })?; + .ok_or(BlockError::EnvelopeBlockRootUnknown(block_root))?; let availability = self .pending_payload_cache .put_executed_payload_envelope(bid, envelope)?; @@ -253,7 +253,7 @@ impl BeaconChain { // been imported. We don't want to repeat work importing a block that is already imported. let fork_choice_reader = self.canonical_head.fork_choice_upgradable_read_lock(); if !fork_choice_reader.contains_block(&block_root) { - return Err(BlockError::EnvelopeBlockRootUnknown { block_root }); + return Err(BlockError::EnvelopeBlockRootUnknown(block_root)); } // TODO(gloas) add defensive check to see if payload envelope is already in fork choice 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 1c7a0389c3..0844bbf0f9 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,13 +8,15 @@ 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]`. + pub fn new_with_capacity(blob_count: usize) -> Self { + Self { + cells: vec![None; blob_count], + } + } + pub fn insert(&mut self, index: usize, cell: &Cell, proof: &KzgProof) { if let Some(existing_cell) = self.cells.get_mut(index) && existing_cell.is_none() 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 20e6d1b52f..0516afd4e9 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 @@ -79,12 +79,16 @@ impl PendingComponents { &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(); + // The Vec-backed `PendingColumn` keys cells by index, so we have to allocate up to + // `num_blobs_expected` entries before inserting; otherwise `cells.get_mut(idx)` returns + // None and the insert is a no-op. let col = self .verified_data_columns .entry(*data_column.index()) - .or_default(); + .or_insert_with(|| PendingColumn::new_with_capacity(num_blobs_expected)); for (cell_idx, (cell, proof)) in data_column .column() .iter() diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 4a9b7f7fa0..68cec11b76 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -2832,11 +2832,42 @@ where .await .expect("newPayload should succeed"); - // Store the envelope. + // Store the envelope and the data columns derived from the block. + // + // Production stores columns inside `import_available_execution_payload_envelope` after + // the cache is satisfied. The harness sidesteps that flow but must still persist columns + // or the `DataColumnMissing` invariant fires for any block with `num_expected_blobs > 0`. + let block = self + .chain + .store + .get_blinded_block(&block_root) + .expect("should read block from store") + .expect("block should exist in store"); + let mut ops = vec![]; + let block_with_full_payload = self + .chain + .store + .make_full_block(&block_root, block.clone()) + .expect("should reconstruct full block"); + let columns = + generate_data_column_sidecars_from_block(&block_with_full_payload, &self.spec); + if !columns.is_empty() + && let Some(store_op) = self.chain.get_blobs_or_columns_store_op( + block_root, + block.slot(), + AvailableBlockData::DataColumns(columns), + ) + { + ops.push(store_op); + } + ops.push(store::StoreOp::PutPayloadEnvelope( + block_root, + std::sync::Arc::new(signed_envelope), + )); self.chain .store - .put_payload_envelope(&block_root, &signed_envelope) - .expect("should store envelope"); + .do_atomically_with_block_and_blobs_cache(ops) + .expect("should persist envelope and columns"); // Update fork choice so it knows the payload was received. self.chain @@ -2857,11 +2888,10 @@ where block: Arc>, ) -> RangeSyncBlock { let block_root = block_root.unwrap_or_else(|| get_block_root(&block)); - let has_blobs = block - .message() - .body() - .blob_kzg_commitments() - .is_ok_and(|c| !c.is_empty()); + // 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 RangeSyncBlock::new( block, diff --git a/beacon_node/beacon_chain/tests/block_verification.rs b/beacon_node/beacon_chain/tests/block_verification.rs index af858874b2..38636540a1 100644 --- a/beacon_node/beacon_chain/tests/block_verification.rs +++ b/beacon_node/beacon_chain/tests/block_verification.rs @@ -323,18 +323,34 @@ fn update_data_column_signed_header( ) { for old_custody_column_sidecar in data_columns.as_mut_slice() { let old_column_sidecar = old_custody_column_sidecar.as_data_column(); - let new_column_sidecar = Arc::new(DataColumnSidecar::Fulu(DataColumnSidecarFulu { - index: *old_column_sidecar.index(), - column: old_column_sidecar.column().clone(), - kzg_commitments: old_column_sidecar.kzg_commitments().unwrap().clone(), - kzg_proofs: old_column_sidecar.kzg_proofs().clone(), - signed_block_header: signed_block.signed_block_header(), - kzg_commitments_inclusion_proof: signed_block - .message() - .body() - .kzg_commitments_merkle_proof() - .unwrap(), - })); + let new_column_sidecar = match old_column_sidecar.as_ref() { + DataColumnSidecar::Fulu(_) => { + Arc::new(DataColumnSidecar::Fulu(DataColumnSidecarFulu { + index: *old_column_sidecar.index(), + column: old_column_sidecar.column().clone(), + kzg_commitments: old_column_sidecar.kzg_commitments().unwrap().clone(), + kzg_proofs: old_column_sidecar.kzg_proofs().clone(), + signed_block_header: signed_block.signed_block_header(), + kzg_commitments_inclusion_proof: signed_block + .message() + .body() + .kzg_commitments_merkle_proof() + .unwrap(), + })) + } + // Gloas columns reference the block by `beacon_block_root` instead of holding the + // block header inline, so updating the parent root just means re-keying the column to + // the new canonical root. + DataColumnSidecar::Gloas(g) => { + Arc::new(DataColumnSidecar::Gloas(types::DataColumnSidecarGloas { + index: g.index, + column: g.column.clone(), + kzg_proofs: g.kzg_proofs.clone(), + slot: g.slot, + beacon_block_root: signed_block.canonical_root(), + })) + } + }; *old_custody_column_sidecar = CustodyDataColumn::from_asserted_custody(new_column_sidecar); } } diff --git a/beacon_node/beacon_chain/tests/store_tests.rs b/beacon_node/beacon_chain/tests/store_tests.rs index 86adf50995..171c83b2a9 100644 --- a/beacon_node/beacon_chain/tests/store_tests.rs +++ b/beacon_node/beacon_chain/tests/store_tests.rs @@ -3133,6 +3133,29 @@ async fn weak_subjectivity_sync_test( let beacon_chain = Arc::new(beacon_chain); let wss_block_root = wss_block.canonical_root(); + + // For Gloas, blobs aren't a standalone shape — the WSS data is the column sidecar set, which + // `get_or_reconstruct_blobs` returns `None` for. Copy the WSS block's columns straight from + // the source store so that the destination has them after checkpoint sync, matching what + // network-driven WSS would produce in production. + if wss_block.fork_name_unchecked().gloas_enabled() + && let Ok(Some(source_columns)) = harness + .chain + .store + .get_data_columns(&wss_block_root, ForkName::Gloas) + && !source_columns.is_empty() + && let Some(store_op) = beacon_chain.get_blobs_or_columns_store_op( + wss_block_root, + wss_block.slot(), + beacon_chain::block_verification_types::AvailableBlockData::DataColumns(source_columns), + ) + { + beacon_chain + .store + .do_atomically_with_block_and_blobs_cache(vec![store_op]) + .unwrap(); + } + let store_wss_block = harness .chain .get_block(&wss_block_root) @@ -3200,12 +3223,43 @@ async fn weak_subjectivity_sync_test( .await .unwrap(); - // Store the envelope and apply it to fork choice. + // Store the envelope, its columns, and apply to fork choice. if let Some(envelope) = &snapshot.execution_envelope { + // Persist data columns for Gloas blocks. This mirrors what production does in + // `import_available_execution_payload_envelope` and what the harness now does in + // `process_envelope` — the WSS forward-sync loop bypasses both, so do it directly. + let mut ops = vec![]; + let columns_block = beacon_chain + .store + .get_blinded_block(&block_root) + .unwrap() + .and_then(|b| beacon_chain.store.make_full_block(&block_root, b).ok()); + if let Some(full_block) = columns_block { + let columns = beacon_chain::test_utils::generate_data_column_sidecars_from_block( + &full_block, + &beacon_chain.spec, + ); + if !columns.is_empty() + && let Some(store_op) = beacon_chain.get_blobs_or_columns_store_op( + block_root, + full_block.slot(), + beacon_chain::block_verification_types::AvailableBlockData::DataColumns( + columns, + ), + ) + { + ops.push(store_op); + } + } + ops.push(store::StoreOp::PutPayloadEnvelope( + block_root, + std::sync::Arc::new(envelope.as_ref().clone()), + )); beacon_chain .store - .put_payload_envelope(&block_root, envelope) + .do_atomically_with_block_and_blobs_cache(ops) .unwrap(); + // Update fork choice so head selection accounts for Full payload status. beacon_chain .canonical_head 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 aec2b1fcb8..4e636060e0 100644 --- a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs @@ -1848,7 +1848,7 @@ impl NetworkBeaconProcessor { // BlobNotRequired is unreachable. Only constructed in `process_gossip_blob` Err(e @ BlockError::InternalError(_)) | Err(e @ BlockError::BlobNotRequired(_)) - | Err(e @ BlockError::EnvelopeBlockRootUnknown { .. }) + | Err(e @ BlockError::EnvelopeBlockRootUnknown(_)) | Err(e @ BlockError::OptimisticSyncNotSupported { .. }) => { error!(error = %e, "Internal block gossip validation error"); return None; 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 2/3] 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); } From d75963bb865090d1f20c0221aa4fa77d44071bb7 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Fri, 1 May 2026 10:28:59 +0200 Subject: [PATCH 3/3] Address remaining review comments - pending_column.rs: split try_to_sidecar into is_complete-checked to_sidecar with typed PendingColumnError so 'incomplete column' is no longer conflated with VariableList size-bound failures (jimmygchen, dapplion). - pending_components.rs: get_cached_data_columns filters by is_complete first, then logs an error if a complete column fails to assemble (dknopik's sanity check on filter_map silent drops). - data_column_verification.rs: add the missing column.slot == bid.slot consistency check in validate_data_column_sidecar_for_gossip_gloas, using the previously-defined-but-unused BlockSlotMismatch error variant (jimmygchen). --- .../src/data_column_verification.rs | 6 +++ .../pending_payload_cache/pending_column.rs | 39 ++++++++++++------- .../pending_components.rs | 33 +++++++++++----- 3 files changed, 53 insertions(+), 25 deletions(-) diff --git a/beacon_node/beacon_chain/src/data_column_verification.rs b/beacon_node/beacon_chain/src/data_column_verification.rs index d205604a71..1c1ba74582 100644 --- a/beacon_node/beacon_chain/src/data_column_verification.rs +++ b/beacon_node/beacon_chain/src/data_column_verification.rs @@ -1097,6 +1097,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/pending_column.rs b/beacon_node/beacon_chain/src/pending_payload_cache/pending_column.rs index 6fd4428f56..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 @@ -36,24 +36,22 @@ 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); @@ -61,13 +59,24 @@ impl PendingColumn { // 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 { + 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 b9679a12a3..4ff429fd87 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::{ AbstractExecPayload, BeaconStateError, ColumnIndex, EthSpec, Hash256, SignedBeaconBlock, @@ -48,18 +48,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() }