Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move low-level operations to JS #967

Merged
merged 24 commits into from
Jun 20, 2023
Merged

Move low-level operations to JS #967

merged 24 commits into from
Jun 20, 2023

Conversation

Trivo25
Copy link
Member

@Trivo25 Trivo25 commented Jun 6, 2023

This PR is a follow up to #932. It further reduces dependencies to OCaml by re-using more of the existing clue as well as moving some additional functionality to JavaScript. The PR also addresses handling of the zero/infinity element by adding further constraints (it breaks existing smart contracts, whereas the previous PR tried to establish a baseline without any major constraint changes).

bindings o1-labs/o1js-bindings#35

Base automatically changed from refactor/group to main June 8, 2023 11:03
src/lib/group.ts Outdated Show resolved Hide resolved
src/lib/group.ts Outdated Show resolved Hide resolved
src/lib/group.ts Outdated Show resolved Hide resolved
src/lib/group.ts Show resolved Hide resolved
src/lib/group.ts Show resolved Hide resolved
@Trivo25 Trivo25 marked this pull request as ready for review June 11, 2023 18:03
Copy link
Member

@mitschabaude mitschabaude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, I have a few comments!

src/lib/group.ts Outdated Show resolved Hide resolved
src/lib/group.ts Outdated Show resolved Hide resolved
src/lib/group.ts Outdated Show resolved Hide resolved
src/lib/group.ts Outdated Show resolved Hide resolved
src/lib/group.ts Outdated Show resolved Hide resolved
src/lib/group.ts Outdated Show resolved Hide resolved
src/lib/group.ts Show resolved Hide resolved
src/lib/group.ts Show resolved Hide resolved
src/lib/group.ts Outdated Show resolved Hide resolved
src/lib/group.ts Outdated Show resolved Hide resolved
src/lib/group.test.ts Show resolved Hide resolved
Copy link
Member

@bkase bkase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably get a once-over by someone on the crypto team since we're changing the constraints here -- maybe @jspada ?

setTimeout(async () => {
await shutdown();
}, 0);
});

describe('Inside circuit', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use the quickcheck generator Gregor made earlier to check some of the algebraic properties for edge cases?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about that as well, however, I think there is no easy way to sample valid Group elements other than brute forcing (or is there?) that's why the existing tests mostly work with the generator and (-1, 2)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used to sample public keys (= group elements) by picking a random private key (= scalar) and scaling the group generator with it.
That's not super efficient but good enough.

However, now we have hashToCurve which should be faster, especially if you skip the hash and only do random field element + mapToCurve

Copy link
Member Author

@Trivo25 Trivo25 Jun 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, I didn't consider that at all 😵‍💫 I'll add some more tests, just to be sure!

Copy link
Member

@mitschabaude mitschabaude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the PR looks good to me now but I agree with Brandon's suggestion to create more test cases using the random generators

and we should cut down the constraints used in Group.add

Comment on lines +155 to +156
"rows": 42,
"digest": "8d5ab248aa4602e6f77a8ab1015615df"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain this blowup in Group.equals() constraints @Trivo25?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it just caused by the added constraints in check()?

Copy link
Member Author

@Trivo25 Trivo25 Jun 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Group Primitive.equals check bundles a couple additional equality checks than just a simple equals(), it does the following:

    let g1 = Provable.witness(Group, () => Group.generator);
    let g2 = Provable.witness(Group, () => Group.generator);
    g1.equals(g2).assertTrue();
    g1.equals(g2).assertFalse();
    g1.equals(g2).assertEquals(true);
    g1.equals(g2).assertEquals(false);

So its calling equals multiple times, which blows up the constraints a bit more. The single equals constraints aren't that bad, just inflated by the batch. The additional constraints come from zero-related checks (such as .check)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I understand that it has more, but the only thing that changed in that circuit should be check() and equals(), so I was wondering what explains the increase in circuit size compared to the previous version

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, its the change in check

@Trivo25 Trivo25 merged commit 1455a7f into main Jun 20, 2023
9 checks passed
@Trivo25 Trivo25 deleted the refactor/group-add branch June 20, 2023 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants