diff --git a/Cargo.lock b/Cargo.lock index eda0a53dd2..af806827ad 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -250,6 +250,7 @@ version = "0.2.0" dependencies = [ "beacon_chain 0.2.0", "clap 2.33.0 (registry+https://github.com/rust-lang/crates.io-index)", + "clap_utils 0.1.0", "client 0.2.0", "ctrlc 3.1.4 (registry+https://github.com/rust-lang/crates.io-index)", "dirs 2.0.2 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/beacon_node/Cargo.toml b/beacon_node/Cargo.toml index 9244d155ae..28fef848a8 100644 --- a/beacon_node/Cargo.toml +++ b/beacon_node/Cargo.toml @@ -41,3 +41,4 @@ eth2-libp2p = { path = "./eth2-libp2p" } eth2_ssz = { path = "../eth2/utils/ssz" } toml = "0.5.4" serde = "1.0.102" +clap_utils = { path = "../eth2/utils/clap_utils" } diff --git a/beacon_node/client/src/notifier.rs b/beacon_node/client/src/notifier.rs index 38da10261e..26cae13121 100644 --- a/beacon_node/client/src/notifier.rs +++ b/beacon_node/client/src/notifier.rs @@ -147,6 +147,7 @@ pub fn spawn_notifier( ); } else { if sync_state.is_synced() { + let block_info = if current_slot > head_slot { format!(" … empty") } else { format!("{}", head_root) }; info!( log_2, "Synced"; @@ -154,6 +155,7 @@ pub fn spawn_notifier( "finalized_root" => format!("{}", finalized_root), "finalized_epoch" => finalized_epoch, "epoch" => current_epoch, + "block" => block_info, "slot" => current_slot, ); } else { diff --git a/beacon_node/eth2-libp2p/src/config.rs b/beacon_node/eth2-libp2p/src/config.rs index 41395cef9e..7a5182baf1 100644 --- a/beacon_node/eth2-libp2p/src/config.rs +++ b/beacon_node/eth2-libp2p/src/config.rs @@ -92,7 +92,7 @@ impl Default for Config { let gossip_message_id = |message: &GossipsubMessage| { MessageId(base64::encode_config( &Sha256::digest(&message.data), - base64::URL_SAFE, + base64::URL_SAFE_NO_PAD, )) }; diff --git a/beacon_node/genesis/src/eth1_genesis_service.rs b/beacon_node/genesis/src/eth1_genesis_service.rs index 53fa26260a..40aa3aec58 100644 --- a/beacon_node/genesis/src/eth1_genesis_service.rs +++ b/beacon_node/genesis/src/eth1_genesis_service.rs @@ -5,7 +5,7 @@ use eth1::{DepositLog, Eth1Block, Service}; use parking_lot::Mutex; use slog::{debug, error, info, trace, Logger}; use state_processing::{ - initialize_beacon_state_from_eth1, is_valid_genesis_state, + eth2_genesis_time, initialize_beacon_state_from_eth1, is_valid_genesis_state, per_block_processing::process_deposit, process_activations, }; use std::sync::Arc; @@ -186,9 +186,15 @@ impl Eth1GenesisService { .blocks() .read() .iter() - // It's only worth scanning blocks that have timestamps _after_ genesis time. It's - // impossible for any other block to trigger genesis. - .filter(|block| block.timestamp >= spec.min_genesis_time) + // Filter out any blocks that would result in a genesis time that is earlier than + // `MIN_GENESIS_TIME`. + // + // Note: any `SafeArith` errors are suppressed here; we simply skip blocks that cause + // overflow/div-by-zero. + .filter(|block| { + eth2_genesis_time(block.timestamp, spec) + .map_or(false, |t| t >= spec.min_genesis_time) + }) // The block cache might be more recently updated than deposit cache. Restrict any // block numbers that are not known by all caches. .filter(|block| { diff --git a/beacon_node/network/src/attestation_service/mod.rs b/beacon_node/network/src/attestation_service/mod.rs index 5c21d45a81..c44ec204ac 100644 --- a/beacon_node/network/src/attestation_service/mod.rs +++ b/beacon_node/network/src/attestation_service/mod.rs @@ -10,7 +10,7 @@ use rand::seq::SliceRandom; use rest_types::ValidatorSubscription; use slog::{crit, debug, error, o, warn}; use slot_clock::SlotClock; -use std::collections::{HashMap, VecDeque}; +use std::collections::VecDeque; use std::sync::Arc; use std::time::{Duration, Instant}; use types::{Attestation, EthSpec, Slot, SubnetId}; @@ -77,8 +77,8 @@ pub struct AttestationService { /// A collection of timeouts for when to unsubscribe from a shard subnet. unsubscriptions: HashSetDelay, - /// A mapping indicating the number of known aggregate validators for a given `ExactSubnet`. - _aggregate_validators_on_subnet: HashMap, + /// A collection timeouts to track the existence of aggregate validator subscriptions at an `ExactSubnet`. + aggregate_validators_on_subnet: HashSetDelay, /// A collection of seen validators. These dictate how many random subnets we should be /// subscribed to. As these time out, we unsubscribe for the required random subnets and update @@ -124,7 +124,7 @@ impl AttestationService { discover_peers: HashSetDelay::new(default_timeout), subscriptions: HashSetDelay::new(default_timeout), unsubscriptions: HashSetDelay::new(default_timeout), - _aggregate_validators_on_subnet: HashMap::new(), + aggregate_validators_on_subnet: HashSetDelay::new(default_timeout), known_validators: HashSetDelay::new(last_seen_val_timeout), log, } @@ -176,10 +176,12 @@ impl AttestationService { // sophisticated logic should be added using known future forks. // TODO: Implement - // set the subscription timer to subscribe to the next subnet if required - if let Err(e) = self.subscribe_to_subnet(exact_subnet) { - warn!(self.log, "Subscription to subnet error"; "error" => e); - return Err(()); + if subscription.is_aggregator { + // set the subscription timer to subscribe to the next subnet if required + if let Err(e) = self.subscribe_to_subnet(exact_subnet) { + warn!(self.log, "Subscription to subnet error"; "error" => e); + return Err(()); + } } } Ok(()) @@ -208,8 +210,11 @@ impl AttestationService { return false; } - // TODO: Correctly handle validation aggregator checks - true + let exact_subnet = ExactSubnet { + subnet_id: subnet.clone(), + slot: attestation.data.slot, + }; + self.aggregate_validators_on_subnet.contains(&exact_subnet) } /* Internal private functions */ @@ -335,6 +340,12 @@ impl AttestationService { } }; + // Regardless of whether or not we have already subscribed to a subnet, track the expiration + // of aggregate validator subscriptions to exact subnets so we know whether or not to drop + // attestations for a given subnet + slot + self.aggregate_validators_on_subnet + .insert_at(exact_subnet.clone(), expected_end_subscription_duration); + // Checks on current subscriptions // Note: We may be connected to a long-lived random subnet. In this case we still add the // subscription timeout and check this case when the timeout fires. This is because a @@ -641,6 +652,8 @@ impl Stream for AttestationService { { let _ = self.handle_known_validator_expiry(); } + // poll to remove entries on expiration, no need to act on expiration events + let _ = self.aggregate_validators_on_subnet.poll().map_err(|e| { error!(self.log, "Failed to check for aggregate validator on subnet expirations"; "error"=> format!("{}", e)); }); // process any generated events if let Some(event) = self.events.pop_front() { diff --git a/beacon_node/src/config.rs b/beacon_node/src/config.rs index 59b1457deb..cd425c1a09 100644 --- a/beacon_node/src/config.rs +++ b/beacon_node/src/config.rs @@ -1,5 +1,6 @@ use beacon_chain::builder::PUBKEY_CACHE_FILENAME; use clap::ArgMatches; +use clap_utils::BAD_TESTNET_DIR_MESSAGE; use client::{config::DEFAULT_DATADIR, ClientConfig, ClientGenesis}; use eth2_libp2p::{Enr, Multiaddr}; use eth2_testnet_config::Eth2TestnetConfig; @@ -385,14 +386,8 @@ pub fn get_eth2_testnet_config( Eth2TestnetConfig::load(testnet_dir.clone()) .map_err(|e| format!("Unable to open testnet dir at {:?}: {}", testnet_dir, e))? } else { - Eth2TestnetConfig::hard_coded().map_err(|e| { - format!( - "The hard-coded testnet directory was invalid. \ - This happens when Lighthouse is migrating between spec versions. \ - Error : {}", - e - ) - })? + Eth2TestnetConfig::hard_coded() + .map_err(|e| format!("{} Error : {}", BAD_TESTNET_DIR_MESSAGE, e))? }) } diff --git a/eth2/operation_pool/src/lib.rs b/eth2/operation_pool/src/lib.rs index ac2c1ed319..a998225197 100644 --- a/eth2/operation_pool/src/lib.rs +++ b/eth2/operation_pool/src/lib.rs @@ -485,7 +485,7 @@ mod release_tests { state_builder.teleport_to_slot(slot); state_builder.build_caches(&spec).unwrap(); let (state, keypairs) = state_builder.build(); - (state, keypairs, MainnetEthSpec::default_spec()) + (state, keypairs, spec) } #[test] @@ -890,4 +890,44 @@ mod release_tests { seen_indices.extend(fresh_indices); } } + + /// Insert two slashings for the same proposer and ensure only one is returned. + #[test] + fn duplicate_proposer_slashing() { + let spec = MainnetEthSpec::default_spec(); + let num_validators = 32; + let mut state_builder = + TestingBeaconStateBuilder::::from_default_keypairs_file_if_exists( + num_validators, + &spec, + ); + state_builder.build_caches(&spec).unwrap(); + let (state, keypairs) = state_builder.build(); + let op_pool = OperationPool::new(); + + let proposer_index = 0; + let slashing1 = TestingProposerSlashingBuilder::double_vote::( + ProposerSlashingTestTask::Valid, + proposer_index, + &keypairs[proposer_index as usize].sk, + &state.fork, + state.genesis_validators_root, + &spec, + ); + let slashing2 = ProposerSlashing { + signed_header_1: slashing1.signed_header_2.clone(), + signed_header_2: slashing1.signed_header_1.clone(), + }; + + // Both slashings should be accepted by the pool. + op_pool + .insert_proposer_slashing(slashing1.clone(), &state, &spec) + .unwrap(); + op_pool + .insert_proposer_slashing(slashing2.clone(), &state, &spec) + .unwrap(); + + // Should only get the second slashing back. + assert_eq!(op_pool.get_slashings(&state, &spec).0, vec![slashing2]); + } } diff --git a/eth2/state_processing/src/genesis.rs b/eth2/state_processing/src/genesis.rs index 9ae9fabdc3..d219b9090c 100644 --- a/eth2/state_processing/src/genesis.rs +++ b/eth2/state_processing/src/genesis.rs @@ -1,6 +1,6 @@ use super::per_block_processing::{errors::BlockProcessingError, process_deposit}; use crate::common::DepositDataTree; -use safe_arith::SafeArith; +use safe_arith::{ArithError, SafeArith}; use tree_hash::TreeHash; use types::DEPOSIT_TREE_DEPTH; use types::*; @@ -15,9 +15,7 @@ pub fn initialize_beacon_state_from_eth1( deposits: Vec, spec: &ChainSpec, ) -> Result, BlockProcessingError> { - let genesis_time = eth1_timestamp - .safe_sub(eth1_timestamp.safe_rem(spec.min_genesis_delay)?)? - .safe_add(2.safe_mul(spec.min_genesis_delay)?)?; + let genesis_time = eth2_genesis_time(eth1_timestamp, spec)?; let eth1_data = Eth1Data { // Temporary deposit root deposit_root: Hash256::zero(), @@ -79,3 +77,14 @@ pub fn process_activations( } Ok(()) } + +/// Returns the `state.genesis_time` for the corresponding `eth1_timestamp`. +/// +/// Does _not_ ensure that the time is greater than `MIN_GENESIS_TIME`. +/// +/// Spec v0.11.1 +pub fn eth2_genesis_time(eth1_timestamp: u64, spec: &ChainSpec) -> Result { + eth1_timestamp + .safe_sub(eth1_timestamp.safe_rem(spec.min_genesis_delay)?)? + .safe_add(2.safe_mul(spec.min_genesis_delay)?) +} diff --git a/eth2/state_processing/src/lib.rs b/eth2/state_processing/src/lib.rs index 86dc2294f0..7b871c1db5 100644 --- a/eth2/state_processing/src/lib.rs +++ b/eth2/state_processing/src/lib.rs @@ -10,7 +10,10 @@ pub mod per_epoch_processing; pub mod per_slot_processing; pub mod test_utils; -pub use genesis::{initialize_beacon_state_from_eth1, is_valid_genesis_state, process_activations}; +pub use genesis::{ + eth2_genesis_time, initialize_beacon_state_from_eth1, is_valid_genesis_state, + process_activations, +}; pub use per_block_processing::{ block_signature_verifier, errors::BlockProcessingError, per_block_processing, signature_sets, BlockSignatureStrategy, BlockSignatureVerifier, VerifySignatures, diff --git a/eth2/state_processing/src/per_block_processing.rs b/eth2/state_processing/src/per_block_processing.rs index 8634bb0e12..a4f2be87ef 100644 --- a/eth2/state_processing/src/per_block_processing.rs +++ b/eth2/state_processing/src/per_block_processing.rs @@ -283,26 +283,25 @@ pub fn process_proposer_slashings( verify_signatures: VerifySignatures, spec: &ChainSpec, ) -> Result<(), BlockProcessingError> { - // Verify proposer slashings in parallel. + // Verify and apply proposer slashings in series. + // We have to verify in series because an invalid block may contain multiple slashings + // for the same validator, and we need to correctly detect and reject that. proposer_slashings - .par_iter() + .into_iter() .enumerate() .try_for_each(|(i, proposer_slashing)| { verify_proposer_slashing(proposer_slashing, &state, verify_signatures, spec) - .map_err(|e| e.into_with_index(i)) - })?; + .map_err(|e| e.into_with_index(i))?; - // Update the state. - for proposer_slashing in proposer_slashings { - slash_validator( - state, - proposer_slashing.signed_header_1.message.proposer_index as usize, - None, - spec, - )?; - } + slash_validator( + state, + proposer_slashing.signed_header_1.message.proposer_index as usize, + None, + spec, + )?; - Ok(()) + Ok(()) + }) } /// Validates each `AttesterSlashing` and updates the state, short-circuiting on an invalid object. diff --git a/eth2/state_processing/src/per_block_processing/tests.rs b/eth2/state_processing/src/per_block_processing/tests.rs index f942cc29f5..e7254db9d8 100644 --- a/eth2/state_processing/src/per_block_processing/tests.rs +++ b/eth2/state_processing/src/per_block_processing/tests.rs @@ -1029,6 +1029,37 @@ fn invalid_proposer_slashing_not_slashable() { ); } +#[test] +fn invalid_proposer_slashing_duplicate_slashing() { + let spec = MainnetEthSpec::default_spec(); + let builder = get_builder(&spec, SLOT_OFFSET, VALIDATOR_COUNT); + let test_task = ProposerSlashingTestTask::Valid; + let (mut block, mut state) = + builder.build_with_proposer_slashing(test_task, 1, None, None, &spec); + + let slashing = block.message.body.proposer_slashings[0].clone(); + let slashed_proposer = slashing.signed_header_1.message.proposer_index; + block.message.body.proposer_slashings.push(slashing); + + let result = per_block_processing( + &mut state, + &block, + None, + BlockSignatureStrategy::NoVerification, + &spec, + ); + + // Expecting ProposerNotSlashable for the 2nd slashing because the validator has been + // slashed by the 1st slashing. + assert_eq!( + result, + Err(BlockProcessingError::ProposerSlashingInvalid { + index: 1, + reason: ProposerSlashingInvalid::ProposerNotSlashable(slashed_proposer) + }) + ); +} + #[test] fn invalid_bad_proposal_1_signature() { let spec = MainnetEthSpec::default_spec(); diff --git a/eth2/utils/clap_utils/src/lib.rs b/eth2/utils/clap_utils/src/lib.rs index d8002d76fa..7bc1b07c07 100644 --- a/eth2/utils/clap_utils/src/lib.rs +++ b/eth2/utils/clap_utils/src/lib.rs @@ -8,6 +8,11 @@ use std::path::PathBuf; use std::str::FromStr; use types::EthSpec; +pub const BAD_TESTNET_DIR_MESSAGE: &str = "The hard-coded testnet directory was invalid. \ + This happens when Lighthouse is migrating between spec versions \ + or when there is no default public network to connect to. \ + During these times you must specify a --testnet-dir."; + /// Attempts to load the testnet dir at the path if `name` is in `matches`, returning an error if /// the path cannot be found or the testnet dir is invalid. /// @@ -20,14 +25,8 @@ pub fn parse_testnet_dir_with_hardcoded_default( Eth2TestnetConfig::load(path.clone()) .map_err(|e| format!("Unable to open testnet dir at {:?}: {}", path, e)) } else { - Eth2TestnetConfig::hard_coded().map_err(|e| { - format!( - "The hard-coded testnet directory was invalid. \ - This happens when Lighthouse is migrating between spec versions. \ - Error : {}", - e - ) - }) + Eth2TestnetConfig::hard_coded() + .map_err(|e| format!("{} Error : {}", BAD_TESTNET_DIR_MESSAGE, e)) } } diff --git a/lcli/src/interop_genesis.rs b/lcli/src/interop_genesis.rs index 61055f9b85..9c8609b5c3 100644 --- a/lcli/src/interop_genesis.rs +++ b/lcli/src/interop_genesis.rs @@ -1,4 +1,5 @@ use clap::ArgMatches; +use clap_utils::parse_ssz_optional; use environment::Environment; use eth2_testnet_config::Eth2TestnetConfig; use genesis::interop_genesis_state; @@ -49,7 +50,9 @@ pub fn run(mut env: Environment, matches: &ArgMatches) -> Result< ) })?; - spec.genesis_fork_version = [1, 3, 3, 7]; + if let Some(v) = parse_ssz_optional(matches, "genesis-fork-version")? { + spec.genesis_fork_version = v; + } let keypairs = generate_deterministic_keypairs(validator_count); let genesis_state = interop_genesis_state(&keypairs, genesis_time, &spec)?; diff --git a/lcli/src/main.rs b/lcli/src/main.rs index 0f51f6abbf..4aa9ce22dd 100644 --- a/lcli/src/main.rs +++ b/lcli/src/main.rs @@ -226,6 +226,14 @@ async fn main() { .takes_value(true) .help("The value for state.genesis_time. Defaults to now."), ) + .arg( + Arg::with_name("genesis-fork-version") + .long("genesis-fork-version") + .value_name("HEX") + .takes_value(true) + .help("Used to avoid reply attacks between testnets. Recommended to set to + non-default."), + ) ) .subcommand( SubCommand::with_name("change-genesis-time")