Print warnings when using dangerous settings for ValidatorSet (#10733)

* 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 <tomusdrw@users.noreply.github.com>

* Improved logging, address grumbles

* Update ethcore/src/engines/validator_set/simple_list.rs

Co-Authored-By: Luke Schoen <ltfschoen@users.noreply.github.com>
This commit is contained in:
David 2019-06-24 14:50:11 +02:00 committed by GitHub
parent 55c046cb88
commit f2dd032884
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 22 additions and 17 deletions

View File

@ -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));
},

View File

@ -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<Address>) -> Result<Vec<H256>, 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);

View File

@ -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()),
}
}

View File

@ -33,9 +33,14 @@ pub struct SimpleList {
impl SimpleList {
/// Create a new `SimpleList`.
pub fn new(validators: Vec<Address>) -> 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<Vec<Address>> for SimpleList {
fn from(validators: Vec<Address>) -> Self {
SimpleList {
validators: validators,
}
SimpleList::new(validators)
}
}