fix: track MakeArray elements as Brillig call arguments in underconstrained check#11816
fix: track MakeArray elements as Brillig call arguments in underconstrained check#11816TomAFrench wants to merge 6 commits intomasterfrom
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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: 04e02c7 | Previous: 1aacfa9 | Ratio |
|---|---|---|---|
test_report_zkpassport_noir-ecdsa_ |
4 s |
3 s |
1.33 |
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: 4ef8d73 | Previous: 667dd9b | 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
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: 9dd1365 | Previous: 331250b | Ratio |
|---|---|---|---|
rollup-tx-merge |
0.002 s |
0.001 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 'ACVM Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 04e02c7 | Previous: 1aacfa9 | Ratio |
|---|---|---|---|
perfectly_parallel_batch_inversion_opcodes |
3135472 ns/iter (± 6503) |
2207346 ns/iter (± 2826) |
1.42 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
| Instruction::Cast(..) | ||
| | Instruction::Truncate { .. } | ||
| | Instruction::MakeArray { .. } | ||
| | Instruction::ArraySet { .. } |
There was a problem hiding this comment.
I think it should be enough to do add MakeArray here, and not do the extended_args further up.
It's not a very general mechanism, but what this seems to achieve is that if we know that the output of the MakeArray is an input to a call which we want to cover by constraints, then it adds the arguments of the MakeArray to the arguments of the call itself. Only the direct arguments, but the effect should be the same.
I reckon it would be potentially easier to understand the whole thing if we tracked the ancestry of everything, and this would not be behind the enable_lookback flag, but that's what its purpose is: to establish the connection between not just the arguments of the call, but anything that descends from the args before the call is made, and in this limited set to the 1 level ancestors as well.
There was a problem hiding this comment.
Although I see the extended_args is going back to collect more ancestors, which is nice.
aakoshh
force-pushed
the
tf/fix-underconstrained-make-array
branch
from
4ef8d73 to
f500834
Compare
3 participants
…
Description
Problem
Resolves #11807
Summary
We currently don't link the inputs to a MakeArray instruction to the result itself, this PR traces back all of the MakeArrays making up the brillig call argument and adds them to the set of arguments
Additional Context
User Documentation
Check one:
PR Checklist
cargo fmton default settings.