feat: add comparison operators #130

kashbrti · 2025-02-18T14:57:57Z

Description

resolves #112

Problem*

Resolves

Summary*

Additional Context

PR Checklist*

I have tested the changes locally.
I have formatted the changes with Prettier and/or cargo fmt on default settings.

kashbrti · 2025-02-18T14:59:00Z

src/fns/constrained_ops.nr

    validate_in_range::<_, MOD_BITS>(result);
-


moved the constraining of the output of __validate_gt_remainder to a separate function called check_gt_with_flags.

kashbrti · 2025-02-18T14:59:42Z

src/fns/unconstrained_helpers.nr

@@ -41,14 +41,19 @@ pub(crate) unconstrained fn __validate_in_field_compute_borrow_flags<let N: u32,
 pub(crate) unconstrained fn __validate_gt_remainder<let N: u32>(


__validate_gt_remainder now also returns the boolean value of the underflow.

the lhs and rhs are swapped if the underflow is true, and the rest of the logic stays the same

github-actions · 2025-02-18T15:08:30Z

Changes to circuit sizes

Generated at commit: 2e3b5b391a07ed65a734826fb41a28315007fb42, compared to commit: 1e9bd8a8b9a5993ce252bb60fc60fee40d0efd87

🧾 Summary (10% most significant diffs)

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
bench_from_field_Fq.json	+1 ❌	+4.76%	+1 ❌	+0.04%
bench_from_field_BN256.json	+1 ❌	+4.76%	+1 ❌	+0.04%

Full diff report 👇

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
bench_from_field_Fq.json	22 (+1)	+4.76%	2,834 (+1)	+0.04%
bench_from_field_BN256.json	22 (+1)	+4.76%	2,840 (+1)	+0.04%

iAmMichaelConnor

Thanks for delving into this code! Just 3 minor comments :)

iAmMichaelConnor · 2025-02-19T19:51:19Z

src/fns/constrained_ops.nr

@@ -572,3 +557,41 @@ pub(crate) fn umod<let N: u32, let MOD_BITS: u32>(
 ) -> [Field; N] {
    udiv_mod::<_, MOD_BITS>(params, numerator, divisor).1
 }
