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

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

Merged
merged 1 commit into from
Jun 20, 2019
Merged

Conversation

niklasad1
Copy link
Collaborator

@niklasad1 niklasad1 commented Jun 5, 2019

Attempt to close #10688

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

@niklasad1 niklasad1 added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jun 5, 2019
@niklasad1 niklasad1 requested a review from tomusdrw June 5, 2019 08:30
ethcore/src/error.rs Outdated Show resolved Hide resolved
@dvdplm
Copy link
Collaborator

dvdplm commented Jun 5, 2019

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?

@jam10o-new jam10o-new added B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷🕷 labels Jun 6, 2019
@dvdplm dvdplm requested a review from seunlanlege June 10, 2019 17:53
@dvdplm 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
@niklasad1
Copy link
Collaborator Author

niklasad1 commented Jun 13, 2019

@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?

@niklasad1 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
@dvdplm
Copy link
Collaborator

dvdplm commented Jun 13, 2019

@andresilva do you have any comment on this?

@folsen
Copy link
Contributor

folsen commented Jun 13, 2019

@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.

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

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

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

niklasad1 commented Jun 18, 2019

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 niklasad1 requested a review from dvdplm June 18, 2019 17:05
@niklasad1 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 niklasad1 added the A0-pleasereview 🤓 Pull request needs code review. label Jun 18, 2019
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))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

@dvdplm
Copy link
Collaborator

dvdplm commented Jun 20, 2019

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

@niklasad1
Copy link
Collaborator Author

@dvdplm good to go from my side

@dvdplm dvdplm merged commit 78d0a86 into master Jun 20, 2019
@ordian ordian deleted the na-aura-oob-dont-add-now branch June 21, 2019 09:16
dvdplm added a commit that referenced this pull request Jun 24, 2019
…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)
dvdplm added a commit that referenced this pull request Jun 25, 2019
…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)
s3krit pushed a commit that referenced this pull request Jun 25, 2019
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
@s3krit s3krit mentioned this pull request Jun 25, 2019
s3krit pushed a commit that referenced this pull request Jun 25, 2019
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
s3krit added a commit that referenced this pull request Jun 25, 2019
@s3krit s3krit mentioned this pull request Jun 25, 2019
s3krit added a commit that referenced this pull request Jun 25, 2019
* 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
s3krit added a commit that referenced this pull request Jun 25, 2019
* 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-pleasereview 🤓 Pull request needs code review. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad block detected: TimestampOverflow
7 participants