noir-lang / noir

Noir is a domain specific language for zero knowledge proofs
https://noir-lang.org
Apache License 2.0
821 stars 177 forks source link

Dead Instruction Elimination pass removes out of bounds array sets #5296

Open Thunkar opened 1 week ago

Thunkar commented 1 week ago

Aim

While moving some tests from constrained to unconstrained so they run faster (we're only checking function logic and assume circuit generation is fine), the code that used to work started failing with "Array index out of bounds" errors.

Expected Behavior

Behavior should be consistent between constrained and unconstrained contexts.

Bug

Out of bounds errors can be masked in certain scenarios, specifically when copying array positions from one to another. However, illegal acceses are always detected as expected in unconstrained functions.

To Reproduce

// Taken from noir-protocol-circuits
fn arr_copy_slice<T, N, M>(src: [T; N], mut dst: [T; M], offset: u32) -> [T; M] {
    for i in 0..dst.len() {
        // We are illegally accessing the src array here if dst.len() > src.len()
        dst[i] = src[i + offset];
    }
    dst
}

unconstrained fn boom<N, M>(src: [Field; N], dst: [Field; M]) -> [Field; M] {
    arr_copy_slice(src, dst, 0)
}

fn main() {
    // This shouldn't work
    let small_array_src = [1; 5];
    let mut big_array_dst = [0; 6];
    let mut result = arr_copy_slice(small_array_src, big_array_dst, 0);

    // Uncomment for fireworks. This *always* get caught in unconstrained land
    //let result = boom();

    // This is fine
    let first = result[0];
    dep::std::println(first);
    // This is also fine, since we are overwriting the "illegal" position.
    // BUT if we comment this line out, fireworks again (since we are now "using" the illegal position)
    result[5] = 1;

    dep::std::println(result[5]);
}

Project Impact

Nice-to-have

Impact Context

No response

Workaround

None

Workaround Description

No response

Additional Context

No response

Installation Method

Compiled from source

Nargo Version

No response

NoirJS Version

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

jfecher commented 1 week ago

Interesting, it looks like the pass to remove unused code removes the array_set instruction in question since it was unused in the final program. This explains why it also still catches it if that index is later printed out without being mutated first.