+
+// a comparison function. returns true if lhs > rhs and false otherwise
+pub(crate) fn cmp<let N: u32, let MOD_BITS: u32>(lhs: [Field; N], rhs: [Field; N]) -> bool {


I'm not sure what the convention should be, but it looks like cmp functions in the noir stdlib always return the Ordering type. Perhaps, therefore, this function should be renamed to something like gt?

if you check bignum.nr I have an impl for the Cmp trait that returns an ordering object.

Oh, sorry, yes I saw that one. I just mean for this function in particular, perhaps its name should be changed to something that isn't cmp.

iAmMichaelConnor · 2025-02-19T19:51:24Z

src/tests/bignum_test.nr

+}
+
+#[test]
+fn test_cmp_BN_2() {


Perhaps it's also worth testing other common edge cases:

0 < 1

0 < -1
And if you're feeling adventurous:

Choose lots of random bignums r and s check that if r < s then r - (p-1)/2 > s

The reason I suggest something like this is because these current tests only test limbs[0] being different; what if the other limbs aren't equal?

Actually, I've written 0 < -1... but I don't actually know what the convention should be for comparison.
I think in the Aztec codebase, we consider the "first half" of the field as "positive" numbers, and the 2nd half of the field as "negative" numbers. So we check whether things are < (p-1)/2.

For the U256 type, regular integer comparison makes sense.
For the prime field types, I'm less sure...

ah that's a good point, at the moment everything is being treated as usual integers between 0 and modulus. I think it'll be quite ugly to have two different impl depending on whether a number is positive or negative.

Sure, being consistent makes sense to me!

iAmMichaelConnor · 2025-02-19T19:52:19Z

src/bignum.nr

+where
+    Params: BigNumParamsGetter<N, MOD_BITS>,
+{
+    fn cmp(self, other: Self) -> Ordering {


Should we also implement this for the RuntimeBignum?

yep will do.

src/fns/constrained_ops.nr

Co-authored-by: Michael Connor <[email protected]>

kashbrti · 2025-02-25T17:50:53Z

@iAmMichaelConnor this should be at a good place for a review!

TomAFrench

Nits but LGTM

TomAFrench · 2025-03-03T12:20:28Z

src/tests/bignum_test.nr

@@ -921,3 +921,56 @@ fn test_sub_underflow_regression() {
    let c = b - a;
    assert(c.limbs == [0, 0, 0]);
 }
+
+#[test]
+fn test_cmp_BN() {


Can you move these all inside of a submodule grouping all of the tests related to cmp?

TomAFrench · 2025-03-03T12:22:56Z

src/tests/bignum_test.nr

+}
+
+#[test]
+fn test_cmp_BN_fuzz(seed: [u8; 2]) {


Suggested change

fn test_cmp_BN_fuzz(seed: [u8; 2]) {

fn test_cmp_BN_fuzz(seed: [u8; 5]) {

There's currently only ~65k potential inputs here.

TomAFrench · 2025-03-03T12:27:46Z

src/fns/constrained_ops.nr

@@ -27,6 +27,8 @@ use crate::params::BigNumParams as P;
 * udiv_mod
 * udiv
 * umod
+ * gt 
+ * check_gt_with_flags
 */


Let's just remove this comment. It's pretty meaningless.

TomAFrench · 2025-03-03T12:29:49Z

src/bignum.nr

+        let comp_result: bool = gt::<_, MOD_BITS>(self.limbs, other.limbs);
+        if self.limbs == other.limbs {
+            Ordering::equal()
+        } else if comp_result {
+            // there was an underflow
+            Ordering::greater()
+        } else {
+            // there was no underflow
+            Ordering::less()
+        }
+    }


Can we push a cmp function into constrained_ops? We can then avoid the duplicated logic between here and RuntimeBigNum.

Khashayar Barooti added 4 commits February 18, 2025 11:09

Merge branch 'main' of github.com:noir-lang/noir-bignum

189e9c6

Merge branch 'main' of github.com:noir-lang/noir-bignum

b18a377

added comparison operators

448489b

formatter

f93ce42

kashbrti commented Feb 18, 2025

View reviewed changes

fixed the wrong failure name

7b4042e

kashbrti mentioned this pull request Feb 18, 2025

feat: added support for comparison operators #86

Closed

2 tasks

added generics that the compiler was confused about

9513b08

kashbrti marked this pull request as ready for review February 18, 2025 15:24

iAmMichaelConnor reviewed Feb 19, 2025

View reviewed changes

iAmMichaelConnor reviewed Feb 20, 2025

View reviewed changes

src/fns/constrained_ops.nr Outdated Show resolved Hide resolved

kashbrti and others added 7 commits February 20, 2025 10:37

Update src/fns/constrained_ops.nr

ec6ff3c

Co-authored-by: Michael Connor <[email protected]>

merged main into branch and added comp for Runtime_bignum

87379f5

changed the name of the constrained fn for comparisons to gt

c558290

formatter

84636f5

added fuzz test for comparison

cf28db2

changed the test to work with even modulus

3bcbcd2

made the test work for non-prime moduli

362e408

TomAFrench approved these changes Mar 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add comparison operators #130

feat: add comparison operators #130

kashbrti commented Feb 18, 2025 •

edited

Loading

kashbrti Feb 18, 2025

kashbrti Feb 18, 2025

kashbrti Feb 18, 2025

github-actions bot commented Feb 18, 2025 •

edited

Loading

iAmMichaelConnor left a comment

iAmMichaelConnor Feb 19, 2025

kashbrti Feb 20, 2025

iAmMichaelConnor Feb 20, 2025

iAmMichaelConnor Feb 19, 2025 •

edited

Loading

iAmMichaelConnor Feb 19, 2025

iAmMichaelConnor Feb 19, 2025

kashbrti Feb 20, 2025

iAmMichaelConnor Feb 20, 2025

iAmMichaelConnor Feb 19, 2025

kashbrti Feb 20, 2025

kashbrti commented Feb 25, 2025

TomAFrench left a comment

TomAFrench Mar 3, 2025

TomAFrench Mar 3, 2025

TomAFrench Mar 3, 2025

TomAFrench Mar 3, 2025

		@@ -41,14 +41,19 @@ pub(crate) unconstrained fn __validate_in_field_compute_borrow_flags<let N: u32,
		pub(crate) unconstrained fn __validate_gt_remainder<let N: u32>(

	fn test_cmp_BN_fuzz(seed: [u8; 2]) {
	fn test_cmp_BN_fuzz(seed: [u8; 5]) {

feat: add comparison operators #130

Are you sure you want to change the base?

feat: add comparison operators #130

Conversation

kashbrti commented Feb 18, 2025 • edited Loading

Description

Problem*

Summary*

Additional Context

PR Checklist*

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 18, 2025 • edited Loading

Changes to circuit sizes

🧾 Summary (10% most significant diffs)

iAmMichaelConnor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iAmMichaelConnor Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kashbrti commented Feb 25, 2025

TomAFrench left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kashbrti commented Feb 18, 2025 •

edited

Loading

github-actions bot commented Feb 18, 2025 •

edited

Loading

iAmMichaelConnor Feb 19, 2025 •

edited

Loading