Add new fork choice struct to beacon chain

beacon_node/beacon_chain/Cargo.toml

@@ -11,7 +11,6 @@ store = { path = "../store" }
 failure = "0.1"
 failure_derive = "0.1"
 hashing = { path = "../../eth2/utils/hashing" }
-fork_choice = { path = "../../eth2/fork_choice" }
 parking_lot = "0.7"
 prometheus = "^0.6"
 log = "0.4"
@@ -26,3 +25,4 @@ ssz_derive = { path = "../../eth2/utils/ssz_derive" }
 state_processing = { path = "../../eth2/state_processing" }
 tree_hash = { path = "../../eth2/utils/tree_hash" }
 types = { path = "../../eth2/types" }
+lmd_ghost = { path = "../../eth2/lmd_ghost" }

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -1,9 +1,10 @@
 use crate::checkpoint::CheckPoint;
 use crate::errors::{BeaconChainError as Error, BlockProductionError};
+use crate::fork_choice::{Error as ForkChoiceError, ForkChoice};
 use crate::iter::{BlockIterator, BlockRootsIterator};
 use crate::metrics::Metrics;
 use crate::persisted_beacon_chain::{PersistedBeaconChain, BEACON_CHAIN_DB_KEY};
-use fork_choice::{ForkChoice, ForkChoiceError};
+use lmd_ghost::LmdGhost;
 use log::{debug, trace};
 use operation_pool::DepositInsertStatus;
 use operation_pool::OperationPool;
@@ -48,7 +49,7 @@ pub enum BlockProcessingOutcome {
 pub trait BeaconChainTypes {
     type Store: store::Store;
     type SlotClock: slot_clock::SlotClock;
-    type ForkChoice: fork_choice::ForkChoice<Self::Store>;
+    type LmdGhost: LmdGhost<Self::Store, Self::EthSpec>;
     type EthSpec: types::EthSpec;
 }
 
@@ -73,7 +74,7 @@ pub struct BeaconChain<T: BeaconChainTypes> {
     genesis_block_root: Hash256,
     /// A state-machine that is updated with information from the network and chooses a canonical
     /// head block.
-    pub fork_choice: RwLock<T::ForkChoice>,
+    pub fork_choice: ForkChoice<T::LmdGhost, T::Store, T::EthSpec>,
     /// Stores metrics about this `BeaconChain`.
     pub metrics: Metrics,
 }
@@ -86,7 +87,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         mut genesis_state: BeaconState<T::EthSpec>,
         genesis_block: BeaconBlock,
         spec: ChainSpec,
-        fork_choice: T::ForkChoice,
+        fork_choice: ForkChoice<T::LmdGhost, T::Store, T::EthSpec>,
     ) -> Result<Self, Error> {
         let state_root = genesis_state.canonical_root();
         store.put(&state_root, &genesis_state)?;
@@ -115,7 +116,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             state: RwLock::new(genesis_state),
             canonical_head,
             genesis_block_root,
-            fork_choice: RwLock::new(fork_choice),
+            fork_choice,
             metrics: Metrics::new()?,
         })
     }
@@ -138,18 +139,19 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             spec.seconds_per_slot,
         );
 
-        let fork_choice = T::ForkChoice::new(store.clone());
+        // let fork_choice = T::ForkChoice::new(store.clone());
+        // let fork_choice: ForkChoice<T::LmdGhost, T::EthSpec> = ForkChoice::new(store.clone());
 
         Ok(Some(BeaconChain {
             spec,
-            store,
             slot_clock,
             op_pool: OperationPool::default(),
             canonical_head: RwLock::new(p.canonical_head),
             state: RwLock::new(p.state),
-            fork_choice: RwLock::new(fork_choice),
+            fork_choice: ForkChoice::new(store.clone()),
             genesis_block_root: p.genesis_block_root,
             metrics: Metrics::new()?,
+            store,
         }))
     }
 
@@ -613,9 +615,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         self.store.put(&state_root, &state)?;
 
         // Register the new block with the fork choice service.
-        self.fork_choice
+        self.fork_choice.process_block(&state, &block)?;
-            .write()
-            .add_block(&block, &block_root, &self.spec)?;
 
         // Execute the fork choice algorithm, enthroning a new head if discovered.
         //
@@ -713,20 +713,8 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         // Start fork choice metrics timer.
         let timer = self.metrics.fork_choice_times.start_timer();
 
-        let justified_root = {
-            let root = self.head().beacon_state.current_justified_root;
-            if root == self.spec.zero_hash {
-                self.genesis_block_root
-            } else {
-                root
-            }
-        };
-
         // Determine the root of the block that is the head of the chain.
-        let beacon_block_root = self
+        let beacon_block_root = self.fork_choice.find_head()?;
-            .fork_choice
-            .write()
-            .find_head(&justified_root, &self.spec)?;
 
         // End fork choice metrics timer.
         timer.observe_duration();

beacon_node/beacon_chain/src/errors.rs

