refactor: op queue (#5648)

A small refactoring of the EccOpQueue to make it safer to use. Work
includes:

Closes #891
Closes #899

- Updating access specifiers on members and methods to make the API
safer, e.g. no direct access to ultra_ops, raw_ops etc.
- Explicitly connecting ultra_ops and raw_ops so that both are updated
simultaneously by a small set of well defined methods. They can no
longer be updated independently.
- Addition of some `..._for_testing()` methods to replace cases where
members that are now private were being accessed directly in tests. Its
still nice to have these for failure testing etc but it is no longer
possible to create bad witnesses without them (unless the API methods
become incorrect).

Note: I've changed the method `empty_row()` to `empty_row_for_testing()`
since it has no practical use aside from testing. I'm not sure who added
this originally. It might be better to just delete it altogether but
given the recent confusion over ECCVM tests passing when they shouldn't
I figured we can take all the testing avenues we can get.

`ClientIVCBench/Full/6      22893 ms        17742 ms            1`

cpp/src/barretenberg/benchmark/goblin_bench/eccvm.bench.cpp

@@ -35,7 +35,7 @@ Builder generate_trace(size_t target_num_gates)
         op_queue->mul_accumulate(b, y);
         op_queue->add_accumulate(a);
         op_queue->mul_accumulate(b, x);
-        op_queue->eq();
+        op_queue->eq_and_reset();
     }
 
     Builder builder{ op_queue };

cpp/src/barretenberg/eccvm/eccvm_circuit_builder.hpp

@@ -116,13 +116,12 @@ class ECCVMCircuitBuilder {
         std::vector<std::pair<size_t, size_t>> msm_mul_index;
         std::vector<size_t> msm_sizes;
 
-        // std::vector<std::vector<size_t>> msm_indices;
-        // std::vector<size_t> active_msm_indices;
-        for (size_t i = 0; i < op_queue->raw_ops.size(); ++i) {
-            const auto& op = op_queue->raw_ops[i];
+        const auto& raw_ops = op_queue->get_raw_ops();
+        size_t op_idx = 0;
+        for (const auto& op : raw_ops) {
             if (op.mul) {
                 if (op.z1 != 0 || op.z2 != 0) {
-                    msm_opqueue_index.push_back(i);
+                    msm_opqueue_index.push_back(op_idx);
                     msm_mul_index.emplace_back(msm_count, active_mul_count);
                 }
                 if (op.z1 != 0) {
@@ -136,9 +135,10 @@ class ECCVMCircuitBuilder {
                 msm_count++;
                 active_mul_count = 0;
             }
+            op_idx++;
         }
         // if last op is a mul we have not correctly computed the total number of msms
-        if (op_queue->raw_ops.back().mul) {
+        if (raw_ops.back().mul) {
             msm_sizes.push_back(active_mul_count);
             msm_count++;
         }
@@ -150,9 +150,8 @@ class ECCVMCircuitBuilder {
 
         run_loop_in_parallel(msm_opqueue_index.size(), [&](size_t start, size_t end) {
             for (size_t i = start; i < end; i++) {
-                //  for (size_t i = 0; i < msm_opqueue_index.size(); ++i) {
                 const size_t opqueue_index = msm_opqueue_index[i];
-                const auto& op = op_queue->raw_ops[opqueue_index];
+                const auto& op = raw_ops[opqueue_index];
                 auto [msm_index, mul_index] = msm_mul_index[i];
                 if (op.z1 != 0) {
                     ASSERT(msms_test.size() > msm_index);

cpp/src/barretenberg/eccvm/eccvm_circuit_builder.test.cpp

@@ -29,11 +29,11 @@ TEST(ECCVMCircuitBuilderTests, BaseCase)
     op_queue->mul_accumulate(b, y);
     op_queue->add_accumulate(a);
     op_queue->mul_accumulate(b, x);
-    op_queue->eq();
+    op_queue->eq_and_reset();
     op_queue->add_accumulate(c);
     op_queue->mul_accumulate(a, x);
     op_queue->mul_accumulate(b, x);
-    op_queue->eq();
+    op_queue->eq_and_reset();
     op_queue->mul_accumulate(a, x);
     op_queue->mul_accumulate(b, x);
     op_queue->mul_accumulate(c, x);
@@ -86,7 +86,7 @@ TEST(ECCVMCircuitBuilderTests, ShortMul)
     Fr x = small_x;
 
     op_queue->mul_accumulate(a, x);
-    op_queue->eq();
+    op_queue->eq_and_reset();
 
     ECCVMCircuitBuilder circuit{ op_queue };
     bool result = ECCVMTraceChecker::check(circuit);
@@ -95,7 +95,6 @@ TEST(ECCVMCircuitBuilderTests, ShortMul)
 
 TEST(ECCVMCircuitBuilderTests, EqFails)
 {
-    using ECCVMOperation = eccvm::VMOperation<G1>;
     std::shared_ptr<ECCOpQueue> op_queue = std::make_shared<ECCOpQueue>();
 
     auto generators = G1::derive_generators("test generators", 3);
@@ -104,14 +103,8 @@ TEST(ECCVMCircuitBuilderTests, EqFails)
 
     op_queue->mul_accumulate(a, x);
     // Tamper with the eq op such that the expected value is incorect
-    op_queue->raw_ops.emplace_back(ECCVMOperation{ .add = false,
-                                                   .mul = false,
-                                                   .eq = true,
-                                                   .reset = true,
-                                                   .base_point = a,
-                                                   .z1 = 0,
-                                                   .z2 = 0,
-                                                   .mul_scalar_full = 0 });
+    op_queue->add_erroneous_equality_op_for_testing();
+
     ECCVMCircuitBuilder circuit{ op_queue };
     bool result = ECCVMTraceChecker::check(circuit);
     EXPECT_EQ(result, false);
@@ -121,7 +114,7 @@ TEST(ECCVMCircuitBuilderTests, EmptyRow)
 {
     std::shared_ptr<ECCOpQueue> op_queue = std::make_shared<ECCOpQueue>();
 
-    op_queue->empty_row();
+    op_queue->empty_row_for_testing();
 
     ECCVMCircuitBuilder circuit{ op_queue };
     bool result = ECCVMTraceChecker::check(circuit);
@@ -137,8 +130,8 @@ TEST(ECCVMCircuitBuilderTests, EmptyRowBetweenOps)
     Fr x = Fr::random_element(&engine);
 
     op_queue->mul_accumulate(a, x);
-    op_queue->empty_row();
-    op_queue->eq();
+    op_queue->empty_row_for_testing();
+    op_queue->eq_and_reset();
 
     ECCVMCircuitBuilder circuit{ op_queue };
     bool result = ECCVMTraceChecker::check(circuit);
@@ -154,7 +147,7 @@ TEST(ECCVMCircuitBuilderTests, EndWithEq)
     Fr x = Fr::random_element(&engine);
 
     op_queue->mul_accumulate(a, x);
-    op_queue->eq();
+    op_queue->eq_and_reset();
 
     ECCVMCircuitBuilder circuit{ op_queue };
     bool result = ECCVMTraceChecker::check(circuit);
@@ -170,7 +163,7 @@ TEST(ECCVMCircuitBuilderTests, EndWithAdd)
     Fr x = Fr::random_element(&engine);
 
     op_queue->mul_accumulate(a, x);
-    op_queue->eq();
+    op_queue->eq_and_reset();
     op_queue->add_accumulate(a);
 
     ECCVMCircuitBuilder circuit{ op_queue };
@@ -187,7 +180,7 @@ TEST(ECCVMCircuitBuilderTests, EndWithMul)
     Fr x = Fr::random_element(&engine);
 
     op_queue->add_accumulate(a);
-    op_queue->eq();
+    op_queue->eq_and_reset();
     op_queue->mul_accumulate(a, x);
 
     ECCVMCircuitBuilder circuit{ op_queue };
@@ -204,10 +197,10 @@ TEST(ECCVMCircuitBuilderTests, EndWithNoop)
     Fr x = Fr::random_element(&engine);
 
     op_queue->add_accumulate(a);
-    op_queue->eq();
+    op_queue->eq_and_reset();
     op_queue->mul_accumulate(a, x);
 
-    op_queue->empty_row();
+    op_queue->empty_row_for_testing();
     ECCVMCircuitBuilder circuit{ op_queue };
     bool result = ECCVMTraceChecker::check(circuit);
     EXPECT_EQ(result, true);
@@ -228,7 +221,7 @@ TEST(ECCVMCircuitBuilderTests, MSM)
             expected += (points[i] * scalars[i]);
             op_queue->mul_accumulate(points[i], scalars[i]);
         }
-        op_queue->eq();
+        op_queue->eq_and_reset();
     };
 
     // single msms

cpp/src/barretenberg/eccvm/eccvm_composer.test.cpp

@@ -43,11 +43,11 @@ ECCVMCircuitBuilder generate_circuit(numeric::RNG* engine = nullptr)
     op_queue->mul_accumulate(b, y);
     op_queue->add_accumulate(a);
     op_queue->mul_accumulate(b, x);
-    op_queue->eq();
+    op_queue->eq_and_reset();
     op_queue->add_accumulate(c);
     op_queue->mul_accumulate(a, x);
     op_queue->mul_accumulate(b, x);
-    op_queue->eq();
+    op_queue->eq_and_reset();
     op_queue->mul_accumulate(a, x);
     op_queue->mul_accumulate(b, x);
     op_queue->mul_accumulate(c, x);
@@ -68,17 +68,10 @@ TEST_F(ECCVMComposerTests, BaseCase)
 
 TEST_F(ECCVMComposerTests, EqFails)
 {
-    using ECCVMOperation = eccvm::VMOperation<G1>;
     auto builder = generate_circuit(&engine);
     // Tamper with the eq op such that the expected value is incorect
-    builder.op_queue->raw_ops.emplace_back(ECCVMOperation{ .add = false,
-                                                           .mul = false,
-                                                           .eq = true,
-                                                           .reset = true,
-                                                           .base_point = G1::affine_one,
-                                                           .z1 = 0,
-                                                           .z2 = 0,
-                                                           .mul_scalar_full = 0 });
+    builder.op_queue->add_erroneous_equality_op_for_testing();
+
     builder.op_queue->num_transcript_rows++;
     ECCVMProver prover(builder);
 

cpp/src/barretenberg/eccvm/eccvm_flavor.hpp

@@ -520,7 +520,7 @@ class ECCVMFlavor {
 
             std::array<std::vector<size_t>, 2> point_table_read_counts;
             const auto transcript_state = ECCVMTranscriptBuilder::compute_transcript_state(
-                builder.op_queue->raw_ops, builder.get_number_of_muls());
+                builder.op_queue->get_raw_ops(), builder.get_number_of_muls());
             const auto precompute_table_state = ECCVMPrecomputedTablesBuilder::compute_precompute_state(flattened_muls);
             const auto msm_state = ECCVMMSMMBuilder::compute_msm_state(
                 msms, point_table_read_counts, builder.get_number_of_muls(), builder.op_queue->get_num_msm_rows());

cpp/src/barretenberg/eccvm/eccvm_transcript.test.cpp

@@ -218,11 +218,11 @@ class ECCVMTranscriptTests : public ::testing::Test {
         op_queue->mul_accumulate(b, y);
         op_queue->add_accumulate(a);
         op_queue->mul_accumulate(b, x);
-        op_queue->eq();
+        op_queue->eq_and_reset();
         op_queue->add_accumulate(c);
         op_queue->mul_accumulate(a, x);
         op_queue->mul_accumulate(b, x);
-        op_queue->eq();
+        op_queue->eq_and_reset();
         op_queue->mul_accumulate(a, x);
         op_queue->mul_accumulate(b, x);
         op_queue->mul_accumulate(c, x);

cpp/src/barretenberg/stdlib_circuit_builders/goblin_ultra_circuit_builder.cpp

@@ -95,25 +95,23 @@ template <typename FF> void GoblinUltraCircuitBuilder_<FF>::add_gates_to_ensure_
 }
 
 /**
- * @brief Add gates for simple point addition (no mul) and add the raw operation data to the op queue
+ * @brief Add simple point addition operation to the op queue and add corresponding gates
  *
  * @param point Point to be added into the accumulator
  */
 template <typename FF>
 ecc_op_tuple GoblinUltraCircuitBuilder_<FF>::queue_ecc_add_accum(const bb::g1::affine_element& point)
 {
-    // Add raw op to queue
-    op_queue->add_accumulate(point);
-
-    // Decompose operation inputs into width-four form and add ecc op gates
-    auto op_tuple = decompose_ecc_operands(add_accum_op_idx, point);
-    populate_ecc_op_wires(op_tuple);
+    // Add the operation to the op queue
+    auto ultra_op = op_queue->add_accumulate(point);
 
+    // Add corresponding gates for the operation
+    ecc_op_tuple op_tuple = populate_ecc_op_wires(ultra_op);
     return op_tuple;
 }
 
 /**
- * @brief Add gates for point mul-then-accumulate and add the raw operation data to the op queue
+ * @brief Add point mul-then-accumulate operation to the op queue and add corresponding gates
  *
  * @tparam FF
  * @param point
@@ -123,97 +121,58 @@ ecc_op_tuple GoblinUltraCircuitBuilder_<FF>::queue_ecc_add_accum(const bb::g1::a
 template <typename FF>
 ecc_op_tuple GoblinUltraCircuitBuilder_<FF>::queue_ecc_mul_accum(const bb::g1::affine_element& point, const FF& scalar)
 {
-    // Add raw op to op queue
-    op_queue->mul_accumulate(point, scalar);
-
-    // Decompose operation inputs into width-four form and add ecc op gates
-    auto op_tuple = decompose_ecc_operands(mul_accum_op_idx, point, scalar);
-    populate_ecc_op_wires(op_tuple);
+    // Add the operation to the op queue
+    auto ultra_op = op_queue->mul_accumulate(point, scalar);
 
+    // Add corresponding gates for the operation
+    ecc_op_tuple op_tuple = populate_ecc_op_wires(ultra_op);
     return op_tuple;
 }
 
 /**
- * @brief Add point equality gates based on the current value of the accumulator internal to the op queue and add the
- * raw operation data to the op queue
+ * @brief Add point equality operation to the op queue based on the value of the internal accumulator and add
+ * corresponding gates
  *
  * @return ecc_op_tuple encoding the point to which equality has been asserted
  */
 template <typename FF> ecc_op_tuple GoblinUltraCircuitBuilder_<FF>::queue_ecc_eq()
 {
-    // Add raw op to op queue
-    auto point = op_queue->eq();
-
-    // Decompose operation inputs into width-four form and add ecc op gates
-    auto op_tuple = decompose_ecc_operands(equality_op_idx, point);
-    populate_ecc_op_wires(op_tuple);
+    // Add the operation to the op queue
+    auto ultra_op = op_queue->eq_and_reset();
 
+    // Add corresponding gates for the operation
+    ecc_op_tuple op_tuple = populate_ecc_op_wires(ultra_op);
     return op_tuple;
 }
 
 /**
- * @brief Decompose ecc operands into components, add corresponding variables, return ecc op tuple
+ * @brief Add goblin ecc op gates for a single operation
  *
- * @param op_idx Index of op code in variables array
- * @param point
- * @param scalar
- * @return ecc_op_tuple Tuple of indices into variables array used to construct pair of ecc op gates
+ * @param ultra_op Operation data expressed in the ultra format
+ * @note All selectors are set to 0 since the ecc op selector is derived later based on the block size/location.
  */
-template <typename FF>
-ecc_op_tuple GoblinUltraCircuitBuilder_<FF>::decompose_ecc_operands(uint32_t op_idx,
-                                                                    const g1::affine_element& point,
-                                                                    const FF& scalar)
+template <typename FF> ecc_op_tuple GoblinUltraCircuitBuilder_<FF>::populate_ecc_op_wires(const UltraOp& ultra_op)
 {
-    // Decompose point coordinates (Fq) into hi-lo chunks (Fr)
-    const size_t CHUNK_SIZE = 2 * DEFAULT_NON_NATIVE_FIELD_LIMB_BITS;
-    auto x_256 = uint256_t(point.x);
-    auto y_256 = uint256_t(point.y);
-    auto x_lo = FF(x_256.slice(0, CHUNK_SIZE));
-    auto x_hi = FF(x_256.slice(CHUNK_SIZE, CHUNK_SIZE * 2));
-    auto y_lo = FF(y_256.slice(0, CHUNK_SIZE));
-    auto y_hi = FF(y_256.slice(CHUNK_SIZE, CHUNK_SIZE * 2));
-
-    // Split scalar into 128 bit endomorphism scalars
-    FF z_1 = 0;
-    FF z_2 = 0;
-    auto converted = scalar.from_montgomery_form();
-    FF::split_into_endomorphism_scalars(converted, z_1, z_2);
-    z_1 = z_1.to_montgomery_form();
-    z_2 = z_2.to_montgomery_form();
-
-    // Populate ultra ops in OpQueue with the decomposed operands
-    op_queue->populate_ultra_ops({ this->variables[op_idx], x_lo, x_hi, y_lo, y_hi, z_1, z_2 });
-
-    // Add variables for decomposition and get indices needed for op wires
-    auto x_lo_idx = this->add_variable(x_lo);
-    auto x_hi_idx = this->add_variable(x_hi);
-    auto y_lo_idx = this->add_variable(y_lo);
-    auto y_hi_idx = this->add_variable(y_hi);
-    auto z_1_idx = this->add_variable(z_1);
-    auto z_2_idx = this->add_variable(z_2);
-
-    return { op_idx, x_lo_idx, x_hi_idx, y_lo_idx, y_hi_idx, z_1_idx, z_2_idx };
-}
-
-/**
- * @brief Add ecc operation to queue
- *
- * @param in Variables array indices corresponding to operation inputs
- * @note We dont explicitly set values for the selectors here since their values are fully determined by
- * the number of ecc op gates. E.g. in the composer we can reconstruct q_ecc_op as the indicator over the range of ecc
- * op gates. All other selectors are simply 0 on this domain.
- */
-template <typename FF> void GoblinUltraCircuitBuilder_<FF>::populate_ecc_op_wires(const ecc_op_tuple& in)
-{
-    this->blocks.ecc_op.populate_wires(in.op, in.x_lo, in.x_hi, in.y_lo);
+    ecc_op_tuple op_tuple;
+    op_tuple.op = get_ecc_op_idx(ultra_op.op_code);
+    op_tuple.x_lo = this->add_variable(ultra_op.x_lo);
+    op_tuple.x_hi = this->add_variable(ultra_op.x_hi);
+    op_tuple.y_lo = this->add_variable(ultra_op.y_lo);
+    op_tuple.y_hi = this->add_variable(ultra_op.y_hi);
+    op_tuple.z_1 = this->add_variable(ultra_op.z_1);
+    op_tuple.z_2 = this->add_variable(ultra_op.z_2);
+
+    this->blocks.ecc_op.populate_wires(op_tuple.op, op_tuple.x_lo, op_tuple.x_hi, op_tuple.y_lo);
     for (auto& selector : this->blocks.ecc_op.selectors) {
         selector.emplace_back(0);
     }
 
-    this->blocks.ecc_op.populate_wires(this->zero_idx, in.y_hi, in.z_1, in.z_2);
+    this->blocks.ecc_op.populate_wires(this->zero_idx, op_tuple.y_hi, op_tuple.z_1, op_tuple.z_2);
     for (auto& selector : this->blocks.ecc_op.selectors) {
         selector.emplace_back(0);
     }
+
+    return op_tuple;
 };
 
 template <typename FF> void GoblinUltraCircuitBuilder_<FF>::set_goblin_ecc_op_code_constant_variables()