feat: Correct circuit construction from acir (AztecProtocol#3757)

This work attempts to add a robust means for construction of bberg
circuits from acir data. The difficulty comes primarily from different
assumptions about constant variables added in the builder constructors.

The problem is essentially that constraints are constructed from acir in
two ways: directly and indirectly. Directly means the witness values
themselves are known at the point of acir generation. In this case gates
are specified directly via indices into a "witness" array. Most
constraints are indirect in the sense that bberg is presented with an
acir opcode, a sha hash say, and has to turn that into constraints using
internal bberg mechanisms. This leads to issues when the "witness" array
(and acir object) and the "variables" array (a bberg builder object) are
offset from one another, as can happen when constant variables are added
in the bberg builder constructors. The issue is further complicated by
the fact that noir applies a PRE-offset (see
[here](noir-lang/noir#3813)) to the indices in
the directly specified gates to account for the fact that the Ultra
builder historically added a single constant in its constructor. The GUH
builder adds 3 more, which is how this issue arose in the first place.
However the issue would have come up regardless once we removed that +1
from noir which everyone agrees should not be there.

The solution was to add a new builder constructor that takes some data
from acir (witness, public_inputs, etc.), populates the analogous
structures in the builder, and then adds the necessary constant
variables. This means that the indices encoded into explicit gates from
acir correctly index into builder.variables, regardless of how many
constants are added subsequently. This solution will also work once we
remove the +1 from noir, with one additional tweak in bberg that is
clearly indicated in the code.

Closes AztecProtocol/barretenberg#815

Author: ledwards2225

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