Skip to content

feat: share the commitment key between instances to reduce mem #8154

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.

Sign up for GitHub

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

Merged
lucasxia01 merged 4 commits into from
Aug 22, 2024
Merged

feat: share the commitment key between instances to reduce mem #8154

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a070c3f
share the comm key !
lucasxia01 Aug 22, 2024
b1c9abf
fix ultra keccak flavor
lucasxia01 Aug 22, 2024
e78d42b
made changes to aztecivc
lucasxia01 Aug 22, 2024
766f7d8
get rid of base class constructors in proving keys
lucasxia01 Aug 22, 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
8 changes: 7 additions & 1 deletion barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,13 @@ void ClientIVC::accumulate(ClientCircuit& circuit, const std::shared_ptr<Verific
circuit.add_recursive_proof(stdlib::recursion::init_default_agg_obj_indices<ClientCircuit>(circuit));

// Construct the prover instance for circuit
auto prover_instance = std::make_shared<ProverInstance>(circuit, trace_structure);
std::shared_ptr<ProverInstance> prover_instance;
if (!initialized) {
prover_instance = std::make_shared<ProverInstance>(circuit, trace_structure);
} else {
prover_instance = std::make_shared<ProverInstance>(
circuit, trace_structure, fold_output.accumulator->proving_key.commitment_key);
}

// Track the maximum size of each block for all circuits porcessed (for debugging purposes only)
max_block_size_tracker.update(circuit);
Expand Down
1 change: 0 additions & 1 deletion barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ class ClientIVC {
public:
GoblinProver goblin;
ProverFoldOutput fold_output;
std::shared_ptr<ProverInstance> prover_accumulator;
std::shared_ptr<VerifierInstance> verifier_accumulator;
std::shared_ptr<VerificationKey> instance_vk;

Expand Down
9 changes: 7 additions & 2 deletions barretenberg/cpp/src/barretenberg/flavor/flavor.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,11 +124,16 @@ template <typename FF, typename CommitmentKey_> class ProvingKey_ {
std::vector<FF> public_inputs;

ProvingKey_() = default;
ProvingKey_(const size_t circuit_size, const size_t num_public_inputs)
ProvingKey_(const size_t circuit_size,
const size_t num_public_inputs,
std::shared_ptr<CommitmentKey_> commitment_key = nullptr)
{
{
if (commitment_key == nullptr) {
ZoneScopedN("init commitment key");
this->commitment_key = std::make_shared<CommitmentKey_>(circuit_size + 1);
} else {
// Don't create another commitment key if we already have one
this->commitment_key = commitment_key;
}
this->evaluation_domain = bb::EvaluationDomain<FF>(circuit_size, circuit_size);
this->circuit_size = circuit_size;
Expand Down
6 changes: 4 additions & 2 deletions barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/mega_flavor.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,8 +416,10 @@ class MegaFlavor {
using Base = ProvingKey_<FF, CommitmentKey>;
using Base::Base;

ProvingKey(const size_t circuit_size, const size_t num_public_inputs)
: Base(circuit_size, num_public_inputs)
ProvingKey(const size_t circuit_size,
const size_t num_public_inputs,
std::shared_ptr<CommitmentKey> commitment_key = nullptr)
: Base(circuit_size, num_public_inputs, commitment_key)
, polynomials(circuit_size){};

std::vector<uint32_t> memory_read_records;
Expand Down
6 changes: 4 additions & 2 deletions barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/ultra_flavor.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,8 +335,10 @@ class UltraFlavor {
using Base = ProvingKey_<FF, CommitmentKey>;
using Base::Base;

ProvingKey(const size_t circuit_size, const size_t num_public_inputs)
: Base(circuit_size, num_public_inputs)
ProvingKey(const size_t circuit_size,

@lucasxia01 lucasxia01 Aug 22, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

had to make these changes to ultra and ultra_keccak because otherwise the call to the constructor was just calling the Base one directly. That meant the polynomials did not get initialized to circuit_size length leading to a segfault.

I briefly tried getting rid of the Base constructors (from the using Base::Base; line), but other parts of the code complained.

@lucasxia01 lucasxia01 Aug 22, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

actually, maybe adding default constructors to ProvingKey of each flavor would fix this and allow us to delete using Base::Base. I find it weird that we would want to directly call the base class constructor

@ledwards2225 ledwards2225 Aug 22, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems ok. Calling the parent class constructor from a child class constructor seems like a really natural pattern to me

@lucasxia01 lucasxia01 Aug 22, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

oh, this comment wasn't referring to that pattern. It was referring to the using Base::Base line which brings in the base class constructors to the derived class, which was causing me to hit a segfault rather than a compile error.

Like I was just calling the base class constructor when I thought it would call the derived class constructor (because I forgot to change the derived constructor to take in the commitment_key).

const size_t num_public_inputs,
std::shared_ptr<CommitmentKey> commitment_key = nullptr)
: Base(circuit_size, num_public_inputs, std::move(commitment_key))
, polynomials(circuit_size){};

std::vector<uint32_t> memory_read_records;
Expand Down
6 changes: 4 additions & 2 deletions barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/ultra_keccak.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,8 +282,10 @@ class UltraKeccakFlavor {
using Base = ProvingKey_<FF, CommitmentKey>;
using Base::Base;

ProvingKey(const size_t circuit_size, const size_t num_public_inputs)
: Base(circuit_size, num_public_inputs)
ProvingKey(const size_t circuit_size,
const size_t num_public_inputs,
std::shared_ptr<CommitmentKey> commitment_key = nullptr)
: Base(circuit_size, num_public_inputs, commitment_key)
, polynomials(circuit_size){};

std::vector<uint32_t> memory_read_records;
Expand Down
7 changes: 5 additions & 2 deletions barretenberg/cpp/src/barretenberg/sumcheck/instance/prover_instance.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,14 @@ template <class Flavor> class ProverInstance_ {
std::vector<FF> gate_challenges;
FF target_sum;

ProverInstance_(Circuit& circuit, TraceStructure trace_structure = TraceStructure::NONE)
ProverInstance_(Circuit& circuit,
TraceStructure trace_structure = TraceStructure::NONE,
std::shared_ptr<typename Flavor::CommitmentKey> commitment_key = nullptr)
{
BB_OP_COUNT_TIME_NAME("ProverInstance(Circuit&)");
circuit.add_gates_to_ensure_all_polys_are_non_zero();
circuit.finalize_circuit();
info("finalized gate count: ", circuit.num_gates);

@lucasxia01 lucasxia01 Aug 22, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

should probably be deleted but I wish it was here so people would see what their gate counts are all the time.

@ludamad ludamad Aug 22, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We are living with clientivc printing data, I'm sure we could live with this for now


// Set flag indicating whether the polynomials will be constructed with fixed block sizes for each gate type
const bool is_structured = (trace_structure != TraceStructure::NONE);
Expand All @@ -70,7 +73,7 @@ template <class Flavor> class ProverInstance_ {
}
{
ZoneScopedN("constructing proving key");
proving_key = ProvingKey(dyadic_circuit_size, circuit.public_inputs.size());
proving_key = ProvingKey(dyadic_circuit_size, circuit.public_inputs.size(), commitment_key);
}

// Construct and add to proving key the wire, selector and copy constraint polynomials
Expand Down