-
Notifications
You must be signed in to change notification settings - Fork 236
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
feat: prove openings of masking polynomials in ECCVM and Translator #9726
Conversation
{ | ||
const size_t num_opening_claims = opening_claims.size(); | ||
|
||
// {ẑⱼ(r)}ⱼ , where ẑⱼ(r) = 1/zⱼ(r) = 1/(r - xⱼ) | ||
// {ẑⱼ(z)}ⱼ , where ẑⱼ(r) = 1/zⱼ(z) = 1/(z - xⱼ) |
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.
typo
@@ -41,6 +41,42 @@ TYPED_TEST(KZGTest, single) | |||
EXPECT_EQ(this->vk()->pairing_check(pairing_points[0], pairing_points[1]), true); | |||
} | |||
|
|||
/** |
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.
We usually commit to and open univariate polynomials represented in the monomial basis. Here is a correct flow for the univariates given in the Lagrange basis.
@@ -439,5 +466,70 @@ template <typename Curve> class ShpleminiVerifier_ { | |||
commitments.emplace_back(std::move(fold_commitments[j])); | |||
} | |||
} | |||
|
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.
main feature of this PR - the method enabling efficient opening of claimed evaluations of Libra masking univariates. no extra challenges or batch mul calls required
@@ -113,6 +134,22 @@ template <typename Curve> class ShplonkProver_ { | |||
idx++; | |||
} | |||
|
|||
// Take into account the constant proof size in 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.
it's just a usual shplonk stuff taking into account that we have const proof size in gemini
@@ -68,7 +68,7 @@ class ECCVMFlavor { | |||
using Relations = Relations_<FF>; | |||
using LookupRelation = ECCVMLookupRelation<FF>; | |||
|
|||
static constexpr size_t MAX_PARTIAL_RELATION_LENGTH = compute_max_partial_relation_length<Relations, HasZK>(); | |||
static constexpr size_t MAX_PARTIAL_RELATION_LENGTH = compute_max_partial_relation_length<Relations>(); |
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.
don't need this anymore as we are switching to a different evaluation masking technique. same in translator
|
||
// Default constructor |
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 a constructor here, moved a bunch of method from sumcheck.hpp
@@ -190,13 +188,11 @@ template <typename Flavor> class SumcheckProver { | |||
SumcheckOutput<Flavor> prove(ProverPolynomials& full_polynomials, | |||
const bb::RelationParameters<FF>& relation_parameters, | |||
const RelationSeparator alpha, | |||
const std::vector<FF>& gate_challenges) | |||
const std::vector<FF>& gate_challenges, | |||
ZKSumcheckData<Flavor> zk_sumcheck_data = ZKSumcheckData<Flavor>()) |
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.
decoupled the creation of a ZKSumcheckData object from the prove method. now it's taken care by a prover that uses a constructor to create these data and then feeds it to the prove method
if constexpr (Flavor::HasZK) { | ||
output += full_libra_purported_value.value(); | ||
if constexpr (IsECCVMRecursiveFlavor<Flavor>) { | ||
output.self_reduce(); |
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.
we have to self_reduce here
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 only necessary for the ECCVM? is it clear why?
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's only the ECCVMRecursiveFlavor, because it's operating with the scalars in bigfield
…col/aztec-packages into si/zk-sumcheck-plus-shplemini
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.
Looks great, just some minor suggestions about style/comments. Also, is there any more cleanup that can be done in the Flavors as a result of this? I'm thinking about the erroneous NUM_ALL_WITNESS_ENTITIES in the MegaFlavor for example..
OpeningClaim batched_claim = ShplonkProver::prove(commitment_key, opening_claims, transcript); | ||
// Create opening claims for Libra masking univariates | ||
std::vector<OpeningClaim> libra_opening_claims; | ||
if (!libra_univariates.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.
its a little redundant to have this check prior to the loop. If it's empty, the loop will just do nothing right?
@@ -254,6 +271,16 @@ template <typename Curve> class ShpleminiVerifier_ { | |||
commitments.emplace_back(g1_identity); | |||
scalars.emplace_back(constant_term_accumulator); | |||
|
|||
if (!libra_univariate_evaluations.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.
Is there a more clarifying condition that could be used here? e.g is this always empty for non-zk flavors and always non-empty for zk flavors?
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.
exactly, but it's handled somewhat manually because the PCS doesn't know anything about the Flavor. added a clarifying comment
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 probably an indication that the PCS now needs to be templated on Flavor instead of just Curve
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.
agree. I think I'll stop adding stuff to Shplemini quite soon and then we'll have to refactor it, because its interface is getting quite messy
size_t num_libra_univariates = libra_univariate_commitments.size(); | ||
|
||
// compute Shplonk denominators and invert them | ||
for (size_t idx = 0; idx < num_libra_univariates; idx++) { |
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.
Is the number of libra univariates different from the size of the multivariate challenge? If not it might be better to use a range loop here on multivariate_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.
yeah, the size of the challenge is CONST_PROOF_SIZE_LOG_N, and the number of libra univariates if the honest log circuit size. prob we'll need to constify it at some point, but I decided to avoid it here
} | ||
|
||
// needed to keep track of the constant term contribution | ||
const size_t idx_of_constant_term = commitments.size() - 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.
another option here is to simply define contant_term or constant_scalar as a ref then update it below. E.g. Fr& constant_scalar = scalars.back();
then constant_scalar += constant_term;
. just personal preference I suppose
for (size_t idx = 0; idx < log_n; idx++) { | ||
// generate random polynomial | ||
Polynomial libra_polynomial = Polynomial::random(LIBRA_UNIVARIATE_LENGTH); | ||
// create a univariate with the same coefficients (to store an array intead of a vector) |
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.
typo
Polynomial libra_polynomial = Polynomial::random(LIBRA_UNIVARIATE_LENGTH); | ||
// create a univariate with the same coefficients (to store an array intead of a vector) | ||
bb::Univariate<Fr, LIBRA_UNIVARIATE_LENGTH> libra_univariate; | ||
for (size_t i = 0; i < LIBRA_UNIVARIATE_LENGTH; i++) { |
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.
FYI it looks like Univariate has it's own random_element()
method - might be cleaner
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! it's ugly here: Univariates are in the Lagrange basis, Polynomials are in the monomial, which means we can't commit to a Univariate without computing its coefficients in the monomial basis or computing the Lagrange basis using SRS + I want to store the masking data as a std::vector<Univariate<FF, 12>> (could/will be turned into std::array) instead of std::vector
auto eval3_shift = poly3.evaluate_mle(mle_opening_point, true); | ||
|
||
// Collect multilinear evaluations for input to prover | ||
// std::vector<Fr> multilinear_evaluations = { eval1, eval2, eval3, eval4, eval2_shift, eval3_shift }; |
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.
unused?
@@ -61,7 +62,20 @@ template <typename Curve> class ShplonkProver_ { | |||
Q.add_scaled(tmp, current_nu); | |||
current_nu *= nu; | |||
} | |||
for (size_t idx = opening_claims.size(); idx < CONST_PROOF_SIZE_LOG_N + 2; idx++) { |
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.
would be nice to add a comment explaining the significance of CONST_PROOF_SIZE_LOG_N + 2
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 a comment
@@ -82,15 +82,18 @@ polynomials, | |||
using View = typename Accumulator::View; | |||
|
|||
if constexpr (read_index == 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.
are these spaces intentional?
if constexpr (Flavor::HasZK) { | ||
output += full_libra_purported_value.value(); | ||
if constexpr (IsECCVMRecursiveFlavor<Flavor>) { | ||
output.self_reduce(); |
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 only necessary for the ECCVM? is it clear why?
…9726) As a part of ZK-fication of Honk, we have to mask the evaluations of round univariates that the prover sends to the verifier. The evaluations were masked in Sumcheck in PR #7517. However, the logic for proving evaluations of Libra masking polynomials was missing. This PR fixes this issue and enables efficient batch opening of these polynomials. * Added necessary logic to Shplonk Prover, Shplemini Prover, and Shplemini Verifer * Better handling of the ZKSumcheckData * Removed methods and reverted changes that became obsolete because of the new ZK strategy * Enabled the opening of Libra masking univariates in ECCVM and Translator
As a part of ZK-fication of Honk, we have to mask the evaluations of round univariates that the prover sends to the verifier. The evaluations were masked in Sumcheck in PR #7517. However, the logic for proving evaluations of Libra masking polynomials was missing. This PR fixes this issue and enables efficient batch opening of these polynomials.