fix: aura don't add `SystemTime::now()` #10720

niklasad1 · 2019-06-05T08:30:37Z

Attempt to close #10688

In AURA we addSystemTime::now again which cases a Timestamp Overflow which this fixes

ethcore/src/error.rs

dvdplm · 2019-06-05T08:49:37Z

A follow-up PR can be made and remove time-utils-library because we are at Rust 1.35 and it SystemTime::checked_** was stabilized in Rust 1.34

Can you file an issue please?

ethcore/src/verification/verification.rs

niklasad1 · 2019-06-13T07:25:57Z

@folsen

As long as it doesn't introduce any consensus-relevant difference, e.g. an old chain with a block with a forged timestamp used to be invalid there and is valid now (or vice versa) so that there could be a split depending on if you reject or if you cap out.

After thinking about this a bit I think it is possible to have consensus issues.

Before: we reject timestamps if it is bigger than i32::max_value (i.e, converting it with SystemTime::now().checked_add(timestamp)). However it is not always true because SystemTime is platform dependent (usually i32 or i64). If we use our own CheckedSystemTime it will always be i32 but rust versions after 1.34 will use the stdlib which is platform dependent

In this PR: Timestamp is represented as Duration and it will be platform independent (secs as u64 internally)

Thus, both might cause consensus issues.

I can revert the Duration stuff and just remove the issue in verify_timestamp?

dvdplm · 2019-06-13T08:03:40Z

@andresilva do you have any comment on this?

folsen · 2019-06-13T12:22:44Z

@niklasad1 I guess we also have to weigh it with how likely we think that any such blocks exist in reality. Given we can't see all chains using Aura, it's hard to judge, but we also have the nice property that Aura chains aren't really adversarial in the same way that a PoW chain is. In PoW you could cause the fork if you could mine a block with the wrong timestamp (by manipulating the system or the block directly). But in an Aura chain we have better assumptions on validator honesty but also have the property of most/all of them running a single client.

andresilva

CheckedSystemTime's aim seems to be to fix the resolution to i32 so that we don't have consensus issues, not sure if replacing this is with Duration is worth it or not. cc @tomusdrw

niklasad1 · 2019-06-18T14:13:58Z

CheckedSystemTime's aim seems to be to fix the resolution to i32 so that we don't have consensus issues, not sure if replacing this is with Duration is worth it or not. cc @tomusdrw

Yes, expect we have the following

#[cfg(not(time_checked_add))]
use time_utils::CheckedSystemTime;

rust >= rust 1.34 will use the standard library and be platform dependent.
Thus, might have consensus issues.

If I revert Duration and explicitly use CheckedSystemTime good to go?

This commit does the following: - Prevent overflow in `verify_timestamp()` by not adding `now` to found faulty timestamp - Use explicit `CheckedSystemTime::checked_add` to prevent potential consensus issues because SystemTime is platform depedent - remove `#[cfg(not(time_checked_add))]` conditional compilation

niklasad1 · 2019-06-18T17:33:38Z

ethcore/src/engines/authority_round/mod.rs

-			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 found = CheckedSystemTime::checked_add(UNIX_EPOCH, Duration::from_secs(oob.found))


We could also saturate oob.found to i32::max_value() which could eliminate this completely?!

dvdplm · 2019-06-20T05:45:15Z

@niklasad1 is this good to go or are you still poking at it?

niklasad1 · 2019-06-20T06:55:17Z

@dvdplm good to go from my side

