doc: add comments to permutation prover structs #355
Conversation
There was a problem hiding this comment.
LGTM but I'm not familiar enough with this to approve. Specially I'm not very familiar with the names for the different domains; I think @davidnevadoc can review the naming better :)
Co-authored-by: Eduard S. <[email protected]>
|
@davidnevadoc |
|
Mark as |
| pub(crate) struct CommittedSet<C: CurveAffine> { | ||
| pub(crate) permutation_product_poly: Polynomial<C::Scalar, Coeff>, | ||
| permutation_product_blind: Blind<C::Scalar>, | ||
| } | ||
|
|
||
| /// Set of permutation product polynomials, which have been **committed**. | ||
| /// | ||
| /// This struct is to contain all the permutation product commitments, from a single circuit. |
There was a problem hiding this comment.
This structs only contains a vector of CommittedSet, which in turn, just holds polynomials that have been committed to, plus their blinders. So, the actual commitments are not store here, right? 🤔
There was a problem hiding this comment.
Yes, exactly. @davidnevadoc
The real commitments are computed & stored into transcript in this part.
The CommittedSet just holds the polynomial which has been committed, and its blinder.
There was a problem hiding this comment.
This struct is to contain all the permutation product commitments
Then I think we should change or remove this.
There was a problem hiding this comment.
I change the doc, as recommended, here. 23ada39
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #355 +/- ##
=======================================
Coverage 84.99% 84.99%
=======================================
Files 85 85
Lines 18684 18684
=======================================
Hits 15880 15880
Misses 2804 2804 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Doc looks good but I find the struct names somewhat confusing (not changed in this PR).
In particular, Committed being a Vec of CommittedSet and not the other way around seems very counter intuitive. Thoughts on this @adria0 @guorong009 ?
Other than that, LGTM 👍
Yes, they are very counter intuitive. |
| /// - `permutation_product_poly`: A (coefficient-form) polynomial representing the permutation grand product. | ||
| /// - `permutation_product_blind`: A scalar value used for blinding, in the commitment of the `permutation_product_poly`. | ||
| /// | ||
| /// It stores a single `Z_P` in [permutation argument specification](https://zcash.github.io/halo2/design/proving-system/permutation.html#argument-specification). |
There was a problem hiding this comment.
Thanks for adding a link the the halo2 specification!
| /// | ||
| /// It indicates that the permutation product polynomials([`Committed`]) have been evaluated in the evaluation domain, and converted to [`Evaluated`](see [`Committed::evaluate`]). | ||
| /// | ||
| /// It also indicates that the permuted product polynomials([`Evaluated`]) can be **opened** (see [`Evaluated::open`]). |
There was a problem hiding this comment.
| /// It also indicates that the permuted product polynomials([`Evaluated`]) can be **opened** (see [`Evaluated::open`]). | |
| /// It also indicates that the permuted product polynomials([`Evaluated`]) can be **opened** (see [`Evaluated::open`]). |
Good point!
Description
Document the permutation prover types, in
halo2_backendRelated issues
Resolve #264
Changes
CommittedSet,CommittedandEvaluatedstructs inhalo2_backend/plonk/permutation/prover.rs