Refactor winning root logic

eth2/state_processing/src/macros.rs

@@ -11,3 +11,14 @@ macro_rules! invalid {
         return Err(Error::Invalid($result));
     };
 }
+
+macro_rules! safe_add_assign {
+    ($a: expr, $b: expr) => {
+        $a = $a.saturating_add($b);
+    };
+}
+macro_rules! safe_sub_assign {
+    ($a: expr, $b: expr) => {
+        $a = $a.saturating_sub($b);
+    };
+}

eth2/state_processing/src/per_epoch_processing.rs

@@ -1,7 +1,8 @@
-use errors::{EpochProcessingError as Error, WinningRootError};
+use attester_sets::AttesterSets;
-use grouped_attesters::GroupedAttesters;
+use errors::EpochProcessingError as Error;
+use inclusion_distance::{inclusion_distance, inclusion_slot};
 use integer_sqrt::IntegerSquareRoot;
-use log::{debug, trace};
+use log::debug;
 use rayon::prelude::*;
 use ssz::TreeHash;
 use std::collections::{HashMap, HashSet};
@@ -9,21 +10,11 @@ use std::iter::FromIterator;
 use types::{validator_registry::get_active_validator_indices, *};
 use winning_root::{winning_root, WinningRoot};
 
+pub mod attester_sets;
 pub mod errors;
