From 4050074e90e406f3c7314728d039e101eb21fb2b Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Thu, 4 Apr 2019 12:40:24 +0200 Subject: [PATCH 01/42] Stop breaking out of loop if a non-canonical hash is found --- ethcore/blockchain/src/blockchain.rs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/ethcore/blockchain/src/blockchain.rs b/ethcore/blockchain/src/blockchain.rs index b528334cad3..efd653f42d0 100644 --- a/ethcore/blockchain/src/blockchain.rs +++ b/ethcore/blockchain/src/blockchain.rs @@ -963,6 +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"); let iter = self.db.key_value().iter_from_prefix(db::COL_EXTRA, &EPOCH_KEY_PREFIX[..]); EpochTransitionIter { chain: self, @@ -988,7 +989,9 @@ impl BlockChain { pub fn epoch_transition_for(&self, parent_hash: H256) -> Option { // slow path: loop back block by block for hash in self.ancestry_iter(parent_hash)? { + trace!(target: "blockchain", "Got hash {} from ancestry_iter", hash); let details = self.block_details(&hash)?; + trace!(target: "blockchain", "Got block details for block #{}", details.number); // look for transition in database. if let Some(transition) = self.epoch_transition(details.number, hash) { @@ -1000,11 +1003,15 @@ impl BlockChain { // // if `block_hash` is canonical it will only return transitions up to // the parent. - if self.block_hash(details.number)? == hash { - return self.epoch_transitions() - .map(|(_, t)| t) - .take_while(|t| t.block_number <= details.number) - .last() + match self.block_hash(details.number) { + Some(h) if h == hash => { + return self.epoch_transitions() + .map(|(_, t)| t) + .take_while(|t| t.block_number <= details.number) + .last() + }, + Some(h) => trace!(target: "blockchain", "Found non-canonical block hash {}", h), + None => trace!(target: "blockchain", "Block hash not found in cache or DB"), } } From 88bafbe8d401c3ed970522f8d64f8228a32d5ec8 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 10 Jun 2019 12:40:16 +0200 Subject: [PATCH 02/42] include expected hash in log msg --- ethcore/blockchain/src/blockchain.rs | 2 +- ethcore/src/engines/authority_round/mod.rs | 6 +++--- ethcore/src/verification/verifier.rs | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/ethcore/blockchain/src/blockchain.rs b/ethcore/blockchain/src/blockchain.rs index efd653f42d0..8d4283d4616 100644 --- a/ethcore/blockchain/src/blockchain.rs +++ b/ethcore/blockchain/src/blockchain.rs @@ -1010,7 +1010,7 @@ impl BlockChain { .take_while(|t| t.block_number <= details.number) .last() }, - Some(h) => trace!(target: "blockchain", "Found non-canonical block hash {}", h), + Some(h) => trace!(target: "blockchain", "Found non-canonical block hash {} (expected {})", h, hash), None => trace!(target: "blockchain", "Block hash not found in cache or DB"), } } diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index 0942c0881ba..3b3d4ee8921 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -247,7 +247,7 @@ impl EpochManager { None => { // this really should never happen unless the block passed // hasn't got a parent in the database. - debug!(target: "engine", "No genesis transition found."); + warn!(target: "engine", "No genesis transition found."); return false; } }; @@ -280,8 +280,8 @@ impl EpochManager { true } - // note new epoch hash. this will force the next block to re-load - // the epoch set + // Note new epoch hash. This will force the next block to re-load + // the epoch set. // TODO: optimize and don't require re-loading after epoch change. fn note_new_epoch(&mut self) { self.force = true; diff --git a/ethcore/src/verification/verifier.rs b/ethcore/src/verification/verifier.rs index 76eb60b9a18..f7221dae81d 100644 --- a/ethcore/src/verification/verifier.rs +++ b/ethcore/src/verification/verifier.rs @@ -38,6 +38,6 @@ pub trait Verifier: Send + Sync /// Do a final verification check for an enacted header vs its expected counterpart. fn verify_block_final(&self, expected: &Header, got: &Header) -> Result<(), Error>; - /// Verify a block, inspecing external state. + /// Verify a block, inspecting external state. fn verify_block_external(&self, header: &Header, engine: &EthEngine) -> Result<(), Error>; } From fd2a19f00a5b773e5f9ac7d8178f49de482b4002 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 10 Jun 2019 12:46:14 +0200 Subject: [PATCH 03/42] More logging --- ethcore/blockchain/src/blockchain.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ethcore/blockchain/src/blockchain.rs b/ethcore/blockchain/src/blockchain.rs index 8d4283d4616..287e286efc0 100644 --- a/ethcore/blockchain/src/blockchain.rs +++ b/ethcore/blockchain/src/blockchain.rs @@ -989,9 +989,9 @@ impl BlockChain { pub fn epoch_transition_for(&self, parent_hash: H256) -> Option { // slow path: loop back block by block for hash in self.ancestry_iter(parent_hash)? { - trace!(target: "blockchain", "Got hash {} from ancestry_iter", hash); + trace!(target: "blockchain", "Block #{}: Got hash {} from ancestry_iter", details.number, hash); let details = self.block_details(&hash)?; - trace!(target: "blockchain", "Got block details for block #{}", details.number); + trace!(target: "blockchain", "Block #{}: Got block details", details.number); // look for transition in database. if let Some(transition) = self.epoch_transition(details.number, hash) { @@ -1010,8 +1010,8 @@ impl BlockChain { .take_while(|t| t.block_number <= details.number) .last() }, - Some(h) => trace!(target: "blockchain", "Found non-canonical block hash {} (expected {})", h, hash), - None => trace!(target: "blockchain", "Block hash not found in cache or DB"), + Some(h) => trace!(target: "blockchain", "Block #{}: Found non-canonical block hash {} (expected {})", details.number, h, hash), + None => trace!(target: "blockchain", "Block #{}: hash {} not found in cache or DB", details.number, hash), } } From 704463d4aea4604b05b20b80a5f77be0f91920f2 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 10 Jun 2019 12:49:16 +0200 Subject: [PATCH 04/42] Scope --- ethcore/blockchain/src/blockchain.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethcore/blockchain/src/blockchain.rs b/ethcore/blockchain/src/blockchain.rs index 287e286efc0..9a8be4d9b4c 100644 --- a/ethcore/blockchain/src/blockchain.rs +++ b/ethcore/blockchain/src/blockchain.rs @@ -989,7 +989,7 @@ impl BlockChain { pub fn epoch_transition_for(&self, parent_hash: H256) -> Option { // slow path: loop back block by block for hash in self.ancestry_iter(parent_hash)? { - trace!(target: "blockchain", "Block #{}: Got hash {} from ancestry_iter", details.number, 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); From 5f9caa57b31eef6a23bd7aedd104558623118a66 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 10 Jun 2019 13:26:36 +0200 Subject: [PATCH 05/42] Syntax --- ethcore/src/engines/authority_round/finality.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/ethcore/src/engines/authority_round/finality.rs b/ethcore/src/engines/authority_round/finality.rs index af57278c9f3..1f2bb559ea8 100644 --- a/ethcore/src/engines/authority_round/finality.rs +++ b/ethcore/src/engines/authority_round/finality.rs @@ -110,7 +110,16 @@ 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> { - if signers.iter().any(|s| !self.signers.contains(s)) { return Err(UnknownValidator) } + if signers + .iter() + .any(|s| { + if !self.signers.contains(s) { + warn!(target: "finality", "Unknown validator: {}", s); + false + } else { true } + }) { + return Err(UnknownValidator) + } for signer in signers.iter() { *self.sign_count.entry(*signer).or_insert(0) += 1; @@ -141,7 +150,7 @@ impl RollingFinality { } } - trace!(target: "finality", "Blocks finalized by {:?}: {:?}", head, newly_finalized); + trace!(target: "finality", "{} Blocks finalized by {:?}: {:?}", newly_finalized.len(), head, newly_finalized); self.last_pushed = Some(head); Ok(newly_finalized) From befe9b87130d799ad830fa1435e65b480b9da0f0 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 10 Jun 2019 13:41:36 +0200 Subject: [PATCH 06/42] Log in blank RollingFinality Escalate bad proposer to warning --- ethcore/src/engines/authority_round/finality.rs | 1 + ethcore/src/engines/authority_round/mod.rs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/ethcore/src/engines/authority_round/finality.rs b/ethcore/src/engines/authority_round/finality.rs index 1f2bb559ea8..fef0ebb30eb 100644 --- a/ethcore/src/engines/authority_round/finality.rs +++ b/ethcore/src/engines/authority_round/finality.rs @@ -39,6 +39,7 @@ pub struct RollingFinality { impl RollingFinality { /// Create a blank finality checker under the given validator set. pub fn blank(signers: Vec
) -> Self { + trace!(target: "finality", "Instantiating blank RollingFinality with {} signers: {:?}", signers.len(), signers); RollingFinality { headers: VecDeque::new(), signers: SimpleList::new(signers), diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index 3b3d4ee8921..8bf838d2dc5 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -614,7 +614,7 @@ fn verify_external(header: &Header, validators: &ValidatorSet, empty_steps_trans }; if is_invalid_proposer { - trace!(target: "engine", "verify_block_external: bad proposer for step: {}", header_step); + warn!(target: "engine", "verify_block_external: bad proposer for step: {}", header_step); Err(EngineError::NotProposer(Mismatch { expected: correct_proposer, found: *header.author() }))? } else { Ok(()) From 1b7acd02c8aef8fdde98de35fea3eef4ed5cde99 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 10 Jun 2019 13:53:51 +0200 Subject: [PATCH 07/42] Check validator set size: warn if 1 or even number --- ethcore/src/engines/validator_set/simple_list.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/ethcore/src/engines/validator_set/simple_list.rs b/ethcore/src/engines/validator_set/simple_list.rs index 0a0294be966..d6f0730e0b4 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 is not supported."); + } + if validator_count % 2 == 0 { + warn!(target: "engine", "Running AuRa with an even number of validators is not supported."); } + SimpleList { validators } } /// Convert into inner representation. From 4afc40b6e0ec59a033eeb9b9a4618a3792843d65 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 10 Jun 2019 14:13:10 +0200 Subject: [PATCH 08/42] More readable code --- ethcore/src/engines/authority_round/finality.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/ethcore/src/engines/authority_round/finality.rs b/ethcore/src/engines/authority_round/finality.rs index fef0ebb30eb..e88458d81aa 100644 --- a/ethcore/src/engines/authority_round/finality.rs +++ b/ethcore/src/engines/authority_round/finality.rs @@ -111,15 +111,13 @@ 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> { - if signers - .iter() - .any(|s| { - if !self.signers.contains(s) { - warn!(target: "finality", "Unknown validator: {}", s); - false - } else { true } - }) { + // 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); return Err(UnknownValidator) + } } for signer in signers.iter() { From c77e8daf2d5913603ab293099957c414b190bdc1 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 10 Jun 2019 15:02:48 +0200 Subject: [PATCH 09/42] Use SimpleList::new --- ethcore/src/engines/validator_set/safe_contract.rs | 14 +++++++++----- ethcore/src/engines/validator_set/simple_list.rs | 4 +--- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/ethcore/src/engines/validator_set/safe_contract.rs b/ethcore/src/engines/validator_set/safe_contract.rs index c210bcc00d6..fc89a97ddba 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 d6f0730e0b4..1ad5b99c459 100644 --- a/ethcore/src/engines/validator_set/simple_list.rs +++ b/ethcore/src/engines/validator_set/simple_list.rs @@ -57,9 +57,7 @@ impl ::std::ops::Deref for SimpleList { impl From> for SimpleList { fn from(validators: Vec
) -> Self { - SimpleList { - validators: validators, - } + SimpleList::new(validators) } } From b6800262179b4331c2b8d9d61ffd69bee4742ba7 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 10 Jun 2019 15:19:23 +0200 Subject: [PATCH 10/42] Extensive logging on unexpected non-canonical hash --- ethcore/blockchain/src/blockchain.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/ethcore/blockchain/src/blockchain.rs b/ethcore/blockchain/src/blockchain.rs index 9a8be4d9b4c..b188834193c 100644 --- a/ethcore/blockchain/src/blockchain.rs +++ b/ethcore/blockchain/src/blockchain.rs @@ -1010,7 +1010,14 @@ impl BlockChain { .take_while(|t| t.block_number <= details.number) .last() }, - Some(h) => trace!(target: "blockchain", "Block #{}: Found non-canonical block hash {} (expected {})", details.number, h, hash), + Some(h) => { + warn!(target: "blockchain", "Block #{}: Found non-canonical block hash {} (expected {})", details.number, h, hash); + + trace!(target: "blockchain", "#{} {} != {} HASH UNEQUAL, #{}", details.number, hash, h, self.block_number(&h).unwrap_or_default() ); + trace!(target: "blockchain", " Version A {}: #{:#?}", hash, self.block_details(&hash)); + trace!(target: "blockchain", " Version B {}: #{:#?}", h, self.block_details(&h)); + + }, None => trace!(target: "blockchain", "Block #{}: hash {} not found in cache or DB", details.number, hash), } } From 2551e0fc6acc4c4784771f19341aec3d800c88bf Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 10 Jun 2019 15:23:32 +0200 Subject: [PATCH 11/42] Wording --- ethcore/blockchain/src/blockchain.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ethcore/blockchain/src/blockchain.rs b/ethcore/blockchain/src/blockchain.rs index b188834193c..7cf6b12dd5c 100644 --- a/ethcore/blockchain/src/blockchain.rs +++ b/ethcore/blockchain/src/blockchain.rs @@ -1013,9 +1013,9 @@ impl BlockChain { Some(h) => { warn!(target: "blockchain", "Block #{}: Found non-canonical block hash {} (expected {})", details.number, h, hash); - trace!(target: "blockchain", "#{} {} != {} HASH UNEQUAL, #{}", details.number, hash, h, self.block_number(&h).unwrap_or_default() ); - trace!(target: "blockchain", " Version A {}: #{:#?}", hash, self.block_details(&hash)); - trace!(target: "blockchain", " Version B {}: #{:#?}", h, self.block_details(&h)); + 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", " Own {}: #{:#?}", h, self.block_details(&h)); }, None => trace!(target: "blockchain", "Block #{}: hash {} not found in cache or DB", details.number, hash), From 8ed48b463467fe05d4e913bd9f72cb9e78200e2a Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 10 Jun 2019 18:11:02 +0200 Subject: [PATCH 12/42] wip --- ethcore/src/engines/authority_round/mod.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index 8bf838d2dc5..a900a149cf9 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -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 @@ -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; @@ -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); From ffbfdacaefc319ce71c3aa0fd664d35541206892 Mon Sep 17 00:00:00 2001 From: David Date: Mon, 10 Jun 2019 18:51:11 +0200 Subject: [PATCH 13/42] Update ethcore/blockchain/src/blockchain.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Tomasz Drwięga --- ethcore/blockchain/src/blockchain.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethcore/blockchain/src/blockchain.rs b/ethcore/blockchain/src/blockchain.rs index 7cf6b12dd5c..df0acddfdbb 100644 --- a/ethcore/blockchain/src/blockchain.rs +++ b/ethcore/blockchain/src/blockchain.rs @@ -1014,7 +1014,7 @@ impl BlockChain { 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", " Ancestor {}: #{:#?}", hash, details); trace!(target: "blockchain", " Own {}: #{:#?}", h, self.block_details(&h)); }, From b257e5e8c623e332bff7d32ddc2a33d774418197 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 10 Jun 2019 18:54:20 +0200 Subject: [PATCH 14/42] Improved logging, address grumbles --- ethcore/blockchain/src/blockchain.rs | 8 ++++---- ethcore/src/engines/authority_round/finality.rs | 2 -- ethcore/src/engines/validator_set/simple_list.rs | 4 ++-- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/ethcore/blockchain/src/blockchain.rs b/ethcore/blockchain/src/blockchain.rs index 7cf6b12dd5c..eab5be9900b 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 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::*; @@ -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,7 +1013,7 @@ 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", "Block #{} Mismatched hashes. Ancestor {} != Own {}", details.number, hash, h); trace!(target: "blockchain", " Ancestor {}: #{:#?}", hash, self.block_details(&hash)); 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 e88458d81aa..e1f050393ea 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/simple_list.rs b/ethcore/src/engines/validator_set/simple_list.rs index 1ad5b99c459..dd8f97f091e 100644 --- a/ethcore/src/engines/validator_set/simple_list.rs +++ b/ethcore/src/engines/validator_set/simple_list.rs @@ -35,10 +35,10 @@ impl SimpleList { pub fn new(validators: Vec
) -> Self { let validator_count = validators.len(); if validator_count == 1 { - warn!(target: "engine", "Running AuRa with a single validator is not supported."); + 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 supported."); + warn!(target: "engine", "Running AuRa with an even number of validators is not recommented (risk of network split)."); } SimpleList { validators } } From 0edf27ec622cbd3fb0b1044e810753cd0b0c0c17 Mon Sep 17 00:00:00 2001 From: David Date: Tue, 11 Jun 2019 06:34:58 +0200 Subject: [PATCH 15/42] Update ethcore/src/engines/validator_set/simple_list.rs Co-Authored-By: Luke Schoen --- ethcore/src/engines/validator_set/simple_list.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethcore/src/engines/validator_set/simple_list.rs b/ethcore/src/engines/validator_set/simple_list.rs index dd8f97f091e..44b6df4cf34 100644 --- a/ethcore/src/engines/validator_set/simple_list.rs +++ b/ethcore/src/engines/validator_set/simple_list.rs @@ -38,7 +38,7 @@ impl SimpleList { 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)."); + warn!(target: "engine", "Running AuRa with an even number of validators is not recommended (risk of network split)."); } SimpleList { validators } } From b32d6325c4f1cfdf1e4d71432b8985b5e789c0d6 Mon Sep 17 00:00:00 2001 From: David Palm Date: Tue, 11 Jun 2019 09:00:20 +0200 Subject: [PATCH 16/42] Report benign misbehaviour iff currently a validator --- ethcore/src/engines/authority_round/mod.rs | 53 ++++++++++++++++------ 1 file changed, 38 insertions(+), 15 deletions(-) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index a900a149cf9..79b834898f3 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -815,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: {}", 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) { + 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()); + } } } @@ -893,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 { @@ -1298,8 +1306,13 @@ impl Engine 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); + } } Err(BlockError::InvalidSeal.into()) @@ -1369,7 +1382,12 @@ impl Engine 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); }, } @@ -1398,7 +1416,11 @@ impl Engine 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 @@ -1830,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())); From eb264b7d7da3a6c68bbaeb036744bf9957b14e4b Mon Sep 17 00:00:00 2001 From: David Palm Date: Tue, 11 Jun 2019 09:56:42 +0200 Subject: [PATCH 17/42] Report malicious behaviour iff we're a validator --- ethcore/src/engines/authority_round/mod.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index 79b834898f3..03353dea1ba 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -1332,9 +1332,13 @@ impl Engine for AuthorityRound { // Ensure header is from the step after parent. if step == parent_step || (header.number() >= self.validate_step_transition && step <= parent_step) { - trace!(target: "engine", "Multiple blocks proposed for step {}.", parent_step); + warn!(target: "engine", "Multiple blocks proposed for step {}.", parent_step); - self.validators.report_malicious(header.author(), set_number, header.number(), Default::default()); + if validators.contains(header.parent_hash(), self.address() ) { + self.validators.report_malicious(header.author(), set_number, header.number(), Default::default()); + } else { + debug!(target: "engine", "Not reporting malicious behaviour because we're not a validator. Own address: {}", self.address()); + } Err(EngineError::DoubleVote(*header.author()))?; } From 8e95d01f3a85518c785402cfa3a50f0b3baf596a Mon Sep 17 00:00:00 2001 From: David Palm Date: Tue, 11 Jun 2019 10:04:15 +0200 Subject: [PATCH 18/42] Escalate to warning and fix wording --- ethcore/src/engines/authority_round/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index 03353dea1ba..033275b8862 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -830,7 +830,7 @@ impl AuthorityRound { } } } else { - debug!(target: "engine", "We are not part of the validator set so not reporting (skipped steps). Own address: {}", self.address()); + warn!(target: "engine", "We are not part of the validator set so not reporting (skipped steps). Own address: {}", self.address()); } } } @@ -1337,7 +1337,7 @@ impl Engine for AuthorityRound { if validators.contains(header.parent_hash(), self.address() ) { self.validators.report_malicious(header.author(), set_number, header.number(), Default::default()); } else { - debug!(target: "engine", "Not reporting malicious behaviour because we're not a validator. Own address: {}", self.address()); + warn!(target: "engine", "Not reporting malicious behaviour (cause: multiple blocks proposed) because we're not a validator. Own address: {}", self.address()); } Err(EngineError::DoubleVote(*header.author()))?; } @@ -1389,7 +1389,7 @@ impl Engine for AuthorityRound { 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: {}", + warn!(target: "engine", "Not reporting benign misbehaviour (cause: invalid empty steps) because we're not a validator. Own address: {}", self.address()); } return Err(err); @@ -1423,7 +1423,7 @@ impl Engine for AuthorityRound { 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()); + warn!(target: "engine", "Not reporting benign misbehaviour (cause:block from incorrect proposer) because we're not a validator. Own address: {}", self.address()); } }, Ok(_) => { From d09fea825d29cfc83330bb01f87dd614d03e5981 Mon Sep 17 00:00:00 2001 From: David Palm Date: Tue, 11 Jun 2019 13:45:37 +0200 Subject: [PATCH 19/42] Test reporting behaviour Don't require node to be part of the validator set to report malicious behaviour --- ethcore/src/engines/authority_round/mod.rs | 79 +++++++++++++++---- .../src/engines/validator_set/simple_list.rs | 5 +- ethcore/src/engines/validator_set/test.rs | 16 +++- 3 files changed, 78 insertions(+), 22 deletions(-) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index 033275b8862..dd16689db6d 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -707,7 +707,7 @@ impl AuthorityRound { empty_steps_transition: our_params.empty_steps_transition, maximum_empty_steps: our_params.maximum_empty_steps, strict_empty_steps_transition: our_params.strict_empty_steps_transition, - machine: machine, + machine, }); // Do not initialize timeouts for tests. @@ -827,6 +827,8 @@ impl AuthorityRound { // Stop reporting once validators start repeating. if !reported.insert(skipped_primary) { break; } self.validators.report_benign(&skipped_primary, set_number, header.number()); + } else { + trace!(target: "engine", "Primary that skipped is self, not self-reporting. Own address: {}", me); } } } else { @@ -1334,11 +1336,7 @@ impl Engine for AuthorityRound { || (header.number() >= self.validate_step_transition && step <= parent_step) { warn!(target: "engine", "Multiple blocks proposed for step {}.", parent_step); - if validators.contains(header.parent_hash(), self.address() ) { - self.validators.report_malicious(header.author(), set_number, header.number(), Default::default()); - } else { - warn!(target: "engine", "Not reporting malicious behaviour (cause: multiple blocks proposed) because we're not a validator. Own address: {}", self.address()); - } + self.validators.report_malicious(header.author(), set_number, header.number(), Default::default()); Err(EngineError::DoubleVote(*header.author()))?; } @@ -1654,7 +1652,7 @@ mod tests { use error::Error; use super::{AuthorityRoundParams, AuthorityRound, EmptyStep, SealedEmptyStep, calculate_score}; - fn aura(f: F) -> Arc where + fn build_aura(f: F) -> Arc where F: FnOnce(&mut AuthorityRoundParams), { let mut params = AuthorityRoundParams { @@ -1676,7 +1674,6 @@ mod tests { // mutate aura params f(&mut params); - // create engine let mut c_params = ::spec::CommonParams::default(); c_params.gas_limit_bound_divisor = 5.into(); @@ -1857,9 +1854,18 @@ mod tests { #[test] fn reports_skipped() { let _ = ::env_logger::try_init(); + + let validator1 = Address::from_low_u64_be(1); + let validator2 = Address::from_low_u64_be(2); let last_benign = Arc::new(AtomicUsize::new(0)); - let aura = aura(|p| { - p.validators = Box::new(TestSet::new(Default::default(), last_benign.clone())); + + let aura = build_aura(|p| { + let validator_set = TestSet::new( + Default::default(), + last_benign.clone(), + vec![validator1, validator2], + ); + p.validators = Box::new(validator_set); }); let mut parent_header: Header = Header::default(); @@ -1874,7 +1880,13 @@ mod tests { assert!(aura.verify_block_family(&header, &parent_header).is_ok()); assert_eq!(last_benign.load(AtomicOrdering::SeqCst), 0); - aura.set_signer(Box::new((Arc::new(AccountProvider::transient_provider()), Default::default(), "".into()))); + aura.set_signer(Box::new( + ( + Arc::new(AccountProvider::transient_provider()), + validator2, + "".into() + ) + )); // Do not report on steps skipped between genesis and first block. header.set_number(1); @@ -1887,9 +1899,44 @@ mod tests { assert_eq!(last_benign.load(AtomicOrdering::SeqCst), 2); } + #[test] + fn skips_benign_reporting_unless_validator() { + let _ = ::env_logger::try_init(); + + let last_benign = Arc::new(AtomicUsize::new(0)); + let aura = build_aura(|p| { + let validator_set = TestSet::default().with_last_benign(last_benign.clone()); + p.validators = Box::new(validator_set); + }); + aura.set_signer(Box::new( + ( + Arc::new(AccountProvider::transient_provider()), + Address::from_low_u64_be(123), + "".into() + ) + )); + let mut parent_header: Header = Header::default(); + parent_header.set_seal(vec![encode(&1usize)]); + parent_header.set_gas_limit("222222".parse::().unwrap()); + let mut header: Header = Header::default(); + header.set_difficulty(calculate_score(1, 3, 0)); + header.set_gas_limit("222222".parse::().unwrap()); + header.set_seal(vec![encode(&3usize)]); + + header.set_number(1); + aura.verify_block_family(&header, &parent_header).unwrap(); + header.set_number(2); + aura.verify_block_family(&header, &parent_header).unwrap(); + header.set_number(3); + aura.verify_block_family(&header, &parent_header).unwrap(); + + // No reporting happened: we're not a validator + assert_eq!(last_benign.load(AtomicOrdering::SeqCst), 0); + } + #[test] fn test_uncles_transition() { - let aura = aura(|params| { + let aura = build_aura(|params| { params.maximum_uncle_count_transition = 1; }); @@ -1925,7 +1972,7 @@ mod tests { #[test] #[should_panic(expected="authority_round: step duration can't be zero")] fn test_step_duration_zero() { - aura(|params| { + build_aura(|params| { params.step_duration = 0; }); } @@ -2317,7 +2364,7 @@ mod tests { #[test] fn test_empty_steps() { - let engine = aura(|p| { + let engine = build_aura(|p| { p.step_duration = 4; p.empty_steps_transition = 0; p.maximum_empty_steps = 0; @@ -2350,7 +2397,7 @@ mod tests { fn should_reject_duplicate_empty_steps() { // given let (_spec, tap, accounts) = setup_empty_steps(); - let engine = aura(|p| { + let engine = build_aura(|p| { p.validators = Box::new(SimpleList::new(accounts.clone())); p.step_duration = 4; p.empty_steps_transition = 0; @@ -2387,7 +2434,7 @@ mod tests { fn should_reject_empty_steps_out_of_order() { // given let (_spec, tap, accounts) = setup_empty_steps(); - let engine = aura(|p| { + let engine = build_aura(|p| { p.validators = Box::new(SimpleList::new(accounts.clone())); p.step_duration = 4; p.empty_steps_transition = 0; diff --git a/ethcore/src/engines/validator_set/simple_list.rs b/ethcore/src/engines/validator_set/simple_list.rs index 44b6df4cf34..4dc506b3a96 100644 --- a/ethcore/src/engines/validator_set/simple_list.rs +++ b/ethcore/src/engines/validator_set/simple_list.rs @@ -36,9 +36,8 @@ impl SimpleList { 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)."); + } else if validator_count != 0 && validator_count % 2 == 0 { + warn!(target: "engine", "Running AuRa with an even number of validators ({}) is not recommended (risk of network split).", validator_count); } SimpleList { validators } } diff --git a/ethcore/src/engines/validator_set/test.rs b/ethcore/src/engines/validator_set/test.rs index c66ff14ad4e..6568e4baced 100644 --- a/ethcore/src/engines/validator_set/test.rs +++ b/ethcore/src/engines/validator_set/test.rs @@ -30,6 +30,7 @@ use machine::{AuxiliaryData, Call, EthereumMachine}; use super::{ValidatorSet, SimpleList}; /// Set used for testing with a single validator. +#[derive(Debug)] pub struct TestSet { validator: SimpleList, last_malicious: Arc, @@ -38,18 +39,27 @@ pub struct TestSet { impl Default for TestSet { fn default() -> Self { - TestSet::new(Default::default(), Default::default()) + TestSet::new( + Default::default(), + Default::default(), + vec![Address::from_str("7d577a597b2742b498cb5cf0c26cdcd726d39e6e").unwrap()] + ) } } impl TestSet { - pub fn new(last_malicious: Arc, last_benign: Arc) -> Self { + pub fn new(last_malicious: Arc, last_benign: Arc, validators: Vec
) -> Self { TestSet { - validator: SimpleList::new(vec![Address::from_str("7d577a597b2742b498cb5cf0c26cdcd726d39e6e").unwrap()]), + validator: SimpleList::new(validators), last_malicious, last_benign, } } + + pub fn with_last_benign(mut self, last_benign: Arc) -> Self { + self.last_benign = last_benign; + self + } } impl HeapSizeOf for TestSet { From 67dda77cc4b28145e8272cabe7a0663ac76ab5cb Mon Sep 17 00:00:00 2001 From: David Palm Date: Tue, 11 Jun 2019 15:58:40 +0200 Subject: [PATCH 20/42] Include missing parent hash in MissingParent error --- ethcore/src/engines/authority_round/mod.rs | 2 +- ethcore/src/engines/mod.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index dd16689db6d..24426bf58c2 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -738,7 +738,7 @@ impl AuthorityRound { if !epoch_manager.zoom_to_after(&*client, &self.machine, &*self.validators, *header.parent_hash()) { debug!(target: "engine", "Unable to zoom to epoch."); - return Err(EngineError::MissingParent.into()) + return Err(EngineError::MissingParent(*header.parent_hash()).into()) } (CowLike::Owned(epoch_manager.validators().clone()), epoch_manager.epoch_transition_number) diff --git a/ethcore/src/engines/mod.rs b/ethcore/src/engines/mod.rs index d51e31ea17a..557d9d06fd2 100644 --- a/ethcore/src/engines/mod.rs +++ b/ethcore/src/engines/mod.rs @@ -90,7 +90,7 @@ pub enum EngineError { /// Requires signer ref, but none registered. RequiresSigner, /// Missing Parent Epoch - MissingParent, + MissingParent(H256), /// Checkpoint is missing CliqueMissingCheckpoint(H256), /// Missing vanity data @@ -139,7 +139,7 @@ impl fmt::Display for EngineError { RequiresClient => format!("Call requires client but none registered"), RequiresSigner => format!("Call requires signer but none registered"), InvalidEngine => format!("Invalid engine specification or implementation"), - MissingParent => format!("Parent Epoch is missing from database"), + MissingParent(ref hash) => format!("Parent Epoch is missing from database: {}", hash), }; f.write_fmt(format_args!("Engine error ({})", msg)) From 353e6b967b8faa375c31e5493f65ff7fe989f7f8 Mon Sep 17 00:00:00 2001 From: David Date: Wed, 12 Jun 2019 10:46:41 +0200 Subject: [PATCH 21/42] Update ethcore/src/engines/validator_set/simple_list.rs Co-Authored-By: Luke Schoen --- ethcore/src/engines/validator_set/simple_list.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethcore/src/engines/validator_set/simple_list.rs b/ethcore/src/engines/validator_set/simple_list.rs index dd8f97f091e..44b6df4cf34 100644 --- a/ethcore/src/engines/validator_set/simple_list.rs +++ b/ethcore/src/engines/validator_set/simple_list.rs @@ -38,7 +38,7 @@ impl SimpleList { 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)."); + warn!(target: "engine", "Running AuRa with an even number of validators is not recommended (risk of network split)."); } SimpleList { validators } } From 7db977bd1e347d438ebdd346f16a7f1dddad37a2 Mon Sep 17 00:00:00 2001 From: David Palm Date: Wed, 12 Jun 2019 13:25:36 +0200 Subject: [PATCH 22/42] docs --- ethcore/src/engines/validator_set/safe_contract.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ethcore/src/engines/validator_set/safe_contract.rs b/ethcore/src/engines/validator_set/safe_contract.rs index fc89a97ddba..f1a3920d457 100644 --- a/ethcore/src/engines/validator_set/safe_contract.rs +++ b/ethcore/src/engines/validator_set/safe_contract.rs @@ -74,6 +74,8 @@ impl ::engines::StateDependentProof for StateProof { /// The validator contract should have the following interface: pub struct ValidatorSafeContract { contract_address: Address, + /// The LRU cache is indexed by the parent_hash, so given a hash, the value + /// is the validator set valid for the blocks following that hash. validators: RwLock>, client: RwLock>>, // TODO [keorn]: remove } From cfde65e23f6b5b60b8f0e34fc3945bb5b6a1b32a Mon Sep 17 00:00:00 2001 From: David Palm Date: Wed, 12 Jun 2019 13:27:54 +0200 Subject: [PATCH 23/42] remove unneeded into() Move check for parent_step == step for clarity&efficiency Remove dead code for Seal::Proposal --- ethcore/src/engines/authority_round/mod.rs | 26 ++++++++++------------ ethcore/src/engines/mod.rs | 2 -- ethcore/src/miner/miner.rs | 26 ++-------------------- 3 files changed, 14 insertions(+), 40 deletions(-) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index 24426bf58c2..d51524fae7e 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -1114,12 +1114,12 @@ impl Engine for AuthorityRound { // filter messages from old and future steps and different parents let empty_steps = if header.number() >= self.empty_steps_transition { - self.empty_steps(parent_step.into(), step.into(), *header.parent_hash()) + self.empty_steps(parent_step, step, *header.parent_hash()) } else { Vec::new() }; - let expected_diff = calculate_score(parent_step, step.into(), empty_steps.len().into()); + let expected_diff = calculate_score(parent_step, step, empty_steps.len()); if header.difficulty() != &expected_diff { debug!(target: "engine", "Aborting seal generation. The step or empty_steps have changed in the meantime. {:?} != {:?}", @@ -1127,12 +1127,17 @@ impl Engine for AuthorityRound { return Seal::None; } - if parent_step > step.into() { + if parent_step > step { warn!(target: "engine", "Aborting seal generation for invalid step: {} > {}", parent_step, step); return Seal::None; + } else if parent_step == step { + // this is guarded against by `can_propose` unless the block was signed + // on the same step (implies same key) and on a different node. + warn!("Attempted to seal block on the same step as parent. Is this authority sealing with more than one node?"); + return Seal::None; } - let (validators, set_number) = match self.epoch_set(header) { + let (validators, epoch_transition_number) = match self.epoch_set(header) { Err(err) => { warn!(target: "engine", "Unable to generate seal: {}", err); return Seal::None; @@ -1141,13 +1146,6 @@ impl Engine for AuthorityRound { }; if is_step_proposer(&*validators, header.parent_hash(), step, header.author()) { - // this is guarded against by `can_propose` unless the block was signed - // on the same step (implies same key) and on a different node. - if parent_step == step { - warn!("Attempted to seal block on the same step as parent. Is this authority sealing with more than one node?"); - return Seal::None; - } - // if there are no transactions to include in the block, we don't seal and instead broadcast a signed // `EmptyStep(step, parent_hash)` message. If we exceed the maximum amount of `empty_step` rounds we proceed // with the seal. @@ -1182,7 +1180,7 @@ impl Engine for AuthorityRound { // report any skipped primaries between the parent block and // the block we're sealing, unless we have empty steps enabled if header.number() < self.empty_steps_transition { - self.report_skipped(header, step, parent_step, &*validators, set_number); + self.report_skipped(header, step, parent_step, &*validators, epoch_transition_number); } let mut fields = vec![ @@ -1758,13 +1756,13 @@ mod tests { engine.set_signer(Box::new((tap.clone(), addr1, "1".into()))); match engine.generate_seal(&b1, &genesis_header) { - Seal::None | Seal::Proposal(_) => panic!("wrong seal"), + Seal::None => panic!("wrong seal"), Seal::Regular(_) => { engine.step(); engine.set_signer(Box::new((tap.clone(), addr2, "0".into()))); match engine.generate_seal(&b2, &genesis_header) { - Seal::Regular(_) | Seal::Proposal(_) => panic!("sealed despite wrong difficulty"), + Seal::Regular(_) => panic!("sealed despite wrong difficulty"), Seal::None => {} } } diff --git a/ethcore/src/engines/mod.rs b/ethcore/src/engines/mod.rs index 557d9d06fd2..6d4da28fb15 100644 --- a/ethcore/src/engines/mod.rs +++ b/ethcore/src/engines/mod.rs @@ -155,8 +155,6 @@ impl error::Error for EngineError { /// Seal type. #[derive(Debug, PartialEq, Eq)] pub enum Seal { - /// Proposal seal; should be broadcasted, but not inserted into blockchain. - Proposal(Vec), /// Regular block seal; should be part of the blockchain. Regular(Vec), /// Engine does not generate seal for this block right now. diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index 67ef9e6181c..83ce1bd4ae3 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -654,8 +654,8 @@ impl Miner { // TODO: (https://github.com/paritytech/parity-ethereum/issues/10407) // This is only used in authority_round path, and should be refactored to merge with the other seal() path. - // Attempts to perform internal sealing (one that does not require work) and handles the result depending on the - // type of Seal. + // Attempts to perform internal sealing (one that does not require work: e.g. Clique + // and Aura) and handles the result depending on the type of Seal. fn seal_and_import_block_internally(&self, chain: &C, block: ClosedBlock) -> bool where C: BlockChain + SealedBlockImporter, { @@ -682,28 +682,6 @@ impl Miner { }; match self.engine.generate_seal(&block, &parent_header) { - // Save proposal for later seal submission and broadcast it. - Seal::Proposal(seal) => { - trace!(target: "miner", "Received a Proposal seal."); - { - let mut sealing = self.sealing.lock(); - sealing.next_mandatory_reseal = Instant::now() + self.options.reseal_max_period; - sealing.queue.set_pending(block.clone()); - sealing.queue.use_last_ref(); - } - - block - .lock() - .seal(&*self.engine, seal) - .map(|sealed| { - chain.broadcast_proposal_block(sealed); - true - }) - .unwrap_or_else(|e| { - warn!("ERROR: seal failed when given internally generated seal: {}", e); - false - }) - }, // Directly import a regular sealed block. Seal::Regular(seal) => { trace!(target: "miner", "Received a Regular seal."); From 54cf92f6cd729701dbeba384751e758b4981cca5 Mon Sep 17 00:00:00 2001 From: David Palm Date: Wed, 12 Jun 2019 13:29:38 +0200 Subject: [PATCH 24/42] typo --- ethcore/src/block.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethcore/src/block.rs b/ethcore/src/block.rs index fcecc53ab88..1b2231e3f0c 100644 --- a/ethcore/src/block.rs +++ b/ethcore/src/block.rs @@ -102,7 +102,7 @@ pub struct ExecutedBlock { pub receipts: Vec, /// Hashes of already executed transactions. pub transactions_set: HashSet, - /// Underlaying state. + /// Underlying state. pub state: State, /// Transaction traces. pub traces: Tracing, From f807e384db26d27121ed5424433ed4929c47a2c2 Mon Sep 17 00:00:00 2001 From: David Palm Date: Wed, 12 Jun 2019 13:30:21 +0200 Subject: [PATCH 25/42] Wording --- ethcore/src/state/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index d0d57287dce..a6298807c67 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -303,7 +303,7 @@ pub fn prove_transaction_virtual + Send + Syn /// Reverting a checkpoint with `revert_to_checkpoint` involves copying /// original values from the latest checkpoint back into `cache`. The code /// takes care not to overwrite cached storage while doing that. -/// checkpoint can be discarded with `discard_checkpoint`. All of the orignal +/// A checkpoint can be discarded with `discard_checkpoint`. All of the original /// backed-up values are moved into a parent checkpoint (if any). /// pub struct State { From 5f8c368ade4f64912db0d5b0f0a41a6f63e5e53a Mon Sep 17 00:00:00 2001 From: David Palm Date: Wed, 19 Jun 2019 14:16:26 +0200 Subject: [PATCH 26/42] naming --- ethcore/src/client/client.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 75227af5aa4..7e71db413c1 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -2417,11 +2417,11 @@ impl ImportSealedBlock for Client { let _import_lock = self.importer.import_lock.lock(); trace_time!("import_sealed_block"); - let block_data = block.rlp_bytes(); + let block_bytes = block.rlp_bytes(); let pending = self.importer.check_epoch_end_signal( &header, - &block_data, + &block_bytes, &block.receipts, block.state.db(), self @@ -2429,7 +2429,7 @@ impl ImportSealedBlock for Client { let route = self.importer.commit_block( block, &header, - encoded::Block::new(block_data), + encoded::Block::new(block_bytes), pending, self ); From 7105f19923e1eb512ffd64fb24833e356113584d Mon Sep 17 00:00:00 2001 From: David Palm Date: Thu, 20 Jun 2019 10:23:36 +0200 Subject: [PATCH 27/42] WIP --- ethcore/src/block.rs | 5 +- ethcore/src/engines/validator_set/contract.rs | 1 + ethcore/src/miner/miner.rs | 96 +++++++++++++------ miner/using-queue/src/lib.rs | 6 +- 4 files changed, 71 insertions(+), 37 deletions(-) diff --git a/ethcore/src/block.rs b/ethcore/src/block.rs index 6f1889dbd19..5f1aced5fe5 100644 --- a/ethcore/src/block.rs +++ b/ethcore/src/block.rs @@ -378,10 +378,7 @@ impl ClosedBlock { // revert rewards (i.e. set state back at last transaction's state). let mut block = self.block; block.state = self.unclosed_state; - OpenBlock { - block: block, - engine: engine, - } + OpenBlock { block, engine } } } diff --git a/ethcore/src/engines/validator_set/contract.rs b/ethcore/src/engines/validator_set/contract.rs index 5560447c447..0afd19fec6e 100644 --- a/ethcore/src/engines/validator_set/contract.rs +++ b/ethcore/src/engines/validator_set/contract.rs @@ -162,6 +162,7 @@ mod tests { #[test] fn reports_validators() { + let _ = ::env_logger::try_init(); let tap = Arc::new(AccountProvider::transient_provider()); let v1 = tap.insert_account(keccak("1").into(), &"".into()).unwrap(); let client = generate_dummy_client_with_spec(Spec::new_validator_contract); diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index e339c86a698..29af2de13ec 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -145,7 +145,7 @@ pub struct MinerOptions { pub tx_queue_strategy: PrioritizationStrategy, /// Simple senders penalization. pub tx_queue_penalization: Penalization, - /// Do we want to mark transactions recieved locally (e.g. RPC) as local if we don't have the sending account? + /// Do we want to mark transactions received locally (e.g. RPC) as local if we don't have the sending account? pub tx_queue_no_unfamiliar_locals: bool, /// Do we refuse to accept service transactions even if sender is certified. pub refuse_service_transactions: bool, @@ -230,6 +230,10 @@ impl SealingWork { fn reseal_allowed(&self) -> bool { Instant::now() > self.next_allowed_reseal } + + fn work_available(&self) -> bool { + self.queue.peek_last_ref().is_some() + } } /// Keeps track of transactions using priority queue and holds currently mined block. @@ -619,6 +623,7 @@ impl Miner { trace!(target: "miner", "requires_reseal: sealing enabled"); + const SEALING_TIMEOUT_IN_BLOCKS : u64 = 5; // Disable sealing if there were no requests for SEALING_TIMEOUT_IN_BLOCKS let had_requests = sealing.last_request.map(|last_request| best_block.saturating_sub(last_request) <= SEALING_TIMEOUT_IN_BLOCKS @@ -675,12 +680,30 @@ impl Miner { Some(h) => { match h.decode() { Ok(decoded_hdr) => decoded_hdr, - Err(_) => return false + Err(e) => { + error!(target: "miner", "seal_block_internally: Could not decode header from parent block (hash={}): {:?}", block.header.parent_hash(), e); + return false + } } } - None => return false, + None => { + trace!(target: "miner", "Parent with hash={} does not exist in our DB", block.header.parent_hash()); + return false + }, }; + // TODO: not the right way to do this. + if let Some(existing_block) = chain.block(BlockId::Number(block.header.number())) { + let existing_block_header = existing_block.decode_header(); + warn!(target: "miner", "seal_block_internally: block {} already exists in the DB with parent_hash={} (timestamp={}, author={})", + block.header.number(), + existing_block_header.parent_hash() + existing_block_header.timestamp(), + existing_block_header.author(), + ); + return false + } + match self.engine.generate_seal(&block, &parent_header) { // Directly import a regular sealed block. Seal::Regular(seal) => { @@ -770,28 +793,32 @@ impl Miner { } /// Prepare a pending block. Returns the preparation status. - fn prepare_pending_block(&self, client: &C) -> BlockPreparationStatus where - C: BlockChain + CallContract + BlockProducer + SealedBlockImporter + Nonce + Sync, + /// Only used by externally sealing engines. + fn prepare_pending_block(&self, client: &C) -> BlockPreparationStatus + where + C: BlockChain + CallContract + BlockProducer + SealedBlockImporter + Nonce + Sync, { trace!(target: "miner", "prepare_pending_block: entering"); - let prepare_new = { - let mut sealing = self.sealing.lock(); - let have_work = sealing.queue.peek_last_ref().is_some(); - trace!(target: "miner", "prepare_pending_block: have_work={}", have_work); - if !have_work { - sealing.enabled = true; - true - } else { - false - } - }; - + // Unless we are `--force-sealing` we create pending blocks if + // 1. we have local pending transactions + // 2. or someone is requesting `eth_getWork` + // When either condition is true, `sealing.enabled` is flipped to true (and if you + // have no more local transactions or stop calling `eth_getWork`, it is set to `false`). + + // Here we check if there are pending blocks already (if so, we don't need to create + // a new one); if there are none, we set `sealing.enabled` to true because the + // calling code expects it to be on (or they wouldn't have called this method). + // Yes, it's a bit convoluted. + let prepare_new_block = self.maybe_enable_sealing(); + + // TODO: Callers check for this already. Ok to remove? if self.engine.sealing_state() != SealingState::External { trace!(target: "miner", "prepare_pending_block: engine not sealing externally; not preparing"); return BlockPreparationStatus::NotPrepared; } - let preparation_status = if prepare_new { + trace!(target: "miner", "prepare_pending_block: entering"); + let preparation_status = if prepare_new_block { // -------------------------------------------------------------------------- // | NOTE Code below requires sealing locks. | // | Make sure to release the locks before calling that method. | @@ -821,25 +848,34 @@ impl Miner { preparation_status } + + /// Set `sealing.enabled` to true if there is available work to do (pending or in the queue). + fn maybe_enable_sealing(&self) -> bool { + let mut sealing = self.sealing.lock(); + if !sealing.work_available() { + trace!(target: "miner", "maybe_enable_sealing: we have work to do so enabling sealing"); + sealing.enabled = true; + true + } else { + false + } + } /// Prepare pending block, check whether sealing is needed, and then update sealing. fn prepare_and_update_sealing(&self, chain: &C) { use miner::MinerService; - - // Make sure to do it after transaction is imported and lock is dropped. - // We need to create pending block and enable sealing. - let sealing_state = self.engine.sealing_state(); - if sealing_state == SealingState::Ready - || self.prepare_pending_block(chain) == BlockPreparationStatus::NotPrepared { - // If new block has not been prepared (means we already had one) - // or Engine might be able to seal internally, - // we need to update sealing. - self.update_sealing(chain); + match self.engine.sealing_state() { + SealingState::Ready => self.update_sealing(chain), + SealingState::External => { + // this calls `maybe_enable_sealing()` + if self.prepare_pending_block(chain) == BlockPreparationStatus::NotPrepared { + self.update_sealing(chain) + } + } + SealingState::NotReady => { self.maybe_enable_sealing(); }, } } } -const SEALING_TIMEOUT_IN_BLOCKS : u64 = 5; - impl miner::MinerService for Miner { type State = State<::state_db::StateDB>; diff --git a/miner/using-queue/src/lib.rs b/miner/using-queue/src/lib.rs index 56e99879db7..b0bc70e851d 100644 --- a/miner/using-queue/src/lib.rs +++ b/miner/using-queue/src/lib.rs @@ -14,9 +14,9 @@ // You should have received a copy of the GNU General Public License // along with Parity Ethereum. If not, see . -//! Queue-like datastructure including notion of usage. +//! Queue-like data structure including notion of usage. -/// Special queue-like datastructure that includes the notion of +/// Special queue-like data structure that includes the notion of /// usage to avoid items that were queued but never used from making it into /// the queue. pub struct UsingQueue { @@ -42,7 +42,7 @@ impl UsingQueue { UsingQueue { pending: None, in_use: vec![], - max_size: max_size, + max_size, } } From 1a304d38c0b9c3e6baec44e6e81ae8363bdad426 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 24 Jun 2019 15:04:28 +0200 Subject: [PATCH 28/42] cleanup --- ethcore/src/engines/authority_round/mod.rs | 44 ++++++++++------------ ethcore/src/engines/validator_set/test.rs | 1 + 2 files changed, 21 insertions(+), 24 deletions(-) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index e3a17daef6c..4fb4f551613 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -1878,13 +1878,11 @@ mod tests { assert!(aura.verify_block_family(&header, &parent_header).is_ok()); assert_eq!(last_benign.load(AtomicOrdering::SeqCst), 0); - aura.set_signer(Box::new( - ( - Arc::new(AccountProvider::transient_provider()), - validator2, - "".into() - ) - )); + aura.set_signer(Box::new(( + Arc::new(AccountProvider::transient_provider()), + validator2, + "".into(), + ))); // Do not report on steps skipped between genesis and first block. header.set_number(1); @@ -1906,13 +1904,11 @@ mod tests { let validator_set = TestSet::default().with_last_benign(last_benign.clone()); p.validators = Box::new(validator_set); }); - aura.set_signer(Box::new( - ( - Arc::new(AccountProvider::transient_provider()), - Address::from_low_u64_be(123), - "".into() - ) - )); + aura.set_signer(Box::new(( + Arc::new(AccountProvider::transient_provider()), + Address::from_low_u64_be(123), + "".into() + ))); let mut parent_header: Header = Header::default(); parent_header.set_seal(vec![encode(&1usize)]); parent_header.set_gas_limit("222222".parse::().unwrap()); @@ -1943,16 +1939,16 @@ mod tests { assert_eq!(aura.maximum_uncle_count(100), 0); } - #[test] - #[should_panic(expected="counter is too high")] - fn test_counter_increment_too_high() { - use super::Step; - let step = Step { - calibrate: false, - inner: AtomicUsize::new(::std::usize::MAX), - duration: 1, - }; - step.increment(); + #[test] + #[should_panic(expected="counter is too high")] + fn test_counter_increment_too_high() { + use super::Step; + let step = Step { + calibrate: false, + inner: AtomicUsize::new(::std::usize::MAX), + duration: 1, + }; + step.increment(); } #[test] diff --git a/ethcore/src/engines/validator_set/test.rs b/ethcore/src/engines/validator_set/test.rs index 5c13c512294..3ddf1776d16 100644 --- a/ethcore/src/engines/validator_set/test.rs +++ b/ethcore/src/engines/validator_set/test.rs @@ -58,6 +58,7 @@ impl TestSet { } } + /// Build a `TestSet` with the given number of benign misbehaviour reports pub fn with_last_benign(mut self, last_benign: Arc) -> Self { self.last_benign = last_benign; self From 43b76e790cd73241921addd66fd08ca6f642bdd5 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 24 Jun 2019 16:34:57 +0200 Subject: [PATCH 29/42] cosmetics --- ethcore/blockchain/src/blockchain.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ethcore/blockchain/src/blockchain.rs b/ethcore/blockchain/src/blockchain.rs index bc500f3355a..110872db721 100644 --- a/ethcore/blockchain/src/blockchain.rs +++ b/ethcore/blockchain/src/blockchain.rs @@ -758,8 +758,8 @@ impl BlockChain { Some(TreeRoute { blocks: from_branch, ancestor: current_from, - index: index, - is_from_route_finalized: is_from_route_finalized, + index, + is_from_route_finalized, }) } From 2c3975a20b5704cba75cd2fa6148308d8934c3cb Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 24 Jun 2019 16:35:09 +0200 Subject: [PATCH 30/42] cosmetics and one less lvar --- ethcore/src/client/client.rs | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 7e71db413c1..5a1f20613a2 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -475,7 +475,16 @@ impl Importer { // // The header passed is from the original block data and is sealed. // TODO: should return an error if ImportRoute is none, issue #9910 - fn commit_block(&self, block: B, header: &Header, block_data: encoded::Block, pending: Option, client: &Client) -> ImportRoute where B: Drain { + fn commit_block( + &self, + block: B, + header: &Header, + block_data: encoded::Block, + pending: Option, + client: &Client + ) -> ImportRoute + where B: Drain + { let hash = &header.hash(); let number = header.number(); let parent = header.parent_hash(); @@ -501,18 +510,17 @@ impl Importer { }; let best = { - let hash = best_hash; - let header = chain.block_header_data(&hash) + let header = chain.block_header_data(&best_hash) .expect("Best block is in the database; qed") .decode() .expect("Stored block header is valid RLP; qed"); - let details = chain.block_details(&hash) + let details = chain.block_details(&best_hash) .expect("Best block is in the database; qed"); ExtendedHeader { parent_total_difficulty: details.total_difficulty - *header.difficulty(), is_finalized: details.is_finalized, - header: header, + header, } }; @@ -550,7 +558,7 @@ impl Importer { }).collect(); let route = chain.insert_block(&mut batch, block_data, receipts.clone(), ExtrasInsert { - fork_choice: fork_choice, + fork_choice, is_finalized, }); From 72db50130e658a65ca3bce7fcb27494e0dd439a1 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 24 Jun 2019 16:35:52 +0200 Subject: [PATCH 31/42] spelling --- util/journaldb/src/traits.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/journaldb/src/traits.rs b/util/journaldb/src/traits.rs index e6ce8acb040..3788609155b 100644 --- a/util/journaldb/src/traits.rs +++ b/util/journaldb/src/traits.rs @@ -64,7 +64,7 @@ pub trait JournalDB: KeyedHashDB { fn latest_era(&self) -> Option; /// Journal recent database operations as being associated with a given era and id. - // TODO: give the overlay to this function so journaldbs don't manage the overlays themeselves. + // TODO: give the overlay to this function so journaldbs don't manage the overlays themselves. fn journal_under(&mut self, batch: &mut DBTransaction, now: u64, id: &H256) -> io::Result; /// Mark a given block as canonical, indicating that competing blocks' states may be pruned out. From 8b2bf8f98181d0d55a95d4c68989d757cb14097d Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 24 Jun 2019 16:36:06 +0200 Subject: [PATCH 32/42] Better loggin when a block is already in chain --- ethcore/sync/src/block_sync.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/ethcore/sync/src/block_sync.rs b/ethcore/sync/src/block_sync.rs index 490bd39a809..7bce02a572d 100644 --- a/ethcore/sync/src/block_sync.rs +++ b/ethcore/sync/src/block_sync.rs @@ -552,12 +552,18 @@ impl BlockDownloader { let result = if let Some(receipts) = receipts { io.chain().queue_ancient_block(block, receipts) } else { + trace_sync!(self, "Importing block #{}/{}", number, h); io.chain().import_block(block) }; match result { Err(EthcoreError::Import(ImportError::AlreadyInChain)) => { - trace_sync!(self, "Block already in chain {:?}", h); + let is_canonical = if io.chain().block_hash(BlockId::Number(number)).is_some() { + "canoncial" + } else { + "not canonical" + }; + trace_sync!(self, "Block #{} is already in chain {:?} – {}", number, h, is_canonical); self.block_imported(&h, number, &parent); }, Err(EthcoreError::Import(ImportError::AlreadyQueued)) => { From 2df7b73e3383a44cacb991f0bf670fb781656132 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 24 Jun 2019 16:39:36 +0200 Subject: [PATCH 33/42] More logging --- ethcore/src/engines/authority_round/mod.rs | 5 +++-- ethcore/src/engines/validator_set/safe_contract.rs | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index f2f9e584a2e..aa3cffe19da 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -249,7 +249,7 @@ impl EpochManager { None => { // this really should never happen unless the block passed // hasn't got a parent in the database. - warn!(target: "engine", "No genesis transition found."); + warn!(target: "engine", "No genesis transition found. Block hash {} does not have a parent in the DB", hash); return false; } }; @@ -737,7 +737,8 @@ impl AuthorityRound { }; if !epoch_manager.zoom_to_after(&*client, &self.machine, &*self.validators, *header.parent_hash()) { - debug!(target: "engine", "Unable to zoom to epoch."); + debug!(target: "engine", "Unable to zoom to epoch - block=#{} parent_hash={} epoch_transistion_number={}", + header.number(), header.parent_hash(), epoch_manager.epoch_transition_number); return Err(EngineError::MissingParent(*header.parent_hash()).into()) } diff --git a/ethcore/src/engines/validator_set/safe_contract.rs b/ethcore/src/engines/validator_set/safe_contract.rs index 154384ffbe8..6d9b5d875c3 100644 --- a/ethcore/src/engines/validator_set/safe_contract.rs +++ b/ethcore/src/engines/validator_set/safe_contract.rs @@ -473,6 +473,7 @@ mod tests { #[test] fn knows_validators() { + let _ = env_logger::try_init(); let tap = Arc::new(AccountProvider::transient_provider()); let s0: Secret = keccak("1").into(); let v0 = tap.insert_account(s0.clone(), &"".into()).unwrap(); From 1a0f82b74ea06feca3ab3d45084fba135a0f96e2 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 24 Jun 2019 20:47:03 +0200 Subject: [PATCH 34/42] On second thought non-validators are allowed to report --- ethcore/src/engines/authority_round/mod.rs | 99 ++++++---------------- 1 file changed, 28 insertions(+), 71 deletions(-) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index 4fb4f551613..3d43a2e5480 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -813,24 +813,22 @@ impl AuthorityRound { return; } - if let (true, me) = (current_step > parent_step + 1, self.address()) { + if let (true, Some(me)) = (current_step > parent_step + 1, self.address()) { debug!(target: "engine", "Author {} built block with step gap. current step: {}, parent step: {}", header.author(), current_step, parent_step); - if validators.contains(header.parent_hash(), &me) { - 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 { - trace!(target: "engine", "Primary that skipped is self, not self-reporting. Own address: {}", me); - } + 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; } + trace!(target: "engine", "Reporting benign misbehaviour (cause: skipped step) at block #{}, epoch set number {}. Own address: {}", + header.number(), set_number, me); + self.validators.report_benign(&skipped_primary, set_number, header.number()); + } else { + trace!(target: "engine", "Primary that skipped is self, not self-reporting. Own address: {}", me); } - } else { - warn!(target: "engine", "We are not part of the validator set so not reporting (skipped steps). Own address: {}", self.address()); } } } @@ -898,8 +896,8 @@ impl AuthorityRound { finalized.unwrap_or_default() } - fn address(&self) -> Address { - self.signer.read().as_ref().map_or_else(|| Address::zero(), |s| s.address()) + fn address(&self) -> Option
{ + self.signer.read().as_ref().map(|s| s.address() ) } } @@ -1300,19 +1298,17 @@ impl Engine for AuthorityRound { // This check runs in Phase 1 where there is no guarantee that the parent block is // already imported, therefore the call to `epoch_set` may fail. In that case we // won't report the misbehavior but this is not a concern because: - // - Only authorities can report and it's expected that they'll be up-to-date and - // importing, therefore the parent header will most likely be available + // - Authorities will have a signing key available to report and it's expected that + // they'll be up-to-date and importing, therefore the parent header will most likely + // be available // - Even if you are an authority that is syncing the chain, the contract will most // likely ignore old reports // - This specific check is only relevant if you're importing (since it checks // against wall clock) - 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); - } + if let Ok((_, set_number)) = self.epoch_set(header) { + trace!(target: "engine", "Reporting benign misbehaviour (cause: InvalidSeal) at block #{}, epoch set number {}. Own address: {}", + header.number(), set_number, self.address().unwrap_or_default()); + self.validators.report_benign(header.author(), set_number, header.number()); } Err(BlockError::InvalidSeal.into()) @@ -1382,12 +1378,8 @@ impl Engine for AuthorityRound { match validate_empty_steps() { Ok(len) => len, Err(err) => { - if self.validators.contains(header.parent_hash(), &self.address()) { - self.validators.report_benign(header.author(), set_number, header.number()); - } else { - warn!(target: "engine", "Not reporting benign misbehaviour (cause: invalid empty steps) because we're not a validator. Own address: {}", - self.address()); - } + trace!(target: "engine", "Reporting benign misbehaviour (cause: invalid empty steps) at block #{}, epoch set number {}. Own address: {}", header.number(), set_number, self.address().unwrap_or_default()); + self.validators.report_benign(header.author(), set_number, header.number()); return Err(err); }, } @@ -1416,11 +1408,9 @@ impl Engine for AuthorityRound { let res = verify_external(header, &*validators, self.empty_steps_transition); match res { Err(Error::Engine(EngineError::NotProposer(_))) => { - if self.validators.contains(header.parent_hash(), &self.address()) { - self.validators.report_benign(header.author(), set_number, header.number()); - } else { - warn!(target: "engine", "Not reporting benign misbehaviour (cause:block from incorrect proposer) because we're not a validator. Own address: {}", self.address()); - } + trace!(target: "engine", "Reporting benign misbehaviour (cause: block from incorrect proposer) at block #{}, epoch set number {}. Own address: {}", + header.number(), set_number, self.address().unwrap_or_default()); + self.validators.report_benign(header.author(), set_number, header.number()); }, Ok(_) => { // we can drop all accumulated empty step messages that are older than this header's step @@ -1893,40 +1883,7 @@ mod tests { header.set_number(2); assert!(aura.verify_block_family(&header, &parent_header).is_ok()); assert_eq!(last_benign.load(AtomicOrdering::SeqCst), 2); - } - - #[test] - fn skips_benign_reporting_unless_validator() { - let _ = ::env_logger::try_init(); - - let last_benign = Arc::new(AtomicUsize::new(0)); - let aura = build_aura(|p| { - let validator_set = TestSet::default().with_last_benign(last_benign.clone()); - p.validators = Box::new(validator_set); - }); - aura.set_signer(Box::new(( - Arc::new(AccountProvider::transient_provider()), - Address::from_low_u64_be(123), - "".into() - ))); - let mut parent_header: Header = Header::default(); - parent_header.set_seal(vec![encode(&1usize)]); - parent_header.set_gas_limit("222222".parse::().unwrap()); - let mut header: Header = Header::default(); - header.set_difficulty(calculate_score(1, 3, 0)); - header.set_gas_limit("222222".parse::().unwrap()); - header.set_seal(vec![encode(&3usize)]); - - header.set_number(1); - aura.verify_block_family(&header, &parent_header).unwrap(); - header.set_number(2); - aura.verify_block_family(&header, &parent_header).unwrap(); - header.set_number(3); - aura.verify_block_family(&header, &parent_header).unwrap(); - - // No reporting happened: we're not a validator - assert_eq!(last_benign.load(AtomicOrdering::SeqCst), 0); - } + }if self.validators.contains(header.parent_hash() #[test] fn test_uncles_transition() { From 05df05300f32a5c94b7469dc74e2472c251cb054 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 24 Jun 2019 21:05:17 +0200 Subject: [PATCH 35/42] cleanup --- ethcore/src/engines/authority_round/mod.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index 3d43a2e5480..44c10b4c2a0 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -1378,7 +1378,8 @@ impl Engine for AuthorityRound { match validate_empty_steps() { Ok(len) => len, Err(err) => { - trace!(target: "engine", "Reporting benign misbehaviour (cause: invalid empty steps) at block #{}, epoch set number {}. Own address: {}", header.number(), set_number, self.address().unwrap_or_default()); + trace!(target: "engine", "Reporting benign misbehaviour (cause: invalid empty steps) at block #{}, epoch set number {}. Own address: {}", + header.number(), set_number, self.address().unwrap_or_default()); self.validators.report_benign(header.author(), set_number, header.number()); return Err(err); }, @@ -1883,7 +1884,7 @@ mod tests { header.set_number(2); assert!(aura.verify_block_family(&header, &parent_header).is_ok()); assert_eq!(last_benign.load(AtomicOrdering::SeqCst), 2); - }if self.validators.contains(header.parent_hash() + } #[test] fn test_uncles_transition() { From 9080630f49f80553996f1eb2412f21514cc1c8f3 Mon Sep 17 00:00:00 2001 From: David Palm Date: Tue, 25 Jun 2019 10:03:11 +0200 Subject: [PATCH 36/42] remove dead code --- ethcore/src/engines/validator_set/test.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/ethcore/src/engines/validator_set/test.rs b/ethcore/src/engines/validator_set/test.rs index 3ddf1776d16..b82bcfe1240 100644 --- a/ethcore/src/engines/validator_set/test.rs +++ b/ethcore/src/engines/validator_set/test.rs @@ -57,12 +57,6 @@ impl TestSet { last_benign, } } - - /// Build a `TestSet` with the given number of benign misbehaviour reports - pub fn with_last_benign(mut self, last_benign: Arc) -> Self { - self.last_benign = last_benign; - self - } } impl ValidatorSet for TestSet { From c5bc98aab186b3202d4e1725be6c845ff7ef8c55 Mon Sep 17 00:00:00 2001 From: David Palm Date: Wed, 26 Jun 2019 10:54:56 +0200 Subject: [PATCH 37/42] Keep track of the hash of the last imported block --- ethcore/src/miner/miner.rs | 43 +++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index 29af2de13ec..1e717c221f0 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -253,6 +253,7 @@ pub struct Miner { accounts: Arc, io_channel: RwLock>>, service_transaction_checker: Option, + last_parent_hash: RwLock, } impl Miner { @@ -314,6 +315,7 @@ impl Miner { } else { Some(ServiceTransactionChecker::default()) }, + last_parent_hash: RwLock::new(H256::zero()) } } @@ -674,34 +676,37 @@ impl Miner { } } - trace!(target: "miner", "seal_block_internally: attempting internal seal."); + trace!(target: "miner", "seal_block_internally: attempting internal seal for block #{}", block.header.number()); let parent_header = match chain.block_header(BlockId::Hash(*block.header.parent_hash())) { Some(h) => { match h.decode() { Ok(decoded_hdr) => decoded_hdr, Err(e) => { - error!(target: "miner", "seal_block_internally: Could not decode header from parent block (hash={}): {:?}", block.header.parent_hash(), e); + error!(target: "miner", "seal_block_internally: Block #{}, Could not decode header from parent block (hash={}): {:?}", block.header.number(), block.header.parent_hash(), e); return false } } } None => { - trace!(target: "miner", "Parent with hash={} does not exist in our DB", block.header.parent_hash()); + trace!(target: "miner", "Block #{}: Parent with hash={} does not exist in our DB", block.header.number(), block.header.parent_hash()); return false }, }; - // TODO: not the right way to do this. - if let Some(existing_block) = chain.block(BlockId::Number(block.header.number())) { - let existing_block_header = existing_block.decode_header(); - warn!(target: "miner", "seal_block_internally: block {} already exists in the DB with parent_hash={} (timestamp={}, author={})", - block.header.number(), - existing_block_header.parent_hash() - existing_block_header.timestamp(), - existing_block_header.author(), - ); - return false + { + let last_parent_hash = self.last_parent_hash.read(); + if parent_header.hash() == *last_parent_hash { + warn!(target: "miner", "Trying to build block #{} on same parent hash. New block's parent hash: {}, last parent hash: {}", + block.header.number(), parent_header.hash(), *last_parent_hash + ); + return false + } + } + { + let mut last_parent_hash = self.last_parent_hash.write(); + trace!(target: "miner", "Block #{}, Setting last parent hash. was: {}, becomes: {}", block.header.number(), *last_parent_hash, block.header.hash()); + *last_parent_hash = block.header.hash(); } match self.engine.generate_seal(&block, &parent_header) { @@ -1656,12 +1661,16 @@ mod tests { #[test] fn internal_seals_without_work() { + let _ = env_logger::try_init(); let spec = Spec::new_instant(); let miner = Miner::new_for_tests(&spec, None); let client = generate_dummy_client(2); - let import = miner.import_external_transactions(&*client, vec![transaction_with_chain_id(spec.chain_id()).into()]).pop().unwrap(); + let import = miner.import_external_transactions( + &*client, + vec![transaction_with_chain_id(spec.chain_id()).into()] + ).pop().unwrap(); assert_eq!(import.unwrap(), ()); miner.update_sealing(&*client); @@ -1669,11 +1678,15 @@ mod tests { assert!(miner.pending_block(0).is_none()); assert_eq!(client.chain_info().best_block_number, 3 as BlockNumber); - assert!(miner.import_own_transaction(&*client, PendingTransaction::new(transaction_with_chain_id(spec.chain_id()).into(), None)).is_ok()); + assert!(miner.import_own_transaction( + &*client, + PendingTransaction::new(transaction_with_chain_id(spec.chain_id()).into(), None) + ).is_ok()); miner.update_sealing(&*client); client.flush_queue(); assert!(miner.pending_block(0).is_none()); + println!("NOW"); assert_eq!(client.chain_info().best_block_number, 4 as BlockNumber); } From 6e72b5187a3b8d5a503ca3292f992e90238c1bfa Mon Sep 17 00:00:00 2001 From: David Palm Date: Wed, 26 Jun 2019 15:41:25 +0200 Subject: [PATCH 38/42] Let it lock --- ethcore/src/miner/miner.rs | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index 1e717c221f0..ab3b675191a 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -694,25 +694,26 @@ impl Miner { }, }; - { - let last_parent_hash = self.last_parent_hash.read(); - if parent_header.hash() == *last_parent_hash { - warn!(target: "miner", "Trying to build block #{} on same parent hash. New block's parent hash: {}, last parent hash: {}", - block.header.number(), parent_header.hash(), *last_parent_hash - ); - return false - } - } - { +// { +// let last_parent_hash = self.last_parent_hash.read(); +// if parent_header.hash() == *last_parent_hash { +// warn!(target: "miner", "Trying to build block #{} on same parent hash. New block's parent hash: {}, last parent hash: {}", +// block.header.number(), parent_header.hash(), *last_parent_hash +// ); +// return false +// } +// } +// { let mut last_parent_hash = self.last_parent_hash.write(); trace!(target: "miner", "Block #{}, Setting last parent hash. was: {}, becomes: {}", block.header.number(), *last_parent_hash, block.header.hash()); *last_parent_hash = block.header.hash(); - } +// } + let res = match self.engine.generate_seal(&block, &parent_header) { // Directly import a regular sealed block. Seal::Regular(seal) => { - trace!(target: "miner", "Received a Regular seal."); + trace!(target: "miner", "Block #{}: Received a Regular seal.", block.header.number()); { let mut sealing = self.sealing.lock(); sealing.next_mandatory_reseal = Instant::now() + self.options.reseal_max_period; @@ -725,12 +726,14 @@ impl Miner { chain.import_sealed_block(sealed).is_ok() }) .unwrap_or_else(|e| { - warn!("ERROR: seal failed when given internally generated seal: {}", e); + warn!("ERROR: Block #{}, importing sealed block failed when given internally generated seal: {}", block.header.number(), e); false }) }, Seal::None => false, - } + }; + trace!(target:"miner", "Block: #{}, seal_and_import_block_internally: done, returning {}", block.header.number(), res); + res } /// Prepares work which has to be done to seal. From 7f1f60462488f0692c1e3a2042e395e5b1243e42 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 1 Jul 2019 12:04:10 +0200 Subject: [PATCH 39/42] Serialize access to block sealing --- ethcore/src/miner/miner.rs | 34 ++++++++++++---------------------- 1 file changed, 12 insertions(+), 22 deletions(-) diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index d105b0a9694..e6f29949950 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -316,7 +316,7 @@ impl Miner { } else { Some(ServiceTransactionChecker::default()) }, - last_parent_hash: RwLock::new(H256::zero()) + last_parent_hash: RwLock::new(H256::zero()) } } @@ -676,45 +676,35 @@ impl Miner { return false } } - - trace!(target: "miner", "seal_block_internally: attempting internal seal for block #{}", block.header.number()); + let block_number = block.header.number(); + trace!(target: "miner", "seal_block_internally: attempting internal seal for block #{}", block_number); let parent_header = match chain.block_header(BlockId::Hash(*block.header.parent_hash())) { Some(h) => { match h.decode() { Ok(decoded_hdr) => decoded_hdr, Err(e) => { - error!(target: "miner", "seal_block_internally: Block #{}, Could not decode header from parent block (hash={}): {:?}", block.header.number(), block.header.parent_hash(), e); + error!(target: "miner", "seal_block_internally: Block #{}, Could not decode header from parent block (hash={}): {:?}", block_number, block.header.parent_hash(), e); return false } } } None => { - trace!(target: "miner", "Block #{}: Parent with hash={} does not exist in our DB", block.header.number(), block.header.parent_hash()); + trace!(target: "miner", "Block #{}: Parent with hash={} does not exist in our DB", block_number, block.header.parent_hash()); return false }, }; -// { -// let last_parent_hash = self.last_parent_hash.read(); -// if parent_header.hash() == *last_parent_hash { -// warn!(target: "miner", "Trying to build block #{} on same parent hash. New block's parent hash: {}, last parent hash: {}", -// block.header.number(), parent_header.hash(), *last_parent_hash -// ); -// return false -// } -// } -// { - let mut last_parent_hash = self.last_parent_hash.write(); - trace!(target: "miner", "Block #{}, Setting last parent hash. was: {}, becomes: {}", block.header.number(), *last_parent_hash, block.header.hash()); - *last_parent_hash = block.header.hash(); -// } + // Take a lock on this parent_hash to stop any other blocks to be sealed with the same parent hash + let mut last_parent_hash = self.last_parent_hash.write(); + trace!(target: "miner", "Block #{}, Setting last parent hash. was: {}, becomes: {}", block_number, *last_parent_hash, block.header.hash()); + *last_parent_hash = block.header.hash(); let res = match self.engine.generate_seal(&block, &parent_header) { // Directly import a regular sealed block. Seal::Regular(seal) => { - trace!(target: "miner", "Block #{}: Received a Regular seal.", block.header.number()); + trace!(target: "miner", "Block #{}: Received a Regular seal.", block_number); { let mut sealing = self.sealing.lock(); sealing.next_mandatory_reseal = Instant::now() + self.options.reseal_max_period; @@ -727,13 +717,13 @@ impl Miner { chain.import_sealed_block(sealed).is_ok() }) .unwrap_or_else(|e| { - warn!("ERROR: Block #{}, importing sealed block failed when given internally generated seal: {}", block.header.number(), e); + warn!("ERROR: Block #{}, importing sealed block failed when given internally generated seal: {}", block_number, e); false }) }, Seal::None => false, }; - trace!(target:"miner", "Block: #{}, seal_and_import_block_internally: done, returning {}", block.header.number(), res); + trace!(target:"miner", "Block: #{}, seal_and_import_block_internally: done, returning {}", block_number, res); res } From dc9e55f8677579a7678dbe0d066122ab6274b9b5 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 1 Jul 2019 15:11:25 +0200 Subject: [PATCH 40/42] Take a lock while sealing a block --- ethcore/src/engines/authority_round/mod.rs | 13 ++-- ethcore/src/engines/validator_set/contract.rs | 1 + ethcore/src/engines/validator_set/test.rs | 1 + ethcore/src/miner/miner.rs | 64 +++++++++++-------- 4 files changed, 47 insertions(+), 32 deletions(-) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index b786d4adc2f..db75e8020b1 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -570,7 +570,7 @@ fn header_empty_steps_signers(header: &Header, empty_steps_transition: u64) -> R fn step_proposer(validators: &dyn ValidatorSet, bh: &H256, step: u64) -> Address { let proposer = validators.get(bh, step as usize); - trace!(target: "engine", "Fetched proposer for step {}: {}", step, proposer); + trace!(target: "engine", "step_proposer: Fetched proposer for step {}: {}", step, proposer); proposer } @@ -823,8 +823,8 @@ impl AuthorityRound { if skipped_primary != me { // Stop reporting once validators start repeating. if !reported.insert(skipped_primary) { break; } - trace!(target: "engine", "Reporting benign misbehaviour (cause: skipped step) at block #{}, epoch set number {}. Own address: {}", - header.number(), set_number, me); + trace!(target: "engine", "Reporting benign misbehaviour (cause: skipped step) at block #{}, epoch set number {}, step proposer={:#x}. Own address: {}", + header.number(), set_number, skipped_primary, me); self.validators.report_benign(&skipped_primary, set_number, header.number()); } else { trace!(target: "engine", "Primary that skipped is self, not self-reporting. Own address: {}", me); @@ -1142,6 +1142,7 @@ impl Engine for AuthorityRound { }; if is_step_proposer(&*validators, header.parent_hash(), step, header.author()) { + trace!(target: "engine", "generate_seal: we are step proposer for step={}, block=#{}", step, header.number()); // if there are no transactions to include in the block, we don't seal and instead broadcast a signed // `EmptyStep(step, parent_hash)` message. If we exceed the maximum amount of `empty_step` rounds we proceed // with the seal. @@ -1150,6 +1151,7 @@ impl Engine for AuthorityRound { empty_steps.len() < self.maximum_empty_steps { if self.step.can_propose.compare_and_swap(true, false, AtomicOrdering::SeqCst) { + trace!(target: "engine", "generate_seal: generating empty step at step={}, block=#{}", step, header.number()); self.generate_empty_step(header.parent_hash()); } @@ -1176,6 +1178,7 @@ impl Engine for AuthorityRound { // report any skipped primaries between the parent block and // the block we're sealing, unless we have empty steps enabled if header.number() < self.empty_steps_transition { + trace!(target: "engine", "generate_seal: reporting misbehaviour for step={}, block=#{}", step, header.number()); self.report_skipped(header, step, parent_step, &*validators, epoch_transition_number); } @@ -1187,7 +1190,7 @@ impl Engine for AuthorityRound { if let Some(empty_steps_rlp) = empty_steps_rlp { fields.push(empty_steps_rlp); } - + trace!(target: "engine", "generate_seal: returning Seal::Regular for step={}, block=#{}", step, header.number()); return Seal::Regular(fields); } } else { @@ -1197,7 +1200,7 @@ impl Engine for AuthorityRound { trace!(target: "engine", "generate_seal: {} not a proposer for step {}.", header.author(), step); } - + trace!(target: "engine", "generate_seal: returning Seal::None for step={}, block=#{}", step, header.number()); Seal::None } diff --git a/ethcore/src/engines/validator_set/contract.rs b/ethcore/src/engines/validator_set/contract.rs index 360d227ea71..5b695525dc5 100644 --- a/ethcore/src/engines/validator_set/contract.rs +++ b/ethcore/src/engines/validator_set/contract.rs @@ -118,6 +118,7 @@ impl ValidatorSet for ValidatorContract { } fn report_benign(&self, address: &Address, _set_block: BlockNumber, block: BlockNumber) { + trace!(target: "engine", "validator set recording benign misbehaviour at block #{} by {:#x}", block, address); let data = validator_report::functions::report_benign::encode_input(*address, block); match self.transact(data) { Ok(_) => warn!(target: "engine", "Reported benign validator misbehaviour {}", address), diff --git a/ethcore/src/engines/validator_set/test.rs b/ethcore/src/engines/validator_set/test.rs index dd84a84f8c8..04e695ef4a4 100644 --- a/ethcore/src/engines/validator_set/test.rs +++ b/ethcore/src/engines/validator_set/test.rs @@ -93,6 +93,7 @@ impl ValidatorSet for TestSet { } fn report_benign(&self, _validator: &Address, _set_block: BlockNumber, block: BlockNumber) { + trace!(target: "engine", "test validator set recording benign misbehaviour"); self.last_benign.store(block as usize, AtomicOrdering::SeqCst) } } diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index e6f29949950..ad620a7a4b4 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -680,7 +680,7 @@ impl Miner { trace!(target: "miner", "seal_block_internally: attempting internal seal for block #{}", block_number); let parent_header = match chain.block_header(BlockId::Hash(*block.header.parent_hash())) { - Some(h) => { + Some(h) => { match h.decode() { Ok(decoded_hdr) => decoded_hdr, Err(e) => { @@ -695,36 +695,46 @@ impl Miner { }, }; - // Take a lock on this parent_hash to stop any other blocks to be sealed with the same parent hash + // Take a lock on this parent_hash to stop any other blocks to be sealed with the same parent hash. + // NOTE: this does not work: when reporting misbehaviour the contract ends up calling back + // here again as part of `import_own_transaction` so we deadlock (cf. reports_validators + // test). let mut last_parent_hash = self.last_parent_hash.write(); trace!(target: "miner", "Block #{}, Setting last parent hash. was: {}, becomes: {}", block_number, *last_parent_hash, block.header.hash()); *last_parent_hash = block.header.hash(); + trace!(target: "miner", "Block #{} ACQUIRED last_hash LOCK", block_number); + + let sealing_result = + match self.engine.generate_seal(&block, &parent_header) { + // Directly import a regular sealed block. + Seal::Regular(seal) => { + trace!(target: "miner", "Block #{}: Received a Regular seal.", block_number); + { + let mut sealing = self.sealing.lock(); + sealing.next_mandatory_reseal = Instant::now() + self.options.reseal_max_period; + } - let res = - match self.engine.generate_seal(&block, &parent_header) { - // Directly import a regular sealed block. - Seal::Regular(seal) => { - trace!(target: "miner", "Block #{}: Received a Regular seal.", block_number); - { - let mut sealing = self.sealing.lock(); - sealing.next_mandatory_reseal = Instant::now() + self.options.reseal_max_period; - } - - block - .lock() - .seal(&*self.engine, seal) - .map(|sealed| { - chain.import_sealed_block(sealed).is_ok() - }) - .unwrap_or_else(|e| { - warn!("ERROR: Block #{}, importing sealed block failed when given internally generated seal: {}", block_number, e); - false - }) - }, - Seal::None => false, - }; - trace!(target:"miner", "Block: #{}, seal_and_import_block_internally: done, returning {}", block_number, res); - res + block + .lock() + .seal(&*self.engine, seal) + .map(|sealed| { + match chain.import_sealed_block(sealed) { + Ok(_) => true, + Err(e) => { + error!(target: "miner", "Block #{}: seal_and_import_block_internally: import_sealed_block returned {:?}", block_number, e); + false + } + } + }) + .unwrap_or_else(|e| { + warn!("ERROR: Block #{}, importing sealed block failed when given internally generated seal: {}", block_number, e); + false + }) + }, + Seal::None => false, + }; + trace!(target:"miner", "Block #{}: seal_and_import_block_internally: done, returning {}", block_number, sealing_result); + sealing_result } /// Prepares work which has to be done to seal. From cd72c6a2c878c8624c6c68f2113c913b7966772f Mon Sep 17 00:00:00 2001 From: David Palm Date: Thu, 4 Jul 2019 14:18:18 +0200 Subject: [PATCH 41/42] Cleanup --- ethcore/src/miner/miner.rs | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index ac1cc1d61f3..eed0b1595ad 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -254,7 +254,6 @@ pub struct Miner { accounts: Arc, io_channel: RwLock>>, service_transaction_checker: Option, - last_parent_hash: RwLock, } impl Miner { @@ -316,7 +315,6 @@ impl Miner { } else { Some(ServiceTransactionChecker::default()) }, - last_parent_hash: RwLock::new(H256::zero()) } } @@ -680,7 +678,7 @@ impl Miner { trace!(target: "miner", "seal_block_internally: attempting internal seal for block #{}", block_number); let parent_header = match chain.block_header(BlockId::Hash(*block.header.parent_hash())) { - Some(h) => { + Some(h) => { match h.decode() { Ok(decoded_hdr) => decoded_hdr, Err(e) => { @@ -688,22 +686,13 @@ impl Miner { return false } } - } + }, None => { trace!(target: "miner", "Block #{}: Parent with hash={} does not exist in our DB", block_number, block.header.parent_hash()); return false }, }; - // Take a lock on this parent_hash to stop any other blocks to be sealed with the same parent hash. - // NOTE: this does not work: when reporting misbehaviour the contract ends up calling back - // here again as part of `import_own_transaction` so we deadlock (cf. reports_validators - // test). - let mut last_parent_hash = self.last_parent_hash.write(); - trace!(target: "miner", "Block #{}, Setting last parent hash. was: {}, becomes: {}", block_number, *last_parent_hash, block.header.hash()); - *last_parent_hash = block.header.hash(); - trace!(target: "miner", "Block #{} ACQUIRED last_hash LOCK", block_number); - let sealing_result = match self.engine.generate_seal(&block, &parent_header) { // Directly import a regular sealed block. @@ -733,7 +722,6 @@ impl Miner { }, Seal::None => false, }; - trace!(target:"miner", "Block #{}: seal_and_import_block_internally: done, returning {}", block_number, sealing_result); sealing_result } @@ -820,13 +808,11 @@ impl Miner { // Yes, it's a bit convoluted. let prepare_new_block = self.maybe_enable_sealing(); - // TODO: Callers check for this already. Ok to remove? if self.engine.sealing_state() != SealingState::External { trace!(target: "miner", "prepare_pending_block: engine not sealing externally; not preparing"); return BlockPreparationStatus::NotPrepared; } - trace!(target: "miner", "prepare_pending_block: entering"); let preparation_status = if prepare_new_block { // -------------------------------------------------------------------------- // | NOTE Code below requires sealing locks. | @@ -1767,7 +1753,6 @@ mod tests { miner.update_sealing(&*client); client.flush_queue(); assert!(miner.pending_block(0).is_none()); - println!("NOW"); assert_eq!(client.chain_info().best_block_number, 4 as BlockNumber); } From 07aa66bd8b2c02ec2919dbd3eb4c5b44fb98ed13 Mon Sep 17 00:00:00 2001 From: David Palm Date: Thu, 4 Jul 2019 17:31:37 +0200 Subject: [PATCH 42/42] whitespace --- ethcore/src/block.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethcore/src/block.rs b/ethcore/src/block.rs index e44c346b7f1..4b22633a977 100644 --- a/ethcore/src/block.rs +++ b/ethcore/src/block.rs @@ -178,7 +178,7 @@ impl<'x> OpenBlock<'x> { ) -> Result { let number = parent.number() + 1; let state = State::from_existing(db, parent.state_root().clone(), engine.account_start_nonce(number), factories)?; - let mut r = OpenBlock { block: ExecutedBlock::new(state, last_hashes, tracing), engine, parent: parent.clone()}; + let mut r = OpenBlock { block: ExecutedBlock::new(state, last_hashes, tracing), engine, parent: parent.clone()}; r.block.header.set_parent_hash(parent.hash()); r.block.header.set_number(number);