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: bye bye shared ptrs for ultra/goblin ultra proving_keys :) #5407

Merged
merged 32 commits into from
Apr 1, 2024
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
d132a10
moved sorted_polynomials to proving key
lucasxia01 Mar 19, 2024
eec17c6
make prover polys constructor from proving key
lucasxia01 Mar 20, 2024
45c5ee2
eliminated initialize_prover_polynomials
lucasxia01 Mar 20, 2024
810d369
moved compute_sorted_list_accumulator to proving_key
lucasxia01 Mar 20, 2024
e76c972
test fix
lucasxia01 Mar 20, 2024
6078832
moved compute_sorted_accumulator_polynomials and made updates to rela…
lucasxia01 Mar 20, 2024
b0855d5
test fix (prover polys wasn't being updated correctly)
lucasxia01 Mar 20, 2024
f4aa979
add OinkProverOutput and make one prove() function
lucasxia01 Mar 21, 2024
1f4e307
proverpolynomials now is just a temporary object during oink
lucasxia01 Mar 21, 2024
3fe93cc
Merge remote-tracking branch 'origin/master' into lx/refactor-prover-…
lucasxia01 Mar 21, 2024
2798eaf
moved compute_logderivative_inverse() from instance to proving_key
lucasxia01 Mar 21, 2024
4030744
moved compute_grand_product_polynomials from instance to proving key
lucasxia01 Mar 21, 2024
e11bb7e
removed the last instance reference that is non instance->proving_key!
lucasxia01 Mar 21, 2024
b4c4354
oink says bye bye to instance 👋
lucasxia01 Mar 21, 2024
2b67229
small fixes to tests
lucasxia01 Mar 21, 2024
abd0bd7
removed stupid shared_ptr calling copy constructor of proving_key (ad…
lucasxia01 Mar 21, 2024
8923c8c
deleting copy constructor of proving key to prevent copying
lucasxia01 Mar 22, 2024
c425e3d
updated based on comments
lucasxia01 Mar 22, 2024
2b6aae9
bye bye shared ptrs for ultra/goblin ultra proving_keys :)
lucasxia01 Mar 22, 2024
2d019cd
bye bye shared ptrs for ultra/goblin ultra proving_keys :)
lucasxia01 Mar 22, 2024
994c767
Merge branch 'lx/clean-shared-ptr' of github.com:AztecProtocol/aztec-…
lucasxia01 Mar 22, 2024
de50495
fixed pg prover (shouldn't move too early)
lucasxia01 Mar 22, 2024
c7192bc
remove comments
lucasxia01 Mar 22, 2024
7e27f9c
proving key gets copied instead of shared
lucasxia01 Mar 22, 2024
70ff641
Merge remote-tracking branch 'origin/master' into lx/clean-shared-ptr
lucasxia01 Mar 25, 2024
6397673
Merge remote-tracking branch 'origin/master' into lx/clean-shared-ptr
lucasxia01 Mar 25, 2024
d1a4965
Merge remote-tracking branch 'origin/master' into lx/clean-shared-ptr
lucasxia01 Mar 26, 2024
03238e8
get rid of reference member var and use std::move()s instead
lucasxia01 Mar 26, 2024
35ae332
fix for pg tests
lucasxia01 Mar 26, 2024
7b4a20e
removing oink prover as a member variable of ultra_prover to fix test…
lucasxia01 Mar 26, 2024
7f79def
Merge remote-tracking branch 'origin/master' into lx/clean-shared-ptr
lucasxia01 Apr 1, 2024
bf9a7f7
Merge remote-tracking branch 'origin/master' into lx/clean-shared-ptr
lucasxia01 Apr 1, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ TEST_F(MockKernelTest, PinFoldingKernelSizes)
kernel_circuit, { func_fold_proof, ivc.vks.func_vk }, {}, kernel_acc);

auto kernel_fold_proof = ivc.accumulate(kernel_circuit);
EXPECT_EQ(ivc.prover_instance->proving_key->log_circuit_size, 17);
EXPECT_EQ(ivc.prover_instance->proving_key.log_circuit_size, 17);

