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

lucasxia01 · 2024-08-22T18:33:44Z

Continuation of #8118.

In AztecIVC (or ClientIVC), when we have multiple instances, we create a commitment key for each one. However, since each of these instances are the same size, there's no need to create a new one for each one.

When we're constructing an instance beyond the first one, we can reuse the same commitment key from the AztecIVC accumulator, which saves ~123MB of memory for 2^17 circuits, a reduction of 15.6%.

After the change, we cut down max memory by 123MB.

lucasxia01 · 2024-08-22T19:18:34Z

barretenberg/cpp/src/barretenberg/sumcheck/instance/prover_instance.hpp

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


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

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

lucasxia01 · 2024-08-22T19:20:24Z

barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/ultra_flavor.hpp

@@ -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,


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.

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

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

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

lucasxia01 · 2024-08-22T19:55:58Z

barretenberg/cpp/src/barretenberg/aztec_ivc/aztec_ivc.cpp

@@ -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) {


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

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

lucasxia01 · 2024-08-22T19:56:38Z

barretenberg/cpp/src/barretenberg/aztec_ivc/aztec_ivc.test.cpp

+ * @details When accumulating only four circuits, we execute all the functionality of a full AztecIVC run.
+ *
+ */
+TEST_F(AztecIVCTests, BasicFour)


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

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

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

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

ledwards2225

LGTM

ledwards2225 · 2024-08-22T20:37:27Z

barretenberg/cpp/src/barretenberg/aztec_ivc/aztec_ivc.test.cpp

+ * @details When accumulating only four circuits, we execute all the functionality of a full AztecIVC run.
+ *
+ */
+TEST_F(AztecIVCTests, BasicFour)


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

ledwards2225 · 2024-08-22T20:55:58Z

barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/ultra_flavor.hpp

@@ -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,


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

ledwards2225 · 2024-08-22T20:57:35Z

barretenberg/cpp/src/barretenberg/aztec_ivc/aztec_ivc.cpp

@@ -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) {


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

🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.50.1</summary> ## [0.50.1](aztec-package-v0.50.0...aztec-package-v0.50.1) (2024-08-23) ### Miscellaneous * **aztec-package:** Synchronize aztec-packages versions </details> <details><summary>barretenberg.js: 0.50.1</summary> ## [0.50.1](barretenberg.js-v0.50.0...barretenberg.js-v0.50.1) (2024-08-23) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-packages: 0.50.1</summary> ## [0.50.1](aztec-packages-v0.50.0...aztec-packages-v0.50.1) (2024-08-23) ### Features * Free instances and circuits earlier to reduce max memory usage ([#8118](#8118)) ([32a04c1](32a04c1)) * Prepare protocol circuits for batch rollup ([#7727](#7727)) ([a126e22](a126e22)) * Share the commitment key between instances to reduce mem ([#8154](#8154)) ([c3dddf8](c3dddf8)) ### Bug Fixes * Cli-wallet manifest ([#8156](#8156)) ([2ffcda3](2ffcda3)) ### Miscellaneous * Replace relative paths to noir-protocol-circuits ([5372ac4](5372ac4)) * Requiring only 1 sig from user ([#8146](#8146)) ([f0b564b](f0b564b)) </details> <details><summary>barretenberg: 0.50.1</summary> ## [0.50.1](barretenberg-v0.50.0...barretenberg-v0.50.1) (2024-08-23) ### Features * Free instances and circuits earlier to reduce max memory usage ([#8118](#8118)) ([32a04c1](32a04c1)) * Share the commitment key between instances to reduce mem ([#8154](#8154)) ([c3dddf8](c3dddf8)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.50.1</summary> ## [0.50.1](AztecProtocol/aztec-packages@aztec-package-v0.50.0...aztec-package-v0.50.1) (2024-08-23) ### Miscellaneous * **aztec-package:** Synchronize aztec-packages versions </details> <details><summary>barretenberg.js: 0.50.1</summary> ## [0.50.1](AztecProtocol/aztec-packages@barretenberg.js-v0.50.0...barretenberg.js-v0.50.1) (2024-08-23) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-packages: 0.50.1</summary> ## [0.50.1](AztecProtocol/aztec-packages@aztec-packages-v0.50.0...aztec-packages-v0.50.1) (2024-08-23) ### Features * Free instances and circuits earlier to reduce max memory usage ([#8118](AztecProtocol/aztec-packages#8118)) ([32a04c1](AztecProtocol/aztec-packages@32a04c1)) * Prepare protocol circuits for batch rollup ([#7727](AztecProtocol/aztec-packages#7727)) ([a126e22](AztecProtocol/aztec-packages@a126e22)) * Share the commitment key between instances to reduce mem ([#8154](AztecProtocol/aztec-packages#8154)) ([c3dddf8](AztecProtocol/aztec-packages@c3dddf8)) ### Bug Fixes * Cli-wallet manifest ([#8156](AztecProtocol/aztec-packages#8156)) ([2ffcda3](AztecProtocol/aztec-packages@2ffcda3)) ### Miscellaneous * Replace relative paths to noir-protocol-circuits ([5372ac4](AztecProtocol/aztec-packages@5372ac4)) * Requiring only 1 sig from user ([#8146](AztecProtocol/aztec-packages#8146)) ([f0b564b](AztecProtocol/aztec-packages@f0b564b)) </details> <details><summary>barretenberg: 0.50.1</summary> ## [0.50.1](AztecProtocol/aztec-packages@barretenberg-v0.50.0...barretenberg-v0.50.1) (2024-08-23) ### Features * Free instances and circuits earlier to reduce max memory usage ([#8118](AztecProtocol/aztec-packages#8118)) ([32a04c1](AztecProtocol/aztec-packages@32a04c1)) * Share the commitment key between instances to reduce mem ([#8154](AztecProtocol/aztec-packages#8154)) ([c3dddf8](AztecProtocol/aztec-packages@c3dddf8)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

lucasxia01 added 2 commits August 22, 2024 18:33

share the comm key !

a070c3f

fix ultra keccak flavor

b1c9abf

lucasxia01 self-assigned this Aug 22, 2024

lucasxia01 commented Aug 22, 2024

View reviewed changes

lucasxia01 requested a review from ledwards2225 August 22, 2024 19:54

made changes to aztecivc

e78d42b

lucasxia01 commented Aug 22, 2024

View reviewed changes

ledwards2225 approved these changes Aug 22, 2024

View reviewed changes

get rid of base class constructors in proving keys

766f7d8

lucasxia01 enabled auto-merge (squash) August 22, 2024 21:26

lucasxia01 merged commit c3dddf8 into master Aug 22, 2024
35 checks passed

lucasxia01 deleted the lx/share-commitment-key branch August 22, 2024 21:41

AztecBot mentioned this pull request Aug 22, 2024

chore(master): Release 0.50.1 #8151

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lucasxia01 commented Aug 22, 2024 •

edited

Loading

lucasxia01 Aug 22, 2024

ludamad Aug 22, 2024

lucasxia01 Aug 22, 2024

lucasxia01 Aug 22, 2024

ledwards2225 Aug 22, 2024

lucasxia01 Aug 22, 2024

lucasxia01 Aug 22, 2024

ledwards2225 Aug 22, 2024

lucasxia01 Aug 22, 2024

ledwards2225 Aug 22, 2024

lucasxia01 Aug 22, 2024

ledwards2225 Aug 22, 2024

ledwards2225 left a comment

ledwards2225 Aug 22, 2024

ledwards2225 Aug 22, 2024

ledwards2225 Aug 22, 2024

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

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

Conversation

lucasxia01 commented Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ledwards2225 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasxia01 commented Aug 22, 2024 •

edited

Loading