Skip to content

Commit

Permalink
fix: use predicate for curve operations (#5076)
Browse files Browse the repository at this point in the history
# 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.
  • Loading branch information
guipublic authored May 30, 2024
1 parent e73cdbb commit 145b909
Show file tree
Hide file tree
Showing 5 changed files with 83 additions and 2 deletions.
10 changes: 9 additions & 1 deletion compiler/noirc_evaluator/src/ssa/ir/instruction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
),
}
}

Expand Down Expand Up @@ -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.
Expand Down
47 changes: 46 additions & 1 deletion compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -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,
Expand All @@ -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<ValueId, Store>) {
for (address, store) in store_values {
let address = *address;
Expand Down
7 changes: 7 additions & 0 deletions test_programs/execution_success/regression_5045/Nargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "regression_5045"
version = "0.1.0"
type = "bin"
authors = [""]

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
is_active = false
20 changes: 20 additions & 0 deletions test_programs/execution_success/regression_5045/src/main.nr
Original file line number Diff line number Diff line change
@@ -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);
}
}

0 comments on commit 145b909

Please sign in to comment.