lighthouse icon indicating copy to clipboard operation
lighthouse copied to clipboard

Upgrade to c-kzg 2.1.0 and alloy-primitives 1.0

Open cakevm opened this issue 8 months ago • 5 comments

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: https://github.com/alloy-rs/alloy/pull/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: https://github.com/sigp/lighthouse/pull/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: https://github.com/sigp/lighthouse/issues/6107#issuecomment-2632542447

cakevm avatar Apr 07 '25 09:04 cakevm

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Apr 07 '25 09:04 CLAassistant

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.

cakevm avatar Apr 08 '25 12:04 cakevm

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 avatar Apr 23 '25 01:04 michaelsproul

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[bot] avatar May 29 '25 14:05 mergify[bot]

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: https://github.com/ethereum/c-kzg-4844/issues/580

cakevm avatar Jun 12 '25 07:06 cakevm

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[bot] avatar Jun 29 '25 01:06 mergify[bot]

I would still like to merge this

michaelsproul avatar Jun 29 '25 02:06 michaelsproul

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.

mergify[bot] avatar Jul 08 '25 02:07 mergify[bot]

@mergify requeue

michaelsproul avatar Jul 08 '25 04:07 michaelsproul

requeue

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

mergify[bot] avatar Jul 08 '25 04:07 mergify[bot]

I don't see why it failed, retrying

michaelsproul avatar Jul 08 '25 04:07 michaelsproul

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] avatar Jul 08 '25 06:07 mergify[bot]

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.

mergify[bot] avatar Jul 08 '25 08:07 mergify[bot]