chore: Cleanup recursion interface (#3744)

This is a recreation of this PR
(#3528) to handle PR
#3729

# 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).

---------

Co-authored-by: kevaundray <kevtheappdev@gmail.com>

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

@@ -1,6 +1,7 @@
 #include "acir_format.hpp"
 #include "barretenberg/common/log.hpp"
 #include "barretenberg/dsl/acir_format/pedersen.hpp"
+#include "barretenberg/dsl/acir_format/recursion_constraint.hpp"
 #include "barretenberg/proof_system/circuit_builder/ultra_circuit_builder.hpp"
 
 namespace acir_format {
@@ -149,23 +150,52 @@ void build_constraints(Builder& builder, acir_format const& constraint_system, b
         create_block_constraints(builder, constraint, has_valid_witness_assignments);
     }
 
-    // Add recursion constraints
     // 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!");
     } else {
+        // These are set and modified whenever we encounter a recursion opcode
+        //
+        // These should not be set by the caller
+        // TODO: Check if this is always the case. ie I won't receive a proof that will set the first
+        // TODO input_aggregation_object to be non-zero.
+        // TODO: if not, we can add input_aggregation_object to the proof too for all recursive proofs
+        // TODO: This might be the case for proof trees where the proofs are created on different machines
+        std::array<uint32_t, RecursionConstraint::AGGREGATION_OBJECT_SIZE> current_input_aggregation_object = {
+            0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
+        };
+        std::array<uint32_t, RecursionConstraint::AGGREGATION_OBJECT_SIZE> current_output_aggregation_object = {
+            0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
+        };
+
+        // Add recursion constraints
         for (size_t i = 0; i < constraint_system.recursion_constraints.size(); ++i) {
             auto& constraint = constraint_system.recursion_constraints[i];
-            create_recursion_constraints(builder, constraint, has_valid_witness_assignments);
-
-            // make sure the verification key records the public input indices of the final recursion output (N.B. up to
-            // the ACIR description to make sure that the final output aggregation object wires are public inputs!)
-            if (i == constraint_system.recursion_constraints.size() - 1) {
-                std::vector<uint32_t> proof_output_witness_indices(constraint.output_aggregation_object.begin(),
-                                                                   constraint.output_aggregation_object.end());
-                builder.set_recursive_proof(proof_output_witness_indices);
+            current_output_aggregation_object = create_recursion_constraints(builder,
+                                                                             constraint,
+                                                                             current_input_aggregation_object,
+                                                                             constraint.nested_aggregation_object,
+                                                                             has_valid_witness_assignments);
+            current_input_aggregation_object = current_output_aggregation_object;
+        }
+
+        // Now that the circuit has been completely built, we add the output aggregation as public
+        // inputs.
+        if (!constraint_system.recursion_constraints.empty()) {
+
+            // First add the output aggregation object as public inputs
+            // Set the indices as public inputs because they are no longer being
+            // created in ACIR
+            for (const auto& idx : current_output_aggregation_object) {
+                builder.set_public_input(idx);
             }
+
+            // Make sure the verification key records the public input indices of the
+            // final recursion output.
+            std::vector<uint32_t> proof_output_witness_indices(current_output_aggregation_object.begin(),
+                                                               current_output_aggregation_object.end());
+            builder.set_recursive_proof(proof_output_witness_indices);
         }
     }
 }

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

@@ -3,6 +3,7 @@
 #include "barretenberg/plonk/transcript/transcript_wrappers.hpp"
 #include "barretenberg/stdlib/recursion/aggregation_state/aggregation_state.hpp"
 #include "barretenberg/stdlib/recursion/verifier/verifier.hpp"
+#include <cstddef>
 
 namespace acir_format {
 
@@ -28,12 +29,19 @@ void generate_dummy_proof() {}
  *       We would either need a separate ACIR opcode where inner_proof_contains_recursive_proof = true,
  *       or we need non-witness data to be provided as metadata in the ACIR opcode
  */
-template <typename Builder>
-void create_recursion_constraints(Builder& builder,
-                                  const RecursionConstraint& input,
-                                  bool has_valid_witness_assignments)
+std::array<uint32_t, RecursionConstraint::AGGREGATION_OBJECT_SIZE> create_recursion_constraints(
+    Builder& builder,
+    const RecursionConstraint& input,
+    std::array<uint32_t, RecursionConstraint::AGGREGATION_OBJECT_SIZE> input_aggregation_object,
+    // TODO: does this need to be a part of the recursion opcode?
+    // TODO: or can we figure it out from the vk?
+    // TODO: either way we could probably have the user explicitly provide it
+    // TODO: in Noir.
+    // Note: this is not being used in Noir at the moment
+    std::array<uint32_t, RecursionConstraint::AGGREGATION_OBJECT_SIZE> nested_aggregation_object,
+    bool has_valid_witness_assignments)
 {
-    const auto& nested_aggregation_indices = input.nested_aggregation_object;
+    const auto& nested_aggregation_indices = nested_aggregation_object;
     bool nested_aggregation_indices_all_zero = true;
     for (const auto& idx : nested_aggregation_indices) {
         nested_aggregation_indices_all_zero &= (idx == 0);
@@ -47,8 +55,12 @@ void create_recursion_constraints(Builder& builder,
         const std::vector<fr> dummy_key = export_dummy_key_in_recursion_format(
             PolynomialManifest(Builder::CIRCUIT_TYPE), inner_proof_contains_recursive_proof);
         const auto manifest = Composer::create_manifest(input.public_inputs.size());
-        const std::vector<barretenberg::fr> dummy_proof =
+        std::vector<barretenberg::fr> dummy_proof =
             export_dummy_transcript_in_recursion_format(manifest, inner_proof_contains_recursive_proof);
+
+        // Remove the public inputs from the dummy proof
+        dummy_proof.erase(dummy_proof.begin(),
+                          dummy_proof.begin() + static_cast<std::ptrdiff_t>(input.public_inputs.size()));
         for (size_t i = 0; i < input.proof.size(); ++i) {
             const auto proof_field_idx = input.proof[i];
             // if we do NOT have a witness assignment (i.e. are just building the proving/verification keys),
@@ -74,7 +86,7 @@ void create_recursion_constraints(Builder& builder,
     // Construct an in-circuit representation of the verification key.
     // For now, the v-key is a circuit constant and is fixed for the circuit.
     // (We may need a separate recursion opcode for this to vary, or add more config witnesses to this opcode)
-    const auto& aggregation_input = input.input_aggregation_object;
+    const auto& aggregation_input = input_aggregation_object;
     aggregation_state_ct previous_aggregation;
 
     // If we have previously recursively verified proofs, `is_aggregation_object_nonzero = true`
@@ -113,7 +125,13 @@ void create_recursion_constraints(Builder& builder,
     }
 
     std::vector<field_ct> proof_fields;
-    proof_fields.reserve(input.proof.size());
+    // Prepend the public inputs to the proof fields because this is how the
+    // core barretenberg library processes proofs (with the public inputs first and not separated)
+    proof_fields.reserve(input.proof.size() + input.public_inputs.size());
+    for (const auto& idx : input.public_inputs) {
+        auto field = field_ct::from_witness_index(&builder, idx);
+        proof_fields.emplace_back(field);
+    }
     for (const auto& idx : input.proof) {
         auto field = field_ct::from_witness_index(&builder, idx);
         proof_fields.emplace_back(field);
@@ -137,12 +155,14 @@ void create_recursion_constraints(Builder& builder,
         result.public_inputs[i].assert_equal(field_ct::from_witness_index(&builder, input.public_inputs[i]));
     }
 
-    // Assign the recursive proof outputs to `output_aggregation_object`
-    for (size_t i = 0; i < result.proof_witness_indices.size(); ++i) {
-        const auto lhs = field_ct::from_witness_index(&builder, result.proof_witness_indices[i]);
-        const auto rhs = field_ct::from_witness_index(&builder, input.output_aggregation_object[i]);
-        lhs.assert_equal(rhs);
-    }
+    // We want to return an array, so just copy the vector into the array
+    ASSERT(result.proof_witness_indices.size() == RecursionConstraint::AGGREGATION_OBJECT_SIZE);
+    std::array<uint32_t, RecursionConstraint::AGGREGATION_OBJECT_SIZE> resulting_output_aggregation_object;
+    std::copy(result.proof_witness_indices.begin(),
+              result.proof_witness_indices.begin() + RecursionConstraint::AGGREGATION_OBJECT_SIZE,
+              resulting_output_aggregation_object.begin());
+
+    return resulting_output_aggregation_object;
 }
 
 /**
@@ -350,8 +370,4 @@ G1AsFields export_g1_affine_element_as_fields(const barretenberg::g1::affine_ele
     return G1AsFields{ x_lo, x_hi, y_lo, y_hi };
 }
 
-template void create_recursion_constraints<UltraCircuitBuilder>(UltraCircuitBuilder& builder,
-                                                                const RecursionConstraint& input,
-                                                                bool has_valid_witness_assignments);
-
 } // namespace acir_format

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

@@ -53,17 +53,24 @@ struct RecursionConstraint {
     std::vector<uint32_t> proof;
     std::vector<uint32_t> public_inputs;
     uint32_t key_hash;
+    // TODO:This is now unused, but we keep it here for backwards compatibility
     std::array<uint32_t, AGGREGATION_OBJECT_SIZE> input_aggregation_object;
+    // TODO: This is now unused, but we keep it here for backwards compatibility
     std::array<uint32_t, AGGREGATION_OBJECT_SIZE> output_aggregation_object;
+    // TODO: This is currently not being used on the Noir level at all
+    // TODO: we don't have a way to specify that the proof we are creating contains a
+    // TODO: aggregation object (ie it is also verifying a proof)
     std::array<uint32_t, AGGREGATION_OBJECT_SIZE> nested_aggregation_object;
 
     friend bool operator==(RecursionConstraint const& lhs, RecursionConstraint const& rhs) = default;
 };
 
-template <typename Builder>
-void create_recursion_constraints(Builder& builder,
-                                  const RecursionConstraint& input,
-                                  bool has_valid_witness_assignments = false);
+std::array<uint32_t, RecursionConstraint::AGGREGATION_OBJECT_SIZE> create_recursion_constraints(
+    Builder& builder,
+    const RecursionConstraint& input,
+    std::array<uint32_t, RecursionConstraint::AGGREGATION_OBJECT_SIZE> input_aggregation_object,
+    std::array<uint32_t, RecursionConstraint::AGGREGATION_OBJECT_SIZE> nested_aggregation_object,
+    bool has_valid_witness_assignments = false);
 
 std::vector<barretenberg::fr> export_key_in_recursion_format(std::shared_ptr<verification_key> const& vkey);
 std::vector<barretenberg::fr> export_dummy_key_in_recursion_format(const PolynomialManifest& polynomial_manifest,

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

@@ -146,7 +146,15 @@ Builder create_outer_circuit(std::vector<Builder>& inner_circuits)
                                                   transcript::HashType::PedersenBlake3s,
                                                   16);
 
-        const std::vector<barretenberg::fr> proof_witnesses = export_transcript_in_recursion_format(transcript);
+        std::vector<barretenberg::fr> proof_witnesses = export_transcript_in_recursion_format(transcript);
+        // - Save the public inputs so that we can set their values.
+        // - Then truncate them from the proof because the ACIR API expects proofs without public inputs
+
+        std::vector<barretenberg::fr> inner_public_input_values(
+            proof_witnesses.begin(), proof_witnesses.begin() + static_cast<std::ptrdiff_t>(num_inner_public_inputs));
+        proof_witnesses.erase(proof_witnesses.begin(),
+                              proof_witnesses.begin() + static_cast<std::ptrdiff_t>(num_inner_public_inputs));
+
         const std::vector<barretenberg::fr> key_witnesses = export_key_in_recursion_format(inner_verifier.key);
 
         const uint32_t key_hash_start_idx = static_cast<uint32_t>(witness_offset);
@@ -202,14 +210,18 @@ Builder create_outer_circuit(std::vector<Builder>& inner_circuits)
         for (const auto& wit : key_witnesses) {
             witness.emplace_back(wit);
         }
+        // Set the values for the inner public inputs
+        // Note: this is confusing, but we minus one here due to the fact that the
+        // witness values have not taken into account that zero is taken up by the zero_idx
+        for (size_t i = 0; i < num_inner_public_inputs; ++i) {
+            witness[inner_public_inputs[i] - 1] = inner_public_input_values[i];
+        }
         witness_offset = key_indices_start_idx + key_witnesses.size();
         circuit_idx++;
     }
 
-    std::vector<uint32_t> public_inputs(output_aggregation_object.begin(), output_aggregation_object.end());
-
     acir_format constraint_system{ .varnum = static_cast<uint32_t>(witness.size() + 1),
-                                   .public_inputs = public_inputs,
+                                   .public_inputs = {},
                                    .logic_constraints = {},
                                    .range_constraints = {},
                                    .sha256_constraints = {},

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

@@ -130,6 +130,16 @@ bool AcirComposer::verify_proof(std::vector<uint8_t> const& proof, bool is_recur
     // Hack. Shouldn't need to do this. 2144 is size with no public inputs.
     builder_.public_inputs.resize((proof.size() - 2144) / 32);
 
+    // TODO: We could get rid of this, if we made the Noir program specify whether something should be
+    // TODO: created with the recursive setting or not. ie:
+    //
+    // #[recursive_friendly]
+    // fn main() {}
+    // would put in the ACIR that we want this to be recursion friendly with a flag maybe and the backend
+    // would set the is_recursive flag to be true.
+    // This would eliminate the need for nargo to have a --recursive flag
+    //
+    // End result is that we may just be able to get it off of builder_, like builder_.is_recursive_friendly
     if (is_recursive) {
         auto verifier = composer.create_verifier(builder_);
         return verifier.verify_proof({ proof });
@@ -152,6 +162,8 @@ std::string AcirComposer::get_solidity_verifier()
 }
 
 /**
+ * TODO: We should change this to return a proof without public inputs, since that is what std::verify_proof
+ * TODO: takes.
  * @brief Takes in a proof buffer and converts into a vector of field elements.
  *        The Recursion opcode requires the proof serialized as a vector of witnesses.
  *        Use this method to get the witness values!

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

@@ -347,11 +347,15 @@ template <typename FF_> class CircuitBuilderBase {
 
         for (const auto& idx : proof_output_witness_indices) {
             set_public_input(idx);
+            // Why is it adding the size of the public input instead of the idx?
             recursive_proof_public_input_indices.push_back((uint32_t)(public_inputs.size() - 1));
         }
     }
 
     /**
+     * TODO: We can remove this and use `add_recursive_proof` once my question has been addressed
+     * TODO: using `add_recursive_proof` also means that we will need to remove the cde which is
+     * TODO: adding the public_inputs
      * @brief Update recursive_proof_public_input_indices with existing public inputs that represent a recursive proof
      *
      * @param proof_output_witness_indices