@@ -1,5 +1,5 @@
+use crate::fork_choice::Error as ForkChoiceError;
 use crate::metrics::Error as MetricsError;
-use fork_choice::ForkChoiceError;
 use state_processing::BlockProcessingError;
 use state_processing::SlotProcessingError;
 use types::*;

beacon_node/beacon_chain/src/fork_choice.rs

@@ -0,0 +1,98 @@
+use crate::BeaconChain;
+use lmd_ghost::LmdGhost;
+use state_processing::common::get_attesting_indices_unsorted;
+use std::marker::PhantomData;
+use std::sync::Arc;
+use store::Store;
+use types::{Attestation, BeaconBlock, BeaconState, BeaconStateError, EthSpec, Hash256};
+
+type Result<T> = std::result::Result<T, Error>;
+
+#[derive(Debug, PartialEq)]
+pub enum Error {
+    BackendError(String),
+    BeaconStateError(BeaconStateError),
+}
+
+pub struct ForkChoice<L, S, E> {
+    backend: L,
+    _phantom_a: PhantomData<S>,
+    _phantom_b: PhantomData<E>,
+}
+
+impl<L, S, E> ForkChoice<L, S, E>
+where
+    L: LmdGhost<S, E>,
+    S: Store,
+    E: EthSpec,
+{
+    pub fn new(store: Arc<S>) -> Self {
+        Self {
+            backend: L::new(store),
+            _phantom_a: PhantomData,
+            _phantom_b: PhantomData,
+        }
+    }
+
+    pub fn find_head(&self) -> Result<Hash256> {
+        self.backend.find_head().map_err(Into::into)
+    }
+
+    pub fn process_attestation(
+        &self,
+        state: &BeaconState<E>,
+        attestation: &Attestation,
+    ) -> Result<()> {
+        // Note: `get_attesting_indices_unsorted` requires that the beacon state caches be built.
+        let validator_indices = get_attesting_indices_unsorted(
+            state,
+            &attestation.data,
+            &attestation.aggregation_bitfield,
+        )?;
+
+        let block_hash = attestation.data.target_root;
+
+        // TODO: what happens when the target root is not the same slot as the block?
+        let block_slot = attestation
+            .data
+            .target_epoch
+            .start_slot(E::slots_per_epoch());
+
+        for validator_index in validator_indices {
+            self.backend
+                .process_message(validator_index, block_hash, block_slot)?;
+        }
+
+        Ok(())
+    }
+
+    /// A helper function which runs `self.process_attestation` on all `Attestation` in the given `BeaconBlock`.
+    ///
+    /// Assumes the block (and therefore it's attestations) are valid. It is a logic error to
+    /// provide an invalid block.
+    pub fn process_block(&self, state: &BeaconState<E>, block: &BeaconBlock) -> Result<()> {
+        // Note: we never count the block as a latest message, only attestations.
+        //
+        // I (Paul H) do not have an explicit reference to this, however I derive it from this
+        // document:
+        //
+        // https://github.com/ethereum/eth2.0-specs/blob/v0.7.0/specs/core/0_fork-choice.md
+        for attestation in &block.body.attestations {
+            self.process_attestation(state, attestation)?;
+        }
+
+        Ok(())
+    }
+}
+
+impl From<BeaconStateError> for Error {
+    fn from(e: BeaconStateError) -> Error {
+        Error::BeaconStateError(e)
+    }
+}
+
+impl From<String> for Error {
+    fn from(e: String) -> Error {
+        Error::BackendError(e)
+    }
+}

beacon_node/beacon_chain/src/lib.rs

@@ -1,6 +1,7 @@
 mod beacon_chain;
 mod checkpoint;
 mod errors;
+mod fork_choice;
 pub mod iter;
 mod metrics;
 mod persisted_beacon_chain;
@@ -8,7 +9,6 @@ mod persisted_beacon_chain;
 pub use self::beacon_chain::{BeaconChain, BeaconChainTypes, BlockProcessingOutcome};
 pub use self::checkpoint::CheckPoint;
 pub use self::errors::{BeaconChainError, BlockProductionError};
