Skip to content

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

Merged
merged 16 commits into from
Jul 9, 2025

Conversation

cakevm
Copy link

@cakevm cakevm commented Apr 7, 2025

Status: Sadly upgrading c-kzg brings a performance degradation that let the simulator CI job failing. Increasing GENESIS_DELAY helps, but I wonder why this is happening now.

Issue Addressed

Update c-kzg from v1 to v2. My motivation here is that alloy-consensus now uses c-kzg in v2 and this results in a conflict when using lighthouse in combination with latest alloy. I tried also to disable the czkg feature in alloy, but the conflict persisted.

See here for the alloy update to c-kzg v2: alloy-rs/alloy#2240

Error:

error: failed to select a version for `c-kzg`.
...
versions that meet the requirements `^1` are: 1.0.3, 1.0.2, 1.0.0

the package `c-kzg` links to the native library `ckzg`, but it conflicts with a previous package which links to `ckzg` as well:
package `c-kzg v2.1.0`
    ... which satisfies dependency `c-kzg = "^2.1"` of package `alloy-consensus v0.13.0`
    ... which satisfies dependency `alloy-consensus = "^0.13.0"` of package ...
...

Proposed Changes

  • Upgrade alloy-consensus to 0.14.0 and disable all default features
  • Upgrade c-kzg to v2.1.0
  • Upgrade alloy-primitives to 1.0.0
  • Adapt the code to the new API c-kzg
  • There is now NO_PRECOMPUTE as my understand from https://github.com/ethereum/c-kzg-4844/pull/545/files we should use 0 here as new_from_trusted_setup_no_precomp does not precomp. But maybe it is misleading. For all other places I used RECOMMENDED_PRECOMP_WIDTH because 8 is matching the recommendation.
  • BYTES_PER_G1_POINT and BYTES_PER_G2_POINT are no longer public in c-kzg
  • I adapted two tests that checking for the 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 bitfield SmallVec #6915
  • Use same fields names, in json, as well as c-kzg and rust_eth_kzg for g1_monomial, g1_lagrange, and g2_monomial

Additional Info

  • The copy and flattening into the data structure for the KzgSettings seems suboptimal. The whole data structure could be already prepared for this like in c-kzg.
  • It seem that the c-kzg lib would provide already those constants (e.g. with c_kzg::ethereum_kzg_settings) and maybe the TrustedSetup could be simplified in the future (e.g. removing it and provide the user to load custom KzgSettings 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 the kzg, but maybe it make sense to seek to a deeper integration of c-kzg in another step, because it requires much more changes. Or it will be removed with this issue here: #6107 (comment)

@CLAassistant
Copy link

CLAassistant commented Apr 7, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@pawanjay176 pawanjay176 left a 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

@cakevm
Copy link
Author

cakevm commented Apr 8, 2025

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.

@michaelsproul
Copy link
Member

I'm going to try a narrower and more targeted cargo update, as the Cargo.lock file is currently very noisy and hard to review.

@michaelsproul michaelsproul changed the title Upgrade to c-kzg 2.1.0 Upgrade to c-kzg 2.1.0 and alloy-primitives 1.0 Apr 23, 2025
@jimmygchen jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. dependencies Pull requests that update a dependency file labels Apr 29, 2025
@mergify mergify bot closed this May 29, 2025
Copy link

mergify bot commented May 29, 2025

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.

@mergify mergify bot added the stale Stale PRs that have been inactive and is now outdated label May 29, 2025
@cakevm
Copy link
Author

cakevm commented Jun 12, 2025

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

@mergify mergify bot closed this Jun 29, 2025
Copy link

mergify bot commented Jun 29, 2025

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.

@michaelsproul michaelsproul reopened this Jun 29, 2025
@michaelsproul
Copy link
Member

I would still like to merge this

@jimmygchen jimmygchen self-assigned this Jul 8, 2025
@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jul 8, 2025
mergify bot added a commit that referenced this pull request Jul 8, 2025
Copy link

mergify bot commented Jul 8, 2025

This pull request has been removed from the queue for the following reason: pull request dequeued.

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.
If you do update this pull request, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@michaelsproul
Copy link
Member

@mergify requeue

Copy link

mergify bot commented Jul 8, 2025

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

@michaelsproul
Copy link
Member

I don't see why it failed, retrying

mergify bot added a commit that referenced this pull request Jul 8, 2025
Copy link

mergify bot commented Jul 8, 2025

This pull request has been removed from the queue for the following reason: checks failed.

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.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

mergify bot added a commit that referenced this pull request Jul 8, 2025
mergify bot added a commit that referenced this pull request Jul 8, 2025
Copy link

mergify bot commented Jul 8, 2025

This pull request has been removed from the queue for the following reason: pull request dequeued.

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.
If you do update this pull request, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@jimmygchen jimmygchen dismissed pawanjay176’s stale review July 9, 2025 04:25

comments addressed

mergify bot added a commit that referenced this pull request Jul 9, 2025
@mergify mergify bot merged commit 734ad90 into sigp:unstable Jul 9, 2025
33 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file ready-for-merge This PR is ready to merge. stale Stale PRs that have been inactive and is now outdated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants