Proper Int64 #171

mitschabaude · 2022-05-23T08:35:56Z

Replace unfinished, unconstrained Int64 with a proper implementation
Closes Finish Int64 implementation #66
Make Int64 API less boilerplate-y by allowing number, string, etc as method arguments
Remove some hacks in toJson conversion, possible bc the new Int64 has the same layout as Currency.Amount.Signed.t

This is stacked on top of #164, so that it can:

Make previously failing Int64 unit tests work
Add more Int64 unit tests

Since this will use updated bindings from MinaProtocol/mina#11042, which fix an aspect of the Field constructor, this also adds a few new tests for the Field constructor.

Closes Field constructor vs static methods #43

mitschabaude · 2022-05-23T21:14:47Z

src/lib/int.ts

-    return Int64.sizeInFields();
+function argToField(
+  name: string,
+  x: { value: Field } | number | string


TODO: add range checks (not constraints, just checks) on constant UInt64 / UInt32 / Int64 as you create them

src/lib/int.ts

mimoo

This is awesome! Code is clear, and this looks good to me, left a few minor comments

src/lib/int.ts

mimoo · 2022-05-23T22:57:58Z

src/lib/int.ts

-    return new Int64(this.value.neg());
+  add(y: Int64 | number | string | bigint | UInt64 | UInt32) {
+    let y_ = argToInt64(y);
+    return Int64.fromField(this.toField().add(y_.toField()));


I'm thinking that with lookups, it'd be easier to just represent the Int64 as a Field and do range checks after every operation to make sure that 2^64 * res is in a [0, 2^128] range. To contrast:

that would be 2 range checks (because our range is smaller than 2^128) and 1 constraint to shift the value being looked up. Including the addition that's 4 constraints)

This PR's implementation is:

2 direct toField (2 constraints)

fromField is 1 toField, 1 assertEquals, 1 Witness

the Witness will also call check which will call 1 square and 1 assertEquals,

check also calls Uint64.Check, which is actually quite heavy due to the lack of lookups as well. But we have to imagine that this would be just two lookup constraints at most eventually (with range lookups)

so that's 8-9 constraints

Not sure if I'm reading this correctly, or if I'm missing something.

Yeah I think you may have a point.. I might reconsider this when range checks become cheap.

Do you mean shifting with 2^64 + res and range-checking that? I don't follow how it works with multiplication & negative numbers.

Currently, it's convenient to have Int64 be {magnitude, sign} because that mirrors the Ocaml data structure. If I make it a single Field constrained to 64 bits, I allow a slightly smaller range than the Ocaml type (64 vs 65 bits)

yeah + not * sorry

mimoo · 2022-05-23T23:15:05Z

src/lib/int.ts

+  if (typeof x === 'bigint') return Int64.fromBigInt(x);
+  if (x instanceof UInt64 || x instanceof UInt32) return Int64.fromUnsigned(x);
+  return x;
+}


why is this not implemented on Int64? Something like Int64.from(x: number | string | bigint | UInt64 | UInt32 | Int64).

and same issue here, the fromNumber and other similar functions don't error if the user tries to input values that are too large (or too small)

great suggestion!

mimoo · 2022-05-23T23:16:25Z

src/lib/int.ts

@@ -53,7 +42,7 @@ export class UInt64 extends CircuitValue {

  static NUM_BITS = 64;

-  divMod(y: UInt64 | number): [UInt64, UInt64] {
+  divMod(y: UInt64 | number | string): [UInt64, UInt64] {


this return type is a bit error prone, why not return something like { quotient, rest } instead?

Especially when we do things like let [q] = or let [, r] =

mimoo · 2022-05-23T23:36:45Z

src/lib/int.ts

-    throw 'repr';
+  equals(y: Int64 | number | string | bigint | UInt64 | UInt32) {
+    let y_ = argToInt64(y);
+    return this.toField().equals(y_.toField());


would this.magnitude.equals(y_.magnitude).and(this.sign.equals(y_.sign) be better? The two equalities could be optimized by the compiler (using the permutation), and thus you'd only have one constraint (the and)

actually never mind, you can't optimize these with the permutation as you don't assert. But maybe the assertEquals could use something like this instead

this.magnitude.assertEquals(y_.magnitude); this.sign.assertEquals(y_.sign);

I didn't do that because I was worried about special-casing for 0, where -0 should equal 0.. The special-casing would also require multiplication constraints I think

good point :o

mitschabaude · 2022-05-25T08:20:55Z

@mimoo addressed all feedback,
also added Field.to/fromBigint() to make the code cleaner in some places

this should be good to go, will merge once the parent PR is merged

mitschabaude added 7 commits May 23, 2022 09:49

replace int64 with int65 example

10c2041

rename, magnitude -> UInt64, polymorphic methods

15c10df

clean up int.ts file structure

d4398b3

make int64 methods even more polymorphic

8546eb8

make balanceChange a normal int64

9fc7b3c

remove custom balance change type

00446ba

fix last commit

a5439c5

mitschabaude marked this pull request as ready for review May 23, 2022 09:03

mitschabaude added 7 commits May 23, 2022 11:15

fix comments referencing "Int65"

fcda1ad

Merge branch 'feature/more-tests' into feature/int64

20671a5

workaround Field(number) bug

38b1616

adapt working Int64 tests

1f8b27a

comment out missing int64 tests / make them work

2f4b6fe

add int64 mul / div test and fixes to make it work

9b8df61

delete old int65 example

6aa0124

mitschabaude changed the base branch from main to feature/more-tests May 23, 2022 10:48

mitschabaude changed the base branch from feature/more-tests to main May 23, 2022 10:53

mitschabaude changed the base branch from main to feature/more-tests May 23, 2022 10:53

fix field constructor and add tests for it

37a22b7

mitschabaude commented May 23, 2022

View reviewed changes

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

mimoo approved these changes May 23, 2022

View reviewed changes

mitschabaude added 3 commits May 24, 2022 20:50

make Field(bigint) work, add fromX constructors

fd3ae6e

add Field.toBigInt, minusOne, ORDER

ade01e0

address pr feedback #171

f41d288

mitschabaude added 3 commits May 25, 2022 10:46

merge upstream, add more overflow tests

d91cf4f

tweak error message

aebb032

expose surprising overflows in skipped tests

d34744d

mitschabaude changed the base branch from feature/more-tests to main May 26, 2022 09:46

mitschabaude added 2 commits May 26, 2022 11:51

Merge branch 'main' into feature/int64

9c20217

update bindings

f59c5f5

mitschabaude merged commit a72606a into main May 26, 2022

mitschabaude mentioned this pull request Jun 6, 2022

Fast zkapp proofs with max_proofs_verified=0 MinaProtocol/mina#11053

Merged

1 task

mitschabaude deleted the feature/int64 branch June 30, 2022 07:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proper Int64 #171

Proper Int64 #171

mitschabaude commented May 23, 2022 •

edited

Loading

mitschabaude May 23, 2022

mimoo left a comment

mimoo May 23, 2022

mitschabaude May 24, 2022

mimoo May 24, 2022

mimoo May 23, 2022 •

edited

Loading

mitschabaude May 24, 2022

mimoo May 23, 2022

mimoo May 23, 2022 •

edited

Loading

mitschabaude May 24, 2022

mimoo May 24, 2022

mitschabaude commented May 25, 2022

Proper Int64 #171

Proper Int64 #171

Conversation

mitschabaude commented May 23, 2022 • edited Loading

Choose a reason for hiding this comment

mimoo 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

mimoo May 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimoo May 23, 2022 • 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 25, 2022

mitschabaude commented May 23, 2022 •

edited

Loading

mimoo May 23, 2022 •

edited

Loading

mimoo May 23, 2022 •

edited

Loading