Centralise Gloas boundary skip in CachedPTCs::try_from_state

CachedPTCs::try_from_state now returns Result<Option<Self>, _> 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

@@ -4909,27 +4909,18 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             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);
+            if 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()
-            {
                 continue;
             }
 
-            if !shuffling_is_cached {
+            state.build_committee_cache(relative_epoch, &self.spec)?;
-                state.build_committee_cache(relative_epoch, &self.spec)?;
+            let committee_cache = state.committee_cache(relative_epoch)?.clone();
-                let committee_cache = state.committee_cache(relative_epoch)?;
+
-                let ptcs = CachedPTCs::from_state(state, shuffling_epoch, &self.spec)?;
+            if let Some(ptcs) = CachedPTCs::try_from_state(state, shuffling_epoch, &self.spec)? {
-                let cached_shuffling = CachedShuffling::new(committee_cache.clone(), ptcs);
+                self.shuffling_cache.write().insert_committee_cache(
-                self.shuffling_cache
+                    shuffling_id,
-                    .write()
+                    CachedShuffling::new(committee_cache, ptcs),
-                    .insert_committee_cache(shuffling_id, cached_shuffling);
+                );
             }
         }
         Ok(())

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,
+    AttestationShufflingId, BeaconState, BeaconStateError, ChainSpec, Epoch, EthSpec, Hash256, PTC,
-    Slot, state::CommitteeCache,
+    RelativeEpoch, Slot, state::CommitteeCache,
 };
 
 use crate::{
@@ -47,19 +47,24 @@ pub enum CachedPTCs<E: EthSpec> {
 }
 
 impl<E: EthSpec> CachedPTCs<E> {
-    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<E>,
         epoch: Epoch,
         spec: &ChainSpec,
-    ) -> Result<Self, BeaconChainError> {
+    ) -> Result<Option<Self>, 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::<Result<Vec<_>, _>>()?;
-            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::<E>::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);
+            }
+        }
+    }
 }

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,
+    BeaconChain, BeaconChainError, BeaconChainTypes,
-    shuffling_cache::CachedShuffling,
+    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<T: BeaconChainTypes>(beacon_chain: &Arc<BeaconChain<T>>) -> 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
+        if let Some(ptcs) = CachedPTCs::try_from_state(&state, shuffling_epoch, &beacon_chain.spec)?
-            .spec
-            .fork_name_at_epoch(shuffling_epoch)
-            .gloas_enabled()
-            && !state.fork_name_unchecked().gloas_enabled()
         {
-            debug!(
+            beacon_chain.shuffling_cache.write().insert_committee_cache(
-                %shuffling_epoch,
+                shuffling_id.clone(),
-                "Skipping priming of attester cache for Gloas boundary epoch"
+                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<T: BeaconChainTypes>(beacon_chain: &Arc<BeaconChain<T>>) -> 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"
+            );
         }
     }