-
Notifications
You must be signed in to change notification settings - Fork 312
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: mega zk features #9774
feat: mega zk features #9774
Conversation
…col/aztec-packages into si/zk-sumcheck-plus-shplemini
@@ -2,7 +2,9 @@ | |||
|
|||
#include "barretenberg/numeric/bitop/get_msb.hpp" | |||
#include "barretenberg/plonk_honk_shared/library/grand_product_delta.hpp" | |||
#include "barretenberg/transcript/transcript.hpp" | |||
#include "barretenberg/stdlib_circuit_builders/mega_recursive_flavor.hpp" | |||
#include "barretenberg/stdlib_circuit_builders/mega_zk_recursive_flavor.hpp" |
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.
moved flavors from the header
@@ -1,10 +1,7 @@ | |||
#include "barretenberg/stdlib/honk_verifier/ultra_recursive_verifier.hpp" | |||
#include "barretenberg/circuit_checker/circuit_checker.hpp" | |||
#include "barretenberg/common/test.hpp" | |||
#include "barretenberg/stdlib/hash/blake3s/blake3s.hpp" | |||
#include "barretenberg/stdlib/hash/pedersen/pedersen.hpp" | |||
#include "barretenberg/stdlib/primitives/curves/bn254.hpp" |
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.
removed redundant inclusions
@@ -81,7 +81,7 @@ class MegaFlavor { | |||
|
|||
// For instances of this flavour, used in folding, we need a unique sumcheck batching challenges for each | |||
// subrelation. This | |||
// is because using powers of alpha would increase the degree of Protogalaxy polynomial $G$ (the combiner) to much. | |||
// is because using powers of alpha would increase the degree of Protogalaxy polynomial $G$ (the combiner) too much. |
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
@@ -46,9 +46,9 @@ template <typename Flavor> struct ZKSumcheckData { | |||
// Constructor | |||
ZKSumcheckData(const size_t multivariate_d, | |||
std::shared_ptr<typename Flavor::Transcript> transcript, | |||
std::shared_ptr<typename Flavor::ProvingKey> proving_key = nullptr) | |||
std::shared_ptr<typename Flavor::CommitmentKey> commitment_key = nullptr) |
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.
minor refinement, to make it work uniformly with Mega and VMs
auto& engine = numeric::get_debug_randomness(); | ||
|
||
class MegaHonkTests : public ::testing::Test { | ||
protected: | ||
using FlavorTypes = ::testing::Types<MegaFlavor, MegaFlavorWithZK>; |
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.
turned it into a typed tests working with Mega and MegaZK
@@ -11,15 +11,17 @@ | |||
|
|||
using namespace bb; | |||
|
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.
now it's typed
Commitment::one(), | ||
transcript); | ||
BatchOpeningClaim<Curve> opening_claim; | ||
if constexpr (!Flavor::HasZK) { |
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 ugly, but it's temporary. i think we could template Shplemini on Flavor and get rid of this mess
// u_{d-1} (1 - u_{d-1}) = 0 \f$. This equation is satisfied with probability ~ 1/|FF|, in such cases the prover | ||
// has to abort and start ZK Sumcheck anew. | ||
if constexpr (Flavor::HasZK) { | ||
check_that_evals_do_not_leak_witness_data(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.
don't need this with the new zk strategy
|
||
auto [multivariate_challenge, claimed_evaluations, sumcheck_verified] = | ||
sumcheck_output = |
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.
sumcheck output is templated on Flavor and has different number members dep on HasZK flag
…packages into si/zk-sumcheck-mega
// BATCHED_RELATION_PARTIAL_LENGTH = algebraic degree of sumcheck relation *after* multiplying by the `pow_zeta` | ||
// random polynomial e.g. For \sum(x) [A(x) * B(x) + C(x)] * PowZeta(X), relation length = 2 and random relation | ||
// length = 3 | ||
static constexpr size_t BATCHED_RELATION_PARTIAL_LENGTH = NativeFlavor::BATCHED_RELATION_PARTIAL_LENGTH; |
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.
Once we have randomization in rows 1,2,3 in the ExecutionTrace, this value will be incremented by 1 compared to the non-zk Mega flavor
{ | ||
setup_zk_sumcheck_data(multivariate_d, transcript, proving_key); | ||
setup_zk_sumcheck_data(multivariate_d, transcript, commitment_key); |
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 doesn't really make sense that ZKSumcheckData
constructor just has a call to another method with the same parameters, it's an unnecessary layer of indirection, you could move the code in the constructor (also from setup_auxiliary_data
).
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.
Oh thanks! I'm still finding my way around constructors, cleaned up
#include "barretenberg/stdlib_circuit_builders/ultra_flavor.hpp" | ||
#include "barretenberg/stdlib_circuit_builders/ultra_keccak_flavor.hpp" | ||
#include "barretenberg/transcript/transcript.hpp" | ||
// #include "barretenberg/transcript/transcript.hpp" |
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.
stale?
proving_key->gate_challenges); | ||
if constexpr (Flavor::HasZK) { | ||
auto commitment_key = std::make_shared<CommitmentKey>(proving_key->proving_key.circuit_size); | ||
zk_sumcheck_data = ZKSumcheckData<Flavor>(numeric::get_msb(polynomial_size), transcript, commitment_key); |
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.
A third allocation of the commitment key is wasteful here (and the fact that you do not deallocate before sumcheck), I would prefer to have the libra polynomials committed to as part of Oink, before the commitment key is deallocated (and before the commitment key is allocated in ECCVM and Translator).
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 seemes logical to have the generation of zk sumcheck data in Oink as other data that sumcheck is operating on
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 for pointing this out! In all three places I switched to allocating a commitment_key of size Flavor::BATCHED_RELATION_PARTIAL_LENGTH, which is tiny and shouldn't cause any memory issues.
as a more general remark: although it's logical to commit to as many polynomials as possible in Oink, I think it's just way cleaner when the creation of these data, the commitments, and the modifications of these data are more localized: now it appears right before the sumcheck and a part of it is submiited to the Shplemini prover + in the Decider (Recursive) Verifier it is not spread between Oink and the Ultra Verifier
class MegaHonkTests : public ::testing::Test { | ||
protected: | ||
using FlavorTypes = ::testing::Types<MegaFlavor, MegaZKFlavor>; | ||
|
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.
will you please rename this file to mega_honk.test.cpp since there is no composer anymore
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.
done
/** | ||
* @brief Benchmark: Construction of a Ultra Honk proof for a circuit determined by the provided circuit function | ||
*/ | ||
static void construct_proof_megahonk_zk(State& state, |
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 that instead of duplicating the mega honk benchmark file you could just template the MegaHonk benchmark
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 tried honestly, but it's not as nice as with the typed test suites, so prob we could keep two separate bench files for now (and maybe delete the one without zk, when we are done with zk)
libra_commitments.push_back(libra_commitment); | ||
}; | ||
} | ||
SumcheckOutput<Flavor> sumcheck_output; |
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.
nit: SumcheckOutput sumcheck_output = sumcheck.verify(...
auto [multivariate_challenge, claimed_evaluations, sumcheck_verified] = | ||
sumcheck.verify(verification_key->relation_parameters, verification_key->alphas, gate_challenges); | ||
|
||
BatchOpeningClaim<Curve> opening_claim; |
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.
same here
Commitment::one(builder), | ||
transcript); | ||
if constexpr (!Flavor::HasZK) { | ||
opening_claim = Shplemini::compute_batch_opening_claim(key->circuit_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.
This if - else can be avoided if translator concatenation polynomials remain the last arguments in the function signature and the libra.commitments and evaluations are always passed to shplemini. The for loop in Shplemini would simply create no claims if the two data structures are 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.
great suggestion, thanks!
const std::vector<bb::Univariate<FF, LENGTH>>& libra_univariates = {}, | ||
const std::vector<FF>& libra_evaluations = {}) | ||
const std::vector<FF>& libra_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.
thanks for the suggestion. re-ordered the args in the prover and the verifier, bc concatenation groups are used less often than libra evals
@@ -33,7 +33,7 @@ template <IsUltraFlavor Flavor> void DeciderProver_<Flavor>::execute_relation_ch | |||
|
|||
PROFILE_THIS_NAME("sumcheck.prove"); | |||
if constexpr (Flavor::HasZK) { | |||
auto commitment_key = std::make_shared<CommitmentKey>(proving_key->proving_key.circuit_size); | |||
auto commitment_key = std::make_shared<CommitmentKey>(Flavor::BATCHED_RELATION_PARTIAL_LENGTH); |
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.
in a follow-up PR you would be able to refine this based on whether a realocation is needed
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 for the updates!
Introduced MegaZKFlavor and its recursive counterpart.
The DeciderProver for MegaZKFLavor (MegaZKProver) generates random polynomials of small size to mask the round univariates in sumcheck. The evaluations of masking polynomials are batched by the ShpleminiProver/Verifier with the Gemini opening claims.
Such proofs could be verified recursively by running UltraRecursiveVerifier with MegaZKRecursiveFlavor.