feat: acir cleanup (#3845)

The primary feature of this PR is a new a constructor of an UltraCircuitBuilder directly from acir-produced data (similar to the one already implemented for GoblinUltraCircuitBuilder). This pattern is necessary if we want to allow const variables to be added in builder constructors in bberg, since otherwise the explicit witness indices generated by acir are incorrectly offset. This change has the benefit of simplifying the code in `acir_format`. It also makes it easier to update bberg once the hardcoded +1 offset in noir is removed (#3887), which was previously accounted for in several undocumented places.

# Checklist:
Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge.
- [ ] If the pull request requires a cryptography review (e.g. cryptographic algorithm implementations) I have added the 'crypto' tag.
- [ ] I have reviewed my diff in github, line by line and removed unexpected formatting changes, testing logs, or commented-out code.
- [ ] Every change is related to the PR description.
- [ ] I have [linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue) this pull request to relevant issues (if any exist).

barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_format.cpp

@@ -7,74 +7,6 @@
 
 namespace acir_format {
 
-/**
- * @brief Populate variables and public_inputs in builder given witness and constraint_system
- * @details This method replaces consecutive calls to add_public_vars then read_witness.
- *
- * @tparam Builder
- * @param builder
- * @param witness
- * @param constraint_system
- */
-template <typename Builder>
-void populate_variables_and_public_inputs(Builder& builder,
-                                          WitnessVector const& witness,
-                                          acir_format const& constraint_system)
-{
-    // TODO(https://github.com/AztecProtocol/barretenberg/issues/816): Decrement the indices in
-    // constraint_system.public_inputs by one to account for the +1 added by default to account for a const zero
-    // variable in noir. This entire block can be removed once the +1 is removed from noir.
-    const uint32_t pre_applied_noir_offset = 1;
-    std::vector<uint32_t> corrected_public_inputs;
-    for (const auto& index : constraint_system.public_inputs) {
-        corrected_public_inputs.emplace_back(index - pre_applied_noir_offset);
-    }
-
-    for (size_t idx = 0; idx < constraint_system.varnum; ++idx) {
-        // TODO(https://github.com/AztecProtocol/barretenberg/issues/815) why is this needed?
-        fr value = idx < witness.size() ? witness[idx] : 0;
-        if (std::find(corrected_public_inputs.begin(), corrected_public_inputs.end(), idx) !=
-            corrected_public_inputs.end()) {
-            builder.add_public_variable(value);
-        } else {
-            builder.add_variable(value);
-        }
-    }
-}
-
-template <typename Builder> void read_witness(Builder& builder, WitnessVector const& witness)
-{
-    builder.variables[0] =
-        0; // TODO(https://github.com/AztecProtocol/barretenberg/issues/816): This the constant 0 hacked in. Bad.
-    for (size_t i = 0; i < witness.size(); ++i) {
-        // TODO(https://github.com/AztecProtocol/barretenberg/issues/816): The i+1 accounts for the fact that 0 is added
-        // as a constant in variables in the UCB constructor. "witness" only contains the values that came directly from
-        // acir.
-        builder.variables[i + 1] = witness[i];
-    }
-}
-
-// TODO(https://github.com/AztecProtocol/barretenberg/issues/815): This function does two things: 1) emplaces back
-// varnum-many 0s into builder.variables (.. why), and (2) populates builder.public_inputs with the correct indices into
-// the variables vector (which at this stage will be populated with zeros). The actual entries of the variables vector
-// are populated in "read_witness"
-template <typename Builder> void add_public_vars(Builder& builder, acir_format const& constraint_system)
-{
-    // TODO(https://github.com/AztecProtocol/barretenberg/issues/816): i = 1 acounting for const 0 in first position?
-    for (size_t i = 1; i < constraint_system.varnum; ++i) {
-        // If the index is in the public inputs vector, then we add it as a public input
-
-        if (std::find(constraint_system.public_inputs.begin(), constraint_system.public_inputs.end(), i) !=
-            constraint_system.public_inputs.end()) {
-
-            builder.add_public_variable(0);
-
-        } else {
-            builder.add_variable(0);
-        }
-    }
-}
-
 template <typename Builder>
 void build_constraints(Builder& builder, acir_format const& constraint_system, bool has_valid_witness_assignments)
 {
@@ -248,53 +180,29 @@ void build_constraints(Builder& builder, acir_format const& constraint_system, b
     }
 }
 
-template <typename Builder> void create_circuit(Builder& builder, acir_format const& constraint_system)
-{
-    if (constraint_system.public_inputs.size() > constraint_system.varnum) {
-        info("create_circuit: too many public inputs!");
-    }
-
-    add_public_vars(builder, constraint_system);
-    build_constraints(builder, constraint_system, false);
-}
-
-template <typename Builder> Builder create_circuit(const acir_format& constraint_system, size_t size_hint)
-{
-    Builder builder(size_hint);
-    create_circuit(builder, constraint_system);
-    return builder;
-}
-
-Builder create_circuit_with_witness(acir_format const& constraint_system,
-                                    WitnessVector const& witness,
-                                    size_t size_hint)
-{
-    Builder builder(size_hint);
-    create_circuit_with_witness(builder, constraint_system, witness);
-    return builder;
-}
-
+/**
+ * @brief Create a circuit from acir constraints and optionally a witness
+ *
+ * @tparam Builder
+ * @param constraint_system
+ * @param size_hint
+ * @param witness
+ * @return Builder
+ */
 template <typename Builder>
-void create_circuit_with_witness(Builder& builder, acir_format const& constraint_system, WitnessVector const& witness)
+Builder create_circuit(const acir_format& constraint_system, size_t size_hint, WitnessVector const& witness)
 {
-    if (constraint_system.public_inputs.size() > constraint_system.varnum) {
-        info("create_circuit_with_witness: too many public inputs!");
-    }
+    Builder builder{ size_hint, witness, constraint_system.public_inputs, constraint_system.varnum };
 
-    // Populate builder.variables and buider.public_inputs
-    populate_variables_and_public_inputs(builder, witness, constraint_system);
+    bool has_valid_witness_assignments = !witness.empty();
+    build_constraints(builder, constraint_system, has_valid_witness_assignments);
 
-    build_constraints(builder, constraint_system, true);
+    return builder;
 }
 
 template UltraCircuitBuilder create_circuit<UltraCircuitBuilder>(const acir_format& constraint_system,
-                                                                 size_t size_hint);
-template void create_circuit_with_witness<UltraCircuitBuilder>(UltraCircuitBuilder& builder,
-                                                               acir_format const& constraint_system,
-                                                               WitnessVector const& witness);
-template void create_circuit_with_witness<GoblinUltraCircuitBuilder>(GoblinUltraCircuitBuilder& builder,
-                                                                     acir_format const& constraint_system,
-                                                                     WitnessVector const& witness);
+                                                                 size_t size_hint,
+                                                                 WitnessVector const& witness);
 template void build_constraints<GoblinUltraCircuitBuilder>(GoblinUltraCircuitBuilder&, acir_format const&, bool);
 
 } // namespace acir_format

barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_format.hpp

@@ -77,19 +77,8 @@ struct acir_format {
 
 using WitnessVector = std::vector<fr, ContainerSlabAllocator<fr>>;
 
-template <typename Builder> void read_witness(Builder& builder, std::vector<barretenberg::fr> const& witness);
-
-template <typename Builder> void create_circuit(Builder& builder, const acir_format& constraint_system);
-
 template <typename Builder = UltraCircuitBuilder>
-Builder create_circuit(const acir_format& constraint_system, size_t size_hint = 0);
-
-Builder create_circuit_with_witness(const acir_format& constraint_system,
-                                    WitnessVector const& witness,
-                                    size_t size_hint = 0);
-
-template <typename Builder>
-void create_circuit_with_witness(Builder& builder, const acir_format& constraint_system, WitnessVector const& witness);
+Builder create_circuit(const acir_format& constraint_system, size_t size_hint = 0, WitnessVector const& witness = {});
 
 template <typename Builder>
 void build_constraints(Builder& builder, acir_format const& constraint_system, bool has_valid_witness_assignments);

barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_format.test.cpp

@@ -51,7 +51,8 @@ TEST_F(AcirFormatTests, TestASingleConstraintNoPubInputs)
         .block_constraints = {},
     };
 
-    auto builder = create_circuit_with_witness(constraint_system, { 0, 0, 1 });
+    WitnessVector witness{ 0, 0, 1 };
+    auto builder = create_circuit(constraint_system, /*size_hint*/ 0, witness);
 
     auto composer = Composer();
     auto prover = composer.create_ultra_with_keccak_prover(builder);
@@ -161,15 +162,10 @@ TEST_F(AcirFormatTests, TestLogicGateFromNoirCircuit)
                                    .block_constraints = {} };
 
     uint256_t inverse_of_five = fr(5).invert();
