-
Notifications
You must be signed in to change notification settings - Fork 2
Fix edge-case bugs in polynomial arithmetic, the specialized FFT, and Merkle path parsing #269
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?
Changes from all commits
5fc0d39
1b377c6
a0c2623
cbe2cc2
93b1c6a
95d35a7
65c2708
90702e1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -182,7 +182,10 @@ impl<F: Field> Polynomial<Assigned<F>, LagrangeCoeff> { | |
| impl<'a, F: Field, B: Basis> Add<&'a Polynomial<F, B>> for Polynomial<F, B> { | ||
| type Output = Polynomial<F, B>; | ||
|
|
||
| /// This function will panic if the operands have different lengths, | ||
| /// i.e. were created for different domain sizes. | ||
| fn add(mut self, rhs: &'a Polynomial<F, B>) -> Polynomial<F, B> { | ||
| assert_eq!(self.values.len(), rhs.values.len()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LGTM |
||
| parallelize(&mut self.values, |lhs, start| { | ||
| for (lhs, rhs) in lhs.iter_mut().zip(rhs.values[start..].iter()) { | ||
| *lhs += *rhs; | ||
|
|
@@ -315,6 +318,38 @@ mod tests { | |
|
|
||
| use super::{EvaluationDomain, Rotation}; | ||
|
|
||
| #[test] | ||
| fn test_polynomial_addition() { | ||
| let domain = EvaluationDomain::<pallas::Base>::new(1, 2); | ||
| let lhs = domain.coeff_from_vec([1, 2, 3, 4].map(pallas::Base::from).to_vec()); | ||
| let rhs = domain.coeff_from_vec([10, 20, 30, 40].map(pallas::Base::from).to_vec()); | ||
|
|
||
| let sum = lhs + &rhs; | ||
| assert_eq!(&sum[..], [11, 22, 33, 44].map(pallas::Base::from)); | ||
| } | ||
|
|
||
| #[test] | ||
| #[should_panic(expected = "left == right")] | ||
| fn test_polynomial_addition_rejects_larger_rhs() { | ||
| let small_domain = EvaluationDomain::<pallas::Base>::new(1, 1); | ||
| let large_domain = EvaluationDomain::<pallas::Base>::new(1, 2); | ||
| let small = small_domain.coeff_from_vec([1, 2].map(pallas::Base::from).to_vec()); | ||
| let large = large_domain.coeff_from_vec([10, 20, 30, 40].map(pallas::Base::from).to_vec()); | ||
|
|
||
| let _ = small + &large; | ||
| } | ||
|
|
||
| #[test] | ||
| #[should_panic(expected = "left == right")] | ||
| fn test_polynomial_addition_rejects_smaller_rhs() { | ||
| let small_domain = EvaluationDomain::<pallas::Base>::new(1, 1); | ||
| let large_domain = EvaluationDomain::<pallas::Base>::new(1, 2); | ||
| let small = small_domain.coeff_from_vec([1, 2].map(pallas::Base::from).to_vec()); | ||
| let large = large_domain.coeff_from_vec([10, 20, 30, 40].map(pallas::Base::from).to_vec()); | ||
|
|
||
| let _ = large + &small; | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_copy_rotated_chunk() { | ||
| let k = 11; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -861,7 +861,16 @@ impl<F: WithSmallOrderMulGroup<3>> EvaluationDomain<F> { | |
| let common = (xn - F::ONE) * self.barycentric_weight; | ||
| for (rotation, result) in rotations.into_iter().zip(results.iter_mut()) { | ||
| let rotation = Rotation(rotation); | ||
| *result = self.rotate_omega(*result * common, rotation); | ||
| *result = if result.is_zero_vartime() { | ||
| // The denominator `x - omega^i` was zero and survived batch | ||
| // inversion as zero: `x` is this basis polynomial's own | ||
| // domain point, so the shared numerator `xn - 1` vanishes | ||
| // too and the removable singularity cancels to | ||
| // `l_i(omega^i) = 1`. | ||
| F::ONE | ||
| } else { | ||
| self.rotate_omega(*result * common, rotation) | ||
| }; | ||
|
Comment on lines
+864
to
+873
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also need to check if this is a performance sensitive routine. |
||
| } | ||
|
|
||
| results | ||
|
|
@@ -1331,6 +1340,41 @@ fn test_l_i() { | |
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_l_i_at_domain_points() { | ||
| use crate::pasta::pallas::Scalar; | ||
| let domain = EvaluationDomain::<Scalar>::new(1, 3); | ||
|
|
||
| for j in 0..8u64 { | ||
| let x = domain.get_omega().pow([j]); | ||
| let xn = x.pow([8]); | ||
| assert_eq!(xn, Scalar::ONE); | ||
|
|
||
| // Rotations are periodic modulo the domain size, so every rotation | ||
| // in -7..=7 congruent to j (two of them, except for j = 0) must | ||
| // evaluate to one, and every other rotation to zero. | ||
| let evaluations = domain.l_i_range(x, xn, -7i32..=7); | ||
| for (evaluation, rotation) in evaluations.iter().zip(-7i32..=7) { | ||
| let expected = if rotation.rem_euclid(8) as u64 == j { | ||
| Scalar::ONE | ||
| } else { | ||
| Scalar::ZERO | ||
| }; | ||
| assert_eq!(*evaluation, expected, "rotation {rotation} at omega^{j}"); | ||
| } | ||
| } | ||
|
|
||
| // A domain point whose own basis polynomial is not requested makes | ||
| // every requested basis polynomial evaluate to zero. | ||
| let x = domain.get_omega().pow([5]); | ||
| let evaluations = domain.l_i_range(x, Scalar::ONE, 0..=3); | ||
| assert!( | ||
| evaluations | ||
| .iter() | ||
| .all(|evaluation| *evaluation == Scalar::ZERO) | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_copy_rotated_chunk_extended() { | ||
| use pasta_curves::pallas; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -250,10 +250,15 @@ fn collapse_polynomials<F: Field>( | |
|
|
||
| /// Create a multi-opening proof. | ||
| /// | ||
| /// A queried polynomial with fewer coefficients than the parameters' domain | ||
| /// size is treated as zero-extended to that size. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns [`std::io::ErrorKind::InvalidInput`] if `queries` is empty or | ||
| /// contains more than one query for the same commitment at the same point. | ||
| /// Returns [`std::io::ErrorKind::InvalidInput`] if `queries` is empty, | ||
| /// contains more than one query for the same commitment at the same point, | ||
| /// or queries a polynomial with more coefficients than the parameters' | ||
| /// domain size. | ||
| pub fn create_proof< | ||
| 'a, | ||
| I, | ||
|
|
@@ -286,11 +291,24 @@ where | |
| let mut q_blinds = vec![Blind(C::Scalar::ZERO); point_sets.len()]; | ||
| for commitment_data in poly_map { | ||
| let set_index = commitment_data.set_index; | ||
| if commitment_data.commitment.poly.num_coeffs() > params.n as usize { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidInput, | ||
| "query polynomial has more coefficients than the parameters' domain size", | ||
| )); | ||
| } | ||
| polynomial_groups[set_index].push(commitment_data.commitment.poly); | ||
| q_blinds[set_index] *= *x_1; | ||
| q_blinds[set_index] += commitment_data.commitment.blind; | ||
| } | ||
| let q_polys = collapse_polynomials(&polynomial_groups, *x_1); | ||
| let mut q_polys = collapse_polynomials(&polynomial_groups, *x_1); | ||
| // Each collapsed polynomial keeps its group head's length. Zero-extend to | ||
| // the parameters' length — absent high coefficients read as zero, exactly | ||
| // as the polynomial's commitment treats them — so the final combination | ||
| // below folds equal-length operands. | ||
| for q_poly in &mut q_polys { | ||
| q_poly.values.resize(params.n as usize, C::Scalar::ZERO); | ||
| } | ||
|
Comment on lines
+305
to
+311
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to check if this affects performance, this seems bad for efficiency |
||
|
|
||
| let q_prime_poly = point_sets | ||
| .iter() | ||
|
|
@@ -386,23 +404,36 @@ mod tests { | |
| use std::fmt::Debug; | ||
| use std::marker::PhantomData; | ||
|
|
||
| // A group's collapse keeps the first polynomial's length: shorter group | ||
| // members are zero-extended and longer ones truncated to it, matching | ||
| // `fold_polynomial_range`'s treatment of absent coefficients. Polynomial | ||
| // addition itself requires equal lengths, so fold each coefficient | ||
| // directly. | ||
| fn reference_collapse<F: Field>( | ||
| groups: &[Vec<&Polynomial<F, Coeff>>], | ||
| challenge: F, | ||
| ) -> Vec<Polynomial<F, Coeff>> { | ||
| groups | ||
| .iter() | ||
| .map(|group| { | ||
| group[1..] | ||
| .iter() | ||
| .fold(group[0].clone(), |accumulator, polynomial| { | ||
| accumulator * challenge + polynomial | ||
| .map(|group| Polynomial { | ||
| values: (0..group[0].values.len()) | ||
| .map(|coefficient_index| { | ||
| group.iter().fold(F::ZERO, |accumulator, polynomial| { | ||
| accumulator * challenge | ||
| + polynomial | ||
| .values | ||
| .get(coefficient_index) | ||
| .copied() | ||
| .unwrap_or(F::ZERO) | ||
| }) | ||
| }) | ||
| .collect(), | ||
| _marker: PhantomData, | ||
| }) | ||
| .collect() | ||
| } | ||
|
|
||
| fn streaming_collapse_matches_operator_collapse<F>() | ||
| fn streaming_collapse_matches_reference<F>() | ||
| where | ||
| F: Field + From<u64> + Debug, | ||
| { | ||
|
|
@@ -462,12 +493,125 @@ mod tests { | |
| } | ||
|
|
||
| #[test] | ||
| fn streaming_collapse_matches_operator_collapse_fp() { | ||
| streaming_collapse_matches_operator_collapse::<Fp>(); | ||
| fn streaming_collapse_matches_reference_fp() { | ||
| streaming_collapse_matches_reference::<Fp>(); | ||
| } | ||
|
|
||
| #[test] | ||
| fn streaming_collapse_matches_reference_fq() { | ||
| streaming_collapse_matches_reference::<Fq>(); | ||
| } | ||
|
|
||
| #[test] | ||
| fn short_query_polynomial_proves_and_verifies() { | ||
| use crate::arithmetic::eval_polynomial; | ||
| use crate::pasta::EpAffine; | ||
| use crate::poly::commitment::{Blind, Params}; | ||
| use crate::poly::multiopen::{ProverQuery, VerifierQuery, create_proof, verify_proof}; | ||
| use crate::transcript::{ | ||
| Blake2bRead, Blake2bWrite, Challenge255, TranscriptRead, TranscriptWrite, | ||
| }; | ||
| use group::Curve; | ||
| use rand::rng; | ||
|
|
||
| let mut rng = rng(); | ||
| let params = Params::<EpAffine>::new(1); | ||
|
|
||
| // A one-coefficient query polynomial is zero-extended to the | ||
| // parameters' length, so its commitment is that of the padded copy. | ||
| let short = Polynomial::<Fq, Coeff> { | ||
| values: vec![Fq::from(5)], | ||
| _marker: PhantomData, | ||
| }; | ||
| let padded = Polynomial::<Fq, Coeff> { | ||
| values: vec![Fq::from(5), Fq::ZERO], | ||
| _marker: PhantomData, | ||
| }; | ||
| let full = Polynomial::<Fq, Coeff> { | ||
| values: vec![Fq::from(3), Fq::from(4)], | ||
| _marker: PhantomData, | ||
| }; | ||
| let short_blind = Blind(Fq::random(&mut rng)); | ||
| let full_blind = Blind(Fq::random(&mut rng)); | ||
| let short_commitment = params.commit(&padded, short_blind).to_affine(); | ||
| let full_commitment = params.commit(&full, full_blind).to_affine(); | ||
|
|
||
| let short_point = Fq::from(97); | ||
| let full_point = Fq::from(43); | ||
| let short_eval = eval_polynomial(&short[..], short_point); | ||
| let full_eval = eval_polynomial(&full[..], full_point); | ||
|
|
||
| let mut transcript = | ||
| Blake2bWrite::<Vec<u8>, EpAffine, Challenge255<EpAffine>>::init(vec![]); | ||
| transcript.write_point(short_commitment).unwrap(); | ||
| transcript.write_point(full_commitment).unwrap(); | ||
| transcript.write_scalar(short_eval).unwrap(); | ||
| transcript.write_scalar(full_eval).unwrap(); | ||
| create_proof( | ||
| ¶ms, | ||
| rng, | ||
| &mut transcript, | ||
| vec![ | ||
| ProverQuery { | ||
| point: short_point, | ||
| poly: &short, | ||
| blind: short_blind, | ||
| }, | ||
| ProverQuery { | ||
| point: full_point, | ||
| poly: &full, | ||
| blind: full_blind, | ||
| }, | ||
| ], | ||
| ) | ||
| .unwrap(); | ||
| let proof = transcript.finalize(); | ||
|
|
||
| let mut transcript = | ||
| Blake2bRead::<&[u8], EpAffine, Challenge255<EpAffine>>::init(&proof[..]); | ||
| assert_eq!(transcript.read_point().unwrap(), short_commitment); | ||
| assert_eq!(transcript.read_point().unwrap(), full_commitment); | ||
| assert_eq!(transcript.read_scalar().unwrap(), short_eval); | ||
| assert_eq!(transcript.read_scalar().unwrap(), full_eval); | ||
| let guard = verify_proof( | ||
| ¶ms, | ||
| &mut transcript, | ||
| vec![ | ||
| VerifierQuery::new_commitment(&short_commitment, short_point, short_eval), | ||
| VerifierQuery::new_commitment(&full_commitment, full_point, full_eval), | ||
| ], | ||
| params.empty_msm(), | ||
| ) | ||
| .unwrap(); | ||
| assert!(guard.use_challenges().eval()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn streaming_collapse_matches_operator_collapse_fq() { | ||
| streaming_collapse_matches_operator_collapse::<Fq>(); | ||
| fn oversized_query_polynomial_is_rejected() { | ||
| use crate::pasta::EpAffine; | ||
| use crate::poly::commitment::{Blind, Params}; | ||
| use crate::poly::multiopen::{ProverQuery, create_proof}; | ||
| use crate::transcript::{Blake2bWrite, Challenge255}; | ||
| use rand::rng; | ||
|
|
||
| let params = Params::<EpAffine>::new(1); | ||
| let oversized = Polynomial::<Fq, Coeff> { | ||
| values: vec![Fq::ONE; 4], | ||
| _marker: PhantomData, | ||
| }; | ||
|
|
||
| let mut transcript = | ||
| Blake2bWrite::<Vec<u8>, EpAffine, Challenge255<EpAffine>>::init(vec![]); | ||
| let result = create_proof( | ||
| ¶ms, | ||
| rng(), | ||
| &mut transcript, | ||
| vec![ProverQuery { | ||
| point: Fq::from(97), | ||
| poly: &oversized, | ||
| blind: Blind(Fq::ZERO), | ||
| }], | ||
| ); | ||
| assert_eq!(result.unwrap_err().kind(), std::io::ErrorKind::InvalidInput); | ||
| } | ||
| } | ||
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.
LGTM