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: Use reference to an array representing a blob instead of an owned KzgBlob (only for peerdas functions) #6179

Merged
merged 5 commits into from
Aug 1, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions beacon_node/beacon_chain/src/kzg_utils.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
use kzg::{Blob as KzgBlob, Bytes48, CellRef as KzgCellRef, Error as KzgError, Kzg};
use kzg::{Bytes48, CellRef as KzgCellRef, Error as KzgError, Kzg, KzgBlobRef};
use std::sync::Arc;
use types::data_column_sidecar::Cell;
use types::{Blob, DataColumnSidecar, EthSpec, Hash256, KzgCommitment, KzgProof};

/// Converts a blob ssz List object to an array to be used with the kzg
/// crypto library.
fn ssz_blob_to_crypto_blob<E: EthSpec>(blob: &Blob<E>) -> Result<KzgBlob, KzgError> {
KzgBlob::from_bytes(blob.as_ref()).map_err(Into::into)
fn ssz_blob_to_crypto_blob<E: EthSpec>(blob: &Blob<E>) -> Result<KzgBlobRef, KzgError> {
let blob_bytes: &[u8] = blob.as_ref();
Ok(blob_bytes.try_into().expect("expected cell to have size {BYTES_PER_BLOB}. This should be guaranteed by the `FixedVector type"))
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main change is here, we push the allocation of the blob into the kzg library

Copy link
Member

@jimmygchen jimmygchen Jul 31, 2024

Choose a reason for hiding this comment

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

Cool, could we adopt the same change for c-kzg-4844? (to prepare it for increased blob count)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think c-kzg would need to allocate under the hood -- I'll open up a PR when I get back on my laptop to track arguments for and against


/// Converts a cell ssz List object to an array to be used with the kzg
Expand All @@ -27,7 +28,7 @@ pub fn validate_blob<E: EthSpec>(
) -> Result<(), KzgError> {
let _timer = crate::metrics::start_timer(&crate::metrics::KZG_VERIFICATION_SINGLE_TIMES);
let kzg_blob = ssz_blob_to_crypto_blob::<E>(blob)?;
kzg.verify_blob_kzg_proof(&kzg_blob, kzg_commitment, kzg_proof)
kzg.verify_blob_kzg_proof(kzg_blob, kzg_commitment, kzg_proof)
}

/// Validate a batch of `DataColumnSidecar`.
Expand Down Expand Up @@ -97,7 +98,7 @@ pub fn compute_blob_kzg_proof<E: EthSpec>(
kzg_commitment: KzgCommitment,
) -> Result<KzgProof, KzgError> {
let kzg_blob = ssz_blob_to_crypto_blob::<E>(blob)?;
kzg.compute_blob_kzg_proof(&kzg_blob, kzg_commitment)
kzg.compute_blob_kzg_proof(kzg_blob, kzg_commitment)
}

/// Compute the kzg commitment for a given blob.
Expand All @@ -106,7 +107,7 @@ pub fn blob_to_kzg_commitment<E: EthSpec>(
blob: &Blob<E>,
) -> Result<KzgCommitment, KzgError> {
let kzg_blob = ssz_blob_to_crypto_blob::<E>(blob)?;
kzg.blob_to_kzg_commitment(&kzg_blob)
kzg.blob_to_kzg_commitment(kzg_blob)
}

/// Compute the kzg proof for a given blob and an evaluation point z.
Expand All @@ -117,7 +118,7 @@ pub fn compute_kzg_proof<E: EthSpec>(
) -> Result<(KzgProof, Hash256), KzgError> {
let z = z.0.into();
let kzg_blob = ssz_blob_to_crypto_blob::<E>(blob)?;
kzg.compute_kzg_proof(&kzg_blob, &z)
kzg.compute_kzg_proof(kzg_blob, &z)
.map(|(proof, z)| (proof, Hash256::from_slice(&z.to_vec())))
}

Expand Down
9 changes: 6 additions & 3 deletions consensus/types/src/data_column_sidecar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use bls::Signature;
use derivative::Derivative;
#[cfg_attr(test, double)]
use kzg::Kzg;
use kzg::{Blob as KzgBlob, CellRef as KzgCellRef, Error as KzgError};
use kzg::{CellRef as KzgCellRef, Error as KzgError};
use kzg::{KzgCommitment, KzgProof};
use merkle_proof::verify_merkle_proof;
#[cfg(test)]
Expand Down Expand Up @@ -128,8 +128,11 @@ impl<E: EthSpec> DataColumnSidecar<E> {
let blob_cells_and_proofs_vec = blobs
.into_par_iter()
.map(|blob| {
let blob = KzgBlob::from_bytes(blob).map_err(KzgError::from)?;
kzg.compute_cells_and_proofs(&blob)
let blob = blob
.as_ref()
.try_into()
.expect("blob should have a guaranteed size due to FixedVector");
kzg.compute_cells_and_proofs(blob)
Comment on lines +131 to +135
Copy link
Member

Choose a reason for hiding this comment

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

I think this is the main change that fixes the stack overflow on das testing right?

The other changes change mainnet code path - it's probably not something we can merge to the unstable branch now, although I think we do want them eventually when c-kzg adopts it or when rust-eth-kzg is production-ready for mainnet.

I think we can merge this branch to das and I'll just cherry-pick the changes relevant to compute_cells_and_proofs onto unstable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep this is correct

})
.collect::<Result<Vec<_>, KzgError>>()?;

Expand Down
54 changes: 37 additions & 17 deletions crypto/kzg/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,11 @@ pub use peerdas_kzg::{
Cell, CellIndex as CellID, CellRef, TrustedSetup as PeerDASTrustedSetup,
};
use peerdas_kzg::{prover::ProverError, verifier::VerifierError, PeerDASContext};

pub type CellsAndKzgProofs = ([Cell; CELLS_PER_EXT_BLOB], [KzgProof; CELLS_PER_EXT_BLOB]);

pub type KzgBlobRef<'a> = &'a [u8; BYTES_PER_BLOB];

#[derive(Debug)]
pub enum Error {
/// An error from the underlying kzg library.
Expand Down Expand Up @@ -75,25 +78,29 @@ impl Kzg {
}

/// Compute the kzg proof given a blob and its kzg commitment.
pub fn compute_blob_kzg_proof(
#[allow(clippy::needless_lifetimes)]
pub fn compute_blob_kzg_proof<'a>(
&self,
blob: &Blob,
blob: KzgBlobRef<'a>,
kzg_commitment: KzgCommitment,
) -> Result<KzgProof, Error> {
c_kzg::KzgProof::compute_blob_kzg_proof(blob, &kzg_commitment.into(), &self.trusted_setup)
let blob = Blob::from_bytes(blob)?;
c_kzg::KzgProof::compute_blob_kzg_proof(&blob, &kzg_commitment.into(), &self.trusted_setup)
.map(|proof| KzgProof(proof.to_bytes().into_inner()))
.map_err(Into::into)
}

/// Verify a kzg proof given the blob, kzg commitment and kzg proof.
pub fn verify_blob_kzg_proof(
#[allow(clippy::needless_lifetimes)]
pub fn verify_blob_kzg_proof<'a>(
&self,
blob: &Blob,
blob: KzgBlobRef<'a>,
kzg_commitment: KzgCommitment,
kzg_proof: KzgProof,
) -> Result<(), Error> {
let blob = Blob::from_bytes(blob)?;
if !c_kzg::KzgProof::verify_blob_kzg_proof(
blob,
&blob,
&kzg_commitment.into(),
&kzg_proof.into(),
&self.trusted_setup,
Expand All @@ -108,9 +115,10 @@ impl Kzg {
///
/// Note: This method is slightly faster than calling `Self::verify_blob_kzg_proof` in a loop sequentially.
/// TODO(pawan): test performance against a parallelized rayon impl.
pub fn verify_blob_kzg_proof_batch(
#[allow(clippy::needless_lifetimes)]
pub fn verify_blob_kzg_proof_batch<'a>(
&self,
blobs: &[Blob],
blobs: &[KzgBlobRef<'a>],
kzg_commitments: &[KzgCommitment],
kzg_proofs: &[KzgProof],
) -> Result<(), Error> {
Expand All @@ -124,8 +132,12 @@ impl Kzg {
.map(|proof| Bytes48::from(*proof))
.collect::<Vec<_>>();

let blobs = blobs
.iter()
.map(|blob| Blob::from_bytes(blob.as_slice()))
.collect::<Result<Vec<_>, _>>()?;
if !c_kzg::KzgProof::verify_blob_kzg_proof_batch(
blobs,
&blobs,
&commitments_bytes,
&proofs_bytes,
&self.trusted_setup,
Expand All @@ -137,19 +149,23 @@ impl Kzg {
}

/// Converts a blob to a kzg commitment.
pub fn blob_to_kzg_commitment(&self, blob: &Blob) -> Result<KzgCommitment, Error> {
c_kzg::KzgCommitment::blob_to_kzg_commitment(blob, &self.trusted_setup)
#[allow(clippy::needless_lifetimes)]
pub fn blob_to_kzg_commitment<'a>(&self, blob: KzgBlobRef<'a>) -> Result<KzgCommitment, Error> {
let blob = Blob::from_bytes(blob)?;
c_kzg::KzgCommitment::blob_to_kzg_commitment(&blob, &self.trusted_setup)
.map(|commitment| KzgCommitment(commitment.to_bytes().into_inner()))
.map_err(Into::into)
}

/// Computes the kzg proof for a given `blob` and an evaluation point `z`
pub fn compute_kzg_proof(
#[allow(clippy::needless_lifetimes)]
pub fn compute_kzg_proof<'a>(
&self,
blob: &Blob,
blob: KzgBlobRef<'a>,
z: &Bytes32,
) -> Result<(KzgProof, Bytes32), Error> {
c_kzg::KzgProof::compute_kzg_proof(blob, z, &self.trusted_setup)
let blob = Blob::from_bytes(blob)?;
c_kzg::KzgProof::compute_kzg_proof(&blob, z, &self.trusted_setup)
.map(|(proof, y)| (KzgProof(proof.to_bytes().into_inner()), y))
.map_err(Into::into)
}
Expand All @@ -173,7 +189,11 @@ impl Kzg {
}

/// Computes the cells and associated proofs for a given `blob` at index `index`.
pub fn compute_cells_and_proofs(&self, blob: &Blob) -> Result<CellsAndKzgProofs, Error> {
#[allow(clippy::needless_lifetimes)]
pub fn compute_cells_and_proofs<'a>(
&self,
blob: KzgBlobRef<'a>,
) -> Result<CellsAndKzgProofs, Error> {
let blob_bytes: &[u8; BYTES_PER_BLOB] = blob
Copy link
Member

Choose a reason for hiding this comment

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

I think this conversion can now be removed as KzgBlobRef is the exact same type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yep that's true, this conversion is infallible

.as_ref()
.try_into()
Expand Down Expand Up @@ -245,11 +265,11 @@ impl Kzg {
}

pub mod mock {
use crate::{Blob, Cell, CellsAndKzgProofs, BYTES_PER_CELL, CELLS_PER_EXT_BLOB};
use crate::{Cell, CellsAndKzgProofs, KzgBlobRef, BYTES_PER_CELL, CELLS_PER_EXT_BLOB};
use crate::{Error, KzgProof};

#[allow(clippy::type_complexity)]
pub fn compute_cells_and_proofs(_blob: &Blob) -> Result<CellsAndKzgProofs, Error> {
pub fn compute_cells_and_proofs(_blob: KzgBlobRef) -> Result<CellsAndKzgProofs, Error> {
let empty_cells = vec![Cell::new([0; BYTES_PER_CELL]); CELLS_PER_EXT_BLOB]
.try_into()
.expect("expected {CELLS_PER_EXT_BLOB} number of items");
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use super::*;
use crate::case_result::compare_result;
use kzg::{Blob as KzgBlob, CellsAndKzgProofs};
use kzg::CellsAndKzgProofs;
use serde::Deserialize;
use std::marker::PhantomData;

Expand Down Expand Up @@ -32,10 +32,10 @@ impl<E: EthSpec> Case for KZGComputeCellsAndKZGProofs<E> {

fn result(&self, _case_index: usize, _fork_name: ForkName) -> Result<(), Error> {
let cells_and_proofs = parse_blob::<E>(&self.input.blob).and_then(|blob| {
let blob = KzgBlob::from_bytes(&blob).map_err(|e| {
let blob = blob.as_ref().try_into().map_err(|e| {
Error::InternalError(format!("Failed to convert blob to kzg blob: {e:?}"))
})?;
KZG.compute_cells_and_proofs(&blob).map_err(|e| {
KZG.compute_cells_and_proofs(blob).map_err(|e| {
Error::InternalError(format!("Failed to compute cells and kzg proofs: {e:?}"))
})
});
Expand Down