Merge pull request #5 from paulhauner/optimistic_conditions_paul

Suggestions for `optimistic_conditions` branch

beacon_node/beacon_chain/src/block_verification.rs

@@ -55,11 +55,9 @@ use crate::{
 };
 use eth2::types::EventKind;
 use execution_layer::PayloadStatusV1Status;
-use fork_choice::{
-    Error as ForkChoiceError, ForkChoice, ForkChoiceStore, PayloadVerificationStatus,
-};
+use fork_choice::{ForkChoice, ForkChoiceStore, PayloadVerificationStatus};
 use parking_lot::RwLockReadGuard;
-use proto_array::{Block as ProtoBlock, ExecutionStatus};
+use proto_array::Block as ProtoBlock;
 use safe_arith::ArithError;
 use slog::{debug, error, Logger};
 use slot_clock::SlotClock;
@@ -1135,31 +1133,26 @@ impl<'a, T: BeaconChainTypes> FullyVerifiedBlock<'a, T> {
         // `randao` may change.
         let payload_verification_status = notify_new_payload(chain, &state, block.message())?;
 
+        // 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 {
-            // Check the optimistic sync conditions before going further
-            // https://github.com/ethereum/consensus-specs/blob/v1.1.9/sync/optimistic.md#when-to-optimistically-import-blocks
             let current_slot = chain
                 .slot_clock
                 .now()
                 .ok_or(BeaconChainError::UnableToReadSlot)?;
-            // pass if current slot is at least SAFE_SLOTS_TO_IMPORT_OPTIMISTICALLY ahead of the block
-            if current_slot - block.slot() < chain.spec.safe_slots_to_import_optimistically {
-                // pass if the justified checkpoint has execution enabled
-                let justified_root = state.current_justified_checkpoint().root;
-                let justified_checkpoint_execution_status = chain
-                    .fork_choice
-                    .read()
-                    .get_block(&justified_root)
-                    .map(|block| block.execution_status)
-                    .ok_or(BeaconChainError::ForkChoiceError(
-                        ForkChoiceError::MissingProtoArrayBlock(justified_root),
-                    ))?;
-                if matches!(
-                    justified_checkpoint_execution_status,
-                    ExecutionStatus::Irrelevant(_)
-                ) {
-                    return Err(ExecutionPayloadError::UnverifiedNonOptimisticCandidate.into());
-                }
+
+            if !chain
+                .fork_choice
+                .read()
+                .is_optimistic_candidate_block(
+                    current_slot,
+                    block.slot(),
+                    &block.parent_root(),
+                    &chain.spec,
+                )
+                .map_err(BeaconChainError::from)?
+            {
+                return Err(ExecutionPayloadError::UnverifiedNonOptimisticCandidate.into());
             }
         }
 

consensus/fork_choice/src/fork_choice.rs

@@ -43,6 +43,12 @@ pub enum Error<T> {
         block_root: Hash256,
         payload_verification_status: PayloadVerificationStatus,
     },
+    MissingJustifiedBlock {
+        justified_checkpoint: Checkpoint,
+    },
+    MissingFinalizedBlock {
+        finalized_checkpoint: Checkpoint,
+    },
 }
 
 impl<T> From<InvalidAttestation> for Error<T> {
@@ -875,12 +881,83 @@ where
         }
     }
 
+    /// Returns the `ProtoBlock` for the justified checkpoint.
+    ///
+    /// ## Notes
+    ///
+    /// This does *not* return the "best justified checkpoint". It returns the justified checkpoint
+    /// that is used for computing balances.
+    pub fn get_justified_block(&self) -> Result<ProtoBlock, Error<T::Error>> {
+        let justified_checkpoint = self.justified_checkpoint();
+        self.get_block(&justified_checkpoint.root)
+            .ok_or(Error::MissingJustifiedBlock {
+                justified_checkpoint,
+            })
+    }
+
+    /// Returns the `ProtoBlock` for the finalized checkpoint.
+    pub fn get_finalized_block(&self) -> Result<ProtoBlock, Error<T::Error>> {
+        let finalized_checkpoint = self.finalized_checkpoint();
+        self.get_block(&finalized_checkpoint.root)
+            .ok_or(Error::MissingFinalizedBlock {
+                finalized_checkpoint,
+            })
+    }
+
     /// Return `true` if `block_root` is equal to the finalized root, or a known descendant of it.
     pub fn is_descendant_of_finalized(&self, block_root: Hash256) -> bool {
         self.proto_array
             .is_descendant(self.fc_store.finalized_checkpoint().root, block_root)
     }
 
+    /// Returns `Ok(false)` if a block is not viable to be imported optimistically.
+    ///
+    /// ## Notes
+    ///
+    /// Equivalent to the function with the same name in the optimistic sync specs:
+    ///
+    /// https://github.com/ethereum/consensus-specs/blob/dev/sync/optimistic.md#helpers
+    pub fn is_optimistic_candidate_block(
+        &self,
+        current_slot: Slot,
+        block_slot: Slot,
+        block_parent_root: &Hash256,
+        spec: &ChainSpec,
+    ) -> Result<bool, Error<T::Error>> {
+        // If the block is sufficiently old, import it.
+        if block_slot + spec.safe_slots_to_import_optimistically <= current_slot {
+            return Ok(true);
+        }
+
+        // If the justified block has execution enabled, then optimistically import any block.
+        if self
+            .get_justified_block()?
+            .execution_status
+            .is_execution_enabled()
+        {
+            return Ok(true);
+        }
+
+        // If the block has an ancestor with a verified parent, import this block.
+        //
+        // TODO: This condition is not yet merged into the spec. See:
+        //
+        // https://github.com/ethereum/consensus-specs/pull/2841
+        //
+        // ## Note
+        //
+        // If `block_parent_root` is unknown this iter will always return `None`.
+        if self
+            .proto_array
+            .iter_nodes(block_parent_root)
+            .any(|node| node.execution_status.is_valid())
+        {
+            return Ok(true);
+        }
+
+        Ok(false)
+    }
+
     /// Return the current finalized checkpoint.
     pub fn finalized_checkpoint(&self) -> Checkpoint {
         *self.fc_store.finalized_checkpoint()

consensus/proto_array/src/proto_array_fork_choice.rs

@@ -1,5 +1,5 @@
 use crate::error::Error;
-use crate::proto_array::{ProposerBoost, ProtoArray};
+use crate::proto_array::{Iter, ProposerBoost, ProtoArray};
 use crate::ssz_container::SszContainer;
 use serde_derive::{Deserialize, Serialize};
 use ssz::{Decode, Encode};
@@ -37,6 +37,14 @@ pub enum ExecutionStatus {
 }
 
 impl ExecutionStatus {
+    pub fn is_valid(&self) -> bool {
+        matches!(self, ExecutionStatus::Valid(_))
+    }
+
+    pub fn is_execution_enabled(&self) -> bool {
+        !matches!(self, ExecutionStatus::Irrelevant(_))
+    }
+
     pub fn irrelevant() -> Self {
         ExecutionStatus::Irrelevant(false)
     }
@@ -302,6 +310,11 @@ impl ProtoArrayForkChoice {
         }
     }
 
+    /// See `ProtoArray::iter_nodes`
+    pub fn iter_nodes<'a>(&'a self, block_root: &Hash256) -> Iter<'a> {
+        self.proto_array.iter_nodes(block_root)
+    }
+
     pub fn as_bytes(&self) -> Vec<u8> {
         SszContainer::from(self).as_ssz_bytes()
     }