Skip to content
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

Merged
merged 10 commits into from
Mar 5, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ template <typename BuilderType> class GoblinUltraRecursiveFlavor_ {
using NativeVerificationKey = NativeFlavor::VerificationKey;

// Note(luke): Eventually this may not be needed at all
Copy link
Contributor Author

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

using VerifierCommitmentKey = bb::VerifierCommitmentKey<Curve>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -114,6 +114,7 @@ template <typename BuilderType> class GoblinUltraRecursiveFlavor_ {
*/
VerificationKey(CircuitBuilder* builder, const std::shared_ptr<NativeVerificationKey>& native_key)
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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...

Copy link
Contributor

Choose a reason for hiding this comment

The 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;
Expand Down
3 changes: 2 additions & 1 deletion barretenberg/cpp/src/barretenberg/flavor/ultra_recursive.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ template <typename BuilderType> class UltraRecursiveFlavor_ {
using NativeVerificationKey = NativeFlavor::VerificationKey;

// Note(luke): Eventually this may not be needed at all
using VerifierCommitmentKey = bb::VerifierCommitmentKey<Curve>;
using VerifierCommitmentKey = bb::VerifierCommitmentKey<NativeFlavor::Curve>;

static constexpr size_t NUM_WIRES = UltraFlavor::NUM_WIRES;
// The number of multivariate polynomials on which a sumcheck prover sumcheck operates (including shifts). We often
Expand Down Expand Up @@ -288,6 +288,7 @@ template <typename BuilderType> class UltraRecursiveFlavor_ {
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;
this->pcs_verification_key = native_key->pcs_verification_key;
this->q_m = Commitment::from_witness(builder, native_key->q_m);
this->q_l = Commitment::from_witness(builder, native_key->q_l);
this->q_r = Commitment::from_witness(builder, native_key->q_r);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 instance->proving_key->commitment_key everywhere we want to use the commitment_key, but this should probably be changed to a reference instead of a shared_ptr

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, feel free resolve in a future PR.

{}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ template <typename Flavor> bool DeciderVerifier_<Flavor>::verify_proof(const Hon
multivariate_challenge,
transcript);

auto verified = accumulator->pcs_verification_key->pairing_check(pairing_points[0], pairing_points[1]);
auto verified =
accumulator->verification_key->pcs_verification_key->pairing_check(pairing_points[0], pairing_points[1]);

return sumcheck_verified.value() && verified;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,12 +129,11 @@ std::shared_ptr<typename ProverInstances::Instance> ProtoGalaxyProver_<ProverIns
auto vanishing_polynomial_at_challenge = challenge * (challenge - FF(1));
std::vector<FF> lagranges{ FF(1) - challenge, challenge };

// TODO(https://github.com/AztecProtocol/barretenberg/issues/881): bad pattern
auto next_accumulator = std::make_shared<Instance>();
std::shared_ptr<Instance> next_accumulator = instances[0];
next_accumulator->is_accumulator = true;
next_accumulator->instance_size = instances[0]->instance_size;
next_accumulator->log_instance_size = instances[0]->log_instance_size;
next_accumulator->commitment_key = instances[0]->commitment_key;
next_accumulator->proving_key = instances[0]->proving_key;

// Compute the next target sum and send the next folding parameters to the verifier
FF next_target_sum =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,11 @@ std::shared_ptr<typename VerifierInstances::Instance> ProtoGalaxyVerifier_<Verif
auto vanishing_polynomial_at_challenge = combiner_challenge * (combiner_challenge - FF(1));
auto lagranges = std::vector<FF>{ FF(1) - combiner_challenge, combiner_challenge };

auto next_accumulator = std::make_shared<Instance>();
std::shared_ptr<Instance> next_accumulator = accumulator;
next_accumulator->instance_size = accumulator->instance_size;
next_accumulator->log_instance_size = accumulator->log_instance_size;
next_accumulator->is_accumulator = true;

// Compute next folding parameters
next_accumulator->target_sum =
perturbator_at_challenge * lagranges[0] + vanishing_polynomial_at_challenge * combiner_quotient_at_challenge;
Expand Down Expand Up @@ -200,6 +201,7 @@ std::shared_ptr<typename VerifierInstances::Instance> ProtoGalaxyVerifier_<Verif

next_accumulator->verification_key =
std::make_shared<VerificationKey>(instances[0]->instance_size, instances[0]->public_input_size);
next_accumulator->verification_key->pcs_verification_key = accumulator->verification_key->pcs_verification_key;
size_t vk_idx = 0;
for (auto& expected_vk : next_accumulator->verification_key->get_all()) {
size_t inst = 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ std::shared_ptr<typename VerifierInstances::Instance> ProtoGalaxyRecursiveVerifi

next_accumulator->verification_key =
std::make_shared<VerificationKey>(instances[0]->instance_size, instances[0]->public_input_size);
next_accumulator->verification_key->pcs_verification_key = accumulator->verification_key->pcs_verification_key;
size_t vk_idx = 0;
for (auto& expected_vk : next_accumulator->verification_key->get_all()) {
size_t inst = 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -280,8 +280,9 @@ template <typename RecursiveFlavor> class ProtoGalaxyRecursiveTests : public tes
// decider verifier and check that the result agrees.
DeciderVerifier native_decider_verifier = composer.create_decider_verifier(verifier_accumulator);
auto native_result = native_decider_verifier.verify_proof(decider_proof);
auto recursive_result = native_decider_verifier.accumulator->pcs_verification_key->pairing_check(
pairing_points[0].get_value(), pairing_points[1].get_value());
auto recursive_result =
native_decider_verifier.accumulator->verification_key->pcs_verification_key->pairing_check(
pairing_points[0].get_value(), pairing_points[1].get_value());
EXPECT_EQ(native_result, recursive_result);

// Ensure that the underlying native and recursive decider verification algorithms agree by ensuring
Expand Down
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"
Expand Down Expand Up @@ -46,7 +47,9 @@ template <IsRecursiveFlavor Flavor> class RecursiveVerifierInstance_ {
RecursiveVerifierInstance_(Builder* builder, std::shared_ptr<NativeVerificationKey> vk)
: builder(builder)
, verification_key(std::make_shared<VerificationKey>(builder, vk))
{}
{
verification_key->pcs_verification_key = vk->pcs_verification_key;
}

RecursiveVerifierInstance_(Builder* builder, const std::shared_ptr<VerifierInstance>& instance)
: pub_inputs_offset((instance->pub_inputs_offset))
Expand All @@ -63,6 +66,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()) {
Expand Down Expand Up @@ -105,7 +109,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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if some of these are unnecessary or not

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

Expand All @@ -117,11 +127,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();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,6 @@ template <class Flavor> class ProverInstance_ {

public:
std::shared_ptr<ProvingKey> proving_key;
// currently commitment_key needs to be here, and not accessed through the proving key, since sometimes the proving
// key is null during protogalaxy proving (TODO(https://github.com/AztecProtocol/barretenberg/issues/881)?)
std::shared_ptr<CommitmentKey> commitment_key;

ProverPolynomials prover_polynomials;
WitnessCommitments witness_commitments;
CommitmentLabels commitment_labels;
Expand Down Expand Up @@ -90,8 +86,6 @@ template <class Flavor> class ProverInstance_ {
proving_key->contains_recursive_proof = contains_recursive_proof;

sorted_polynomials = construct_sorted_list_polynomials<Flavor>(circuit, dyadic_circuit_size);

commitment_key = proving_key->commitment_key;
}

ProverInstance_() = default;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,6 @@ template <class Flavor, size_t NUM_ = 2> class VerifierInstance_ {
using RelationSeparator = typename Flavor::RelationSeparator;

std::shared_ptr<VerificationKey> verification_key;
// TODO(https://github.com/AztecProtocol/barretenberg/issues/881)?: Access throutgh vk by making sure vk is
// initialized in Protogalaxy?
std::shared_ptr<VerifierCommitmentKey> pcs_verification_key;
std::vector<FF> public_inputs;
size_t pub_inputs_offset = 0;
size_t public_input_size;
Expand All @@ -38,11 +35,8 @@ template <class Flavor, size_t NUM_ = 2> class VerifierInstance_ {

WitnessCommitments witness_commitments;
CommitmentLabels commitment_labels;
VerifierInstance_()
: pcs_verification_key(std::make_shared<VerifierCommitmentKey>()){};
VerifierInstance_(std::shared_ptr<VerificationKey> vk)
: verification_key(std::move(vk))
, pcs_verification_key(std::make_shared<VerifierCommitmentKey>())
{}
};
} // namespace bb
Loading