From 81b7698428cf0148ff609f1da301b89d881e1384 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Mon, 10 Dec 2018 18:58:38 +0000 Subject: [PATCH] Strict empty steps validation (#10041) * Add two failings tests for strict empty steps. * Implement strict validation of empty steps. --- ethcore/src/engines/authority_round/mod.rs | 304 ++++++++++++--------- ethcore/src/engines/validator_set/test.rs | 10 +- json/src/spec/authority_round.rs | 2 + 3 files changed, 190 insertions(+), 126 deletions(-) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index bf404b476..6cf7f4091 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -81,6 +81,8 @@ pub struct AuthorityRoundParams { pub empty_steps_transition: u64, /// Number of accepted empty steps. pub maximum_empty_steps: usize, + /// Transition block to strict empty steps validation. + pub strict_empty_steps_transition: u64, } const U16_MAX: usize = ::std::u16::MAX as usize; @@ -110,6 +112,7 @@ impl From for AuthorityRoundParams { maximum_uncle_count: p.maximum_uncle_count.map_or(0, Into::into), empty_steps_transition: p.empty_steps_transition.map_or(u64::max_value(), |n| ::std::cmp::max(n.into(), 1)), maximum_empty_steps: p.maximum_empty_steps.map_or(0, Into::into), + strict_empty_steps_transition: p.strict_empty_steps_transition.map_or(0, Into::into), } } } @@ -421,6 +424,7 @@ pub struct AuthorityRound { maximum_uncle_count_transition: u64, maximum_uncle_count: usize, empty_steps_transition: u64, + strict_empty_steps_transition: u64, maximum_empty_steps: usize, machine: EthereumMachine, } @@ -674,6 +678,7 @@ impl AuthorityRound { maximum_uncle_count: our_params.maximum_uncle_count, empty_steps_transition: our_params.empty_steps_transition, maximum_empty_steps: our_params.maximum_empty_steps, + strict_empty_steps_transition: our_params.strict_empty_steps_transition, machine: machine, }); @@ -1250,8 +1255,11 @@ impl Engine for AuthorityRound { // 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 { let validate_empty_steps = || -> Result { + let strict_empty_steps = header.number() >= self.strict_empty_steps_transition; let empty_steps = header_empty_steps(header)?; let empty_steps_len = empty_steps.len(); + let mut prev_empty_step = 0; + for empty_step in empty_steps { if empty_step.step <= parent_step || empty_step.step >= step { Err(EngineError::InsufficientProof( @@ -1267,7 +1275,20 @@ impl Engine for AuthorityRound { Err(EngineError::InsufficientProof( format!("invalid empty step proof: {:?}", empty_step)))?; } + + if strict_empty_steps { + if empty_step.step <= prev_empty_step { + Err(EngineError::InsufficientProof(format!( + "{} empty step: {:?}", + if empty_step.step == prev_empty_step { "duplicate" } else { "unordered" }, + empty_step + )))?; + } + + prev_empty_step = empty_step.step; + } } + Ok(empty_steps_len) }; @@ -1518,10 +1539,40 @@ mod tests { use spec::Spec; use transaction::{Action, Transaction}; use engines::{Seal, Engine, EngineError, EthEngine}; - use engines::validator_set::TestSet; + use engines::validator_set::{TestSet, SimpleList}; use error::{Error, ErrorKind}; use super::{AuthorityRoundParams, AuthorityRound, EmptyStep, SealedEmptyStep, calculate_score}; + fn aura(f: F) -> Arc where + F: FnOnce(&mut AuthorityRoundParams), + { + let mut params = AuthorityRoundParams { + step_duration: 1, + start_step: Some(1), + validators: Box::new(TestSet::default()), + validate_score_transition: 0, + validate_step_transition: 0, + immediate_transitions: true, + maximum_uncle_count_transition: 0, + maximum_uncle_count: 0, + empty_steps_transition: u64::max_value(), + maximum_empty_steps: 0, + block_reward: Default::default(), + block_reward_contract_transition: 0, + block_reward_contract: Default::default(), + strict_empty_steps_transition: 0, + }; + + // mutate aura params + f(&mut params); + + // create engine + let mut c_params = ::spec::CommonParams::default(); + c_params.gas_limit_bound_divisor = 5.into(); + let machine = ::machine::EthereumMachine::regular(c_params, Default::default()); + AuthorityRound::new(params, machine).unwrap() + } + #[test] fn has_valid_metadata() { let engine = Spec::new_test_round().engine; @@ -1695,28 +1746,9 @@ mod tests { #[test] fn reports_skipped() { let last_benign = Arc::new(AtomicUsize::new(0)); - let params = AuthorityRoundParams { - step_duration: 1, - start_step: Some(1), - validators: Box::new(TestSet::new(Default::default(), last_benign.clone())), - validate_score_transition: 0, - validate_step_transition: 0, - immediate_transitions: true, - maximum_uncle_count_transition: 0, - maximum_uncle_count: 0, - empty_steps_transition: u64::max_value(), - maximum_empty_steps: 0, - block_reward: Default::default(), - block_reward_contract_transition: 0, - block_reward_contract: Default::default(), - }; - - let aura = { - let mut c_params = ::spec::CommonParams::default(); - c_params.gas_limit_bound_divisor = 5.into(); - let machine = ::machine::EthereumMachine::regular(c_params, Default::default()); - AuthorityRound::new(params, machine).unwrap() - }; + let aura = aura(|p| { + p.validators = Box::new(TestSet::new(Default::default(), last_benign.clone())); + }); let mut parent_header: Header = Header::default(); parent_header.set_seal(vec![encode(&1usize)]); @@ -1745,29 +1777,9 @@ mod tests { #[test] fn test_uncles_transition() { - let last_benign = Arc::new(AtomicUsize::new(0)); - let params = AuthorityRoundParams { - step_duration: 1, - start_step: Some(1), - validators: Box::new(TestSet::new(Default::default(), last_benign.clone())), - validate_score_transition: 0, - validate_step_transition: 0, - immediate_transitions: true, - maximum_uncle_count_transition: 1, - maximum_uncle_count: 0, - empty_steps_transition: u64::max_value(), - maximum_empty_steps: 0, - block_reward: Default::default(), - block_reward_contract_transition: 0, - block_reward_contract: Default::default(), - }; - - let aura = { - let mut c_params = ::spec::CommonParams::default(); - c_params.gas_limit_bound_divisor = 5.into(); - let machine = ::machine::EthereumMachine::regular(c_params, Default::default()); - AuthorityRound::new(params, machine).unwrap() - }; + let aura = aura(|params| { + params.maximum_uncle_count_transition = 1; + }); assert_eq!(aura.maximum_uncle_count(0), 2); assert_eq!(aura.maximum_uncle_count(1), 0); @@ -1801,27 +1813,9 @@ mod tests { #[test] #[should_panic(expected="authority_round: step duration can't be zero")] fn test_step_duration_zero() { - let last_benign = Arc::new(AtomicUsize::new(0)); - let params = AuthorityRoundParams { - step_duration: 0, - start_step: Some(1), - validators: Box::new(TestSet::new(Default::default(), last_benign.clone())), - validate_score_transition: 0, - validate_step_transition: 0, - immediate_transitions: true, - maximum_uncle_count_transition: 0, - maximum_uncle_count: 0, - empty_steps_transition: u64::max_value(), - maximum_empty_steps: 0, - block_reward: Default::default(), - block_reward_contract_transition: 0, - block_reward_contract: Default::default(), - }; - - let mut c_params = ::spec::CommonParams::default(); - c_params.gas_limit_bound_divisor = 5.into(); - let machine = ::machine::EthereumMachine::regular(c_params, Default::default()); - AuthorityRound::new(params, machine).unwrap(); + aura(|params| { + params.step_duration = 0; + }); } fn setup_empty_steps() -> (Spec, Arc, Vec
) { @@ -1849,6 +1843,23 @@ mod tests { SealedEmptyStep { signature, step } } + fn set_empty_steps_seal(header: &mut Header, step: u64, block_signature: ðkey::Signature, empty_steps: &[SealedEmptyStep]) { + header.set_seal(vec![ + encode(&(step as usize)), + encode(&(&**block_signature as &[u8])), + ::rlp::encode_list(&empty_steps), + ]); + } + + fn assert_insufficient_proof(result: Result, contains: &str) { + match result { + Err(Error(ErrorKind::Engine(EngineError::InsufficientProof(ref s)), _)) =>{ + assert!(s.contains(contains), "Expected {:?} to contain {:?}", s, contains); + }, + e => assert!(false, "Unexpected result: {:?}", e), + } + } + #[test] fn broadcast_empty_step_message() { let (spec, tap, accounts) = setup_empty_steps(); @@ -2050,46 +2061,31 @@ mod tests { // empty step with invalid step let empty_steps = vec![SealedEmptyStep { signature: 0.into(), step: 2 }]; - header.set_seal(vec![ - encode(&2usize), - encode(&(&*signature as &[u8])), - ::rlp::encode_list(&empty_steps), - ]); + set_empty_steps_seal(&mut header, 2, &signature, &empty_steps); - assert!(match engine.verify_block_family(&header, &parent_header) { - Err(Error(ErrorKind::Engine(EngineError::InsufficientProof(ref s)), _)) - if s.contains("invalid step") => true, - _ => false, - }); + assert_insufficient_proof( + engine.verify_block_family(&header, &parent_header), + "invalid step" + ); // empty step with invalid signature let empty_steps = vec![SealedEmptyStep { signature: 0.into(), step: 1 }]; - header.set_seal(vec![ - encode(&2usize), - encode(&(&*signature as &[u8])), - ::rlp::encode_list(&empty_steps), - ]); + set_empty_steps_seal(&mut header, 2, &signature, &empty_steps); - assert!(match engine.verify_block_family(&header, &parent_header) { - Err(Error(ErrorKind::Engine(EngineError::InsufficientProof(ref s)), _)) - if s.contains("invalid empty step proof") => true, - _ => false, - }); + assert_insufficient_proof( + engine.verify_block_family(&header, &parent_header), + "invalid empty step proof" + ); // empty step with valid signature from incorrect proposer for step engine.set_signer(tap.clone(), addr1, "1".into()); let empty_steps = vec![sealed_empty_step(engine, 1, &parent_header.hash())]; - header.set_seal(vec![ - encode(&2usize), - encode(&(&*signature as &[u8])), - ::rlp::encode_list(&empty_steps), - ]); + set_empty_steps_seal(&mut header, 2, &signature, &empty_steps); - assert!(match engine.verify_block_family(&header, &parent_header) { - Err(Error(ErrorKind::Engine(EngineError::InsufficientProof(ref s)), _)) - if s.contains("invalid empty step proof") => true, - _ => false, - }); + assert_insufficient_proof( + engine.verify_block_family(&header, &parent_header), + "invalid empty step proof" + ); // valid empty steps engine.set_signer(tap.clone(), addr1, "1".into()); @@ -2100,11 +2096,7 @@ mod tests { let empty_steps = vec![empty_step2, empty_step3]; header.set_difficulty(calculate_score(0, 4, 2)); let signature = tap.sign(addr1, Some("1".into()), header.bare_hash()).unwrap(); - header.set_seal(vec![ - encode(&4usize), - encode(&(&*signature as &[u8])), - ::rlp::encode_list(&empty_steps), - ]); + set_empty_steps_seal(&mut header, 4, &signature, &empty_steps); assert!(engine.verify_block_family(&header, &parent_header).is_ok()); } @@ -2216,28 +2208,11 @@ mod tests { #[test] fn test_empty_steps() { - let last_benign = Arc::new(AtomicUsize::new(0)); - let params = AuthorityRoundParams { - step_duration: 4, - start_step: Some(1), - validators: Box::new(TestSet::new(Default::default(), last_benign.clone())), - validate_score_transition: 0, - validate_step_transition: 0, - immediate_transitions: true, - maximum_uncle_count_transition: 0, - maximum_uncle_count: 0, - empty_steps_transition: 0, - maximum_empty_steps: 10, - block_reward: Default::default(), - block_reward_contract_transition: 0, - block_reward_contract: Default::default(), - }; - - let mut c_params = ::spec::CommonParams::default(); - c_params.gas_limit_bound_divisor = 5.into(); - let machine = ::machine::EthereumMachine::regular(c_params, Default::default()); - let engine = AuthorityRound::new(params, machine).unwrap(); - + let engine = aura(|p| { + p.step_duration = 4; + p.empty_steps_transition = 0; + p.maximum_empty_steps = 0; + }); let parent_hash: H256 = 1.into(); let signature = H520::default(); @@ -2261,4 +2236,85 @@ mod tests { assert_eq!(engine.empty_steps(0, 3, parent_hash), vec![]); assert_eq!(engine.empty_steps(0, 4, parent_hash), vec![step(3)]); } + + #[test] + fn should_reject_duplicate_empty_steps() { + // given + let (_spec, tap, accounts) = setup_empty_steps(); + let engine = aura(|p| { + p.validators = Box::new(SimpleList::new(accounts.clone())); + p.step_duration = 4; + p.empty_steps_transition = 0; + p.maximum_empty_steps = 0; + }); + + let mut parent = Header::default(); + parent.set_seal(vec![encode(&0usize)]); + + let mut header = Header::default(); + header.set_number(parent.number() + 1); + header.set_parent_hash(parent.hash()); + header.set_author(accounts[0]); + + // when + engine.set_signer(tap.clone(), accounts[1], "0".into()); + let empty_steps = vec![ + sealed_empty_step(&*engine, 1, &parent.hash()), + sealed_empty_step(&*engine, 1, &parent.hash()), + ]; + let step = 2; + let signature = tap.sign(accounts[0], Some("1".into()), header.bare_hash()).unwrap(); + set_empty_steps_seal(&mut header, step, &signature, &empty_steps); + header.set_difficulty(calculate_score(0, step, empty_steps.len())); + + // then + assert_insufficient_proof( + engine.verify_block_family(&header, &parent), + "duplicate empty step" + ); + } + + #[test] + fn should_reject_empty_steps_out_of_order() { + // given + let (_spec, tap, accounts) = setup_empty_steps(); + let engine = aura(|p| { + p.validators = Box::new(SimpleList::new(accounts.clone())); + p.step_duration = 4; + p.empty_steps_transition = 0; + p.maximum_empty_steps = 0; + }); + + let mut parent = Header::default(); + parent.set_seal(vec![encode(&0usize)]); + + let mut header = Header::default(); + header.set_number(parent.number() + 1); + header.set_parent_hash(parent.hash()); + header.set_author(accounts[0]); + + // when + engine.set_signer(tap.clone(), accounts[1], "0".into()); + let es1 = sealed_empty_step(&*engine, 1, &parent.hash()); + engine.set_signer(tap.clone(), accounts[0], "1".into()); + let es2 = sealed_empty_step(&*engine, 2, &parent.hash()); + + let mut empty_steps = vec![es2, es1]; + + let step = 3; + let signature = tap.sign(accounts[1], Some("0".into()), header.bare_hash()).unwrap(); + set_empty_steps_seal(&mut header, step, &signature, &empty_steps); + header.set_difficulty(calculate_score(0, step, empty_steps.len())); + + // then make sure it's rejected because of the order + assert_insufficient_proof( + engine.verify_block_family(&header, &parent), + "unordered empty step" + ); + + // now try to fix the order + empty_steps.reverse(); + set_empty_steps_seal(&mut header, step, &signature, &empty_steps); + assert_eq!(engine.verify_block_family(&header, &parent).unwrap(), ()); + } } diff --git a/ethcore/src/engines/validator_set/test.rs b/ethcore/src/engines/validator_set/test.rs index 6459803d1..9650526e5 100644 --- a/ethcore/src/engines/validator_set/test.rs +++ b/ethcore/src/engines/validator_set/test.rs @@ -34,12 +34,18 @@ pub struct TestSet { last_benign: Arc, } +impl Default for TestSet { + fn default() -> Self { + TestSet::new(Default::default(), Default::default()) + } +} + impl TestSet { pub fn new(last_malicious: Arc, last_benign: Arc) -> Self { TestSet { validator: SimpleList::new(vec![Address::from_str("7d577a597b2742b498cb5cf0c26cdcd726d39e6e").unwrap()]), - last_malicious: last_malicious, - last_benign: last_benign, + last_malicious, + last_benign, } } } diff --git a/json/src/spec/authority_round.rs b/json/src/spec/authority_round.rs index 2ee38c49c..a1adbef12 100644 --- a/json/src/spec/authority_round.rs +++ b/json/src/spec/authority_round.rs @@ -56,6 +56,8 @@ pub struct AuthorityRoundParams { pub empty_steps_transition: Option, /// Maximum number of accepted empty steps. pub maximum_empty_steps: Option, + /// Strict validation of empty steps transition block. + pub strict_empty_steps_transition: Option, } /// Authority engine deserialization.