-    auto builder = create_circuit_with_witness(constraint_system,
-                                               {
-                                                   5,
-                                                   10,
-                                                   15,
-                                                   5,
-                                                   inverse_of_five,
-                                                   1,
-                                               });
+    WitnessVector witness{
+        5, 10, 15, 5, inverse_of_five, 1,
+    };
+    auto builder = create_circuit(constraint_system, /*size_hint*/ 0, witness);
 
     auto composer = Composer();
     auto prover = composer.create_ultra_with_keccak_prover(builder);
@@ -259,7 +255,7 @@ TEST_F(AcirFormatTests, TestSchnorrVerifyPass)
         witness[i] = message_string[i];
     }
 
-    auto builder = create_circuit_with_witness(constraint_system, witness);
+    auto builder = create_circuit(constraint_system, /*size_hint*/ 0, witness);
 
     auto composer = Composer();
     auto prover = composer.create_ultra_with_keccak_prover(builder);
@@ -352,7 +348,7 @@ TEST_F(AcirFormatTests, TestSchnorrVerifySmallRange)
     }
 
     // TODO: actually sign a schnorr signature!
-    auto builder = create_circuit_with_witness(constraint_system, witness);
+    auto builder = create_circuit(constraint_system, /*size_hint*/ 0, witness);
 
     auto composer = Composer();
     auto prover = composer.create_ultra_with_keccak_prover(builder);