-pub use fork_choice;
 pub use parking_lot;
 pub use slot_clock;
 pub use state_processing::per_block_processing::errors::{

eth2/lmd_ghost/src/lib.rs

@@ -1,20 +1,15 @@
-pub mod reduced_tree;
+mod reduced_tree;
 
-use state_processing::common::get_attesting_indices_unsorted;
-use std::marker::PhantomData;
 use std::sync::Arc;
-use types::{Attestation, BeaconBlock, BeaconState, BeaconStateError, EthSpec, Hash256, Slot};
+use store::Store;
+use types::{EthSpec, Hash256, Slot};
 
-type Result<T> = std::result::Result<T, Error>;
+pub use reduced_tree::ThreadSafeReducedTree;
 
-#[derive(Debug, PartialEq)]
+pub type Result<T> = std::result::Result<T, String>;
-pub enum Error {
-    BackendError(String),
-    BeaconStateError(BeaconStateError),
-}
 
-pub trait LmdGhostBackend<T, E: EthSpec>: Send + Sync {
+pub trait LmdGhost<S: Store, E: EthSpec>: Send + Sync {
-    fn new(store: Arc<T>) -> Self;
+    fn new(store: Arc<S>) -> Self;
 
     fn process_message(
         &self,
@@ -25,75 +20,3 @@ pub trait LmdGhostBackend<T, E: EthSpec>: Send + Sync {
 
     fn find_head(&self) -> Result<Hash256>;
 }
-
-pub struct ForkChoice<T, E> {
-    backend: T,
-    _phantom: PhantomData<E>,
-}
-
-impl<T, E> ForkChoice<T, E>
-where
-    T: LmdGhostBackend<T, E>,
-    E: EthSpec,
-{
-    pub fn new(store: Arc<T>) -> Self {
-        Self {
-            backend: T::new(store),
-            _phantom: PhantomData,
-        }
-    }
-
-    pub fn find_head(&self) -> Result<Hash256> {
-        self.backend.find_head()
-    }
-
-    pub fn process_attestation(
-        &self,
-        state: &BeaconState<E>,
-        attestation: &Attestation,
-    ) -> Result<()> {
-        let validator_indices = get_attesting_indices_unsorted(
-            state,
-            &attestation.data,
-            &attestation.aggregation_bitfield,
-        )?;
-
-        let block_hash = attestation.data.target_root;
-
-        // TODO: what happens when the target root is not the same slot as the block?
-        let block_slot = attestation
-            .data
-            .target_epoch
-            .start_slot(E::slots_per_epoch());
-
-        for validator_index in validator_indices {
-            self.backend
-                .process_message(validator_index, block_hash, block_slot)?;
-        }
-
-        Ok(())
-    }
-
-    pub fn process_block(
-        &self,
-        state: &BeaconState<E>,
-        block: &BeaconBlock,
-        block_hash: Hash256,
-        block_proposer: usize,
-    ) -> Result<()> {
-        self.backend
-            .process_message(block_proposer, block_hash, block.slot)?;
-
-        for attestation in &block.body.attestations {
-            self.process_attestation(state, attestation)?;
-        }
-
-        Ok(())
-    }
-}
-
-impl From<BeaconStateError> for Error {
-    fn from(e: BeaconStateError) -> Error {
-        Error::BeaconStateError(e)
-    }
-}

eth2/lmd_ghost/src/reduced_tree.rs

@@ -1,4 +1,4 @@
-use super::{Error as SuperError, LmdGhostBackend};
+use super::{LmdGhost, Result as SuperResult};
 use parking_lot::RwLock;
 use std::collections::HashMap;
 use std::marker::PhantomData;
@@ -8,8 +8,6 @@ use types::{BeaconBlock, BeaconState, EthSpec, Hash256, Slot};
 
 type Result<T> = std::result::Result<T, Error>;
 
-pub const SKIP_LIST_LEN: usize = 16;
-
 #[derive(Debug, PartialEq)]
 pub enum Error {
     MissingNode(Hash256),
@@ -51,10 +49,6 @@ impl Node {
     pub fn has_votes(&self) -> bool {
         !self.voters.is_empty()
     }
-
-    pub fn is_genesis(&self) -> bool {
-        self.parent_hash.is_some()
-    }
 }
 
 impl Node {
@@ -69,7 +63,7 @@ pub struct Vote {
     slot: Slot,
 }
 
-impl<T, E> LmdGhostBackend<T, E> for ThreadSafeReducedTree<T, E>
+impl<T, E> LmdGhost<T, E> for ThreadSafeReducedTree<T, E>
 where
     T: Store,
     E: EthSpec,
@@ -85,21 +79,21 @@ where
         validator_index: usize,
         block_hash: Hash256,
         block_slot: Slot,
-    ) -> std::result::Result<(), SuperError> {
+    ) -> SuperResult<()> {
         self.core
             .write()
             .process_message(validator_index, block_hash, block_slot)
             .map_err(Into::into)
     }
 
-    fn find_head(&self) -> std::result::Result<Hash256, SuperError> {
+    fn find_head(&self) -> SuperResult<Hash256> {
         unimplemented!();
     }
 }
 
-impl From<Error> for SuperError {
+impl From<Error> for String {
-    fn from(e: Error) -> SuperError {
+    fn from(e: Error) -> String {
-        SuperError::BackendError(format!("{:?}", e))
+        format!("{:?}", e)
     }
 }
 
@@ -415,11 +409,6 @@ where
         &self.0[i]
     }
 
-    pub fn get_mut(&mut self, i: usize) -> &mut T {
-        self.ensure(i);
-        &mut self.0[i]
-    }
-
     pub fn insert(&mut self, i: usize, element: T) {
         self.ensure(i);
         self.0[i] = element;