Ignore blocks that skip a large distance from their parent (#1530)

## Proposed Changes

To mitigate the impact of minority forks on RAM and disk usage, this change rejects blocks whose parent lies more than 320 slots (10 epochs, ~1 hour) in the past. The behaviour is configurable via `lighthouse bn --max-skip-slots N`, and can be turned off entirely using `--max-skip-slots none`.

Co-authored-by: Paul Hauner <paul@paulhauner.com>

beacon_node/beacon_chain/src/attestation_verification.rs

@@ -220,6 +220,12 @@ pub enum Error {
     ///
     /// The peer has sent an invalid message.
     Invalid(AttestationValidationError),
+    /// The attestation head block is too far behind the attestation slot, causing many skip slots.
+    /// This is deemed a DoS risk.
+    TooManySkippedSlots {
+        head_block_slot: Slot,
+        attestation_slot: Slot,
+    },
     /// There was an error whilst processing the attestation. It is not known if it is valid or invalid.
     ///
     /// ## Peer scoring
@@ -319,6 +325,7 @@ impl<T: BeaconChainTypes> VerifiedAggregatedAttestation<T> {
         }?;
 
         // Ensure the block being voted for (attestation.data.beacon_block_root) passes validation.
+        // Don't enforce the skip slot restriction for aggregates.
         //
         // This indirectly checks to see if the `attestation.data.beacon_block_root` is in our fork
         // choice. Any known, non-finalized, processed block should be in fork choice, so this
@@ -327,7 +334,7 @@ impl<T: BeaconChainTypes> VerifiedAggregatedAttestation<T> {
         //
         // Attestations must be for a known block. If the block is unknown, we simply drop the
         // attestation and do not delay consideration for later.
-        verify_head_block_is_known(chain, &attestation)?;
+        verify_head_block_is_known(chain, &attestation, None)?;
 
         // Ensure that the attestation has participants.
         if attestation.aggregation_bits.is_zero() {
@@ -433,7 +440,9 @@ impl<T: BeaconChainTypes> VerifiedUnaggregatedAttestation<T> {
 
         // Attestations must be for a known block. If the block is unknown, we simply drop the
         // attestation and do not delay consideration for later.
-        verify_head_block_is_known(chain, &attestation)?;
+        //
+        // Enforce a maximum skip distance for unaggregated attestations.
+        verify_head_block_is_known(chain, &attestation, chain.config.import_max_skip_slots)?;
 
         let (indexed_attestation, committees_per_slot) =
             obtain_indexed_attestation_and_committees_per_slot(chain, &attestation)?;
@@ -531,12 +540,22 @@ impl<T: BeaconChainTypes> VerifiedUnaggregatedAttestation<T> {
 fn verify_head_block_is_known<T: BeaconChainTypes>(
     chain: &BeaconChain<T>,
     attestation: &Attestation<T::EthSpec>,
+    max_skip_slots: Option<u64>,
 ) -> Result<(), Error> {
-    if chain
+    if let Some(block) = chain
         .fork_choice
         .read()
-        .contains_block(&attestation.data.beacon_block_root)
+        .get_block(&attestation.data.beacon_block_root)
     {
+        // Reject any block that exceeds our limit on skipped slots.
+        if let Some(max_skip_slots) = max_skip_slots {
+            if block.slot > attestation.data.slot + max_skip_slots {
+                return Err(Error::TooManySkippedSlots {
+                    head_block_slot: block.slot,
+                    attestation_slot: attestation.data.slot,
+                });
+            }
+        }
         Ok(())
     } else {
         Err(Error::UnknownHeadBlock {

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -7,6 +7,7 @@ use crate::block_verification::{
     signature_verify_chain_segment, BlockError, FullyVerifiedBlock, GossipVerifiedBlock,
     IntoFullyVerifiedBlock,
 };
+use crate::chain_config::ChainConfig;
 use crate::errors::{BeaconChainError as Error, BlockProductionError};
 use crate::eth1_chain::{Eth1Chain, Eth1ChainBackend};
 use crate::events::{EventHandler, EventKind};
@@ -161,6 +162,8 @@ pub trait BeaconChainTypes: Send + Sync + 'static {
 /// operations and chooses a canonical head.
 pub struct BeaconChain<T: BeaconChainTypes> {
     pub spec: ChainSpec,
+    /// Configuration for `BeaconChain` runtime behaviour.
+    pub config: ChainConfig,
     /// Persistent storage for blocks, states, etc. Typically an on-disk store, such as LevelDB.
     pub store: Arc<HotColdDB<T::EthSpec, T::HotStore, T::ColdStore>>,
     /// Database migrator for running background maintenance on the store.

beacon_node/beacon_chain/src/block_verification.rs

@@ -92,6 +92,8 @@ pub enum BlockError<T: EthSpec> {
     /// It's unclear if this block is valid, but it cannot be processed without already knowing
     /// its parent.
     ParentUnknown(Box<SignedBeaconBlock<T>>),
+    /// The block skips too many slots and is a DoS risk.
+    TooManySkippedSlots { parent_slot: Slot, block_slot: Slot },
     /// The block slot is greater than the present slot.
     ///
     /// ## Peer scoring
@@ -645,6 +647,16 @@ impl<'a, T: BeaconChainTypes> FullyVerifiedBlock<'a, T> {
             return Err(BlockError::ParentUnknown(Box::new(block)));
         }
 
+        // Reject any block that exceeds our limit on skipped slots.
+        if let Some(max_skip_slots) = chain.config.import_max_skip_slots {
+            if block.slot() > parent.beacon_block.slot() + max_skip_slots {
+                return Err(BlockError::TooManySkippedSlots {
+                    parent_slot: parent.beacon_block.slot(),
+                    block_slot: block.slot(),
+                });
+            }
+        }
+
         /*
          *  Perform cursory checks to see if the block is even worth processing.
          */

beacon_node/beacon_chain/src/builder.rs

@@ -11,6 +11,7 @@ use crate::shuffling_cache::ShufflingCache;
 use crate::snapshot_cache::{SnapshotCache, DEFAULT_SNAPSHOT_CACHE_SIZE};
 use crate::timeout_rw_lock::TimeoutRwLock;
 use crate::validator_pubkey_cache::ValidatorPubkeyCache;
+use crate::ChainConfig;
 use crate::{
     BeaconChain, BeaconChainTypes, BeaconForkChoiceStore, BeaconSnapshot, Eth1Chain,
     Eth1ChainBackend, EventHandler,
@@ -110,6 +111,7 @@ pub struct BeaconChainBuilder<T: BeaconChainTypes> {
     pubkey_cache_path: Option<PathBuf>,
     validator_pubkey_cache: Option<ValidatorPubkeyCache>,
     spec: ChainSpec,
+    chain_config: ChainConfig,
     disabled_forks: Vec<String>,
     log: Option<Logger>,
     graffiti: Graffiti,
@@ -157,6 +159,7 @@ where
             disabled_forks: Vec::new(),
             validator_pubkey_cache: None,
             spec: TEthSpec::default_spec(),
+            chain_config: ChainConfig::default(),
             log: None,
             graffiti: Graffiti::default(),
         }
@@ -171,6 +174,15 @@ where
         self
     }
 
+    /// Sets the maximum number of blocks that will be skipped when processing
+    /// some consensus messages.
+    ///
+    /// Set to `None` for no limit.
+    pub fn import_max_skip_slots(mut self, n: Option<u64>) -> Self {
+        self.chain_config.import_max_skip_slots = n;
+        self
+    }
+
     /// Sets the store (database).
     ///
     /// Should generally be called early in the build chain.
@@ -406,6 +418,12 @@ where
         self
     }
 
+    /// Sets the `ChainConfig` that determines `BeaconChain` runtime behaviour.
+    pub fn chain_config(mut self, config: ChainConfig) -> Self {
+        self.chain_config = config;
+        self
+    }
+
     /// Consumes `self`, returning a `BeaconChain` if all required parameters have been supplied.
     ///
     /// An error will be returned at runtime if all required parameters have not been configured.
@@ -489,6 +507,7 @@ where
 
         let beacon_chain = BeaconChain {
             spec: self.spec,
+            config: self.chain_config,
             store,
             store_migrator: self
                 .store_migrator

beacon_node/beacon_chain/src/chain_config.rs

@@ -0,0 +1,20 @@
+use serde_derive::{Deserialize, Serialize};
+
+pub const DEFAULT_IMPORT_BLOCK_MAX_SKIP_SLOTS: u64 = 10 * 32;
+
+#[derive(Debug, PartialEq, Eq, Clone, Deserialize, Serialize)]
+pub struct ChainConfig {
+    /// Maximum number of slots to skip when importing a consensus message (e.g., block,
+    /// attestation, etc).
+    ///
+    /// If `None`, there is no limit.
+    pub import_max_skip_slots: Option<u64>,
+}
+
+impl Default for ChainConfig {
+    fn default() -> Self {
+        Self {
+            import_max_skip_slots: Some(DEFAULT_IMPORT_BLOCK_MAX_SKIP_SLOTS),
+        }
+    }
+}

beacon_node/beacon_chain/src/lib.rs

@@ -8,6 +8,7 @@ mod beacon_fork_choice_store;
 mod beacon_snapshot;
 mod block_verification;
 pub mod builder;
+pub mod chain_config;
 mod errors;
 pub mod eth1_chain;
 pub mod events;
@@ -32,6 +33,7 @@ pub use self::beacon_chain::{
     ForkChoiceError, StateSkipConfig,
 };
 pub use self::beacon_snapshot::BeaconSnapshot;
+pub use self::chain_config::ChainConfig;
 pub use self::errors::{BeaconChainError, BlockProductionError};
 pub use attestation_verification::Error as AttestationError;
 pub use beacon_fork_choice_store::{BeaconForkChoiceStore, Error as ForkChoiceStoreError};

beacon_node/beacon_chain/src/test_utils.rs

@@ -152,6 +152,7 @@ impl<E: EthSpec> BeaconChainHarness<DiskHarnessType<E>> {
         let chain = BeaconChainBuilder::new(eth_spec_instance)
             .logger(log.clone())
             .custom_spec(spec.clone())
+            .import_max_skip_slots(None)
             .store(store.clone())
             .store_migrator(BlockingMigrator::new(store, log.clone()))
             .data_dir(data_dir.path().to_path_buf())
@@ -190,6 +191,7 @@ impl<E: EthSpec> BeaconChainHarness<DiskHarnessType<E>> {
         let chain = BeaconChainBuilder::new(eth_spec_instance)
             .logger(log.clone())
             .custom_spec(spec)
+            .import_max_skip_slots(None)
             .store(store.clone())
             .store_migrator(<BlockingMigrator<_, _, _> as Migrate<E, _, _>>::new(
                 store,

beacon_node/client/src/builder.rs

@@ -135,6 +135,7 @@ where
         let eth_spec_instance = self.eth_spec_instance.clone();
         let data_dir = config.data_dir.clone();
         let disabled_forks = config.disabled_forks.clone();
+        let chain_config = config.chain.clone();
         let graffiti = config.graffiti;
 
         let store =
@@ -153,6 +154,7 @@ where
             .store_migrator(store_migrator)
             .data_dir(data_dir)
             .custom_spec(spec.clone())
+            .chain_config(chain_config)
             .disabled_forks(disabled_forks)
             .graffiti(graffiti);
 

beacon_node/client/src/config.rs

@@ -64,6 +64,7 @@ pub struct Config {
     pub store: store::StoreConfig,
     pub network: network::NetworkConfig,
     pub rest_api: rest_api::Config,
+    pub chain: beacon_chain::ChainConfig,
     pub websocket_server: websocket_server::Config,
     pub eth1: eth1::Config,
 }
@@ -78,6 +79,7 @@ impl Default for Config {
             genesis: <_>::default(),
             store: <_>::default(),
             network: NetworkConfig::default(),
+            chain: <_>::default(),
             rest_api: <_>::default(),
             websocket_server: <_>::default(),
             spec_constants: TESTNET_SPEC_CONSTANTS.into(),

beacon_node/network/src/beacon_processor/mod.rs

@@ -1150,6 +1150,22 @@ pub fn handle_attestation_verification_failure<E: EthSpec>(
              * The peer has published an invalid consensus message.
              */
         }
+        AttnError::TooManySkippedSlots {
+            head_block_slot,
+            attestation_slot,
+        } => {
+            /*
+             * The attestation references a head block that is too far behind the attestation slot.
+             *
+             * The message is not necessarily invalid, but we choose to ignore it.
+             */
+            debug!(
+                log,
+                "Rejected long skip slot attestation";
+                "head_block_slot" => head_block_slot,
+                "attestation_slot" => attestation_slot,
+            )
+        }
         AttnError::BeaconChainError(e) => {
             /*
              * Lighthouse hit an unexpected error whilst processing the attestation. It

beacon_node/network/src/metrics.rs

@@ -234,6 +234,10 @@ lazy_static! {
         "gossip_attestation_error_invalid_state_processing",
         "Count of a specific error type (see metric name)"
     );
+    pub static ref GOSSIP_ATTESTATION_ERROR_INVALID_TOO_MANY_SKIPPED_SLOTS: Result<IntCounter> = try_create_int_counter(
+        "gossip_attestation_error_invalid_too_many_skipped_slots",
+        "Count of a specific error type (see metric name)"
+    );
     pub static ref GOSSIP_ATTESTATION_ERROR_BEACON_CHAIN_ERROR: Result<IntCounter> = try_create_int_counter(
         "gossip_attestation_error_beacon_chain_error",
         "Count of a specific error type (see metric name)"
@@ -291,6 +295,9 @@ pub fn register_attestation_error(error: &AttnError) {
             inc_counter(&GOSSIP_ATTESTATION_ERROR_INVALID_SUBNET_ID)
         }
         AttnError::Invalid(_) => inc_counter(&GOSSIP_ATTESTATION_ERROR_INVALID_STATE_PROCESSING),
+        AttnError::TooManySkippedSlots { .. } => {
+            inc_counter(&GOSSIP_ATTESTATION_ERROR_INVALID_TOO_MANY_SKIPPED_SLOTS)
+        }
         AttnError::BeaconChainError(_) => inc_counter(&GOSSIP_ATTESTATION_ERROR_BEACON_CHAIN_ERROR),
     }
 }

beacon_node/src/cli.rs

@@ -249,4 +249,17 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
                 .value_name("GRAFFITI")
                 .takes_value(true)
         )
+        .arg(
+            Arg::with_name("max-skip-slots")
+                .long("max-skip-slots")
+                .help(
+                    "Refuse to skip more than this many slots when processing a block or attestation. \
+                    This prevents nodes on minority forks from wasting our time and RAM, \
+                    but might need to be raised or set to 'none' in times of extreme network \
+                    outage."
+                )
+                .value_name("NUM_SLOTS")
+                .takes_value(true)
+                .default_value("320")
+        )
 }

beacon_node/src/config.rs

@@ -386,6 +386,16 @@ pub fn get_config<E: EthSpec>(
     client_config.graffiti[..trimmed_graffiti_len]
         .copy_from_slice(&raw_graffiti[..trimmed_graffiti_len]);
 
+    if let Some(max_skip_slots) = cli_args.value_of("max-skip-slots") {
+        client_config.chain.import_max_skip_slots = match max_skip_slots {
+            "none" => None,
+            n => Some(
+                n.parse()
+                    .map_err(|_| "Invalid max-skip-slots".to_string())?,
+            ),
+        };
+    }
+
     Ok(client_config)
 }