Remove HeadSafetyStatus

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -75,7 +75,6 @@ use state_processing::{
     state_advance::{complete_state_advance, partial_state_advance},
     BlockSignatureStrategy, SigVerifiedOp, VerifyBlockRoot,
 };
-use std::borrow::Cow;
 use std::cmp::Ordering;
 use std::collections::HashMap;
 use std::collections::HashSet;
@@ -231,25 +230,6 @@ pub trait BeaconChainTypes: Send + Sync + 'static {
     type EthSpec: types::EthSpec;
 }
 
-/// Indicates the EL payload verification status of the head beacon block.
-#[derive(Debug, PartialEq)]
-pub enum HeadSafetyStatus {
-    /// The head block has either been verified by an EL or is does not require EL verification
-    /// (e.g., it is pre-merge or pre-terminal-block).
-    ///
-    /// If the block is post-terminal-block, `Some(execution_payload.block_hash)` is included with
-    /// the variant.
-    Safe(Option<ExecutionBlockHash>),
-    /// The head block execution payload has not yet been verified by an EL.
-    ///
-    /// The `execution_payload.block_hash` of the head block is returned.
-    Unsafe(ExecutionBlockHash),
-    /// The head block execution payload was deemed to be invalid by an EL.
-    ///
-    /// The `execution_payload.block_hash` of the head block is returned.
-    Invalid(ExecutionBlockHash),
-}
-
 pub type BeaconForkChoice<T> = ForkChoice<
     BeaconForkChoiceStore<
         <T as BeaconChainTypes>::EthSpec,
@@ -1430,8 +1410,13 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
     pub fn get_aggregated_attestation(
         &self,
         data: &AttestationData,
-    ) -> Option<Attestation<T::EthSpec>> {
-        self.naive_aggregation_pool.read().get(data)
+    ) -> Result<Option<Attestation<T::EthSpec>>, Error> {
+        if let Some(attestation) = self.naive_aggregation_pool.read().get(data) {
+            self.filter_optimistic_attestation(attestation)
+                .map(Option::Some)
+        } else {
+            Ok(None)
+        }
     }
 
     /// Returns an aggregated `Attestation`, if any, that has a matching
@@ -1442,10 +1427,43 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         &self,
         slot: Slot,
         attestation_data_root: &Hash256,
-    ) -> Option<Attestation<T::EthSpec>> {
-        self.naive_aggregation_pool
+    ) -> Result<Option<Attestation<T::EthSpec>>, Error> {
+        if let Some(attestation) = self
+            .naive_aggregation_pool
             .read()
             .get_by_slot_and_root(slot, attestation_data_root)
+        {
+            self.filter_optimistic_attestation(attestation)
+                .map(Option::Some)
+        } else {
+            Ok(None)
+        }
+    }
+
+    /// Returns `Ok(attestation)` if the supplied `attestation` references a valid
+    /// `beacon_block_root`.
+    fn filter_optimistic_attestation(
+        &self,
+        attestation: Attestation<T::EthSpec>,
+    ) -> Result<Attestation<T::EthSpec>, Error> {
+        let beacon_block_root = attestation.data.beacon_block_root;
+        match self
+            .fork_choice
+            .read()
+            .get_block_execution_status(&beacon_block_root)
+        {
+            // The attestation references a block that is not in fork choice, it must be
+            // pre-finalization.
+            None => Err(Error::CannotAttestToFinalizedBlock { beacon_block_root }),
+            // The attestation references a fully valid `beacon_block_root`.
+            Some(execution_status) if execution_status.is_valid_or_irrelevant() => Ok(attestation),
+            // The attestation references a block that has not been verified by an EL (i.e. it
+            // is optimistic or invalid). Don't return the block, return an error instead.
+            Some(execution_status) => Err(Error::HeadBlockNotFullyVerified {
+                beacon_block_root,
+                execution_status,
+            }),
+        }
     }
 
     /// Return an aggregated `SyncCommitteeContribution` matching the given `root`.
@@ -1481,6 +1499,8 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         //
         // In effect, the early attester cache prevents slow database IO from causing missed
         // head/target votes.
+        //
+        // The early attester cache should never contain an optimistically imported block.
         match self
             .early_attester_cache
             .try_attest(request_slot, request_index, &self.spec)
@@ -1597,6 +1617,22 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         }
         drop(head_timer);
 
