From 7bf9d20d5a26b0847748d37ade733f46b70729da Mon Sep 17 00:00:00 2001 From: maramihali Date: Wed, 27 Mar 2024 16:39:39 -0400 Subject: [PATCH] chore: add goblin ops in add_gates_to_ensure_all_polys_are_non_zero (#5468) Resolves https://github.com/AztecProtocol/barretenberg/issues/843. Removes the need to add golin ecc ops for each UGH circuit by ensuring we add the dummy gates as part of the function used to ensure other polynomials are non-zero due to the absence of specific gates. However, we keep adding goblin ecc gates to circuits in situations when we want to test Goblin and also in ClientIVC because merge proving is done prior to ProverInstance creation. --- .../src/barretenberg/client_ivc/client_ivc.test.cpp | 10 ++++++++-- .../src/barretenberg/dsl/acir_format/acir_format.cpp | 4 ---- .../dsl/acir_proofs/goblin_acir_composer.cpp | 2 +- .../src/barretenberg/goblin/goblin_recursion.test.cpp | 3 ++- .../cpp/src/barretenberg/goblin/mock_circuits.hpp | 8 +++++--- .../goblin_ultra_circuit_builder.cpp | 4 ++++ .../stdlib_circuit_builders/mock_circuits.hpp | 9 ++++----- 7 files changed, 24 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 a07295684751..60d52d73ba8d 100644 --- a/barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp +++ b/barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp @@ -38,14 +38,20 @@ 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) { 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; } 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 2cc00db97c71..75db931222ce 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 5d754a168a7c..4ef7c02c3457 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 @@ -17,7 +17,7 @@ void GoblinAcirComposer::create_circuit(acir_format::AcirFormat& constraint_syst 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. + // to give ECCVM and Translator some ECC ops to process. MockCircuits::construct_goblin_ecc_op_circuit(builder_); } diff --git a/barretenberg/cpp/src/barretenberg/goblin/goblin_recursion.test.cpp b/barretenberg/cpp/src/barretenberg/goblin/goblin_recursion.test.cpp index 49b3f08028f8..623c1118b4ab 100644 --- a/barretenberg/cpp/src/barretenberg/goblin/goblin_recursion.test.cpp +++ b/barretenberg/cpp/src/barretenberg/goblin/goblin_recursion.test.cpp @@ -45,7 +45,8 @@ 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); diff --git a/barretenberg/cpp/src/barretenberg/goblin/mock_circuits.hpp b/barretenberg/cpp/src/barretenberg/goblin/mock_circuits.hpp index 4f4fbdee852a..cb87096063ee 100644 --- a/barretenberg/cpp/src/barretenberg/goblin/mock_circuits.hpp +++ b/barretenberg/cpp/src/barretenberg/goblin/mock_circuits.hpp @@ -70,9 +70,11 @@ class GoblinMockCircuits { 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. + // 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); } 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 0267a5be4fa4..f6a4c2901a1f 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(); } /** 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 8b27ab0afb21..cb523ce7186a 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/mock_circuits.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/mock_circuits.hpp @@ -62,16 +62,15 @@ class MockCircuits { } /** - * @brief Populate a builder with some arbitrary goblinized ECC ops + * @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 and an equality op - auto point = Point::one() * FF::random_element(); - auto scalar = FF::random_element(); - builder.queue_ecc_mul_accum(point, scalar); + // 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(); } };