Update fork choice for v0.9.1

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -326,6 +326,44 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         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<ReverseBlockRootIterator<T::EthSpec, T::Store>, 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<Option<Hash256>, 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<T: BeaconChainTypes> BeaconChain<T> {
         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<Option<BeaconState<T::EthSpec>>, 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<T: BeaconChainTypes> BeaconChain<T> {
 
             Ok(AttestationProcessingOutcome::Invalid(e))
         } else {
-            // Provide the attestation to fork choice, updating the validator latest messages but
+            // If the attestation is from the current or previous epoch, supply it to the fork
-            // _without_ finding and updating the head.
+            // choice. This is FMD GHOST.
-            if let Err(e) = self
+            let current_epoch = self.epoch()?;
-                .fork_choice
+            if attestation.data.target.epoch == current_epoch
-                .process_attestation(&state, &attestation, block)
+                || attestation.data.target.epoch == current_epoch - 1
             {
-                error!(
+                // Provide the attestation to fork choice, updating the validator latest messages but
-                    self.log,
+                // _without_ finding and updating the head.
-                    "Add attestation to fork choice failed";
+                if let Err(e) = self
-                    "fork_choice_integrity" => format!("{:?}", self.fork_choice.verify_integrity()),
+                    .fork_choice
-                    "beacon_block_root" =>  format!("{}", attestation.data.beacon_block_root),
+                    .process_attestation(&state, &attestation, block)
-                    "error" => format!("{:?}", e)
+                {
-                );
+                    error!(
-                return Err(e.into());
+                        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<T: BeaconChainTypes> BeaconChain<T> {
             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";

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,

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<T> = std::result::Result<T, Error>;
@@ -16,6 +17,7 @@ pub enum Error {
     BackendError(String),
     BeaconStateError(BeaconStateError),
     StoreError(StoreError),
+    BeaconChainError(Box<BeaconChainError>),
 }
 
 pub struct ForkChoice<T: BeaconChainTypes> {
@@ -26,6 +28,10 @@ pub struct ForkChoice<T: BeaconChainTypes> {
     /// 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<Checkpoint>,
+    /// The best justified checkpoint we've seen, which may be ahead of `justified_checkpoint`.
+    best_justified_checkpoint: RwLock<Checkpoint>,
 }
 
 impl<T: BeaconChainTypes> ForkChoice<T> {
@@ -38,38 +44,86 @@ impl<T: BeaconChainTypes> ForkChoice<T> {
         genesis_block: &BeaconBlock<T::EthSpec>,
         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<T>,
+        new_justified_checkpoint: &Checkpoint,
+    ) -> Result<bool> {
+        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<T>) -> Result<Hash256> {
         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) =
+            let current_justified_checkpoint = self.justified_checkpoint.read().clone();
-                if state.current_epoch() + 1 > state.current_justified_checkpoint.epoch {
+
-                    (
+            let (block_root, block_justified_slot) = (
-                        state.current_justified_checkpoint.root,
+                current_justified_checkpoint.root,
-                        start_slot(state.current_justified_checkpoint.epoch),
+                current_justified_checkpoint
-                    )
+                    .epoch
-                } else {
+                    .start_slot(T::EthSpec::slots_per_epoch()),
-                    (
+            );
-                        state.finalized_checkpoint.root,
-                        start_slot(state.finalized_checkpoint.epoch),
-                    )
-                };
 
             let block = chain
                 .store
@@ -83,12 +137,17 @@ impl<T: BeaconChainTypes> ForkChoice<T> {
                 block_root
             };
 
-            let state = chain
+            let mut state = chain
-                .store
+                .get_state(&block.state_root)?
-                .get::<BeaconState<T::EthSpec>>(&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<T: BeaconChainTypes> ForkChoice<T> {
 
     /// 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<T>,
         state: &BeaconState<T::EthSpec>,
         block: &BeaconBlock<T::EthSpec>,
         block_root: Hash256,
@@ -137,6 +197,16 @@ impl<T: BeaconChainTypes> ForkChoice<T> {
             }
         }
 
+        // 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<BeaconStateError> for Error {
     }
 }
 
+impl From<BeaconChainError> for Error {
+    fn from(e: BeaconChainError) -> Error {
+        Error::BeaconChainError(Box::new(e))
+    }
+}
+
 impl From<StoreError> for Error {
     fn from(e: StoreError) -> Error {
         Error::StoreError(e)

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"

eth2/lmd_ghost/src/lib.rs

@@ -49,7 +49,7 @@ pub trait LmdGhost<S: Store, E: EthSpec>: 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<()>;
 }

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 {
+            for child in children {
-                self.retain_subtree(child_hash, subtree_hash)?;
+                self.retain_subtree(child.hash, subtree_hash)?;
             }
 
             self.nodes.remove(&current_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> {
+    // Corresponds to the loop in `get_head` in the spec.
-        if start_node.does_not_have_children() {
+    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::<Result<Vec<&Node>>>()?;
+
+        if children.is_empty() {
             Ok(start_node)
         } else {
-            let children = start_node
-                .children
-                .iter()
-                .map(|hash| self.get_node(*hash))
-                .collect::<Result<Vec<&Node>>>()?;
-
-            // TODO: check if `max_by` is `O(n^2)`.
             let best_child = children
                 .iter()
-                .max_by(|a, b| {
+                .max_by_key(|child| (child.weight, child.block_hash))
-                    if a.weight != b.weight {
-                        a.weight.cmp(&b.weight)
-                    } else {
-                        a.block_hash.cmp(&b.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 {
+            for child in &node.children {
-                weight += self.update_weight(child, weight_fn)?;
+                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() {
+                    if node.children.len() == 1 && !node.has_votes() {
-                        let child_hash = node.children[0];
+                        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<Option<Hash256>> {
+        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<Hash256> {
+        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<Slot> {
+        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
+                if let Some(successor) =
-                    .iter_ancestors(child_hash)?
+                    self.find_ancestor_successor_opt(node.block_hash, child_hash)?
-                    .take_while(|(_, slot)| *slot >= self.root_slot())
-                    .any(|(ancestor, _slot)| ancestor == node.block_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::<std::result::Result<(), String>>()?;
 
                 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<Hash256>,
-    pub children: Vec<Hash256>,
+    pub children: Vec<ChildLink>,
     pub weight: u64,
     pub block_hash: Hash256,
     pub voters: Vec<usize>,
 }
 
-impl Node {
+#[derive(Default, Clone, Debug)]
-    pub fn does_not_have_children(&self) -> bool {
+pub struct ChildLink {
-        self.children.is_empty()
+    /// 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);

eth2/lmd_ghost/tests/test.rs

@@ -45,7 +45,9 @@ struct ForkedHarness {
     pub genesis_block: BeaconBlock<TestEthSpec>,
     pub honest_head: RootAndSlot,
     pub faulty_head: RootAndSlot,
+    /// Honest roots in reverse order (slot high to low)
     pub honest_roots: Vec<RootAndSlot>,
+    /// Faulty roots in reverse order (slot high to low)
     pub faulty_roots: Vec<RootAndSlot>,
 }
 
@@ -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 (head_root, _) = harness.honest_roots.first().unwrap();
-    let (finalized_root, _) = harness.honest_roots.last().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;

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<String>,
     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
              */