From 145b90945486907cb6db75d3f3f93a58d19b2a32 Mon Sep 17 00:00:00 2001 From: guipublic <47281315+guipublic@users.noreply.github.com> Date: Thu, 30 May 2024 15:40:15 +0200 Subject: [PATCH] fix: use predicate for curve operations (#5076) # Description ## Problem\* Resolves #5045 ## Summary\* use predicate to set is_infinite to true when side_effects are disabled, to ensure that the curve operation will not fail. ## Additional Context ## Documentation\* Check one: - [X] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [X] I have tested the changes locally. - [X] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../noirc_evaluator/src/ssa/ir/instruction.rs | 10 +++- .../src/ssa/opt/flatten_cfg.rs | 47 ++++++++++++++++++- .../regression_5045/Nargo.toml | 7 +++ .../regression_5045/Prover.toml | 1 + .../regression_5045/src/main.nr | 20 ++++++++ 5 files changed, 83 insertions(+), 2 deletions(-) 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/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 5110140bfcc..7517b54aac3 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -123,7 +123,12 @@ impl Intrinsic { | Intrinsic::IsUnconstrained => 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 + ), } } @@ -340,6 +345,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. diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 690c0244f62..e07f947db8d 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::{acir::AcirField, FieldElement}; +use acvm::{acir::AcirField, 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/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..5444a86ec82 --- /dev/null +++ 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 new file mode 100644 index 00000000000..015fb1b2555 --- /dev/null +++ b/test_programs/execution_success/regression_5045/src/main.nr @@ -0,0 +1,20 @@ +use dep::std::embedded_curve_ops::EmbeddedCurvePoint; +use dep::std::embedded_curve_ops::EmbeddedCurveScalar; + +fn main(is_active: bool) { + let a = EmbeddedCurvePoint { + x: 0x1d8eb4378a3bde41e0b6a9a8dcbd21b7ff9c51bdd6ca13ce989abbbf90df3666, + y: 0x06075b63354f2504f9cddba0b94ed0cef35fc88615e69ec1f853b51eb79a24a0, + is_infinite: false + }; + + 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( + [a, bad], + [EmbeddedCurveScalar { lo: 1, hi: 0 }, EmbeddedCurveScalar { lo: 1, hi: 0 }] + ); + assert(e[0] != d.x); + } +}