Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Log validator set changes in EpochManager #10734

Merged
merged 36 commits into from
Jun 25, 2019
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
4050074
Stop breaking out of loop if a non-canonical hash is found
HCastano Apr 4, 2019
88bafbe
include expected hash in log msg
dvdplm Jun 10, 2019
fd2a19f
More logging
dvdplm Jun 10, 2019
704463d
Scope
dvdplm Jun 10, 2019
5f9caa5
Syntax
dvdplm Jun 10, 2019
befe9b8
Log in blank RollingFinality
dvdplm Jun 10, 2019
1b7acd0
Check validator set size: warn if 1 or even number
dvdplm Jun 10, 2019
4afc40b
More readable code
dvdplm Jun 10, 2019
1ed6505
Merge branch 'dp/hc-non-canon-block-logging' into dp/chore/aura-warn-…
dvdplm Jun 10, 2019
c77e8da
Use SimpleList::new
dvdplm Jun 10, 2019
b680026
Extensive logging on unexpected non-canonical hash
dvdplm Jun 10, 2019
2551e0f
Wording
dvdplm Jun 10, 2019
06c0ba4
Merge branch 'dp/hc-non-canon-block-logging' into dp/chore/aura-warn-…
dvdplm Jun 10, 2019
8ed48b4
wip
dvdplm Jun 10, 2019
ffbfdac
Update ethcore/blockchain/src/blockchain.rs
dvdplm Jun 10, 2019
b257e5e
Improved logging, address grumbles
dvdplm Jun 10, 2019
b5743b8
Merge branch 'dp/chore/aura-warn-when-validators-is-1-or-even' of git…
dvdplm Jun 10, 2019
070e585
Merge branch 'master' into dp/chore/aura-warn-when-validators-is-1-or…
dvdplm Jun 10, 2019
4f00ac7
Merge branch 'master' into dp/chore/aura-log-validator-set-in-epoch-m…
dvdplm Jun 10, 2019
d338411
Merge branch 'dp/chore/aura-warn-when-validators-is-1-or-even' into d…
dvdplm Jun 10, 2019
0edf27e
Update ethcore/src/engines/validator_set/simple_list.rs
dvdplm Jun 11, 2019
b32d632
Report benign misbehaviour iff currently a validator
dvdplm Jun 11, 2019
b62d11f
Merge branch 'dp/chore/aura-log-validator-set-in-epoch-manager' of gi…
dvdplm Jun 11, 2019
eb264b7
Report malicious behaviour iff we're a validator
dvdplm Jun 11, 2019
8e95d01
Escalate to warning and fix wording
dvdplm Jun 11, 2019
d09fea8
Test reporting behaviour
dvdplm Jun 11, 2019
67dda77
Include missing parent hash in MissingParent error
dvdplm Jun 11, 2019
353e6b9
Update ethcore/src/engines/validator_set/simple_list.rs
dvdplm Jun 12, 2019
5dff9e1
Merge branch 'master' into dp/chore/aura-warn-when-validators-is-1-or…
dvdplm Jun 19, 2019
ded29d8
Merge branch 'dp/chore/aura-warn-when-validators-is-1-or-even' into d…
dvdplm Jun 19, 2019
fbb5994
Merge branch 'master' into dp/chore/aura-log-validator-set-in-epoch-m…
dvdplm Jun 24, 2019
1a304d3
cleanup
dvdplm Jun 24, 2019
1a0f82b
On second thought non-validators are allowed to report
dvdplm Jun 24, 2019
05df053
cleanup
dvdplm Jun 24, 2019
8a83bd8
Merge branch 'master' into dp/chore/aura-log-validator-set-in-epoch-m…
dvdplm Jun 25, 2019
9080630
remove dead code
dvdplm Jun 25, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions ethcore/blockchain/src/blockchain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ use ethereum_types::{H256, Bloom, BloomRef, U256};
use heapsize::HeapSizeOf;
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::*;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Expand All @@ -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));

},
Expand Down
2 changes: 0 additions & 2 deletions ethcore/src/engines/authority_round/finality.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
64 changes: 47 additions & 17 deletions ethcore/src/engines/authority_round/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,8 @@ impl EpochManager {

self.force = false;
debug!(target: "engine", "Zooming to epoch after block {}", hash);
trace!(target: "engine", "Current validator set: {:?}", self.validators());


// epoch_transition_for can be an expensive call, but in the absence of
// forks it will only need to be called for the block directly after
Expand All @@ -257,7 +259,7 @@ impl EpochManager {
let (signal_number, set_proof, _) = destructure_proofs(&last_transition.proof)
.expect("proof produced by this engine; therefore it is valid; qed");

trace!(target: "engine", "extracting epoch set for epoch ({}, {}) signalled at #{}",
trace!(target: "engine", "extracting epoch validator set for epoch ({}, {}) signalled at #{}",
last_transition.block_number, last_transition.block_hash, signal_number);

let first = signal_number == 0;
Expand All @@ -268,7 +270,12 @@ impl EpochManager {
set_proof,
)
.ok()
.map(|(list, _)| list.into_inner())
.map(|(list, _)| {
trace!(target: "engine", "Updating finality checker with new validator set extracted from epoch ({}, {}): {:?}",
last_transition.block_number, last_transition.block_hash, &list);

list.into_inner()
})
.expect("proof produced by this engine; therefore it is valid; qed");

self.finality_checker = RollingFinality::blank(epoch_set);
Expand Down Expand Up @@ -808,19 +815,23 @@ impl AuthorityRound {
return;
}

if let (true, Some(me)) = (current_step > parent_step + 1, self.signer.read().as_ref().map(|s| s.address())) {
if let (true, me) = (current_step > parent_step + 1, self.address()) {
debug!(target: "engine", "Author {} built block with step gap. current step: {}, parent step: {}",
dvdplm marked this conversation as resolved.
Show resolved Hide resolved
header.author(), current_step, parent_step);
let mut reported = HashSet::new();
for step in parent_step + 1..current_step {
let skipped_primary = step_proposer(validators, header.parent_hash(), step);
// Do not report this signer.
if skipped_primary != me {
// Stop reporting once validators start repeating.
if !reported.insert(skipped_primary) { break; }
self.validators.report_benign(&skipped_primary, set_number, header.number());
}
}
if validators.contains(header.parent_hash(), &me) {
dvdplm marked this conversation as resolved.
Show resolved Hide resolved
let mut reported = HashSet::new();
for step in parent_step + 1..current_step {
let skipped_primary = step_proposer(validators, header.parent_hash(), step);
// Do not report this signer.
if skipped_primary != me {
// Stop reporting once validators start repeating.
if !reported.insert(skipped_primary) { break; }
self.validators.report_benign(&skipped_primary, set_number, header.number());
}
}
} else {
debug!(target: "engine", "We are not part of the validator set so not reporting (skipped steps). Own address: {}", self.address());
}
}
}

Expand Down Expand Up @@ -886,6 +897,10 @@ impl AuthorityRound {
let finalized = epoch_manager.finality_checker.push_hash(chain_head.hash(), vec![*chain_head.author()]);
finalized.unwrap_or_default()
}

fn address(&self) -> Address {
self.signer.read().as_ref().map_or_else(|| Address::zero(), |s| s.address())
}
}

fn unix_now() -> Duration {
Expand Down Expand Up @@ -1291,8 +1306,13 @@ impl Engine<EthereumMachine> for AuthorityRound {
// likely ignore old reports
// - This specific check is only relevant if you're importing (since it checks
// against wall clock)
if let Ok((_, set_number)) = self.epoch_set(header) {
self.validators.report_benign(header.author(), set_number, header.number());
if let Ok((set, set_number)) = self.epoch_set(header) {
let me = self.address();
if set.contains(header.parent_hash(), &me) {
self.validators.report_benign(header.author(), set_number, header.number());
} else {
debug!(target: "engine", "Not reporting benign misbehaviour (cause: InvalidSeal) because we're not part of the validator set, Own address: {}", me);
dvdplm marked this conversation as resolved.
Show resolved Hide resolved
}
}

Err(BlockError::InvalidSeal.into())
Expand Down Expand Up @@ -1362,7 +1382,12 @@ impl Engine<EthereumMachine> for AuthorityRound {
match validate_empty_steps() {
Ok(len) => len,
Err(err) => {
self.validators.report_benign(header.author(), set_number, header.number());
if self.validators.contains(header.parent_hash(), &self.address()) {
self.validators.report_benign(header.author(), set_number, header.number());
} else {
debug!(target: "engine", "Not reporting benign misbehaviour (cause: invalid empty steps) because we're not part of the validator set. Own address: {}",
self.address());
}
return Err(err);
},
}
Expand Down Expand Up @@ -1391,7 +1416,11 @@ impl Engine<EthereumMachine> for AuthorityRound {
let res = verify_external(header, &*validators, self.empty_steps_transition);
match res {
Err(Error::Engine(EngineError::NotProposer(_))) => {
self.validators.report_benign(header.author(), set_number, header.number());
if self.validators.contains(header.parent_hash(), &self.address()) {
self.validators.report_benign(header.author(), set_number, header.number());
} else {
debug!(target: "engine", "We are not part of the validator set so not reporting (block from incorrect proposer). Own address: {}", self.address());
}
},
Ok(_) => {
// we can drop all accumulated empty step messages that are older than this header's step
Expand Down Expand Up @@ -1823,6 +1852,7 @@ mod tests {

#[test]
fn reports_skipped() {
let _ = ::env_logger::try_init();
let last_benign = Arc::new(AtomicUsize::new(0));
let aura = aura(|p| {
p.validators = Box::new(TestSet::new(Default::default(), last_benign.clone()));
Expand Down
14 changes: 9 additions & 5 deletions ethcore/src/engines/validator_set/safe_contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
},
}
Expand Down Expand Up @@ -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);
Expand All @@ -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)))
Expand All @@ -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()),
}
}
Expand Down
13 changes: 8 additions & 5 deletions ethcore/src/engines/validator_set/simple_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 recommented (risk of network split).");
dvdplm marked this conversation as resolved.
Show resolved Hide resolved
}
SimpleList { validators }
}

/// Convert into inner representation.
Expand All @@ -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)
}
}

Expand Down