Skip to content

fix: correct index out of bounds location#11685

Merged
asterite merged 6 commits intomasterfrom
ab/improve-index-out-of-bounds-location
Feb 25, 2026
Merged

fix: correct index out of bounds location#11685
asterite merged 6 commits intomasterfrom
ab/improve-index-out-of-bounds-location

Conversation

@asterite
Copy link
Collaborator

Description

Problem

Resolves https://cantina.xyz/code/50033e8c-8b46-41bc-b019-62098708057b/findings?s=-status:spam,withdrawn+order_by:severity_asc&finding=7

Summary

The finding suggests that it might be incorrect to use the last ACIR opcode location for the failure location. However, what was "wrong" was that when we generated a constrain for the index out of bounds, we did it in the incorrect location. Now we do it in the same location that the array_get happens.

Additional Context

I wanted to add a regression test for this but I noticed that we don't capture snapshots of execution_failure tests. Then I tried to do that but I found that the error messages we produce for ACIR, Brillig and comptime are sometimes different. Then I thought it might be better to do that in a separate PR, either by storing separate snapshots for different runtimes, or by trying to always produce the same error in all runtimes.

While doing this I noticed that one execution_failure test was failing because it lacked an input argument, and not because it failed to execute, so I fixed that in this PR too.

User Documentation

Check one:

  • No user documentation needed.
  • Changes in docs/ included in this PR.
  • [For Experimental Features] Changes in docs/ to be submitted in a separate PR.

PR Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@asterite asterite changed the title Ab/improve index out of bounds location @asterite fix: correct index out of bounds location Feb 25, 2026
@asterite asterite changed the title @asterite fix: correct index out of bounds location fix: correct index out of bounds location Feb 25, 2026
@TomAFrench
Copy link
Member

I wanted to add a regression test for this but I noticed that we don't capture snapshots of execution_failure tests. Then I tried to do that but I found that the error messages we produce for ACIR, Brillig and comptime are sometimes different. Then I thought it might be better to do that in a separate PR, either by storing separate snapshots for different runtimes, or by trying to always produce the same error in all runtimes.

Yeah, harmonising these would be very nice but would be a fair bit of work.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Brillig Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 6c8b20e Previous: 34e3758 Ratio
rollup-tx-merge 0.002 s 0.001 s 2

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: e73f3dd Previous: 34e3758 Ratio
rollup-checkpoint-merge 0.003 s 0.002 s 1.50

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 6c8b20e Previous: 34e3758 Ratio
test_report_zkpassport_noir-ecdsa_ 3 s 1 s 3

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

@asterite
Copy link
Collaborator Author

Yeah, harmonising these would be very nice but would be a fair bit of work.

Yes. It's also hard because in comptime we usually have more context to provide a better error message, and it would be bad to degrade these errors to match ACIR/Brillig. For now I think I'll just capture snapshosts of the different backends.

@asterite asterite enabled auto-merge February 25, 2026 14:07
@asterite asterite added this pull request to the merge queue Feb 25, 2026
Merged via the queue into master with commit fffdad0 Feb 25, 2026
136 of 137 checks passed
@asterite asterite deleted the ab/improve-index-out-of-bounds-location branch February 25, 2026 16:50
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