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

chore: Add c-kzg library behind a runtime flag #6119

Draft
wants to merge 6 commits into
base: das
Choose a base branch
from

Conversation

kevaundray
Copy link
Contributor

Issue Addressed

This adds back the c-kzg library (updated to the latest commit).

This PR is reliant on #6117 since the new c-kzg API requires g1_monomial. Once that gets merged, then the lines modified should reduce.

Proposed Changes

Please list or describe the changes introduced by this PR.

Additional Info

Please provide any additional information. For example, future considerations
or information useful for reviewers.

@kevaundray
Copy link
Contributor Author

This closes #6107 though note that we cannot have two c-kzg libraries in this repository because the c-kzg library links to a c static lib, so the mainnet path will always be "affected" -- Another solution would be to maintain a branch that gets merged into stable which has the rust library and the c-kzg-4844 code. Then another branch which is used for peerdas which has both c-kzg-peerdas and the rust lib. This mainly just imposes a cost to maintaining both branches.

Comment on lines -177 to +211
let blob_bytes: &[u8; BYTES_PER_BLOB] = blob
.as_ref()
.try_into()
.expect("Expected blob to have size {BYTES_PER_BLOB}");
if self.use_ckzg {
let (cells, proofs) =
c_kzg::Cell::compute_cells_and_kzg_proofs(blob, &self.trusted_setup)?;

let (cells, proofs) = self
.context
.prover_ctx()
.compute_cells_and_kzg_proofs(blob_bytes)
.map_err(Error::ProverKZG)?;
let cells = cells.map(|c| Box::new(c.to_bytes()));
let proofs = proofs.map(|p| KzgProof::from(p.to_bytes().into_inner()));

// Convert the proof type to a c-kzg proof type
let c_kzg_proof = proofs.map(KzgProof);
Ok((cells, c_kzg_proof))
Ok((cells, proofs))
} else {
let blob_bytes: &[u8; BYTES_PER_BLOB] = blob
.as_ref()
.try_into()
.expect("Expected blob to have size {BYTES_PER_BLOB}");

let (cells, proofs) = self
.context
.prover_ctx()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally two new files are created for these two codepaths

Comment on lines +75 to +77
&trusted_setup.g1_monomial_points(),
&trusted_setup.g1_lagrange_points(),
&trusted_setup.g2_monomial_points(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we changed this from a list of points to a list of flat bytes, I think we should update the names.

Suggested change
&trusted_setup.g1_monomial_points(),
&trusted_setup.g1_lagrange_points(),
&trusted_setup.g2_monomial_points(),
&trusted_setup.g1_monomial_bytes(),
&trusted_setup.g1_lagrange_bytes(),
&trusted_setup.g2_monomial_bytes(),

Comment on lines +23 to +39
pub fn g1_lagrange_points(&self) -> Vec<u8> {
self.g1_lagrange_points.iter().flat_map(|p| {
let stripped = strip_prefix(p);
hex::decode(stripped).expect("expected g1 lagrange points to be well formed hex strings")
}).collect()
}
pub fn g1_monomial_points(&self) -> Vec<u8> {
self.g1_monomial_points.iter().flat_map(|p| {
let stripped = strip_prefix(p);
hex::decode(stripped).expect("expected g1 monomial points to be well formed hex strings")
}).collect()
}
pub fn g2_monomial_points(&self) -> Vec<u8> {
self.g2_monomial_points.iter().flat_map(|p|{
let stripped = strip_prefix(p);
hex::decode(stripped).expect("expected g2 monomial points to be well formed hex strings")
}).collect()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit, I've been trying to use consistent order for these: g1_monomial, g1_lagrange, g2_monomial.

Can we move the middle function to the top?

Comment on lines +215 to +217
let c_kzg_proof = proofs.map(KzgProof);

Ok((cells, c_kzg_proof))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be plural. Plus minor c_kzg vs ckzg nit. Going with ckzg, like use_ckzg.

Suggested change
let c_kzg_proof = proofs.map(KzgProof);
Ok((cells, c_kzg_proof))
let ckzg_proofs = proofs.map(KzgProof);
Ok((cells, ckzg_proofs))

@jimmygchen
Copy link
Member

jimmygchen commented Jul 22, 2024

@kevaundray I had a chat with @michaelsproul and he suggested that it might not be a good idea to have two versions of c-kzg - we had some issues with it before and had to rename a bunch of things to avoid symbol collisions.

So it might be better to hold off on this one and add this switch once the c-kzg main version supports PeerDAS. Right now we're going to focus on getting our das stuff onto unstable (main dev branch).

@mergify mergify bot deleted the branch sigp:das August 27, 2024 04:10
@mergify mergify bot closed this Aug 27, 2024
@michaelsproul michaelsproul reopened this Aug 27, 2024
@michaelsproul michaelsproul changed the base branch from das to unstable August 27, 2024 04:20
@michaelsproul michaelsproul changed the base branch from unstable to das August 27, 2024 05:30
@michaelsproul
Copy link
Member

Now that we've merged das to unstable please update to point at unstable by either:

  1. Rebasing on unstable (if your branch has a small number of commits that are easy to tease out), or
  2. Merging origin/das into this PR: git fetch origin; git merge origin/das. This will result in the smallest number of conflict resolutions and is better for branches that already contain merge commits or have extensive history.

@jimmygchen jimmygchen added the work-in-progress PR is a work-in-progress label Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
das Data Availability Sampling work-in-progress PR is a work-in-progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants