diff --git a/acvm-repo/acvm/src/compiler/mod.rs b/acvm-repo/acvm/src/compiler/mod.rs index c9c6ceb0552..85840735ae4 100644 --- a/acvm-repo/acvm/src/compiler/mod.rs +++ b/acvm-repo/acvm/src/compiler/mod.rs @@ -1,9 +1,9 @@ -//! The `compiler` module contains several pass to transform an ACIR program. -//! Roughly, the passes are spearated into the `optimizers` which try to reduce the number of opcodes +//! The `compiler` module contains several passes to transform an ACIR program. +//! Roughly, the passes are separated into the `optimizers` which try to reduce the number of opcodes //! and the `transformers` which adapt the opcodes to the proving backend. //! //! # Optimizers -//! - GeneralOptimizer: simple pass which simplify AssertZero opcodes when possible (e.g remove terms with null coeficient) +//! - GeneralOptimizer: simple pass which simplifies AssertZero opcodes when possible (e.g remove terms with null coefficient) //! - UnusedMemoryOptimizer: simple pass which removes MemoryInit opcodes when they are not used (e.g no corresponding MemoryOp opcode) //! - RangeOptimizer: forward pass to collect range check information, and backward pass to remove the ones that are redundant. //! @@ -25,11 +25,11 @@ use acir::{ }; // The various passes that we can use over ACIR +pub use optimizers::optimize; mod optimizers; mod simulator; mod transformers; -pub use optimizers::optimize; use optimizers::optimize_internal; pub use simulator::CircuitSimulator; use transformers::transform_internal; @@ -50,9 +50,9 @@ pub struct AcirTransformationMap { impl AcirTransformationMap { /// Builds a map from a vector of pointers to the old acir opcodes. /// The index in the vector is the new opcode index. - /// The value of the vector is the old opcode index pointed. + /// The value of the vector is where the old opcode index was pointed. /// E.g: If acir_opcode_positions = 0,1,2,4,5,5,6 - /// that means that old index 0,1,2,4,5,5,6 are mapped to the new indexes: 0,1,2,3,4,5,6 + /// that means that old indices 0,1,2,4,5,5,6 are mapped to the new indexes: 0,1,2,3,4,5,6 /// This gives the following map: /// 0 -> 0 /// 1 -> 1 @@ -71,8 +71,8 @@ impl AcirTransformationMap { /// Returns the new opcode location(s) corresponding to the old opcode. /// An OpcodeLocation contains the index of the opcode in the vector of opcodes /// This function returns the new OpcodeLocation by 'updating' the index within the given OpcodeLocation - /// using the AcirTransformationMap. In fact it does not update the given OpcodeLocation 'in-memory' but rather - /// returns a new one, and even a vector of OpcodeLocation in case there are multiple new indexes corresponding + /// using the AcirTransformationMap. In fact, it does not update the given OpcodeLocation 'in-memory' but rather + /// returns a new one, and even a vector of OpcodeLocation's in case there are multiple new indexes corresponding /// to the old opcode index. pub fn new_locations( &self, @@ -95,7 +95,7 @@ impl AcirTransformationMap { ) } - /// This function is similar to `new_locations()`, but only deal with + /// This function is similar to `new_locations()`, but only deals with /// the AcirOpcodeLocation variant pub fn new_acir_locations( &self, @@ -120,7 +120,7 @@ fn transform_assert_messages( .into_iter() .flat_map(|(location, message)| { let new_locations = map.new_locations(location); - new_locations.into_iter().map(move |new_location| (new_location, message.clone())) + new_locations.map(move |new_location| (new_location, message.clone())) }) .collect() } @@ -128,11 +128,11 @@ fn transform_assert_messages( /// Applies backend specific optimizations to a [`Circuit`]. /// /// optimize_internal: -/// - General optimizer: canonalize AssertZero opcodes. +/// - General optimizer: canonicalize AssertZero opcodes. /// - Unused Memory: remove unused MemoryInit opcodes. /// - Redundant Ranges: remove RANGE opcodes that are redundant. /// -/// transform_internal: run multiple times (up to 3) until the output stabilizes. +/// transform_internal: run multiple times (up to 4) until the output stabilizes. /// - CSAT: limit AssertZero opcodes to the Circuit's width. /// - Eliminate intermediate variables: Combine AssertZero opcodes used only once. /// - Redundant Ranges: some RANGEs may be redundant as a side effect of the previous pass. @@ -146,8 +146,14 @@ pub fn compile( let (acir, acir_opcode_positions) = optimize_internal(acir, acir_opcode_positions, brillig_side_effects); - let (mut acir, acir_opcode_positions) = - transform_internal(acir, expression_width, acir_opcode_positions, brillig_side_effects); + let max_transformer_passes_or_default = None; + let (mut acir, acir_opcode_positions, _opcodes_hash_stabilized) = transform_internal( + acir, + expression_width, + acir_opcode_positions, + brillig_side_effects, + max_transformer_passes_or_default, + ); let transformation_map = AcirTransformationMap::new(&acir_opcode_positions); acir.assert_messages = transform_assert_messages(acir.assert_messages, &transformation_map); diff --git a/acvm-repo/acvm/src/compiler/optimizers/general.rs b/acvm-repo/acvm/src/compiler/optimizers/general.rs index 7a16b848fc9..2ee42b0b359 100644 --- a/acvm-repo/acvm/src/compiler/optimizers/general.rs +++ b/acvm-repo/acvm/src/compiler/optimizers/general.rs @@ -13,7 +13,7 @@ pub(crate) struct GeneralOptimizer; impl GeneralOptimizer { pub(crate) fn optimize(opcode: Expression) -> Expression { - // XXX: Perhaps this optimization can be done on the fly + // TODO(https://github.com/noir-lang/noir/issues/10109): Perhaps this optimization can be done on the fly let opcode = simplify_mul_terms(opcode); simplify_linear_terms(opcode) } @@ -50,7 +50,7 @@ fn simplify_mul_terms(mut gate: Expression) -> Expression { fn simplify_linear_terms(mut gate: Expression) -> Expression { let mut hash_map: IndexMap = IndexMap::new(); - // Canonicalize the ordering of the terms, lets just order by variable name + // Canonicalize the ordering of the terms, let's just order by variable name for (scale, witness) in gate.linear_combinations.into_iter() { *hash_map.entry(witness).or_insert_with(F::zero) += scale; } @@ -207,4 +207,22 @@ mod tests { ASSERT 0 = 0 "); } + + #[test] + fn simplify_mul_terms_example() { + let src = " + private parameters: [w0, w1] + public parameters: [] + return values: [] + ASSERT 0*w1*w1 + 2*w2*w1 - w2*w1 - w1*w2 = 0 + "; + let circuit = Circuit::from_str(src).unwrap(); + let optimized_circuit = optimize(circuit); + assert_circuit_snapshot!(optimized_circuit, @r" + private parameters: [w0, w1] + public parameters: [] + return values: [] + ASSERT 0 = 0 + "); + } } diff --git a/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs b/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs index 27ee46448d0..d57e921ac12 100644 --- a/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs +++ b/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs @@ -26,11 +26,12 @@ impl MergeExpressionsOptimizer { deleted_gates: BTreeSet::new(), } } + /// This pass analyzes the circuit and identifies intermediate variables that are /// only used in two AssertZero opcodes. It then merges the opcode which produces the /// intermediate variable into the second one that uses it /// - /// The first pass maps witnesses to the index of the opcodes using them. + /// The first pass maps witnesses to the indices of the opcodes using them. /// Public inputs are not considered because they cannot be simplified. /// Witnesses used by MemoryInit opcodes are put in a separate map and marked as used by a Brillig call /// if the memory block is an input to the call. @@ -48,12 +49,11 @@ impl MergeExpressionsOptimizer { /// Then we will remove the first one and modify the second one like this: /// 2*w3*w4 + w4 - w2 - 1/2*w3 - 1/2*w2*w3 = 0 /// - /// This transformation is relevant for Plonk-ish backends although they have a limited width because - /// they can potentially handle expressions with large linear combinations using 'big-add' gates. - /// /// Pre-condition: - /// - This pass is only relevant for backends that can handle unlimited width - /// - CSAT pass should have been run prior to this one. + /// - This pass is relevant for backends that can handle unlimited width and + /// Plonk-ish backends. Although they have a limited width, they can potentially + /// handle expressions with large linear combinations using 'big-add' gates. + /// - The CSAT pass should have been run prior to this one. pub(crate) fn eliminate_intermediate_variable( &mut self, circuit: &Circuit, @@ -68,7 +68,7 @@ impl MergeExpressionsOptimizer { let circuit_io: BTreeSet = circuit.circuit_arguments().union(&circuit.public_inputs().0).cloned().collect(); - let mut used_witness: BTreeMap> = BTreeMap::new(); + let mut used_witnesses: BTreeMap> = BTreeMap::new(); for (i, opcode) in circuit.opcodes.iter().enumerate() { let witnesses = self.witness_inputs(opcode); if let Opcode::MemoryInit { block_id, .. } = opcode { @@ -77,40 +77,38 @@ impl MergeExpressionsOptimizer { for w in witnesses { // We do not simplify circuit inputs and outputs if !circuit_io.contains(&w) { - used_witness.entry(w).or_default().insert(i); + used_witnesses.entry(w).or_default().insert(i); } } } // For each opcode, try to get a target opcode to merge with - for (i, opcode) in circuit.opcodes.iter().enumerate() { + for (op1, opcode) in circuit.opcodes.iter().enumerate() { if !matches!(opcode, Opcode::AssertZero(_)) { continue; } - if let Some(opcode) = self.get_opcode(i, circuit) { + if let Some(opcode) = self.get_opcode(op1, circuit) { let input_witnesses = self.witness_inputs(&opcode); for w in input_witnesses { - let Some(gates_using_w) = used_witness.get(&w) else { + let Some(gates_using_w) = used_witnesses.get(&w) else { continue; }; // We only consider witness which are used in exactly two arithmetic gates if gates_using_w.len() == 2 { let first = *gates_using_w.first().expect("gates_using_w.len == 2"); let second = *gates_using_w.last().expect("gates_using_w.len == 2"); - let b = if second == i { + let op2 = if second == op1 { first } else { // sanity check - assert!(i == first); + assert!(op1 == first); second }; + // Merge the opcode with smaller index into the other one - // by updating modified_gates/deleted_gates/used_witness + // by updating modified_gates/deleted_gates/used_witnesses // returns false if it could not merge them - let mut merge_opcodes = |op1, op2| -> bool { - if op1 == op2 { - return false; - } + if op1 != op2 { let (source, target) = if op1 < op2 { (op1, op2) } else { (op2, op1) }; let source_opcode = self.get_opcode(source, circuit); let target_opcode = self.get_opcode(target, circuit); @@ -125,29 +123,24 @@ impl MergeExpressionsOptimizer { { self.modified_gates.insert(target, Opcode::AssertZero(expr)); self.deleted_gates.insert(source); - // Update the 'used_witness' map to account for the merge. - let witness_list = CircuitSimulator::expr_wit(&expr_use); + // Update the 'used_witnesses' map to account for the merge. + let witness_list = CircuitSimulator::expr_witness(&expr_use); let witness_list = witness_list - .chain(CircuitSimulator::expr_wit(&expr_define)); + .chain(CircuitSimulator::expr_witness(&expr_define)); for w2 in witness_list { if !circuit_io.contains(&w2) { - used_witness.entry(w2).and_modify(|v| { + used_witnesses.entry(w2).and_modify(|v| { v.insert(target); v.remove(&source); }); } } - return true; + // We need to stop here and continue with the next opcode + // because the merge invalidates the current opcode. + break; } } - false - }; - - if merge_opcodes(b, i) { - // We need to stop here and continue with the next opcode - // because the merge invalidates the current opcode. - break; } } } @@ -159,24 +152,24 @@ impl MergeExpressionsOptimizer { let mut new_acir_opcode_positions = Vec::new(); for (i, opcode_position) in acir_opcode_positions.iter().enumerate() { - if let Some(op) = self.get_opcode(i, circuit) { - new_circuit.push(op); + if let Some(opcode) = self.get_opcode(i, circuit) { + new_circuit.push(opcode); new_acir_opcode_positions.push(*opcode_position); } } (new_circuit, new_acir_opcode_positions) } - fn for_each_brillig_input_wit(&self, input: &BrilligInputs, mut f: impl FnMut(Witness)) { + fn for_each_brillig_input_witness(&self, input: &BrilligInputs, mut f: impl FnMut(Witness)) { match input { BrilligInputs::Single(expr) => { - for witness in CircuitSimulator::expr_wit(expr) { + for witness in CircuitSimulator::expr_witness(expr) { f(witness); } } BrilligInputs::Array(exprs) => { for expr in exprs { - for witness in CircuitSimulator::expr_wit(expr) { + for witness in CircuitSimulator::expr_witness(expr) { f(witness); } } @@ -189,7 +182,7 @@ impl MergeExpressionsOptimizer { } } - fn for_each_brillig_output_wit(&self, output: &BrilligOutputs, mut f: impl FnMut(Witness)) { + fn for_each_brillig_output_witness(&self, output: &BrilligOutputs, mut f: impl FnMut(Witness)) { match output { BrilligOutputs::Simple(witness) => f(*witness), BrilligOutputs::Array(witnesses) => { @@ -203,7 +196,7 @@ impl MergeExpressionsOptimizer { // Returns the input witnesses used by the opcode fn witness_inputs(&self, opcode: &Opcode) -> BTreeSet { match opcode { - Opcode::AssertZero(expr) => CircuitSimulator::expr_wit(expr).collect(), + Opcode::AssertZero(expr) => CircuitSimulator::expr_witness(expr).collect(), Opcode::BlackBoxFuncCall(bb_func) => { let mut witnesses = bb_func.get_input_witnesses(); witnesses.extend(bb_func.get_outputs_vec()); @@ -212,8 +205,8 @@ impl MergeExpressionsOptimizer { } Opcode::MemoryOp { block_id: _, op } => { //index and value - let witnesses = CircuitSimulator::expr_wit(&op.index); - witnesses.chain(CircuitSimulator::expr_wit(&op.value)).collect() + let witnesses = CircuitSimulator::expr_witness(&op.index); + witnesses.chain(CircuitSimulator::expr_witness(&op.value)).collect() } Opcode::MemoryInit { block_id: _, init, block_type: _ } => { @@ -222,12 +215,12 @@ impl MergeExpressionsOptimizer { Opcode::BrilligCall { inputs, outputs, .. } => { let mut witnesses = BTreeSet::new(); for i in inputs { - self.for_each_brillig_input_wit(i, |witness| { + self.for_each_brillig_input_witness(i, |witness| { witnesses.insert(witness); }); } for i in outputs { - self.for_each_brillig_output_wit(i, |witness| { + self.for_each_brillig_output_witness(i, |witness| { witnesses.insert(witness); }); } @@ -238,7 +231,7 @@ impl MergeExpressionsOptimizer { witnesses.extend(outputs); if let Some(p) = predicate { - witnesses.extend(CircuitSimulator::expr_wit(p)); + witnesses.extend(CircuitSimulator::expr_witness(p)); } witnesses } @@ -250,24 +243,28 @@ impl MergeExpressionsOptimizer { fn merge_expression( target: &Expression, expr: &Expression, - w: Witness, + witness: Witness, ) -> Option> { // Check that the witness is not part of multiplication terms for m in &target.mul_terms { - if m.1 == w || m.2 == w { + if m.1 == witness || m.2 == witness { return None; } } for m in &expr.mul_terms { - if m.1 == w || m.2 == w { + if m.1 == witness || m.2 == witness { return None; } } for k in &target.linear_combinations { - if k.1 == w { + if k.1 == witness { for i in &expr.linear_combinations { - if i.1 == w { + if i.1 == witness { + assert!( + i.0 != F::zero(), + "merge_expression: attempting to divide k.0 by F::zero" + ); let expr = target.add_mul(-(k.0 / i.0), expr); let expr = GeneralOptimizer::optimize(expr); return Some(expr); @@ -278,15 +275,15 @@ impl MergeExpressionsOptimizer { None } - /// Returns the 'updated' opcode at index 'g' in the circuit + /// Returns the 'updated' opcode at the given index in the circuit /// The modifications to the circuits are stored with 'deleted_gates' and 'modified_gates' /// These structures are used to give the 'updated' opcode. /// For instance, if the opcode has been deleted inside 'deleted_gates', then it returns None. - fn get_opcode(&self, g: usize, circuit: &Circuit) -> Option> { - if self.deleted_gates.contains(&g) { + fn get_opcode(&self, index: usize, circuit: &Circuit) -> Option> { + if self.deleted_gates.contains(&index) { return None; } - self.modified_gates.get(&g).or(circuit.opcodes.get(g)).cloned() + self.modified_gates.get(&index).or(circuit.opcodes.get(index)).cloned() } } @@ -296,10 +293,14 @@ mod tests { assert_circuit_snapshot, compiler::{CircuitSimulator, optimizers::MergeExpressionsOptimizer}, }; - use acir::{FieldElement, circuit::Circuit}; + use acir::{ + AcirField, FieldElement, + circuit::Circuit, + native_types::{Expression, Witness}, + }; fn merge_expressions(circuit: Circuit) -> Circuit { - assert!(CircuitSimulator::default().check_circuit(&circuit).is_none()); + assert!(CircuitSimulator::check_circuit(&circuit).is_none()); let mut merge_optimizer = MergeExpressionsOptimizer::new(); let acir_opcode_positions = vec![0; 20]; let (opcodes, _) = @@ -308,7 +309,7 @@ mod tests { optimized_circuit.opcodes = opcodes; // check that the circuit is still valid after optimization - assert!(CircuitSimulator::default().check_circuit(&optimized_circuit).is_none()); + assert!(CircuitSimulator::check_circuit(&optimized_circuit).is_none()); optimized_circuit } @@ -404,4 +405,23 @@ mod tests { let optimized_circuit = merge_expressions(circuit.clone()); assert_eq!(circuit, optimized_circuit); } + + #[test] + #[should_panic(expected = "merge_expression: attempting to divide k.0 by F::zero")] + fn merge_expression_on_zero_linear_combination_panics() { + let opcode_a = Expression { + mul_terms: vec![], + linear_combinations: vec![(FieldElement::one(), Witness(0))], + q_c: FieldElement::zero(), + }; + let opcode_b = Expression { + mul_terms: vec![], + linear_combinations: vec![(FieldElement::zero(), Witness(0))], + q_c: FieldElement::zero(), + }; + assert_eq!( + MergeExpressionsOptimizer::merge_expression(&opcode_a, &opcode_b, Witness(0),), + Some(opcode_a) + ); + } } diff --git a/acvm-repo/acvm/src/compiler/optimizers/mod.rs b/acvm-repo/acvm/src/compiler/optimizers/mod.rs index 0aea1072694..e5a569e20c1 100644 --- a/acvm-repo/acvm/src/compiler/optimizers/mod.rs +++ b/acvm-repo/acvm/src/compiler/optimizers/mod.rs @@ -83,17 +83,11 @@ pub(super) fn optimize_internal( let (acir, acir_opcode_positions) = memory_optimizer.remove_unused_memory_initializations(acir_opcode_positions); - // let (acir, acir_opcode_positions) = - // ConstantBackpropagationOptimizer::backpropagate_constants(acir, acir_opcode_positions); - // Range optimization pass let range_optimizer = RangeOptimizer::new(acir, brillig_side_effects); let (acir, acir_opcode_positions) = range_optimizer.replace_redundant_ranges(acir_opcode_positions); - // let (acir, acir_opcode_positions) = - // ConstantBackpropagationOptimizer::backpropagate_constants(acir, acir_opcode_positions); - info!("Number of opcodes after: {}", acir.opcodes.len()); (acir, acir_opcode_positions) diff --git a/acvm-repo/acvm/src/compiler/optimizers/redundant_range.rs b/acvm-repo/acvm/src/compiler/optimizers/redundant_range.rs index 51cfa0866e2..af12c3e2d89 100644 --- a/acvm-repo/acvm/src/compiler/optimizers/redundant_range.rs +++ b/acvm-repo/acvm/src/compiler/optimizers/redundant_range.rs @@ -1,5 +1,5 @@ //! The redundant range constraint optimization pass aims to remove any [BlackBoxFunc::Range] opcodes -//! which doesn't result in additional restrictions on the value of witnesses. +//! which doesn't result in additional restrictions on the values of witnesses. //! //! Suppose we had the following pseudo-code: //! @@ -113,6 +113,10 @@ impl<'a, F: AcirField> RangeOptimizer<'a, F> { if expr.is_degree_one_univariate() { let (k, witness) = expr.linear_combinations[0]; let constant = expr.q_c; + assert!( + k != F::zero(), + "collect_ranges: attempting to divide -constant by F::zero()" + ); let witness_value = -constant / k; if witness_value.is_zero() { @@ -157,8 +161,8 @@ impl<'a, F: AcirField> RangeOptimizer<'a, F> { continue; }; - // Check if the witness has already been recorded and if the witness - // size is more than the current one, we replace it + // Check if the witness has already been recorded and if the witness' + // recorded size is more than the current one, we replace it infos .entry(witness) .and_modify(|info| { @@ -195,7 +199,7 @@ impl<'a, F: AcirField> RangeOptimizer<'a, F> { ) -> (Circuit, Vec) { let mut new_order_list = Vec::with_capacity(order_list.len()); let mut optimized_opcodes = Vec::with_capacity(self.circuit.opcodes.len()); - // Consider the index beyond the last as a pseudo size effect by which time all constraints need to be inserted. + // Consider the index beyond the last as a pseudo side effect by which time all constraints need to be inserted. let mut next_side_effect = self.circuit.opcodes.len(); // Going in reverse so we can propagate the side effect information backwards. for (idx, opcode) in self.circuit.opcodes.into_iter().enumerate().rev() { @@ -261,12 +265,19 @@ mod tests { use std::collections::BTreeMap; use crate::{ - assert_circuit_snapshot, - compiler::optimizers::redundant_range::{RangeOptimizer, memory_block_implied_max_bits}, + FieldElement, assert_circuit_snapshot, + compiler::{ + CircuitSimulator, + optimizers::{ + Opcode, + redundant_range::{RangeOptimizer, memory_block_implied_max_bits}, + }, + }, }; use acir::{ + AcirField, circuit::{Circuit, brillig::BrilligFunctionId}, - native_types::Witness, + native_types::{Expression, Witness}, }; #[test] @@ -285,13 +296,14 @@ mod tests { fn retain_lowest_range_size() { // The optimizer should keep the lowest bit size range constraint let src = " - private parameters: [] + private parameters: [w1] public parameters: [] return values: [] BLACKBOX::RANGE input: w1, bits: 32 BLACKBOX::RANGE input: w1, bits: 16 "; let circuit = Circuit::from_str(src).unwrap(); + assert!(CircuitSimulator::check_circuit(&circuit).is_none()); let acir_opcode_positions = circuit.opcodes.iter().enumerate().map(|(i, _)| i).collect(); let brillig_side_effects = BTreeMap::new(); @@ -307,8 +319,9 @@ mod tests { ); let (optimized_circuit, _) = optimizer.replace_redundant_ranges(acir_opcode_positions); + assert!(CircuitSimulator::check_circuit(&optimized_circuit).is_none()); assert_circuit_snapshot!(optimized_circuit, @r" - private parameters: [] + private parameters: [w1] public parameters: [] return values: [] BLACKBOX::RANGE input: w1, bits: 16 @@ -319,7 +332,7 @@ mod tests { fn remove_duplicates() { // The optimizer should remove all duplicate range opcodes. let src = " - private parameters: [] + private parameters: [w1, w2] public parameters: [] return values: [] BLACKBOX::RANGE input: w1, bits: 16 @@ -328,13 +341,15 @@ mod tests { BLACKBOX::RANGE input: w2, bits: 23 "; let circuit = Circuit::from_str(src).unwrap(); + assert!(CircuitSimulator::check_circuit(&circuit).is_none()); let acir_opcode_positions = circuit.opcodes.iter().enumerate().map(|(i, _)| i).collect(); let brillig_side_effects = BTreeMap::new(); let optimizer = RangeOptimizer::new(circuit, &brillig_side_effects); let (optimized_circuit, _) = optimizer.replace_redundant_ranges(acir_opcode_positions); + assert!(CircuitSimulator::check_circuit(&optimized_circuit).is_none()); assert_circuit_snapshot!(optimized_circuit, @r" - private parameters: [] + private parameters: [w1, w2] public parameters: [] return values: [] BLACKBOX::RANGE input: w1, bits: 16 @@ -347,7 +362,7 @@ mod tests { // The optimizer should not remove or change non-range opcodes // The four AssertZero opcodes should remain unchanged. let src = " - private parameters: [] + private parameters: [w1] public parameters: [] return values: [] BLACKBOX::RANGE input: w1, bits: 16 @@ -358,13 +373,15 @@ mod tests { ASSERT 0 = 0 "; let circuit = Circuit::from_str(src).unwrap(); + assert!(CircuitSimulator::check_circuit(&circuit).is_none()); let acir_opcode_positions = circuit.opcodes.iter().enumerate().map(|(i, _)| i).collect(); let brillig_side_effects = BTreeMap::new(); let optimizer = RangeOptimizer::new(circuit, &brillig_side_effects); let (optimized_circuit, _) = optimizer.replace_redundant_ranges(acir_opcode_positions); + assert!(CircuitSimulator::check_circuit(&optimized_circuit).is_none()); assert_circuit_snapshot!(optimized_circuit, @r" - private parameters: [] + private parameters: [w1] public parameters: [] return values: [] BLACKBOX::RANGE input: w1, bits: 16 @@ -377,33 +394,66 @@ mod tests { #[test] fn constant_implied_ranges() { - // The optimizer should use knowledge about constant witness assignments to remove range opcodes. + // The optimizer should use knowledge about constant witness assignments to remove range opcodes, when possible. + // In this case, the `BLACKBOX::RANGE` opcode is expected to be removed because its range is larger than + // the range checked by the `ASSERT` opcode let src = " - private parameters: [] + private parameters: [w1] public parameters: [] return values: [] BLACKBOX::RANGE input: w1, bits: 16 ASSERT w1 = 0 "; let circuit = Circuit::from_str(src).unwrap(); + assert!(CircuitSimulator::check_circuit(&circuit).is_none()); let acir_opcode_positions = circuit.opcodes.iter().enumerate().map(|(i, _)| i).collect(); let brillig_side_effects = BTreeMap::new(); let optimizer = RangeOptimizer::new(circuit, &brillig_side_effects); let (optimized_circuit, _) = optimizer.replace_redundant_ranges(acir_opcode_positions); + assert!(CircuitSimulator::check_circuit(&optimized_circuit).is_none()); assert_circuit_snapshot!(optimized_circuit, @r" - private parameters: [] + private parameters: [w1] public parameters: [] return values: [] ASSERT w1 = 0 "); } + #[test] + fn large_constant_implied_ranges() { + // The optimizer should use knowledge about constant witness assignments to remove range opcodes, when possible. + // In this case, the `BLACKBOX::RANGE` opcode is expected to be retained because its range is smaller than + // the range checked by the `ASSERT` opcode + let src = " + private parameters: [w1] + public parameters: [] + return values: [] + BLACKBOX::RANGE input: w1, bits: 8 + ASSERT w1 = 256 + "; + let circuit = Circuit::from_str(src).unwrap(); + assert!(CircuitSimulator::check_circuit(&circuit).is_none()); + + let acir_opcode_positions = circuit.opcodes.iter().enumerate().map(|(i, _)| i).collect(); + let brillig_side_effects = BTreeMap::new(); + let optimizer = RangeOptimizer::new(circuit, &brillig_side_effects); + let (optimized_circuit, _) = optimizer.replace_redundant_ranges(acir_opcode_positions); + assert!(CircuitSimulator::check_circuit(&optimized_circuit).is_none()); + assert_circuit_snapshot!(optimized_circuit, @r" + private parameters: [w1] + public parameters: [] + return values: [] + BLACKBOX::RANGE input: w1, bits: 8 + ASSERT w1 = 256 + "); + } + #[test] fn potential_side_effects() { // The optimizer should not remove range constraints if doing so might allow invalid side effects to go through. let src = " - private parameters: [] + private parameters: [w1, w2] public parameters: [] return values: [] BLACKBOX::RANGE input: w1, bits: 32 @@ -422,6 +472,7 @@ mod tests { ASSERT w1 = 0 "; let circuit = Circuit::from_str(src).unwrap(); + assert!(CircuitSimulator::check_circuit(&circuit).is_none()); let acir_opcode_positions: Vec = circuit.opcodes.iter().enumerate().map(|(i, _)| i).collect(); @@ -432,10 +483,11 @@ mod tests { let optimizer = RangeOptimizer::new(circuit, &brillig_side_effects); let (optimized_circuit, _) = optimizer.replace_redundant_ranges(acir_opcode_positions.clone()); + assert!(CircuitSimulator::check_circuit(&optimized_circuit).is_none()); // `BLACKBOX::RANGE [w1]:32 bits []` remains: The minimum does not propagate backwards. assert_circuit_snapshot!(optimized_circuit, @r" - private parameters: [] + private parameters: [w1, w2] public parameters: [] return values: [] BLACKBOX::RANGE input: w1, bits: 32 @@ -454,9 +506,10 @@ mod tests { #[test] fn array_implied_ranges() { - // The optimizer should use knowledge about array lengths and witnesses used to index these to remove range opcodes. + // The optimizer should use knowledge about array lengths and witnesses used to index these to remove range opcodes, when possible. + // The `BLACKBOX::RANGE` call is removed because its range is larger than the array's length let src = " - private parameters: [] + private parameters: [w0, w1] public parameters: [] return values: [] BLACKBOX::RANGE input: w1, bits: 16 @@ -464,17 +517,68 @@ mod tests { READ w2 = b0[w1] "; let circuit = Circuit::from_str(src).unwrap(); + assert!(CircuitSimulator::check_circuit(&circuit).is_none()); let acir_opcode_positions = circuit.opcodes.iter().enumerate().map(|(i, _)| i).collect(); let brillig_side_effects = BTreeMap::new(); let optimizer = RangeOptimizer::new(circuit, &brillig_side_effects); let (optimized_circuit, _) = optimizer.replace_redundant_ranges(acir_opcode_positions); + assert!(CircuitSimulator::check_circuit(&optimized_circuit).is_none()); assert_circuit_snapshot!(optimized_circuit, @r" - private parameters: [] + private parameters: [w0, w1] public parameters: [] return values: [] INIT b0 = [w0, w0, w0, w0, w0, w0, w0, w0] READ w2 = b0[w1] "); } + + #[test] + fn large_array_implied_ranges() { + // The optimizer should use knowledge about array lengths and witnesses used to index these to remove range opcodes, when possible. + // The `BLACKBOX::RANGE` call is not removed because its range is smaller than the array's length + let src = " + private parameters: [w0, w1] + public parameters: [] + return values: [] + BLACKBOX::RANGE input: w1, bits: 2 + INIT b0 = [w0, w0, w0, w0, w0, w0, w0, w0] + READ w2 = b0[w1] + "; + let circuit = Circuit::from_str(src).unwrap(); + assert!(CircuitSimulator::check_circuit(&circuit).is_none()); + + let acir_opcode_positions = circuit.opcodes.iter().enumerate().map(|(i, _)| i).collect(); + let brillig_side_effects = BTreeMap::new(); + let optimizer = RangeOptimizer::new(circuit, &brillig_side_effects); + let (optimized_circuit, _) = optimizer.replace_redundant_ranges(acir_opcode_positions); + assert!(CircuitSimulator::check_circuit(&optimized_circuit).is_none()); + assert_circuit_snapshot!(optimized_circuit, @r" + private parameters: [w0, w1] + public parameters: [] + return values: [] + BLACKBOX::RANGE input: w1, bits: 2 + INIT b0 = [w0, w0, w0, w0, w0, w0, w0, w0] + READ w2 = b0[w1] + "); + } + + #[test] + #[should_panic(expected = "collect_ranges: attempting to divide -constant by F::zero()")] + fn collect_ranges_zero_linear_combination_panics() { + let src = " + private parameters: [w1] + public parameters: [] + return values: [] + "; + let mut circuit = Circuit::from_str(src).unwrap(); + let expr = Expression { + mul_terms: vec![], + linear_combinations: vec![(FieldElement::zero(), Witness(0))], + q_c: FieldElement::one(), + }; + let opcode = Opcode::AssertZero(expr); + circuit.opcodes.push(opcode); + RangeOptimizer::collect_ranges(&circuit); + } } diff --git a/acvm-repo/acvm/src/compiler/optimizers/unused_memory.rs b/acvm-repo/acvm/src/compiler/optimizers/unused_memory.rs index dcc3d423a9d..8ee1a7b013b 100644 --- a/acvm-repo/acvm/src/compiler/optimizers/unused_memory.rs +++ b/acvm-repo/acvm/src/compiler/optimizers/unused_memory.rs @@ -76,7 +76,7 @@ impl UnusedMemoryOptimizer { #[cfg(test)] mod tests { - use crate::assert_circuit_snapshot; + use crate::{assert_circuit_snapshot, compiler::CircuitSimulator}; use super::*; @@ -90,9 +90,11 @@ mod tests { ASSERT w0 - w1 - w2 = 0 "; let circuit = Circuit::from_str(src).unwrap(); + assert!(CircuitSimulator::check_circuit(&circuit).is_none()); let unused_memory = UnusedMemoryOptimizer::new(circuit); assert_eq!(unused_memory.unused_memory_initializations.len(), 1); let (circuit, _) = unused_memory.remove_unused_memory_initializations(vec![0, 1]); + assert!(CircuitSimulator::check_circuit(&circuit).is_none()); assert_circuit_snapshot!(circuit, @r" private parameters: [w0, w1] public parameters: [] @@ -111,9 +113,11 @@ mod tests { ASSERT w2 = w0 - w1 "; let circuit = Circuit::from_str(src).unwrap(); + assert!(CircuitSimulator::check_circuit(&circuit).is_none()); let unused_memory = UnusedMemoryOptimizer::new(circuit.clone()); assert_eq!(unused_memory.unused_memory_initializations.len(), 1); let (optimized_circuit, _) = unused_memory.remove_unused_memory_initializations(vec![0, 1]); + assert!(CircuitSimulator::check_circuit(&optimized_circuit).is_none()); assert_eq!(optimized_circuit, circuit); } } diff --git a/acvm-repo/acvm/src/compiler/simulator.rs b/acvm-repo/acvm/src/compiler/simulator.rs index 092f776f6d7..e1d71ed5f6f 100644 --- a/acvm-repo/acvm/src/compiler/simulator.rs +++ b/acvm-repo/acvm/src/compiler/simulator.rs @@ -9,29 +9,33 @@ use acir::{ }; use std::collections::HashSet; -/// Simulate a symbolic solve for a circuit +/// Simulate solving a circuit symbolically /// Instead of evaluating witness values from the inputs, like the PWG module is doing, -/// this pass simply mark the witness that can be evaluated, from the known inputs, +/// this pass simply marks the witness that can be evaluated, from the known inputs, /// and incrementally from the previously marked witnesses. -/// This avoid any computation on a big field which makes the process efficient. +/// This avoids any computation on a big field which makes the process efficient. /// When all the witness of an opcode are marked as solvable, it means that the /// opcode is solvable. #[derive(Default)] pub struct CircuitSimulator { /// Track the witnesses that can be solved - solvable_witness: HashSet, + solvable_witnesses: HashSet, /// Track whether a [`BlockId`] has been initialized initialized_blocks: HashSet, } impl CircuitSimulator { - /// Simulate a symbolic solve for a circuit by keeping track of the witnesses that can be solved. - /// Returns the index of the opcode that cannot be solved, if any. + pub fn check_circuit(circuit: &Circuit) -> Option { + Self::default().run_check_circuit(circuit) + } + + /// Simulate solving a circuit symbolically by keeping track of the witnesses that can be solved. + /// Returns the index of an opcode that cannot be solved, if any. #[tracing::instrument(level = "trace", skip_all)] - pub fn check_circuit(&mut self, circuit: &Circuit) -> Option { + fn run_check_circuit(&mut self, circuit: &Circuit) -> Option { let circuit_inputs = circuit.circuit_arguments(); - self.solvable_witness.extend(circuit_inputs.iter()); + self.solvable_witnesses.extend(circuit_inputs.iter()); for (i, op) in circuit.opcodes.iter().enumerate() { if !self.try_solve(op) { return Some(i); @@ -42,22 +46,22 @@ impl CircuitSimulator { /// Check if the Opcode can be solved, and if yes, add the solved witness to set of solvable witness fn try_solve(&mut self, opcode: &Opcode) -> bool { - let mut unresolved = HashSet::new(); match opcode { Opcode::AssertZero(expr) => { + let mut unresolved = HashSet::new(); for (_, w1, w2) in &expr.mul_terms { - if !self.solvable_witness.contains(w1) { - if !self.solvable_witness.contains(w2) { + if !self.solvable_witnesses.contains(w1) { + if !self.solvable_witnesses.contains(w2) { return false; } unresolved.insert(*w1); } - if !self.solvable_witness.contains(w2) && w1 != w2 { + if !self.solvable_witnesses.contains(w2) && w1 != w2 { unresolved.insert(*w2); } } for (_, w) in &expr.linear_combinations { - if !self.solvable_witness.contains(w) { + if !self.solvable_witnesses.contains(w) { unresolved.insert(*w); } } @@ -100,7 +104,7 @@ impl CircuitSimulator { } Opcode::MemoryInit { block_id, init, .. } => { for w in init { - if !self.solvable_witness.contains(w) { + if !self.solvable_witnesses.contains(w) { return false; } } @@ -131,7 +135,7 @@ impl CircuitSimulator { } Opcode::Call { id: _, inputs, outputs, predicate } => { for w in inputs { - if !self.solvable_witness.contains(w) { + if !self.solvable_witnesses.contains(w) { return false; } } @@ -150,23 +154,25 @@ impl CircuitSimulator { /// Adds the witness to set of solvable witness pub(crate) fn mark_solvable(&mut self, witness: Witness) { - self.solvable_witness.insert(witness); + self.solvable_witnesses.insert(witness); } pub fn can_solve_function_input(&self, input: &FunctionInput) -> bool { if let FunctionInput::Witness(w) = input { - return self.solvable_witness.contains(w); + return self.solvable_witnesses.contains(w); } true } + fn can_solve_expression(&self, expr: &Expression) -> bool { - for w in Self::expr_wit(expr) { - if !self.solvable_witness.contains(&w) { + for w in Self::expr_witness(expr) { + if !self.solvable_witnesses.contains(&w) { return false; } } true } + fn can_solve_brillig_input(&mut self, input: &BrilligInputs) -> bool { match input { BrilligInputs::Single(expr) => self.can_solve_expression(expr), @@ -183,7 +189,7 @@ impl CircuitSimulator { } } - pub(crate) fn expr_wit(expr: &Expression) -> impl Iterator { + pub(crate) fn expr_witness(expr: &Expression) -> impl Iterator { expr.mul_terms .iter() .flat_map(|i| [i.1, i.2]) @@ -204,7 +210,7 @@ mod tests { return values: [] "; let empty_circuit = Circuit::from_str(src).unwrap(); - assert!(CircuitSimulator::default().check_circuit(&empty_circuit).is_none()); + assert!(CircuitSimulator::check_circuit(&empty_circuit).is_none()); } #[test] @@ -216,7 +222,35 @@ mod tests { ASSERT w2 = w1 "; let connected_circuit = Circuit::from_str(src).unwrap(); - assert!(CircuitSimulator::default().check_circuit(&connected_circuit).is_none()); + assert!(CircuitSimulator::check_circuit(&connected_circuit).is_none()); + } + + #[test] + fn reports_true_for_connected_circuit_with_range() { + let src = " + private parameters: [w1, w3] + public parameters: [] + return values: [] + ASSERT w2 = w1 + BLACKBOX::RANGE input: w3, bits: 8 + "; + let connected_circuit = Circuit::from_str(src).unwrap(); + + assert!(CircuitSimulator::check_circuit(&connected_circuit).is_none()); + } + + #[test] + fn reports_false_for_disconnected_circuit() { + let src = " + private parameters: [w1] + public parameters: [] + return values: [] + ASSERT w2 = w1 + ASSERT w4 = w3 + "; + let disconnected_circuit = Circuit::from_str(src).unwrap(); + + assert!(CircuitSimulator::check_circuit(&disconnected_circuit).is_some()); } #[test] @@ -229,7 +263,7 @@ mod tests { ASSERT w3 = w2 "; let circuit = Circuit::from_str(src).unwrap(); - assert!(CircuitSimulator::default().check_circuit(&circuit).is_none()); + assert!(CircuitSimulator::check_circuit(&circuit).is_none()); } #[test] @@ -243,7 +277,7 @@ mod tests { ASSERT w2 = w1 "; let circuit = Circuit::from_str(src).unwrap(); - assert!(CircuitSimulator::default().check_circuit(&circuit).is_none()); + assert!(CircuitSimulator::check_circuit(&circuit).is_none()); } #[test] @@ -256,7 +290,7 @@ mod tests { ASSERT w2 = w1 "; let circuit = Circuit::from_str(src).unwrap(); - assert!(CircuitSimulator::default().check_circuit(&circuit).is_none()); + assert!(CircuitSimulator::check_circuit(&circuit).is_none()); } #[test] @@ -269,7 +303,7 @@ mod tests { ASSERT w2 = w1 "; let circuit = Circuit::from_str(src).unwrap(); - assert!(CircuitSimulator::default().check_circuit(&circuit).is_none()); + assert!(CircuitSimulator::check_circuit(&circuit).is_none()); } #[test] @@ -282,7 +316,7 @@ mod tests { ASSERT w4 = w3 "; let disconnected_circuit = Circuit::from_str(src).unwrap(); - assert_eq!(CircuitSimulator::default().check_circuit(&disconnected_circuit), Some(1)); + assert_eq!(CircuitSimulator::check_circuit(&disconnected_circuit), Some(1)); } #[test] @@ -295,7 +329,7 @@ mod tests { INIT b0 = [w0] "; let circuit = Circuit::from_str(src).unwrap(); - assert_eq!(CircuitSimulator::default().check_circuit(&circuit), Some(1)); + assert_eq!(CircuitSimulator::check_circuit(&circuit), Some(1)); } #[test] @@ -308,7 +342,7 @@ mod tests { INIT b0 = [w0] "; let circuit = Circuit::from_str(src).unwrap(); - assert_eq!(CircuitSimulator::default().check_circuit(&circuit), Some(1)); + assert_eq!(CircuitSimulator::check_circuit(&circuit), Some(1)); } #[test] @@ -322,6 +356,6 @@ mod tests { ASSERT w0 = w1*w1 "; let circuit = Circuit::from_str(src).unwrap(); - assert_eq!(CircuitSimulator::default().check_circuit(&circuit), Some(0)); + assert_eq!(CircuitSimulator::check_circuit(&circuit), Some(0)); } } diff --git a/acvm-repo/acvm/src/compiler/transformers/csat.rs b/acvm-repo/acvm/src/compiler/transformers/csat.rs index 4be7dd53ff7..2d65bacc1a9 100644 --- a/acvm-repo/acvm/src/compiler/transformers/csat.rs +++ b/acvm-repo/acvm/src/compiler/transformers/csat.rs @@ -127,15 +127,15 @@ impl CSatTransformer { // First check that this is not a simple opcode which does not need optimization // // If the opcode only has one mul term, then this algorithm cannot optimize it any further - // Either it can be represented in a single arithmetic equation or it's fan-in is too large and we need intermediate variables for those - // large-fan-in optimization is not this algorithms purpose. + // Either it can be represented in a single arithmetic equation or its fan-in is too large and we need intermediate variables for those + // Large-fan-in optimization is not this algorithm's purpose. // If the opcode has 0 mul terms, then it is an add opcode and similarly it can either fit into a single assert-zero opcode or it has a large fan-in if opcode.mul_terms.len() <= 1 { return opcode; } // We now know that this opcode has multiple mul terms and can possibly be simplified into multiple full opcodes - // We need to create a (wl, wr) IndexMap and then check the simplified fan-in to verify if we have terms both with wl and wr + // We need to create a (w_l, w_r) IndexMap and then check the simplified fan-in to verify if we have terms both with w_l and w_r // In general, we can then push more terms into the opcode until we are at width-1 then the last variable will be the intermediate variable // @@ -143,33 +143,27 @@ impl CSatTransformer { // subset of the terms that can be represented as full opcodes let mut new_opcode = Expression::default(); let mut remaining_mul_terms = Vec::with_capacity(opcode.mul_terms.len()); - for pair in opcode.mul_terms { - // We want to layout solvable intermediate variable, if we cannot solve one of the witness + for (scale, w_l, w_r) in opcode.mul_terms { + // We want to layout solvable intermediate variables, if we cannot solve one of the witnesses // that means the intermediate opcode will not be immediately solvable - if !self.solvable_witness.contains(&pair.1) || !self.solvable_witness.contains(&pair.2) - { - remaining_mul_terms.push(pair); + if !self.solvable_witness.contains(&w_l) || !self.solvable_witness.contains(&w_r) { + remaining_mul_terms.push((scale, w_l, w_r)); continue; } - // Check if this pair is present in the simplified fan-in + // Check if this (scale, w_l, w_r) triple is present in the simplified fan-in // We are assuming that the fan-in/fan-out has been simplified. // Note this function is not public, and can only be called within the optimize method, so this guarantee will always hold let index_wl = - opcode.linear_combinations.iter().position(|(_scale, witness)| *witness == pair.1); + opcode.linear_combinations.iter().position(|(_scale, witness)| *witness == w_l); let index_wr = - opcode.linear_combinations.iter().position(|(_scale, witness)| *witness == pair.2); + opcode.linear_combinations.iter().position(|(_scale, witness)| *witness == w_r); match (index_wl, index_wr) { - (None, _) => { + (None, _) | (_, None) => { // This means that the polynomial does not contain both terms // Just push the Qm term as it cannot form a full opcode - new_opcode.mul_terms.push(pair); - } - (_, None) => { - // This means that the polynomial does not contain both terms - // Just push the Qm term as it cannot form a full opcode - new_opcode.mul_terms.push(pair); + new_opcode.mul_terms.push((scale, w_l, w_r)); } (Some(x), Some(y)) => { // This means that we can form a full opcode with this Qm term @@ -181,7 +175,7 @@ impl CSatTransformer { // Lets create an intermediate opcode to store this full opcode // let mut intermediate_opcode = Expression::default(); - intermediate_opcode.mul_terms.push(pair); + intermediate_opcode.mul_terms.push((scale, w_l, w_r)); // Add the left and right wires intermediate_opcode.linear_combinations.push(left_wire_term); @@ -202,7 +196,7 @@ impl CSatTransformer { } } - // Now we have used up 2 spaces in our assert-zero opcode. The width now dictates, how many more we can add + // Now we have used up 2 spaces in our assert-zero opcode. The width now dictates how many more we can add let mut remaining_space = self.width - 2 - 1; // We minus 1 because we need an extra space to contain the intermediate variable // Keep adding terms until we have no more left, or we reach the width let mut remaining_linear_terms = @@ -223,27 +217,26 @@ impl CSatTransformer { } opcode.linear_combinations.extend(remaining_linear_terms); - // Constraint this intermediate_opcode to be equal to the temp variable by adding it into the IndexMap + // Constrain this intermediate_opcode to be equal to the temp variable by adding it into the IndexMap // We need a unique name for our intermediate variable - // XXX: Another optimization, which could be applied in another algorithm + // TODO(https://github.com/noir-lang/noir/issues/10192): Another optimization, which could be applied in another algorithm // If two opcodes have a large fan-in/out and they share a few common terms, then we should create intermediate variables for them // Do some sort of subset matching algorithm for this on the terms of the polynomial - let inter_var = Self::get_or_create_intermediate_vars( + let intermediate_var = Self::get_or_create_intermediate_var( intermediate_variables, intermediate_opcode, num_witness, ); // Add intermediate variable to the new opcode instead of the full opcode - self.mark_solvable(inter_var.1); - new_opcode.linear_combinations.push(inter_var); + self.mark_solvable(intermediate_var.1); + new_opcode.linear_combinations.push(intermediate_var); } }; } - opcode.mul_terms = remaining_mul_terms; // Add the rest of the elements back into the new_opcode - new_opcode.mul_terms.extend(opcode.mul_terms); + new_opcode.mul_terms.extend(remaining_mul_terms); new_opcode.linear_combinations.extend(opcode.linear_combinations); new_opcode.q_c = opcode.q_c; new_opcode.sort(); @@ -252,7 +245,7 @@ impl CSatTransformer { /// Normalize an expression by dividing it by its first coefficient /// The first coefficient here means coefficient of the first linear term, or of the first quadratic term if no linear terms exist. - /// The function panic if the input expression is constant + /// This function panics if the input expression is constant or if the first coefficient's inverse is F::zero() fn normalize(mut expr: Expression) -> (F, Expression) { expr.sort(); let a = if !expr.linear_combinations.is_empty() { @@ -260,14 +253,16 @@ impl CSatTransformer { } else { expr.mul_terms[0].0 }; - (a, &expr * a.inverse()) + let a_inverse = a.inverse(); + assert!(a_inverse != F::zero(), "normalize: the first coefficient is non-invertible"); + (a, &expr * a_inverse) } /// Get or generate a scaled intermediate witness which is equal to the provided expression /// The sets of previously generated witness and their (normalized) expression is cached in the intermediate_variables map /// If there is no cache hit, we generate a new witness (and add the expression to the cache) /// else, we return the cached witness along with the scaling factor so it is equal to the provided expression - fn get_or_create_intermediate_vars( + fn get_or_create_intermediate_var( intermediate_variables: &mut IndexMap, (F, Witness)>, expr: Expression, num_witness: &mut u32, @@ -276,6 +271,10 @@ impl CSatTransformer { if intermediate_variables.contains_key(&normalized_expr) { let (l, iv) = intermediate_variables[&normalized_expr]; + assert!( + l != F::zero(), + "get_or_create_intermediate_var: attempting to divide l by F::zero()" + ); (k / l, iv) } else { let inter_var = Witness(*num_witness); @@ -295,7 +294,7 @@ impl CSatTransformer { // One thing to note is that the multiplication wires do not match any of the fan-in/out wires. This is guaranteed as we have // just completed the full opcode optimization algorithm. // - //Actually we can optimize in two ways here: We can create an intermediate variable which is equal to the fan-in terms + // Actually we can optimize in two ways here: We can create an intermediate variable which is equal to the fan-in terms // t = qL1 * wL3 + qR1 * wR4 -> width = 3 // This `t` value can only use width - 1 terms // The opcode now looks like: qM1 * wL1 * wR2 + t + qR2 * wR5+ qO1 * wO5 + qC @@ -312,7 +311,7 @@ impl CSatTransformer { // Same Example: qM1 * wL1 * wR2 + qL1 * wL3 + qR1 * wR4+ qR2 * wR5 + qO1 * wO5 + qC // t = qM1 * wL1 * wR2 // The opcode now looks like: t + qL1 * wL3 + qR1 * wR4+ qR2 * wR5 + qO1 * wO5 + qC - // Still assuming width3, we still need to use width-1 terms for the intermediate variables, however we can stop at an earlier stage because + // Still assuming width-3, we still need to use width-1 terms for the intermediate variables, however we can stop at an earlier stage because // the opcode does not need the multiplier term to match with any of the fan-in terms // t2 = t + qL1 * wL3 // The opcode now looks like: t2 + qR1 * wR4+ qR2 * wR5 + qO1 * wO5 + qC @@ -338,28 +337,26 @@ impl CSatTransformer { return opcode; } - // 2. Create Intermediate variables for the multiplication opcodes + // Create Intermediate variables for the multiplication opcodes let mut remaining_mul_terms = Vec::with_capacity(opcode.mul_terms.len()); - for mul_term in opcode.mul_terms { - if self.solvable_witness.contains(&mul_term.1) - && self.solvable_witness.contains(&mul_term.2) - { + for (scale, w_l, w_r) in opcode.mul_terms { + if self.solvable_witness.contains(&w_l) && self.solvable_witness.contains(&w_r) { let mut intermediate_opcode = Expression::default(); // Push mul term into the opcode - intermediate_opcode.mul_terms.push(mul_term); + intermediate_opcode.mul_terms.push((scale, w_l, w_r)); // Get an intermediate variable which squashes the multiplication term - let inter_var = Self::get_or_create_intermediate_vars( + let intermediate_var = Self::get_or_create_intermediate_var( intermediate_variables, intermediate_opcode, num_witness, ); // Add intermediate variable as a part of the fan-in for the original opcode - opcode.linear_combinations.push(inter_var); - self.mark_solvable(inter_var.1); + opcode.linear_combinations.push(intermediate_var); + self.mark_solvable(intermediate_var.1); } else { - remaining_mul_terms.push(mul_term); + remaining_mul_terms.push((scale, w_l, w_r)); } } @@ -376,7 +373,7 @@ impl CSatTransformer { // Stores the intermediate variables that are used to // reduce the fan in. - let mut added = Vec::new(); + let mut added_vars = Vec::new(); while opcode.linear_combinations.len() > self.width { // Collect as many terms up to the given width-1 and constrain them to an intermediate variable @@ -396,13 +393,13 @@ impl CSatTransformer { opcode.linear_combinations = remaining_linear_terms; let not_full = intermediate_opcode.linear_combinations.len() < self.width - 1; if intermediate_opcode.linear_combinations.len() > 1 { - let inter_var = Self::get_or_create_intermediate_vars( + let intermediate_var = Self::get_or_create_intermediate_var( intermediate_variables, intermediate_opcode, num_witness, ); - self.mark_solvable(inter_var.1); - added.push(inter_var); + self.mark_solvable(intermediate_var.1); + added_vars.push(intermediate_var); } // The intermediate opcode is not full, but the opcode still has too many terms if not_full && opcode.linear_combinations.len() > self.width { @@ -412,7 +409,7 @@ impl CSatTransformer { // Add back the intermediate variables to // keep consistency with the original equation. - opcode.linear_combinations.extend(added); + opcode.linear_combinations.extend(added_vars); self.partial_opcode_scan_optimization(opcode, intermediate_variables, num_witness) } } @@ -515,4 +512,49 @@ mod tests { let expr = Expression::from_str("-555*w8*w10 + w10 + w11 - w13").unwrap(); assert!(fits_in_one_identity(&expr, 4)); } + + #[test] + #[should_panic(expected = "normalize: the first coefficient is non-invertible")] + fn normalize_on_zero_linear_combination_panics() { + let expr = Expression { + mul_terms: vec![], + linear_combinations: vec![(FieldElement::zero(), Witness(0))], + q_c: FieldElement::zero(), + }; + CSatTransformer::normalize(expr); + } + + #[test] + #[should_panic(expected = "normalize: the first coefficient is non-invertible")] + fn normalize_on_zero_mul_term_scale_panics() { + let expr = Expression { + mul_terms: vec![(FieldElement::zero(), Witness(0), Witness(1))], + linear_combinations: vec![], + q_c: FieldElement::zero(), + }; + CSatTransformer::normalize(expr); + } + + #[test] + #[should_panic( + expected = "get_or_create_intermediate_var: attempting to divide l by F::zero()" + )] + fn get_or_create_intermediate_var_with_zero_panics() { + let expr = Expression { + mul_terms: vec![(FieldElement::one(), Witness(0), Witness(1))], + linear_combinations: vec![], + q_c: FieldElement::zero(), + }; + + let mut intermediate_variables = IndexMap::new(); + intermediate_variables.insert(expr.clone(), (FieldElement::zero(), Witness(0))); + + let mut num_witness = 2; + + CSatTransformer::get_or_create_intermediate_var( + &mut intermediate_variables, + expr, + &mut num_witness, + ); + } } diff --git a/acvm-repo/acvm/src/compiler/transformers/mod.rs b/acvm-repo/acvm/src/compiler/transformers/mod.rs index c6c03167ebd..c39b022e601 100644 --- a/acvm-repo/acvm/src/compiler/transformers/mod.rs +++ b/acvm-repo/acvm/src/compiler/transformers/mod.rs @@ -1,30 +1,32 @@ -/// This module applies backend specific transformation to a [`Circuit`]. +/// This module applies backend specific transformations to a [`Circuit`]. /// -/// ## CSAT: transforms AssertZero opcodes into AssertZero opcodes having the required width. +/// ## CSAT: transforms AssertZero opcodes into AssertZero opcodes having the required width. /// /// For instance, if the width is 4, the AssertZero opcode x1 + x2 + x3 + x4 + x5 - y = 0 will be transformed using 2 intermediate variables (z1,z2): /// x1 + x2 + x3 = z1 /// x4 + x5 = z2 /// z1 + z2 - y = 0 -/// If x1,..x5 are inputs to the program, they are taggeg as 'solvable', and would be used to compute the value of y. -/// If we generate the intermediate variable x4 + x5 - y = z3, we get an unsolvable circuit because this AssertZero opcode will have two unkwnon values: y and z3 -/// So the CSAT transformation keep track of which witness would be solved for each opcode in order to only generate solvable intermediat variables. +/// If x1,..x5 are inputs to the program, they are tagged as 'solvable', and would be used to compute the value of y. +/// If we generate the intermediate variable x4 + x5 - y = z3, we get an unsolvable circuit because this AssertZero opcode will have two unknown values: y and z3 +/// So the CSAT transformation keeps track of which witnesses would be solved for each opcode in order to only generate solvable intermediate variables. +/// +/// ## Eliminate intermediate variables /// -/// ## eliminate intermediate variables /// The 'eliminate intermediate variables' pass will remove any intermediate variables (for instance created by the previous transformation) /// that are used in exactly two AssertZero opcodes. /// This results in arithmetic opcodes having linear combinations of potentially large width. -/// For instance if the intermediate variable is z1 is only used in y: -/// z1 = x1 + x2 +x3 +/// For instance if the intermediate variable is z1 and is only used in y: +/// z1 = x1 + x2 + x3 /// y = z1 + x4 /// We remove it, undoing the work done during the CSAT transformation: y = x1 + x2 + x3 + x4 /// /// We do this because the backend is expected to handle linear combinations of 'unbounded width' in a more efficient way /// than the 'CSAT transformation'. -/// However, it is worth to compute intermediate variables if they are used in more than one other opcode. +/// However, it is worthwhile to compute intermediate variables if they are used in more than one other opcode. /// /// ## redundant_range -/// The 'range optimization' pass, from the optimizers module will remove any redundant range opcodes again. +/// +/// The 'range optimization' pass, from the optimizers module, will remove any redundant range opcodes. use std::collections::BTreeMap; use acir::{ @@ -51,22 +53,32 @@ use super::{ transform_assert_messages, }; -/// We need multiple passes to stabilize the output. -/// The value was determined by running tests. -const MAX_TRANSFORMER_PASSES: usize = 3; +/// We use multiple passes to stabilize the output in many cases +const DEFAULT_MAX_TRANSFORMER_PASSES: usize = 3; /// Applies backend specific optimizations to a [`Circuit`]. +/// +/// Pre-Conditions: +/// - General Optimizer must run before this pass, +/// when max_transformer_passes_or_default.unwrap_or(DEFAULT_MAX_TRANSFORMER_PASSES) is greater than 0 +/// - `expression_width` must be at least `MIN_EXPRESSION_WIDTH`, when bounded pub fn transform( acir: Circuit, expression_width: ExpressionWidth, brillig_side_effects: &BTreeMap, + max_transformer_passes_or_default: Option, ) -> (Circuit, AcirTransformationMap) { // Track original acir opcode positions throughout the transformation passes of the compilation // by applying the modifications done to the circuit opcodes and also to the opcode_positions (delete and insert) let acir_opcode_positions = acir.opcodes.iter().enumerate().map(|(i, _)| i).collect(); - let (mut acir, acir_opcode_positions) = - transform_internal(acir, expression_width, acir_opcode_positions, brillig_side_effects); + let (mut acir, acir_opcode_positions, _opcodes_hash_stabilized) = transform_internal( + acir, + expression_width, + acir_opcode_positions, + brillig_side_effects, + max_transformer_passes_or_default, + ); let transformation_map = AcirTransformationMap::new(&acir_opcode_positions); @@ -80,24 +92,36 @@ pub fn transform( /// Accepts an injected `acir_opcode_positions` to allow transformations to be applied directly after optimizations. /// /// Does multiple passes until the output stabilizes. +/// +/// Pre-Conditions: +/// - General Optimizer must run before this pass, +/// when max_transformer_passes_or_default.unwrap_or(DEFAULT_MAX_TRANSFORMER_PASSES) is greater than 0 +/// - `expression_width` must be at least `MIN_EXPRESSION_WIDTH`, when bounded #[tracing::instrument(level = "trace", name = "transform_acir", skip(acir, acir_opcode_positions))] pub(super) fn transform_internal( mut acir: Circuit, expression_width: ExpressionWidth, mut acir_opcode_positions: Vec, brillig_side_effects: &BTreeMap, -) -> (Circuit, Vec) { + max_transformer_passes_or_default: Option, +) -> (Circuit, Vec, bool) { if acir.opcodes.len() == 1 && matches!(acir.opcodes[0], Opcode::BrilligCall { .. }) { info!("Program is fully unconstrained, skipping transformation pass"); - return (acir, acir_opcode_positions); + return (acir, acir_opcode_positions, true); } // Allow multiple passes until we have stable output. let mut prev_opcodes_hash = rustc_hash::FxBuildHasher.hash_one(&acir.opcodes); + // Checking for stable output after MAX_TRANSFORMER_PASSES + let mut opcodes_hash_stabilized = false; + + let max_transformer_passes = + max_transformer_passes_or_default.unwrap_or(DEFAULT_MAX_TRANSFORMER_PASSES); + // For most test programs it would be enough to loop here, but some of them // don't stabilize unless we also repeat the backend agnostic optimizations. - for _ in 0..MAX_TRANSFORMER_PASSES { + for _ in 0..max_transformer_passes { info!("Number of opcodes {}", acir.opcodes.len()); let (new_acir, new_acir_opcode_positions) = transform_internal_once( acir, @@ -112,15 +136,17 @@ pub(super) fn transform_internal( let new_opcodes_hash = rustc_hash::FxBuildHasher.hash_one(&acir.opcodes); if new_opcodes_hash == prev_opcodes_hash { + opcodes_hash_stabilized = true; break; } prev_opcodes_hash = new_opcodes_hash; } + // After the elimination of intermediate variables the `current_witness_index` is potentially higher than it needs to be, // which would cause gaps if we ran the optimization a second time, making it look like new variables were added. acir.current_witness_index = max_witness(&acir).witness_index(); - (acir, acir_opcode_positions) + (acir, acir_opcode_positions, opcodes_hash_stabilized) } /// Accepts an injected `acir_opcode_positions` to allow transformations to be applied directly after optimizations. @@ -129,6 +155,10 @@ pub(super) fn transform_internal( /// If it is bounded, it first performs the 'CSAT transformation' in one pass, by creating intermediate variables when necessary. /// Then it performs `eliminate_intermediate_variable()` which (re-)combine intermediate variables used only once. /// It concludes with a round of `replace_redundant_ranges()` which removes range checks made redundant by the previous pass. +/// +/// Pre-Conditions: +/// - General Optimizer must run before this pass +/// - `expression_width` must be at least `MIN_EXPRESSION_WIDTH`, when bounded #[tracing::instrument( level = "trace", name = "transform_acir_once", @@ -140,6 +170,11 @@ fn transform_internal_once( acir_opcode_positions: Vec, brillig_side_effects: &BTreeMap, ) -> (Circuit, Vec) { + // 1. CSAT transformation + // Process each opcode in the circuit by marking the solvable witnesses and transforming the AssertZero opcodes + // to the required width by creating intermediate variables. + // Knowing if a witness is solvable avoids creating un-solvable intermediate variables. + // If the expression width is unbounded, we don't need to do anything. let mut transformer = match &expression_width { ExpressionWidth::Unbounded => { @@ -154,11 +189,6 @@ fn transform_internal_once( } }; - // 1. CSAT transformation - // Process each opcode in the circuit by marking the solvable witnesses and transforming the AssertZero opcodes - // to the required width by creating intermediate variables. - // Knowing if a witness is solvable avoids creating un-solvable intermediate variables. - let mut new_acir_opcode_positions: Vec = Vec::with_capacity(acir_opcode_positions.len()); // Optimize the assert-zero gates by reducing them into the correct width and // creating intermediate variables when necessary @@ -166,7 +196,7 @@ fn transform_internal_once( let mut next_witness_index = acir.current_witness_index + 1; // maps a normalized expression to the intermediate variable which represents the expression, along with its 'norm' - // the 'norm' is simply the value of the first non zero coefficient in the expression, taken from the linear terms, or quadratic terms if there is none. + // the 'norm' is simply the value of the first non-zero coefficient in the expression, taken from the linear terms, or quadratic terms if there is none. let mut intermediate_variables: IndexMap, (F, Witness)> = IndexMap::new(); for (index, opcode) in acir.opcodes.into_iter().enumerate() { match opcode { @@ -221,9 +251,9 @@ fn transform_internal_once( Opcode::BrilligCall { ref outputs, .. } => { for output in outputs { match output { - BrilligOutputs::Simple(w) => transformer.mark_solvable(*w), - BrilligOutputs::Array(v) => { - for witness in v { + BrilligOutputs::Simple(witness) => transformer.mark_solvable(*witness), + BrilligOutputs::Array(witnesses) => { + for witness in witnesses { transformer.mark_solvable(*witness); } } @@ -324,8 +354,8 @@ where /// Fold many witnesses into the state. fn fold_many<'w, I: Iterator>(&mut self, witnesses: I) { - for w in witnesses { - self.fold(*w); + for witness in witnesses { + self.fold(*witness); } } @@ -343,8 +373,8 @@ where self.fold_expr(value); } Opcode::MemoryInit { block_id: _, init, block_type: _ } => { - for w in init { - self.fold(*w); + for witness in init { + self.fold(*witness); } } // We keep the display for a BrilligCall and circuit Call separate as they @@ -395,10 +425,10 @@ where fn fold_brillig_outputs(&mut self, outputs: &[BrilligOutputs]) { for output in outputs { match output { - BrilligOutputs::Simple(w) => { - self.fold(*w); + BrilligOutputs::Simple(witness) => { + self.fold(*witness); } - BrilligOutputs::Array(ws) => self.fold_many(ws.iter()), + BrilligOutputs::Array(witnesses) => self.fold_many(witnesses.iter()), } } } @@ -522,3 +552,103 @@ where } } } + +#[cfg(test)] +mod tests { + use crate::compiler::{CircuitSimulator, transform_internal}; + use acir::circuit::{Circuit, ExpressionWidth, brillig::BrilligFunctionId}; + use std::collections::BTreeMap; + + #[test] + fn test_max_transformer_passes() { + let formatted_acir = r#"private parameters: [w0] + public parameters: [] + return values: [w1, w2, w3, w4, w5, w6, w7, w8, w9, w10, w11, w12, w13, w14, w15, w16, w17, w18, w19, w20, w21, w22, w23, w24, w25, w26, w27, w28, w29, w30, w31] + BRILLIG CALL func: 0, inputs: [w0, 31, 256], outputs: [w32, w33, w34, w35, w36, w37, w38, w39, w40, w41, w42, w43, w44, w45, w46, w47, w48, w49, w50, w51, w52, w53, w54, w55, w56, w57, w58, w59, w60, w61, w62] + BLACKBOX::RANGE input: w35, bits: 8 + BLACKBOX::RANGE input: w36, bits: 8 + BLACKBOX::RANGE input: w37, bits: 8 + BLACKBOX::RANGE input: w38, bits: 8 + BLACKBOX::RANGE input: w39, bits: 8 + BLACKBOX::RANGE input: w40, bits: 8 + BLACKBOX::RANGE input: w41, bits: 8 + BLACKBOX::RANGE input: w42, bits: 8 + BLACKBOX::RANGE input: w43, bits: 8 + BLACKBOX::RANGE input: w44, bits: 8 + BLACKBOX::RANGE input: w45, bits: 8 + BLACKBOX::RANGE input: w46, bits: 8 + BLACKBOX::RANGE input: w47, bits: 8 + BLACKBOX::RANGE input: w48, bits: 8 + BLACKBOX::RANGE input: w49, bits: 8 + BLACKBOX::RANGE input: w50, bits: 8 + BLACKBOX::RANGE input: w51, bits: 8 + BLACKBOX::RANGE input: w52, bits: 8 + BLACKBOX::RANGE input: w53, bits: 8 + BLACKBOX::RANGE input: w54, bits: 8 + BLACKBOX::RANGE input: w55, bits: 8 + BLACKBOX::RANGE input: w56, bits: 8 + BLACKBOX::RANGE input: w57, bits: 8 + BLACKBOX::RANGE input: w58, bits: 8 + BLACKBOX::RANGE input: w59, bits: 8 + BLACKBOX::RANGE input: w60, bits: 8 + BLACKBOX::RANGE input: w61, bits: 8 + BLACKBOX::RANGE input: w62, bits: 8 + ASSERT w32 = w0 - 256*w33 - 65536*w34 - 16777216*w35 - 4294967296*w36 - 1099511627776*w37 - 281474976710656*w38 - 72057594037927936*w39 - 18446744073709551616*w40 - 4722366482869645213696*w41 - 1208925819614629174706176*w42 - 309485009821345068724781056*w43 - 79228162514264337593543950336*w44 - 20282409603651670423947251286016*w45 - 5192296858534827628530496329220096*w46 - 1329227995784915872903807060280344576*w47 - 340282366920938463463374607431768211456*w48 - 87112285931760246646623899502532662132736*w49 - 22300745198530623141535718272648361505980416*w50 - 5708990770823839524233143877797980545530986496*w51 - 1461501637330902918203684832716283019655932542976*w52 - 374144419156711147060143317175368453031918731001856*w53 - 95780971304118053647396689196894323976171195136475136*w54 - 24519928653854221733733552434404946937899825954937634816*w55 - 6277101735386680763835789423207666416102355444464034512896*w56 - 1606938044258990275541962092341162602522202993782792835301376*w57 - 411376139330301510538742295639337626245683966408394965837152256*w58 - 105312291668557186697918027683670432318895095400549111254310977536*w59 - 26959946667150639794667015087019630673637144422540572481103610249216*w60 - 6901746346790563787434755862277025452451108972170386555162524223799296*w61 - 1766847064778384329583297500742918515827483896875618958121606201292619776*w62 + ASSERT w32 = 60 + ASSERT w33 = 33 + ASSERT w34 = 31 + ASSERT w0 = 16777216*w35 + 4294967296*w36 + 1099511627776*w37 + 281474976710656*w38 + 72057594037927936*w39 + 18446744073709551616*w40 + 4722366482869645213696*w41 + 1208925819614629174706176*w42 + 309485009821345068724781056*w43 + 79228162514264337593543950336*w44 + 20282409603651670423947251286016*w45 + 5192296858534827628530496329220096*w46 + 1329227995784915872903807060280344576*w47 + 340282366920938463463374607431768211456*w48 + 87112285931760246646623899502532662132736*w49 + 22300745198530623141535718272648361505980416*w50 + 5708990770823839524233143877797980545530986496*w51 + 1461501637330902918203684832716283019655932542976*w52 + 374144419156711147060143317175368453031918731001856*w53 + 95780971304118053647396689196894323976171195136475136*w54 + 24519928653854221733733552434404946937899825954937634816*w55 + 6277101735386680763835789423207666416102355444464034512896*w56 + 1606938044258990275541962092341162602522202993782792835301376*w57 + 411376139330301510538742295639337626245683966408394965837152256*w58 + 105312291668557186697918027683670432318895095400549111254310977536*w59 + 26959946667150639794667015087019630673637144422540572481103610249216*w60 + 6901746346790563787434755862277025452451108972170386555162524223799296*w61 + 1766847064778384329583297500742918515827483896875618958121606201292619776*w62 + 2040124 + ASSERT w62 = w1 + ASSERT w61 = w2 + ASSERT w60 = w3 + ASSERT w59 = w4 + ASSERT w58 = w5 + ASSERT w57 = w6 + ASSERT w56 = w7 + ASSERT w55 = w8 + ASSERT w54 = w9 + ASSERT w53 = w10 + ASSERT w52 = w11 + ASSERT w51 = w12 + ASSERT w50 = w13 + ASSERT w49 = w14 + ASSERT w48 = w15 + ASSERT w47 = w16 + ASSERT w46 = w17 + ASSERT w45 = w18 + ASSERT w44 = w19 + ASSERT w43 = w20 + ASSERT w42 = w21 + ASSERT w41 = w22 + ASSERT w40 = w23 + ASSERT w39 = w24 + ASSERT w38 = w25 + ASSERT w37 = w26 + ASSERT w36 = w27 + ASSERT w35 = w28 + ASSERT w29 = 31 + ASSERT w30 = 33 + ASSERT w31 = 60 + "#; + + let acir = Circuit::from_str(formatted_acir).unwrap(); + assert!(CircuitSimulator::check_circuit(&acir).is_none()); + let expression_width = ExpressionWidth::Bounded { width: 4 }; + let acir_opcode_positions = vec![ + 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, 29, 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, 64, + ]; + let mut brillig_side_effects = BTreeMap::new(); + brillig_side_effects.insert(BrilligFunctionId(0), false); + + let (_, _, opcodes_hash_stabilized) = transform_internal( + acir, + expression_width, + acir_opcode_positions.clone(), + &brillig_side_effects, + None, + ); + assert!(!opcodes_hash_stabilized); + } +} diff --git a/acvm-repo/acvm/src/pwg/arithmetic.rs b/acvm-repo/acvm/src/pwg/arithmetic.rs index cf771815827..52261fb6b35 100644 --- a/acvm-repo/acvm/src/pwg/arithmetic.rs +++ b/acvm-repo/acvm/src/pwg/arithmetic.rs @@ -73,7 +73,7 @@ impl ExpressionSolver { insert_value(&w1, assignment, initial_witness) } } else { - // TODO: can we be more specific with this error? + // TODO(https://github.com/noir-lang/noir/issues/10191): can we be more specific with this error? Err(OpcodeResolutionError::OpcodeNotSolvable( OpcodeNotSolvable::ExpressionHasTooManyUnknowns(opcode.clone()), )) @@ -282,6 +282,10 @@ fn quick_invert(numerator: F, denominator: F) -> F { } else if denominator == -F::one() { -numerator } else { + assert!( + denominator != F::zero(), + "quick_invert: attempting to divide numerator by F::zero()" + ); numerator / denominator } } @@ -299,6 +303,12 @@ mod tests { assert_eq!(quick_invert(numerator, -FieldElement::one()), numerator / -FieldElement::one()); } + #[test] + #[should_panic(expected = "quick_invert: attempting to divide numerator by F::zero()")] + fn quick_invert_zero_denominator() { + quick_invert(FieldElement::one(), FieldElement::zero()); + } + #[test] fn solves_simple_assignment() { let a = Witness(0); diff --git a/compiler/noirc_evaluator/src/acir/acir_context/mod.rs b/compiler/noirc_evaluator/src/acir/acir_context/mod.rs index 216d2ced98b..69519f19c9b 100644 --- a/compiler/noirc_evaluator/src/acir/acir_context/mod.rs +++ b/compiler/noirc_evaluator/src/acir/acir_context/mod.rs @@ -1271,8 +1271,6 @@ impl AcirContext { limb_vars.reverse(); } - // `Intrinsic::ToRadix` returns slices which are represented - // by tuples with the structure (length, slice contents) Ok(AcirValue::Array(limb_vars.into())) } diff --git a/justfile b/justfile index f316ffd086c..75aa953efaa 100644 --- a/justfile +++ b/justfile @@ -112,7 +112,7 @@ fuzz-nightly: install-rust-tools NOIR_AST_FUZZER_FORCE_NON_DETERMINISTIC=1 cargo nextest run -p noir_ast_fuzzer_fuzz --no-fail-fast -cargo-mutants-args := if ci == "1" { "--in-place -vV" } else { "" } +cargo-mutants-args := if ci == "1" { "--in-place -vV" } else { "-j2" } mutation-test base="master": install-rust-tools #!/usr/bin/env bash @@ -120,8 +120,9 @@ mutation-test base="master": install-rust-tools trap "rm -rf $tmpdir" EXIT git diff origin/{{base}}.. | tee $tmpdir/git.diff - cargo mutants --no-shuffle --test-tool=nextest --workspace --in-diff $tmpdir/git.diff {{cargo-mutants-args}} - + cargo mutants --no-shuffle --test-tool=nextest -p acir_field -p acir -p acvm -p brillig -p brillig_vm -p blackbox_solver --in-diff $tmpdir/git.diff {{cargo-mutants-args}} + cargo mutants --no-shuffle --test-tool=nextest -p noirc_evaluator --in-diff $tmpdir/git.diff {{cargo-mutants-args}} + # Checks if there are any pending insta.rs snapshots and errors if any exist. check-pending-snapshots: #!/usr/bin/env bash diff --git a/tooling/nargo/src/ops/check.rs b/tooling/nargo/src/ops/check.rs index 7cef18faac9..7e1b73a47f2 100644 --- a/tooling/nargo/src/ops/check.rs +++ b/tooling/nargo/src/ops/check.rs @@ -6,8 +6,7 @@ use noirc_errors::CustomDiagnostic; #[tracing::instrument(level = "trace", skip_all)] pub fn check_program(compiled_program: &CompiledProgram) -> Result<(), ErrorsAndWarnings> { for (i, circuit) in compiled_program.program.functions.iter().enumerate() { - let mut simulator = CircuitSimulator::default(); - if let Some(opcode) = simulator.check_circuit(circuit) { + if let Some(opcode) = CircuitSimulator::check_circuit(circuit) { let diag = if let Some(call_stack) = compiled_program.debug[i].acir_locations.get(&AcirOpcodeLocation::new(opcode)) {