From 8113ce67a5ef7d6adf92b2ce37909289606e9f77 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Sat, 15 Aug 2020 11:12:46 +1000 Subject: [PATCH 1/9] Allow for skipping state roots in block replay --- .../src/attestation_verification.rs | 7 ++- beacon_node/store/src/hot_cold_store.rs | 52 ++++++++++++++++--- 2 files changed, 50 insertions(+), 9 deletions(-) diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index aa24d08b2d6..9c8ec92e596 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/beacon_node/beacon_chain/src/attestation_verification.rs @@ -775,7 +775,12 @@ where metrics::start_timer(&metrics::ATTESTATION_PROCESSING_STATE_READ_TIMES); let mut state = chain - .get_state(&target_block.state_root, Some(target_block.slot))? + .store + .get_inconsistent_state_for_attestation_verification_only( + &target_block.state_root, + Some(target_block.slot), + ) + .map_err(BeaconChainError::from)? .ok_or_else(|| BeaconChainError::MissingBeaconState(target_block.state_root))?; metrics::stop_timer(state_read_timer); diff --git a/beacon_node/store/src/hot_cold_store.rs b/beacon_node/store/src/hot_cold_store.rs index 7bad9ef1567..bf81b67ddf0 100644 --- a/beacon_node/store/src/hot_cold_store.rs +++ b/beacon_node/store/src/hot_cold_store.rs @@ -30,6 +30,11 @@ use types::*; /// 32-byte key for accessing the `split` of the freezer DB. pub const SPLIT_DB_KEY: &str = "FREEZERDBSPLITFREEZERDBSPLITFREE"; +pub enum BlockReplay { + Accurate, + InconsistentStateRoots, +} + /// On-disk database that stores finalized states efficiently. /// /// Stores vector fields like the `block_roots` and `state_roots` separately, and only stores @@ -230,16 +235,30 @@ impl, Cold: ItemStore> HotColdDB // chain. This way we avoid returning a state that doesn't match `state_root`. self.load_cold_state(state_root) } else { - self.load_hot_state(state_root) + self.load_hot_state(state_root, BlockReplay::Accurate) } } else { - match self.load_hot_state(state_root)? { + match self.load_hot_state(state_root, BlockReplay::Accurate)? { Some(state) => Ok(Some(state)), None => self.load_cold_state(state_root), } } } + 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); + + if slot.map_or(false, |slot| slot < self.get_split_slot()) { + Ok(None) + } else { + self.load_hot_state(state_root, BlockReplay::InconsistentStateRoots) + } + } + /// 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 @@ -283,8 +302,11 @@ impl, Cold: ItemStore> HotColdDB }) = self.load_hot_state_summary(state_root)? { // NOTE: minor inefficiency here because we load an unnecessary hot state summary + // + // `BlockReplay` should be irrelevant here since we never replay blocks for an epoch + // boundary state in the hot DB. let state = self - .load_hot_state(&epoch_boundary_state_root)? + .load_hot_state(&epoch_boundary_state_root, BlockReplay::Accurate)? .ok_or_else(|| { HotColdDBError::MissingEpochBoundaryState(epoch_boundary_state_root) })?; @@ -415,7 +437,11 @@ impl, Cold: ItemStore> HotColdDB /// Load a post-finalization state from the hot database. /// /// Will replay blocks from the nearest epoch boundary. - pub fn load_hot_state(&self, state_root: &Hash256) -> Result>, Error> { + pub fn load_hot_state( + &self, + state_root: &Hash256, + block_replay: BlockReplay, + ) -> Result>, Error> { metrics::inc_counter(&metrics::BEACON_STATE_HOT_GET_COUNT); if let Some(HotStateSummary { @@ -436,7 +462,7 @@ impl, Cold: ItemStore> HotColdDB } else { let blocks = self.load_blocks_to_replay(boundary_state.slot, slot, latest_block_root)?; - self.replay_blocks(boundary_state, blocks, slot)? + self.replay_blocks(boundary_state, blocks, slot, block_replay)? }; Ok(Some(state)) @@ -567,7 +593,7 @@ impl, Cold: ItemStore> HotColdDB )?; // 3. Replay the blocks on top of the low restore point. - self.replay_blocks(low_restore_point, blocks, slot) + self.replay_blocks(low_restore_point, blocks, slot, BlockReplay::Accurate) } /// Get a suitable block root for backtracking from `high_restore_point` to the state at `slot`. @@ -626,6 +652,7 @@ impl, Cold: ItemStore> HotColdDB mut state: BeaconState, blocks: Vec>, target_slot: Slot, + block_replay: BlockReplay, ) -> Result, Error> { let state_root_from_prev_block = |i: usize, state: &BeaconState| { if i > 0 { @@ -650,10 +677,16 @@ impl, Cold: ItemStore> HotColdDB per_slot_processing(&mut state, state_root, &self.spec) .map_err(HotColdDBError::BlockReplaySlotError)?; } + + let state_root = match block_replay { + BlockReplay::Accurate => None, + BlockReplay::InconsistentStateRoots => Some(Hash256::zero()), + }; + per_block_processing( &mut state, &block, - None, + state_root, BlockSignatureStrategy::NoVerification, &self.spec, ) @@ -661,7 +694,10 @@ impl, Cold: ItemStore> HotColdDB } while state.slot < target_slot { - let state_root = state_root_from_prev_block(blocks.len(), &state); + let state_root = match block_replay { + BlockReplay::Accurate => state_root_from_prev_block(blocks.len(), &state), + BlockReplay::InconsistentStateRoots => Some(Hash256::zero()), + }; per_slot_processing(&mut state, state_root, &self.spec) .map_err(HotColdDBError::BlockReplaySlotError)?; } From f93a681ce10508d4f49edc7a6298e2ecebd030df Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Sat, 15 Aug 2020 11:34:40 +1000 Subject: [PATCH 2/9] Avoid cloning caches for atomic db ops --- beacon_node/beacon_chain/src/block_verification.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 38acf1b1799..88c111f6ef4 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -672,7 +672,10 @@ impl<'a, T: BeaconChainTypes> FullyVerifiedBlock<'a, T> { let state_root = state.update_tree_hash_cache()?; let op = if state.slot % T::EthSpec::slots_per_epoch() == 0 { - StoreOp::PutState(state_root.into(), Cow::Owned(state.clone())) + StoreOp::PutState( + state_root.into(), + Cow::Owned(state.clone_with(CloneConfig::none())), + ) } else { StoreOp::PutStateSummary( state_root.into(), From 2ab752bf907bf68bf682bf421ad5727da2b78386 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Sat, 15 Aug 2020 11:43:29 +1000 Subject: [PATCH 3/9] Fix mistake in previous commit --- beacon_node/store/src/hot_cold_store.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/beacon_node/store/src/hot_cold_store.rs b/beacon_node/store/src/hot_cold_store.rs index bf81b67ddf0..c04b47aae40 100644 --- a/beacon_node/store/src/hot_cold_store.rs +++ b/beacon_node/store/src/hot_cold_store.rs @@ -673,20 +673,18 @@ impl, Cold: ItemStore> HotColdDB } while state.slot < block.message.slot { - let state_root = state_root_from_prev_block(i, &state); + let state_root = match block_replay { + BlockReplay::Accurate => state_root_from_prev_block(i, &state), + BlockReplay::InconsistentStateRoots => Some(Hash256::zero()), + }; per_slot_processing(&mut state, state_root, &self.spec) .map_err(HotColdDBError::BlockReplaySlotError)?; } - let state_root = match block_replay { - BlockReplay::Accurate => None, - BlockReplay::InconsistentStateRoots => Some(Hash256::zero()), - }; - per_block_processing( &mut state, &block, - state_root, + None, BlockSignatureStrategy::NoVerification, &self.spec, ) From 64393a4596e9726dddf10566580a5beff7584412 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Sat, 15 Aug 2020 12:02:36 +1000 Subject: [PATCH 4/9] Reject known gossip blocks quicker --- beacon_node/beacon_chain/src/block_verification.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 88c111f6ef4..988bfdea3d4 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -390,9 +390,17 @@ impl GossipVerifiedBlock { }); } + let block_root = get_block_root(&block); + // Do not gossip a block from a finalized slot. check_block_against_finalized_slot(&block.message, chain)?; + // Check if the block is already known. We know it is post-finalization, so it is + // sufficient to check the fork choice. + if chain.fork_choice.read().contains_block(&block_root) { + return Err(BlockError::BlockIsAlreadyKnown); + } + // Check that we have not already received a block with a valid signature for this slot. if chain .observed_block_producers @@ -415,7 +423,6 @@ impl GossipVerifiedBlock { )?; let (mut parent, block) = load_parent(block, chain)?; - let block_root = get_block_root(&block); let state = cheap_state_advance_to_obtain_committees( &mut parent.beacon_state, From 765edf04944dbc06750a23b2feee6c457da6ab3b Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Sat, 15 Aug 2020 12:46:42 +1000 Subject: [PATCH 5/9] Update comment --- beacon_node/beacon_chain/src/block_verification.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 988bfdea3d4..bfbc3d2987e 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -397,6 +397,11 @@ impl GossipVerifiedBlock { // Check if the block is already known. We know it is post-finalization, so it is // sufficient to check the fork choice. + // + // In normal operation this isn't necessary, however it is useful immediately after a + // reboot if the `observed_block_producers` cache is empty. In that case, without this + // check, we will load the parent and state from disk only to find out later that we + // already know this block. if chain.fork_choice.read().contains_block(&block_root) { return Err(BlockError::BlockIsAlreadyKnown); } From 52ab57613d3fda7c4a2833b0e39773e6e2a4037a Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Sat, 15 Aug 2020 12:46:53 +1000 Subject: [PATCH 6/9] Fix bug in state-root-less replay --- beacon_node/store/src/hot_cold_store.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/beacon_node/store/src/hot_cold_store.rs b/beacon_node/store/src/hot_cold_store.rs index c04b47aae40..6ea51690278 100644 --- a/beacon_node/store/src/hot_cold_store.rs +++ b/beacon_node/store/src/hot_cold_store.rs @@ -30,6 +30,7 @@ use types::*; /// 32-byte key for accessing the `split` of the freezer DB. pub const SPLIT_DB_KEY: &str = "FREEZERDBSPLITFREEZERDBSPLITFREE"; +#[derive(PartialEq)] pub enum BlockReplay { Accurate, InconsistentStateRoots, @@ -650,10 +651,19 @@ impl, Cold: ItemStore> HotColdDB fn replay_blocks( &self, mut state: BeaconState, - blocks: Vec>, + mut blocks: Vec>, target_slot: Slot, block_replay: BlockReplay, ) -> Result, Error> { + if block_replay == BlockReplay::InconsistentStateRoots { + for i in 0..blocks.len() { + blocks[i].message.state_root = Hash256::zero(); + if i > 0 { + blocks[i].message.parent_root = blocks[i - 1].canonical_root() + } + } + } + let state_root_from_prev_block = |i: usize, state: &BeaconState| { if i > 0 { let prev_block = &blocks[i - 1].message; From f2590e347f501d2370608fac208be8f833f54fc0 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Sat, 15 Aug 2020 12:47:06 +1000 Subject: [PATCH 7/9] Introduced ForcedFixedLenIter --- .../types/src/beacon_state/tree_hash_cache.rs | 40 +++++++++++++++---- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/consensus/types/src/beacon_state/tree_hash_cache.rs b/consensus/types/src/beacon_state/tree_hash_cache.rs index 0c8899c0255..117c6aa136b 100644 --- a/consensus/types/src/beacon_state/tree_hash_cache.rs +++ b/consensus/types/src/beacon_state/tree_hash_cache.rs @@ -7,6 +7,7 @@ use rayon::prelude::*; use ssz_derive::{Decode, Encode}; use ssz_types::VariableList; use std::cmp::Ordering; +use std::iter::ExactSizeIterator; use tree_hash::{mix_in_length, MerkleHasher, TreeHash}; /// The number of fields on a beacon state. @@ -288,17 +289,17 @@ impl ValidatorsListTreeHashCache { fn recalculate_tree_hash_root(&mut self, validators: &[Validator]) -> Result { let mut list_arena = std::mem::take(&mut self.list_arena); - let leaves = self - .values - .leaves(validators)? - .into_iter() - .flatten() - .map(|h| h.to_fixed_bytes()) - .collect::>(); + let leaves = self.values.leaves(validators)?; + let num_leaves = leaves.iter().map(|arena| arena.len()).sum(); + + let leaves_iter = ForcedLengthIterator { + iter: leaves.into_iter().flatten().map(|h| h.to_fixed_bytes()), + len: num_leaves, + }; let list_root = self .list_cache - .recalculate_merkle_root(&mut list_arena, leaves.into_iter())?; + .recalculate_merkle_root(&mut list_arena, leaves_iter)?; self.list_arena = list_arena; @@ -306,6 +307,29 @@ impl ValidatorsListTreeHashCache { } } +/// Provides a wrapper around some `iter` if the number of items in the iterator is known to the +/// programmer but not the compiler. This allows use of `ExactSizeIterator` in some occasions. +/// +/// Care should be taken to ensure `len` is accurate. +struct ForcedLengthIterator { + iter: I, + len: usize, +} + +impl> Iterator for ForcedLengthIterator { + type Item = V; + + fn next(&mut self) -> Option { + self.iter.next() + } +} + +impl> ExactSizeIterator for ForcedLengthIterator { + fn len(&self) -> usize { + self.len + } +} + /// Provides a cache for each of the `Validator` objects in `state.validators` and computes the /// roots of these using Rayon parallelization. #[derive(Debug, PartialEq, Clone, Default, Encode, Decode)] From f28c9cd7b29de21dc3ea4cb7d0171a35d97ce4d6 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 17 Aug 2020 14:00:10 +1000 Subject: [PATCH 8/9] Tidy --- beacon_node/store/src/hot_cold_store.rs | 14 ++++++++++++++ .../types/src/beacon_state/tree_hash_cache.rs | 8 ++++---- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/beacon_node/store/src/hot_cold_store.rs b/beacon_node/store/src/hot_cold_store.rs index 6ea51690278..0c979debdc2 100644 --- a/beacon_node/store/src/hot_cold_store.rs +++ b/beacon_node/store/src/hot_cold_store.rs @@ -30,9 +30,13 @@ use types::*; /// 32-byte key for accessing the `split` of the freezer DB. pub const SPLIT_DB_KEY: &str = "FREEZERDBSPLITFREEZERDBSPLITFREE"; +/// Defines how blocks should be replayed on states. #[derive(PartialEq)] pub enum BlockReplay { + /// Perform all transitions faithfully to the specification. Accurate, + /// Don't compute state roots, eventually computing an invalid beacon state that can only be + /// used for obtaining shuffling. InconsistentStateRoots, } @@ -246,6 +250,16 @@ impl, Cold: ItemStore> HotColdDB } } + /// 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 state 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. pub fn get_inconsistent_state_for_attestation_verification_only( &self, state_root: &Hash256, diff --git a/consensus/types/src/beacon_state/tree_hash_cache.rs b/consensus/types/src/beacon_state/tree_hash_cache.rs index 117c6aa136b..6abc795a137 100644 --- a/consensus/types/src/beacon_state/tree_hash_cache.rs +++ b/consensus/types/src/beacon_state/tree_hash_cache.rs @@ -292,7 +292,7 @@ impl ValidatorsListTreeHashCache { let leaves = self.values.leaves(validators)?; let num_leaves = leaves.iter().map(|arena| arena.len()).sum(); - let leaves_iter = ForcedLengthIterator { + let leaves_iter = ForcedExactSizeIterator { iter: leaves.into_iter().flatten().map(|h| h.to_fixed_bytes()), len: num_leaves, }; @@ -311,12 +311,12 @@ impl ValidatorsListTreeHashCache { /// programmer but not the compiler. This allows use of `ExactSizeIterator` in some occasions. /// /// Care should be taken to ensure `len` is accurate. -struct ForcedLengthIterator { +struct ForcedExactSizeIterator { iter: I, len: usize, } -impl> Iterator for ForcedLengthIterator { +impl> Iterator for ForcedExactSizeIterator { type Item = V; fn next(&mut self) -> Option { @@ -324,7 +324,7 @@ impl> Iterator for ForcedLengthIterator { } } -impl> ExactSizeIterator for ForcedLengthIterator { +impl> ExactSizeIterator for ForcedExactSizeIterator { fn len(&self) -> usize { self.len } From 21c4546e3aa9f563eacc8051e485c803e90bbb71 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 17 Aug 2020 17:05:36 +1000 Subject: [PATCH 9/9] Address Michael's comments --- beacon_node/beacon_chain/src/block_verification.rs | 2 +- beacon_node/store/src/hot_cold_store.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index bfbc3d2987e..92460dba125 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -686,7 +686,7 @@ impl<'a, T: BeaconChainTypes> FullyVerifiedBlock<'a, T> { let op = if state.slot % T::EthSpec::slots_per_epoch() == 0 { StoreOp::PutState( state_root.into(), - Cow::Owned(state.clone_with(CloneConfig::none())), + Cow::Owned(state.clone_with(CloneConfig::committee_caches_only())), ) } else { StoreOp::PutStateSummary( diff --git a/beacon_node/store/src/hot_cold_store.rs b/beacon_node/store/src/hot_cold_store.rs index 0c979debdc2..692e747d79f 100644 --- a/beacon_node/store/src/hot_cold_store.rs +++ b/beacon_node/store/src/hot_cold_store.rs @@ -251,7 +251,7 @@ impl, Cold: ItemStore> HotColdDB } /// 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 state from the hot store are + /// upon that state (e.g., state roots). Additionally, only states from the hot store are /// returned. /// /// See `Self::get_state` for information about `slot`.