From fac72a2b28100dd950438a8df933bad6efe6c6f5 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 21 Sep 2020 12:40:52 +1000 Subject: [PATCH 1/5] Add more sophisticated parent check --- .../beacon_chain/src/block_verification.rs | 23 ++++++------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index c6d8ab02b59..bf682500bd3 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -636,23 +636,14 @@ impl<'a, T: BeaconChainTypes> FullyVerifiedBlock<'a, T> { parent: BeaconSnapshot, chain: &BeaconChain, ) -> Result> { - // Reject any block if its parent is not known to fork choice. - // - // A block that is not in fork choice is either: + // Do not process a block that doesn't descend from the finalized root. // - // - Not yet imported: we should reject this block because we should only import a child - // after its parent has been fully imported. - // - Pre-finalized: if the parent block is _prior_ to finalization, we should ignore it - // because it will revert finalization. Note that the finalized block is stored in fork - // choice, so we will not reject any child of the finalized block (this is relevant during - // genesis). - if !chain - .fork_choice - .read() - .contains_block(&block.parent_root()) - { - return Err(BlockError::ParentUnknown(Box::new(block))); - } + // We check this *before* we load the parent so that we can return a more detailed error. + let block = check_block_is_finalized_descendant::( + block, + &chain.fork_choice.read(), + &chain.store, + )?; // Reject any block that exceeds our limit on skipped slots. check_block_skip_slots(chain, &parent.beacon_block.message, &block.message)?; From 40245be68ef1228b5f1cced201a1ab86484ab96e Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 21 Sep 2020 12:41:26 +1000 Subject: [PATCH 2/5] First attempt at consistency fix --- beacon_node/beacon_chain/src/beacon_chain.rs | 42 ++--- beacon_node/beacon_chain/src/builder.rs | 185 ++++++++++++------- beacon_node/client/src/builder.rs | 4 +- consensus/fork_choice/src/fork_choice.rs | 8 +- 4 files changed, 145 insertions(+), 94 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 1caaec5fea4..a52c23f6f0a 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -242,39 +242,39 @@ impl BeaconChain { /// We want to ensure that the head never out dates the fork choice to avoid having references /// to blocks that do not exist in fork choice. pub fn persist_head_and_fork_choice(&self) -> Result<(), Error> { - let canonical_head_block_root = self + let canonical_head = self .canonical_head .try_read_for(HEAD_LOCK_TIMEOUT) - .ok_or_else(|| Error::CanonicalHeadLockTimeout)? - .beacon_block_root; + .ok_or_else(|| Error::CanonicalHeadLockTimeout)?; + let fork_choice = self.fork_choice.read(); let persisted_head = PersistedBeaconChain { - canonical_head_block_root, + canonical_head_block_root: canonical_head.beacon_block_root, genesis_block_root: self.genesis_block_root, ssz_head_tracker: self.head_tracker.to_ssz_container(), }; - let fork_choice_timer = metrics::start_timer(&metrics::PERSIST_FORK_CHOICE); - - let fork_choice = self.fork_choice.read(); - - self.store.put_item( - &Hash256::from_slice(&FORK_CHOICE_DB_KEY), - &PersistedForkChoice { - fork_choice: fork_choice.to_persisted(), - fork_choice_store: fork_choice.fc_store().to_persisted(), - }, - )?; + let persisted_fork_choice = PersistedForkChoice { + fork_choice: fork_choice.to_persisted(), + fork_choice_store: fork_choice.fc_store().to_persisted(), + }; + drop(canonical_head); drop(fork_choice); - metrics::stop_timer(fork_choice_timer); - let head_timer = metrics::start_timer(&metrics::PERSIST_HEAD); - - self.store - .put_item(&Hash256::from_slice(&BEACON_CHAIN_DB_KEY), &persisted_head)?; + { + let _timer = metrics::start_timer(&metrics::PERSIST_FORK_CHOICE); + self.store.put_item( + &Hash256::from_slice(&FORK_CHOICE_DB_KEY), + &persisted_fork_choice, + )?; + } - metrics::stop_timer(head_timer); + { + let _timer = metrics::start_timer(&metrics::PERSIST_HEAD); + self.store + .put_item(&Hash256::from_slice(&BEACON_CHAIN_DB_KEY), &persisted_head)?; + } Ok(()) } diff --git a/beacon_node/beacon_chain/src/builder.rs b/beacon_node/beacon_chain/src/builder.rs index 6ff94cfaad9..ed02a80ca90 100644 --- a/beacon_node/beacon_chain/src/builder.rs +++ b/beacon_node/beacon_chain/src/builder.rs @@ -26,6 +26,7 @@ use std::marker::PhantomData; use std::path::PathBuf; use std::sync::Arc; use std::time::Duration; +use std::time::Instant; use store::{HotColdDB, ItemStore}; use types::{ BeaconBlock, BeaconState, ChainSpec, EthSpec, Graffiti, Hash256, Signature, SignedBeaconBlock, @@ -97,11 +98,11 @@ pub struct BeaconChainBuilder { #[allow(clippy::type_complexity)] store: Option>>, store_migrator: Option, - canonical_head: Option>, - /// The finalized checkpoint to anchor the chain. May be genesis or a higher - /// checkpoint. - pub finalized_snapshot: Option>, + pub canonical_head: Option>, genesis_block_root: Option, + fork_choice: Option< + ForkChoice, T::EthSpec>, + >, op_pool: Option>, eth1_chain: Option>, event_handler: Option, @@ -147,8 +148,8 @@ where store: None, store_migrator: None, canonical_head: None, - finalized_snapshot: None, genesis_block_root: None, + fork_choice: None, op_pool: None, eth1_chain: None, event_handler: None, @@ -270,6 +271,10 @@ where .clone() .ok_or_else(|| "resume_from_db requires a store.".to_string())?; + /* + * Persisted beacon chain. + */ + let chain = store .get_item::(&Hash256::from_slice(&BEACON_CHAIN_DB_KEY)) .map_err(|e| format!("DB error when reading persisted beacon chain: {:?}", e))? @@ -279,11 +284,20 @@ where })?; self.genesis_block_root = Some(chain.genesis_block_root); + + /* + * Head tracker + */ + self.head_tracker = Some( HeadTracker::from_ssz_container(&chain.ssz_head_tracker) .map_err(|e| format!("Failed to decode head tracker for database: {:?}", e))?, ); + /* + * Canonical head + */ + let head_block_root = chain.canonical_head_block_root; let head_block = store .get_item::>(&head_block_root) @@ -295,6 +309,17 @@ where .map_err(|e| format!("DB error when reading head state: {:?}", e))? .ok_or_else(|| "Head state not found in store".to_string())?; + self.canonical_head = Some(BeaconSnapshot { + beacon_block_root: head_block_root, + beacon_block: head_block, + beacon_state_root: head_state_root, + beacon_state: head_state, + }); + + /* + * Operations pool + */ + self.op_pool = Some( store .get_item::>(&Hash256::from_slice(&OP_POOL_DB_KEY)) @@ -303,36 +328,35 @@ where .unwrap_or_else(OperationPool::new), ); - let finalized_block_root = head_state.finalized_checkpoint.root; - let finalized_block = store - .get_item::>(&finalized_block_root) - .map_err(|e| format!("DB error when reading finalized block: {:?}", e))? - .ok_or_else(|| "Finalized block not found in store".to_string())?; - let finalized_state_root = finalized_block.state_root(); - let finalized_state = store - .get_state(&finalized_state_root, Some(finalized_block.slot())) - .map_err(|e| format!("DB error when reading finalized state: {:?}", e))? - .ok_or_else(|| "Finalized state not found in store".to_string())?; - - self.finalized_snapshot = Some(BeaconSnapshot { - beacon_block_root: finalized_block_root, - beacon_block: finalized_block, - beacon_state_root: finalized_state_root, - beacon_state: finalized_state, - }); - - self.canonical_head = Some(BeaconSnapshot { - beacon_block_root: head_block_root, - beacon_block: head_block, - beacon_state_root: head_state_root, - beacon_state: head_state, - }); + /* + * Validator public key cache + */ let pubkey_cache = ValidatorPubkeyCache::load_from_file(pubkey_cache_path) .map_err(|e| format!("Unable to open persisted pubkey cache: {:?}", e))?; self.validator_pubkey_cache = Some(pubkey_cache); + /* + * Fork choice + */ + + let persisted_fork_choice = store + .get_item::(&Hash256::from_slice(&FORK_CHOICE_DB_KEY)) + .map_err(|e| format!("DB error when reading persisted fork choice: {:?}", e))? + .ok_or_else(|| "Fork choice not found in store".to_string())?; + + let fc_store = BeaconForkChoiceStore::from_persisted( + persisted_fork_choice.fork_choice_store, + store.clone(), + ) + .map_err(|e| format!("Unable to load ForkChoiceStore: {:?}", e))?; + + self.fork_choice = Some( + ForkChoice::from_persisted(persisted_fork_choice.fork_choice, fc_store) + .map_err(|e| format!("Unable to parse persisted fork choice from disk: {:?}", e))?, + ); + Ok(self) } @@ -374,12 +398,32 @@ where ) })?; - self.finalized_snapshot = Some(BeaconSnapshot { + let head = BeaconSnapshot { beacon_block_root, beacon_block, beacon_state_root, beacon_state, - }); + }; + + let fc_store = BeaconForkChoiceStore::get_forkchoice_store(store.clone(), &head); + + self.fork_choice = Some( + // TODO: can I really use "from_genesis" here? maybe only allow genesis state. + ForkChoice::from_genesis(fc_store, &head.beacon_block.message) + .map_err(|e| format!("Unable to build initialize ForkChoice: {:?}", e))?, + ); + + let pubkey_cache_path = self + .pubkey_cache_path + .as_ref() + .ok_or_else(|| "Cannot build without a pubkey cache path".to_string())?; + + self.validator_pubkey_cache = Some( + ValidatorPubkeyCache::new(&head.beacon_state, pubkey_cache_path) + .map_err(|e| format!("Unable to init validator pubkey cache: {:?}", e))?, + ); + + self.canonical_head = Some(head); Ok(self.empty_op_pool()) } @@ -457,53 +501,54 @@ where .store .clone() .ok_or_else(|| "Cannot build without a store.".to_string())?; - - // If this beacon chain is being loaded from disk, use the stored head. Otherwise, just use - // the finalized checkpoint (which is probably genesis). - let mut canonical_head = if let Some(head) = self.canonical_head { - head - } else { - self.finalized_snapshot - .ok_or_else(|| "Cannot build without a state".to_string())? - }; + let validator_pubkey_cache = self + .validator_pubkey_cache + .ok_or_else(|| "Cannot build without a validator pubkey cache.".to_string())?; + let fork_choice = self + .fork_choice + .ok_or_else(|| "Cannot build without a fork choice.".to_string())?; + let mut canonical_head = self + .canonical_head + .ok_or_else(|| "Cannot build without a canonical head.".to_string())?; + + info!(log, "Building head state caches"); + let start = Instant::now(); canonical_head .beacon_state .build_all_caches(&self.spec) .map_err(|e| format!("Failed to build state caches: {:?}", e))?; + info!( + log, + "Built head state caches"; + "elapsed" => format!("{:?}", Instant::now().duration_since(start)) + ); + if canonical_head.beacon_block.state_root() != canonical_head.beacon_state_root { return Err("beacon_block.state_root != beacon_state".to_string()); } - let pubkey_cache_path = self - .pubkey_cache_path - .ok_or_else(|| "Cannot build without a pubkey cache path".to_string())?; - - let validator_pubkey_cache = self.validator_pubkey_cache.map(Ok).unwrap_or_else(|| { - ValidatorPubkeyCache::new(&canonical_head.beacon_state, pubkey_cache_path) - .map_err(|e| format!("Unable to init validator pubkey cache: {:?}", e)) - })?; - - let persisted_fork_choice = store - .get_item::(&Hash256::from_slice(&FORK_CHOICE_DB_KEY)) - .map_err(|e| format!("DB error when reading persisted fork choice: {:?}", e))?; - - let fork_choice = if let Some(persisted) = persisted_fork_choice { - let fc_store = - BeaconForkChoiceStore::from_persisted(persisted.fork_choice_store, store.clone()) - .map_err(|e| format!("Unable to load ForkChoiceStore: {:?}", e))?; - - ForkChoice::from_persisted(persisted.fork_choice, fc_store) - .map_err(|e| format!("Unable to parse persisted fork choice from disk: {:?}", e))? - } else { - let genesis = &canonical_head; - - let fc_store = BeaconForkChoiceStore::get_forkchoice_store(store.clone(), genesis); - - ForkChoice::from_genesis(fc_store, &genesis.beacon_block.message) - .map_err(|e| format!("Unable to build initialize ForkChoice: {:?}", e))? - }; + // Perform a check to ensure that the finalization points of the head and fork choice are + // consistent. + // + // This is a sanity check to detect database corruption. + let fc_finalized = fork_choice.finalized_checkpoint(); + let head_finalized = canonical_head.beacon_state.finalized_checkpoint; + if fc_finalized != head_finalized { + if head_finalized.root == Hash256::zero() + && head_finalized.epoch == fc_finalized.epoch + && fc_finalized.root == canonical_head.beacon_block_root + { + // This is a legal edge-case encountered during genesis. + } else { + return Err(format!( + "Database corrupt: fork choice is finalized at {:?} whilst head is finalized at \ + {:?}", + fc_finalized, head_finalized + )); + } + } let beacon_chain = BeaconChain { spec: self.spec, @@ -634,9 +679,9 @@ where /// Requires the state to be initialized. pub fn testing_slot_clock(self, slot_duration: Duration) -> Result { let genesis_time = self - .finalized_snapshot + .canonical_head .as_ref() - .ok_or_else(|| "testing_slot_clock requires an initialized state")? + .ok_or_else(|| "testing_slot_clock requires a canonical head")? .beacon_state .genesis_time; diff --git a/beacon_node/client/src/builder.rs b/beacon_node/client/src/builder.rs index c77ddecef2d..d856a71ad0c 100644 --- a/beacon_node/client/src/builder.rs +++ b/beacon_node/client/src/builder.rs @@ -713,9 +713,9 @@ where .ok_or_else(|| "system_time_slot_clock requires a beacon_chain_builder")?; let genesis_time = beacon_chain_builder - .finalized_snapshot + .canonical_head .as_ref() - .ok_or_else(|| "system_time_slot_clock requires an initialized beacon state")? + .ok_or_else(|| "system_time_slot_clock requires a canonical head")? .beacon_state .genesis_time; diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 804771330fb..99f998e5584 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -3,7 +3,8 @@ use std::marker::PhantomData; use proto_array::{Block as ProtoBlock, ProtoArrayForkChoice}; use ssz_derive::{Decode, Encode}; use types::{ - BeaconBlock, BeaconState, BeaconStateError, Epoch, EthSpec, Hash256, IndexedAttestation, Slot, + BeaconBlock, BeaconState, BeaconStateError, Checkpoint, Epoch, EthSpec, Hash256, + IndexedAttestation, Slot, }; use crate::ForkChoiceStore; @@ -758,6 +759,11 @@ where .is_descendant(self.fc_store.finalized_checkpoint().root, block_root) } + /// Return the current finalized checkpoint. + pub fn finalized_checkpoint(&self) -> Checkpoint { + *self.fc_store.finalized_checkpoint() + } + /// Returns the latest message for a given validator, if any. /// /// Returns `(block_root, block_slot)`. From 7da460826b7ef8b56912f7b7bf0387e8e66d924f Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 21 Sep 2020 14:53:46 +1000 Subject: [PATCH 3/5] Unify fork choice with chain --- beacon_node/beacon_chain/src/beacon_chain.rs | 29 ++++------- beacon_node/beacon_chain/src/builder.rs | 48 +++++++++---------- beacon_node/beacon_chain/src/metrics.rs | 2 - .../src/persisted_beacon_chain.rs | 27 ++++++++++- common/slot_clock/src/lib.rs | 3 ++ common/slot_clock/src/manual_slot_clock.rs | 8 ++-- .../slot_clock/src/system_time_slot_clock.rs | 6 ++- 7 files changed, 68 insertions(+), 55 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index a52c23f6f0a..1feca5dac1d 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -19,8 +19,7 @@ use crate::observed_attestations::{Error as AttestationObservationError, Observe use crate::observed_attesters::{ObservedAggregators, ObservedAttesters}; use crate::observed_block_producers::ObservedBlockProducers; use crate::observed_operations::{ObservationOutcome, ObservedOperations}; -use crate::persisted_beacon_chain::PersistedBeaconChain; -use crate::persisted_fork_choice::PersistedForkChoice; +use crate::persisted_beacon_chain::{PersistedBeaconChain, PersistedForkChoice}; use crate::shuffling_cache::ShufflingCache; use crate::snapshot_cache::SnapshotCache; use crate::timeout_rw_lock::TimeoutRwLock; @@ -242,33 +241,21 @@ impl BeaconChain { /// We want to ensure that the head never out dates the fork choice to avoid having references /// to blocks that do not exist in fork choice. pub fn persist_head_and_fork_choice(&self) -> Result<(), Error> { - let canonical_head = self - .canonical_head - .try_read_for(HEAD_LOCK_TIMEOUT) - .ok_or_else(|| Error::CanonicalHeadLockTimeout)?; let fork_choice = self.fork_choice.read(); - let persisted_head = PersistedBeaconChain { - canonical_head_block_root: canonical_head.beacon_block_root, - genesis_block_root: self.genesis_block_root, - ssz_head_tracker: self.head_tracker.to_ssz_container(), - }; - let persisted_fork_choice = PersistedForkChoice { fork_choice: fork_choice.to_persisted(), fork_choice_store: fork_choice.fc_store().to_persisted(), }; - drop(canonical_head); - drop(fork_choice); + let persisted_head = PersistedBeaconChain { + genesis_time: self.slot_clock.genesis_duration().as_secs(), + genesis_block_root: self.genesis_block_root, + ssz_head_tracker: self.head_tracker.to_ssz_container(), + persisted_fork_choice, + }; - { - let _timer = metrics::start_timer(&metrics::PERSIST_FORK_CHOICE); - self.store.put_item( - &Hash256::from_slice(&FORK_CHOICE_DB_KEY), - &persisted_fork_choice, - )?; - } + drop(fork_choice); { let _timer = metrics::start_timer(&metrics::PERSIST_HEAD); diff --git a/beacon_node/beacon_chain/src/builder.rs b/beacon_node/beacon_chain/src/builder.rs index ed02a80ca90..27c69f9bb49 100644 --- a/beacon_node/beacon_chain/src/builder.rs +++ b/beacon_node/beacon_chain/src/builder.rs @@ -1,12 +1,9 @@ -use crate::beacon_chain::{ - BEACON_CHAIN_DB_KEY, ETH1_CACHE_DB_KEY, FORK_CHOICE_DB_KEY, OP_POOL_DB_KEY, -}; +use crate::beacon_chain::{BEACON_CHAIN_DB_KEY, ETH1_CACHE_DB_KEY, OP_POOL_DB_KEY}; use crate::eth1_chain::{CachingEth1Backend, SszEth1}; use crate::events::NullEventHandler; use crate::head_tracker::HeadTracker; use crate::migrate::Migrate; use crate::persisted_beacon_chain::PersistedBeaconChain; -use crate::persisted_fork_choice::PersistedForkChoice; use crate::shuffling_cache::ShufflingCache; use crate::snapshot_cache::{SnapshotCache, DEFAULT_SNAPSHOT_CACHE_SIZE}; use crate::timeout_rw_lock::TimeoutRwLock; @@ -294,11 +291,32 @@ where .map_err(|e| format!("Failed to decode head tracker for database: {:?}", e))?, ); + /* + * Fork choice + */ + + let fc_store = BeaconForkChoiceStore::from_persisted( + chain.persisted_fork_choice.fork_choice_store, + store.clone(), + ) + .map_err(|e| format!("Unable to load ForkChoiceStore: {:?}", e))?; + + let mut fork_choice = + ForkChoice::from_persisted(chain.persisted_fork_choice.fork_choice, fc_store) + .map_err(|e| format!("Unable to parse persisted fork choice from disk: {:?}", e))?; + + let current_slot = chain.genesis_time / (self.spec.milliseconds_per_slot / 1_000); + + let head_block_root = fork_choice + .get_head(current_slot.into()) + .map_err(|e| format!("Unable to find head block: {:?}", e))?; + + self.fork_choice = Some(fork_choice); + /* * Canonical head */ - let head_block_root = chain.canonical_head_block_root; let head_block = store .get_item::>(&head_block_root) .map_err(|e| format!("DB error when reading head block: {:?}", e))? @@ -337,26 +355,6 @@ where self.validator_pubkey_cache = Some(pubkey_cache); - /* - * Fork choice - */ - - let persisted_fork_choice = store - .get_item::(&Hash256::from_slice(&FORK_CHOICE_DB_KEY)) - .map_err(|e| format!("DB error when reading persisted fork choice: {:?}", e))? - .ok_or_else(|| "Fork choice not found in store".to_string())?; - - let fc_store = BeaconForkChoiceStore::from_persisted( - persisted_fork_choice.fork_choice_store, - store.clone(), - ) - .map_err(|e| format!("Unable to load ForkChoiceStore: {:?}", e))?; - - self.fork_choice = Some( - ForkChoice::from_persisted(persisted_fork_choice.fork_choice, fc_store) - .map_err(|e| format!("Unable to parse persisted fork choice from disk: {:?}", e))?, - ); - Ok(self) } diff --git a/beacon_node/beacon_chain/src/metrics.rs b/beacon_node/beacon_chain/src/metrics.rs index be2a5626f5d..615d2c6c081 100644 --- a/beacon_node/beacon_chain/src/metrics.rs +++ b/beacon_node/beacon_chain/src/metrics.rs @@ -244,8 +244,6 @@ lazy_static! { try_create_histogram("beacon_persist_op_pool", "Time taken to persist the operations pool"); pub static ref PERSIST_ETH1_CACHE: Result = try_create_histogram("beacon_persist_eth1_cache", "Time taken to persist the eth1 caches"); - pub static ref PERSIST_FORK_CHOICE: Result = - try_create_histogram("beacon_persist_fork_choice", "Time taken to persist the fork choice struct"); /* * Eth1 diff --git a/beacon_node/beacon_chain/src/persisted_beacon_chain.rs b/beacon_node/beacon_chain/src/persisted_beacon_chain.rs index 2ca29e9ede4..caf7e0d4fc8 100644 --- a/beacon_node/beacon_chain/src/persisted_beacon_chain.rs +++ b/beacon_node/beacon_chain/src/persisted_beacon_chain.rs @@ -1,14 +1,17 @@ +use crate::beacon_fork_choice_store::PersistedForkChoiceStore as ForkChoiceStore; use crate::head_tracker::SszHeadTracker; +use fork_choice::PersistedForkChoice as ForkChoice; use ssz::{Decode, Encode}; use ssz_derive::{Decode, Encode}; use store::{DBColumn, Error as StoreError, StoreItem}; use types::Hash256; -#[derive(Clone, Encode, Decode)] +#[derive(Encode, Decode)] pub struct PersistedBeaconChain { - pub canonical_head_block_root: Hash256, + pub genesis_time: u64, pub genesis_block_root: Hash256, pub ssz_head_tracker: SszHeadTracker, + pub persisted_fork_choice: PersistedForkChoice, } impl StoreItem for PersistedBeaconChain { @@ -24,3 +27,23 @@ impl StoreItem for PersistedBeaconChain { Self::from_ssz_bytes(bytes).map_err(Into::into) } } + +#[derive(Encode, Decode)] +pub struct PersistedForkChoice { + pub fork_choice: ForkChoice, + pub fork_choice_store: ForkChoiceStore, +} + +impl StoreItem for PersistedForkChoice { + fn db_column() -> DBColumn { + DBColumn::ForkChoice + } + + fn as_store_bytes(&self) -> Vec { + self.as_ssz_bytes() + } + + fn from_store_bytes(bytes: &[u8]) -> std::result::Result { + Self::from_ssz_bytes(bytes).map_err(Into::into) + } +} diff --git a/common/slot_clock/src/lib.rs b/common/slot_clock/src/lib.rs index 41c847498a6..2c22f782e8c 100644 --- a/common/slot_clock/src/lib.rs +++ b/common/slot_clock/src/lib.rs @@ -52,6 +52,9 @@ pub trait SlotClock: Send + Sync + Sized { /// Returns the first slot to be returned at the genesis time. fn genesis_slot(&self) -> Slot; + /// Returns the genesis time, as a duration since UNIX epoch. + fn genesis_duration(&self) -> Duration; + /// Returns the slot if the internal clock were advanced by `duration`. fn now_with_future_tolerance(&self, tolerance: Duration) -> Option { self.slot_of(self.now_duration()?.checked_add(tolerance)?) diff --git a/common/slot_clock/src/manual_slot_clock.rs b/common/slot_clock/src/manual_slot_clock.rs index 04235b6ca52..2a807832d75 100644 --- a/common/slot_clock/src/manual_slot_clock.rs +++ b/common/slot_clock/src/manual_slot_clock.rs @@ -41,10 +41,6 @@ impl ManualSlotClock { self.set_slot(self.now().unwrap().as_u64() + 1) } - pub fn genesis_duration(&self) -> &Duration { - &self.genesis_duration - } - /// Returns the duration between UNIX epoch and the start of `slot`. pub fn start_of(&self, slot: Slot) -> Option { let slot = slot @@ -143,6 +139,10 @@ impl SlotClock for ManualSlotClock { self.slot_duration } + fn genesis_duration(&self) -> Duration { + self.genesis_duration + } + fn duration_to_slot(&self, slot: Slot) -> Option { self.duration_to_slot(slot, *self.current_time.read()) } diff --git a/common/slot_clock/src/system_time_slot_clock.rs b/common/slot_clock/src/system_time_slot_clock.rs index 2ed917a91a0..f18ff227b4b 100644 --- a/common/slot_clock/src/system_time_slot_clock.rs +++ b/common/slot_clock/src/system_time_slot_clock.rs @@ -24,7 +24,7 @@ impl SlotClock for SystemTimeSlotClock { fn is_prior_to_genesis(&self) -> Option { let now = SystemTime::now().duration_since(UNIX_EPOCH).ok()?; - Some(now < *self.clock.genesis_duration()) + Some(now < self.clock.genesis_duration()) } fn now_duration(&self) -> Option { @@ -49,6 +49,10 @@ impl SlotClock for SystemTimeSlotClock { self.clock.slot_duration() } + fn genesis_duration(&self) -> Duration { + self.clock.genesis_duration() + } + fn duration_to_slot(&self, slot: Slot) -> Option { let now = SystemTime::now().duration_since(UNIX_EPOCH).ok()?; self.clock.duration_to_slot(slot, now) From 09e8a2e1857d010fe27f91db910ed631f8912495 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 21 Sep 2020 15:20:40 +1000 Subject: [PATCH 4/5] Tidy, fix slot calc --- beacon_node/beacon_chain/src/beacon_chain.rs | 1 - beacon_node/beacon_chain/src/builder.rs | 6 +++--- beacon_node/beacon_chain/src/test_utils.rs | 4 +--- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 1feca5dac1d..72325a500a6 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -69,7 +69,6 @@ pub const VALIDATOR_PUBKEY_CACHE_LOCK_TIMEOUT: Duration = Duration::from_secs(1) pub const BEACON_CHAIN_DB_KEY: [u8; 32] = [0; 32]; pub const OP_POOL_DB_KEY: [u8; 32] = [0; 32]; pub const ETH1_CACHE_DB_KEY: [u8; 32] = [0; 32]; -pub const FORK_CHOICE_DB_KEY: [u8; 32] = [0; 32]; /// The result of a chain segment processing. pub enum ChainSegmentResult { diff --git a/beacon_node/beacon_chain/src/builder.rs b/beacon_node/beacon_chain/src/builder.rs index 27c69f9bb49..d2dd1cd0064 100644 --- a/beacon_node/beacon_chain/src/builder.rs +++ b/beacon_node/beacon_chain/src/builder.rs @@ -305,10 +305,11 @@ where ForkChoice::from_persisted(chain.persisted_fork_choice.fork_choice, fc_store) .map_err(|e| format!("Unable to parse persisted fork choice from disk: {:?}", e))?; - let current_slot = chain.genesis_time / (self.spec.milliseconds_per_slot / 1_000); + let current_slot = Slot::new(chain.genesis_time * 1_000 / self.spec.milliseconds_per_slot) + + self.spec.genesis_slot; let head_block_root = fork_choice - .get_head(current_slot.into()) + .get_head(current_slot) .map_err(|e| format!("Unable to find head block: {:?}", e))?; self.fork_choice = Some(fork_choice); @@ -406,7 +407,6 @@ where let fc_store = BeaconForkChoiceStore::get_forkchoice_store(store.clone(), &head); self.fork_choice = Some( - // TODO: can I really use "from_genesis" here? maybe only allow genesis state. ForkChoice::from_genesis(fc_store, &head.beacon_block.message) .map_err(|e| format!("Unable to build initialize ForkChoice: {:?}", e))?, ); diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 8690c2e8d2d..e487f599c69 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -1,6 +1,4 @@ -pub use crate::beacon_chain::{ - BEACON_CHAIN_DB_KEY, ETH1_CACHE_DB_KEY, FORK_CHOICE_DB_KEY, OP_POOL_DB_KEY, -}; +pub use crate::beacon_chain::{BEACON_CHAIN_DB_KEY, ETH1_CACHE_DB_KEY, OP_POOL_DB_KEY}; use crate::migrate::{BlockingMigrator, Migrate, NullMigrator}; pub use crate::persisted_beacon_chain::PersistedBeaconChain; use crate::slog::Drain; From afa708105de770ec8d61409e0912c9914398c9d4 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 21 Sep 2020 15:23:41 +1000 Subject: [PATCH 5/5] Fix clippy lints --- beacon_node/beacon_chain/src/builder.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/beacon_node/beacon_chain/src/builder.rs b/beacon_node/beacon_chain/src/builder.rs index d2dd1cd0064..bf05aaee292 100644 --- a/beacon_node/beacon_chain/src/builder.rs +++ b/beacon_node/beacon_chain/src/builder.rs @@ -97,6 +97,7 @@ pub struct BeaconChainBuilder { store_migrator: Option, pub canonical_head: Option>, genesis_block_root: Option, + #[allow(clippy::type_complexity)] fork_choice: Option< ForkChoice, T::EthSpec>, >, @@ -404,7 +405,7 @@ where beacon_state, }; - let fc_store = BeaconForkChoiceStore::get_forkchoice_store(store.clone(), &head); + let fc_store = BeaconForkChoiceStore::get_forkchoice_store(store, &head); self.fork_choice = Some( ForkChoice::from_genesis(fc_store, &head.beacon_block.message)