Make `BeaconChain::kzg` field mandatory #6267

eserilev · 2024-08-16T00:13:57Z

Issue Addressed

Proposed Changes

Make the kzg field in BeaconChain and BeaconChainBuilder mandatory

eserilev · 2024-08-16T00:16:08Z

beacon_node/client/src/builder.rs

+            }
+        } else {
+            // TODO(kzg) this causes us failed tests i.e. http_server_genesis_state
+            panic!("Failed to load KZG");


this panic causes some of our tests to fail i.e. http_server_genesis_state. I'm assuming a trusted setup isnt available pre-deneb. So for tests that run a pre-deneb network, is there some default KZG value we want to pass in here?

I think we should try loading the KZG setup regardless of whether the network is pre-Deneb (if possible).

Wouldn't that slow things significantly? Post PeerDAS loading KZG is slow given the precomputation optimization

Oh. Hmm. Maybe we need to find a way to only load it once in tests, as otherwise we'll be loading it 1000s of times, even if we just load it for peer DAS tests?

A new feature in 0.5.0 allows a user to specify whether they want precomputation or not: crate-crypto/rust-eth-kzg#161

Its under breaking changes: Allow downstream users to avoid the precomputations that are needed for the FixedBaseMSM algorithm in FK20 since it adds a new parameter that was not there before.

I can push another release today/tomorrow if that would help -- Noting that if you want the same computing and verifying speeds without precomputation, you can increase the number of threads you initialize the library with to something like 4 instead of 1

thanks Kev! That new feature sounds like exactly what we need here. I'll try it out and report back if I'm having issues

0.5 includes a few API changes, so it might be better if I made the PR so as to not waste your time, if this blocking for this PR :)

I've opened #6309 and added you as a co-author to the commits!

dapplion · 2024-08-28T08:02:35Z

This PR adds a new trusted setup, is that expected?

eserilev · 2024-08-29T01:42:02Z

removed the extraneous trusted setup. It looks like loading KZG has added like 5 min to a few CI test groups. I'm going to dig in there and make sure were not doing any KZG precomputation when possible. Kevs recent PR makes this easy to do

eserilev · 2024-08-29T18:06:50Z

There no longer seems to be any noticeable difference in CI times between this branch and unstable

michaelsproul · 2024-09-17T01:05:24Z

beacon_node/beacon_chain/src/beacon_chain.rs

-                let kzg = self
-                    .kzg
-                    .as_ref()
-                    .ok_or(BlockProductionError::TrustedSetupNotInitialized)?;


We can delete this error

michaelsproul · 2024-09-17T01:10:35Z

beacon_node/beacon_chain/benches/benches.rs

+    let kzg = if spec.deneb_fork_epoch.is_some() {
+        KZG.clone()
+    } else {
+        KZG_NO_PRECOMP.clone()
+    };


maybe rather than duplicating this logic, which will need to be tweaked when we enable Peer DAS in more tests, we could define a function in test_utils that takes the ChainSpec and returns an Arc<Kzg> based on the values of deneb_fork_epoch and eip7594_fork_epoch?

michaelsproul · 2024-09-17T01:11:13Z

beacon_node/beacon_chain/src/builder.rs

+        let kzg = if spec.deneb_fork_epoch.is_some() {
+            KZG.clone()
+        } else {
+            KZG_NO_PRECOMP.clone()
+        };


can use the same new method here

michaelsproul · 2024-09-17T01:12:51Z

beacon_node/beacon_chain/src/data_column_verification.rs

-        .kzg
-        .clone()
-        .ok_or(GossipDataColumnError::KzgNotInitialized)?;
+    let kzg = chain.kzg.clone();


Not really an issue, but I don't think this clone is necessary

michaelsproul · 2024-09-17T01:13:32Z

beacon_node/beacon_chain/src/test_utils.rs

+        let kzg = if spec.deneb_fork_epoch.is_some() {
+            KZG.clone()
+        } else {
+            KZG_NO_PRECOMP.clone()
+        };


can use the same func here

michaelsproul · 2024-09-17T01:14:17Z

beacon_node/beacon_chain/tests/store_tests.rs

