Remove time-utils - #11410
Conversation
|
It looks like @vorot93 signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
There was a problem hiding this comment.
This may cause consensus issues because that SystemTime is platform dependent. It may represent it as i64 or i32 where some platforms might cap ì32::max_value and others at i64::max_value. Thus, previously forged timestamp might become valid. That's why we kept the library, it will always cap at i32::max.
|
I'm not sure we should worry about https://en.wikipedia.org/wiki/Year_2038_problem for now, the code will probably change by then. |
I'm not referring to the Still, we might have some plausibility checks on the timestamps but I'm not sure if those catches this. |
|
What if a block timestamp is forged but still fits |
I was not talking about that just the difference between i32/i64 for SystemTime. However, actually we check that timestamp is less than But still, we are doing the If somebody can prove how we are sure I'm fine merging it. |
| let max = oob.max.and_then(|m| CheckedSystemTime::checked_add(UNIX_EPOCH, Duration::from_secs(m))); | ||
| let min = oob.min.and_then(|m| CheckedSystemTime::checked_add(UNIX_EPOCH, Duration::from_secs(m))); | ||
| let max = oob.max.and_then(|m| UNIX_EPOCH.checked_add(Duration::from_secs(m))); | ||
| let min = oob.min.and_then(|m| UNIX_EPOCH.checked_add(Duration::from_secs(m))); |
There was a problem hiding this comment.
for authority-round the issue of SystemTime being platform dependant doesn't really matter, since we use it in the error path here anyway.
| // Don't waste time checking blocks from the future | ||
| { | ||
| let limit = CheckedSystemTime::checked_add(SystemTime::now(), Duration::from_secs(self.period)) | ||
| let limit = SystemTime::now().checked_add(Duration::from_secs(self.period)) |
There was a problem hiding this comment.
here it doesn't matter if we're not taking 2038 problem into account as self.period is a config param and if it's misconfigured, it's not going to work anyway
| let hdr = Duration::from_secs(header.timestamp()); | ||
| if hdr > limit_as_dur { | ||
| let found = CheckedSystemTime::checked_add(UNIX_EPOCH, hdr).ok_or(BlockError::TimestampOverflow)?; | ||
| let found = UNIX_EPOCH.checked_add(hdr).ok_or(BlockError::TimestampOverflow)?; |
There was a problem hiding this comment.
and here we're in the error path anyway
| let max = CheckedSystemTime::checked_add(UNIX_EPOCH, Duration::from_secs(header.timestamp())); | ||
| let found = CheckedSystemTime::checked_add(UNIX_EPOCH, Duration::from_secs(limit)) | ||
| let max = UNIX_EPOCH.checked_add(Duration::from_secs(header.timestamp())); | ||
| let found = UNIX_EPOCH.checked_add(Duration::from_secs(limit)) |
| // 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 = CheckedSystemTime::checked_add(UNIX_EPOCH, Duration::from_secs(timestamp.saturating_add(period))); | ||
| let inturn = UNIX_EPOCH.checked_add(Duration::from_secs(timestamp.saturating_add(period))); |
There was a problem hiding this comment.
this is the tricky part that needs to be audited
e8fd2af to
d53528b
Compare
SystemTime::checked_addandSystemTime::checked_subhave been stable since 1.34.