Attestation processing (#497)

* Renamed fork_choice::process_attestation_from_block

* Processing attestation in fork choice

* Retrieving state from store and checking signature

* Looser check on beacon state validity.

* Cleaned up get_attestation_state

* Expanded fork choice api to provide latest validator message.

* Checking if the an attestation contains a latest message

* Correct process_attestation error handling.

* Copy paste error in comment fixed.

* Tidy ancestor iterators

* Getting attestation slot via helper method

* Refactored attestation creation in test utils

* Revert "Refactored attestation creation in test utils"

This reverts commit 4d277fe4239a7194758b18fb5c00dfe0b8231306.

* Integration tests for free attestation processing

* Implicit conflicts resolved.

* formatting

* Do first pass on Grants code

* Add another attestation processing test

* Tidy attestation processing

* Remove old code fragment

* Add non-compiling half finished changes

* Simplify, fix bugs, add tests for chain iters

* Remove attestation processing from op pool

* Fix bug with fork choice, tidy

* Fix overly restrictive check in fork choice.

* Ensure committee cache is build during attn proc

* Ignore unknown blocks at fork choice

* Various minor fixes

* Make fork choice write lock in to read lock

* Remove unused method

* Tidy comments

* Fix attestation prod. target roots change

* Fix compile error in store iters

* Reject any attestation prior to finalization

* Fix minor PR comments

* Remove duplicated attestation finalization check

* Remove awkward `let` statement

beacon_node/beacon_chain/Cargo.toml

@@ -24,3 +24,4 @@ lmd_ghost = { path = "../../eth2/lmd_ghost" }
 
 [dev-dependencies]
 rand = "0.5.5"
+lazy_static = "1.3.0"

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -11,9 +11,12 @@ use operation_pool::{OperationPool, PersistedOperationPool};
 use parking_lot::{RwLock, RwLockReadGuard};
 use slog::{error, info, warn, Logger};
 use slot_clock::SlotClock;
-use state_processing::per_block_processing::errors::{
+use state_processing::per_block_processing::{
-    AttestationValidationError, AttesterSlashingValidationError, DepositValidationError,
+    errors::{
-    ExitValidationError, ProposerSlashingValidationError, TransferValidationError,
+        AttestationValidationError, AttesterSlashingValidationError, DepositValidationError,
+        ExitValidationError, ProposerSlashingValidationError, TransferValidationError,
+    },
+    verify_attestation_for_state, VerifySignatures,
 };
 use state_processing::{
     per_block_processing, per_block_processing_without_verifying_block_signature,
@@ -54,6 +57,26 @@ pub enum BlockProcessingOutcome {
     PerBlockProcessingError(BlockProcessingError),
 }
 
+#[derive(Debug, PartialEq)]
+pub enum AttestationProcessingOutcome {
+    Processed,
+    UnknownHeadBlock {
+        beacon_block_root: Hash256,
+    },
+    /// The attestation is attesting to a state that is later than itself. (Viz., attesting to the
+    /// future).
+    AttestsToFutureState {
+        state: Slot,
+        attestation: Slot,
+    },
+    /// The slot is finalized, no need to import.
+    FinalizedSlot {
+        attestation: Epoch,
+        finalized: Epoch,
+    },
+    Invalid(AttestationValidationError),
+}
+
 pub trait BeaconChainTypes {
     type Store: store::Store;
     type SlotClock: slot_clock::SlotClock;
@@ -237,15 +260,12 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
     /// - Iterator returns `(Hash256, Slot)`.
     /// - As this iterator starts at the `head` of the chain (viz., the best block), the first slot
     ///     returned may be earlier than the wall-clock slot.
-    pub fn rev_iter_block_roots(
+    pub fn rev_iter_block_roots(&self) -> ReverseBlockRootIterator<T::EthSpec, T::Store> {
-        &self,
-        slot: Slot,
-    ) -> ReverseBlockRootIterator<T::EthSpec, T::Store> {
         let state = &self.head().beacon_state;
         let block_root = self.head().beacon_block_root;
         let block_slot = state.slot;
 
-        let iter = BlockRootsIterator::owned(self.store.clone(), state.clone(), slot);
+        let iter = BlockRootsIterator::owned(self.store.clone(), state.clone());
 
         ReverseBlockRootIterator::new((block_root, block_slot), iter)
     }
@@ -259,15 +279,12 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
     /// - Iterator returns `(Hash256, Slot)`.
     /// - As this iterator starts at the `head` of the chain (viz., the best block), the first slot
     ///     returned may be earlier than the wall-clock slot.
-    pub fn rev_iter_state_roots(
+    pub fn rev_iter_state_roots(&self) -> ReverseStateRootIterator<T::EthSpec, T::Store> {
-        &self,
-        slot: Slot,
-    ) -> ReverseStateRootIterator<T::EthSpec, T::Store> {
         let state = &self.head().beacon_state;
         let state_root = self.head().beacon_state_root;
         let state_slot = state.slot;
 
-        let iter = StateRootsIterator::owned(self.store.clone(), state.clone(), slot);
+        let iter = StateRootsIterator::owned(self.store.clone(), state.clone());
 
         ReverseStateRootIterator::new((state_root, state_slot), iter)
     }
@@ -484,6 +501,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         } else {
             *state.get_block_root(current_epoch_start_slot)?
         };
+
         let target = Checkpoint {
             epoch: state.current_epoch(),
             root: target_root,
@@ -513,38 +531,212 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         })
     }
 
-    /// Accept a new attestation from the network.
+    /// Accept a new, potentially invalid attestation from the network.
     ///
-    /// If valid, the attestation is added to the `op_pool` and aggregated with another attestation
+    /// If valid, the attestation is added to `self.op_pool` and `self.fork_choice`.
-    /// if possible.
+    ///
+    /// Returns an `Ok(AttestationProcessingOutcome)` if the chain was able to make a determination
+    /// about the `attestation` (whether it was invalid or not). Returns an `Err` if there was an
+    /// error during this process and no determination was able to be made.
+    ///
+    /// ## Notes
+    ///
+    /// - Whilst the `attestation` is added to fork choice, the head is not updated. That must be
+    /// done separately.
     pub fn process_attestation(
         &self,
         attestation: Attestation<T::EthSpec>,
-    ) -> Result<(), AttestationValidationError> {
+    ) -> Result<AttestationProcessingOutcome, Error> {
+        // From the store, load the attestation's "head block".
+        //
+        // An honest validator would have set this block to be the head of the chain (i.e., the
+        // result of running fork choice).
+        if let Some(attestation_head_block) = self
+            .store
+            .get::<BeaconBlock<T::EthSpec>>(&attestation.data.beacon_block_root)?
+        {
+            // Attempt to process the attestation using the `self.head()` state.
+            //
+            // This is purely an effort to avoid loading a `BeaconState` unnecessarily from the DB.
+            // Take a read lock on the head beacon state.
+            let state = &self.head().beacon_state;
+
+            // If it turns out that the attestation was made using the head state, then there
+            // is no need to load a state from the database to process the attestation.
+            //
+            // Note: use the epoch of the target because it indicates which epoch the
+            // attestation was created in. You cannot use the epoch of the head block, because
+            // the block doesn't necessarily need to be in the same epoch as the attestation
+            // (e.g., if there are skip slots between the epoch the block was created in and
+            // the epoch for the attestation).
+            //
+            // This check also ensures that the slot for `data.beacon_block_root` is not higher
+            // than `state.root` by ensuring that the block is in the history of `state`.
+            if state.current_epoch() == attestation.data.target.epoch
+                && (attestation.data.beacon_block_root == self.head().beacon_block_root
+                    || state
+                        .get_block_root(attestation_head_block.slot)
+                        .map(|root| *root == attestation.data.beacon_block_root)
+                        .unwrap_or_else(|_| false))
+            {
+                // The head state is able to be used to validate this attestation. No need to load
+                // anything from the database.
+                return self.process_attestation_for_state_and_block(
+                    attestation.clone(),
+                    state,
+                    &attestation_head_block,
+                );
+            }
+
+            // Ensure the read-lock from `self.head()` is dropped.
+            //
+            // This is likely unnecessary, however it remains as a reminder to ensure this lock
+            // isn't hogged.
+            std::mem::drop(state);
+
+            // Use the `data.beacon_block_root` to load the state from the latest non-skipped
+            // slot preceding the attestation's creation.
+            //
+            // This state is guaranteed to be in the same chain as the attestation, but it's
+            // not guaranteed to be from the same slot or epoch as the attestation.
+            let mut state: BeaconState<T::EthSpec> = self
+                .store
+                .get(&attestation_head_block.state_root)?
+                .ok_or_else(|| Error::MissingBeaconState(attestation_head_block.state_root))?;
+
+            // Ensure the state loaded from the database matches the state of the attestation
+            // head block.
+            //
+            // The state needs to be advanced from the current slot through to the epoch in
+            // which the attestation was created in. It would be an error to try and use
+            // `state.get_attestation_data_slot(..)` because the state matching the
+            // `data.beacon_block_root` isn't necessarily in a nearby epoch to the attestation
+            // (e.g., if there were lots of skip slots since the head of the chain and the
+            // epoch creation epoch).
+            for _ in state.slot.as_u64()
+                ..attestation
+                    .data
+                    .target
+                    .epoch
+                    .start_slot(T::EthSpec::slots_per_epoch())
+                    .as_u64()
+            {
+                per_slot_processing(&mut state, &self.spec)?;
+            }
+
+            state.build_committee_cache(RelativeEpoch::Current, &self.spec)?;
+
+            let attestation_slot = state.get_attestation_data_slot(&attestation.data)?;
+
+            // Reject any attestation where the `state` loaded from `data.beacon_block_root`
+            // has a higher slot than the attestation.
+            //
+            // Permitting this would allow for attesters to vote on _future_ slots.
+            if attestation_slot > state.slot {
+                Ok(AttestationProcessingOutcome::AttestsToFutureState {
+                    state: state.slot,
+                    attestation: attestation_slot,
+                })
+            } else {
+                self.process_attestation_for_state_and_block(
+                    attestation,
+                    &state,
+                    &attestation_head_block,
+                )
+            }
+        } else {
+            // Drop any attestation where we have not processed `attestation.data.beacon_block_root`.
+            //
+            // This is likely overly restrictive, we could store the attestation for later
+            // processing.
+            warn!(
+                self.log,
+                "Dropped attestation for unknown block";
+                "block" => format!("{}", attestation.data.beacon_block_root)
+            );
+            Ok(AttestationProcessingOutcome::UnknownHeadBlock {
+                beacon_block_root: attestation.data.beacon_block_root,
+            })
+        }
+    }
+
+    /// Verifies the `attestation` against the `state` to which it is attesting.
+    ///
+    /// Updates fork choice with any new latest messages, but _does not_ find or update the head.
+    ///
+    /// ## Notes
+    ///
+    /// The given `state` must fulfil one of the following conditions:
+    ///
+    /// - `state` corresponds to the `block.state_root` identified by
+    /// `attestation.data.beacon_block_root`. (Viz., `attestation` was created using `state`).
+    /// - `state.slot` is in the same epoch as `data.target.epoch` and
+    /// `attestation.data.beacon_block_root` is in the history of `state`.
+    ///
+    /// Additionally, `attestation.data.beacon_block_root` **must** be available to read in
+    /// `self.store` _and_ be the root of the given `block`.
+    ///
+    /// If the given conditions are not fulfilled, the function may error or provide a false
+    /// negative (indicating that a given `attestation` is invalid when it is was validly formed).
+    fn process_attestation_for_state_and_block(
+        &self,
+        attestation: Attestation<T::EthSpec>,
+        state: &BeaconState<T::EthSpec>,
+        block: &BeaconBlock<T::EthSpec>,
+    ) -> Result<AttestationProcessingOutcome, Error> {
         self.metrics.attestation_processing_requests.inc();
         let timer = self.metrics.attestation_processing_times.start_timer();
 
-        let result = self
+        // Find the highest between:
-            .op_pool
+        //
-            .insert_attestation(attestation, &*self.state.read(), &self.spec);
+        // - The highest valid finalized epoch we've ever seen (i.e., the head).
+        // - The finalized epoch that this attestation was created against.
+        let finalized_epoch = std::cmp::max(
+            self.head().beacon_state.finalized_checkpoint.epoch,
+            state.finalized_checkpoint.epoch,
+        );
+
+        let result = if block.slot <= finalized_epoch.start_slot(T::EthSpec::slots_per_epoch()) {
+            // Ignore any attestation where the slot of `data.beacon_block_root` is equal to or
+            // prior to the finalized epoch.
+            //
+            // For any valid attestation if the `beacon_block_root` is prior to finalization, then
+            // all other parameters (source, target, etc) must all be prior to finalization and
+            // therefore no longer interesting.
+            Ok(AttestationProcessingOutcome::FinalizedSlot {
+                attestation: block.slot.epoch(T::EthSpec::slots_per_epoch()),
+                finalized: finalized_epoch,
+            })
+        } else if let Err(e) =
+            verify_attestation_for_state(state, &attestation, &self.spec, VerifySignatures::True)
+        {
+            warn!(
+                self.log,
+                "Invalid attestation";
+                "state_epoch" => state.current_epoch(),
+                "error" => format!("{:?}", e),
+            );
+
+            Ok(AttestationProcessingOutcome::Invalid(e))
+        } else {
+            // Provide the attestation to fork choice, updating the validator latest messages but
+            // _without_ finding and updating the head.
+            self.fork_choice
+                .process_attestation(&state, &attestation, block)?;
+
+            // Provide the valid attestation to op pool, which may choose to retain the
+            // attestation for inclusion in a future block.
+            self.op_pool
+                .insert_attestation(attestation, state, &self.spec)?;
+
+            // Update the metrics.
+            self.metrics.attestation_processing_successes.inc();
+
+            Ok(AttestationProcessingOutcome::Processed)
+        };
 
         timer.observe_duration();
 
-        if result.is_ok() {
-            self.metrics.attestation_processing_successes.inc();
-        }
-
-        // TODO: process attestation. Please consider:
-        //
-        //  - Because a block was not added to the op pool does not mean it's invalid (it might
-        //  just be old).
-        //  - The attestation should be rejected if we don't know the block (ideally it should be
-        //  queued, but this may be overkill).
-        //  - The attestation _must_ be validated against it's state before being added to fork
-        //  choice.
-        //  - You can avoid verifying some attestations by first checking if they're a latest
-        //  message. This would involve expanding the `LmdGhost` API.
-
         result
     }
 
@@ -612,7 +804,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             return Ok(BlockProcessingOutcome::GenesisBlock);
         }
 
-        let block_root = block.block_header().canonical_root();
+        let block_root = block.canonical_root();
 
         if block_root == self.genesis_block_root {
             return Ok(BlockProcessingOutcome::GenesisBlock);
@@ -658,6 +850,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             per_slot_processing(&mut state, &self.spec)?;
         }
 
+        state.build_committee_cache(RelativeEpoch::Previous, &self.spec)?;
         state.build_committee_cache(RelativeEpoch::Current, &self.spec)?;
 
         // Apply the received block to its parent state (which has been transitioned into this

beacon_node/beacon_chain/src/errors.rs

@@ -1,5 +1,8 @@
 use crate::fork_choice::Error as ForkChoiceError;
 use crate::metrics::Error as MetricsError;
+use state_processing::per_block_processing::errors::{
+    AttestationValidationError, IndexedAttestationValidationError,
+};
 use state_processing::BlockProcessingError;
 use state_processing::SlotProcessingError;
 use types::*;
@@ -23,6 +26,7 @@ pub enum BeaconChainError {
         previous_epoch: Epoch,
         new_epoch: Epoch,
     },
+    UnableToFindTargetRoot(Slot),
     BeaconStateError(BeaconStateError),
     DBInconsistent(String),
     DBError(store::Error),
@@ -31,6 +35,11 @@ pub enum BeaconChainError {
     MissingBeaconState(Hash256),
     SlotProcessingError(SlotProcessingError),
     MetricsError(String),
+    NoStateForAttestation {
+        beacon_block_root: Hash256,
+    },
+    AttestationValidationError(AttestationValidationError),
+    IndexedAttestationValidationError(IndexedAttestationValidationError),
 }
 
 easy_from_to!(SlotProcessingError, BeaconChainError);
@@ -53,3 +62,5 @@ pub enum BlockProductionError {
 easy_from_to!(BlockProcessingError, BlockProductionError);
 easy_from_to!(BeaconStateError, BlockProductionError);
 easy_from_to!(SlotProcessingError, BlockProductionError);
+easy_from_to!(AttestationValidationError, BeaconChainError);
+easy_from_to!(IndexedAttestationValidationError, BeaconChainError);

beacon_node/beacon_chain/src/fork_choice.rs

@@ -3,7 +3,9 @@ use lmd_ghost::LmdGhost;
 use state_processing::common::get_attesting_indices;
 use std::sync::Arc;
 use store::{Error as StoreError, Store};
-use types::{Attestation, BeaconBlock, BeaconState, BeaconStateError, Epoch, EthSpec, Hash256};
+use types::{
+    Attestation, BeaconBlock, BeaconState, BeaconStateError, Epoch, EthSpec, Hash256, Slot,
+};
 
 type Result<T> = std::result::Result<T, Error>;
 
@@ -17,8 +19,8 @@ pub enum Error {
 }
 
 pub struct ForkChoice<T: BeaconChainTypes> {
-    backend: T::LmdGhost,
     store: Arc<T::Store>,
+    backend: T::LmdGhost,
     /// Used for resolving the `0x00..00` alias back to genesis.
     ///
     /// Does not necessarily need to be the _actual_ genesis, it suffices to be the finalized root
@@ -117,18 +119,34 @@ impl<T: BeaconChainTypes> ForkChoice<T> {
         //
         // 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_from_block(state, attestation)?;
+            // If the `data.beacon_block_root` block is not known to us, simply ignore the latest
+            // vote.
+            if let Some(block) = self
+                .store
+                .get::<BeaconBlock<T::EthSpec>>(&attestation.data.beacon_block_root)?
+            {
+                self.process_attestation(state, attestation, &block)?;
+            }
         }
 
+        // 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.
+        //
+        // A case where a block without any votes can be the head is where it is the only child of
+        // a block that has the majority of votes applied to it.
         self.backend.process_block(block, block_root)?;
 
         Ok(())
     }
 
-    fn process_attestation_from_block(
+    /// Process an attestation which references `block` in `attestation.data.beacon_block_root`.
+    ///
+    /// Assumes the attestation is valid.
+    pub fn process_attestation(
         &self,
         state: &BeaconState<T::EthSpec>,
         attestation: &Attestation<T::EthSpec>,
+        block: &BeaconBlock<T::EthSpec>,
     ) -> Result<()> {
         let block_hash = attestation.data.beacon_block_root;
 
@@ -147,26 +165,26 @@ impl<T: BeaconChainTypes> ForkChoice<T> {
         // to genesis just by being present in the chain.
         //
         // Additionally, don't add any block hash to fork choice unless we have imported the block.
-        if block_hash != Hash256::zero()
+        if block_hash != Hash256::zero() {
-            && self
-                .store
-                .exists::<BeaconBlock<T::EthSpec>>(&block_hash)
-                .unwrap_or(false)
-        {
             let validator_indices =
                 get_attesting_indices(state, &attestation.data, &attestation.aggregation_bits)?;
 
-            let block_slot = state.get_attestation_data_slot(&attestation.data)?;
-
             for validator_index in validator_indices {
                 self.backend
-                    .process_attestation(validator_index, block_hash, block_slot)?;
+                    .process_attestation(validator_index, block_hash, block.slot)?;
             }
         }
 
         Ok(())
     }
 
+    /// Returns the latest message for a given validator, if any.
+    ///
+    /// Returns `(block_root, block_slot)`.
+    pub fn latest_message(&self, validator_index: usize) -> Option<(Hash256, Slot)> {
+        self.backend.latest_message(validator_index)
+    }
+
     /// Inform the fork choice that the given block (and corresponding root) have been finalized so
     /// it may prune it's storage.
     ///

beacon_node/beacon_chain/src/lib.rs

@@ -7,7 +7,9 @@ mod metrics;
 mod persisted_beacon_chain;
 pub mod test_utils;
 
-pub use self::beacon_chain::{BeaconChain, BeaconChainTypes, BlockProcessingOutcome};
+pub use self::beacon_chain::{
+    AttestationProcessingOutcome, BeaconChain, BeaconChainTypes, BlockProcessingOutcome,
+};
 pub use self::checkpoint::CheckPoint;
 pub use self::errors::{BeaconChainError, BlockProductionError};
 pub use lmd_ghost;

beacon_node/beacon_chain/src/test_utils.rs

@@ -84,14 +84,30 @@ where
 {
     /// Instantiate a new harness with `validator_count` initial validators.
     pub fn new(validator_count: usize) -> Self {
+        let state_builder = TestingBeaconStateBuilder::from_default_keypairs_file_if_exists(
+            validator_count,
+            &E::default_spec(),
+        );
+        let (genesis_state, keypairs) = state_builder.build();
+
+        Self::from_state_and_keypairs(genesis_state, keypairs)
+    }
+
+    /// Instantiate a new harness with an initial validator for each key supplied.
+    pub fn from_keypairs(keypairs: Vec<Keypair>) -> Self {
+        let state_builder = TestingBeaconStateBuilder::from_keypairs(keypairs, &E::default_spec());
+        let (genesis_state, keypairs) = state_builder.build();
+
+        Self::from_state_and_keypairs(genesis_state, keypairs)
+    }
+
+    /// Instantiate a new harness with the given genesis state and a keypair for each of the
+    /// initial validators in the given state.
+    pub fn from_state_and_keypairs(genesis_state: BeaconState<E>, keypairs: Vec<Keypair>) -> Self {
         let spec = E::default_spec();
 
         let store = Arc::new(MemoryStore::open());
 
-        let state_builder =
-            TestingBeaconStateBuilder::from_default_keypairs_file_if_exists(validator_count, &spec);
-        let (genesis_state, keypairs) = state_builder.build();
-
         let mut genesis_block = BeaconBlock::empty(&spec);
         genesis_block.state_root = Hash256::from_slice(&genesis_state.tree_hash_root());
 
@@ -178,12 +194,7 @@ where
             if let BlockProcessingOutcome::Processed { block_root } = outcome {
                 head_block_root = Some(block_root);
 
-                self.add_attestations_to_op_pool(
+                self.add_free_attestations(&attestation_strategy, &new_state, block_root, slot);
-                    &attestation_strategy,
-                    &new_state,
-                    block_root,
-                    slot,
-                );
             } else {
                 panic!("block should be successfully processed: {:?}", outcome);
             }
@@ -198,7 +209,7 @@ where
     fn get_state_at_slot(&self, state_slot: Slot) -> BeaconState<E> {
         let state_root = self
             .chain
-            .rev_iter_state_roots(self.chain.head().beacon_state.slot - 1)
+            .rev_iter_state_roots()
             .find(|(_hash, slot)| *slot == state_slot)
             .map(|(hash, _slot)| hash)
             .expect("could not find state root");
@@ -263,16 +274,38 @@ where
         (block, state)
     }
 
-    /// Adds attestations to the `BeaconChain` operations pool to be included in future blocks.
+    /// Adds attestations to the `BeaconChain` operations pool and fork choice.
     ///
     /// The `attestation_strategy` dictates which validators should attest.
-    fn add_attestations_to_op_pool(
+    fn add_free_attestations(
         &self,
         attestation_strategy: &AttestationStrategy,
         state: &BeaconState<E>,
         head_block_root: Hash256,
         head_block_slot: Slot,
     ) {
+        self.get_free_attestations(
+            attestation_strategy,
+            state,
+            head_block_root,
+            head_block_slot,
+        )
+        .into_iter()
+        .for_each(|attestation| {
+            self.chain
+                .process_attestation(attestation)
+                .expect("should process attestation");
+        });
+    }
+
+    /// Generates a `Vec<Attestation>` for some attestation strategy and head_block.
+    pub fn get_free_attestations(
+        &self,
+        attestation_strategy: &AttestationStrategy,
+        state: &BeaconState<E>,
+        head_block_root: Hash256,
+        head_block_slot: Slot,
+    ) -> Vec<Attestation<E>> {
         let spec = &self.spec;
         let fork = &state.fork;
 
@@ -281,6 +314,8 @@ where
             AttestationStrategy::SomeValidators(vec) => vec.clone(),
         };
 
+        let mut vec = vec![];
+
         state
             .get_crosslink_committees_at_slot(state.slot)
             .expect("should get committees")
@@ -326,19 +361,17 @@ where
                             agg_sig
                         };
 
-                        let attestation = Attestation {
+                        vec.push(Attestation {
                             aggregation_bits,
                             data,
                             custody_bits,
                             signature,
-                        };
+                        })
-
-                        self.chain
-                            .process_attestation(attestation)
-                            .expect("should process attestation");
                     }
                 }
             });
+
+        vec
     }
 
     /// Creates two forks:

beacon_node/beacon_chain/tests/tests.rs

@@ -1,29 +1,104 @@
 #![cfg(not(debug_assertions))]
 
+#[macro_use]
+extern crate lazy_static;
+
 use beacon_chain::test_utils::{
     AttestationStrategy, BeaconChainHarness, BlockStrategy, CommonTypes, PersistedBeaconChain,
     BEACON_CHAIN_DB_KEY,
 };
+use beacon_chain::AttestationProcessingOutcome;
 use lmd_ghost::ThreadSafeReducedTree;
 use rand::Rng;
 use store::{MemoryStore, Store};
 use types::test_utils::{SeedableRng, TestRandom, XorShiftRng};
-use types::{Deposit, EthSpec, Hash256, MinimalEthSpec, Slot};
+use types::{Deposit, EthSpec, Hash256, Keypair, MinimalEthSpec, RelativeEpoch, Slot};
 
 // Should ideally be divisible by 3.
 pub const VALIDATOR_COUNT: usize = 24;
 
+lazy_static! {
+    /// A cached set of keys.
+    static ref KEYPAIRS: Vec<Keypair> = types::test_utils::generate_deterministic_keypairs(VALIDATOR_COUNT);
+}
+
 type TestForkChoice = ThreadSafeReducedTree<MemoryStore, MinimalEthSpec>;
 
 fn get_harness(validator_count: usize) -> BeaconChainHarness<TestForkChoice, MinimalEthSpec> {
-    let harness = BeaconChainHarness::new(validator_count);
+    let harness = BeaconChainHarness::from_keypairs(KEYPAIRS[0..validator_count].to_vec());
 
-    // Move past the zero slot.
     harness.advance_slot();
 
     harness
 }
 
+#[test]
+fn iterators() {
+    let num_blocks_produced = MinimalEthSpec::slots_per_epoch() * 2 - 1;
+
+    let harness = get_harness(VALIDATOR_COUNT);
+
+    harness.extend_chain(
+        num_blocks_produced as usize,
+        BlockStrategy::OnCanonicalHead,
+        // No need to produce attestations for this test.
+        AttestationStrategy::SomeValidators(vec![]),
+    );
+
+    let block_roots: Vec<(Hash256, Slot)> = harness.chain.rev_iter_block_roots().collect();
+    let state_roots: Vec<(Hash256, Slot)> = harness.chain.rev_iter_state_roots().collect();
+
+    assert_eq!(
+        block_roots.len(),
+        state_roots.len(),
+        "should be an equal amount of block and state roots"
+    );
+
+    assert!(
+        block_roots.iter().any(|(_root, slot)| *slot == 0),
+        "should contain genesis block root"
+    );
+    assert!(
+        state_roots.iter().any(|(_root, slot)| *slot == 0),
+        "should contain genesis state root"
+    );
+
+    assert_eq!(
+        block_roots.len(),
+        num_blocks_produced as usize + 1,
+        "should contain all produced blocks, plus the genesis block"
+    );
+
+    block_roots.windows(2).for_each(|x| {
+        assert_eq!(
+            x[1].1,
+            x[0].1 - 1,
+            "block root slots should be decreasing by one"
+        )
+    });
+    state_roots.windows(2).for_each(|x| {
+        assert_eq!(
+            x[1].1,
+            x[0].1 - 1,
+            "state root slots should be decreasing by one"
+        )
+    });
+
+    let head = &harness.chain.head();
+
+    assert_eq!(
+        *block_roots.first().expect("should have some block roots"),
+        (head.beacon_block_root, head.beacon_block.slot),
+        "first block root and slot should be for the head block"
+    );
+
+    assert_eq!(
+        *state_roots.first().expect("should have some state roots"),
+        (head.beacon_state_root, head.beacon_state.slot),
+        "first state root and slot should be for the head state"
+    );
+}
+
 #[test]
 fn chooses_fork() {
     let harness = get_harness(VALIDATOR_COUNT);
@@ -251,3 +326,136 @@ fn roundtrip_operation_pool() {
 
     assert_eq!(harness.chain.op_pool, restored_op_pool);
 }
+
+#[test]
+fn free_attestations_added_to_fork_choice_some_none() {
+    let num_blocks_produced = MinimalEthSpec::slots_per_epoch() / 2;
+
+    let harness = get_harness(VALIDATOR_COUNT);
+
+    harness.extend_chain(
+        num_blocks_produced as usize,
+        BlockStrategy::OnCanonicalHead,
+        AttestationStrategy::AllValidators,
+    );
+
+    let state = &harness.chain.head().beacon_state;
+    let fork_choice = &harness.chain.fork_choice;
+
+    let validator_slots: Vec<(usize, Slot)> = (0..VALIDATOR_COUNT)
+        .into_iter()
+        .map(|validator_index| {
+            let slot = state
+                .get_attestation_duties(validator_index, RelativeEpoch::Current)
+                .expect("should get attester duties")
+                .unwrap()
+                .slot;
+
+            (validator_index, slot)
+        })
+        .collect();
+
+    for (validator, slot) in validator_slots.clone() {
+        let latest_message = fork_choice.latest_message(validator);
+
+        if slot <= num_blocks_produced && slot != 0 {
+            assert_eq!(
+                latest_message.unwrap().1,
+                slot,
+                "Latest message slot for {} should be equal to slot {}.",
+                validator,
+                slot
+            )
+        } else {
+            assert!(
+                latest_message.is_none(),
+                "Latest message slot should be None."
+            )
+        }
+    }
+}
+
+#[test]
+fn attestations_with_increasing_slots() {
+    let num_blocks_produced = MinimalEthSpec::slots_per_epoch() * 5;
+
+    let harness = get_harness(VALIDATOR_COUNT);
+
+    let mut attestations = vec![];
+
+    for _ in 0..num_blocks_produced {
+        harness.extend_chain(
+            2,
+            BlockStrategy::OnCanonicalHead,
+            // Don't produce & include any attestations (we'll collect them later).
+            AttestationStrategy::SomeValidators(vec![]),
+        );
+
+        attestations.append(&mut harness.get_free_attestations(
+            &AttestationStrategy::AllValidators,
+            &harness.chain.head().beacon_state,
+            harness.chain.head().beacon_block_root,
+            harness.chain.head().beacon_block.slot,
+        ));
+
+        harness.advance_slot();
+    }
+
+    for attestation in attestations {
+        assert_eq!(
+            harness.chain.process_attestation(attestation),
+            Ok(AttestationProcessingOutcome::Processed)
+        )
+    }
+}
+
+#[test]
+fn free_attestations_added_to_fork_choice_all_updated() {
+    let num_blocks_produced = MinimalEthSpec::slots_per_epoch() * 2 - 1;
+
+    let harness = get_harness(VALIDATOR_COUNT);
+
+    harness.extend_chain(
+        num_blocks_produced as usize,
+        BlockStrategy::OnCanonicalHead,
+        AttestationStrategy::AllValidators,
+    );
+
+    let state = &harness.chain.head().beacon_state;
+    let fork_choice = &harness.chain.fork_choice;
+
+    let validators: Vec<usize> = (0..VALIDATOR_COUNT).collect();
+    let slots: Vec<Slot> = validators
+        .iter()
+        .map(|&v| {
+            state
+                .get_attestation_duties(v, RelativeEpoch::Current)
+                .expect("should get attester duties")
+                .unwrap()
+                .slot
+        })
+        .collect();
+    let validator_slots: Vec<(&usize, Slot)> = validators.iter().zip(slots).collect();
+
+    for (validator, slot) in validator_slots {
+        let latest_message = fork_choice.latest_message(*validator);
+
+        assert_eq!(
+            latest_message.unwrap().1,
+            slot,
+            "Latest message slot should be equal to attester duty."
+        );
+
+        if slot != num_blocks_produced {
+            let block_root = state
+                .get_block_root(slot)
+                .expect("Should get block root at slot");
+
+            assert_eq!(
+                latest_message.unwrap().0,
+                *block_root,
+                "Latest message block root should be equal to block at slot."
+            );
+        }
+    }
+}

beacon_node/network/src/sync/simple_sync.rs

@@ -266,8 +266,7 @@ impl<T: BeaconChainTypes> SimpleSync<T> {
 
     fn root_at_slot(&self, target_slot: Slot) -> Option<Hash256> {
         self.chain
-            .rev_iter_block_roots(target_slot)
+            .rev_iter_block_roots()
-            .take(1)
             .find(|(_root, slot)| *slot == target_slot)
             .map(|(root, _slot)| root)
     }
@@ -280,8 +279,6 @@ impl<T: BeaconChainTypes> SimpleSync<T> {
         req: BeaconBlockRootsRequest,
         network: &mut NetworkContext,
     ) {
-        let state = &self.chain.head().beacon_state;
-
         debug!(
             self.log,
             "BlockRootsRequest";
@@ -292,8 +289,9 @@ impl<T: BeaconChainTypes> SimpleSync<T> {
 
         let mut roots: Vec<BlockRootSlot> = self
             .chain
-            .rev_iter_block_roots(std::cmp::min(req.start_slot + req.count, state.slot))
+            .rev_iter_block_roots()
             .take_while(|(_root, slot)| req.start_slot <= *slot)
+            .filter(|(_root, slot)| *slot < req.start_slot + req.count)
             .map(|(block_root, slot)| BlockRootSlot { slot, block_root })
             .collect();
 
@@ -391,8 +389,6 @@ impl<T: BeaconChainTypes> SimpleSync<T> {
         req: BeaconBlockHeadersRequest,
         network: &mut NetworkContext,
     ) {
-        let state = &self.chain.head().beacon_state;
-
         debug!(
             self.log,
             "BlockHeadersRequest";
@@ -405,8 +401,9 @@ impl<T: BeaconChainTypes> SimpleSync<T> {
         // Collect the block roots.
         let mut roots: Vec<Hash256> = self
             .chain
-            .rev_iter_block_roots(std::cmp::min(req.start_slot + count, state.slot))
+            .rev_iter_block_roots()
             .take_while(|(_root, slot)| req.start_slot <= *slot)
+            .filter(|(_root, slot)| *slot < req.start_slot + count)
             .map(|(root, _slot)| root)
             .collect();
 
@@ -631,7 +628,12 @@ impl<T: BeaconChainTypes> SimpleSync<T> {
         _network: &mut NetworkContext,
     ) {
         match self.chain.process_attestation(msg) {
-            Ok(()) => info!(self.log, "ImportedAttestation"; "source" => "gossip"),
+            Ok(outcome) => info!(
+                self.log,
+                "Processed attestation";
+                "source" => "gossip",
+                "outcome" => format!("{:?}", outcome)
+            ),
             Err(e) => {
                 warn!(self.log, "InvalidAttestation"; "source" => "gossip", "error" => format!("{:?}", e))
             }

beacon_node/rpc/src/attestation.rs

@@ -1,4 +1,4 @@
-use beacon_chain::{BeaconChain, BeaconChainTypes};
+use beacon_chain::{BeaconChain, BeaconChainError, BeaconChainTypes};
 use eth2_libp2p::PubsubMessage;
 use eth2_libp2p::Topic;
 use eth2_libp2p::BEACON_ATTESTATION_TOPIC;
@@ -163,7 +163,7 @@ impl<T: BeaconChainTypes> AttestationService for AttestationServiceInstance<T> {
 
                 resp.set_success(true);
             }
-            Err(e) => {
+            Err(BeaconChainError::AttestationValidationError(e)) => {
                 // Attestation was invalid
                 warn!(
                     self.log,
@@ -174,6 +174,36 @@ impl<T: BeaconChainTypes> AttestationService for AttestationServiceInstance<T> {
                 resp.set_success(false);
                 resp.set_msg(format!("InvalidAttestation: {:?}", e).as_bytes().to_vec());
             }
+            Err(BeaconChainError::IndexedAttestationValidationError(e)) => {
+                // Indexed attestation was invalid
+                warn!(
+                    self.log,
+                    "PublishAttestation";
+                    "type" => "invalid_attestation",
+                    "error" => format!("{:?}", e),
+                );
+                resp.set_success(false);
+                resp.set_msg(
+                    format!("InvalidIndexedAttestation: {:?}", e)
+                        .as_bytes()
+                        .to_vec(),
+                );
+            }
+            Err(e) => {
+                // Some other error
+                warn!(
+                    self.log,
+                    "PublishAttestation";
+                    "type" => "beacon_chain_error",
+                    "error" => format!("{:?}", e),
+                );
+                resp.set_success(false);
+                resp.set_msg(
+                    format!("There was a beacon chain error: {:?}", e)
+                        .as_bytes()
+                        .to_vec(),
+                );
+            }
         };
 
         let error_log = self.log.clone();

beacon_node/store/src/iter.rs

@@ -15,21 +15,21 @@ pub trait AncestorIter<U: Store, I: Iterator> {
 }
 
 impl<'a, U: Store, E: EthSpec> AncestorIter<U, BlockRootsIterator<'a, E, U>> for BeaconBlock<E> {
-    /// Iterates across all the prior block roots of `self`, starting at the most recent and ending
+    /// Iterates across all available prior block roots of `self`, starting at the most recent and ending
     /// at genesis.
     fn try_iter_ancestor_roots(&self, store: Arc<U>) -> Option<BlockRootsIterator<'a, E, U>> {
         let state = store.get::<BeaconState<E>>(&self.state_root).ok()??;
 
-        Some(BlockRootsIterator::owned(store, state, self.slot))
+        Some(BlockRootsIterator::owned(store, state))
     }
 }
 
 impl<'a, U: Store, E: EthSpec> AncestorIter<U, StateRootsIterator<'a, E, U>> for BeaconState<E> {
-    /// Iterates across all the prior state roots of `self`, starting at the most recent and ending
+    /// Iterates across all available prior state roots of `self`, starting at the most recent and ending
     /// at genesis.
     fn try_iter_ancestor_roots(&self, store: Arc<U>) -> Option<StateRootsIterator<'a, E, U>> {
         // The `self.clone()` here is wasteful.
-        Some(StateRootsIterator::owned(store, self.clone(), self.slot))
+        Some(StateRootsIterator::owned(store, self.clone()))
     }
 }
 
@@ -41,19 +41,19 @@ pub struct StateRootsIterator<'a, T: EthSpec, U> {
 }
 
 impl<'a, T: EthSpec, U: Store> StateRootsIterator<'a, T, U> {
-    pub fn new(store: Arc<U>, beacon_state: &'a BeaconState<T>, start_slot: Slot) -> Self {
+    pub fn new(store: Arc<U>, beacon_state: &'a BeaconState<T>) -> Self {
         Self {
             store,
+            slot: beacon_state.slot,
             beacon_state: Cow::Borrowed(beacon_state),
-            slot: start_slot + 1,
         }
     }
 
-    pub fn owned(store: Arc<U>, beacon_state: BeaconState<T>, start_slot: Slot) -> Self {
+    pub fn owned(store: Arc<U>, beacon_state: BeaconState<T>) -> Self {
         Self {
             store,
+            slot: beacon_state.slot,
             beacon_state: Cow::Owned(beacon_state),
-            slot: start_slot + 1,
         }
     }
 }
@@ -97,16 +97,16 @@ pub struct BlockIterator<'a, T: EthSpec, U> {
 
 impl<'a, T: EthSpec, U: Store> BlockIterator<'a, T, U> {
     /// Create a new iterator over all blocks in the given `beacon_state` and prior states.
-    pub fn new(store: Arc<U>, beacon_state: &'a BeaconState<T>, start_slot: Slot) -> Self {
+    pub fn new(store: Arc<U>, beacon_state: &'a BeaconState<T>) -> Self {
         Self {
-            roots: BlockRootsIterator::new(store, beacon_state, start_slot),
+            roots: BlockRootsIterator::new(store, beacon_state),
         }
     }
 
     /// Create a new iterator over all blocks in the given `beacon_state` and prior states.
-    pub fn owned(store: Arc<U>, beacon_state: BeaconState<T>, start_slot: Slot) -> Self {
+    pub fn owned(store: Arc<U>, beacon_state: BeaconState<T>) -> Self {
         Self {
-            roots: BlockRootsIterator::owned(store, beacon_state, start_slot),
+            roots: BlockRootsIterator::owned(store, beacon_state),
         }
     }
 }
@@ -137,20 +137,20 @@ pub struct BlockRootsIterator<'a, T: EthSpec, U> {
 
 impl<'a, T: EthSpec, U: Store> BlockRootsIterator<'a, T, U> {
     /// Create a new iterator over all block roots in the given `beacon_state` and prior states.
-    pub fn new(store: Arc<U>, beacon_state: &'a BeaconState<T>, start_slot: Slot) -> Self {
+    pub fn new(store: Arc<U>, beacon_state: &'a BeaconState<T>) -> Self {
         Self {
             store,
+            slot: beacon_state.slot,
             beacon_state: Cow::Borrowed(beacon_state),
-            slot: start_slot + 1,
         }
     }
 
     /// Create a new iterator over all block roots in the given `beacon_state` and prior states.
-    pub fn owned(store: Arc<U>, beacon_state: BeaconState<T>, start_slot: Slot) -> Self {
+    pub fn owned(store: Arc<U>, beacon_state: BeaconState<T>) -> Self {
         Self {
             store,
+            slot: beacon_state.slot,
             beacon_state: Cow::Owned(beacon_state),
-            slot: start_slot + 1,
         }
     }
 }
@@ -227,7 +227,7 @@ mod test {
         state_b.state_roots[0] = state_a_root;
         store.put(&state_a_root, &state_a).unwrap();
 
-        let iter = BlockRootsIterator::new(store.clone(), &state_b, state_b.slot - 1);
+        let iter = BlockRootsIterator::new(store.clone(), &state_b);
 
         assert!(
             iter.clone().find(|(_root, slot)| *slot == 0).is_some(),
@@ -276,7 +276,7 @@ mod test {
         store.put(&state_a_root, &state_a).unwrap();
         store.put(&state_b_root, &state_b).unwrap();
 
-        let iter = StateRootsIterator::new(store.clone(), &state_b, state_b.slot - 1);
+        let iter = StateRootsIterator::new(store.clone(), &state_b);
 
         assert!(
             iter.clone().find(|(_root, slot)| *slot == 0).is_some(),

eth2/lmd_ghost/src/lib.rs

@@ -43,4 +43,7 @@ pub trait LmdGhost<S: Store, E: EthSpec>: Send + Sync {
         finalized_block: &BeaconBlock<E>,
         finalized_block_root: Hash256,
     ) -> Result<()>;
+
+    /// Returns the latest message for a given validator index.
+    fn latest_message(&self, validator_index: usize) -> Option<(Hash256, Slot)>;
 }

eth2/lmd_ghost/src/reduced_tree.rs

@@ -109,6 +109,10 @@ where
             .update_root(new_block.slot, new_root)
             .map_err(|e| format!("update_finalized_root failed: {:?}", e))
     }
+
+    fn latest_message(&self, validator_index: usize) -> Option<(Hash256, Slot)> {
+        self.core.read().latest_message(validator_index)
+    }
 }
 
 struct ReducedTree<T, E> {
@@ -254,6 +258,13 @@ where
         Ok(head_node.block_hash)
     }
 
+    pub fn latest_message(&self, validator_index: usize) -> Option<(Hash256, Slot)> {
+        match self.latest_votes.get_ref(validator_index) {
+            Some(Some(v)) => Some((v.hash.clone(), v.slot.clone())),
+            _ => None,
+        }
+    }
+
     fn find_head_from<'a>(&'a self, start_node: &'a Node) -> Result<&'a Node> {
         if start_node.does_not_have_children() {
             Ok(start_node)
@@ -600,11 +611,7 @@ where
         let block = self.get_block(child)?;
         let state = self.get_state(block.state_root)?;
 
-        Ok(BlockRootsIterator::owned(
+        Ok(BlockRootsIterator::owned(self.store.clone(), state))
-            self.store.clone(),
-            state,
-            block.slot - 1,
-        ))
     }
 
     /// Verify the integrity of `self`. Returns `Ok(())` if the tree has integrity, otherwise returns `Err(description)`.
@@ -769,6 +776,10 @@ where
         &self.0[i]
     }
 
+    pub fn get_ref(&self, i: usize) -> Option<&T> {
+        self.0.get(i)
+    }
+
     pub fn insert(&mut self, i: usize, element: T) {
         self.ensure(i);
         self.0[i] = element;

eth2/operation_pool/src/lib.rs

@@ -15,9 +15,10 @@ use state_processing::per_block_processing::errors::{
     ExitValidationError, ProposerSlashingValidationError, TransferValidationError,
 };
 use state_processing::per_block_processing::{
-    get_slashable_indices_modular, verify_attestation, verify_attestation_time_independent_only,
+    get_slashable_indices_modular, verify_attestation_for_block_inclusion,
     verify_attester_slashing, verify_exit, verify_exit_time_independent_only,
     verify_proposer_slashing, verify_transfer, verify_transfer_time_independent_only,
+    VerifySignatures,
 };
 use std::collections::{btree_map::Entry, hash_map, BTreeMap, HashMap, HashSet};
 use std::marker::PhantomData;
@@ -64,15 +65,16 @@ impl<T: EthSpec> OperationPool<T> {
     }
 
     /// Insert an attestation into the pool, aggregating it with existing attestations if possible.
+    ///
+    /// ## Note
+    ///
+    /// This function assumes the given `attestation` is valid.
     pub fn insert_attestation(
         &self,
         attestation: Attestation<T>,
         state: &BeaconState<T>,
         spec: &ChainSpec,
     ) -> Result<(), AttestationValidationError> {
-        // Check that attestation signatures are valid.
-        verify_attestation_time_independent_only(state, &attestation, spec)?;
-
         let id = AttestationId::from_data(&attestation.data, state, spec);
 
         // Take a write lock on the attestations map.
@@ -128,7 +130,15 @@ impl<T: EthSpec> OperationPool<T> {
             })
             .flat_map(|(_, attestations)| attestations)
             // That are valid...
-            .filter(|attestation| verify_attestation(state, attestation, spec).is_ok())
+            .filter(|attestation| {
+                verify_attestation_for_block_inclusion(
+                    state,
+                    attestation,
+                    spec,
+                    VerifySignatures::True,
+                )
+                .is_ok()
+            })
             .map(|att| AttMaxCover::new(att, earliest_attestation_validators(att, state)));
 
         maximum_cover(valid_attestations, T::MaxAttestations::to_usize())

eth2/state_processing/src/per_block_processing.rs

@@ -15,8 +15,7 @@ pub use is_valid_indexed_attestation::{
     is_valid_indexed_attestation, is_valid_indexed_attestation_without_signature,
 };
 pub use verify_attestation::{
-    verify_attestation, verify_attestation_time_independent_only,
+    verify_attestation_for_block_inclusion, verify_attestation_for_state,
-    verify_attestation_without_signature,
 };
 pub use verify_deposit::{
     get_existing_validator_index, verify_deposit_merkle_proof, verify_deposit_signature,
@@ -37,6 +36,12 @@ mod verify_exit;
 mod verify_proposer_slashing;
 mod verify_transfer;
 
+#[derive(PartialEq)]
+pub enum VerifySignatures {
+    True,
+    False,
+}
+
 /// Updates the state for a new block, whilst validating that the block is valid.
 ///
 /// Returns `Ok(())` if the block is valid and the state was successfully updated. Otherwise
@@ -312,7 +317,8 @@ pub fn process_attestations<T: EthSpec>(
         .par_iter()
         .enumerate()
         .try_for_each(|(i, attestation)| {
-            verify_attestation(state, attestation, spec).map_err(|e| e.into_with_index(i))
+            verify_attestation_for_block_inclusion(state, attestation, spec, VerifySignatures::True)
+                .map_err(|e| e.into_with_index(i))
         })?;
 
     // Update the state in series.

eth2/state_processing/src/per_block_processing/verify_attestation.rs

@@ -1,4 +1,5 @@
 use super::errors::{AttestationInvalid as Invalid, AttestationValidationError as Error};
+use super::VerifySignatures;
 use crate::common::get_indexed_attestation;
 use crate::per_block_processing::{
     is_valid_indexed_attestation, is_valid_indexed_attestation_without_signature,
@@ -6,67 +7,25 @@ use crate::per_block_processing::{
 use tree_hash::TreeHash;
 use types::*;
 
-/// Indicates if an `Attestation` is valid to be included in a block in the current epoch of the
+/// Returns `Ok(())` if the given `attestation` is valid to be included in a block that is applied
-/// given state.
+/// to `state`. Otherwise, returns a descriptive `Err`.
 ///
-/// Returns `Ok(())` if the `Attestation` is valid, otherwise indicates the reason for invalidity.
+/// Optionally verifies the aggregate signature, depending on `verify_signatures`.
 ///
 /// Spec v0.8.0
-pub fn verify_attestation<T: EthSpec>(
+pub fn verify_attestation_for_block_inclusion<T: EthSpec>(
     state: &BeaconState<T>,
     attestation: &Attestation<T>,
     spec: &ChainSpec,
-) -> Result<(), Error> {
+    verify_signatures: VerifySignatures,
-    verify_attestation_parametric(state, attestation, spec, true, false)
-}
-
-/// Like `verify_attestation` but doesn't run checks which may become true in future states.
-pub fn verify_attestation_time_independent_only<T: EthSpec>(
-    state: &BeaconState<T>,
-    attestation: &Attestation<T>,
-    spec: &ChainSpec,
-) -> Result<(), Error> {
-    verify_attestation_parametric(state, attestation, spec, true, true)
-}
-
-/// Indicates if an `Attestation` is valid to be included in a block in the current epoch of the
-/// given state, without validating the aggregate signature.
-///
-/// Returns `Ok(())` if the `Attestation` is valid, otherwise indicates the reason for invalidity.
-///
-/// Spec v0.8.0
-pub fn verify_attestation_without_signature<T: EthSpec>(
-    state: &BeaconState<T>,
-    attestation: &Attestation<T>,
-    spec: &ChainSpec,
-) -> Result<(), Error> {
-    verify_attestation_parametric(state, attestation, spec, false, false)
-}
-
-/// Indicates if an `Attestation` is valid to be included in a block in the current epoch of the
-/// given state, optionally validating the aggregate signature.
-///
-///
-/// Spec v0.8.0
-fn verify_attestation_parametric<T: EthSpec>(
-    state: &BeaconState<T>,
-    attestation: &Attestation<T>,
-    spec: &ChainSpec,
-    verify_signature: bool,
-    time_independent_only: bool,
 ) -> Result<(), Error> {
     let data = &attestation.data;
-    verify!(
-        data.crosslink.shard < T::ShardCount::to_u64(),
-        Invalid::BadShard
-    );
 
     // Check attestation slot.
     let attestation_slot = state.get_attestation_data_slot(&data)?;
 
     verify!(
-        time_independent_only
+        attestation_slot + spec.min_attestation_inclusion_delay <= state.slot,
-            || attestation_slot + spec.min_attestation_inclusion_delay <= state.slot,
         Invalid::IncludedTooEarly {
             state: state.slot,
             delay: spec.min_attestation_inclusion_delay,
@@ -81,27 +40,47 @@ fn verify_attestation_parametric<T: EthSpec>(
         }
     );
 
-    // Verify the Casper FFG vote and crosslink data.
+    verify_attestation_for_state(state, attestation, spec, verify_signatures)
-    if !time_independent_only {
+}
-        let parent_crosslink = verify_casper_ffg_vote(attestation, state)?;
 
-        verify!(
+/// Returns `Ok(())` if `attestation` is a valid attestation to the chain that precedes the given
-            data.crosslink.parent_root == Hash256::from_slice(&parent_crosslink.tree_hash_root()),
+/// `state`.
-            Invalid::BadParentCrosslinkHash
+///
-        );
+/// Returns a descriptive `Err` if the attestation is malformed or does not accurately reflect the
-        verify!(
+/// prior blocks in `state`.
-            data.crosslink.start_epoch == parent_crosslink.end_epoch,
+///
-            Invalid::BadParentCrosslinkStartEpoch
+/// Spec v0.8.0
-        );
+pub fn verify_attestation_for_state<T: EthSpec>(
-        verify!(
+    state: &BeaconState<T>,
-            data.crosslink.end_epoch
+    attestation: &Attestation<T>,
-                == std::cmp::min(
+    spec: &ChainSpec,
-                    data.target.epoch,
+    verify_signature: VerifySignatures,
-                    parent_crosslink.end_epoch + spec.max_epochs_per_crosslink
+) -> Result<(), Error> {
-                ),
+    let data = &attestation.data;
-            Invalid::BadParentCrosslinkEndEpoch
+    verify!(
-        );
+        data.crosslink.shard < T::ShardCount::to_u64(),
-    }
+        Invalid::BadShard
+    );
+
+    // Verify the Casper FFG vote and crosslink data.
+    let parent_crosslink = verify_casper_ffg_vote(attestation, state)?;
+
+    verify!(
+        data.crosslink.parent_root == Hash256::from_slice(&parent_crosslink.tree_hash_root()),
+        Invalid::BadParentCrosslinkHash
+    );
+    verify!(
+        data.crosslink.start_epoch == parent_crosslink.end_epoch,
+        Invalid::BadParentCrosslinkStartEpoch
+    );
+    verify!(
+        data.crosslink.end_epoch
+            == std::cmp::min(
+                data.target.epoch,
+                parent_crosslink.end_epoch + spec.max_epochs_per_crosslink
+            ),
+        Invalid::BadParentCrosslinkEndEpoch
+    );
 
     // Crosslink data root is zero (to be removed in phase 1).
     verify!(
@@ -111,7 +90,7 @@ fn verify_attestation_parametric<T: EthSpec>(
 
     // Check signature and bitfields
     let indexed_attestation = get_indexed_attestation(state, attestation)?;
-    if verify_signature {
+    if verify_signature == VerifySignatures::True {
         is_valid_indexed_attestation(state, &indexed_attestation, spec)?;
     } else {
         is_valid_indexed_attestation_without_signature(state, &indexed_attestation, spec)?;

eth2/types/src/beacon_block.rs

@@ -61,6 +61,11 @@ impl<T: EthSpec> BeaconBlock<T> {
         }
     }
 
+    /// Returns the epoch corresponding to `self.slot`.
+    pub fn epoch(&self) -> Epoch {
+        self.slot.epoch(T::slots_per_epoch())
+    }
+
     /// Returns the `signed_root` of the block.
     ///
     /// Spec v0.8.1