fix(acir_gen): Do not call into AcirValue::flatten during AcirValue::flat_numeric_types#9894
Conversation
Changes to circuit sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
![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: da360a9 | Previous: a2ec2e5 | Ratio |
|---|---|---|---|
test_report_zkpassport_noir-ecdsa_ |
2 s |
1 s |
2 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
| array.into_iter().flat_map(|elem| elem.flat_numeric_types()).collect() | ||
| } | ||
| AcirValue::DynamicArray(AcirDynamicArray { value_types, .. }) => value_types, | ||
| _ => unreachable!("An AcirValue::Var cannot be used as an array value"), |
There was a problem hiding this comment.
@TomAFrench comments such as these is why I think it would be great to get a walkthrough of how ACIR arrays are supposed to work in general. cc @asterite
There was a problem hiding this comment.
Yeah they could use a revamp and improved documentation in general. I'll be going over the arrays soon for audit prep work.
There was a problem hiding this comment.
Agreed, I can maybe do some docs writing next week while doing the ignition review.
| @@ -156,11 +157,11 @@ impl AcirValue { | |||
|
|
|||
| pub(super) fn flat_numeric_types(self) -> Vec<NumericType> { | |||
There was a problem hiding this comment.
It would be nice to add some docs to this method, in particular why this can work for all AcirValue variants, but flatten does not, while they conceptually sound similar.
There was a problem hiding this comment.
Added some comments
|
Merging into the parent @aakoshh |
3 participants
Description
Problem*
Alternative to #9887
In #9860 we look to be panicking due to attempting flatten an
AcirValue::Arraycontaining anAcirValue::DynamicArray. This panic is ultimately triggered by a call to flat_numeric_types. We should be able to safely write an array value that contains dynamic arrays (in fact this call toflat_numeric_typesoccurs afterarray_set_value).Summary*
I switched to iterating through each array element in
flat_numeric_typesrather than callingAcirValue::flattendirectly.Additional Context
These issues could be avoid with #6231 as well as arrays will not be able to contain one another.
Documentation*
Check one:
PR Checklist*
cargo fmton default settings.