From 95d6dba79bc1726e98985b2960ef70c3c220959f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Tue, 26 Jun 2018 16:54:58 +0100 Subject: [PATCH 01/12] aura: only report after checking for repeated skipped primaries --- ethcore/src/engines/authority_round/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index dee9c76025f..4b74e766afc 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -1129,9 +1129,9 @@ impl Engine for AuthorityRound { let skipped_primary = step_proposer(&*self.validators, &parent.hash(), s); // Do not report this signer. if skipped_primary != me { - self.validators.report_benign(&skipped_primary, set_number, header.number()); // Stop reporting once validators start repeating. if !reported.insert(skipped_primary) { break; } + self.validators.report_benign(&skipped_primary, set_number, header.number()); } } } From 8013df608c3202bc9a539f3a1a82bf28123b3ce8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Wed, 27 Jun 2018 16:53:32 +0100 Subject: [PATCH 02/12] aura: refactor duplicate code for getting epoch validator set --- ethcore/src/engines/authority_round/mod.rs | 67 ++++++++++------------ 1 file changed, 31 insertions(+), 36 deletions(-) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index 4b74e766afc..9640423e428 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -656,6 +656,26 @@ impl AuthorityRound { Ok(engine) } + // fetch correct validator set for epoch at header, taking into account + // finality of previous transitions. + fn epoch_set(&self, header: &Header) -> Result { + let mut epoch_manager = self.epoch_manager.lock(); + let client = match self.client.read().as_ref().and_then(|weak| weak.upgrade()) { + Some(client) => client, + None => { + debug!(target: "engine", "Unable to verify sig: missing client ref."); + return Err(EngineError::RequiresClient.into()) + } + }; + + if !epoch_manager.zoom_to(&*client, &self.machine, &*self.validators, header) { + debug!(target: "engine", "Unable to zoom to epoch."); + return Err(EngineError::RequiresClient.into()) + } + + Ok(epoch_manager.validators().clone()) + } + fn empty_steps(&self, from_step: U256, to_step: U256, parent_hash: H256) -> Vec { self.empty_steps.lock().iter().filter(|e| { U256::from(e.step) > from_step && @@ -879,29 +899,20 @@ impl Engine for AuthorityRound { return Seal::None; } - // fetch correct validator set for current epoch, taking into account - // finality of previous transitions. let active_set; - let validators = if self.immediate_transitions { &*self.validators } else { - let mut epoch_manager = self.epoch_manager.lock(); - let client = match self.client.read().as_ref().and_then(|weak| weak.upgrade()) { - Some(client) => client, - None => { - warn!(target: "engine", "Unable to generate seal: missing client ref."); + match self.epoch_set(header) { + Err(err) => { + warn!(target: "engine", "Unable to generate seal: {}", err); return Seal::None; - } - }; - - if !epoch_manager.zoom_to(&*client, &self.machine, &*self.validators, header) { - debug!(target: "engine", "Unable to zoom to epoch."); - return Seal::None; + }, + Ok(set) => { + active_set = set; + &active_set + }, } - - active_set = epoch_manager.validators().clone(); - &active_set as &_ }; if is_step_proposer(validators, header.parent_hash(), step, header.author()) { @@ -1142,29 +1153,13 @@ impl Engine for AuthorityRound { // Check the validators. fn verify_block_external(&self, header: &Header) -> Result<(), Error> { - // fetch correct validator set for current epoch, taking into account - // finality of previous transitions. let active_set; let validators = if self.immediate_transitions { &*self.validators } else { - // get correct validator set for epoch. - let client = match self.client.read().as_ref().and_then(|weak| weak.upgrade()) { - Some(client) => client, - None => { - debug!(target: "engine", "Unable to verify sig: missing client ref."); - return Err(EngineError::RequiresClient.into()) - } - }; - - let mut epoch_manager = self.epoch_manager.lock(); - if !epoch_manager.zoom_to(&*client, &self.machine, &*self.validators, header) { - debug!(target: "engine", "Unable to zoom to epoch."); - return Err(EngineError::RequiresClient.into()) - } - - active_set = epoch_manager.validators().clone(); - &active_set as &_ + let set = self.epoch_set(header)?; + active_set = set; + &active_set }; // verify signature against fixed list, but reports should go to the From ed1028e4245a65a7ac26881eeb3e73d6fd5565fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Wed, 27 Jun 2018 17:23:32 +0100 Subject: [PATCH 03/12] aura: verify_external: report on validator set contract instance --- ethcore/src/engines/authority_round/mod.rs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index 9640423e428..556a14ad301 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -29,7 +29,7 @@ use client::EngineClient; use engines::{Engine, Seal, EngineError, ConstructedVerifier}; use engines::block_reward; use engines::block_reward::{BlockRewardContract, RewardKind}; -use error::{Error, BlockError}; +use error::{Error, ErrorKind, BlockError}; use ethjson; use machine::{AuxiliaryData, Call, EthereumMachine}; use hash::keccak; @@ -575,7 +575,6 @@ 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); - validators.report_benign(header.author(), header.number(), header.number()); Err(EngineError::NotProposer(Mismatch { expected: correct_proposer, found: header.author().clone() }))? } else { Ok(()) @@ -1165,9 +1164,16 @@ impl Engine for AuthorityRound { // verify signature against fixed list, but reports should go to the // contract itself. let res = verify_external(header, validators, self.empty_steps_transition); - if res.is_ok() { - let header_step = header_step(header, self.empty_steps_transition)?; - self.clear_empty_steps(header_step.into()); + match res { + Err(Error(ErrorKind::Engine(EngineError::NotProposer(_)), _)) => { + self.validators.report_benign(header.author(), header.number(), header.number()); + }, + Ok(_) => { + // we can drop all accumulated empty step messages that are older than this header's step + let header_step = header_step(header, self.empty_steps_transition)?; + self.clear_empty_steps(header_step.into()); + }, + _ => {}, } res } From afcb9472f3860d5d6931e1614cad7ebdabe069c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Wed, 27 Jun 2018 17:30:27 +0100 Subject: [PATCH 04/12] aura: use correct validator set epoch number when reporting --- ethcore/src/engines/authority_round/mod.rs | 25 ++++++++++------------ 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index 556a14ad301..a822cf5ce4e 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -657,7 +657,7 @@ impl AuthorityRound { // fetch correct validator set for epoch at header, taking into account // finality of previous transitions. - fn epoch_set(&self, header: &Header) -> Result { + fn epoch_set(&self, header: &Header) -> Result<(SimpleList, BlockNumber), Error> { let mut epoch_manager = self.epoch_manager.lock(); let client = match self.client.read().as_ref().and_then(|weak| weak.upgrade()) { Some(client) => client, @@ -672,7 +672,7 @@ impl AuthorityRound { return Err(EngineError::RequiresClient.into()) } - Ok(epoch_manager.validators().clone()) + Ok((epoch_manager.validators().clone(), epoch_manager.epoch_transition_number)) } fn empty_steps(&self, from_step: U256, to_step: U256, parent_hash: H256) -> Vec { @@ -907,7 +907,7 @@ impl Engine for AuthorityRound { warn!(target: "engine", "Unable to generate seal: {}", err); return Seal::None; }, - Ok(set) => { + Ok((set, _)) => { active_set = set; &active_set }, @@ -1070,12 +1070,9 @@ impl Engine for AuthorityRound { ))); } - // TODO [ToDr] Should this go from epoch manager? - // If yes then probably benign reporting needs to be moved further in the verification. - let set_number = header.number(); - match verify_timestamp(&self.step.inner, header_step(header, self.empty_steps_transition)?) { Err(BlockError::InvalidSeal) => { + let (_, set_number) = self.epoch_set(header)?; self.validators.report_benign(header.author(), set_number, header.number()); Err(BlockError::InvalidSeal.into()) } @@ -1088,8 +1085,8 @@ impl Engine for AuthorityRound { fn verify_block_family(&self, header: &Header, parent: &Header) -> Result<(), Error> { let step = header_step(header, self.empty_steps_transition)?; let parent_step = header_step(parent, self.empty_steps_transition)?; - // TODO [ToDr] Should this go from epoch manager? - let set_number = header.number(); + + let (_, set_number) = self.epoch_set(header)?; // Ensure header is from the step after parent. if step == parent_step @@ -1153,12 +1150,12 @@ impl Engine for AuthorityRound { // Check the validators. fn verify_block_external(&self, header: &Header) -> Result<(), Error> { let active_set; - let validators = if self.immediate_transitions { - &*self.validators + let (validators, set_number) = if self.immediate_transitions { + (&*self.validators, header.number()) } else { - let set = self.epoch_set(header)?; + let (set, set_number) = self.epoch_set(header)?; active_set = set; - &active_set + (&active_set as &_, set_number) }; // verify signature against fixed list, but reports should go to the @@ -1166,7 +1163,7 @@ impl Engine for AuthorityRound { let res = verify_external(header, validators, self.empty_steps_transition); match res { Err(Error(ErrorKind::Engine(EngineError::NotProposer(_)), _)) => { - self.validators.report_benign(header.author(), header.number(), header.number()); + 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 From fbafc04144bf5e4608c4f46599a6f0da038137ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Wed, 27 Jun 2018 17:46:06 +0100 Subject: [PATCH 05/12] aura: use epoch set when verifying blocks --- ethcore/src/engines/authority_round/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index a822cf5ce4e..ef91592009e 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -1086,7 +1086,7 @@ impl Engine for AuthorityRound { let step = header_step(header, self.empty_steps_transition)?; let parent_step = header_step(parent, self.empty_steps_transition)?; - let (_, set_number) = self.epoch_set(header)?; + let (validators, set_number) = self.epoch_set(header)?; // Ensure header is from the step after parent. if step == parent_step @@ -1113,7 +1113,7 @@ impl Engine for AuthorityRound { format!("empty step proof for invalid parent hash: {:?}", empty_step.parent_hash)))?; } - if !empty_step.verify(&*self.validators).unwrap_or(false) { + if !empty_step.verify(&validators).unwrap_or(false) { Err(EngineError::InsufficientProof( format!("invalid empty step proof: {:?}", empty_step)))?; } @@ -1133,7 +1133,7 @@ impl Engine for AuthorityRound { header.author(), step, parent_step); let mut reported = HashSet::new(); for s in parent_step + 1..step { - let skipped_primary = step_proposer(&*self.validators, &parent.hash(), s); + let skipped_primary = step_proposer(&validators, &parent.hash(), s); // Do not report this signer. if skipped_primary != me { // Stop reporting once validators start repeating. From 03210a84a3dfa3e93bbb878c6237fcf57c61f5c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Wed, 27 Jun 2018 19:04:23 +0100 Subject: [PATCH 06/12] aura: report skipped primaries when generating seal --- ethcore/src/engines/authority_round/mod.rs | 41 +++++++++++++--------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index ef91592009e..9b9eb80f3de 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -719,6 +719,23 @@ impl AuthorityRound { } } } + + fn report_skipped(&self, header: &Header, current_step: usize, parent_step: usize, validators: &ValidatorSet, set_number: u64) { + if let (true, Some(me)) = (current_step > parent_step + 1, self.signer.read().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()); + } + } + } + } } fn unix_now() -> Duration { @@ -945,9 +962,15 @@ impl Engine for AuthorityRound { // only issue the seal if we were the first to reach the compare_and_swap. if self.step.can_propose.compare_and_swap(true, false, AtomicOrdering::SeqCst) { - + // we can drop all accumulated empty step messages that are + // older than the parent step since we're including them in + // the seal self.clear_empty_steps(parent_step); + // report any skipped primaries between the parent block and + // the block we're sealing + self.report_skipped(header, step, u64::from(parent_step) as usize, validators, set_number); + let mut fields = vec![ encode(&step).into_vec(), encode(&(&H520::from(signature) as &[u8])).into_vec(), @@ -1127,21 +1150,7 @@ impl Engine for AuthorityRound { } } else { - // Report skipped primaries. - if let (true, Some(me)) = (step > parent_step + 1, self.signer.read().address()) { - debug!(target: "engine", "Author {} built block with step gap. current step: {}, parent step: {}", - header.author(), step, parent_step); - let mut reported = HashSet::new(); - for s in parent_step + 1..step { - let skipped_primary = step_proposer(&validators, &parent.hash(), s); - // 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()); - } - } - } + self.report_skipped(header, step, parent_step, validators, set_number); } Ok(()) From dccc9e74599b54115d30c567413578f33192be07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Wed, 27 Jun 2018 19:04:59 +0100 Subject: [PATCH 07/12] aura: handle immediate transitions --- ethcore/src/engines/authority_round/mod.rs | 26 ++++++++++++++++------ 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index 9b9eb80f3de..fa8822a89ac 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -916,17 +916,17 @@ impl Engine for AuthorityRound { } let active_set; - let validators = if self.immediate_transitions { - &*self.validators + let (validators, set_number) = if self.immediate_transitions { + (&*self.validators, header.number()) } else { match self.epoch_set(header) { Err(err) => { warn!(target: "engine", "Unable to generate seal: {}", err); return Seal::None; }, - Ok((set, _)) => { + Ok((set, set_number)) => { active_set = set; - &active_set + (&active_set as &_, set_number) }, } }; @@ -1095,7 +1095,12 @@ impl Engine for AuthorityRound { match verify_timestamp(&self.step.inner, header_step(header, self.empty_steps_transition)?) { Err(BlockError::InvalidSeal) => { - let (_, set_number) = self.epoch_set(header)?; + let set_number = if self.immediate_transitions { + header.number() + } else { + self.epoch_set(header)?.1 + }; + self.validators.report_benign(header.author(), set_number, header.number()); Err(BlockError::InvalidSeal.into()) } @@ -1109,7 +1114,14 @@ impl Engine for AuthorityRound { let step = header_step(header, self.empty_steps_transition)?; let parent_step = header_step(parent, self.empty_steps_transition)?; - let (validators, set_number) = self.epoch_set(header)?; + let active_set; + let (validators, set_number) = if self.immediate_transitions { + (&*self.validators, header.number()) + } else { + let (set, set_number) = self.epoch_set(header)?; + active_set = set; + (&active_set as &_, set_number) + }; // Ensure header is from the step after parent. if step == parent_step @@ -1136,7 +1148,7 @@ impl Engine for AuthorityRound { format!("empty step proof for invalid parent hash: {:?}", empty_step.parent_hash)))?; } - if !empty_step.verify(&validators).unwrap_or(false) { + if !empty_step.verify(validators).unwrap_or(false) { Err(EngineError::InsufficientProof( format!("invalid empty step proof: {:?}", empty_step)))?; } From 39ae1c22bcb3b52ccabba24831f132c6a37c2d3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Thu, 28 Jun 2018 15:05:18 +0100 Subject: [PATCH 08/12] aura: don't report skipped steps from genesis to first block --- ethcore/src/engines/authority_round/mod.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index fa8822a89ac..d7b55299f88 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -721,6 +721,11 @@ impl AuthorityRound { } fn report_skipped(&self, header: &Header, current_step: usize, parent_step: usize, validators: &ValidatorSet, set_number: u64) { + // we're building on top of the genesis block so don't report any skipped steps + if header.number() == 1 { + return; + } + if let (true, Some(me)) = (current_step > parent_step + 1, self.signer.read().address()) { debug!(target: "engine", "Author {} built block with step gap. current step: {}, parent step: {}", header.author(), current_step, parent_step); From 3e2096acb7bd13317dd0b17a668ef84461407fca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Wed, 4 Jul 2018 17:41:23 +0100 Subject: [PATCH 09/12] aura: fix reporting test --- ethcore/src/engines/authority_round/mod.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index d7b55299f88..4332794e4f8 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -1605,7 +1605,6 @@ mod tests { parent_header.set_seal(vec![encode(&1usize).into_vec()]); parent_header.set_gas_limit("222222".parse::().unwrap()); let mut header: Header = Header::default(); - header.set_number(1); header.set_gas_limit("222222".parse::().unwrap()); header.set_seal(vec![encode(&3usize).into_vec()]); @@ -1615,8 +1614,15 @@ mod tests { aura.set_signer(Arc::new(AccountProvider::transient_provider()), Default::default(), "".into()); + // Do not report on steps skipped between genesis and first block. + header.set_number(1); + assert!(aura.verify_block_family(&header, &parent_header).is_ok()); + assert_eq!(last_benign.load(AtomicOrdering::SeqCst), 0); + + // Report on skipped steps otherwise. + header.set_number(2); assert!(aura.verify_block_family(&header, &parent_header).is_ok()); - assert_eq!(last_benign.load(AtomicOrdering::SeqCst), 1); + assert_eq!(last_benign.load(AtomicOrdering::SeqCst), 2); } #[test] From 3641f1722f39f5e1a9b206b80ba403dd2a7e9a2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Thu, 5 Jul 2018 14:34:39 +0100 Subject: [PATCH 10/12] aura: refactor duplicate code to handle immediate_transitions --- ethcore/src/engines/authority_round/mod.rs | 107 +++++++++--------- ethcore/src/engines/validator_set/mod.rs | 2 +- .../src/engines/validator_set/simple_list.rs | 6 + 3 files changed, 58 insertions(+), 57 deletions(-) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index 4332794e4f8..6e12b4ade54 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -16,12 +16,13 @@ //! A blockchain engine that supports a non-instant BFT proof-of-authority. +use std::collections::{BTreeMap, HashSet}; use std::fmt; +use std::iter::FromIterator; +use std::ops::Deref; use std::sync::atomic::{AtomicUsize, AtomicBool, Ordering as AtomicOrdering}; use std::sync::{Weak, Arc}; use std::time::{UNIX_EPOCH, SystemTime, Duration}; -use std::collections::{BTreeMap, HashSet}; -use std::iter::FromIterator; use account_provider::AccountProvider; use block::*; @@ -606,6 +607,23 @@ impl AsMillis for Duration { } } +// A type for storing owned or borrowed data that has a common type. +// Useful for returning either a borrow or owned data from a function. +enum CowLike<'a, A: 'a + ?Sized, B> { + Borrowed(&'a A), + Owned(B), +} + +impl<'a, A: ?Sized, B> Deref for CowLike<'a, A, B> where B: AsRef { + type Target = A; + fn deref(&self) -> &A { + match self { + CowLike::Borrowed(b) => b, + CowLike::Owned(o) => o.as_ref(), + } + } +} + impl AuthorityRound { /// Create a new instance of AuthorityRound engine. pub fn new(our_params: AuthorityRoundParams, machine: EthereumMachine) -> Result, Error> { @@ -657,22 +675,26 @@ impl AuthorityRound { // fetch correct validator set for epoch at header, taking into account // finality of previous transitions. - fn epoch_set(&self, header: &Header) -> Result<(SimpleList, BlockNumber), Error> { - let mut epoch_manager = self.epoch_manager.lock(); - let client = match self.client.read().as_ref().and_then(|weak| weak.upgrade()) { - Some(client) => client, - None => { - debug!(target: "engine", "Unable to verify sig: missing client ref."); + fn epoch_set<'a>(&'a self, header: &Header) -> Result<(CowLike, BlockNumber), Error> { + Ok(if self.immediate_transitions { + (CowLike::Borrowed(&*self.validators), header.number()) + } else { + let mut epoch_manager = self.epoch_manager.lock(); + let client = match self.client.read().as_ref().and_then(|weak| weak.upgrade()) { + Some(client) => client, + None => { + debug!(target: "engine", "Unable to verify sig: missing client ref."); + return Err(EngineError::RequiresClient.into()) + } + }; + + if !epoch_manager.zoom_to(&*client, &self.machine, &*self.validators, header) { + debug!(target: "engine", "Unable to zoom to epoch."); return Err(EngineError::RequiresClient.into()) } - }; - - if !epoch_manager.zoom_to(&*client, &self.machine, &*self.validators, header) { - debug!(target: "engine", "Unable to zoom to epoch."); - return Err(EngineError::RequiresClient.into()) - } - Ok((epoch_manager.validators().clone(), epoch_manager.epoch_transition_number)) + (CowLike::Owned(epoch_manager.validators().clone()), epoch_manager.epoch_transition_number) + }) } fn empty_steps(&self, from_step: U256, to_step: U256, parent_hash: H256) -> Vec { @@ -920,23 +942,15 @@ impl Engine for AuthorityRound { return Seal::None; } - let active_set; - let (validators, set_number) = if self.immediate_transitions { - (&*self.validators, header.number()) - } else { - match self.epoch_set(header) { - Err(err) => { - warn!(target: "engine", "Unable to generate seal: {}", err); - return Seal::None; - }, - Ok((set, set_number)) => { - active_set = set; - (&active_set as &_, set_number) - }, - } + let (validators, set_number) = match self.epoch_set(header) { + Err(err) => { + warn!(target: "engine", "Unable to generate seal: {}", err); + return Seal::None; + }, + Ok(ok) => ok, }; - if is_step_proposer(validators, header.parent_hash(), step, header.author()) { + 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.into() { @@ -974,7 +988,7 @@ impl Engine for AuthorityRound { // report any skipped primaries between the parent block and // the block we're sealing - self.report_skipped(header, step, u64::from(parent_step) as usize, validators, set_number); + self.report_skipped(header, step, u64::from(parent_step) as usize, &*validators, set_number); let mut fields = vec![ encode(&step).into_vec(), @@ -1100,12 +1114,7 @@ impl Engine for AuthorityRound { match verify_timestamp(&self.step.inner, header_step(header, self.empty_steps_transition)?) { Err(BlockError::InvalidSeal) => { - let set_number = if self.immediate_transitions { - header.number() - } else { - self.epoch_set(header)?.1 - }; - + let set_number = self.epoch_set(header)?.1; self.validators.report_benign(header.author(), set_number, header.number()); Err(BlockError::InvalidSeal.into()) } @@ -1119,14 +1128,7 @@ impl Engine for AuthorityRound { let step = header_step(header, self.empty_steps_transition)?; let parent_step = header_step(parent, self.empty_steps_transition)?; - let active_set; - let (validators, set_number) = if self.immediate_transitions { - (&*self.validators, header.number()) - } else { - let (set, set_number) = self.epoch_set(header)?; - active_set = set; - (&active_set as &_, set_number) - }; + let (validators, set_number) = self.epoch_set(header)?; // Ensure header is from the step after parent. if step == parent_step @@ -1153,7 +1155,7 @@ impl Engine for AuthorityRound { format!("empty step proof for invalid parent hash: {:?}", empty_step.parent_hash)))?; } - if !empty_step.verify(validators).unwrap_or(false) { + if !empty_step.verify(&*validators).unwrap_or(false) { Err(EngineError::InsufficientProof( format!("invalid empty step proof: {:?}", empty_step)))?; } @@ -1167,7 +1169,7 @@ impl Engine for AuthorityRound { } } else { - self.report_skipped(header, step, parent_step, validators, set_number); + self.report_skipped(header, step, parent_step, &*validators, set_number); } Ok(()) @@ -1175,18 +1177,11 @@ impl Engine for AuthorityRound { // Check the validators. fn verify_block_external(&self, header: &Header) -> Result<(), Error> { - let active_set; - let (validators, set_number) = if self.immediate_transitions { - (&*self.validators, header.number()) - } else { - let (set, set_number) = self.epoch_set(header)?; - active_set = set; - (&active_set as &_, set_number) - }; + let (validators, set_number) = self.epoch_set(header)?; // verify signature against fixed list, but reports should go to the // contract itself. - let res = verify_external(header, validators, self.empty_steps_transition); + let res = verify_external(header, &*validators, self.empty_steps_transition); match res { Err(Error(ErrorKind::Engine(EngineError::NotProposer(_)), _)) => { self.validators.report_benign(header.author(), set_number, header.number()); diff --git a/ethcore/src/engines/validator_set/mod.rs b/ethcore/src/engines/validator_set/mod.rs index 26b57d78f24..d7018d14e9a 100644 --- a/ethcore/src/engines/validator_set/mod.rs +++ b/ethcore/src/engines/validator_set/mod.rs @@ -53,7 +53,7 @@ pub fn new_validator_set(spec: ValidatorSpec) -> Box { } /// A validator set. -pub trait ValidatorSet: Send + Sync { +pub trait ValidatorSet: Send + Sync + 'static { /// Get the default "Call" helper, for use in general operation. // TODO [keorn]: this is a hack intended to migrate off of // a strict dependency on state always being available. diff --git a/ethcore/src/engines/validator_set/simple_list.rs b/ethcore/src/engines/validator_set/simple_list.rs index e1339250ef3..dc269600d00 100644 --- a/ethcore/src/engines/validator_set/simple_list.rs +++ b/ethcore/src/engines/validator_set/simple_list.rs @@ -104,6 +104,12 @@ impl ValidatorSet for SimpleList { } } +impl AsRef for SimpleList { + fn as_ref(&self) -> &ValidatorSet { + self + } +} + #[cfg(test)] mod tests { use std::str::FromStr; From 86b3fdf1ffddb759ac24b843e5708aec4ad89abf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Thu, 5 Jul 2018 14:36:39 +0100 Subject: [PATCH 11/12] aura: let reporting fail on verify_block_basic --- ethcore/src/engines/authority_round/mod.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index 6e12b4ade54..677af048e5f 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -1114,8 +1114,10 @@ impl Engine for AuthorityRound { match verify_timestamp(&self.step.inner, header_step(header, self.empty_steps_transition)?) { Err(BlockError::InvalidSeal) => { - let set_number = self.epoch_set(header)?.1; - self.validators.report_benign(header.author(), set_number, header.number()); + if let Ok((_, set_number)) = self.epoch_set(header) { + self.validators.report_benign(header.author(), set_number, header.number()); + } + Err(BlockError::InvalidSeal.into()) } Err(e) => Err(e.into()), From b3e9c730bcc3026f8de39c59e12ada64a4d96a75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Thu, 5 Jul 2018 15:38:49 +0100 Subject: [PATCH 12/12] aura: add comment about possible failure of reporting --- ethcore/src/engines/authority_round/mod.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index 677af048e5f..1b070fbb867 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -1114,6 +1114,15 @@ impl Engine for AuthorityRound { match verify_timestamp(&self.step.inner, header_step(header, self.empty_steps_transition)?) { Err(BlockError::InvalidSeal) => { + // 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 + // - 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_number)) = self.epoch_set(header) { self.validators.report_benign(header.author(), set_number, header.number()); }