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

Make raw_ops and ultra_ops private fields #899

Closed
maramihali opened this issue Mar 8, 2024 · 1 comment · Fixed by AztecProtocol/aztec-packages#5648
Closed

Make raw_ops and ultra_ops private fields #899

maramihali opened this issue Mar 8, 2024 · 1 comment · Fixed by AztecProtocol/aztec-packages#5648
Assignees

Comments

@maramihali
Copy link
Contributor

No description provided.

maramihali added a commit to AztecProtocol/aztec-packages that referenced this issue Mar 8, 2024
…e ultra ops are populated through function (#5084)

Resolves AztecProtocol/barretenberg#897.

Add API functionality so we don't populate the `ultra_ops` manually in
the circuit builder and ensure ECCVM cannot update the op_queue and is
always constructed from existing `op_queue`.

Opens AztecProtocol/barretenberg#899 as
ultimately we might want to have the `ultra_ops` and `raw_ops` only
accessible and modifiable by funcitons.
AztecBot pushed a commit that referenced this issue Mar 9, 2024
…e ultra ops are populated through function (#5084)

Resolves #897.

Add API functionality so we don't populate the `ultra_ops` manually in
the circuit builder and ensure ECCVM cannot update the op_queue and is
always constructed from existing `op_queue`.

Opens #899 as
ultimately we might want to have the `ultra_ops` and `raw_ops` only
accessible and modifiable by funcitons.
@ledwards2225 ledwards2225 self-assigned this Apr 10, 2024
@ledwards2225
Copy link
Collaborator

Making these private and also improving the op_queue API in general

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
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants