-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(extract timestamp_checked_add
as lib)
#10383
Changes from all commits
ca67e1c
48b3b88
ae10434
4e07fea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,6 +47,9 @@ use types::header::{Header, ExtendedHeader}; | |
use types::ancestry_action::AncestryAction; | ||
use unexpected::{Mismatch, OutOfBounds}; | ||
|
||
#[cfg(not(time_checked_add))] | ||
use time_utils::CheckedSystemTime; | ||
|
||
mod finality; | ||
|
||
/// `AuthorityRound` params. | ||
|
@@ -574,8 +577,15 @@ fn verify_timestamp(step: &Step, header_step: u64) -> Result<(), BlockError> { | |
// NOTE This error might be returned only in early stage of verification (Stage 1). | ||
// Returning it further won't recover the sync process. | ||
trace!(target: "engine", "verify_timestamp: block too early"); | ||
let oob = oob.map(|n| SystemTime::now() + Duration::from_secs(n)); | ||
Err(BlockError::TemporarilyInvalid(oob).into()) | ||
|
||
let now = SystemTime::now(); | ||
let found = now.checked_add(Duration::from_secs(oob.found)).ok_or(BlockError::TimestampOverflow)?; | ||
let max = oob.max.and_then(|m| now.checked_add(Duration::from_secs(m))); | ||
let min = oob.min.and_then(|m| now.checked_add(Duration::from_secs(m))); | ||
|
||
let new_oob = OutOfBounds { min, max, found }; | ||
|
||
Err(BlockError::TemporarilyInvalid(new_oob).into()) | ||
}, | ||
Ok(_) => Ok(()), | ||
} | ||
|
@@ -611,6 +621,7 @@ fn combine_proofs(signal_number: BlockNumber, set_proof: &[u8], finality_proof: | |
stream.out() | ||
} | ||
|
||
|
||
fn destructure_proofs(combined: &[u8]) -> Result<(BlockNumber, &[u8], &[u8]), Error> { | ||
let rlp = Rlp::new(combined); | ||
Ok(( | ||
|
@@ -626,7 +637,7 @@ trait AsMillis { | |
|
||
impl AsMillis for Duration { | ||
fn as_millis(&self) -> u64 { | ||
self.as_secs()*1_000 + (self.subsec_nanos()/1_000_000) as u64 | ||
self.as_secs() * 1_000 + (self.subsec_nanos() / 1_000_000) as u64 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Duration::as_millis is available since rust 1.33, though it returns There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer to keep it out this PR. If people agree with Seems to be ok for the use case in |
||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ | |
// along with Parity Ethereum. If not, see <http://www.gnu.org/licenses/>. | ||
|
||
#![warn(missing_docs, unused_extern_crates)] | ||
#![cfg_attr(feature = "time_checked_add", feature(time_checked_add))] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did not find a use for this line (just tested stable and nightly with changing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sorpaas grumble: #10383 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can remove those if @sorpaas agree |
||
|
||
//! Ethcore library | ||
//! | ||
|
@@ -148,6 +149,9 @@ extern crate fetch; | |
#[cfg(all(test, feature = "price-info"))] | ||
extern crate parity_runtime; | ||
|
||
#[cfg(not(time_checked_add))] | ||
extern crate time_utils; | ||
|
||
pub mod block; | ||
pub mod builtin; | ||
pub mod client; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
[package] | ||
name = "time-utils" | ||
version = "0.1.0" | ||
authors = ["Parity Technologies <admin@parity.io>"] | ||
description = "Time utilities for checked arithmetic" | ||
license = "GPL3" | ||
edition = "2018" | ||
|
||
[dependencies] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
use std::time::{Duration, SystemTime, UNIX_EPOCH}; | ||
|
||
/// Temporary trait for `checked operations` on SystemTime until these are available in standard library | ||
pub trait CheckedSystemTime { | ||
/// Returns `Some<SystemTime>` when the result less or equal to `i32::max_value` to prevent `SystemTime` to panic because | ||
/// it is platform specific, possible representations are i32, i64, u64 and Duration. `None` otherwise | ||
fn checked_add(self, _d: Duration) -> Option<SystemTime>; | ||
/// Returns `Some<SystemTime>` when the result is successful and `None` when it is not | ||
fn checked_sub(self, _d: Duration) -> Option<SystemTime>; | ||
} | ||
|
||
impl CheckedSystemTime for SystemTime { | ||
fn checked_add(self, dur: Duration) -> Option<SystemTime> { | ||
let this_dur = self.duration_since(UNIX_EPOCH).ok()?; | ||
let total_time = this_dur.checked_add(dur)?; | ||
|
||
if total_time.as_secs() <= i32::max_value() as u64 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so basically we can only add a maximum of 68 years to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, good question Seun :P It is because SystemTime represents seconds as This basically prevents the overflow in easy-way (but limits all platforms with limit of 2038) i.e, not have to rely on any complicated conditional compilation. But this should be removed/deprecated when |
||
Some(self + dur) | ||
} else { | ||
None | ||
} | ||
} | ||
|
||
fn checked_sub(self, dur: Duration) -> Option<SystemTime> { | ||
let this_dur = self.duration_since(UNIX_EPOCH).ok()?; | ||
let total_time = this_dur.checked_sub(dur)?; | ||
|
||
if total_time.as_secs() <= i32::max_value() as u64 { | ||
Some(self - dur) | ||
} else { | ||
None | ||
} | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
#[test] | ||
fn it_works() { | ||
use super::CheckedSystemTime; | ||
use std::time::{Duration, SystemTime, UNIX_EPOCH}; | ||
|
||
assert!(CheckedSystemTime::checked_add(UNIX_EPOCH, Duration::new(i32::max_value() as u64 + 1, 0)).is_none()); | ||
assert!(CheckedSystemTime::checked_add(UNIX_EPOCH, Duration::new(i32::max_value() as u64, 0)).is_some()); | ||
assert!(CheckedSystemTime::checked_add(UNIX_EPOCH, Duration::new(i32::max_value() as u64 - 1, 1_000_000_000)).is_some()); | ||
|
||
assert!(CheckedSystemTime::checked_sub(UNIX_EPOCH, Duration::from_secs(120)).is_none()); | ||
assert!(CheckedSystemTime::checked_sub(SystemTime::now(), Duration::from_secs(1000)).is_some()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should return error on failing
checked_add
here (same for min but max would have failed before). I do not know the authority round logic enough to say.At the same time if it goes over max value we got a natural bound check from
checked_add
so it is probably not an issue.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we shouldn't because you will get an error message something like:
OutOfBounds { min: None, max: None, found: val }
which should be more informative than
TimestampOverflow
However I think it would be better represent those
OutOfBounds
as Duration instead because they are easier to reason about i.e, will always represent seconds as u64!Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
saturating_add
would fit better, my fear was that we were removing a upper bound by making this change but thinking twice it goes directly into error so it does not matter and you are right it only depends upon what we want to display, so it is ok as it is.Edit: does not seems to be use for something else than display in trace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I get your point saturating_add is a good idea but it is very messy for SystemTime