[Merged by Bors] - Restrict fork choice getters to finalized blocks

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -3,8 +3,9 @@ use crate::attestation_verification::{
     VerifiedUnaggregatedAttestation,
 };
 use crate::block_verification::{
-    check_block_relevancy, get_block_root, signature_verify_chain_segment, BlockError,
-    FullyVerifiedBlock, GossipVerifiedBlock, IntoFullyVerifiedBlock,
+    check_block_is_finalized_descendant, check_block_relevancy, get_block_root,
+    signature_verify_chain_segment, BlockError, FullyVerifiedBlock, GossipVerifiedBlock,
+    IntoFullyVerifiedBlock,
 };
 use crate::errors::{BeaconChainError as Error, BlockProductionError};
 use crate::eth1_chain::{Eth1Chain, Eth1ChainBackend};
@@ -1214,6 +1215,14 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
                 // However, we will potentially get a `ParentUnknown` on a later block. The sync
                 // protocol will need to ensure this is handled gracefully.
                 Err(BlockError::WouldRevertFinalizedSlot { .. }) => continue,
+                // The block has a known parent that does not descend from the finalized block.
+                // There is no need to process this block or any children.
+                Err(BlockError::NotFinalizedDescendant { block_parent_root }) => {
+                    return ChainSegmentResult::Failed {
+                        imported_blocks,
+                        error: BlockError::NotFinalizedDescendant { block_parent_root },
+                    }
+                }
                 // If there was an error whilst determining if the block was invalid, return that
                 // error.
                 Err(BlockError::BeaconChainError(e)) => {
@@ -1415,7 +1424,6 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         fully_verified_block: FullyVerifiedBlock<T>,
     ) -> Result<Hash256, BlockError<T::EthSpec>> {
         let signed_block = fully_verified_block.block;
-        let block = &signed_block.message;
         let block_root = fully_verified_block.block_root;
         let state = fully_verified_block.state;
         let parent_block = fully_verified_block.parent_block;
@@ -1427,7 +1435,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
 
         // Iterate through the attestations in the block and register them as an "observed
         // attestation". This will stop us from propagating them on the gossip network.
-        for a in &block.body.attestations {
+        for a in &signed_block.message.body.attestations {
             match self.observed_attestations.observe_attestation(a, None) {
                 // If the observation was successful or if the slot for the attestation was too
                 // low, continue.
@@ -1477,6 +1485,11 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
 
         let mut fork_choice = self.fork_choice.write();
 
+        // Do not import a block that doesn't descend from the finalized root.
+        let signed_block =
+            check_block_is_finalized_descendant::<T, _>(signed_block, &fork_choice, &self.store)?;
+        let block = &signed_block.message;
+
         // Register the new block with the fork choice service.
         {
             let _fork_choice_block_timer =

beacon_node/beacon_chain/src/block_verification.rs

@@ -48,6 +48,7 @@ use crate::{
     },
     metrics, BeaconChain, BeaconChainError, BeaconChainTypes, BeaconSnapshot,
 };
+use fork_choice::{ForkChoice, ForkChoiceStore};
 use parking_lot::RwLockReadGuard;
 use slog::{error, Logger};
 use slot_clock::SlotClock;
@@ -62,7 +63,7 @@ use std::borrow::Cow;
 use std::convert::TryFrom;
 use std::fs;
 use std::io::Write;
-use store::{Error as DBError, HotStateSummary, StoreOp};
+use store::{Error as DBError, HotColdDB, HotStateSummary, StoreOp};
 use tree_hash::TreeHash;
 use types::{
     BeaconBlock, BeaconState, BeaconStateError, ChainSpec, CloneConfig, EthSpec, Hash256,
@@ -118,6 +119,13 @@ pub enum BlockError<T: EthSpec> {
         block_slot: Slot,
         finalized_slot: Slot,
     },
+    /// The block conflicts with finalization, no need to propagate.
+    ///
+    /// ## Peer scoring
+    ///
+    /// It's unclear if this block is valid, but it conflicts with finality and shouldn't be
+    /// imported.
+    NotFinalizedDescendant { block_parent_root: Hash256 },
     /// Block is already known, no need to re-import.
     ///
     /// ## Peer scoring
@@ -397,6 +405,15 @@ impl<T: BeaconChainTypes> GossipVerifiedBlock<T> {
             });
         }
 
+        // Do not process a block that doesn't descend from the finalized root.
+        //
+        // We check this *before* we load the parent so that we can return a more detailed error.
+        let block = check_block_is_finalized_descendant::<T, _>(
+            block,
+            &chain.fork_choice.read(),
+            &chain.store,
+        )?;
+
         let (mut parent, block) = load_parent(block, chain)?;
         let block_root = get_block_root(&block);
 
@@ -779,6 +796,36 @@ fn check_block_against_finalized_slot<T: BeaconChainTypes>(
     }
 }
 
+/// Returns `Ok(block)` if the block descends from the finalized root.
+pub fn check_block_is_finalized_descendant<T: BeaconChainTypes, F: ForkChoiceStore<T::EthSpec>>(
+    block: SignedBeaconBlock<T::EthSpec>,
+    fork_choice: &ForkChoice<F, T::EthSpec>,
+    store: &HotColdDB<T::EthSpec, T::HotStore, T::ColdStore>,
+) -> Result<SignedBeaconBlock<T::EthSpec>, BlockError<T::EthSpec>> {
+    if fork_choice.is_descendant_of_finalized(block.parent_root()) {
+        Ok(block)
+    } else {
+        // If fork choice does *not* consider the parent to be a descendant of the finalized block,
+        // then there are two more cases:
+        //
+        // 1. We have the parent stored in our database. Because fork-choice has confirmed the
+        //    parent is *not* in our post-finalization DAG, all other blocks must be either
+        //    pre-finalization or conflicting with finalization.
+        // 2. The parent is unknown to us, we probably want to download it since it might actually
+        //    descend from the finalized root.
+        if store
+            .item_exists::<SignedBeaconBlock<T::EthSpec>>(&block.parent_root())
+            .map_err(|e| BlockError::BeaconChainError(e.into()))?
+        {
+            Err(BlockError::NotFinalizedDescendant {
+                block_parent_root: block.parent_root(),
+            })
+        } else {
+            Err(BlockError::ParentUnknown(Box::new(block)))
+        }
+    }
+}
+
 /// Performs simple, cheap checks to ensure that the block is relevant to be imported.
 ///
 /// `Ok(block_root)` is returned if the block passes these checks and should progress with