Skip to content

Conversation

@zhenfeizhang
Copy link
Contributor

@zhenfeizhang zhenfeizhang commented Jun 12, 2025

this PR

future work

Some future work for integration:

  • original whir only works for Goldilocks and does not have SIMD support. We will need to
    • either write Arkwork's FF trait for Goldilcoksx8,
    • or fork whir and change its backend to Expander's FF
  • original whir uses sponge-fish as Fiat-Shamir. we will need to replace it with ours; or find a way to pass our transcript state to sponge-fish

On expander side:

  • we need to find a good way to batch commit. it is still not clear what is the best way to do it.
    • we could use the FRI trick to hash all polynomials into a single one. But we will need to open all polynomials at a same point. this requires modifications to the GKR code
    • we could also think about using sumcheck to merge the polynomials as introduced in Hyrax and KZG batch verification #290. how to properly merge non-homomorphic commitments requires some more thoughts

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Summary of Changes

Hello @zhenfeizhang, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly expands the cryptographic backend options by integrating the WHIR PCS for Goldilocks and introduces a generalized batch opening mechanism for existing PCS like Hyrax and KZG, leveraging a new SumCheck implementation. This enhances proof efficiency and adds flexibility to the commitment layer.

Highlights

  • WHIR PCS Integration: Integrated the WHIR Polynomial Commitment Scheme (PCS) into the Expander framework, specifically tailored for the Goldilocks field. This adds a new PCS option for circuits using Goldilocks.
  • Batch Opening Functionality: Introduced generic batch opening capabilities for multiple polynomials evaluated at multiple points. This functionality is implemented for both Hyrax and KZG PCS using a SumCheck-based approach.
  • Generic SumCheck Protocol: Added a new generic SumCheck protocol implementation for Sum-of-Products polynomials, which serves as a core component for the new multi-point batch opening scheme.
  • Dependency Updates and Additions: Updated numerous dependencies and added new ones, including the whir crate (from a specific fork), spongefish, spongefish-pow, and several Arkworks crates (ark-ff, ark-ec, etc.) required by WHIR.
  • KZG Module Refactoring: Restructured the KZG implementation into distinct univariate (uni_kzg) and bivariate (bi_kzg) modules for better organization and clarity.
  • Field Conversion Implementations: Added From trait implementations for converting between Expander's Goldilocks and GoldilocksExt2 fields and Arkworks' Field64 and Field64_2 types, enabling interoperability with Arkworks-based libraries like WHIR.
  • Polynomial Trait Enhancements: Enhanced the MultiLinearPoly struct with PartialEq and Eq derives and added methods like fix_bottom_variable and eval_reverse_order, along with Add and Mul trait implementations, supporting the new SumCheck logic.
  • PCS Trait Extension: Extended the core ExpanderPCS trait to include associated types and methods for batch opening (BatchOpening, multi_points_batch_open, multi_points_batch_verify), standardizing the interface for batch proofs.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configureGemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces significant enhancements by integrating the WHIR polynomial commitment scheme and adding batch opening capabilities to existing schemes like Hyrax and KZG. The KZG codebase has also been refactored for better organization between univariate and bivariate schemes. These are substantial changes that expand the toolkit's cryptographic functionalities.

My review focuses on the correctness of these new integrations and refactorings, particularly the trait modifications and the logic within the PCS implementations. I've identified a critical issue in the KZG verification logic and a few areas where clarity or error handling could be improved, especially concerning MPI support in new batching functions and trait bound changes. The use of git dependencies for whir and spongefish is noted and is acceptable for this stage of development, but should be revisited for stable releases.

@zhenfeizhang
Copy link
Contributor Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This PR integrates the WHIR Polynomial Commitment Scheme (PCS) into the Expander framework. Key changes include adding WhirPCS and its associated types, updating the PolynomialCommitmentScheme trait, and modifying dependencies. There are critical issues with the serialization of WhirCommitment and several unwrap() calls in the WhirPCS implementation that need to be addressed for robustness and correctness.

