Spec Atomicity bug updates

Author: realbigsean

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -50,8 +50,8 @@ use crate::{metrics, BeaconChainError};
 use eth2::types::{
     EventKind, SseBlock, SseChainReorg, SseFinalizedCheckpoint, SseHead, SseLateHead, SyncDuty,
 };
+use fork_choice::{AttestationFromBlock, ForkChoice};
 use execution_layer::ExecutionLayer;
-use fork_choice::ForkChoice;
 use futures::channel::mpsc::Sender;
 use itertools::process_results;
 use itertools::Itertools;
@@ -1698,7 +1698,11 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
 
         self.fork_choice
             .write()
-            .on_attestation(self.slot()?, verified.indexed_attestation())
+            .on_attestation(
+                self.slot()?,
+                verified.indexed_attestation(),
+                AttestationFromBlock::False,
+            )
             .map_err(Into::into)
     }
 
@@ -2470,7 +2474,11 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             let indexed_attestation = get_indexed_attestation(committee.committee, attestation)
                 .map_err(|e| BlockError::BeaconChainError(e.into()))?;
 
-            match fork_choice.on_attestation(current_slot, &indexed_attestation) {
+            match fork_choice.on_attestation(
+                current_slot,
+                &indexed_attestation,
+                AttestationFromBlock::True,
+            ) {
                 Ok(()) => Ok(()),
                 // Ignore invalid attestations whilst importing attestations from a block. The
                 // block might be very old and therefore the attestations useless to fork choice.

beacon_node/beacon_chain/src/beacon_fork_choice_store.rs

@@ -344,8 +344,8 @@ where
 pub struct PersistedForkChoiceStore {
     balances_cache: BalancesCache,
     time: Slot,
-    finalized_checkpoint: Checkpoint,
-    justified_checkpoint: Checkpoint,
+    pub finalized_checkpoint: Checkpoint,
+    pub justified_checkpoint: Checkpoint,
     justified_balances: Vec<u64>,
     best_justified_checkpoint: Checkpoint,
 }

beacon_node/beacon_chain/src/schema_change.rs

@@ -119,6 +119,11 @@ pub fn migrate_schema<T: BeaconChainTypes>(
                     .map_err(StoreError::SchemaMigrationError)?;
                 };
 
+                migrate_schema_6::update_store_justified_checkpoint::<T>(
+                    &mut persisted_fork_choice,
+                )
+                .map_err(StoreError::SchemaMigrationError)?;
+
                 // Store the converted fork choice store under the same key.
                 db.put_item::<PersistedForkChoice>(&FORK_CHOICE_DB_KEY, &persisted_fork_choice)?;
             }

beacon_node/beacon_chain/src/schema_change/migrate_schema_6.rs

@@ -21,6 +21,31 @@ use store::Error as StoreError;
 // selector.
 four_byte_option_impl!(four_byte_option_usize, usize);
 
+pub(crate) fn update_store_justified_checkpoint<T: BeaconChainTypes>(
+    persisted_fork_choice: &mut PersistedForkChoice,
+) -> Result<(), String> {
+    let bytes = persisted_fork_choice.fork_choice.proto_array_bytes.clone();
+    let container = SszContainer::from_ssz_bytes(bytes.as_slice()).unwrap();
+    let mut fork_choice: ProtoArrayForkChoice = container.into();
+
+    let justified_checkpoint = fork_choice
+        .core_proto_array()
+        .nodes
+        .iter()
+        .find_map(|node| {
+            (node.finalized_checkpoint
+                == Some(persisted_fork_choice.fork_choice_store.finalized_checkpoint))
+            .then(|| node.justified_checkpoint)
+            .flatten()
+        })
+        .ok_or("Proto node with current finalized checkpoint not found")?;
+
+    fork_choice.core_proto_array_mut().justified_checkpoint = justified_checkpoint;
+    persisted_fork_choice.fork_choice.proto_array_bytes = fork_choice.as_bytes();
+    persisted_fork_choice.fork_choice_store.justified_checkpoint = justified_checkpoint;
+    Ok(())
+}
+
 pub(crate) fn update_with_reinitialized_fork_choice<T: BeaconChainTypes>(
     persisted_fork_choice: &mut PersistedForkChoice,
     db: Arc<HotColdDB<T::EthSpec, T::HotStore, T::ColdStore>>,

consensus/fork_choice/src/fork_choice.rs

@@ -218,6 +218,15 @@ fn dequeue_attestations(
     std::mem::replace(queued_attestations, remaining)
 }
 
+/// Denotes whether an attestation we are processing received from a block or from gossip.
+/// Equivalent to the `is_from_block` bool in:
+///
+/// https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/fork-choice.md#validate_on_attestation
+pub enum AttestationFromBlock {
+    True,
+    False,
+}
+
 /// Provides an implementation of "Ethereum 2.0 Phase 0 -- Beacon Chain Fork Choice":
 ///
 /// https://github.com/ethereum/eth2.0-specs/blob/v0.12.1/specs/phase0/fork-choice.md#ethereum-20-phase-0----beacon-chain-fork-choice
@@ -537,25 +546,9 @@ where
         if state.finalized_checkpoint().epoch > self.fc_store.finalized_checkpoint().epoch {
             self.fc_store
                 .set_finalized_checkpoint(state.finalized_checkpoint());
-            let finalized_slot =
-                compute_start_slot_at_epoch::<E>(self.fc_store.finalized_checkpoint().epoch);
-
-            // Note: the `if` statement here is not part of the specification, but I claim that it
-            // is an optimization and equivalent to the specification. See this PR for more
-            // information:
-            //
-            // https://github.com/ethereum/eth2.0-specs/pull/1880
-            if *self.fc_store.justified_checkpoint() != state.current_justified_checkpoint()
-                && (state.current_justified_checkpoint().epoch
-                    > self.fc_store.justified_checkpoint().epoch
-                    || self
-                        .get_ancestor(self.fc_store.justified_checkpoint().root, finalized_slot)?
-                        != Some(self.fc_store.finalized_checkpoint().root))
-            {
-                self.fc_store
-                    .set_justified_checkpoint(state.current_justified_checkpoint())
-                    .map_err(Error::UnableToSetJustifiedCheckpoint)?;
-            }
+            self.fc_store
+                .set_justified_checkpoint(state.current_justified_checkpoint())
+                .map_err(Error::UnableToSetJustifiedCheckpoint)?;
         }
 
         let target_slot = block
@@ -629,6 +622,35 @@ where
         Ok(())
     }
 
+    /// Validates the `epoch` against the current time according to the fork choice store.
+    ///
+    /// ## Specification
+    ///
+    /// Equivalent to:
+    ///
+    /// https://github.com/ethereum/eth2.0-specs/blob/v0.12.1/specs/phase0/fork-choice.md#validate_target_epoch_against_current_time
+    fn validate_target_epoch_against_current_time(
+        &self,
+        target_epoch: Epoch,
+    ) -> Result<(), InvalidAttestation> {
+        let slot_now = self.fc_store.get_current_slot();
+        let epoch_now = slot_now.epoch(E::slots_per_epoch());
+
+        // Attestation must be from the current or previous epoch.
+        if target_epoch > epoch_now {
+            return Err(InvalidAttestation::FutureEpoch {
+                attestation_epoch: target_epoch,
+                current_epoch: epoch_now,
+            });
+        } else if target_epoch + 1 < epoch_now {
+            return Err(InvalidAttestation::PastEpoch {
+                attestation_epoch: target_epoch,
+                current_epoch: epoch_now,
+            });
+        }
+        Ok(())
+    }
+
     /// Validates the `indexed_attestation` for application to fork choice.
     ///
     /// ## Specification
@@ -639,6 +661,7 @@ where
     fn validate_on_attestation(
         &self,
         indexed_attestation: &IndexedAttestation<E>,
+        is_from_block: AttestationFromBlock,
     ) -> Result<(), InvalidAttestation> {
         // There is no point in processing an attestation with an empty bitfield. Reject
         // it immediately.
@@ -649,21 +672,10 @@ where
             return Err(InvalidAttestation::EmptyAggregationBitfield);
         }
 
-        let slot_now = self.fc_store.get_current_slot();
-        let epoch_now = slot_now.epoch(E::slots_per_epoch());
         let target = indexed_attestation.data.target;
 
-        // Attestation must be from the current or previous epoch.
-        if target.epoch > epoch_now {
-            return Err(InvalidAttestation::FutureEpoch {
-                attestation_epoch: target.epoch,
-                current_epoch: epoch_now,
-            });
-        } else if target.epoch + 1 < epoch_now {
-            return Err(InvalidAttestation::PastEpoch {
-                attestation_epoch: target.epoch,
-                current_epoch: epoch_now,
-            });
+        if matches!(is_from_block, AttestationFromBlock::False) {
+            self.validate_target_epoch_against_current_time(target.epoch)?;
         }
 
         if target.epoch != indexed_attestation.data.slot.epoch(E::slots_per_epoch()) {
@@ -746,6 +758,7 @@ where
         &mut self,
         current_slot: Slot,
         attestation: &IndexedAttestation<E>,
+        is_from_block: AttestationFromBlock,
     ) -> Result<(), Error<T::Error>> {
         // Ensure the store is up-to-date.
         self.update_time(current_slot)?;
@@ -767,7 +780,7 @@ where
             return Ok(());
         }
 
-        self.validate_on_attestation(attestation)?;
+        self.validate_on_attestation(attestation, is_from_block)?;
 
         if attestation.data.slot < self.fc_store.get_current_slot() {
             for validator_index in attestation.attesting_indices.iter() {

consensus/fork_choice/src/lib.rs

@@ -2,8 +2,8 @@ mod fork_choice;
 mod fork_choice_store;
 
 pub use crate::fork_choice::{
-    Error, ForkChoice, InvalidAttestation, InvalidBlock, PayloadVerificationStatus,
-    PersistedForkChoice, QueuedAttestation,
+    AttestationFromBlock, Error, ForkChoice, InvalidAttestation, InvalidBlock,
+    PayloadVerificationStatus, PersistedForkChoice, QueuedAttestation,
 };
 pub use fork_choice_store::ForkChoiceStore;
 pub use proto_array::Block as ProtoBlock;