diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index 7112868f6..e75dac0c0 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, Duration}; -use std::collections::{BTreeMap, HashSet}; -use std::iter::FromIterator; use account_provider::AccountProvider; use block::*; @@ -564,7 +565,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(()) @@ -596,6 +596,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> { @@ -643,6 +660,30 @@ impl AuthorityRound { Ok(engine) } + // fetch correct validator set for epoch at header, taking into account + // finality of previous transitions. + 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()) + } + + (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 { self.empty_steps.lock().iter().filter(|e| { U256::from(e.step) > from_step && @@ -688,6 +729,28 @@ 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); + 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 { @@ -867,32 +930,15 @@ 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."); - return Seal::None; - } - }; - - if !epoch_manager.zoom_to(&*client, &self.machine, &*self.validators, header) { - debug!(target: "engine", "Unable to zoom to epoch."); + let (validators, set_number) = match self.epoch_set(header) { + Err(err) => { + warn!(target: "engine", "Unable to generate seal: {}", err); return Seal::None; - } - - active_set = epoch_manager.validators().clone(); - &active_set as &_ + }, + 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() { @@ -923,9 +969,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(), @@ -1030,13 +1082,21 @@ 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) => { - self.validators.report_benign(header.author(), set_number, header.number()); + // 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()); + } + Err(BlockError::InvalidSeal.into()) } Err(e) => Err(e.into()), @@ -1048,8 +1108,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 (validators, set_number) = self.epoch_set(header)?; // Ensure header is from the step after parent. if step == parent_step @@ -1076,7 +1136,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)))?; } @@ -1090,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(&*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.report_skipped(header, step, parent_step, &*validators, set_number); } Ok(()) @@ -1112,37 +1158,21 @@ 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 (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); - if res.is_ok() { - let header_step = header_step(header, self.empty_steps_transition)?; - self.clear_empty_steps(header_step.into()); + 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()); + }, + 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 } @@ -1542,7 +1572,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()]); @@ -1552,8 +1581,15 @@ mod tests { aura.set_signer(Arc::new(AccountProvider::transient_provider()), Default::default(), Default::default()); + // 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), 1); + 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), 2); } #[test] diff --git a/ethcore/src/engines/validator_set/mod.rs b/ethcore/src/engines/validator_set/mod.rs index de2164f91..b141e6648 100644 --- a/ethcore/src/engines/validator_set/mod.rs +++ b/ethcore/src/engines/validator_set/mod.rs @@ -55,7 +55,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 bb67c9778..dfca70bcb 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;