Comment on lines +12 to 19
type Params: Clone + Debug;
type Poly: Clone + Debug + Default;
type EvalPoint: Clone + Debug + Default;
type ScratchPad: Clone + Debug + Default + ExpSerde;
type ScratchPad: Debug;

type SRS: Clone + Debug + Default + ExpSerde + StructuredReferenceString;
type Commitment: Clone + Debug + Default + ExpSerde;
type Commitment: ExpSerde;
type Opening: Clone + Debug + Default + ExpSerde;
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

The Commitment: ExpSerde bound conflicts with the ExpSerde implementation for WhirPCS::Commitment in serdes/src/serdes.rs. The serialize_into for Witness is a no-op, and deserialize_from is unimplemented!(). Address this conflict by either implementing proper serialization/deserialization or removing the ExpSerde bound.

Comment on lines +216 to +222
impl ExpSerde for Witness<Field64_2, KeccakMerkleTreeParams<Field64_2>> {
fn serialize_into<W: Write>(&self, mut _writer: W) -> SerdeResult<()> {
Ok(())
}
fn deserialize_from<R: Read>(mut _reader: R) -> SerdeResult<Self> {
unimplemented!("Witness deserialization is not implemented for witness");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

The ExpSerde implementation for Witness is non-functional. serialize_into is a no-op, and deserialize_from is unimplemented!(). Implement proper serialization/deserialization or remove the ExpSerde bound.


let committer = CommitmentWriter::new(params.clone());

committer.commit(prover_state, whir_poly.clone()).unwrap()
Copy link
Contributor

Choose a reason for hiding this comment

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

high

Handle the Result from committer.commit to prevent panics. Propagate the error or implement error recovery logic.

Comment on lines +114 to +116
prover
.prove(prover_state, statement.clone(), witness)
.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

high

Handle the Result from prover.prove to prevent panics. Propagate the error or implement error recovery logic.

Comment on lines +149 to +151
let parsed_commitment = commitment_reader
.parse_commitment(&mut verifier_state)
.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

high

Handle the Result from commitment_reader.parse_commitment to prevent panics. Return false from verify if parsing fails.

derivative = "2.2.0"
transpose = "0.2.3"
# whir = { git = "https://github.com/WizardOfMenlo/whir", rev = "9d7a97b169542f8d0787db482ee58f7b9b660848" }
whir = { git = "https://github.com/zhenfeizhang/whir" }
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

Pinning the whir dependency to a specific commit hash or tag improves reproducibility and prevents unexpected changes from upstream.

impl From<Goldilocks> for Field64 {
#[inline(always)]
fn from(x: Goldilocks) -> Self {
Field64::from_bigint(BigInteger64::new([x.v])).unwrap()
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

Clarify the assumption that x.v is always a canonical Goldilocks field element to ensure from_bigint doesn't fail.

type Opening = Vec<u8>;

fn init_scratch_pad(params: &Self::Params) -> Self::ScratchPad {
// todo: session identifier can be sampled from transcript?
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

Consider sampling the session identifier from the transcript to enhance security against cross-protocol or replay attacks.

prover_state: &mut Self::ScratchPad,
_transcript: &mut impl gkr_engine::Transcript,
) -> (GoldilocksExt2, Self::Opening) {
// todo: avoid cloning commitment if possible
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

Avoid cloning the commitment if possible, as it could impact performance. Explore alternative API structures or optimizations in whir.

Comment on lines 41 to 49
fn open(
params: &Self::Params,
commitment: &Self::Commitment,
proving_key: &<Self::SRS as StructuredReferenceString>::PKey,
poly: &Self::Poly,
x: &Self::EvalPoint,
scratch_pad: &Self::ScratchPad,
scratch_pad: &mut Self::ScratchPad,
transcript: &mut impl Transcript,
) -> (F, Self::Opening);
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The signature of the open method has changed. Ensure these are intentional and all implementers of PolynomialCommitmentScheme are updated accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants