Skip to content

Commit

Permalink
fix: fix assert_split_transformed_value_arrays conditional access i…
Browse files Browse the repository at this point in the history
…ndex underflow (#12540)

Closes #10592

## Bug

We previously commented out the array checks for private logs in tail to
public (#10593)
because a constraint was mysteriously failing
(#10530).

It failed because of an attempted underflow array access e.g. `array[i -
1]` where `i = 0`. The thing is that the array access shouldn't have
been called because it was wrapped in an if statement.

I don't think this has anything to do with logs, it just so happened
that logs were the first tx effect array to have `num_non_revertibles =
0` in the above bot run.

## Fix

In this case we had a private log with `.counter() = 8`, `split_counter
= 3`, and `num_non_revertibles = 0`. However the constraint failure* was
coming from `sorted_array[num_non_revertibles - 1]` below:
```rust
 if num_non_revertibles != 0 {
        assert(
            sorted_array[num_non_revertibles - 1].counter() < split_counter,
            "counter of last non-revertible item is not less than the split counter",
        );
    }
```
I added a hacky fix which prevents the constraint failure by simply
multiplying the LHS by zero if the array access will fail:
```rust
   if num_non_revertibles != 0 {
        let is_non_zero = (num_non_revertibles != 0) as u32;
        assert(
            sorted_array[num_non_revertibles - 1].counter()*is_non_zero < split_counter,
            "counter of last non-revertible item is not less than the split counter",
        );
    }
```

*(If we had incorrectly entered the if statement and the assertion
failed, the error would be `Assertion failed`, but the error was
`Constraint failure`).

## Repro

I included the `Prover.toml` which came from the above failure - it's
valid and should pass. To see the failure remove `is_non_zero` from the
above code (in `assert_split_transformed_value_arrays.nr`), then run:

`nargo execute --package private_kernel_tail_to_public`

Add back the fix and run the same to see it passing.
  • Loading branch information
MirandaWood authored Mar 7, 2025
1 parent 866582e commit 1ead899
Show file tree
Hide file tree
Showing 4 changed files with 138 additions and 180 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@ use dep::types::{
log_hash::ScopedLogHash,
note_hash::ScopedNoteHash,
nullifier::ScopedNullifier,
private_log::{PrivateLog, PrivateLogData},
public_call_request::PublicCallRequest,
side_effect::Counted,
side_effect::{Counted, scoped::Scoped},
},
messaging::l2_to_l1_message::ScopedL2ToL1Message,
utils::arrays::{
Expand Down Expand Up @@ -86,13 +87,13 @@ impl TailToPublicOutputValidator {
);

// private_logs
// assert_split_transformed_value_arrays(
// prev_data.private_logs,
// output_non_revertible.private_logs,
// output_revertible.private_logs,
// |l: Scoped<PrivateLogData>, out: PrivateLog| out == l.inner.log,
// split_counter,
// );
assert_split_transformed_value_arrays(
prev_data.private_logs,
output_non_revertible.private_logs,
output_revertible.private_logs,
|l: Scoped<PrivateLogData>, out: PrivateLog| out == l.inner.log,
split_counter,
);
}

fn validate_propagated_sorted_values(self) {
Expand Down
Loading

0 comments on commit 1ead899

Please sign in to comment.