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)
 {
@@ -238,53 +170,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

@@ -74,19 +74,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

@@ -49,7 +49,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);
@@ -157,15 +158,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);
@@ -253,7 +249,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);
@@ -344,7 +340,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);
@@ -420,7 +416,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);
@@ -462,7 +459,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/block_constraint.test.cpp

@@ -130,7 +130,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

@@ -109,7 +109,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);
 
@@ -188,7 +188,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();

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

@@ -148,7 +148,7 @@ TEST(ECDSASecp256r1, test_hardcoded)
         message, pub_key, signature);
     EXPECT_EQ(we_ballin, true);
 
-    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_r1_constraint.result), 1);
     auto composer = Composer();
@@ -186,7 +186,7 @@ TEST(ECDSASecp256r1, 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_r1_constraint.result), 1);
     auto composer = Composer();
@@ -263,7 +263,7 @@ TEST(ECDSASecp256r1, 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_r1_constraint.result), 0);
     auto composer = Composer();

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

@@ -102,15 +102,10 @@ Builder create_inner_circuit()
                                    .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);
 
     return builder;
 }
@@ -260,7 +255,7 @@ Builder create_outer_circuit(std::vector<Builder>& inner_circuits)
                                    .constraints = {},
                                    .block_constraints = {} };
 
-    auto outer_circuit = create_circuit_with_witness(constraint_system, witness);
+    auto outer_circuit = create_circuit(constraint_system, /*size_hint*/ 0, witness);
 
     return outer_circuit;
 }

barretenberg/cpp/src/barretenberg/dsl/acir_proofs/acir_composer.cpp

@@ -50,8 +50,8 @@ std::vector<uint8_t> AcirComposer::create_proof(acir_format::acir_format& constr
                                                 bool is_recursive)
 {
     vinfo("building circuit with witness...");
-    builder_ = acir_format::Builder(size_hint_);
-    create_circuit_with_witness(builder_, constraint_system, witness);
+    builder_ = acir_format::create_circuit(constraint_system, size_hint_, witness);
+
     vinfo("gates: ", builder_.get_total_circuit_size());
 
     auto composer = [&]() {
@@ -82,11 +82,11 @@ std::vector<uint8_t> AcirComposer::create_proof(acir_format::acir_format& constr
 void AcirComposer::create_goblin_circuit(acir_format::acir_format& constraint_system,
                                          acir_format::WitnessVector& witness)
 {
-    // The public inputs in constraint_system do not index into "witness" but rather into the future "variables" which
-    // it assumes will be equal to witness but with a prepended zero. We want to remove this +1 so that public_inputs
-    // properly indexes into witness because we're about to make calls like add_variable(witness[public_inputs[idx]]).
-    // Once the +1 is removed from noir, this correction can be removed entirely and we can use
-    // constraint_system.public_inputs directly.
+    // TODO(https://github.com/AztecProtocol/barretenberg/issues/816): The public inputs in constraint_system do not
+    // index into "witness" but rather into the future "variables" which it assumes will be equal to witness but with a
+    // prepended zero. We want to remove this +1 so that public_inputs properly indexes into witness because we're about
+    // to make calls like add_variable(witness[public_inputs[idx]]). Once the +1 is removed from noir, this correction
+    // can be removed entirely and we can use constraint_system.public_inputs directly.
     const uint32_t pre_applied_noir_offset = 1;
     std::vector<uint32_t> corrected_public_inputs;
     for (const auto& index : constraint_system.public_inputs) {

barretenberg/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp

@@ -98,7 +98,10 @@ template <typename FF> class GoblinUltraCircuitBuilder_ : public UltraCircuitBui
         : UltraCircuitBuilder_<arithmetization::UltraHonk<FF>>()
         , op_queue(op_queue_in)
     {
-        // Add the witness variables known directly from acir
+        // TODO(https://github.com/AztecProtocol/barretenberg/issues/816): NOTE: This still works even though the
+        // witness indices in the explicit acir gates are +1 offset because we're still adding the const 0 before
+        // anything else via the UCB constructor. Once we remove the +1 from Noir, we'll need to update the UCB by
+        // moving that const 0 to get these tests to pass again. Add the witness variables known directly from acir
         for (size_t idx = 0; idx < varnum; ++idx) {
             // Zeros are added for variables whose existence is known but whose values are not yet known. The values may
             // be "set" later on via the assert_equal mechanism.