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

fix(drand): StateGetBeaconEntry uses chain beacons for historical epochs #12428

Merged
merged 3 commits into from
Sep 27, 2024

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Sep 3, 2024

Fixes: #12414

Previously StateGetBeaconEntry would always try and use a drand beacon to get the appropriate round. But as drand has shut down old beacons and we've removed client details from Lotus, it has stopped working for historical beacons.
This fix restores historical beacon entries by using the on-chain lookup, however it now follows the rules used by StateGetRandomnessFromBeacon and the get_beacon_randomness syscall which has some quirks with null rounds prior to nv14. See #12414 (comment) for specifics.

StateGetBeaconEntry still blocks for future epochs and uses live drand beacon clients to wait for and fetch rounds as they are available.


Notes for review: the existing logic in chain/rand/rand.go should not change but I've had to do some refactoring to be able to return the beacon entries as well and not just the randomness. So the file looks a bit different. Please pay special attention to that because it's used for the get_beacon_randomness syscall and the StateGetRandomnessFromBeacon API. Aside from that, this should only impact StateGetBeaconEntry.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

@rvagg rvagg changed the title fix(drand): StateGetBeaconEntry uses chain beacons for historical epochs fix(drand): StateGetBeaconEntry uses chain beacons for historical epochs Sep 3, 2024
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

@rjan90
Copy link
Contributor

rjan90 commented Sep 9, 2024

I added a release/backport to this PR, and will spin up a Calibration-node with this patch, to confirm that I can get the beacon that Hubert encountered issues with: https://filecoinproject.slack.com/archives/CPFTWMY7N/p1724405387736799?thread_ts=1724401761.517049&cid=CPFTWMY7N

Copy link
Contributor

@AnomalRoil AnomalRoil left a comment

Choose a reason for hiding this comment

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

Thanks a lot, this should significantly reduce the quantity of nodes trying to query the old defunct incentinet network we lost 👍🏻

chain/gen/genesis/miners.go Show resolved Hide resolved
chain/rand/rand.go Show resolved Hide resolved
node/impl/full/state_test.go Show resolved Hide resolved
@rvagg
Copy link
Member Author

rvagg commented Sep 12, 2024

Opened #12452 to capture some thoughts about the 20 tipset walkback(s) in the code that I think are unnecessary. I just don't want to adjust the current get-beacon-from-chain logic in this PR so I left it alone.

@rjan90 rjan90 mentioned this pull request Sep 16, 2024
49 tasks
@rvagg
Copy link
Member Author

rvagg commented Sep 17, 2024

Calling for objections before I land this. It would be great to get some more review eyes but I understand it's niche.

@BigLep
Copy link
Member

BigLep commented Sep 17, 2024

Per 2024-09-17 FilOz meeting, @jennijuju asked that @Stebalien or @Kubuxu take this to get this landed.

@Stebalien @Kubuxu : see @rvagg 's offer for a sync discussion in https://filecoinproject.slack.com/archives/CP50PPW2X/p1726550218788619 if that is helpful

Also, this one is needed for #12464 which we want to start this week.

@BigLep
Copy link
Member

BigLep commented Sep 18, 2024

Notes from 2024-09-18 Lotus standup conversation: even though this has been opened for a couple of weeks, it affects pre-quicknet, and it was desired to get in as part of Lotus v1.29.2 this week, we won't merge it until it gets reviewed properly. We know @Stebalien and @Kubuxu are more tied up this week with a colo, but we assume (and we'll followup) that they'll engage next week.

@Kubuxu
Copy link
Contributor

Kubuxu commented Sep 19, 2024

I will try to fetch in the context. I looked at it few weeks ago but I could not reach a conclusion at that time.

Copy link
Contributor

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

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

SGTM, apart from that one comment, where I feel iffy about the intent.

In the future, I suggest separating the commits into non-functional change refactors/renames and functional changes.

…pochs

Fixes: #12414

Previously StateGetBeaconEntry would always try and use a drand beacon to get
the appropriate round. But as drand has shut down old beacons and we've
removed client details from Lotus, it has stopped working for historical
beacons.
This fix restores historical beacon entries by using the on-chain lookup,
however it now follows the rules used by StateGetRandomnessFromBeacon and the
get_beacon_randomness syscall which has some quirks with null rounds prior to
nv14. See #12414 (comment)
for specifics.

StateGetBeaconEntry still blocks for future epochs and uses live drand beacon
clients to wait for and fetch rounds as they are available.
chain/store/store.go Outdated Show resolved Hide resolved
@rjan90 rjan90 merged commit e16e08a into master Sep 27, 2024
83 checks passed
@rjan90 rjan90 deleted the rvagg/StateGetBeaconEntry branch September 27, 2024 10:55
rjan90 pushed a commit that referenced this pull request Sep 27, 2024
…pochs (#12428)

* fix(drand): `StateGetBeaconEntry` uses chain beacons for historical epochs

Fixes: #12414

Previously StateGetBeaconEntry would always try and use a drand beacon to get
the appropriate round. But as drand has shut down old beacons and we've
removed client details from Lotus, it has stopped working for historical
beacons.
This fix restores historical beacon entries by using the on-chain lookup,
however it now follows the rules used by StateGetRandomnessFromBeacon and the
get_beacon_randomness syscall which has some quirks with null rounds prior to
nv14. See #12414 (comment)
for specifics.

StateGetBeaconEntry still blocks for future epochs and uses live drand beacon
clients to wait for and fetch rounds as they are available.

* fixup! fix(drand): `StateGetBeaconEntry` uses chain beacons for historical epochs

* fixup! fix(drand): `StateGetBeaconEntry` uses chain beacons for historical epochs
rjan90 pushed a commit that referenced this pull request Sep 27, 2024
…pochs (#12428)

* fix(drand): `StateGetBeaconEntry` uses chain beacons for historical epochs

Fixes: #12414

Previously StateGetBeaconEntry would always try and use a drand beacon to get
the appropriate round. But as drand has shut down old beacons and we've
removed client details from Lotus, it has stopped working for historical
beacons.
This fix restores historical beacon entries by using the on-chain lookup,
however it now follows the rules used by StateGetRandomnessFromBeacon and the
get_beacon_randomness syscall which has some quirks with null rounds prior to
nv14. See #12414 (comment)
for specifics.

StateGetBeaconEntry still blocks for future epochs and uses live drand beacon
clients to wait for and fetch rounds as they are available.

* fixup! fix(drand): `StateGetBeaconEntry` uses chain beacons for historical epochs

* fixup! fix(drand): `StateGetBeaconEntry` uses chain beacons for historical epochs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🎉 Done
Development

Successfully merging this pull request may close these issues.

StateGetBeaconEntry uses drand client to look up round instead of using tipset data
5 participants