From 37dc3d463dca7820bd2c574ca534d2d67db80af9 Mon Sep 17 00:00:00 2001 From: Viktor Kirilov Date: Wed, 29 Apr 2020 02:25:54 +0300 Subject: [PATCH 1/7] =?UTF-8?q?[lcli]=20the=20fork=20version=20is=20now=20?= =?UTF-8?q?inferred=20from=20the=20spec=20and=20can=20also=20be=E2=80=A6?= =?UTF-8?q?=20(#1068)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [lcli] the fork version is now inferred from the spec and can also be overridden for the interop-genesis subcommand with a command line flag just like for the new-testnet subcommand * fixed formatting --- lcli/src/interop_genesis.rs | 5 ++++- lcli/src/main.rs | 8 ++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) 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 be1a93b704..f4cd748f26 100644 --- a/lcli/src/main.rs +++ b/lcli/src/main.rs @@ -225,6 +225,14 @@ 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") From 7f2121205aed5d0a5dadd11e09e2970f57e56254 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 29 Apr 2020 11:37:14 +1000 Subject: [PATCH 2/7] Ensure genesis is not triggered too early (#1052) --- beacon_node/genesis/src/eth1_genesis_service.rs | 14 ++++++++++---- eth2/state_processing/src/genesis.rs | 17 +++++++++++++---- eth2/state_processing/src/lib.rs | 5 ++++- 3 files changed, 27 insertions(+), 9 deletions(-) diff --git a/beacon_node/genesis/src/eth1_genesis_service.rs b/beacon_node/genesis/src/eth1_genesis_service.rs index 14df35fa17..ffefb58b23 100644 --- a/beacon_node/genesis/src/eth1_genesis_service.rs +++ b/beacon_node/genesis/src/eth1_genesis_service.rs @@ -10,7 +10,7 @@ use futures::{ 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; @@ -223,9 +223,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/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, From 18ca94dc297575b9e3d72dfcf20661ed9588f679 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Thu, 30 Apr 2020 15:21:43 +1000 Subject: [PATCH 3/7] Fix duplicate proposer slashing bug (#1086) Remove parallelism from proposer slashing verification. Closes #1065 --- eth2/operation_pool/src/lib.rs | 42 ++++++++++++++++++- .../src/per_block_processing.rs | 27 ++++++------ .../src/per_block_processing/tests.rs | 31 ++++++++++++++ 3 files changed, 85 insertions(+), 15 deletions(-) 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/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(); From 78a08ec1e6b899e4faa06ba3b773aa47f40e805b Mon Sep 17 00:00:00 2001 From: Age Manning Date: Thu, 30 Apr 2020 15:33:53 +1000 Subject: [PATCH 4/7] Remove padding from gossipsub ids (#1083) --- beacon_node/eth2-libp2p/src/config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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, )) }; From dea01be00eb3be983ee92a070fd562493a63c5b4 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Thu, 30 Apr 2020 01:39:10 -0400 Subject: [PATCH 5/7] Improve aggregate validator logic (#1020) * track whether we have aggregate validator subscriptions to exact subnets, so we know whether or not to drop incoming attestations * fix is aggregator check * fix CI Co-authored-by: Age Manning --- .../network/src/attestation_service/mod.rs | 33 +++++++++++++------ 1 file changed, 23 insertions(+), 10 deletions(-) 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() { From 8bf0ef8d30504b076fad42aec7315cdf3aa877df Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 30 Apr 2020 16:19:15 +1000 Subject: [PATCH 6/7] Add more detail to bad hardcoded dir warning (#1069) --- Cargo.lock | 1 + beacon_node/Cargo.toml | 1 + beacon_node/src/config.rs | 11 +++-------- eth2/utils/clap_utils/src/lib.rs | 15 +++++++-------- 4 files changed, 12 insertions(+), 16 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cbf59a0c20..b6e814b420 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/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/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)) } } From f4ac0422e2edb9ebb0f17ac214459307a486985e Mon Sep 17 00:00:00 2001 From: Raw Pong Ghmoa <58883403+q9f@users.noreply.github.com> Date: Thu, 30 Apr 2020 08:20:54 +0200 Subject: [PATCH 7/7] beacon/notifier: display block information for current slot (#1084) --- beacon_node/client/src/notifier.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/beacon_node/client/src/notifier.rs b/beacon_node/client/src/notifier.rs index 9ce68bedef..1bb649b99e 100644 --- a/beacon_node/client/src/notifier.rs +++ b/beacon_node/client/src/notifier.rs @@ -122,6 +122,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"; @@ -129,6 +130,7 @@ pub fn spawn_notifier( "finalized_root" => format!("{}", finalized_root), "finalized_epoch" => finalized_epoch, "epoch" => current_epoch, + "block" => block_info, "slot" => current_slot, ); } else {