review(stark): address PR #735 review — coverage, dead code, cleanups

crypto/stark/src/prover.rs

@@ -88,11 +88,16 @@ pub enum ProvingError {
     /// out of disk space, fd exhaustion, or mmap failure.
     #[cfg(feature = "disk-spill")]
     DiskSpill(String),
+    /// An internal FFT/LDE computation failed (e.g. domain size exceeds the
+    /// field's two-adicity, or a degenerate coset offset). Distinct from
+    /// `WrongParameter` because the cause is internal prover machinery, not a
+    /// caller-supplied parameter. Carries the underlying `FFTError`'s message.
+    Fft(String),
 }
 
 impl From<FFTError> for ProvingError {
     fn from(e: FFTError) -> Self {
-        ProvingError::WrongParameter(format!("{e}"))
+        ProvingError::Fft(format!("{e}"))
     }
 }
 
@@ -573,7 +578,8 @@ pub trait IsStarkProver<
             "num_rows must be a power of two for reverse_index"
         );
 
-        const ROWS_PER_LEAF: usize = crate::commitment::ROWS_PER_LEAF; // = 2
+        // Local alias for the canonical constant, used several times below.
+        const ROWS_PER_LEAF: usize = crate::commitment::ROWS_PER_LEAF;
         let num_leaves = num_rows / ROWS_PER_LEAF;
         let subset_cols = col_end - col_start;
         let byte_len = <FieldElement<E> as ByteConversion>::BYTE_LEN;
@@ -884,28 +890,6 @@ pub trait IsStarkProver<
         Ok(())
     }
 
-    /// Commit an already-LDE-expanded plain column set: build the row-paired
-    /// Merkle tree, spill it to disk if requested, and wrap it as a plain
-    /// `TableCommit`. Shared by the main-trace (non-preprocessed) and aux-trace
-    /// commit paths.
-    fn commit_plain<C>(
-        columns: &[Vec<FieldElement<C>>],
-        #[cfg(feature = "disk-spill")] storage_mode: StorageMode,
-        #[cfg(feature = "disk-spill")] spill_label: &str,
-    ) -> Result<TableCommit<C>, ProvingError>
-    where
-        C: IsField,
-        FieldElement<C>: AsBytes + Sync + Send + math::traits::ByteConversion,
-    {
-        #[allow(unused_mut)]
-        let (mut tree, root) =
-            crate::commitment::commit_bit_reversed(columns, crate::commitment::ROWS_PER_LEAF)
-                .ok_or(ProvingError::EmptyCommitment)?;
-        #[cfg(feature = "disk-spill")]
-        Self::spill_tree(&mut tree, storage_mode, spill_label)?;
-        Ok(TableCommit::plain(tree, root))
-    }
-
     /// Recompute Round1 from the trace, reusing the Merkle trees stored in commitments.
     ///
     /// Only used by `run_debug_checks` — Phase D consumes the cached LDE

crypto/stark/src/tests/commitment_tests.rs