+        // Only attest to a block if it is fully verified (i.e. not optimistic or invalid).
+        match self
+            .fork_choice
+            .read()
+            .get_block_execution_status(&beacon_block_root)
+        {
+            Some(execution_status) if execution_status.is_valid_or_irrelevant() => (),
+            Some(execution_status) => {
+                return Err(Error::HeadBlockNotFullyVerified {
+                    beacon_block_root,
+                    execution_status,
+                })
+            }
+            None => return Err(Error::HeadMissingFromForkChoice(beacon_block_root)),
+        };
+
         /*
          *  Phase 2/2:
          *
@@ -1657,64 +1693,6 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         })
     }
 
-    /// Produces an "unaggregated" attestation for the given `slot` and `index` that attests to
-    /// `beacon_block_root`. The provided `state` should match the `block.state_root` for the
-    /// `block` identified by `beacon_block_root`.
-    ///
-    /// The attestation doesn't _really_ have anything about it that makes it unaggregated per say,
-    /// however this function is only required in the context of forming an unaggregated
-    /// attestation. It would be an (undetectable) violation of the protocol to create a
-    /// `SignedAggregateAndProof` based upon the output of this function.
-    pub fn produce_unaggregated_attestation_for_block(
-        &self,
-        slot: Slot,
-        index: CommitteeIndex,
-        beacon_block_root: Hash256,
-        mut state: Cow<BeaconState<T::EthSpec>>,
-        state_root: Hash256,
-    ) -> Result<Attestation<T::EthSpec>, Error> {
-        let epoch = slot.epoch(T::EthSpec::slots_per_epoch());
-
-        if state.slot() > slot {
-            return Err(Error::CannotAttestToFutureState);
-        } else if state.current_epoch() < epoch {
-            let mut_state = state.to_mut();
-            // Only perform a "partial" state advance since we do not require the state roots to be
-            // accurate.
-            partial_state_advance(
-                mut_state,
-                Some(state_root),
-                epoch.start_slot(T::EthSpec::slots_per_epoch()),
-                &self.spec,
-            )?;
-            mut_state.build_committee_cache(RelativeEpoch::Current, &self.spec)?;
-        }
-
-        let committee_len = state.get_beacon_committee(slot, index)?.committee.len();
-
-        let target_slot = epoch.start_slot(T::EthSpec::slots_per_epoch());
-        let target_root = if state.slot() <= target_slot {
-            beacon_block_root
-        } else {
-            *state.get_block_root(target_slot)?
-        };
-
-        Ok(Attestation {
-            aggregation_bits: BitList::with_capacity(committee_len)?,
-            data: AttestationData {
-                slot,
-                index,
-                beacon_block_root,
-                source: state.current_justified_checkpoint(),
-                target: Checkpoint {
-                    epoch,
-                    root: target_root,
-                },
-            },
-            signature: AggregateSignature::empty(),
-        })
-    }
-
     /// Performs the same validation as `Self::verify_unaggregated_attestation_for_gossip`, but for
     /// multiple attestations using batch BLS verification. Batch verification can provide
     /// significant CPU-time savings compared to individual verification.
@@ -2678,13 +2656,20 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             }
         }
 
-        // If the block is recent enough, check to see if it becomes the head block. If so, apply it
-        // to the early attester cache. This will allow attestations to the block without waiting
-        // for the block and state to be inserted to the database.
+        // If the block is recent enough and it was not optimistically imported, check to see if it
+        // becomes the head block. If so, apply it to the early attester cache. This will allow
+        // attestations to the block without waiting for the block and state to be inserted to the
+        // database.
         //
         // Only performing this check on recent blocks avoids slowing down sync with lots of calls
         // to fork choice `get_head`.
-        if block.slot() + EARLY_ATTESTER_CACHE_HISTORIC_SLOTS >= current_slot {
+        //
+        // Optimistically imported blocks are not added to the cache since the cache is only useful
+        // for a small window of time and the complexity of keeping track of the optimistic status
+        // is not worth it.
+        if !payload_verification_status.is_optimistic()
+            && block.slot() + EARLY_ATTESTER_CACHE_HISTORIC_SLOTS >= current_slot
+        {
             let new_head_root = fork_choice
                 .get_head(current_slot, &self.spec)
                 .map_err(BeaconChainError::from)?;
@@ -4159,24 +4144,15 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         }
     }
 
-    /// Returns the status of the current head block, regarding the validity of the execution
-    /// payload.
-    pub fn head_safety_status(&self) -> Result<HeadSafetyStatus, BeaconChainError> {
+    /// Returns the execution status of the head block.
+    pub fn head_execution_status(&self) -> Result<ExecutionStatus, BeaconChainError> {
         let head = self.head_info()?;
         let head_block = self
             .fork_choice
             .read()
             .get_block(&head.block_root)
             .ok_or(BeaconChainError::HeadMissingFromForkChoice(head.block_root))?;
-
-        let status = match head_block.execution_status {
-            ExecutionStatus::Valid(block_hash) => HeadSafetyStatus::Safe(Some(block_hash)),
-            ExecutionStatus::Invalid(block_hash) => HeadSafetyStatus::Invalid(block_hash),
-            ExecutionStatus::Unknown(block_hash) => HeadSafetyStatus::Unsafe(block_hash),
-            ExecutionStatus::Irrelevant(_) => HeadSafetyStatus::Safe(None),
-        };
-
-        Ok(status)
+        Ok(head_block.execution_status)
     }
 
     /// This function takes a configured weak subjectivity `Checkpoint` and the latest finalized `Checkpoint`.

beacon_node/beacon_chain/src/block_verification.rs

@@ -1165,7 +1165,7 @@ impl<'a, T: BeaconChainTypes> FullyVerifiedBlock<'a, T> {
 
         // If the payload did not validate or invalidate the block, check to see if this block is
         // valid for optimistic import.
-        if payload_verification_status == PayloadVerificationStatus::NotVerified {
+        if payload_verification_status.is_optimistic() {
             let current_slot = chain
                 .slot_clock
                 .now()

beacon_node/beacon_chain/src/errors.rs

@@ -9,6 +9,7 @@ use crate::observed_aggregates::Error as ObservedAttestationsError;
 use crate::observed_attesters::Error as ObservedAttestersError;
 use crate::observed_block_producers::Error as ObservedBlockProducersError;
 use execution_layer::PayloadStatus;
+use fork_choice::ExecutionStatus;
 use futures::channel::mpsc::TrySendError;
 use operation_pool::OpPoolError;
 use safe_arith::ArithError;
@@ -162,6 +163,13 @@ pub enum BeaconChainError {
         fork_choice: Hash256,
     },
     InvalidSlot(Slot),
+    HeadBlockNotFullyVerified {
+        beacon_block_root: Hash256,
+        execution_status: ExecutionStatus,
+    },
+    CannotAttestToFinalizedBlock {
+        beacon_block_root: Hash256,
+    },
 }
 
 easy_from_to!(SlotProcessingError, BeaconChainError);

beacon_node/beacon_chain/src/execution_payload.rs

@@ -61,7 +61,7 @@ pub fn notify_new_payload<T: BeaconChainTypes>(
         Ok(status) => match status {
             PayloadStatus::Valid => Ok(PayloadVerificationStatus::Verified),
             PayloadStatus::Syncing | PayloadStatus::Accepted => {
-                Ok(PayloadVerificationStatus::NotVerified)
+                Ok(PayloadVerificationStatus::Optimistic)
             }
             PayloadStatus::Invalid {
                 latest_valid_hash, ..
@@ -193,7 +193,7 @@ pub fn validate_execution_payload_for_gossip<T: BeaconChainTypes>(
 
         let is_merge_transition_complete = match parent_block.execution_status {
             // Optimistically declare that an "unknown" status block has completed the merge.
-            ExecutionStatus::Valid(_) | ExecutionStatus::Unknown(_) => true,
+            ExecutionStatus::Valid(_) | ExecutionStatus::Optimistic(_) => true,
             // It's impossible for an irrelevant block to have completed the merge. It is pre-merge
             // by definition.
             ExecutionStatus::Irrelevant(_) => false,

beacon_node/beacon_chain/src/fork_revert.rs

@@ -172,7 +172,7 @@ pub fn reset_fork_choice_to_finalization<E: EthSpec, Hot: ItemStore<E>, Cold: It
         // retro-actively determine if they were valid or not.
         //
         // This scenario is so rare that it seems OK to double-verify some blocks.
-        let payload_verification_status = PayloadVerificationStatus::NotVerified;
+        let payload_verification_status = PayloadVerificationStatus::Optimistic;
 
         let (block, _) = block.deconstruct();
         fork_choice

beacon_node/beacon_chain/src/lib.rs

@@ -41,8 +41,8 @@ mod validator_pubkey_cache;
 
 pub use self::beacon_chain::{
     AttestationProcessingOutcome, BeaconChain, BeaconChainTypes, BeaconStore, ChainSegmentResult,
-    ForkChoiceError, HeadInfo, HeadSafetyStatus, ProduceBlockVerification, StateSkipConfig,
-    WhenSlotSkipped, INVALID_JUSTIFIED_PAYLOAD_SHUTDOWN_REASON, MAXIMUM_GOSSIP_CLOCK_DISPARITY,
+    ForkChoiceError, HeadInfo, ProduceBlockVerification, StateSkipConfig, WhenSlotSkipped,
+    INVALID_JUSTIFIED_PAYLOAD_SHUTDOWN_REASON, MAXIMUM_GOSSIP_CLOCK_DISPARITY,
 };
 pub use self::beacon_snapshot::BeaconSnapshot;
 pub use self::chain_config::ChainConfig;
@@ -53,6 +53,7 @@ pub use beacon_fork_choice_store::{BeaconForkChoiceStore, Error as ForkChoiceSto
 pub use block_verification::{BlockError, ExecutionPayloadError, GossipVerifiedBlock};
 pub use eth1_chain::{Eth1Chain, Eth1ChainBackend};
 pub use events::ServerSentEventHandler;
+pub use fork_choice::ExecutionStatus;
 pub use metrics::scrape_for_metrics;
 pub use parking_lot;
 pub use slot_clock;