From 146c5cc96a38c168106f259b1c17a2a7a6057b97 Mon Sep 17 00:00:00 2001 From: Vladimir Komendantskiy Date: Thu, 4 Jul 2019 18:08:31 +0100 Subject: [PATCH 1/4] Aura: Report malice on sibling blocks from the same validator This was originally written by @vkomenda, then squashed for easier rebasing on master. Cleanup of `received_step_hashes` was moved to `verify_block_family`, since `on_prepare_block` does not exist on master, and a unit test was added. Original commit messages: added the map of received block header hashes do not return an error and remove older received block records optimised older record removal block hash comparison optimisation and the weak client ref fix SIBLING_MALICE_DETECTION_PERIOD constant review comments using step numbers instead of block numbers --- ethcore/engines/authority-round/src/lib.rs | 58 ++++++++++++++++++++-- ethcore/engines/validator-set/src/test.rs | 14 +++++- 2 files changed, 68 insertions(+), 4 deletions(-) diff --git a/ethcore/engines/authority-round/src/lib.rs b/ethcore/engines/authority-round/src/lib.rs index 99ee36b7826..0b65119ac38 100644 --- a/ethcore/engines/authority-round/src/lib.rs +++ b/ethcore/engines/authority-round/src/lib.rs @@ -482,6 +482,8 @@ pub struct AuthorityRound { two_thirds_majority_transition: BlockNumber, maximum_empty_steps: usize, machine: Machine, + /// History of step hashes recently received from peers. + received_step_hashes: RwLock>, } // header-chain validator. @@ -572,7 +574,7 @@ fn header_expected_seal_fields(header: &Header, empty_steps_transition: u64) -> fn header_step(header: &Header, empty_steps_transition: u64) -> Result { Rlp::new(&header.seal().get(0).unwrap_or_else(|| - panic!("was either checked with verify_block_basic or is genesis; has {} fields; qed (Make sure the spec + panic!("was either checked with verify_block_basic or is genesis; has {} fields; qed (Make sure the spec \ file has a correct genesis seal)", header_expected_seal_fields(header, empty_steps_transition)) )) .as_val() @@ -751,6 +753,7 @@ impl AuthorityRound { two_thirds_majority_transition: our_params.two_thirds_majority_transition, strict_empty_steps_transition: our_params.strict_empty_steps_transition, machine, + received_step_hashes: RwLock::new(Default::default()), }); // Do not initialize timeouts for tests. @@ -1377,6 +1380,25 @@ impl Engine for AuthorityRound { Err(EngineError::DoubleVote(*header.author()))?; } + // Report malice if the validator produced other sibling blocks in the same step. + let received_step_key = (step, *header.author()); + let new_hash = header.hash(); + if self.received_step_hashes.read().get(&received_step_key).into_iter().any(|h| *h != new_hash) { + trace!(target: "engine", "Validator {} produced sibling blocks in the same step", header.author()); + self.validators.report_malicious(header.author(), set_number, header.number(), Default::default()); + } else { + self.received_step_hashes.write().insert(received_step_key, new_hash); + } + + // Remove older step hash records. + const SIBLING_MALICE_DETECTION_PERIOD: u64 = 100; + let oldest_step = parent_step.saturating_sub(SIBLING_MALICE_DETECTION_PERIOD); + if oldest_step > 0 { + let mut rsh = self.received_step_hashes.write(); + let new_rsh = rsh.split_off(&(oldest_step, Address::zero())); + *rsh = new_rsh; + } + // If empty step messages are enabled we will validate the messages in the seal, missing messages are not // reported as there's no way to tell whether the empty step message was never sent or simply not included. let empty_steps_len = if header.number() >= self.empty_steps_transition { @@ -1936,8 +1958,6 @@ 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)); @@ -1980,6 +2000,38 @@ mod tests { assert_eq!(last_benign.load(AtomicOrdering::SeqCst), 2); } + #[test] + fn reports_multiple_blocks_per_step() { + let tap = AccountProvider::transient_provider(); + let addr0 = tap.insert_account(keccak("0").into(), &"0".into()).unwrap(); + let addr1 = tap.insert_account(keccak("1").into(), &"1".into()).unwrap(); + + let validator_set = TestSet::from_validators(vec![addr0, addr1]); + let aura = build_aura(|p| p.validators = Box::new(validator_set.clone())); + + aura.set_signer(Some(Box::new((Arc::new(tap), addr0, "0".into())))); + + let mut parent_header: Header = Header::default(); + parent_header.set_number(2); + parent_header.set_seal(vec![encode(&1usize)]); + parent_header.set_gas_limit("222222".parse::().unwrap()); + let mut header: Header = Header::default(); + header.set_number(3); + header.set_difficulty(calculate_score(1, 2, 0)); + header.set_gas_limit("222222".parse::().unwrap()); + header.set_seal(vec![encode(&2usize)]); + header.set_author(addr1); + + // First sibling block. + assert!(aura.verify_block_family(&header, &parent_header).is_ok()); + assert_eq!(validator_set.last_malicious(), 0); + + // Second sibling block: should be reported. + header.set_gas_limit("222223".parse::().unwrap()); + assert!(aura.verify_block_family(&header, &parent_header).is_ok()); + assert_eq!(validator_set.last_malicious(), 3); + } + #[test] fn test_uncles_transition() { let aura = build_aura(|params| { diff --git a/ethcore/engines/validator-set/src/test.rs b/ethcore/engines/validator-set/src/test.rs index 0198c2d5b75..4293542b4e9 100644 --- a/ethcore/engines/validator-set/src/test.rs +++ b/ethcore/engines/validator-set/src/test.rs @@ -36,7 +36,7 @@ use parity_bytes::Bytes; use super::{ValidatorSet, SimpleList}; /// Set used for testing with a single validator. -#[derive(MallocSizeOf, Debug)] +#[derive(Clone, MallocSizeOf, Debug)] pub struct TestSet { validator: SimpleList, #[ignore_malloc_size_of = "zero sized"] @@ -63,6 +63,18 @@ impl TestSet { last_benign, } } + + pub fn from_validators(validators: Vec
) -> Self { + TestSet::new(Default::default(), Default::default(), validators) + } + + pub fn last_malicious(&self) -> usize { + self.last_malicious.load(AtomicOrdering::SeqCst) + } + + pub fn last_benign(&self) -> usize { + self.last_benign.load(AtomicOrdering::SeqCst) + } } impl ValidatorSet for TestSet { From 96ca4229af129c9974d16bde389c73d9eec421f5 Mon Sep 17 00:00:00 2001 From: Andreas Fackler Date: Fri, 11 Oct 2019 12:06:18 +0200 Subject: [PATCH 2/4] Add docs; use map_or. --- ethcore/engines/authority-round/src/lib.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/ethcore/engines/authority-round/src/lib.rs b/ethcore/engines/authority-round/src/lib.rs index 0b65119ac38..aff6f681f0b 100644 --- a/ethcore/engines/authority-round/src/lib.rs +++ b/ethcore/engines/authority-round/src/lib.rs @@ -19,6 +19,17 @@ //! It is recommended to use the `two_thirds_majority_transition` option, to defend against the //! ["Attack of the Clones"](https://arxiv.org/pdf/1902.10244.pdf). Newly started networks can //! set this option to `0`, to use a 2/3 quorum from the beginning. +//! +//! To support on-chain governance, the [ValidatorSet] is pluggable: Aura supports simple +//! constant lists of validators as well as smart contract-based dynamic validator sets. +//! Misbehavior is reported to the [ValidatorSet] as well, so that e.g. governance contracts +//! can penalize or ban attacker's nodes. +//! +//! * "Benign" misbehavior are faults that can happen in normal operation, like failing +//! to propose a block in your slot, which could be due to a temporary network outage, or +//! wrong timestamps (due to out-of-sync clocks). +//! * "Malicious" reports are made only if the sender misbehaved deliberately (or due to a +//! software bug), e.g. if they proposed multiple blocks with the same step number. use std::collections::{BTreeMap, BTreeSet, HashSet}; use std::{cmp, fmt}; @@ -1383,7 +1394,7 @@ impl Engine for AuthorityRound { // Report malice if the validator produced other sibling blocks in the same step. let received_step_key = (step, *header.author()); let new_hash = header.hash(); - if self.received_step_hashes.read().get(&received_step_key).into_iter().any(|h| *h != new_hash) { + if self.received_step_hashes.read().get(&received_step_key).map_or(false, |h| *h != new_hash) { trace!(target: "engine", "Validator {} produced sibling blocks in the same step", header.author()); self.validators.report_malicious(header.author(), set_number, header.number(), Default::default()); } else { From d4003e25209c81f5b26c3a45e071fefc2d0a7566 Mon Sep 17 00:00:00 2001 From: Andreas Fackler Date: Fri, 11 Oct 2019 16:17:02 +0200 Subject: [PATCH 3/4] Update step hash record comment. Co-Authored-By: David --- ethcore/engines/authority-round/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethcore/engines/authority-round/src/lib.rs b/ethcore/engines/authority-round/src/lib.rs index aff6f681f0b..b81a09b7d40 100644 --- a/ethcore/engines/authority-round/src/lib.rs +++ b/ethcore/engines/authority-round/src/lib.rs @@ -1401,7 +1401,7 @@ impl Engine for AuthorityRound { self.received_step_hashes.write().insert(received_step_key, new_hash); } - // Remove older step hash records. + // Remove hash records older than 100 steps (picked as a reasonable trade-off between memory consumption and fault-tolerance). const SIBLING_MALICE_DETECTION_PERIOD: u64 = 100; let oldest_step = parent_step.saturating_sub(SIBLING_MALICE_DETECTION_PERIOD); if oldest_step > 0 { From 53c320c1fc633481fb1beff609bddd985f7e55a7 Mon Sep 17 00:00:00 2001 From: Andreas Fackler Date: Fri, 11 Oct 2019 17:13:29 +0200 Subject: [PATCH 4/4] Remove hash records after 2 rounds instead of 100 steps. --- ethcore/engines/authority-round/src/lib.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ethcore/engines/authority-round/src/lib.rs b/ethcore/engines/authority-round/src/lib.rs index b81a09b7d40..2f4202f168e 100644 --- a/ethcore/engines/authority-round/src/lib.rs +++ b/ethcore/engines/authority-round/src/lib.rs @@ -1401,9 +1401,10 @@ impl Engine for AuthorityRound { self.received_step_hashes.write().insert(received_step_key, new_hash); } - // Remove hash records older than 100 steps (picked as a reasonable trade-off between memory consumption and fault-tolerance). - const SIBLING_MALICE_DETECTION_PERIOD: u64 = 100; - let oldest_step = parent_step.saturating_sub(SIBLING_MALICE_DETECTION_PERIOD); + // Remove hash records older than two full rounds of steps (picked as a reasonable trade-off between + // memory consumption and fault-tolerance). + let sibling_malice_detection_period = 2 * validators.count(&parent.hash()) as u64; + let oldest_step = parent_step.saturating_sub(sibling_malice_detection_period); if oldest_step > 0 { let mut rsh = self.received_step_hashes.write(); let new_rsh = rsh.split_off(&(oldest_step, Address::zero()));