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

fix: Remove proving key buf len estimation #26

Merged
merged 1 commit into from
Mar 13, 2023

Conversation

vezenovm
Copy link

@vezenovm vezenovm commented Mar 6, 2023

In https://github.com/AztecProtocol/barretenberg/blob/f2fdebe037d4d2d90761f98e28b4b0d[…]3/cpp/src/aztec/join_split_example/proofs/join_split/c_bind.cpp we estimate the size of the proving key before writing it to a buffer (as per the comments it is difficult to compute the size of the serialized key, and there was a desire to keep memory usage down). This is fine for individual circuits, however, when trying to generalize the circuits for Noir this will lead to a max proving key size. It looks like keeping this write strategy will require constructing some sort of method for estimating the proving key size if I don't want to allocate a massive buffer each time.

This PR switches to using the same write strategy for proving keys that is used for verification keys (https://github.com/AztecProtocol/barretenberg/blob/f2fdebe037d4d2d90761f98e28b4b0d[…]3/cpp/src/aztec/join_split_example/proofs/join_split/c_bind.cpp). Using this same write strategy with proving keys looks to be working for my tests. I successfully proved and verified circuits locally using varying examples of small to large circuits. This seems like the best strategy at the moment especially if the current comments in bb mention that estimating the proving key is nontrivial.

@vezenovm vezenovm added the bug Something isn't working label Mar 6, 2023
@phated
Copy link

phated commented Mar 6, 2023

@vezenovm please make sure to submit changes like these upstream (at https://github.com/AztecProtocol/barretenberg) too. We're very close to making the switch and continuing to diverge will delay it further.

@vezenovm
Copy link
Author

vezenovm commented Mar 6, 2023

@vezenovm please make sure to submit changes like these upstream (at https://github.com/AztecProtocol/barretenberg) too. We're very close to making the switch and continuing to diverge will delay it further.

Will do

@vezenovm vezenovm changed the title Remove proving key buf len estimation (fix): Remove proving key buf len estimation Mar 6, 2023
@vezenovm vezenovm changed the title (fix): Remove proving key buf len estimation fix: Remove proving key buf len estimation Mar 6, 2023
@vezenovm vezenovm requested review from guipublic and kevaundray March 7, 2023 01:28
@kevaundray kevaundray merged commit dbd5443 into kw/noir-dsl Mar 13, 2023
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 this pull request may close these issues.

3 participants