Skip to content
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

Adapt ckzg changes for runtime parameters #4818

Closed
wants to merge 8 commits into from

Conversation

pawanjay176
Copy link
Member

Issue Addressed

Resolves #4440

Proposed Changes

Adopt the changes in ethereum/c-kzg-4844#376 to change FIELD_ELEMENTS_PER_BLOB into a runtime instead of compile time parameter

This is a strict improvement imo to what we had in the compile time constants world (atleast for lighthouse) for the following reasons:

  1. We don't need the KzgPreset trait since the code for minimal and mainnet parameters are the same. This reduces a lot of generics that was introduced mainly to allow for working with different sized blobs in the compile type constants case
  2. Since ckzg accepts byte slices for blobs, we can just pass the ssz blob to the ckzg functions directly without any clones. This reduces the number of blobs we have to hold in memory at any given time.
    Imo, this also doesn't reduce type safety because the ssz blobs are already type checked based on if we are running with mainnet preset or mimimal preset.

The only type safety we loose is it's now possible to load a minimal trusted setup for a mainnet preset lighthouse binary. We can easily get over this issue by just introducing this check when building a beacon chain struct which would exit lighthouse on startup

if EthSpec::field_elements_per_blob() != kzg.field_elements_per_blob() {
     return Err("Trusted setup is inconsistent with spec preset".to_string())
}

We do similar checks for loading invalid genesis bytes or invalid checkpoint states, so I don't think this is a big deal. In case we want additional compile time guarantees, we should be able to wrap the ckzg::KzgSettings struct into a const generics newtype that depends on EthSpec in lighthouse.

@pawanjay176 pawanjay176 added work-in-progress PR is a work-in-progress deneb labels Oct 9, 2023
@realbigsean
Copy link
Member

realbigsean commented Oct 10, 2023

Here's what I was thinking wrt runtime integration: pawanjay176#7

If we view the Kzg struct as a bridge between lighthouse types and kzg, making it generic across EthSpec becomes natural, we can change all its methods to only accept lh types (which are already generic in this dimension). Also still doesn't require the KzgPreset generic.

  • Obviously only matters if kzg continues to require different config accross specs
  • not sure if moving it to our consensus/types crate is the right place but it has to go somewhere other than the kzg crate to avoid a circular dep
  • seems like kzg_utils.rs used to try to fill this gap, so we can dump it if we do this instead

@pawanjay176
Copy link
Member Author

not sure if moving it to our consensus/types crate is the right place but it has to go somewhere other than the kzg crate to avoid a circular dep

I don't think consensus/types is the best place either. It's unnecessary burden on downstream users of our types lib. We could feature gate it in consensus/types though.

I would like to wait for the upstream PR to get merged or dropped before deciding anything though, since there's talk about just ditching minimal blob sizes.

@michaelsproul michaelsproul deleted the branch sigp:unstable October 16, 2023 23:58
@michaelsproul michaelsproul reopened this Oct 17, 2023
@michaelsproul michaelsproul changed the base branch from deneb-free-blobs to unstable October 17, 2023 00:03
@pawanjay176
Copy link
Member Author

Closing this as ckzg moved away from runtime parameters support

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deneb work-in-progress PR is a work-in-progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants