-
Notifications
You must be signed in to change notification settings - Fork 871
Upgrade to c-kzg 2.1.0 and alloy-primitives 1.0 #7271
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
base: unstable
Are you sure you want to change the base?
Conversation
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.
Looking good
Not sure how urgently we want this to be in unstable. I would prefer not to have this in any of the stable releases that go into the pectra releases for now
Thank you for taking the time to have a look. It’s not a priority. I came across this as a transient dependency and initially thought updating the library would be a quick win. But given the issues that have come up, that doesn’t seem to be the case. I’m fine with either closing this PR or leaving it open until there’s a decision on whether c-kzg will be removed. |
I'm going to try a narrower and more targeted |
/// `recover_cells_and_kzg_proofs` are not used. | ||
/// | ||
/// See: <https://github.com/ethereum/c-kzg-4844/pull/545/files> | ||
pub const NO_PRECOMPUTE: u64 = 0; |
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.
Note that we don't currently use c-kzg4844
for PeerDAS, so the methods above from c-kzg aren't being used. We are using the rust-eth-kzg
for PeerDAS support, and we're already doing precomputation there:
lighthouse/crypto/kzg/src/lib.rs
Lines 95 to 100 in 6ee8a7d
let context = DASContext::new( | |
&peerdas_trusted_setup, | |
rust_eth_kzg::UsePrecomp::Yes { | |
width: rust_eth_kzg::constants::RECOMMENDED_PRECOMP_WIDTH, | |
}, | |
); |
We should be able to just use NO_PRECOMPUTE
for c-kzg until we decide to support using c-kzg for PeerDAS (#6107). Otherwise we end up doing precomputation for both libraries, and it would slow down startup time and double the memory usage to cache the precomputation results.
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.
Good catch! The simulator tests are failing, and it's probably because of the startup time regression.
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.
Right, from the link provided, it looks like it still takes 1.69s to precompute, even when precompute is set to 0:
https://github.com/ethereum/c-kzg-4844/pull/545/files
I think it would be ok to bump up genesis delay slightly to make this work.
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.
@jimmygchen Awesome! I absolutely missed that. Thank you for the insights about the different kzg libs. I adapted it to use always NO_PRECOMPUTE
and increased the GENESIS_DELAY
. Using now 38
instead of 32
seems to be the sweet spot.
Status: Sadly upgrading
c-kzg
brings a performance degradation that let the simulator CI job failing. IncreasingGENESIS_DELAY
helps, but I wonder why this is happening now.Issue Addressed
Update
c-kzg
fromv1
tov2
. My motivation here is thatalloy-consensus
now usesc-kzg
inv2
and this results in a conflict when using lighthouse in combination with latest alloy. I tried also to disable theczkg
feature in alloy, but the conflict persisted.See here for the alloy update to
c-kzg v2
: alloy-rs/alloy#2240Error:
Proposed Changes
alloy-consensus
to0.14.0
and disable all default featuresc-kzg
tov2.1.0
alloy-primitives
to1.0.0
c-kzg
NO_PRECOMPUTE
as my understand from https://github.com/ethereum/c-kzg-4844/pull/545/files we should use0
here asnew_from_trusted_setup_no_precomp
does not precomp. But maybe it is misleading. For all other places I usedRECOMMENDED_PRECOMP_WIDTH
because8
is matching the recommendation.BYTES_PER_G1_POINT
andBYTES_PER_G2_POINT
are no longer public inc-kzg
Attestation
bitfield size. But I could not pinpoint to what has changed and why now 8 bytes less. I would be happy about any hint, and if this is correct. I found related a PR here: Bump SSZ version for larger bitfieldSmallVec
#6915c-kzg
andrust_eth_kzg
forg1_monomial
,g1_lagrange
, andg2_monomial
Additional Info
KzgSettings
seems suboptimal. The whole data structure could be already prepared for this like inc-kzg
.c-kzg
lib would provide already those constants (e.g. withc_kzg::ethereum_kzg_settings
) and maybe theTrustedSetup
could be simplified in the future (e.g. removing it and provide the user to load customKzgSettings
from json.PS: I did not tested this with a real network, I hope that the test coverage from other crates would highlight if kzg is broken with this change. I just compared by hand that (e.g.
g1_monomial
) is correctly read. I am open to add test for the correct loading of thekzg
, but maybe it make sense to seek to a deeper integration ofc-kzg
in another step, because it requires much more changes. Or it will be removed with this issue here: #6107 (comment)