From 1abb9d39b9a9e1a79a8b2bee7fc588fe3806a544 Mon Sep 17 00:00:00 2001 From: MauroFab Date: Mon, 29 Jun 2026 17:31:48 -0300 Subject: [PATCH] =?UTF-8?q?review(stark):=20address=20PR=20#735=20review?= =?UTF-8?q?=20=E2=80=94=20coverage,=20dead=20code,=20cleanups?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the row-pair commitment PR. Excludes the intentional proof-format break (proof_sym removal / leaf-count change), which is by design. Test coverage (in their own files under crypto/stark/src/tests/, per the crate's test structure): - tests/commitment_tests.rs: direct unit tests pinning the row-pair leaf layout (R=1 and R=2) against an independent reference, wrapper agreement, commit-root consistency, and empty-input short-circuit. Previously the leaf layout was only covered transitively via full prove->verify, and GPU parity tests compared against an inline reimpl rather than this module. - tests/row_pair_opening_tests.rs: two negative tests for the row-pair verify_opening_pair — a tampered symmetric trace evaluation and a corrupted Merkle authentication path must both be rejected. Removing proof_sym deleted the old "symmetric opening mismatch" rejection class; these restore it (an impl ignoring evaluations_sym / the auth path would otherwise pass every existing test). Reuses the now pub(crate) make_valid_simple_proof helper. - cuda_path_integration.rs: restore assert!(gpu_comp_poly_tree_calls() > 0). try_build_comp_poly_tree_gpu is dispatched unconditionally (round 2, after the parts-count branch), so it fires for the common number_of_parts == 2 (degree-3) case — it was NOT obsolete. Keep the genuinely-dead parts-LDE assertion dropped. Cleanups: - Delete dead fn commit_plain (zero callers; main/aux commit inline commit_rows_bit_reversed + spill_tree, which are row-major and incompatible with its column-major signature — the dedup never landed). - Delete orphaned pub fn columns2rows (all callers removed by #735). - Add ProvingError::Fft and map FFTError to it instead of WrongParameter (internal FFT failure is not a caller-supplied-parameter error). - Fix stale profiler label commit_composition_poly -> commit_bit_reversed. - Drop the rot-prone "// = 2" comment on the local ROWS_PER_LEAF alias. --- crypto/stark/src/prover.rs | 32 ++----- crypto/stark/src/tests/commitment_tests.rs | 96 +++++++++++++++++++ crypto/stark/src/tests/mod.rs | 2 + .../stark/src/tests/row_pair_opening_tests.rs | 73 ++++++++++++++ crypto/stark/src/tests/small_trace_tests.rs | 2 +- crypto/stark/src/trace.rs | 16 ---- prover/src/instruments.rs | 2 +- prover/tests/cuda_path_integration.rs | 23 +++-- 8 files changed, 198 insertions(+), 48 deletions(-) create mode 100644 crypto/stark/src/tests/commitment_tests.rs create mode 100644 crypto/stark/src/tests/row_pair_opening_tests.rs diff --git a/crypto/stark/src/prover.rs b/crypto/stark/src/prover.rs index a3c64c4fb..288288c06 100644 --- a/crypto/stark/src/prover.rs +++ b/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 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 = 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( - columns: &[Vec>], - #[cfg(feature = "disk-spill")] storage_mode: StorageMode, - #[cfg(feature = "disk-spill")] spill_label: &str, - ) -> Result, ProvingError> - where - C: IsField, - FieldElement: 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 diff --git a/crypto/stark/src/tests/commitment_tests.rs b/crypto/stark/src/tests/commitment_tests.rs new file mode 100644 index 000000000..f1684112b --- /dev/null +++ b/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; + +/// 3 columns × 8 rows of distinct, nonzero values. +fn sample_columns() -> Vec> { + (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], rows_per_leaf: usize, leaf_idx: usize) -> Commitment { + let num_rows = columns[0].len(); + let byte_len = ::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::::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::::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![]; + 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![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()); +} diff --git a/crypto/stark/src/tests/mod.rs b/crypto/stark/src/tests/mod.rs index 8c0897ac1..7a3884832 100644 --- a/crypto/stark/src/tests/mod.rs +++ b/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; diff --git a/crypto/stark/src/tests/row_pair_opening_tests.rs b/crypto/stark/src/tests/row_pair_opening_tests.rs new file mode 100644 index 000000000..89e7dc8f1 --- /dev/null +++ b/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; + +/// 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::::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::::new(&[]) + ), + "Verifier must reject a corrupted main-trace Merkle authentication path" + ); +} diff --git a/crypto/stark/src/tests/small_trace_tests.rs b/crypto/stark/src/tests/small_trace_tests.rs index 8373ae9d6..1e081910b 100644 --- a/crypto/stark/src/tests/small_trace_tests.rs +++ b/crypto/stark/src/tests/small_trace_tests.rs @@ -17,7 +17,7 @@ use crate::{ type Felt = FieldElement; -fn make_valid_simple_proof() -> ( +pub(crate) fn make_valid_simple_proof() -> ( SimpleAdditionAIR, crate::proof::stark::StarkProof< GoldilocksField, diff --git a/crypto/stark/src/trace.rs b/crypto/stark/src/trace.rs index da4a53f6e..72b77947a 100644 --- a/crypto/stark/src/trace.rs +++ b/crypto/stark/src/trace.rs @@ -645,22 +645,6 @@ where Table::new(table_data, table_width) } -pub fn columns2rows(columns: Vec>) -> Vec> -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( x: &FieldElement, frame_offsets: &[usize], diff --git a/prover/src/instruments.rs b/prover/src/instruments.rs index f15223e18..b787af0da 100644 --- a/prover/src/instruments.rs +++ b/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), diff --git a/prover/tests/cuda_path_integration.rs b/prover/tests/cuda_path_integration.rs index 0c0bb00c2..521ecd0d3 100644 --- a/prover/tests/cuda_path_integration.rs +++ b/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");