-
Notifications
You must be signed in to change notification settings - Fork 103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: DSL recursion changes #383
Closed
Closed
Changes from all commits
Commits
Show all changes
48 commits
Select commit
Hold shift + click to select a range
69eba2d
Added recursion constraint into dsl + tests
zac-williamson 7a80056
fix dsl ecdsa constraint. Added ecdsa dsl tests
zac-williamson 2436dcf
changed dsl recursion to pass proof/key as witnesses
zac-williamson 292f626
recursive verification key no longer encapsulates a native key
zac-williamson bc02eff
added serialization test for acir_proofs
zac-williamson d9656e9
Added serialization methods into dsl for recursive proof composition
zac-williamson 209f813
added verificaiton key hash into RecursionConstraint
zac-williamson 6496228
fixed compiler errors in stdlib_recursion_tests
zac-williamson 5f21e22
feat(dsl)!: Noir recursion updates (#379)
vezenovm f4253ad
master merge conflicts
vezenovm 25f5574
EnvReferenceString undefined reference to error workaround
vezenovm 1c8cc77
add missing recursion_constraint field from acir_format tests
vezenovm 6109828
add recursion_constraints field back in acir_proofs.test inner circuit
vezenovm 8358741
fix inner circuit in RecursionConstraint.TestRecursionConstraint
vezenovm b65c442
merge conflictss w/ master
vezenovm 551277b
Empty-Commit
vezenovm ce46c01
add verify_recursive_proof method for simulating recursive verificati…
vezenovm 87d7b40
master merge conflicts in acir_format and updates to generate method …
vezenovm 11e97be
add ecc module to env package to fix linking of ennv_load_prover_crs …
vezenovm 941b275
add back reference strinng to stdlib recursion key to pass sol verifi…
vezenovm 6d86e39
remove prints from recursive verifier test
vezenovm 5a458d6
fix dirty free for when serializing vk to fields, was working on macb…
vezenovm 7399ea7
Merge branch 'master' into mv/noir-recursion
vezenovm 07a22f6
fix ecdsa tests after master merge
vezenovm 74a8327
missing keccak constraints fields in acir format and proofs tests
vezenovm 18cfb94
one more missing keccak constraint
vezenovm 7de20cd
merge conflicts w/ master
vezenovm 155903e
mismatched acir format structs for gcc build
vezenovm 683a875
missing block constraints in acir tests
vezenovm 7ecb574
feat(dsl)!: Arbitrary depth recursion (#433)
vezenovm a2f6cbb
move export key into dsl package
vezenovm b56d3ca
chore: remove unused export key in recursion format from main proof s…
vezenovm 97ede2e
pr review: moved from_witness comment and renamed from_field_pt_vector
vezenovm c64d8b5
moved export transcript in recursion format to DSL package
vezenovm a590470
use false for clang-format SortIncludes
vezenovm 4d262e3
Merge branch 'master' into mv/noir-recursion
vezenovm f3baab0
move order of acir functions
vezenovm a793b2d
remove ecc bb_module declaration in env package
vezenovm f954266
chore: remove usage of magic numbers when slicing g1::affine_element …
vezenovm 0f0d519
introduce NUM_AGGREGATION_ELEMENTS constant and use it through recurs…
vezenovm 294c1fa
nit ASSERT(result != -1) in get_public_input_index
vezenovm 517ed2c
remove unused found var
vezenovm 92159e0
cast -1
vezenovm d8ee388
fix up verify_recursive_proof and add a test for it
vezenovm cf9bdd6
moved from tempalte for has_valid_witness_assignments to a flag
vezenovm 2096676
chore: add comments to AcirProofs.TestVerifyRecursiveProofPass
vezenovm bb4404f
Merge branch 'master' into mv/noir-recursion
vezenovm 5f551ea
minor formatting changes to remove files from PR
zac-williamson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 assignproof_output_witness_indices
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theoutput_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 usingassign_object_to_proof_outputs
even if we can get rid ofset_recursive_proof
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that is a good point. One alternative is to pass in a
bool add_to_public_inputs
parameter intocreate_recursion_constraint
. Ifadd_to_public_inputs == true
then we callassign_object_to_proof_outputs
onaggregation_state_ct result
(line 116)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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