chore: add goblin ops in add_gates_to_ensure_all_polys_are_non_zero (#…

…5468)

Resolves AztecProtocol/barretenberg#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.

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

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

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

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

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

barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/goblin_ultra_circuit_builder.cpp

@@ -82,6 +82,10 @@ template <typename FF> void GoblinUltraCircuitBuilder_<FF>::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();
 }
 
 /**

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