diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index bc65a4ffc..1405b3931 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -49,12 +49,16 @@ mod finality; pub struct AuthorityRoundParams { /// Gas limit divisor. pub gas_limit_bound_divisor: U256, - /// Time to wait before next block or authority switching. - pub step_duration: Duration, /// Block reward. pub block_reward: U256, /// Namereg contract address. pub registrar: Address, + /// Time to wait before next block or authority switching, + /// in seconds. + /// + /// Deliberately typed as u16 as too high of a value leads + /// to slow block issuance. + pub step_duration: u16, /// Starting step, pub start_step: Option, /// Valid validators. @@ -73,11 +77,18 @@ pub struct AuthorityRoundParams { pub maximum_uncle_count: usize, } +const U16_MAX: usize = ::std::u16::MAX as usize; + impl From for AuthorityRoundParams { fn from(p: ethjson::spec::AuthorityRoundParams) -> Self { + let mut step_duration_usize: usize = p.step_duration.into(); + if step_duration_usize > U16_MAX { + step_duration_usize = U16_MAX; + warn!(target: "engine", "step_duration is too high ({}), setting it to {}", step_duration_usize, U16_MAX); + } AuthorityRoundParams { gas_limit_bound_divisor: p.gas_limit_bound_divisor.into(), - step_duration: Duration::from_secs(p.step_duration.into()), + step_duration: step_duration_usize as u16, validators: new_validator_set(p.validators), block_reward: p.block_reward.map_or_else(U256::zero, Into::into), registrar: p.registrar.map_or_else(Address::new, Into::into), @@ -97,36 +108,74 @@ impl From for AuthorityRoundParams { struct Step { calibrate: bool, // whether calibration is enabled. inner: AtomicUsize, - duration: Duration, + duration: u16, } 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); + use std::usize; + // fetch_add won't panic on overflow but will rather wrap + // around, leading to zero as the step counter, which might + // lead to unexpected situations, so it's better to shut down. + if self.inner.fetch_add(1, AtomicOrdering::SeqCst) == usize::MAX { + error!(target: "engine", "Step counter is too high: {}, aborting", usize::MAX); + panic!("step counter is too high: {}", usize::MAX); + } + } + fn calibrate(&self) { if self.calibrate { - let new_step = unix_now().as_secs() / self.duration.as_secs(); + 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 REJECTED_STEP_DRIFT: usize = 4; + + // Verify if the step is correct. + if given <= self.load() { + 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 { + let d = self.duration as u64; + Err(Some(OutOfBounds { + min: None, + max: Some(d * current as u64), + found: d * given as u64, + })) } else { - false + Ok(()) } } } @@ -322,22 +371,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(()) + } } } } @@ -370,8 +425,12 @@ impl AsMillis for Duration { impl AuthorityRound { /// Create a new instance of AuthorityRound engine. pub fn new(params: CommonParams, our_params: AuthorityRoundParams, builtins: BTreeMap) -> Result, Error> { + if our_params.step_duration == 0 { + error!(target: "engine", "Authority Round step duration can't be zero, aborting"); + panic!("authority_round: step duration can't be zero") + } let should_timeout = our_params.start_step.is_none(); - let initial_step = our_params.start_step.unwrap_or_else(|| (unix_now().as_secs() / our_params.step_duration.as_secs())) as usize; + let initial_step = our_params.start_step.unwrap_or_else(|| (unix_now().as_secs() / (our_params.step_duration as u64))) as usize; let engine = Arc::new( AuthorityRound { params: params, @@ -519,6 +578,7 @@ impl Engine for AuthorityRound { .expect("Header has been verified; qed").into(); let step = self.step.load(); + let expected_diff = calculate_score(parent_step, step.into()); if header.difficulty() != &expected_diff { @@ -558,6 +618,13 @@ impl Engine for AuthorityRound { }; if is_step_proposer(validators, header.parent_hash(), step, header.author()) { + // this is guarded against by `can_propose` unless the block was signed + // on the same step (implies same key) and on a different node. + if parent_step == step.into() { + warn!("Attempted to seal block on the same step as parent. Is this authority sealing with more than one node?"); + return Seal::None; + } + if let Ok(signature) = self.sign(header.bare_hash()) { trace!(target: "engine", "generate_seal: Issuing a block for step {}.", step); @@ -1099,9 +1166,9 @@ mod tests { let last_benign = Arc::new(AtomicUsize::new(0)); let params = AuthorityRoundParams { gas_limit_bound_divisor: U256::from_str("400").unwrap(), - step_duration: Default::default(), block_reward: Default::default(), registrar: Default::default(), + step_duration: 1, start_step: Some(1), validators: Box::new(TestSet::new(Default::default(), last_benign.clone())), validate_score_transition: 0, @@ -1137,9 +1204,9 @@ mod tests { let last_benign = Arc::new(AtomicUsize::new(0)); let params = AuthorityRoundParams { gas_limit_bound_divisor: 5.into(), - step_duration: Default::default(), block_reward: Default::default(), registrar: Default::default(), + step_duration: 1, start_step: Some(1), validators: Box::new(TestSet::new(Default::default(), last_benign.clone())), validate_score_transition: 0, @@ -1156,4 +1223,50 @@ mod tests { assert_eq!(aura.maximum_uncle_count(1), 0); assert_eq!(aura.maximum_uncle_count(100), 0); } + + #[test] + #[should_panic(expected="counter is too high")] + fn test_counter_increment_too_high() { + use super::Step; + let step = Step { + calibrate: false, + inner: AtomicUsize::new(::std::usize::MAX), + duration: 1, + }; + step.increment(); + } + + #[test] + #[should_panic(expected="counter is too high")] + fn test_counter_duration_remaining_too_high() { + use super::Step; + let step = Step { + calibrate: false, + inner: AtomicUsize::new(::std::usize::MAX), + duration: 1, + }; + step.duration_remaining(); + } + + #[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, + block_reward: Default::default(), + eip155_transition: 0, + registrar: Default::default(), + gas_limit_bound_divisor: 5.into(), + }; + + AuthorityRound::new(Default::default(), params, Default::default()).unwrap(); + } } diff --git a/ethcore/src/engines/validator_set/contract.rs b/ethcore/src/engines/validator_set/contract.rs index 46f5c8512..8af9a5a7c 100644 --- a/ethcore/src/engines/validator_set/contract.rs +++ b/ethcore/src/engines/validator_set/contract.rs @@ -159,22 +159,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, None).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, None).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, None).is_err()); diff --git a/ethcore/src/error.rs b/ethcore/src/error.rs index 2d908cdb6..01f9a6eb6 100644 --- a/ethcore/src/error.rs +++ b/ethcore/src/error.rs @@ -157,6 +157,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. @@ -202,6 +204,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 7e9a70f7c..209f64f2e 100644 --- a/ethcore/src/verification/queue/mod.rs +++ b/ethcore/src/verification/queue/mod.rs @@ -500,7 +500,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(_)) => {}, _ => { self.verification.bad.lock().insert(h.clone()); } diff --git a/ethcore/src/verification/verification.rs b/ethcore/src/verification/verification.rs index d220d4220..a03abad1d 100644 --- a/ethcore/src/verification/verification.rs +++ b/ethcore/src/verification/verification.rs @@ -230,11 +230,20 @@ pub fn verify_header_params(header: &Header, engine: &Engine, is_full: bool) -> return Err(From::from(BlockError::ExtraDataOutOfBounds(OutOfBounds { min: None, max: Some(maximum_extra_data_size), found: header.extra_data().len() }))); } 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(()) } @@ -298,11 +307,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), } } @@ -561,11 +572,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); diff --git a/json/src/spec/authority_round.rs b/json/src/spec/authority_round.rs index 67a1d8a68..d024ddf34 100644 --- a/json/src/spec/authority_round.rs +++ b/json/src/spec/authority_round.rs @@ -26,7 +26,7 @@ pub struct AuthorityRoundParams { /// Gas limit divisor. #[serde(rename="gasLimitBoundDivisor")] pub gas_limit_bound_divisor: Uint, - /// Block duration. + /// Block duration, in seconds. #[serde(rename="stepDuration")] pub step_duration: Uint, /// Valid authorities