-
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: drand: calculation of round from Filecoin epochs #8606
Conversation
a689d30
to
bded013
Compare
Added test. |
chain/beacon/drand/drand.go
Outdated
// we take the time from genesis divided by the periods in seconds, that | ||
// gives us the number of periods since genesis. We also add +1 because | ||
// round 1 starts at genesis time. | ||
return uint64(math.Floor(float64(fromGenesis)/db.interval.Seconds())) + 1 |
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.
I feel a bit uneasy using floats for this.
As you only need Floor it should be simple to do using pure ints.
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.
I agree, simplifying this.
Hey, how does it work during the upgrade? |
Ahh, right in this upgrade we only add on one breacon. Ok, I can see how that can work. |
bded013
to
f405112
Compare
Yeah, so what should happen is just that the first block after the upgrade kicks in should have 2 beacon entries (exactly as we have after a null tipset). |
f405112
to
6924a3d
Compare
dround := (latestTs - db.drandGenTime) / uint64(db.interval.Seconds()) | ||
return dround | ||
} | ||
|
||
func (db *DrandBeacon) maxBeaconRoundV2(latestTs uint64) uint64 { | ||
if latestTs < db.drandGenTime { | ||
return 1 |
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.
not 0?
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.
When latestTs == db.drandGenTime
, the round will be 1. I don't know if it matters, but it may make more sense to let the round be 0 before that.
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.
Not super important for mainnet, but I think your argument makes sense. Changing.
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.
Actually, looking at Drand code, round 0 seems to indicate a special value that says "give me the latest entry".
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.
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.
Gonna leave as is, for now.
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.
SGWM
Related Issues
Resolves #2170
Proposed Changes
This PR was originally opened for the v14 upgrade, but descoped.
Additional Info
Checklist
Before you mark the PR ready for review, please make sure that:
<PR type>: <area>: <change being made>
fix: mempool: Introduce a cache for valid signatures
PR type
: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, testarea
: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps