fix(brillig_vm): Remove slice padding for foreign call inputs

acvm-repo/brillig/src/foreign_call.rs

@@ -1,7 +1,7 @@
 use acir_field::AcirField;
 use serde::{Deserialize, Serialize};
 
-/// Single output of a [foreign call][crate::Opcode::ForeignCall].
+/// Single input or output of a [foreign call][crate::Opcode::ForeignCall].
 #[derive(Debug, PartialEq, Eq, Serialize, Deserialize, Clone)]
 #[serde(untagged)]
 pub enum ForeignCallParam<F> {
@@ -22,6 +22,7 @@ impl<F> From<Vec<F>> for ForeignCallParam<F> {
 }
 
 impl<F: AcirField> ForeignCallParam<F> {
+    /// Convert the fields in the parameter into a vector, used to flatten data.
     pub fn fields(&self) -> Vec<F> {
         match self {
             ForeignCallParam::Single(value) => vec![*value],

acvm-repo/brillig/src/opcodes.rs

@@ -85,6 +85,9 @@ impl HeapValueType {
         HeapValueType::Simple(BitSize::Field)
     }
 
+    /// Returns the total number of field elements required to represent this type in memory.
+    ///
+    /// Returns `None` for `Vector`, as their size is not statically known.
     pub fn flattened_size(&self) -> Option<usize> {
         match self {
             HeapValueType::Simple(_) => Some(1),

acvm-repo/brillig_vm/src/lib.rs

@@ -525,11 +525,50 @@
                     // resolved inputs back to the caller. Once the caller pushes to `foreign_call_results`,
                     // they can then make another call to the VM that starts at this opcode
                     // but has the necessary results to proceed with execution.
+
+                    // With slices we might have more items in the HeapVector than the semantic length
+                    // indicated by the field preceding the pointer to the vector in the inputs.
+                    // This happens when SSA merges slices of different length, which can result in
+                    // a vector that has room for the longer of the two, partially filled with items
+                    // from the shorter. There are ways to deal with this on the receiver side,
+                    // but it is cumbersome, and the cleanest solution is not to send the extra empty
+                    // items at all. To do this, however, we need infer which input is the vector length.
+                    let mut vector_length: Option<usize> = None;
+
                     let resolved_inputs = inputs
                         .iter()
                         .zip(input_value_types)
-                        .map(|(input, input_type)| self.get_memory_values(*input, input_type))
+                        .map(|(input, input_type)| {
+                            let mut input = self.get_memory_values(*input, input_type);
+                            // Truncate slices to their semantic length, which we remember from the preceding field.
+                            match input_type {
+                                HeapValueType::Simple(BitSize::Integer(IntegerBitSize::U32)) => {
+                                    // If we have a single u32 we may have a slice representation, so store this input.
+                                    // On the next iteration, if we have a vector then we know we have the dynamic length
+                                    // for that slice.
+                                    let ForeignCallParam::Single(length) = input else {
+                                        unreachable!("expected u32; got {input:?}");
+                                    };
+                                    vector_length = Some(length.to_u128() as usize);
+                                }
+                                HeapValueType::Vector { value_types } => {
+                                    if let Some(length) = vector_length {
+                                        let type_size = vector_element_size(value_types);
+                                        let mut fields = input.fields();
+                                        fields.truncate(length * type_size);
+                                        input = ForeignCallParam::Array(fields);
+                                    }
+                                    vector_length = None;
+                                }
+                                _ => {
+                                    // Otherwise we are not dealing with a u32 followed by a vector.
+                                    vector_length = None;
+                                }
+                            }
+                            input
+                        })
                         .collect::<Vec<_>>();
+
                     return self.wait_for_foreign_call(function.clone(), resolved_inputs);
                 }
 
@@ -654,6 +693,7 @@
         self.status.clone()
     }
 
+    /// Get input data from memory to pass to foreign calls.
     fn get_memory_values(
         &self,
         input: ValueOrArray,
@@ -710,11 +750,15 @@
                 0 == size % value_types.len(),
                 "array/vector does not contain a whole number of elements"
             );
+
+            // We want to send vectors to foreign functions truncated to their semantic length.
+            let mut vector_length: Option<usize> = None;
+
             (0..size)
                 .zip(value_types.iter().cycle())
-                .flat_map(|(i, value_type)| {
+                .map(|(i, value_type)| {
                     let value_address = start.offset(i);
-                    match value_type {
+                    let values = match value_type {
                         HeapValueType::Simple(_) => {
                             vec![self.memory.read(value_address)]
                         }
@@ -739,7 +783,27 @@
                                 value_types,
                             )
                         }
+                    };
+                    (value_type, values)
+                })
+                .flat_map(|(value_type, mut values)| {
+                    match value_type {
+                        HeapValueType::Simple(BitSize::Integer(IntegerBitSize::U32)) => {
+                            vector_length = Some(values[0].to_usize());
+                        }
+                        HeapValueType::Vector { value_types } => {
+                            if let Some(length) = vector_length {
+                                let type_size = vector_element_size(value_types);
+                                values.truncate(length * type_size);
+                            }
+                            vector_length = None;
+                        }
+                        _ => {
+                            // Otherwise we are not dealing with a u32 followed by a vector.
+                            vector_length = None;
+                        }
                     }
+                    values
                 })
                 .collect::<Vec<_>>()
         }
@@ -1095,6 +1159,19 @@
     }
 }
 
+/// Returns the total number of field elements required to represent the elements in the vector in memory.
+///
+/// Panics if the vector contains nested vectors. Such types are not supported and are rejected by the frontend.
+fn vector_element_size(value_types: &[HeapValueType]) -> usize {
+    value_types
+        .iter()
+        .map(|typ| {
+            typ.flattened_size()
+                .unwrap_or_else(|| panic!("unexpected nested dynamic element type: {typ:?}"))
+        })
+        .sum()
+}
+
 #[cfg(test)]
 mod tests {
     use crate::memory::MEMORY_ADDRESSING_BIT_SIZE;
@@ -2789,7 +2866,7 @@
     }
 
     #[test]
     fn aborts_when_foreign_call_returns_data_which_doesnt_match_vector_elements() {
         let calldata: Vec<FieldElement> = vec![];
 
         let opcodes = &[

compiler/noirc_evaluator/src/ssa/interpreter/intrinsics.rs

@@ -688,8 +688,7 @@ impl<W: Write> Interpreter<'_, W> {
             // Everything up to the first meta is part of _some_ value.
             // We'll let each parser take as many fields as they need.
             let meta_idx = args.len() - 1 - num_values;
-            let input_as_fields =
-                (3..meta_idx).flat_map(|i| value_to_fields(&args[i])).collect::<Vec<_>>();
+            let input_as_fields = values_to_fields(&args[3..meta_idx]);
             let field_iterator = &mut input_as_fields.into_iter();
 
             let mut fragments = Vec::new();
@@ -701,8 +700,7 @@ impl<W: Write> Interpreter<'_, W> {
             PrintableValueDisplay::FmtString(message, fragments)
         } else {
             let meta_idx = args.len() - 2;
-            let input_as_fields =
-                (1..meta_idx).flat_map(|i| value_to_fields(&args[i])).collect::<Vec<_>>();
+            let input_as_fields = values_to_fields(&args[1..meta_idx]);
             let printable_type = value_to_printable_type(&args[meta_idx])?;
             let printable_value =
                 decode_printable_value(&mut input_as_fields.into_iter(), &printable_type);
@@ -783,37 +781,56 @@ fn new_embedded_curve_point(
     ))
 }
 
-/// Convert a [Value] to a vector of [FieldElement] for printing.
-fn value_to_fields(value: &Value) -> Vec<FieldElement> {
-    fn go(value: &Value, fields: &mut Vec<FieldElement>) {
-        match value {
-            Value::Numeric(numeric_value) => fields.push(numeric_value.convert_to_field()),
-            Value::Reference(reference_value) => {
-                if let Some(value) = reference_value.element.borrow().as_ref() {
-                    go(value, fields);
+/// Convert a slice of [Value] to a flattened vector of [FieldElement] for printing.
+///
+/// It takes a slice, rather than individual values, so that it can try to
+/// pair up `u32` fields indicating the size of a `Slice` with its elements
+/// following in the next value, and truncate the data to the semantic length.
+fn values_to_fields(values: &[Value]) -> Vec<FieldElement> {
+    fn go<'a, I>(values: I, fields: &mut Vec<FieldElement>)
+    where
+        I: Iterator<Item = &'a Value>,
+    {
+        let mut vector_length: Option<usize> = None;
+        for value in values {
+            match value {
+                Value::Numeric(numeric_value) => fields.push(numeric_value.convert_to_field()),
+                Value::Reference(reference_value) => {
+                    if let Some(value) = reference_value.element.borrow().as_ref() {
+                        go(std::iter::once(value), fields);
+                    }
                 }
-            }
-            Value::ArrayOrSlice(array_value) => {
-                for value in array_value.elements.borrow().iter() {
-                    go(value, fields);
+                Value::ArrayOrSlice(array_value) => {
+                    let length = match vector_length {
+                        Some(length) if array_value.is_slice => {
+                            length * array_value.element_types.len()
+                        }
+                        _ => array_value.elements.borrow().len(),
+                    };
+                    go(array_value.elements.borrow().iter().take(length), fields);
+                }
+                Value::Function(id) => {
+                    // Based on `decode_printable_value` it will expect consume the environment as well,
+                    // but that's catered for the by the SSA generation: the env is passed as separate values.
+                    fields.push(FieldElement::from(id.to_u32()));
+                }
+                Value::Intrinsic(x) => {
+                    panic!("didn't expect to print intrinsics: {x}")
+                }
+                Value::ForeignFunction(x) => {
+                    panic!("didn't expect to print foreign functions: {x}")
                 }
             }
-            Value::Function(id) => {
-                // Based on `decode_printable_value` it will expect consume the environment as well,
-                // but that's catered for the by the SSA generation: the env is passed as separate values.
-                fields.push(FieldElement::from(id.to_u32()));
-            }
-            Value::Intrinsic(x) => {
-                panic!("didn't expect to print intrinsics: {x}")
-            }
-            Value::ForeignFunction(x) => {
-                panic!("didn't expect to print foreign functions: {x}")
+            // Chamber the length for a potential vector following it.
+            if let Value::Numeric(NumericValue::U32(length)) = value {
+                vector_length = Some(*length as usize);
+            } else {
+                vector_length = None;
             }
         }
     }
-
     let mut fields = Vec::new();
-    go(value, &mut fields);
+    go(values.iter(), &mut fields);
     fields
 }
 

compiler/noirc_printable_type/src/lib.rs

@@ -292,7 +292,7 @@ pub fn format_field_string<F: AcirField>(field: F) -> String {
     "0x".to_owned() + &trimmed_field
 }
 
-/// Assumes that `field_iterator` contains enough field elements in order to decode the [PrintableType]
+/// Assumes that `field_iterator` contains enough field elements in order to decode the [PrintableType].
 pub fn decode_printable_value<F: AcirField>(
     field_iterator: &mut impl Iterator<Item = F>,
     typ: &PrintableType,
@@ -302,25 +302,27 @@ pub fn decode_printable_value<F: AcirField>(
         | PrintableType::SignedInteger { .. }
         | PrintableType::UnsignedInteger { .. }
         | PrintableType::Boolean => {
-            let field_element = field_iterator.next().unwrap();
+            let field_element = field_iterator.next().expect("not enough data: expected bool");
 
             PrintableValue::Field(field_element)
         }
         PrintableType::Array { length, typ } => {
             let length = *length as usize;
             let mut array_elements = Vec::with_capacity(length);
+
             for _ in 0..length {
                 array_elements.push(decode_printable_value(field_iterator, typ));
             }
 
             PrintableValue::Vec { array_elements, is_slice: false }
         }
         PrintableType::Slice { typ } => {
-            let length = field_iterator
-                .next()
-                .expect("not enough data to decode variable array length")
-                .to_u128() as usize;
+            let length =
+                field_iterator.next().expect("not enough data: expected slice length").to_u128()
+                    as usize;
+
             let mut array_elements = Vec::with_capacity(length);
+
             for _ in 0..length {
                 array_elements.push(decode_printable_value(field_iterator, typ));
             }
@@ -371,7 +373,7 @@ pub fn decode_printable_value<F: AcirField>(
         PrintableType::Function { env, .. } => {
             // we want to consume the fields from the environment, but for now they are not actually printed
             let _env = decode_printable_value(field_iterator, env);
-            let func_id = field_iterator.next().unwrap();
+            let func_id = field_iterator.next().expect("not enough data: expected function ID");
             PrintableValue::Field(func_id)
         }
         PrintableType::Reference { typ, .. } => {
@@ -380,7 +382,7 @@ pub fn decode_printable_value<F: AcirField>(
         }
         PrintableType::Unit => PrintableValue::Field(F::zero()),
         PrintableType::Enum { name: _, variants } => {
-            let tag = field_iterator.next().unwrap();
+            let tag = field_iterator.next().expect("not enough data: expected enum tag");
             let tag_value = tag.to_u128() as usize;
 
             let (_name, variant_types) = &variants[tag_value];
@@ -412,6 +414,16 @@ pub enum TryFromParamsError {
 }
 
 impl<F: AcirField> PrintableValueDisplay<F> {
+    /// Decode the print parameters after the first _newline_ flag has already been split.
+    ///
+    /// The last parameter is expected to be the flag indicating whether we are dealing
+    /// with a format string.
+    ///
+    /// We expect at least 3 arguments (tuples are passed as multiple values):
+    /// * normal: value.0, ..., value.i, meta, false
+    /// * formatted: msg, N, value1.0, ..., value1.i, ..., valueN.0, ..., valueN.j, meta1, ..., metaN, true
+    ///
+    /// The meta parts are JSON descriptors of the corresponding types, which guide the decoding.
     pub fn try_from_params(
         foreign_call_inputs: &[ForeignCallParam<F>],
     ) -> Result<PrintableValueDisplay<F>, TryFromParamsError> {
@@ -426,23 +438,42 @@ impl<F: AcirField> PrintableValueDisplay<F> {
     }
 }
 
+/// Flatten input parameters into a field vector.
+///
+/// Slices are expected to have exactly as many elements as indicated by their corresponding length,
+/// with any extra elements pruned by the caller already.
+fn flatten_inputs<F: AcirField>(input_values: &[ForeignCallParam<F>]) -> impl Iterator<Item = F> {
+    input_values.iter().flat_map(|param| param.fields())
+}
+
+/// Decode parameters for a normal call, without format string.
+///
+/// It will have a single meta descriptor:
+///
+/// value.0, ..., value.i, meta
 fn convert_string_inputs<F: AcirField>(
     foreign_call_inputs: &[ForeignCallParam<F>],
 ) -> Result<PrintableValueDisplay<F>, TryFromParamsError> {
     // Fetch the PrintableType from the foreign call input
     // The remaining input values should hold what is to be printed
     let (printable_type_as_values, input_values) =
         foreign_call_inputs.split_last().ok_or(TryFromParamsError::MissingForeignCallInputs)?;
+
     let printable_type = fetch_printable_type(printable_type_as_values)?;
 
     // We must use a flat map here as each value in a struct will be in a separate input value
-    let mut input_values_as_fields = input_values.iter().flat_map(|param| param.fields());
+    let mut input_values_as_fields = flatten_inputs(input_values);
 
     let value = decode_printable_value(&mut input_values_as_fields, &printable_type);
 
     Ok(PrintableValueDisplay::Plain(value, printable_type))
 }
 
+/// Decode parameters for a call with format string.
+///
+/// It will have the format message, followed by the number of arguments, and their values:
+///
+/// msg, N, value1.0, ..., value1.i, ..., valueN.0, ..., valueN.j, meta1, ..., metaN
 fn convert_fmt_string_inputs<F: AcirField>(
     foreign_call_inputs: &[ForeignCallParam<F>],
 ) -> Result<PrintableValueDisplay<F>, TryFromParamsError> {
@@ -461,8 +492,8 @@ fn convert_fmt_string_inputs<F: AcirField>(
 
     let types_start_at = input_and_printable_types.len() - num_values;
 
-    let mut input_iter =
-        input_and_printable_types[0..types_start_at].iter().flat_map(|param| param.fields());
+    let mut input_iter = flatten_inputs(&input_and_printable_types[0..types_start_at]);
+
     for printable_type in input_and_printable_types.iter().skip(types_start_at) {
         let printable_type = fetch_printable_type(printable_type)?;
         let value = decode_printable_value(&mut input_iter, &printable_type);
@@ -473,6 +504,7 @@ fn convert_fmt_string_inputs<F: AcirField>(
     Ok(PrintableValueDisplay::FmtString(message_as_string, output))
 }
 
+/// Decode the JSON type descriptor of the arguments passed to the print.
 fn fetch_printable_type<F: AcirField>(
     printable_type: &ForeignCallParam<F>,
 ) -> Result<PrintableType, TryFromParamsError> {