-mod grouped_attesters;
+pub mod inclusion_distance;
-mod tests;
+pub mod tests;
-mod winning_root;
+pub mod winning_root;
-
-macro_rules! safe_add_assign {
-    ($a: expr, $b: expr) => {
-        $a = $a.saturating_add($b);
-    };
-}
-macro_rules! safe_sub_assign {
-    ($a: expr, $b: expr) => {
-        $a = $a.saturating_sub($b);
-    };
-}
 
 pub fn per_epoch_processing(state: &mut BeaconState, spec: &ChainSpec) -> Result<(), Error> {
     let current_epoch = state.current_epoch(spec);
@@ -40,7 +31,7 @@ pub fn per_epoch_processing(state: &mut BeaconState, spec: &ChainSpec) -> Result
     state.build_epoch_cache(RelativeEpoch::Current, spec)?;
     state.build_epoch_cache(RelativeEpoch::Next, spec)?;
 
-    let attesters = GroupedAttesters::new(&state, spec)?;
+    let attesters = AttesterSets::new(&state, spec)?;
 
     let active_validator_indices = get_active_validator_indices(
         &state.validator_registry,
@@ -86,12 +77,14 @@ pub fn per_epoch_processing(state: &mut BeaconState, spec: &ChainSpec) -> Result
     process_validator_registry(state, spec)?;
 
     // Final updates
-    state.latest_active_index_roots[(next_epoch.as_usize()
+    let active_tree_root = get_active_validator_indices(
-        + spec.activation_exit_delay as usize)
-        % spec.latest_active_index_roots_length] = hash_tree_root(get_active_validator_indices(
         &state.validator_registry,
         next_epoch + Epoch::from(spec.activation_exit_delay),
-    ));
+    )
+    .hash_tree_root();
+    state.latest_active_index_roots[(next_epoch.as_usize()
+        + spec.activation_exit_delay as usize)
+        % spec.latest_active_index_roots_length] = Hash256::from(&active_tree_root[..]);
 
     state.latest_slashed_balances[next_epoch.as_usize() % spec.latest_slashed_exit_length] =
         state.latest_slashed_balances[current_epoch.as_usize() % spec.latest_slashed_exit_length];
@@ -114,10 +107,6 @@ pub fn per_epoch_processing(state: &mut BeaconState, spec: &ChainSpec) -> Result
     Ok(())
 }
 
-fn hash_tree_root<T: TreeHash>(input: Vec<T>) -> Hash256 {
-    Hash256::from(&input.hash_tree_root()[..])
-}
-
 /// Spec v0.4.0
 fn process_eth1_data(state: &mut BeaconState, spec: &ChainSpec) {
     let next_epoch = state.next_epoch(spec);
@@ -210,7 +199,7 @@ fn process_justification(
     state.justified_epoch = new_justified_epoch;
 }
 
-pub type WinningRootHashSet = HashMap<u64, Result<WinningRoot, WinningRootError>>;
+pub type WinningRootHashSet = HashMap<u64, WinningRoot>;
 
 fn process_crosslinks(
     state: &mut BeaconState,
@@ -219,33 +208,25 @@ fn process_crosslinks(
     let current_epoch_attestations: Vec<&PendingAttestation> = state
         .latest_attestations
         .par_iter()
-        .filter(|a| {
+        .filter(|a| a.data.slot.epoch(spec.slots_per_epoch) == state.current_epoch(spec))
-            (a.data.slot / spec.slots_per_epoch).epoch(spec.slots_per_epoch)
-                == state.current_epoch(spec)
-        })
         .collect();
 
     let previous_epoch_attestations: Vec<&PendingAttestation> = state
         .latest_attestations
         .par_iter()
-        .filter(|a| {
+        .filter(|a| a.data.slot.epoch(spec.slots_per_epoch) == state.previous_epoch(spec))
-            (a.data.slot / spec.slots_per_epoch).epoch(spec.slots_per_epoch)
-                == state.previous_epoch(spec)
-        })
         .collect();
 
-    let mut winning_root_for_shards: HashMap<u64, Result<WinningRoot, WinningRootError>> =
+    let mut winning_root_for_shards: WinningRootHashSet = HashMap::new();
-        HashMap::new();
-    // for slot in state.slot.saturating_sub(2 * spec.slots_per_epoch)..state.slot {
-    for slot in state.previous_epoch(spec).slot_iter(spec.slots_per_epoch) {
-        trace!(
-            "Finding winning root for slot: {} (epoch: {})",
-            slot,
-            slot.epoch(spec.slots_per_epoch)
-        );
 
-        // Clone is used to remove the borrow. It becomes an issue later when trying to mutate
+    let previous_and_current_epoch_slots: Vec<Slot> = state
-        // `state.balances`.
+        .previous_epoch(spec)
+        .slot_iter(spec.slots_per_epoch)
+        .chain(state.current_epoch(spec).slot_iter(spec.slots_per_epoch))
+        .collect();
+
+    for slot in previous_and_current_epoch_slots {
+        // Clone removes the borrow which becomes an issue when mutating `state.balances`.
         let crosslink_committees_at_slot =
             state.get_crosslink_committees_at_slot(slot, spec)?.clone();
 
@@ -258,31 +239,33 @@ fn process_crosslinks(
                 &current_epoch_attestations[..],
                 &previous_epoch_attestations[..],
                 spec,
-            );
+            )?;
 
-            if let Ok(winning_root) = &winning_root {
+            if let Some(winning_root) = winning_root {
                 let total_committee_balance = state.get_total_balance(&crosslink_committee, spec);
 
+                // TODO: I think this has a bug.
                 if (3 * winning_root.total_attesting_balance) >= (2 * total_committee_balance) {
                     state.latest_crosslinks[shard as usize] = Crosslink {
                         epoch: state.current_epoch(spec),
                         crosslink_data_root: winning_root.crosslink_data_root,
                     }
                 }
+                winning_root_for_shards.insert(shard, winning_root);
             }
-            winning_root_for_shards.insert(shard, winning_root);
         }
     }
 
     Ok(winning_root_for_shards)
 }
 
+/// Spec v0.4.0
 fn process_rewards_and_penalities(
     state: &mut BeaconState,
     active_validator_indices: HashSet<usize>,
-    attesters: &GroupedAttesters,
+    attesters: &AttesterSets,
     previous_total_balance: u64,
-    winning_root_for_shards: &HashMap<u64, Result<WinningRoot, WinningRootError>>,
+    winning_root_for_shards: &WinningRootHashSet,
     spec: &ChainSpec,
 ) -> Result<(), Error> {
     let next_epoch = state.next_epoch(spec);
@@ -290,62 +273,60 @@ fn process_rewards_and_penalities(
     let previous_epoch_attestations: Vec<&PendingAttestation> = state
         .latest_attestations
         .par_iter()
-        .filter(|a| {
+        .filter(|a| a.data.slot.epoch(spec.slots_per_epoch) == state.previous_epoch(spec))
-            (a.data.slot / spec.slots_per_epoch).epoch(spec.slots_per_epoch)
-                == state.previous_epoch(spec)
-        })
         .collect();
 
     let base_reward_quotient = previous_total_balance.integer_sqrt() / spec.base_reward_quotient;
+
     if base_reward_quotient == 0 {
         return Err(Error::BaseRewardQuotientIsZero);
     }
 
-    /*
+    // Justification and finalization
-     * Justification and finalization
-     */
-    let epochs_since_finality = next_epoch - state.finalized_epoch;
 
-    let active_validator_indices_hashset: HashSet<usize> =
+    let epochs_since_finality = next_epoch - state.finalized_epoch;
-        HashSet::from_iter(active_validator_indices.iter().cloned());
 
     if epochs_since_finality <= 4 {
         for index in 0..state.validator_balances.len() {
             let base_reward = state.base_reward(index, base_reward_quotient, spec);
 
+            // Expected FFG source
             if attesters.previous_epoch.indices.contains(&index) {
                 safe_add_assign!(
                     state.validator_balances[index],
                     base_reward * attesters.previous_epoch.balance / previous_total_balance
                 );
-            } else if active_validator_indices_hashset.contains(&index) {
+            } else if active_validator_indices.contains(&index) {
                 safe_sub_assign!(state.validator_balances[index], base_reward);
             }
 
+            // Expected FFG target
             if attesters.previous_epoch_boundary.indices.contains(&index) {
                 safe_add_assign!(
                     state.validator_balances[index],
                     base_reward * attesters.previous_epoch_boundary.balance
                         / previous_total_balance
                 );
-            } else if active_validator_indices_hashset.contains(&index) {
+            } else if active_validator_indices.contains(&index) {
                 safe_sub_assign!(state.validator_balances[index], base_reward);
             }
 
+            // Expected beacon chain head
             if attesters.previous_epoch_head.indices.contains(&index) {
                 safe_add_assign!(
                     state.validator_balances[index],
                     base_reward * attesters.previous_epoch_head.balance / previous_total_balance
                 );
-            } else if active_validator_indices_hashset.contains(&index) {
+            } else if active_validator_indices.contains(&index) {
                 safe_sub_assign!(state.validator_balances[index], base_reward);
             }
         }
 
+        // Inclusion distance
         for &index in &attesters.previous_epoch.indices {
             let base_reward = state.base_reward(index, base_reward_quotient, spec);
             let inclusion_distance =
-                state.inclusion_distance(&previous_epoch_attestations, index, spec)?;
+                inclusion_distance(state, &previous_epoch_attestations, index, spec)?;
 
             safe_add_assign!(
                 state.validator_balances[index],
@@ -356,7 +337,8 @@ fn process_rewards_and_penalities(
         for index in 0..state.validator_balances.len() {
             let inactivity_penalty =
                 state.inactivity_penalty(index, epochs_since_finality, base_reward_quotient, spec);
-            if active_validator_indices_hashset.contains(&index) {
+
+            if active_validator_indices.contains(&index) {
                 if !attesters.previous_epoch.indices.contains(&index) {
                     safe_sub_assign!(state.validator_balances[index], inactivity_penalty);
                 }
@@ -380,7 +362,7 @@ fn process_rewards_and_penalities(
         for &index in &attesters.previous_epoch.indices {
             let base_reward = state.base_reward(index, base_reward_quotient, spec);
             let inclusion_distance =
-                state.inclusion_distance(&previous_epoch_attestations, index, spec)?;
+                inclusion_distance(state, &previous_epoch_attestations, index, spec)?;
 
             safe_sub_assign!(
                 state.validator_balances[index],
@@ -390,61 +372,69 @@ fn process_rewards_and_penalities(
         }
     }
 
-    trace!("Processed validator justification and finalization rewards/penalities.");
+    // Attestation inclusion
 
-    /*
-     * Attestation inclusion
-     */
     for &index in &attesters.previous_epoch.indices {
-        let inclusion_slot = state.inclusion_slot(&previous_epoch_attestations[..], index, spec)?;
+        let inclusion_slot = inclusion_slot(state, &previous_epoch_attestations[..], index, spec)?;
+
         let proposer_index = state
             .get_beacon_proposer_index(inclusion_slot, spec)
             .map_err(|_| Error::UnableToDetermineProducer)?;
+
         let base_reward = state.base_reward(proposer_index, base_reward_quotient, spec);
+
         safe_add_assign!(
             state.validator_balances[proposer_index],
             base_reward / spec.attestation_inclusion_reward_quotient
         );
     }
 
-    /*
+    //Crosslinks
-     * Crosslinks
+
-     */
     for slot in state.previous_epoch(spec).slot_iter(spec.slots_per_epoch) {
-        // Clone is used to remove the borrow. It becomes an issue later when trying to mutate
+        // Clone removes the borrow which becomes an issue when mutating `state.balances`.
-        // `state.balances`.
         let crosslink_committees_at_slot =
             state.get_crosslink_committees_at_slot(slot, spec)?.clone();
 
-        for (_crosslink_committee, shard) in crosslink_committees_at_slot {
+        for (crosslink_committee, shard) in crosslink_committees_at_slot {
             let shard = shard as u64;
 
-            if let Some(Ok(winning_root)) = winning_root_for_shards.get(&shard) {
+            // Note: I'm a little uncertain of the logic here -- I am waiting for spec v0.5.0 to
-                // TODO: remove the map.
+            // clear it up.
+            //
+            // What happens here is:
+            //
+            // - If there was some crosslink root elected by the super-majority of this committee,
+            // then we reward all who voted for that root and penalize all that did not.
+            // - However, if there _was not_ some super-majority-voted crosslink root, then penalize
+            // all the validators.
+            //
+            // I'm not quite sure that the second case (no super-majority crosslink) is correct.
+            if let Some(winning_root) = winning_root_for_shards.get(&shard) {
+                // Hash set de-dedups and (hopefully) offers a speed improvement from faster
+                // lookups.
                 let attesting_validator_indices: HashSet<usize> =
                     HashSet::from_iter(winning_root.attesting_validator_indices.iter().cloned());
 
-                for index in 0..state.validator_balances.len() {
+                for &index in &crosslink_committee {
                     let base_reward = state.base_reward(index, base_reward_quotient, spec);
 
+                    let total_balance = state.get_total_balance(&crosslink_committee, spec);
+
                     if attesting_validator_indices.contains(&index) {
                         safe_add_assign!(
                             state.validator_balances[index],
-                            base_reward * winning_root.total_attesting_balance
+                            base_reward * winning_root.total_attesting_balance / total_balance
-                                / winning_root.total_balance
                         );
                     } else {
                         safe_sub_assign!(state.validator_balances[index], base_reward);
                     }
                 }
+            } else {
+                for &index in &crosslink_committee {
+                    let base_reward = state.base_reward(index, base_reward_quotient, spec);
 
-                for index in &winning_root.attesting_validator_indices {
+                    safe_sub_assign!(state.validator_balances[index], base_reward);
-                    let base_reward = state.base_reward(*index, base_reward_quotient, spec);
-                    safe_add_assign!(
-                        state.validator_balances[*index],
-                        base_reward * winning_root.total_attesting_balance
-                            / winning_root.total_balance
-                    );
                 }
             }
         }
@@ -453,6 +443,7 @@ fn process_rewards_and_penalities(
     Ok(())
 }
 
+// Spec v0.4.0
 fn process_validator_registry(state: &mut BeaconState, spec: &ChainSpec) -> Result<(), Error> {
     let current_epoch = state.current_epoch(spec);
     let next_epoch = state.next_epoch(spec);
@@ -460,11 +451,6 @@ fn process_validator_registry(state: &mut BeaconState, spec: &ChainSpec) -> Resu
     state.previous_shuffling_epoch = state.current_shuffling_epoch;
     state.previous_shuffling_start_shard = state.current_shuffling_start_shard;
 
-    debug!(
-        "setting previous_shuffling_seed to : {}",
-        state.current_shuffling_seed
-    );
-
     state.previous_shuffling_seed = state.current_shuffling_seed;
 
     let should_update_validator_registy = if state.finalized_epoch
@@ -479,7 +465,6 @@ fn process_validator_registry(state: &mut BeaconState, spec: &ChainSpec) -> Resu
     };
 
     if should_update_validator_registy {
-        trace!("updating validator registry.");
         state.update_validator_registry(spec);
 
         state.current_shuffling_epoch = next_epoch;
@@ -488,7 +473,6 @@ fn process_validator_registry(state: &mut BeaconState, spec: &ChainSpec) -> Resu
             % spec.shard_count;
         state.current_shuffling_seed = state.generate_seed(state.current_shuffling_epoch, spec)?
     } else {
-        trace!("not updating validator registry.");
         let epochs_since_last_registry_update =
             current_epoch - state.validator_registry_update_epoch;
         if (epochs_since_last_registry_update > 1)

eth2/state_processing/src/per_epoch_processing/attester_sets.rs

@@ -17,7 +17,7 @@ impl Attesters {
     }
 }
 
-pub struct GroupedAttesters {
+pub struct AttesterSets {
     pub current_epoch: Attesters,
     pub current_epoch_boundary: Attesters,
     pub previous_epoch: Attesters,
@@ -25,7 +25,7 @@ pub struct GroupedAttesters {
     pub previous_epoch_head: Attesters,
 }
 
-impl GroupedAttesters {
+impl AttesterSets {
     pub fn new(state: &BeaconState, spec: &ChainSpec) -> Result<Self, BeaconStateError> {
         let mut current_epoch = Attesters::default();
         let mut current_epoch_boundary = Attesters::default();

eth2/state_processing/src/per_epoch_processing/errors.rs

@@ -1,11 +1,5 @@
 use types::*;
 
-#[derive(Debug, PartialEq)]
-pub enum WinningRootError {
-    NoWinningRoot,
-    BeaconStateError(BeaconStateError),
-}
-
 #[derive(Debug, PartialEq)]
 pub enum EpochProcessingError {
     UnableToDetermineProducer,
@@ -14,7 +8,6 @@ pub enum EpochProcessingError {
     NoRandaoSeed,
     BeaconStateError(BeaconStateError),
     InclusionError(InclusionError),
-    WinningRootError(WinningRootError),
 }
 
 impl From<InclusionError> for EpochProcessingError {
@@ -29,8 +22,15 @@ impl From<BeaconStateError> for EpochProcessingError {
     }
 }
 
-impl From<BeaconStateError> for WinningRootError {
+#[derive(Debug, PartialEq)]
-    fn from(e: BeaconStateError) -> WinningRootError {
+pub enum InclusionError {
-        WinningRootError::BeaconStateError(e)
+    /// The validator did not participate in an attestation in this period.
+    NoAttestationsForValidator,
+    BeaconStateError(BeaconStateError),
+}
+
+impl From<BeaconStateError> for InclusionError {
+    fn from(e: BeaconStateError) -> InclusionError {
+        InclusionError::BeaconStateError(e)
     }
 }

eth2/state_processing/src/per_epoch_processing/inclusion_distance.rs

@@ -0,0 +1,61 @@
+use super::errors::InclusionError;
+use types::*;
+
+/// Returns the distance between the first included attestation for some validator and this
+/// slot.
+///
+/// Note: In the spec this is defined "inline", not as a helper function.
+///
+/// Spec v0.4.0
+pub fn inclusion_distance(
+    state: &BeaconState,
+    attestations: &[&PendingAttestation],
+    validator_index: usize,
+    spec: &ChainSpec,
+) -> Result<u64, InclusionError> {
+    let attestation = earliest_included_attestation(state, attestations, validator_index, spec)?;
+    Ok((attestation.inclusion_slot - attestation.data.slot).as_u64())
+}
+
+/// Returns the slot of the earliest included attestation for some validator.
+///
+/// Note: In the spec this is defined "inline", not as a helper function.
+///
+/// Spec v0.4.0
+pub fn inclusion_slot(
+    state: &BeaconState,
+    attestations: &[&PendingAttestation],
+    validator_index: usize,
+    spec: &ChainSpec,
+) -> Result<Slot, InclusionError> {
+    let attestation = earliest_included_attestation(state, attestations, validator_index, spec)?;
+    Ok(attestation.inclusion_slot)
+}
+
+/// Finds the earliest included attestation for some validator.
+///
+/// Note: In the spec this is defined "inline", not as a helper function.
+///
+/// Spec v0.4.0
+fn earliest_included_attestation(
+    state: &BeaconState,
+    attestations: &[&PendingAttestation],
+    validator_index: usize,
+    spec: &ChainSpec,
+) -> Result<PendingAttestation, InclusionError> {
+    let mut included_attestations = vec![];
+
+    for (i, a) in attestations.iter().enumerate() {
+        let participants =
+            state.get_attestation_participants(&a.data, &a.aggregation_bitfield, spec)?;
+        if participants.iter().any(|i| *i == validator_index) {
+            included_attestations.push(i);
+        }
+    }
+
+    let earliest_attestation_index = included_attestations
+        .iter()
+        .min_by_key(|i| attestations[**i].inclusion_slot)
+        .ok_or_else(|| InclusionError::NoAttestationsForValidator)?;
+    Ok(attestations[*earliest_attestation_index].clone())
+}

eth2/state_processing/src/per_epoch_processing/winning_root.rs

@@ -1,86 +1,118 @@
-use super::errors::WinningRootError;
+use std::collections::HashSet;
-use std::collections::HashMap;
+use std::iter::FromIterator;
 use types::*;
 
 #[derive(Clone)]
 pub struct WinningRoot {
     pub crosslink_data_root: Hash256,
     pub attesting_validator_indices: Vec<usize>,
-    pub total_balance: u64,
     pub total_attesting_balance: u64,
 }
 
+impl WinningRoot {
+    /// Returns `true` if `self` is a "better" candidate than `other`.
+    ///
+    /// A winning root is "better" than another if it has a higher `total_attesting_balance`. Ties
+    /// are broken by favouring the lower `crosslink_data_root` value.
+    ///
+    /// Spec v0.4.0
+    pub fn is_better_than(&self, other: &Self) -> bool {
+        if self.total_attesting_balance > other.total_attesting_balance {
+            true
+        } else if self.total_attesting_balance == other.total_attesting_balance {
+            self.crosslink_data_root < other.crosslink_data_root
+        } else {
+            false
+        }
+    }
+}
+
+/// Returns the `crosslink_data_root` with the highest total attesting balance for the given shard.
+/// Breaks ties by favouring the smaller `crosslink_data_root` hash.
+///
+/// The `WinningRoot` object also contains additional fields that are useful in later stages of
+/// per-epoch processing.
+///
+/// Spec v0.4.0
 pub fn winning_root(
     state: &BeaconState,
     shard: u64,
     current_epoch_attestations: &[&PendingAttestation],
     previous_epoch_attestations: &[&PendingAttestation],
     spec: &ChainSpec,
-) -> Result<WinningRoot, WinningRootError> {
+) -> Result<Option<WinningRoot>, BeaconStateError> {
-    let mut attestations = current_epoch_attestations.to_vec();
+    let mut winning_root: Option<WinningRoot> = None;
-    attestations.append(&mut previous_epoch_attestations.to_vec());
 
-    let mut candidates: HashMap<Hash256, WinningRoot> = HashMap::new();
+    let crosslink_data_roots: HashSet<Hash256> = HashSet::from_iter(
-
+        previous_epoch_attestations
-    let mut highest_seen_balance = 0;
-
-    for a in &attestations {
-        if a.data.shard != shard {
-            continue;
-        }
-
-        let crosslink_data_root = &a.data.crosslink_data_root;
-
-        if candidates.contains_key(crosslink_data_root) {
-            continue;
-        }
-
-        let attesting_validator_indices = attestations
             .iter()
-            .try_fold::<_, _, Result<_, BeaconStateError>>(vec![], |mut acc, a| {
+            .chain(current_epoch_attestations.iter())
-                if (a.data.shard == shard) && (a.data.crosslink_data_root == *crosslink_data_root) {
+            .filter_map(|a| {
-                    acc.append(&mut state.get_attestation_participants(
+                if a.data.shard == shard {
-                        &a.data,
+                    Some(a.data.crosslink_data_root)
-                        &a.aggregation_bitfield,
+                } else {
-                        spec,
+                    None
-                    )?);
                 }
-                Ok(acc)
+            }),
-            })?;
+    );
 
-        let total_balance: u64 = attesting_validator_indices
+    for crosslink_data_root in crosslink_data_roots {
-            .iter()
+        let attesting_validator_indices = get_attesting_validator_indices(
-            .fold(0, |acc, i| acc + state.get_effective_balance(*i, spec));
+            state,
+            shard,
+            current_epoch_attestations,
+            previous_epoch_attestations,
+            &crosslink_data_root,
+            spec,
+        )?;
 
         let total_attesting_balance: u64 = attesting_validator_indices
             .iter()
             .fold(0, |acc, i| acc + state.get_effective_balance(*i, spec));
 
-        if total_attesting_balance > highest_seen_balance {
+        let candidate = WinningRoot {
-            highest_seen_balance = total_attesting_balance;
+            crosslink_data_root,
-        }
-
-        let candidate_root = WinningRoot {
-            crosslink_data_root: *crosslink_data_root,
             attesting_validator_indices,
             total_attesting_balance,
-            total_balance,
         };
 
-        candidates.insert(*crosslink_data_root, candidate_root);
+        if let Some(ref winner) = winning_root {
+            if candidate.is_better_than(&winner) {
+                winning_root = Some(candidate);
+            }
+        } else {
+            winning_root = Some(candidate);
+        }
     }
 
-    Ok(candidates
+    Ok(winning_root)
-        .iter()
+}
-        .filter_map(|(_hash, candidate)| {
+
-            if candidate.total_attesting_balance == highest_seen_balance {
+/// Returns all indices which voted for a given crosslink. May contain duplicates.
-                Some(candidate)
+///
-            } else {
+/// Spec v0.4.0
-                None
+fn get_attesting_validator_indices(
-            }
+    state: &BeaconState,
-        })
+    shard: u64,
-        .min_by_key(|candidate| candidate.crosslink_data_root)
+    current_epoch_attestations: &[&PendingAttestation],
-        .ok_or_else(|| WinningRootError::NoWinningRoot)?
+    previous_epoch_attestations: &[&PendingAttestation],
-        // TODO: avoid clone.
+    crosslink_data_root: &Hash256,
-        .clone())
+    spec: &ChainSpec,
+) -> Result<Vec<usize>, BeaconStateError> {
+    let mut indices = vec![];
+
+    for a in current_epoch_attestations
+        .iter()
+        .chain(previous_epoch_attestations.iter())
+    {
+        if (a.data.shard == shard) && (a.data.crosslink_data_root == *crosslink_data_root) {
+            indices.append(&mut state.get_attestation_participants(
+                &a.data,
+                &a.aggregation_bitfield,
+                spec,
+            )?);
+        }
+    }
+
+    Ok(indices)
 }

eth2/types/src/beacon_state.rs

@@ -56,13 +56,6 @@ pub enum Error {
     EpochCacheUninitialized(RelativeEpoch),
 }
 
-#[derive(Debug, PartialEq)]
-pub enum InclusionError {
-    /// The validator did not participate in an attestation in this period.
-    NoAttestationsForValidator,
-    Error(Error),
-}
-
 macro_rules! safe_add_assign {
     ($a: expr, $b: expr) => {
         $a = $a.saturating_add($b);
@@ -1123,67 +1116,6 @@ impl BeaconState {
                 / 2
     }
 
-    /// Returns the distance between the first included attestation for some validator and this
-    /// slot.
-    ///
-    /// Note: In the spec this is defined "inline", not as a helper function.
-    ///
-    /// Spec v0.4.0
-    pub fn inclusion_distance(
-        &self,
-        attestations: &[&PendingAttestation],
-        validator_index: usize,
-        spec: &ChainSpec,
-    ) -> Result<u64, InclusionError> {
-        let attestation =
-            self.earliest_included_attestation(attestations, validator_index, spec)?;
-        Ok((attestation.inclusion_slot - attestation.data.slot).as_u64())
-    }
-
-    /// Returns the slot of the earliest included attestation for some validator.
-    ///
-    /// Note: In the spec this is defined "inline", not as a helper function.
-    ///
-    /// Spec v0.4.0
-    pub fn inclusion_slot(
-        &self,
-        attestations: &[&PendingAttestation],
-        validator_index: usize,
-        spec: &ChainSpec,
-    ) -> Result<Slot, InclusionError> {
-        let attestation =
-            self.earliest_included_attestation(attestations, validator_index, spec)?;
-        Ok(attestation.inclusion_slot)
-    }
-
-    /// Finds the earliest included attestation for some validator.
-    ///
-    /// Note: In the spec this is defined "inline", not as a helper function.
-    ///
-    /// Spec v0.4.0
-    fn earliest_included_attestation(
-        &self,
-        attestations: &[&PendingAttestation],
-        validator_index: usize,
-        spec: &ChainSpec,
-    ) -> Result<PendingAttestation, InclusionError> {
-        let mut included_attestations = vec![];
-
-        for (i, a) in attestations.iter().enumerate() {
-            let participants =
-                self.get_attestation_participants(&a.data, &a.aggregation_bitfield, spec)?;
-            if participants.iter().any(|i| *i == validator_index) {
-                included_attestations.push(i);
-            }
-        }
-
-        let earliest_attestation_index = included_attestations
-            .iter()
-            .min_by_key(|i| attestations[**i].inclusion_slot)
-            .ok_or_else(|| InclusionError::NoAttestationsForValidator)?;
-        Ok(attestations[*earliest_attestation_index].clone())
-    }
-
     /// Returns the base reward for some validator.
     ///
     /// Note: In the spec this is defined "inline", not as a helper function.
@@ -1226,12 +1158,6 @@ fn hash_tree_root<T: TreeHash>(input: Vec<T>) -> Hash256 {
     Hash256::from(&input.hash_tree_root()[..])
 }
 
-impl From<Error> for InclusionError {
-    fn from(e: Error) -> InclusionError {
-        InclusionError::Error(e)
-    }
-}
-
 impl Encodable for BeaconState {
     fn ssz_append(&self, s: &mut SszStream) {
         s.append(&self.slot);

eth2/types/src/lib.rs

@@ -40,9 +40,7 @@ pub use crate::attestation_data_and_custody_bit::AttestationDataAndCustodyBit;
 pub use crate::attester_slashing::AttesterSlashing;
 pub use crate::beacon_block::BeaconBlock;
 pub use crate::beacon_block_body::BeaconBlockBody;
-pub use crate::beacon_state::{
+pub use crate::beacon_state::{BeaconState, Error as BeaconStateError, RelativeEpoch};
-    BeaconState, Error as BeaconStateError, InclusionError, RelativeEpoch,
-};
 pub use crate::chain_spec::{ChainSpec, Domain};
 pub use crate::crosslink::Crosslink;
 pub use crate::deposit::Deposit;