feat: acir cleanup

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