perf(beacon): stop re-verifying stored custody columns at block import

crates/blockchain/src/beacon_envelope.rs

@@ -58,7 +58,7 @@ use ethlambda_state_transition::beacon::fork_choice::{
 use ethlambda_state_transition::beacon::stf::ExecutionEngine;
 use ethlambda_state_transition::beacon::stf::gloas as stf_gloas;
 use ethlambda_types::ShortRoot;
-use ethlambda_types::beacon::containers::{DataColumnSidecar, SignedBeaconBlock, gloas};
+use ethlambda_types::beacon::containers::{SignedBeaconBlock, gloas};
 use ethlambda_types::primitives::{H256, HashTreeRoot as _};
 use tracing::{debug, error, info, trace, warn};
 
@@ -416,40 +416,22 @@ impl BlockChainServer {
             return false;
         }
 
-        let sidecars = if commitment_count == 0 {
-            Vec::new()
-        } else {
-            match self.stored_gloas_columns(slot, &root) {
-                Some(sidecars) => sidecars,
-                None => {
-                    // Every sampled column was listed as stored a moment ago,
-                    // so failing to read one back is a storage or decode
-                    // fault, not a wait. Holding would name nothing missing
-                    // and be redriven every slot for as long as the fault
-                    // lasts, so the envelope is dropped (and asked for again
-                    // if children wait on it).
-                    warn!(
-                        %slot,
-                        block_root = %ShortRoot(&root.0),
-                        "Dropping an envelope: a stored sampled column could not be read back"
-                    );
-                    self.refetch_envelope_if_children_wait(root);
-                    return false;
-                }
-            }
-        };
-
+        // Every sampled column is stored, and a stored column has passed the
+        // structural check, the inclusion proof's gloas counterpart and the KZG
+        // batch in the p2p layer (gossip validation, or `chain_checks_gloas`
+        // for a fetched or parked one) against this block's bid, so none is
+        // read back and verified a second time here. With no commitments there
+        // is nothing to sample and nothing to verify.
         // The consensus checks come first, as `verify_execution_payload_envelope`
         // orders them, so the execution client is only ever asked about an
         // envelope this node would accept: signed by the builder, matching
         // the bid and the state, and with its data available.
         self.advance_beacon_clock_to(held.event_ms);
         let config = self.store.config();
-        if let Err(err) = fork_choice::check_execution_payload_envelope(
+        if let Err(err) = fork_choice::check_execution_payload_envelope_columns_verified(
             &self.store,
             &held.envelope,
             &config,
-            &sidecars,
             &ExecutionEngine::valid(),
         ) {
             // p2p judged the envelope before it got here, so no peer is
@@ -607,28 +589,6 @@ impl BlockChainServer {
         self.envelopes.work.extend(roots);
     }
 
-    /// Every sampled column stored for `root`, decoded as gloas sidecars, or
-    /// `None` if any is missing or not one.
-    fn stored_gloas_columns(
-        &self,
-        slot: u64,
-        root: &H256,
-    ) -> Option<Vec<gloas::DataColumnSidecar>> {
-        self.custody_columns
-            .iter()
-            .map(
-                |&index| match self.store.get_data_column(slot, root, index) {
-                    Ok(Some(DataColumnSidecar::Gloas(sidecar))) => Some(sidecar),
-                    Ok(Some(DataColumnSidecar::Fulu(_))) | Ok(None) => None,
-                    Err(err) => {
-                        error!(%err, "Failed to read back a stored data column sidecar");
-                        None
-                    }
-                },
-            )
-            .collect()
-    }
-
     /// Hold `held` for a block that is in the store but has no post-state yet,
     /// after checking it against that block's bid.
     ///

crates/blockchain/src/lib.rs

@@ -765,24 +765,6 @@ pub enum ImportOutcome {
     Held,
 }
 
-/// The availability evidence for `block`, or `None` if a column this node
-/// custodies has not arrived yet.
-///
-/// `None` is not "unavailable": it means the question cannot be answered yet,
-/// which is why the caller holds the block rather than rejecting it. A partial
-/// set must never be passed on as evidence, because
-/// `is_data_available_columns` is vacuously true over an empty list and would
-/// import a block whose data nobody has.
-///
-/// Only fulu blocks reach the column shape. A deneb or electra block carrying
-/// blobs would need the blob-and-proof shape, which this node has no source
-/// for, so it is admitted with a log rather than held forever against a
-/// pipeline that does not exist.
-///
-/// `ethlambda_storage::Table::DataColumns` is never pruned today (see its own
-/// doc comment), which is what lets the sidecar-collecting `.expect()`s below
-/// assume a column confirmed present a moment ago by `custody_columns_present`
-/// is still there to decode; a future pruner has to keep that window safe too.
 /// Whether a block at `block_slot` still falls inside the window this node may
 /// insist on data availability for.
 ///
@@ -809,6 +791,28 @@ fn da_check_required_for_slot(block_slot: u64, current_slot: u64, config: &Confi
     block_epoch >= boundary
 }
 
+/// The availability evidence for `block`, or `None` if a column this node
+/// custodies has not arrived yet.
+///
+/// `None` is not "unavailable": it means the question cannot be answered yet,
+/// which is why the caller holds the block rather than rejecting it. A partial
+/// set must never be passed on as evidence, because
+/// `is_data_available_columns` is vacuously true over an empty list and would
+/// import a block whose data nobody has.
+///
+/// Only fulu blocks reach the column shape. A deneb or electra block carrying
+/// blobs would need the blob-and-proof shape, which this node has no source
+/// for, so it is admitted with a log rather than held forever against a
+/// pipeline that does not exist.
+///
+/// A fulu block whose custody columns are all present gets
+/// [`fork_choice::DataAvailability::ColumnsVerified`]: the sidecars are not
+/// read or decoded, because the table only ever holds ones the p2p layer
+/// already checked in full (structure, inclusion proof, KZG), and the spec's
+/// per-sidecar check would repeat that work on the import thread. Debug builds
+/// keep a safety net on the write side instead, in `on_checked_data_columns`.
+/// Mirrors lighthouse, whose verified-column types carry that proof so import
+/// never re-runs KZG.
 fn data_availability_for(
     store: &Store,
     block: &SignedBeaconBlock,
@@ -834,7 +838,7 @@ fn data_availability_for(
 
             // An empty custody set means there is nothing outstanding, not
             // that the question is unanswerable: `custody_columns_present`
-            // below is vacuously true and the resulting `Columns(vec![])` is
+            // below is vacuously true and the resulting evidence is
             // vacuously available, which is the right answer for a caller
             // that supplied every block itself. Only the replay harness
             // (`BlockChainServer::for_replay`) is in that position. A node
@@ -846,32 +850,18 @@ fn data_availability_for(
             let block_root = block.message_hash_tree_root();
 
             // Presence first, over the full set: a column this node has not
-            // yet verified must not even be looked up, since silently
-            // dropping the missing ones would hand back a shorter-but-still-
-            // non-empty list that reads as complete evidence to the caller.
+            // yet stored must not be waved through, since evidence built from
+            // a partial set would read as complete to the caller.
             if !custody_columns_present(store, slot, &block_root, custody_columns) {
                 return None;
             }
 
-            let sidecars = custody_columns
-                .iter()
-                .map(|&index| {
-                    let sidecar = store
-                        .get_data_column(slot, &block_root, index)
-                        .expect("a sidecar this node verified before storing decodes")
-                        .expect("presence just confirmed above");
-                    match sidecar {
-                        DataColumnSidecar::Fulu(sidecar) => sidecar,
-                        // The row decodes by its slot's fork, and this arm is
-                        // the fulu block at that slot.
-                        DataColumnSidecar::Gloas(_) => {
-                            unreachable!("a fulu block's slot decodes its rows as fulu")
-                        }
-                    }
-                })
-                .collect();
-
-            Some(fork_choice::DataAvailability::Columns(sidecars))
+            // Present means verified: `DataColumns` takes only sidecars the
+            // p2p layer's column checks accepted (see `on_checked_data_columns`),
+            // keyed by the root of the header those checks judged. So the
+            // sidecars are neither read back nor verified again here; the KZG
+            // batch they would cost is the one each already paid on arrival.
+            Some(fork_choice::DataAvailability::ColumnsVerified)
         }
         SignedBeaconBlock::Phase0(_)
         | SignedBeaconBlock::Altair(_)
@@ -5070,7 +5060,26 @@ mod tests {
         }
         assert!(matches!(
             data_availability_for(&store, &block, &CUSTODY).unwrap(),
-            fork_choice::DataAvailability::Columns(sidecars) if sidecars.len() == 2
+            fork_choice::DataAvailability::ColumnsVerified
+        ));
+    }
+
+    #[test]
+    fn complete_columns_are_vouched_for_without_being_read() {
+        // The rows are bytes no sidecar decodes from, so any read of them
+        // would fail: presence alone is what the evidence rests on, since the
+        // table holds only sidecars the p2p layer already checked.
+        let store = beacon_store_fulu_at_genesis(0, 0);
+        let block = fulu_block_with_commitments(&store, 2);
+        let root = block.message_hash_tree_root();
+        for index in CUSTODY {
+            store
+                .put_data_column_sidecar(block.slot(), &root, index, vec![0xff])
+                .unwrap();
+        }
+        assert!(matches!(
+            data_availability_for(&store, &block, &CUSTODY),
+            Some(fork_choice::DataAvailability::ColumnsVerified)
         ));
     }
 
@@ -5082,7 +5091,7 @@ mod tests {
         // where a node is configured, not re-derived per block.
         //
         // With no column to require, `custody_columns_present` is vacuously
-        // true and the collected evidence is `Columns(vec![])` rather than
+        // true and the evidence is `ColumnsVerified` rather than
         // `NotRequired`: the commitments are still there, so this is "nothing
         // outstanding", not "no check needed", but the two are equally
         // admissible to `is_data_available_columns`.
@@ -5094,7 +5103,7 @@ mod tests {
         assert!(
             matches!(
                 evidence,
-                Some(fork_choice::DataAvailability::Columns(sidecars)) if sidecars.is_empty()
+                Some(fork_choice::DataAvailability::ColumnsVerified)
             ),
             "an empty custody set has nothing outstanding, so the block is admissible"
         );

crates/blockchain/state_transition/src/beacon/fork_choice.rs

@@ -629,7 +629,25 @@ pub enum DataAvailability {
         proofs: Vec<KzgProof>,
     },
     /// Fulu's shape: the column sidecars sampled for this block.
+    ///
+    /// [`is_data_available_columns`] runs the specification's full check on
+    /// every one of them, KZG batch included. This is what the fixture suites
+    /// supply, so they keep exercising the real check.
     Columns(Vec<fulu::DataColumnSidecar>),
+    /// Fulu's shape, when the caller has already verified every sampled column
+    /// for this block and holds no sidecars to show for it.
+    ///
+    /// Constructing this is a promise: each sidecar this node samples for the
+    /// block passed [`verify_data_column_sidecar`], the inclusion proof and
+    /// [`verify_data_column_sidecar_kzg_proofs`], and was stored under the
+    /// block's own root. Only a caller that gates every write of those
+    /// sidecars on those checks may make it (the chain actor does, by storing
+    /// only what the p2p layer's column checks accepted), since
+    /// [`is_data_available_columns`] takes it on trust and runs nothing. It
+    /// exists so import does not decode every stored sidecar and repeat the
+    /// KZG batch the sidecar already paid for on arrival; a caller with
+    /// unchecked sidecars must pass [`Self::Columns`] instead.
+    ColumnsVerified,
 }
 
 /// What an execution client said about the payload of the block being imported
@@ -4073,7 +4091,7 @@ pub fn is_data_available_blobs(
     let (blobs, proofs) = match evidence {
         DataAvailability::Blobs { blobs, proofs } => (blobs.as_slice(), proofs.as_slice()),
         DataAvailability::NotRequired => (&[][..], &[][..]),
-        DataAvailability::Columns(_) => {
+        DataAvailability::Columns(_) | DataAvailability::ColumnsVerified => {
             return Err(Error::SpecAssert(
                 "a deneb or electra block's data availability is judged on blobs",
             ));
@@ -4237,10 +4255,13 @@ pub fn verify_data_column_sidecar_inclusion_proof(sidecar: &fulu::DataColumnSide
 /// required columns have been sampled" must reject the block itself rather
 /// than relying on this to do it, since nothing about an empty list is
 /// distinguishable here from "this block needed no sampling at all".
+///
+/// [`DataAvailability::ColumnsVerified`] is accepted without running anything:
+/// its constructor promised the same checks already ran on each sidecar.
 pub fn is_data_available_columns(evidence: &DataAvailability, config: &Config) -> Result<bool> {
     let sidecars = match evidence {
         DataAvailability::Columns(sidecars) => sidecars,
-        DataAvailability::NotRequired => return Ok(true),
+        DataAvailability::NotRequired | DataAvailability::ColumnsVerified => return Ok(true),
         DataAvailability::Blobs { .. } => {
             return Err(Error::SpecAssert(
                 "a fulu block's data availability is judged on column sidecars",
@@ -5420,6 +5441,36 @@ pub fn check_execution_payload_envelope(
     config: &Config,
     sidecars: &[gloas::DataColumnSidecar],
     engine: &stf::ExecutionEngine,
+) -> Result<()> {
+    check_envelope(store, signed_envelope, config, Some(sidecars), engine)
+}
+
+/// [`check_execution_payload_envelope`] for a caller that has already verified
+/// every sampled column of the block, and so holds no sidecars to pass.
+///
+/// The caller promises what [`DataAvailability::ColumnsVerified`] promises,
+/// against the commitments of this block's bid: each sampled sidecar passed
+/// the gloas structural check and the KZG batch, and is stored under the
+/// block's own root. Everything else is checked exactly as in
+/// [`check_execution_payload_envelope`]; only the data-availability step is
+/// taken on trust, so the KZG batch is not repeated on every envelope.
+pub fn check_execution_payload_envelope_columns_verified(
+    store: &Store,
+    signed_envelope: &gloas::SignedExecutionPayloadEnvelope,
+    config: &Config,
+    engine: &stf::ExecutionEngine,
+) -> Result<()> {
+    check_envelope(store, signed_envelope, config, None, engine)
+}
+
+/// The body of both public checks. `sidecars` is `None` when the caller has
+/// vouched for every sampled column, which skips only the availability step.
+fn check_envelope(
+    store: &Store,
+    signed_envelope: &gloas::SignedExecutionPayloadEnvelope,
+    config: &Config,
+    sidecars: Option<&[gloas::DataColumnSidecar]>,
+    engine: &stf::ExecutionEngine,
 ) -> Result<()> {
     let envelope = &signed_envelope.message;
     let block_root = envelope.beacon_block_root;
@@ -5440,11 +5491,13 @@ pub fn check_execution_payload_envelope(
         .ok_or(Error::SpecAssert(
             "envelope.beacon_block_root in store.blocks",
         ))?;
-    let kzg_commitments = &gloas_bid(&block)?.blob_kzg_commitments;
-    verify(
-        is_data_available_gloas_columns(sidecars, kzg_commitments)?,
-        "is_data_available(envelope.beacon_block_root)",
-    )?;
+    if let Some(sidecars) = sidecars {
+        let kzg_commitments = &gloas_bid(&block)?.blob_kzg_commitments;
+        verify(
+            is_data_available_gloas_columns(sidecars, kzg_commitments)?,
+            "is_data_available(envelope.beacon_block_root)",
+        )?;
+    }
 
     // Verify the execution payload envelope.
     stf::gloas::verify_execution_payload_envelope(&state, signed_envelope, config, engine)?;

docs/data_storage.md

@@ -480,7 +480,11 @@ import. That ordering is what lets the availability
 gate read a block's columns before the block itself is allowed to import (a
 column has to exist first for the gate to find it), and what lets a restart
 keep every sidecar this node already paid a KZG batch to verify rather than
-re-fetching and re-verifying them. One consequence: a sidecar is stored for
+re-fetching and re-verifying them. The same property is what lets block import
+(and a gloas envelope's import) treat a present column as verified: the availability
+gate checks presence only and hands fork choice `DataAvailability::ColumnsVerified`,
+so the stored sidecars are not decoded and their KZG proofs are not checked a second
+time. One consequence: a sidecar is stored for
 any block whose header, parent and proposer check out, whether or not that
 block ever actually imports — an equivocation or an orphaned fork's sidecars
 land here too, since gossip only requires a known, unfinalized parent, not a