-
Notifications
You must be signed in to change notification settings - Fork 725
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
Make BeaconChain::kzg
field mandatory
#6267
base: unstable
Are you sure you want to change the base?
Conversation
beacon_node/client/src/builder.rs
Outdated
} | ||
} else { | ||
// TODO(kzg) this causes us failed tests i.e. http_server_genesis_state | ||
panic!("Failed to load KZG"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should try loading the KZG setup regardless of whether the network is pre-Deneb (if possible).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't that slow things significantly? Post PeerDAS loading KZG is slow given the precomputation optimization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened #6309 and added you as a co-author to the commits!
BeaconChain
kzg field requiredBeaconChain::kzg
field mandatory
This PR adds a new trusted setup, is that expected? |
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 |
There no longer seems to be any noticeable difference in CI times between this branch and unstable |
let kzg = self | ||
.kzg | ||
.as_ref() | ||
.ok_or(BlockProductionError::TrustedSetupNotInitialized)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can delete this error
let kzg = if spec.deneb_fork_epoch.is_some() { | ||
KZG.clone() | ||
} else { | ||
KZG_NO_PRECOMP.clone() | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
let kzg = if spec.deneb_fork_epoch.is_some() { | ||
KZG.clone() | ||
} else { | ||
KZG_NO_PRECOMP.clone() | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can use the same new method here
.kzg | ||
.clone() | ||
.ok_or(GossipDataColumnError::KzgNotInitialized)?; | ||
let kzg = chain.kzg.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really an issue, but I don't think this clone is necessary
let kzg = if spec.deneb_fork_epoch.is_some() { | ||
KZG.clone() | ||
} else { | ||
KZG_NO_PRECOMP.clone() | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can use the same func here
@@ -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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same func here
not sure if using pre-comp here will have a perf impact, was this intentional?
@@ -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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
let kzg = if spec.deneb_fork_epoch.is_some() { | ||
KZG.clone() | ||
} else { | ||
KZG_NO_PRECOMP.clone() | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func
@@ -51,18 +51,41 @@ impl From<c_kzg::Error> for Error { | |||
#[derive(Debug)] | |||
pub struct Kzg { | |||
trusted_setup: KzgSettings, | |||
context: Option<DASContext>, | |||
context: DASContext, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there was no noticeable performance impact that I noticed (test-wise at least)
crypto/kzg/src/trusted_setup.rs
Outdated
Self { | ||
g1_monomial_points: Default::default(), | ||
g1_points: Default::default(), | ||
g2_points: Default::default(), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 isNone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Issue Addressed
Closes #6260
Proposed Changes
Make the kzg field in
BeaconChain
andBeaconChainBuilder
mandatory