From fee1130a6c720c40f1a1e28294540d6d9c7bdfb7 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 21 Dec 2023 23:43:39 +0000 Subject: [PATCH] chore(dsl): Abstract nested aggregation object from ACIR (#3765) The nested aggregation object in a RecursionConstraint is currently not used at the Noir level at all. We want to enable having the user specify whether the proof they want to verify is itself a recursive proof, but we also do not want to have an explicit field on the opcode determining this as this would be a barretenberg leakage into the ACVM recursion opcode. We instead move to having the `proof` field in the recursion constraint adhere to a barretenberg specific structure, where the expected proof should be stripped of its public inputs, except in the case where we have a nested proof. When setting up the barretenberg circuit from ACIR we can then determine how the `nested_aggregation_object` constant indices should be set from the size of the proof object. Until the recursive verifier can move to an implementation where the nested aggregation object does not have to be a circuit constant we need the user to specify whether they want to aggregation over a nested proof. If we move to not requiring these circuit constants we can have the proof inputs to the recursive aggregation builtin be the same for both use cases. # 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 --- .../dsl/acir_format/acir_format.cpp | 66 ++++++++++++++----- .../dsl/acir_format/recursion_constraint.cpp | 13 ++-- .../dsl/acir_format/recursion_constraint.hpp | 12 ++-- .../acir_format/recursion_constraint.test.cpp | 35 +++++++--- 4 files changed, 92 insertions(+), 34 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_format.cpp b/barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_format.cpp index 6f66e74ce9f..b36ba94d33e 100644 --- a/barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_format.cpp +++ b/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 namespace acir_format { @@ -158,10 +159,10 @@ 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 current_input_aggregation_object = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }; @@ -169,13 +170,45 @@ void build_constraints(Builder& builder, acir_format const& constraint_system, b 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 = recursion_proof_size_without_public_inputs(); + // 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 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(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(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 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(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) { diff --git a/barretenberg/cpp/src/barretenberg/dsl/acir_format/recursion_constraint.cpp b/barretenberg/cpp/src/barretenberg/dsl/acir_format/recursion_constraint.cpp index f01d4f60f66..813f2022745 100644 --- a/barretenberg/cpp/src/barretenberg/dsl/acir_format/recursion_constraint.cpp +++ b/barretenberg/cpp/src/barretenberg/dsl/acir_format/recursion_constraint.cpp @@ -33,11 +33,6 @@ std::array create_recurs Builder& builder, const RecursionConstraint& input, std::array 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 nested_aggregation_object, bool has_valid_witness_assignments) { @@ -141,6 +136,7 @@ std::array create_recurs std::shared_ptr 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_( &builder, vkey, transcript, previous_aggregation); @@ -358,6 +354,13 @@ std::vector export_dummy_transcript_in_recursion_format(const return fields; } +size_t recursion_proof_size_without_public_inputs() +{ + 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; diff --git a/barretenberg/cpp/src/barretenberg/dsl/acir_format/recursion_constraint.hpp b/barretenberg/cpp/src/barretenberg/dsl/acir_format/recursion_constraint.hpp index 345b6744cd6..8f6618106d2 100644 --- a/barretenberg/cpp/src/barretenberg/dsl/acir_format/recursion_constraint.hpp +++ b/barretenberg/cpp/src/barretenberg/dsl/acir_format/recursion_constraint.hpp @@ -53,13 +53,14 @@ struct RecursionConstraint { std::vector proof; std::vector 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 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 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 nested_aggregation_object; friend bool operator==(RecursionConstraint const& lhs, RecursionConstraint const& rhs) = default; @@ -79,6 +80,7 @@ std::vector export_dummy_key_in_recursion_format(const Polynom std::vector export_transcript_in_recursion_format(const transcript::StandardTranscript& transcript); std::vector export_dummy_transcript_in_recursion_format(const transcript::Manifest& manifest, const bool contains_recursive_proof); +size_t recursion_proof_size_without_public_inputs(); // 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 diff --git a/barretenberg/cpp/src/barretenberg/dsl/acir_format/recursion_constraint.test.cpp b/barretenberg/cpp/src/barretenberg/dsl/acir_format/recursion_constraint.test.cpp index b80d437e6d5..a64f65462bc 100644 --- a/barretenberg/cpp/src/barretenberg/dsl/acir_format/recursion_constraint.test.cpp +++ b/barretenberg/cpp/src/barretenberg/dsl/acir_format/recursion_constraint.test.cpp @@ -139,8 +139,8 @@ Builder create_outer_circuit(std::vector& 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& inner_circuits) std::vector 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 inner_public_input_values( proof_witnesses.begin(), proof_witnesses.begin() + static_cast(num_inner_public_inputs)); - proof_witnesses.erase(proof_witnesses.begin(), - proof_witnesses.begin() + static_cast(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(num_inner_public_inputs)); + } const std::vector key_witnesses = export_key_in_recursion_format(inner_verifier.key); @@ -187,8 +192,13 @@ Builder create_outer_circuit(std::vector& inner_circuits) for (size_t i = 0; i < key_size; ++i) { key_indices.emplace_back(static_cast(i + key_indices_start_idx)); } - for (size_t i = 0; i < num_inner_public_inputs; ++i) { - inner_public_inputs.push_back(static_cast(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(i + public_input_start_idx)); + } } RecursionConstraint recursion_constraint{ @@ -201,21 +211,30 @@ Builder create_outer_circuit(std::vector& 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++; }