fix(extract `timestamp_checked_add` as lib) by niklasad1 · Pull Request #10383 · openethereum/parity-ethereum

niklasad1 · 2019-02-19T19:20:55Z

Split out timestamp_checked_add into a library and fix some suspect usage of SystemTime
Conditional compilation only to use our time library when not "time_checked_add" is available in the standard library

seunlanlege · 2019-02-27T08:08:11Z

+		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 {


so basically we can only add a maximum of 68 years to SystemTime with this method, just curious why this limit.

Right, good question Seun :P

It is because SystemTime represents seconds as i32, i64, u64 or Duration (traditional OS's represent those as time_t i32 or i64).

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 Rust 1.33 is available (2019-02-28)

sorpaas · 2019-03-14T10:42:53Z

 use types::ancestry_action::AncestryAction;
 use unexpected::{Mismatch, OutOfBounds};

+#[cfg(not(feature = "time_checked_add"))]


Is this referring to rust feature or crate feature? I think for rust feature, we may need to define it to be used here:

#![cfg_attr(feature = "time_checked_add", feature(time_checked_add))]

Yeah, it is a rust feature.

EDIT: I forgot that you need import the trait in every file where it is used so I still need cfg stuff but I can add this to be on safe-side. However, I think that it shouldn't be needed!

ordian · 2019-03-14T12:40:22Z

 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


Duration::as_millis is available since rust 1.33, though it returns u128. Do we want to remove this trait? (Maybe out of scope of this PR)

I prefer to keep it out this PR. If people agree with this approach it should be easy just to move the AsMillis trait to the time_util library. However, the implementation above truncates Duration into u64 which we don't want in the library.

Seems to be ok for the use case in authority_round

This commit adds conditional compilation checks that falls back to `our time-lib` when `time_checked_add` is not available in the standard library Note, `time_checked_add` covers both `checked_add` and `checked_sub`

cheme

LGTM, waiting for 'time_checked_add' in stable.

cheme · 2019-03-15T09:09:03Z

 // 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))]


I did not find a use for this line (just tested stable and nightly with changing checked_add name and commenting this line: nightly pass, stable fail because of changed name).

@sorpaas grumble: #10383 (comment)

I can remove those if @sorpaas agree

cheme · 2019-03-15T09:23:36Z

+
+			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)));


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.

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?

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.

Right, I get your point saturating_add is a good idea but it is very messy for SystemTime

sorpaas

I think this should be ready for merge. #10383 (comment) is a mustn't grumble but still nice to have.

* fix(extract `timestamp_checked_add` as lib) * fix(whisper types): remove unused `EmptyTopics` * fix(time-lib): feature-flag to use time-lib or std This commit adds conditional compilation checks that falls back to `our time-lib` when `time_checked_add` is not available in the standard library Note, `time_checked_add` covers both `checked_add` and `checked_sub` * fix(grumble): use cfg_attr to define rustc feature

