fix(beacon): verify a block's signature before it waits for its parent or columns

bin/ethlambda/src/main.rs

@@ -1806,12 +1806,17 @@ async fn fetch_initial_beacon_state(
                 .expect("repair_head leaves the head naming a real block");
             let gap = current_slot.saturating_sub(head_slot);
 
+            // Both resuming returns cache the head's state first, so gossip
+            // validation can check a parentless block's signature from the
+            // first message on; see `Store::cache_head_state`.
             if gap <= MAX_RESUMABLE_DB_STATE_AGE {
                 info!(head_slot, current_slot, gap, "Resuming from existing DB");
+                store.cache_head_state()?;
                 return Ok(store);
             }
             if checkpoint_urls.is_empty() {
                 warn!(head_slot, current_slot, gap, "DB is stale; resuming anyway");
+                store.cache_head_state()?;
                 return Ok(store);
             }
             warn!(head_slot, current_slot, gap, "DB is stale; checkpoint sync");

crates/blockchain/src/lib.rs

@@ -6,6 +6,7 @@ use ethlambda_network_api::{
 use ethlambda_state_transition::beacon::error::Error as BeaconError;
 use ethlambda_state_transition::beacon::fork_choice;
 use ethlambda_state_transition::beacon::helpers::accessors::CommitteeCacheExt;
+use ethlambda_state_transition::beacon::precheck::{PrecheckError, Reference, precheck_block};
 use ethlambda_state_transition::is_proposer;
 use ethlambda_storage::{ALL_TABLES, CacheKey, Chain, Store};
 use ethlambda_types::{
@@ -685,12 +686,37 @@ impl LeanDuties {
 /// [`BeaconError`]. Wrapping both here, rather than picking one chain's
 /// error type to stand in for both, keeps each chain's own error type
 /// exactly as its own module defines it.
+///
+/// [`ImportError::Precheck`] is the chain actor's own: a beacon block refused
+/// before it could be held for its custody columns (see
+/// [`BlockChainServer::precheck_before_waiting`]).
 #[derive(Debug, thiserror::Error)]
 enum ImportError {
     #[error(transparent)]
     Lean(#[from] StoreError),
     #[error(transparent)]
     Beacon(#[from] BeaconError),
+    #[error("refused before waiting for its custody columns: {0}")]
+    Precheck(#[from] PrecheckError),
+}
+
+/// What a beacon block would wait for, if [`BlockChainServer::precheck_before_waiting`]
+/// lets it.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+enum Wait {
+    /// Its parent, which has no post-state yet.
+    Parent,
+    /// Its own custody columns, with the parent already imported.
+    Columns,
+}
+
+impl Wait {
+    fn label(self) -> &'static str {
+        match self {
+            Self::Parent => "parent",
+            Self::Columns => "columns",
+        }
+    }
 }
 
 /// What [`BlockChainServer::process_block`] did with the block it was given.
@@ -1716,6 +1742,13 @@ impl BlockChainServer {
                     match evidence {
                         Some(evidence) => evidence,
                         None => {
+                            if let Err(err) = self.precheck_before_waiting(
+                                &beacon_block,
+                                block_root,
+                                Wait::Columns,
+                            ) {
+                                return (timings, Err(err.into()));
+                            }
                             timings.columns_wait_start =
                                 timings.columns_wait_start.or(timings.da_check_end);
                             self.hold_block_for_columns(beacon_block, current_slot, timings);
@@ -2653,6 +2686,23 @@ impl BlockChainServer {
             .has_state(&parent_root)
             .expect("DB read should succeed")
         {
+            if self.store.chain() == Chain::Beacon
+                && let Err(err) =
+                    self.precheck_before_waiting(&signed_block, block_root, Wait::Parent)
+            {
+                warn!(
+                    %slot,
+                    proposer,
+                    block_root = %ShortRoot(&block_root.0),
+                    parent_root = %ShortRoot(&parent_root.0),
+                    %err,
+                    "Refusing a block before it waits for its parent"
+                );
+                // Anything already parked on this root waits for a block that
+                // will never import.
+                self.discard_pending_subtree(block_root);
+                return None;
+            }
             info!(%slot, %parent_root, %block_root, "Block parent missing, storing as pending");
             timings.guards_end = Some(Instant::now());
             timings.parent_wait_start = timings.parent_wait_start.or(timings.guards_end);
@@ -2944,6 +2994,51 @@ impl BlockChainServer {
         }
     }
 
+    /// Judge a beacon block by [`precheck_block`] before it is kept waiting,
+    /// for its parent or for its custody columns.
+    ///
+    /// A waiting block is kept unjudged until what it waits for arrives, and
+    /// a held one has its columns asked for again every slot until finality
+    /// evicts it. Mainnet gossip carries altered copies of real blocks (a
+    /// blob transaction re-encoded after signing, the original signature
+    /// kept) whose root nobody signed and no peer has columns for, so a block
+    /// that would fail its import is refused before it waits rather than
+    /// after. Gossip validation already refuses those, but range sync and
+    /// by-root fetches never pass through it.
+    ///
+    /// Judged against the parent's post-state when it waits for its columns
+    /// (every rule), and against the head's when it waits for its parent (the
+    /// signature only). A proposer the head state does not hold is refused
+    /// too: its signature cannot be checked, and a real block refused here
+    /// still arrives again through range sync once its parent has imported.
+    fn precheck_before_waiting(
+        &self,
+        block: &SignedBeaconBlock,
+        block_root: H256,
+        wait: Wait,
+    ) -> Result<(), PrecheckError> {
+        let config = self.store.config();
+        let reference_root = match wait {
+            Wait::Parent => self.store.head().expect("the head row exists"),
+            Wait::Columns => block.parent_root(),
+        };
+        // Both are post-states the store must have: the head's by the
+        // invariant every head move keeps, the parent's because the caller
+        // only asks once `has_state` said so.
+        let reference_state = self
+            .store
+            .get_state(&reference_root)
+            .expect("DB read should succeed")
+            .expect("the reference block has a post-state");
+        let reference = match wait {
+            Wait::Parent => Reference::Recent(&reference_state),
+            Wait::Columns => Reference::Parent(&reference_state),
+        };
+        precheck_block(block, block_root, reference, &config).inspect_err(|err| {
+            metrics::inc_beacon_blocks_refused_before_waiting(err.label(), wait.label())
+        })
+    }
+
     /// Keep `block` until every column this node custodies for it has arrived.
     ///
     /// The same shape as a block held for a missing parent: the block itself is

crates/blockchain/src/metrics.rs

@@ -949,6 +949,17 @@ static LEAN_BLOCKS_HELD_FOR_COLUMNS: std::sync::LazyLock<IntGauge> =
         .unwrap()
     });
 
+static LEAN_BEACON_BLOCKS_REFUSED_BEFORE_WAITING_TOTAL: std::sync::LazyLock<IntCounterVec> =
+    std::sync::LazyLock::new(|| {
+        register_int_counter_vec!(
+            "lean_beacon_blocks_refused_before_waiting_total",
+            "Beacon blocks the chain actor refused instead of keeping them waiting, by the \
+             precheck rule they broke and what they would have waited for",
+            &["reason", "wait"]
+        )
+        .unwrap()
+    });
+
 static LEAN_SIDECARS_AWAITING_PARENT: std::sync::LazyLock<IntGauge> =
     std::sync::LazyLock::new(|| {
         register_int_gauge!(
@@ -1424,6 +1435,15 @@ pub fn set_blocks_held_for_columns(count: u64) {
     LEAN_BLOCKS_HELD_FOR_COLUMNS.set(count as i64);
 }
 
+/// A beacon block refused before it waited for its parent or its custody
+/// columns. `reason` is a `PrecheckError` label and `wait` one of `parent`,
+/// `columns`: both are fixed sets, so a block's contents add no label value.
+pub fn inc_beacon_blocks_refused_before_waiting(reason: &'static str, wait: &'static str) {
+    LEAN_BEACON_BLOCKS_REFUSED_BEFORE_WAITING_TOTAL
+        .with_label_values(&[reason, wait])
+        .inc();
+}
+
 /// Sidecars currently parked against a parent root with no post-state.
 ///
 /// Reads as the queue depth of the recovery path the availability gate depends

crates/blockchain/state_transition/src/beacon/gossip/block.rs

@@ -63,14 +63,15 @@ pub fn stateful_checks(store: &Store, block: &SignedBeaconBlock, block_root: Roo
     // [IGNORE] The parent has been seen and passed validation (MAY queue).
     // A parent without a post-state may have failed, which the specification
     // rejects; without a bad-block cache this cannot tell failed from not yet
-    // imported, so it queues. See the design spec's deviations.
+    // imported, so it queues. See the design spec's deviations. Queues only
+    // a block whose signature verified, though: see `queue_if_signed`.
     let Some(parent_state) = parent_state else {
         let reason = if parent_known {
             QueueReason::ParentNotReady
         } else {
             QueueReason::ParentUnknown
         };
-        return queue_unless_forged(store, block, block_root, reason);
+        return queue_if_signed(store, block, block_root, reason);
     };
     // [REJECT] After its parent, expected proposer (when the parent's
     // lookahead can answer), known proposer, valid signature.
@@ -111,13 +112,28 @@ pub fn validate(
     stateful_checks(store, block, block.message_hash_tree_root())
 }
 
-/// `Queue(reason)`, unless the head state already shows the signature is forged.
+/// `Queue(reason)` for a block whose signature the head state verifies;
+/// `Reject` for one it shows is forged; `Ignore(SignatureUnverified)` for one
+/// it cannot judge.
 ///
 /// A block that cannot be judged against its parent yet can still be judged
 /// on its signature: validator indices never move, so the head state's key for
 /// the proposer is the one the block was signed with. Prysm does the same
 /// before queueing.
-fn queue_unless_forged(
+///
+/// A block that cannot be judged even that far is dropped, not queued. A
+/// queued block goes on to the chain actor, which parks it until its parent
+/// imports and then holds it until its custody columns arrive. Mainnet gossip
+/// carries altered copies of real blocks (a blob transaction re-encoded after
+/// signing, the original signature kept): no peer has columns for such a
+/// root, so a queued one sat held until finality evicted it, its columns
+/// asked for again every slot. Dropping costs a real block only its gossip
+/// delivery; a child's by-root fetch or range sync still brings it in.
+///
+/// The head state is read only through [`Store::cached_state`], for the
+/// reason [`stateful_checks`] gives. A resumed node caches it at startup, so
+/// the window before its first import is covered too.
+fn queue_if_signed(
     store: &Store,
     block: &SignedBeaconBlock,
     block_root: Root,
@@ -128,16 +144,19 @@ fn queue_unless_forged(
         .ok()
         .and_then(|head| store.cached_state(CacheKey::BlockState(head)));
     let Some(head_state) = head_state else {
-        return Outcome::Queue(reason);
+        return Outcome::Ignore(IgnoreReason::SignatureUnverified);
     };
     match precheck_block(
         block,
         block_root,
         Reference::Recent(&head_state),
         &store.config(),
     ) {
+        Ok(()) => Outcome::Queue(reason),
         Err(PrecheckError::BadSignature) => Outcome::Reject(RejectReason::BadSignature),
-        _ => Outcome::Queue(reason),
+        // `UnknownProposer`, the only other error a recent state reports: a
+        // validator newer than the head state, whose key it does not hold.
+        Err(_) => Outcome::Ignore(IgnoreReason::SignatureUnverified),
     }
 }
 
@@ -282,18 +301,39 @@ mod tests {
     }
 
     #[test]
-    fn a_block_whose_parent_was_never_seen_is_queued() {
+    fn a_block_whose_parent_was_never_seen_is_dropped_with_no_state_to_check_it() {
         let store = store(0);
         let SignedBeaconBlock::Fulu(mut orphan) = fulu_block(5, 1, 0) else {
             unreachable!("fulu_block builds a fulu block");
         };
         orphan.message.parent_root = Root::repeat_byte(0x11);
         let orphan = SignedBeaconBlock::Fulu(orphan);
-        // No head state is cached either, so the signature cannot be judged
-        // and the block is queued as it is.
+        // No head state is cached either, so the signature cannot be judged,
+        // and a block that cannot be judged is not queued.
         assert_eq!(
             stateful_checks(&store, &orphan, Root::repeat_byte(5)),
-            Outcome::Queue(QueueReason::ParentUnknown)
+            Outcome::Ignore(IgnoreReason::SignatureUnverified)
+        );
+    }
+
+    #[test]
+    fn an_unknown_parent_by_a_proposer_the_head_state_lacks_is_dropped() {
+        let store = store(0);
+        let head_state = fulu_parent(3);
+        store.cache_state(CacheKey::BlockState(Root::ZERO), Arc::new(head_state));
+
+        let proposer = 1_000;
+        let SignedBeaconBlock::Fulu(mut orphan) = fulu_block(5, proposer, 0) else {
+            unreachable!("fulu_block builds a fulu block");
+        };
+        orphan.message.parent_root = Root::repeat_byte(0x11);
+        let orphan = SignedBeaconBlock::Fulu(orphan);
+
+        // Neither forged nor verified: the head state holds no key for the
+        // proposer to check the signature with.
+        assert_eq!(
+            stateful_checks(&store, &orphan, orphan.message_hash_tree_root()),
+            Outcome::Ignore(IgnoreReason::SignatureUnverified)
         );
     }
 
@@ -303,7 +343,7 @@ mod tests {
         let config = store.config();
         let proposer: ValidatorIndex = 3;
         // `store()` sets the head to `Root::ZERO`, the same root
-        // `queue_unless_forged` reads through `Store::head`.
+        // `queue_if_signed` reads through `Store::head`.
         let head_state = fulu_parent(proposer);
         store.cache_state(
             CacheKey::BlockState(Root::ZERO),

crates/blockchain/state_transition/src/beacon/gossip/column.rs

@@ -269,12 +269,13 @@ fn advanced_proposer(
 /// `Queue(reason)`, unless the head state already shows the header's
 /// signature is forged.
 ///
-/// Mirrors [`super::block::queue_unless_forged`]: validator indices never
-/// move, so the head state's key for the header's proposer is the one it was
-/// signed with, even when that state cannot yet say whether this proposer is
-/// the *expected* one for the slot. A column queued here is written to the
-/// chain actor's `PendingDataColumns`, so without this check a forged one
-/// would sit there rather than being refused up front.
+/// Mirrors the signature check in [`super::block::queue_if_signed`]:
+/// validator indices never move, so the head state's key for the header's
+/// proposer is the one it was signed with, even when that state cannot yet say
+/// whether this proposer is the *expected* one for the slot. A column queued
+/// here is written to the chain actor's `PendingDataColumns`, so without this
+/// check a forged one would sit there rather than being refused up front.
+/// Unlike a block, a sidecar no cached state can judge is still queued.
 fn queue_unless_forged(store: &Store, sidecar: &DataColumnSidecar, reason: QueueReason) -> Outcome {
     let head_state = store
         .head()

crates/blockchain/state_transition/src/beacon/gossip/mod.rs

@@ -112,6 +112,12 @@ pub enum IgnoreReason {
     FinalizedNotAncestor,
     /// An ancestor lies outside what the state's `block_roots` can answer.
     AncestryUnknown,
+    /// A block whose parent has no post-state yet, and whose proposer
+    /// signature no cached state could check. Dropped rather than queued: a
+    /// queued block goes on to the chain actor, which would keep it, unjudged,
+    /// until its parent and then its columns arrive, and a forged one has no
+    /// columns to wait for.
+    SignatureUnverified,
 }
 
 impl IgnoreReason {
@@ -130,6 +136,7 @@ impl IgnoreReason {
             Self::StateUnavailable => "state_unavailable",
             Self::FinalizedNotAncestor => "finalized_not_ancestor",
             Self::AncestryUnknown => "ancestry_unknown",
+            Self::SignatureUnverified => "signature_unverified",
         }
     }
 }