diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 27f99dc490..209ac59e19 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/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::{ - CachedShuffling, ShufflingCache, get_ptcs_for_shuffling_epoch, with_cached_shuffling, -}; +use crate::shuffling_cache::{CachedPTCs, CachedShuffling, ShufflingCache, with_cached_shuffling}; use crate::sync_committee_verification::{ Error as SyncCommitteeError, VerifiedSyncCommitteeMessage, VerifiedSyncContribution, }; @@ -4927,11 +4925,11 @@ impl BeaconChain { 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(()) diff --git a/beacon_node/beacon_chain/src/shuffling_cache.rs b/beacon_node/beacon_chain/src/shuffling_cache.rs index 4f97f06b16..01513a41fd 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, BeaconStateError, ChainSpec, Epoch, EthSpec, Hash256, PTC, - RelativeEpoch, Slot, state::CommitteeCache, + AttestationShufflingId, BeaconState, ChainSpec, Epoch, EthSpec, Hash256, PTC, RelativeEpoch, + Slot, state::CommitteeCache, }; use crate::{ @@ -37,11 +37,35 @@ const MAX_CONCURRENT_PROMISES: usize = 2; #[derive(Clone)] pub struct CachedShuffling { pub committee_cache: Arc, - pub ptcs: Option>>, + pub ptcs: CachedPTCs, +} + +#[derive(Clone)] +pub enum CachedPTCs { + PreGloas, + PostGloas(Vec>, Epoch), +} + +impl CachedPTCs { + pub fn from_state( + state: &BeaconState, + epoch: Epoch, + spec: &ChainSpec, + ) -> Result { + if shuffling_requires_ptcs(epoch, spec) { + let ptcs = epoch + .slot_iter(E::slots_per_epoch()) + .map(|slot| state.get_ptc(slot, spec)) + .collect::, _>>()?; + Ok(Self::PostGloas(ptcs, epoch)) + } else { + Ok(Self::PreGloas) + } + } } impl CachedShuffling { - pub fn new(committee_cache: Arc, ptcs: Option>>) -> Self { + pub fn new(committee_cache: Arc, ptcs: CachedPTCs) -> Self { Self { committee_cache, ptcs, @@ -49,20 +73,16 @@ impl CachedShuffling { } pub fn ptc_for_slot(&self, slot: Slot) -> Option<&PTC> { - self.ptcs - .as_ref()? - .get(slot.as_usize() % E::slots_per_epoch() as usize) - } - - fn ensure_ptcs_for_gloas_shuffling( - &self, - shuffling_epoch: Epoch, - spec: &ChainSpec, - ) -> Result<(), BeaconChainError> { - if shuffling_requires_ptcs(shuffling_epoch, spec) && self.ptcs.is_none() { - Err(BeaconChainError::MissingPtcForGloasShuffling { shuffling_epoch }) - } else { - Ok(()) + match &self.ptcs { + CachedPTCs::PreGloas => None, // Should we error here? + CachedPTCs::PostGloas(ptcs, epoch) => { + if slot.epoch(E::slots_per_epoch()) != *epoch { + None // Also we should error here? + } else { + // Note: This may return Option also if construction was buggy + ptcs.get(slot.as_usize() % E::slots_per_epoch() as usize) + } + } } } } @@ -173,7 +193,9 @@ impl ShufflingCache { 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 ShufflingCache { }) } - 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( + pub fn insert_committee_cache( &mut self, key: AttestationShufflingId, cached_shuffling: CachedShuffling, - spec: &ChainSpec, ) -> Result<(), BeaconChainError> { - cached_shuffling.ensure_ptcs_for_gloas_shuffling(key.shuffling_epoch, spec)?; - match self.cache.get(&key) { - Some(CacheItem::Committee(existing)) => { - existing.ensure_ptcs_for_gloas_shuffling(key.shuffling_epoch, spec)?; + Some(CacheItem::Committee(_existing)) => { + // 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) - .map_err(BeaconChainError::from)?; + let ptcs = CachedPTCs::from_state(&state, shuffling_epoch, spec)?; 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( - state: &BeaconState, - shuffling_epoch: Epoch, - spec: &ChainSpec, -) -> Result>>, BeaconStateError> { - if shuffling_requires_ptcs(shuffling_epoch, spec) { - shuffling_epoch - .slot_iter(E::slots_per_epoch()) - .map(|slot| state.get_ptc(slot, spec)) - .collect::, _>>() - .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; -} - -impl ToArcCommitteeCache for CommitteeCache { - fn to_arc_committee_cache(&self) -> Arc { - Arc::new(self.clone()) - } -} - -impl ToArcCommitteeCache for Arc { - fn to_arc_committee_cache(&self) -> Arc { - 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) -> CachedShuffling { - 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::>(); 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, - &spec, + cached_shuffling(committee_cache.clone()), ) .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(); } diff --git a/beacon_node/beacon_chain/src/state_advance_timer.rs b/beacon_node/beacon_chain/src/state_advance_timer.rs index f387563e13..4969b8df5f 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, get_ptcs_for_shuffling_epoch}, + BeaconChain, BeaconChainError, BeaconChainTypes, chain_config::FORK_CHOICE_LOOKAHEAD_FACTOR, + shuffling_cache::CachedShuffling, }; use slot_clock::SlotClock; use state_processing::per_slot_processing; @@ -410,17 +410,12 @@ fn advance_head(beacon_chain: &Arc>) -> Resu "Skipping priming of attester cache for Gloas boundary epoch" ); } else { - let ptcs = get_ptcs_for_shuffling_epoch(&state, shuffling_epoch, &beacon_chain.spec) - .map_err(BeaconChainError::from)?; + 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_with_ptcs( - shuffling_id.clone(), - cached_shuffling, - &beacon_chain.spec, - )?; + .insert_committee_cache(shuffling_id.clone(), cached_shuffling)?; debug!( ?head_block_root, diff --git a/beacon_node/http_api/src/beacon/states.rs b/beacon_node/http_api/src/beacon/states.rs index d68c777428..628a403f28 100644 --- a/beacon_node/http_api/src/beacon/states.rs +++ b/beacon_node/http_api/src/beacon/states.rs @@ -391,7 +391,7 @@ pub fn get_beacon_state_committees( 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( // 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(mut cache_write) = chain + && let Some(_shuffling_id) = shuffling_id + && let Some(_cache_write) = chain .shuffling_cache .try_write_for(std::time::Duration::from_secs(1)) { - let decision_block_root = - shuffling_id.shuffling_decision_block; - if let Err(error) = cache_write.insert_committee_cache( - shuffling_id.clone(), - &possibly_built_cache, - &chain.spec, - ) { - tracing::warn!( - %epoch, - ?decision_block_root, - ?error, - "Priming committee cache failed" - ); - } + // TODO: Do we really need to insert into the committee + // cache? Then we need to be able to produce PTCs for + // historical epochs, or limit the range of query.epoch + // against the state_id + // Theoretically we COULD compute the PTC for historical + // epochs, but should we? If we don't we need to insert + // historical committees to the cache without PTC, so we + // have to have a type of entry that does not have a PTC + // just to support the caching in this route: I persoanlly + // hate this. } - possibly_built_cache + committee_cache }; // Use either the supplied slot or all slots in the epoch.