Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: added support for comparison operators #86

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion src/bignum.nr
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
use std::cmp::Ordering;
use crate::utils::map::map;

use crate::params::BigNumParamsGetter;

use crate::fns::{
constrained_ops::{
add, assert_is_not_equal, conditional_select, derive_from_seed, div, eq, mul, neg, sub,
udiv, udiv_mod, umod, validate_in_field, validate_in_range,
udiv, udiv_mod, umod, validate_in_field, validate_in_range, cmp,
}, expressions::{__compute_quadratic_expression, evaluate_quadratic_expression},
serialization::{from_be_bytes, to_le_bytes},
unconstrained_ops::{
Expand Down Expand Up @@ -373,3 +374,20 @@ where
}
}

impl<let N: u32, let MOD_BITS: u32, Params> std::cmp::Ord for BigNum<N, MOD_BITS, Params>
where
Params: BigNumParamsGetter<N, MOD_BITS>,
{
fn cmp(self, other: Self) -> Ordering {
let comp_result: bool = cmp::<_, MOD_BITS>(self.limbs, other.limbs);
if self.limbs == other.limbs {
Ordering::equal()
} else if comp_result {
// there was an underflow
Ordering::less()
} else {
// there was no underflow
Ordering::greater()
}
}
}
54 changes: 53 additions & 1 deletion src/fns/constrained_ops.nr
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::fns::{
expressions::evaluate_quadratic_expression,
unconstrained_helpers::{
__add_with_flags, __neg_with_flags, __sub_with_flags, __validate_gt_remainder,
__validate_in_field_compute_borrow_flags,
__validate_in_field_compute_borrow_flags, __cmp_remainder,
}, unconstrained_ops::{__div, __mul, __udiv_mod},
};

Expand Down Expand Up @@ -294,6 +294,58 @@ pub(crate) fn validate_gt<let N: u32, let MOD_BITS: u32>(lhs: [Field; N], rhs: [
assert(result_limb == 0);
}

pub(crate) fn cmp<let N: u32, let MOD_BITS: u32>(lhs: [Field; N], rhs: [Field; N]) -> bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not return an Ordering from this function?

// so we do... p - x - r = 0 and there might be borrow flags
// a - b = r
// p + a - b - r = 0
let (comp_result, result, carry_flags, borrow_flags) = unsafe { __cmp_remainder(lhs, rhs) };
validate_in_range::<_, MOD_BITS>(result);

let borrow_shift = 0x1000000000000000000000000000000;
let carry_shift = 0x1000000000000000000000000000000;
Comment on lines +340 to +341
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the same value so why not have a single constant? Also this should be named to make it clear how big they are.


let mut addend: [Field; N] = [0; N];
if comp_result == true {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was first thinking about skipping the if statement by casting the bool to Field and then computing intended_lhs = (1- comp_result as Field)* lhs + comp_result * rhs but not sure whether looping over the limbs and doing field multiplications would be any better than a conditional.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you just want to swap the lhs and rhs then you can do

let (lhs, rhs) = if comp_result {
  (rhs, lhs)
} else {
  (lhs, rhs)
}

This would end up with needing to merge two [Field;N] values but that's more efficient than duplicating the logic currently in the if-statement. (This could be made better by just returning the element-wise difference between the two arrays rather than directly reordering them)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The best thing to do in this situation however would be to integrate the benchmarking system I added to https://github.com/noir-lang/noir-library-starter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks I was wondering if there's a way to do a swap in a single line :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need to reduce this copy-pasting.

let result_limb = rhs[0] - lhs[0] + addend[0] - result[0] - 1
+ (borrow_flags[0] as Field * borrow_shift)
- (carry_flags[0] as Field * carry_shift);
assert(result_limb == 0);

for i in 1..N - 1 {
let result_limb = rhs[i] - lhs[i] + addend[i] - result[i] - borrow_flags[i - 1] as Field
+ carry_flags[i - 1] as Field
+ ((borrow_flags[i] as Field - carry_flags[i] as Field) * borrow_shift);
assert(result_limb == 0);
}

let result_limb = rhs[N - 1] - lhs[N - 1] + addend[N - 1]
- result[N - 1]
- borrow_flags[N - 2] as Field
+ carry_flags[N - 2] as Field;
assert(result_limb == 0);
true
} else {
let result_limb = lhs[0] - rhs[0] + addend[0] - result[0] - 1
+ (borrow_flags[0] as Field * borrow_shift)
- (carry_flags[0] as Field * carry_shift);
assert(result_limb == 0);

for i in 1..N - 1 {
let result_limb = lhs[i] - rhs[i] + addend[i] - result[i] - borrow_flags[i - 1] as Field
+ carry_flags[i - 1] as Field
+ ((borrow_flags[i] as Field - carry_flags[i] as Field) * borrow_shift);
assert(result_limb == 0);
}

let result_limb = lhs[N - 1] - rhs[N - 1] + addend[N - 1]
- result[N - 1]
- borrow_flags[N - 2] as Field
+ carry_flags[N - 2] as Field;
assert(result_limb == 0);
false
}
}

