Fixes for misbehavior reporting in AuthorityRound (#8998)

* aura: only report after checking for repeated skipped primaries

* aura: refactor duplicate code for getting epoch validator set

* aura: verify_external: report on validator set contract instance

* aura: use correct validator set epoch number when reporting

* aura: use epoch set when verifying blocks

* aura: report skipped primaries when generating seal

* aura: handle immediate transitions

* aura: don't report skipped steps from genesis to first block

* aura: fix reporting test

* aura: refactor duplicate code to handle immediate_transitions

* aura: let reporting fail on verify_block_basic

* aura: add comment about possible failure of reporting

ethcore/src/engines/authority_round/mod.rs

@@ -16,20 +16,21 @@
 
 //! 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::*;
 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 +576,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(())
@@ -607,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<A> {
+ 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<Arc<Self>, Error> {
@@ -656,6 +673,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<ValidatorSet, SimpleList>, 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<EmptyStep> {
  self.empty_steps.lock().iter().filter(|e| {
  U256::from(e.step) > from_step &&
@@ -701,6 +742,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 {
@@ -880,32 +943,15 @@ impl Engine<EthereumMachine> 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() {
@@ -936,9 +982,15 @@ impl Engine<EthereumMachine> 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(),
@@ -1057,13 +1109,21 @@ impl Engine<EthereumMachine> 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()),
@@ -1075,8 +1135,8 @@ impl Engine<EthereumMachine> 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
@@ -1103,7 +1163,7 @@ impl Engine<EthereumMachine> 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)))?;
  }
@@ -1117,59 +1177,29 @@ impl Engine<EthereumMachine> 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(())
  }
 
  // 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(ErrorKind::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
  }
@@ -1574,7 +1604,6 @@ mod tests {
  parent_header.set_seal(vec![encode(&1usize).into_vec()]);
  parent_header.set_gas_limit("222222".parse::<U256>().unwrap());
  let mut header: Header = Header::default();
- header.set_number(1);
  header.set_gas_limit("222222".parse::<U256>().unwrap());
  header.set_seal(vec![encode(&3usize).into_vec()]);
 
@@ -1584,8 +1613,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), 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]

ethcore/src/engines/validator_set/mod.rs

@@ -53,7 +53,7 @@ pub fn new_validator_set(spec: ValidatorSpec) -> Box<ValidatorSet> {
 }
 
 /// 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.

ethcore/src/engines/validator_set/simple_list.rs

@@ -104,6 +104,12 @@ impl ValidatorSet for SimpleList {
  }
 }
 
+impl AsRef<ValidatorSet> for SimpleList {
+ fn as_ref(&self) -> &ValidatorSet {
+ self
+ }
+}
+
 #[cfg(test)]
 mod tests {
  use std::str::FromStr;