feat: Correct circuit construction from acir

barretenberg/acir_tests/Dockerfile.bb

@@ -11,6 +11,6 @@ COPY . .
 # This ensures we test independent pk construction through real/garbage witness data paths.
 RUN FLOW=prove_then_verify ./run_acir_tests.sh
 # TODO(https://github.com/AztecProtocol/barretenberg/issues/811) make this able to run the default test
-RUN FLOW=prove_and_verify_goblin ./run_acir_tests.sh assert_statement
+RUN FLOW=prove_and_verify_goblin ./run_acir_tests.sh
 # Run 1_mul through native bb build, all_cmds flow, to test all cli args.
 RUN VERBOSE=1 FLOW=all_cmds ./run_acir_tests.sh 1_mul

barretenberg/acir_tests/Dockerfile.bb.js

@@ -15,7 +15,7 @@ ENV VERBOSE=1
 # Run double_verify_proof through bb.js on node to check 512k support.
 RUN BIN=../ts/dest/node/main.js FLOW=prove_then_verify ./run_acir_tests.sh double_verify_proof
 # TODO(https://github.com/AztecProtocol/barretenberg/issues/811) make this able to run double_verify_proof
-RUN BIN=../ts/dest/node/main.js FLOW=prove_and_verify_goblin ./run_acir_tests.sh assert_statement
+RUN BIN=../ts/dest/node/main.js FLOW=prove_and_verify_goblin ./run_acir_tests.sh
 # Run 1_mul through bb.js build, all_cmds flow, to test all cli args.
 RUN BIN=../ts/dest/node/main.js FLOW=all_cmds ./run_acir_tests.sh 1_mul
 # Run double_verify_proof through bb.js on chrome testing multi-threaded browser support.

barretenberg/acir_tests/flows/prove_and_verify_goblin.sh

@@ -3,4 +3,7 @@ set -eu
 
 VFLAG=${VERBOSE:+-v}
 
-$BIN prove_and_verify_goblin $VFLAG -c $CRS_PATH -b ./target/acir.gz
+$BIN prove_and_verify_goblin $VFLAG -c $CRS_PATH -b ./target/acir.gz
+
+# This command can be used to run all of the tests in sequence with the debugger
+# lldb-16 -o run -b -- $BIN prove_and_verify_goblin $VFLAG -c $CRS_PATH -b ./target/acir.gz

barretenberg/cpp/src/barretenberg/bb/main.cpp

@@ -52,17 +52,18 @@ acir_proofs::AcirComposer init(acir_format::acir_format& constraint_system)
 
 void init_reference_strings()
 {
-    // TODO(https://github.com/AztecProtocol/barretenberg/issues/811): Don't hardcode subgroup size
-    size_t subgroup_size = 32768;
+    // TODO(https://github.com/AztecProtocol/barretenberg/issues/811): Don't hardcode subgroup size. Currently set to
+    // max circuit size present in acir tests suite.
+    size_t hardcoded_subgroup_size_hack = 262144;
 
     // TODO(https://github.com/AztecProtocol/barretenberg/issues/811) reduce duplication with above
     // Must +1!
-    auto g1_data = get_bn254_g1_data(CRS_PATH, subgroup_size + 1);
+    auto g1_data = get_bn254_g1_data(CRS_PATH, hardcoded_subgroup_size_hack + 1);
     auto g2_data = get_bn254_g2_data(CRS_PATH);
     srs::init_crs_factory(g1_data, g2_data);
 
     // Must +1!
-    auto grumpkin_g1_data = get_grumpkin_g1_data(CRS_PATH, subgroup_size + 1);
+    auto grumpkin_g1_data = get_grumpkin_g1_data(CRS_PATH, hardcoded_subgroup_size_hack + 1);
     srs::init_grumpkin_crs_factory(grumpkin_g1_data);
 }
 
@@ -145,23 +146,18 @@ bool proveAndVerifyGoblin(const std::string& bytecodePath,
                           const std::string& witnessPath,
                           [[maybe_unused]] bool recursive)
 {
-    info("Construct constraint_system and witness.");
     auto constraint_system = get_constraint_system(bytecodePath);
     auto witness = get_witness(witnessPath);
 
     init_reference_strings();
 
-    info("Construct goblin circuit from constraint system and witness.");
     acir_proofs::AcirComposer acir_composer;
     acir_composer.create_goblin_circuit(constraint_system, witness);
 
-    info("Construct goblin proof.");
     auto proof = acir_composer.create_goblin_proof();
 
-    info("verify_goblin_proof.");
     auto verified = acir_composer.verify_goblin_proof(proof);
 
-    vinfo("verified: ", verified);
     return verified;
 }
 

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

@@ -154,7 +154,9 @@ void build_constraints(Builder& builder, acir_format const& constraint_system, b
     // TODO(https://github.com/AztecProtocol/barretenberg/issues/817): disable these for UGH for now since we're not yet
     // dealing with proper recursion
     if constexpr (IsGoblinBuilder<Builder>) {
-        info("WARNING: this circuit contains recursion_constraints!");
+        if (constraint_system.recursion_constraints.size() > 0) {
+            info("WARNING: this circuit contains recursion_constraints!");
+        }
     } else {
         // These are set and modified whenever we encounter a recursion opcode
         //
@@ -272,36 +274,6 @@ void create_circuit_with_witness(Builder& builder, acir_format const& constraint
     build_constraints(builder, constraint_system, true);
 }
 
-/**
- * @brief Apply an offset to the indices stored in the wires
- * @details This method is needed due to the following: Noir constructs "wires" as indices into a "witness" vector.
- * This is analogous to the wires and variables vectors in bberg builders. Were it not for the addition of constant
- * variables in the constructors of a builder (e.g. zero), we would simply have noir.wires = builder.wires and
- * noir.witness = builder.variables. To account for k-many constant variables in the first entries of the variables
- * array, we have something like variables = variables.append(noir.witness). Accordingly, the indices in noir.wires
- * have to be incremented to account for the offset at which noir.wires was placed into variables.
- *
- * @tparam Builder
- * @param builder
- */
-template <typename Builder> void apply_wire_index_offset(Builder& builder)
-{
-    // For now, noir has a hard coded witness index offset = 1. Once this is removed, this pre-applied offset goes
-    // away
-    const uint32_t pre_applied_noir_offset = 1;
-    auto offset = static_cast<uint32_t>(builder.num_vars_added_in_constructor - pre_applied_noir_offset);
-    info("Applying offset = ", offset);
-
-    // Apply the offset to the indices stored the wires that were generated from acir. (Do not apply the offset to
-    // those values that were added in the builder constructor).
-    size_t start_index = builder.num_vars_added_in_constructor;
-    for (auto& wire : builder.wires) {
-        for (size_t idx = start_index; idx < wire.size(); ++idx) {
-            wire[idx] += offset;
-        }
-    }
-}
-
 template UltraCircuitBuilder create_circuit<UltraCircuitBuilder>(const acir_format& constraint_system,
                                                                  size_t size_hint);
 template void create_circuit_with_witness<UltraCircuitBuilder>(UltraCircuitBuilder& builder,
@@ -310,7 +282,6 @@ template void create_circuit_with_witness<UltraCircuitBuilder>(UltraCircuitBuild
 template void create_circuit_with_witness<GoblinUltraCircuitBuilder>(GoblinUltraCircuitBuilder& builder,
                                                                      acir_format const& constraint_system,
                                                                      WitnessVector const& witness);
-template void apply_wire_index_offset<GoblinUltraCircuitBuilder>(GoblinUltraCircuitBuilder& builder);
-template void apply_wire_index_offset<UltraCircuitBuilder>(UltraCircuitBuilder& builder);
+template void build_constraints<GoblinUltraCircuitBuilder>(GoblinUltraCircuitBuilder&, acir_format const&, bool);
 
 } // namespace acir_format

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

@@ -86,6 +86,7 @@ Builder create_circuit_with_witness(const acir_format& constraint_system,
 template <typename Builder>
 void create_circuit_with_witness(Builder& builder, const acir_format& constraint_system, WitnessVector const& witness);
 
-template <typename Builder> void apply_wire_index_offset(Builder& builder);
+template <typename Builder>
+void build_constraints(Builder& builder, acir_format const& constraint_system, bool has_valid_witness_assignments);
 
 } // namespace acir_format

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

@@ -81,17 +81,26 @@ 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)
 {
-    // Provide the builder with the op queue owned by the goblin instance
-    goblin_builder_.op_queue = goblin.op_queue;
-
-    create_circuit_with_witness(goblin_builder_, constraint_system, 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.
+    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);
+    }
 
-    info("after create_circuit_with_witness: num_gates = ", goblin_builder_.num_gates);
+    // Construct a builder using the witness and public input data from acir
+    goblin_builder_ =
+        acir_format::GoblinBuilder{ goblin.op_queue, witness, corrected_public_inputs, constraint_system.varnum };
 
-    // Correct for the addition of const variables in the builder constructor
-    acir_format::apply_wire_index_offset(goblin_builder_);
+    // Populate constraints in the builder via the data in constraint_system
+    acir_format::build_constraints(goblin_builder_, constraint_system, true);
 
-    // Add some arbitrary op gates to ensure the associated polynomials are non-zero
+    // TODO(https://github.com/AztecProtocol/barretenberg/issues/817): Add some arbitrary op gates to ensure the
+    // associated polynomials are non-zero and to give ECCVM and Translator some ECC ops to process.
     GoblinTestingUtils::construct_goblin_ecc_op_circuit(goblin_builder_);
 }
 

barretenberg/cpp/src/barretenberg/eccvm/eccvm_verifier.cpp

@@ -195,11 +195,12 @@ template <typename Flavor> bool ECCVMVerifier_<Flavor>::verify_proof(const plonk
     // Construct batched commitment for NON-shifted polynomials
     size_t commitment_idx = 0;
     for (auto& commitment : commitments.get_unshifted()) {
-        // TODO(@zac-williamson) ensure ECCVM polynomial commitments are never points at infinity (#2214)
+        // TODO(@zac-williamson)(https://github.com/AztecProtocol/barretenberg/issues/820) ensure ECCVM polynomial
+        // commitments are never points at infinity
         if (commitment.y != 0) {
             batched_commitment_unshifted += commitment * rhos[commitment_idx];
         } else {
-            info("ECCVM Verifier: point at infinity (unshifted)");
+            // TODO(https://github.com/AztecProtocol/barretenberg/issues/820)
         }
         ++commitment_idx;
     }
@@ -210,7 +211,7 @@ template <typename Flavor> bool ECCVMVerifier_<Flavor>::verify_proof(const plonk
         if (commitment.y != 0) {
             batched_commitment_to_be_shifted += commitment * rhos[commitment_idx];
         } else {
-            info("ECCVM Verifier: point at infinity (to be shifted)");
+            // TODO(https://github.com/AztecProtocol/barretenberg/issues/820)
         }
         ++commitment_idx;
     }

barretenberg/cpp/src/barretenberg/goblin/goblin.hpp

@@ -169,14 +169,11 @@ class Goblin {
         auto instance = composer.create_instance(circuit_builder);
         auto prover = composer.create_prover(instance);
         auto ultra_proof = prover.construct_proof();
-        instance_inspector::inspect_instance(instance);
 
         // TODO(https://github.com/AztecProtocol/barretenberg/issues/811): no merge prover for now since we're not
         // mocking the first set of ecc ops
         // // Construct and store the merge proof to be recursively verified on the next call to accumulate
-        // info("create_merge_prover");
         // auto merge_prover = composer.create_merge_prover(op_queue);
-        // info("merge_prover.construct_proof()");
         // merge_proof = merge_prover.construct_proof();
 
         // if (!merge_proof_exists) {
@@ -190,7 +187,6 @@ class Goblin {
     // ACIRHACK
     Proof prove_for_acir()
     {
-        info("Goblin.prove(): op_queue size = ", op_queue->ultra_ops[0].size());
         Proof proof;
 
         proof.merge_proof = std::move(merge_proof);
@@ -216,9 +212,7 @@ class Goblin {
     {
         // ACIRHACK
         // MergeVerifier merge_verifier;
-        // info("constructed merge_verifier");
         // bool merge_verified = merge_verifier.verify_proof(proof.merge_proof);
-        // info("verified merge proof. result: ", merge_verified);
 
         auto eccvm_verifier = eccvm_composer->create_verifier(*eccvm_builder);
         bool eccvm_verified = eccvm_verifier.verify_proof(proof.eccvm_proof);
@@ -235,17 +229,21 @@ class Goblin {
     // ACIRHACK
     std::vector<uint8_t> construct_proof(GoblinUltraCircuitBuilder& builder)
     {
-        info("goblin: construct_proof");
+        // Construct a GUH proof
         accumulate_for_acir(builder);
-        info("accumulate complete.");
-        std::vector<uint8_t> goblin_proof = prove_for_acir().to_buffer();
-        std::vector<uint8_t> result(accumulator.proof.proof_data.size() + goblin_proof.size());
+
+        std::vector<uint8_t> result(accumulator.proof.proof_data.size());
 
         const auto insert = [&result](const std::vector<uint8_t>& buf) {
             result.insert(result.end(), buf.begin(), buf.end());
         };
+
         insert(accumulator.proof.proof_data);
-        insert(goblin_proof);
+
+        // TODO(https://github.com/AztecProtocol/barretenberg/issues/819): Skip ECCVM/Translator proof for now
+        // std::vector<uint8_t> goblin_proof = prove_for_acir().to_buffer();
+        // insert(goblin_proof);
+
         return result;
     }
 
@@ -256,15 +254,13 @@ class Goblin {
         const auto extract_final_kernel_proof = [&]([[maybe_unused]] auto& input_proof) { return accumulator.proof; };
 
         GoblinUltraVerifier verifier{ accumulator.verification_key };
-        info("constructed GUH verifier");
         bool verified = verifier.verify_proof(extract_final_kernel_proof(proof));
-        info("                           verified GUH proof; result: ", verified);
 
-        const auto extract_goblin_proof = [&]([[maybe_unused]] auto& input_proof) { return proof_; };
-        auto goblin_proof = extract_goblin_proof(proof);
-        info("extracted goblin proof");
-        verified = verified && verify_for_acir(goblin_proof);
-        info("verified goblin proof");
+        // TODO(https://github.com/AztecProtocol/barretenberg/issues/819): Skip ECCVM/Translator verification for now
+        // const auto extract_goblin_proof = [&]([[maybe_unused]] auto& input_proof) { return proof_; };
+        // auto goblin_proof = extract_goblin_proof(proof);
+        // verified = verified && verify_for_acir(goblin_proof);
+
         return verified;
     }
 };

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

@@ -246,6 +246,14 @@ template <typename FF> void GoblinUltraCircuitBuilder_<FF>::populate_ecc_op_wire
     num_ecc_op_gates += 2;
 };
 
+template <typename FF> void GoblinUltraCircuitBuilder_<FF>::set_goblin_ecc_op_code_constant_variables()
+{
+    null_op_idx = this->zero_idx;
+    add_accum_op_idx = this->put_constant_variable(FF(EccOpCode::ADD_ACCUM));
+    mul_accum_op_idx = this->put_constant_variable(FF(EccOpCode::MUL_ACCUM));
+    equality_op_idx = this->put_constant_variable(FF(EccOpCode::EQUALITY));
+}
+
 template <typename FF>
 void GoblinUltraCircuitBuilder_<FF>::create_poseidon2_external_gate(const poseidon2_external_gate_<FF>& in)
 {

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

@@ -1,13 +1,7 @@
 #pragma once
-#include "barretenberg/polynomials/polynomial.hpp"
 #include "barretenberg/proof_system/arithmetization/arithmetization.hpp"
 #include "barretenberg/proof_system/op_queue/ecc_op_queue.hpp"
-#include "barretenberg/proof_system/plookup_tables/plookup_tables.hpp"
-#include "barretenberg/proof_system/plookup_tables/types.hpp"
-#include "barretenberg/proof_system/types/merkle_hash_type.hpp"
-#include "barretenberg/proof_system/types/pedersen_commitment_type.hpp"
 #include "ultra_circuit_builder.hpp"
-#include <optional>
 
 namespace proof_system {
 
@@ -20,8 +14,6 @@ template <typename FF> class GoblinUltraCircuitBuilder_ : public UltraCircuitBui
     static constexpr size_t DEFAULT_NON_NATIVE_FIELD_LIMB_BITS =
         UltraCircuitBuilder_<arithmetization::UltraHonk<FF>>::DEFAULT_NON_NATIVE_FIELD_LIMB_BITS;
 
-    size_t num_vars_added_in_constructor = 0; // needed in constructing circuit from acir
-
     size_t num_ecc_op_gates = 0; // number of ecc op "gates" (rows); these are placed at the start of the circuit
 
     // Stores record of ecc operations and performs corresponding native operations internally
@@ -70,6 +62,7 @@ template <typename FF> class GoblinUltraCircuitBuilder_ : public UltraCircuitBui
   private:
     void populate_ecc_op_wires(const ecc_op_tuple& in);
     ecc_op_tuple decompose_ecc_operands(uint32_t op, const g1::affine_element& point, const FF& scalar = FF::zero());
+    void set_goblin_ecc_op_code_constant_variables();
 
   public:
     GoblinUltraCircuitBuilder_(const size_t size_hint = 0,
@@ -78,16 +71,48 @@ template <typename FF> class GoblinUltraCircuitBuilder_ : public UltraCircuitBui
         , op_queue(op_queue_in)
     {
         // Set indices to constants corresponding to Goblin ECC op codes
-        null_op_idx = this->zero_idx;
-        add_accum_op_idx = this->put_constant_variable(FF(EccOpCode::ADD_ACCUM));
-        mul_accum_op_idx = this->put_constant_variable(FF(EccOpCode::MUL_ACCUM));
-        equality_op_idx = this->put_constant_variable(FF(EccOpCode::EQUALITY));
-        num_vars_added_in_constructor = this->variables.size();
+        set_goblin_ecc_op_code_constant_variables();
     };
     GoblinUltraCircuitBuilder_(std::shared_ptr<ECCOpQueue> op_queue_in)
         : GoblinUltraCircuitBuilder_(0, op_queue_in)
     {}
 
+    /**
+     * @brief Constructor from data generated from ACIR
+     *
+     * @param op_queue_in Op queue to which goblinized group ops will be added
+     * @param witness_values witnesses values known to acir
+     * @param public_inputs indices of public inputs in witness array
+     * @param varnum number of known witness
+     *
+     * @note The size of witness_values may be less than varnum. The former is the set of actual witness values known at
+     * the time of acir generation. The former may be larger and essentially acounts for placeholders for witnesses that
+     * we know will exist but whose values are not known during acir generation. Both are in general less than the total
+     * number of variables/witnesses that might be present for a circuit generated from acir, since many gates will
+     * depend on the details of the bberg implementation (or more generally on the backend used to process acir).
+     */
+    GoblinUltraCircuitBuilder_(std::shared_ptr<ECCOpQueue> op_queue_in,
+                               auto& witness_values,
+                               std::vector<uint32_t>& public_inputs,
+                               size_t varnum)
+        : UltraCircuitBuilder_<arithmetization::UltraHonk<FF>>()
+        , op_queue(op_queue_in)
+    {
+        // 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.
+            auto value = idx < witness_values.size() ? witness_values[idx] : 0;
+            this->add_variable(value);
+        }
+
+        // Add the public_inputs from acir
+        this->public_inputs = public_inputs;
+
+        // Set indices to constants corresponding to Goblin ECC op codes
+        set_goblin_ecc_op_code_constant_variables();
+    };
+
     void finalize_circuit();
     void add_gates_to_ensure_all_polys_are_non_zero();