feat: implement FIP-0063 #11572

arajasek · 2024-01-12T18:29:44Z

Related Issues

Implements FIP-0063.

Proposed Changes

Introduce the new entry to the Drand Schedule, switching to Quicknet at PineappleHeight + 1
Refactor VerifyEntry to receive the previous entry's signature only, not the Entry struct. This signature field is nil for Quicknet entries.
Bumping drand deps to pull in Quicknet logic

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

Commits have a clear commit message.
PR title is in the form of of <PR type>: <area>: <change being made>
- example: fix: mempool: Introduce a cache for valid signatures
- PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
- area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
If the PR affects users (e.g., new feature, bug fix, system requirements change), update the CHANGELOG.md and add details to the UNRELEASED section.
New features have usage guidelines and / or documentation updates in
- Lotus Documentation
- Discussion Tutorials
Tests exist for new functionality or change in behavior
CI is green

build/drand.go

chain/beacon/beacon.go

arajasek · 2024-01-12T18:31:42Z

chain/beacon/drand/drand.go

-	if curr.Round != prev.Round+1 {
-		return xerrors.Errorf("invalid beacon entry: cur (%d) != prev (%d) + 1", curr.Round, prev.Round)
-	}


It's safe to drop this for both "current" Drand and quicknet, right?

I think so - it doesn't seems like in the current implementation the previous beacon should be cached, so there's no way for someone to e.g. provide a historical beacon that gets validated, though might be worth confirming

hmm well actually we only check the MaxBeaconRoundForEpoch - perhaps it is possible to provide a historical beacon without a check of the round?

I think it is safe although it (EDIT) was additional check for consistency that drand is behaving well.

chain/beacon/drand/drand.go

build/params_mainnet.go

build/drand.go

Kubuxu · 2024-01-22T17:18:08Z

chain/beacon/beacon.go

