feat: DSL recursion changes #383
Conversation
recursive verification key no longer encapsulates a reference string (neither were fundamentally needed!)
Added passing serialization tests Fixed format of RecursionConstraint to be compatible with existing ACIR formatting
Can be used by backend to force recursive verification key to be a specific value (it is represented via witnesses and therefore is not by-default constraint to represent a specific circuit)
* merge master in zw/noir-recursion * add dsl files to commit * namespace stuff and debugging * c bind functions * constraint test and comment removal * revert some changes to RecursionConstraint while debugging serialization issues * dispaly error when trying to use create_circuit_with_witness * is_recursive flag as part of new_proof and verify_proof, new RecursiveProver type in dsl, and serialization changes for recursion acir_proofs methods * remove debug output from TestSerializationWithRecursion
…and env_load_verifier_crs
…er generation tests
…ook so not caught earlier, but need more clarity on ubuntu
There was a problem hiding this comment.
OK, I've reviewed everything not in the directories dsl or solidity_helpers. Most of what I have to say are little comments for clarifiation / disucssion and thoughts for improvements that could be pursued later.
I think I noticed one constraint that goes away, and it would be good to fix this by adding the appropriate constraint in the from_witness verification key constructor. It's debatable whether that's actually a good idea -- I think it's probably ideal to keep as much of the recursive verifier logic as possible in one place (for security analysis we don't want branching in the space of circuits we create--hard to keep track of all of the possibilities). But, on that note,
UltraPlonK must not be considered secure we audit it
and our goal is to get something performant into developers' hands now, so it's certainly reasonable to move things around as you have and address the missing constraint in some way.
cpp/src/barretenberg/plonk/proof_system/verification_key/verification_key.cpp
Outdated
Show resolved
Hide resolved
cpp/src/barretenberg/stdlib/recursion/verification_key/verification_key.hpp
Outdated
Show resolved
Hide resolved
cpp/src/barretenberg/stdlib/recursion/verification_key/verification_key.hpp
Outdated
Show resolved
Hide resolved
cpp/src/barretenberg/stdlib/recursion/verification_key/verification_key.hpp
Show resolved
Hide resolved
cpp/src/barretenberg/stdlib/recursion/verification_key/verification_key.hpp
Show resolved
Hide resolved
cpp/src/barretenberg/stdlib/recursion/verification_key/verification_key.hpp
Show resolved
Hide resolved
| @@ -325,18 +343,14 @@ aggregation_state<Curve> verify_proof(typename Curve::Composer* context, | |||
| rhs_scalars.push_back(random_separator); | |||
| } | |||
|
|
|||
| // Check if recursive proof information is correctly set. | |||
| key->contains_recursive_proof.assert_equal(key->base_key->contains_recursive_proof, | |||
There was a problem hiding this comment.
from_field_pt_vector, but if we use the from_witness of a verification constructor of a verification key we will be missing this constraint!
There was a problem hiding this comment.
@zac-williamson This is the constraint that was removed
There was a problem hiding this comment.
In the new from_field_pt_vector, key->contains_recursive_proof is a circuit constant, it's not a witness (it's value is constructed out of a regular bool type)
The constraint is removed because there is no longer a contains_recursive_proof witness value at all - we effectively generate 2 different circuits depending on whether the input into from_field_pt_vector describes a recursive circuit or not.
| // (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(), |
There was a problem hiding this comment.
I added a method into aggregation_state that unifies these two steps (populating witness indices and calling set_recursive_proof): void aggregation_state::assign_object_to_proof_outputs
Would you mind updating to use it? I think it would allows us to remove the set_recursive_proof method and remove the complexity of having to assign proof_output_witness_indices
There was a problem hiding this comment.
It does look like we may still have to assign proof_output_witness_indices as I have to construct an aggregation_state in order to use assign_object_to_proof_outputs. It also looks like constructing the aggregation state will require fetching all the values of the output_aggregation_state from the composer to construct P0 and P1. I can still of course push an update to show how it would look but it does seem that there is some extra complexity to using assign_object_to_proof_outputs even if we can get rid of set_recursive_proof
There was a problem hiding this comment.
Oh that is a good point. One alternative is to pass in a bool add_to_public_inputs parameter into create_recursion_constraint. If add_to_public_inputs == true then we call assign_object_to_proof_outputs on aggregation_state_ct result (line 116)
There was a problem hiding this comment.
Unfortunately that alternative looks to also complicate how we currently construct our constraints. Calling result.assign_object_proof_outputs would mean we are now including the output aggregation object on the result aggregation state directly rather than it being a separate field on our RecursionConstraint. This change would require some larger changes in how we construct our recursion constraints and should be left as a separate optimization.
There was a problem hiding this comment.
sounds good. Might be a good idea to create a GH Issue to refactor the AggregationState interface to more easily handle this case + regular recursion w/o exposing internals like proof_output_witness_indices
| const auto element = barretenberg::g1::affine_element(barretenberg::g1::one * scalar); | ||
| const uint256_t x = element.x; | ||
| const uint256_t y = element.y; | ||
| const barretenberg::fr x_lo = x.slice(0, 136); |
There was a problem hiding this comment.
Could we replace these magic numbers with derived values? These come from proof_system::plonk::NUM_LIMB_BITS_IN_FIELD_SIMULATION (68)
There was a problem hiding this comment.
Made a new function export_g1_affine_element_as_fields that uses constexprs to determine the limb slices
| } | ||
| } | ||
|
|
||
| template void create_recursion_constraints<false>(Composer&, const RecursionConstraint&); |
There was a problem hiding this comment.
This differs from how it is done for ecdsa_constraints -- we should add dummy_constraints in one way
There was a problem hiding this comment.
I'll switch from template to a flag
| std::vector<acir_format::field_ct> proof_fields(proof_length / 32); | ||
| std::vector<acir_format::field_ct> key_fields(vk_length / 32); | ||
| for (size_t i = 0; i < proof_length / 32; i++) { | ||
| // TODO(maxim): The stdlib pairing primitives fetch the context from the elements being used in the pairing |
There was a problem hiding this comment.
Issue for this TODO can be found here: #447
|
Superseded by #485. |
Description
This PR enables the starting skeleton for recursion in Noir.
There were heavy merge conflicts between
zw/noir-recursionand master. I originally made this branchmv/recursion-master-mergein this PR: #381, but it included all the changes from master in the diff. This branches checks outzw/noir-recursionand brought it up-to-date with master. The diff should now represent only the recursion changes.There was a change in the recursion work that removed the reference string from the std lib verification key which required usage of the env crs. All the stdlib recursion tests now use the env crs. However, in the solidity tests using the env crs resulted in an error that we were trying to invert zero in the field. As a quick workaround I added back the reference string field to the stdlib verification key.
Checklist:
/markdown/specshave been updated.@briefdescribing the intended functionality.