Move Group to JavaScript #932

Trivo25 · 2023-05-28T14:51:56Z

This PR aims to move the Group implementation from OCaml to JavaScript, reducing OCaml dependencies as best as possible. The new Group implementation covers the same API surface as the old one and produces the exact same constraints (important for compatibility with already existent projects)

This will allow us to easier maintain more of the SnarkyJS elements by relying less on OCaml as well as giving us more control over what we are doing. Additionally, in the future this will enable a snarkyjs/light version by relying on native JS arithmetic instead of requiring users to load a lot of bindings for simple calculations -- builds on top of #902 and is motivated by the same reasons

o1-labs/o1js-bindings#25

closes #924

Trivo25 · 2023-06-02T12:04:42Z

src/examples/primitive_constraint_system.ts

@@ -0,0 +1,66 @@
+import { Field, Group, Poseidon, Provable, Scalar } from 'snarkyjs';
+
+function mock(obj: { [K: string]: (...args: any) => void }, name: string) {


We didn't really have any regression tests that tested Group and its constraints, I just hacked this mock contract-like thing together to integrate it into the contract regression tests

Trivo25 · 2023-06-02T12:05:02Z

src/examples/primitive_constraint_system.ts

+  };
+}
+
+const GroupMock = {


this one adds some "tests" for Group constraints

Trivo25 · 2023-06-02T12:05:31Z

src/examples/vk_regression.ts


 // usage ./run ./src/examples/vk_regression.ts --bundle --dump ./src/examples/regression_test.json
 let dump = process.argv[4] === '--dump';
 let jsonPath = process.argv[dump ? 5 : 4];

-const Contracts: (typeof SmartContract)[] = [
+type MinimumConstraintSystem = {


some minor changes to accommodate the Group constraint tests, see above

Trivo25 · 2023-06-02T12:07:04Z

src/lib/group.ts

+   * Coerces anything group-like to a {@link Group}.
+   */
+  constructor(g: {
+    x: FieldVar | Field | number | string | bigint;


Using the overloaded constructor so that the new implementation is equivalent to the old OCaml one, which allowed both (x, y) as well as {x, y} as arguments. I am not using the FieldLike or GroupLike type from above so users arent confused by it and see "the actual type".

is this a spot where you can use satisfies to assert it's the same as FieldLike? Would add nice info to the vscode hints too

I don't think so (or at least I am unsure?) plus I actually just pushed a change that doesnt overload the constructor since that didn't work out with the toFunctionConstructor we use now so we dont need that anymore

Trivo25 · 2023-06-02T12:07:15Z

src/lib/group.ts

+
+export { Group };
+
+type FieldLike = FieldVar | Field | number | string | bigint;


just to make the internal code a bit more readable

src/lib/group.ts

Trivo25 · 2023-06-02T12:11:16Z

src/lib/group.ts

+    let equal_y = y1.equals(y2);
+    return equal_x.and(equal_y);*/
+
+      let z = Snarky.group.equals(Group.#toTuple(this), Group.#toTuple(g));


I am still trying to figure out why this isn't producing the same set of constraints:

I tried implementing it with x1.equals(x2).and(y1.equals(y2)) - which is (at least to my knowledge) equaivalent to the OCaml implementation of

Boolean.all [ Impl.Field.equal x1 x2; Impl.Field.equal y1 y2 ]
where Boolean.all just calls b1 && b2 for tuples. it produces the same amount of rows/gates but with slightly different coefficients?

Trivo25 · 2023-06-02T12:12:32Z

src/lib/group.ts

+        return Group.#fromProjective(g_proj);
+      }
+    } else {
+      let [, x, y] = Snarky.group.add(Group.#toTuple(this), Group.#toTuple(g));


I would like to move this entirely to JavaScript, but wanted to establish a baseline first. I will stack another PR with the detailed implementation on top of this one soon

Trivo25 · 2023-06-02T12:12:47Z

src/lib/group.ts

+      let g_proj = Pallas.scale(this.#toAffine(), BigInt(scalar.toJSON()));
+      return Group.#fromProjective(g_proj);
+    } else {
+      let [, x, y] = Snarky.group.scale(Group.#toTuple(this), [


similar to the comment above

Trivo25 · 2023-06-02T12:17:03Z

src/lib/group.ts

+    let { x: x2, y: y2 } = g;
+
+    x1.assertEquals(x2, message);
+    // y1.assertEquals(y2, message); need to enable this later on, but it breaks constraint backwards compatibility


The OCaml implementation only checked if the x coordinates are equal, allowing devs to prove g1 === g2 even if g1.y !== g2.y

Trivo25 · 2023-06-02T12:21:14Z

src/lib/group.ts

+  ) {
+    let x, y: FieldLike;
+
+    if (isGroupLike(arg1)) {


i feel like this is a bit hacky, if anyone has recommendations let me know :D

mitschabaude

Awesome work, I like how this keeps the OCaml API slim.

A few comments, most notably regarding consistent treatment of the zero Group element. I haven't looked up myself how the in-snark OCaml methods treat zero, so I'll leave it to you to identify a consistent way of doing it (and ideally document it)

src/lib/core.ts

src/examples/regression_test.json

src/lib/group.test.ts

src/lib/group.ts

mitschabaude · 2023-06-05T09:35:49Z

src/lib/group.ts

+    } else {
+      let [, x, y] = Snarky.group.scale(this.#toTuple(), [
+        0,
+        ...fields.map((f) => f.value).reverse(),


while I appreciate keeping the Snarky interface low-level and having logic like reversing the scalar bits on the JS side, it would be good if a doccomment on Snarky.group.scale documented this unexpected behaviour

src/lib/group.ts

mitschabaude · 2023-06-05T09:42:39Z

src/lib/group.ts

+  /**
+   * Adds a {@link Group} element to another one.
+   */
+  static add(g1: Group, g2: Group) {


IMO all these extra static methods are weird and they should either be deprecated or be present consistently on all classes, i.e. added to Field for example

I am also not a massive fan, I would rather deprecate them

We can always recreate them with bind anyway -- JS devs are used to this; so I agree we should deprecate

src/snarky.d.ts

mitschabaude · 2023-06-05T13:10:54Z

note: this PR has some conflicts with the Scalar move. I prepared a branch refactor/group-and-scalar (snarkyjs + bindings) which combines the two, that can be merged into whichever of the two PRs gets merged second

bkase

This great!

There are a lot of subtle security gotchas in this sort of code -- as a side-effect of moving more of our logic to TypeScript, it becomes more likely that auditors will be productive reviewing this kind of code. Maybe we should be making a queue of std library code that we should get third-party audited and figuring out when to prioritize it.

bkase · 2023-06-05T13:58:44Z

src/examples/vk_regression.ts

@@ -1,22 +1,39 @@
 import fs from 'fs';


can you add this to CI? We can remove it later when we make breaking changes to the backwards compatibility on purpose

it already is running in the CI! The PR gets blocked if a constrained mismatch is detected

bkase · 2023-06-05T14:00:31Z

src/lib/group.test.ts

-        }).not.toThrow();
-      });
-
-      it('((2^64/2)+(2^64/2)) does not throw', () => {


why did you remove these other test cases?

The values we used to test weren't valid Group elements (OCaml implementation never checked that, except for variables), and since I added a check that makes sure we only accept valid elements, the old tests started to fail - I added new ones

bkase · 2023-06-05T14:02:28Z

src/lib/group.ts

+   * Coerces anything group-like to a {@link Group}.
+   */
+  constructor(g: {
+    x: FieldVar | Field | number | string | bigint;


is this a spot where you can use satisfies to assert it's the same as FieldLike? Would add nice info to the vscode hints too

bkase · 2023-06-05T14:04:39Z

src/lib/group.ts

+      let onCurve =
+        add(mul(x_bigint, mul(x_bigint, x_bigint)), Pallas.b) ===
+        square(y_bigint);


I think we should support zero and pay the constraint costs if we have to pick one -- we can make a TODO to make some way to create a more efficient Group later.

bkase · 2023-06-07T14:13:19Z

src/lib/group.ts

+      let onCurve =
+        add(mul(x_bigint, mul(x_bigint, x_bigint)), Pallas.b) ===
+        square(y_bigint);


safe and unsafe is a great idea 👍 , but yeah can come after

Trivo25 added 12 commits May 28, 2023 15:01

add group primitive constraint system tests

ced6291

add group file

78fcc56

add satic functions

d67a83b

add missing methods WIP

6c71bc3

minor

afac2e5

ocaml type defs

970bd42

bindings

54cf872

move error helper

0fb0e12

minor

fe6cb8f

expose onCurve

15e9934

make Provable-like

7623c5b

move error helper

de70719

Base automatically changed from refactor/field to main May 31, 2023 14:14

Trivo25 added 9 commits May 31, 2023 16:19

add helper and variable-scale

23ba415

expose scale

a01f5b4

implement constant scale

d85e35c

implement equals

0fbc0bb

fix assertEquals

2c17981

assertEquals comment

d47737b

comments

ade4f62

expose new group impl

44913b8

make new api uniform with old implementation

38f92b6

Trivo25 marked this pull request as ready for review June 1, 2023 16:16

Trivo25 added 7 commits June 1, 2023 18:30

Merge branch 'main' into refactor/group

98a698b

bindings

5eeacad

Merge branch 'main' into refactor/group

af66472

revert broken null pointer bindings

9c32d81

print more method details

ee16ef7

fix scalar conversion

7a7dfbf

fix poseidon

a616415

Trivo25 added 5 commits June 2, 2023 12:58

remove class declaration

7742e8a

remove ocaml group impl

ea47516

fix poseidon return type

1b50bf3

abstract group implementation

06666a0

bindings

600e4f3

Trivo25 commented Jun 2, 2023

View reviewed changes

Trivo25 changed the title ~~refactor group WIP~~ Move Group to JavaScript Jun 2, 2023

Trivo25 commented Jun 2, 2023

View reviewed changes

Trivo25 added 4 commits June 2, 2023 14:26

change tuple helper

93f3d75

add curve check

bf8d50f

undo overloaded constructor

8870a0d

fix test

c4bb385

mitschabaude reviewed Jun 5, 2023

View reviewed changes

address feedback

7616b13

deprecate static functions

cb870d9

bkase requested changes Jun 5, 2023

View reviewed changes

mitschabaude approved these changes Jun 5, 2023

View reviewed changes

Trivo25 added 2 commits June 6, 2023 12:57

Merge branch 'main' into refactor/group

123b9ac

bindings

85c0c05

bkase approved these changes Jun 7, 2023

View reviewed changes

Trivo25 merged commit 7b5eb01 into main Jun 8, 2023
9 checks passed

Trivo25 deleted the refactor/group branch June 8, 2023 11:03

mitschabaude mentioned this pull request Jun 8, 2023

Remove uses of JS classes in OCaml, Part 1 #956

Merged

Trivo25 mentioned this pull request Jun 11, 2023

Move low-level operations to JS #967

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Group to JavaScript #932

Move Group to JavaScript #932

Trivo25 commented May 28, 2023 •

edited

Loading

Trivo25 Jun 2, 2023

Trivo25 Jun 2, 2023

Trivo25 Jun 2, 2023

Trivo25 Jun 2, 2023

bkase Jun 5, 2023

Trivo25 Jun 5, 2023 •

edited

Loading

Trivo25 Jun 2, 2023

Trivo25 Jun 2, 2023

Trivo25 Jun 2, 2023 •

edited

Loading

Trivo25 Jun 2, 2023

Trivo25 Jun 2, 2023

Trivo25 Jun 2, 2023

mitschabaude left a comment •

edited

Loading

mitschabaude Jun 5, 2023

mitschabaude Jun 5, 2023

Trivo25 Jun 5, 2023

bkase Jun 5, 2023

mitschabaude commented Jun 5, 2023

bkase left a comment •

edited

Loading

bkase Jun 5, 2023

Trivo25 Jun 5, 2023

bkase Jun 5, 2023

Trivo25 Jun 5, 2023 •

edited

Loading

bkase Jun 5, 2023

bkase Jun 5, 2023

bkase Jun 7, 2023

		@@ -0,0 +1,66 @@
		import { Field, Group, Poseidon, Provable, Scalar } from 'snarkyjs';

		function mock(obj: { [K: string]: (...args: any) => void }, name: string) {


		export { Group };

		type FieldLike = FieldVar \| Field \| number \| string \| bigint;

Move Group to JavaScript #932

Move Group to JavaScript #932

Conversation

Trivo25 commented May 28, 2023 • 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

Trivo25 Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trivo25 Jun 2, 2023 • 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 left a comment • 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

mitschabaude commented Jun 5, 2023

bkase left a comment • 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

Trivo25 Jun 5, 2023 • 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

Trivo25 commented May 28, 2023 •

edited

Loading

Trivo25 Jun 5, 2023 •

edited

Loading

Trivo25 Jun 2, 2023 •

edited

Loading

mitschabaude left a comment •

edited

Loading

bkase left a comment •

edited

Loading

Trivo25 Jun 5, 2023 •

edited

Loading