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

refactor: Protogalaxy verifier matches prover 2 #8477

Merged
merged 16 commits into from
Sep 13, 2024
Merged

refactor: Protogalaxy verifier matches prover 2 #8477

merged 16 commits into from
Sep 13, 2024

Conversation

codygunton
Copy link
Contributor

@codygunton codygunton commented Sep 10, 2024 •
edited
Loading

This PR reorganizes the native folding verifier to better match the folding prover. This amounts to encapsulating some logic in functions and a mild reshuffling. It's tempting to also implement the verifier as a series of pure-ish round functions, but I don't do that for lack of time.

@codygunton codygunton self-assigned this Sep 10, 2024
@codygunton codygunton changed the title Cg/pg verifier like prover 2 refactor: Protogalaxy verifier matches prover 2 Sep 10, 2024
Base automatically changed from cg/pg-verifier-like-prover to master September 11, 2024 16:21
@codygunton codygunton force-pushed the cg/pg-verifier-like-prover-2 branch from dff43df to e480a58 Compare September 11, 2024 16:29
@codygunton codygunton force-pushed the cg/pg-verifier-like-prover-2 branch from 9638d9f to 49bc83c Compare September 12, 2024 03:32
@codygunton codygunton marked this pull request as ready for review September 12, 2024 03:32
codygunton
codygunton commented Sep 12, 2024
View reviewed changes
barretenberg/cpp/src/barretenberg/commitment_schemes/shplonk/shplemini_verifier.hpp
@@ -3,7 +3,6 @@
#include "barretenberg/commitment_schemes/commitment_key.hpp"
#include "barretenberg/commitment_schemes/gemini/gemini.hpp"
#include "barretenberg/commitment_schemes/shplonk/shplonk.hpp"
#include "barretenberg/commitment_schemes/utils/batch_mul_native.hpp"
Copy link
Contributor Author

@codygunton codygunton Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cost of including headers in headers is much higher than the cost of including headers in cpp files. He header should be a description of interfaces (this sometimes breaks down/we get lazy around templates) and the dependency structure of those should be exactly what is expressed by the inclusion of headers in headers. We don't achieve this, but we should still not include things in headers when not needed as a best practice (esp when the header to be included contains template definitions!).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining this and for the clean-up, will be more careful with the inclusions

barretenberg/cpp/src/barretenberg/commitment_schemes/utils/batch_mul_native.hpp
@@ -4,40 +4,48 @@
#include <vector>

namespace bb {
template <typename Curve> class CommitmentSchemesUtils {
/**
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably this should be relocated somewhere but I need to move on.

barretenberg/cpp/src/barretenberg/protogalaxy/protogalaxy_verifier.cpp
@@ -35,123 +36,106 @@ void ProtogalaxyVerifier_<DeciderVerificationKeys>::run_oink_verifier_on_each_in
}
}

template <typename FF, size_t NUM>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just computed inline; I make it a function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, this computation could re-use some methods from polynomial_arithmetic/univariate/polynomial classes. Not necessary in terms of efficiency, but the code would look cleaner. Could do that in my refactoring work.

barretenberg/cpp/src/barretenberg/commitment_schemes/utils/batch_mul_native.hpp
* @note This is used only for native verification and is not optimized for efficiency
*/
template <typename Commitment, typename FF>
static Commitment batch_mul_native(const std::vector<Commitment>& _points, const std::vector<FF>& _scalars)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make utility function in a class be simply a function template (static class members useful for reducing verbosity when we have complicated types, e.g. around flavors).

barretenberg/cpp/src/barretenberg/protogalaxy/protogalaxy_verifier.cpp
std::vector<FF> perturbator_coeffs(log_circuit_size + 1, 0);
if (accumulator->is_accumulator) {
for (size_t idx = 1; idx <= log_circuit_size; idx++) {
perturbator_coeffs[idx] =
transcript->template receive_from_prover<FF>("perturbator_" + std::to_string(idx));
}
}
const FF perturbator_challenge = transcript->template get_challenge<FF>("perturbator_challenge");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shuffled things a bit to match prover and paper order better.

barretenberg/cpp/src/barretenberg/protogalaxy/protogalaxy_verifier.cpp
vk_idx++;
}
commitment_idx++;
// // Fold the commitments
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor folding to be more succinct, putting grouping logic in the DeciderVerificationKeys object. Ultimately I think this class should go away (you can shove state it in and it's hard to track state in Pg) and these should just be functions, but for now I keep the pattern and put the functions here.

...erg/cpp/src/barretenberg/stdlib/protogalaxy_verifier/protogalaxy_recursive_verifier.test.cpp
@@ -14,6 +14,8 @@
#include "barretenberg/ultra_honk/ultra_prover.hpp"
#include "barretenberg/ultra_honk/ultra_verifier.hpp"

auto& engine = bb::numeric::get_debug_randomness();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to debug an issue with the relation parameters, and using this deterministic randomness made sure that the same parameters were used from run to run in my test.

@iakovenkos iakovenkos self-requested a review September 13, 2024 09:12
iakovenkos
iakovenkos reviewed Sep 13, 2024
View reviewed changes
barretenberg/cpp/src/barretenberg/protogalaxy/protogalaxy_verifier.cpp
}
for (auto [combination, to_combine] :
zip_view(next_accumulator->witness_commitments.get_all(), keys_to_fold.get_witness_commitments())) {
combination = batch_mul_native(to_combine, lagranges);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels more readable!

iakovenkos
iakovenkos reviewed Sep 13, 2024
View reviewed changes
Copy link
Contributor

@iakovenkos iakovenkos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall and improves the clarity of the Protogalaxy verifier code. I believe compute_vanishing_polynomial_and_lagrange_evaluations should be replaced by standard methods or moved to polynomial arithmetic module, cause it takes too much space while being not so PG-specific. Could do this in my re-factoring PR.

@iakovenkos iakovenkos self-requested a review September 13, 2024 10:24
@codygunton codygunton merged commit 58882b1 into master Sep 13, 2024
37 checks passed
@codygunton codygunton deleted the cg/pg-verifier-like-prover-2 branch September 13, 2024 12:58
@AztecBot AztecBot mentioned this pull request Sep 13, 2024
Merged
spypsy pushed a commit that referenced this pull request Sep 17, 2024
@AztecBot
chore(master): Release 0.55.1 (#8533)
25d75ff 
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-package: 0.55.1</summary>

##
[0.55.1](aztec-package-v0.55.0...aztec-package-v0.55.1)
(2024-09-17)


### Features

* CI deploy on sepolia
([#8514](#8514))
([54f0344](54f0344))
* Tx verification & peer scoring on p2p layer. bot support for
EasyPrivateToken
([#8298](#8298))
([beb651f](beb651f))


### Miscellaneous

* Remove ARCHIVER_L1_START_BLOCK
([#8554](#8554))
([bc8d461](bc8d461))
</details>

<details><summary>barretenberg.js: 0.55.1</summary>

##
[0.55.1](barretenberg.js-v0.55.0...barretenberg.js-v0.55.1)
(2024-09-17)


### Miscellaneous

* **barretenberg.js:** Synchronize aztec-packages versions
</details>

<details><summary>aztec-packages: 0.55.1</summary>

##
[0.55.1](aztec-packages-v0.55.0...aztec-packages-v0.55.1)
(2024-09-17)


### Features

* `TXE::store_note_in_cache` --&gt; `TXE::add_note`
([#8547](#8547))
([5a6aaeb](5a6aaeb))
* Add a `comptime` string type for string handling at compile-time
(noir-lang/noir#6026)
([cd7983a](cd7983a))
* CI deploy on sepolia
([#8514](#8514))
([54f0344](54f0344))
* Default to outputting witness with file named after package
(noir-lang/noir#6031)
([cd7983a](cd7983a))
* Let LSP suggest trait impl methods as you are typing them
(noir-lang/noir#6029)
([cd7983a](cd7983a))
* NFT with "transient" storage shield flow
([#8129](#8129))
([578f67c](578f67c))
* Optimize allocating immediate amounts of memory
([#8579](#8579))
([e0185e7](e0185e7))
* Spartan iac
([#8455](#8455))
([16fba46](16fba46))
* Sync from aztec-packages (noir-lang/noir#6028)
([cd7983a](cd7983a))
* Tx verification & peer scoring on p2p layer. bot support for
EasyPrivateToken
([#8298](#8298))
([beb651f](beb651f))
* Unconstraining keys in unconstrained encryption
([#7912](#7912))
([eb9275a](eb9275a))
* Update args hash to be a flat poseidon
([#8571](#8571))
([0c54224](0c54224))
* Use poseidon for fn selectors
([#8239](#8239))
([41891db](41891db))


### Bug Fixes

* Disable side-effects for no_predicates functions
(noir-lang/noir#6027)
([cd7983a](cd7983a))
* Native world state test issues
([#8546](#8546))
([aab8773](aab8773))
* Remove special case for epoch 0
([#8549](#8549))
([b035d01](b035d01))
* Serialize AvmVerificationKeyData
([#8529](#8529))
([78c94a4](78c94a4))


### Miscellaneous

* 7791: Disable world_state test suite
([#8594](#8594))
([ee21583](ee21583))
* Add jq to aztec image
([#8542](#8542))
([a7fb791](a7fb791))
* Add sync suite
([#8550](#8550))
([ce0a9db](ce0a9db))
* **ci:** Action to redo typo PRs
([#8553](#8553))
([3ed5879](3ed5879))
* **ci:** Fix master
([#8534](#8534))
([47c368f](47c368f))
* **ci:** Fix redo-typo-pr.yml
([abf9802](abf9802))
* **ci:** Fix redo-typo-pr.yml
([#8555](#8555))
([7f1673c](7f1673c))
* **ci:** Hotfix
([ffd31aa](ffd31aa))
* **ci:** Hotfix arm ci
([979f267](979f267))
* **ci:** Optimize disk usage in arm run
([#8564](#8564))
([33e6aa4](33e6aa4))
* **ci:** Use labels and if branch=master to control jobs
([#8508](#8508))
([68a2226](68a2226))
* GitHub Actions Deployments to Amazon EKS
([#8563](#8563))
([6fae8f0](6fae8f0))
* Moves add gate out of aux
([#8541](#8541))
([c3ad163](c3ad163))
* Protogalaxy verifier matches prover 2
([#8477](#8477))
([58882b1](58882b1))
* Redo typo PR by ankushgoel27
([#8595](#8595))
([7ca6d24](7ca6d24))
* Redo typo PR by Ocheretovich
([#8559](#8559))
([c4296ba](c4296ba))
* Redo typo PR by Olexandr88
([#8560](#8560))
([e35d148](e35d148))
* Redo typo PR by skaunov
([#8557](#8557))
([8a1e7c3](8a1e7c3))
* Release Noir(0.34.0) (noir-lang/noir#5692)
([cd7983a](cd7983a))
* Remove ARCHIVER_L1_START_BLOCK
([#8554](#8554))
([bc8d461](bc8d461))
* Remove redundant e2e tests and organize
([#8561](#8561))
([de2b775](de2b775))
* Remove unused imports
([#8556](#8556))
([e11242e](e11242e))
* Replace relative paths to noir-protocol-circuits
([2336986](2336986))
* Replace relative paths to noir-protocol-circuits
([9668ed5](9668ed5))
</details>

<details><summary>barretenberg: 0.55.1</summary>

##
[0.55.1](barretenberg-v0.55.0...barretenberg-v0.55.1)
(2024-09-17)


### Bug Fixes

* Native world state test issues
([#8546](#8546))
([aab8773](aab8773))


### Miscellaneous

* 7791: Disable world_state test suite
([#8594](#8594))
([ee21583](ee21583))
* Moves add gate out of aux
([#8541](#8541))
([c3ad163](c3ad163))
* Protogalaxy verifier matches prover 2
([#8477](#8477))
([58882b1](58882b1))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
AztecBot added a commit to AztecProtocol/barretenberg that referenced this pull request Sep 18, 2024
@AztecBot
chore(master): Release 0.55.1 (#8533)
6585af6 
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-package: 0.55.1</summary>

##
[0.55.1](AztecProtocol/aztec-packages@aztec-package-v0.55.0...aztec-package-v0.55.1)
(2024-09-17)


### Features

* CI deploy on sepolia
([#8514](AztecProtocol/aztec-packages#8514))
([54f0344](AztecProtocol/aztec-packages@54f0344))
* Tx verification & peer scoring on p2p layer. bot support for
EasyPrivateToken
([#8298](AztecProtocol/aztec-packages#8298))
([beb651f](AztecProtocol/aztec-packages@beb651f))


### Miscellaneous

* Remove ARCHIVER_L1_START_BLOCK
([#8554](AztecProtocol/aztec-packages#8554))
([bc8d461](AztecProtocol/aztec-packages@bc8d461))
</details>

<details><summary>barretenberg.js: 0.55.1</summary>

##
[0.55.1](AztecProtocol/aztec-packages@barretenberg.js-v0.55.0...barretenberg.js-v0.55.1)
(2024-09-17)


### Miscellaneous

* **barretenberg.js:** Synchronize aztec-packages versions
</details>

<details><summary>aztec-packages: 0.55.1</summary>

##
[0.55.1](AztecProtocol/aztec-packages@aztec-packages-v0.55.0...aztec-packages-v0.55.1)
(2024-09-17)


### Features

* `TXE::store_note_in_cache` --&gt; `TXE::add_note`
([#8547](AztecProtocol/aztec-packages#8547))
([5a6aaeb](AztecProtocol/aztec-packages@5a6aaeb))
* Add a `comptime` string type for string handling at compile-time
(noir-lang/noir#6026)
([cd7983a](AztecProtocol/aztec-packages@cd7983a))
* CI deploy on sepolia
([#8514](AztecProtocol/aztec-packages#8514))
([54f0344](AztecProtocol/aztec-packages@54f0344))
* Default to outputting witness with file named after package
(noir-lang/noir#6031)
([cd7983a](AztecProtocol/aztec-packages@cd7983a))
* Let LSP suggest trait impl methods as you are typing them
(noir-lang/noir#6029)
([cd7983a](AztecProtocol/aztec-packages@cd7983a))
* NFT with "transient" storage shield flow
([#8129](AztecProtocol/aztec-packages#8129))
([578f67c](AztecProtocol/aztec-packages@578f67c))
* Optimize allocating immediate amounts of memory
([#8579](AztecProtocol/aztec-packages#8579))
([e0185e7](AztecProtocol/aztec-packages@e0185e7))
* Spartan iac
([#8455](AztecProtocol/aztec-packages#8455))
([16fba46](AztecProtocol/aztec-packages@16fba46))
* Sync from aztec-packages (noir-lang/noir#6028)
([cd7983a](AztecProtocol/aztec-packages@cd7983a))
* Tx verification & peer scoring on p2p layer. bot support for
EasyPrivateToken
([#8298](AztecProtocol/aztec-packages#8298))
([beb651f](AztecProtocol/aztec-packages@beb651f))
* Unconstraining keys in unconstrained encryption
([#7912](AztecProtocol/aztec-packages#7912))
([eb9275a](AztecProtocol/aztec-packages@eb9275a))
* Update args hash to be a flat poseidon
([#8571](AztecProtocol/aztec-packages#8571))
([0c54224](AztecProtocol/aztec-packages@0c54224))
* Use poseidon for fn selectors
([#8239](AztecProtocol/aztec-packages#8239))
([41891db](AztecProtocol/aztec-packages@41891db))


### Bug Fixes

* Disable side-effects for no_predicates functions
(noir-lang/noir#6027)
([cd7983a](AztecProtocol/aztec-packages@cd7983a))
* Native world state test issues
([#8546](AztecProtocol/aztec-packages#8546))
([aab8773](AztecProtocol/aztec-packages@aab8773))
* Remove special case for epoch 0
([#8549](AztecProtocol/aztec-packages#8549))
([b035d01](AztecProtocol/aztec-packages@b035d01))
* Serialize AvmVerificationKeyData
([#8529](AztecProtocol/aztec-packages#8529))
([78c94a4](AztecProtocol/aztec-packages@78c94a4))


### Miscellaneous

* 7791: Disable world_state test suite
([#8594](AztecProtocol/aztec-packages#8594))
([ee21583](AztecProtocol/aztec-packages@ee21583))
* Add jq to aztec image
([#8542](AztecProtocol/aztec-packages#8542))
([a7fb791](AztecProtocol/aztec-packages@a7fb791))
* Add sync suite
([#8550](AztecProtocol/aztec-packages#8550))
([ce0a9db](AztecProtocol/aztec-packages@ce0a9db))
* **ci:** Action to redo typo PRs
([#8553](AztecProtocol/aztec-packages#8553))
([3ed5879](AztecProtocol/aztec-packages@3ed5879))
* **ci:** Fix master
([#8534](AztecProtocol/aztec-packages#8534))
([47c368f](AztecProtocol/aztec-packages@47c368f))
* **ci:** Fix redo-typo-pr.yml
([abf9802](AztecProtocol/aztec-packages@abf9802))
* **ci:** Fix redo-typo-pr.yml
([#8555](AztecProtocol/aztec-packages#8555))
([7f1673c](AztecProtocol/aztec-packages@7f1673c))
* **ci:** Hotfix
([ffd31aa](AztecProtocol/aztec-packages@ffd31aa))
* **ci:** Hotfix arm ci
([979f267](AztecProtocol/aztec-packages@979f267))
* **ci:** Optimize disk usage in arm run
([#8564](AztecProtocol/aztec-packages#8564))
([33e6aa4](AztecProtocol/aztec-packages@33e6aa4))
* **ci:** Use labels and if branch=master to control jobs
([#8508](AztecProtocol/aztec-packages#8508))
([68a2226](AztecProtocol/aztec-packages@68a2226))
* GitHub Actions Deployments to Amazon EKS
([#8563](AztecProtocol/aztec-packages#8563))
([6fae8f0](AztecProtocol/aztec-packages@6fae8f0))
* Moves add gate out of aux
([#8541](AztecProtocol/aztec-packages#8541))
([c3ad163](AztecProtocol/aztec-packages@c3ad163))
* Protogalaxy verifier matches prover 2
([#8477](AztecProtocol/aztec-packages#8477))
([58882b1](AztecProtocol/aztec-packages@58882b1))
* Redo typo PR by ankushgoel27
([#8595](AztecProtocol/aztec-packages#8595))
([7ca6d24](AztecProtocol/aztec-packages@7ca6d24))
* Redo typo PR by Ocheretovich
([#8559](AztecProtocol/aztec-packages#8559))
([c4296ba](AztecProtocol/aztec-packages@c4296ba))
* Redo typo PR by Olexandr88
([#8560](AztecProtocol/aztec-packages#8560))
([e35d148](AztecProtocol/aztec-packages@e35d148))
* Redo typo PR by skaunov
([#8557](AztecProtocol/aztec-packages#8557))
([8a1e7c3](AztecProtocol/aztec-packages@8a1e7c3))
* Release Noir(0.34.0) (noir-lang/noir#5692)
([cd7983a](AztecProtocol/aztec-packages@cd7983a))
* Remove ARCHIVER_L1_START_BLOCK
([#8554](AztecProtocol/aztec-packages#8554))
([bc8d461](AztecProtocol/aztec-packages@bc8d461))
* Remove redundant e2e tests and organize
([#8561](AztecProtocol/aztec-packages#8561))
([de2b775](AztecProtocol/aztec-packages@de2b775))
* Remove unused imports
([#8556](AztecProtocol/aztec-packages#8556))
([e11242e](AztecProtocol/aztec-packages@e11242e))
* Replace relative paths to noir-protocol-circuits
([2336986](AztecProtocol/aztec-packages@2336986))
* Replace relative paths to noir-protocol-circuits
([9668ed5](AztecProtocol/aztec-packages@9668ed5))
</details>

<details><summary>barretenberg: 0.55.1</summary>

##
[0.55.1](AztecProtocol/aztec-packages@barretenberg-v0.55.0...barretenberg-v0.55.1)
(2024-09-17)


### Bug Fixes

* Native world state test issues
([#8546](AztecProtocol/aztec-packages#8546))
([aab8773](AztecProtocol/aztec-packages@aab8773))


### Miscellaneous

* 7791: Disable world_state test suite
([#8594](AztecProtocol/aztec-packages#8594))
([ee21583](AztecProtocol/aztec-packages@ee21583))
* Moves add gate out of aux
([#8541](AztecProtocol/aztec-packages#8541))
([c3ad163](AztecProtocol/aztec-packages@c3ad163))
* Protogalaxy verifier matches prover 2
([#8477](AztecProtocol/aztec-packages#8477))
([58882b1](AztecProtocol/aztec-packages@58882b1))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iakovenkos iakovenkos Awaiting requested review from iakovenkos

Assignees

@codygunton codygunton

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@codygunton @iakovenkos