Complete integration of Mavros (including lookups)#443
Conversation
CSP benchmarks
Prover time, peak RSS, peak heap, and verifier time are arithmetic means across the iterations. Peak heap comes from the largest No baseline available yet — deltas will appear once this workflow has produced at least one successful Results
|
| // Public re-exports for items used by integration tests and benchmarks. | ||
| pub use {ec_arith::ec_scalar_mul, r1cs::solve_witness_vec}; | ||
|
|
||
| fn log_commit_input(label: &str, values: &[FieldElement], scheme_domain_len: usize) { |
There was a problem hiding this comment.
Can we have this in something like tracing.rs and if needed at multiple places then can put it in common?
There was a problem hiding this comment.
O(n) is_zero() scan on the prove hot path at info level. Drop nonzero_entries, or gate on tracing::enabled!(Level::DEBUG), or move the whole call to debug!.
| .collect::<Result<Vec<_>>>()? | ||
| }; | ||
|
|
||
| log_commit_input("noir_w1", &w1, 1usize << self.whir_for_witness.m); |
| ); | ||
|
|
||
| let mut phase1 = phase1; | ||
| let w1 = take(&mut phase1.out_wit_pre_comm); |
There was a problem hiding this comment.
This take() is unsafe with Mavros lookup tables. Phase1Result.tables stores raw multiplicities_wit pointers into out_wit_pre_comm; commit() pads/resizes the moved vector before run_phase2, so those pointers can become stale if the Vec reallocates. Please keep phase1.out_wit_pre_comm owned by phase1 until after run_phase2 (e.g. clone for the commitment), or change Mavros to store offsets instead of raw pointers.
| ); | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
Can you add cases for w1_size == num_witnesses (w2=0), w1_size == 0, and an exact-power-of-two pair that exercises the (1<<m) - w1_size < 4*m_0 bump. Also cover the analogous path in new_for_r1cs.
|
Leaving the |
| #[cfg(not(target_arch = "wasm32"))] | ||
| pub mod input_utils; | ||
| pub(crate) mod r1cs; | ||
| mod tracing; |
There was a problem hiding this comment.
mod tracing; shadows the extern tracing crate, forcing ::tracing::{...} at every import site (lib.rs:8, r1cs.rs:3, whir_r1cs.rs:2). Rename to mod logging / mod trace_utils.
| pub lookups_data_size: usize, | ||
| } | ||
|
|
||
| impl WitnessLayout { |
There was a problem hiding this comment.
The new size() / pre_commitment_size() / post_commitment_size() methods are pub but have no /// docs. Workspace lints cargo doc -D warnings, so one-liners each.
| .instance(&instance); | ||
| let mut merlin = ProverState::new(&ds, TranscriptSponge::from_config(self.hash_config)); | ||
|
|
||
| info!( |
There was a problem hiding this comment.
this 11-field info! can collapse to info!(?self.witness_layout, ?self.constraints_layout, scheme_domain_len = ..., "Mavros witness layout"); WitnessLayout/ConstraintsLayout already derive Debug.
| total_witness_size = self.witness_layout.size(), | ||
| constraints_algebraic_size = self.constraints_layout.algebraic_size, | ||
| constraints_total_size = self.constraints_layout.size(), | ||
| scheme_domain_len = 1usize << self.whir_for_witness.m, |
There was a problem hiding this comment.
nit: 1usize << self.whir_for_witness.m repeats at :314, :322, :352 (and :188, :226 in NoirProver). Add a const fn WhirR1CSScheme::domain_size(&self) -> usize.
2 participants
Adds a new test for sha256_35 (35 rounds).
The initial integration contained a few issues: