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

[curves] introduce wrappers for Fp and Fq #765

Closed
wants to merge 17 commits into from
Closed

[curves] introduce wrappers for Fp and Fq #765

wants to merge 17 commits into from

Conversation

mimoo
Copy link
Contributor

@mimoo mimoo commented Sep 26, 2022

This introduces wrapper types for ark_ff::BigInteger256 as well as for our own Fp and Fq. These are supposed to add no real overhead as most functions are inlined:

Proof creation/proof creation (SRS size 2^15)                                                                          
                        time:   [3.3081 s 3.3659 s 3.4263 s]
                        change: [-0.7666% +2.6613% +5.8995%] (p = 0.15 > 0.05)
                        No change in performance detected.

(compare this with #750)

The benefit is that we can implement ocaml traits on these.

I also implemented serde serialize/deserialize on them, but I didn't manage to remove the serde_as/serde_with stuff elsewhere. I think it's doable though.

@mimoo mimoo requested a review from mrmr1993 September 26, 2022 20:49
Copy link
Member

@mrmr1993 mrmr1993 left a comment

Choose a reason for hiding this comment

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

Really nice!
As a sanity check, it would be good to make a Mina PR and make sure that CI is happy there

@mimoo
Copy link
Contributor Author

mimoo commented Sep 26, 2022

playing with it here: MinaProtocol/mina#11906

it seems to compile the stubs. Not sure if mina compiles yet, and haven't touched the wasm part.

Copy link
Member

@mrmr1993 mrmr1993 left a comment

Choose a reason for hiding this comment

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

See comment. Can you rework this so that it's only moving the OCaml types into this repo, and leave the rest on the Mina side?

curves/src/pasta/arkworks/fields.rs Outdated Show resolved Hide resolved
@github-actions
Copy link

Stale pull request message

@github-actions github-actions bot closed this Dec 5, 2022
@mimoo mimoo reopened this Dec 5, 2022
@mimoo
Copy link
Contributor Author

mimoo commented Dec 5, 2022

probably we should try this approach first: arkworks-rs/algebra#506

EDIT: actually nevermind, this won't help us with the ocaml traits

@mimoo
Copy link
Contributor Author

mimoo commented Dec 6, 2022

ping @mrmr1993

@mimoo
Copy link
Contributor Author

mimoo commented Dec 6, 2022

urg actually I don't think this will solve all of our problems for serde at least... Because things like G::Scalar are not bound by Serialize/Deserialize. We might have to do the same for AffineCurve

@github-actions github-actions bot closed this Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants