Merge pull request #7368 from paritytech/td-future-blocks

Wait for future blocks in AuRa

ethcore/src/engines/authority_round/mod.rs

@@ -93,29 +93,59 @@ impl Step {
 	fn load(&self) -> usize { self.inner.load(AtomicOrdering::SeqCst) }
 	fn duration_remaining(&self) -> Duration {
 		let now = unix_now();
-		let step_end = self.duration * (self.load() as u32 + 1);
-		if step_end > now {
-			step_end - now
-		} else {
-			Duration::from_secs(0)
+		let expected_seconds = (self.load() as u64)
+			.checked_add(1)
+			.and_then(|ctr| ctr.checked_mul(self.duration as u64))
+			.map(Duration::from_secs);
+
+		match expected_seconds {
+			Some(step_end) if step_end > now => step_end - now,
+			Some(_) => Duration::from_secs(0),
+			None => {
+				let ctr = self.load();
+				error!(target: "engine", "Step counter is too high: {}, aborting", ctr);
+				panic!("step counter is too high: {}", ctr)
+			},
 		}
 	}
+
 	fn increment(&self) {
 		self.inner.fetch_add(1, AtomicOrdering::SeqCst);
 	}
+
 	fn calibrate(&self) {
 		if self.calibrate {
 			let new_step = unix_now().as_secs() / self.duration.as_secs();
 			self.inner.store(new_step as usize, AtomicOrdering::SeqCst);
 		}
 	}
-	fn is_future(&self, given: usize) -> bool {
-		if given > self.load() + 1 {
-			// Make absolutely sure that the given step is correct.
-			self.calibrate();
-			given > self.load() + 1
+
+	fn check_future(&self, given: usize) -> Result<(), Option<OutOfBounds<u64>>> {
+		const VALID_STEP_DRIFT: usize = 1;
+		const REJECTED_STEP_DRIFT: usize = 4;
+
+		// Give one step slack if step is lagging, double vote is still not possible.
+		if given <= self.load() + VALID_STEP_DRIFT {
+			return Ok(());
+		}
+
+		// Make absolutely sure that the given step is incorrect.
+		self.calibrate();
+		let current = self.load();
+
+		// reject blocks too far in the future
+		if given > current + REJECTED_STEP_DRIFT {
+			Err(None)
+		// wait a bit for blocks in near future
+		} else if given > current + VALID_STEP_DRIFT {
+			let d = self.duration as u64;
+			Err(Some(OutOfBounds {
+				min: None,
+				max: Some(d * (current + VALID_STEP_DRIFT) as u64),
+				found: d * given as u64,
+			}))
 		} else {
-			false
+			Ok(())
 		}
 	}
 }
@@ -298,22 +328,28 @@ fn verify_external<F: Fn(Report)>(header: &Header, validators: &ValidatorSet, st
 {
 	let header_step = header_step(header)?;
 
-	// Give one step slack if step is lagging, double vote is still not possible.
-	if step.is_future(header_step) {
-		trace!(target: "engine", "verify_block_external: block from the future");
-		report(Report::Benign(*header.author(), header.number()));
-		Err(BlockError::InvalidSeal)?
-	} else {
-		let proposer_signature = header_signature(header)?;
-		let correct_proposer = validators.get(header.parent_hash(), header_step);
-		let is_invalid_proposer = *header.author() != correct_proposer ||
-			!verify_address(&correct_proposer, &proposer_signature, &header.bare_hash())?;
-
-		if is_invalid_proposer {
-			trace!(target: "engine", "verify_block_external: bad proposer for step: {}", header_step);
-			Err(EngineError::NotProposer(Mismatch { expected: correct_proposer, found: header.author().clone() }))?
-		} else {
-			Ok(())
+	match step.check_future(header_step) {
+		Err(None) => {
+			trace!(target: "engine", "verify_block_external: block from the future");
+			report(Report::Benign(*header.author(), header.number()));
+			return Err(BlockError::InvalidSeal.into())
+		},
+		Err(Some(oob)) => {
+			trace!(target: "engine", "verify_block_external: block too early");
+			return Err(BlockError::TemporarilyInvalid(oob).into())
+		},
+		Ok(_) => {
+			let proposer_signature = header_signature(header)?;
+			let correct_proposer = validators.get(header.parent_hash(), header_step);
+			let is_invalid_proposer = *header.author() != correct_proposer ||
+				!verify_address(&correct_proposer, &proposer_signature, &header.bare_hash())?;
+
+			if is_invalid_proposer {
+				trace!(target: "engine", "verify_block_external: bad proposer for step: {}", header_step);
+				Err(EngineError::NotProposer(Mismatch { expected: correct_proposer, found: header.author().clone() }))?
+			} else {
+				Ok(())
+			}
 		}
 	}
 }

ethcore/src/engines/validator_set/contract.rs

@@ -171,22 +171,36 @@ mod tests {
 
 		// Make sure reporting can be done.
 		client.miner().set_gas_floor_target(1_000_000.into());
-
 		client.miner().set_engine_signer(v1, "".into()).unwrap();
+
+		// Check a block that is a bit in future, reject it but don't report the validator.
 		let mut header = Header::default();
 		let seal = vec![encode(&5u8).into_vec(), encode(&(&H520::default() as &[u8])).into_vec()];
 		header.set_seal(seal);
 		header.set_author(v1);
 		header.set_number(2);
 		header.set_parent_hash(client.chain_info().best_block_hash);
+		assert!(client.engine().verify_block_external(&header).is_err());
+		client.engine().step();
+		assert_eq!(client.chain_info().best_block_number, 0);
 
+		// Now create one that is more in future. That one should be rejected and validator should be reported.
+		let mut header = Header::default();
+		let seal = vec![encode(&8u8).into_vec(), encode(&(&H520::default() as &[u8])).into_vec()];
+		header.set_seal(seal);
+		header.set_author(v1);
+		header.set_number(2);
+		header.set_parent_hash(client.chain_info().best_block_hash);
 		// `reportBenign` when the designated proposer releases block from the future (bad clock).
 		assert!(client.engine().verify_block_external(&header).is_err());
 		// Seal a block.
 		client.engine().step();
 		assert_eq!(client.chain_info().best_block_number, 1);
 		// Check if the unresponsive validator is `disliked`.
-		assert_eq!(client.call_contract(BlockId::Latest, validator_contract, "d8f2e0bf".from_hex().unwrap()).unwrap().to_hex(), "0000000000000000000000007d577a597b2742b498cb5cf0c26cdcd726d39e6e");
+		assert_eq!(
+			client.call_contract(BlockId::Latest, validator_contract, "d8f2e0bf".from_hex().unwrap()).unwrap().to_hex(),
+			"0000000000000000000000007d577a597b2742b498cb5cf0c26cdcd726d39e6e"
+		);
 		// Simulate a misbehaving validator by handling a double proposal.
 		let header = client.best_block_header().decode();
 		assert!(client.engine().verify_block_family(&header, &header).is_err());

ethcore/src/error.rs

@@ -168,6 +168,8 @@ pub enum BlockError {
 	InvalidReceiptsRoot(Mismatch<H256>),
 	/// Timestamp header field is invalid.
 	InvalidTimestamp(OutOfBounds<u64>),
+	/// Timestamp header field is too far in future.
+	TemporarilyInvalid(OutOfBounds<u64>),
 	/// Log bloom header field is invalid.
 	InvalidLogBloom(Mismatch<LogBloom>),
 	/// Parent hash field of header is invalid; this is an invalid error indicating a logic flaw in the codebase.
@@ -213,6 +215,7 @@ impl fmt::Display for BlockError {
 			InvalidGasLimit(ref oob) => format!("Invalid gas limit: {}", oob),
 			InvalidReceiptsRoot(ref mis) => format!("Invalid receipts trie root in header: {}", mis),
 			InvalidTimestamp(ref oob) => format!("Invalid timestamp in header: {}", oob),
+			TemporarilyInvalid(ref oob) => format!("Future timestamp in header: {}", oob),
 			InvalidLogBloom(ref oob) => format!("Invalid log bloom in header: {}", oob),
 			InvalidParentHash(ref mis) => format!("Invalid parent hash: {}", mis),
 			InvalidNumber(ref mis) => format!("Invalid number in header: {}", mis),

ethcore/src/verification/queue/mod.rs

@@ -505,7 +505,7 @@ impl<K: Kind> VerificationQueue<K> {
 			Err(err) => {
 				match err {
 					// Don't mark future blocks as bad.
-					Error::Block(BlockError::InvalidTimestamp(ref e)) if e.max.is_some() => {},
+					Error::Block(BlockError::TemporarilyInvalid(_)) => {},
 					_ => {
 						self.verification.bad.lock().insert(h.clone());
 					}

ethcore/src/verification/verification.rs

@@ -268,11 +268,20 @@ pub fn verify_header_params(header: &Header, engine: &EthEngine, is_full: bool)
 	}
 
 	if is_full {
-		let max_time = get_time().sec as u64 + 30;
-		if header.timestamp() > max_time {
-			return Err(From::from(BlockError::InvalidTimestamp(OutOfBounds { max: Some(max_time), min: None, found: header.timestamp() })))
+		const ACCEPTABLE_DRIFT_SECS: u64 = 15;
+		let max_time = get_time().sec as u64 + ACCEPTABLE_DRIFT_SECS;
+		let invalid_threshold = max_time + ACCEPTABLE_DRIFT_SECS * 9;
+		let timestamp = header.timestamp();
+
+		if timestamp > invalid_threshold {
+			return Err(From::from(BlockError::InvalidTimestamp(OutOfBounds { max: Some(max_time), min: None, found: timestamp })))
+		}
+
+		if timestamp > max_time {
+			return Err(From::from(BlockError::TemporarilyInvalid(OutOfBounds { max: Some(max_time), min: None, found: timestamp })))
 		}
 	}
+
 	Ok(())
 }
 
@@ -354,11 +363,13 @@ mod tests {
 		}
 	}
 
-	fn check_fail_timestamp(result: Result<(), Error>) {
+	fn check_fail_timestamp(result: Result<(), Error>, temp: bool) {
+		let name = if temp { "TemporarilyInvalid" } else { "InvalidTimestamp" };
 		match result {
-			Err(Error::Block(BlockError::InvalidTimestamp(_))) => (),
-			Err(other) => panic!("Block verification failed.\nExpected: InvalidTimestamp\nGot: {:?}", other),
-			Ok(_) => panic!("Block verification failed.\nExpected: InvalidTimestamp\nGot: Ok"),
+			Err(Error::Block(BlockError::InvalidTimestamp(_))) if !temp => (),
+			Err(Error::Block(BlockError::TemporarilyInvalid(_))) if temp => (),
+			Err(other) => panic!("Block verification failed.\nExpected: {}\nGot: {:?}", name, other),
+			Ok(_) => panic!("Block verification failed.\nExpected: {}\nGot: Ok", name),
 		}
 	}
 
@@ -638,11 +649,16 @@ mod tests {
 
 		header = good.clone();
 		header.set_timestamp(2450000000);
-		check_fail_timestamp(basic_test(&create_test_block_with_data(&header, &good_transactions, &good_uncles), engine));
+		check_fail_timestamp(basic_test(&create_test_block_with_data(&header, &good_transactions, &good_uncles), engine), false);
 
 		header = good.clone();
+<<<<<<< HEAD
 		header.set_timestamp(get_time().sec as u64 + 40);
 		check_fail_timestamp(basic_test(&create_test_block_with_data(&header, &good_transactions, &good_uncles), engine));
+=======
+		header.set_timestamp(get_time().sec as u64 + 20);
+		check_fail_timestamp(basic_test(&create_test_block_with_data(&header, &good_transactions, &good_uncles), engine), true);
+>>>>>>> f3e7d94... Merge pull request #7368 from paritytech/td-future-blocks
 
 		header = good.clone();
 		header.set_number(9);