diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index d63447e463..ec50f878b1 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -326,6 +326,44 @@ impl BeaconChain { ReverseBlockRootIterator::new((head.beacon_block_root, head.beacon_block.slot), iter) } + /// Traverse backwards from `block_root` to find the block roots of its ancestors. + /// + /// ## Notes + /// + /// `slot` always decreases by `1`. + /// - Skipped slots contain the root of the closest prior + /// non-skipped slot (identical to the way they are stored in `state.block_roots`) . + /// - Iterator returns `(Hash256, Slot)`. + /// - The provided `block_root` is included as the first item in the iterator. + pub fn rev_iter_block_roots_from( + &self, + block_root: Hash256, + ) -> Result, Error> { + let block = self + .get_block(&block_root)? + .ok_or_else(|| Error::MissingBeaconBlock(block_root))?; + let state = self + .get_state(&block.state_root)? + .ok_or_else(|| Error::MissingBeaconState(block.state_root))?; + let iter = BlockRootsIterator::owned(self.store.clone(), state); + Ok(ReverseBlockRootIterator::new( + (block_root, block.slot), + iter, + )) + } + + /// Traverse backwards from `block_root` to find the root of the ancestor block at `slot`. + pub fn get_ancestor_block_root( + &self, + block_root: Hash256, + slot: Slot, + ) -> Result, Error> { + Ok(self + .rev_iter_block_roots_from(block_root)? + .find(|(_, ancestor_slot)| *ancestor_slot == slot) + .map(|(ancestor_block_root, _)| ancestor_block_root)) + } + /// Iterates across all `(state_root, slot)` pairs from the head of the chain (inclusive) to /// the earliest reachable ancestor (may or may not be genesis). /// @@ -356,6 +394,18 @@ impl BeaconChain { Ok(self.store.get(block_root)?) } + /// Returns the state at the given root, if any. + /// + /// ## Errors + /// + /// May return a database error. + pub fn get_state( + &self, + state_root: &Hash256, + ) -> Result>, Error> { + Ok(self.store.get(state_root)?) + } + /// Returns a `Checkpoint` representing the head block and state. Contains the "best block"; /// the head of the canonical `BeaconChain`. /// @@ -871,20 +921,27 @@ impl BeaconChain { Ok(AttestationProcessingOutcome::Invalid(e)) } else { - // Provide the attestation to fork choice, updating the validator latest messages but - // _without_ finding and updating the head. - if let Err(e) = self - .fork_choice - .process_attestation(&state, &attestation, block) + // If the attestation is from the current or previous epoch, supply it to the fork + // choice. This is FMD GHOST. + let current_epoch = self.epoch()?; + if attestation.data.target.epoch == current_epoch + || attestation.data.target.epoch == current_epoch - 1 { - error!( - self.log, - "Add attestation to fork choice failed"; - "fork_choice_integrity" => format!("{:?}", self.fork_choice.verify_integrity()), - "beacon_block_root" => format!("{}", attestation.data.beacon_block_root), - "error" => format!("{:?}", e) - ); - return Err(e.into()); + // Provide the attestation to fork choice, updating the validator latest messages but + // _without_ finding and updating the head. + if let Err(e) = self + .fork_choice + .process_attestation(&state, &attestation, block) + { + error!( + self.log, + "Add attestation to fork choice failed"; + "fork_choice_integrity" => format!("{:?}", self.fork_choice.verify_integrity()), + "beacon_block_root" => format!("{}", attestation.data.beacon_block_root), + "error" => format!("{:?}", e) + ); + return Err(e.into()); + } } // Provide the valid attestation to op pool, which may choose to retain the @@ -1194,7 +1251,10 @@ impl BeaconChain { metrics::start_timer(&metrics::BLOCK_PROCESSING_FORK_CHOICE_REGISTER); // Register the new block with the fork choice service. - if let Err(e) = self.fork_choice.process_block(&state, &block, block_root) { + if let Err(e) = self + .fork_choice + .process_block(self, &state, &block, block_root) + { error!( self.log, "Add block to fork choice failed"; diff --git a/beacon_node/beacon_chain/src/errors.rs b/beacon_node/beacon_chain/src/errors.rs index 0306899288..66eb65b8e5 100644 --- a/beacon_node/beacon_chain/src/errors.rs +++ b/beacon_node/beacon_chain/src/errors.rs @@ -18,7 +18,6 @@ macro_rules! easy_from_to { #[derive(Debug, PartialEq)] pub enum BeaconChainError { InsufficientValidators, - BadRecentBlockRoots, UnableToReadSlot, RevertedFinalizedEpoch { previous_epoch: Epoch, diff --git a/beacon_node/beacon_chain/src/fork_choice.rs b/beacon_node/beacon_chain/src/fork_choice.rs index 26084e04a7..bcda2bcecf 100644 --- a/beacon_node/beacon_chain/src/fork_choice.rs +++ b/beacon_node/beacon_chain/src/fork_choice.rs @@ -1,10 +1,11 @@ -use crate::{metrics, BeaconChain, BeaconChainTypes}; +use crate::{errors::BeaconChainError, metrics, BeaconChain, BeaconChainTypes}; use lmd_ghost::LmdGhost; -use state_processing::common::get_attesting_indices; +use parking_lot::RwLock; +use state_processing::{common::get_attesting_indices, per_slot_processing}; use std::sync::Arc; use store::{Error as StoreError, Store}; use types::{ - Attestation, BeaconBlock, BeaconState, BeaconStateError, Epoch, EthSpec, Hash256, Slot, + Attestation, BeaconBlock, BeaconState, BeaconStateError, Checkpoint, EthSpec, Hash256, Slot, }; type Result = std::result::Result; @@ -16,6 +17,7 @@ pub enum Error { BackendError(String), BeaconStateError(BeaconStateError), StoreError(StoreError), + BeaconChainError(Box), } pub struct ForkChoice { @@ -26,6 +28,10 @@ pub struct ForkChoice { /// Does not necessarily need to be the _actual_ genesis, it suffices to be the finalized root /// whenever the struct was instantiated. genesis_block_root: Hash256, + /// The fork choice rule's current view of the justified checkpoint. + justified_checkpoint: RwLock, + /// The best justified checkpoint we've seen, which may be ahead of `justified_checkpoint`. + best_justified_checkpoint: RwLock, } impl ForkChoice { @@ -38,38 +44,86 @@ impl ForkChoice { genesis_block: &BeaconBlock, genesis_block_root: Hash256, ) -> Self { + let genesis_slot = genesis_block.slot; + let justified_checkpoint = Checkpoint { + epoch: genesis_slot.epoch(T::EthSpec::slots_per_epoch()), + root: genesis_block_root, + }; Self { store: store.clone(), backend: T::LmdGhost::new(store, genesis_block, genesis_block_root), genesis_block_root, + justified_checkpoint: RwLock::new(justified_checkpoint.clone()), + best_justified_checkpoint: RwLock::new(justified_checkpoint), } } + /// Determine whether the fork choice's view of the justified checkpoint should be updated. + /// + /// To prevent the bouncing attack, an update is allowed only in these conditions: + /// + /// * We're in the first SAFE_SLOTS_TO_UPDATE_JUSTIFIED slots of the epoch, or + /// * The new justified checkpoint is a descendant of the current justified checkpoint + fn should_update_justified_checkpoint( + &self, + chain: &BeaconChain, + new_justified_checkpoint: &Checkpoint, + ) -> Result { + if Self::compute_slots_since_epoch_start(chain.slot()?) + < chain.spec.safe_slots_to_update_justified + { + return Ok(true); + } + + let justified_checkpoint = self.justified_checkpoint.read().clone(); + + let current_justified_block = chain + .get_block(&justified_checkpoint.root)? + .ok_or_else(|| Error::MissingBlock(justified_checkpoint.root))?; + + let new_justified_block = chain + .get_block(&new_justified_checkpoint.root)? + .ok_or_else(|| Error::MissingBlock(new_justified_checkpoint.root))?; + + let slots_per_epoch = T::EthSpec::slots_per_epoch(); + + Ok( + new_justified_block.slot > justified_checkpoint.epoch.start_slot(slots_per_epoch) + && chain.get_ancestor_block_root( + new_justified_checkpoint.root, + current_justified_block.slot, + )? == Some(justified_checkpoint.root), + ) + } + + /// Calculate how far `slot` lies from the start of its epoch. + fn compute_slots_since_epoch_start(slot: Slot) -> u64 { + let slots_per_epoch = T::EthSpec::slots_per_epoch(); + (slot - slot.epoch(slots_per_epoch).start_slot(slots_per_epoch)).as_u64() + } + + /// Run the fork choice rule to determine the head. pub fn find_head(&self, chain: &BeaconChain) -> Result { let timer = metrics::start_timer(&metrics::FORK_CHOICE_FIND_HEAD_TIMES); - let start_slot = |epoch: Epoch| epoch.start_slot(T::EthSpec::slots_per_epoch()); - - // From the specification: - // - // Let justified_head be the descendant of finalized_head with the highest epoch that has - // been justified for at least 1 epoch ... If no such descendant exists, - // set justified_head to finalized_head. let (start_state, start_block_root, start_block_slot) = { - let state = &chain.head().beacon_state; + // Check if we should update our view of the justified checkpoint. + // Doing this check here should be quasi-equivalent to the update in the `on_tick` + // function of the spec, so long as `find_head` is called at least once during the first + // SAFE_SLOTS_TO_UPDATE_JUSTIFIED slots. + let best_justified_checkpoint = self.best_justified_checkpoint.read(); + if self.should_update_justified_checkpoint(chain, &best_justified_checkpoint)? { + *self.justified_checkpoint.write() = best_justified_checkpoint.clone(); + } - let (block_root, block_slot) = - if state.current_epoch() + 1 > state.current_justified_checkpoint.epoch { - ( - state.current_justified_checkpoint.root, - start_slot(state.current_justified_checkpoint.epoch), - ) - } else { - ( - state.finalized_checkpoint.root, - start_slot(state.finalized_checkpoint.epoch), - ) - }; + let current_justified_checkpoint = self.justified_checkpoint.read().clone(); + + let (block_root, block_justified_slot) = ( + current_justified_checkpoint.root, + current_justified_checkpoint + .epoch + .start_slot(T::EthSpec::slots_per_epoch()), + ); let block = chain .store @@ -83,12 +137,17 @@ impl ForkChoice { block_root }; - let state = chain - .store - .get::>(&block.state_root)? + let mut state = chain + .get_state(&block.state_root)? .ok_or_else(|| Error::MissingState(block.state_root))?; - (state, block_root, block_slot) + // Fast-forward the state to the start slot of the epoch where it was justified. + for _ in block.slot.as_u64()..block_justified_slot.as_u64() { + per_slot_processing(&mut state, &chain.spec) + .map_err(|e| BeaconChainError::SlotProcessingError(e))? + } + + (state, block_root, block_justified_slot) }; // A function that returns the weight for some validator index. @@ -111,10 +170,11 @@ impl ForkChoice { /// Process all attestations in the given `block`. /// - /// Assumes the block (and therefore it's attestations) are valid. It is a logic error to + /// Assumes the block (and therefore its attestations) are valid. It is a logic error to /// provide an invalid block. pub fn process_block( &self, + chain: &BeaconChain, state: &BeaconState, block: &BeaconBlock, block_root: Hash256, @@ -137,6 +197,16 @@ impl ForkChoice { } } + // Check if we should update our view of the justified checkpoint + if state.current_justified_checkpoint.epoch > self.justified_checkpoint.read().epoch { + *self.best_justified_checkpoint.write() = state.current_justified_checkpoint.clone(); + if self + .should_update_justified_checkpoint(chain, &state.current_justified_checkpoint)? + { + *self.justified_checkpoint.write() = state.current_justified_checkpoint.clone(); + } + } + // This does not apply a vote to the block, it just makes fork choice aware of the block so // it can still be identified as the head even if it doesn't have any votes. // @@ -228,6 +298,12 @@ impl From for Error { } } +impl From for Error { + fn from(e: BeaconChainError) -> Error { + Error::BeaconChainError(Box::new(e)) + } +} + impl From for Error { fn from(e: StoreError) -> Error { Error::StoreError(e) diff --git a/eth2/lmd_ghost/Cargo.toml b/eth2/lmd_ghost/Cargo.toml index e26b85626c..d78faaab39 100644 --- a/eth2/lmd_ghost/Cargo.toml +++ b/eth2/lmd_ghost/Cargo.toml @@ -8,6 +8,7 @@ edition = "2018" parking_lot = "0.9.0" store = { path = "../../beacon_node/store" } types = { path = "../types" } +itertools = "0.8.1" [dev-dependencies] criterion = "0.3.0" diff --git a/eth2/lmd_ghost/src/lib.rs b/eth2/lmd_ghost/src/lib.rs index 167cd36eaf..d58affe146 100644 --- a/eth2/lmd_ghost/src/lib.rs +++ b/eth2/lmd_ghost/src/lib.rs @@ -49,7 +49,7 @@ pub trait LmdGhost: Send + Sync { /// Runs an integrity verification function on fork choice algorithm. /// - /// Returns `Ok(())` if the underlying fork choice has maintained it's integrity, + /// Returns `Ok(())` if the underlying fork choice has maintained its integrity, /// `Err(description)` otherwise. fn verify_integrity(&self) -> Result<()>; } diff --git a/eth2/lmd_ghost/src/reduced_tree.rs b/eth2/lmd_ghost/src/reduced_tree.rs index b786887f11..85540785db 100644 --- a/eth2/lmd_ghost/src/reduced_tree.rs +++ b/eth2/lmd_ghost/src/reduced_tree.rs @@ -4,6 +4,7 @@ //! //! This implementation is incomplete and has known bugs. Do not use in production. use super::{LmdGhost, Result as SuperResult}; +use itertools::Itertools; use parking_lot::RwLock; use std::collections::HashMap; use std::fmt; @@ -20,6 +21,7 @@ pub enum Error { MissingBlock(Hash256), MissingState(Hash256), MissingChild(Hash256), + MissingSuccessor(Hash256, Hash256), NotInTree(Hash256), NoCommonAncestor((Hash256, Hash256)), StoreError(StoreError), @@ -177,8 +179,8 @@ where if current_hash != subtree_hash { let children = self.get_node(current_hash)?.children.clone(); - for child_hash in children { - self.retain_subtree(child_hash, subtree_hash)?; + for child in children { + self.retain_subtree(child.hash, subtree_hash)?; } self.nodes.remove(¤t_hash); @@ -239,7 +241,7 @@ where let _root_weight = self.update_weight(start_block_root, weight_fn)?; let start_node = self.get_node(start_block_root)?; - let head_node = self.find_head_from(start_node)?; + let head_node = self.find_head_from(start_node, start_block_slot)?; Ok(head_node.block_hash) } @@ -251,31 +253,32 @@ where } } - fn find_head_from<'a>(&'a self, start_node: &'a Node) -> Result<&'a Node> { - if start_node.does_not_have_children() { + // Corresponds to the loop in `get_head` in the spec. + fn find_head_from<'a>( + &'a self, + start_node: &'a Node, + justified_slot: Slot, + ) -> Result<&'a Node> { + let children = start_node + .children + .iter() + // This check is primarily for the first iteration, where we must ensure that + // we only consider votes that were made after the last justified checkpoint. + .filter(|c| c.successor_slot > justified_slot) + .map(|c| self.get_node(c.hash)) + .collect::>>()?; + + if children.is_empty() { Ok(start_node) } else { - let children = start_node - .children - .iter() - .map(|hash| self.get_node(*hash)) - .collect::>>()?; - - // TODO: check if `max_by` is `O(n^2)`. let best_child = children .iter() - .max_by(|a, b| { - if a.weight != b.weight { - a.weight.cmp(&b.weight) - } else { - a.block_hash.cmp(&b.block_hash) - } - }) + .max_by_key(|child| (child.weight, child.block_hash)) // There can only be no maximum if there are no children. This code path is guarded // against that condition. .expect("There must be a maximally weighted node."); - self.find_head_from(best_child) + self.find_head_from(best_child, justified_slot) } } @@ -288,8 +291,8 @@ where let mut weight = 0; - for &child in &node.children { - weight += self.update_weight(child, weight_fn)?; + for child in &node.children { + weight += self.update_weight(child.hash, weight_fn)?; } for &voter in &node.voters { @@ -323,13 +326,13 @@ where // // Load the child of the node and set it's parent to be the parent of this // node (viz., graft the node's child to the node's parent) - let child = self.get_mut_node(node.children[0])?; + let child = self.get_mut_node(node.children[0].hash)?; child.parent_hash = node.parent_hash; // Graft the parent of this node to it's child. if let Some(parent_hash) = node.parent_hash { let parent = self.get_mut_node(parent_hash)?; - parent.replace_child(node.block_hash, node.children[0])?; + parent.replace_child_hash(node.block_hash, node.children[0].hash)?; } self.nodes.remove(&vote.hash); @@ -376,15 +379,16 @@ where let node = node.clone(); if let Some(parent_hash) = node.parent_hash { - if (node.children.len() == 1) && !node.has_votes() { - let child_hash = node.children[0]; + if node.children.len() == 1 && !node.has_votes() { + let child = &node.children[0]; // Graft the single descendant `node` to the `parent` of node. - self.get_mut_node(child_hash)?.parent_hash = Some(parent_hash); + self.get_mut_node(child.hash)?.parent_hash = Some(parent_hash); // Detach `node` from `parent`, replacing it with `child`. + // Preserve the parent's direct descendant slot. self.get_mut_node(parent_hash)? - .replace_child(hash, child_hash)?; + .replace_child_hash(hash, child.hash)?; true } else { @@ -442,6 +446,40 @@ where Ok(()) } + /// Find the direct successor block of `ancestor` if `descendant` is a descendant. + fn find_ancestor_successor_opt( + &self, + ancestor: Hash256, + descendant: Hash256, + ) -> Result> { + Ok(std::iter::once(descendant) + .chain( + self.iter_ancestors(descendant)? + .take_while(|(_, slot)| *slot >= self.root_slot()) + .map(|(block_hash, _)| block_hash), + ) + .tuple_windows() + .find_map(|(successor, block_hash)| { + if block_hash == ancestor { + Some(successor) + } else { + None + } + })) + } + + /// Same as `find_ancestor_successor_opt` but will return an error instead of an option. + fn find_ancestor_successor(&self, ancestor: Hash256, descendant: Hash256) -> Result { + self.find_ancestor_successor_opt(ancestor, descendant)? + .ok_or_else(|| Error::MissingSuccessor(ancestor, descendant)) + } + + /// Look up the successor of the given `ancestor`, returning the slot of that block. + fn find_ancestor_successor_slot(&self, ancestor: Hash256, descendant: Hash256) -> Result { + let successor_hash = self.find_ancestor_successor(ancestor, descendant)?; + Ok(self.get_block(successor_hash)?.slot) + } + /// Add `node` to the reduced tree, returning an error if `node` is not rooted in the tree. fn add_node(&mut self, mut node: Node) -> Result<()> { // Find the highest (by slot) ancestor of the given node in the reduced tree. @@ -460,7 +498,9 @@ where // `node` to it. // 3. Graft `node` to an existing node. if !prev_in_tree.children.is_empty() { - for &child_hash in &prev_in_tree.children { + for child_link in &prev_in_tree.children { + let child_hash = child_link.hash; + // 1. Graft the new node between two existing nodes. // // If `node` is a descendant of `prev_in_tree` but an ancestor of a child connected to @@ -468,19 +508,20 @@ where // // This means that `node` can be grafted between `prev_in_tree` and the child that is a // descendant of both `node` and `prev_in_tree`. - if self - .iter_ancestors(child_hash)? - .take_while(|(_, slot)| *slot >= self.root_slot()) - .any(|(ancestor, _slot)| ancestor == node.block_hash) + if let Some(successor) = + self.find_ancestor_successor_opt(node.block_hash, child_hash)? { let child = self.get_mut_node(child_hash)?; // Graft `child` to `node`. child.parent_hash = Some(node.block_hash); // Graft `node` to `child`. - node.children.push(child_hash); + node.children.push(ChildLink { + hash: child_hash, + successor_slot: self.get_block(successor)?.slot, + }); // Detach `child` from `prev_in_tree`, replacing it with `node`. - prev_in_tree.replace_child(child_hash, node.block_hash)?; + prev_in_tree.replace_child_hash(child_hash, node.block_hash)?; // Graft `node` to `prev_in_tree`. node.parent_hash = Some(prev_in_tree.block_hash); @@ -495,7 +536,8 @@ where // any of the children of `prev_in_tree`, we know that `node` is on a different fork to // all of the children of `prev_in_tree`. if node.parent_hash.is_none() { - for &child_hash in &prev_in_tree.children { + for child_link in &prev_in_tree.children { + let child_hash = child_link.hash; // Find the highest (by slot) common ancestor between `node` and `child`. // // The common ancestor is the last block before `node` and `child` forked. @@ -506,24 +548,37 @@ where // must add this new block into the tree (because it is a decision node // between two forks). if ancestor_hash != prev_in_tree.block_hash { - let child = self.get_mut_node(child_hash)?; - // Create a new `common_ancestor` node which represents the `ancestor_hash` // block, has `prev_in_tree` as the parent and has both `node` and `child` // as children. let common_ancestor = Node { block_hash: ancestor_hash, parent_hash: Some(prev_in_tree.block_hash), - children: vec![node.block_hash, child_hash], + children: vec![ + ChildLink { + hash: node.block_hash, + successor_slot: self.find_ancestor_successor_slot( + ancestor_hash, + node.block_hash, + )?, + }, + ChildLink { + hash: child_hash, + successor_slot: self + .find_ancestor_successor_slot(ancestor_hash, child_hash)?, + }, + ], ..Node::default() }; + let child = self.get_mut_node(child_hash)?; + // Graft `child` and `node` to `common_ancestor`. child.parent_hash = Some(common_ancestor.block_hash); node.parent_hash = Some(common_ancestor.block_hash); // Detach `child` from `prev_in_tree`, replacing it with `common_ancestor`. - prev_in_tree.replace_child(child_hash, common_ancestor.block_hash)?; + prev_in_tree.replace_child_hash(child_hash, common_ancestor.block_hash)?; // Store the new `common_ancestor` node. self.nodes @@ -540,7 +595,11 @@ where // // Graft `node` to `prev_in_tree` and `prev_in_tree` to `node` node.parent_hash = Some(prev_in_tree.block_hash); - prev_in_tree.children.push(node.block_hash); + prev_in_tree.children.push(ChildLink { + hash: node.block_hash, + successor_slot: self + .find_ancestor_successor_slot(prev_in_tree.block_hash, node.block_hash)?, + }); } // Update `prev_in_tree`. A mutable reference was not maintained to satisfy the borrow @@ -655,7 +714,17 @@ where node.children .iter() - .map(|child| verify_node_exists(*child, "child_must_exist".to_string())) + .map(|child| { + verify_node_exists(child.hash, "child_must_exist".to_string())?; + + if self.find_ancestor_successor_slot(node.block_hash, child.hash)? + == child.successor_slot + { + Ok(()) + } else { + Err("successor slot on child link is incorrect".to_string()) + } + }) .collect::>()?; verify_node_exists(node.block_hash, "block hash must exist".to_string())?; @@ -698,25 +767,35 @@ where #[derive(Default, Clone, Debug)] pub struct Node { + /// Hash of the parent node in the reduced tree (not necessarily parent block). pub parent_hash: Option, - pub children: Vec, + pub children: Vec, pub weight: u64, pub block_hash: Hash256, pub voters: Vec, } -impl Node { - pub fn does_not_have_children(&self) -> bool { - self.children.is_empty() - } +#[derive(Default, Clone, Debug)] +pub struct ChildLink { + /// Hash of the child block (may not be a direct descendant). + pub hash: Hash256, + /// Slot of the block which is a direct descendant on the chain leading to `hash`. + /// + /// Node <--- Successor <--- ... <--- Child + pub successor_slot: Slot, +} - pub fn replace_child(&mut self, old: Hash256, new: Hash256) -> Result<()> { +impl Node { + /// Replace a child with a new child, whilst preserving the successor slot. + /// + /// The new child should have the same ancestor successor block as the old one. + pub fn replace_child_hash(&mut self, old: Hash256, new: Hash256) -> Result<()> { let i = self .children .iter() - .position(|&c| c == old) + .position(|c| c.hash == old) .ok_or_else(|| Error::MissingChild(old))?; - self.children[i] = new; + self.children[i].hash = new; Ok(()) } @@ -725,7 +804,7 @@ impl Node { let i = self .children .iter() - .position(|&c| c == child) + .position(|c| c.hash == child) .ok_or_else(|| Error::MissingChild(child))?; self.children.remove(i); diff --git a/eth2/lmd_ghost/tests/test.rs b/eth2/lmd_ghost/tests/test.rs index 49e9ff738a..13eaf207a8 100644 --- a/eth2/lmd_ghost/tests/test.rs +++ b/eth2/lmd_ghost/tests/test.rs @@ -45,7 +45,9 @@ struct ForkedHarness { pub genesis_block: BeaconBlock, pub honest_head: RootAndSlot, pub faulty_head: RootAndSlot, + /// Honest roots in reverse order (slot high to low) pub honest_roots: Vec, + /// Faulty roots in reverse order (slot high to low) pub faulty_roots: Vec, } @@ -222,7 +224,7 @@ fn single_voter_persistent_instance_reverse_order() { "New tree should have integrity" ); - for (root, slot) in harness.honest_roots.iter().rev() { + for (root, slot) in &harness.honest_roots { lmd.process_attestation(0, *root, *slot) .expect("fork choice should accept attestations to honest roots in reverse"); @@ -234,11 +236,15 @@ fn single_voter_persistent_instance_reverse_order() { } // The honest head should be selected. - let (head_root, head_slot) = harness.honest_roots.first().unwrap(); - let (finalized_root, _) = harness.honest_roots.last().unwrap(); + let (head_root, _) = harness.honest_roots.first().unwrap(); + let (finalized_root, finalized_slot) = harness.honest_roots.last().unwrap(); assert_eq!( - lmd.find_head(*head_slot, *finalized_root, ForkedHarness::weight_function), + lmd.find_head( + *finalized_slot, + *finalized_root, + ForkedHarness::weight_function + ), Ok(*head_root), "Honest head should be selected" ); @@ -250,7 +256,7 @@ fn single_voter_persistent_instance_reverse_order() { fn single_voter_many_instance_honest_blocks_voting_forwards() { let harness = &FORKED_HARNESS; - for (root, slot) in &harness.honest_roots { + for (root, slot) in harness.honest_roots.iter().rev() { let lmd = harness.new_fork_choice(); lmd.process_attestation(0, *root, *slot) .expect("fork choice should accept attestations to honest roots"); @@ -269,7 +275,7 @@ fn single_voter_many_instance_honest_blocks_voting_in_reverse() { let harness = &FORKED_HARNESS; // Same as above, but in reverse order (votes on the highest honest block first). - for (root, slot) in harness.honest_roots.iter().rev() { + for (root, slot) in &harness.honest_roots { let lmd = harness.new_fork_choice(); lmd.process_attestation(0, *root, *slot) .expect("fork choice should accept attestations to honest roots in reverse"); @@ -288,7 +294,7 @@ fn single_voter_many_instance_honest_blocks_voting_in_reverse() { fn single_voter_many_instance_faulty_blocks_voting_forwards() { let harness = &FORKED_HARNESS; - for (root, slot) in &harness.faulty_roots { + for (root, slot) in harness.faulty_roots.iter().rev() { let lmd = harness.new_fork_choice(); lmd.process_attestation(0, *root, *slot) .expect("fork choice should accept attestations to faulty roots"); @@ -306,7 +312,7 @@ fn single_voter_many_instance_faulty_blocks_voting_forwards() { fn single_voter_many_instance_faulty_blocks_voting_in_reverse() { let harness = &FORKED_HARNESS; - for (root, slot) in harness.faulty_roots.iter().rev() { + for (root, slot) in &harness.faulty_roots { let lmd = harness.new_fork_choice(); lmd.process_attestation(0, *root, *slot) .expect("fork choice should accept attestations to faulty roots in reverse"); @@ -319,6 +325,49 @@ fn single_voter_many_instance_faulty_blocks_voting_in_reverse() { } } +/// Ensure that votes with slots before the justified slot are not counted. +#[test] +fn discard_votes_before_justified_slot() { + let harness = &FORKED_HARNESS; + + let lmd = harness.new_fork_choice(); + + let (genesis_root, genesis_slot) = *harness.honest_roots.last().unwrap(); + + dbg!(&harness.honest_roots); + + // Add attestations from all validators for all honest blocks. + for (root, slot) in harness.honest_roots.iter().rev() { + for i in 0..VALIDATOR_COUNT { + lmd.process_attestation(i, *root, *slot) + .expect("should accept attestations in increasing order"); + } + + // Head starting from 0 checkpoint (genesis) should be current root + assert_eq!( + lmd.find_head(genesis_slot, genesis_root, ForkedHarness::weight_function), + Ok(*root), + "Honest head should be selected" + ); + + assert_eq!( + lmd.find_head( + genesis_slot + 1, + genesis_root, + ForkedHarness::weight_function + ), + Ok(genesis_root) + ); + } + + let head = harness.harness.chain.head(); + + dbg!(&harness.honest_roots); + dbg!(head.beacon_state.current_justified_checkpoint); + + // assert!(false); +} + /// Ensures that the finalized root can be set to all values in `roots`. fn test_update_finalized_root(roots: &[(Hash256, Slot)]) { let harness = &FORKED_HARNESS; diff --git a/eth2/types/src/chain_spec.rs b/eth2/types/src/chain_spec.rs index fc4a71b07c..3e182733f2 100644 --- a/eth2/types/src/chain_spec.rs +++ b/eth2/types/src/chain_spec.rs @@ -84,6 +84,11 @@ pub struct ChainSpec { domain_deposit: u32, domain_voluntary_exit: u32, + /* + * Fork choice + */ + pub safe_slots_to_update_justified: u64, + pub boot_nodes: Vec, pub network_id: u8, } @@ -184,6 +189,11 @@ impl ChainSpec { domain_deposit: 3, domain_voluntary_exit: 4, + /* + * Fork choice + */ + safe_slots_to_update_justified: 8, + /* * Network specific */