Skip to content

fix: fix assert_split_transformed_value_arrays conditional access index underflow#12540

Merged
MirandaWood merged 1 commit into
masterfrom
mw/index-underflow-fix
Mar 7, 2025
Merged

fix: fix assert_split_transformed_value_arrays conditional access index underflow#12540
MirandaWood merged 1 commit into
masterfrom
mw/index-underflow-fix

Conversation

@MirandaWood

Copy link
Copy Markdown
Contributor

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:

 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:

   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.

@MirandaWood MirandaWood requested a review from LeilaWang March 6, 2025 18:05

@LeilaWang LeilaWang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tracking this down!

@asterite

Copy link
Copy Markdown
Contributor

@MirandaWood Do you know if the bug still exists? When I follow the "Repro" steps, running nargo execute --package private_kernel_tail_to_public works fine for me. Could you try the repro steps on your machine to see if it fails?

@MirandaWood

MirandaWood commented Mar 13, 2025

Copy link
Copy Markdown
Contributor Author

@MirandaWood Do you know if the bug still exists? When I follow the "Repro" steps, running nargo execute --package private_kernel_tail_to_public works fine for me. Could you try the repro steps on your machine to see if it fails?

Hi, I believe it does since we saw it today in another if statement (#12711). Which prover.toml did you use? It may have been updated since this PR to one that doesn't reach this code, or one that does not lead to num_non_revertibles = 0

edit: using the prover.toml on this PR fails on my side when removing the fix

@asterite

Copy link
Copy Markdown
Contributor

@MirandaWood Thank you, that was it! I didn't realize Prover.toml might have been changed since this PR. Using the one in this PR I was able to reproduce the issue.

Maddiaa0 pushed a commit that referenced this pull request Mar 14, 2025
…ex overflow (#12711)

Fixes exactly the same problem as #12540, but on the following if
statement as an index overflow. See that description for more details!

This is being looked into in noir:
noir-lang/noir#7612
MirandaWood added a commit that referenced this pull request Jun 4, 2025
## Finalises integration of batched blobs

`mw/blob-batching-integration` adds batching to the rollup .nr circuits
only (=> will not run in the repo). This PR brings those changes
downstream to the typescript and L1 contracts. Main changes:

- L1 Contracts:
- No longer calls the point evaluation precompile on `propose`, instead
injects the blob commitments, check they correspond to the broadcast
blobs, and stores them in the `blobCommitmentsHash`
- Does not store any blob public inputs apart from the
`blobCommitmentsHash` (no longer required)
- Calls the point evaluation precompile once on `submitEpochRootProof`
for ALL blobs in the epoch
- Uses the same precompile inputs as pubic inputs to the root proof
verification alonge with the `blobCommitmentsHash` to link the circuit
batched blob, real L1 blobs, and the batched blob verified on L1
- Refactors mock blob oracle
- Injects the final blob challenges used on each blob into all block
building methods in `orchestrator`
- Accumulates blobs in ts when building blocks and uses as inputs to
each rollup circuit
- Returns the blob inputs required for `submitEpochRootProof` on
`finaliseEpoch()`
- Updates nr structs in ts plus fixtures and tests


## TODOs/Current issues

- ~When using real proofs (e.g.
`yarn-project/prover-client/src/test/bb_prover_full_rollup.test.ts`),
the root rollup proof is generated correctly but fails verification
checks in `bb` due to incorrect number of public inputs. Changing the
number correctly updates vks and all constants elsewhere, but `bb` does
not change.~ EDIT: solved - must include the `is_inf` point member for
now (see below TODO)
- ~The `Prover.toml` for block-root is not executing. The error
manifests in the same way as that in
#12540 (but may be
different).~ EDIT: temporarily fixed - details in this repro (#14381)
and noir issue (noir-lang/noir#8563).
- BLS points in noir take up 9 fields (4 for each coordinate as a limbed
bignum, 1 for the `is_inf` flag) but can be compressed to only 2. For
recursive verification in block root and above, would it be worth the
gates to compress these? It depends whether the gate cost of compression
is more/less than gate cost of recursively verifying 7 more public
inputs.

## PR Stack

- [ ] `mw/blob-batching` <- main feature
- [ ] ^ `mw/blob-batching-bls-utils` <- BLS12-381 bigcurve and bignum
utils (noir) (#13583)
- [ ] ^ `mw/blob-batching-bls-utils-ts` <- BLS12-381 bigcurve and bignum
utils (ts) (#13606)
- [ ] ^ `mw/blob-batching-integration` <- Integrate batching into noir
protocol circuits (#13817)
- [x] ^ `mw/blob-batching-integration-ts-sol` <- Integrate batching into
ts and solidity (#14329)

---------

Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
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.

bug: assert_split_transformed_value_arrays fails to prove for private logs

3 participants