From 90ddaba1dbbd50e0c2dbe311c5ae62167d44ce1a Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 18 Mar 2022 13:07:23 +1100 Subject: [PATCH] Delete inaccurate block replay --- beacon_node/beacon_chain/src/beacon_chain.rs | 70 +++++++++---------- beacon_node/beacon_chain/tests/store_tests.rs | 42 ----------- beacon_node/store/src/hot_cold_store.rs | 36 ---------- 3 files changed, 34 insertions(+), 114 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 328101a52b..8d9be671bf 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -3919,6 +3919,17 @@ impl BeaconChain { "head_block_root" => head_block_root.to_string(), ); + // If the block's state will be so far ahead of `shuffling_epoch` that even its + // previous epoch committee cache will be too new, then error. Callers of this function + // shouldn't be requesting such old shufflings for this `head_block_root`. + let head_block_epoch = head_block.slot.epoch(T::EthSpec::slots_per_epoch()); + if head_block_epoch > shuffling_epoch + 1 { + return Err(Error::InvalidStateForShuffling { + state_epoch: head_block_epoch, + shuffling_epoch, + }); + } + let state_read_timer = metrics::start_timer(&metrics::ATTESTATION_PROCESSING_STATE_READ_TIMES); @@ -3939,59 +3950,46 @@ impl BeaconChain { } })?; + // Compute the `target_slot` to advance the block's state to. + // + // Since there's a one-epoch look-ahead on the attester shuffling, it suffices to + // only advance into the first slot of the epoch prior to `shuffling_epoch`. + // + // 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( + shuffling_epoch + .saturating_sub(1_u64) + .start_slot(T::EthSpec::slots_per_epoch()), + head_block.slot, + ); + // If the head state is useful for this request, use it. Otherwise, read a state from - // disk. + // disk that is advanced as close as possible to `target_slot`. let (mut state, state_root) = if let Some((state, state_root)) = head_state_opt { (state, state_root) } else { - let state_root = head_block.state_root; - let state = self + let (state_root, state) = self .store - .get_inconsistent_state_for_attestation_verification_only( - &state_root, - Some(head_block.slot), - )? + .get_advanced_state(head_block_root, target_slot, head_block.state_root)? .ok_or(Error::MissingBeaconState(head_block.state_root))?; (state, state_root) }; - /* - * IMPORTANT - * - * Since it's possible that - * `Store::get_inconsistent_state_for_attestation_verification_only` was used to obtain - * the state, we cannot rely upon the following fields: - * - * - `state.state_roots` - * - `state.block_roots` - * - * These fields should not be used for the rest of this function. - */ - metrics::stop_timer(state_read_timer); let state_skip_timer = metrics::start_timer(&metrics::ATTESTATION_PROCESSING_STATE_SKIP_TIMES); - // If the state is in an earlier epoch, advance it. If it's from a later epoch, reject - // it. + // 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 { - // Since there's a one-epoch look-ahead on the attester shuffling, it suffices to - // only advance into the slot prior to the `shuffling_epoch`. - let target_slot = shuffling_epoch - .saturating_sub(1_u64) - .start_slot(T::EthSpec::slots_per_epoch()); - - // Advance the state into the required slot, using the "partial" method since the state - // roots are not relevant for the shuffling. + // 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, &self.spec)?; - } else if state.current_epoch() > shuffling_epoch { - return Err(Error::InvalidStateForShuffling { - state_epoch: state.current_epoch(), - shuffling_epoch, - }); } - metrics::stop_timer(state_skip_timer); + let committee_building_timer = metrics::start_timer(&metrics::ATTESTATION_PROCESSING_COMMITTEE_BUILDING_TIMES); diff --git a/beacon_node/beacon_chain/tests/store_tests.rs b/beacon_node/beacon_chain/tests/store_tests.rs index a6a395ecd7..e8b97cae63 100644 --- a/beacon_node/beacon_chain/tests/store_tests.rs +++ b/beacon_node/beacon_chain/tests/store_tests.rs @@ -412,48 +412,6 @@ fn forwards_iter_block_and_state_roots_until() { test_range(Slot::new(0), head_state.slot()); } -#[test] -fn block_replay_with_inaccurate_state_roots() { - let num_blocks_produced = E::slots_per_epoch() * 3 + 31; - let db_path = tempdir().unwrap(); - let store = get_store(&db_path); - let harness = get_harness(store.clone(), LOW_VALIDATOR_COUNT); - let chain = &harness.chain; - - harness.extend_chain( - num_blocks_produced as usize, - BlockStrategy::OnCanonicalHead, - AttestationStrategy::AllValidators, - ); - - // Slot must not be 0 mod 32 or else no blocks will be replayed. - let (mut head_state, head_root) = harness.get_current_state_and_root(); - assert_ne!(head_state.slot() % 32, 0); - - let mut fast_head_state = store - .get_inconsistent_state_for_attestation_verification_only( - &head_root, - Some(head_state.slot()), - ) - .unwrap() - .unwrap(); - assert_eq!(head_state.validators(), fast_head_state.validators()); - - head_state.build_all_committee_caches(&chain.spec).unwrap(); - fast_head_state - .build_all_committee_caches(&chain.spec) - .unwrap(); - - assert_eq!( - head_state - .get_cached_active_validator_indices(RelativeEpoch::Current) - .unwrap(), - fast_head_state - .get_cached_active_validator_indices(RelativeEpoch::Current) - .unwrap() - ); -} - #[test] fn block_replayer_hooks() { let db_path = tempdir().unwrap(); diff --git a/beacon_node/store/src/hot_cold_store.rs b/beacon_node/store/src/hot_cold_store.rs index e535d67395..c3582f87da 100644 --- a/beacon_node/store/src/hot_cold_store.rs +++ b/beacon_node/store/src/hot_cold_store.rs @@ -416,42 +416,6 @@ impl, Cold: ItemStore> HotColdDB .map(|state| (state_root, state))) } - /// Fetch a state from the store, but don't compute all of the values when replaying blocks - /// upon that state (e.g., state roots). Additionally, only states from the hot store are - /// returned. - /// - /// See `Self::get_state` for information about `slot`. - /// - /// ## Warning - /// - /// The returned state **is not a valid beacon state**, it can only be used for obtaining - /// shuffling to process attestations. At least the following components of the state will be - /// broken/invalid: - /// - /// - `state.state_roots` - /// - `state.block_roots` - // FIXME(sproul): delete this whole function - pub fn get_inconsistent_state_for_attestation_verification_only( - &self, - state_root: &Hash256, - slot: Option, - ) -> Result>, Error> { - metrics::inc_counter(&metrics::BEACON_STATE_GET_COUNT); - - let split_slot = self.get_split_slot(); - - if slot.map_or(false, |slot| slot < split_slot) { - Err(HotColdDBError::AttestationStateIsFinalized { - split_slot, - request_slot: slot, - state_root: *state_root, - } - .into()) - } else { - self.get_hot_state(state_root) - } - } - /// Delete a state, ensuring it is removed from the LRU cache, as well as from on-disk. /// /// It is assumed that all states being deleted reside in the hot DB, even if their slot is less