@@ -0,0 +1,96 @@
+//! Unit tests for the Merkle commitment layer (`crate::commitment`): they pin
+//! the bit-reversed, row-grouped leaf byte layout that the GPU kernels and the
+//! verifier's `verify_opening_pair` must match. Previously this layout was only
+//! covered transitively through full prove→verify.
+
+use crate::commitment::{
+    ROWS_PER_LEAF, commit_bit_reversed, keccak_leaves_bit_reversed,
+    keccak_leaves_bit_reversed_grouped, keccak_leaves_row_pair_bit_reversed,
+};
+use crate::config::{BatchedMerkleTree, BatchedMerkleTreeBackend, Commitment};
+use math::fft::bit_reversing::reverse_index;
+use math::field::{element::FieldElement, goldilocks::GoldilocksField};
+use math::traits::ByteConversion;
+
+type F = GoldilocksField;
+type Felt = FieldElement<F>;
+
+/// 3 columns × 8 rows of distinct, nonzero values.
+fn sample_columns() -> Vec<Vec<Felt>> {
+    (0..3u64)
+        .map(|c| (0..8u64).map(|r| Felt::from(100 * c + r + 1)).collect())
+        .collect()
+}
+
+/// Independent reference for one leaf, written straight from the module-doc
+/// layout (`rows_per_leaf` consecutive bit-reversed rows, column-major within
+/// each row, big-endian), hashed once with the same backend the prover uses.
+/// Structurally separate from the production `map_init` loop, so a transposed
+/// row/column order or a wrong bit-reversal in production fails this check.
+fn expected_leaf(columns: &[Vec<Felt>], rows_per_leaf: usize, leaf_idx: usize) -> Commitment {
+    let num_rows = columns[0].len();
+    let byte_len = <Felt as ByteConversion>::BYTE_LEN;
+    let mut buf = vec![0u8; rows_per_leaf * columns.len() * byte_len];
+    let mut offset = 0;
+    for k in 0..rows_per_leaf {
+        let br = reverse_index(rows_per_leaf * leaf_idx + k, num_rows as u64);
+        for col in columns {
+            col[br].write_bytes_be(&mut buf[offset..offset + byte_len]);
+            offset += byte_len;
+        }
+    }
+    BatchedMerkleTreeBackend::<F>::hash_bytes(&buf)
+}
+
+#[test]
+fn grouped_leaves_match_documented_layout_for_r1_and_r2() {
+    let columns = sample_columns();
+    let num_rows = columns[0].len();
+    for &rows_per_leaf in &[1usize, 2usize] {
+        let leaves = keccak_leaves_bit_reversed_grouped(&columns, rows_per_leaf);
+        assert_eq!(
+            leaves.len(),
+            num_rows / rows_per_leaf,
+            "leaf count for rows_per_leaf={rows_per_leaf}"
+        );
+        for (i, leaf) in leaves.iter().enumerate() {
+            assert_eq!(
+                *leaf,
+                expected_leaf(&columns, rows_per_leaf, i),
+                "leaf {i} for rows_per_leaf={rows_per_leaf}"
+            );
+        }
+    }
+}
+
+#[test]
+fn wrappers_agree_with_grouped() {
+    let columns = sample_columns();
+    assert_eq!(
+        keccak_leaves_bit_reversed(&columns),
+        keccak_leaves_bit_reversed_grouped(&columns, 1)
+    );
+    assert_eq!(
+        keccak_leaves_row_pair_bit_reversed(&columns),
+        keccak_leaves_bit_reversed_grouped(&columns, ROWS_PER_LEAF)
+    );
+}
+
+#[test]
+fn commit_root_matches_tree_built_over_leaves() {
+    let columns = sample_columns();
+    let leaves = keccak_leaves_bit_reversed_grouped(&columns, ROWS_PER_LEAF);
+    let tree = BatchedMerkleTree::<F>::build_from_hashed_leaves(leaves).unwrap();
+    let (_, root) = commit_bit_reversed(&columns, ROWS_PER_LEAF).unwrap();
+    assert_eq!(root, tree.root);
+}
+
+#[test]
+fn empty_and_zero_row_inputs_short_circuit() {
+    let empty: Vec<Vec<Felt>> = vec![];
+    assert!(keccak_leaves_bit_reversed_grouped(&empty, ROWS_PER_LEAF).is_empty());
+    assert!(commit_bit_reversed(&empty, ROWS_PER_LEAF).is_none());
+    let zero_rows: Vec<Vec<Felt>> = vec![vec![]];
+    assert!(keccak_leaves_bit_reversed_grouped(&zero_rows, ROWS_PER_LEAF).is_empty());
+    assert!(commit_bit_reversed(&zero_rows, ROWS_PER_LEAF).is_none());
+}

crypto/stark/src/tests/mod.rs

@@ -2,12 +2,14 @@ pub mod air_tests;
 #[cfg(feature = "debug-checks")]
 pub mod bus_debug_tests;
 pub mod bus_tests;