-		if e >= bp.Start {
+		// The new network version kicks in at one _after_ the upgrade height itself,
+		// so we switch to the new beacon when _strictly_ greater than bp.Start
+		if e > bp.Start {


Won't this break the existing migration point?

Oh, you're right, I forgot that we upgraded from incentinet to mainnet. I'll see how best to fix that up.

So... how bad is it if we switch one epoch early? Or, say, 10 epochs early at a completely different upgrade height to reduce the amount of shit happening at the same time?

As long as we switch the same in miner, no issue. It's mostly just code/system smell.

I like that, I'm actually inclined to switch later than the upgrade height (get your migration and other nonsense done first, then we switch beacons).

I'm just thinking we may want to ship two upgrades at the same time instead of trying to bundle them into one.

chain/sync.go

Stebalien · 2024-01-22T23:32:09Z

chain/beacon/beacon.go

-		if e >= bp.Start {
+		// The new network version kicks in at one _after_ the upgrade height itself,
+		// so we switch to the new beacon when _strictly_ greater than bp.Start
+		if e > bp.Start {


So... how bad is it if we switch one epoch early? Or, say, 10 epochs early at a completely different upgrade height to reduce the amount of shit happening at the same time?

Stebalien · 2024-01-22T23:54:45Z

chain/consensus/filcns/filecoin.go

@@ -129,6 +129,7 @@ func (filec *FilecoinEC) ValidateBlock(ctx context.Context, b *types.FullBlock)
 		return xerrors.Errorf("failed to get lookback tipset for block: %w", err)
 	}

+	// TODO: Optimization: We don't need to get latest beacon entry after nv22 and can supply null instead


Could you:

Link to an issue (creating one).

Explain (somewhere) why?

chain/beacon/drand/drand.go

Kubuxu · 2024-01-24T16:12:09Z

build/drand.go

+			"https://api2.drand.sh",
+			"https://api3.drand.sh",
+			"https://drand.cloudflare.com",
+			"https://api.drand.secureweb3.com:6875", // Storswift


Is this endpoint monitored by any of the drand monitoring infra? We were burned in the past by 3rd party bootstrappers going offline.

the endpoint, no, though we get some alerting if their node goes down (which is normally a good indicator).
The client uses all the addresses as fallback so it shouldn’t cause an outage though - what was the issue in the past?

@CluEleSsUK @AnomalRoil ?

The general issue in the past was that we had a significant number of third-party bootstrappers (let's say 70% of all), and the majority of them were down.

It didn't cause an outage per se, but it slowed down bootstraping.

arajasek · 2024-01-25T15:21:08Z

Rebased on top of DDO work that's landed.

Stebalien · 2024-01-25T16:48:52Z

build/params_butterfly.go

@@ -16,7 +16,8 @@ import (
 )

 var DrandSchedule = map[abi.ChainEpoch]DrandEnum{
-	0: DrandMainnet,
+	0:                           DrandMainnet,
+	UpgradePineappleHeight + 10: DrandQuicknet,


Can we make this a constant in the upgrade schedule with a new name?

I.e., pinapple height, then mango height?

Yes, certainly.

hanabi1224 · 2024-01-29T08:11:19Z

chain/beacon/beacon.go

 	MaxBeaconRoundForEpoch(network.Version, abi.ChainEpoch) uint64
 }

 func ValidateBlockValues(bSchedule Schedule, nv network.Version, h *types.BlockHeader, parentEpoch abi.ChainEpoch,
 	prevEntry types.BeaconEntry) error {
-	{
+	// Before nv22 we had "chained" beacons, and so required two entries at a fork
+	if nv < network.Version22 {


If drand network upgrade does not align with chain network upgrade, should we check the exact drand network upgrade epoch here?

This is a fork logic which works off the DrandSchedule. So, the meaning of this is to disable the old fork logic before switching to the new Drand network.

@hanabi1224 Thanks for the question! I don't think we need to check the exact epoch here -- as Kuba said, the intention is to say "before nv22, special-case fork logic this way". For the upcoming change to quicknet, we DON'T want to use the old fork logic, and instead just seamlessly switch to quicknet.

hanabi1224 · 2024-01-29T08:34:35Z

chain/beacon/beacon.go

@@ -79,7 +80,12 @@ func ValidateBlockValues(bSchedule Schedule, nv network.Version, h *types.BlockH
 		return xerrors.Errorf("expected to have beacon entries in this block, but didn't find any")
 	}

-	// Verify that the last beacon entry's round corresponds to the round we expect
+	if nv < network.Version22 && prevEntry.Round == 0 {


Ditto, should we decouple drand network version and chain network version here?

Similar to above, I don't think so, no. From nv22 onwards we'll be using "unchained" randomness, and so can simply verify round 0 too.

arajasek requested a review from a team as a code owner January 12, 2024 18:29

arajasek requested review from snadrus and magik6k and removed request for a team January 12, 2024 18:29

arajasek commented Jan 12, 2024

View reviewed changes

build/drand.go Outdated Show resolved Hide resolved

arajasek commented Jan 12, 2024

View reviewed changes

chain/beacon/beacon.go Outdated Show resolved Hide resolved

arajasek commented Jan 12, 2024

View reviewed changes

chain/beacon/drand/drand.go Outdated Show resolved Hide resolved

arajasek force-pushed the asr/fip-0063 branch from 7ccaa9d to 9e080ad Compare January 12, 2024 18:34

CluEleSsUK reviewed Jan 12, 2024

View reviewed changes

build/params_mainnet.go Outdated Show resolved Hide resolved

hanabi1224 mentioned this pull request Jan 16, 2024

feat: support drand quicknet ChainSafe/forest#3873

Merged

4 tasks

arajasek requested a review from Stebalien January 22, 2024 15:29

Kubuxu reviewed Jan 22, 2024

View reviewed changes

build/drand.go Show resolved Hide resolved

Kubuxu reviewed Jan 22, 2024

View reviewed changes

chain/sync.go Outdated Show resolved Hide resolved

Stebalien reviewed Jan 23, 2024

View reviewed changes

Kubuxu reviewed Jan 24, 2024

View reviewed changes

arajasek force-pushed the asr/fip-0063 branch 2 times, most recently from 06c5ad6 to 403887a Compare January 25, 2024 15:20

Kubuxu approved these changes Jan 25, 2024

View reviewed changes

Stebalien approved these changes Jan 25, 2024

View reviewed changes

hanabi1224 reviewed Jan 29, 2024

View reviewed changes

hanabi1224 mentioned this pull request Jan 29, 2024

fix: incorporate latest changes from lotus FIP-0063 implementation PR ChainSafe/forest#3911

Merged

4 tasks

feat: implement FIP-0063

7613cdf

arajasek force-pushed the asr/fip-0063 branch from 403887a to 7613cdf Compare January 29, 2024 17:31

Stebalien approved these changes Jan 29, 2024

View reviewed changes

arajasek merged commit 78edf46 into feat/nv22 Jan 29, 2024
89 of 90 checks passed

arajasek deleted the asr/fip-0063 branch January 29, 2024 19:10

simlecode mentioned this pull request Jan 31, 2024

feat: implement fip 0063 filecoin-project/venus#6269

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement FIP-0063 #11572

feat: implement FIP-0063 #11572

arajasek commented Jan 12, 2024

arajasek Jan 12, 2024

CluEleSsUK Jan 12, 2024

CluEleSsUK Jan 12, 2024

Kubuxu Jan 22, 2024 •

edited

Kubuxu Jan 22, 2024 •

edited

arajasek Jan 22, 2024

Stebalien Jan 22, 2024

Kubuxu Jan 24, 2024

arajasek Jan 24, 2024

Stebalien Jan 24, 2024

Stebalien Jan 22, 2024

Stebalien Jan 22, 2024

Kubuxu Jan 24, 2024

CluEleSsUK Jan 24, 2024

arajasek Jan 24, 2024

Kubuxu Jan 25, 2024

arajasek commented Jan 25, 2024

Stebalien Jan 25, 2024

Stebalien Jan 25, 2024

arajasek Jan 29, 2024

hanabi1224 Jan 29, 2024

Kubuxu Jan 29, 2024

arajasek Jan 29, 2024

hanabi1224 Jan 29, 2024

arajasek Jan 29, 2024

feat: implement FIP-0063 #11572

feat: implement FIP-0063 #11572

Conversation

arajasek commented Jan 12, 2024

Related Issues

Proposed Changes

Additional Info

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kubuxu Jan 22, 2024 • edited

Choose a reason for hiding this comment

Kubuxu Jan 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arajasek commented Jan 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kubuxu Jan 22, 2024 •

edited

Kubuxu Jan 22, 2024 •

edited