fix: make translator use ultra rather than eccvm ops (#13489)

While attempting to implement the consistency check between the Merge
and Translator Verifier I realised that the TranslatorCircuitBuilder is
constructed using the VMops, redundantly converted to UltraOps. This PR
addressed the issue and attempts a small cleanup on the
`UltraEccOpsTable`.

Closes AztecProtocol/barretenberg#1266

Author: maramihali

barretenberg/cpp/src/barretenberg/op_queue/ecc_op_queue.hpp

@@ -5,7 +5,6 @@
 #include "barretenberg/op_queue/ecc_ops_table.hpp"
 #include "barretenberg/op_queue/eccvm_row_tracker.hpp"
 #include "barretenberg/polynomials/polynomial.hpp"
-#include "barretenberg/stdlib/primitives/bigfield/constants.hpp"
 namespace bb {
 
 /**
@@ -28,15 +27,17 @@ class ECCOpQueue {
     // The operations written to the queue are also performed natively; the result is stored in accumulator
     Point accumulator = point_at_infinity;
 
-    static constexpr size_t DEFAULT_NON_NATIVE_FIELD_LIMB_BITS = stdlib::NUM_LIMB_BITS_IN_FIELD_SIMULATION;
-
     EccvmOpsTable eccvm_ops_table;    // table of ops in the ECCVM format
     UltraEccOpsTable ultra_ops_table; // table of ops in the Ultra-arithmetization format
 
     // Storage for the reconstructed eccvm ops table in contiguous memory. (Intended to be constructed once and for all
     // prior to ECCVM construction to avoid repeated prepending of subtables in physical memory).
     std::vector<ECCVMOperation> eccvm_ops_reconstructed;
 
+    // Storage for the reconstructed ultra ops table in contiguous memory. (Intended to be constructed once and for all
+    // prior to Translator circuit construction to avoid repeated prepending of subtables in physical memory).
+    std::vector<UltraOp> ultra_ops_reconstructed;
+
     // Tracks number of muls and size of eccvm in real time as the op queue is updated
     EccvmRowTracker eccvm_row_tracker;
 
@@ -72,9 +73,15 @@ class ECCOpQueue {
     // Reconstruct the full table of eccvm ops in contiguous memory from the independent subtables
     void construct_full_eccvm_ops_table() { eccvm_ops_reconstructed = eccvm_ops_table.get_reconstructed(); }
 
+    // Reconstruct the full table of ultra ops in contiguous memory from the independent subtables
+    void construct_full_ultra_ops_table() { ultra_ops_reconstructed = ultra_ops_table.get_reconstructed(); }
+
     size_t get_ultra_ops_table_num_rows() const { return ultra_ops_table.ultra_table_size(); }
     size_t get_current_ultra_ops_subtable_num_rows() const { return ultra_ops_table.current_ultra_subtable_size(); }
 
+    // TODO(https://github.com/AztecProtocol/barretenberg/issues/1339): Consider making the ultra and eccvm ops getters
+    // more memory efficient
+
     // Get the full table of ECCVM ops in contiguous memory; construct it if it has not been constructed already
     std::vector<ECCVMOperation>& get_eccvm_ops()
     {
@@ -84,6 +91,14 @@ class ECCOpQueue {
         return eccvm_ops_reconstructed;
     }
 
+    std::vector<UltraOp>& get_ultra_ops()
+    {
+        if (ultra_ops_reconstructed.empty()) {
+            construct_full_ultra_ops_table();
+        }
+        return ultra_ops_reconstructed;
+    }
+
     /**
      * @brief Get the number of rows in the 'msm' column section, for all msms in the circuit
      */
@@ -239,7 +254,7 @@ class ECCOpQueue {
         ultra_op.op_code = op_code;
 
         // Decompose point coordinates (Fq) into hi-lo chunks (Fr)
-        const size_t CHUNK_SIZE = 2 * DEFAULT_NON_NATIVE_FIELD_LIMB_BITS;
+        const size_t CHUNK_SIZE = 2 * stdlib::NUM_LIMB_BITS_IN_FIELD_SIMULATION;
         uint256_t x_256(point.x);
         uint256_t y_256(point.y);
         ultra_op.return_is_infinity = point.is_point_at_infinity();

barretenberg/cpp/src/barretenberg/op_queue/ecc_ops_table.hpp

@@ -3,6 +3,7 @@
 #include "barretenberg/ecc/curves/bn254/bn254.hpp"
 #include "barretenberg/eccvm/eccvm_builder_types.hpp"
 #include "barretenberg/polynomials/polynomial.hpp"
+#include "barretenberg/stdlib/primitives/bigfield/constants.hpp"
 #include <deque>
 namespace bb {
 
@@ -35,6 +36,7 @@ struct EccOpCode {
 
 struct UltraOp {
     using Fr = curve::BN254::ScalarField;
+    using Fq = curve::BN254::BaseField;
     EccOpCode op_code;
     Fr x_lo;
     Fr x_hi;
@@ -43,34 +45,32 @@ struct UltraOp {
     Fr z_1;
     Fr z_2;
     bool return_is_infinity;
-};
-
-template <typename CycleGroup> struct VMOperation {
-    EccOpCode op_code = {};
-    typename CycleGroup::affine_element base_point = typename CycleGroup::affine_element{ 0, 0 };
-    uint256_t z1 = 0;
-    uint256_t z2 = 0;
-    typename CycleGroup::subgroup_field mul_scalar_full = 0;
-    bool operator==(const VMOperation<CycleGroup>& other) const = default;
 
     /**
      * @brief Get the point in standard form i.e. as two coordinates x and y in the base field or as a point at
      * infinity whose coordinates are set to (0,0).
      *
-     * @details These are represented as uint265_t to make chunking easier, the function being used in translator
-     * where each coordinate is chunked to efficiently be represented in the scalar field.
      */
-    std::array<uint256_t, 2> get_base_point_standard_form() const
+    std::array<Fq, 2> get_base_point_standard_form() const
     {
-        uint256_t x(base_point.x);
-        uint256_t y(base_point.y);
-        if (base_point.is_point_at_infinity()) {
-            x = 0;
-            y = 0;
+        if (return_is_infinity) {
+            return { Fq(0), Fq(0) };
         }
+        auto x = Fq((uint256_t(x_hi) << 2 * stdlib::NUM_LIMB_BITS_IN_FIELD_SIMULATION) + uint256_t(x_lo));
+        auto y = Fq((uint256_t(y_hi) << 2 * stdlib::NUM_LIMB_BITS_IN_FIELD_SIMULATION) + uint256_t(y_lo));
+
         return { x, y };
     }
 };
+
+template <typename CycleGroup> struct VMOperation {
+    EccOpCode op_code = {};
+    typename CycleGroup::affine_element base_point = typename CycleGroup::affine_element{ 0, 0 };
+    uint256_t z1 = 0;
+    uint256_t z2 = 0;
+    typename CycleGroup::subgroup_field mul_scalar_full = 0;
+    bool operator==(const VMOperation<CycleGroup>& other) const = default;
+};
 using ECCVMOperation = VMOperation<curve::BN254::Group>;
 
 /**
@@ -141,7 +141,7 @@ template <typename OpFormat> class EccOpsTable {
     }
 };
 
-/***
+/**
  * @brief A VM operation is represented as one row with 6 columns in the ECCVM version of the Op Queue.
  * | OP | X | Y | z_1 | z_2 | mul_scalar_full |
  */
@@ -181,6 +181,7 @@ class UltraEccOpsTable {
     size_t previous_ultra_table_size() const { return (ultra_table_size() - current_ultra_subtable_size()); }
     void create_new_subtable(size_t size_hint = 0) { table.create_new_subtable(size_hint); }
     void push(const UltraOp& op) { table.push(op); }
+    std::vector<UltraOp> get_reconstructed() const { return table.get_reconstructed(); }
 
     // Construct the columns of the full ultra ecc ops table
     ColumnPolynomials construct_table_columns() const

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

@@ -544,89 +544,52 @@ void TranslatorCircuitBuilder::create_accumulation_gate(const AccumulationInput
     bb::constexpr_for<0, TOTAL_COUNT, 1>([&]<size_t i>() { ASSERT(std::get<i>(wires).size() == num_gates); });
 }
 
-/**
- * @brief Given an ECCVM operation, previous accumulator and necessary challenges, compute witnesses for one
- * accumulation
- *
- * @tparam Fq
- * @return TranslatorCircuitBuilder::AccumulationInput
- */
-
-TranslatorCircuitBuilder::AccumulationInput TranslatorCircuitBuilder::compute_witness_values_for_one_ecc_op(
-    const ECCVMOperation& ecc_op,
-    const Fq previous_accumulator,
-    const Fq batching_challenge_v,
-    const Fq evaluation_input_x)
-{
-    // Get the Opcode value
-    Fr op(ecc_op.op_code.value());
-    Fr p_x_lo(0);
-    Fr p_x_hi(0);
-    Fr p_y_lo(0);
-    Fr p_y_hi(0);
-
-    // Split P.x and P.y into their representations in bn254 transcript
-    // if we have a point at infinity, set x/y to zero
-    // in the biggroup_goblin class we use `assert_equal` statements to validate
-    // the original in-circuit coordinate values are also zero
-    const auto [x_256, y_256] = ecc_op.get_base_point_standard_form();
-
-    p_x_lo = Fr(uint256_t(x_256).slice(0, 2 * NUM_LIMB_BITS));
-    p_x_hi = Fr(uint256_t(x_256).slice(2 * NUM_LIMB_BITS, 4 * NUM_LIMB_BITS));
-    p_y_lo = Fr(uint256_t(y_256).slice(0, 2 * NUM_LIMB_BITS));
-    p_y_hi = Fr(uint256_t(y_256).slice(2 * NUM_LIMB_BITS, 4 * NUM_LIMB_BITS));
-
-    // Generate the full witness values
-    return generate_witness_values(op,
-                                   p_x_lo,
-                                   p_x_hi,
-                                   p_y_lo,
-                                   p_y_hi,
-                                   Fr(ecc_op.z1),
-                                   Fr(ecc_op.z2),
-                                   previous_accumulator,
-                                   batching_challenge_v,
-                                   evaluation_input_x);
-}
-
-// TODO(https://github.com/AztecProtocol/barretenberg/issues/1266): Evaluate whether this method can reuse existing data
-// in the op queue for improved efficiency
 void TranslatorCircuitBuilder::feed_ecc_op_queue_into_circuit(const std::shared_ptr<ECCOpQueue> ecc_op_queue)
 {
     using Fq = bb::fq;
-    const auto& eccvm_ops = ecc_op_queue->get_eccvm_ops();
+    const auto& ultra_ops = ecc_op_queue->get_ultra_ops();
     std::vector<Fq> accumulator_trace;
     Fq current_accumulator(0);
-    if (eccvm_ops.empty()) {
+    if (ultra_ops.empty()) {
         return;
     }
-    // Rename for ease of use
-    auto x = evaluation_input_x;
-    auto v = batching_challenge_v;
 
     // We need to precompute the accumulators at each step, because in the actual circuit we compute the values starting
     // from the later indices. We need to know the previous accumulator to create the gate
-    for (size_t i = 0; i < eccvm_ops.size(); i++) {
-        const auto& ecc_op = eccvm_ops[eccvm_ops.size() - 1 - i];
-        current_accumulator *= x;
-        const auto [x_256, y_256] = ecc_op.get_base_point_standard_form();
+    for (size_t i = 0; i < ultra_ops.size(); i++) {
+        const auto& ultra_op = ultra_ops[ultra_ops.size() - 1 - i];
+        current_accumulator *= evaluation_input_x;
+        const auto [x_256, y_256] = ultra_op.get_base_point_standard_form();
         current_accumulator +=
-            (Fq(ecc_op.op_code.value()) + v * (x_256 + v * (y_256 + v * (ecc_op.z1 + v * ecc_op.z2))));
+            Fq(ultra_op.op_code.value()) +
+            batching_challenge_v *
+                (x_256 + batching_challenge_v *
+                             (y_256 + batching_challenge_v *
+                                          (uint256_t(ultra_op.z_1) + batching_challenge_v * uint256_t(ultra_op.z_2))));
         accumulator_trace.push_back(current_accumulator);
     }
 
     // We don't care about the last value since we'll recompute it during witness generation anyway
     accumulator_trace.pop_back();
 
-    for (const auto& eccvm_op : eccvm_ops) {
+    for (const auto& ultra_op : ultra_ops) {
         Fq previous_accumulator = 0;
         // Pop the last value from accumulator trace and use it as previous accumulator
         if (!accumulator_trace.empty()) {
             previous_accumulator = accumulator_trace.back();
             accumulator_trace.pop_back();
         }
         // Compute witness values
-        auto one_accumulation_step = compute_witness_values_for_one_ecc_op(eccvm_op, previous_accumulator, v, x);
+        AccumulationInput one_accumulation_step = generate_witness_values(ultra_op.op_code.value(),
+                                                                          ultra_op.x_lo,
+                                                                          ultra_op.x_hi,
+                                                                          ultra_op.y_lo,
+                                                                          ultra_op.y_hi,
+                                                                          ultra_op.z_1,
+                                                                          ultra_op.z_2,
+                                                                          previous_accumulator,
+                                                                          batching_challenge_v,
+                                                                          evaluation_input_x);
 
         // And put them into the wires
         create_accumulation_gate(one_accumulation_step);

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

@@ -469,10 +469,6 @@ class TranslatorCircuitBuilder : public CircuitBuilderBase<bb::fr> {
                                                      const Fq previous_accumulator,
                                                      const Fq batching_challenge_v,
                                                      const Fq evaluation_input_x);
-    static AccumulationInput compute_witness_values_for_one_ecc_op(const ECCVMOperation& ecc_op,
-                                                                   const Fq previous_accumulator,
-                                                                   const Fq batching_challenge_v,
-                                                                   const Fq evaluation_input_x);
 };
 
 } // namespace bb

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

@@ -105,16 +105,16 @@ TEST(TranslatorCircuitBuilder, SeveralOperationCorrectness)
     // Get an inverse
     Fq x_inv = x.invert();
     // Compute the batched evaluation of polynomials (multiplying by inverse to go from lower to higher)
-    const auto& eccvm_ops = op_queue->get_eccvm_ops();
-    for (const auto& ecc_op : eccvm_ops) {
+    const auto& ultra_ops = op_queue->get_ultra_ops();
+    for (const auto& ecc_op : ultra_ops) {
         op_accumulator = op_accumulator * x_inv + ecc_op.op_code.value();
         const auto [x_u256, y_u256] = ecc_op.get_base_point_standard_form();
         p_x_accumulator = p_x_accumulator * x_inv + x_u256;
         p_y_accumulator = p_y_accumulator * x_inv + y_u256;
-        z_1_accumulator = z_1_accumulator * x_inv + ecc_op.z1;
-        z_2_accumulator = z_2_accumulator * x_inv + ecc_op.z2;
+        z_1_accumulator = z_1_accumulator * x_inv + uint256_t(ecc_op.z_1);
+        z_2_accumulator = z_2_accumulator * x_inv + uint256_t(ecc_op.z_2);
     }
-    Fq x_pow = x.pow(eccvm_ops.size() - 1);
+    Fq x_pow = x.pow(ultra_ops.size() - 1);
 
     // Multiply by an appropriate power of x to get rid of the inverses
     Fq result = ((((z_2_accumulator * batching_challenge + z_1_accumulator) * batching_challenge + p_y_accumulator) *