From 7d4e4c7a62bfc74594cf4c752d06db08dc2a18a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Thu, 21 Dec 2017 15:01:05 +0100 Subject: [PATCH] Mark future blocks as temporarily invalid. --- ethcore/src/engines/authority_round/mod.rs | 84 ++++++++++++------- ethcore/src/engines/validator_set/contract.rs | 18 +++- ethcore/src/error.rs | 3 + ethcore/src/verification/queue/mod.rs | 2 +- ethcore/src/verification/verification.rs | 29 +++++-- 5 files changed, 94 insertions(+), 42 deletions(-) diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index d9e1cb3b7..fba2eeb12 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -112,16 +112,12 @@ impl Step { let now = unix_now(); let expected_seconds = (self.load() as u64) .checked_add(1) - .and_then(|ctr| ctr.checked_mul(self.duration as u64)); + .and_then(|ctr| ctr.checked_mul(self.duration as u64)) + .map(Duration::from_secs); + match expected_seconds { - Some(secs) => { - let step_end = Duration::from_secs(secs); - if step_end > now { - step_end - now - } else { - Duration::from_secs(0) - } - }, + 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); @@ -130,6 +126,7 @@ impl Step { } } + fn increment(&self) { use std::usize; // fetch_add won't panic on overflow but will rather wrap @@ -141,19 +138,40 @@ impl Step { } } + fn calibrate(&self) { if self.calibrate { let new_step = unix_now().as_secs() / (self.duration as u64); 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>> { + 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(()) } } } @@ -343,22 +361,28 @@ fn verify_external(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())?; + 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(()) + 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(()) + } } } } diff --git a/ethcore/src/engines/validator_set/contract.rs b/ethcore/src/engines/validator_set/contract.rs index 54e86b98b..05b85eba4 100644 --- a/ethcore/src/engines/validator_set/contract.rs +++ b/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()); diff --git a/ethcore/src/error.rs b/ethcore/src/error.rs index 49b873a47..6c7dbfa19 100644 --- a/ethcore/src/error.rs +++ b/ethcore/src/error.rs @@ -167,6 +167,8 @@ pub enum BlockError { InvalidReceiptsRoot(Mismatch), /// Timestamp header field is invalid. InvalidTimestamp(OutOfBounds), + /// Timestamp header field is too far in future. + TemporarilyInvalid(OutOfBounds), /// Log bloom header field is invalid. InvalidLogBloom(Mismatch), /// Parent hash field of header is invalid; this is an invalid error indicating a logic flaw in the codebase. @@ -212,6 +214,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), diff --git a/ethcore/src/verification/queue/mod.rs b/ethcore/src/verification/queue/mod.rs index bf7c9ef5c..7b3fddd38 100644 --- a/ethcore/src/verification/queue/mod.rs +++ b/ethcore/src/verification/queue/mod.rs @@ -505,7 +505,7 @@ impl VerificationQueue { 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(ref e)) if e.max.is_some() => {}, _ => { self.verification.bad.lock().insert(h.clone()); } diff --git a/ethcore/src/verification/verification.rs b/ethcore/src/verification/verification.rs index a418236ed..05a96e354 100644 --- a/ethcore/src/verification/verification.rs +++ b/ethcore/src/verification/verification.rs @@ -273,11 +273,20 @@ pub fn verify_header_params(header: &Header, engine: &EthEngine, is_full: bool) } if is_full { - let max_time = get_time().sec as u64 + 15; - 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(()) } @@ -359,11 +368,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), } } @@ -643,11 +654,11 @@ 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(); 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)); + check_fail_timestamp(basic_test(&create_test_block_with_data(&header, &good_transactions, &good_uncles), engine), true); header = good.clone(); header.set_timestamp(get_time().sec as u64 + 10);