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 all 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/aztec_ivc/aztec_ivc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,13 @@ void AztecIVC::accumulate(ClientCircuit& circuit, const std::shared_ptr<Verifica
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) {
Copy link
Contributor Author

@lucasxia01 lucasxia01 Aug 22, 2024

Choose a reason for hiding this comment

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

a little ugly since we now have two blocks of if (!initialized) {} else {}

Copy link
Contributor

@ledwards2225 ledwards2225 Aug 22, 2024

Choose a reason for hiding this comment

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

not great but ok for now. I'd think the eventual solution would be for IVC to initialize and manage its own commitment key then pass it to individual provers

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);
}

// Set the instance verification key from precomputed if available, else compute it
instance_vk = precomputed_vk ? precomputed_vk : std::make_shared<VerificationKey>(prover_instance->proving_key);
Expand Down
18 changes: 18 additions & 0 deletions barretenberg/cpp/src/barretenberg/aztec_ivc/aztec_ivc.test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,24 @@ TEST_F(AztecIVCTests, Basic)
EXPECT_TRUE(ivc.prove_and_verify());
};

/**
* @brief A simple test demonstrating IVC for four mock circuits, which is slightly more than minimal.
* @details When accumulating only four circuits, we execute all the functionality of a full AztecIVC run.
*
*/
TEST_F(AztecIVCTests, BasicFour)
Copy link
Contributor Author

@lucasxia01 lucasxia01 Aug 22, 2024

Choose a reason for hiding this comment

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

could disable for CI given we have n=2 and n=6 already.

Copy link
Contributor

@ledwards2225 ledwards2225 Aug 22, 2024

Choose a reason for hiding this comment

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

why even add this? this case is already contained in the failure test below

Copy link
Contributor Author

@lucasxia01 lucasxia01 Aug 22, 2024

Choose a reason for hiding this comment

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

I'm using it to measure memory since it's the minimal case that does everything. Yeah I guess the failure test does 4 circuits but it was a failure test I guess.

I can just disable this and leave a comment saying it's for memory benchmarking

Copy link
Contributor

@ledwards2225 ledwards2225 Aug 22, 2024

Choose a reason for hiding this comment

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

oh I see, didn't realize it was the memory case

{
AztecIVC ivc;

MockCircuitProducer circuit_producer;
for (size_t idx = 0; idx < 4; ++idx) {
Builder circuit = circuit_producer.create_next_circuit(ivc);
ivc.accumulate(circuit);
}

EXPECT_TRUE(ivc.prove_and_verify());
};

/**
* @brief Check that the IVC fails to verify if an intermediate fold proof is invalid
* @details When accumulating 4 circuits, there are 3 fold proofs to verify (the first two are recursively verfied and
Expand Down
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
2 changes: 1 addition & 1 deletion barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ TEST_F(ClientIVCTests, Basic)
};

/**
* @brief A simple-as-possible test demonstrating IVC for three mock circuits
* @brief A simple test demonstrating IVC for three mock circuits which does more logic than just two circuits.
*
*/
TEST_F(ClientIVCTests, BasicThree)
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
9 changes: 5 additions & 4 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 @@ -412,12 +412,13 @@ class MegaFlavor {
*/
class ProvingKey : public ProvingKey_<FF, CommitmentKey> {
public:
// Expose constructors on the base class
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() = default;
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
9 changes: 5 additions & 4 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 @@ -331,12 +331,13 @@ class UltraFlavor {
*/
class ProvingKey : public ProvingKey_<FF, CommitmentKey> {
public:
// Expose constructors on the base class
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() = default;
ProvingKey(const size_t circuit_size,
Copy link
Contributor Author

@lucasxia01 lucasxia01 Aug 22, 2024

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.

Copy link
Contributor Author

@lucasxia01 lucasxia01 Aug 22, 2024

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

Copy link
Contributor

@ledwards2225 ledwards2225 Aug 22, 2024

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

Copy link
Contributor Author

@lucasxia01 lucasxia01 Aug 22, 2024

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
9 changes: 5 additions & 4 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 @@ -278,12 +278,13 @@ class UltraKeccakFlavor {
*/
class ProvingKey : public ProvingKey_<FF, CommitmentKey> {
public:
// Expose constructors on the base class
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() = default;
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);
Copy link
Contributor Author

@lucasxia01 lucasxia01 Aug 22, 2024

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.

Copy link
Collaborator

@ludamad ludamad Aug 22, 2024

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