[BASE] Plonk #49
[BASE] Plonk #49
Conversation
|
Thanks for opening this Draft PR @vesselinux. Commits doing similar things like plonk: computing alpha powers
plonk: computing alpha powers
plonk: computing alpha powers(and many more) should be squashed together + formatting commits should normally not be needed if you have your text editor configured to run the script on saving. |
libsnark/gadgetlib1/gadgets/hashes/sha256/sha256_components.tcc
Outdated
Show resolved
Hide resolved
…ly accumulate result thus removing the need to reserve additional memory for intermediate results (following draft PR #49 review comment)
…function plonk_usrs_derive_from_secret that directly returns a correctly initialized usrs object. this change addresses a review comment on draft PR #49.
vesselinux
force-pushed
the
plonk
branch
3 times, most recently
from
c29db3b
to
967fe3e
Compare
| { | ||
| assert(omega.size() == NUM_HSETS); | ||
| assert(omega[0].size() > 0); | ||
| size_t num_gates = omega[0].size(); |
There was a problem hiding this comment.
There could be a lot of room for error if this is passed in implicitly in the length of the first array. Should it also be checked to ensure it's a power of 2? Also that it's less than 2^(ppT::s) (I think it's s)
There was a problem hiding this comment.
Potentially this and plonk_roots_of_unity_omega_to_subgroup_H can be replaced by a single function which takes an
There was a problem hiding this comment.
Proposed change accepted and applied. More specifically, replaced function plonk_roots_of_unity_omega_to_subgroup_H with plonk_multiplicative_subgroups_H_k1H_k2H, which recomputes the vector omega in order to avoid passing it as an input parameter potentially with the wrong size (as was done in plonk_roots_of_unity_omega_to_subgroup_H). Note that the function plonk_compute_roots_of_unity_omega is still kept (and used in plonk_multiplicative_subgroups_H_k1H_k2H) since (as mentioned in a reply to an earlier review comment) the omega vector is used in multiple places (e.g. as the omega_roots member of the srs class and the circuit_t structure).
| const srs<ppT> &srs) | ||
| { | ||
| using Field = libff::Fr<ppT>; | ||
| int nwitness = 3; |
| } | ||
|
|
||
| // break this->t_poly_long into three parts: lo, mid, hi, each of degree | ||
| // 7. note: (srs.num_gates+3) is the length of the CRS = |
There was a problem hiding this comment.
"7"? Is that for the example circuit?
There was a problem hiding this comment.
Indeed, degree 7 comes from the example circuit which has 8 gates (6 gates + 2 dummy to make power of 2 for the FFT). Corrected to degree (num_gates - 1)
| libfqfft::_polynomial_addition<Field>(r_poly, r_poly, r_part[3]); | ||
|
|
||
| // TODO: make a separate unit test for r_poly | ||
| #if 0 |
There was a problem hiding this comment.
Is this going to be moved into a test. It seems example is already not in scope, so can it be removed?
There was a problem hiding this comment.
Yes, the idea is to have a separate unit test under tests/ to check the value of r_poly (and the #if 0 guard is meant as a reminder). However this involves breaking down the function plonk_prover<ppT>::round_five() into several sub-functions which may require a bit more effort. Similar refactoring is desirable for plonk_prover<ppT>::round_three() . I added all of these points as TODO in Issue #52 .
| transcript_hash_t<ppT> &transcript_hash) | ||
| { | ||
| // step 4 output | ||
| libff::Fr<ppT> beta; |
There was a problem hiding this comment.
This function declares, then copies, then moves. I think you can just pass Field(transcript_hash.beta) directly to the constructor.
There was a problem hiding this comment.
Move semantics don't do anything for field elements, so step_four_t should just take const references, and you can just pass transcript_hash.beta, ... directly.
There was a problem hiding this comment.
Good points. Fixed.
|
OK. I think we can call this v0.1. Let's keep this branch as the stable version, and we can make issues against this and future changes in PRs tagretting this branch. This should make future PRs a bit quicker and easier to review. |
|
@vesselinux can you rebase onto the latest |
Done. |
…k1,k2 constants for plonk
…k2 (#94 (comment)) + removed some redundant comments
… test function and removed redundant check 3 from the bls12-381-specific unit test (#94 (comment))
…end with full-stop at the end (#94 (comment))
90 plonk setting k1 k2
…onvenience of the gates helper function (#97 (review))
…on of gates matrix code
…es taking only the number of PIs as opposed to a vector of their indices (#97 (comment)); added utility function to compute teh values of the PIs from the witness to be passed to the verifier.
…edits, minor code optimisations
Plonk: helper function to build gates matrix (depends on #70)
…d unit tests for the simple quadratic residue circuit for all curves (addresses issue #103)
…thod buffer_clear to reset (#107 (comment)); minor edits in comments (#107 (comment)).
…-curves Test simple Plonk circuit on all curves
Initial implementation of the Plonk zk-SNARK proof system.