Check that PTCs exist when inserting into the cache

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -77,9 +77,7 @@ use crate::persisted_custody::persist_custody_context;
 use crate::persisted_fork_choice::PersistedForkChoice;
 use crate::pre_finalization_cache::PreFinalizationBlockCache;
 use crate::proposer_preferences_verification::proposer_preference_cache::GossipVerifiedProposerPreferenceCache;
-use crate::shuffling_cache::{
+use crate::shuffling_cache::{CachedPTCs, CachedShuffling, ShufflingCache, with_cached_shuffling};
-    CachedShuffling, ShufflingCache, get_ptcs_for_shuffling_epoch, with_cached_shuffling,
-};
 use crate::sync_committee_verification::{
     Error as SyncCommitteeError, VerifiedSyncCommitteeMessage, VerifiedSyncContribution,
 };
@@ -4927,11 +4925,11 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             if !shuffling_is_cached {
                 state.build_committee_cache(relative_epoch, &self.spec)?;
                 let committee_cache = state.committee_cache(relative_epoch)?;
-                let ptcs = get_ptcs_for_shuffling_epoch(state, shuffling_epoch, &self.spec)?;
+                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_with_ptcs(shuffling_id, cached_shuffling, &self.spec)?;
+                    .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, BeaconStateError, ChainSpec, Epoch, EthSpec, Hash256, PTC,
+    AttestationShufflingId, BeaconState, ChainSpec, Epoch, EthSpec, Hash256, PTC, RelativeEpoch,
-    RelativeEpoch, Slot, state::CommitteeCache,
+    Slot, state::CommitteeCache,
 };
 
 use crate::{
@@ -37,11 +37,35 @@ const MAX_CONCURRENT_PROMISES: usize = 2;
 #[derive(Clone)]
 pub struct CachedShuffling<E: EthSpec> {
     pub committee_cache: Arc<CommitteeCache>,
-    pub ptcs: Option<Vec<PTC<E>>>,
+    pub ptcs: CachedPTCs<E>,
+}
+
+#[derive(Clone)]
+pub enum CachedPTCs<E: EthSpec> {
+    PreGloas,
+    PostGloas(Vec<PTC<E>>, Epoch),
+}
+
+impl<E: EthSpec> CachedPTCs<E> {
+    pub fn from_state(
+        state: &BeaconState<E>,
+        epoch: Epoch,
+        spec: &ChainSpec,
+    ) -> Result<Self, BeaconChainError> {
+        if shuffling_requires_ptcs(epoch, spec) {
+            let ptcs = epoch
+                .slot_iter(E::slots_per_epoch())
+                .map(|slot| state.get_ptc(slot, spec))
+                .collect::<Result<Vec<_>, _>>()?;
+            Ok(Self::PostGloas(ptcs, epoch))
+        } else {
+            Ok(Self::PreGloas)
+        }
+    }
 }
 
 impl<E: EthSpec> CachedShuffling<E> {
-    pub fn new(committee_cache: Arc<CommitteeCache>, ptcs: Option<Vec<PTC<E>>>) -> Self {
+    pub fn new(committee_cache: Arc<CommitteeCache>, ptcs: CachedPTCs<E>) -> Self {
         Self {
             committee_cache,
             ptcs,
@@ -49,20 +73,16 @@ impl<E: EthSpec> CachedShuffling<E> {
     }
 
     pub fn ptc_for_slot(&self, slot: Slot) -> Option<&PTC<E>> {
-        self.ptcs
+        match &self.ptcs {
-            .as_ref()?
+            CachedPTCs::PreGloas => None, // Should we error here?
-            .get(slot.as_usize() % E::slots_per_epoch() as usize)
+            CachedPTCs::PostGloas(ptcs, epoch) => {
-    }
+                if slot.epoch(E::slots_per_epoch()) != *epoch {
-
+                    None // Also we should error here?
-    fn ensure_ptcs_for_gloas_shuffling(
+                } else {
-        &self,
+                    // Note: This may return Option also if construction was buggy
-        shuffling_epoch: Epoch,
+                    ptcs.get(slot.as_usize() % E::slots_per_epoch() as usize)
-        spec: &ChainSpec,
+                }
-    ) -> Result<(), BeaconChainError> {
+            }
-        if shuffling_requires_ptcs(shuffling_epoch, spec) && self.ptcs.is_none() {
-            Err(BeaconChainError::MissingPtcForGloasShuffling { shuffling_epoch })
-        } else {
-            Ok(())
         }
     }
 }
@@ -173,7 +193,9 @@ impl<E: EthSpec> ShufflingCache<E> {
         self.cache.iter().all(|(key, item)| {
             if shuffling_requires_ptcs(key.shuffling_epoch, spec) {
                 match item {
-                    CacheItem::Committee(cached_shuffling) => cached_shuffling.ptcs.is_some(),
+                    CacheItem::Committee(cached_shuffling) => {
+                        matches!(cached_shuffling.ptcs, CachedPTCs::PostGloas(..))
+                    }
                     CacheItem::Promise(_) => true,
                 }
             } else {
@@ -182,30 +204,14 @@ impl<E: EthSpec> ShufflingCache<E> {
         })
     }
 
-    pub fn insert_committee_cache<C: ToArcCommitteeCache>(
+    pub fn insert_committee_cache(
-        &mut self,
-        key: AttestationShufflingId,
-        committee_cache: &C,
-        spec: &ChainSpec,
-    ) -> Result<(), BeaconChainError> {
-        self.insert_committee_cache_with_ptcs(
-            key,
-            CachedShuffling::new(committee_cache.to_arc_committee_cache(), None),
-            spec,
-        )
-    }
-
-    pub fn insert_committee_cache_with_ptcs(
         &mut self,
         key: AttestationShufflingId,
         cached_shuffling: CachedShuffling<E>,
-        spec: &ChainSpec,
     ) -> Result<(), BeaconChainError> {
-        cached_shuffling.ensure_ptcs_for_gloas_shuffling(key.shuffling_epoch, spec)?;
-
         match self.cache.get(&key) {
-            Some(CacheItem::Committee(existing)) => {
+            Some(CacheItem::Committee(_existing)) => {
-                existing.ensure_ptcs_for_gloas_shuffling(key.shuffling_epoch, spec)?;
+                // What should we do?
             }
             // Replace the committee if it's not present or if it's a promise. A bird in the hand is
             // worth two in the promise-bush!
@@ -324,7 +330,6 @@ where
         drop(shuffling_cache);
 
         let cached_shuffling = cache_item.wait()?;
-        cached_shuffling.ensure_ptcs_for_gloas_shuffling(shuffling_epoch, spec)?;
         map_fn(&cached_shuffling, shuffling_id.shuffling_decision_block)
     } else {
         // Create an entry in the cache that "promises" this value will eventually be computed.
@@ -434,14 +439,13 @@ where
             .committee_cache(relative_epoch)
             .map_err(BeaconChainError::from)?
             .clone();
-        let ptcs = get_ptcs_for_shuffling_epoch(&state, shuffling_epoch, spec)
+        let ptcs = CachedPTCs::from_state(&state, shuffling_epoch, spec)?;
-            .map_err(BeaconChainError::from)?;
         let shuffling_decision_block = shuffling_id.shuffling_decision_block;
         let cached_shuffling = CachedShuffling::new(committee_cache, ptcs);
 
         shuffling_cache_lock
             .write()
-            .insert_committee_cache_with_ptcs(shuffling_id, cached_shuffling.clone(), spec)?;
+            .insert_committee_cache(shuffling_id, cached_shuffling.clone())?;
 
         metrics::stop_timer(committee_building_timer);
 
@@ -451,40 +455,6 @@ where
     }
 }
 
-/// Return the PTCs associated with each slot in `shuffling_epoch`, when the state supports PTCs.
-pub fn get_ptcs_for_shuffling_epoch<E: EthSpec>(
-    state: &BeaconState<E>,
-    shuffling_epoch: Epoch,
-    spec: &ChainSpec,
-) -> Result<Option<Vec<PTC<E>>>, BeaconStateError> {
-    if shuffling_requires_ptcs(shuffling_epoch, spec) {
-        shuffling_epoch
-            .slot_iter(E::slots_per_epoch())
-            .map(|slot| state.get_ptc(slot, spec))
-            .collect::<Result<Vec<_>, _>>()
-            .map(Some)
-    } else {
-        Ok(None)
-    }
-}
-
-/// A helper trait to allow lazy-cloning of the committee cache when inserting into the cache.
-pub trait ToArcCommitteeCache {
-    fn to_arc_committee_cache(&self) -> Arc<CommitteeCache>;
-}
-
-impl ToArcCommitteeCache for CommitteeCache {
-    fn to_arc_committee_cache(&self) -> Arc<CommitteeCache> {
-        Arc::new(self.clone())
-    }
-}
-
-impl ToArcCommitteeCache for Arc<CommitteeCache> {
-    fn to_arc_committee_cache(&self) -> Arc<CommitteeCache> {
-        self.clone()
-    }
-}
-
 /// Contains the shuffling IDs for a beacon block.
 #[derive(Clone)]
 pub struct BlockShufflingIds {
@@ -574,14 +544,8 @@ mod test {
         ShufflingCache::new(TEST_CACHE_SIZE, head_shuffling_ids)
     }
 
-    fn test_spec() -> ChainSpec {
-        // Use a Fulu spec specifically because behaviour changes at Gloas.
-        // The Gloas tests explicitly enable Gloas.
-        ForkName::Fulu.make_genesis_spec(E::default_spec())
-    }
-
     fn cached_shuffling(committee_cache: Arc<CommitteeCache>) -> CachedShuffling<E> {
-        CachedShuffling::new(committee_cache, None)
+        CachedShuffling::new(committee_cache, CachedPTCs::PreGloas)
     }
 
     /// Returns two different committee caches for testing.
@@ -749,11 +713,10 @@ mod test {
     #[test]
     fn should_insert_committee_cache() {
         let mut cache = new_shuffling_cache();
-        let spec = test_spec();
         let id_a = shuffling_id(1);
         let committee_cache_a = Arc::new(CommitteeCache::default());
         cache
-            .insert_committee_cache(id_a.clone(), &committee_cache_a, &spec)
+            .insert_committee_cache(id_a.clone(), cached_shuffling(committee_cache_a.clone()))
             .unwrap();
         assert!(
             matches!(cache.get(&id_a).unwrap(), CacheItem::Committee(cached_shuffling) if cached_shuffling.committee_cache == committee_cache_a),
@@ -761,34 +724,19 @@ mod test {
         );
     }
 
-    #[test]
-    fn should_reject_gloas_committee_cache_without_ptc() {
-        let mut cache = new_shuffling_cache();
-        let spec = ForkName::Gloas.make_genesis_spec(E::default_spec());
-        let id = shuffling_id(1);
-        let committee_cache = Arc::new(CommitteeCache::default());
-
-        let result = cache.insert_committee_cache(id.clone(), &committee_cache, &spec);
-
-        assert!(matches!(
-            result,
-            Err(BeaconChainError::MissingPtcForGloasShuffling { shuffling_epoch })
-                if shuffling_epoch == id.shuffling_epoch
-        ));
-        assert!(!cache.contains(&id), "should not insert invalid cache");
-    }
-
     #[test]
     fn should_prune_committee_cache_with_lowest_epoch() {
         let mut cache = new_shuffling_cache();
-        let spec = test_spec();
         let shuffling_id_and_committee_caches = (0..(TEST_CACHE_SIZE + 1))
             .map(|i| (shuffling_id(i as u64), Arc::new(CommitteeCache::default())))
             .collect::<Vec<_>>();
 
         for (shuffling_id, committee_cache) in shuffling_id_and_committee_caches.iter() {
             cache
-                .insert_committee_cache(shuffling_id.clone(), committee_cache, &spec)
+                .insert_committee_cache(
+                    shuffling_id.clone(),
+                    cached_shuffling(committee_cache.clone()),
+                )
                 .unwrap();
         }
 
@@ -813,7 +761,6 @@ mod test {
     #[test]
     fn should_retain_head_state_shufflings() {
         let mut cache = new_shuffling_cache();
-        let spec = test_spec();
         let current_epoch = 10;
         let committee_cache = Arc::new(CommitteeCache::default());
 
@@ -824,7 +771,7 @@ mod test {
                 shuffling_decision_block: Hash256::from_low_u64_be(current_epoch + i as u64),
             };
             cache
-                .insert_committee_cache(shuffling_id, &committee_cache, &spec)
+                .insert_committee_cache(shuffling_id, cached_shuffling(committee_cache.clone()))
                 .unwrap();
         }
 
@@ -839,16 +786,21 @@ mod test {
 
         // Insert head state shuffling ids. Should not be overridden by other shuffling ids.
         cache
-            .insert_committee_cache(head_shuffling_ids.current.clone(), &committee_cache, &spec)
+            .insert_committee_cache(
+                head_shuffling_ids.current.clone(),
+                cached_shuffling(committee_cache.clone()),
+            )
             .unwrap();
         cache
-            .insert_committee_cache(head_shuffling_ids.next.clone(), &committee_cache, &spec)
+            .insert_committee_cache(
+                head_shuffling_ids.next.clone(),
+                cached_shuffling(committee_cache.clone()),
+            )
             .unwrap();
         cache
             .insert_committee_cache(
                 head_shuffling_ids.previous.clone().unwrap(),
-                &committee_cache,
+                cached_shuffling(committee_cache.clone()),
-                &spec,
             )
             .unwrap();
 
@@ -859,7 +811,7 @@ mod test {
                 shuffling_decision_block: Hash256::from_low_u64_be(i as u64),
             };
             cache
-                .insert_committee_cache(shuffling_id, &committee_cache, &spec)
+                .insert_committee_cache(shuffling_id, cached_shuffling(committee_cache.clone()))
                 .unwrap();
         }
 

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,
+    BeaconChain, BeaconChainError, BeaconChainTypes, chain_config::FORK_CHOICE_LOOKAHEAD_FACTOR,
-    chain_config::FORK_CHOICE_LOOKAHEAD_FACTOR,
+    shuffling_cache::CachedShuffling,
-    shuffling_cache::{CachedShuffling, get_ptcs_for_shuffling_epoch},
 };
 use slot_clock::SlotClock;
 use state_processing::per_slot_processing;
@@ -410,17 +410,12 @@ fn advance_head<T: BeaconChainTypes>(beacon_chain: &Arc<BeaconChain<T>>) -> Resu
                 "Skipping priming of attester cache for Gloas boundary epoch"
             );
         } else {
-            let ptcs = get_ptcs_for_shuffling_epoch(&state, shuffling_epoch, &beacon_chain.spec)
+            let ptcs = CachedPTCs::from_state(&state, shuffling_epoch, &beacon_chain.spec)?;
-                .map_err(BeaconChainError::from)?;
             let cached_shuffling = CachedShuffling::new(committee_cache.clone(), ptcs);
             beacon_chain
                 .shuffling_cache
                 .write()
-                .insert_committee_cache_with_ptcs(
+                .insert_committee_cache(shuffling_id.clone(), cached_shuffling)?;
-                    shuffling_id.clone(),
-                    cached_shuffling,
-                    &beacon_chain.spec,
-                )?;
 
             debug!(
                 ?head_block_root,

beacon_node/http_api/src/beacon/states.rs

@@ -391,7 +391,7 @@ pub fn get_beacon_state_committees<T: BeaconChainTypes>(
                                     if let Some(shuffling) = maybe_cached_shuffling {
                                         shuffling
                                     } else {
-                                        let possibly_built_cache =
+                                        let committee_cache =
                                             match RelativeEpoch::from_epoch(current_epoch, epoch) {
                                                 Ok(relative_epoch)
                                                     if state.committee_cache_is_initialized(
@@ -444,28 +444,24 @@ pub fn get_beacon_state_committees<T: BeaconChainTypes>(
                                         // size is not the default value).
                                         if chain.config.shuffling_cache_size
                                             != beacon_chain::shuffling_cache::DEFAULT_CACHE_SIZE
-                                            && let Some(shuffling_id) = shuffling_id
+                                            && let Some(_shuffling_id) = shuffling_id
-                                            && let Some(mut cache_write) = chain
+                                            && let Some(_cache_write) = chain
                                                 .shuffling_cache
                                                 .try_write_for(std::time::Duration::from_secs(1))
                                         {
-                                            let decision_block_root =
+                                            // TODO: Do we really need to insert into the committee
-                                                shuffling_id.shuffling_decision_block;
+                                            // cache? Then we need to be able to produce PTCs for
-                                            if let Err(error) = cache_write.insert_committee_cache(
+                                            // historical epochs, or limit the range of query.epoch
-                                                shuffling_id.clone(),
+                                            // against the state_id
-                                                &possibly_built_cache,
+                                            // Theoretically we COULD compute the PTC for historical
-                                                &chain.spec,
+                                            // epochs, but should we? If we don't we need to insert
-                                            ) {
+                                            // historical committees to the cache without PTC, so we
-                                                tracing::warn!(
+                                            // have to have a type of entry that does not have a PTC
-                                                    %epoch,
+                                            // just to support the caching in this route: I persoanlly
-                                                    ?decision_block_root,
+                                            // hate this.
-                                                    ?error,
-                                                    "Priming committee cache failed"
-                                                );
-                                            }
                                         }
 
-                                        possibly_built_cache
+                                        committee_cache
                                     };
 
                                 // Use either the supplied slot or all slots in the epoch.