diff --git a/ethcore/src/engines/clique/block_state.rs b/ethcore/src/engines/clique/block_state.rs index 2df1d869fc1..32d9bbfdf4d 100644 --- a/ethcore/src/engines/clique/block_state.rs +++ b/ethcore/src/engines/clique/block_state.rs @@ -28,6 +28,9 @@ use types::BlockNumber; use types::header::Header; use unexpected::Mismatch; +#[cfg(not(feature = "time_checked_add"))] +use time_utils::CheckedSystemTime; + /// Type that keeps track of the state for a given vote // Votes that go against the proposal aren't counted since it's equivalent to not voting #[derive(Copy, Clone, Debug, PartialEq, PartialOrd)] @@ -262,16 +265,30 @@ impl CliqueBlockState { Ok(()) } - pub fn calc_next_timestamp(&mut self, header: &Header, period: u64) { - let base_time = UNIX_EPOCH + Duration::from_secs(header.timestamp()); + /// Calculate the next timestamp for `inturn` and `noturn` fails if any of them can't be represented as + /// `SystemTime` + // TODO(niklasad1): refactor this method to be in constructor of `CliqueBlockState` instead. + // This is a quite bad API because we must mutate both variables even when already `inturn` fails + // That's why we can't return early and must have the `if-else` in the end + pub fn calc_next_timestamp(&mut self, timestamp: u64, period: u64) -> Result<(), Error> { + let inturn = UNIX_EPOCH.checked_add(Duration::from_secs(timestamp.saturating_add(period))); - self.next_timestamp_inturn = Some(base_time + Duration::from_secs(period)); + self.next_timestamp_inturn = inturn; let delay = Duration::from_millis( rand::thread_rng().gen_range(0u64, (self.signers.len() as u64 / 2 + 1) * SIGNING_DELAY_NOTURN_MS)); - self.next_timestamp_noturn = Some(base_time + Duration::from_secs(period) + delay); + self.next_timestamp_noturn = inturn.map(|inturn| { + inturn + delay + }); + + if self.next_timestamp_inturn.is_some() && self.next_timestamp_noturn.is_some() { + Ok(()) + } else { + Err(BlockError::TimestampOverflow)? + } } + /// Returns true if the block difficulty should be `inturn` pub fn is_inturn(&self, current_block_number: u64, author: &Address) -> bool { if let Some(pos) = self.signers.iter().position(|x| *author == *x) { return current_block_number % self.signers.len() as u64 == pos as u64; @@ -279,12 +296,13 @@ impl CliqueBlockState { false } + /// Returns whether the signer is authorized to sign a block pub fn is_authorized(&self, author: &Address) -> bool { self.signers.binary_search(author).is_ok() && !self.recent_signers.contains(author) } - // Returns whether it makes sense to cast the specified vote in the - // current state (e.g. don't try to add an already authorized signer). + /// Returns whether it makes sense to cast the specified vote in the + /// current state (e.g. don't try to add an already authorized signer). pub fn is_valid_vote(&self, address: &Address, vote_type: VoteType) -> bool { let in_signer = self.signers.binary_search(address).is_ok(); match vote_type { @@ -293,11 +311,12 @@ impl CliqueBlockState { } } + /// Returns the list of current signers pub fn signers(&self) -> &Vec
{ return &self.signers; } - // Note this method will always return `true` but it is intended for a unifrom `API` + // Note this method will always return `true` but it is intended for a uniform `API` fn add_vote(&mut self, pending_vote: PendingVote, kind: VoteType) -> bool { self.votes.entry(pending_vote) diff --git a/ethcore/src/engines/clique/mod.rs b/ethcore/src/engines/clique/mod.rs index 46c72ddf530..82e174a6b87 100644 --- a/ethcore/src/engines/clique/mod.rs +++ b/ethcore/src/engines/clique/mod.rs @@ -79,6 +79,9 @@ use unexpected::{Mismatch, OutOfBounds}; use types::BlockNumber; use types::header::{ExtendedHeader, Header}; +#[cfg(not(feature = "time_checked_add"))] +use time_utils::CheckedSystemTime; + use self::block_state::CliqueBlockState; use self::params::CliqueParams; use self::step_service::StepService; @@ -241,7 +244,8 @@ impl Clique { let mut state = CliqueBlockState::new( extract_signers(header)?); - state.calc_next_timestamp(header, self.period); + // TODO(niklasad1): refactor to perform this check in the `CliqueBlockState` constructor instead + state.calc_next_timestamp(header.timestamp(), self.period)?; Ok(state) } @@ -326,7 +330,7 @@ impl Clique { for item in chain { new_state.apply(item, false)?; } - new_state.calc_next_timestamp(header, self.period); + new_state.calc_next_timestamp(header.timestamp(), self.period)?; block_state_by_hash.insert(header.hash(), new_state.clone()); let elapsed = backfill_start.elapsed(); @@ -436,7 +440,7 @@ impl Engine for Clique { // locally sealed block don't go through valid_block_family(), so we have to record state here. let mut new_state = state.clone(); new_state.apply(&header, is_checkpoint)?; - new_state.calc_next_timestamp(&header, self.period); + new_state.calc_next_timestamp(header.timestamp(), self.period)?; self.block_state_by_hash.write().insert(header.hash(), new_state); trace!(target: "engine", "on_seal_block: finished, final header: {:?}", header); @@ -532,15 +536,23 @@ impl Engine for Clique { // Don't waste time checking blocks from the future { - // TODO(niklasad): checked_add - let limit = SystemTime::now().duration_since(UNIX_EPOCH).unwrap_or_default() + Duration::from_secs(self.period); + let limit = SystemTime::now().checked_add(Duration::from_secs(self.period)) + .ok_or(BlockError::TimestampOverflow)?; + + // This should succeed under the contraints that the system clock works + let limit_as_dur = limit.duration_since(UNIX_EPOCH).map_err(|e| { + Box::new(format!("Converting SystemTime to Duration failed: {}", e)) + })?; + let hdr = Duration::from_secs(header.timestamp()); - if hdr > limit { - return Err(BlockError::TemporarilyInvalid(OutOfBounds { + if hdr > limit_as_dur { + let found = UNIX_EPOCH.checked_add(hdr).ok_or(BlockError::TimestampOverflow)?; + + Err(BlockError::TemporarilyInvalid(OutOfBounds { min: None, - max: Some(UNIX_EPOCH + limit), - found: UNIX_EPOCH + Duration::from_secs(header.timestamp()), - }))?; + max: Some(limit), + found, + }))? } } @@ -643,11 +655,16 @@ impl Engine for Clique { } // Ensure that the block's timestamp isn't too close to it's parent - if parent.timestamp().saturating_add(self.period) > header.timestamp() { + let limit = parent.timestamp().saturating_add(self.period); + if limit > header.timestamp() { + let max = UNIX_EPOCH.checked_add(Duration::from_secs(header.timestamp())); + let found = UNIX_EPOCH.checked_add(Duration::from_secs(limit)) + .ok_or(BlockError::TimestampOverflow)?; + Err(BlockError::InvalidTimestamp(OutOfBounds { - min: Some(UNIX_EPOCH + Duration::from_secs(parent.timestamp().saturating_add(self.period))), - max: None, - found: UNIX_EPOCH + Duration::from_secs(header.timestamp()) + min: None, + max, + found, }))? } @@ -656,7 +673,7 @@ impl Engine for Clique { // Try to apply current state, apply() will further check signer and recent signer. let mut new_state = parent_state.clone(); new_state.apply(header, header.number() % self.epoch_length == 0)?; - new_state.calc_next_timestamp(header, self.period); + new_state.calc_next_timestamp(header.timestamp(), self.period)?; self.block_state_by_hash.write().insert(header.hash(), new_state); Ok(()) @@ -664,7 +681,7 @@ impl Engine for Clique { fn genesis_epoch_data(&self, header: &Header, _call: &Call) -> Result, String> { let mut state = self.new_checkpoint_state(header).expect("Unable to parse genesis data."); - state.calc_next_timestamp(header, self.period); + state.calc_next_timestamp(header.timestamp(), self.period).map_err(|e| format!("{}", e))?; self.block_state_by_hash.write().insert(header.hash(), state); // no proof.