Skip to content

fix for non-contiguous initial value witnesses in SpiceWitness#254

Merged
Bisht13 merged 4 commits into
mainfrom
ysh/initial-value-witnesses
Jan 19, 2026
Merged

fix for non-contiguous initial value witnesses in SpiceWitness#254
Bisht13 merged 4 commits into
mainfrom
ysh/initial-value-witnesses

Conversation

@yash25198
Copy link
Copy Markdown
Collaborator

Summary

For Issue #253
Fixes incorrect memory reads in SpiceWitness solver by storing actual witness indices instead of assuming contiguous allocation. The old implementation would read garbage values when memory blocks are initialized with repeated values.

Problem

The SpiceWitnesses struct assumed initial value witnesses were stored contiguously starting from initial_values_start:

// OLD: Assumes witnesses are at indices [start, start+1, start+2, ...]
pub initial_values_start: usize,

The solver would then read memory like:

let rv_final = witness[initial_values_start..initial_values_start + memory_length].to_vec();

When compiling Noir to R1CS, fetch_r1cs_witness_index() maps ACIR witnesses to R1CS witnesses. If the same ACIR witness appears multiple times (e.g., initializing an array with zeros), it returns the same R1CS index for all of them.

Example: BoundedVec<u8, 32> initialized with zeros

ACIR witnesses for init: [w5, w5, w5, w5, w5, ...]  (all point to same "0" witness)
                            ↓   ↓   ↓   ↓   ↓
R1CS indices:            [54, 54, 54, 54, 54, ...]  (all map to index 54)

But the old impl assumed:

Expected by old impl:    [54, 55, 56, 57, 58, ...]  (contiguous range)

What happens when solving:

// Old impl reads witness[54..54+32], i.e., witness[54..86]
// But indices 55, 56, 57... contain UNRELATED witness values!
//
// Memory slot 0: witness[54] = 0  ✓ correct
// Memory slot 1: witness[55] = ???  ✗ GARBAGE (some other witness)
// Memory slot 2: witness[56] = ???  ✗ GARBAGE
// ...

This causes silent corruption: the prover computes wrong values, and proving fails with cryptic errors or produces invalid proofs.

Fix

Change from a start index to storing the actual witness index for each memory slot:

// Store actual indices (may have duplicates)
pub initial_value_witnesses: Vec<usize>,

The solver now reads from the correct locations:

// Read from actual indices
let rv_final: Vec<_> = self.initial_value_witnesses
    .iter()
    .map(|&idx| witness[idx])
    .collect();

// Memory slot 0: witness[54] = 0  ✓
// Memory slot 1: witness[54] = 0  ✓ (same index, correct value!)
// Memory slot 2: witness[54] = 0  ✓
// ...

Test Cases Added

bounded-vec

Tests BoundedVec concatenation with conditional increments. This exercises:

  • Memory blocks initialized with repeated zeros (all slots point to same witness)
  • Conditional writes based on dynamic length
  • The exact pattern that triggered this bug
fn main(data: BoundedVec<u8, 32>, suffix: [u8; 4]) -> pub [u8; 36] {
    let result = concat(data, suffix);
    result.storage()
}

brillig-unconstrained

Tests Brillig (unconstrained) functions that modify arrays, mimicking patterns seen in sha256 stdlib usage.

Test

  • cargo test --package provekit-bench --test compiler -- bounded-vec
  • cargo test --package provekit-bench --test compiler -- brillig-unconstrained
  • Full test suite: cargo test --package provekit-bench --test compiler

@yash25198 yash25198 changed the title fix for non-coniguous initial value witnesses in SpiceWitness fix for non-contiguous initial value witnesses in SpiceWitness Jan 16, 2026
@yash25198 yash25198 requested a review from Bisht13 January 16, 2026 20:29
@yash25198 yash25198 marked this pull request as ready for review January 17, 2026 02:47
… formatting

- Add trailing newline to bounded-vec/src/main.nr
- Add debug_assert to verify initial_value_witnesses.len() == memory_length
- Replace eprintln + assert with descriptive assert message in noir_to_r1cs.rs
- Revert unrelated variable renames in witness_builder.rs (SpiceMultisetFactor)
- Revert unrelated variable renames in prover/witness/ram.rs (Load/Store arms)

These changes keep the PR focused on the actual bug fix.
@Bisht13 Bisht13 merged commit 66d8597 into main Jan 19, 2026
3 of 5 checks passed
@Bisht13 Bisht13 deleted the ysh/initial-value-witnesses branch January 19, 2026 06:22
dcbuild3r pushed a commit that referenced this pull request May 16, 2026
fix for non-contiguous initial value witnesses in SpiceWitness
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants