-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix calculation of Drand round from Filecoin epochs #7389
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking comments so LGTM !
@@ -42,10 +44,10 @@ type BeaconPoint struct { | |||
type RandomBeacon interface { | |||
Entry(context.Context, uint64) <-chan Response | |||
VerifyEntry(types.BeaconEntry, types.BeaconEntry) error | |||
MaxBeaconRoundForEpoch(abi.ChainEpoch) uint64 | |||
MaxBeaconRoundForEpoch(network.Version, abi.ChainEpoch) uint64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use https://github.com/filecoin-project/lotus/pull/7389/files#diff-587eaf0df6b60dbf741d19bbe39439f6f48ffe171e82a71c76b82484ba48f386R23-R38 for determining the right version ? Now this PR is putting the both logic (1) select the right version (2) return the right randomness in the same method impacting all functions, while the Schedule seems to have been made to clearly separate the logics so it's easy to upgrade later on (without changing necessarily the API of RandomBeacon or the rest of the code)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link doesn't work :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but reflecting nikkolasg comment, should we just use beacon schedule and upgrade the beacon to another "version" of itself, instead of adding this in?
dround := (latestTs - db.drandGenTime) / uint64(db.interval.Seconds()) | ||
return dround | ||
} | ||
|
||
func (db *DrandBeacon) maxBeaconRoundV2(latestTs uint64) uint64 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the function is correct, I'm simply suggesting that it may make sense to simply take the one from drand implementation ? https://github.com/drand/drand/blob/master/chain/time.go#L52 but no worries
d3652f0
to
21bb0e7
Compare
Closing for now -- Chocolate has enough changing that we shouldn't be squeezing this in- |
Closes #2170
Opened against dev branch for now, will move to be against feat/nv14 when #7376 lands