diff --git a/bin/ethlambda/src/main.rs b/bin/ethlambda/src/main.rs index 01774b51..ba4b76cf 100644 --- a/bin/ethlambda/src/main.rs +++ b/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"); diff --git a/crates/blockchain/src/lib.rs b/crates/blockchain/src/lib.rs index 0044889d..b0c29a72 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::{ @@ -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 diff --git a/crates/blockchain/src/metrics.rs b/crates/blockchain/src/metrics.rs index 3a40d0d7..5c86bb40 100644 --- a/crates/blockchain/src/metrics.rs +++ b/crates/blockchain/src/metrics.rs @@ -949,6 +949,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!( @@ -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 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 6d7c3136..d5ca55aa 100644 --- a/crates/net/p2p/src/beacon/verdict.rs +++ b/crates/net/p2p/src/beacon/verdict.rs @@ -127,8 +127,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 @@ -166,7 +174,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 { @@ -196,7 +204,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 { .. } => {} } } } @@ -363,13 +371,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, @@ -539,9 +548,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( @@ -550,6 +560,7 @@ mod tests { _source: BlockSource, _arrival: BlockArrival, ) -> Result<(), ActorError> { + self.1.store(true, Ordering::SeqCst); Ok(()) } fn new_attestation(&self, _attestation: SignedAttestation) -> Result<(), ActorError> { @@ -693,7 +704,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)), @@ -709,6 +723,45 @@ 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) + )); + } + /// An accepted electra aggregate is what block production packs other /// nodes' votes from, so `forward` has to put it in the pool; a phase0 one /// has no place there, since the pool holds electra attestations. @@ -767,7 +820,10 @@ mod tests { #[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 96addfbd..eec0e844 100644 --- a/crates/storage/src/store.rs +++ b/crates/storage/src/store.rs @@ -2645,6 +2645,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`]) @@ -5555,6 +5575,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())); diff --git a/docs/beacon_wire.md b/docs/beacon_wire.md index 59912834..955ff080 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 bf215a3c..b23ddc22 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -251,7 +251,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. @@ -383,6 +383,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 @@ -459,6 +466,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