-
Notifications
You must be signed in to change notification settings - Fork 233
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
feat: generalize protogalaxy to multiple instances #5510
Conversation
Benchmark resultsNo metrics with a significant change found. Detailed resultsAll benchmarks are run on txs on the This benchmark source data is available in JSON format on S3 here. Values are compared against data from master at commit L2 block published to L1Each column represents the number of txs on an L2 block published to L1.
L2 chain processingEach column represents the number of blocks on the L2 chain where each block has 16 txs.
Circuits statsStats on running time and I/O sizes collected for every circuit run across all benchmarks.
Tree insertion statsThe duration to insert a fixed batch of leaves into each tree type.
MiscellaneousTransaction sizes based on how many contract classes are registered in the tx.
Transaction size based on fee payment method
Transaction processing duration by data writes.
|
Testing if merge fixes github actions |
…cProtocol/aztec-packages into lx/generalize-pg-multiple-instances
barretenberg/cpp/src/barretenberg/protogalaxy/protogalaxy_verifier.cpp
Outdated
Show resolved
Hide resolved
@@ -31,7 +31,10 @@ | |||
print( | |||
f"{column['function']:<{max_label_length}}{column['ms']:>8} {column['%']:>8}") | |||
for key in to_keep: | |||
time_ms = bench[key]/1e6 | |||
if key not in bench: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
making this script less brittle
@@ -0,0 +1,61 @@ | |||
import json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could delete this script; was used to get a breakdown of protogalaxy bench
} else if constexpr (LENGTH == 4) { | ||
static constexpr Fr inverse_six = Fr(6).invert(); | ||
|
||
// These formulas were found by inverting the matrix representation of the coefficients of the 4 evaluations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could add more documentation here, or add it to the hackmd cited in LENGTH == 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that you are basically recomputing monomial coefficeints by hand, but I think it's worth pointing that out explicitly in code, so somebody not from the crypto team wouldn't get confused by this code.
} else if constexpr (ProverInstances::NUM == 4) { | ||
lagrange_0 = (FF(1) - FF(point)) * (FF(2) - FF(point)) * (FF(3) - FF(point)) * inverse_six; | ||
vanishing_polynomial = FF(point) * (FF(point) - 1) * (FF(point) - 2) * (FF(point) - 3); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe should add an else { ASSERT(false); } here to make sure nothing goes wrong if we ever add more k
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think we should, but a static_assert
@@ -28,19 +29,28 @@ template <typename Flavor> void fold_one(State& state) noexcept | |||
MockCircuits::construct_arithmetic_circuit(builder, log2_num_gates); | |||
return std::make_shared<ProverInstance>(builder); | |||
}; | |||
std::vector<std::shared_ptr<Instance>> instances; | |||
for (size_t i = 0; i < k + 1; ++i) { | |||
instances.emplace_back(construct_instance()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mb it's worth paralellising this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its outside the actual benchmark so its fine
@@ -56,6 +59,23 @@ template <typename Flavor> class ProtoGalaxyTests : public testing::Test { | |||
} | |||
} | |||
|
|||
// constructs num_insts number of prover and verifier instances | |||
static TupleOfInstances construct_instances(size_t num_insts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not parallelise this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a benchmark so I wasn't thinking about performance, but we could
challenge * (FF(2) - challenge) * (FF(3) - challenge) * inverse_two, | ||
challenge * (challenge - FF(1)) * (FF(3) - challenge) * inverse_two, | ||
challenge * (challenge - FF(1)) * (challenge - FF(2)) * inverse_six }; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add an else with a static_assert(False)?
combiner_challenge * (FF(2) - combiner_challenge) * (FF(3) - combiner_challenge) * inverse_two, | ||
combiner_challenge * (combiner_challenge - FF(1)) * (FF(3) - combiner_challenge) * inverse_two, | ||
combiner_challenge * (combiner_challenge - FF(1)) * (combiner_challenge - FF(2)) * inverse_six }; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, please add else static_assert(false)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.34.0</summary> ## [0.34.0](aztec-package-v0.33.0...aztec-package-v0.34.0) (2024-04-10) ### Miscellaneous * Reduce log verbosity in local e2e tests ([#5622](#5622)) ([c496a10](c496a10)) </details> <details><summary>barretenberg.js: 0.34.0</summary> ## [0.34.0](barretenberg.js-v0.33.0...barretenberg.js-v0.34.0) (2024-04-10) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-cli: 0.34.0</summary> ## [0.34.0](aztec-cli-v0.33.0...aztec-cli-v0.34.0) (2024-04-10) ### Miscellaneous * Reduce log verbosity in local e2e tests ([#5622](#5622)) ([c496a10](c496a10)) </details> <details><summary>aztec-packages: 0.34.0</summary> ## [0.34.0](aztec-packages-v0.33.0...aztec-packages-v0.34.0) (2024-04-10) ### ⚠ BREAKING CHANGES * remove fixed-length keccak256 ([#5617](#5617)) ### Features * **acvm_js:** Execute program (noir-lang/noir#4694) ([ff28080](ff28080)) * Add `remove_enable_side_effects` SSA pass (noir-lang/noir#4224) ([ff28080](ff28080)) * Allow slices to brillig entry points (noir-lang/noir#4713) ([ff28080](ff28080)) * **avm:** Basic AVM-ACVM interoperability ([#5595](#5595)) ([d872445](d872445)) * **avm:** Make authwit work with avm ([#5594](#5594)) ([b02d1e1](b02d1e1)) * **docs:** Documenting noir codegen (noir-lang/noir#4454) ([ff28080](ff28080)) * Generalize protogalaxy to multiple instances ([#5510](#5510)) ([f038b70](f038b70)) * Improve nargo check cli with --override flag and feedback for existing files (noir-lang/noir#4575) ([ff28080](ff28080)) * Improve optimisations on range constraints (noir-lang/noir#4690) ([ff28080](ff28080)) * Improve SSA type-awareness in EQ and MUL instructions (noir-lang/noir#4691) ([ff28080](ff28080)) * **nargo:** Multiple circuits info for binary programs (noir-lang/noir#4719) ([ff28080](ff28080)) * Stdlib databus ([#5598](#5598)) ([633a711](633a711)) ### Bug Fixes * **acvm:** Mark outputs of Opcode::Call solvable (noir-lang/noir#4708) ([ff28080](ff28080)) * Do not retry RPC requests on 4xx errors ([#5634](#5634)) ([5af2b95](5af2b95)) * Field comparisons (noir-lang/noir#4704) ([ff28080](ff28080)) * Last use analysis & make it an SSA pass (noir-lang/noir#4686) ([ff28080](ff28080)) * **ssa:** Do not use get_value_max_num_bits when we want pure type information (noir-lang/noir#4700) ([ff28080](ff28080)) * Unknown slice lengths coming from as_slice (noir-lang/noir#4725) ([ff28080](ff28080)) * Use empty artifact in test ([#5640](#5640)) ([1d18a5e](1d18a5e)) ### Miscellaneous * Check for references to private functions during path resolution (noir-lang/noir#4622) ([ff28080](ff28080)) * **ci:** Fix cutting new versions of the docs (noir-lang/noir#4737) ([ff28080](ff28080)) * **ci:** Replace `yarn build:js:only` script (noir-lang/noir#4735) ([ff28080](ff28080)) * **ci:** Stop updating version list before cutting new docs version (noir-lang/noir#4726) ([ff28080](ff28080)) * Disable earthly cloud ([#5639](#5639)) ([47e9c25](47e9c25)) * Fix clippy errors (noir-lang/noir#4684) ([ff28080](ff28080)) * Reduce log verbosity in local e2e tests ([#5622](#5622)) ([c496a10](c496a10)) * Remove `FunctionInput::dummy` (noir-lang/noir#4723) ([ff28080](ff28080)) * Remove conditional compilation around `acvm_js` package (noir-lang/noir#4702) ([ff28080](ff28080)) * Remove docker CI flow (noir-lang/noir#4724) ([ff28080](ff28080)) * Remove fixed-length keccak256 ([#5617](#5617)) ([40480b3](40480b3)) * Remove last traces of nix (noir-lang/noir#4679) ([ff28080](ff28080)) * Remove unused env vars from `Cross.toml` (noir-lang/noir#4717) ([ff28080](ff28080)) * Replace relative paths to noir-protocol-circuits ([bc214c5](bc214c5)) * Simplify how `acvm_backend.wasm` is embedded (noir-lang/noir#4703) ([ff28080](ff28080)) * Simplify how blns is loaded into tests (noir-lang/noir#4705) ([ff28080](ff28080)) * Update condition for clearing warning comment on release PRs (noir-lang/noir#4739) ([ff28080](ff28080)) * Update from vulnerable version of h2 (noir-lang/noir#4714) ([ff28080](ff28080)) * Update JS publish workflow to upload build artifacts correctly. (noir-lang/noir#4734) ([ff28080](ff28080)) * Use is_entry_point helper on RuntimeType (noir-lang/noir#4678) ([ff28080](ff28080)) </details> <details><summary>barretenberg: 0.34.0</summary> ## [0.34.0](barretenberg-v0.33.0...barretenberg-v0.34.0) (2024-04-10) ### ⚠ BREAKING CHANGES * remove fixed-length keccak256 ([#5617](#5617)) ### Features * Generalize protogalaxy to multiple instances ([#5510](#5510)) ([f038b70](f038b70)) * Stdlib databus ([#5598](#5598)) ([633a711](633a711)) ### Miscellaneous * Remove fixed-length keccak256 ([#5617](#5617)) ([40480b3](40480b3)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.34.0</summary> ## [0.34.0](AztecProtocol/aztec-packages@aztec-package-v0.33.0...aztec-package-v0.34.0) (2024-04-10) ### Miscellaneous * Reduce log verbosity in local e2e tests ([#5622](AztecProtocol/aztec-packages#5622)) ([c496a10](AztecProtocol/aztec-packages@c496a10)) </details> <details><summary>barretenberg.js: 0.34.0</summary> ## [0.34.0](AztecProtocol/aztec-packages@barretenberg.js-v0.33.0...barretenberg.js-v0.34.0) (2024-04-10) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-cli: 0.34.0</summary> ## [0.34.0](AztecProtocol/aztec-packages@aztec-cli-v0.33.0...aztec-cli-v0.34.0) (2024-04-10) ### Miscellaneous * Reduce log verbosity in local e2e tests ([#5622](AztecProtocol/aztec-packages#5622)) ([c496a10](AztecProtocol/aztec-packages@c496a10)) </details> <details><summary>aztec-packages: 0.34.0</summary> ## [0.34.0](AztecProtocol/aztec-packages@aztec-packages-v0.33.0...aztec-packages-v0.34.0) (2024-04-10) ### ⚠ BREAKING CHANGES * remove fixed-length keccak256 ([#5617](AztecProtocol/aztec-packages#5617)) ### Features * **acvm_js:** Execute program (noir-lang/noir#4694) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Add `remove_enable_side_effects` SSA pass (noir-lang/noir#4224) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Allow slices to brillig entry points (noir-lang/noir#4713) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * **avm:** Basic AVM-ACVM interoperability ([#5595](AztecProtocol/aztec-packages#5595)) ([d872445](AztecProtocol/aztec-packages@d872445)) * **avm:** Make authwit work with avm ([#5594](AztecProtocol/aztec-packages#5594)) ([b02d1e1](AztecProtocol/aztec-packages@b02d1e1)) * **docs:** Documenting noir codegen (noir-lang/noir#4454) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Generalize protogalaxy to multiple instances ([#5510](AztecProtocol/aztec-packages#5510)) ([f038b70](AztecProtocol/aztec-packages@f038b70)) * Improve nargo check cli with --override flag and feedback for existing files (noir-lang/noir#4575) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Improve optimisations on range constraints (noir-lang/noir#4690) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Improve SSA type-awareness in EQ and MUL instructions (noir-lang/noir#4691) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * **nargo:** Multiple circuits info for binary programs (noir-lang/noir#4719) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Stdlib databus ([#5598](AztecProtocol/aztec-packages#5598)) ([633a711](AztecProtocol/aztec-packages@633a711)) ### Bug Fixes * **acvm:** Mark outputs of Opcode::Call solvable (noir-lang/noir#4708) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Do not retry RPC requests on 4xx errors ([#5634](AztecProtocol/aztec-packages#5634)) ([5af2b95](AztecProtocol/aztec-packages@5af2b95)) * Field comparisons (noir-lang/noir#4704) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Last use analysis & make it an SSA pass (noir-lang/noir#4686) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * **ssa:** Do not use get_value_max_num_bits when we want pure type information (noir-lang/noir#4700) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Unknown slice lengths coming from as_slice (noir-lang/noir#4725) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Use empty artifact in test ([#5640](AztecProtocol/aztec-packages#5640)) ([1d18a5e](AztecProtocol/aztec-packages@1d18a5e)) ### Miscellaneous * Check for references to private functions during path resolution (noir-lang/noir#4622) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * **ci:** Fix cutting new versions of the docs (noir-lang/noir#4737) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * **ci:** Replace `yarn build:js:only` script (noir-lang/noir#4735) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * **ci:** Stop updating version list before cutting new docs version (noir-lang/noir#4726) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Disable earthly cloud ([#5639](AztecProtocol/aztec-packages#5639)) ([47e9c25](AztecProtocol/aztec-packages@47e9c25)) * Fix clippy errors (noir-lang/noir#4684) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Reduce log verbosity in local e2e tests ([#5622](AztecProtocol/aztec-packages#5622)) ([c496a10](AztecProtocol/aztec-packages@c496a10)) * Remove `FunctionInput::dummy` (noir-lang/noir#4723) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Remove conditional compilation around `acvm_js` package (noir-lang/noir#4702) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Remove docker CI flow (noir-lang/noir#4724) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Remove fixed-length keccak256 ([#5617](AztecProtocol/aztec-packages#5617)) ([40480b3](AztecProtocol/aztec-packages@40480b3)) * Remove last traces of nix (noir-lang/noir#4679) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Remove unused env vars from `Cross.toml` (noir-lang/noir#4717) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Replace relative paths to noir-protocol-circuits ([bc214c5](AztecProtocol/aztec-packages@bc214c5)) * Simplify how `acvm_backend.wasm` is embedded (noir-lang/noir#4703) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Simplify how blns is loaded into tests (noir-lang/noir#4705) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Update condition for clearing warning comment on release PRs (noir-lang/noir#4739) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Update from vulnerable version of h2 (noir-lang/noir#4714) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Update JS publish workflow to upload build artifacts correctly. (noir-lang/noir#4734) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Use is_entry_point helper on RuntimeType (noir-lang/noir#4678) ([ff28080](AztecProtocol/aztec-packages@ff28080)) </details> <details><summary>barretenberg: 0.34.0</summary> ## [0.34.0](AztecProtocol/aztec-packages@barretenberg-v0.33.0...barretenberg-v0.34.0) (2024-04-10) ### ⚠ BREAKING CHANGES * remove fixed-length keccak256 ([#5617](AztecProtocol/aztec-packages#5617)) ### Features * Generalize protogalaxy to multiple instances ([#5510](AztecProtocol/aztec-packages#5510)) ([f038b70](AztecProtocol/aztec-packages@f038b70)) * Stdlib databus ([#5598](AztecProtocol/aztec-packages#5598)) ([633a711](AztecProtocol/aztec-packages@633a711)) ### Miscellaneous * Remove fixed-length keccak256 ([#5617](AztecProtocol/aztec-packages#5617)) ([40480b3](AztecProtocol/aztec-packages@40480b3)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Closes AztecProtocol/barretenberg#764.
This PR aims to generalize protogalaxy to multiple instances. In particular, we care about k=2 (1 accumulator and 2 instances) and k=3 and and their performance relative to folding k=1 instance.
We achieve the following numbers:
and client IVC benchmark stays the same: