Tidy per epoch processing

- Add comments to ValidatorStatuses
- Add some checks to guard against a bad statuses list
- Remove unused attester_sets.rs file.

eth2/state_processing/src/per_epoch_processing.rs

@@ -7,7 +7,6 @@ use types::{validator_registry::get_active_validator_indices, *};
 use validator_statuses::{TotalBalances, ValidatorStatuses};
 use winning_root::{winning_root, WinningRoot};
 
-pub mod attester_sets;
 pub mod errors;
 pub mod inclusion_distance;
 pub mod tests;
@@ -271,12 +270,18 @@ pub fn process_rewards_and_penalities(
     let base_reward_quotient =
         total_balances.previous_epoch.integer_sqrt() / spec.base_reward_quotient;
 
+    // Guard against a divide-by-zero during the validator balance update.
     if base_reward_quotient == 0 {
         return Err(Error::BaseRewardQuotientIsZero);
     }
+    // Guard against a divide-by-zero during the validator balance update.
     if total_balances.previous_epoch == 0 {
         return Err(Error::PreviousTotalBalanceIsZero);
     }
+    // Guard against an out-of-bounds during the validator balance update.
+    if statuses.statuses.len() != state.validator_balances.len() {
+        return Err(Error::ValidatorStatusesInconsistent);
+    }
 
     // Justification and finalization
 
@@ -288,7 +293,7 @@ pub fn process_rewards_and_penalities(
         .enumerate()
         .map(|(index, &balance)| {
             let mut balance = balance;
-            let status = &statuses.get(index);
+            let status = &statuses.statuses[index];
             let base_reward = state.base_reward(index, base_reward_quotient, spec);
 
             if epochs_since_finality <= 4 {
@@ -367,8 +372,13 @@ pub fn process_rewards_and_penalities(
 
     // Attestation inclusion
 
+    // Guard against an out-of-bounds during the attester inclusion balance update.
+    if statuses.statuses.len() != state.validator_registry.len() {
+        return Err(Error::ValidatorStatusesInconsistent);
+    }
+
     for (index, _validator) in state.validator_registry.iter().enumerate() {
-        let status = &statuses.get(index);
+        let status = &statuses.statuses[index];
 
         if status.is_previous_epoch_attester {
             let proposer_index = status.inclusion_info.proposer_index;

eth2/state_processing/src/per_epoch_processing/attester_sets.rs

@@ -1,133 +0,0 @@
-use fnv::FnvHashSet;
-use types::*;
-
-/// A set of validator indices, along with the total balance of all those attesters.
-#[derive(Default)]
-pub struct Attesters {
-    /// A set of validator indices.
-    pub indices: FnvHashSet<usize>,
-    /// The total balance of all validators in `self.indices`.
-    pub balance: u64,
-}
-
-impl Attesters {
-    /// Add the given indices to the set, incrementing the sets balance by the provided balance.
-    fn add(&mut self, additional_indices: &[usize], additional_balance: u64) {
-        self.indices.reserve(additional_indices.len());
-        for i in additional_indices {
-            self.indices.insert(*i);
-        }
-        self.balance = self.balance.saturating_add(additional_balance);
-    }
-}
-
-/// A collection of `Attester` objects, representing set of attesters that are rewarded/penalized
-/// during an epoch transition.
-pub struct AttesterSets {
-    /// All validators who attested during the state's current epoch.
-    pub current_epoch: Attesters,
-    /// All validators who attested that the beacon block root of the first slot of the state's
-    /// current epoch is the same as the one stored in this state.
-    ///
-    /// In short validators who agreed with the state about the first slot of the current epoch.
-    pub current_epoch_boundary: Attesters,
-    /// All validators who attested during the state's previous epoch.
-    pub previous_epoch: Attesters,
-    /// All validators who attested that the beacon block root of the first slot of the state's
-    /// previous epoch is the same as the one stored in this state.
-    ///
-    /// In short, validators who agreed with the state about the first slot of the previous epoch.
-    pub previous_epoch_boundary: Attesters,
-    /// All validators who attested that the beacon block root at the pending attestation's slot is
-    /// the same as the one stored in this state.
-    ///
-    /// In short, validators who agreed with the state about the current beacon block root when
-    /// they attested.
-    pub previous_epoch_head: Attesters,
-}
-
-impl AttesterSets {
-    /// Loop through all attestations in the state and instantiate a complete `AttesterSets` struct.
-    ///
-    /// Spec v0.4.0
-    pub fn new(state: &BeaconState, spec: &ChainSpec) -> Result<Self, BeaconStateError> {
-        let mut current_epoch = Attesters::default();
-        let mut current_epoch_boundary = Attesters::default();
-        let mut previous_epoch = Attesters::default();
-        let mut previous_epoch_boundary = Attesters::default();
-        let mut previous_epoch_head = Attesters::default();
-
-        for a in &state.latest_attestations {
-            let attesting_indices =
-                state.get_attestation_participants(&a.data, &a.aggregation_bitfield, spec)?;
-            let attesting_balance = state.get_total_balance(&attesting_indices, spec);
-
-            if is_from_epoch(a, state.current_epoch(spec), spec) {
-                current_epoch.add(&attesting_indices, attesting_balance);
-
-                if has_common_epoch_boundary_root(a, state, state.current_epoch(spec), spec)? {
-                    current_epoch_boundary.add(&attesting_indices, attesting_balance);
-                }
-            } else if is_from_epoch(a, state.previous_epoch(spec), spec) {
-                previous_epoch.add(&attesting_indices, attesting_balance);
-
-                if has_common_epoch_boundary_root(a, state, state.previous_epoch(spec), spec)? {
-                    previous_epoch_boundary.add(&attesting_indices, attesting_balance);
-                }
-
-                if has_common_beacon_block_root(a, state, spec)? {
-                    previous_epoch_head.add(&attesting_indices, attesting_balance);
-                }
-            }
-        }
-
-        Ok(Self {
-            current_epoch,
-            current_epoch_boundary,
-            previous_epoch,
-            previous_epoch_boundary,
-            previous_epoch_head,
-        })
-    }
-}
-
-/// Returns `true` if some `PendingAttestation` is from the supplied `epoch`.
-///
-/// Spec v0.4.0
-fn is_from_epoch(a: &PendingAttestation, epoch: Epoch, spec: &ChainSpec) -> bool {
-    a.data.slot.epoch(spec.slots_per_epoch) == epoch
-}
-
-/// Returns `true` if a `PendingAttestation` and `BeaconState` share the same beacon block hash for
-/// the first slot of the given epoch.
-///
-/// Spec v0.4.0
-fn has_common_epoch_boundary_root(
-    a: &PendingAttestation,
-    state: &BeaconState,
-    epoch: Epoch,
-    spec: &ChainSpec,
-) -> Result<bool, BeaconStateError> {
-    let slot = epoch.start_slot(spec.slots_per_epoch);
-    let state_boundary_root = *state
-        .get_block_root(slot, spec)
-        .ok_or_else(|| BeaconStateError::InsufficientBlockRoots)?;
-
-    Ok(a.data.epoch_boundary_root == state_boundary_root)
-}
-
-/// Returns `true` if a `PendingAttestation` and `BeaconState` share the same beacon block hash for
-/// the current slot of the `PendingAttestation`.
-///
-/// Spec v0.4.0
-fn has_common_beacon_block_root(
-    a: &PendingAttestation,
-    state: &BeaconState,
-    spec: &ChainSpec,
-) -> Result<bool, BeaconStateError> {
-    let state_block_root = *state
-        .get_block_root(a.data.slot, spec)
-        .ok_or_else(|| BeaconStateError::InsufficientBlockRoots)?;
-
-    Ok(a.data.beacon_block_root == state_block_root)
-}

eth2/state_processing/src/per_epoch_processing/errors.rs

@@ -8,6 +8,7 @@ pub enum EpochProcessingError {
     NoRandaoSeed,
     PreviousTotalBalanceIsZero,
     InclusionDistanceZero,
+    ValidatorStatusesInconsistent,
     /// Unable to get the inclusion distance for a validator that should have an inclusion
     /// distance. This indicates an internal inconsistency.
     ///

eth2/state_processing/src/per_epoch_processing/validator_statuses.rs

@@ -1,26 +1,40 @@
 use super::WinningRootHashSet;
 use types::*;
 
+/// Sets the boolean `var` on `self` to be true if it is true on `other`. Otherwise leaves `self`
+/// as is.
 macro_rules! set_self_if_other_is_true {
     ($self_: ident, $other: ident, $var: ident) => {
-        $self_.$var = $other.$var & !$self_.$var;
+        if $other.$var {
+            $self_.$var = true;
+        }
     };
 }
 
+/// The information required to reward some validator for their participation in a "winning"
+/// crosslink root.
 #[derive(Default, Clone)]
 pub struct WinningRootInfo {
+    /// The total balance of the crosslink committee.
     pub total_committee_balance: u64,
+    /// The total balance of the crosslink committee that attested for the "winning" root.
     pub total_attesting_balance: u64,
 }
 
+/// The information required to reward a block producer for including an attestation in a block.
 #[derive(Clone)]
 pub struct InclusionInfo {
+    /// The earliest slot a validator had an attestation included in the previous epoch.
     pub slot: Slot,
+    /// The distance between the attestation slot and the slot that attestation was included in a
+    /// block.
     pub distance: Slot,
+    /// The index of the proposer at the slot where the attestation was included.
     pub proposer_index: usize,
 }
 
 impl Default for InclusionInfo {
+    /// Defaults to `slot` and `distance` at their maximum values and `proposer_index` at zero.
     fn default() -> Self {
         Self {
             slot: Slot::max_value(),
@@ -31,6 +45,8 @@ impl Default for InclusionInfo {
 }
 
 impl InclusionInfo {
+    /// Tests if some `other` `InclusionInfo` has a lower inclusion slot than `self`. If so,
+    /// replaces `self` with `other`.
     pub fn update(&mut self, other: &Self) {
         if other.slot < self.slot {
             self.slot = other.slot;
@@ -40,23 +56,43 @@ impl InclusionInfo {
     }
 }
 
+/// Information required to reward some validator during the current and previous epoch.
 #[derive(Default, Clone)]
 pub struct AttesterStatus {
+    /// True if the validator was active in the state's _current_ epoch.
     pub is_active_in_current_epoch: bool,
+    /// True if the validator was active in the state's _previous_ epoch.
     pub is_active_in_previous_epoch: bool,
 
+    /// True if the validator had an attestation included in the _current_ epoch.
     pub is_current_epoch_attester: bool,
+    /// True if the validator's beacon block root attestation for the first slot of the _current_
+    /// epoch matches the block root known to the state.
     pub is_current_epoch_boundary_attester: bool,
+    /// True if the validator had an attestation included in the _previous_ epoch.
     pub is_previous_epoch_attester: bool,
+    /// True if the validator's beacon block root attestation for the first slot of the _previous_
+    /// epoch matches the block root known to the state.
     pub is_previous_epoch_boundary_attester: bool,
+    /// True if the validator's beacon block root attestation in the _previous_ epoch at the
+    /// attestation's slot (`attestation_data.slot`) matches the block root known to the state.
     pub is_previous_epoch_head_attester: bool,
 
+    /// Information used to reward the block producer of this validators earliest-included
+    /// attestation.
     pub inclusion_info: InclusionInfo,
+    /// Information used to reward/penalize the validator if they voted in the super-majority for
+    /// some shard block.
     pub winning_root_info: Option<WinningRootInfo>,
 }
 
 impl AttesterStatus {
-    /// Note: does not update the winning root info.
+    /// Accepts some `other` `AttesterStatus` and updates `self` if required.
+    ///
+    /// Will never set one of the `bool` fields to `false`, it will only set it to `true` if other
+    /// contains a `true` field.
+    ///
+    /// Note: does not update the winning root info, this is done manually.
     pub fn update(&mut self, other: &Self) {
         // Update all the bool fields, only updating `self` if `other` is true (never setting
         // `self` to false).
@@ -72,24 +108,46 @@ impl AttesterStatus {
     }
 }
 
+/// The total effective balances for different sets of validators during the previous and current
+/// epochs.
 #[derive(Default, Clone)]
 pub struct TotalBalances {
+    /// The total effective balance of all active validators during the _current_ epoch.
     pub current_epoch: u64,
+    /// The total effective balance of all active validators during the _previous_ epoch.
     pub previous_epoch: u64,
+    /// The total effective balance of all validators who attested during the _current_ epoch.
     pub current_epoch_attesters: u64,
+    /// The total effective balance of all validators who attested during the _current_ epoch and
+    /// agreed with the state about the beacon block at the first slot of the _current_ epoch.
     pub current_epoch_boundary_attesters: u64,
+    /// The total effective balance of all validators who attested during the _previous_ epoch.
     pub previous_epoch_attesters: u64,
+    /// The total effective balance of all validators who attested during the _previous_ epoch and
+    /// agreed with the state about the beacon block at the first slot of the _previous_ epoch.
     pub previous_epoch_boundary_attesters: u64,
+    /// The total effective balance of all validators who attested during the _previous_ epoch and
+    /// agreed with the state about the beacon block at the time of attestation.
     pub previous_epoch_head_attesters: u64,
 }
 
+/// Summarised information about validator participation in the _previous and _current_ epochs of
+/// some `BeaconState`.
 #[derive(Clone)]
 pub struct ValidatorStatuses {
-    statuses: Vec<AttesterStatus>,
+    /// Information about each individual validator from the state's validator registy.
+    pub statuses: Vec<AttesterStatus>,
+    /// Summed balances for various sets of validators.
     pub total_balances: TotalBalances,
 }
 
 impl ValidatorStatuses {
+    /// Initializes a new instance, determining:
+    ///
+    /// - Active validators
+    /// - Total balances for the current and previous epochs.
+    ///
+    /// Spec v0.4.0
     pub fn new(state: &BeaconState, spec: &ChainSpec) -> Self {
         let mut statuses = Vec::with_capacity(state.validator_registry.len());
         let mut total_balances = TotalBalances::default();
@@ -116,10 +174,10 @@ impl ValidatorStatuses {
         }
     }
 
-    pub fn get(&self, i: usize) -> &AttesterStatus {
+    /// Process some attestations from the given `state` updating the `statuses` and
-        &self.statuses[i]
+    /// `total_balances` fields.
-    }
+    ///
-
+    /// Spec v0.4.0
     pub fn process_attestations(
         &mut self,
         state: &BeaconState,
@@ -174,6 +232,10 @@ impl ValidatorStatuses {
         Ok(())
     }
 
+    /// Update the `statuses` for each validator based upon whether or not they attested to the
+    /// "winning" shard block root for the previous epoch.
+    ///
+    /// Spec v0.4.0
     pub fn process_winning_roots(
         &mut self,
         state: &BeaconState,
@@ -207,6 +269,10 @@ impl ValidatorStatuses {
     }
 }
 
+/// Returns the distance between when the attestation was created and when it was included in a
+/// block.
+///
+/// Spec v0.4.0
 fn inclusion_distance(a: &PendingAttestation) -> Slot {
     a.inclusion_slot - a.data.slot
 }