From 5413208c190f7b85cb2d8be4d3c95199ff53b20f Mon Sep 17 00:00:00 2001 From: maramihali Date: Tue, 26 Mar 2024 19:14:03 +0000 Subject: [PATCH 1/5] add goblin ops in add_gates_to_ensure_all_polys_are_non_zero rather than separately --- .../src/barretenberg/client_ivc/client_ivc.test.cpp | 5 ++--- .../src/barretenberg/dsl/acir_format/acir_format.cpp | 4 ---- .../dsl/acir_proofs/goblin_acir_composer.cpp | 4 ---- .../barretenberg/goblin/goblin_recursion.test.cpp | 1 - .../cpp/src/barretenberg/goblin/mock_circuits.hpp | 12 +++++------- .../goblin_ultra_circuit_builder.cpp | 4 ++++ 6 files changed, 11 insertions(+), 19 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp b/barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp index a0729568475..a948b0aa093 100644 --- a/barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp +++ b/barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp @@ -46,7 +46,6 @@ class ClientIVCTests : public ::testing::Test { { Builder circuit{ ivc.goblin.op_queue }; MockCircuits::construct_arithmetic_circuit(circuit, log2_num_gates); - MockCircuits::construct_goblin_ecc_op_circuit(circuit); return circuit; } @@ -114,11 +113,11 @@ TEST_F(ClientIVCTests, Full) Builder function_circuit = create_mock_circuit(ivc); ivc.initialize(function_circuit); - auto function_vk = std::make_shared(ivc.prover_fold_output.accumulator->proving_key); - auto foo_verifier_instance = std::make_shared(function_vk); // Accumulate kernel circuit (first kernel mocked as simple circuit since no folding proofs yet) Builder kernel_circuit = create_mock_circuit(ivc); FoldProof kernel_fold_proof = ivc.accumulate(kernel_circuit); + auto function_vk = std::make_shared(ivc.prover_fold_output.accumulator->proving_key); + auto foo_verifier_instance = std::make_shared(function_vk); // This will have a different verification key because we added the recursive merge verification to the circuit auto function_vk_with_merge = std::make_shared(ivc.prover_instance->proving_key); auto kernel_vk = function_vk_with_merge; diff --git a/barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_format.cpp b/barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_format.cpp index 2cc00db97c7..75db931222c 100644 --- a/barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_format.cpp +++ b/barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_format.cpp @@ -247,10 +247,6 @@ GoblinUltraCircuitBuilder create_circuit(const AcirFormat& constraint_system, bool has_valid_witness_assignments = !witness.empty(); acir_format::build_constraints(builder, constraint_system, has_valid_witness_assignments); - // TODO(https://github.com/AztecProtocol/barretenberg/issues/817): Add some arbitrary op gates to ensure the - // associated polynomials are non-zero and to give ECCVM and Translator some ECC ops to process. - MockCircuits::construct_goblin_ecc_op_circuit(builder); - return builder; }; diff --git a/barretenberg/cpp/src/barretenberg/dsl/acir_proofs/goblin_acir_composer.cpp b/barretenberg/cpp/src/barretenberg/dsl/acir_proofs/goblin_acir_composer.cpp index 5d754a168a7..f93ca21690c 100644 --- a/barretenberg/cpp/src/barretenberg/dsl/acir_proofs/goblin_acir_composer.cpp +++ b/barretenberg/cpp/src/barretenberg/dsl/acir_proofs/goblin_acir_composer.cpp @@ -15,10 +15,6 @@ void GoblinAcirComposer::create_circuit(acir_format::AcirFormat& constraint_syst // Populate constraints in the builder via the data in constraint_system acir_format::build_constraints(builder_, constraint_system, true); - - // TODO(https://github.com/AztecProtocol/barretenberg/issues/817): Add some arbitrary op gates to ensure the - // associated polynomials are non-zero and to give ECCVM and Translator some ECC ops to process. - MockCircuits::construct_goblin_ecc_op_circuit(builder_); } std::vector GoblinAcirComposer::accumulate_and_prove() diff --git a/barretenberg/cpp/src/barretenberg/goblin/goblin_recursion.test.cpp b/barretenberg/cpp/src/barretenberg/goblin/goblin_recursion.test.cpp index 49b3f08028f..c24ddb223ea 100644 --- a/barretenberg/cpp/src/barretenberg/goblin/goblin_recursion.test.cpp +++ b/barretenberg/cpp/src/barretenberg/goblin/goblin_recursion.test.cpp @@ -48,7 +48,6 @@ TEST_F(GoblinRecursionTests, Vanilla) // Construct and accumulate a mock function circuit GoblinUltraCircuitBuilder function_circuit{ goblin.op_queue }; MockCircuits::construct_arithmetic_circuit(function_circuit, /*target_log2_dyadic_size=*/8); - MockCircuits::construct_goblin_ecc_op_circuit(function_circuit); goblin.merge(function_circuit); auto function_accum = construct_accumulator(function_circuit); diff --git a/barretenberg/cpp/src/barretenberg/goblin/mock_circuits.hpp b/barretenberg/cpp/src/barretenberg/goblin/mock_circuits.hpp index 4f4fbdee852..fd9c6a24e82 100644 --- a/barretenberg/cpp/src/barretenberg/goblin/mock_circuits.hpp +++ b/barretenberg/cpp/src/barretenberg/goblin/mock_circuits.hpp @@ -69,11 +69,6 @@ class GoblinMockCircuits { stdlib::generate_sha256_test_circuit(builder, NUM_ITERATIONS); // min gates: ~39k stdlib::generate_ecdsa_verification_test_circuit(builder, NUM_ITERATIONS); // min gates: ~41k stdlib::generate_merkle_membership_test_circuit(builder, NUM_ITERATIONS); // min gates: ~29k - - // Note: its not clear whether goblin ops will be supported for function circuits initially but currently - // UGH can only be used if some op gates are included so for now we'll assume each function circuit has - // some. - MockCircuits::construct_goblin_ecc_op_circuit(builder); } /** @@ -92,8 +87,11 @@ class GoblinMockCircuits { { bb::GoblinUltraCircuitBuilder builder{ op_queue }; - // Add some goblinized ecc ops - MockCircuits::construct_goblin_ecc_op_circuit(builder); + // Add some goblinized ecc ops: a mul accum op and an equality op + auto point = Point::one() * FF::random_element(); + auto scalar = FF::random_element(); + builder.queue_ecc_mul_accum(point, scalar); + builder.queue_ecc_eq(); op_queue->set_size_data(); diff --git a/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/goblin_ultra_circuit_builder.cpp b/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/goblin_ultra_circuit_builder.cpp index 0267a5be4fa..f6a4c2901a1 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/goblin_ultra_circuit_builder.cpp +++ b/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/goblin_ultra_circuit_builder.cpp @@ -82,6 +82,10 @@ template void GoblinUltraCircuitBuilder_::add_gates_to_ensure_ // dummy gate to be read into by previous poseidon internal gate via shifts this->create_dummy_gate( this->blocks.poseidon_internal, this->zero_idx, this->zero_idx, this->zero_idx, this->zero_idx); + + // add dummy mul accum op and an equality op + this->queue_ecc_mul_accum(bb::g1::affine_element::one() * FF::random_element(), FF::random_element()); + this->queue_ecc_eq(); } /** From ddce4ab4aa48bb3591190585af61d50a8f6539e3 Mon Sep 17 00:00:00 2001 From: maramihali Date: Tue, 26 Mar 2024 19:23:25 +0000 Subject: [PATCH 2/5] remove function --- .../barretenberg/client_ivc/client_ivc.test.cpp | 4 ++-- .../stdlib_circuit_builders/mock_circuits.hpp | 14 -------------- 2 files changed, 2 insertions(+), 16 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp b/barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp index a948b0aa093..9d4e55c0c29 100644 --- a/barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp +++ b/barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp @@ -113,11 +113,11 @@ TEST_F(ClientIVCTests, Full) Builder function_circuit = create_mock_circuit(ivc); ivc.initialize(function_circuit); + auto function_vk = std::make_shared(ivc.prover_fold_output.accumulator->proving_key); + auto foo_verifier_instance = std::make_shared(function_vk); // Accumulate kernel circuit (first kernel mocked as simple circuit since no folding proofs yet) Builder kernel_circuit = create_mock_circuit(ivc); FoldProof kernel_fold_proof = ivc.accumulate(kernel_circuit); - auto function_vk = std::make_shared(ivc.prover_fold_output.accumulator->proving_key); - auto foo_verifier_instance = std::make_shared(function_vk); // This will have a different verification key because we added the recursive merge verification to the circuit auto function_vk_with_merge = std::make_shared(ivc.prover_instance->proving_key); auto kernel_vk = function_vk_with_merge; diff --git a/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/mock_circuits.hpp b/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/mock_circuits.hpp index 8b27ab0afb2..2e48ba1bbb4 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/mock_circuits.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/mock_circuits.hpp @@ -60,19 +60,5 @@ class MockCircuits { builder.create_big_add_gate({ a_idx, b_idx, c_idx, d_idx, FF(1), FF(1), FF(1), FF(-1), FF(0) }); } } - - /** - * @brief Populate a builder with some arbitrary goblinized ECC ops - * - * @param builder - */ - static void construct_goblin_ecc_op_circuit(GoblinUltraCircuitBuilder& builder) - { - // Add a mul accum op and an equality op - auto point = Point::one() * FF::random_element(); - auto scalar = FF::random_element(); - builder.queue_ecc_mul_accum(point, scalar); - builder.queue_ecc_eq(); - } }; } // namespace bb \ No newline at end of file From eff14e88a4e525624cc81aa4c82b1ba92e0e15b0 Mon Sep 17 00:00:00 2001 From: maramihali Date: Tue, 26 Mar 2024 20:27:39 +0000 Subject: [PATCH 3/5] update PR from review --- .../src/barretenberg/client_ivc/client_ivc.test.cpp | 4 ++-- .../dsl/acir_proofs/goblin_acir_composer.cpp | 4 ++++ .../barretenberg/goblin/goblin_recursion.test.cpp | 4 +++- .../cpp/src/barretenberg/goblin/mock_circuits.hpp | 7 ++----- .../stdlib_circuit_builders/mock_circuits.hpp | 13 +++++++++++++ 5 files changed, 24 insertions(+), 8 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp b/barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp index 9d4e55c0c29..825c7ea38ec 100644 --- a/barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp +++ b/barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp @@ -38,8 +38,8 @@ class ClientIVCTests : public ::testing::Test { /** * @brief Construct mock circuit with arithmetic gates and goblin ops - * @details Currently default sized to 2^16 to match kernel. (Note: op gates will bump size to next power of - 2) + * @details Currently default sized to 2^16 to match kernel. (Note: dummy op gates added to avoid non-zero + * polynomials will bump size to next power of 2) * */ static Builder create_mock_circuit(ClientIVC& ivc, size_t log2_num_gates = 15) diff --git a/barretenberg/cpp/src/barretenberg/dsl/acir_proofs/goblin_acir_composer.cpp b/barretenberg/cpp/src/barretenberg/dsl/acir_proofs/goblin_acir_composer.cpp index f93ca21690c..4ef7c02c345 100644 --- a/barretenberg/cpp/src/barretenberg/dsl/acir_proofs/goblin_acir_composer.cpp +++ b/barretenberg/cpp/src/barretenberg/dsl/acir_proofs/goblin_acir_composer.cpp @@ -15,6 +15,10 @@ void GoblinAcirComposer::create_circuit(acir_format::AcirFormat& constraint_syst // Populate constraints in the builder via the data in constraint_system acir_format::build_constraints(builder_, constraint_system, true); + + // TODO(https://github.com/AztecProtocol/barretenberg/issues/817): Add some arbitrary op gates to ensure the + // to give ECCVM and Translator some ECC ops to process. + MockCircuits::construct_goblin_ecc_op_circuit(builder_); } std::vector GoblinAcirComposer::accumulate_and_prove() diff --git a/barretenberg/cpp/src/barretenberg/goblin/goblin_recursion.test.cpp b/barretenberg/cpp/src/barretenberg/goblin/goblin_recursion.test.cpp index c24ddb223ea..623c1118b4a 100644 --- a/barretenberg/cpp/src/barretenberg/goblin/goblin_recursion.test.cpp +++ b/barretenberg/cpp/src/barretenberg/goblin/goblin_recursion.test.cpp @@ -45,9 +45,11 @@ TEST_F(GoblinRecursionTests, Vanilla) size_t NUM_CIRCUITS = 2; for (size_t circuit_idx = 0; circuit_idx < NUM_CIRCUITS; ++circuit_idx) { - // Construct and accumulate a mock function circuit + // Construct and accumulate a mock function circuit containing both arbitrary arithmetic gates and goblin + // ecc op gates to make it a meaningful test GoblinUltraCircuitBuilder function_circuit{ goblin.op_queue }; MockCircuits::construct_arithmetic_circuit(function_circuit, /*target_log2_dyadic_size=*/8); + MockCircuits::construct_goblin_ecc_op_circuit(function_circuit); goblin.merge(function_circuit); auto function_accum = construct_accumulator(function_circuit); diff --git a/barretenberg/cpp/src/barretenberg/goblin/mock_circuits.hpp b/barretenberg/cpp/src/barretenberg/goblin/mock_circuits.hpp index fd9c6a24e82..7b7ff37f056 100644 --- a/barretenberg/cpp/src/barretenberg/goblin/mock_circuits.hpp +++ b/barretenberg/cpp/src/barretenberg/goblin/mock_circuits.hpp @@ -87,11 +87,8 @@ class GoblinMockCircuits { { bb::GoblinUltraCircuitBuilder builder{ op_queue }; - // Add some goblinized ecc ops: a mul accum op and an equality op - auto point = Point::one() * FF::random_element(); - auto scalar = FF::random_element(); - builder.queue_ecc_mul_accum(point, scalar); - builder.queue_ecc_eq(); + // Add some goblinized ecc ops + MockCircuits::construct_goblin_ecc_op_circuit(builder); op_queue->set_size_data(); diff --git a/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/mock_circuits.hpp b/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/mock_circuits.hpp index 2e48ba1bbb4..cb523ce7186 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/mock_circuits.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/mock_circuits.hpp @@ -60,5 +60,18 @@ class MockCircuits { builder.create_big_add_gate({ a_idx, b_idx, c_idx, d_idx, FF(1), FF(1), FF(1), FF(-1), FF(0) }); } } + + /** + * @brief Populate a builder with some arbitrary goblinized ECC ops, one of each type + * + * @param builder + */ + static void construct_goblin_ecc_op_circuit(GoblinUltraCircuitBuilder& builder) + { + // Add a mul accum op, an add accum op and an equality op + builder.queue_ecc_add_accum(Point::one() * FF::random_element()); + builder.queue_ecc_mul_accum(Point::one() * FF::random_element(), FF::random_element()); + builder.queue_ecc_eq(); + } }; } // namespace bb \ No newline at end of file From 0a5f456335728ef4029541aa12fe06996ba255e9 Mon Sep 17 00:00:00 2001 From: maramihali Date: Wed, 27 Mar 2024 19:43:58 +0000 Subject: [PATCH 4/5] add issue and document for function circuit --- barretenberg/cpp/src/barretenberg/goblin/mock_circuits.hpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/barretenberg/cpp/src/barretenberg/goblin/mock_circuits.hpp b/barretenberg/cpp/src/barretenberg/goblin/mock_circuits.hpp index 7b7ff37f056..cb87096063e 100644 --- a/barretenberg/cpp/src/barretenberg/goblin/mock_circuits.hpp +++ b/barretenberg/cpp/src/barretenberg/goblin/mock_circuits.hpp @@ -69,6 +69,13 @@ class GoblinMockCircuits { stdlib::generate_sha256_test_circuit(builder, NUM_ITERATIONS); // min gates: ~39k stdlib::generate_ecdsa_verification_test_circuit(builder, NUM_ITERATIONS); // min gates: ~41k stdlib::generate_merkle_membership_test_circuit(builder, NUM_ITERATIONS); // min gates: ~29k + + // TODO(https://github.com/AztecProtocol/barretenberg/issues/911): We require goblin ops to be added to the + // function circuit because we cannot support zero commtiments. While the builder handles this at + // ProverInstance creation stage via the add_gates_to_ensure_all_polys_are_non_zero function for other UGH + // circuits (where we don't explicitly need to add goblin ops), in ClientIVC merge proving happens prior to + // folding where the absense of goblin ecc ops will result in zero commitments. + MockCircuits::construct_goblin_ecc_op_circuit(builder); } /** From af71b661ccc5c5985c2f56879c1a9afcf770b793 Mon Sep 17 00:00:00 2001 From: maramihali Date: Wed, 27 Mar 2024 20:05:53 +0000 Subject: [PATCH 5/5] fix test --- .../cpp/src/barretenberg/client_ivc/client_ivc.test.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp b/barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp index 825c7ea38ec..60d52d73ba8 100644 --- a/barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp +++ b/barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp @@ -46,6 +46,13 @@ class ClientIVCTests : public ::testing::Test { { Builder circuit{ ivc.goblin.op_queue }; MockCircuits::construct_arithmetic_circuit(circuit, log2_num_gates); + + // TODO(https://github.com/AztecProtocol/barretenberg/issues/911): We require goblin ops to be added to the + // function circuit because we cannot support zero commtiments. While the builder handles this at + // finalisation stage via the add_gates_to_ensure_all_polys_are_non_zero function for other UGH + // circuits (where we don't explicitly need to add goblin ops), in ClientIVC merge proving happens prior to + // folding where the absense of goblin ecc ops will result in zero commitments. + MockCircuits::construct_goblin_ecc_op_circuit(circuit); return circuit; }