GoblinUltraCircuitBuilder circuit_3{ ivc.goblin.op_queue };
GoblinMockCircuits::construct_mock_function_circuit(circuit_3);
Expand All @@ -49,5 +49,5 @@ TEST_F(MockKernelTest, PinFoldingKernelSizes)
{ func_fold_proof, ivc.vks.func_vk },
kernel_acc);
auto instance = std::make_shared<ClientIVC::ProverInstance>(kernel_circuit);
EXPECT_EQ(instance->proving_key->log_circuit_size, 17);
EXPECT_EQ(instance->proving_key.log_circuit_size, 17);
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,10 @@
namespace bb {

template <class Flavor>
void ExecutionTrace_<Flavor>::populate(Builder& builder,
const std::shared_ptr<typename Flavor::ProvingKey>& proving_key)
void ExecutionTrace_<Flavor>::populate(Builder& builder, typename Flavor::ProvingKey& proving_key)
{
// Construct wire polynomials, selector polynomials, and copy cycles from raw circuit data
auto trace_data = construct_trace_data(builder, proving_key->circuit_size);
auto trace_data = construct_trace_data(builder, proving_key.circuit_size);

add_wires_and_selectors_to_proving_key(trace_data, builder, proving_key);

Expand All @@ -23,46 +22,48 @@ void ExecutionTrace_<Flavor>::populate(Builder& builder,
}

// Compute the permutation argument polynomials (sigma/id) and add them to proving key
compute_permutation_argument_polynomials<Flavor>(builder, proving_key.get(), trace_data.copy_cycles);
compute_permutation_argument_polynomials<Flavor>(builder, &proving_key, trace_data.copy_cycles);
}

template <class Flavor>
void ExecutionTrace_<Flavor>::add_wires_and_selectors_to_proving_key(
TraceData& trace_data, Builder& builder, const std::shared_ptr<typename Flavor::ProvingKey>& proving_key)
void ExecutionTrace_<Flavor>::add_wires_and_selectors_to_proving_key(TraceData& trace_data,
Builder& builder,
typename Flavor::ProvingKey& proving_key)
{
if constexpr (IsHonkFlavor<Flavor>) {
for (auto [pkey_wire, trace_wire] : zip_view(proving_key->get_wires(), trace_data.wires)) {
for (auto [pkey_wire, trace_wire] : zip_view(proving_key.get_wires(), trace_data.wires)) {
pkey_wire = trace_wire.share();
}
for (auto [pkey_selector, trace_selector] : zip_view(proving_key->get_selectors(), trace_data.selectors)) {
for (auto [pkey_selector, trace_selector] : zip_view(proving_key.get_selectors(), trace_data.selectors)) {
pkey_selector = trace_selector.share();
}
proving_key->pub_inputs_offset = trace_data.pub_inputs_offset;
proving_key.pub_inputs_offset = trace_data.pub_inputs_offset;
} else if constexpr (IsPlonkFlavor<Flavor>) {
for (size_t idx = 0; idx < trace_data.wires.size(); ++idx) {
std::string wire_tag = "w_" + std::to_string(idx + 1) + "_lagrange";
proving_key->polynomial_store.put(wire_tag, std::move(trace_data.wires[idx]));
proving_key.polynomial_store.put(wire_tag, std::move(trace_data.wires[idx]));
}
for (size_t idx = 0; idx < trace_data.selectors.size(); ++idx) {
proving_key->polynomial_store.put(builder.selector_names[idx] + "_lagrange",
std::move(trace_data.selectors[idx]));
proving_key.polynomial_store.put(builder.selector_names[idx] + "_lagrange",
std::move(trace_data.selectors[idx]));
}
}
}

template <class Flavor>
void ExecutionTrace_<Flavor>::add_memory_records_to_proving_key(
TraceData& trace_data, Builder& builder, const std::shared_ptr<typename Flavor::ProvingKey>& proving_key)
void ExecutionTrace_<Flavor>::add_memory_records_to_proving_key(TraceData& trace_data,
Builder& builder,
typename Flavor::ProvingKey& proving_key)
requires IsUltraPlonkOrHonk<Flavor>
{
ASSERT(proving_key->memory_read_records.empty() && proving_key->memory_write_records.empty());
ASSERT(proving_key.memory_read_records.empty() && proving_key.memory_write_records.empty());

// Update indices of RAM/ROM reads/writes based on where block containing these gates sits in the trace
for (auto& index : builder.memory_read_records) {
proving_key->memory_read_records.emplace_back(index + trace_data.ram_rom_offset);
proving_key.memory_read_records.emplace_back(index + trace_data.ram_rom_offset);
}
for (auto& index : builder.memory_write_records) {
proving_key->memory_write_records.emplace_back(index + trace_data.ram_rom_offset);
proving_key.memory_write_records.emplace_back(index + trace_data.ram_rom_offset);
}
}

Expand Down Expand Up @@ -135,32 +136,32 @@ template <class Flavor> void ExecutionTrace_<Flavor>::populate_public_inputs_blo
}

template <class Flavor>
void ExecutionTrace_<Flavor>::add_ecc_op_wires_to_proving_key(
Builder& builder, const std::shared_ptr<typename Flavor::ProvingKey>& proving_key)
void ExecutionTrace_<Flavor>::add_ecc_op_wires_to_proving_key(Builder& builder,
typename Flavor::ProvingKey& proving_key)
requires IsGoblinFlavor<Flavor>
{
// Initialize the ecc op wire polynomials to zero on the whole domain
std::array<Polynomial, NUM_WIRES> op_wire_polynomials;
for (auto& poly : op_wire_polynomials) {
poly = Polynomial{ proving_key->circuit_size };
poly = Polynomial{ proving_key.circuit_size };
}
Polynomial ecc_op_selector{ proving_key->circuit_size };
Polynomial ecc_op_selector{ proving_key.circuit_size };

// Copy the ecc op data from the conventional wires into the op wires over the range of ecc op gates
const size_t op_wire_offset = Flavor::has_zero_row ? 1 : 0;
for (auto [ecc_op_wire, wire] : zip_view(op_wire_polynomials, proving_key->get_wires())) {
for (auto [ecc_op_wire, wire] : zip_view(op_wire_polynomials, proving_key.get_wires())) {
for (size_t i = 0; i < builder.blocks.ecc_op.size(); ++i) {
size_t idx = i + op_wire_offset;
ecc_op_wire[idx] = wire[idx];
ecc_op_selector[idx] = 1; // construct the selector as the indicator on the ecc op block
}
}

proving_key->ecc_op_wire_1 = op_wire_polynomials[0].share();
proving_key->ecc_op_wire_2 = op_wire_polynomials[1].share();
proving_key->ecc_op_wire_3 = op_wire_polynomials[2].share();
proving_key->ecc_op_wire_4 = op_wire_polynomials[3].share();
proving_key->lagrange_ecc_op = ecc_op_selector.share();
proving_key.ecc_op_wire_1 = op_wire_polynomials[0].share();
proving_key.ecc_op_wire_2 = op_wire_polynomials[1].share();
proving_key.ecc_op_wire_3 = op_wire_polynomials[2].share();
proving_key.ecc_op_wire_4 = op_wire_polynomials[3].share();
proving_key.lagrange_ecc_op = ecc_op_selector.share();
}

template class ExecutionTrace_<UltraFlavor>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ template <class Flavor> class ExecutionTrace_ {
*
* @param builder
*/
static void populate(Builder& builder, const std::shared_ptr<ProvingKey>&);
static void populate(Builder& builder, ProvingKey&);

private:
/**
Expand All @@ -54,7 +54,7 @@ template <class Flavor> class ExecutionTrace_ {
*/
static void add_wires_and_selectors_to_proving_key(TraceData& trace_data,
Builder& builder,
const std::shared_ptr<typename Flavor::ProvingKey>& proving_key);
typename Flavor::ProvingKey& proving_key);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use the alias here rather than this heavier notation.


/**
* @brief Add the memory records indicating which rows correspond to RAM/ROM reads/writes
Expand All @@ -70,7 +70,7 @@ template <class Flavor> class ExecutionTrace_ {
*/
static void add_memory_records_to_proving_key(TraceData& trace_data,
Builder& builder,
const std::shared_ptr<typename Flavor::ProvingKey>& proving_key)
typename Flavor::ProvingKey& proving_key)
requires IsUltraPlonkOrHonk<Flavor>;

/**
Expand Down Expand Up @@ -98,8 +98,7 @@ template <class Flavor> class ExecutionTrace_ {
* @param builder
* @param proving_key
*/
static void add_ecc_op_wires_to_proving_key(Builder& builder,
const std::shared_ptr<typename Flavor::ProvingKey>& proving_key)
static void add_ecc_op_wires_to_proving_key(Builder& builder, typename Flavor::ProvingKey& proving_key)
requires IsGoblinFlavor<Flavor>;
};

Expand Down
13 changes: 0 additions & 13 deletions barretenberg/cpp/src/barretenberg/flavor/flavor.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -165,19 +165,6 @@ class VerificationKey_ : public PrecomputedCommitments {
this->log_circuit_size = numeric::get_msb(circuit_size);
this->num_public_inputs = num_public_inputs;
};

template <typename ProvingKeyPtr> VerificationKey_(const ProvingKeyPtr& proving_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.

this function with a provingkeyptr template is pretty bad

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead, I put them in the specific flavor classes

{
this->pcs_verification_key = std::make_shared<VerifierCommitmentKey>();
this->circuit_size = proving_key->circuit_size;
this->log_circuit_size = numeric::get_msb(this->circuit_size);
this->num_public_inputs = proving_key->num_public_inputs;
this->pub_inputs_offset = proving_key->pub_inputs_offset;

for (auto [polynomial, commitment] : zip_view(proving_key->get_precomputed_polynomials(), this->get_all())) {
commitment = proving_key->commitment_key->commit(polynomial);
}
}
};

// Because of how Gemini is written, is importat to put the polynomials out in this order.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ TEST_F(MockCircuitsPinning, FunctionSizes)
GoblinMockCircuits::construct_mock_function_circuit(app_circuit, large);
auto instance = std::make_shared<ProverInstance>(app_circuit);
if (large) {
EXPECT_EQ(instance->proving_key->log_circuit_size, 19);
EXPECT_EQ(instance->proving_key.log_circuit_size, 19);
} else {
EXPECT_EQ(instance->proving_key->log_circuit_size, 17);
EXPECT_EQ(instance->proving_key.log_circuit_size, 17);
};
};
run_test(true);
Expand All @@ -51,9 +51,9 @@ TEST_F(MockCircuitsPinning, RecursionKernelSizes)

