feat: support drand quicknet #3873

hanabi1224 · 2024-01-15T09:43:56Z

Summary of changes

Changes introduced in this pull request:

~~Implement quicket beacon verification with unit tests~~
~~Refactor mainnet beacon verification to support batching with unit tests and replace bls_signatues::verify_messages which is designed to verify bls messages in tipsets instead of beacons~~
Make beacon verification cache size bounded. (HashMap -> LruCache)
Add network upgrade logic (referencing feat: implement FIP-0063 filecoin-project/lotus#11572)
Add Quicknet config with unit tests
Some naming and comment updates
Refactor verify_bls_aggregate

Reference issue to close (if applicable)

Closes #3763

Change checklist

I have performed a self-review of my own code,
I have made corresponding changes to the documentation,
I have added tests that prove my fix is effective or that my feature works (if possible),
I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

hanabi1224 · 2024-01-16T06:57:10Z

src/beacon/drand.rs

@@ -75,17 +74,10 @@ impl BeaconSchedule {
        parent_epoch: ChainEpoch,
        prev: &BeaconEntry,
    ) -> Result<Vec<BeaconEntry>, anyhow::Error> {
-        let (cb_epoch, curr_beacon) = self.beacon_for_epoch(epoch)?;


This aligns with https://github.com/filecoin-project/lotus/pull/11572/files#diff-587eaf0df6b60dbf741d19bbe39439f6f48ffe171e82a71c76b82484ba48f386R110

hanabi1224 · 2024-01-16T06:58:01Z

src/beacon/drand.rs

-
-        let chain_info = &config.chain_info;
-
-        if cfg!(debug_assertions) && config.network_type == DrandNetwork::Mainnet {


This has been covered by unit tests

hanabi1224 · 2024-01-16T07:02:54Z

src/beacon/drand.rs

-        };
-        // Signature
-        let sig = Signature::from_bytes(curr.data())?;
-        let sig_match = bls_signatures::verify_messages(&sig, &[&digest], &[self.pub_key.key()?]);


bls_signatures::verify_messages is designed for validating bls_messages in full tipsets, with 1 signature and a list of (message, public key) pairs to validate. While for drand beacon verification, there are 1 public key and a list of (message, signature) pairs to validate, so re-writing the logic to allow benefiting from parallelization.

hanabi1224 · 2024-01-16T07:10:21Z

src/blocks/header.rs

            .beacon_for_epoch(parent_epoch)
            .map_err(|e| Error::Validation(e.to_string()))?;

-        if cb_epoch != pb_epoch {


This aligns with https://github.com/filecoin-project/lotus/pull/11572/files#diff-587eaf0df6b60dbf741d19bbe39439f6f48ffe171e82a71c76b82484ba48f386R55

aatifsyed

Not expert-review, just a few nits

aatifsyed · 2024-01-17T12:11:46Z

Cargo.toml

@@ -26,6 +26,8 @@ bls-signatures = { version = "0.15", default-features = false, features = [
  "multicore",
  "blst-portable",
 ] } # prevent SIGINT on CI runners by using portable assembly
+blst = { version = "0.3", features = ["portable"] }


I'm surprised that we need both these libraries - it looks like we're only using blst::BLST_ERROR - can you confirm this is necessary?

Good catch, it should be ok to downcast to anyhow::Error here

blst removed

aatifsyed · 2024-01-17T12:13:16Z

src/beacon/drand.rs

        let max_round = curr_beacon.max_beacon_round_for_epoch(network_version, epoch);
+        // We don't expect this to ever be the case


should we log or panic! on this branch?

This comment and logic are from the lotus PR, I'm not 100% sure if it's true and I'd be careful adding panic here that might crash forest in some edge cases

// We don't expect this to ever be the case if maxRound == prev.Round { return nil, nil }

let's add at least a massive error log so there's a chance it catches our attention, just in case

@LesnyRumcajs Log added, please re-approve if it looks good.

src/beacon/drand.rs

aatifsyed · 2024-01-17T12:20:38Z

src/networks/mainnet/mod.rs

@@ -170,6 +170,11 @@ pub(super) static DRAND_SCHEDULE: [DrandPoint<'static>; 2] = [
        height: SMOKE_HEIGHT,
        config: &DRAND_MAINNET,
    },
+    DrandPoint {
+        // height is TDB


what does this mean?

We need to update the right height to perform the upgrade later. It should be the Pineapple upgrade Epoch but it's not defined in forest yet.

Co-authored-by: Aatif Syed <38045910+aatifsyed@users.noreply.github.com>

LesnyRumcajs · 2024-01-19T11:20:18Z

@hanabi1224 shall we remove the changes included in #3887 from this PR?

hanabi1224 · 2024-01-19T11:27:12Z

@hanabi1224 shall we remove the changes included in #3887 from this PR?

@LesnyRumcajs I plan to merge this after #3887 so the changes will be gone after resolving merge conflicts, sounds good?

LesnyRumcajs · 2024-01-19T11:33:31Z

src/beacon/drand.rs

-            let entry = curr_beacon.entry(cur).await?;
-            cur = entry.round() - 1;
-            out.push(entry);
+        // We only ever need one entry after nv22 (FIP-0063)


the quicknet change will be at exactly the same epoch as the nv22 upgrade?

The logic and comments here replicate lotus, and yes, I think the planned upgrade epoch is the pineapple upgrade

LesnyRumcajs

Did you manage to test it on a devnet?

src/networks/devnet/mod.rs

src/networks/mainnet/mod.rs

hanabi1224 · 2024-01-19T11:50:46Z

Did you manage to test it on a devnet?

@LesnyRumcajs Not yet, that would require the miner changes ready on lotus side as discussed in the sync call. On our side, I believe I just need to update the lotus tag and devnet upgrade epoch accordingly and it will be covered by the devnet checks CI job

Co-authored-by: Hubert <hubert@chainsafe.io>

hanabi1224 · 2024-01-22T05:44:12Z

@hanabi1224 shall we remove the changes included in #3887 from this PR?

@LesnyRumcajs removed.

hanabi1224 · 2024-01-29T08:44:17Z

src/beacon/drand.rs

-            cur = entry.round() - 1;
-            out.push(entry);
+        // We only ever need one entry after nv22 (FIP-0063)
+        if network_version > NetworkVersion::V21 {


What if epoch is between PineappleUpgradeHeight and PineappleUpgradeHeight + 10

hanabi1224 force-pushed the hm/drand-quicknet branch 4 times, most recently from c1d9ed6 to 9e96b1e Compare January 15, 2024 11:33

feat: support drand quicket

55eb7af

hanabi1224 force-pushed the hm/drand-quicknet branch from 9e96b1e to 55eb7af Compare January 15, 2024 11:56

hanabi1224 added 2 commits January 15, 2024 22:39

cleanup

0894163

Merge remote-tracking branch 'origin/main' into hm/drand-quicknet

fcfad3e

hanabi1224 commented Jan 16, 2024

View reviewed changes

bounded cache size

091123f

hanabi1224 marked this pull request as ready for review January 16, 2024 07:50

hanabi1224 requested a review from a team as a code owner January 16, 2024 07:50

hanabi1224 requested review from LesnyRumcajs and aatifsyed and removed request for a team January 16, 2024 07:50

hanabi1224 changed the title ~~[WIP] feat: support drand quicket~~ feat: support drand quicket Jan 16, 2024

hanabi1224 added 5 commits January 16, 2024 16:59

bug fix

7f6abe0

naming fixes

3f8a875

Merge remote-tracking branch 'origin/main' into hm/drand-quicknet

501e54d

Merge branch 'main' into hm/drand-quicknet

9d8d70f

cleanup repetitive code

a66a9c8

aatifsyed approved these changes Jan 17, 2024

View reviewed changes

aatifsyed changed the title ~~feat: support drand quicket~~ feat: support drand quicknet Jan 17, 2024

hanabi1224 and others added 3 commits January 17, 2024 21:25

Update src/beacon/drand.rs

d281239

Co-authored-by: Aatif Syed <38045910+aatifsyed@users.noreply.github.com>

no blst crate

41c542e

fix

c5bd19a

aatifsyed approved these changes Jan 17, 2024

View reviewed changes

hanabi1224 mentioned this pull request Jan 19, 2024

feat: verification module for drand quicknet #3887

Merged

4 tasks

LesnyRumcajs reviewed Jan 19, 2024

View reviewed changes

src/networks/devnet/mod.rs Outdated Show resolved Hide resolved

src/networks/mainnet/mod.rs Outdated Show resolved Hide resolved

hanabi1224 and others added 2 commits January 19, 2024 21:19

Apply suggestions from code review

1c3fd48

Co-authored-by: Hubert <hubert@chainsafe.io>

Merge branch 'main' into hm/drand-quicknet

d44185a

LesnyRumcajs approved these changes Jan 19, 2024

View reviewed changes

hanabi1224 added 2 commits January 19, 2024 23:11

Merge remote-tracking branch 'origin/main' into hm/drand-quicknet

0646902

log

2947604

LesnyRumcajs approved these changes Jan 22, 2024

View reviewed changes

hanabi1224 added this pull request to the merge queue Jan 22, 2024

Merged via the queue into main with commit fa4e286 Jan 22, 2024
27 checks passed

hanabi1224 deleted the hm/drand-quicknet branch January 22, 2024 09:30

hanabi1224 commented Jan 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support drand quicknet #3873

feat: support drand quicknet #3873

hanabi1224 commented Jan 15, 2024 •

edited

Loading

hanabi1224 Jan 16, 2024 •

edited

Loading

hanabi1224 Jan 16, 2024

hanabi1224 Jan 16, 2024

hanabi1224 Jan 16, 2024

aatifsyed left a comment

aatifsyed Jan 17, 2024

hanabi1224 Jan 17, 2024

hanabi1224 Jan 17, 2024

aatifsyed Jan 17, 2024

hanabi1224 Jan 17, 2024 •

edited

Loading

LesnyRumcajs Jan 19, 2024

hanabi1224 Jan 22, 2024

aatifsyed Jan 17, 2024

hanabi1224 Jan 17, 2024

LesnyRumcajs commented Jan 19, 2024

hanabi1224 commented Jan 19, 2024

LesnyRumcajs Jan 19, 2024

hanabi1224 Jan 19, 2024

LesnyRumcajs left a comment

hanabi1224 commented Jan 19, 2024 •

edited

Loading

hanabi1224 commented Jan 22, 2024 •

edited

Loading

hanabi1224 Jan 29, 2024


		let chain_info = &config.chain_info;

		if cfg!(debug_assertions) && config.network_type == DrandNetwork::Mainnet {

		let max_round = curr_beacon.max_beacon_round_for_epoch(network_version, epoch);
		// We don't expect this to ever be the case

feat: support drand quicknet #3873

feat: support drand quicknet #3873

Conversation

hanabi1224 commented Jan 15, 2024 • edited Loading

Summary of changes

Reference issue to close (if applicable)

Other information and links

Change checklist

hanabi1224 Jan 16, 2024 • edited Loading

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

aatifsyed left a 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

hanabi1224 Jan 17, 2024 • edited Loading

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

LesnyRumcajs commented Jan 19, 2024

hanabi1224 commented Jan 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LesnyRumcajs left a comment

Choose a reason for hiding this comment

hanabi1224 commented Jan 19, 2024 • edited Loading

hanabi1224 commented Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

hanabi1224 commented Jan 15, 2024 •

edited

Loading

hanabi1224 Jan 16, 2024 •

edited

Loading

hanabi1224 Jan 17, 2024 •

edited

Loading

hanabi1224 commented Jan 19, 2024 •

edited

Loading

hanabi1224 commented Jan 22, 2024 •

edited

Loading