chore(dsl): Abstract nested aggregation object from ACIR

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

@@ -3,6 +3,7 @@
 #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"
+#include <cstddef>
 
 namespace acir_format {
 
@@ -158,24 +159,56 @@ void build_constraints(Builder& builder, acir_format const& constraint_system, b
         // 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
+        // TODO(maxim): Check if this is always the case. ie I won't receive a proof that will set the first
+        // TODO(maxim): input_aggregation_object to be non-zero.
+        // TODO(maxim): if not, we can add input_aggregation_object to the proof too for all recursive proofs
+        // TODO(maxim): 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
         };
 
+        // Get the size of proof with no public inputs prepended to it
+        // This is used while processing recursion constraints to determine whether
+        // the proof we are verifying contains a recursive proof itself
+        auto proof_size_no_pub_inputs = transcript_in_recursion_format_size();
+
         // Add recursion constraints
-        for (size_t i = 0; i < constraint_system.recursion_constraints.size(); ++i) {
-            auto& constraint = constraint_system.recursion_constraints[i];
+        for (auto constraint : constraint_system.recursion_constraints) {
+            // A proof passed into the constraint should be stripped of its public inputs, except in the case where a
+            // proof contains an aggregation object itself. We refer to this as the `nested_aggregation_object`. The
+            // verifier circuit requires that the indices to a nested proof aggregation state are a circuit constant.
+            // The user tells us they how they want these constants set by keeping the nested aggregation object
+            // attached to the proof as public inputs. As this is the only object that can prepended to the proof if the
+            // proof is above the expected size (with public inputs stripped)
+            std::array<uint32_t, RecursionConstraint::AGGREGATION_OBJECT_SIZE> nested_aggregation_object = {};
+            // If the proof has public inputs attached to it, we should handle setting the nested aggregation object
+            if (constraint.proof.size() > proof_size_no_pub_inputs) {
+                // The public inputs attached to a proof should match the aggregation object in size
+                ASSERT(constraint.proof.size() - proof_size_no_pub_inputs ==
+                       RecursionConstraint::AGGREGATION_OBJECT_SIZE);
+                for (size_t i = 0; i < RecursionConstraint::AGGREGATION_OBJECT_SIZE; ++i) {
+                    // Set the nested aggregation object indices to the current size of the public inputs
+                    // This way we know that the nested aggregation object indices will always be the last
+                    // indices of the public inputs
+                    nested_aggregation_object[i] = static_cast<uint32_t>(constraint.public_inputs.size());
+                    // Attach the nested aggregation object to the end of the public inputs to fill in
+                    // the slot where the nested aggregation object index will point into
+                    constraint.public_inputs.emplace_back(constraint.proof[i]);
+                }
+                // Remove the aggregation object so that they can be handled as normal public inputs
+                // in they way taht the recursion constraint expects
+                constraint.proof.erase(constraint.proof.begin(),
+                                       constraint.proof.begin() +
+                                           static_cast<std::ptrdiff_t>(RecursionConstraint::AGGREGATION_OBJECT_SIZE));
+            }
+
             current_output_aggregation_object = create_recursion_constraints(builder,
                                                                              constraint,
                                                                              current_input_aggregation_object,
-                                                                             constraint.nested_aggregation_object,
+                                                                             nested_aggregation_object,
                                                                              has_valid_witness_assignments);
             current_input_aggregation_object = current_output_aggregation_object;
         }
@@ -241,25 +274,26 @@ void create_circuit_with_witness(Builder& builder, acir_format const& constraint
 
 /**
  * @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.
+ * @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
+    // 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).
+    // 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) {

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

@@ -33,11 +33,6 @@ std::array<uint32_t, RecursionConstraint::AGGREGATION_OBJECT_SIZE> create_recurs
     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)
 {
@@ -141,6 +136,7 @@ std::array<uint32_t, RecursionConstraint::AGGREGATION_OBJECT_SIZE> create_recurs
     std::shared_ptr<verification_key_ct> vkey = verification_key_ct::from_field_elements(
         &builder, key_fields, inner_proof_contains_recursive_proof, nested_aggregation_indices);
     vkey->program_width = noir_recursive_settings::program_width;
+
     Transcript_ct transcript(&builder, manifest, proof_fields, input.public_inputs.size());
     aggregation_state_ct result = proof_system::plonk::stdlib::recursion::verify_proof_<bn254, noir_recursive_settings>(
         &builder, vkey, transcript, previous_aggregation);
@@ -358,6 +354,13 @@ std::vector<barretenberg::fr> export_dummy_transcript_in_recursion_format(const
     return fields;
 }
 
+size_t transcript_in_recursion_format_size()
+{
+    const auto manifest = Composer::create_manifest(0);
+    auto dummy_transcript = export_dummy_transcript_in_recursion_format(manifest, false);
+    return dummy_transcript.size();
+}
+
 G1AsFields export_g1_affine_element_as_fields(const barretenberg::g1::affine_element& group_element)
 {
     const uint256_t x = group_element.x;

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

@@ -53,13 +53,14 @@ 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
+    // TODO(maxim):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
+    // TODO(maxim): 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)
+    // TODO(maxim): This is currently not being used on the Noir level at all,
+    // TODO(maxim): but we keep it here for backwards compatibility
+    // TODO(maxim): The object is now currently contained by the `proof` field
+    // TODO(maxim): and is handled when serializing ACIR to a barretenberg circuit
     std::array<uint32_t, AGGREGATION_OBJECT_SIZE> nested_aggregation_object;
 
     friend bool operator==(RecursionConstraint const& lhs, RecursionConstraint const& rhs) = default;
@@ -79,6 +80,7 @@ std::vector<barretenberg::fr> export_dummy_key_in_recursion_format(const Polynom
 std::vector<barretenberg::fr> export_transcript_in_recursion_format(const transcript::StandardTranscript& transcript);
 std::vector<barretenberg::fr> export_dummy_transcript_in_recursion_format(const transcript::Manifest& manifest,
                                                                           const bool contains_recursive_proof);
+size_t transcript_in_recursion_format_size();
 
 // In order to interact with a recursive aggregation state inside of a circuit, we need to represent its internal G1
 // elements as field elements. This happens in multiple locations when creating a recursion constraint. The struct and

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

@@ -139,8 +139,8 @@ Builder create_outer_circuit(std::vector<Builder>& inner_circuits)
         auto inner_verifier = inner_composer.create_verifier(inner_circuit);
 
         const bool has_nested_proof = inner_verifier.key->contains_recursive_proof;
-        const size_t num_inner_public_inputs = inner_circuit.get_public_inputs().size();
 
+        const size_t num_inner_public_inputs = inner_circuit.get_public_inputs().size();
         transcript::StandardTranscript transcript(inner_proof.proof_data,
                                                   Composer::create_manifest(num_inner_public_inputs),
                                                   transcript::HashType::PedersenBlake3s,
@@ -149,11 +149,16 @@ Builder create_outer_circuit(std::vector<Builder>& inner_circuits)
         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));
+
+        // We want to make sure that we do not remove the nested aggregation object in the case of the proof we want to
+        // recursively verify contains a recursive proof itself. We are safe to keep all the inner public inputs
+        // as in these tests the outer circuits do not have public inputs themselves
+        if (!has_nested_proof) {
+            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);
 
@@ -187,8 +192,13 @@ Builder create_outer_circuit(std::vector<Builder>& inner_circuits)
         for (size_t i = 0; i < key_size; ++i) {
             key_indices.emplace_back(static_cast<uint32_t>(i + key_indices_start_idx));
         }
-        for (size_t i = 0; i < num_inner_public_inputs; ++i) {
-            inner_public_inputs.push_back(static_cast<uint32_t>(i + public_input_start_idx));
+        // In the case of a nested proof we keep the nested aggregation object attached to the proof,
+        // thus we do not explicitly have to keep the public inputs while setting up the initial recursion constraint.
+        // They will later be attached as public inputs when creating the circuit.
+        if (!has_nested_proof) {
+            for (size_t i = 0; i < num_inner_public_inputs; ++i) {
+                inner_public_inputs.push_back(static_cast<uint32_t>(i + public_input_start_idx));
+            }
         }
 
         RecursionConstraint recursion_constraint{
@@ -201,21 +211,30 @@ Builder create_outer_circuit(std::vector<Builder>& inner_circuits)
             .nested_aggregation_object = nested_aggregation_object,
         };
         recursion_constraints.push_back(recursion_constraint);
+
         for (size_t i = 0; i < proof_indices_start_idx - witness_offset; ++i) {
             witness.emplace_back(0);
         }
         for (const auto& wit : proof_witnesses) {
             witness.emplace_back(wit);
         }
+
         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];
+        //
+        // We once again have to check whether we have a nested proof, because if we do have one
+        // then we could get a segmentation fault as `inner_public_inputs` was never filled with values.
+        if (!has_nested_proof) {
+            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++;
     }