-
Notifications
You must be signed in to change notification settings - Fork 1
Add pairing gadgets (depends on #26) #29
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
Conversation
22227da to
44a4e8c
Compare
314fa1a to
7ed8c1e
Compare
d5afd76 to
ff116d0
Compare
ff116d0 to
52d6074
Compare
@dtebbs Were the interfaces change needed for the code migration? If not, could we split this PR in 2 parts:
Reviewing large chunks of code that are moved and mutated from one project to another is not the easiest, so I'd rather stick to 1 PR = 1 thing here and limit the scopes of the contributions to make it easier to review (and make the process less error prone). |
In most cases the interface changes are tied in with moving the code Potentially we could create those PRs in the reverse order, i.e.
If you think that would make a big difference then let me know and I'll look into it. |
| const size_t num_primary_inputs, | ||
| const std::string &annotation_prefix); | ||
| void generate_r1cs_witness( | ||
| const r1cs_gg_ppzksnark_verification_key<other_curve<ppT>> &vk); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where's generate_r1cs_constraints?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok this interface has also been modified AFAICT, the _all_bits attribute isn't use anymore and the gen_constraints function seems to be missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok. this class seems to be the equivalent of libzecale::r1cs_gg_ppzksnark_verification_key_scalar_variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add this function back to stick to the interface, as in https://github.com/clearmatics/zecale/blob/master/libzecale/circuits/groth16_verifier/r1cs_gg_ppzksnark_verifier_gadget.tcc#L138-L142
Even if the body is empty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the empty functions for now so this PR doesn't get side-tracked. However, going forward I'd be keen to have a better distinction between these "variable" types (which should only have a "generate_r1cs_witness" method), and genuine "gadgets". Added #36 with some notes (TLDR: these objects should not really be expected to have a generate_constraints method).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening this ticket. I'm in favor or rethinking the distinction between gadgets and variables, but to keep the code consistent before we do so, and stick to the gadget interface, it's good to have this function added back. Let's revisit this in another PR.
libsnark/gadgetlib1/gadgets/verifiers/r1cs_ppzksnark_verifier_gadget.hpp
Show resolved
Hide resolved
libsnark/gadgetlib1/tests/test_r1cs_gg_ppzksnark_verifier_gadget.cpp
Outdated
Show resolved
Hide resolved
Ok I see. That's making this PR a little bit annoying to review since it moves quite a lot of code and the review is not simply about verifying the |
… group element gadgets
dbc3717 to
73a3cef
Compare
Thanks @AntoineRondelet . Rebased onto |
Moves many gadgets, including BLS12-377 pairing gadgets, from clearmatics/zecale, along with some accompanying interface changes.