-
Notifications
You must be signed in to change notification settings - Fork 55
[BUG] Fix HyperKZG verify ignoring pairing check result #316
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @jsemldonado, 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 addresses a critical bug in the HyperKZG polynomial commitment scheme's verification process, where the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
There was a problem hiding this 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 addresses a critical vulnerability in coeff_form_uni_hyperkzg_verify where proof verification was being skipped entirely. The fix correctly propagates the result of the pairing check, ensuring proofs are actually verified. The inclusion of a regression test that confirms a corrupted proof is rejected is a great addition. My review includes a minor suggestion to improve the readability of the new test code by reducing repetition.
I am having trouble creating individual review comments. Click here to see my feedback.
poly_commit/src/kzg/uni_kzg/hyper_kzg.rs (255-257)
This change fixes a critical bug where the result of the pairing check was discarded and true was always returned, effectively bypassing proof verification. Returning the result of coeff_form_uni_kzg_verify directly is the correct fix.
poly_commit/tests/test_uni_kzg.rs (108-145)
To improve readability and reduce repetition, you could introduce a type alias for HyperUniKZGPCS<Bn256>. This would make the repeated calls to the PolynomialCommitmentScheme trait methods cleaner and easier to read.
type PCS = HyperUniKZGPCS<Bn256>;
let mut rng = test_rng();
let num_vars = 4;
let (srs, _) =
<PCS as PolynomialCommitmentScheme<Fr>>::gen_srs_for_testing(
&num_vars, &mut rng,
);
let (proving_key, verification_key) = srs.into_keys();
let poly = MultiLinearPoly::<Fr>::random(num_vars, &mut rng);
let x: Vec<Fr> = (0..num_vars).map(|_| Fr::random_unsafe(&mut rng)).collect();
let mut scratch_pad = ();
let commitment =
<PCS as PolynomialCommitmentScheme<Fr>>::commit(
&num_vars, &proving_key, &poly, &mut scratch_pad,
);
let mut transcript = BytesHashTranscript::<Keccak256hasher>::new();
let (eval, opening) = <PCS as PolynomialCommitmentScheme<Fr>>::open(
&num_vars, &proving_key, &poly, &x, &scratch_pad, &mut transcript,
);
// Valid proof should pass
let mut transcript_v = BytesHashTranscript::<Keccak256hasher>::new();
assert!(<PCS as PolynomialCommitmentScheme<Fr>>::verify(
&num_vars, &verification_key, &commitment, &x, eval, &opening, &mut transcript_v,
));
// Corrupted proof should fail
let mut corrupted = opening.clone();
corrupted.quotient_delta_x_commitment =
(corrupted.quotient_delta_x_commitment.to_curve() * Fr::from(2u64)).to_affine();
let mut transcript_c = BytesHashTranscript::<Keccak256hasher>::new();
assert!(!<PCS as PolynomialCommitmentScheme<Fr>>::verify(
&num_vars, &verification_key, &commitment, &x, eval, &corrupted, &mut transcript_c,
));
coeff_form_uni_hyperkzg_verifywas discarding the result of the pairing check and returningtrueunconditionally.This meant any proof would pass verification.
Fixed by returning the actual result. Added regression test that corrupts a proof and verifies it gets rejected.