auto instance = std::make_shared<ProverInstance>(kernel_circuit);
if (large) {
EXPECT_EQ(instance->proving_key->log_circuit_size, 17);
EXPECT_EQ(instance->proving_key.log_circuit_size, 17);
} else {
EXPECT_EQ(instance->proving_key->log_circuit_size, 17);
EXPECT_EQ(instance->proving_key.log_circuit_size, 17);
};
}
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ std::shared_ptr<plonk::proving_key> StandardComposer::compute_proving_key(Circui
subgroup_size, circuit_constructor.public_inputs.size(), crs, CircuitType::STANDARD);

// Construct and add to proving key the wire, selector and copy constraint polynomials
Trace::populate(circuit_constructor, circuit_proving_key);
Trace::populate(circuit_constructor, *circuit_proving_key);
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it would have spiraled here to handle not through a pointer here? Seems reasonable to leave as a pointer in that case since Plonk is on its way out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed


// Make all selectors nonzero
enforce_nonzero_selector_polynomials(circuit_constructor, circuit_proving_key.get());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ std::shared_ptr<proving_key> UltraComposer::compute_proving_key(CircuitBuilder&
std::make_shared<plonk::proving_key>(subgroup_size, circuit.public_inputs.size(), crs, CircuitType::ULTRA);

// Construct and add to proving key the wire, selector and copy constraint polynomials
Trace::populate(circuit, circuit_proving_key);
Trace::populate(circuit, *circuit_proving_key);

enforce_nonzero_selector_polynomials(circuit, circuit_proving_key.get());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ void print_databus_info(auto& prover_instance)
{
info("\nInstance Inspector: Printing databus gate info.");
auto& key = prover_instance->proving_key;
for (size_t idx = 0; idx < prover_instance->proving_key->circuit_size; ++idx) {
for (size_t idx = 0; idx < prover_instance->proving_key.circuit_size; ++idx) {
if (key->q_busread[idx] == 1) {
info("idx = ", idx);
info("q_busread = ", key->q_busread[idx]);
Expand Down
12 changes: 6 additions & 6 deletions barretenberg/cpp/src/barretenberg/protogalaxy/combiner.test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ TEST(Protogalaxy, CombinerOn2Instances)
/*log_circuit_size=*/1, idx * 128);
restrict_to_standard_arithmetic_relation(prover_polynomials);
instance->prover_polynomials = std::move(prover_polynomials);
instance->proving_key = std::make_shared<Flavor::ProvingKey>();
instance->proving_key->circuit_size = 2;
instance->proving_key = Flavor::ProvingKey();
instance->proving_key.circuit_size = 2;
instance_data[idx] = instance;
}

Expand Down Expand Up @@ -78,8 +78,8 @@ TEST(Protogalaxy, CombinerOn2Instances)
/*log_circuit_size=*/1);
restrict_to_standard_arithmetic_relation(prover_polynomials);
instance->prover_polynomials = std::move(prover_polynomials);
instance->proving_key = std::make_shared<Flavor::ProvingKey>();
instance->proving_key->circuit_size = 2;
instance->proving_key = Flavor::ProvingKey();
instance->proving_key.circuit_size = 2;
instance_data[idx] = instance;
}

Expand Down Expand Up @@ -169,8 +169,8 @@ TEST(Protogalaxy, CombinerOn4Instances)
auto prover_polynomials = get_zero_prover_polynomials<Flavor>(
/*log_circuit_size=*/1);
instance->prover_polynomials = std::move(prover_polynomials);
instance->proving_key = std::make_shared<Flavor::ProvingKey>();
instance->proving_key->circuit_size = 2;
instance->proving_key = Flavor::ProvingKey();
instance->proving_key.circuit_size = 2;
instance_data[idx] = instance;
}

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->proving_key->commitment_key)
, commitment_key(inst->proving_key.commitment_key)
{}