+pub mod commitment_tests;
 pub mod domain_cache_stats;
 pub mod fri_tests;
 pub mod grinding_tests;
 pub mod proof_options_tests;
 pub mod prove_verify_roundtrip_tests;
 pub mod prover_tests;
+pub mod row_pair_opening_tests;
 pub mod small_trace_tests;
 #[cfg(feature = "disk-spill")]
 pub mod table_disk_spill_tests;

crypto/stark/src/tests/row_pair_opening_tests.rs

@@ -0,0 +1,73 @@
+//! Negative tests for the row-pair trace opening verification
+//! (`verifier::verify_opening_pair`). The row pair `(2·iota, 2·iota+1)` is
+//! committed as a single Merkle leaf, so one `proof` authenticates both
+//! `evaluations` and `evaluations_sym`. Removing the old separate `proof_sym`
+//! opening deleted the "symmetric opening mismatch" rejection class; these
+//! tests restore it — an implementation that ignored `evaluations_sym` or the
+//! authentication path would otherwise pass every other test.
+
+use super::small_trace_tests::make_valid_simple_proof;
+use crate::verifier::{IsStarkVerifier, Verifier};
+use crypto::fiat_shamir::default_transcript::DefaultTranscript;
+use math::field::{element::FieldElement, goldilocks::GoldilocksField};
+
+type Felt = FieldElement<GoldilocksField>;
+
+/// Tampering the value at the symmetric LDE position must break verification:
+/// the committed leaf hashed `evaluations ‖ evaluations_sym`, so a perturbed
+/// `evaluations_sym` no longer reconstructs the committed leaf.
+#[test_log::test]
+fn test_verify_rejects_tampered_main_trace_evaluations_sym() {
+    let (air, mut proof) = make_valid_simple_proof();
+
+    let opening = proof
+        .deep_poly_openings
+        .first_mut()
+        .expect("test precondition: a valid proof has at least one deep poly opening");
+    assert!(
+        !opening.main_trace_polys.evaluations_sym.is_empty(),
+        "test precondition: the main-trace opening has at least one symmetric evaluation",
+    );
+    // Perturb (not resize) the first symmetric evaluation.
+    opening.main_trace_polys.evaluations_sym[0] =
+        &opening.main_trace_polys.evaluations_sym[0] + Felt::one();
+
+    assert!(
+        !Verifier::verify(
+            &proof,
+            &air,
+            &mut DefaultTranscript::<GoldilocksField>::new(&[])
+        ),
+        "Verifier must reject a tampered symmetric trace evaluation"
+    );
+}
+
+/// The row-pair Merkle authentication path itself must be checked. Corrupting a
+/// node in `main_trace_polys.proof.merkle_path` is caught ONLY by
+/// `verify_opening_pair` (the deep-composition reconstruction does not touch the
+/// auth path), so this proves the single row-pair path is actually authenticated
+/// against the committed root rather than ignored.
+#[test_log::test]
+fn test_verify_rejects_tampered_main_trace_merkle_path() {
+    let (air, mut proof) = make_valid_simple_proof();
+
+    let opening = proof
+        .deep_poly_openings
+        .first_mut()
+        .expect("test precondition: a valid proof has at least one deep poly opening");
+    let path = &mut opening.main_trace_polys.proof.merkle_path;
+    assert!(
+        !path.is_empty(),
+        "test precondition: the row-pair trace tree has a non-trivial authentication path",
+    );
+    path[0][0] ^= 0x01;
+
+    assert!(
+        !Verifier::verify(
+            &proof,
+            &air,
+            &mut DefaultTranscript::<GoldilocksField>::new(&[])
+        ),
+        "Verifier must reject a corrupted main-trace Merkle authentication path"
+    );
+}

crypto/stark/src/tests/small_trace_tests.rs

