-
Notifications
You must be signed in to change notification settings - Fork 896
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
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 |
Hi @cakevm, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time. |
I did now some research and guess I have maybe found something, that could cause this slower runtime in the tests with version 2. It looks like that the initialization behavior has changed to always init FK20 and support cell operations, which would not be required here. But lead to a longer init time. For more details see: ethereum/c-kzg-4844#580 |
Hi @cakevm, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time. |
I would still like to merge this |
# Conflicts: # Cargo.lock # Cargo.toml
This pull request has been removed from the queue for the following reason: Pull request #7271 has been dequeued. The pull request could not be merged. This could be related to an activated branch protection or ruleset rule that prevents us from merging. (details: 1 review requesting changes and 1 approving review by reviewers with write access.). You can check the last failing draft PR here: #7712. You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. |
@mergify requeue |
✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically |
I don't see why it failed, retrying |
This pull request has been removed from the queue for the following reason: The merge conditions cannot be satisfied due to failing checks: You can check the last failing draft PR here: #7714. You may have to fix your CI before adding the pull request to the queue again. |
This pull request has been removed from the queue for the following reason: Pull request #7271 has been dequeued. The pull request could not be merged. This could be related to an activated branch protection or ruleset rule that prevents us from merging. (details: 1 review requesting changes and 1 approving review by reviewers with write access.). You can check the last failing draft PR here: #7715. You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. |
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)