Add gossip conditions from spec v0.12.3 (#1667)

## Issue Addressed

NA

## Proposed Changes

There are four new conditions introduced in v0.12.3:

 1. _[REJECT]_ The attestation's epoch matches its target -- i.e. `attestation.data.target.epoch ==
  compute_epoch_at_slot(attestation.data.slot)`
1. _[REJECT]_ The attestation's target block is an ancestor of the block named in the LMD vote -- i.e.
  `get_ancestor(store, attestation.data.beacon_block_root, compute_start_slot_at_epoch(attestation.data.target.epoch)) == attestation.data.target.root`
1. _[REJECT]_ The committee index is within the expected range -- i.e. `data.index < get_committee_count_per_slot(state, data.target.epoch)`.
1. _[REJECT]_ The number of aggregation bits matches the committee size -- i.e.
  `len(attestation.aggregation_bits) == len(get_beacon_committee(state, data.slot, data.index))`.

This PR implements new logic to suit (1) and (2). Tests are added for (3) and (4), although they were already implicitly enforced.

## Additional Info

- There's a bit of edge-case with target root verification that I raised here: ethereum/consensus-specs#2001 (comment)
- I've had to add an `--ignore` to `cargo audit` to get CI to pass. See #1669

beacon_node/beacon_chain/src/attestation_verification.rs

@@ -37,6 +37,7 @@ use crate::{
     BeaconChain, BeaconChainError, BeaconChainTypes,
 };
 use bls::verify_signature_sets;
+use proto_array::Block as ProtoBlock;
 use slog::debug;
 use slot_clock::SlotClock;
 use state_processing::{
@@ -226,6 +227,21 @@ pub enum Error {
         head_block_slot: Slot,
         attestation_slot: Slot,
     },
+    /// The attestation has an invalid target epoch.
+    ///
+    /// ## Peer scoring
+    ///
+    /// The peer has sent an invalid message.
+    InvalidTargetEpoch { slot: Slot, epoch: Epoch },
+    /// The attestation references an invalid target block.
+    ///
+    /// ## Peer scoring
+    ///
+    /// The peer has sent an invalid message.
+    InvalidTargetRoot {
+        attestation: Hash256,
+        expected: Hash256,
+    },
     /// There was an error whilst processing the attestation. It is not known if it is valid or invalid.
     ///
     /// ## Peer scoring
@@ -425,6 +441,16 @@ impl<T: BeaconChainTypes> VerifiedUnaggregatedAttestation<T> {
         subnet_id: SubnetId,
         chain: &BeaconChain<T>,
     ) -> Result<Self, Error> {
+        let attestation_epoch = attestation.data.slot.epoch(T::EthSpec::slots_per_epoch());
+
+        // Check the attestation's epoch matches its target.
+        if attestation_epoch != attestation.data.target.epoch {
+            return Err(Error::InvalidTargetEpoch {
+                slot: attestation.data.slot,
+                epoch: attestation.data.target.epoch,
+            });
+        }
+
         // Ensure attestation is within the last ATTESTATION_PROPAGATION_SLOT_RANGE slots (within a
         // MAXIMUM_GOSSIP_CLOCK_DISPARITY allowance).
         //
@@ -433,16 +459,49 @@ impl<T: BeaconChainTypes> VerifiedUnaggregatedAttestation<T> {
 
         // Check to ensure that the attestation is "unaggregated". I.e., it has exactly one
         // aggregation bit set.
-        let num_aggreagtion_bits = attestation.aggregation_bits.num_set_bits();
-        if num_aggreagtion_bits != 1 {
-            return Err(Error::NotExactlyOneAggregationBitSet(num_aggreagtion_bits));
+        let num_aggregation_bits = attestation.aggregation_bits.num_set_bits();
+        if num_aggregation_bits != 1 {
+            return Err(Error::NotExactlyOneAggregationBitSet(num_aggregation_bits));
         }
 
         // Attestations must be for a known block. If the block is unknown, we simply drop the
         // attestation and do not delay consideration for later.
         //
         // Enforce a maximum skip distance for unaggregated attestations.
-        verify_head_block_is_known(chain, &attestation, chain.config.import_max_skip_slots)?;
+        let head_block =
+            verify_head_block_is_known(chain, &attestation, chain.config.import_max_skip_slots)?;
+
+        // Check the attestation target root.
+        let head_block_epoch = head_block.slot.epoch(T::EthSpec::slots_per_epoch());
+        if head_block_epoch > attestation_epoch {
+            // The attestation points to a head block from an epoch later than the attestation.
+            //
+            // Whilst this seems clearly invalid in the "spirit of the protocol", there is nothing
+            // in the specification to prevent these messages from propagating.
+            //
+            // Reference:
+            // https://github.com/ethereum/eth2.0-specs/pull/2001#issuecomment-699246659
+        } else {
+            let target_root = if head_block_epoch == attestation_epoch {
+                // If the block is in the same epoch as the attestation, then use the target root
+                // from the block.
+                head_block.target_root
+            } else {
+                // If the head block is from a previous epoch then skip slots will cause the head block
+                // root to become the target block root.
+                //
+                // We know the head block is from a previous epoch due to a previous check.
+                head_block.root
+            };
+
+            // Reject any attestation with an invalid target root.
+            if target_root != attestation.data.target.root {
+                return Err(Error::InvalidTargetRoot {
+                    attestation: attestation.data.target.root,
+                    expected: target_root,
+                });
+            }
+        }
 
         let (indexed_attestation, committees_per_slot) =
             obtain_indexed_attestation_and_committees_per_slot(chain, &attestation)?;
@@ -541,7 +600,7 @@ fn verify_head_block_is_known<T: BeaconChainTypes>(
     chain: &BeaconChain<T>,
     attestation: &Attestation<T::EthSpec>,
     max_skip_slots: Option<u64>,
-) -> Result<(), Error> {
+) -> Result<ProtoBlock, Error> {
     if let Some(block) = chain
         .fork_choice
         .read()
@@ -556,7 +615,7 @@ fn verify_head_block_is_known<T: BeaconChainTypes>(
                 });
             }
         }
-        Ok(())
+        Ok(block)
     } else {
         Err(Error::UnknownHeadBlock {
             beacon_block_root: attestation.data.beacon_block_root,
@@ -718,7 +777,7 @@ pub fn obtain_indexed_attestation_and_committees_per_slot<T: BeaconChainTypes>(
     map_attestation_committee(chain, attestation, |(committee, committees_per_slot)| {
         get_indexed_attestation(committee.committee, &attestation)
             .map(|attestation| (attestation, committees_per_slot))
-            .map_err(|e| BeaconChainError::from(e).into())
+            .map_err(Error::Invalid)
     })
 }
 

beacon_node/beacon_chain/tests/attestation_verification.rs

@@ -11,13 +11,15 @@ use beacon_chain::{
     BeaconChain, BeaconChainTypes,
 };
 use int_to_bytes::int_to_bytes32;
-use state_processing::per_slot_processing;
+use state_processing::{
+    per_block_processing::errors::AttestationValidationError, per_slot_processing,
+};
 use store::config::StoreConfig;
 use tree_hash::TreeHash;
 use types::{
-    test_utils::generate_deterministic_keypair, AggregateSignature, Attestation, EthSpec, Hash256,
-    Keypair, MainnetEthSpec, SecretKey, SelectionProof, SignedAggregateAndProof, SignedBeaconBlock,
-    SubnetId, Unsigned,
+    test_utils::generate_deterministic_keypair, AggregateSignature, Attestation, BeaconStateError,
+    BitList, EthSpec, Hash256, Keypair, MainnetEthSpec, SecretKey, SelectionProof,
+    SignedAggregateAndProof, SignedBeaconBlock, SubnetId, Unsigned,
 };
 
 pub type E = MainnetEthSpec;
@@ -582,6 +584,31 @@ fn unaggregated_gossip_verification() {
         };
     }
 
+    /*
+     * The following test ensures:
+     *
+     * Spec v0.12.3
+     *
+     * The committee index is within the expected range -- i.e. `data.index <
+     * get_committee_count_per_slot(state, data.target.epoch)`.
+     */
+    assert_invalid!(
+        "attestation with invalid committee index",
+        {
+            let mut a = valid_attestation.clone();
+            a.data.index = harness
+                .chain
+                .head()
+                .unwrap()
+                .beacon_state
+                .get_committee_count_at_slot(a.data.slot)
+                .unwrap();
+            a
+        },
+        subnet_id,
+        AttnError::NoCommitteeForSlotAndIndex { .. }
+    );
+
     /*
      * The following test ensures:
      *
@@ -642,6 +669,7 @@ fn unaggregated_gossip_verification() {
         {
             let mut a = valid_attestation.clone();
             a.data.slot = early_slot;
+            a.data.target.epoch = early_slot.epoch(E::slots_per_epoch());
             a
         },
         subnet_id,
@@ -654,6 +682,27 @@ fn unaggregated_gossip_verification() {
         if attestation_slot == early_slot && earliest_permissible_slot == current_slot - E::slots_per_epoch() - 1
     );
 
+    /*
+     * The following test ensures:
+     *
+     * Spec v0.12.3
+     *
+     * The attestation's epoch matches its target -- i.e. `attestation.data.target.epoch ==
+     *   compute_epoch_at_slot(attestation.data.slot)`
+     *
+     */
+
+    assert_invalid!(
+        "attestation with invalid target epoch",
+        {
+            let mut a = valid_attestation.clone();
+            a.data.target.epoch += 1;
+            a
+        },
+        subnet_id,
+        AttnError::InvalidTargetEpoch { .. }
+    );
+
     /*
      * The following two tests ensure:
      *
@@ -694,6 +743,32 @@ fn unaggregated_gossip_verification() {
         AttnError::NotExactlyOneAggregationBitSet(2)
     );
 
+    /*
+     * The following test ensures:
+     *
+     * Spec v0.12.3
+     *
+     * The number of aggregation bits matches the committee size -- i.e.
+     *   `len(attestation.aggregation_bits) == len(get_beacon_committee(state, data.slot,
+     *   data.index))`.
+     */
+    assert_invalid!(
+        "attestation with invalid bitfield",
+        {
+            let mut a = valid_attestation.clone();
+            let bits = a.aggregation_bits.iter().collect::<Vec<_>>();
+            a.aggregation_bits = BitList::with_capacity(bits.len() + 1).unwrap();
+            for (i, bit) in bits.into_iter().enumerate() {
+                a.aggregation_bits.set(i, bit).unwrap();
+            }
+            a
+        },
+        subnet_id,
+        AttnError::Invalid(AttestationValidationError::BeaconStateError(
+            BeaconStateError::InvalidBitfield
+        ))
+    );
+
     /*
      * The following test ensures that:
      *
@@ -717,6 +792,26 @@ fn unaggregated_gossip_verification() {
         if beacon_block_root == unknown_root
     );
 
+    /*
+     * The following test ensures that:
+     *
+     * Spec v0.12.3
+     *
+     * The attestation's target block is an ancestor of the block named in the LMD vote
+     */
+
+    let unknown_root = Hash256::from_low_u64_le(424242);
+    assert_invalid!(
+        "attestation with invalid target root",
+        {
+            let mut a = valid_attestation.clone();
+            a.data.target.root = unknown_root;
+            a
+        },
+        subnet_id,
+        AttnError::InvalidTargetRoot { .. }
+    );
+
     /*
      * The following test ensures that:
      *

beacon_node/network/src/beacon_processor/worker.rs

@@ -847,6 +847,32 @@ impl<T: BeaconChainTypes> Worker<T> {
                 );
                 self.penalize_peer(peer_id.clone(), PeerAction::LowToleranceError);
             }
+            AttnError::InvalidTargetEpoch { .. } => {
+                /*
+                 * The attestation is malformed.
+                 *
+                 * The peer has published an invalid consensus message.
+                 */
+                self.propagate_validation_result(
+                    message_id,
+                    peer_id.clone(),
+                    MessageAcceptance::Reject,
+                );
+                self.penalize_peer(peer_id.clone(), PeerAction::LowToleranceError);
+            }
+            AttnError::InvalidTargetRoot { .. } => {
+                /*
+                 * The attestation is malformed.
+                 *
+                 * The peer has published an invalid consensus message.
+                 */
+                self.propagate_validation_result(
+                    message_id,
+                    peer_id.clone(),
+                    MessageAcceptance::Reject,
+                );
+                self.penalize_peer(peer_id.clone(), PeerAction::LowToleranceError);
+            }
             AttnError::TooManySkippedSlots {
                 head_block_slot,
                 attestation_slot,

beacon_node/network/src/metrics.rs

@@ -244,7 +244,9 @@ lazy_static! {
         "beacon_processor_aggregated_attestation_imported_total",
         "Total number of aggregated attestations imported to fork choice, etc."
     );
+}
 
+lazy_static! {
     /*
      * Attestation Errors
      */
@@ -336,6 +338,14 @@ lazy_static! {
         "gossipsub_attestation_error_invalid_too_many_skipped_slots",
         "Count of a specific error type (see metric name)"
     );
+    pub static ref GOSSIP_ATTESTATION_ERROR_INVALID_TARGET_ROOT: Result<IntCounter> = try_create_int_counter(
+        "gossip_attestation_error_invalid_target_root",
+        "Count of a specific error type (see metric name)"
+    );
+    pub static ref GOSSIP_ATTESTATION_ERROR_INVALID_TARGET_EPOCH: Result<IntCounter> = try_create_int_counter(
+        "gossip_attestation_error_invalid_target_epoch",
+        "Count of a specific error type (see metric name)"
+    );
     pub static ref GOSSIP_ATTESTATION_ERROR_BEACON_CHAIN_ERROR: Result<IntCounter> = try_create_int_counter(
         "gossipsub_attestation_error_beacon_chain_error",
         "Count of a specific error type (see metric name)"
@@ -393,6 +403,12 @@ pub fn register_attestation_error(error: &AttnError) {
             inc_counter(&GOSSIP_ATTESTATION_ERROR_INVALID_SUBNET_ID)
         }
         AttnError::Invalid(_) => inc_counter(&GOSSIP_ATTESTATION_ERROR_INVALID_STATE_PROCESSING),
+        AttnError::InvalidTargetRoot { .. } => {
+            inc_counter(&GOSSIP_ATTESTATION_ERROR_INVALID_TARGET_ROOT)
+        }
+        AttnError::InvalidTargetEpoch { .. } => {
+            inc_counter(&GOSSIP_ATTESTATION_ERROR_INVALID_TARGET_EPOCH)
+        }
         AttnError::TooManySkippedSlots { .. } => {
             inc_counter(&GOSSIP_ATTESTATION_ERROR_INVALID_TOO_MANY_SKIPPED_SLOTS)
         }

consensus/proto_array/src/proto_array_fork_choice.rs

@@ -18,6 +18,7 @@ pub struct VoteTracker {
 /// A block that is to be applied to the fork choice.
 ///
 /// A simplified version of `types::BeaconBlock`.
+#[derive(Clone, Debug, PartialEq)]
 pub struct Block {
     pub slot: Slot,
     pub root: Hash256,