pub(crate) fn neg<let N: u32, let MOD_BITS: u32>(
params: P<N, MOD_BITS>,
val: [Field; N],
Expand Down
48 changes: 48 additions & 0 deletions src/fns/unconstrained_helpers.nr
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,54 @@ pub(crate) unconstrained fn __validate_gt_remainder<let N: u32>(
(result, carry_flags, borrow_flags)
}

pub(crate) unconstrained fn __cmp_remainder<let N: u32>(
lhs: [Field; N],
rhs: [Field; N],
) -> (bool, [Field; N], [bool; N], [bool; N]) {
let mut a_u60: U60Repr<N, 2> = U60Repr::from(lhs);
let mut b_u60: U60Repr<N, 2> = U60Repr::from(rhs);
let mut temp_u60: U60Repr<N, 2> = U60Repr::one();
let underflow = b_u60.gte(a_u60);

if underflow {
temp_u60 = a_u60;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wonder how bad this is in brillig @TomAFrench. should I do what I did in the constrained version and just duplicate the code?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you not refactor this function to have a private implementation which accepts two U60Reprs?

pub(crate) unconstrained fn __cmp_remainder<let N: u32>(
    lhs: [Field; N],
    rhs: [Field; N],
) -> (bool, [Field; N], [bool; N], [bool; N]) {
    let mut a_u60: U60Repr<N, 2> = U60Repr::from(lhs);
    let mut b_u60: U60Repr<N, 2> = U60Repr::from(rhs);
    let mut temp_u60: U60Repr<N, 2> = U60Repr::one();
    let underflow = b_u60.gte(a_u60);

    if underflow {
        __cmp_remainder_priv(b_u60, a_u60);
    } else {
        __cmp_remainder_priv(a_u60, b_u60);
    }

Again, benchmarking would be way to make these decisions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep I can make a private function that does the second part (computing carries etc). will do that now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did the refactoring. I think we can refactor it a lot here. these operations are done over and over in all the with_flags functions.

a_u60 = b_u60;
b_u60 = temp_u60;
}
b_u60 += U60Repr::one();

let mut result_u60: U60Repr<N, 2> = U60Repr { limbs: [0; 2 * N] };

let mut carry_in: u64 = 0;
let mut borrow_in: u64 = 0;
let mut borrow_flags: [bool; N] = [false; N];
let mut carry_flags: [bool; N] = [false; N];
for i in 0..2 * N {
let mut add_term: u64 = a_u60.limbs[i] + carry_in;
let mut carry = (add_term >= TWO_POW_60) as u64;
add_term -= carry * TWO_POW_60;
carry_in = carry;

let sub_term = b_u60.limbs[i] + borrow_in;
let mut borrow = (sub_term > add_term) as u64;
result_u60.limbs[i] = borrow * TWO_POW_60 + add_term - sub_term;

borrow_in = borrow;

if ((i & 1) == 1) {
if (carry & borrow == 1) {
carry = 0;
borrow = 0;
}
carry_flags[i / 2] = carry as bool;
borrow_flags[i / 2] = borrow as bool;
}
}
let result = U60Repr::into(result_u60);
println(underflow);
(underflow, result, carry_flags, borrow_flags)
}

pub(crate) unconstrained fn __neg_with_flags<let N: u32, let MOD_BITS: u32>(
params: P<N, MOD_BITS>,
val: [Field; N],
Expand Down
20 changes: 19 additions & 1 deletion src/runtime_bignum.nr
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
use crate::params::BigNumParams;
use crate::utils::map::map;
use std::cmp::Ordering;

use crate::fns::{
constrained_ops::{
add, assert_is_not_equal, conditional_select, derive_from_seed, div, eq, mul, neg, sub,
udiv, udiv_mod, umod, validate_in_field, validate_in_range,
udiv, udiv_mod, umod, validate_in_field, validate_in_range, cmp,
}, expressions::{__compute_quadratic_expression, evaluate_quadratic_expression},
serialization::{from_be_bytes, to_le_bytes},
unconstrained_ops::{
Expand Down Expand Up @@ -438,3 +439,20 @@ impl<let N: u32, let MOD_BITS: u32> std::cmp::Eq for RuntimeBigNum<N, MOD_BITS>
eq::<_, MOD_BITS>(params, self.limbs, other.limbs)
}
}

impl<let N: u32, let MOD_BITS: u32> Ord for RuntimeBigNum<N, MOD_BITS> {
fn cmp(self, other: Self) -> std::cmp::Ordering {
let params = self.params;
assert(params == other.params);
let comp_result: bool = cmp::<_, MOD_BITS>(self.limbs, other.limbs);
if self.limbs == other.limbs {
Ordering::equal()
} else if comp_result {
// there was an underflow
Ordering::less()
} else {
// there was no underflow
Ordering::greater()
}
}
}
43 changes: 43 additions & 0 deletions src/tests/bignum_test.nr
Original file line number Diff line number Diff line change
Expand Up @@ -790,3 +790,46 @@ fn test_expressions() {
assert(wx_constrained.limbs == wx.limbs);
}

#[test]
fn test_cmp_BN() {
let mut a: Fq = BigNum::modulus();
let mut b: Fq = BigNum::modulus();

a.limbs[0] -= 2;
b.limbs[0] -= 1;

assert(a < b);
}

#[test(should_fail_with = "Failed constraint")]
fn test_cmp_BN_fail() {
let mut a: Fq = BigNum::modulus();
let mut b: Fq = BigNum::modulus();

a.limbs[0] -= 1;
b.limbs[0] -= 2;

assert(a < b);
}

#[test]
fn test_cmp_BN_2() {
let mut a: Fq = BigNum::modulus();
let mut b: Fq = BigNum::modulus();

a.limbs[0] -= 1;
b.limbs[0] -= 2;

assert(a > b);
}

#[test(should_fail_with = "Failed constraint")]
fn test_cmp_BN_fail_2() {
let mut a: Fq = BigNum::modulus();
let mut b: Fq = BigNum::modulus();

a.limbs[0] -= 2;
b.limbs[0] -= 1;

assert(a > b);
}
Loading