* version: bump beta * Сaching through docker volume (#10477) * _old codebase_ before docker update * before docker update, testing runnr * docker update, testing the caching * distributed job cargo homes * distributed job cargo homes 2 * distributed job cargo homes 3 * dockerfile with gitlab checkout, audit uses template * dockerfile gets repo in volume * change builds_dir * trying docker cache for repo * repo cached automatically * after script is not concatenated * check sccache non-cacheable reasons nature * watch cache * log sccache * log sccache 2 * debug log sccache * fix debug log sccache * fix debug log sccache 2 * debug log cache 3 * debug log cache 3 * trace log all sccache * test wo cargo cache * test w removed cargo cache * report non-cacheable reasons, cargo cache is back and empty * report non-cacheable reasons, cargo cache is back and empty 2 * report non-cacheable reasons, cargo cache is back and empty 3 * wrap into after_script * restore CI tags `qa` -> `linux-docker` * return to main runners, this will fail until config on runners And Dockerfile won't be updated * typo fix CI lint * return to docker tag * fix win&mac build (#10486) add CARGO_HOME: "${CI_PROJECT_DIR}/.cargo" * fix(extract `timestamp_checked_add` as lib) (#10383) * fix(extract `timestamp_checked_add` as lib) * fix(whisper types): remove unused `EmptyTopics` * fix(time-lib): feature-flag to use time-lib or std This commit adds conditional compilation checks that falls back to `our time-lib` when `time_checked_add` is not available in the standard library Note, `time_checked_add` covers both `checked_add` and `checked_sub` * fix(grumble): use cfg_attr to define rustc feature

* version: bump stable * Сaching through docker volume (#10477) * _old codebase_ before docker update * before docker update, testing runnr * docker update, testing the caching * distributed job cargo homes * distributed job cargo homes 2 * distributed job cargo homes 3 * dockerfile with gitlab checkout, audit uses template * dockerfile gets repo in volume * change builds_dir * trying docker cache for repo * repo cached automatically * after script is not concatenated * check sccache non-cacheable reasons nature * watch cache * log sccache * log sccache 2 * debug log sccache * fix debug log sccache * fix debug log sccache 2 * debug log cache 3 * debug log cache 3 * trace log all sccache * test wo cargo cache * test w removed cargo cache * report non-cacheable reasons, cargo cache is back and empty * report non-cacheable reasons, cargo cache is back and empty 2 * report non-cacheable reasons, cargo cache is back and empty 3 * wrap into after_script * restore CI tags `qa` -> `linux-docker` * return to main runners, this will fail until config on runners And Dockerfile won't be updated * typo fix CI lint * return to docker tag * fix win&mac build (#10486) add CARGO_HOME: "${CI_PROJECT_DIR}/.cargo" * fix(extract `timestamp_checked_add` as lib) (#10383) * fix(extract `timestamp_checked_add` as lib) * fix(whisper types): remove unused `EmptyTopics` * fix(time-lib): feature-flag to use time-lib or std This commit adds conditional compilation checks that falls back to `our time-lib` when `time_checked_add` is not available in the standard library Note, `time_checked_add` covers both `checked_add` and `checked_sub` * fix(grumble): use cfg_attr to define rustc feature

* master: (48 commits) ethcore: remove eth social and easthub chain configs (#10531) Initial support sccache for windows build (#10520) fix(light): make `OnDemand` generic instead of using the concrete type (#10514) private-tx: replace error_chain (#10510) Add trace information to eth_estimateGas (#10519) ethcore: add clique engine (#9981) verbose flag for cpp tests (#10524) Add a more realistic Batch test (#10511) docs: add changelogs for 2.3.{6,7,8} and 2.4.{1,2,3} (#10494) fix(rpc): fix a bunch of clippy lints (#10493) fix Sha3/keccak256 hash calculation for binaries (#10509) Add additional request tests (#10503) whisper/cli: add p2p port and ip parameters (#10057) fix(time-utils): add missing license (#10497) fix(extract `timestamp_checked_add` as lib) (#10383) fix(rpc): lint `unused_extern_crates` + fix warns (#10489) fix win&mac build (#10486) Сaching through docker volume (#10477) OpenBlock::new take IntoIterator instead of mutable ref to Iterator (#10480) simplify block module and usage (#10479) ...

niklasad1 force-pushed the na-fix-systemtime branch from 80841b2 to 6271096 Compare February 19, 2019 20:38

niklasad1 added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Feb 19, 2019

5chdn added this to the 2.4 milestone Feb 20, 2019

niklasad1 added B1-patch-beta 🕷🕷 B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. labels Feb 20, 2019

5chdn modified the milestones: 2.4, 2.5, 2.6 Feb 21, 2019

niklasad1 mentioned this pull request Feb 23, 2019

Replace timestamp_checked_add with SystemTime::checked_add when available in rustc #10406

Closed

seunlanlege reviewed Feb 27, 2019

View reviewed changes

niklasad1 force-pushed the na-fix-systemtime branch from 6271096 to 0c27fc7 Compare March 11, 2019 10:55

sorpaas reviewed Mar 14, 2019

View reviewed changes

niklasad1 mentioned this pull request Mar 14, 2019

prevent SystemTime panics goerli/parity-goerli#65

Merged

ordian reviewed Mar 14, 2019

View reviewed changes

niklasad1 added 4 commits March 14, 2019 14:27

fix(extract timestamp_checked_add as lib)

ca67e1c

fix(whisper types): remove unused EmptyTopics

48b3b88

fix(time-lib): feature-flag to use time-lib or std

ae10434

This commit adds conditional compilation checks that falls back to `our time-lib` when `time_checked_add` is not available in the standard library Note, `time_checked_add` covers both `checked_add` and `checked_sub`

fix(grumble): use cfg_attr to define rustc feature

4e07fea

niklasad1 force-pushed the na-fix-systemtime branch from 0c27fc7 to 4e07fea Compare March 14, 2019 15:32

cheme approved these changes Mar 15, 2019

View reviewed changes

soc1c added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 15, 2019

sorpaas approved these changes Mar 19, 2019

View reviewed changes

soc1c merged commit 037fd1b into master Mar 19, 2019

soc1c deleted the na-fix-systemtime branch March 19, 2019 22:17

This was referenced Mar 19, 2019

2.3.7 stable backports #10487

Merged

2.4.2 beta backports #10488

Merged

This was referenced Mar 20, 2019

Use timestamp_checked_add in calc_next_timestamp in ethcore/src/engines/clique/block_state.rs #10409

Closed

ethcore: add clique engine #9981

Merged

niklasad1 mentioned this pull request Apr 11, 2019

fix(whisper): change expiry unix_time + ttl + work #10587

Merged

Conversation

niklasad1 commented Feb 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seunlanlege Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niklasad1 Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sorpaas Mar 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niklasad1 Mar 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ordian Mar 14, 2019

Choose a reason for hiding this comment

Uh oh!

niklasad1 Mar 14, 2019

Choose a reason for hiding this comment

Uh oh!

cheme left a comment

Choose a reason for hiding this comment

Uh oh!

cheme Mar 15, 2019

Choose a reason for hiding this comment

Uh oh!

niklasad1 Mar 15, 2019

Choose a reason for hiding this comment

Uh oh!

niklasad1 Mar 18, 2019

Choose a reason for hiding this comment

Uh oh!

cheme Mar 15, 2019

Choose a reason for hiding this comment

Uh oh!

niklasad1 Mar 15, 2019

Choose a reason for hiding this comment

Uh oh!

cheme Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niklasad1 Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sorpaas left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

niklasad1 commented Feb 19, 2019 •

edited

Loading

seunlanlege Feb 27, 2019 •

edited

Loading

niklasad1 Feb 27, 2019 •

edited

Loading

sorpaas Mar 14, 2019 •

edited

Loading

niklasad1 Mar 14, 2019 •

edited

Loading

cheme Mar 15, 2019 •

edited

Loading

niklasad1 Mar 15, 2019 •

edited

Loading