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
This commit is contained in:
parent
25b900e30a
commit
1a7581361d
@ -16,12 +16,13 @@
|
|||||||
|
|
||||||
//! A blockchain engine that supports a non-instant BFT proof-of-authority.
|
//! A blockchain engine that supports a non-instant BFT proof-of-authority.
|
||||||
|
|
||||||
|
use std::collections::{BTreeMap, HashSet};
|
||||||
use std::fmt;
|
use std::fmt;
|
||||||
|
use std::iter::FromIterator;
|
||||||
|
use std::ops::Deref;
|
||||||
use std::sync::atomic::{AtomicUsize, AtomicBool, Ordering as AtomicOrdering};
|
use std::sync::atomic::{AtomicUsize, AtomicBool, Ordering as AtomicOrdering};
|
||||||
use std::sync::{Weak, Arc};
|
use std::sync::{Weak, Arc};
|
||||||
use std::time::{UNIX_EPOCH, SystemTime, Duration};
|
use std::time::{UNIX_EPOCH, SystemTime, Duration};
|
||||||
use std::collections::{BTreeMap, HashSet};
|
|
||||||
use std::iter::FromIterator;
|
|
||||||
|
|
||||||
use account_provider::AccountProvider;
|
use account_provider::AccountProvider;
|
||||||
use block::*;
|
use block::*;
|
||||||
@ -29,7 +30,7 @@ use client::EngineClient;
|
|||||||
use engines::{Engine, Seal, EngineError, ConstructedVerifier};
|
use engines::{Engine, Seal, EngineError, ConstructedVerifier};
|
||||||
use engines::block_reward;
|
use engines::block_reward;
|
||||||
use engines::block_reward::{BlockRewardContract, RewardKind};
|
use engines::block_reward::{BlockRewardContract, RewardKind};
|
||||||
use error::{Error, BlockError};
|
use error::{Error, ErrorKind, BlockError};
|
||||||
use ethjson;
|
use ethjson;
|
||||||
use machine::{AuxiliaryData, Call, EthereumMachine};
|
use machine::{AuxiliaryData, Call, EthereumMachine};
|
||||||
use hash::keccak;
|
use hash::keccak;
|
||||||
@ -575,7 +576,6 @@ fn verify_external(header: &Header, validators: &ValidatorSet, empty_steps_trans
|
|||||||
|
|
||||||
if is_invalid_proposer {
|
if is_invalid_proposer {
|
||||||
trace!(target: "engine", "verify_block_external: bad proposer for step: {}", header_step);
|
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() }))?
|
Err(EngineError::NotProposer(Mismatch { expected: correct_proposer, found: header.author().clone() }))?
|
||||||
} else {
|
} else {
|
||||||
Ok(())
|
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 {
|
impl AuthorityRound {
|
||||||
/// Create a new instance of AuthorityRound engine.
|
/// Create a new instance of AuthorityRound engine.
|
||||||
pub fn new(our_params: AuthorityRoundParams, machine: EthereumMachine) -> Result<Arc<Self>, Error> {
|
pub fn new(our_params: AuthorityRoundParams, machine: EthereumMachine) -> Result<Arc<Self>, Error> {
|
||||||
@ -656,6 +673,30 @@ impl AuthorityRound {
|
|||||||
Ok(engine)
|
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> {
|
fn empty_steps(&self, from_step: U256, to_step: U256, parent_hash: H256) -> Vec<EmptyStep> {
|
||||||
self.empty_steps.lock().iter().filter(|e| {
|
self.empty_steps.lock().iter().filter(|e| {
|
||||||
U256::from(e.step) > from_step &&
|
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 {
|
fn unix_now() -> Duration {
|
||||||
@ -880,32 +943,15 @@ impl Engine<EthereumMachine> for AuthorityRound {
|
|||||||
return Seal::None;
|
return Seal::None;
|
||||||
}
|
}
|
||||||
|
|
||||||
// fetch correct validator set for current epoch, taking into account
|
let (validators, set_number) = match self.epoch_set(header) {
|
||||||
// finality of previous transitions.
|
Err(err) => {
|
||||||
let active_set;
|
warn!(target: "engine", "Unable to generate seal: {}", err);
|
||||||
|
|
||||||
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;
|
return Seal::None;
|
||||||
}
|
},
|
||||||
|
Ok(ok) => ok,
|
||||||
};
|
};
|
||||||
|
|
||||||
if !epoch_manager.zoom_to(&*client, &self.machine, &*self.validators, header) {
|
if is_step_proposer(&*validators, header.parent_hash(), step, header.author()) {
|
||||||
debug!(target: "engine", "Unable to zoom to epoch.");
|
|
||||||
return Seal::None;
|
|
||||||
}
|
|
||||||
|
|
||||||
active_set = epoch_manager.validators().clone();
|
|
||||||
&active_set as &_
|
|
||||||
};
|
|
||||||
|
|
||||||
if is_step_proposer(validators, header.parent_hash(), step, header.author()) {
|
|
||||||
// this is guarded against by `can_propose` unless the block was signed
|
// this is guarded against by `can_propose` unless the block was signed
|
||||||
// on the same step (implies same key) and on a different node.
|
// on the same step (implies same key) and on a different node.
|
||||||
if parent_step == step.into() {
|
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.
|
// 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) {
|
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);
|
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![
|
let mut fields = vec![
|
||||||
encode(&step).into_vec(),
|
encode(&step).into_vec(),
|
||||||
encode(&(&H520::from(signature) as &[u8])).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)?) {
|
match verify_timestamp(&self.step.inner, header_step(header, self.empty_steps_transition)?) {
|
||||||
Err(BlockError::InvalidSeal) => {
|
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());
|
self.validators.report_benign(header.author(), set_number, header.number());
|
||||||
|
}
|
||||||
|
|
||||||
Err(BlockError::InvalidSeal.into())
|
Err(BlockError::InvalidSeal.into())
|
||||||
}
|
}
|
||||||
Err(e) => Err(e.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> {
|
fn verify_block_family(&self, header: &Header, parent: &Header) -> Result<(), Error> {
|
||||||
let step = header_step(header, self.empty_steps_transition)?;
|
let step = header_step(header, self.empty_steps_transition)?;
|
||||||
let parent_step = header_step(parent, 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.
|
// Ensure header is from the step after parent.
|
||||||
if step == parent_step
|
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)))?;
|
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(
|
Err(EngineError::InsufficientProof(
|
||||||
format!("invalid empty step proof: {:?}", empty_step)))?;
|
format!("invalid empty step proof: {:?}", empty_step)))?;
|
||||||
}
|
}
|
||||||
@ -1117,21 +1177,7 @@ impl Engine<EthereumMachine> for AuthorityRound {
|
|||||||
}
|
}
|
||||||
|
|
||||||
} else {
|
} else {
|
||||||
// Report skipped primaries.
|
self.report_skipped(header, step, parent_step, &*validators, set_number);
|
||||||
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; }
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
Ok(())
|
Ok(())
|
||||||
@ -1139,37 +1185,21 @@ impl Engine<EthereumMachine> for AuthorityRound {
|
|||||||
|
|
||||||
// Check the validators.
|
// Check the validators.
|
||||||
fn verify_block_external(&self, header: &Header) -> Result<(), Error> {
|
fn verify_block_external(&self, header: &Header) -> Result<(), Error> {
|
||||||
// fetch correct validator set for current epoch, taking into account
|
let (validators, set_number) = self.epoch_set(header)?;
|
||||||
// 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 &_
|
|
||||||
};
|
|
||||||
|
|
||||||
// verify signature against fixed list, but reports should go to the
|
// verify signature against fixed list, but reports should go to the
|
||||||
// contract itself.
|
// contract itself.
|
||||||
let res = verify_external(header, validators, self.empty_steps_transition);
|
let res = verify_external(header, &*validators, self.empty_steps_transition);
|
||||||
if res.is_ok() {
|
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)?;
|
let header_step = header_step(header, self.empty_steps_transition)?;
|
||||||
self.clear_empty_steps(header_step.into());
|
self.clear_empty_steps(header_step.into());
|
||||||
|
},
|
||||||
|
_ => {},
|
||||||
}
|
}
|
||||||
res
|
res
|
||||||
}
|
}
|
||||||
@ -1574,7 +1604,6 @@ mod tests {
|
|||||||
parent_header.set_seal(vec![encode(&1usize).into_vec()]);
|
parent_header.set_seal(vec![encode(&1usize).into_vec()]);
|
||||||
parent_header.set_gas_limit("222222".parse::<U256>().unwrap());
|
parent_header.set_gas_limit("222222".parse::<U256>().unwrap());
|
||||||
let mut header: Header = Header::default();
|
let mut header: Header = Header::default();
|
||||||
header.set_number(1);
|
|
||||||
header.set_gas_limit("222222".parse::<U256>().unwrap());
|
header.set_gas_limit("222222".parse::<U256>().unwrap());
|
||||||
header.set_seal(vec![encode(&3usize).into_vec()]);
|
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());
|
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!(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]
|
#[test]
|
||||||
|
@ -53,7 +53,7 @@ pub fn new_validator_set(spec: ValidatorSpec) -> Box<ValidatorSet> {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/// A validator set.
|
/// A validator set.
|
||||||
pub trait ValidatorSet: Send + Sync {
|
pub trait ValidatorSet: Send + Sync + 'static {
|
||||||
/// Get the default "Call" helper, for use in general operation.
|
/// Get the default "Call" helper, for use in general operation.
|
||||||
// TODO [keorn]: this is a hack intended to migrate off of
|
// TODO [keorn]: this is a hack intended to migrate off of
|
||||||
// a strict dependency on state always being available.
|
// a strict dependency on state always being available.
|
||||||
|
@ -104,6 +104,12 @@ impl ValidatorSet for SimpleList {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
impl AsRef<ValidatorSet> for SimpleList {
|
||||||
|
fn as_ref(&self) -> &ValidatorSet {
|
||||||
|
self
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod tests {
|
mod tests {
|
||||||
use std::str::FromStr;
|
use std::str::FromStr;
|
||||||
|
Loading…
Reference in New Issue
Block a user