From eaffbbff216a0ab281a1c34b1a3f6a3ee1edb385 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, 28 Sep 2026 16:17:14 -0300 Subject: [PATCH 1/2] fix(beacon): verify a block's signature before it waits for its parent or columns A beacon block that waits, for its parent's post-state and then for its custody columns, 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): nobody signed their root and no peer has columns for them. Gossip's queue path checked the signature only against a cached head state, and queued the block anyway when it found none. After a resume nothing caches the head state before the first import, so in that window every parentless block reached the chain actor unchecked, and an altered one sat held for columns until finality. Range sync and by-root fetches never pass through gossip validation at all. - Gossip queues a parentless block only once the head state verifies its signature. One it cannot judge is ignored as `signature_unverified`, and only an accepted or queued block is forwarded, so an overloaded one no longer reaches the actor unjudged. - `precheck_block` against a recent state reports an unknown proposer instead of passing it, so `Ok` always means the signature verified. - The chain actor runs `precheck_block` before parking a block for its parent (signature, against the head state) and before holding it for columns (every rule, against the parent's post-state). - Resuming from the DB caches the head's state, so gossip can judge a parentless block from the first message on. --- bin/ethlambda/src/main.rs | 5 + crates/blockchain/src/lib.rs | 95 +++++++++++++++++++ crates/blockchain/src/metrics.rs | 20 ++++ .../src/beacon/gossip/block.rs | 62 +++++++++--- .../src/beacon/gossip/column.rs | 13 +-- .../state_transition/src/beacon/gossip/mod.rs | 7 ++ .../state_transition/src/beacon/precheck.rs | 38 +++++--- crates/net/p2p/src/beacon/verdict.rs | 88 +++++++++++++---- crates/storage/src/store.rs | 42 ++++++++ 9 files changed, 322 insertions(+), 48 deletions(-) diff --git a/bin/ethlambda/src/main.rs b/bin/ethlambda/src/main.rs index d9413098..8f3d939d 100644 --- a/bin/ethlambda/src/main.rs +++ b/bin/ethlambda/src/main.rs @@ -1724,12 +1724,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"); diff --git a/crates/blockchain/src/lib.rs b/crates/blockchain/src/lib.rs index 74d511b5..17201e0c 100644 --- a/crates/blockchain/src/lib.rs +++ b/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::{ @@ -632,12 +633,37 @@ struct 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. @@ -1648,6 +1674,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, timings); @@ -2585,6 +2618,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); @@ -2863,6 +2913,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 diff --git a/crates/blockchain/src/metrics.rs b/crates/blockchain/src/metrics.rs index 95a31967..e4f04c1e 100644 --- a/crates/blockchain/src/metrics.rs +++ b/crates/blockchain/src/metrics.rs @@ -921,6 +921,17 @@ static LEAN_BLOCKS_HELD_FOR_COLUMNS: std::sync::LazyLock = .unwrap() }); +static LEAN_BEACON_BLOCKS_REFUSED_BEFORE_WAITING_TOTAL: std::sync::LazyLock = + 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 = std::sync::LazyLock::new(|| { register_int_gauge!( @@ -1370,6 +1381,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 diff --git a/crates/blockchain/state_transition/src/beacon/gossip/block.rs b/crates/blockchain/state_transition/src/beacon/gossip/block.rs index 02360ef2..22ac8c20 100644 --- a/crates/blockchain/state_transition/src/beacon/gossip/block.rs +++ b/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,7 +144,7 @@ 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, @@ -136,8 +152,11 @@ fn queue_unless_forged( 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), diff --git a/crates/blockchain/state_transition/src/beacon/gossip/column.rs b/crates/blockchain/state_transition/src/beacon/gossip/column.rs index 2d7130ab..e362357e 100644 --- a/crates/blockchain/state_transition/src/beacon/gossip/column.rs +++ b/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() diff --git a/crates/blockchain/state_transition/src/beacon/gossip/mod.rs b/crates/blockchain/state_transition/src/beacon/gossip/mod.rs index 2acc7077..fbc6ab6c 100644 --- a/crates/blockchain/state_transition/src/beacon/gossip/mod.rs +++ b/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", } } } diff --git a/crates/blockchain/state_transition/src/beacon/precheck.rs b/crates/blockchain/state_transition/src/beacon/precheck.rs index f2c9ee0a..8f269d6c 100644 --- a/crates/blockchain/state_transition/src/beacon/precheck.rs +++ b/crates/blockchain/state_transition/src/beacon/precheck.rs @@ -42,7 +42,12 @@ pub enum PrecheckError { proposer: ValidatorIndex, expected: ValidatorIndex, }, - #[error("proposer {proposer} names no validator in the parent's state")] + /// Against [`Reference::Parent`], a refusal: the state the block builds + /// on has no such validator. Against [`Reference::Recent`], only that + /// this state cannot answer for the proposer (a validator newer than it), + /// so the signature went unchecked; the caller decides what that costs + /// the block. + #[error("proposer {proposer} names no validator in the reference state")] UnknownProposer { proposer: ValidatorIndex }, #[error("the signature is not the proposer's over this block")] BadSignature, @@ -67,11 +72,11 @@ pub enum Reference<'a> { /// The post-state of the block's own parent. Every rule applies. Parent(&'a BeaconState), /// Some recent state of the chain, for a block whose parent has no - /// post-state yet. Only the signature is checked, and only when this - /// state already holds the proposer. Validator indices never move, so a - /// key found here is the key the block was signed with; a validator newer - /// than this state is simply not one it can answer for, and such a block - /// passes rather than being refused on missing information. + /// post-state yet. Only the signature is checked. Validator indices never + /// move, so a key found here is the key the block was signed with. A + /// validator newer than this state is one it cannot answer for, reported + /// as [`PrecheckError::UnknownProposer`] rather than passed: `Ok` from + /// this reference always means the signature verified. Recent(&'a BeaconState), } @@ -110,13 +115,10 @@ pub fn precheck_block( Reference::Recent(state) => state, }; - let pubkey = match (state.validator(proposer), reference) { - (Ok(validator), _) => &validator.pubkey, - (Err(_), Reference::Parent(_)) => { - return Err(PrecheckError::UnknownProposer { proposer }); - } - (Err(_), Reference::Recent(_)) => return Ok(()), - }; + let pubkey = &state + .validator(proposer) + .map_err(|_| PrecheckError::UnknownProposer { proposer })? + .pubkey; let fork_version = config.fork_version(config.fork_at_epoch(compute_epoch_at_slot(slot))); let domain = compute_domain( @@ -322,7 +324,7 @@ mod tests { } #[test] - fn a_proposer_missing_from_the_parents_state_is_refused_but_not_from_a_recent_one() { + fn a_proposer_missing_from_the_reference_state_is_unknown_to_both_references() { let config = Config::mainnet(); // A pre-fulu parent, so the lookahead cannot answer first. let parent = keyed_state(ForkName::Electra); @@ -336,7 +338,13 @@ mod tests { check(&unknown, Reference::Parent(&parent), &config), Err(PrecheckError::UnknownProposer { proposer: 100 }) ); - assert_eq!(check(&unknown, Reference::Recent(&parent), &config), Ok(())); + // A recent state reports the same thing rather than passing the + // block: its signature went unchecked, and `Ok` must never say + // otherwise. + assert_eq!( + check(&unknown, Reference::Recent(&parent), &config), + Err(PrecheckError::UnknownProposer { proposer: 100 }) + ); } /// The first block of a fork is signed under the new version while its diff --git a/crates/net/p2p/src/beacon/verdict.rs b/crates/net/p2p/src/beacon/verdict.rs index bff0cd42..218484d0 100644 --- a/crates/net/p2p/src/beacon/verdict.rs +++ b/crates/net/p2p/src/beacon/verdict.rs @@ -126,8 +126,16 @@ impl Validated { /// Hand this object on towards the chain actor, given its gossip /// `outcome`. /// - /// A block goes straight to the chain actor whatever the outcome, since - /// its import runs the state transition, which judges it again. A column + /// A block goes on only on `Accept` or `Queue`, the two outcomes that + /// mean its proposer signature verified (see + /// `gossip::block::queue_if_signed`). Its import runs the state transition, + /// which judges it again, but a block that has to wait first (for its + /// parent, then for its custody columns) is kept unjudged the whole time, + /// so one whose signature nobody checked must never reach the actor: + /// mainnet gossip carries altered copies of real blocks that no peer has + /// columns for. Anything else (`Overloaded` included) is dropped, and a + /// real block dropped here still arrives through a child's by-root fetch + /// or range sync. A column /// goes straight there only on `Accept`: the chain actor keeps a column /// without checking it, so one gossip did not finish judging goes through /// [`column_checks`] first. An aggregate goes on only on `Accept`, and @@ -144,7 +152,7 @@ impl Validated { return; }; match self { - Self::Block { block, .. } => { + Self::Block { block, .. } if matches!(outcome, Outcome::Accept | Outcome::Queue(_)) => { // `decode_start` is the wire arrival, so the import's decode // section spans the decode and gossip validation. let arrival = BlockArrival { @@ -174,7 +182,7 @@ impl Validated { .new_beacon_aggregate(aggregate, attesting_indices, arrival) .inspect_err(|err| warn!(%err, "Failed to forward a gossip aggregate")); } - Self::Aggregate { .. } | Self::Attestation { .. } => {} + Self::Block { .. } | Self::Aggregate { .. } | Self::Attestation { .. } => {} } } } @@ -286,13 +294,14 @@ pub(crate) fn report(server: &P2PServer, id: GossipId, outcome: Outcome) -> bool /// /// With every permit taken, the object is reported `Ignore(Overloaded)` /// instead of queued: queueing it would only make its verdict later than -/// gossipsub's cache can wait for, so it never propagates unvalidated. A block -/// still goes on towards the chain actor regardless: its import runs the -/// state transition, which judges it again. A column goes through -/// [`column_checks`] instead (see [`Validated::forward`]); dropping either -/// here would leave the actor to learn of it only through a child's by-root -/// fetch or range sync, both far slower than gossip. An aggregate or a subnet -/// attestation is not forwarded on this outcome at all: see +/// gossipsub's cache can wait for, so it never propagates unvalidated. A +/// column still goes on, through [`column_checks`] (see +/// [`Validated::forward`]); dropping it here would leave the actor to learn of +/// it only through a by-root fetch, far slower than gossip. A block is +/// dropped: its signature went unchecked, and [`Validated::forward`] keeps +/// every such block off the chain actor. None was measured overloaded on the +/// mainnet followers over the 24 hours before this changed. An aggregate or a +/// subnet attestation is not forwarded on this outcome either: see /// [`Validated::forward`]'s own documentation for why. pub(crate) fn spawn_stateful_checks( server: &P2PServer, @@ -430,9 +439,10 @@ mod tests { } /// A [`P2PToBlockChain`] stand-in that only records whether - /// `new_beacon_aggregate` was called, for the tests that check `forward` - /// keeps an aggregate off the chain actor on every outcome but `Accept`. - struct RecordingChain(AtomicBool); + /// `new_beacon_aggregate` (the first flag) or `new_block` (the second) + /// was called, for the tests that check which outcomes `forward` lets + /// through to the chain actor. + struct RecordingChain(AtomicBool, AtomicBool); impl P2PToBlockChain for RecordingChain { fn new_block( @@ -441,6 +451,7 @@ mod tests { _source: BlockSource, _arrival: BlockArrival, ) -> Result<(), ActorError> { + self.1.store(true, Ordering::SeqCst); Ok(()) } fn new_attestation(&self, _attestation: SignedAttestation) -> Result<(), ActorError> { @@ -584,7 +595,10 @@ mod tests { #[tokio::test] async fn an_overloaded_aggregate_is_not_forwarded() { let mut server = unconnected_beacon_server(Config::mainnet(), 0).await; - let chain = Arc::new(RecordingChain(AtomicBool::new(false))); + let chain = Arc::new(RecordingChain( + AtomicBool::new(false), + AtomicBool::new(false), + )); server.blockchain = Some(chain.clone()); let object = Validated::Aggregate { aggregate: Box::new(phase0_aggregate(5, 1)), @@ -600,12 +614,54 @@ mod tests { assert!(!chain.0.load(Ordering::SeqCst)); } + /// A block reaches the chain actor only on the outcomes that mean its + /// signature verified: a block the validation pool had no permit for was + /// never judged at all, so it is dropped rather than handed on. + #[tokio::test] + async fn only_an_accepted_or_queued_block_is_forwarded() { + let mut server = unconnected_beacon_server(Config::mainnet(), 0).await; + let forwarded = |server: &mut P2PServer, outcome: Outcome| { + let chain = Arc::new(RecordingChain( + AtomicBool::new(false), + AtomicBool::new(false), + )); + server.blockchain = Some(chain.clone()); + let object = Validated::Block { + block: Box::new(fulu_block(5, 1)), + block_root: Root::repeat_byte(5), + }; + object.forward(server, Instant::now(), outcome); + chain.1.load(Ordering::SeqCst) + }; + + assert!(forwarded(&mut server, Outcome::Accept)); + assert!(forwarded( + &mut server, + Outcome::Queue(QueueReason::ParentNotReady) + )); + assert!(!forwarded( + &mut server, + Outcome::Ignore(IgnoreReason::Overloaded) + )); + assert!(!forwarded( + &mut server, + Outcome::Ignore(IgnoreReason::SignatureUnverified) + )); + assert!(!forwarded( + &mut server, + Outcome::Reject(RejectReason::BadSignature) + )); + } + /// A subnet attestation is never forwarded, on any outcome, `Accept` /// included: see `Validated::forward`'s own documentation for why. #[tokio::test] async fn a_subnet_attestation_is_never_forwarded_even_on_accept() { let mut server = unconnected_beacon_server(Config::mainnet(), 0).await; - let chain = Arc::new(RecordingChain(AtomicBool::new(false))); + let chain = Arc::new(RecordingChain( + AtomicBool::new(false), + AtomicBool::new(false), + )); server.blockchain = Some(chain.clone()); let object = Validated::Attestation { attestation: Box::new(electra_attestation(5, 1)), diff --git a/crates/storage/src/store.rs b/crates/storage/src/store.rs index 604c2cb2..92030d93 100644 --- a/crates/storage/src/store.rs +++ b/crates/storage/src/store.rs @@ -2612,6 +2612,26 @@ impl Store { self.state_cache.lock().unwrap().put(key, state); } + /// Loads the head block's post-state into the state cache, for a store + /// just resumed from disk. + /// + /// Beacon gossip validation reads states only through + /// [`Self::cached_state`], and after a resume nothing else puts the head's + /// there before the first import: fork choice judges a head from an + /// earlier epoch by its persisted unrealized justification, not its state. + /// Until then, a block whose parent has no post-state yet would find no + /// state to check its proposer signature against, and be dropped. + /// + /// A head with no state leaves the cache as it was: the resume path has + /// already refused a directory like that before it gets here. + pub fn cache_head_state(&self) -> Result<(), Error> { + let head = self.head()?; + if let Some(state) = self.get_state(&head)? { + self.cache_state(CacheKey::BlockState(head), state); + } + Ok(()) + } + /// The committee-shuffling cache shared by every clone of this `Store`. /// /// Returns a cloned `Arc` (an atomic increment, like [`Self::config`]) @@ -5200,6 +5220,28 @@ mod tests { ); } + #[test] + fn a_resumed_store_caches_its_head_state_on_request() { + let backend = Arc::new(InMemoryBackend::new()); + { + // `beacon_test_store` names `H256::ZERO` as the head. Dropping the + // store drains its state writer, so the state is on the backend, + // not only in this instance's cache, before the resume below. + let mut store = beacon_test_store(backend.clone()); + store + .insert_state(H256::ZERO, beacon_test_state(7)) + .expect("insert"); + } + + let resumed = Store::from_db_state(backend).unwrap().unwrap(); + let key = CacheKey::BlockState(H256::ZERO); + assert!(resumed.cached_state(key).is_none()); + + resumed.cache_head_state().expect("cache head state"); + let cached = resumed.cached_state(key).expect("the head state is cached"); + assert_eq!(cached.slot(), 7); + } + #[test] fn a_beacon_state_read_is_served_from_the_cache_the_second_time() { let mut store = beacon_test_store(Arc::new(InMemoryBackend::new())); From 4f517ba0050367f320bb1ccf0ae594011eefd1ef 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, 28 Sep 2026 16:17:27 -0300 Subject: [PATCH 2/2] docs(beacon): describe which gossip blocks reach the chain actor now The previous commit stopped forwarding a block gossip could not verify the signature of, `Overloaded` included, so the "still forwarded" and "either way" passages no longer hold. It also added an ignore reason and a counter nothing documented yet. --- docs/beacon_wire.md | 19 +++++++++++++++---- docs/metrics.md | 10 +++++++++- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/docs/beacon_wire.md b/docs/beacon_wire.md index 3e55a72b..2ad898ca 100644 --- a/docs/beacon_wire.md +++ b/docs/beacon_wire.md @@ -140,10 +140,21 @@ message; a message whose dependency is not ready yet is IGNOREd. See [Aggregate attestations](#aggregate-attestations) for the two topics with their own section. -Blocks and data column sidecars are, either way, handed to the chain actor, -which parks what it cannot import yet and imports the rest immediately, such -as a sidecar whose slot merely falls outside its parent state's proposer -lookahead. An aggregate reaches the chain actor only on `Accept`, carrying the +A block reaches the chain actor only once its proposer signature has +verified: on `Accept`, or on `Queue` for one whose parent has no post-state +yet but whose signature the cached head state checked. A block gossip could +not judge that far (no cached head state, a proposer that state does not +hold, or no free validation permit) is dropped, because the chain actor keeps +a waiting block unjudged until its parent and then its custody columns +arrive, and mainnet gossip carries altered copies of real blocks that no peer +has columns for. A real block dropped here still arrives through a child's +by-root fetch or range sync, and the chain actor checks the signature of +those too before it lets one wait (see +`lean_beacon_blocks_refused_before_waiting_total` in +[metrics.md](./metrics.md)). Data column sidecars are, either way, handed to +the chain actor, which parks what it cannot import yet and imports the rest +immediately, such as a sidecar whose slot merely falls outside its parent +state's proposer lookahead. An aggregate reaches the chain actor only on `Accept`, carrying the attesting indices gossip validation resolved. A subnet attestation never reaches it at all, on any outcome: verifying and relaying it is the whole of what this node owes the topic (see above), so there is nothing further for the diff --git a/docs/metrics.md b/docs/metrics.md index 214392e8..631d9400 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -245,7 +245,7 @@ The bookkeeping an import triggers (chain-event emission, the finality eviction `decode` is a gossip-only section, so `queue` is the only one a fetched block crosses before the chain actor. The req/resp codec has already turned the bytes into a block before any handler sees one, leaving no decode boundary to take; the path reports nothing rather than a zero, since a zero reads as free work rather than as unmeasured work and would drag the decode histogram down with samples that measured nothing. Two consequences: `decode` is a gossip population even though the `source` label allows `sync`, and a fetched block's `total` starts later in its life than a gossiped block's, having never counted the request round trip at all. -On the beacon wire, `decode` for `source="gossip"` spans more than its name says. `BlockArrival::decode_start` is still the wire arrival, but `handed_off` is stamped only once the block has a gossip verdict, so the section also covers the cheap, stateless checks, the stateful check's own `spawn_blocking` task, and the verdict's trip back through the p2p actor's mailbox. There is no wait for a free validation slot to attribute here either: `try_acquire_owned` never blocks, and a message arriving with none free is reported `Ignore(Overloaded)` (and still forwarded to the chain actor) rather than queued. A rising `decode` on the beacon wire alone therefore does not mean decoding got slower; check `lean_beacon_gossip_validation_seconds` before assuming so. +On the beacon wire, `decode` for `source="gossip"` spans more than its name says. `BlockArrival::decode_start` is still the wire arrival, but `handed_off` is stamped only once the block has a gossip verdict, so the section also covers the cheap, stateless checks, the stateful check's own `spawn_blocking` task, and the verdict's trip back through the p2p actor's mailbox. There is no wait for a free validation slot to attribute here either: `try_acquire_owned` never blocks, and a message arriving with none free is reported `Ignore(Overloaded)` rather than queued. A block reported that way never reaches the chain actor, so it contributes no import timing at all. A rising `decode` on the beacon wire alone therefore does not mean decoding got slower; check `lean_beacon_gossip_validation_seconds` before assuming so. `engine` and `fcu` are execution-client round trips. They are I/O waits rather than work, so a node whose import time is dominated by them is waiting on its execution client, not spending CPU. @@ -377,6 +377,13 @@ only), `not_aggregator` (aggregate only), `not_in_committee`, only), `target_not_ancestor`, `wrong_subnet` (attestation only) on the reject side. `already_seen` and `overloaded` are shared with every other topic. +`beacon_block` has one ignore reason of its own, `signature_unverified`: a +block whose parent has no post-state yet, and whose proposer signature no +cached state could check (no head state cached, or a proposer newer than the +head state). It is dropped rather than queued, so it never reaches the chain +actor. A resumed node caches its head state at startup, so a restart alone +does not produce it. + Two permit pools bound the blocking-thread half of validation: `gossip_validation_permits` for blocks and columns, `attestation_validation_permits` for aggregates and subnet attestations. They @@ -453,6 +460,7 @@ spec. | `lean_data_column_kzg_verify_seconds` | Histogram | Time spent batch-verifying one sidecar's cells against its own commitments | On each KZG batch verification, in gossip validation or the chain checks | | 0.001, 0.0025, 0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1.0 | | `lean_data_column_fetch_failures_total` | Counter | `DataColumnsByRoot` lookups this node gave up on, by reason | On lookup abandonment | reason=no_peers,max_retries | | | `lean_blocks_held_for_columns` | Gauge | Blocks held out of fork choice pending their custody columns | On every hold, release, and finality eviction of the held-block set | | | +| `lean_beacon_blocks_refused_before_waiting_total` | Counter | Blocks the chain actor refused instead of parking them for their parent or holding them for their columns, by the precheck rule they broke | On each refusal in `precheck_before_waiting`, whatever path delivered the block | reason=not_after_parent,wrong_proposer,unknown_proposer,bad_signature, wait=parent,columns | | | `lean_sidecars_awaiting_parent` | Gauge | Sidecars parked until their block's parent has a post-state | On every park, replay, and finality eviction of the parked set | | | `lean_data_columns_rejected_total` counts the chain checks, which run in the