chore: fixing the sizes of VMs in CIVC by iakovenkos · Pull Request #11793 · AztecProtocol/aztec-packages

iakovenkos · 2025-02-06T19:23:55Z

The sizes of Translator and ECCVM circuits leak the amount of circuits that have been folded, so we need to fix them.

benchmark_client_ivc after fixing the sizes (note that regression is expected, as the IPA prover is opening a dense polynomial of size 2^16 instead of 2^15 + there is not much skipping in Sumcheck except for SetRelation)

ECCVMProver(CircuitBuilder&)(t) 190 0.87%
ECCVMProver::construct_proof(t) 2405 10.95%
TranslatorProver::construct_proof(t) 1316 5.99%
Goblin::merge(t) 132 0.60%

Total time accounted for: 21963ms/23040ms = 95.32%

Benchmark on master:

ECCVMProver(CircuitBuilder&)(t) 182 0.85%
ECCVMProver::construct_proof(t) 1612 7.51%
TranslatorProver::construct_proof(t) 1650 7.69%
Goblin::merge(t) 132 0.62%

Total time accounted for: 21468ms/22538ms = 95.25%

iakovenkos · 2025-02-06T19:28:46Z

barretenberg/cpp/src/barretenberg/translator_vm/translator_proving_key.hpp

    {
+        // Check that the Translator Circuit does not exceed the fixed upper bound, the current value 8192 corresponds
+        // to 10 rounds of folding (i.e. 20 circuits)
+        ASSERT(circuit.num_gates < Flavor::MINIMUM_MINI_CIRCUIT_SIZE);


maybe this has to be a runtime error?

can convert in the same way described above

iakovenkos · 2025-02-07T09:29:00Z

barretenberg/cpp/src/barretenberg/eccvm/eccvm_flavor.hpp

            const auto log_num_rows = static_cast<size_t>(numeric::get_msb64(num_rows));
-            const size_t dyadic_num_rows = 1UL << (log_num_rows + (1UL << log_num_rows == num_rows ? 0 : 1));
+
+            ASSERT(1UL << CONST_ECCVM_LOG_N > 1UL << (log_num_rows + (1UL << log_num_rows == num_rows ? 0 : 1)));


need to change to a runtime check?

Couple of suggestions here: 1) make well named variables for each of the expressions in these conditionals here so they're easier to digest. 2) For runtime checks, I usually just convert something like this to a
if (condition) {info("Relevant warning"); ASSERT(false);}. For now this is better than having nothing at all at runtime - in the future maybe we can improve the pattern further

thanks for the suggestion!

simplified the logic here

iakovenkos · 2025-02-07T09:36:15Z

barretenberg/cpp/src/barretenberg/relations/ecc_vm/ecc_set_relation.hpp

        3   // left-shiftable polynomial sub-relation
    };

+    template <typename AllEntities> inline static bool skip(const AllEntities& in)


I looked into the other relations - does not seem there are any simple skipping conditions

ledwards2225

Overall LGTM - my two requests would be tests demonstrating the independence of the CIVC VK on the IVC inputs and some benchmark data showing the delta that results from this change.

ledwards2225 · 2025-02-07T14:50:16Z

barretenberg/cpp/src/barretenberg/eccvm/eccvm_flavor.hpp

            const auto log_num_rows = static_cast<size_t>(numeric::get_msb64(num_rows));
-            const size_t dyadic_num_rows = 1UL << (log_num_rows + (1UL << log_num_rows == num_rows ? 0 : 1));
+
+            ASSERT(1UL << CONST_ECCVM_LOG_N > 1UL << (log_num_rows + (1UL << log_num_rows == num_rows ? 0 : 1)));


Couple of suggestions here: 1) make well named variables for each of the expressions in these conditionals here so they're easier to digest. 2) For runtime checks, I usually just convert something like this to a
if (condition) {info("Relevant warning"); ASSERT(false);}. For now this is better than having nothing at all at runtime - in the future maybe we can improve the pattern further

ledwards2225 · 2025-02-07T14:51:02Z

barretenberg/cpp/src/barretenberg/eccvm/eccvm_flavor.hpp

                              precompute_round_shift,             // column 22
-                              transcript_accumulator_empty_shift, // column 23
                              precompute_select_shift,            // column 24
+                              transcript_accumulator_empty_shift, // column 23


column number comments are off here

thanks, fixed

ledwards2225 · 2025-02-07T14:51:11Z

