From 7dc7f0d7337c64f601b6b8eedbda5fd61ba81fe8 Mon Sep 17 00:00:00 2001 From: Alon Titelman Date: Mon, 16 Sep 2024 15:44:32 +0300 Subject: [PATCH] Reassigned TODO(spapini)s. --- crates/prover/src/constraint_framework/component.rs | 4 ++-- crates/prover/src/constraint_framework/logup.rs | 2 +- crates/prover/src/constraint_framework/mod.rs | 2 +- .../prover/src/constraint_framework/simd_domain.rs | 2 +- crates/prover/src/core/air/mod.rs | 1 - crates/prover/src/core/backend/cpu/fri.rs | 1 - crates/prover/src/core/backend/cpu/grind.rs | 1 - crates/prover/src/core/backend/mod.rs | 2 +- crates/prover/src/core/backend/simd/bit_reverse.rs | 4 ++-- crates/prover/src/core/backend/simd/circle.rs | 13 ++++++------- crates/prover/src/core/backend/simd/fft/ifft.rs | 2 +- crates/prover/src/core/backend/simd/fft/mod.rs | 2 +- crates/prover/src/core/backend/simd/fft/rfft.rs | 3 +-- crates/prover/src/core/backend/simd/fri.rs | 1 + crates/prover/src/core/backend/simd/grind.rs | 4 ++-- crates/prover/src/core/backend/simd/quotients.rs | 7 ++++--- crates/prover/src/core/channel/blake2s.rs | 3 +-- crates/prover/src/core/channel/poseidon252.rs | 5 ++--- crates/prover/src/core/fields/secure_column.rs | 2 +- crates/prover/src/core/fri.rs | 5 +---- crates/prover/src/core/pcs/verifier.rs | 2 -- crates/prover/src/core/poly/circle/evaluation.rs | 3 ++- crates/prover/src/core/poly/circle/ops.rs | 2 +- 23 files changed, 32 insertions(+), 41 deletions(-) diff --git a/crates/prover/src/constraint_framework/component.rs b/crates/prover/src/constraint_framework/component.rs index c0d8319fa..66bc5c130 100644 --- a/crates/prover/src/constraint_framework/component.rs +++ b/crates/prover/src/constraint_framework/component.rs @@ -143,8 +143,8 @@ impl ComponentProver for FrameworkComponent { let component_evals = trace.evals.sub_tree(&self.trace_locations); // Extend trace if necessary. - // TODO(spapini): Don't extend when eval_size < committed_size. Instead, pick a good - // subdomain. + // TODO: Don't extend when eval_size < committed_size. Instead, pick a good + // subdomain. (For larger blowup factors). let need_to_extend = component_evals .iter() .flatten() diff --git a/crates/prover/src/constraint_framework/logup.rs b/crates/prover/src/constraint_framework/logup.rs index bd8521645..158df41ae 100644 --- a/crates/prover/src/constraint_framework/logup.rs +++ b/crates/prover/src/constraint_framework/logup.rs @@ -118,7 +118,7 @@ impl LookupElements { }) - EF::from(self.z) } - // TODO(spapini): Try to remove this. + pub fn dummy() -> Self { Self { z: SecureField::one(), diff --git a/crates/prover/src/constraint_framework/mod.rs b/crates/prover/src/constraint_framework/mod.rs index 87069d344..9cb886384 100644 --- a/crates/prover/src/constraint_framework/mod.rs +++ b/crates/prover/src/constraint_framework/mod.rs @@ -25,7 +25,7 @@ use crate::core::fields::FieldExpOps; /// A trait for evaluating expressions at some point or row. pub trait EvalAtRow { - // TODO(spapini): Use a better trait for these, like 'Algebra' or something. + // TODO(Ohad): Use a better trait for these, like 'Algebra' or something. /// The field type holding values of columns for the component. These are the inputs to the /// constraints. It might be [BaseField] packed types, or even [SecureField], when evaluating /// the columns out of domain. diff --git a/crates/prover/src/constraint_framework/simd_domain.rs b/crates/prover/src/constraint_framework/simd_domain.rs index ef3662a84..0ae32ebd3 100644 --- a/crates/prover/src/constraint_framework/simd_domain.rs +++ b/crates/prover/src/constraint_framework/simd_domain.rs @@ -55,7 +55,7 @@ impl<'a> EvalAtRow for SimdDomainEvaluator<'a> { type F = VeryPackedBaseField; type EF = VeryPackedSecureField; - // TODO(spapini): Remove all boundary checks. + // TODO(Ohad): Add debug boundary checks. fn next_interaction_mask( &mut self, interaction: usize, diff --git a/crates/prover/src/core/air/mod.rs b/crates/prover/src/core/air/mod.rs index fcdd4d5f8..05d8d10f5 100644 --- a/crates/prover/src/core/air/mod.rs +++ b/crates/prover/src/core/air/mod.rs @@ -19,7 +19,6 @@ pub mod mask; /// evaluate the constraints. /// For instance, all interaction elements are assumed to be present in it. /// Therefore, an AIR is generated only after the initial trace commitment phase. -// TODO(spapini): consider renaming this struct. pub trait Air { fn components(&self) -> Vec<&dyn Component>; } diff --git a/crates/prover/src/core/backend/cpu/fri.rs b/crates/prover/src/core/backend/cpu/fri.rs index 693fb9926..a23f2f1ca 100644 --- a/crates/prover/src/core/backend/cpu/fri.rs +++ b/crates/prover/src/core/backend/cpu/fri.rs @@ -8,7 +8,6 @@ use crate::core::poly::line::LineEvaluation; use crate::core::poly::twiddles::TwiddleTree; use crate::core::poly::BitReversedOrder; -// TODO(spapini): Optimized these functions as well. impl FriOps for CpuBackend { fn fold_line( eval: &LineEvaluation, diff --git a/crates/prover/src/core/backend/cpu/grind.rs b/crates/prover/src/core/backend/cpu/grind.rs index 9ca424861..67211d93e 100644 --- a/crates/prover/src/core/backend/cpu/grind.rs +++ b/crates/prover/src/core/backend/cpu/grind.rs @@ -4,7 +4,6 @@ use crate::core::proof_of_work::GrindOps; impl GrindOps for CpuBackend { fn grind(channel: &C, pow_bits: u32) -> u64 { - // TODO(spapini): This is a naive implementation. Optimize it. let mut nonce = 0; loop { let mut channel = channel.clone(); diff --git a/crates/prover/src/core/backend/mod.rs b/crates/prover/src/core/backend/mod.rs index f6eae913e..288ecc123 100644 --- a/crates/prover/src/core/backend/mod.rs +++ b/crates/prover/src/core/backend/mod.rs @@ -43,7 +43,7 @@ pub trait ColumnOps { pub type Col = >::Column; -// TODO(spapini): Consider removing the generic parameter and only support BaseField. +// TODO(alont): Consider removing the generic parameter and only support BaseField. pub trait Column: Clone + Debug + FromIterator { /// Creates a new column of zeros with the given length. fn zeros(len: usize) -> Self; diff --git a/crates/prover/src/core/backend/simd/bit_reverse.rs b/crates/prover/src/core/backend/simd/bit_reverse.rs index 13d6585de..d6062d381 100644 --- a/crates/prover/src/core/backend/simd/bit_reverse.rs +++ b/crates/prover/src/core/backend/simd/bit_reverse.rs @@ -52,7 +52,7 @@ pub fn bit_reverse_m31(data: &mut [PackedBaseField]) { let log_size = data.len().ilog2(); let a_bits = log_size - 2 * W_BITS - VEC_BITS; - // TODO(spapini): when doing multithreading, do it over a. + // TODO(AlonH): when doing multithreading, do it over a. for a in 0u32..1 << a_bits { for w_l in 0u32..1 << W_BITS { let w_l_rev = w_l.reverse_bits() >> (u32::BITS - W_BITS); @@ -66,7 +66,7 @@ pub fn bit_reverse_m31(data: &mut [PackedBaseField]) { } // Read first chunk. - // TODO(spapini): Think about optimizing a_bits. + // TODO(andrew): Think about optimizing a_bits. What does this mean? let chunk0 = array::from_fn(|i| unsafe { *data.get_unchecked(idx + (i << (2 * W_BITS + a_bits))) }); diff --git a/crates/prover/src/core/backend/simd/circle.rs b/crates/prover/src/core/backend/simd/circle.rs index e930f77b2..b8f77c4c5 100644 --- a/crates/prover/src/core/backend/simd/circle.rs +++ b/crates/prover/src/core/backend/simd/circle.rs @@ -119,7 +119,7 @@ impl SimdBackend { } } -// TODO(spapini): Everything is returned in redundant representation, where values can also be P. +// TODO(shahars): Everything is returned in redundant representation, where values can also be P. // Decide if and when it's ok and what to do if it's not. impl PolyOps for SimdBackend { // The twiddles type is i32, and not BaseField. This is because the fast AVX mul implementation @@ -131,7 +131,7 @@ impl PolyOps for SimdBackend { coset: CanonicCoset, values: Col, ) -> CircleEvaluation { - // TODO(spapini): Optimize. + // TODO(Ohad): Optimize. let eval = CpuBackend::new_canonical_ordered(coset, values.into_cpu_vec()); CircleEvaluation::new( eval.domain, @@ -157,7 +157,7 @@ impl PolyOps for SimdBackend { ); } - // TODO(spapini): Fuse this multiplication / rotation. + // TODO(alont): Cache this inversion. let inv = PackedBaseField::broadcast(BaseField::from(eval.domain.size()).inverse()); values.data.iter_mut().for_each(|x| *x *= inv); @@ -211,7 +211,7 @@ impl PolyOps for SimdBackend { } fn extend(poly: &CirclePoly, log_size: u32) -> CirclePoly { - // TODO(spapini): Optimize or get rid of extend. + // TODO(shahars): Get rid of extends. poly.evaluate(CanonicCoset::new(log_size).circle_domain()) .interpolate() } @@ -221,8 +221,7 @@ impl PolyOps for SimdBackend { domain: CircleDomain, twiddles: &TwiddleTree, ) -> CircleEvaluation { - // TODO(spapini): Precompute twiddles. - // TODO(spapini): Handle small cases. + // TODO(AlonH): Handle small cases. let log_size = domain.log_size(); let fft_log_size = poly.log_size(); assert!( @@ -279,7 +278,7 @@ impl PolyOps for SimdBackend { let mut twiddles = Vec::with_capacity(coset.size()); let mut itwiddles = Vec::with_capacity(coset.size()); - // TODO(spapini): Optimize. + // TODO(alont): Optimize. for layer in &rfft::get_twiddle_dbls(coset) { twiddles.extend(layer); } diff --git a/crates/prover/src/core/backend/simd/fft/ifft.rs b/crates/prover/src/core/backend/simd/fft/ifft.rs index eb34da490..84801efb4 100644 --- a/crates/prover/src/core/backend/simd/fft/ifft.rs +++ b/crates/prover/src/core/backend/simd/fft/ifft.rs @@ -328,7 +328,7 @@ pub fn vecwise_ibutterflies( twiddle2_dbl: [u32; 4], twiddle3_dbl: [u32; 2], ) -> (PackedBaseField, PackedBaseField) { - // TODO(spapini): The permute can be fused with the _mm512_srli_epi64 inside the butterfly. + // TODO(andrew): Can the permute be fused with the _mm512_srli_epi64 inside the butterfly? // Each `ibutterfly` take 2 512-bit registers, and does 16 butterflies element by element. // We need to permute the 512-bit registers to get the right order for the butterflies. diff --git a/crates/prover/src/core/backend/simd/fft/mod.rs b/crates/prover/src/core/backend/simd/fft/mod.rs index ca44979e8..7aa74c909 100644 --- a/crates/prover/src/core/backend/simd/fft/mod.rs +++ b/crates/prover/src/core/backend/simd/fft/mod.rs @@ -10,7 +10,7 @@ pub const CACHED_FFT_LOG_SIZE: u32 = 16; pub const MIN_FFT_LOG_SIZE: u32 = 5; -// TODO(spapini): FFTs return a redundant representation, that can get the value P. need to reduce +// TODO(andrew): FFTs return a redundant representation, that can get the value P. need to reduce // it somewhere. /// Transposes the SIMD vectors in the given array. diff --git a/crates/prover/src/core/backend/simd/fft/rfft.rs b/crates/prover/src/core/backend/simd/fft/rfft.rs index 6d51fd09d..af1939b41 100644 --- a/crates/prover/src/core/backend/simd/fft/rfft.rs +++ b/crates/prover/src/core/backend/simd/fft/rfft.rs @@ -365,8 +365,7 @@ pub fn vecwise_butterflies( twiddle2_dbl: [u32; 4], twiddle3_dbl: [u32; 2], ) -> (PackedBaseField, PackedBaseField) { - // TODO(spapini): Compute twiddle0 from twiddle1. - // TODO(spapini): The permute can be fused with the _mm512_srli_epi64 inside the butterfly. + // TODO(andrew): Can the permute be fused with the _mm512_srli_epi64 inside the butterfly? // The implementation is the exact reverse of vecwise_ibutterflies(). // See the comments in its body for more info. let t = simd_swizzle!( diff --git a/crates/prover/src/core/backend/simd/fri.rs b/crates/prover/src/core/backend/simd/fri.rs index 97212497b..0fb1cbb0b 100644 --- a/crates/prover/src/core/backend/simd/fri.rs +++ b/crates/prover/src/core/backend/simd/fri.rs @@ -19,6 +19,7 @@ use crate::core::poly::twiddles::TwiddleTree; use crate::core::poly::utils::domain_line_twiddles_from_tree; use crate::core::poly::BitReversedOrder; +// TODO(andrew) Is this optimized? impl FriOps for SimdBackend { fn fold_line( eval: &LineEvaluation, diff --git a/crates/prover/src/core/backend/simd/grind.rs b/crates/prover/src/core/backend/simd/grind.rs index 221a4557b..485f5e28c 100644 --- a/crates/prover/src/core/backend/simd/grind.rs +++ b/crates/prover/src/core/backend/simd/grind.rs @@ -20,7 +20,7 @@ const GRIND_HI_BITS: u32 = 64 - GRIND_LOW_BITS; impl GrindOps for SimdBackend { fn grind(channel: &Blake2sChannel, pow_bits: u32) -> u64 { - // TODO(spapini): support more than 32 bits. + // TODO(first): support more than 32 bits. assert!(pow_bits <= 32, "pow_bits > 32 is not supported"); let digest = channel.digest(); let digest: &[u32] = cast_slice(&digest.0[..]); @@ -62,7 +62,7 @@ fn grind_blake(digest: &[u32], hi: u64, pow_bits: u32) -> Option { None } -// TODO(spapini): This is a naive implementation. Optimize it. +// TODO(shahars): This is a naive implementation. Optimize it. #[cfg(not(target_arch = "wasm32"))] impl GrindOps for SimdBackend { fn grind(channel: &Poseidon252Channel, pow_bits: u32) -> u64 { diff --git a/crates/prover/src/core/backend/simd/quotients.rs b/crates/prover/src/core/backend/simd/quotients.rs index ff8e1c580..5d71a1091 100644 --- a/crates/prover/src/core/backend/simd/quotients.rs +++ b/crates/prover/src/core/backend/simd/quotients.rs @@ -34,7 +34,7 @@ impl QuotientOps for SimdBackend { log_blowup_factor: u32, ) -> SecureEvaluation { // Split the domain into a subdomain and a shift coset. - // TODO(spapini): Move to the caller when Columns support slices. + // TODO(andrew): Move to the caller when Columns support slices. let (subdomain, mut subdomain_shifts) = domain.split(log_blowup_factor); if subdomain.log_size() < LOG_N_LANES + 2 { // Fall back to the CPU backend for small domains. @@ -83,7 +83,7 @@ impl QuotientOps for SimdBackend { ); // Extend the evaluation to the full domain. - // TODO(spapini): Try to optimize out all these copies. + // TODO(Ohad): Try to optimize out all these copies. for (ci, &c) in subdomain_shifts.iter().enumerate() { let subdomain = subdomain.shift(c); @@ -123,7 +123,8 @@ fn accumulate_quotients_on_subdomain( .array_chunks::<4>() .enumerate() { - // TODO(spapini): Use optimized domain iteration. + // TODO(andrew): Spapini said: Use optimized domain iteration. Is there a better way to do + // this? let (y01, _) = points[0].y.deinterleave(points[1].y); let (y23, _) = points[2].y.deinterleave(points[3].y); let (spaced_ys, _) = y01.deinterleave(y23); diff --git a/crates/prover/src/core/channel/blake2s.rs b/crates/prover/src/core/channel/blake2s.rs index 3887b0b5f..86565658b 100644 --- a/crates/prover/src/core/channel/blake2s.rs +++ b/crates/prover/src/core/channel/blake2s.rs @@ -74,6 +74,7 @@ impl Channel for Blake2sChannel { msg[1] = (nonce >> 32) as u32; let res = compress(std::array::from_fn(|i| digest[i]), msg, 0, 0, 0, 0); + // TODO(shahars) Channel should always finalize hash. self.update_digest(unsafe { std::mem::transmute(res) }); } @@ -105,8 +106,6 @@ impl Channel for Blake2sChannel { hash_input.extend_from_slice(&padded_counter); - // TODO(spapini): Are we worried about this drawing hash colliding with mix_digest? - self.channel_time.inc_sent(); Blake2sHasher::hash(&hash_input).into() } diff --git a/crates/prover/src/core/channel/poseidon252.rs b/crates/prover/src/core/channel/poseidon252.rs index e9db84593..c0960fc3e 100644 --- a/crates/prover/src/core/channel/poseidon252.rs +++ b/crates/prover/src/core/channel/poseidon252.rs @@ -32,7 +32,7 @@ impl Poseidon252Channel { res } - // TODO(spapini): Understand if we really need uniformity here. + // TODO(shahars): Understand if we really need uniformity here. /// Generates a close-to uniform random vector of BaseField elements. fn draw_base_felts(&mut self) -> [BaseField; 8] { let shift = (1u64 << 31).into(); @@ -61,7 +61,6 @@ impl Channel for Poseidon252Channel { u128::from_le_bytes(std::array::from_fn(|i| bytes[i])).trailing_zeros() } - // TODO(spapini): Optimize. fn mix_felts(&mut self, felts: &[SecureField]) { let shift = (1u64 << 31).into(); let mut res = Vec::with_capacity(felts.len() / 2 + 2); @@ -77,7 +76,7 @@ impl Channel for Poseidon252Channel { ); } - // TODO(spapini): do we need length padding? + // TODO(shahars): do we need length padding? self.update_digest(poseidon_hash_many(&res)); } diff --git a/crates/prover/src/core/fields/secure_column.rs b/crates/prover/src/core/fields/secure_column.rs index 073d21bc2..872954f86 100644 --- a/crates/prover/src/core/fields/secure_column.rs +++ b/crates/prover/src/core/fields/secure_column.rs @@ -16,7 +16,7 @@ pub struct SecureColumnByCoords> { pub columns: [Col; SECURE_EXTENSION_DEGREE], } impl SecureColumnByCoords { - // TODO(spapini): Remove when we no longer use CircleEvaluation. + // TODO(first): Remove. pub fn to_vec(&self) -> Vec { (0..self.len()).map(|i| self.at(i)).collect() } diff --git a/crates/prover/src/core/fri.rs b/crates/prover/src/core/fri.rs index 9f13de253..ce5d4bddb 100644 --- a/crates/prover/src/core/fri.rs +++ b/crates/prover/src/core/fri.rs @@ -686,7 +686,6 @@ impl FriLayerVerifier { commitment, vec![self.domain.log_size(); SECURE_EXTENSION_DEGREE], ); - // TODO(spapini): Propagate error. merkle_verifier .verify( [(self.domain.log_size(), decommitment_positions)] @@ -784,8 +783,6 @@ struct FriLayerProver, H: MerkleHasher> { impl, H: MerkleHasher> FriLayerProver { fn new(evaluation: LineEvaluation) -> Self { - // TODO(spapini): Commit on slice. - // TODO(spapini): Merkle tree in backend. let merkle_tree = MerkleProver::commit(evaluation.values.columns.iter().collect_vec()); #[allow(unreachable_code)] FriLayerProver { @@ -822,7 +819,7 @@ impl, H: MerkleHasher> FriLayerProver { } let commitment = self.merkle_tree.root(); - // TODO(spapini): Use _evals. + // TODO(andrew): Use _evals. let (_evals, decommitment) = self.merkle_tree.decommit( [(self.evaluation.len().ilog2(), decommit_positions)] .into_iter() diff --git a/crates/prover/src/core/pcs/verifier.rs b/crates/prover/src/core/pcs/verifier.rs index 81107b455..959393faa 100644 --- a/crates/prover/src/core/pcs/verifier.rs +++ b/crates/prover/src/core/pcs/verifier.rs @@ -116,8 +116,6 @@ impl CommitmentSchemeVerifier { }) .flatten(); - // TODO(spapini): Properly defined column log size and dinstinguish between poly and - // commitment. let fri_answers = fri_answers( self.column_log_sizes().flatten().into_iter().collect(), &samples, diff --git a/crates/prover/src/core/poly/circle/evaluation.rs b/crates/prover/src/core/poly/circle/evaluation.rs index 4cf23b976..2279b740d 100644 --- a/crates/prover/src/core/poly/circle/evaluation.rs +++ b/crates/prover/src/core/poly/circle/evaluation.rs @@ -36,8 +36,9 @@ impl, F: ExtensionOf, EvalOrder> CircleEvaluation, B: FieldOps> CircleEvaluation { - // TODO(spapini): Remove. Is this even used. + // TODO(alont): Remove. Is this even used. pub fn get_at(&self, point_index: CirclePointIndex) -> F { self.values .at(self.domain.find(point_index).expect("Not in domain")) diff --git a/crates/prover/src/core/poly/circle/ops.rs b/crates/prover/src/core/poly/circle/ops.rs index 40b86cb68..6c1dcccbc 100644 --- a/crates/prover/src/core/poly/circle/ops.rs +++ b/crates/prover/src/core/poly/circle/ops.rs @@ -9,7 +9,7 @@ use crate::core::poly::BitReversedOrder; /// Operations on BaseField polynomials. pub trait PolyOps: FieldOps + Sized { - // TODO(spapini): Use a column instead of this type. + // TODO(alont): Use a column instead of this type. /// The type for precomputed twiddles. type Twiddles;