From 0f5e1cbbaeb3d278e125aa6b7d2f56f916146d4f Mon Sep 17 00:00:00 2001 From: guipublic Date: Thu, 29 Aug 2024 14:52:52 +0000 Subject: [PATCH 1/8] ec addition for non-zero points --- .../src/ssa/acir_gen/acir_ir/acir_variable.rs | 1 + noir_stdlib/src/embedded_curve_ops.nr | 16 ++++++++++++++++ 2 files changed, 17 insertions(+) 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 317cf43669c..1bd87ae92b5 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 @@ -1439,6 +1439,7 @@ impl AcirContext { | BlackBoxFunc::AND | BlackBoxFunc::XOR | BlackBoxFunc::AES128Encrypt + | BlackBoxFunc::EmbeddedCurveAdd ); // Convert `AcirVar` to `FunctionInput` let inputs = self.prepare_inputs_for_black_box_func_call(inputs, allow_constant_inputs)?; diff --git a/noir_stdlib/src/embedded_curve_ops.nr b/noir_stdlib/src/embedded_curve_ops.nr index 6b70b6ddef0..b8cc3691a38 100644 --- a/noir_stdlib/src/embedded_curve_ops.nr +++ b/noir_stdlib/src/embedded_curve_ops.nr @@ -130,3 +130,19 @@ fn embedded_curve_add( #[foreign(embedded_curve_add)] fn embedded_curve_add_array_return(_point1: EmbeddedCurvePoint, _point2: EmbeddedCurvePoint) -> [Field; 3] {} + +/// Same as embedded_curve_add except that it assumes the inputs and the output are not zero +/// As a result, the performance is 50% better (depending on the backend) +pub fn embedded_curve_add_unsafe( + point1: EmbeddedCurvePoint, + point2: EmbeddedCurvePoint +) -> EmbeddedCurvePoint { + assert(point1.is_infinite == false); + assert(point2.is_infinite == false); + let point_array = embedded_curve_add_array_return(point1, point2); + let x = point_array[0]; + let y = point_array[1]; + assert(point_array[2] == 0); + + EmbeddedCurvePoint { x, y, is_infinite: false } +} From 9e25919a99e098d1cb4fdda6c1b7247b97c9bdc6 Mon Sep 17 00:00:00 2001 From: guipublic Date: Thu, 29 Aug 2024 14:56:01 +0000 Subject: [PATCH 2/8] update comment --- noir_stdlib/src/embedded_curve_ops.nr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/noir_stdlib/src/embedded_curve_ops.nr b/noir_stdlib/src/embedded_curve_ops.nr index b8cc3691a38..5123f7696cd 100644 --- a/noir_stdlib/src/embedded_curve_ops.nr +++ b/noir_stdlib/src/embedded_curve_ops.nr @@ -132,7 +132,7 @@ fn embedded_curve_add( fn embedded_curve_add_array_return(_point1: EmbeddedCurvePoint, _point2: EmbeddedCurvePoint) -> [Field; 3] {} /// Same as embedded_curve_add except that it assumes the inputs and the output are not zero -/// As a result, the performance is 50% better (depending on the backend) +/// As a result, the performance is 50% worse with the 'safe' addition (depending on the backend) pub fn embedded_curve_add_unsafe( point1: EmbeddedCurvePoint, point2: EmbeddedCurvePoint From 3ea62ca11fdf22a09c60784ca417957780ec0b7d Mon Sep 17 00:00:00 2001 From: guipublic Date: Wed, 4 Sep 2024 11:32:55 +0000 Subject: [PATCH 3/8] safe ec_add in Noir --- noir_stdlib/src/embedded_curve_ops.nr | 60 ++++++++++++++++++++------- 1 file changed, 46 insertions(+), 14 deletions(-) diff --git a/noir_stdlib/src/embedded_curve_ops.nr b/noir_stdlib/src/embedded_curve_ops.nr index 5123f7696cd..fcd65aa58a8 100644 --- a/noir_stdlib/src/embedded_curve_ops.nr +++ b/noir_stdlib/src/embedded_curve_ops.nr @@ -113,26 +113,59 @@ pub fn fixed_base_scalar_mul(scalar: EmbeddedCurveScalar) -> EmbeddedCurvePoint multi_scalar_mul([g1], [scalar]) } -// This is a hack as returning an `EmbeddedCurvePoint` from a foreign function in brillig returns a [BrilligVariable::SingleAddr; 2] rather than BrilligVariable::BrilligArray +/// This function only assumes that the points are on the curve +/// It handles corner cases around the infinity point causing some overhead compared to embedded_curve_add_not_nul and embedded_curve_add_unsafe +// This is a hack because returning an `EmbeddedCurvePoint` from a foreign function in brillig returns a [BrilligVariable::SingleAddr; 2] rather than BrilligVariable::BrilligArray // as is defined in the brillig bytecode format. This is a workaround which allows us to fix this without modifying the serialization format. // docs:start:embedded_curve_add -fn embedded_curve_add( - point1: EmbeddedCurvePoint, - point2: EmbeddedCurvePoint -) -> EmbeddedCurvePoint -// docs:end:embedded_curve_add -{ - let point_array = embedded_curve_add_array_return(point1, point2); - let x = point_array[0]; - let y = point_array[1]; - EmbeddedCurvePoint { x, y, is_infinite: point_array[2] == 1 } +pub fn embedded_curve_add(point1: EmbeddedCurvePoint, point2: EmbeddedCurvePoint) -> EmbeddedCurvePoint { + // docs:end:embedded_curve_add + let x_coordinates_match = point1.x == point2.x; + let y_coordinates_match = point1.y == point2.y; + let double_predicate = (x_coordinates_match & y_coordinates_match); + let infinity_predicate = (x_coordinates_match & !y_coordinates_match); + let point1_1 = EmbeddedCurvePoint { x: point1.x + (x_coordinates_match as Field), y: point1.y, is_infinite: false }; + // point1_1 is guaranteed to have a different abscissa than point2 + let mut result = embedded_curve_add_unsafe(point1_1, point2); + result.is_infinite = x_coordinates_match; + + // dbl if x_match, y_match + let double = embedded_curve_add_unsafe(point1, point1); + let mut result = if double_predicate { double } else { result }; + + // infinity if x_match, !y_match + if point1.is_infinite { + result= point2; + } + if point2.is_infinite { + result = point1; + } + let mut result_is_infinity = infinity_predicate & (!point1.is_infinite & !point2.is_infinite); + result.is_infinite = result_is_infinity | (point1.is_infinite & point2.is_infinite); + result.is_infinite = result_is_infinity; + result } #[foreign(embedded_curve_add)] fn embedded_curve_add_array_return(_point1: EmbeddedCurvePoint, _point2: EmbeddedCurvePoint) -> [Field; 3] {} -/// Same as embedded_curve_add except that it assumes the inputs and the output are not zero -/// As a result, the performance is 50% worse with the 'safe' addition (depending on the backend) +/// This function assumes that: +/// The points are on the curve, and +/// The points have not the same abscissa, and +/// Neither point is the infinity point. +/// If it is used with correct input, the function ensures the correct non-zero result is returned. +/// Except for points on the curve, the other assumptions are checked by the function. It will cause assertion failure if they are not respected. +pub fn embedded_curve_add_not_nul( + point1: EmbeddedCurvePoint, + point2: EmbeddedCurvePoint +) -> EmbeddedCurvePoint { + assert(point1.x - point2.x != 0); + embedded_curve_add_unsafe(point1, point2) +} + +/// Unsafe ec addition +/// It assumes (but does not check) that the points have not the same absissa, or that they are the same variable +/// It also assumes neither point is the infinity point. pub fn embedded_curve_add_unsafe( point1: EmbeddedCurvePoint, point2: EmbeddedCurvePoint @@ -142,7 +175,6 @@ pub fn embedded_curve_add_unsafe( let point_array = embedded_curve_add_array_return(point1, point2); let x = point_array[0]; let y = point_array[1]; - assert(point_array[2] == 0); EmbeddedCurvePoint { x, y, is_infinite: false } } From 05aeecb057e640ae49bddae1d2c0303b79160e16 Mon Sep 17 00:00:00 2001 From: guipublic Date: Wed, 4 Sep 2024 16:51:39 +0000 Subject: [PATCH 4/8] fix issues found by integration tests --- noir_stdlib/src/embedded_curve_ops.nr | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/noir_stdlib/src/embedded_curve_ops.nr b/noir_stdlib/src/embedded_curve_ops.nr index fcd65aa58a8..470468e6b25 100644 --- a/noir_stdlib/src/embedded_curve_ops.nr +++ b/noir_stdlib/src/embedded_curve_ops.nr @@ -124,14 +124,14 @@ pub fn embedded_curve_add(point1: EmbeddedCurvePoint, point2: EmbeddedCurvePoint let y_coordinates_match = point1.y == point2.y; let double_predicate = (x_coordinates_match & y_coordinates_match); let infinity_predicate = (x_coordinates_match & !y_coordinates_match); - let point1_1 = EmbeddedCurvePoint { x: point1.x + (x_coordinates_match as Field), y: point1.y, is_infinite: false }; + let point1_1 = EmbeddedCurvePoint { x: point1.x + (x_coordinates_match as Field), y: point1.y, is_infinite: x_coordinates_match }; // point1_1 is guaranteed to have a different abscissa than point2 let mut result = embedded_curve_add_unsafe(point1_1, point2); result.is_infinite = x_coordinates_match; // dbl if x_match, y_match let double = embedded_curve_add_unsafe(point1, point1); - let mut result = if double_predicate { double } else { result }; + result = if double_predicate { double } else { result }; // infinity if x_match, !y_match if point1.is_infinite { @@ -142,7 +142,6 @@ pub fn embedded_curve_add(point1: EmbeddedCurvePoint, point2: EmbeddedCurvePoint } let mut result_is_infinity = infinity_predicate & (!point1.is_infinite & !point2.is_infinite); result.is_infinite = result_is_infinity | (point1.is_infinite & point2.is_infinite); - result.is_infinite = result_is_infinity; result } @@ -160,6 +159,8 @@ pub fn embedded_curve_add_not_nul( point2: EmbeddedCurvePoint ) -> EmbeddedCurvePoint { assert(point1.x - point2.x != 0); + assert(point1.is_infinite == false); + assert(point2.is_infinite == false); embedded_curve_add_unsafe(point1, point2) } @@ -170,8 +171,6 @@ pub fn embedded_curve_add_unsafe( point1: EmbeddedCurvePoint, point2: EmbeddedCurvePoint ) -> EmbeddedCurvePoint { - assert(point1.is_infinite == false); - assert(point2.is_infinite == false); let point_array = embedded_curve_add_array_return(point1, point2); let x = point_array[0]; let y = point_array[1]; From 98ed35580d9e57596fda36c00d315f0fe10acd39 Mon Sep 17 00:00:00 2001 From: guipublic Date: Thu, 19 Sep 2024 08:32:03 +0000 Subject: [PATCH 5/8] nargo fmt --- noir_stdlib/src/embedded_curve_ops.nr | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/noir_stdlib/src/embedded_curve_ops.nr b/noir_stdlib/src/embedded_curve_ops.nr index 2eface706fa..30a354a8413 100644 --- a/noir_stdlib/src/embedded_curve_ops.nr +++ b/noir_stdlib/src/embedded_curve_ops.nr @@ -151,10 +151,7 @@ fn embedded_curve_add_array_return(_point1: EmbeddedCurvePoint, _point2: Embedde /// Neither point is the infinity point. /// If it is used with correct input, the function ensures the correct non-zero result is returned. /// Except for points on the curve, the other assumptions are checked by the function. It will cause assertion failure if they are not respected. -pub fn embedded_curve_add_not_nul( - point1: EmbeddedCurvePoint, - point2: EmbeddedCurvePoint -) -> EmbeddedCurvePoint { +pub fn embedded_curve_add_not_nul(point1: EmbeddedCurvePoint, point2: EmbeddedCurvePoint) -> EmbeddedCurvePoint { assert(point1.x - point2.x != 0); assert(point1.is_infinite == false); assert(point2.is_infinite == false); From 2d2b4936ff01897dee6629f629b083638e0a6159 Mon Sep 17 00:00:00 2001 From: guipublic Date: Fri, 20 Sep 2024 12:18:59 +0000 Subject: [PATCH 6/8] code review --- noir_stdlib/src/embedded_curve_ops.nr | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/noir_stdlib/src/embedded_curve_ops.nr b/noir_stdlib/src/embedded_curve_ops.nr index 6576dddd5d0..445b6d96554 100644 --- a/noir_stdlib/src/embedded_curve_ops.nr +++ b/noir_stdlib/src/embedded_curve_ops.nr @@ -162,24 +162,23 @@ fn embedded_curve_add_array_return(_point1: EmbeddedCurvePoint, _point2: Embedde /// This function assumes that: /// The points are on the curve, and -/// The points have not the same abscissa, and +/// The points don't share an x-coordinate, and /// Neither point is the infinity point. /// If it is used with correct input, the function ensures the correct non-zero result is returned. /// Except for points on the curve, the other assumptions are checked by the function. It will cause assertion failure if they are not respected. pub fn embedded_curve_add_not_nul(point1: EmbeddedCurvePoint, point2: EmbeddedCurvePoint) -> EmbeddedCurvePoint { - assert(point1.x - point2.x != 0); - assert(point1.is_infinite == false); - assert(point2.is_infinite == false); + assert(point1.x != point2.x); + assert(!point1.is_infinite); + assert(!point2.is_infinite); embedded_curve_add_unsafe(point1, point2) } /// Unsafe ec addition -/// It assumes (but does not check) that the points have not the same absissa, or that they are the same variable +/// If the inputs are the same, it will perform a doubling, but only if point1 and point2 are the same variable. +/// If they have the same value but are different variables, the result will be incorrect because in this case +/// it assumes (but does not check) that the points' x-coordinates are not equal. /// It also assumes neither point is the infinity point. -pub fn embedded_curve_add_unsafe( - point1: EmbeddedCurvePoint, - point2: EmbeddedCurvePoint -) -> EmbeddedCurvePoint { +pub fn embedded_curve_add_unsafe(point1: EmbeddedCurvePoint, point2: EmbeddedCurvePoint) -> EmbeddedCurvePoint { let point_array = embedded_curve_add_array_return(point1, point2); let x = point_array[0]; let y = point_array[1]; From f4506959d119d12582095f55733dfb8093136ab9 Mon Sep 17 00:00:00 2001 From: guipublic Date: Fri, 20 Sep 2024 12:23:50 +0000 Subject: [PATCH 7/8] document ec add opcode --- acvm-repo/acir/src/circuit/black_box_functions.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/acvm-repo/acir/src/circuit/black_box_functions.rs b/acvm-repo/acir/src/circuit/black_box_functions.rs index fb7d9eb584c..9f07a131ed3 100644 --- a/acvm-repo/acir/src/circuit/black_box_functions.rs +++ b/acvm-repo/acir/src/circuit/black_box_functions.rs @@ -164,7 +164,15 @@ pub enum BlackBoxFunc { /// ultimately fail. RecursiveAggregation, - /// Addition over the embedded curve on which the witness is defined. + /// Addition over the embedded curve on which the witness is defined + /// The opcode makes the following assumptions but does not enforce them because + /// it is more efficient to do it only when required. For instance, adding two + /// points that are on the curve it guarantee to give a point on the curve. + /// + /// It assumes that the points are on the curve. + /// If the inputs are the same witnesses index, it will perform a doubling, + /// If not, it assumes that the points' x-coordinates are not equal. + /// It also assumes neither point is the infinity point. EmbeddedCurveAdd, /// BigInt addition From 430980ba7c62f64d34ebfecb374b7f51dc3ae086 Mon Sep 17 00:00:00 2001 From: guipublic Date: Fri, 20 Sep 2024 12:26:40 +0000 Subject: [PATCH 8/8] cargo fmt --- acvm-repo/acir/src/circuit/black_box_functions.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acvm-repo/acir/src/circuit/black_box_functions.rs b/acvm-repo/acir/src/circuit/black_box_functions.rs index 9f07a131ed3..6639f1fbe71 100644 --- a/acvm-repo/acir/src/circuit/black_box_functions.rs +++ b/acvm-repo/acir/src/circuit/black_box_functions.rs @@ -168,7 +168,7 @@ pub enum BlackBoxFunc { /// The opcode makes the following assumptions but does not enforce them because /// it is more efficient to do it only when required. For instance, adding two /// points that are on the curve it guarantee to give a point on the curve. - /// + /// /// It assumes that the points are on the curve. /// If the inputs are the same witnesses index, it will perform a doubling, /// If not, it assumes that the points' x-coordinates are not equal.