From f2dd032884107f30c4b6e6573febbf84b2215fd2 Mon Sep 17 00:00:00 2001 From: David Date: Mon, 24 Jun 2019 14:50:11 +0200 Subject: [PATCH] Print warnings when using dangerous settings for ValidatorSet (#10733) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Stop breaking out of loop if a non-canonical hash is found * include expected hash in log msg * More logging * Scope * Syntax * Log in blank RollingFinality Escalate bad proposer to warning * Check validator set size: warn if 1 or even number * More readable code * Use SimpleList::new * Extensive logging on unexpected non-canonical hash * Wording * Update ethcore/blockchain/src/blockchain.rs Co-Authored-By: Tomasz Drwięga * Improved logging, address grumbles * Update ethcore/src/engines/validator_set/simple_list.rs Co-Authored-By: Luke Schoen --- ethcore/blockchain/src/blockchain.rs | 10 +++++----- ethcore/src/engines/authority_round/finality.rs | 2 -- ethcore/src/engines/validator_set/safe_contract.rs | 14 +++++++++----- ethcore/src/engines/validator_set/simple_list.rs | 13 ++++++++----- 4 files changed, 22 insertions(+), 17 deletions(-) diff --git a/ethcore/blockchain/src/blockchain.rs b/ethcore/blockchain/src/blockchain.rs index 6656fcc0c..dbe18f253 100644 --- a/ethcore/blockchain/src/blockchain.rs +++ b/ethcore/blockchain/src/blockchain.rs @@ -42,7 +42,7 @@ use ethereum_types::{H256, Bloom, BloomRef, U256}; use util_mem::{MallocSizeOf, allocators::new_malloc_size_ops}; use itertools::Itertools; use kvdb::{DBTransaction, KeyValueDB}; -use log::{trace, warn, info}; +use log::{trace, debug, warn, info}; use parity_bytes::Bytes; use parking_lot::{Mutex, RwLock}; use rayon::prelude::*; @@ -963,7 +963,7 @@ impl BlockChain { /// Iterate over all epoch transitions. /// This will only return transitions within the canonical chain. pub fn epoch_transitions(&self) -> EpochTransitionIter { - trace!(target: "blockchain", "Iterating over all epoch transitions"); + debug!(target: "blockchain", "Iterating over all epoch transitions"); let iter = self.db.key_value().iter_from_prefix(db::COL_EXTRA, &EPOCH_KEY_PREFIX[..]); EpochTransitionIter { chain: self, @@ -991,7 +991,7 @@ impl BlockChain { for hash in self.ancestry_iter(parent_hash)? { trace!(target: "blockchain", "Got parent hash {} from ancestry_iter", hash); let details = self.block_details(&hash)?; - trace!(target: "blockchain", "Block #{}: Got block details", details.number); + trace!(target: "blockchain", "Block #{}: Got block details for parent hash {}", details.number, hash); // look for transition in database. if let Some(transition) = self.epoch_transition(details.number, hash) { @@ -1013,8 +1013,8 @@ impl BlockChain { Some(h) => { warn!(target: "blockchain", "Block #{}: Found non-canonical block hash {} (expected {})", details.number, h, hash); - trace!(target: "blockchain", "Block #{} Mismatched hashes. Ancestor {} != Own {} – Own block #{}", details.number, hash, h, self.block_number(&h).unwrap_or_default() ); - trace!(target: "blockchain", " Ancestor {}: #{:#?}", hash, self.block_details(&hash)); + trace!(target: "blockchain", "Block #{} Mismatched hashes. Ancestor {} != Own {}", details.number, hash, h); + trace!(target: "blockchain", " Ancestor {}: #{:#?}", hash, details); trace!(target: "blockchain", " Own {}: #{:#?}", h, self.block_details(&h)); }, diff --git a/ethcore/src/engines/authority_round/finality.rs b/ethcore/src/engines/authority_round/finality.rs index e88458d81..e1f050393 100644 --- a/ethcore/src/engines/authority_round/finality.rs +++ b/ethcore/src/engines/authority_round/finality.rs @@ -111,8 +111,6 @@ impl RollingFinality { /// Returns a list of all newly finalized headers. // TODO: optimize with smallvec. pub fn push_hash(&mut self, head: H256, signers: Vec
) -> Result, UnknownValidator> { - // TODO: seems bad to iterate over signers twice like this. - // Can do the work in a single loop and call `clear()` if an unknown validator was found? for their_signer in signers.iter() { if !self.signers.contains(their_signer) { warn!(target: "finality", "Unknown validator: {}", their_signer); diff --git a/ethcore/src/engines/validator_set/safe_contract.rs b/ethcore/src/engines/validator_set/safe_contract.rs index e8db31423..18477d64d 100644 --- a/ethcore/src/engines/validator_set/safe_contract.rs +++ b/ethcore/src/engines/validator_set/safe_contract.rs @@ -208,11 +208,11 @@ impl ValidatorSafeContract { match value { Ok(new) => { - debug!(target: "engine", "Set of validators obtained: {:?}", new); + debug!(target: "engine", "Got validator set from contract: {:?}", new); Some(SimpleList::new(new)) }, Err(s) => { - debug!(target: "engine", "Set of validators could not be updated: {}", s); + debug!(target: "engine", "Could not get validator set from contract: {}", s); None }, } @@ -335,7 +335,7 @@ impl ValidatorSet for ValidatorSafeContract { Some(receipts) => match self.extract_from_event(bloom, header, receipts) { None => ::engines::EpochChange::No, Some(list) => { - info!(target: "engine", "Signal for transition within contract. New list: {:?}", + info!(target: "engine", "Signal for transition within contract. New validator list: {:?}", &*list); let proof = encode_proof(&header, receipts); @@ -359,7 +359,7 @@ impl ValidatorSet for ValidatorSafeContract { let addresses = check_first_proof(machine, self.contract_address, old_header, &state_items) .map_err(::engines::EngineError::InsufficientProof)?; - trace!(target: "engine", "extracted epoch set at #{}: {} addresses", + trace!(target: "engine", "Extracted epoch validator set at block #{}: {} addresses", number, addresses.len()); Ok((SimpleList::new(addresses), Some(old_hash))) @@ -380,7 +380,11 @@ impl ValidatorSet for ValidatorSafeContract { let bloom = self.expected_bloom(&old_header); match self.extract_from_event(bloom, &old_header, &receipts) { - Some(list) => Ok((list, Some(old_header.hash()))), + Some(list) => { + trace!(target: "engine", "Extracted epoch validator set at block #{}: {} addresses", old_header.number(), list.len()); + + Ok((list, Some(old_header.hash()))) + }, None => Err(::engines::EngineError::InsufficientProof("No log event in proof.".into()).into()), } } diff --git a/ethcore/src/engines/validator_set/simple_list.rs b/ethcore/src/engines/validator_set/simple_list.rs index d9a4c6e1c..958bda202 100644 --- a/ethcore/src/engines/validator_set/simple_list.rs +++ b/ethcore/src/engines/validator_set/simple_list.rs @@ -33,9 +33,14 @@ pub struct SimpleList { impl SimpleList { /// Create a new `SimpleList`. pub fn new(validators: Vec
) -> Self { - SimpleList { - validators: validators, + let validator_count = validators.len(); + if validator_count == 1 { + warn!(target: "engine", "Running AuRa with a single validator implies instant finality. Use a database?"); } + if validator_count % 2 == 0 { + warn!(target: "engine", "Running AuRa with an even number of validators is not recommended (risk of network split)."); + } + SimpleList { validators } } /// Convert into inner representation. @@ -52,9 +57,7 @@ impl ::std::ops::Deref for SimpleList { impl From> for SimpleList { fn from(validators: Vec
) -> Self { - SimpleList { - validators: validators, - } + SimpleList::new(validators) } }