From 7f43ba77b9481f9618e8c2bfbe10423e54cb5485 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Thu, 21 May 2026 06:45:25 +0200 Subject: [PATCH] Centralise Gloas boundary skip in CachedPTCs::try_from_state CachedPTCs::try_from_state now returns Result, _> and internalises the boundary rule (pre-Gloas state, Gloas shuffling epoch => Ok(None)). Callers (block import priming, state advance timer, with_cached_shuffling miss path) just skip insertion on None instead of duplicating the guard. The unit test exercises the three boundary cases against a pre-Gloas state. --- beacon_node/beacon_chain/src/beacon_chain.rs | 27 +++------ .../beacon_chain/src/shuffling_cache.rs | 60 ++++++++++++++++--- .../beacon_chain/src/state_advance_timer.rs | 33 +++++----- 3 files changed, 76 insertions(+), 44 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index f4021cbd12..d0dddb549f 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -4909,27 +4909,18 @@ impl BeaconChain { let shuffling_id = AttestationShufflingId::new(block_root, state, relative_epoch)?; let shuffling_epoch = relative_epoch.into_epoch(state.current_epoch()); - let shuffling_is_cached = self.shuffling_cache.read().contains(&shuffling_id); - - // Skip priming the cache for `shuffling_epoch` if it is Gloas but the state is not: - // we do not have the PTCs on hand in this case. - if self - .spec - .fork_name_at_epoch(shuffling_epoch) - .gloas_enabled() - && !state.fork_name_unchecked().gloas_enabled() - { + if self.shuffling_cache.read().contains(&shuffling_id) { continue; } - if !shuffling_is_cached { - state.build_committee_cache(relative_epoch, &self.spec)?; - let committee_cache = state.committee_cache(relative_epoch)?; - let ptcs = CachedPTCs::from_state(state, shuffling_epoch, &self.spec)?; - let cached_shuffling = CachedShuffling::new(committee_cache.clone(), ptcs); - self.shuffling_cache - .write() - .insert_committee_cache(shuffling_id, cached_shuffling); + state.build_committee_cache(relative_epoch, &self.spec)?; + let committee_cache = state.committee_cache(relative_epoch)?.clone(); + + if let Some(ptcs) = CachedPTCs::try_from_state(state, shuffling_epoch, &self.spec)? { + self.shuffling_cache.write().insert_committee_cache( + shuffling_id, + CachedShuffling::new(committee_cache, ptcs), + ); } } Ok(()) diff --git a/beacon_node/beacon_chain/src/shuffling_cache.rs b/beacon_node/beacon_chain/src/shuffling_cache.rs index 59f4027726..daaede6ed1 100644 --- a/beacon_node/beacon_chain/src/shuffling_cache.rs +++ b/beacon_node/beacon_chain/src/shuffling_cache.rs @@ -7,8 +7,8 @@ use parking_lot::RwLock; use state_processing::state_advance::partial_state_advance; use tracing::debug; use types::{ - AttestationShufflingId, BeaconState, ChainSpec, Epoch, EthSpec, Hash256, PTC, RelativeEpoch, - Slot, state::CommitteeCache, + AttestationShufflingId, BeaconState, BeaconStateError, ChainSpec, Epoch, EthSpec, Hash256, PTC, + RelativeEpoch, Slot, state::CommitteeCache, }; use crate::{ @@ -47,19 +47,24 @@ pub enum CachedPTCs { } impl CachedPTCs { - pub fn from_state( + /// Returns `None` at the Gloas fork boundary (pre-Gloas state, Gloas shuffling epoch); the + /// on-demand miss path in `with_cached_shuffling` handles those. + pub fn try_from_state( state: &BeaconState, epoch: Epoch, spec: &ChainSpec, - ) -> Result { + ) -> Result, BeaconChainError> { if shuffling_requires_ptcs(epoch, spec) { + if !state.fork_name_unchecked().gloas_enabled() { + return Ok(None); + } let ptcs = epoch .slot_iter(E::slots_per_epoch()) .map(|slot| state.get_ptc(slot, spec)) .collect::, _>>()?; - Ok(Self::PostGloas(ptcs, epoch)) + Ok(Some(Self::PostGloas(ptcs, epoch))) } else { - Ok(Self::PreGloas) + Ok(Some(Self::PreGloas)) } } } @@ -436,7 +441,11 @@ where .committee_cache(relative_epoch) .map_err(BeaconChainError::from)? .clone(); - let ptcs = CachedPTCs::from_state(&state, shuffling_epoch, spec)?; + // The state has been advanced through the upgrade if needed, so `try_from_state` + // cannot return None here. + let ptcs = CachedPTCs::try_from_state(&state, shuffling_epoch, spec)?.ok_or( + BeaconChainError::BeaconStateError(BeaconStateError::IncorrectStateVariant), + )?; let shuffling_decision_block = shuffling_id.shuffling_decision_block; let cached_shuffling = CachedShuffling::new(committee_cache, ptcs); @@ -816,4 +825,41 @@ mod test { "should limit cache size" ); } + + /// Pre-Gloas state across the Gloas fork: epoch G-1 returns `Some(PreGloas)`, epoch G and + /// G+1 return `None` (the boundary skip). + #[test] + fn try_from_state_skips_at_gloas_boundary() { + create_test_tracing_subscriber(); + + let mut spec = ForkName::Fulu.make_genesis_spec(E::default_spec()); + let gloas_fork_epoch = Epoch::new(2); + spec.gloas_fork_epoch = Some(gloas_fork_epoch); + + let harness = BeaconChainHarness::builder(MinimalEthSpec) + .spec(Arc::new(spec.clone())) + .deterministic_keypairs(8) + .fresh_ephemeral_store() + .build(); + let state = harness.get_current_state(); + assert!(!state.fork_name_unchecked().gloas_enabled()); + + for (epoch, expect_pre_gloas) in [ + (gloas_fork_epoch - 1, true), + (gloas_fork_epoch, false), + (gloas_fork_epoch + 1, false), + ] { + let result = CachedPTCs::::try_from_state(&state, epoch, &spec) + .expect("must not error at the boundary"); + if expect_pre_gloas { + assert!( + matches!(result, Some(CachedPTCs::PreGloas)), + "epoch {}: expected Some(PreGloas)", + epoch + ); + } else { + assert!(result.is_none(), "epoch {}: expected None", epoch); + } + } + } } diff --git a/beacon_node/beacon_chain/src/state_advance_timer.rs b/beacon_node/beacon_chain/src/state_advance_timer.rs index d89722e3cd..6408f861f8 100644 --- a/beacon_node/beacon_chain/src/state_advance_timer.rs +++ b/beacon_node/beacon_chain/src/state_advance_timer.rs @@ -13,11 +13,11 @@ //! 1. We are required to store an additional `BeaconState` for the head block. This consumes //! memory. //! 2. There's a possibility that the head block is never built upon, causing wasted CPU cycles. -use crate::shuffling_cache::CachedPTCs; use crate::validator_monitor::HISTORIC_EPOCHS as VALIDATOR_MONITOR_HISTORIC_EPOCHS; use crate::{ - BeaconChain, BeaconChainError, BeaconChainTypes, chain_config::FORK_CHOICE_LOOKAHEAD_FACTOR, - shuffling_cache::CachedShuffling, + BeaconChain, BeaconChainError, BeaconChainTypes, + chain_config::FORK_CHOICE_LOOKAHEAD_FACTOR, + shuffling_cache::{CachedPTCs, CachedShuffling}, }; use slot_clock::SlotClock; use state_processing::per_slot_processing; @@ -396,26 +396,16 @@ fn advance_head(beacon_chain: &Arc>) -> Resu .map_err(BeaconChainError::from)?; let committee_cache = state .committee_cache(RelativeEpoch::Next) - .map_err(BeaconChainError::from)?; + .map_err(BeaconChainError::from)? + .clone(); let shuffling_epoch = RelativeEpoch::Next.into_epoch(state.current_epoch()); - if beacon_chain - .spec - .fork_name_at_epoch(shuffling_epoch) - .gloas_enabled() - && !state.fork_name_unchecked().gloas_enabled() + if let Some(ptcs) = CachedPTCs::try_from_state(&state, shuffling_epoch, &beacon_chain.spec)? { - debug!( - %shuffling_epoch, - "Skipping priming of attester cache for Gloas boundary epoch" + beacon_chain.shuffling_cache.write().insert_committee_cache( + shuffling_id.clone(), + CachedShuffling::new(committee_cache, ptcs), ); - } else { - let ptcs = CachedPTCs::from_state(&state, shuffling_epoch, &beacon_chain.spec)?; - let cached_shuffling = CachedShuffling::new(committee_cache.clone(), ptcs); - beacon_chain - .shuffling_cache - .write() - .insert_committee_cache(shuffling_id.clone(), cached_shuffling); debug!( ?head_block_root, @@ -424,6 +414,11 @@ fn advance_head(beacon_chain: &Arc>) -> Resu current_epoch = %current_slot.epoch(T::EthSpec::slots_per_epoch()), "Primed proposer and attester caches" ); + } else { + debug!( + %shuffling_epoch, + "Skipping priming of attester cache for Gloas boundary epoch" + ); } }