diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index e9a4a34643..5f4379c323 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -4909,18 +4909,29 @@ impl BeaconChain { ) -> Result<(), BlockError> { for relative_epoch in [RelativeEpoch::Current, RelativeEpoch::Next] { 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() + { + continue; + } + if !shuffling_is_cached { state.build_committee_cache(relative_epoch, &self.spec)?; let committee_cache = state.committee_cache(relative_epoch)?; - let shuffling_epoch = relative_epoch.into_epoch(state.current_epoch()); let ptcs = get_ptcs_for_shuffling_epoch(state, shuffling_epoch, &self.spec)?; let cached_shuffling = CachedShuffling::new(committee_cache.clone(), ptcs); self.shuffling_cache .write() - .insert_committee_cache_with_ptc(shuffling_id, cached_shuffling); + .insert_committee_cache_with_ptc(shuffling_id, cached_shuffling, &self.spec)?; } } Ok(()) diff --git a/beacon_node/beacon_chain/src/errors.rs b/beacon_node/beacon_chain/src/errors.rs index 9802f091e0..19e0c693c2 100644 --- a/beacon_node/beacon_chain/src/errors.rs +++ b/beacon_node/beacon_chain/src/errors.rs @@ -135,6 +135,9 @@ pub enum BeaconChainError { state_epoch: Epoch, shuffling_epoch: Epoch, }, + MissingPtcForGloasShuffling { + shuffling_epoch: Epoch, + }, SyncDutiesError(BeaconStateError), InconsistentForwardsIter { request_slot: Slot, diff --git a/beacon_node/beacon_chain/src/payload_attestation_verification/tests.rs b/beacon_node/beacon_chain/src/payload_attestation_verification/tests.rs index 89a050acd6..d4b82c41fc 100644 --- a/beacon_node/beacon_chain/src/payload_attestation_verification/tests.rs +++ b/beacon_node/beacon_chain/src/payload_attestation_verification/tests.rs @@ -300,8 +300,18 @@ async fn ptc_cache_is_primed_at_gloas_fork_boundary() { .mock_execution_layer() .build(); + harness.extend_to_slot(fork_boundary_slot).await; + for slot in test_slots { - harness.extend_to_slot(slot).await; + harness.chain.slot_clock.set_slot(slot.as_u64()); + assert!( + harness + .chain + .shuffling_cache + .read() + .check_gloas_ptcs_invariant(&harness.spec), + "shuffling cache should satisfy the Gloas PTC invariant" + ); let head = harness.chain.canonical_head.cached_head(); let state = &head.snapshot.beacon_state; diff --git a/beacon_node/beacon_chain/src/shuffling_cache.rs b/beacon_node/beacon_chain/src/shuffling_cache.rs index 0a6dc87f05..f7fa66b0bd 100644 --- a/beacon_node/beacon_chain/src/shuffling_cache.rs +++ b/beacon_node/beacon_chain/src/shuffling_cache.rs @@ -53,6 +53,22 @@ impl CachedShuffling { .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(()) + } + } +} + +fn shuffling_requires_ptcs(shuffling_epoch: Epoch, spec: &ChainSpec) -> bool { + spec.fork_name_at_epoch(shuffling_epoch).gloas_enabled() } #[derive(Clone)] @@ -151,31 +167,53 @@ impl ShufflingCache { self.cache.contains_key(key) } + /// Check that all entries for Gloas epochs have PTCs. + #[cfg(test)] + pub fn check_gloas_ptcs_invariant(&self, spec: &ChainSpec) -> bool { + 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::Promise(_) => true, + } + } else { + true + } + }) + } + pub fn insert_committee_cache( &mut self, key: AttestationShufflingId, committee_cache: &C, - ) { + spec: &ChainSpec, + ) -> Result<(), BeaconChainError> { self.insert_committee_cache_with_ptc( key, CachedShuffling::new(committee_cache.to_arc_committee_cache(), None), - ); + spec, + ) } pub fn insert_committee_cache_with_ptc( &mut self, key: AttestationShufflingId, cached_shuffling: CachedShuffling, - ) { - if self - .cache - .get(&key) + 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)?; + } // 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! - .is_none_or(CacheItem::is_promise) - { - self.insert_cache_item(key, CacheItem::Committee(cached_shuffling)); + Some(CacheItem::Promise(_)) | None => { + self.insert_cache_item(key, CacheItem::Committee(cached_shuffling)); + } } + Ok(()) } /// Prunes the cache first before inserting a new cache item. @@ -286,6 +324,7 @@ 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. @@ -338,12 +377,18 @@ where // If the `head_block` is already ahead of that slot, then we should load the state at that // slot, as we've determined above that the `shuffling_epoch` cache will not be too far in // the past. - let target_slot = std::cmp::max( + let mut target_slot = std::cmp::max( shuffling_epoch .saturating_sub(1_u64) .start_slot(T::EthSpec::slots_per_epoch()), head_block.slot, ); + if spec.gloas_fork_epoch == Some(shuffling_epoch) { + target_slot = std::cmp::max( + target_slot, + shuffling_epoch.start_slot(T::EthSpec::slots_per_epoch()), + ); + } // If the head state is useful for this request, use it. Otherwise, read a state from disk // that is advanced as close as possible to `target_slot`. @@ -365,7 +410,9 @@ where // If the state is still in an earlier epoch, advance it to the `target_slot` so that its // next epoch committee cache matches the `shuffling_epoch`. - if state.current_epoch() + 1 < shuffling_epoch { + let advance_to_gloas_fork = spec.gloas_fork_epoch == Some(shuffling_epoch) + && state.current_epoch() < shuffling_epoch; + if state.current_epoch() + 1 < shuffling_epoch || advance_to_gloas_fork { // Advance the state into the required slot, using the "partial" method since the state // roots are not relevant for the shuffling. partial_state_advance(&mut state, Some(state_root), target_slot, spec) @@ -394,7 +441,7 @@ where shuffling_cache_lock .write() - .insert_committee_cache_with_ptc(shuffling_id, cached_shuffling.clone()); + .insert_committee_cache_with_ptc(shuffling_id, cached_shuffling.clone(), spec)?; metrics::stop_timer(committee_building_timer); @@ -410,7 +457,7 @@ pub fn get_ptcs_for_shuffling_epoch( shuffling_epoch: Epoch, spec: &ChainSpec, ) -> Result>>, BeaconStateError> { - if state.fork_name_unchecked().gloas_enabled() { + if shuffling_requires_ptcs(shuffling_epoch, spec) { shuffling_epoch .slot_iter(E::slots_per_epoch()) .map(|slot| state.get_ptc(slot, spec)) @@ -527,6 +574,12 @@ 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) } @@ -696,24 +749,47 @@ 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); + cache + .insert_committee_cache(id_a.clone(), &committee_cache_a, &spec) + .unwrap(); assert!( matches!(cache.get(&id_a).unwrap(), CacheItem::Committee(cached_shuffling) if cached_shuffling.committee_cache == committee_cache_a), "should insert committee cache" ); } + #[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); + cache + .insert_committee_cache(shuffling_id.clone(), committee_cache, &spec) + .unwrap(); } for i in 1..(TEST_CACHE_SIZE + 1) { @@ -737,6 +813,7 @@ 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()); @@ -746,7 +823,9 @@ mod test { shuffling_epoch: (current_epoch + 1).into(), shuffling_decision_block: Hash256::from_low_u64_be(current_epoch + i as u64), }; - cache.insert_committee_cache(shuffling_id, &committee_cache); + cache + .insert_committee_cache(shuffling_id, &committee_cache, &spec) + .unwrap(); } // Now, update the head shuffling ids @@ -759,12 +838,19 @@ mod test { cache.update_head_shuffling_ids(head_shuffling_ids.clone()); // Insert head state shuffling ids. Should not be overridden by other shuffling ids. - cache.insert_committee_cache(head_shuffling_ids.current.clone(), &committee_cache); - cache.insert_committee_cache(head_shuffling_ids.next.clone(), &committee_cache); - cache.insert_committee_cache( - head_shuffling_ids.previous.clone().unwrap(), - &committee_cache, - ); + cache + .insert_committee_cache(head_shuffling_ids.current.clone(), &committee_cache, &spec) + .unwrap(); + cache + .insert_committee_cache(head_shuffling_ids.next.clone(), &committee_cache, &spec) + .unwrap(); + cache + .insert_committee_cache( + head_shuffling_ids.previous.clone().unwrap(), + &committee_cache, + &spec, + ) + .unwrap(); // Insert a few entries for older epochs. for i in 0..TEST_CACHE_SIZE { @@ -772,7 +858,9 @@ mod test { shuffling_epoch: Epoch::from(i), shuffling_decision_block: Hash256::from_low_u64_be(i as u64), }; - cache.insert_committee_cache(shuffling_id, &committee_cache); + cache + .insert_committee_cache(shuffling_id, &committee_cache, &spec) + .unwrap(); } assert!( diff --git a/beacon_node/beacon_chain/src/state_advance_timer.rs b/beacon_node/beacon_chain/src/state_advance_timer.rs index cc67105dd9..fbd171c425 100644 --- a/beacon_node/beacon_chain/src/state_advance_timer.rs +++ b/beacon_node/beacon_chain/src/state_advance_timer.rs @@ -397,25 +397,39 @@ fn advance_head(beacon_chain: &Arc>) -> Resu let committee_cache = state .committee_cache(RelativeEpoch::Next) .map_err(BeaconChainError::from)?; - let ptcs = get_ptcs_for_shuffling_epoch( - &state, - RelativeEpoch::Next.into_epoch(state.current_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_ptc(shuffling_id.clone(), cached_shuffling); + let shuffling_epoch = RelativeEpoch::Next.into_epoch(state.current_epoch()); - debug!( - ?head_block_root, - next_epoch_shuffling_root = ?shuffling_id.shuffling_decision_block, - state_epoch = %state.current_epoch(), - current_epoch = %current_slot.epoch(T::EthSpec::slots_per_epoch()), - "Primed proposer and attester caches" - ); + if beacon_chain + .spec + .fork_name_at_epoch(shuffling_epoch) + .gloas_enabled() + && !state.fork_name_unchecked().gloas_enabled() + { + debug!( + %shuffling_epoch, + "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 cached_shuffling = CachedShuffling::new(committee_cache.clone(), ptcs); + beacon_chain + .shuffling_cache + .write() + .insert_committee_cache_with_ptc( + shuffling_id.clone(), + cached_shuffling, + &beacon_chain.spec, + )?; + + debug!( + ?head_block_root, + next_epoch_shuffling_root = ?shuffling_id.shuffling_decision_block, + state_epoch = %state.current_epoch(), + current_epoch = %current_slot.epoch(T::EthSpec::slots_per_epoch()), + "Primed proposer and attester caches" + ); + } } let final_slot = state.slot(); diff --git a/beacon_node/http_api/src/beacon/states.rs b/beacon_node/http_api/src/beacon/states.rs index cdc60f0418..d68c777428 100644 --- a/beacon_node/http_api/src/beacon/states.rs +++ b/beacon_node/http_api/src/beacon/states.rs @@ -449,10 +449,20 @@ pub fn get_beacon_state_committees( .shuffling_cache .try_write_for(std::time::Duration::from_secs(1)) { - cache_write.insert_committee_cache( - shuffling_id, + 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" + ); + } } possibly_built_cache