Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Remove time-utils #11410

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Remove time-utils #11410

wants to merge 2 commits into from

Conversation

vorot93
Copy link

@vorot93 vorot93 commented Jan 27, 2020

SystemTime::checked_add and SystemTime::checked_sub have been stable since 1.34.

@parity-cla-bot
Copy link

It looks like @vorot93 signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

README.md Show resolved Hide resolved
@dvdplm dvdplm requested a review from niklasad1 January 27, 2020 08:33
@dvdplm dvdplm added A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust. labels Jan 27, 2020
Copy link
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@niklasad1 niklasad1 added A0-pleasereview 🤓 Pull request needs code review. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Jan 27, 2020
@ordian
Copy link
Collaborator

ordian commented Jan 27, 2020

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.

@niklasad1
Copy link
Collaborator

niklasad1 commented Jan 27, 2020

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 2028 year problem. I mean that a Block's timestamp is an u64 and it may be forged. So, it may cause consensus issues. Whether a node decides that i32::max_value + 10 is overflow and another one decides that it is legit on different platforms.

Still, we might have some plausibility checks on the timestamps but I'm not sure if those catches this.

@vorot93
Copy link
Author

vorot93 commented Jan 27, 2020

What if a block timestamp is forged but still fits i32? This code can't catch this in any way.

@niklasad1
Copy link
Collaborator

niklasad1 commented Jan 28, 2020

What if a block timestamp is forged but still fits i32? This code can't catch this in any way.

I was not talking about that just the difference between i32/i64 for SystemTime.

However, actually we check that timestamp is less than now + 15 seconds and reject timestamps further on in the future, see https://github.com/paritytech/parity-ethereum/blob/master/ethcore/verification/src/verification.rs#L339-#L364.

But still, we are doing the checked arithmetics before doing those checks so I don't think we are safe on different platforms. In practice, we should return TimestampOverflow or TemporaryInvalid in such cases because we are still a long time away from year 2028 and I'm not sure exactly on the cases in different engines such as AuRA or Clique.

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)));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for authority-round the issue of SystemTime being platform dependant doesn't really matter, since we use it in the error path here anyway.

@@ -571,7 +570,7 @@ impl Engine for Clique {

// 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))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -581,7 +580,7 @@ impl Engine for Clique {

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)?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error path again

@@ -273,7 +272,7 @@ impl CliqueBlockState {
// 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 = 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)));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the tricky part that needs to be audited

@sorpaas sorpaas added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants