resolve comments from tests

Author: maramihali

barretenberg/cpp/src/barretenberg/boomerang_value_detection/graph_description_goblin.test.cpp

@@ -40,37 +40,25 @@ class BoomerangGoblinRecursiveVerifierTests : public testing::Test {
      *
      * @return ProverOutput
      */
-    static ProverOutput create_goblin_prover_output(const size_t NUM_CIRCUITS = 3)
+    static ProverOutput create_goblin_prover_output()
     {
         Goblin goblin;
-        // Construct and accumulate multiple circuits
-        for (size_t idx = 0; idx < NUM_CIRCUITS - 1; ++idx) {
-            MegaCircuitBuilder builder{ goblin.op_queue };
-            GoblinMockCircuits::construct_simple_circuit(builder);
-            goblin.prove_merge();
-        }
-
-        auto goblin_transcript = std::make_shared<Goblin::Transcript>();
+        GoblinMockCircuits::construct_and_merge_mock_circuits(goblin, 5);
 
-        Goblin goblin_final;
-        goblin_final.op_queue = goblin.op_queue;
-        MegaCircuitBuilder builder{ goblin_final.op_queue };
-        GoblinMockCircuits::construct_simple_circuit(builder);
-        goblin_final.op_queue->merge();
+        // Merge the ecc ops from the newly constructed circuit
+        auto goblin_proof = goblin.prove(MergeSettings::APPEND);
         // Subtable values and commitments - needed for (Recursive)MergeVerifier
         MergeCommitments merge_commitments;
-        auto t_current = goblin_final.op_queue->construct_current_ultra_ops_subtable_columns();
-        auto T_prev = goblin_final.op_queue->construct_previous_ultra_ops_table_columns();
-        CommitmentKey<curve::BN254> pcs_commitment_key(goblin_final.op_queue->get_ultra_ops_table_num_rows());
+        auto t_current = goblin.op_queue->construct_current_ultra_ops_subtable_columns();
+        auto T_prev = goblin.op_queue->construct_previous_ultra_ops_table_columns();
+        CommitmentKey<curve::BN254> pcs_commitment_key(goblin.op_queue->get_ultra_ops_table_num_rows());
         for (size_t idx = 0; idx < MegaFlavor::NUM_WIRES; idx++) {
             merge_commitments.t_commitments[idx] = pcs_commitment_key.commit(t_current[idx]);
             merge_commitments.T_prev_commitments[idx] = pcs_commitment_key.commit(T_prev[idx]);
         }
 
         // Output is a goblin proof plus ECCVM/Translator verification keys
-        return { goblin_final.prove(),
-                 { std::make_shared<ECCVMVK>(), std::make_shared<TranslatorVK>() },
-                 merge_commitments };
+        return { goblin_proof, { std::make_shared<ECCVMVK>(), std::make_shared<TranslatorVK>() }, merge_commitments };
     }
 };
 
@@ -94,7 +82,7 @@ TEST_F(BoomerangGoblinRecursiveVerifierTests, graph_description_basic)
     }
 
     GoblinRecursiveVerifier verifier{ &builder, verifier_input };
-    GoblinRecursiveVerifierOutput output = verifier.verify(proof, recursive_merge_commitments);
+    GoblinRecursiveVerifierOutput output = verifier.verify(proof, recursive_merge_commitments, MergeSettings::APPEND);
     output.points_accumulator.set_public();
     // Construct and verify a proof for the Goblin Recursive Verifier circuit
     {

barretenberg/cpp/src/barretenberg/constants.hpp

@@ -8,6 +8,8 @@ namespace bb {
 // permutation argument polynomials (sigmas, ids) are unique, e.g. id[i][j] == id[m][n] iff (i == m && j == n)
 constexpr uint32_t PERMUTATION_ARGUMENT_VALUE_SEPARATOR = 1 << 28;
 
+// The fixed size of the Translator trace where each accumulation gate, corresponding to one UltraOp, will occupy two
+// rows.
 static constexpr uint32_t CONST_TRANSLATOR_MINI_CIRCUIT_LOG_SIZE = 14;
 
 // -1 as each op occupies two rows in Translator trace

barretenberg/cpp/src/barretenberg/relations/translator_vm/translator_decomposition_relation_impl.hpp

@@ -74,6 +74,7 @@ void TranslatorDecompositionRelationImpl<FF>::accumulate(ContainerOverSubrelatio
         auto accumulator_high_limbs_range_constraint_3_shift = View(in.accumulator_high_limbs_range_constraint_3_shift);
         auto op = View(in.op);
         auto lagrange_even_in_minicircuit = View(in.lagrange_even_in_minicircuit);
+        auto not_even_or_no_op_scaled = lagrange_even_in_minicircuit * op * scaling_factor;
 
         // Contribution 1, accumulator lowest limb decomposition
         auto tmp_1 =
@@ -82,8 +83,7 @@ void TranslatorDecompositionRelationImpl<FF>::accumulate(ContainerOverSubrelatio
               accumulator_low_limbs_range_constraint_3 * MICRO_LIMB_SHIFTx3 +
               accumulator_low_limbs_range_constraint_4 * MICRO_LIMB_SHIFTx4) -
              accumulators_binary_limbs_0);
-        tmp_1 *= lagrange_even_in_minicircuit * op;
-        tmp_1 *= scaling_factor;
+        tmp_1 *= not_even_or_no_op_scaled;
         std::get<0>(accumulators) += tmp_1;
 
         // Contribution 2, accumulator second limb decomposition
@@ -93,8 +93,7 @@ void TranslatorDecompositionRelationImpl<FF>::accumulate(ContainerOverSubrelatio
                        accumulator_low_limbs_range_constraint_3_shift * MICRO_LIMB_SHIFTx3 +
                        accumulator_low_limbs_range_constraint_4_shift * MICRO_LIMB_SHIFTx4) -
                       accumulators_binary_limbs_1);
-        tmp_2 *= lagrange_even_in_minicircuit * op;
-        tmp_2 *= scaling_factor;
+        tmp_2 *= not_even_or_no_op_scaled;
         std::get<1>(accumulators) += tmp_2;
 
         // Contribution 3, accumulator second highest limb decomposition
@@ -104,8 +103,7 @@ void TranslatorDecompositionRelationImpl<FF>::accumulate(ContainerOverSubrelatio
               accumulator_high_limbs_range_constraint_3 * MICRO_LIMB_SHIFTx3 +
               accumulator_high_limbs_range_constraint_4 * MICRO_LIMB_SHIFTx4) -
              accumulators_binary_limbs_2);
-        tmp_3 *= lagrange_even_in_minicircuit * op;
-        tmp_3 *= scaling_factor;
+        tmp_3 *= not_even_or_no_op_scaled;
         std::get<2>(accumulators) += tmp_3;
 
         // Contribution 4, accumulator highest limb decomposition
@@ -114,8 +112,7 @@ void TranslatorDecompositionRelationImpl<FF>::accumulate(ContainerOverSubrelatio
                        accumulator_high_limbs_range_constraint_2_shift * MICRO_LIMB_SHIFTx2 +
                        accumulator_high_limbs_range_constraint_3_shift * MICRO_LIMB_SHIFTx3) -
                       accumulators_binary_limbs_3);
-        tmp_4 *= lagrange_even_in_minicircuit * op;
-        tmp_4 *= scaling_factor;
+        tmp_4 *= not_even_or_no_op_scaled;
         std::get<3>(accumulators) += tmp_4;
     }();
 

barretenberg/cpp/src/barretenberg/stdlib/goblin_verifier/goblin_recursive_verifier.test.cpp

@@ -40,13 +40,11 @@ class GoblinRecursiveVerifierTests : public testing::Test {
      *
      * @return ProverOutput
      */
-    static ProverOutput create_goblin_prover_output(Builder* outer_builder = nullptr,
-                                                    [[maybe_unused]] const size_t NUM_CIRCUITS = 3)
+    static ProverOutput create_goblin_prover_output(Builder* outer_builder = nullptr, const size_t num_circuits = 5)
     {
 
-        // TODO: do something here which is going to be painful
         Goblin goblin;
-        GoblinMockCircuits::construct_and_merge_mock_circuits(goblin, 5);
+        GoblinMockCircuits::construct_and_merge_mock_circuits(goblin, num_circuits);
 
         // Merge the ecc ops from the newly constructed circuit
         auto goblin_proof = goblin.prove(MergeSettings::APPEND);
@@ -152,11 +150,11 @@ TEST_F(GoblinRecursiveVerifierTests, IndependentVKHash)
         return { builder.blocks, outer_verification_key };
     };
 
-    auto [blocks_2, verification_key_2] = get_blocks(2);
-    auto [blocks_4, verification_key_4] = get_blocks(4);
+    auto [blocks_5, verification_key_5] = get_blocks(5);
+    auto [blocks_6, verification_key_6] = get_blocks(6);
 
-    compare_ultra_blocks_and_verification_keys<OuterFlavor>({ blocks_2, blocks_4 },
-                                                            { verification_key_2, verification_key_4 });
+    compare_ultra_blocks_and_verification_keys<OuterFlavor>({ blocks_5, blocks_6 },
+                                                            { verification_key_5, verification_key_6 });
 }
 
 /**

barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/mega_circuit_builder.cpp

@@ -62,7 +62,7 @@ template <typename FF> void MegaCircuitBuilder_<FF>::add_mega_gates_to_ensure_al
     read_return_data(read_idx);
 
     if (op_queue->get_unmerged_subtable_size() == 0) {
-        // Add a mul dummy op if the subtable to avoid column polynomial being zero (it has to be a mul rather than an
+        // Add a mul dummy op in the subtable to avoid column polynomial being zero (it has to be a mul rather than an
         // add to ensure all 4 column polynomials contain some data)
         this->queue_ecc_mul_accum(bb::g1::affine_element::one(), 2);
         this->queue_ecc_eq();

barretenberg/cpp/src/barretenberg/translator_vm/translator.test.cpp

@@ -10,11 +10,10 @@
 #include <gtest/gtest.h>
 using namespace bb;
 
-namespace {
 using CircuitBuilder = TranslatorFlavor::CircuitBuilder;
 using Transcript = TranslatorFlavor::Transcript;
 using OpQueue = ECCOpQueue;
-auto& engine = numeric::get_debug_randomness();
+static auto& engine = numeric::get_debug_randomness();
 
 class TranslatorTests : public ::testing::Test {
     using G1 = g1::affine_element;
@@ -32,7 +31,6 @@ class TranslatorTests : public ::testing::Test {
         }
     }
 
-    // Helper function to create an MSM
     static void add_mixed_ops(std::shared_ptr<bb::ECCOpQueue>& op_queue, size_t count = 100)
     {
         auto P1 = G1::random_element();
@@ -142,22 +140,6 @@ TEST_F(TranslatorTests, Basic)
     EXPECT_TRUE(verified);
 }
 
-// TEST_F(TranslatorTests, BasicWithNoOps)
-// {
-//     using Fq = fq;
-
-//     Fq batching_challenge_v = Fq::random_element();
-//     Fq evaluation_challenge_x = Fq::random_element();
-
-//     // Generate a circuit and its verification key (computed at runtime from the proving key)
-//     CircuitBuilder circuit_builder = generate_test_circuit(
-//         batching_challenge_v, evaluation_challenge_x, /*circuit_size_parameter=*/500, /*add_no_ops_region=*/true);
-
-//     EXPECT_TRUE(TranslatorCircuitChecker::check(circuit_builder));
-//     bool verified = prove_and_verify(circuit_builder, evaluation_challenge_x, batching_challenge_v);
-//     EXPECT_TRUE(verified);
-// }
-
 /**
  * @brief Ensure that the fixed VK from the default constructor agrees with those computed manually for an arbitrary
  * circuit
@@ -204,4 +186,3 @@ TEST_F(TranslatorTests, FixedVK)
     compare_computed_vk_against_fixed(circuit_size_parameter_1);
     compare_computed_vk_against_fixed(circuit_size_parameter_2);
 }
-} // namespace

barretenberg/cpp/src/barretenberg/translator_vm/translator_circuit_builder.cpp

@@ -405,7 +405,7 @@ void TranslatorCircuitBuilder::populate_wires_from_ultra_op(const UltraOp& ultra
     auto& op_wire = std::get<WireIds::OP>(wires);
     op_wire.push_back(add_variable(ultra_op.op_code.value()));
     // Similarly to the ColumnPolynomials in the merge protocol, the op_wire is 0 at every second index
-    op_wire.push_back(zero_idx);
+    op_wire.push_back(add_variable(0));
 
     insert_pair_into_wire(WireIds::X_LOW_Y_HI, ultra_op.x_lo, ultra_op.y_hi);
 
@@ -529,8 +529,8 @@ void TranslatorCircuitBuilder::feed_ecc_op_queue_into_circuit(const std::shared_
     // TODO(https://github.com/AztecProtocol/barretenberg/issues/1360): We'll also have to eventually process random
     // data in the merge protocol (added for zero knowledge)/
     for (auto& wire : wires) {
-        wire.push_back(add_variable(zero_idx));
-        wire.push_back(add_variable(zero_idx));
+        wire.push_back(add_variable(0));
+        wire.push_back(add_variable(0));
     }
     num_gates += 2;
 
@@ -568,8 +568,8 @@ void TranslatorCircuitBuilder::feed_ecc_op_queue_into_circuit(const std::shared_
             // appropriately transfered the accumulator value at k across the entire no-op range, both in even and odd
             // indices.
             for (size_t j = 0; j < ACCUMULATORS_BINARY_LIMBS_0; j++) {
-                wires[j].push_back(add_variable(zero_idx));
-                wires[j].push_back(add_variable(zero_idx));
+                wires[j].push_back(add_variable(0));
+                wires[j].push_back(add_variable(0));
             }
             size_t idx = 0;
             for (size_t j = ACCUMULATORS_BINARY_LIMBS_0; j < ACCUMULATORS_BINARY_LIMBS_3 + 1; j++) {
@@ -578,8 +578,8 @@ void TranslatorCircuitBuilder::feed_ecc_op_queue_into_circuit(const std::shared_
                 idx++;
             }
             for (size_t j = ACCUMULATORS_BINARY_LIMBS_3 + 1; j < TOTAL_COUNT; j++) {
-                wires[j].push_back(add_variable(zero_idx));
-                wires[j].push_back(add_variable(zero_idx));
+                wires[j].push_back(add_variable(0));
+                wires[j].push_back(add_variable(0));
             }
             num_gates += 2;
             continue;

barretenberg/cpp/src/barretenberg/translator_vm/translator_circuit_builder.test.cpp

@@ -112,6 +112,7 @@ TEST(TranslatorCircuitBuilder, SeveralOperationCorrectness)
 
     // Sample the evaluation input x
     Fq x = Fq::random_element();
+    // Compute x_pow (power given by the degree of the polynomial) to be number of real ultra ops - 1
     Fq x_pow = Fq(1);
     // Get an inverse
     Fq x_inv = x.invert();