Move Field to JS #902

mitschabaude · 2023-05-08T14:06:45Z

closes #869

The main goal of this PR is to improve the maintainability of snarkyjs' core, replacing the OCaml implementation of the Field class we had before with a new TS class Field. This will be followed up by similar refactors to other core types Bool, Group and Scalar, and a major cleanup of the API surface between OCaml and TypeScript.

Changes:

use new lower level Snarky.field module exposed from OCaml, see snarky.d.ts
reimplements the entire Field class, see the new field.ts. strategy:
- constant (out of snark) operations are implemented in TS on top of the TS field arithmetic (bindings/crypto). this is an important preparation step for being able to load the jsoo bundle async after page load
- some simple provable methods, like mul() or equals(), are reimplemented on top of low level snarky building blocks: constraint-adding functions and exists. The goal of this is to give more visibility into the workings of snarkyjs for the community.
- some more complicated provable methods, like toBits() and greaterThan(), are kept in OCaml to limit scope
- the internal composition of a Field (as { value: Cvar(Constant | Var(int) | ... ) }) is explicitly encoded as part of the public type. Again, this is to improve visibility into what's going on.
all previously deprecated methods on Field are removed. It just felt like a waste of time reimplementing all of them
the isZero() method is deprecated because it is equivalent to equals(0)
a new efficient assertNotEquals() method is added
comprehensive tests are added in field.unit-test.ts

An important property of this PR is that it does not change any of the example verification keys in vk_regression.json. In all reimplementations, we aimed for exact equivalence of constraints generated. That way, we can be sure that we didn't introduce wrong constraints accidentally. (I found optimizations to a few of the provable methods that would change constraints, but those are delayed for possible future PRs that are much smaller in scope.)

In some sense, the PR leaves snarkyjs in an in-between stage: The old OCaml field class is still used under the hood by Bool, Group and Scalar, and returned by functions such as Poseidon.hash. So, we have to make the new class fully compatible with the old class. We might revisit some design choices (such as storing out-of-circuit Fields as Uint8array, and not bigints) when this restriction is lifted later.

As for user-facing changes, we arguably improve the error messages in Field methods on constants, and also fix quite a few bugs as a side effect of reimplementing stuff:
closes #858
closes #469
closes #432
closes #743
closes #865
closes #723 (I think it's fair to say that this makes assertion errors clearer, compared to the weird hex spelling in the Ocaml error)

mitschabaude

some comments for reviewers!

mitschabaude · 2023-05-24T10:27:03Z

src/examples/zkapps/voting/test.ts

      feePayer,
-      'assert_equal'
+      'Member already exists!'
    );


some changes to errors required us to update some of the tests

mitschabaude · 2023-05-24T10:33:45Z

src/lib/core.ts

+/**
+ * An element of a finite field.
+ */
+const Field = toFunctionConstructor(InternalField);


Here is where Field is turned from a class into a "class that can also be used as a function", to support Field(x)

mitschabaude · 2023-05-24T10:35:57Z

src/lib/provable-context.ts

@@ -0,0 +1,110 @@
+import { Context } from './global-context.js';


this file collects the lower level global state / circuit runners that were previously in provable.ts and proof_system.ts. This is done so that core modules like Field can use this without creating a circular dependency with the higher-level stuff in provable.ts and proof_system.ts

mitschabaude · 2023-05-24T10:37:24Z

src/lib/provable.ts