barretenberg/cpp/src/barretenberg/eccvm/eccvm_flavor.hpp

                         entities.precompute_round,             // column 22
-                         entities.transcript_accumulator_empty, // column 23
                         entities.precompute_select,            // column 24
+                         entities.transcript_accumulator_empty, // column 23


ledwards2225 · 2025-02-07T14:52:29Z

barretenberg/cpp/src/barretenberg/eccvm/eccvm_flavor.hpp

+            : Base(
+                  // If we want a fixed size, use 2^(CONST_ECCVM_LOG_N).
+                  // Otherwise, compute the real circuit size from the builder.
+                  fixed_size ? (1UL << CONST_ECCVM_LOG_N)


this is another case where the logic is too complicated for a single inline conditional. I'd break it into separate readable variables

that's fair, changed the way it's initialized

ledwards2225 · 2025-02-07T14:55:52Z

barretenberg/cpp/src/barretenberg/translator_vm/translator_proving_key.hpp

    {
+        // Check that the Translator Circuit does not exceed the fixed upper bound, the current value 8192 corresponds
+        // to 10 rounds of folding (i.e. 20 circuits)
+        ASSERT(circuit.num_gates < Flavor::MINIMUM_MINI_CIRCUIT_SIZE);


can convert in the same way described above

ledwards2225 · 2025-02-07T14:58:08Z

barretenberg/cpp/src/barretenberg/eccvm/eccvm_flavor.hpp

+     * @brief Containter for transcript accumulators, they stand out as the only to-be-shifted wires that are always
+     * populated until the dyadic size of the circuit.
+     */
+    template <typename DataType> class WireToBeShiftedAccumulatorEntities {


General question: Are the updates to the flavors here just to coherently organize polynomials into groups that are independent/dependent on the "fixed size" for commitment efficiency?

That's right + wanted to avoid duplication.
In the commitment phase, the iteration was over the get_wires(), now its over get_wires_without_accumulators() (possibly short) and get_accumulators() (always max size).

iakovenkos · 2025-02-07T19:20:36Z

barretenberg/cpp/src/barretenberg/eccvm/eccvm_prover.cpp


    // Construct the proving key; populates all polynomials except for witness polys
-    key = std::make_shared<ProvingKey>(builder);
+    key = std::make_shared<ProvingKey>(builder, fixed_size);


the compiler was choosing the wrong constructor, so I added this condition

ledwards2225 · 2025-02-10T15:01:13Z

barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp

+    };
+
+    // Check that the Mega selectors are fixed.
+    compare_selectors(civc_vk_2.mega->get_selectors(), civc_vk_20.mega->get_selectors());


This is a nicely designed test but just checking the selectors here is in general not sufficient. For example, this would not catch a difference in copy constraints between the two circuits. The best thing to do is to simply check equality on the VKs themselves. At least for Mega, this requires first setting the pcs_verification_key = nullptr in both since in general that wont point to the same address. (See for example the test GenerateResetKernelVKFromConstraints). I assume there is a similar issue for the eccvm/translator

thanks for pointing this out

ledwards2225 · 2025-02-11T14:50:59Z

barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp

-    // Check that the ECCVM selectors are fixed.
-    compare_selectors(civc_vk_2.eccvm->get_all(), civc_vk_20.eccvm->get_all());
+    // Check the equality of the ECCVM components of the ClientIVC VKeys.
+    EXPECT_EQ(*civc_vk_2.eccvm.get(), *civc_vk_2.eccvm.get());


Looks like youre checking vk_2 against itself here and below!

…ec-packages into si/const_vk_client_ivc

ledwards2225

LGTM pending passing tests!

🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.76.2</summary> ## [0.76.2](aztec-package-v0.76.1...aztec-package-v0.76.2) (2025-02-11) ### Miscellaneous * **logging:** Support explicit FORCE_COLOR parameter ([#11902](#11902)) ([3b3f859](3b3f859)) </details> <details><summary>barretenberg.js: 0.76.2</summary> ## [0.76.2](barretenberg.js-v0.76.1...barretenberg.js-v0.76.2) (2025-02-11) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-packages: 0.76.2</summary> ## [0.76.2](aztec-packages-v0.76.1...aztec-packages-v0.76.2) (2025-02-11) ### Features * Batch writes to the proving broker database ([#11900](#11900)) ([608f887](608f887)) ### Bug Fixes * Cleanup also post test_kind.sh ([#11886](#11886)) ([50cdb15](50cdb15)) * Dont skip wasm civc tests ([#11909](#11909)) ([0395e0b](0395e0b)) * Note hash collision ([#11869](#11869)) ([f289b7c](f289b7c)) * Orchestrator test ([#11901](#11901)) ([f1bb51c](f1bb51c)) * Smt_verification: negative bitvecs, changed gates indicies. acir_formal_proofs: noir-style signed division ([#11649](#11649)) ([4146496](4146496)) * Update path of stern logs ([#11906](#11906)) ([05afb5b](05afb5b)) ### Miscellaneous * Arm runner start fix ([#11903](#11903)) ([6c83c40](6c83c40)) * Fixing the sizes of VMs in CIVC ([#11793](#11793)) ([1afddbd](1afddbd)) * **logging:** Support explicit FORCE_COLOR parameter ([#11902](#11902)) ([3b3f859](3b3f859)) * Misc fixes to devnet deploy flow ([#11738](#11738)) ([bc4cca7](bc4cca7)) * Remove warnings from noir protocol circuits ([#11803](#11803)) ([c6cc3d3](c6cc3d3)) * Replace relative paths to noir-protocol-circuits ([74d6e6a](74d6e6a)) * Replacing use of capsules 1.0 with pxe_db + nuking capsules 1.0 ([#11885](#11885)) ([72be678](72be678)) </details> <details><summary>barretenberg: 0.76.2</summary> ## [0.76.2](barretenberg-v0.76.1...barretenberg-v0.76.2) (2025-02-11) ### Bug Fixes * Note hash collision ([#11869](#11869)) ([f289b7c](f289b7c)) * Smt_verification: negative bitvecs, changed gates indicies. acir_formal_proofs: noir-style signed division ([#11649](#11649)) ([4146496](4146496)) ### Miscellaneous * Fixing the sizes of VMs in CIVC ([#11793](#11793)) ([1afddbd](1afddbd)) </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.76.2</summary> ## [0.76.2](AztecProtocol/aztec-packages@aztec-package-v0.76.1...aztec-package-v0.76.2) (2025-02-11) ### Miscellaneous * **logging:** Support explicit FORCE_COLOR parameter ([#11902](AztecProtocol/aztec-packages#11902)) ([3b3f859](AztecProtocol/aztec-packages@3b3f859)) </details> <details><summary>barretenberg.js: 0.76.2</summary> ## [0.76.2](AztecProtocol/aztec-packages@barretenberg.js-v0.76.1...barretenberg.js-v0.76.2) (2025-02-11) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-packages: 0.76.2</summary> ## [0.76.2](AztecProtocol/aztec-packages@aztec-packages-v0.76.1...aztec-packages-v0.76.2) (2025-02-11) ### Features * Batch writes to the proving broker database ([#11900](AztecProtocol/aztec-packages#11900)) ([608f887](AztecProtocol/aztec-packages@608f887)) ### Bug Fixes * Cleanup also post test_kind.sh ([#11886](AztecProtocol/aztec-packages#11886)) ([50cdb15](AztecProtocol/aztec-packages@50cdb15)) * Dont skip wasm civc tests ([#11909](AztecProtocol/aztec-packages#11909)) ([0395e0b](AztecProtocol/aztec-packages@0395e0b)) * Note hash collision ([#11869](AztecProtocol/aztec-packages#11869)) ([f289b7c](AztecProtocol/aztec-packages@f289b7c)) * Orchestrator test ([#11901](AztecProtocol/aztec-packages#11901)) ([f1bb51c](AztecProtocol/aztec-packages@f1bb51c)) * Smt_verification: negative bitvecs, changed gates indicies. acir_formal_proofs: noir-style signed division ([#11649](AztecProtocol/aztec-packages#11649)) ([4146496](AztecProtocol/aztec-packages@4146496)) * Update path of stern logs ([#11906](AztecProtocol/aztec-packages#11906)) ([05afb5b](AztecProtocol/aztec-packages@05afb5b)) ### Miscellaneous * Arm runner start fix ([#11903](AztecProtocol/aztec-packages#11903)) ([6c83c40](AztecProtocol/aztec-packages@6c83c40)) * Fixing the sizes of VMs in CIVC ([#11793](AztecProtocol/aztec-packages#11793)) ([1afddbd](AztecProtocol/aztec-packages@1afddbd)) * **logging:** Support explicit FORCE_COLOR parameter ([#11902](AztecProtocol/aztec-packages#11902)) ([3b3f859](AztecProtocol/aztec-packages@3b3f859)) * Misc fixes to devnet deploy flow ([#11738](AztecProtocol/aztec-packages#11738)) ([bc4cca7](AztecProtocol/aztec-packages@bc4cca7)) * Remove warnings from noir protocol circuits ([#11803](AztecProtocol/aztec-packages#11803)) ([c6cc3d3](AztecProtocol/aztec-packages@c6cc3d3)) * Replace relative paths to noir-protocol-circuits ([74d6e6a](AztecProtocol/aztec-packages@74d6e6a)) * Replacing use of capsules 1.0 with pxe_db + nuking capsules 1.0 ([#11885](AztecProtocol/aztec-packages#11885)) ([72be678](AztecProtocol/aztec-packages@72be678)) </details> <details><summary>barretenberg: 0.76.2</summary> ## [0.76.2](AztecProtocol/aztec-packages@barretenberg-v0.76.1...barretenberg-v0.76.2) (2025-02-11) ### Bug Fixes * Note hash collision ([#11869](AztecProtocol/aztec-packages#11869)) ([f289b7c](AztecProtocol/aztec-packages@f289b7c)) * Smt_verification: negative bitvecs, changed gates indicies. acir_formal_proofs: noir-style signed division ([#11649](AztecProtocol/aztec-packages#11649)) ([4146496](AztecProtocol/aztec-packages@4146496)) ### Miscellaneous * Fixing the sizes of VMs in CIVC ([#11793](AztecProtocol/aztec-packages#11793)) ([1afddbd](AztecProtocol/aztec-packages@1afddbd)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

iakovenkos added 10 commits January 27, 2025 18:23

measuring stuff

9e12eb4

wip

0d7d3c2

commit to chunks?

8c401ee

somewhat amortized commitments cost

5ad09c4

easy optimizations

0d38b3b

removed noise

34383c7

Merge branch 'master' into si/const_vk_client_ivc

ccc9dc1

eccvm getters to isolate the long wires

7a99d97

added a mechanism to turn on fixed size in eccvm + tests

1dea9ad

comments

dd2f56a

iakovenkos changed the title ~~chore: fixing the size of VMs~~ chore: fixing the sizes of VMs Feb 6, 2025

iakovenkos commented Feb 6, 2025

View reviewed changes

iakovenkos changed the title ~~chore: fixing the sizes of VMs~~ chore: fixing the sizes of VMs in CIVC Feb 6, 2025

iakovenkos commented Feb 7, 2025

View reviewed changes

docs

5631090

iakovenkos marked this pull request as ready for review February 7, 2025 09:37

fix tests

c310c8a

iakovenkos self-assigned this Feb 7, 2025

iakovenkos linked an issue Feb 7, 2025 that may be closed by this pull request

Fixed ECCVM/Translator circuit size AztecProtocol/barretenberg#1202

Closed

ledwards2225 requested changes Feb 7, 2025

View reviewed changes

iakovenkos and others added 2 commits February 7, 2025 17:48

test + resolved comments

69cf76b

Merge branch 'master' into si/const_vk_client_ivc

e3dc49a

iakovenkos commented Feb 7, 2025

View reviewed changes

tests fixed

f47c3d4

ledwards2225 reviewed Feb 10, 2025

View reviewed changes

iakovenkos and others added 2 commits February 11, 2025 10:45

modified VKIndependenceTest

3ff127b

Merge branch 'master' into si/const_vk_client_ivc

6a60788

ledwards2225 reviewed Feb 11, 2025

View reviewed changes

test fix

59abff0

Merge branch 'si/const_vk_client_ivc' of github.com:AztecProtocol/azt…

1b5788d

…ec-packages into si/const_vk_client_ivc

ledwards2225 approved these changes Feb 11, 2025

View reviewed changes

Merge branch 'master' into si/const_vk_client_ivc

b4065bb

iakovenkos merged commit 1afddbd into master Feb 11, 2025
25 checks passed

iakovenkos deleted the si/const_vk_client_ivc branch February 11, 2025 16:26

AztecBot mentioned this pull request Feb 11, 2025

chore(master): Release 0.76.2 #11899

Merged

iakovenkos mentioned this pull request Mar 25, 2025

Avoid leaking the actual ECCVM trace size in proof AztecProtocol/barretenberg#1198

Closed

Conversation

iakovenkos commented Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ledwards2225 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ledwards2225 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

iakovenkos commented Feb 6, 2025 •

edited

Loading