Remove attestation processing from op pool

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -11,12 +11,15 @@ 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::{
-    AttesterSlashingValidationError, DepositValidationError, ExitValidationError,
+    errors::{
-    ProposerSlashingValidationError, TransferValidationError,
+        AttestationValidationError, AttesterSlashingValidationError, DepositValidationError,
+        ExitValidationError, ProposerSlashingValidationError, TransferValidationError,
+    },
+    verify_attestation_for_state, VerifySignatures,
 };
 use state_processing::{
-    common, per_block_processing, per_block_processing_without_verifying_block_signature,
+    per_block_processing, per_block_processing_without_verifying_block_signature,
     per_slot_processing, BlockProcessingError,
 };
 use std::sync::Arc;
@@ -58,6 +61,7 @@ pub enum BlockProcessingOutcome {
 pub enum AttestationProcessingOutcome {
     Processed,
     UnknownHeadBlock { beacon_block_root: Hash256 },
+    Invalid(AttestationValidationError),
 }
 
 pub trait BeaconChainTypes {
@@ -543,9 +547,6 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
                 }
             };
 
-            // TODO: we could try and see if the "speculative state" (e.g., self.state) can support
-            // this, without needing to load it from the db.
-
             if let Some(outcome) = optional_outcome {
                 outcome
             } else {
@@ -583,6 +584,25 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         }
     }
 
+    /// 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 `block.slot` and
+    /// `attestation.data.beacon_block_root` is in `state.block_roots`. (Viz., the attestation was
+    /// attesting to an ancestor of `state` from the same epoch as `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>,
@@ -592,6 +612,39 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         self.metrics.attestation_processing_requests.inc();
         let timer = self.metrics.attestation_processing_times.start_timer();
 
+        let result = 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();
+
+        result
+
+        /*
         if self
             .fork_choice
             .should_process_attestation(state, &attestation)?
@@ -619,6 +672,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         result
             .map(|_| AttestationProcessingOutcome::Processed)
             .map_err(|e| Error::AttestationValidationError(e))
+            */
     }
 
     /// Accept some deposit and queue it for inclusion in an appropriate block.

beacon_node/beacon_chain/src/errors.rs

@@ -26,6 +26,7 @@ pub enum BeaconChainError {
         previous_epoch: Epoch,
         new_epoch: Epoch,
     },
+    UnableToFindTargetRoot(Slot),
     BeaconStateError(BeaconStateError),
     DBInconsistent(String),
     DBError(store::Error),

beacon_node/beacon_chain/src/fork_choice.rs

@@ -20,7 +20,6 @@ pub enum Error {
 
 pub struct ForkChoice<T: BeaconChainTypes> {
     backend: T::LmdGhost,
-    store: Arc<T::Store>,
     /// 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
@@ -39,7 +38,6 @@ impl<T: BeaconChainTypes> ForkChoice<T> {
         genesis_block_root: Hash256,
     ) -> Self {
         Self {
-            store: store.clone(),
             backend: T::LmdGhost::new(store, genesis_block, genesis_block_root),
             genesis_block_root,
         }
@@ -119,7 +117,7 @@ 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(state, attestation)?;
+            self.process_attestation(state, attestation, block)?;
         }
 
         self.backend.process_block(block, block_root)?;
@@ -127,13 +125,14 @@ impl<T: BeaconChainTypes> ForkChoice<T> {
         Ok(())
     }
 
-    /// Process an attestation.
+    /// 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;
 
@@ -152,20 +151,13 @@ 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)?;
             }
         }
 

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

@@ -14,7 +14,9 @@ pub use self::verify_proposer_slashing::verify_proposer_slashing;
 pub use is_valid_indexed_attestation::{
     is_valid_indexed_attestation, is_valid_indexed_attestation_without_signature,
 };
-pub use verify_attestation::{verify_attestation_for_block, verify_attestation_for_state};
+pub use verify_attestation::{
+    verify_attestation_for_block_inclusion, verify_attestation_for_state,
+};
 pub use verify_deposit::{
     get_existing_validator_index, verify_deposit_merkle_proof, verify_deposit_signature,
 };
@@ -315,7 +317,7 @@ pub fn process_attestations<T: EthSpec>(
         .par_iter()
         .enumerate()
         .try_for_each(|(i, attestation)| {
-            verify_attestation_for_block(state, attestation, spec, VerifySignatures::True)
+            verify_attestation_for_block_inclusion(state, attestation, spec, VerifySignatures::True)
                 .map_err(|e| e.into_with_index(i))
         })?;
 

eth2/state_processing/src/per_block_processing/verify_attestation.rs

@@ -7,11 +7,13 @@ 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, optionally validating the aggregate signature.
+/// to `state`. Otherwise, returns a descriptive `Err`.
+///
+/// Optionally verifies the aggregate signature, depending on `verify_signatures`.
 ///
 /// Spec v0.8.0
-pub fn verify_attestation_for_block<T: EthSpec>(
+pub fn verify_attestation_for_block_inclusion<T: EthSpec>(
     state: &BeaconState<T>,
     attestation: &Attestation<T>,
     spec: &ChainSpec,
@@ -41,7 +43,7 @@ pub fn verify_attestation_for_block<T: EthSpec>(
     verify_attestation_for_state(state, attestation, spec, verify_signatures)
 }
 
-/// Returns `Ok(())` if `attestation` is a valid attestation to the chain that preceeds the given
+/// Returns `Ok(())` if `attestation` is a valid attestation to the chain that precedes the given
 /// `state`.
 ///
 /// Returns a descriptive `Err` if the attestation is malformed or does not accurately reflect the