@@ -17,7 +17,7 @@ use crate::{
 
 type Felt = FieldElement<GoldilocksField>;
 
-fn make_valid_simple_proof() -> (
+pub(crate) fn make_valid_simple_proof() -> (
     SimpleAdditionAIR<GoldilocksField>,
     crate::proof::stark::StarkProof<
         GoldilocksField,

crypto/stark/src/trace.rs

@@ -645,22 +645,6 @@ where
     Table::new(table_data, table_width)
 }
 
-pub fn columns2rows<F>(columns: Vec<Vec<F>>) -> Vec<Vec<F>>
-where
-    F: Clone,
-{
-    let num_rows = columns[0].len();
-    let num_cols = columns.len();
-
-    (0..num_rows)
-        .map(|row_index| {
-            (0..num_cols)
-                .map(|col_index| columns[col_index][row_index].clone())
-                .collect()
-        })
-        .collect()
-}
-
 pub(crate) fn compute_frame_evaluation_points<F, E>(
     x: &FieldElement<E>,
     frame_offsets: &[usize],

prover/src/instruments.rs

@@ -173,7 +173,7 @@ pub fn print_report(
             let mut sub_ops: Vec<(&str, Duration)> = vec![
                 ("R2  evaluate", total_constraints),
                 ("R2  decompose_and_extend_d2", total_comp_decompose),
-                ("R2  commit_composition_poly", total_comp_commit),
+                ("R2  commit_bit_reversed (comp-poly)", total_comp_commit),
                 ("R3  OOD evaluation", total_ood),
                 ("R4  deep_composition_poly_evals", total_deep_comp),
                 ("R4  interpolate+evaluate_fft", total_deep_extend),

prover/tests/cuda_path_integration.rs

@@ -11,8 +11,8 @@
 use lambda_vm_prover::test_utils::asm_elf_bytes;
 use lambda_vm_prover::{prove, verify};
 use stark::gpu_lde::{
-    gpu_bary_calls, gpu_batch_invert_calls, gpu_deep_calls, gpu_fri_calls, gpu_lde_calls,
-    reset_all_gpu_call_counters,
+    gpu_bary_calls, gpu_batch_invert_calls, gpu_comp_poly_tree_calls, gpu_deep_calls,
+    gpu_fri_calls, gpu_lde_calls, reset_all_gpu_call_counters,
 };
 
 #[test]
@@ -36,11 +36,22 @@ fn gpu_path_fires_end_to_end() {
     // path.
     assert!(gpu_bary_calls() > 0, "R3 GPU barycentric did not fire");
 
-    // The R2 parts-LDE and comp-poly-tree GPU dispatches are obsolete since
+    // R2 parts-LDE (try_evaluate_parts_on_lde_gpu_keep) is obsolete: since
     // #699/#700 routed degree-3 tables through the 2-part decompose_and_extend_d2
-    // + fused coset_lde_full path: no VM AIR has number_of_parts > 2 anymore, so
-    // try_evaluate_parts_on_lde_gpu_keep (and its paired tree build) no longer
-    // fire. (These assertions bit-rotted against the fused composition LDE.)
+    // + fused coset_lde_full path, no VM AIR has number_of_parts > 2, so that
+    // dispatch lives only in the dead `> 2` fallback. We intentionally do NOT
+    // assert gpu_parts_lde_calls() here.
+    //
+    // The comp-poly-tree build is NOT obsolete, though: try_build_comp_poly_tree_gpu
+    // is called unconditionally after the parts-count branch (prover.rs round 2),
+    // so it fires for the common number_of_parts == 2 (degree-3) case. A silent
+    // CPU fallback here would still verify, so the end-to-end check below cannot
+    // catch it — this counter assertion is what guards the GPU comp-poly-tree
+    // dispatch.
+    assert!(
+        gpu_comp_poly_tree_calls() > 0,
+        "R2 GPU comp-poly tree did not fire"
+    );
 
     // DEEP fires once per table that took the R1 GPU path.
     assert!(gpu_deep_calls() > 0, "R4 GPU DEEP composition did not fire");