…anager * master: Print warnings when using dangerous settings for ValidatorSet (#10733) ethcore/res: activate atlantis classic hf on block 8772000 (#10766) refactor: Fix indentation (#10740) Updated Bn128PairingImpl to use optimized batch pairing (#10765) fix: aura don't add `SystemTime::now()` (#10720) Initialize private tx logger only if private tx functionality is enabled (#10758) Remove unused code (#10762) Remove calls to heapsize (#10432)

…dp/fix/prevent-building-block-on-top-of-same-parent * dp/chore/aura-log-validator-set-in-epoch-manager: remove dead code Treat empty account the same as non-exist accounts in EIP-1052 (#10775) docs: Update Readme with TOC, Contributor Guideline. Update Cargo package descriptions (#10652) cleanup On second thought non-validators are allowed to report Move Engine::register_client to be before other I/O handler registration (#10767) cleanup Print warnings when using dangerous settings for ValidatorSet (#10733) ethcore/res: activate atlantis classic hf on block 8772000 (#10766) refactor: Fix indentation (#10740) Updated Bn128PairingImpl to use optimized batch pairing (#10765) fix: aura don't add `SystemTime::now()` (#10720) Initialize private tx logger only if private tx functionality is enabled (#10758) Remove unused code (#10762) Remove calls to heapsize (#10432)

This commit does the following: - Prevent overflow in `verify_timestamp()` by not adding `now` to found faulty timestamp - Use explicit `CheckedSystemTime::checked_add` to prevent potential consensus issues because SystemTime is platform depedent - remove `#[cfg(not(time_checked_add))]` conditional compilation

This reverts commit f104784.

* ethcore/res: activate atlantis classic hf on block 8772000 (#10766) * fix docker tags for publishing (#10741) * fix: aura don't add `SystemTime::now()` (#10720) This commit does the following: - Prevent overflow in `verify_timestamp()` by not adding `now` to found faulty timestamp - Use explicit `CheckedSystemTime::checked_add` to prevent potential consensus issues because SystemTime is platform depedent - remove `#[cfg(not(time_checked_add))]` conditional compilation * Update version * Treat empty account the same as non-exist accounts in EIP-1052 (#10775) * DevP2p: Get node IP address and udp port from Socket, if not included in PING packet (#10705) * get node IP address and udp port from Socket, if not included in PING packet * prevent bootnodes from being added to host nodes * code corrections * code corrections * code corrections * code corrections * docs * code corrections * code corrections * Apply suggestions from code review Co-Authored-By: David <dvdplm@gmail.com> * Add a way to signal shutdown to snapshotting threads (#10744) * Add a way to signal shutdown to snapshotting threads * Pass Progress to fat_rlps() so we can abort from there too. * Checking for abort in a single spot * Remove nightly-only weak/strong counts * fix warning * Fix tests * Add dummy impl to abort snapshots * Add another dummy impl for TestSnapshotService * Remove debugging code * Return error instead of the odd Ok(()) Switch to AtomicU64 * revert .as_bytes() change * fix build * fix build maybe

* ethcore/res: activate atlantis classic hf on block 8772000 (#10766) * fix docker tags for publishing (#10741) * merge-backports * Update version * remove clique engine from backports * Reset blockchain properly (#10669) * delete BlockDetails from COL_EXTRA * better proofs * added tests * PR suggestions * adds rpc error message for --no-ancient-blocks (#10608) * adds error message for --no-ancient-blocks, closes #10261 * Apply suggestions from code review Co-Authored-By: seunlanlege <seunlanlege@gmail.com> * Treat empty account the same as non-exist accounts in EIP-1052 (#10775) * fix: aura don't add `SystemTime::now()` (#10720) This commit does the following: - Prevent overflow in `verify_timestamp()` by not adding `now` to found faulty timestamp - Use explicit `CheckedSystemTime::checked_add` to prevent potential consensus issues because SystemTime is platform depedent - remove `#[cfg(not(time_checked_add))]` conditional compilation * DevP2p: Get node IP address and udp port from Socket, if not included in PING packet (#10705) * get node IP address and udp port from Socket, if not included in PING packet * prevent bootnodes from being added to host nodes * code corrections * code corrections * code corrections * code corrections * docs * code corrections * code corrections * Apply suggestions from code review Co-Authored-By: David <dvdplm@gmail.com> * Revert "fix: aura don't add `SystemTime::now()` (#10720)" This reverts commit f104784. * Add a way to signal shutdown to snapshotting threads (#10744) * Add a way to signal shutdown to snapshotting threads * Pass Progress to fat_rlps() so we can abort from there too. * Checking for abort in a single spot * Remove nightly-only weak/strong counts * fix warning * Fix tests * Add dummy impl to abort snapshots * Add another dummy impl for TestSnapshotService * Remove debugging code * Return error instead of the odd Ok(()) Switch to AtomicU64 * revert .as_bytes() change * fix build * fix build maybe

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

niklasad1 requested a review from tomusdrw June 5, 2019 08:30

niklasad1 commented Jun 5, 2019

View reviewed changes

ethcore/src/error.rs Outdated Show resolved Hide resolved

dvdplm approved these changes Jun 5, 2019

View reviewed changes

jam10o-new added B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷🕷 labels Jun 6, 2019

jam10o-new mentioned this pull request Jun 6, 2019

Bad block detected: TimestampOverflow #10688

Closed

dvdplm requested a review from seunlanlege June 10, 2019 17:53

dvdplm added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 10, 2019

This was referenced Jun 11, 2019

backports for stable 2.4.7 #10736

Merged

backports for beta 2.5.2 #10737

Merged

folsen reviewed Jun 12, 2019

View reviewed changes

ethcore/src/verification/verification.rs Outdated Show resolved Hide resolved

niklasad1 force-pushed the na-aura-oob-dont-add-now branch from cf0cda7 to eb7405a Compare June 12, 2019 12:34

niklasad1 added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Jun 13, 2019

danzipie mentioned this pull request Jun 14, 2019

New node unable to sync energywebfoundation/ewc-system-tests#18

Closed

dvdplm added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Jun 17, 2019

andresilva reviewed Jun 17, 2019

View reviewed changes

niklasad1 force-pushed the na-aura-oob-dont-add-now branch from eb7405a to 425a96f Compare June 18, 2019 17:05

niklasad1 requested a review from dvdplm June 18, 2019 17:05

niklasad1 removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Jun 18, 2019

niklasad1 added the A0-pleasereview 🤓 Pull request needs code review. label Jun 18, 2019

niklasad1 commented Jun 18, 2019

View reviewed changes

seunlanlege approved these changes Jun 18, 2019

View reviewed changes

saschagoebel mentioned this pull request Jun 19, 2019

Investigate which node is making the network crash trustlines-protocol/blockchain#132

Closed

ordian added this to the 2.6 milestone Jun 19, 2019

dvdplm merged commit 78d0a86 into master Jun 20, 2019

ordian deleted the na-aura-oob-dont-add-now branch June 21, 2019 09:16

s3krit mentioned this pull request Jun 25, 2019

Beta 2.5.3 #10776

Merged

s3krit added a commit that referenced this pull request Jun 25, 2019

Revert "fix: aura don't add SystemTime::now() (#10720)"

d42fc61

This reverts commit f104784.

s3krit mentioned this pull request Jun 25, 2019

Stable 2.4.8 backports #10779

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: aura don't add `SystemTime::now()` #10720

fix: aura don't add `SystemTime::now()` #10720

niklasad1 commented Jun 5, 2019 •

edited

dvdplm commented Jun 5, 2019

niklasad1 commented Jun 13, 2019 •

edited

dvdplm commented Jun 13, 2019

folsen commented Jun 13, 2019

andresilva left a comment

niklasad1 commented Jun 18, 2019 •

edited

niklasad1 Jun 18, 2019

dvdplm commented Jun 20, 2019

niklasad1 commented Jun 20, 2019

fix: aura don't add SystemTime::now() #10720

fix: aura don't add SystemTime::now() #10720

Conversation

niklasad1 commented Jun 5, 2019 • edited

dvdplm commented Jun 5, 2019

niklasad1 commented Jun 13, 2019 • edited

dvdplm commented Jun 13, 2019

folsen commented Jun 13, 2019

andresilva left a comment

Choose a reason for hiding this comment

niklasad1 commented Jun 18, 2019 • edited

niklasad1 Jun 18, 2019

Choose a reason for hiding this comment

dvdplm commented Jun 20, 2019

niklasad1 commented Jun 20, 2019

fix: aura don't add `SystemTime::now()` #10720

fix: aura don't add `SystemTime::now()` #10720

niklasad1 commented Jun 5, 2019 •

edited

niklasad1 commented Jun 13, 2019 •

edited

niklasad1 commented Jun 18, 2019 •

edited