fix(ACIR): check vector length is not zero before pop_front#11107
fix(ACIR): check vector length is not zero before pop_front#11107
Conversation
Changes to circuit sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
There was a problem hiding this comment.
⚠️ 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: 3240f52 | Previous: 1d4b60b | Ratio |
|---|---|---|---|
test_report_zkpassport_noir-ecdsa_ |
2 s |
1 s |
2 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
⚠️ 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: da67f30 | Previous: 79e93d8 | Ratio |
|---|---|---|---|
rollup-block-root-single-tx |
0.003 s |
0.002 s |
1.50 |
rollup-checkpoint-merge |
0.004 s |
0.003 s |
1.33 |
rollup-root |
0.005 s |
0.004 s |
1.25 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'ACVM Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: da67f30 | Previous: 79e93d8 | Ratio |
|---|---|---|---|
perfectly_parallel_batch_inversion_opcodes |
2791165 ns/iter (± 3117) |
2262216 ns/iter (± 3305) |
1.23 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
|
It seems moving the fix to ssagen leads to worse performance than just adding the check in ACIR. |
|
jfecher
left a comment
There was a problem hiding this comment.
I'm alright with fixing this first and worrying about optimizations later unless it'd be trivial to improve
It should just be removal from ACIR gen. @asterite I am realizing now though that someone could write a malicious SSA though that does not include these checks. We probably would need to move all these checks (for In a follow-up, we could have these checks included by an SSA pass so that we do not have repeated logic for Brillig gen and ACIR gen. |
This reverts commit b8f909f.
|
@vezenovm All good! I had to leave for a while so I couldn't revert the last PR, otherwise I would have as soon as I saw those regressions :-) |
There was a problem hiding this comment.
⚠️ 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: 3240f52 | Previous: 1d4b60b | Ratio |
|---|---|---|---|
rollup-block-root-single-tx |
0.003 s |
0.002 s |
1.50 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
Description
Problem
Resolves https://github.com/noir-lang/noir/security/advisories/GHSA-h47v-w5hw-q4x3
Summary
Additional Context
User Documentation
Check one:
PR Checklist
cargo fmton default settings.