Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Merged by Bors] - Apply hotfix for inconsistent head #1639

Closed
wants to merge 6 commits into from
Closed

[Merged by Bors] - Apply hotfix for inconsistent head #1639

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
810b1c5
Apply hotfix for inconsistent head
paulhauner Sep 21, 2020
4be1f9f
Add comment
paulhauner Sep 21, 2020
1ac1c18
Fix clippy lint
paulhauner Sep 21, 2020
2b1357d
Avoid recreating slot clock
paulhauner Sep 21, 2020
3da70ee
Dont compare fork choice directly
paulhauner Sep 21, 2020
5bb065b
Fix issue with pre-genesis start
paulhauner Sep 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
155 changes: 91 additions & 64 deletions beacon_node/beacon_chain/src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use fork_choice::ForkChoice;
use operation_pool::{OperationPool, PersistedOperationPool};
use parking_lot::RwLock;
use slog::{info, Logger};
use slot_clock::{SlotClock, TestingSlotClock};
use slot_clock::{SlotClock, SystemTimeSlotClock, TestingSlotClock};
use std::marker::PhantomData;
use std::path::PathBuf;
use std::sync::Arc;
Expand Down Expand Up @@ -97,11 +97,12 @@ pub struct BeaconChainBuilder<T: BeaconChainTypes> {
#[allow(clippy::type_complexity)]
store: Option<Arc<HotColdDB<T::EthSpec, T::HotStore, T::ColdStore>>>,
store_migrator: Option<T::StoreMigrator>,
canonical_head: Option<BeaconSnapshot<T::EthSpec>>,
/// The finalized checkpoint to anchor the chain. May be genesis or a higher
/// checkpoint.
pub finalized_snapshot: Option<BeaconSnapshot<T::EthSpec>>,
pub canonical_head: Option<BeaconSnapshot<T::EthSpec>>,
genesis_block_root: Option<Hash256>,
#[allow(clippy::type_complexity)]
fork_choice: Option<
ForkChoice<BeaconForkChoiceStore<T::EthSpec, T::HotStore, T::ColdStore>, T::EthSpec>,
>,
op_pool: Option<OperationPool<T::EthSpec>>,
eth1_chain: Option<Eth1Chain<T::Eth1Chain, T::EthSpec>>,
event_handler: Option<T::EventHandler>,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -278,13 +279,43 @@ where
.to_string()
})?;

self.genesis_block_root = Some(chain.genesis_block_root);
self.head_tracker = Some(
HeadTracker::from_ssz_container(&chain.ssz_head_tracker)
.map_err(|e| format!("Failed to decode head tracker for database: {:?}", e))?,
let persisted_fork_choice = store
.get_item::<PersistedForkChoice>(&Hash256::from_slice(&FORK_CHOICE_DB_KEY))
.map_err(|e| format!("DB error when reading persisted fork choice: {:?}", e))?
.ok_or_else(|| "No persisted fork choice present in database.".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))?;

let mut fork_choice =
ForkChoice::from_persisted(persisted_fork_choice.fork_choice, fc_store)
.map_err(|e| format!("Unable to parse persisted fork choice from disk: {:?}", e))?;

let genesis_block = store
.get_item::<SignedBeaconBlock<TEthSpec>>(&chain.genesis_block_root)
.map_err(|e| format!("DB error when reading genesis block: {:?}", e))?
.ok_or_else(|| "Genesis block not found in store".to_string())?;
let genesis_state = store
.get_state(&genesis_block.state_root(), Some(genesis_block.slot()))
.map_err(|e| format!("DB error when reading genesis state: {:?}", e))?
.ok_or_else(|| "Genesis block not found in store".to_string())?;

let slot_clock = SystemTimeSlotClock::new(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should conjure a SystemTimeSlotClock from thin air here, as I think it will wreak havoc on the tests that use the testing slot clock. Instead I think we should use self.slot_clock, and patch up the existing couple of places where resume_from_db is called on the builder before slot_clock or testing_slot_clock (e.g. in beacon_chain/src/test_utils.rs)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yeah agree it's going to be bad for testing slot clock. The problem is that there's a chicken-and-egg problem; we need to know the genesis time (i.e., the genesis state) start the slot clock and we need the slot clock in order to learn the canonical head.

Presently those two actions are lumped together in one function and not generic across the slot clocks... I'll see what I can do...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I managed a decent solution (without changing the schema) that solves this. Let me know what you think :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! Addresses the chicken-and-egg problem nicely!

self.spec.genesis_slot,
Duration::from_secs(genesis_state.genesis_time),
Duration::from_millis(self.spec.milliseconds_per_slot),
);
let current_slot = slot_clock
.now()
.ok_or_else(|| "Unable to read slot".to_string())?;

