diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 15bb47ef0a..03b4b22bf3 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/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()); } } diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 3ab07c6af1..a9a4e24be7 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -43,6 +43,12 @@ pub enum Error { block_root: Hash256, payload_verification_status: PayloadVerificationStatus, }, + MissingJustifiedBlock { + justified_checkpoint: Checkpoint, + }, + MissingFinalizedBlock { + finalized_checkpoint: Checkpoint, + }, } impl From for Error { @@ -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> { + 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> { + 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> { + // 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() diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 891eafabe9..d640c89c9f 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/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 { SszContainer::from(self).as_ssz_bytes() }