Add PeerDAS KZG lib integration (construction & KZG verification) by jimmygchen · Pull Request #6212 · sigp/lighthouse

jimmygchen · 2024-08-01T13:32:35Z

Issue Addressed

Add PeerDAS KZG library and use it for data column construction and KZG verification.

Proposed Changes

Upstream PRs:

Add data column kzg verification and update c-kzg. #5701
chore: add rust PeerdasKZG crypto library for peerdas functionality and rollback c-kzg dependency to 4844 version #5941
chore: update peerdas-kzg library #6118
chore: Use reference to an array representing a blob instead of an owned KzgBlob (only for peerdas functions) #6179

Thanks @kevaundray for the PRs!

Additional Info

I've also moved build_data_column_sidecars and reconstruct_data_columns functions from consensus/types to kzg_utils in the beacon_chain crates, where all other KZG related logic are located.

This PR doesn't include the following, in order to keep the PR size moderate and solely focused on KZG lib integration:

Constructing the data column sidecars when proposing a block
PeerDAS KZG spec tests

…ell kzg verification (sigp#5701, sigp#5941, sigp#6118, sigp#6179) Co-authored-by: kevaundray <kevtheappdev@gmail.com>

dapplion

LGTM, ok to move the code to kzg_utils

jimmygchen · 2024-08-02T13:33:25Z

I'm looking into the windows CI failure.

realbigsean · 2024-08-05T17:30:28Z

+        //
+        // Note: One can also use `from_json` to initialize it from the consensus-specs
+        // json string.
+        let peerdas_trusted_setup = PeerDASTrustedSetup::default();


Looks like there's an embedded trusted setup here, so we're loading the same setup twice right? so maybe we can at least add an equivalence check here if there's no way to consolidate the two yet

Yep that's right. Yeah I think equivalence check is a good idea, or potentially cloning the values from existing trusted setup. There are a few more things I've done on another WIP branch, but I'm still thinking about whether it's a good idea:

Should we only load this if PeerDAS is scheduled (None otherwise), so we don't impact mainnet code path now (I have a branch here that does this)

Load PeerDAS setup from existing trusted_setup using clone (code), so we avoid reading file system and also avoid breaking --trusted-setup flag - although we don't really use this anymore

I'm going to cherry-pick both changes into this branch.

With (2), I just did a bench on both approaches (loading from json / clone and encode existing trusted setup) - cloning is actually slightly faster than re-loading from json (-20ms), plus we get consistency, and we won't need to perform equivalent check.

I haven't done #1, it actually involves touching a lot of files and I'm not sure if it's worth it, as loading the PeerDAS trusted setup only adds ~550ms on startup and ~100mb of memory for storing the precomputation. Existing Deneb code path still uses c-kzg so should be unaffected.

Once both libraries are feature complete and stable for deneb and PeerDAS, we could do this properly and only load one library with a cli flag.

What do you think @realbigsean ?

Replaced loading embedded trusted setup with cloning the existing trusted setup values.
11c53a8

Added conditionally load KZG on startup only if PeerDAS is enabled: 6881981

Once both libraries are feature complete and stable for deneb and PeerDAS, we could do this properly and only load one library with a cli flag.

That sounds good!

…nnecessary `needless_lifetimes`. Co-authored-by: realbigsean <sean@sigmaprime.io>

…y and maintain `--trusted-setup` functionality.

…eerdas-kzg

dapplion · 2024-08-10T03:28:46Z

 #[derive(Debug)]
 pub struct Kzg {
    trusted_setup: KzgSettings,
+    context: DASContext,


Is it possible to conditionally load the DAS context for peerdas networks only? It's kindoff a bad penalty to force mainnet users to pay the extra 110MB in memory for an unused feature

Done in 6881981

dapplion

LGTM

jimmygchen · 2024-08-12T23:50:18Z

@mergify queue

mergify · 2024-08-12T23:50:24Z

queue

✅ The pull request has been merged automatically

Details

The pull request has been merged automatically at 6dc614f

…gp#6212) * Add peerdas KZG library and use it for data column construction and cell kzg verification (sigp#5701, sigp#5941, sigp#6118, sigp#6179) Co-authored-by: kevaundray <kevtheappdev@gmail.com> * Update `rust_eth_kzg` crate to published version. * Update kzg metrics buckets. * Merge branch 'unstable' into peerdas-kzg * Update KZG version to fix windows mem allocation. * Refactor common logic from build sidecar and reconstruction. Remove unnecessary `needless_lifetimes`. Co-authored-by: realbigsean <sean@sigmaprime.io> * Copy existing trusted setup into `PeerDASTrustedSetup` for consistency and maintain `--trusted-setup` functionality. * Merge branch 'unstable' into peerdas-kzg * Merge branch 'peerdas-kzg' of github.com:jimmygchen/lighthouse into peerdas-kzg * Merge branch 'unstable' into peerdas-kzg * Merge branch 'unstable' into peerdas-kzg * Load PeerDAS KZG only if PeerDAS is enabled.

Add peerdas KZG library and use it for data column construction and c…

88b6bd9

…ell kzg verification (sigp#5701, sigp#5941, sigp#6118, sigp#6179) Co-authored-by: kevaundray <kevtheappdev@gmail.com>

jimmygchen added ready-for-review The code is ready for review das Data Availability Sampling das-unstable-merge labels Aug 1, 2024

jimmygchen mentioned this pull request Aug 1, 2024

Progressively merge das to unstable #6072

Closed

jimmygchen changed the title ~~Add PeerDAS KZG library and use it for data column construction and KZG verification~~ Add PeerDAS KZG lib integration (construction & KZG verification) Aug 1, 2024

dapplion approved these changes Aug 1, 2024

View reviewed changes

Comment thread beacon_node/beacon_chain/src/metrics.rs

jimmygchen added 2 commits August 2, 2024 17:15

Update rust_eth_kzg crate to published version.

456b2b7

Update kzg metrics buckets.

2fdcc35

realbigsean requested changes Aug 5, 2024

View reviewed changes

jimmygchen added 2 commits August 6, 2024 11:59

Merge branch 'unstable' into peerdas-kzg

9c78bea

Update KZG version to fix windows mem allocation.

fd65fe5

jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Aug 6, 2024

jimmygchen and others added 2 commits August 7, 2024 22:14

Refactor common logic from build sidecar and reconstruction. Remove u…

9b69f1d

…nnecessary `needless_lifetimes`. Co-authored-by: realbigsean <sean@sigmaprime.io>

Copy existing trusted setup into PeerDASTrustedSetup for consistenc…

11c53a8

…y and maintain `--trusted-setup` functionality.

jimmygchen requested a review from realbigsean August 7, 2024 12:56

jimmygchen added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Aug 7, 2024

jimmygchen added 3 commits August 7, 2024 22:56

Merge branch 'unstable' into peerdas-kzg

dad1b71

Merge branch 'peerdas-kzg' of github.com:jimmygchen/lighthouse into p…

bb09748

…eerdas-kzg

Merge branch 'unstable' into peerdas-kzg

2758071

dapplion reviewed Aug 10, 2024

View reviewed changes

jimmygchen added 2 commits August 12, 2024 13:39

Merge branch 'unstable' into peerdas-kzg

8156092

Load PeerDAS KZG only if PeerDAS is enabled.

856fa89

jimmygchen force-pushed the peerdas-kzg branch from 6881981 to 856fa89 Compare August 12, 2024 05:40

dapplion approved these changes Aug 12, 2024

View reviewed changes

dapplion requested a review from pawanjay176 August 12, 2024 13:47

realbigsean approved these changes Aug 12, 2024

View reviewed changes

realbigsean added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Aug 12, 2024

mergify Bot added a commit that referenced this pull request Aug 12, 2024

Merge of #6212

8f5d62b

This was referenced Aug 12, 2024

merge queue: embarking unstable (f2fdbe7) and [#6224 + #6212 + #4380] together #6252

Closed

Implement data columns by network boilerplate #6224

Merged

Reuse password option prompts again on a wrong password #4380

Merged

mergify Bot merged commit 6dc614f into sigp:unstable Aug 13, 2024

jimmygchen mentioned this pull request Nov 5, 2024

Make BeaconChain::kzg field mandatory #6267

Merged

Conversation

jimmygchen commented Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Addressed

Proposed Changes

Additional Info

Uh oh!

dapplion left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jimmygchen commented Aug 2, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

realbigsean Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

jimmygchen Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimmygchen Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

jimmygchen Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimmygchen Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

jimmygchen Aug 12, 2024

Choose a reason for hiding this comment

Uh oh!

realbigsean Aug 12, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dapplion Aug 10, 2024

Choose a reason for hiding this comment

Uh oh!

jimmygchen Aug 12, 2024

Choose a reason for hiding this comment

Uh oh!

dapplion left a comment

Choose a reason for hiding this comment

Uh oh!

jimmygchen commented Aug 12, 2024

Uh oh!

mergify Bot commented Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ The pull request has been merged automatically

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jimmygchen commented Aug 1, 2024 •

edited

Loading

jimmygchen Aug 6, 2024 •

edited

Loading

jimmygchen Aug 7, 2024 •

edited

Loading

mergify Bot commented Aug 12, 2024 •

edited

Loading