-
Notifications
You must be signed in to change notification settings - Fork 188
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
refactor: shplonk + gemini #8167
Conversation
}; | ||
|
||
/** | ||
Shplonk verifier optimized to verify gemini opening claims. |
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.
Could you add some approprate tags like @brief
, @detail
, @warning
etc?
auto& transcript) | ||
static std::tuple<std::vector<Fr>, std::vector<Fr>, std::vector<Commitment>> reduce_efficient_verification( | ||
const size_t log_circuit_size, // log circuit size | ||
Fr& r, // gemini challenge |
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.
Please don't give anything a single-letter name. It makes it very hard to figure out where this is use, and it also makes renaming the parameter hard. r_challenge
isn't great but it's better for these reasons. Something more specific that will stand the test of time (what if another protocol has a challenge called r
?) is better but since r_challenge
is already in use it prob makes sense to go with that.
GroupElement& batched_f, /* unshifted */ | ||
GroupElement& batched_g, /* to-be-shifted */ | ||
auto& transcript) | ||
static std::tuple<std::vector<Fr>, std::vector<Fr>, std::vector<Commitment>> reduce_efficient_verification( |
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 think the word "reduce" should only be used when you're outputting new claims. Is there a different and more informative native? Or has the notion of "claim" changed?
A_l_fold[j] = A_l[j << 1] + u_l * (A_l[(j << 1) + 1] - A_l[j << 1]); | ||
} | ||
auto idx = static_cast<ptrdiff_t>(j); | ||
auto idx_to_2 = static_cast<ptrdiff_t>(j << 1); |
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.
This is really 2_to_idx
@@ -141,7 +141,7 @@ std::vector<typename GeminiProver_<Curve>::Polynomial> GeminiProver_<Curve>::com | |||
* @param r_challenge univariate opening challenge | |||
*/ | |||
template <typename Curve> | |||
GeminiProverOutput<Curve> GeminiProver_<Curve>::compute_fold_polynomial_evaluations( | |||
std::vector<ProverOpeningClaim<Curve>> GeminiProver_<Curve>::compute_fold_polynomial_evaluations( |
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.
It looks like, with this change, GeminiProverOutput
is no longer in use. Seems reasonable to me--if you agree, will you delete the definition of that struct?
std::span<const Fr> r_squares, | ||
std::span<const Fr> fold_polynomial_evals) | ||
{ | ||
const size_t num_variables = multivariate_challenge.size(); |
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.
IMO you're not getting a lot of mileage out of these aliases, in fact two of them are used only once, and this just amounts to noise unless the aliases add significant clarity.
* @brief Utility for native batch multiplication of group elements | ||
* @note This is used only for native verification and is not optimized for efficiency | ||
*/ | ||
static Commitment batch_mul_native(const std::vector<Commitment>& _points, const std::vector<Fr>& _scalars) |
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.
If Zeromorph weren't going away I would suggest to relocate this function and reuse it. I might still do that myself as a best practice (what if Zm doesn't go away?) but idk, it's not super important here.
return { { z_challenge, Fr(0) }, G_commitment }; | ||
}; | ||
|
||
/** |
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.
Please use the @
tags in your leading comments for clarity / so that doxygen can parse them
// get Shplonk opening point z, it used to check that the evaluation claims and the correctness of the batching | ||
const Fr z_challenge = transcript->template get_challenge<Fr>("Shplonk:z"); | ||
// accumulator for scalar that will be multiplied by [1]_1 | ||
auto constant_term_accumulator = Fr(0); |
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.
Please don't use this pattern; rather use Fr constant_term_accumulator{ 0 };
. We should only instantiate objects with auto
if we have a very compelling reason to do so. Types help us to understand what things are!
log_circuit_size + 2) is performed. In the native setting, these operations are performed sequentially. | ||
* | ||
*/ | ||
static OpeningClaim<Curve> verify_gemini( |
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.
Maybe for unifority we could rename this to something like reduce_verification_for_gemini
?
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.
Also: it feels kind of wrong that so much has moved out of gemini and into here. It also makes it harder to understand the diff. If our implementation of Shplonk now depends on our implementation of Gemini, then perhaps it makes sense to leave more of this stuff in Gemini, keep around the original version for reference (also who knows if we'll want it...) and share code with Shplonk at the expense of including Gemini in Shplonk? If we really wanted, we could make a second moving-around PR where Shplemini is implemented in a third file and there's not Gemini <-> Shplonk dependency.
modified and merged as #8351 |
To perform batch opening of several polynomials and their shifts, the verifier performs (NUM_ALL_ENTITIES + log_circuit_size + 2) group operations, which are combined in 1 batch_mul in the recursive setting
The logic of the Gemini verifier significantly simplified, most of the operations are moved to the method verify_gemini in ShplonkVerifier
Deleted Gemini tests, because they became redundant
Both schemes could now be seamlessly coupled with DeciderVerifier and UltraRecursiveVerifier (tested in si/gemini-zeromorph-comparison)
Const proof size not supported