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

@@ -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<ethjson::spec::AuthorityRoundParams> 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<EthereumMachine> 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<usize, Error> {
+				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<EthereumMachine> 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: F) -> Arc<AuthorityRound> 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 {
+		let aura = aura(|p| {
-			step_duration: 1,
+			p.validators = Box::new(TestSet::new(Default::default(), last_benign.clone()));
-			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 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 aura = aura(|params| {
-		let params = AuthorityRoundParams {
+			params.maximum_uncle_count_transition = 1;
-			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()
-		};
 
 		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));
+		aura(|params| {
-		let params = AuthorityRoundParams {
+			params.step_duration = 0;
-			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();
 	}
 
 	fn setup_empty_steps() -> (Spec, Arc<AccountProvider>, Vec<Address>) {
@@ -1849,6 +1843,23 @@ mod tests {
 		SealedEmptyStep { signature, step }
 	}
 
+	fn set_empty_steps_seal(header: &mut Header, step: u64, block_signature: &ethkey::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<T: ::std::fmt::Debug>(result: Result<T, Error>, 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![
+		set_empty_steps_seal(&mut header, 2, &signature, &empty_steps);
-			encode(&2usize),
-			encode(&(&*signature as &[u8])),
-			::rlp::encode_list(&empty_steps),
-		]);
 
-		assert!(match engine.verify_block_family(&header, &parent_header) {
+		assert_insufficient_proof(
-			Err(Error(ErrorKind::Engine(EngineError::InsufficientProof(ref s)), _))
+			engine.verify_block_family(&header, &parent_header),
-				if s.contains("invalid step") => true,
+			"invalid step"
-			_ => false,
+		);
-		});
 
 		// empty step with invalid signature
 		let empty_steps = vec![SealedEmptyStep { signature: 0.into(), step: 1 }];
-		header.set_seal(vec![
+		set_empty_steps_seal(&mut header, 2, &signature, &empty_steps);
-			encode(&2usize),
-			encode(&(&*signature as &[u8])),
-			::rlp::encode_list(&empty_steps),
-		]);
 
-		assert!(match engine.verify_block_family(&header, &parent_header) {
+		assert_insufficient_proof(
-			Err(Error(ErrorKind::Engine(EngineError::InsufficientProof(ref s)), _))
+			engine.verify_block_family(&header, &parent_header),
-				if s.contains("invalid empty step proof") => true,
+			"invalid empty step proof"
-			_ => false,
+		);
-		});
 
 		// 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![
+		set_empty_steps_seal(&mut header, 2, &signature, &empty_steps);
-			encode(&2usize),
-			encode(&(&*signature as &[u8])),
-			::rlp::encode_list(&empty_steps),
-		]);
 
-		assert!(match engine.verify_block_family(&header, &parent_header) {
+		assert_insufficient_proof(
-			Err(Error(ErrorKind::Engine(EngineError::InsufficientProof(ref s)), _))
+			engine.verify_block_family(&header, &parent_header),
-				if s.contains("invalid empty step proof") => true,
+			"invalid empty step proof"
-			_ => false,
+		);
-		});
 
 		// 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![
+		set_empty_steps_seal(&mut header, 4, &signature, &empty_steps);
-			encode(&4usize),
-			encode(&(&*signature as &[u8])),
-			::rlp::encode_list(&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 engine = aura(|p| {
-		let params = AuthorityRoundParams {
+			p.step_duration = 4;
-			step_duration: 4,
+			p.empty_steps_transition = 0;
-			start_step: Some(1),
+			p.maximum_empty_steps = 0;
-			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 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(), ());
+	}
 }

ethcore/src/engines/validator_set/test.rs

@@ -34,12 +34,18 @@ pub struct TestSet {
 	last_benign: Arc<AtomicUsize>,
 }
 
+impl Default for TestSet {
+	fn default() -> Self {
+		TestSet::new(Default::default(), Default::default())
+	}
+}
+
 impl TestSet {
 	pub fn new(last_malicious: Arc<AtomicUsize>, last_benign: Arc<AtomicUsize>) -> Self {
 		TestSet {
 			validator: SimpleList::new(vec![Address::from_str("7d577a597b2742b498cb5cf0c26cdcd726d39e6e").unwrap()]),
-			last_malicious: last_malicious,
+			last_malicious,
-			last_benign: last_benign,
+			last_benign,
 		}
 	}
 }

json/src/spec/authority_round.rs

@@ -56,6 +56,8 @@ pub struct AuthorityRoundParams {
 	pub empty_steps_transition: Option<Uint>,
 	/// Maximum number of accepted empty steps.
 	pub maximum_empty_steps: Option<Uint>,
+	/// Strict validation of empty steps transition block.
+	pub strict_empty_steps_transition: Option<Uint>,
 }
 
 /// Authority engine deserialization.