Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Precise Ticker for Slot Interval #511

Closed
rawfalafel opened this issue Sep 13, 2018 · 6 comments
Closed

Implement Precise Ticker for Slot Interval #511

rawfalafel opened this issue Sep 13, 2018 · 6 comments
Assignees
Labels
Bug Something isn't working Good First Issue Good for newcomers
Milestone

Comments

@rawfalafel
Copy link
Contributor

rawfalafel commented Sep 13, 2018

Background
In the current spec, new blocks are allowed to advance the chain every slotDuration seconds. As a concrete example, if the genesis block is published at time T, the first block can be published at time T+slotDuration, the second block time T+2*slotDuration, etc. Prysm currently has a naive implementation that uses the standard library Ticker, but it has two issues.

  1. The clock doesn't tick precisely at T+N*slotDuration, where N is a non-negative integer. This means that the ticker will be "off beat" from the correct beat, even if the interval length is correct.
  2. The clock may drift over time. The ticker uses a monotonic clock, meaning that it won't adjust to miniscule inaccuracies in the interval length. This is an issue if we expect the beacon chain to run for days/weeks/months.

Requirements
Replace the current naive ticker with a ticker that addresses both issues above. It's probably best to confirm that clock drift is an issue, although this thread seems to confirm that it is. The thread also contains a sample solution by Russ Cox -- that may be helpful 😃

@danielschonfeld
Copy link
Contributor

@rawfalafel are you referring to SLOT_DURATION instead of cycleLength ?

@rawfalafel
Copy link
Contributor Author

@rawfalafel are you referring to SLOT_DURATION instead of cycleLength ?

@danielschonfeld yeah, thanks!

Sent with GitHawk

@danielschonfeld
Copy link
Contributor

danielschonfeld commented Sep 14, 2018

I am no expert on this, but from reading Russ Cox's comments and code examples, it seems to me that both him and the original OP come to agreement that there is no drift problem with Go or their ticker implementation. It seems that the problem is between the virtualized environment (Azure VM in this case) and the real world clock.

Russ also goes on to say that Go's implementation will generally stay loyal to the start time its aligned to.

Having that said, raises two questions:

  1. Do you agree with my assertion? If not, please explain
  2. If you do agree, the next question is whether or not we should go the extra length to account and adjust for virtualized environments unpredictability?

@rawfalafel
Copy link
Contributor Author

From the thread

It does seem that for whatever reason your VM's monotonic timer is running (1+2/99)X faster than wall time, assuming wall time is correct

We're also making the assumption that the wall time is always correct, but the monotonic clock might be too fast or too slow.

Russ also goes on to say that Go's implementation will generally stay loyal to the start time its aligned to.

Right. Go's ticker stays loyal to the monotonic clock, even though the monotonic clock isn't aligned with the wall clock.

I realized I left out an important detail. We're assuming that the wall clock is always in sync with every other node's wall clock using NTP server. We can verify this by occasionally comparing time.Now() with NTP's current time and failing if the difference is over some threshold.

@rauljordan
Copy link
Contributor

Could be useful perhaps to check NTP at each cycle transition as that is a sensitive point for all beacon clients

@rauljordan rauljordan changed the title Implement precise ticker for slot interval Implement Precise Ticker for Slot Interval Sep 21, 2018
@rawfalafel rawfalafel self-assigned this Oct 5, 2018
@prestonvanloon prestonvanloon added this to the Ruby - v1.1 milestone Oct 9, 2018
@rauljordan
Copy link
Contributor

Closed by #635

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Good First Issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants