diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index 9c8ec92e5..6502e106b 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/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 VerifiedAggregatedAttestation { }?; // 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 VerifiedAggregatedAttestation { // // 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 VerifiedUnaggregatedAttestation { // 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 VerifiedUnaggregatedAttestation { fn verify_head_block_is_known( chain: &BeaconChain, attestation: &Attestation, + max_skip_slots: Option, ) -> 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 { diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 6fa06d3a8..ef94274fa 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/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 { 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>, /// Database migrator for running background maintenance on the store. diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 92460dba1..520ebcea5 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -92,6 +92,8 @@ pub enum BlockError { /// It's unclear if this block is valid, but it cannot be processed without already knowing /// its parent. ParentUnknown(Box>), + /// 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. */ diff --git a/beacon_node/beacon_chain/src/builder.rs b/beacon_node/beacon_chain/src/builder.rs index b0c467920..6ff94cfaa 100644 --- a/beacon_node/beacon_chain/src/builder.rs +++ b/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 { pubkey_cache_path: Option, validator_pubkey_cache: Option, spec: ChainSpec, + chain_config: ChainConfig, disabled_forks: Vec, log: Option, 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) -> 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 diff --git a/beacon_node/beacon_chain/src/chain_config.rs b/beacon_node/beacon_chain/src/chain_config.rs new file mode 100644 index 000000000..ea8c5dd52 --- /dev/null +++ b/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, +} + +impl Default for ChainConfig { + fn default() -> Self { + Self { + import_max_skip_slots: Some(DEFAULT_IMPORT_BLOCK_MAX_SKIP_SLOTS), + } + } +} diff --git a/beacon_node/beacon_chain/src/lib.rs b/beacon_node/beacon_chain/src/lib.rs index 3c597bc72..941627c1f 100644 --- a/beacon_node/beacon_chain/src/lib.rs +++ b/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}; diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 5dccde9ad..a8e86db8b 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -152,6 +152,7 @@ impl BeaconChainHarness> { 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 BeaconChainHarness> { let chain = BeaconChainBuilder::new(eth_spec_instance) .logger(log.clone()) .custom_spec(spec) + .import_max_skip_slots(None) .store(store.clone()) .store_migrator( as Migrate>::new( store, diff --git a/beacon_node/client/src/builder.rs b/beacon_node/client/src/builder.rs index 94d67b7c3..d9edbe1d2 100644 --- a/beacon_node/client/src/builder.rs +++ b/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); diff --git a/beacon_node/client/src/config.rs b/beacon_node/client/src/config.rs index aaf0df46c..19088e785 100644 --- a/beacon_node/client/src/config.rs +++ b/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(), diff --git a/beacon_node/network/src/beacon_processor/mod.rs b/beacon_node/network/src/beacon_processor/mod.rs index 33022c0ad..6cce8a28a 100644 --- a/beacon_node/network/src/beacon_processor/mod.rs +++ b/beacon_node/network/src/beacon_processor/mod.rs @@ -1150,6 +1150,22 @@ pub fn handle_attestation_verification_failure( * 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 diff --git a/beacon_node/network/src/metrics.rs b/beacon_node/network/src/metrics.rs index fa17d5f91..18064d798 100644 --- a/beacon_node/network/src/metrics.rs +++ b/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 = 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 = 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), } } diff --git a/beacon_node/src/cli.rs b/beacon_node/src/cli.rs index 152f84c44..5bb79c7df 100644 --- a/beacon_node/src/cli.rs +++ b/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") + ) } diff --git a/beacon_node/src/config.rs b/beacon_node/src/config.rs index bef56e36e..b2da523ec 100644 --- a/beacon_node/src/config.rs +++ b/beacon_node/src/config.rs @@ -386,6 +386,16 @@ pub fn get_config( 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) }