-
Notifications
You must be signed in to change notification settings - Fork 268
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: share code between provers #4655
Conversation
Benchmark resultsMetrics with a significant change:
Detailed resultsAll benchmarks are run on txs on the This benchmark source data is available in JSON format on S3 here. Values are compared against data from master at commit L2 block published to L1Each column represents the number of txs on an L2 block published to L1.
L2 chain processingEach column represents the number of blocks on the L2 chain where each block has 16 txs.
Circuits statsStats on running time and I/O sizes collected for every circuit run across all benchmarks.
Tree insertion statsThe duration to insert a fixed batch of leaves into each tree type.
MiscellaneousTransaction sizes based on how many contract classes are registered in the tx.
Transaction processing duration by data writes.
|
|
||
auto [beta, gamma] = | ||
transcript->template get_challenges<typename Flavor::FF>(domain_separator + "beta", domain_separator + "gamma"); | ||
instance->relation_parameters.beta = beta; |
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.
was this forgotten in protogalaxy?
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.
no, line 70 in the protogalaxy_prover.cpp code
auto [beta, gamma] =
transcript->template get_challenges<FF>(domain_separator + "_beta", domain_separator + "_gamma");
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.
No, I meant setting the instance->relation_parameters
witness_commitments.w_o = commitment_key->commit(instance->proving_key->w_o); | ||
|
||
auto wire_comms = witness_commitments.get_wires(); | ||
auto commitment_labels = instance->commitment_labels; |
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.
hoping this auto makes it a const reference
@@ -42,7 +42,7 @@ class UltraTranscriptTests : public ::testing::Test { | |||
size_t frs_per_uint32 = bb::field_conversion::calc_num_bn254_frs<uint32_t>(); | |||
|
|||
size_t round = 0; | |||
manifest_expected.add_entry(round, "circuit_size", frs_per_uint32); | |||
manifest_expected.add_entry(round, "instance_size", frs_per_uint32); |
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.
hacky-ish fix but seems like the best thing to do to align ultra honk prover and folding prover. We need to have the same string in both cases.
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.
see my comment in ultra_verifier
@@ -59,7 +59,7 @@ template <typename Flavor> bool UltraVerifier_<Flavor>::verify_proof(const HonkP | |||
CommitmentLabels commitment_labels; | |||
|
|||
// TODO(Adrian): Change the initialization of the transcript to take the VK hash? | |||
const auto circuit_size = transcript->template receive_from_prover<uint32_t>("circuit_size"); | |||
const auto circuit_size = transcript->template receive_from_prover<uint32_t>("instance_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.
there is no concept of instance in the ultra_verifier so let's stick to 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.
Ok, so you're saying its fine to use circuit_size instead of instance_size for protogalaxy too?
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 don't have strong opinions, since this will be refactored soon I think it's fine to have circuit_size
in some places and instance_size
in the places where we operate on an instance
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.
nono, we have to pick one, because we are sharing the code. I've just picked instance_size for now, but maybe circuit_size is more adept.
@@ -42,7 +42,7 @@ class UltraTranscriptTests : public ::testing::Test { | |||
size_t frs_per_uint32 = bb::field_conversion::calc_num_bn254_frs<uint32_t>(); | |||
|
|||
size_t round = 0; | |||
manifest_expected.add_entry(round, "circuit_size", frs_per_uint32); | |||
manifest_expected.add_entry(round, "instance_size", frs_per_uint32); |
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.
see my comment in ultra_verifier
auto public_input_i = instance->public_inputs[i]; | ||
transcript->send_to_verifier("public_input_" + std::to_string(i), public_input_i); | ||
} | ||
execute_preamble_round_(instance, transcript); |
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'm not a big fan of this nesting with an extra _
, I was envisioning something like PreambleRound::execute(...)
, like maybe having a generic class with an execute
function that all this different types of rounds inherit, a static class if possible
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 I dont see the need for these functions anymore, construct_proof function should look like
PreambleRound::execute(..)
SortedListAccumulatorRound::execute(...)
....
Moreover, we can do this rounds with sumcheck and zeromorph as well, the code is still duplicated across provers. And the decider prover should also benefit from code reuse
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 thought you were removing a lot of the shared code in the decider prover in your PR?
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 all of the extra _ functions too.
{ | ||
instance->initialize_prover_polynomials(); |
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 believe prover polynomials initialise should be in this prover_setup
function, also prover_setup
is a very confusing name in this situation because this code is really processing a circuit. Also because this is something unique to ProtoGalaxy at the moment I dont see the benefit of moving part of it in another file. It makes it harder to understand what instance finalisation means
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.
deleted prover_setup and moved initialize_prover_polynomials() to the constructor of the presumcheck prover
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 this work needs to be rethought a bit, I gave some suggestions in my comments.
test_round_inner(state, prover, index); | ||
state.ResumeTiming(); | ||
// NOTE: google bench is very finnicky, must end in ResumeTiming() for correctness |
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.
not sure why this was removed in the protogalaxy rounds bench. This is required for google bench to function.
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.
hmm, this is not strictly related to this PR and we should be a bit more conservative withchanges to benchmarks so maybe address in a follow-up?
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.
sure, I can just make a separate PR for this
@@ -70,16 +70,15 @@ void ProtoGalaxyVerifier_<VerifierInstances>::receive_and_finalise_instance(cons | |||
inst->log_instance_size = static_cast<size_t>(numeric::get_msb(inst->instance_size)); | |||
inst->public_input_size = | |||
transcript->template receive_from_prover<uint32_t>(domain_separator + "_public_input_size"); | |||
inst->pub_inputs_offset = |
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 above the public inputs to align with ultra_prover's ordering of what it sends
, commitment_key(commitment_key) | ||
{ | ||
instance->initialize_prover_polynomials(); |
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 was moved to the PreSumcheckProver
pre_sumcheck_prover.execute_sorted_list_accumulator_round(); | ||
pre_sumcheck_prover.execute_log_derivative_inverse_round(); | ||
pre_sumcheck_prover.execute_grand_product_computation_round(); | ||
for (size_t idx = 0; idx < Flavor::NUM_SUBRELATIONS - 1; idx++) { | ||
instance->alphas[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.
this alphas generation is part of sumcheck so its not included in the shared prover
@@ -59,7 +59,7 @@ template <typename Flavor> bool UltraVerifier_<Flavor>::verify_proof(const HonkP | |||
CommitmentLabels commitment_labels; | |||
|
|||
// TODO(Adrian): Change the initialization of the transcript to take the VK hash? | |||
const auto circuit_size = transcript->template receive_from_prover<uint32_t>("circuit_size"); | |||
const auto circuit_size = transcript->template receive_from_prover<uint32_t>("instance_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.
i don't have strong opinions, since this will be refactored soon I think it's fine to have circuit_size
in some places and instance_size
in the places where we operate on an instance
barretenberg/cpp/src/barretenberg/protogalaxy/presumcheck_prover.hpp
Outdated
Show resolved
Hide resolved
|
||
namespace bb { | ||
|
||
template <IsUltraFlavor Flavor> class PreSumcheckProver { |
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.
Can you please add documentation to this class.. also maybe we should discuss whether a prover inside a prover is the right approach, I was envisioning an architecture where we have some shared rounds that can be in a utility class, similar to the utility class for operating on relations that both sumcheck and protogalaxy uses. It would be nice to have this class be static but might require some more refactoring work.
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 add more comments. Prover inside prover is already our current standard. We have a SumcheckProver and ZeromorphProver both inside UltraProver.
I agree that the PreSumCheckProver can be split into further classes for each Round, and can eventually be made static or something like that.
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.
That is a good point, I forgot about those. What do you think about creating a PreSumchVerifier as well (since we have a SumcheckVerifier and ZeromorphVerifier)? If you prefer not to do this in this PR, I think an issue should be added
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 can just go into a followup PR.
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.
ok, please add an issue
instance->witness_commitments.z_lookup); | ||
for (size_t idx = 0; idx < NUM_SUBRELATIONS - 1; idx++) { | ||
PreSumcheckProver<Flavor> pre_sumcheck_prover(instance, commitment_key, transcript, domain_separator + '_'); | ||
pre_sumcheck_prover.execute_preamble_round(); |
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 would call these just pre_sumcheck to be more compliant with how the other inner provers are used (Sumcheck and Zeromorph)
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 it should be clarified if its the prover
template <IsUltraFlavor Flavor> void PreSumcheckProver<Flavor>::execute_grand_product_computation_round() | ||
{ | ||
auto& witness_commitments = instance->witness_commitments; | ||
const auto& commitment_labels = instance->commitment_labels; |
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.
commitment_labels are static and come from flavor so they could become a field in PreSumcheckProver
rather than extracting from instance every time
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 making a const reference of them, so it should be fine?
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.
actually, next PR will do this and remove it from instance at the same time.
for (size_t idx = 0; idx < NUM_SUBRELATIONS - 1; idx++) { | ||
PreSumcheckProver<Flavor> pre_sumcheck_prover(instance, commitment_key, transcript, domain_separator + '_'); | ||
pre_sumcheck_prover.execute_preamble_round(); | ||
pre_sumcheck_prover.execute_wire_commitments_round(); |
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.
can you please add comments summarising what each of these calls do for readability in a similar way they are found in the UltraProver
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
@@ -187,19 +67,19 @@ template <IsUltraFlavor Flavor> HonkProof& UltraProver_<Flavor>::export_proof() | |||
template <IsUltraFlavor Flavor> HonkProof& UltraProver_<Flavor>::construct_proof() | |||
{ | |||
// Add circuit size public input size and public inputs to transcript-> | |||
execute_preamble_round(); | |||
pre_sumcheck_prover.execute_preamble_round(); |
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 here I would call these just pre_sumcheck
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.
Approved, thank you for the back and forth, will you please resolve my final comments, mostly asking for some renaming and a bit more documentation
c862f96
to
f8d66fc
Compare
Merge remote-tracking branch 'origin/master' into lx/provers-round-sharing
const auto public_input_size = | ||
transcript->template receive_from_prover<FF>(domain_separator + "_public_input_size"); | ||
inst->verification_key->circuit_size = uint32_t(instance_size.get_value()); | ||
inst->verification_key->log_circuit_size = | ||
static_cast<size_t>(numeric::get_msb(inst->verification_key->circuit_size)); | ||
inst->verification_key->num_public_inputs = uint32_t(public_input_size.get_value()); | ||
const auto pub_inputs_offset = |
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.
reordered for consistency with oink prover
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.
Just read at a high level -- looks good!
void execute_log_derivative_inverse_round(); | ||
void execute_grand_product_computation_round(); | ||
|
||
std::shared_ptr<Instance> instance; |
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.
Our convention is to put data members at the top. Would you please?
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.
LGTM, will you please resolve my last two comments (type + request for an extra comment)?
time_if_index(SORTED_LIST_ACCUMULATOR, [&] { prover.execute_sorted_list_accumulator_round(); }); | ||
time_if_index(LOG_DERIVATIVE_INVERSE, [&] { prover.execute_log_derivative_inverse_round(); }); | ||
time_if_index(GRAND_PRODUCT_COMPUTATION, [&] { prover.execute_grand_product_computation_round(); }); | ||
time_if_index(PREAMBLE, [&] { prover.oink_prover.execute_preamble_round(); }); |
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.
oink?😅😅
const auto num_public_inputs = static_cast<uint32_t>(instance->proving_key->num_public_inputs); | ||
transcript->send_to_verifier(domain_separator + "_instance_size", instance_size); | ||
transcript->send_to_verifier(domain_separator + "_public_input_size", num_public_inputs); | ||
// Add circuit size public input size and public inputs to transcript-> |
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 arrow
transcript->send_to_verifier(domain_separator + "_" + commitment_labels.z_perm, | ||
instance->witness_commitments.z_perm); | ||
transcript->send_to_verifier(domain_separator + "_" + commitment_labels.z_lookup, | ||
instance->witness_commitments.z_lookup); | ||
for (size_t idx = 0; idx < NUM_SUBRELATIONS - 1; 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.
Can you please add a comment explaining why we generate alphas here in PG?
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.27.1</summary> ## [0.27.1](aztec-package-v0.27.0...aztec-package-v0.27.1) (2024-03-12) ### Miscellaneous * **aztec-package:** Synchronize aztec-packages versions </details> <details><summary>barretenberg.js: 0.27.1</summary> ## [0.27.1](barretenberg.js-v0.27.0...barretenberg.js-v0.27.1) (2024-03-12) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-cli: 0.27.1</summary> ## [0.27.1](aztec-cli-v0.27.0...aztec-cli-v0.27.1) (2024-03-12) ### Miscellaneous * **aztec-cli:** Synchronize aztec-packages versions </details> <details><summary>aztec-packages: 0.27.1</summary> ## [0.27.1](aztec-packages-v0.27.0...aztec-packages-v0.27.1) (2024-03-12) ### Features * Further ClientIVC breakdown ([#5146](#5146)) ([c8e1cb8](c8e1cb8)) * Nullifier non membership ([#5152](#5152)) ([426bd6d](426bd6d)) ### Bug Fixes * Increase the json limit for RPC requests ([#5161](#5161)) ([419958c](419958c)) * Move timers for ClientIVC breakdown ([#5145](#5145)) ([5457edb](5457edb)) ### Miscellaneous * **boxes:** Adding clone contract option ([#4980](#4980)) ([a427aa5](a427aa5)) * Share code between provers ([#4655](#4655)) ([ef10d65](ef10d65)) </details> <details><summary>barretenberg: 0.27.1</summary> ## [0.27.1](barretenberg-v0.27.0...barretenberg-v0.27.1) (2024-03-12) ### Features * Further ClientIVC breakdown ([#5146](#5146)) ([c8e1cb8](c8e1cb8)) ### Bug Fixes * Move timers for ClientIVC breakdown ([#5145](#5145)) ([5457edb](5457edb)) ### Miscellaneous * Share code between provers ([#4655](#4655)) ([ef10d65](ef10d65)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.27.1</summary> ## [0.27.1](AztecProtocol/aztec-packages@aztec-package-v0.27.0...aztec-package-v0.27.1) (2024-03-12) ### Miscellaneous * **aztec-package:** Synchronize aztec-packages versions </details> <details><summary>barretenberg.js: 0.27.1</summary> ## [0.27.1](AztecProtocol/aztec-packages@barretenberg.js-v0.27.0...barretenberg.js-v0.27.1) (2024-03-12) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-cli: 0.27.1</summary> ## [0.27.1](AztecProtocol/aztec-packages@aztec-cli-v0.27.0...aztec-cli-v0.27.1) (2024-03-12) ### Miscellaneous * **aztec-cli:** Synchronize aztec-packages versions </details> <details><summary>aztec-packages: 0.27.1</summary> ## [0.27.1](AztecProtocol/aztec-packages@aztec-packages-v0.27.0...aztec-packages-v0.27.1) (2024-03-12) ### Features * Further ClientIVC breakdown ([#5146](AztecProtocol/aztec-packages#5146)) ([c8e1cb8](AztecProtocol/aztec-packages@c8e1cb8)) * Nullifier non membership ([#5152](AztecProtocol/aztec-packages#5152)) ([426bd6d](AztecProtocol/aztec-packages@426bd6d)) ### Bug Fixes * Increase the json limit for RPC requests ([#5161](AztecProtocol/aztec-packages#5161)) ([419958c](AztecProtocol/aztec-packages@419958c)) * Move timers for ClientIVC breakdown ([#5145](AztecProtocol/aztec-packages#5145)) ([5457edb](AztecProtocol/aztec-packages@5457edb)) ### Miscellaneous * **boxes:** Adding clone contract option ([#4980](AztecProtocol/aztec-packages#4980)) ([a427aa5](AztecProtocol/aztec-packages@a427aa5)) * Share code between provers ([#4655](AztecProtocol/aztec-packages#4655)) ([ef10d65](AztecProtocol/aztec-packages@ef10d65)) </details> <details><summary>barretenberg: 0.27.1</summary> ## [0.27.1](AztecProtocol/aztec-packages@barretenberg-v0.27.0...barretenberg-v0.27.1) (2024-03-12) ### Features * Further ClientIVC breakdown ([#5146](AztecProtocol/aztec-packages#5146)) ([c8e1cb8](AztecProtocol/aztec-packages@c8e1cb8)) ### Bug Fixes * Move timers for ClientIVC breakdown ([#5145](AztecProtocol/aztec-packages#5145)) ([5457edb](AztecProtocol/aztec-packages@5457edb)) ### Miscellaneous * Share code between provers ([#4655](AztecProtocol/aztec-packages#4655)) ([ef10d65](AztecProtocol/aztec-packages@ef10d65)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
A lot of code is repeated between the Decider prover, folding prover, and ultra honk prover. This PR aims to reduce duplication by creating a PreSumcheckProver, which supports the 5 round functions before sumcheck.
It does not address the shared code between verifiers or the shared code between prover and verifier. It also is an initial step at a round abstraction, where each round is implemented as a separate class and the data being used/modified in each round is clearly defined.
Resolves AztecProtocol/barretenberg#795.