Enhance core logic: AIR constraints, Executor decoding, and Trace col…#19
Enhance core logic: AIR constraints, Executor decoding, and Trace col…#19this-vishalsingh wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the RV32IM proving stack by expanding trace-related structures (including bit/byte decompositions for constraints/lookups), refining executor trace/memory/syscall representations, and reformatting tests/benches for readability and consistency.
Changes:
- Extend
TraceColumnsexport to include bit- and byte-decomposition columns and add memory-efficient column conversion helpers. - Reformat and slightly reorganize executor trace/syscall-related types (notably
MemOp) and assorted core modules for consistency. - Reformat crypto syscall tests and benchmarks (SHA256/RIPEMD160/MODEXP/KECCAK/ECRECOVER/BLAKE2B) without changing intended behavior.
Reviewed changes
Copilot reviewed 19 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/trace/src/columns.rs | Emits additional bit/byte decomposition columns and adds into_columns() + memory estimation helpers (docs/constants need alignment with actual emitted column count). |
| crates/executor/src/trace.rs | Reformats MemOp variants (architecture-dependent usize fields should be reconsidered for deterministic serialization). |
| crates/executor/src/cpu.rs | Reformats syscall handling and error returns; no functional deltas spotted in reviewed hunks. |
| crates/executor/src/memory.rs | Formatting-only changes in alignment error construction and docs. |
| crates/executor/src/decode.rs | Formatting-only changes in instruction formatting helpers. |
| crates/executor/src/syscall.rs | Formatting-only changes in enum docs and layout. |
| crates/executor/src/lib.rs | Reorders module declarations/exports. |
| crates/executor/src/error.rs | Formatting-only changes to error attributes/struct layout. |
| crates/executor/src/elf.rs | Formatting-only changes to error construction and parsing helpers. |
| crates/air/src/lib.rs | Reorders module exports. |
| crates/air/src/memory.rs | Whitespace/doc formatting only. |
| crates/air/src/cpu.rs | Signature formatting/layout changes; constraint helpers and tests remain as before. |
| crates/air/benches/constraint_bench.rs | Reorders imports + formatting; adds explicit CpuTraceRow/ConstraintEvaluator import ordering. |
| crates/executor/benches/syscall_bench.rs | Import reordering + formatting only. |
| crates/executor/tests/test_sha256.rs | Formatting-only changes. |
| crates/executor/tests/test_ripemd160.rs | Formatting-only changes. |
| crates/executor/tests/test_modexp.rs | Formatting-only changes. |
| crates/executor/tests/test_keccak.rs | Formatting-only changes; some assertions rewritten to multi-line for readability. |
| crates/executor/tests/test_ecrecover.rs | Import ordering + formatting-only changes. |
| crates/executor/tests/test_blake2b.rs | Formatting-only changes. |
Comments suppressed due to low confidence (1)
crates/trace/src/columns.rs:82
NUM_CPU_COLUMNSand the surrounding docs still state the trace has 77 columns, butTraceColumnscontains additional bit/byte decomposition columns andto_columns()/into_columns()emit them. Consider renaming this to something likeNUM_CORE_CPU_COLUMNSand introducing aNUM_TRACE_COLUMNS(e.g., 77 + 326 + 45) that matches the actual output, and use it for capacity estimates and documentation.
/// Number of columns in the CPU trace.
pub const NUM_CPU_COLUMNS: usize = 77;
/// Complete trace columns for RV32IM AIR constraints.
///
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| //! // Convert to flat vector for prover | ||
| //! let column_vec = columns.to_columns(); // Vec<Vec<M31>> | ||
| //! assert_eq!(column_vec.len(), 77); |
There was a problem hiding this comment.
The module-level example asserts columns.to_columns().len() == 77, but to_columns() now appends bit-decomposition and byte-decomposition columns (77 core + 326 + 45 = 289). This example is now incorrect and will mislead callers; update the expected length (ideally via a constant) or adjust to_columns() to return only the core 77 columns and expose a separate method for the extended trace.
| Ecrecover { input_ptr: u32, output_ptr: u32 }, | ||
| /// SHA-256 hash operation (delegated to specialized circuit). | ||
| Sha256 { message_ptr: usize, message_len: usize, digest_ptr: usize }, | ||
| Sha256 { | ||
| message_ptr: usize, | ||
| message_len: usize, | ||
| digest_ptr: usize, | ||
| }, | ||
| /// RIPEMD-160 hash operation (delegated to specialized circuit). | ||
| Ripemd160 { message_ptr: usize, message_len: usize, digest_ptr: usize }, | ||
| Ripemd160 { | ||
| message_ptr: usize, | ||
| message_len: usize, | ||
| digest_ptr: usize, | ||
| }, | ||
| /// Modular exponentiation (delegated to specialized circuit for RSA/crypto). | ||
| Modexp { base_ptr: usize, exp_ptr: usize, mod_ptr: usize, result_ptr: usize }, | ||
| Modexp { | ||
| base_ptr: usize, | ||
| exp_ptr: usize, | ||
| mod_ptr: usize, | ||
| result_ptr: usize, | ||
| }, | ||
| /// Blake2b hash operation (delegated to specialized circuit). | ||
| Blake2b { message_ptr: usize, message_len: usize, digest_ptr: usize }, | ||
| Blake2b { | ||
| message_ptr: usize, | ||
| message_len: usize, | ||
| digest_ptr: usize, | ||
| }, |
There was a problem hiding this comment.
Several MemOp variants store pointers/lengths as usize (e.g., Sha256, Ripemd160, Modexp, Blake2b) while others use u32. Because ExecutionTrace is Serialize/Deserialize, using usize makes the trace format architecture-dependent (32-bit vs 64-bit) and complicates interoperability/determinism. Prefer u32 for all pointers/lengths in the trace, matching the CPU address space and the other MemOp variants.
…umns