chore: typos and some refactors, tests, etc in noirc_evaluator/src/acir

acvm-repo/acir/src/native_types/expression/mod.rs

@@ -138,6 +138,11 @@ impl<F> Expression<F> {
         self.mul_terms.sort_by(|a, b| a.1.cmp(&b.1).then(a.2.cmp(&b.2)));
         self.linear_combinations.sort_by(|a, b| a.1.cmp(&b.1));
     }
+
+    pub(crate) fn is_sorted(&self) -> bool {
+        self.mul_terms.iter().is_sorted_by(|a, b| a.1.cmp(&b.1).then(a.2.cmp(&b.2)).is_le())
+            && self.linear_combinations.iter().is_sorted_by(|a, b| a.1.cmp(&b.1).is_le())
+    }
 }
 
 impl<F: AcirField> Expression<F> {

acvm-repo/acir/src/native_types/expression/ordering.rs

@@ -8,6 +8,10 @@ use super::Expression;
 
 impl<F: Ord> Ord for Expression<F> {
     fn cmp(&self, other: &Self) -> Ordering {
+        // `get_max_term` has a comment: "This function assumes the gate is sorted"
+        assert!(self.is_sorted());
+        assert!(other.is_sorted());
+
         let mut i1 = self.get_max_idx();
         let mut i2 = other.get_max_idx();
         let mut result = Ordering::Equal;

compiler/noirc_evaluator/src/acir/acir_context/black_box.rs

@@ -33,7 +33,7 @@ impl<F: AcirField> AcirContext<F> {
 
                 assert_eq!(
                     output_count,
-                    input_size + (16 - input_size % 16),
+                    input_size + 16 - input_size % 16,
                     "output count mismatch"
                 );
 
@@ -105,7 +105,7 @@ impl<F: AcirField> AcirContext<F> {
         let mut inputs =
             self.prepare_inputs_for_black_box_func_call(inputs, allow_constant_inputs)?;
         if name == BlackBoxFunc::EmbeddedCurveAdd {
-            inputs = self.all_or_nothing_for_ec_add(inputs)?;
+            inputs = self.all_variables_or_constants_for_ec_add(inputs)?;
         }
         Ok(inputs)
     }
@@ -147,24 +147,29 @@ impl<F: AcirField> AcirContext<F> {
         Ok(witnesses)
     }
 
-    /// EcAdd has 6 inputs representing the two points to add
-    /// Each point must be either all constant, or all witnesses
-    fn all_or_nothing_for_ec_add(
+    /// [`BlackBoxFunc::EmbeddedCurveAdd`] has 6 inputs representing the two points to add
+    /// Each point must be either all constants, or all witnesses,
+    /// where constants are converted to witnesses here if mixed constant and witnesses are
+    /// encountered
+    fn all_variables_or_constants_for_ec_add(
         &mut self,
         inputs: Vec<Vec<FunctionInput<F>>>,
     ) -> Result<Vec<Vec<FunctionInput<F>>>, RuntimeError> {
         let mut has_constant = false;
         let mut has_witness = false;
         let mut result = inputs.clone();
         for (i, input) in inputs.iter().enumerate() {
+            assert_eq!(input.len(), 1);
             if input[0].is_constant() {
                 has_constant = true;
             } else {
                 has_witness = true;
             }
+
             if i % 3 == 2 {
                 if has_constant && has_witness {
-                    // Convert the constants to witness if mixed constant and witness,
+                    // Convert the constants to witnesses if mixed constants and witnesses are
+                    // encountered
                     for j in i - 2..i + 1 {
                         if let FunctionInput::Constant(constant) = inputs[j][0] {
                             let constant = self.add_constant(constant);

compiler/noirc_evaluator/src/acir/acir_context/brillig_call.rs

@@ -21,12 +21,13 @@ impl<F: AcirField> AcirContext<F> {
         outputs: Vec<AcirType>,
     ) -> Result<Vec<AcirValue>, RuntimeError> {
         let stdlib_func_bytecode = &self.brillig_stdlib.get_code(brillig_stdlib_func).clone();
+        let safe_return_values = false;
         self.brillig_call(
             predicate,
             stdlib_func_bytecode,
             inputs,
             outputs,
-            false,
+            safe_return_values,
             PLACEHOLDER_BRILLIG_INDEX,
             Some(brillig_stdlib_func),
         )

compiler/noirc_evaluator/src/acir/acir_context/generated_acir/brillig_directive.rs

@@ -9,6 +9,7 @@ use acvm::acir::{
 
 use crate::brillig::brillig_ir::artifact::GeneratedBrillig;
 
+// TODO(https://github.com/noir-lang/noir/issues/10256): Use `Option<BrilligFunctionId>` in place of `PLACEHOLDER_BRILLIG_INDEX`
 /// Brillig calls such as for the Brillig std lib are resolved only after code generation is finished.
 /// This index should be used when adding a Brillig call during code generation.
 /// Code generation should then keep track of that unresolved call opcode which will be resolved with the
@@ -49,11 +50,11 @@ pub enum BrilligStdlibFunc {
     ToLeBytes,
 }
 
-/// Generates brillig bytecode which computes the inverse of its input if not null, and zero else.
+/// Generates brillig bytecode which computes the inverse of its input if not null, else zero.
 pub(crate) fn directive_invert<F: AcirField>() -> GeneratedBrillig<F> {
-    //  We generate the following code:
+    // We generate the following code:
     // fn invert(x : Field) -> Field {
-    //    1/ x
+    //    1 / x
     // }
 
     // The input argument, ie the value that will be inverted.
@@ -151,28 +152,28 @@ pub(crate) fn directive_quotient<F: AcirField>() -> GeneratedBrillig<F> {
                 offset_address: MemoryAddress::direct(11),
             },
             // No cast, since calldata is typed as field by default
-            //q = a/b is set into register (2)
+            // q = a/b is set into register (2)
             BrilligOpcode::BinaryFieldOp {
                 op: BinaryFieldOp::IntegerDiv, // We want integer division, not field division!
                 lhs: MemoryAddress::direct(0),
                 rhs: MemoryAddress::direct(1),
                 destination: MemoryAddress::direct(2),
             },
-            //(1)= q*b
+            // (1)= q*b
             BrilligOpcode::BinaryFieldOp {
                 op: BinaryFieldOp::Mul,
                 lhs: MemoryAddress::direct(2),
                 rhs: MemoryAddress::direct(1),
                 destination: MemoryAddress::direct(1),
             },
-            //(1) = a-q*b
+            // (1) = a-q*b
             BrilligOpcode::BinaryFieldOp {
                 op: BinaryFieldOp::Sub,
                 lhs: MemoryAddress::direct(0),
                 rhs: MemoryAddress::direct(1),
                 destination: MemoryAddress::direct(1),
             },
-            //(0) = q
+            // (0) = q
             BrilligOpcode::Mov {
                 destination: MemoryAddress::direct(0),
                 source: MemoryAddress::direct(2),
@@ -262,14 +263,14 @@ pub(crate) fn directive_to_radix<F: AcirField>() -> GeneratedBrillig<F> {
             rhs: radix,
             destination: MemoryAddress::direct(3),
         },
-        //(4) = (3)*256
+        // (4) = (3)*256
         BrilligOpcode::BinaryFieldOp {
             op: BinaryFieldOp::Mul,
             lhs: MemoryAddress::direct(3),
             rhs: radix,
             destination: MemoryAddress::direct(4),
         },
-        //(4) = a-(3)*256 (remainder)
+        // (4) = a-(3)*256 (remainder)
         BrilligOpcode::BinaryFieldOp {
             op: BinaryFieldOp::Sub,
             lhs: MemoryAddress::direct(0),
@@ -289,7 +290,7 @@ pub(crate) fn directive_to_radix<F: AcirField>() -> GeneratedBrillig<F> {
             destination: result_pointer,
             bit_size: memory_adr_int_size,
         },
-        //a := quotient
+        // a := quotient
         BrilligOpcode::Mov {
             destination: MemoryAddress::direct(0),
             source: MemoryAddress::direct(3),

compiler/noirc_evaluator/src/acir/acir_context/generated_acir/mod.rs

@@ -44,7 +44,7 @@ pub struct GeneratedAcir<F: AcirField> {
     /// This field is private should only ever be accessed through its getter and setter.
     ///
     /// Equivalent to acvm::acir::circuit::Circuit's field of the same name.
-    pub current_witness_index: Option<u32>,
+    current_witness_index: Option<u32>,
 
     /// The opcodes of which the compiled ACIR will comprise.
     pub opcodes: Vec<AcirOpcode<F>>,
@@ -63,7 +63,6 @@ pub struct GeneratedAcir<F: AcirField> {
     pub brillig_locations: BTreeMap<BrilligFunctionId, BrilligOpcodeToLocationsMap>,
 
     /// Source code location of the current instruction being processed
-    /// None if we do not know the location
     pub(crate) call_stack_id: CallStackId,
 
     /// Correspondence between an opcode index and the error message associated with it.
@@ -336,7 +335,7 @@ impl<F: AcirField> GeneratedAcir<F> {
     pub(crate) fn radix_le_decompose(
         &mut self,
         input_expr: &Expression<F>,
-        radix: u32,
+        radix: u128,
         limb_count: u32,
         bit_size: u32,
     ) -> Result<Vec<Witness>, RuntimeError> {
@@ -387,7 +386,7 @@ impl<F: AcirField> GeneratedAcir<F> {
     pub(crate) fn brillig_to_radix(
         &mut self,
         expr: &Expression<F>,
-        radix: u32,
+        radix: u128,
         limb_count: u32,
     ) -> Vec<Witness> {
         // Create the witness for the result
@@ -404,7 +403,7 @@ impl<F: AcirField> GeneratedAcir<F> {
         let radix_expr = Expression {
             mul_terms: Vec::new(),
             linear_combinations: Vec::new(),
-            q_c: F::from(u128::from(radix)),
+            q_c: F::from(radix),
         };
         let inputs = vec![
             BrilligInputs::Single(expr.clone()),
@@ -589,7 +588,7 @@ impl<F: AcirField> GeneratedAcir<F> {
         brillig_function_index: BrilligFunctionId,
         stdlib_func: Option<BrilligStdlibFunc>,
     ) {
-        // Check whether we have a call to this Brillig function already exists.
+        // Check whether a call to this Brillig function already exists.
         // This helps us optimize the Brillig metadata to only be stored once per Brillig entry point.
         let inserted_func_before = self.brillig_locations.contains_key(&brillig_function_index);
 
@@ -720,15 +719,13 @@ fn black_box_expected_output_size(name: BlackBoxFunc) -> Option<usize> {
 
         BlackBoxFunc::Sha256Compression => Some(8),
 
-        // Can only apply a range constraint to one
-        // witness at a time.
         BlackBoxFunc::RANGE => Some(0),
 
         // Signature verification algorithms will return a boolean
         BlackBoxFunc::EcdsaSecp256k1 | BlackBoxFunc::EcdsaSecp256r1 => Some(1),
 
         // Output of operations over the embedded curve
-        // will be 2 field elements representing the point.
+        // will be 3 field elements representing the point, i.e. (x,y,infinite)
         BlackBoxFunc::MultiScalarMul | BlackBoxFunc::EmbeddedCurveAdd => Some(3),
 
         // Recursive aggregation has a variable number of outputs