@@ -299,7 +299,7 @@ async fn light_client_updates_test() {
        .unwrap()
        .unwrap();

-    let kzg = spec.deneb_fork_epoch.map(|_| KZG.clone());
+    let kzg = KZG_NO_PRECOMP.clone();


same func here

not sure if using pre-comp here will have a perf impact, was this intentional?

michaelsproul · 2024-09-17T01:14:27Z

beacon_node/beacon_chain/tests/store_tests.rs

@@ -2680,7 +2679,8 @@ async fn weak_subjectivity_sync_test(slots: Vec<Slot>, checkpoint_slot: Slot) {
    let store = get_store(&temp2);
    let spec = test_spec::<E>();
    let seconds_per_slot = spec.seconds_per_slot;
-    let kzg = spec.deneb_fork_epoch.map(|_| KZG.clone());
+
+    let kzg = KZG_NO_PRECOMP.clone();


same as above

michaelsproul · 2024-09-17T01:15:16Z

beacon_node/network/src/subnet_service/tests/mod.rs

+        let kzg = if spec.deneb_fork_epoch.is_some() {
+            KZG.clone()
+        } else {
+            KZG_NO_PRECOMP.clone()
+        };


michaelsproul · 2024-09-17T01:16:48Z

crypto/kzg/src/lib.rs

@@ -51,18 +51,41 @@ impl From<c_kzg::Error> for Error {
 #[derive(Debug)]
 pub struct Kzg {
    trusted_setup: KzgSettings,
-    context: Option<DASContext>,
+    context: DASContext,


I guess we could keep this DASContext optional for the pre-peerDAS case, but then again if there's no perf impact maybe it's OK to keep it initialized

there was no noticeable performance impact that I noticed (test-wise at least)

michaelsproul · 2024-09-17T01:21:30Z

crypto/kzg/src/trusted_setup.rs

+            Self {
+                g1_monomial_points: Default::default(),
+                g1_points: Default::default(),
+                g2_points: Default::default(),
+            }


This default impl feels a little sketchy, as I imagine the inner default impls here use some junk values?

I think we could remove the need for this by making client::Config::trusted_setup either:

a Vec<u8> which is parsed on node startup, or

an Option<TrustedSetup>, which we error out on at startup if it is None

The only pace the default TrustedSetup is used is in the default client::Config impl. I've moved the trusted setup parsing into the Config default impl, which will error out if the trusted_setup file isnt found. You think that should be ok? We could alternatively change client::Config::trusted_setup to optional or a Vec as you said

eserilev added 2 commits August 15, 2024 17:02

make kzg field required

36c0a33

update todo

ed0bb55

eserilev commented Aug 16, 2024

View reviewed changes

eserilev added code-quality work-in-progress PR is a work-in-progress labels Aug 16, 2024

eserilev changed the title ~~Make BeaconChain kzg field required~~ Make BeaconChain::kzg field mandatory Aug 16, 2024

eserilev added 3 commits August 16, 2024 00:49

always load trusted setup WIP

a640f73

fmt

54a4723

use new rust_eth_kzg version

569867d

eserilev added 4 commits August 28, 2024 13:26

merge conlficts

fef6ae1

add kzg fn with trusted setup disabled

bebbcff

as_slice

6068a89

add kzg with no precomp

ddf5064

eserilev added 4 commits August 28, 2024 18:46

ignore udep for kzg

3049c9d

refactor kzg init

b2abfe6

fix peerdas kzg schedule

f2d337e

fix

9de6e0f

eserilev added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Aug 29, 2024

eserilev added 2 commits August 29, 2024 15:58

udeps

1c03920

uuuudeps

35a754a

michaelsproul added the v6.0.0 New major release for hierarchical state diffs label Sep 16, 2024

eserilev added 5 commits September 16, 2024 16:59

merge conflict resolved

08dcd52

merge conflict

17bc4b2

merge conflicts

091c2bd

resolve TODO

89f36fc

update

edf0344

michaelsproul requested changes Sep 17, 2024

View reviewed changes

michaelsproul 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 Sep 17, 2024

eserilev added 4 commits September 17, 2024 15:06

move kzg to a test util fn

af7c183

remove trusted setup default impl

7b5cebf

lint fmt

2656af4

fix failing test

8e9af45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `BeaconChain::kzg` field mandatory #6267

Make `BeaconChain::kzg` field mandatory #6267

eserilev commented Aug 16, 2024

eserilev Aug 16, 2024

michaelsproul Aug 16, 2024

dapplion Aug 16, 2024

michaelsproul Aug 16, 2024

kevaundray Aug 19, 2024

eserilev Aug 20, 2024

kevaundray Aug 20, 2024

kevaundray Aug 27, 2024

dapplion commented Aug 28, 2024

eserilev commented Aug 29, 2024 •

edited

Loading

eserilev commented Aug 29, 2024

michaelsproul Sep 17, 2024

michaelsproul Sep 17, 2024

michaelsproul Sep 17, 2024

michaelsproul Sep 17, 2024

michaelsproul Sep 17, 2024

michaelsproul Sep 17, 2024

michaelsproul Sep 17, 2024

michaelsproul Sep 17, 2024

michaelsproul Sep 17, 2024

eserilev Sep 17, 2024

michaelsproul Sep 17, 2024

eserilev Sep 17, 2024

Make BeaconChain::kzg field mandatory #6267

Are you sure you want to change the base?

Make BeaconChain::kzg field mandatory #6267

Conversation

eserilev commented Aug 16, 2024

Issue Addressed

Proposed Changes

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

dapplion commented Aug 28, 2024

eserilev commented Aug 29, 2024 • edited Loading

eserilev commented Aug 29, 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

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

Make `BeaconChain::kzg` field mandatory #6267

Make `BeaconChain::kzg` field mandatory #6267

eserilev commented Aug 29, 2024 •

edited

Loading