Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Op Queue Aggregation Limitations #723

Closed
ledwards2225 opened this issue Sep 11, 2023 · 0 comments · Fixed by AztecProtocol/aztec-packages#4854
Closed

Op Queue Aggregation Limitations #723

ledwards2225 opened this issue Sep 11, 2023 · 0 comments · Fixed by AztecProtocol/aztec-packages#4854
Assignees
@maramihali

Comments

@ledwards2225
Copy link
Collaborator

ledwards2225 commented Sep 11, 2023 •
edited
Loading

The Goblin op queue transcript aggregation protocol computes the updated op queue transcript commitment by aggregating the previous aggregate op queue data with the contributions from the current circuit. There is currently no mechanism for handling the "first" circuit in a series since this naively leads to zero commitments due to the "empty" previous data. One fix would be to allow for the serialization of the zero commitment on the first iteration (along with related adjustments like handling the evaluation of an empty polynomial). Another would be to have the first recursive verifier circuit differ from subsequent ones accordingly. The current workaround is to simply add mock data to the op queue representing the first circuit in a stack.
@ledwards2225 ledwards2225 mentioned this issue Oct 7, 2023
Merged
4 tasks
ledwards2225 added a commit to AztecProtocol/aztec-packages that referenced this issue Oct 10, 2023
@ledwards2225
feat: Separate aggregation protocol (#2736)
ad16937 
This PR moves the Goblin ECC op queue transcript aggregation protocol
from the main Honk protocol to its own separate mini-protocol, referred
to as "Merge" (based on Zac's original Goblin doc). Zac pointed out that
this was likely the right approach once we go to incorporate folding.

This is also a necessary step for completing integration of ZeroMorph
(and deprecation of Gemini/Shplonk) because the univariate evaluation
claims related to this merge protocol would have otherwise needed to be
incorporated via Shplonk.

This work automatically resolves one of the issues previously described
in bberg [723](AztecProtocol/barretenberg#723)
related to the size of the transcript polynomials being tied to the size
of the present circuit.
AztecBot pushed a commit that referenced this issue Oct 11, 2023
@ledwards2225 @AztecBot
feat: Separate aggregation protocol (#2736)
b646895 
This PR moves the Goblin ECC op queue transcript aggregation protocol
from the main Honk protocol to its own separate mini-protocol, referred
to as "Merge" (based on Zac's original Goblin doc). Zac pointed out that
this was likely the right approach once we go to incorporate folding.

This is also a necessary step for completing integration of ZeroMorph
(and deprecation of Gemini/Shplonk) because the univariate evaluation
claims related to this merge protocol would have otherwise needed to be
incorporated via Shplonk.

This work automatically resolves one of the issues previously described
in bberg [723](#723)
related to the size of the transcript polynomials being tied to the size
of the present circuit.
@maramihali maramihali mentioned this issue Feb 29, 2024
Merged
maramihali added a commit to AztecProtocol/aztec-packages that referenced this issue Mar 12, 2024
@maramihali
refactor: interaction for a mock first circuit handled inside the `Ec…
d9cbdc8 
…cOpQueue` (#4854)

Closes AztecProtocol/barretenberg#723.

Because of issues with handling zero-commitments, we need to populate
the `EccOpQueue` with some mock ops from a first circuit in order for
the first merge prover to work without problems. This was done
client-side which required manually calling either
`GoblinMockCircuits::perform_op_queue_interactions_for_mock_first_circuit`
or `op_queue->populate_with_mock_initital_data` (two functions for the
same purpose) everywhere goblin and the merge protocol is used/tested.
This PR ensures only
`GoblinMockCircuits::perform_op_queue_interactions_for_mock_first_circuit`
is used across the codebase since it's the one updating both `ultra_ops`
and `raw_ops`, removing the other function, and isolates mocking inside
the Goblin class so it doesn't have to be done in benchmarks.

Note: tried to isolate it inside EccOpQueue, which is possible but then
requires us to have an op_offset (from which non-mocked operations
start) which complicates some code and tests unnecessarily.
AztecBot pushed a commit that referenced this issue Mar 13, 2024
@maramihali @AztecBot
refactor: interaction for a mock first circuit handled inside the `Ec…
3fb89c5 
…cOpQueue` (#4854)

Closes #723.

Because of issues with handling zero-commitments, we need to populate
the `EccOpQueue` with some mock ops from a first circuit in order for
the first merge prover to work without problems. This was done
client-side which required manually calling either
`GoblinMockCircuits::perform_op_queue_interactions_for_mock_first_circuit`
or `op_queue->populate_with_mock_initital_data` (two functions for the
same purpose) everywhere goblin and the merge protocol is used/tested.
This PR ensures only
`GoblinMockCircuits::perform_op_queue_interactions_for_mock_first_circuit`
is used across the codebase since it's the one updating both `ultra_ops`
and `raw_ops`, removing the other function, and isolates mocking inside
the Goblin class so it doesn't have to be done in benchmarks.

Note: tried to isolate it inside EccOpQueue, which is possible but then
requires us to have an op_offset (from which non-mocked operations
start) which complicates some code and tests unnecessarily.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@maramihali maramihali

Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@maramihali @ledwards2225