From 396fdcceaad761532d6d1d5d45ddd557355159f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Mon, 2 Nov 2026 16:36:08 -0300 Subject: [PATCH] perf(beacon): stop re-verifying stored custody columns at block import The mainnet profile showed about 8 ms of CPU per block in verify_cell_kzg_proof_batch under on_block, plus a DB read and SSZ decode of every custody column, all repeating work each sidecar already paid for on arrival: DataColumns only receives sidecars the p2p layer accepted (gossip validation or column::chain_checks, which run the structural check, the inclusion proof and the KZG batch), keyed by the root of the header those checks judged. Add DataAvailability::ColumnsVerified, a caller's promise that every sampled column was verified, and have data_availability_for return it once the custody columns are present, without reading them. DataAvailability::Columns keeps the full spec check, so the fork_choice fixtures still exercise it. The gloas envelope path gets the same treatment through check_execution_payload_envelope_columns_verified. The debug-build safety net stays on the write side (on_checked_data_columns re-runs chain_checks). Mirrors lighthouse's verified-column types, which carry the proof of verification so import never re-runs KZG. --- crates/blockchain/src/beacon_envelope.rs | 56 ++--------- crates/blockchain/src/lib.rs | 97 ++++++++++--------- .../src/beacon/fork_choice.rs | 67 +++++++++++-- docs/data_storage.md | 6 +- 4 files changed, 126 insertions(+), 100 deletions(-) diff --git a/crates/blockchain/src/beacon_envelope.rs b/crates/blockchain/src/beacon_envelope.rs index a893a651c..abea3690c 100644 --- a/crates/blockchain/src/beacon_envelope.rs +++ b/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> { - 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. /// diff --git a/crates/blockchain/src/lib.rs b/crates/blockchain/src/lib.rs index 251048b7a..aaf6284dc 100644 --- a/crates/blockchain/src/lib.rs +++ b/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" ); diff --git a/crates/blockchain/state_transition/src/beacon/fork_choice.rs b/crates/blockchain/state_transition/src/beacon/fork_choice.rs index 02c959376..0ca0fa559 100644 --- a/crates/blockchain/state_transition/src/beacon/fork_choice.rs +++ b/crates/blockchain/state_transition/src/beacon/fork_choice.rs @@ -626,7 +626,25 @@ pub enum DataAvailability { proofs: Vec, }, /// 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'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 @@ -4061,7 +4079,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", )); @@ -4225,10 +4243,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 { 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", @@ -5408,6 +5429,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; @@ -5428,11 +5479,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)?; diff --git a/docs/data_storage.md b/docs/data_storage.md index 0f8d62db8..bfde3aaba 100644 --- a/docs/data_storage.md +++ b/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