/**
Expand All @@ -28,7 +28,7 @@ DeciderProver_<Flavor>::DeciderProver_(const std::shared_ptr<Instance>& inst,
template <IsUltraFlavor Flavor> void DeciderProver_<Flavor>::execute_relation_check_rounds()
{
using Sumcheck = SumcheckProver<Flavor>;
auto instance_size = accumulator->proving_key->circuit_size;
auto instance_size = accumulator->proving_key.circuit_size;
auto sumcheck = Sumcheck(instance_size, transcript);
sumcheck_output = sumcheck.prove(accumulator);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ template <typename Flavor> class ProtoGalaxyTests : public testing::Test {

static void check_accumulator_target_sum_manual(std::shared_ptr<ProverInstance>& accumulator, bool expected_result)
{
auto instance_size = accumulator->proving_key->circuit_size;
auto instance_size = accumulator->proving_key.circuit_size;
auto expected_honk_evals = ProtoGalaxyProver::compute_full_honk_evaluations(
accumulator->prover_polynomials, accumulator->alphas, accumulator->relation_parameters);
// Construct pow(\vec{betas*}) as in the paper
Expand Down Expand Up @@ -122,11 +122,11 @@ template <typename Flavor> class ProtoGalaxyTests : public testing::Test {
instance->relation_parameters.beta = FF::random_element();
instance->relation_parameters.gamma = FF::random_element();

instance->proving_key->compute_sorted_accumulator_polynomials(instance->relation_parameters.eta);
instance->proving_key.compute_sorted_accumulator_polynomials(instance->relation_parameters.eta);
if constexpr (IsGoblinFlavor<Flavor>) {
instance->proving_key->compute_logderivative_inverse(instance->relation_parameters);
instance->proving_key.compute_logderivative_inverse(instance->relation_parameters);
}
instance->proving_key->compute_grand_product_polynomials(instance->relation_parameters);
instance->proving_key.compute_grand_product_polynomials(instance->relation_parameters);
instance->prover_polynomials = ProverPolynomials(instance->proving_key);

for (auto& alpha : instance->alphas) {
Expand Down
Loading
Loading