From ba2c541ec45de92bba98de34771b73cbb7865c93 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 14 Feb 2024 18:09:02 +0000 Subject: [PATCH] fix(acir): Use types on dynamic arrays (#4364) # Description ## Problem\* Resolves #4356 Supercedes https://github.com/noir-lang/noir/pull/4360 ## Summary\* An ACIR dynamic array is a pointer to flat memory. We have been treating this flat memory as a list of fields, however, this breaks if we do in fact need accurate numeric type information such as when working black box function inputs. For example for hash inputs we set up the byte array based upon the bit size. This needs to be the correct bit size or else we will get a lot of extra garbage when calling `fetch_nearest_bytes` on a FieldElement. This PR attaches a list of `Vec` to the `AcirDynamicArray` structure. This gives us the expected output result for `sha` then. We probably could restrict the `AcirDynamicArray` to be created only through a constructor where we check that the `value_types` match the supplied len in size. I left it for a follow-up as this is a quick fix but I can do it as part of this PR. ## Additional Context ## Documentation\* Check one: - [ ] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** Documentation to be submitted in a separate PR. # PR Checklist\* - [ ] I have tested the changes locally. - [ ] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: TomAFrench --- .../src/ssa/acir_gen/acir_ir/acir_variable.rs | 17 +++- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 87 ++++++++++++++++--- .../array_dynamic_blackbox_input/Nargo.toml | 7 ++ .../array_dynamic_blackbox_input/Prover.toml | 4 + .../array_dynamic_blackbox_input/src/main.nr | 27 ++++++ .../Nargo.toml | 7 ++ .../Prover.toml | 23 +++++ .../src/main.nr | 20 +++++ 8 files changed, 177 insertions(+), 15 deletions(-) create mode 100644 test_programs/execution_success/array_dynamic_blackbox_input/Nargo.toml create mode 100644 test_programs/execution_success/array_dynamic_blackbox_input/Prover.toml create mode 100644 test_programs/execution_success/array_dynamic_blackbox_input/src/main.nr create mode 100644 test_programs/execution_success/array_dynamic_nested_blackbox_input/Nargo.toml create mode 100644 test_programs/execution_success/array_dynamic_nested_blackbox_input/Prover.toml create mode 100644 test_programs/execution_success/array_dynamic_nested_blackbox_input/src/main.nr diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs index 2360d053887..fb11bae556c 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs @@ -67,6 +67,13 @@ impl AcirType { pub(crate) fn unsigned(bit_size: u32) -> Self { AcirType::NumericType(NumericType::Unsigned { bit_size }) } + + pub(crate) fn to_numeric_type(&self) -> NumericType { + match self { + AcirType::NumericType(numeric_type) => *numeric_type, + AcirType::Array(_, _) => unreachable!("cannot fetch a numeric type for an array type"), + } + } } impl From for AcirType { @@ -88,6 +95,12 @@ impl<'a> From<&'a SsaType> for AcirType { } } +impl From for AcirType { + fn from(value: NumericType) -> Self { + AcirType::NumericType(value) + } +} + #[derive(Debug, Default)] /// Context object which holds the relationship between /// `Variables`(AcirVar) and types such as `Expression` and `Witness` @@ -1415,13 +1428,13 @@ impl AcirContext { } Ok(values) } - AcirValue::DynamicArray(AcirDynamicArray { block_id, len, .. }) => { + AcirValue::DynamicArray(AcirDynamicArray { block_id, len, value_types, .. }) => { try_vecmap(0..len, |i| { let index_var = self.add_constant(i); Ok::<(AcirVar, AcirType), InternalError>(( self.read_from_memory(block_id, &index_var)?, - AcirType::field(), + value_types[i].into(), )) }) } diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 46be6efcadd..8d4d0668534 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -99,6 +99,18 @@ pub(crate) struct AcirDynamicArray { block_id: BlockId, /// Length of the array len: usize, + /// An ACIR dynamic array is a flat structure, so we use + /// the inner structure of an `AcirType::NumericType` directly. + /// Some usages of ACIR arrays (e.g. black box functions) require the bit size + /// of every value to be known, thus we store the types as part of the dynamic + /// array definition. + /// + /// A dynamic non-homogenous array can potentially have values of differing types. + /// Thus, we store a vector of types rather than a single type, as a dynamic non-homogenous array + /// is still represented in ACIR by a single `AcirDynamicArray` structure. + /// + /// The length of the value types vector must match the `len` field in this structure. + value_types: Vec, /// Identification for the ACIR dynamic array /// inner element type sizes array element_type_sizes: Option, @@ -150,6 +162,16 @@ impl AcirValue { AcirValue::DynamicArray(_) => unimplemented!("Cannot flatten a dynamic array"), } } + + fn flat_numeric_types(self) -> Vec { + match self { + AcirValue::Array(_) => { + self.flatten().into_iter().map(|(_, typ)| typ.to_numeric_type()).collect() + } + AcirValue::DynamicArray(AcirDynamicArray { value_types, .. }) => value_types, + _ => unreachable!("An AcirValue::Var cannot be used as an array value"), + } + } } impl Ssa { @@ -1007,9 +1029,15 @@ impl Context { } else { None }; + + let value_types = self.convert_value(array_id, dfg).flat_numeric_types(); + // Compiler sanity check + assert_eq!(value_types.len(), array_len, "ICE: The length of the flattened type array should match the length of the dynamic array"); + let result_value = AcirValue::DynamicArray(AcirDynamicArray { block_id: result_block_id, len: array_len, + value_types, element_type_sizes, }); self.define_result(dfg, instruction, result_value); @@ -1093,7 +1121,7 @@ impl Context { &mut self, array_typ: &Type, array_id: ValueId, - array_acir_value: Option, + supplied_acir_value: Option<&AcirValue>, dfg: &DataFlowGraph, ) -> Result { let element_type_sizes = self.internal_block_id(&array_id); @@ -1119,26 +1147,23 @@ impl Context { Value::Instruction { .. } | Value::Param { .. } => { // An instruction representing the slice means it has been processed previously during ACIR gen. // Use the previously defined result of an array operation to fetch the internal type information. - let array_acir_value = if let Some(array_acir_value) = array_acir_value { - array_acir_value - } else { - self.convert_value(array_id, dfg) - }; + let array_acir_value = &self.convert_value(array_id, dfg); + let array_acir_value = supplied_acir_value.unwrap_or(array_acir_value); match array_acir_value { AcirValue::DynamicArray(AcirDynamicArray { element_type_sizes: inner_elem_type_sizes, .. }) => { if let Some(inner_elem_type_sizes) = inner_elem_type_sizes { - if self.initialized_arrays.contains(&inner_elem_type_sizes) { - let type_sizes_array_len = self.internal_mem_block_lengths.get(&inner_elem_type_sizes).copied().ok_or_else(|| + if self.initialized_arrays.contains(inner_elem_type_sizes) { + let type_sizes_array_len = *self.internal_mem_block_lengths.get(inner_elem_type_sizes).ok_or_else(|| InternalError::General { message: format!("Array {array_id}'s inner element type sizes array does not have a tracked length"), call_stack: self.acir_context.get_call_stack(), } )?; self.copy_dynamic_array( - inner_elem_type_sizes, + *inner_elem_type_sizes, element_type_sizes, type_sizes_array_len, )?; @@ -1683,15 +1708,24 @@ impl Context { Some(self.init_element_type_sizes_array( &slice_typ, slice_contents, - Some(new_slice_val), + Some(&new_slice_val), dfg, )?) } else { None }; + + let value_types = new_slice_val.flat_numeric_types(); + assert_eq!( + value_types.len(), + new_elem_size, + "ICE: Value types array must match new slice size" + ); + let result = AcirValue::DynamicArray(AcirDynamicArray { block_id: result_block_id, len: new_elem_size, + value_types, element_type_sizes, }); Ok(vec![AcirValue::Var(new_slice_length, AcirType::field()), result]) @@ -1738,15 +1772,24 @@ impl Context { Some(self.init_element_type_sizes_array( &slice_typ, slice_contents, - Some(new_slice_val), + Some(&new_slice_val), dfg, )?) } else { None }; + + let value_types = new_slice_val.flat_numeric_types(); + assert_eq!( + value_types.len(), + new_slice_size, + "ICE: Value types array must match new slice size" + ); + let result = AcirValue::DynamicArray(AcirDynamicArray { block_id: result_block_id, len: new_slice_size, + value_types, element_type_sizes, }); @@ -1943,15 +1986,24 @@ impl Context { Some(self.init_element_type_sizes_array( &slice_typ, slice_contents, - Some(slice), + Some(&slice), dfg, )?) } else { None }; + + let value_types = slice.flat_numeric_types(); + assert_eq!( + value_types.len(), + slice_size, + "ICE: Value types array must match new slice size" + ); + let result = AcirValue::DynamicArray(AcirDynamicArray { block_id: result_block_id, len: slice_size, + value_types, element_type_sizes, }); @@ -2059,15 +2111,24 @@ impl Context { Some(self.init_element_type_sizes_array( &slice_typ, slice_contents, - Some(new_slice_val), + Some(&new_slice_val), dfg, )?) } else { None }; + + let value_types = new_slice_val.flat_numeric_types(); + assert_eq!( + value_types.len(), + slice_size, + "ICE: Value types array must match new slice size" + ); + let result = AcirValue::DynamicArray(AcirDynamicArray { block_id: result_block_id, len: slice_size, + value_types, element_type_sizes, }); diff --git a/test_programs/execution_success/array_dynamic_blackbox_input/Nargo.toml b/test_programs/execution_success/array_dynamic_blackbox_input/Nargo.toml new file mode 100644 index 00000000000..03da304acc3 --- /dev/null +++ b/test_programs/execution_success/array_dynamic_blackbox_input/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "array_dynamic_blackbox_input" +type = "bin" +authors = [""] +compiler_version = ">=0.24.0" + +[dependencies] \ No newline at end of file diff --git a/test_programs/execution_success/array_dynamic_blackbox_input/Prover.toml b/test_programs/execution_success/array_dynamic_blackbox_input/Prover.toml new file mode 100644 index 00000000000..cc60eb8a8ba --- /dev/null +++ b/test_programs/execution_success/array_dynamic_blackbox_input/Prover.toml @@ -0,0 +1,4 @@ +index = "1" +leaf = ["51", "109", "224", "175", "60", "42", "79", "222", "117", "255", "174", "79", "126", "242", "74", "34", "100", "35", "20", "200", "109", "89", "191", "219", "41", "10", "118", "217", "165", "224", "215", "109"] +path = ["0", "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12", "13", "14", "15", "16", "17", "18", "19", "20", "21", "22", "23", "24", "25", "26", "27", "28", "29", "30", "31", "32", "33", "34", "35", "36", "37", "38", "39", "40", "41", "42", "43", "44", "45", "46", "47", "48", "49", "50", "51", "52", "53", "54", "55", "56", "57", "58", "59", "60", "61", "62", "63"] +root = [79, 230, 126, 184, 98, 125, 226, 58, 117, 45, 140, 15, 72, 118, 89, 173, 117, 161, 166, 0, 214, 125, 13, 16, 113, 81, 173, 156, 97, 15, 57, 216] diff --git a/test_programs/execution_success/array_dynamic_blackbox_input/src/main.nr b/test_programs/execution_success/array_dynamic_blackbox_input/src/main.nr new file mode 100644 index 00000000000..aabf7fc9d5c --- /dev/null +++ b/test_programs/execution_success/array_dynamic_blackbox_input/src/main.nr @@ -0,0 +1,27 @@ +fn main(leaf: [u8; 32], path: [u8; 64], index: u32, root: [u8; 32]) { + compute_root(leaf, path, index, root); +} + +fn compute_root(leaf: [u8; 32], path: [u8; 64], _index: u32, root: [u8; 32]) { + let mut current = leaf; + let mut index = _index; + + for i in 0..2 { + let mut hash_input = [0; 64]; + let offset = i * 32; + let is_right = (index & 1) != 0; + let a = if is_right { 32 } else { 0 }; + let b = if is_right { 0 } else { 32 }; + + for j in 0..32 { + hash_input[j + a] = current[j]; + hash_input[j + b] = path[offset + j]; + } + + current = dep::std::hash::sha256(hash_input); + index = index >> 1; + } + + // Regression for issue #4258 + assert(root == current); +} \ No newline at end of file diff --git a/test_programs/execution_success/array_dynamic_nested_blackbox_input/Nargo.toml b/test_programs/execution_success/array_dynamic_nested_blackbox_input/Nargo.toml new file mode 100644 index 00000000000..07d867d433f --- /dev/null +++ b/test_programs/execution_success/array_dynamic_nested_blackbox_input/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "array_dynamic_nested_blackbox_input" +type = "bin" +authors = [""] +compiler_version = ">=0.24.0" + +[dependencies] \ No newline at end of file diff --git a/test_programs/execution_success/array_dynamic_nested_blackbox_input/Prover.toml b/test_programs/execution_success/array_dynamic_nested_blackbox_input/Prover.toml new file mode 100644 index 00000000000..1f291532414 --- /dev/null +++ b/test_programs/execution_success/array_dynamic_nested_blackbox_input/Prover.toml @@ -0,0 +1,23 @@ +y = "3" +hash_result = [50, 53, 90, 252, 105, 236, 223, 30, 135, 229, 193, 172, 51, 139, 8, 32, 188, 104, 151, 115, 129, 168, 27, 71, 203, 47, 40, 228, 89, 177, 129, 100] + +[[x]] +a = "1" +b = ["2", "3", "20"] + +[x.bar] +inner = ["100", "101", "102"] + +[[x]] +a = "4" # idx = 3, flattened start idx = 7 +b = ["5", "6", "21"] # idx = 4, flattened start idx = 8 + +[x.bar] +inner = ["103", "104", "105"] # idx = 5, flattened start idx = 11 + +[[x]] +a = "7" +b = ["8", "9", "22"] + +[x.bar] +inner = ["106", "107", "108"] \ No newline at end of file diff --git a/test_programs/execution_success/array_dynamic_nested_blackbox_input/src/main.nr b/test_programs/execution_success/array_dynamic_nested_blackbox_input/src/main.nr new file mode 100644 index 00000000000..8faaf69dfc8 --- /dev/null +++ b/test_programs/execution_success/array_dynamic_nested_blackbox_input/src/main.nr @@ -0,0 +1,20 @@ +struct Bar { + inner: [u8; 3], +} + +struct Foo { + a: Field, + b: [Field; 3], + bar: Bar, +} + +fn main(mut x: [Foo; 3], y: pub Field, hash_result: pub [u8; 32]) { + // Simple dynamic array set for entire inner most array + x[y - 1].bar.inner = [106, 107, 10]; + let mut hash_input = x[y - 1].bar.inner; + // Make sure that we are passing a dynamic array to the black box function call + // by setting the array using a dynamic index here + hash_input[y - 1] = 0; + let hash = dep::std::hash::sha256(hash_input); + assert_eq(hash, hash_result); +}