@@ -194,12 +198,10 @@ function witness<T, S extends FlexibleProvable<T> = FlexibleProvable<T>>(

  let id = snarkContext.enter({ ...ctx, inWitnessBlock: true });
  try {
-    fields = Snarky.exists(type.sizeInFields(), () => {
+    let [, ...fieldVars] = Snarky.exists(type.sizeInFields(), () => {


exists operates no longer on OCaml field classes (which we try to remove wherever we can) but directly on Field.t and Field.Constant.t, the OCaml field types

mitschabaude · 2023-05-24T10:40:10Z

src/snarky.d.ts

@@ -170,22 +265,22 @@ declare class Field {
   * let sum = a.add(5)
   * ```
   */
-  add(y: Field | number | string | boolean): Field;
+  add(y: Field | number | string | bigint): Field;


I think nobody ever used a boolean to specify a field element, anywhere. I removed this feature and made sure you can use bigint everywhere instead

mitschabaude · 2023-05-24T10:49:00Z

src/lib/field.ts

+  add(y: Field | bigint | number | string): Field {
+    if (this.isConstant() && isConstant(y)) {
+      return new Field(Fp.add(this.toBigInt(), toFp(y)));
+    }
+    // return new AST node Add(x, y)
+    let z = Snarky.field.add(this.value, Field.#toVar(y));
+    return new Field(z);
+  }


this method shows the typical patterns that we use everywhere:

the input is tested for constants (without yet converting y to a constant field element, because the bigint -> Uint8array conversion is fairly expensive)

if everything is constant, we use Fp which is the TS finite field implementation

if not, we use lower level Snarky functions that operate on FieldVar (in OCaml: Field.t or field Cvar.t)

we wrap the resulting variables back into a Field

Since this is a pattern we use everywhere, can/should we abstract it away?

function binop<T>(x: Field, y: Fieldlike, tsImpl: (Field, Fieldlike) => T, snarkyImpl: (Field, Fieldlike) => T): T { if (x.isConstant() && isConstant(y)) { return tsImpl(x.toBigInt(), toFp(y)); } return snarkyImpl(x.value, Field.#toVar(y)); }

Something like that ^^

We could do that, but I don't feel it would make the code better. IMO, patterns are often better than abstractions, as in this case.

Abstractions use specialized concepts (binop(x, y, tsImpl, snarkyImpl)) that have to be learned, while patterns use plain language features like if conditions that everyone already knows

Abstractions get leaky, or are only partially applicable. Patterns are flexible to adapt to any use case.

Unrelated reason why I'm not in favor is that I try to avoid all wrapper functions, because they add two lines to our stack traces and our stack traces are too long:

... at binop (snarkyjs/src/lib/field.ts:192:11) at <anonymous> (snarkyjs/src/lib/field.ts:210:18) ...

mitschabaude · 2023-05-24T10:50:40Z

src/lib/field.ts

+  /**
+   * Multiplies this {@link Field} element with another coercible to a field.
+   */
+  mul(y: Field | bigint | number | string): Field {


I thought it was quite instructive how mul() works, and how complicated it actually is!

mitschabaude · 2023-05-24T10:52:32Z

src/lib/field.ts

+  /**
+   * @deprecated use `x.equals(0)` which is equivalent
+   */
+  isZero() {


isZero() is the base method for implementing equals(). Still, I think there should be just one way to do things, so I deprecated isZero() with the plan to make it a private helper method in the future.

Btw, I think how this is implemented is VERY instructive and interesting

mitschabaude · 2023-05-24T10:54:43Z

src/lib/field.ts

+});
+
+function isField(x: unknown): x is Field {
+  return x instanceof Field || (x as any) instanceof SnarkyFieldConstructor;


in some places we still have to deal with the possibility of field classes created in OCaml

mitschabaude · 2023-05-24T10:56:53Z

src/lib/field.unit-test.ts

+);
+
+// arithmetic, both in- and outside provable code
+equivalent2((x, y) => x.add(y), Fp.add);


the idea of these tests is that you have two implementations - the TS finite field Fp, and Field - and you test that they are equivalent, both in- and outside provable code. "Equivalent" also means that one throws an error if and only if the other throws an error

src/lib/field.ts

Trivo25

first pass looks great!

src/lib/field.ts

Trivo25 · 2023-05-24T13:07:23Z

src/snarky.d.ts

@@ -43,11 +45,19 @@ declare interface ProvablePure<T> extends Provable<T> {
  check: (x: T) => void;
 }

+// ocaml types
+type MlTuple<X, Y> = [0, X, Y];


is there a reason why OCaml arrays always start with a leading 0?

Here's the source of truth: https://github.com/ocsigen/js_of_ocaml#data-representation

But I don't know the reason for that leading 0. Maybe @dannywillems knows :D

I would guess it is to mimic the memory layout of the native compiler. The native Caml runtime will represent tuples, records and arrays with block with a tag 0, documentation here.

Tuples, records, and arrays are all represented identically at runtime as a block with tag 0. Tuples and records have constant sizes determined at compile time, whereas arrays can be of variable length. While arrays are restricted to containing a single type of element in the OCaml type system, this is not required by the memory representation.

And in the JS runtime, it seems it is mimic (tuple, records and arrays are identical, i.e. a JS array with the first element set to 0.

src/lib/field.test.ts

Trivo25

awesome! lgtm

src/lib/field.ts

bkase

Amazing! Excited for snarkyjs/light!

src/lib/field.ts

bkase · 2023-05-30T14:12:12Z

src/lib/field.ts

+   *
+   * @return A bigint equivalent to the bigint representation of the Field.
+   */
+  toBigInt() {


can we change this debugToBigInt() or toBigIntDebug() something
like that to make it clear in the name that it's only for debugging?

We've seen so many people make this mistake, it's probably worth it.

I imagine tactically, you'd need to deprecate these since they're in-use already and we can remove them fully later.

I think I'm going to punt on these suggestions, to discuss more as we have a bit of design space here.

What about the following ideas:

On variables, toBigInt() uses As_prover.read_var which is only available in an asProver block. This leads to the infamous and confusing error Can't evaluate prover code outside an as_prover block

=> why don't we just pass in a read_var / toConstant() function to asProver and witness blocks?

Provable.asProver((toConstant) => { console.log(toConstant(myValue).toString()) }

This would be the only supported way to ever call As_prover.read_var, and it would always be valid.

However, on constant field elements, the toBigInt() / toString() functions are still very useful, and not only for debugging, but mainly for IO. That's why I'm also not convinced of the name change. However, with the change above, we would never even try read_var inside toBigInt() and would fail with a simpler error message:

Field.toBigInt(): This method can't be called on variables. <some more info about variables vs constants>

This would get ideal once we start having a separate type for constants. Then, toBigInt() would only be present on the constant type.

Another idea that I've been kicking around is that we shouldn't emphasize using constant Field elements so much. Instead, everything that's not a variable should just be a plain JS type, like bigint or number, and it should be possible to use that in place of constant Field elements everywhere

Instead, everything that's not a variable should just be a plain JS type, like bigint or number, and it should be possible to use that in place of constant Field elements everywhere

I like this ^^, especially because you can monkey-patch things in JavaScript (though of course we should be careful)

src/lib/field.ts

bkase · 2023-05-30T14:19:28Z

src/lib/field.ts

+  add(y: Field | bigint | number | string): Field {
+    if (this.isConstant() && isConstant(y)) {
+      return new Field(Fp.add(this.toBigInt(), toFp(y)));
+    }
+    // return new AST node Add(x, y)
+    let z = Snarky.field.add(this.value, Field.#toVar(y));
+    return new Field(z);
+  }


Since this is a pattern we use everywhere, can/should we abstract it away?

function binop<T>(x: Field, y: Fieldlike, tsImpl: (Field, Fieldlike) => T, snarkyImpl: (Field, Fieldlike) => T): T { if (x.isConstant() && isConstant(y)) { return tsImpl(x.toBigInt(), toFp(y)); } return snarkyImpl(x.value, Field.#toVar(y)); }

Something like that ^^

src/lib/field.ts

mitschabaude added 16 commits May 8, 2023 16:00

Merge branch 'refactor/js-circuit' into refactor/field

f3e42e2

fixup

50c3f71

start on new field class

625b04b

simplify

864dbb1

fixup class redefinition

722ea52

type internal field

dae7cad

implement all field methods in the constant case

670af90

import Field from new file and fix some type errors

3c7ff65

temporarily switch back to ocaml FIeld

c1c0de4

assertion messages

c7eb3a9

accept field like inputs

3cc7b20

simpler & better field constructor

bef33b8

some fixes

2351566

also check if y is constant

5cf185b

implement variable field ops with original field

abfe250

make all unit test work

4a1729a

Base automatically changed from refactor/js-circuit to main May 10, 2023 20:08

mitschabaude added 8 commits May 10, 2023 22:30

Merge branch 'main' into refactor/field

2e8b443

low level snarky field

28d23c3

start using lower-level snarky field ops

9330630

bump target bc TS messes up private class fields

72bd575

equals

b9f6b46

lower level exists, improve internal API

4f2dd19

implement remaining Field API

455f2ca

copy over doccomments

91a4cfe

mitschabaude force-pushed the refactor/field branch from 59e9894 to 91a4cfe Compare May 22, 2023 05:39

mitschabaude added 4 commits May 22, 2023 07:39

bindings

f2a80a6

work around esbuild bug for web version

17500a4

minor

331b548

fix test expectations about assertEquals error

70d740f

mitschabaude marked this pull request as ready for review May 23, 2023 16:11

This was referenced May 23, 2023

Improve "range check helper" error messages #901

Closed

Tutorial 1 expected assert_equal output is incorrectly output as hexadecimal #865

Closed

mitschabaude added 2 commits May 24, 2023 11:56

break out lower level provable runners from provable.ts

23900c6

error in compare, some cleanup

cc2d5b9

mitschabaude commented May 24, 2023

View reviewed changes

mitschabaude mentioned this pull request May 24, 2023

Move Field to JS o1-labs/o1js-bindings#12

Merged

changelog

a56fa81

Trivo25 reviewed May 24, 2023

View reviewed changes

src/lib/field.ts Show resolved Hide resolved

Merge branch 'main' into refactor/field

2736eee

Trivo25 reviewed May 24, 2023

View reviewed changes

mitschabaude mentioned this pull request May 25, 2023

Feature/update snarkyjs docs #917

Merged

Trivo25 reviewed May 25, 2023

View reviewed changes

src/lib/field.test.ts Outdated Show resolved Hide resolved

Trivo25 approved these changes May 25, 2023

View reviewed changes

src/lib/field.ts Show resolved Hide resolved

src/lib/field.ts Show resolved Hide resolved

src/lib/field.ts Outdated Show resolved Hide resolved

src/lib/field.ts Show resolved Hide resolved

mitschabaude added 5 commits May 25, 2023 20:51

Merge branch 'main' into refactor/field

4cab7c8

merge field tests

d220950

comment explaining vars vs constants

dae4a08

move over new doccomments

b58938a

doccomment

5a11762

bkase requested changes May 30, 2023

View reviewed changes

mitschabaude mentioned this pull request May 31, 2023

Move Scalar to JS #925

Closed

mitschabaude added 2 commits May 31, 2023 11:41

address feedback

8eebafe

Merge branch 'main' into refactor/field

d6b7f5b

mitschabaude mentioned this pull request May 31, 2023

Improve Can't evaluate prover code outside an as_prover block error message #899

Closed

bkase approved these changes May 31, 2023

View reviewed changes

mitschabaude merged commit c331808 into main May 31, 2023

mitschabaude deleted the refactor/field branch May 31, 2023 14:14

mitschabaude mentioned this pull request Jun 1, 2023

Move Scalar to JS #935

Merged

Trivo25 mentioned this pull request Jun 5, 2023

Move Group to JavaScript #932

Merged

mitschabaude mentioned this pull request Jun 8, 2023

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Field to JS #902

Move Field to JS #902

mitschabaude commented May 8, 2023 •

edited

Loading

mitschabaude left a comment

mitschabaude May 24, 2023

mitschabaude May 24, 2023

mitschabaude May 24, 2023

mitschabaude May 24, 2023

mitschabaude May 24, 2023

mitschabaude May 24, 2023

bkase May 30, 2023

mitschabaude May 31, 2023 •

edited

Loading

mitschabaude May 24, 2023

mitschabaude May 24, 2023

mitschabaude May 24, 2023

mitschabaude May 24, 2023

Trivo25 left a comment

Trivo25 May 24, 2023

mitschabaude May 24, 2023

dannywillems May 24, 2023

Trivo25 left a comment

bkase left a comment

bkase May 30, 2023

mitschabaude May 31, 2023 •

edited

Loading

bkase May 31, 2023

bkase May 30, 2023

		@@ -0,0 +1,110 @@
		import { Context } from './global-context.js';

Move Field to JS #902

Move Field to JS #902

Conversation

mitschabaude commented May 8, 2023 • edited Loading

mitschabaude 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitschabaude May 31, 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 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

Trivo25 left a comment

Choose a reason for hiding this comment

bkase left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitschabaude May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitschabaude commented May 8, 2023 •

edited

Loading

mitschabaude May 31, 2023 •

edited

Loading

mitschabaude May 31, 2023 •

edited

Loading