Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
14 changes: 10 additions & 4 deletions gkr_engine/src/poly_commit/definition.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
use polynomials::MultilinearExtension;
use rand::RngCore;
use serdes::ExpSerde;
use std::io::{BufReader, BufWriter};
use std::{fmt::Debug, str::FromStr};

use crate::{ExpErrors, ExpanderSingleVarChallenge, FieldEngine, MPIEngine, Transcript};

/// Buffer capacity for SRS file I/O (64 MB).
const SRS_IO_BUFFER_CAPACITY: usize = 64 * 1024 * 1024;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

A 64MB buffer capacity for BufReader and BufWriter is quite large and likely excessive. Since the SRS data is mostly read or written in large chunks (via read_exact or write_all), BufReader/BufWriter will often bypass the internal buffer for these large operations. A smaller, more standard buffer size (e.g., 64KB or 1MB) would be more memory-efficient without sacrificing performance.


pub trait StructuredReferenceString {
type PKey: Clone + Debug + ExpSerde + Send + Sync + 'static;
type VKey: Clone + Debug + ExpSerde + Send + Sync + 'static;
Expand Down Expand Up @@ -63,18 +67,20 @@ pub trait ExpanderPCS<F: FieldEngine> {
match path {
Some(path) => {
match std::fs::File::open(path) {
Ok(mut file) => {
Ok(file) => {
// file exists; deserialize SRS from file
Self::SRS::deserialize_from(&mut file).unwrap_or_else(|_| {
let mut reader = BufReader::with_capacity(SRS_IO_BUFFER_CAPACITY, file);
Self::SRS::deserialize_from(&mut reader).unwrap_or_else(|_| {
panic!("Failed to deserialize SRS for {} PCS", Self::NAME)
})
}
Err(_e) => {
// file does not exist; generate SRS and store to file
let srs = Self::gen_srs(params, mpi_engine, rng);
let mut file =
let file =
std::fs::File::create(path).expect("Failed to create SRS file");
srs.serialize_into(&mut file)
let mut writer = BufWriter::with_capacity(SRS_IO_BUFFER_CAPACITY, file);
srs.serialize_into(&mut writer)
.expect("Failed to serialize SRS to file");
srs
}
Expand Down
79 changes: 78 additions & 1 deletion poly_commit/src/kzg/uni_kzg/structs_kzg.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
use derivative::Derivative;
use gkr_engine::StructuredReferenceString;
use halo2curves::group::prime::PrimeCurveAffine;
use halo2curves::group::UncompressedEncoding;
use halo2curves::{pairing::Engine, CurveAffine};
use rayon::prelude::*;
use serdes::{ExpSerde, SerdeResult};
use std::io::{Read, Write};

#[derive(Clone, Copy, Debug, PartialEq, Eq, Derivative)]
#[derivative(Default(bound = ""))]
Expand Down Expand Up @@ -34,7 +38,7 @@ where

/// Structured reference string for univariate KZG polynomial commitment scheme.
/// The univariate polynomial here is of coefficient form.
#[derive(Clone, Debug, PartialEq, Eq, Derivative, ExpSerde)]
#[derive(Clone, Debug, PartialEq, Eq, Derivative)]
#[derivative(Default(bound = ""))]
pub struct CoefFormUniKZGSRS<E: Engine>
where
Expand All @@ -48,6 +52,79 @@ where
pub tau_g2: E::G2Affine,
}

/// Custom ExpSerde implementation with parallel deserialization for fast SRS loading.
/// Uses UNCOMPRESSED format to avoid expensive square root computation during load.
impl<E: Engine> ExpSerde for CoefFormUniKZGSRS<E>
where
E::G1Affine: ExpSerde
+ CurveAffine<ScalarExt = E::Fr, CurveExt = E::G1>
+ UncompressedEncoding
+ Send
+ Sync,
Comment on lines +59 to +63

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The ExpSerde trait bound on E::G1Affine appears to be redundant. This custom implementation of ExpSerde for CoefFormUniKZGSRS serializes powers_of_tau using to_uncompressed and does not rely on an ExpSerde implementation for E::G1Affine. Removing this bound would make the trait constraints more precise.

    E::G1Affine: CurveAffine<ScalarExt = E::Fr, CurveExt = E::G1>
        + UncompressedEncoding
        + Send
        + Sync,

E::G2Affine: CurveAffine<ScalarExt = E::Fr, CurveExt = E::G2> + ExpSerde,
{
fn serialize_into<W: Write>(&self, mut writer: W) -> SerdeResult<()> {
// Serialize length
self.powers_of_tau.len().serialize_into(&mut writer)?;

// Get uncompressed point size
let point_size =
std::mem::size_of::<<E::G1Affine as UncompressedEncoding>::Uncompressed>();
let total_size = self.powers_of_tau.len() * point_size;

// Pre-allocate buffer and convert all points to uncompressed format in parallel
let mut buffer = vec![0u8; total_size];
buffer
.par_chunks_mut(point_size)
.zip(self.powers_of_tau.par_iter())
.for_each(|(chunk, point)| {
let uncompressed = point.to_uncompressed();
chunk.copy_from_slice(uncompressed.as_ref());
});

// Write all at once
writer.write_all(&buffer)?;

// Serialize tau_g2
self.tau_g2.serialize_into(&mut writer)?;
Ok(())
}

fn deserialize_from<R: Read>(mut reader: R) -> SerdeResult<Self> {
// Read length
let len = usize::deserialize_from(&mut reader)?;

// Uncompressed G1 point size (e.g. BN256: 64 bytes)
let point_size =
std::mem::size_of::<<E::G1Affine as UncompressedEncoding>::Uncompressed>();
let total_bytes = len * point_size;

let mut buffer = vec![0u8; total_bytes];
reader.read_exact(&mut buffer)?;

// Parse points in parallel - using from_uncompressed_unchecked (no square root needed)
let powers_of_tau: Vec<E::G1Affine> = buffer
.par_chunks(point_size)
.map(|chunk| {
let mut uncompressed =
<E::G1Affine as UncompressedEncoding>::Uncompressed::default();
uncompressed.as_mut().copy_from_slice(chunk);
E::G1Affine::from_uncompressed_unchecked(&uncompressed)
.into_option()
.ok_or(serdes::SerdeError::DeserializeError)
Comment on lines +112 to +114

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using from_uncompressed_unchecked skips critical validation checks, such as ensuring the point lies on the curve and belongs to the correct subgroup. While this is a significant performance optimization for loading trusted SRS files, it introduces a security risk if the SRS file is tampered with or loaded from an untrusted source. Consider adding a comment warning about this or providing a way to perform validation if needed.

})
.collect::<SerdeResult<Vec<_>>>()?;

// Read tau_g2
let tau_g2 = E::G2Affine::deserialize_from(&mut reader)?;

Ok(Self {
powers_of_tau,
tau_g2,
})
}
}

impl<E: Engine> StructuredReferenceString for CoefFormUniKZGSRS<E>
where
<E as Engine>::G1Affine: ExpSerde + CurveAffine<ScalarExt = E::Fr, CurveExt = E::G1>,
Expand Down
28 changes: 27 additions & 1 deletion poly_commit/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use gkr_engine::{
};
use polynomials::{MultiLinearPoly, MultilinearExtension, MutableMultilinearExtension};

/// Initialize PCS for testing without SRS caching (always regenerates SRS)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The docstring states that this function initializes PCS 'without SRS caching', but the implementation was changed to use a hardcoded path in /tmp, which enables caching. This documentation should be updated to reflect the actual behavior.

Suggested change
/// Initialize PCS for testing without SRS caching (always regenerates SRS)
/// Initialize PCS for testing with SRS caching

#[allow(clippy::type_complexity)]
pub fn expander_pcs_init_testing_only<FieldConfig: FieldEngine, PCS: ExpanderPCS<FieldConfig>>(
n_input_vars: usize,
Expand All @@ -14,19 +15,44 @@ pub fn expander_pcs_init_testing_only<FieldConfig: FieldEngine, PCS: ExpanderPCS
<PCS::SRS as StructuredReferenceString>::PKey,
<PCS::SRS as StructuredReferenceString>::VKey,
PCS::ScratchPad,
) {
let srs_path = format!("/tmp/kzg_srs_{}.bin", n_input_vars);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using a hardcoded path in /tmp with a predictable filename is insecure on multi-user systems as it is vulnerable to symlink attacks. Additionally, it can lead to permission issues or collisions if multiple users run the same tests. It is recommended to use a more secure location or at least use std::env::temp_dir() to respect environment variables and improve cross-platform compatibility.

expander_pcs_init_with_srs_path::<FieldConfig, PCS>(n_input_vars, mpi_config, Some(&srs_path))
Comment on lines +19 to +20

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Hardcoding the temporary path to /tmp is not portable and will fail on non-Unix-like systems such as Windows. It's better to use std::env::temp_dir() to get a platform-specific temporary directory.

Suggested change
let srs_path = format!("/tmp/kzg_srs_{}.bin", n_input_vars);
expander_pcs_init_with_srs_path::<FieldConfig, PCS>(n_input_vars, mpi_config, Some(&srs_path))
let srs_path = std::env::temp_dir().join(format!("kzg_srs_{}.bin", n_input_vars));
expander_pcs_init_with_srs_path::<FieldConfig, PCS>(n_input_vars, mpi_config, srs_path.to_str())

}

/// Initialize PCS with optional SRS file caching
///
/// # Arguments
/// * `n_input_vars` - Number of input variables (determines SRS size as 2^n)
/// * `mpi_config` - MPI configuration
/// * `srs_path` - Optional path to SRS cache file:
/// - `Some(path)`: If file exists, load SRS from it; otherwise generate and save to it
/// - `None`: Always regenerate SRS (no caching)
#[allow(clippy::type_complexity)]
pub fn expander_pcs_init_with_srs_path<FieldConfig: FieldEngine, PCS: ExpanderPCS<FieldConfig>>(
n_input_vars: usize,
mpi_config: &impl MPIEngine,
srs_path: Option<&str>,
) -> (
PCS::Params,
<PCS::SRS as StructuredReferenceString>::PKey,
<PCS::SRS as StructuredReferenceString>::VKey,
PCS::ScratchPad,
) {
let mut rng = test_rng();

let pcs_params =
<PCS as ExpanderPCS<FieldConfig>>::gen_params(n_input_vars, mpi_config.world_size());

let pcs_setup = <PCS as ExpanderPCS<FieldConfig>>::gen_or_load_srs_for_testing(
&pcs_params,
mpi_config,
&mut rng,
None,
srs_path,
);

let (pcs_proving_key, pcs_verification_key) = pcs_setup.into_keys();

let pcs_scratch = <PCS as ExpanderPCS<FieldConfig>>::init_scratch_pad(&pcs_params, mpi_config);

(
Expand Down
2 changes: 1 addition & 1 deletion serdes/src/serdes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ impl<V: ExpSerde> ExpSerde for Vec<V> {
}

fn deserialize_from<R: Read>(mut reader: R) -> SerdeResult<Self> {
let mut v = Self::default();
let len = usize::deserialize_from(&mut reader)?;
let mut v = Vec::with_capacity(len);
Comment on lines 71 to +72

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Pre-allocating a Vec with with_capacity(len) where len is read directly from the input stream can lead to Out-Of-Memory (OOM) vulnerabilities if the input is malicious or corrupted (e.g., a very large len value). It is safer to bound the maximum allowed length or use a more conservative allocation strategy.

for _ in 0..len {
v.push(V::deserialize_from(&mut reader)?);
}
Expand Down
Loading