let head_block_root = fork_choice
.get_head(current_slot)
.map_err(|e| format!("Unable to get fork choice head: {:?}", e))?;

let head_block_root = chain.canonical_head_block_root;
let head_block = store
.get_item::<SignedBeaconBlock<TEthSpec>>(&head_block_root)
.map_err(|e| format!("DB error when reading head block: {:?}", e))?
Expand All @@ -303,24 +334,6 @@ where
.unwrap_or_else(OperationPool::new),
);

let finalized_block_root = head_state.finalized_checkpoint.root;
let finalized_block = store
.get_item::<SignedBeaconBlock<TEthSpec>>(&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,
Expand All @@ -331,7 +344,13 @@ where
let pubkey_cache = ValidatorPubkeyCache::load_from_file(pubkey_cache_path)
.map_err(|e| format!("Unable to open persisted pubkey cache: {:?}", e))?;

self.genesis_block_root = Some(chain.genesis_block_root);
self.head_tracker = Some(
HeadTracker::from_ssz_container(&chain.ssz_head_tracker)
.map_err(|e| format!("Failed to decode head tracker for database: {:?}", e))?,
);
self.validator_pubkey_cache = Some(pubkey_cache);
self.fork_choice = Some(fork_choice);

Ok(self)
}
Expand Down Expand Up @@ -374,12 +393,20 @@ where
)
})?;

self.finalized_snapshot = Some(BeaconSnapshot {
let genesis = BeaconSnapshot {
beacon_block_root,
beacon_block,
beacon_state_root,
beacon_state,
});
};

let fc_store = BeaconForkChoiceStore::get_forkchoice_store(store, &genesis);

let fork_choice = ForkChoice::from_genesis(fc_store, &genesis.beacon_block.message)
.map_err(|e| format!("Unable to build initialize ForkChoice: {:?}", e))?;

self.fork_choice = Some(fork_choice);
self.canonical_head = Some(genesis);

Ok(self.empty_op_pool())
}
Expand Down Expand Up @@ -457,15 +484,37 @@ 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 mut canonical_head = self
.canonical_head
.clone()
.ok_or_else(|| "Cannot build without a canonical head.".to_string())?;
let fork_choice = self
.fork_choice
.ok_or_else(|| "Cannot build without fork choice.".to_string())?;
let genesis_block_root = self
.genesis_block_root
.ok_or_else(|| "Cannot build without a genesis block root".to_string())?;

// 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 == genesis_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
));
}
}

canonical_head
.beacon_state
Expand All @@ -485,26 +534,6 @@ where
.map_err(|e| format!("Unable to init validator pubkey cache: {:?}", e))
})?;

let persisted_fork_choice = store
.get_item::<PersistedForkChoice>(&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))?
};

let beacon_chain = BeaconChain {
spec: self.spec,
config: self.chain_config,
Expand Down Expand Up @@ -533,9 +562,7 @@ where
eth1_chain: self.eth1_chain,
genesis_validators_root: canonical_head.beacon_state.genesis_validators_root,
canonical_head: TimeoutRwLock::new(canonical_head.clone()),
genesis_block_root: self
.genesis_block_root
.ok_or_else(|| "Cannot build without a genesis block root".to_string())?,
genesis_block_root,
fork_choice: RwLock::new(fork_choice),
event_handler: self
.event_handler
Expand Down Expand Up @@ -634,7 +661,7 @@ where
/// Requires the state to be initialized.
pub fn testing_slot_clock(self, slot_duration: Duration) -> Result<Self, String> {
let genesis_time = self
.finalized_snapshot
.canonical_head
.as_ref()
.ok_or_else(|| "testing_slot_clock requires an initialized state")?
.beacon_state
Expand Down
7 changes: 7 additions & 0 deletions beacon_node/beacon_chain/src/persisted_beacon_chain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,13 @@ use types::Hash256;

#[derive(Clone, Encode, Decode)]
pub struct PersistedBeaconChain {
/// This value is ignored to resolve the issue described here:
///
/// https://github.com/sigp/lighthouse/pull/1639
///
/// The following PR will clean-up and remove this field:
///
/// https://github.com/sigp/lighthouse/pull/1638
pub canonical_head_block_root: Hash256,
pub genesis_block_root: Hash256,
pub ssz_head_tracker: SszHeadTracker,
Expand Down
2 changes: 1 addition & 1 deletion beacon_node/client/src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,7 +713,7 @@ 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")?
.beacon_state
Expand Down
8 changes: 7 additions & 1 deletion consensus/fork_choice/src/fork_choice.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)`.
Expand Down