Poly commit refactoring #35
Conversation
…-commit into refactor_pc_dev
There was a problem hiding this comment.
As pointed out in my comments, or discussed offline:
- let us find out if we can take
G: Groupas template forPolynomialCommitmentand related traits. If so, let us declare the domain extended scheme as polynomial commitment with the "vector group" as commitment group. - let us
trim()return again aPCParametersand reduce to a single functionmax_degree()in the traits for the key material, - let us clarify in comments why the IPA implementation of the
fn get_hash()in the latter traits does "break" with the original intention.
…one when trimming
DanieleDiBenedetto
force-pushed
the
refactor_ginger_dev
branch
from
dd4f66b
to
3e7f313
Compare
There was a problem hiding this comment.
The problems we faced in the implementation of the succinct verifier gadget (which partially generic in the concrete scheme) are due to our functional abstraction of the types for commitments and randomnesses, which leaves the concrete form/structure open and hence inaccessible.
To overcome this problem, and also for the reason of a more natural definition of our Group trait, I propose the following:
-
Let us remove the
ScalarFieldand theMulby such from theGrouptrait (and put it back in theCurvetrait). Imposing a prime torsion is a rather structural than functional requirement. (It enforces that the implementor is either a prime group, or a vectorized version of it.) -
We return to a polynomial commitment scheme over a Curve, i.e.
PolynomialCommitment<G: Curve>, and give bothCommitmentandRandomnessesa specific structure, by demanding that they are vectors of items which implementG, or allow to iterate over such (usingIntoIterator). -
As we do not have the scalar multiplication in the group trait anymore, we provide separate helper functions (or trait implementations) for the addition, and scalar multiplication for these associated types.
This allows an implementation for G:EndoMulCurve which uses endomorphism based scalar multiplication instead of the ordinary one. (Hence we could drop the workaround by letting the Fiat Shamir primitive output the expanded native scalar representation of the squeezed challenges.)
| ) | ||
| } | ||
|
|
||
| /// The verifier for proofs generated by `fn single_point_multi_poly_open()`. |
There was a problem hiding this comment.
Maybe worth to comment, that both values and labeled commitments reflect the same order.
There was a problem hiding this comment.
We often pass as parameters Iterators over references, and we must guarantee they have the same life time as the function itself
There was a problem hiding this comment.
I am not sure if I understand. Is it clear that if one implements IntoIterator for labeled_commitments and values that they iterate in a consistent manner, so at each iteration step value and commitment "belong together"?
If it is not clear, isn't it worth to document this demand?
There was a problem hiding this comment.
Put an additional comment on this
| // If for some `i`, `polynomials[i].is_hiding() == false`, then the | ||
| // corresponding randomness is `G::ScalarField::empty()`. | ||
| // TODO: move this function back to poly-commit when merging into mono-repo. | ||
| fn commit_many<'a>( |
There was a problem hiding this comment.
Just out of curiosity: What exactly is the reason that we re-introduce commit_many having it in the trait?
There was a problem hiding this comment.
Ah well it was needed and it doesn't really harm, so
| ) | ||
| } | ||
|
|
||
| /// The verifier for proofs generated by `fn single_point_multi_poly_open()`. |
There was a problem hiding this comment.
I am not sure if I understand. Is it clear that if one implements IntoIterator for labeled_commitments and values that they iterate in a consistent manner, so at each iteration step value and commitment "belong together"?
If it is not clear, isn't it worth to document this demand?
src/benches/open_proof.rs
Outdated
|
|
||
| criterion_group!( | ||
| name = tweedle_open_proof; | ||
| config = Criterion::default().sample_size(10); |
There was a problem hiding this comment.
Personally, I am not a big fan of such low sample sizes. Why not increasing it to 30 at minimum?
| for n in 16..22 { | ||
| bench_open_proof::<TweedleDee, Blake2s>( | ||
| c, | ||
| "open proof in tweedle-dee, coeffs", | ||
| 1 << n | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| fn bench_open_proof_tweedle_dum(c: &mut Criterion) { | ||
|
|
||
| for n in 16..22 { | ||
| bench_open_proof::<TweedleDum, Blake2s>( | ||
| c, | ||
| "open proof in tweedle-dum, coeffs", | ||
| 1 << n | ||
| ); | ||
| } |
There was a problem hiding this comment.
It think it is good enough to do the benches up to 21. Is there a reason for the 2^22?
There was a problem hiding this comment.
The iteration for the upper bound won't be executed, so it really benches only up to 2^21
src/lib.rs
Outdated
| pub type QueryMap<'a, F> = BTreeMap<PointLabel, (F, BTreeSet<PolynomialLabel>)>; | ||
|
|
||
| /// `Evaluations` is the result of querying a set of labeled polynomials or linear combinations | ||
| /// `p` at a `QueryMap` `Q`. | ||
| /// It maps each element of `Q` to the resulting evaluation, e.g. `evaluation.get((label, query))` | ||
| /// should equal to `p[label].evaluate(query)`. | ||
| pub type Evaluations<'a, F> = BTreeMap<(PolynomialLabel, PointLabel), F>; |
There was a problem hiding this comment.
These lifetimes are useless actually
This PR contains the phase 2 poly-commit refactoring based on refactored ginger-lib