Fix bug with fork choice, tidy

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -524,7 +524,13 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
 
                 // 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.
-                if state.current_epoch() == attestation_head_block.epoch()
+                //
+                // 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).
+                if state.current_epoch() == attestation.data.target.epoch
                     && (state
                         .get_block_root(attestation_head_block.slot)
                         .map(|root| *root == attestation.data.beacon_block_root)
@@ -546,7 +552,11 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             if let Some(outcome) = optional_outcome {
                 outcome
             } else {
-                // The state required to verify this attestation must be loaded from the database.
+                // Use the `data.beacon_block_root` to load the state from the latest non-skipped
+                // slot preceding the attestations 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)?
@@ -554,7 +564,21 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
 
                 // Ensure the state loaded from the database matches the state of the attestation
                 // head block.
-                for _ in state.slot.as_u64()..attestation_head_block.slot.as_u64() {
+                //
+                // 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)?;
                 }
 
@@ -639,36 +663,6 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         timer.observe_duration();
 
         result
-
-        /*
-        if self
-            .fork_choice
-            .should_process_attestation(state, &attestation)?
-        {
-            // TODO: check validation.
-            let indexed_attestation = common::get_indexed_attestation(state, &attestation)?;
-            per_block_processing::is_valid_indexed_attestation(
-                state,
-                &indexed_attestation,
-                &self.spec,
-            )?;
-            self.fork_choice.process_attestation(&state, &attestation)?;
-        }
-
-        let result = self
-            .op_pool
-            .insert_attestation(attestation, state, &self.spec);
-
-        timer.observe_duration();
-
-        if result.is_ok() {
-            self.metrics.attestation_processing_successes.inc();
-        }
-
-        result
-            .map(|_| AttestationProcessingOutcome::Processed)
-            .map_err(|e| Error::AttestationValidationError(e))
-            */
     }
 
     /// Accept some deposit and queue it for inclusion in an appropriate block.
@@ -735,7 +729,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);
@@ -781,6 +775,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/fork_choice.rs

@@ -19,6 +19,7 @@ pub enum Error {
 }
 
 pub struct ForkChoice<T: BeaconChainTypes> {
+    store: Arc<T::Store>,
     backend: T::LmdGhost,
     /// Used for resolving the `0x00..00` alias back to genesis.
     ///
@@ -38,6 +39,7 @@ 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,
         }
@@ -117,9 +119,19 @@ 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, block)?;
+            let block = self
+                .store
+                .get::<BeaconBlock<T::EthSpec>>(&attestation.data.beacon_block_root)?
+                .ok_or_else(|| Error::MissingBlock(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(())

beacon_node/beacon_chain/src/test_utils.rs

@@ -349,14 +349,12 @@ where
                             agg_sig
                         };
 
-                        let attestation = Attestation {
+                        vec.push(Attestation {
                             aggregation_bits,
                             data,
                             custody_bits,
                             signature,
-                        };
+                        })
-
-                        vec.push(attestation)
                     }
                 }
             });

beacon_node/beacon_chain/tests/tests.rs

@@ -342,27 +342,29 @@ fn free_attestations_added_to_fork_choice_some_none() {
     let state = &harness.chain.head().beacon_state;
     let fork_choice = &harness.chain.fork_choice;
 
-    let validators: Vec<usize> = (0..VALIDATOR_COUNT).collect();
+    let validator_slots: Vec<(usize, Slot)> = (0..VALIDATOR_COUNT)
-    let slots: Vec<Slot> = validators
+        .into_iter()
-        .iter()
+        .map(|validator_index| {
-        .map(|&v| {
+            let slot = state
-            state
+                .get_attestation_duties(validator_index, RelativeEpoch::Current)
-                .get_attestation_duties(v, RelativeEpoch::Current)
                 .expect("should get attester duties")
                 .unwrap()
-                .slot
+                .slot;
+
+            (validator_index, slot)
         })
         .collect();
-    let validator_slots: Vec<(&usize, Slot)> = validators.iter().zip(slots).collect();
 
     for (validator, slot) in validator_slots.clone() {
-        let latest_message = fork_choice.latest_message(*validator);
+        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 should be equal to attester duty."
+                "Latest message slot for {} should be equal to slot {}.",
+                validator,
+                slot
             )
         } else {
             assert!(

eth2/state_processing/src/per_block_processing/errors.rs

@@ -136,6 +136,9 @@ pub enum AttestationInvalid {
         delay: u64,
         attestation: Slot,
     },
+    /// The attestation is attesting to a state that is later than itself. (Viz., attesting to the
+    /// future).
+    AttestsToFutureState { state: Slot, attestation: Slot },
     /// Attestation slot is too far in the past to be included in a block.
     IncludedTooLate { state: Slot, attestation: Slot },
     /// Attestation target epoch does not match the current or previous epoch.

eth2/state_processing/src/per_block_processing/verify_attestation.rs

@@ -62,6 +62,17 @@ pub fn verify_attestation_for_state<T: EthSpec>(
         Invalid::BadShard
     );
 
+    let attestation_slot = state.get_attestation_data_slot(&data)?;
+
+    // An attestation cannot attest to a state that is later than itself.
+    verify!(
+        attestation_slot <= state.slot,
+        Invalid::AttestsToFutureState {
+            state: state.slot,
+            attestation: attestation_slot
+        }
+    );
+
     // Verify the Casper FFG vote and crosslink data.
     let parent_crosslink = verify_casper_ffg_vote(attestation, state)?;