@@ -430,7 +426,8 @@ TEST_F(AcirFormatTests, TestVarKeccak)
         .block_constraints = {},
     };
 
-    auto builder = create_circuit_with_witness(constraint_system, { 4, 2, 6, 2 });
+    WitnessVector witness{ 4, 2, 6, 2 };
+    auto builder = create_circuit(constraint_system, /*size_hint*/ 0, witness);
 
     auto composer = Composer();
     auto prover = composer.create_ultra_with_keccak_prover(builder);
@@ -474,7 +471,7 @@ TEST_F(AcirFormatTests, TestKeccakPermutation)
                            18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34,
                            35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50 };
 
-    auto builder = create_circuit_with_witness(constraint_system, witness);
+    auto builder = create_circuit(constraint_system, /*size_hint=*/0, witness);
 
     auto composer = Composer();
     auto prover = composer.create_ultra_with_keccak_prover(builder);

barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_to_constraint_buf.hpp

@@ -19,8 +19,22 @@
 
 namespace acir_format {
 
+/**
+ * @brief Construct a poly_tuple for a standard width-3 arithmetic gate from its acir representation
+ *
+ * @param arg acir representation of an 3-wire arithmetic operation
+ * @return poly_triple
+ * @note In principle Circuit::Expression can accommodate arbitrarily many quadratic and linear terms but in practice
+ * the ones processed here have a max of 1 and 3 respectively, in accordance with the standard width-3 arithmetic gate.
+ */
 poly_triple serialize_arithmetic_gate(Circuit::Expression const& arg)
 {
+    // TODO(https://github.com/AztecProtocol/barretenberg/issues/816): Instead of zeros for a,b,c, what we really want
+    // is something like the bberg zero_idx. Hardcoding these to 0 has the same effect only if zero_idx == 0, as has
+    // historically been the case. If that changes, this might break since now "0" points to some non-zero witness in
+    // variables. (From some testing, it seems like it may not break but only because the selectors multiplying the
+    // erroneously non-zero value are zero. Still, it seems like a bad idea to have erroneous wire values even if they
+    // dont break the relation. They'll still add cost in commitments, for example).
     poly_triple pt{
         .a = 0,
         .b = 0,
@@ -31,32 +45,53 @@ poly_triple serialize_arithmetic_gate(Circuit::Expression const& arg)
         .q_o = 0,
         .q_c = 0,
     };
-    // Think this never longer than 1?
-    for (const auto& e : arg.mul_terms) {
-        uint256_t qm(std::get<0>(e));
-        uint32_t a = std::get<1>(e).value;
-        uint32_t b = std::get<2>(e).value;
-        pt.q_m = qm;
-        pt.a = a;
-        pt.b = b;
+
+    // Flags indicating whether each witness index for the present poly_tuple has been set
+    bool a_set = false;
+    bool b_set = false;
+    bool c_set = false;
+
+    // If necessary, set values for quadratic term (q_m * w_l * w_r)
+    ASSERT(arg.mul_terms.size() <= 1); // We can only accommodate 1 quadratic term
+    // Note: mul_terms are tuples of the form {selector_value, witness_idx_1, witness_idx_2}
+    if (!arg.mul_terms.empty()) {
+        const auto& mul_term = arg.mul_terms[0];
+        pt.q_m = uint256_t(std::get<0>(mul_term));
+        pt.a = std::get<1>(mul_term).value;
+        pt.b = std::get<2>(mul_term).value;
+        a_set = true;
+        b_set = true;
     }
-    for (const auto& e : arg.linear_combinations) {
-        barretenberg::fr x(uint256_t(std::get<0>(e)));
-        uint32_t witness = std::get<1>(e).value;
 
-        if (pt.a == 0 || pt.a == witness) {
-            pt.a = witness;
-            pt.q_l = x;
-        } else if (pt.b == 0 || pt.b == witness) {
-            pt.b = witness;
-            pt.q_r = x;
-        } else if (pt.c == 0 || pt.c == witness) {
-            pt.c = witness;
-            pt.q_o = x;
+    // If necessary, set values for linears terms q_l * w_l, q_r * w_r and q_o * w_o
+    ASSERT(arg.linear_combinations.size() <= 3); // We can only accommodate 3 linear terms
+    for (const auto& linear_term : arg.linear_combinations) {
+        barretenberg::fr selector_value(uint256_t(std::get<0>(linear_term)));
+        uint32_t witness_idx = std::get<1>(linear_term).value;
+
+        // If the witness index has not yet been set or if the corresponding linear term is active, set the witness
+        // index and the corresponding selector value.
+        // TODO(https://github.com/AztecProtocol/barretenberg/issues/816): May need to adjust the pt.a == witness_idx
+        // check (and the others like it) since we initialize a,b,c with 0 but 0 becomes a valid witness index if the +1
+        // offset is removed from noir.
+        if (!a_set || pt.a == witness_idx) { // q_l * w_l
+            pt.a = witness_idx;
+            pt.q_l = selector_value;
+            a_set = true;
+        } else if (!b_set || pt.b == witness_idx) { // q_r * w_r
+            pt.b = witness_idx;
+            pt.q_r = selector_value;
+            b_set = true;
+        } else if (!c_set || pt.c == witness_idx) { // q_o * w_o
+            pt.c = witness_idx;
+            pt.q_o = selector_value;
+            c_set = true;
         } else {
-            throw_or_abort("Cannot assign linear term to a constrain of width 3");
+            throw_or_abort("Cannot assign linear term to a constraint of width 3");
         }
     }
+
+    // Set constant value q_c
     pt.q_c = uint256_t(arg.q_c);
     return pt;
 }
@@ -259,9 +294,7 @@ acir_format circuit_buf_to_acir_format(std::vector<uint8_t> const& buf)
     auto circuit = Circuit::Circuit::bincodeDeserialize(buf);
 
     acir_format af;
-    // TODO(https://github.com/AztecProtocol/barretenberg/issues/816): this +1 seems to be accounting for the const 0 at
-    // the first index in variables
-    af.varnum = circuit.current_witness_index + 1;
+    af.varnum = circuit.current_witness_index;
     af.public_inputs = join({ map(circuit.public_parameters.value, [](auto e) { return e.value; }),
                               map(circuit.return_values.value, [](auto e) { return e.value; }) });
     std::map<uint32_t, BlockConstraint> block_id_to_block_constraint;
@@ -299,10 +332,16 @@ WitnessVector witness_buf_to_witness_data(std::vector<uint8_t> const& buf)
 {
     auto w = WitnessMap::WitnessMap::bincodeDeserialize(buf);
     WitnessVector wv;
+    // TODO(https://github.com/AztecProtocol/barretenberg/issues/816): Does "index" need to be initialized to 0 once we
+    // get rid of the +1 offeet in noir?
     size_t index = 1;
     for (auto& e : w.value) {
+        // TODO(https://github.com/AztecProtocol/barretenberg/issues/824): Document what this mechanism is doing. It
+        // appears to be somewhat unrelated to the const 0 issue (but see discussion in issue for caveats). See 2_div
+        // for an example of when this gets used. Seems like a mechanism for adding 0s intermittently between known
+        // witness values.
         while (index < e.first.value) {
-            wv.push_back(barretenberg::fr(0)); // TODO(https://github.com/AztecProtocol/barretenberg/issues/816)?
+            wv.push_back(barretenberg::fr(0));
             index++;
         }
         wv.push_back(barretenberg::fr(uint256_t(e.second)));

barretenberg/cpp/src/barretenberg/dsl/acir_format/block_constraint.test.cpp

@@ -132,7 +132,7 @@ TEST_F(UltraPlonkRAM, TestBlockConstraint)
         .block_constraints = { block },
     };
 
-    auto builder = create_circuit_with_witness(constraint_system, witness_values);
+    auto builder = create_circuit(constraint_system, /*size_hint*/ 0, witness_values);
 
     auto composer = Composer();
     auto prover = composer.create_prover(builder);

barretenberg/cpp/src/barretenberg/dsl/acir_format/ecdsa_secp256k1.test.cpp

@@ -111,7 +111,7 @@ TEST_F(ECDSASecp256k1, TestECDSAConstraintSucceed)
         .block_constraints = {},
     };
 
-    auto builder = create_circuit_with_witness(constraint_system, witness_values);
+    auto builder = create_circuit(constraint_system, /*size_hint*/ 0, witness_values);
 
     EXPECT_EQ(builder.get_variable(ecdsa_k1_constraint.result), 1);
 
@@ -194,7 +194,7 @@ TEST_F(ECDSASecp256k1, TestECDSAConstraintFail)
         .block_constraints = {},
     };
 
-    auto builder = create_circuit_with_witness(constraint_system, witness_values);
+    auto builder = create_circuit(constraint_system, /*size_hint*/ 0, witness_values);
     EXPECT_EQ(builder.get_variable(ecdsa_k1_constraint.result), 0);
 
     auto composer = Composer();