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.

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

Create connection between ultra_ops and raw_ops #891

Closed
maramihali opened this issue Mar 4, 2024 · 0 comments · Fixed by AztecProtocol/aztec-packages#5648
Closed

Create connection between ultra_ops and raw_ops #891

maramihali opened this issue Mar 4, 2024 · 0 comments · Fixed by AztecProtocol/aztec-packages#5648
Assignees
Labels
bug Something isn't working

Comments

@maramihali
Copy link
Contributor

maramihali commented Mar 4, 2024

In the current implementation, we can add some random field elements in ultra_ops at the beginning (with valid commitments) and, as long as those are not reflected in the raw_ops and these remain only populated with genuine operations the goblin full proofs will not fail because these structures are updated by different functions.

@maramihali maramihali added the bug Something isn't working label Mar 4, 2024
@maramihali maramihali changed the title Equivalence check between ultra_ops and raw_ops Create connection betweenultra_ops and raw_ops Mar 8, 2024
@codygunton codygunton changed the title Create connection betweenultra_ops and raw_ops Create connection between ultra_ops and raw_ops Mar 18, 2024
@ledwards2225 ledwards2225 self-assigned this Apr 11, 2024
ledwards2225 added a commit to AztecProtocol/aztec-packages that referenced this issue Apr 11, 2024
A small refactoring of the EccOpQueue to make it safer to use. Work
includes:

Closes AztecProtocol/barretenberg#891
Closes AztecProtocol/barretenberg#899

- Updating access specifiers on members and methods to make the API
safer, e.g. no direct access to ultra_ops, raw_ops etc.
- Explicitly connecting ultra_ops and raw_ops so that both are updated
simultaneously by a small set of well defined methods. They can no
longer be updated independently.
- Addition of some `..._for_testing()` methods to replace cases where
members that are now private were being accessed directly in tests. Its
still nice to have these for failure testing etc but it is no longer
possible to create bad witnesses without them (unless the API methods
become incorrect).

Note: I've changed the method `empty_row()` to `empty_row_for_testing()`
since it has no practical use aside from testing. I'm not sure who added
this originally. It might be better to just delete it altogether but
given the recent confusion over ECCVM tests passing when they shouldn't
I figured we can take all the testing avenues we can get.

`ClientIVCBench/Full/6      22893 ms        17742 ms            1`
AztecBot pushed a commit that referenced this issue Apr 12, 2024
A small refactoring of the EccOpQueue to make it safer to use. Work
includes:

Closes #891
Closes #899

- Updating access specifiers on members and methods to make the API
safer, e.g. no direct access to ultra_ops, raw_ops etc.
- Explicitly connecting ultra_ops and raw_ops so that both are updated
simultaneously by a small set of well defined methods. They can no
longer be updated independently.
- Addition of some `..._for_testing()` methods to replace cases where
members that are now private were being accessed directly in tests. Its
still nice to have these for failure testing etc but it is no longer
possible to create bad witnesses without them (unless the API methods
become incorrect).

Note: I've changed the method `empty_row()` to `empty_row_for_testing()`
since it has no practical use aside from testing. I'm not sure who added
this originally. It might be better to just delete it altogether but
given the recent confusion over ECCVM tests passing when they shouldn't
I figured we can take all the testing avenues we can get.

`ClientIVCBench/Full/6      22893 ms        17742 ms            1`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants