From b610340a2d674428b50ab551fb4756b07fccebf9 Mon Sep 17 00:00:00 2001 From: guipublic Date: Wed, 22 May 2024 14:52:33 +0000 Subject: [PATCH 1/6] use predicate for curve operations --- .../src/ssa/acir_gen/acir_ir/acir_variable.rs | 46 ++++++++++++++++++- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 7 ++- .../noirc_evaluator/src/ssa/ir/instruction.rs | 15 +++++- .../src/ssa/opt/remove_enable_side_effects.rs | 6 ++- 4 files changed, 67 insertions(+), 7 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs index 90cdbb650c2..5bfbbf980e0 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs @@ -424,7 +424,7 @@ impl AcirContext { self.add_mul_var(sum, -FieldElement::from(2_i128), prod) } else { let inputs = vec![AcirValue::Var(lhs, typ.clone()), AcirValue::Var(rhs, typ)]; - let outputs = self.black_box_function(BlackBoxFunc::XOR, inputs, 1)?; + let outputs = self.black_box_function(BlackBoxFunc::XOR, inputs, 1, None)?; Ok(outputs[0]) } } @@ -454,7 +454,7 @@ impl AcirContext { self.mul_var(lhs, rhs) } else { let inputs = vec![AcirValue::Var(lhs, typ.clone()), AcirValue::Var(rhs, typ)]; - let outputs = self.black_box_function(BlackBoxFunc::AND, inputs, 1)?; + let outputs = self.black_box_function(BlackBoxFunc::AND, inputs, 1, None)?; Ok(outputs[0]) } } @@ -1184,11 +1184,13 @@ impl AcirContext { /// Calls a Blackbox function on the given inputs and returns a given set of outputs /// to represent the result of the blackbox function. + /// current_side_effect_enabled is required for MultiScalarMul and EmbeddedCurveAdd which have side effects pub(crate) fn black_box_function( &mut self, name: BlackBoxFunc, mut inputs: Vec, mut output_count: usize, + current_side_effect_enabled: Option, ) -> Result, RuntimeError> { // Separate out any arguments that should be constants let (constant_inputs, constant_outputs) = match name { @@ -1344,6 +1346,38 @@ impl AcirContext { } _ => (vec![], vec![]), }; + //Issue #5045: We set curve points to infinity if enable_side_effect is false. + match name { + BlackBoxFunc::MultiScalarMul => match inputs[0] { + AcirValue::Array(ref mut values) => { + let mut i = 2; + while i < values.len() { + values[i] = AcirValue::Var( + self.var_or_one( + values[i].clone().into_var()?, + current_side_effect_enabled.unwrap(), + )?, + AcirType::unsigned(1), + ); + i += 3; + } + } + _ => unreachable!(), + }, + BlackBoxFunc::EmbeddedCurveAdd => { + let input_with_predicate = self.var_or_one( + inputs[2].clone().into_var()?, + current_side_effect_enabled.unwrap(), + )?; + inputs[2] = AcirValue::Var(input_with_predicate, AcirType::unsigned(1)); + let input_with_predicate = self.var_or_one( + inputs[5].clone().into_var()?, + current_side_effect_enabled.unwrap(), + )?; + inputs[5] = AcirValue::Var(input_with_predicate, AcirType::unsigned(1)); + } + _ => (), + } // Convert `AcirVar` to `FunctionInput` let inputs = self.prepare_inputs_for_black_box_func_call(inputs)?; @@ -1368,6 +1402,14 @@ impl AcirContext { Ok(results) } + // Computes: if predicate { var } else { one } + fn var_or_one(&mut self, var: AcirVar, predicate: AcirVar) -> Result { + let one = self.add_constant(FieldElement::from(1_u128)); + let not_pred = self.sub_var(one, predicate)?; + let with_predicate = self.mul_var(var, predicate)?; + self.add_var(not_pred, with_predicate) + } + /// Black box function calls expect their inputs to be in a specific data structure (FunctionInput). /// /// This function will convert `AcirVar` into `FunctionInput` for a blackbox function call. diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index fefe5f6f8e6..94dd2f0263f 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -2068,7 +2068,12 @@ impl<'a> Context<'a> { sum + dfg.try_get_array_length(*result_id).unwrap_or(1) }); - let vars = self.acir_context.black_box_function(black_box, inputs, output_count)?; + let vars = self.acir_context.black_box_function( + black_box, + inputs, + output_count, + Some(self.current_side_effects_enabled_var), + )?; Ok(self.convert_vars_to_values(vars, dfg, result_ids)) } diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index f136d3c5fb2..8afd617a904 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -119,7 +119,12 @@ impl Intrinsic { | Intrinsic::AsField => false, // Some black box functions have side-effects - Intrinsic::BlackBox(func) => matches!(func, BlackBoxFunc::RecursiveAggregation), + Intrinsic::BlackBox(func) => matches!( + func, + BlackBoxFunc::RecursiveAggregation + | BlackBoxFunc::MultiScalarMul + | BlackBoxFunc::EmbeddedCurveAdd + ), } } @@ -365,7 +370,13 @@ impl Instruction { Instruction::Call { func, .. } => match dfg[*func] { Value::Function(_) => true, Value::Intrinsic(intrinsic) => { - matches!(intrinsic, Intrinsic::SliceInsert | Intrinsic::SliceRemove) + matches!( + intrinsic, + Intrinsic::SliceInsert + | Intrinsic::SliceRemove + | Intrinsic::BlackBox(BlackBoxFunc::EmbeddedCurveAdd) + | Intrinsic::BlackBox(BlackBoxFunc::MultiScalarMul) + ) } _ => false, }, diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs index 9309652d508..69e42205c72 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs @@ -10,7 +10,7 @@ //! before the [Instruction]. Continue inserting instructions until the next [Instruction::EnableSideEffects] is encountered. use std::collections::HashSet; -use acvm::FieldElement; +use acvm::{acir::BlackBoxFunc, FieldElement}; use crate::ssa::{ ir::{ @@ -146,7 +146,9 @@ impl Context { | Intrinsic::SlicePopBack | Intrinsic::SlicePopFront | Intrinsic::SliceInsert - | Intrinsic::SliceRemove => true, + | Intrinsic::SliceRemove + | Intrinsic::BlackBox(BlackBoxFunc::MultiScalarMul) + | Intrinsic::BlackBox(BlackBoxFunc::EmbeddedCurveAdd) => true, Intrinsic::ArrayLen | Intrinsic::AssertConstant From b0b2a91392d28fbc79d3dd6caf507c5d4a24dce6 Mon Sep 17 00:00:00 2001 From: guipublic Date: Wed, 22 May 2024 15:18:16 +0000 Subject: [PATCH 2/6] allows removal of unused ec ops --- compiler/noirc_evaluator/src/ssa/ir/instruction.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 8afd617a904..e53705d691d 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -340,6 +340,9 @@ impl Instruction { // Some `Intrinsic`s have side effects so we must check what kind of `Call` this is. Call { func, .. } => match dfg[*func] { + // Explicitly allows removal of unused ec operations, even if they can fail + Value::Intrinsic(Intrinsic::BlackBox(BlackBoxFunc::MultiScalarMul)) + | Value::Intrinsic(Intrinsic::BlackBox(BlackBoxFunc::EmbeddedCurveAdd)) => true, Value::Intrinsic(intrinsic) => !intrinsic.has_side_effects(), // All foreign functions are treated as having side effects. From 1423b650694f5d89ab6eac4ffd5a50255ce6dedb Mon Sep 17 00:00:00 2001 From: guipublic Date: Wed, 22 May 2024 15:26:33 +0000 Subject: [PATCH 3/6] add regression test --- .../regression_5045/Nargo.toml | 7 ++++++ .../regression_5045/Prover.toml | 0 .../regression_5045/src/main.nr | 22 +++++++++++++++++++ 3 files changed, 29 insertions(+) create mode 100644 test_programs/execution_success/regression_5045/Nargo.toml create mode 100644 test_programs/execution_success/regression_5045/Prover.toml create mode 100644 test_programs/execution_success/regression_5045/src/main.nr diff --git a/test_programs/execution_success/regression_5045/Nargo.toml b/test_programs/execution_success/regression_5045/Nargo.toml new file mode 100644 index 00000000000..8f56d392fec --- /dev/null +++ b/test_programs/execution_success/regression_5045/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "regression_5045" +version = "0.1.0" +type = "bin" +authors = [""] + +[dependencies] \ No newline at end of file diff --git a/test_programs/execution_success/regression_5045/Prover.toml b/test_programs/execution_success/regression_5045/Prover.toml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/test_programs/execution_success/regression_5045/src/main.nr b/test_programs/execution_success/regression_5045/src/main.nr new file mode 100644 index 00000000000..5e3abd7f42c --- /dev/null +++ b/test_programs/execution_success/regression_5045/src/main.nr @@ -0,0 +1,22 @@ +use dep::std::embedded_curve_ops::EmbeddedCurvePoint; +use dep::std::embedded_curve_ops::EmbeddedCurveScalar; + +fn main() { + let a = EmbeddedCurvePoint { + x: 0x1d8eb4378a3bde41e0b6a9a8dcbd21b7ff9c51bdd6ca13ce989abbbf90df3666, + y: 0x06075b63354f2504f9cddba0b94ed0cef35fc88615e69ec1f853b51eb79a24a0, + is_infinite: false + }; + + let aa = a.double(); + + if aa.x != a.add(a).x { + let bad = EmbeddedCurvePoint { x: 0, y: 5, is_infinite: false }; + let d = bad.double(); + let e = dep::std::embedded_curve_ops::multi_scalar_mul( + [a, bad], + [EmbeddedCurveScalar { lo: 1, hi: 0 }, EmbeddedCurveScalar { lo: 1, hi: 0 }] + ); + assert(e[0] != d.x); + } +} From 71602195ad9d8aeef5a04f6d9142c5da774edaa3 Mon Sep 17 00:00:00 2001 From: guipublic Date: Thu, 23 May 2024 10:47:21 +0000 Subject: [PATCH 4/6] move the code to flattening pass --- .../src/ssa/acir_gen/acir_ir/acir_variable.rs | 45 +----------------- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 7 +-- .../noirc_evaluator/src/ssa/ir/instruction.rs | 8 +--- .../src/ssa/opt/flatten_cfg.rs | 47 ++++++++++++++++++- .../src/ssa/opt/remove_enable_side_effects.rs | 6 +-- 5 files changed, 52 insertions(+), 61 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs index 5bfbbf980e0..3a1e184b86c 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs @@ -424,7 +424,7 @@ impl AcirContext { self.add_mul_var(sum, -FieldElement::from(2_i128), prod) } else { let inputs = vec![AcirValue::Var(lhs, typ.clone()), AcirValue::Var(rhs, typ)]; - let outputs = self.black_box_function(BlackBoxFunc::XOR, inputs, 1, None)?; + let outputs = self.black_box_function(BlackBoxFunc::XOR, inputs, 1)?; Ok(outputs[0]) } } @@ -454,7 +454,7 @@ impl AcirContext { self.mul_var(lhs, rhs) } else { let inputs = vec![AcirValue::Var(lhs, typ.clone()), AcirValue::Var(rhs, typ)]; - let outputs = self.black_box_function(BlackBoxFunc::AND, inputs, 1, None)?; + let outputs = self.black_box_function(BlackBoxFunc::AND, inputs, 1)?; Ok(outputs[0]) } } @@ -1190,7 +1190,6 @@ impl AcirContext { name: BlackBoxFunc, mut inputs: Vec, mut output_count: usize, - current_side_effect_enabled: Option, ) -> Result, RuntimeError> { // Separate out any arguments that should be constants let (constant_inputs, constant_outputs) = match name { @@ -1346,38 +1345,6 @@ impl AcirContext { } _ => (vec![], vec![]), }; - //Issue #5045: We set curve points to infinity if enable_side_effect is false. - match name { - BlackBoxFunc::MultiScalarMul => match inputs[0] { - AcirValue::Array(ref mut values) => { - let mut i = 2; - while i < values.len() { - values[i] = AcirValue::Var( - self.var_or_one( - values[i].clone().into_var()?, - current_side_effect_enabled.unwrap(), - )?, - AcirType::unsigned(1), - ); - i += 3; - } - } - _ => unreachable!(), - }, - BlackBoxFunc::EmbeddedCurveAdd => { - let input_with_predicate = self.var_or_one( - inputs[2].clone().into_var()?, - current_side_effect_enabled.unwrap(), - )?; - inputs[2] = AcirValue::Var(input_with_predicate, AcirType::unsigned(1)); - let input_with_predicate = self.var_or_one( - inputs[5].clone().into_var()?, - current_side_effect_enabled.unwrap(), - )?; - inputs[5] = AcirValue::Var(input_with_predicate, AcirType::unsigned(1)); - } - _ => (), - } // Convert `AcirVar` to `FunctionInput` let inputs = self.prepare_inputs_for_black_box_func_call(inputs)?; @@ -1402,14 +1369,6 @@ impl AcirContext { Ok(results) } - // Computes: if predicate { var } else { one } - fn var_or_one(&mut self, var: AcirVar, predicate: AcirVar) -> Result { - let one = self.add_constant(FieldElement::from(1_u128)); - let not_pred = self.sub_var(one, predicate)?; - let with_predicate = self.mul_var(var, predicate)?; - self.add_var(not_pred, with_predicate) - } - /// Black box function calls expect their inputs to be in a specific data structure (FunctionInput). /// /// This function will convert `AcirVar` into `FunctionInput` for a blackbox function call. diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 94dd2f0263f..fefe5f6f8e6 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -2068,12 +2068,7 @@ impl<'a> Context<'a> { sum + dfg.try_get_array_length(*result_id).unwrap_or(1) }); - let vars = self.acir_context.black_box_function( - black_box, - inputs, - output_count, - Some(self.current_side_effects_enabled_var), - )?; + let vars = self.acir_context.black_box_function(black_box, inputs, output_count)?; Ok(self.convert_vars_to_values(vars, dfg, result_ids)) } diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index e53705d691d..083e5e57013 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -373,13 +373,7 @@ impl Instruction { Instruction::Call { func, .. } => match dfg[*func] { Value::Function(_) => true, Value::Intrinsic(intrinsic) => { - matches!( - intrinsic, - Intrinsic::SliceInsert - | Intrinsic::SliceRemove - | Intrinsic::BlackBox(BlackBoxFunc::EmbeddedCurveAdd) - | Intrinsic::BlackBox(BlackBoxFunc::MultiScalarMul) - ) + matches!(intrinsic, Intrinsic::SliceInsert | Intrinsic::SliceRemove) } _ => false, }, diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 0f8b49b40ec..4913bbba279 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -134,7 +134,7 @@ use fxhash::FxHashMap as HashMap; use std::collections::{BTreeMap, HashSet}; -use acvm::FieldElement; +use acvm::{acir::BlackBoxFunc, FieldElement}; use iter_extended::vecmap; use crate::ssa::{ @@ -769,7 +769,38 @@ impl<'f> Context<'f> { Instruction::Call { func, arguments } } + //Issue #5045: We set curve points to infinity if condition is false + Value::Intrinsic(Intrinsic::BlackBox(BlackBoxFunc::EmbeddedCurveAdd)) => { + arguments[2] = self.var_or_one(arguments[2], condition, call_stack.clone()); + arguments[5] = self.var_or_one(arguments[5], condition, call_stack.clone()); + Instruction::Call { func, arguments } + } + Value::Intrinsic(Intrinsic::BlackBox(BlackBoxFunc::MultiScalarMul)) => { + let mut array_with_predicate = im::Vector::new(); + let array_typ; + if let Value::Array { array, typ } = + &self.inserter.function.dfg[arguments[0]] + { + array_typ = typ.clone(); + for (i, value) in array.clone().iter().enumerate() { + if i % 3 == 2 { + array_with_predicate.push_back(self.var_or_one( + *value, + condition, + call_stack.clone(), + )); + } else { + array_with_predicate.push_back(*value); + } + } + } else { + unreachable!(); + } + arguments[0] = + self.inserter.function.dfg.make_array(array_with_predicate, array_typ); + Instruction::Call { func, arguments } + } _ => Instruction::Call { func, arguments }, }, other => other, @@ -779,6 +810,20 @@ impl<'f> Context<'f> { } } + // Computes: if condition { var } else { 1 } + fn var_or_one(&mut self, var: ValueId, condition: ValueId, call_stack: CallStack) -> ValueId { + let field = self.insert_instruction( + Instruction::binary(BinaryOp::Mul, var, condition), + call_stack.clone(), + ); + let not_condition = + self.insert_instruction(Instruction::Not(condition), call_stack.clone()); + self.insert_instruction( + Instruction::binary(BinaryOp::Add, field, not_condition), + call_stack, + ) + } + fn undo_stores_in_then_branch(&mut self, store_values: &HashMap) { for (address, store) in store_values { let address = *address; diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs index 69e42205c72..9309652d508 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs @@ -10,7 +10,7 @@ //! before the [Instruction]. Continue inserting instructions until the next [Instruction::EnableSideEffects] is encountered. use std::collections::HashSet; -use acvm::{acir::BlackBoxFunc, FieldElement}; +use acvm::FieldElement; use crate::ssa::{ ir::{ @@ -146,9 +146,7 @@ impl Context { | Intrinsic::SlicePopBack | Intrinsic::SlicePopFront | Intrinsic::SliceInsert - | Intrinsic::SliceRemove - | Intrinsic::BlackBox(BlackBoxFunc::MultiScalarMul) - | Intrinsic::BlackBox(BlackBoxFunc::EmbeddedCurveAdd) => true, + | Intrinsic::SliceRemove => true, Intrinsic::ArrayLen | Intrinsic::AssertConstant From a0fbe7d2bdb7bb11bdea589a0efbef812551e9ab Mon Sep 17 00:00:00 2001 From: guipublic Date: Thu, 30 May 2024 11:32:59 +0000 Subject: [PATCH 5/6] code review --- test_programs/execution_success/regression_5045/Prover.toml | 1 + test_programs/execution_success/regression_5045/src/main.nr | 6 ++---- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/test_programs/execution_success/regression_5045/Prover.toml b/test_programs/execution_success/regression_5045/Prover.toml index e69de29bb2d..5444a86ec82 100644 --- a/test_programs/execution_success/regression_5045/Prover.toml +++ b/test_programs/execution_success/regression_5045/Prover.toml @@ -0,0 +1 @@ +is_active = false \ No newline at end of file diff --git a/test_programs/execution_success/regression_5045/src/main.nr b/test_programs/execution_success/regression_5045/src/main.nr index 5e3abd7f42c..015fb1b2555 100644 --- a/test_programs/execution_success/regression_5045/src/main.nr +++ b/test_programs/execution_success/regression_5045/src/main.nr @@ -1,16 +1,14 @@ use dep::std::embedded_curve_ops::EmbeddedCurvePoint; use dep::std::embedded_curve_ops::EmbeddedCurveScalar; -fn main() { +fn main(is_active: bool) { let a = EmbeddedCurvePoint { x: 0x1d8eb4378a3bde41e0b6a9a8dcbd21b7ff9c51bdd6ca13ce989abbbf90df3666, y: 0x06075b63354f2504f9cddba0b94ed0cef35fc88615e69ec1f853b51eb79a24a0, is_infinite: false }; - let aa = a.double(); - - if aa.x != a.add(a).x { + if is_active { let bad = EmbeddedCurvePoint { x: 0, y: 5, is_infinite: false }; let d = bad.double(); let e = dep::std::embedded_curve_ops::multi_scalar_mul( From 2f203e9272eedf8825ae209a7b9c113c13d80999 Mon Sep 17 00:00:00 2001 From: guipublic Date: Thu, 30 May 2024 13:19:47 +0000 Subject: [PATCH 6/6] code review --- .../noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs index 69e2f1263a6..4a0f9f798ff 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs @@ -1219,7 +1219,6 @@ impl AcirContext { /// Calls a Blackbox function on the given inputs and returns a given set of outputs /// to represent the result of the blackbox function. - /// current_side_effect_enabled is required for MultiScalarMul and EmbeddedCurveAdd which have side effects pub(crate) fn black_box_function( &mut self, name: BlackBoxFunc,