-
Notifications
You must be signed in to change notification settings - Fork 234
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: Remove commitment key copy out of instance #4893
Changes from all commits
4af6511
a8ca2ae
db915c0
6d58cd0
4e87f7e
5e21259
d6ff97f
bc4c1e2
c00520a
9c07446
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,7 +41,7 @@ template <typename BuilderType> class GoblinUltraRecursiveFlavor_ { | |
using NativeVerificationKey = NativeFlavor::VerificationKey; | ||
|
||
// Note(luke): Eventually this may not be needed at all | ||
using VerifierCommitmentKey = bb::VerifierCommitmentKey<Curve>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. previously was using the wrong Curve. We want to use the native curve here. |
||
using VerifierCommitmentKey = bb::VerifierCommitmentKey<NativeFlavor::Curve>; | ||
|
||
static constexpr size_t NUM_WIRES = GoblinUltraFlavor::NUM_WIRES; | ||
// The number of multivariate polynomials on which a sumcheck prover sumcheck operates (including shifts). We often | ||
|
@@ -114,6 +114,7 @@ template <typename BuilderType> class GoblinUltraRecursiveFlavor_ { | |
*/ | ||
VerificationKey(CircuitBuilder* builder, const std::shared_ptr<NativeVerificationKey>& native_key) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: is there any point in keeping the other constructor here? It seems dangerous to not set up the verification key with the pcs_verification_key... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Feel free to investigate and resolve here though it may make sense to resolve in a broader PR related to the non-use of copy constructors in the Pg code. |
||
{ | ||
this->pcs_verification_key = native_key->pcs_verification_key; | ||
this->circuit_size = native_key->circuit_size; | ||
this->log_circuit_size = numeric::get_msb(this->circuit_size); | ||
this->num_public_inputs = native_key->num_public_inputs; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,7 @@ DeciderProver_<Flavor>::DeciderProver_(const std::shared_ptr<Instance>& inst, | |
const std::shared_ptr<Transcript>& transcript) | ||
: accumulator(std::move(inst)) | ||
, transcript(transcript) | ||
, commitment_key(inst->commitment_key) | ||
, commitment_key(inst->proving_key->commitment_key) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. storing commitment_key is really only useful because now we don't have to write There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense, feel free resolve in a future PR. |
||
{} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
#pragma once | ||
#include "barretenberg/commitment_schemes/verification_key.hpp" | ||
#include "barretenberg/flavor/flavor.hpp" | ||
#include "barretenberg/relations/relation_parameters.hpp" | ||
#include "barretenberg/sumcheck/instance/verifier_instance.hpp" | ||
|
@@ -63,6 +64,7 @@ template <IsRecursiveFlavor Flavor> class RecursiveVerifierInstance_ { | |
public_input_idx++; | ||
} | ||
verification_key = std::make_shared<VerificationKey>(instance_size, public_input_size); | ||
verification_key->pcs_verification_key = instance->verification_key->pcs_verification_key; | ||
auto other_vks = instance->verification_key->get_all(); | ||
size_t vk_idx = 0; | ||
for (auto& vk : verification_key->get_all()) { | ||
|
@@ -105,7 +107,13 @@ template <IsRecursiveFlavor Flavor> class RecursiveVerifierInstance_ { | |
*/ | ||
VerifierInstance get_value() | ||
{ | ||
VerifierInstance inst; | ||
auto inst_verification_key = std::make_shared<NativeVerificationKey>(instance_size, public_input_size); | ||
inst_verification_key->pcs_verification_key = verification_key->pcs_verification_key; | ||
for (auto [vk, inst_vk] : zip_view(verification_key->get_all(), inst_verification_key->get_all())) { | ||
inst_vk = vk.get_value(); | ||
} | ||
|
||
VerifierInstance inst(inst_verification_key); | ||
inst.pub_inputs_offset = pub_inputs_offset; | ||
inst.public_input_size = public_input_size; | ||
inst.log_instance_size = log_instance_size; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure if some of these are unnecessary or not There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same |
||
|
@@ -117,11 +125,6 @@ template <IsRecursiveFlavor Flavor> class RecursiveVerifierInstance_ { | |
inst_public_input = public_input.get_value(); | ||
} | ||
|
||
inst.verification_key = std::make_shared<NativeVerificationKey>(instance_size, public_input_size); | ||
for (auto [vk, inst_vk] : zip_view(verification_key->get_all(), inst.verification_key->get_all())) { | ||
inst_vk = vk.get_value(); | ||
} | ||
|
||
for (auto [alpha, inst_alpha] : zip_view(alphas, inst.alphas)) { | ||
inst_alpha = alpha.get_value(); | ||
} | ||
|
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 what this comment means, but this PR makes sure that we use it