Fix and finite field operations (isSquare, sqrt) #287

MartinMinkov · 2024-07-22T21:09:16Z

Description

This pull request addresses issues and has a slight optimization change in the finite field operations.

The following tasks were accomplished:

Fixed isSquare and sqrt methods to ensure input is taken modulo p
Optimized the equal method to avoid unnecessary modulo operations

Additional Information

These changes ensure that the finite field operations work correctly for all input values and improve performance by reducing unnecessary computations. The optimization in the equal method is particularly beneficial for frequently used operations.

It also addresses the issue found in the audit where with this code:

import { createForeignField } from 'o1js';

class Field17 extends createForeignField(17n) {}

// This works fine
console.log(Field17.Bigint.sqrt(0n));
// this runs forever  normally --- with the changes it prints 0n
console.log(Field17.Bigint.sqrt(17n));

mitschabaude

some comments!

comment on performance: canonicalize() is more efficient in the case its input is already canonical (in that case, it avoids the mod() call thanks to the inequality checks)
it's less efficient in the case the input needs to be reduced.

overall, I think this trade-off is worth it, so I support changing canonicalize() to mod() all over the field module even though they do the same!

mitschabaude · 2024-07-23T08:38:48Z

crypto/finite-field.ts

    },
    fromBigint(x: bigint) {
-      return mod(x, p);
+      return canonicalize(x);
    },
    rot(


rot(), leftShift() and rightShift() are not finite field operations, and reducing modulo the field size makes no sense here -- so please remove canonicalize() in these functions (they shouldn't have been put into finite-field.ts in the first place)

Fixed! 145a928

mitschabaude · 2024-07-23T08:43:39Z

crypto/finite-field.ts

    },
    power(x: bigint, n: bigint) {
-      return power(x, n, p);
+      return canonicalize(power(x, n, p));


power already returns a reduced result, can you remove canonicalize() here?

mitschabaude · 2024-07-23T08:45:07Z

crypto/finite-field.ts

    },
    isSquare(x: bigint) {
-      return isSquare(x, p);
+      return isSquare(canonicalize(x), p);


this step should be part of the isSquare() function -- can you do mod(., p) on the input there and remove canonicalize() here?

Fixed! 145a928

mitschabaude · 2024-07-23T08:45:56Z

crypto/finite-field.ts

+      const result = sqrt(
+        canonicalize(x),


this step seems to fix a bug and so is an important part of the sqrt() function -- can you do mod(., p) on the input there and remove canonicalize() here?

Fixed! 145a928

mitschabaude · 2024-07-23T08:46:58Z

crypto/finite-field.ts

+        twoadicRoot,
+        twoadicity
+      );
+      return result !== undefined ? canonicalize(result) : undefined;


the result returned by sqrt() is already reduced so please remove canonicalize() here

(i.e. you can return sqrt(...) directly)

mitschabaude · 2024-07-23T08:49:23Z

crypto/finite-field.ts

    },
    dot(x: bigint[], y: bigint[]) {
      let z = 0n;
      let n = x.length;
      for (let i = 0; i < n; i++) {
-        z += x[i] * y[i];
+        z += canonicalize(x[i]) * canonicalize(y[i]);


this is not needed, it's handled by the final call to mod() / canonicalize() before returning

(in fact, I introduced the dot() method precisely to save calls to mod() compared to just using multiplication and addition, for performance of Poseidon)

mitschabaude · 2024-07-23T08:51:37Z

crypto/finite-field.ts

    },
    equal(x: bigint, y: bigint) {
-      return mod(x - y, p) === 0n;
+      return canonicalize(x) === canonicalize(y);


at first glance I thought this is less efficient than the original, but since inputs are typically canonical, it should actually be more efficient since we never have to mod()!

Removed the canonical function and just replaced with inline checks to mod here! 145a928

mitschabaude · 2024-07-23T08:52:45Z

crypto/finite-field.ts

    },
    random() {
-      return randomField(p, sizeInBytes, hiBitMask);
+      return canonicalize(randomField(p, sizeInBytes, hiBitMask));


randomField() already returns a canonical result, so please remove canonicalize() here

Trivo25 · 2024-07-23T08:53:43Z

crypto/finite-field.ts

+   * {@link sqrt()}, and others. By using this function, we ensure consistent behavior
+   * across all field operations regardless of the input's initial representation.
+   */
+  function canonicalize(x: bigint) {


I actually don't see how this is different to mod(x, p) (except performance in some particular cases) and why we should replacemod with this new function - we could simply check inputs (and outputs) by calling mod(x, p) to assert that they are canonical, like this:

- return sqrt(x, p, oddFactor, twoadicRoot, twoadicity); + return sqrt(mod(x, p), p, oddFactor, twoadicRoot, twoadicity);

and use canonicalize for performance critical parts

My original thinking was that by introducing checks in certain methods and not others, it could lead to a similar situation in the future. By blanket applying a function to alter the output to all of these methods, we'd have a way similar way for all methods to move forward on. This turns out not to be a favourable choice though!

I have reverted the changes and just applied mod to the inputs of the offending functions that cause the bugs outlined in the script above.

fix(finite-field): optimize equal method by avoiding modulo operation when possible refactor(finite-field): use more descriptive variable names in equal method The isSquare and sqrt methods were not taking the input modulo p, which could lead to incorrect results for inputs outside the field. The equal method was performing a modulo operation unnecessarily in some cases, which could be avoided by first checking if the inputs are already in the correct range. The variable names in the equal method were also improved for clarity.

… and isSquare methods The commit adds two new test cases to the finite field unit tests: 1. It asserts that calling `sqrt` with a non-canonical zero (i.e., `p`) should return the same result as calling it with a canonical zero (`0n`). This ensures that the `sqrt` method handles non-canonical zero inputs correctly. 2. It asserts that calling `isSquare` with a non-canonical zero (`p`) should return the same result as calling it with a canonical zero (`0n`). This ensures that the `isSquare` method treats non-canonical zero inputs the same way as canonical zero. These tests were added to improve the robustness of the finite field implementation by verifying that it correctly handles non-canonical zero values, which are equivalent to the canonical zero value in the field.

Trivo25 · 2024-07-30T08:56:28Z

crypto/finite-field.ts

@@ -317,7 +317,9 @@ function createField(
      return mod(z, p);
    },
    equal(x: bigint, y: bigint) {
-      return mod(x - y, p) === 0n;
+      let x_ = x >= 0n && x < p ? x : mod(x, p);


I think I am okay with this, but let's add a comment here explaining what and why we do it this particular way, i can imagine it might seem a little weird to the naked eye

fixed here! 71f2e69

mitschabaude · 2024-07-30T09:41:51Z

crypto/finite-field.ts

    isSquare(x: bigint) {
-      return isSquare(x, p);
+      return isSquare(mod(x, p), p);
    },
    sqrt(x: bigint) {
-      return sqrt(x, p, oddFactor, twoadicRoot, twoadicity);
+      return sqrt(mod(x, p), p, oddFactor, twoadicRoot, twoadicity);


so what I meant in my other review here is that I'd like those changes to be made to the original sqrt and isSquare functions defined at the top-level of this file. just so that we don't have two versions of sqrt(), one with a bug and another that fixes the bug

I know that currently the original sqrt() / isSquare() are not even exported directly, but other basic functions are, and if someone decides to export sqrt() directly in the future they probably won't think of fixing its bug

fixed here! cc3f632

mitschabaude

the following two methods also need mod() on their inputs to always behave correctly:

negate() (can just do mod(-x, p))
isEven()

…stead of p - x for improved readability and consistency with other functions fix(finite-field): use mod in isEven function to ensure the result is always within the field range and avoid potential issues with large numbers

…g input to ensure it's in the field The changes in the `sqrt` and `isSquare` functions were made to ensure that the input value is always within the finite field before performing any operations. This is done by modding the input value by the field's prime modulus `p` at the beginning of each function. The reason for this change is to handle cases where the input value might be outside the field, which could lead to incorrect results or even errors. By modding the input, we guarantee that all operations are performed on values within the field, ensuring the correctness of the results. This refactoring improves the robustness and reliability of the `sqrt` and `isSquare` functions, making them more consistent with the expected behavior of finite field arithmetic.

- Assert that negating non-canonical zero (p) returns canonical zero (0n) - Assert that non-canonical zero (p) is considered even These additional test cases ensure that the finite field implementation handles non-canonical zero correctly by treating it as equivalent to the canonical zero representation.

…the number modulo p before checking the least significant bit This fixes a bug where the isEven function would incorrectly classify numbers greater than the field size as even. By first reducing the number modulo p, we ensure that the least significant bit correctly determines the parity of the number within the field.

… method implementation

mitschabaude

sweet!

MartinMinkov changed the title ~~feat(finite-field): add canonicalize function~~ Add canonicalize function for finite-field.ts Jul 22, 2024

MartinMinkov mentioned this pull request Jul 22, 2024

Fix and finite field operations in bindings/crypto module o1-labs/o1js#1765

Merged

MartinMinkov marked this pull request as ready for review July 22, 2024 21:15

MartinMinkov requested review from Trivo25, mitschabaude, ymekuria and shimkiv July 22, 2024 21:34

mitschabaude reviewed Jul 23, 2024

View reviewed changes

Trivo25 reviewed Jul 23, 2024

View reviewed changes

MartinMinkov force-pushed the audit/field-curve-canon branch from 1d0cb94 to 145a928 Compare July 23, 2024 15:58

MartinMinkov changed the title ~~Add canonicalize function for finite-field.ts~~ Fix and finite field operations (isSquare, sqrt) Jul 23, 2024

MartinMinkov force-pushed the audit/field-curve-canon branch from 145a928 to 6f46ec8 Compare July 23, 2024 16:08

MartinMinkov requested review from mitschabaude and Trivo25 July 23, 2024 16:17

MartinMinkov added 2 commits July 29, 2024 10:12

MartinMinkov force-pushed the audit/field-curve-canon branch from 9aad038 to 901bdde Compare July 29, 2024 17:12

Trivo25 reviewed Jul 30, 2024

View reviewed changes

mitschabaude reviewed Jul 30, 2024

View reviewed changes

mitschabaude suggested changes Jul 30, 2024

View reviewed changes

MartinMinkov requested review from mitschabaude and Trivo25 July 30, 2024 17:06

MartinMinkov added 4 commits July 30, 2024 10:12

docs(finite-field): add comment explaining the logic behind the equal…

71f2e69

… method implementation

MartinMinkov force-pushed the audit/field-curve-canon branch from ef82364 to 71f2e69 Compare July 30, 2024 17:12

mitschabaude approved these changes Jul 31, 2024

View reviewed changes

MartinMinkov merged commit 4ff31bc into main Jul 31, 2024
1 check passed

MartinMinkov deleted the audit/field-curve-canon branch July 31, 2024 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix and finite field operations (isSquare, sqrt) #287

Fix and finite field operations (isSquare, sqrt) #287

MartinMinkov commented Jul 22, 2024 •

edited

Loading

mitschabaude left a comment

mitschabaude Jul 23, 2024 •

edited

Loading

MartinMinkov Jul 23, 2024

mitschabaude Jul 23, 2024

MartinMinkov Jul 23, 2024

mitschabaude Jul 23, 2024

MartinMinkov Jul 23, 2024

mitschabaude Jul 23, 2024

MartinMinkov Jul 23, 2024

mitschabaude Jul 23, 2024

MartinMinkov Jul 23, 2024

mitschabaude Jul 23, 2024

MartinMinkov Jul 23, 2024

mitschabaude Jul 23, 2024

MartinMinkov Jul 23, 2024

mitschabaude Jul 23, 2024

MartinMinkov Jul 23, 2024

Trivo25 Jul 23, 2024 •

edited

Loading

MartinMinkov Jul 23, 2024

Trivo25 Jul 30, 2024

MartinMinkov Jul 30, 2024

mitschabaude Jul 30, 2024 •

edited

Loading

mitschabaude Jul 30, 2024

MartinMinkov Jul 30, 2024

mitschabaude left a comment

mitschabaude left a comment

Fix and finite field operations (isSquare, sqrt) #287

Fix and finite field operations (isSquare, sqrt) #287

Conversation

MartinMinkov commented Jul 22, 2024 • edited Loading

Description

Additional Information

mitschabaude left a comment

Choose a reason for hiding this comment

mitschabaude Jul 23, 2024 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trivo25 Jul 23, 2024 • 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

mitschabaude Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitschabaude left a comment

Choose a reason for hiding this comment

mitschabaude left a comment

Choose a reason for hiding this comment

MartinMinkov commented Jul 22, 2024 •

edited

Loading

mitschabaude Jul 23, 2024 •

edited

Loading

Trivo25 Jul 23, 2024 •

edited

Loading

mitschabaude Jul 30, 2024 •

edited

Loading