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

Drand rounds are not computed as drand computes them #2170

Closed
nikkolasg opened this issue Jun 29, 2020 · 10 comments
Closed

Drand rounds are not computed as drand computes them #2170

nikkolasg opened this issue Jun 29, 2020 · 10 comments
Assignees
Labels
area/chain Area: Chain impact/consensus Impact: Consensus kind/enhancement Kind: Enhancement need/team-input Hint: Needs Team Input P2 P2: Should be resolved

Comments

@nikkolasg
Copy link
Contributor

nikkolasg commented Jun 29, 2020

Describe the bug

Lotus doesn't use the same "timestamp-to-round" mapping than drand:
Drand does the following (code):

math.Floor((now - genesis) / drandPeriod) + 1 

because round 1 happens at genesis time.
Lotus however does the following (code) :

math.Floor((now - genesis) / drandPeriod) 

It's a breaking change to move to drand, but it would be the preferred way.

Consequences: the lotus chain uses a round before the one it should use, having an offset of 2 instead of 1 (i.e. normally it looks at a drand round for one epoch before, but it's in fact using the randomness of one more previous drand round).

@nicola
Copy link

nicola commented Jun 30, 2020

We can either leave as it is and update the spec (at the cost of having randomness with offset 2), or we can update lotus (but it will be a breaking change).

@nicola
Copy link

nicola commented Jun 30, 2020

When making a block for a given round that block timestamp is when the block must be broadcasted by.

This means that you are likely generating that block at timestamp-1, so you must have it already by timestamp-1.

In order to make this work, it must come from a drand round at that time.


https://github.com/filecoin-project/lotus/blob/master/chain/beacon/drand/drand.go#L185


Final outcome: update the spec to reflect lotus code, since lotus is mitigating the issue described above.

@nikkolasg
Copy link
Contributor Author

  • There is one line that compute the filecoin timestamp given a filecoin epoch. This method already is shifting the result by one
latestTs := ((uint64(filEpoch) * db.filRoundTime) + db.filGenTime) - db.filRoundTime

The - db.filRoundTime at the end is saying "i take it one round before" (in Lotus, round 0 is at genesis, round 1 at genesis+roundTime, etc).

  • Then once more, we compute the drand round shifted by one by doing:
dround := (latestTs - db.drandGenTime) / uint64(db.interval.Seconds()) 

Remember in drand we are supposed to do a +1 when computing the round.

I understand and well aware about the concern raised above but I think we are doing the fix "twice" instead of just one, resulting in a shift by two rounds.
@Kubuxu @nicola @whyrusleeping

@nicola
Copy link

nicola commented Jul 22, 2020

Ping @Kubuxu did we fix this? Can we close this?

@nikkolasg
Copy link
Contributor Author

PR subbmitted #3519

@nicola nicola added the impact/consensus Impact: Consensus label Sep 7, 2020
@jennijuju jennijuju added this to the Network v14 milestone Jul 8, 2021
@jennijuju jennijuju added the P2 P2: Should be resolved label Jul 12, 2021
@arajasek arajasek removed this from the Network v14 milestone Sep 29, 2021
@arajasek
Copy link
Contributor

Definitely descoping this from Chocolate in light of the blowup in complexity in #7376

@nicola
Copy link

nicola commented Mar 27, 2022

hey @arajasek was this ever fixed?

@arajasek
Copy link
Contributor

@nicola No, this is still an outstanding issue.

@jennijuju
Copy link
Member

fixed in nv16

@nicola
Copy link

nicola commented Jul 13, 2022

Congrats!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/chain Area: Chain impact/consensus Impact: Consensus kind/enhancement Kind: Enhancement need/team-input Hint: Needs Team Input P2 P2: Should be resolved
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

6 participants