-
Notifications
You must be signed in to change notification settings - Fork 260
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: MSM sorting #7351
feat: MSM sorting #7351
Conversation
EXPECT_EQ(msm_result, expected_msm_result); | ||
} | ||
|
||
TYPED_TEST(SortedMsmTests, ReduceMsmInputsSimple) |
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 a comment what this test is testing?
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.
done, thanks, not sure why I missed this
EXPECT_EQ(msm_result, expected_msm_result); | ||
} | ||
|
||
TYPED_TEST(SortedMsmTests, ReduceMsmInputs) |
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 describe what the test is for
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.
done
{ | ||
// Create the array containing the indices of the scalars and points sorted by scalar value | ||
const size_t num_points = points.size(); | ||
std::iota(index.begin(), index.end(), 0); |
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.
Probably would be good to parallelise both for larger ranges. For sort you can do similar to
aztec-packages/barretenberg/cpp/src/barretenberg/honk/proof_system/permutation_library.hpp
Lines 188 to 192 in 59b92ca
#ifdef NO_TBB | |
std::sort(extra_denominator_uint.begin(), extra_denominator_uint.end()); | |
#else | |
std::sort(std::execution::par_unseq, extra_denominator_uint.begin(), extra_denominator.end()); | |
#endif |
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.
Good call thanks. BTW is there a typo in the code you linked? Seems like in else
it should be extra_denominator_uint.end()
instead of extra_denominator.end()
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.
@Rumata888 I got the following error when running ./bootstrap.sh
in cpp ( on v0.46.1 ).
.../honk/proof_system/permutation_library.hpp:191:74: fatal error: use of undeclared identifier 'extra_denominator'; did you mean 'extra_denominator_uint'?
191 | std::sort(std::execution::par_unseq, extra_denominator_uint.begin(), extra_denominator.end());
| ^~~~~~~~~~~~~~~~~
| extra_denominator_uint
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.
Is it the same error on latest master?
* @return Curve::AffineElement | ||
*/ | ||
template <typename Curve> | ||
typename Curve::AffineElement MsmSorter<Curve>::affine_add_with_denominator(const G1& point_1, |
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.
Since we call this all the time in one loop it would be helpful to inline it, I think
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.
Thanks, done
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.
It's a very clean PR with lots of docs (thank you) and everything looks good, The only issue I see is that the whole algorithm is single-threaded which will probably impede it quite a lot if we use it in a multi threaded environment
@Rumata888 Thanks for the helpful review, as always. As part of the follow where this work will be integrated, I'll ensure things are multithreaded and proven out in benchmarks that demonstrate when (if!) this is advantageous. |
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.46.3</summary> ## [0.46.3](aztec-package-v0.46.2...aztec-package-v0.46.3) (2024-07-11) ### Miscellaneous * **aztec-package:** Synchronize aztec-packages versions </details> <details><summary>barretenberg.js: 0.46.3</summary> ## [0.46.3](barretenberg.js-v0.46.2...barretenberg.js-v0.46.3) (2024-07-11) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-packages: 0.46.3</summary> ## [0.46.3](aztec-packages-v0.46.2...aztec-packages-v0.46.3) (2024-07-11) ### Features * Add CLI argument for debugging comptime blocks (noir-lang/noir#5192) ([97ecff5](97ecff5)) * Add reset tiny and optimize tail ([#7422](#7422)) ([399917b](399917b)) * **avm:** Calldatacopy and return gadget ([#7415](#7415)) ([ec39e4e](ec39e4e)), closes [#7381](#7381) [#7211](#7211) * **avm:** Make ProverPolynomials::get_row return references ([#7419](#7419)) ([108fc5f](108fc5f)) * Integrate new proving systems in e2e ([#6971](#6971)) ([723a0c1](723a0c1)) * Lsp rename/find-all-references for struct members (noir-lang/noir#5443) ([97ecff5](97ecff5)) * MSM sorting ([#7351](#7351)) ([5cbdc54](5cbdc54)) * **optimization:** Deduplicate more instructions (noir-lang/noir#5457) ([97ecff5](97ecff5)) * Prefix operator overload trait dispatch (noir-lang/noir#5423) ([97ecff5](97ecff5)) * Remove proof from L1 Rollup process ([#7347](#7347)) ([2645eab](2645eab)), closes [#7346](#7346) * Remove ram tables in note_getter ([#7434](#7434)) ([fd67da3](fd67da3)) * Sync from aztec-packages (noir-lang/noir#5467) ([97ecff5](97ecff5)) * Typing return values of embedded_curve_ops ([#7413](#7413)) ([db96077](db96077)) ### Bug Fixes * **avm:** Fixes AVM full tests and decrease timeout to 35 minutes ([#7438](#7438)) ([2a7494b](2a7494b)) * Memory init with no other ops gate counting ([#7427](#7427)) ([e7177ba](e7177ba)) * Pass secrets to ci-arm.yml ([#7436](#7436)) ([619501d](619501d)) * Remove compile-time error for invalid indices (noir-lang/noir#5466) ([97ecff5](97ecff5)) * Using different generators in private refund ([#7414](#7414)) ([59b92ca](59b92ca)), closes [#7320](#7320) ### Miscellaneous * **bb:** Fix double increment ([#7428](#7428)) ([7870a58](7870a58)) * **boxes:** Adding an init command for an empty project ([#7398](#7398)) ([a6a605d](a6a605d)) * Bump bb to 0.45.1 (noir-lang/noir#5469) ([97ecff5](97ecff5)) * Disable flaky cheat code test ([7b8c2ba](7b8c2ba)) * Document EmbeddedCurvePoint (noir-lang/noir#5468) ([97ecff5](97ecff5)) * Minimize usage of get_row in inverse computation ([#7431](#7431)) ([f177887](f177887)) * Private refund cleanup ([#7403](#7403)) ([ebec8ff](ebec8ff)) * Replace relative paths to noir-protocol-circuits ([842f6d1](842f6d1)) * Unbundle `check_array_is_initialized` (noir-lang/noir#5451) ([97ecff5](97ecff5)) </details> <details><summary>barretenberg: 0.46.3</summary> ## [0.46.3](barretenberg-v0.46.2...barretenberg-v0.46.3) (2024-07-11) ### Features * **avm:** Calldatacopy and return gadget ([#7415](#7415)) ([ec39e4e](ec39e4e)), closes [#7381](#7381) [#7211](#7211) * **avm:** Make ProverPolynomials::get_row return references ([#7419](#7419)) ([108fc5f](108fc5f)) * Integrate new proving systems in e2e ([#6971](#6971)) ([723a0c1](723a0c1)) * MSM sorting ([#7351](#7351)) ([5cbdc54](5cbdc54)) ### Bug Fixes * **avm:** Fixes AVM full tests and decrease timeout to 35 minutes ([#7438](#7438)) ([2a7494b](2a7494b)) * Memory init with no other ops gate counting ([#7427](#7427)) ([e7177ba](e7177ba)) ### Miscellaneous * **bb:** Fix double increment ([#7428](#7428)) ([7870a58](7870a58)) * Minimize usage of get_row in inverse computation ([#7431](#7431)) ([f177887](f177887)) </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.46.3</summary> ## [0.46.3](AztecProtocol/aztec-packages@aztec-package-v0.46.2...aztec-package-v0.46.3) (2024-07-11) ### Miscellaneous * **aztec-package:** Synchronize aztec-packages versions </details> <details><summary>barretenberg.js: 0.46.3</summary> ## [0.46.3](AztecProtocol/aztec-packages@barretenberg.js-v0.46.2...barretenberg.js-v0.46.3) (2024-07-11) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-packages: 0.46.3</summary> ## [0.46.3](AztecProtocol/aztec-packages@aztec-packages-v0.46.2...aztec-packages-v0.46.3) (2024-07-11) ### Features * Add CLI argument for debugging comptime blocks (noir-lang/noir#5192) ([97ecff5](AztecProtocol/aztec-packages@97ecff5)) * Add reset tiny and optimize tail ([#7422](AztecProtocol/aztec-packages#7422)) ([399917b](AztecProtocol/aztec-packages@399917b)) * **avm:** Calldatacopy and return gadget ([#7415](AztecProtocol/aztec-packages#7415)) ([ec39e4e](AztecProtocol/aztec-packages@ec39e4e)), closes [#7381](AztecProtocol/aztec-packages#7381) [#7211](AztecProtocol/aztec-packages#7211) * **avm:** Make ProverPolynomials::get_row return references ([#7419](AztecProtocol/aztec-packages#7419)) ([108fc5f](AztecProtocol/aztec-packages@108fc5f)) * Integrate new proving systems in e2e ([#6971](AztecProtocol/aztec-packages#6971)) ([723a0c1](AztecProtocol/aztec-packages@723a0c1)) * Lsp rename/find-all-references for struct members (noir-lang/noir#5443) ([97ecff5](AztecProtocol/aztec-packages@97ecff5)) * MSM sorting ([#7351](AztecProtocol/aztec-packages#7351)) ([5cbdc54](AztecProtocol/aztec-packages@5cbdc54)) * **optimization:** Deduplicate more instructions (noir-lang/noir#5457) ([97ecff5](AztecProtocol/aztec-packages@97ecff5)) * Prefix operator overload trait dispatch (noir-lang/noir#5423) ([97ecff5](AztecProtocol/aztec-packages@97ecff5)) * Remove proof from L1 Rollup process ([#7347](AztecProtocol/aztec-packages#7347)) ([2645eab](AztecProtocol/aztec-packages@2645eab)), closes [#7346](AztecProtocol/aztec-packages#7346) * Remove ram tables in note_getter ([#7434](AztecProtocol/aztec-packages#7434)) ([fd67da3](AztecProtocol/aztec-packages@fd67da3)) * Sync from aztec-packages (noir-lang/noir#5467) ([97ecff5](AztecProtocol/aztec-packages@97ecff5)) * Typing return values of embedded_curve_ops ([#7413](AztecProtocol/aztec-packages#7413)) ([db96077](AztecProtocol/aztec-packages@db96077)) ### Bug Fixes * **avm:** Fixes AVM full tests and decrease timeout to 35 minutes ([#7438](AztecProtocol/aztec-packages#7438)) ([2a7494b](AztecProtocol/aztec-packages@2a7494b)) * Memory init with no other ops gate counting ([#7427](AztecProtocol/aztec-packages#7427)) ([e7177ba](AztecProtocol/aztec-packages@e7177ba)) * Pass secrets to ci-arm.yml ([#7436](AztecProtocol/aztec-packages#7436)) ([619501d](AztecProtocol/aztec-packages@619501d)) * Remove compile-time error for invalid indices (noir-lang/noir#5466) ([97ecff5](AztecProtocol/aztec-packages@97ecff5)) * Using different generators in private refund ([#7414](AztecProtocol/aztec-packages#7414)) ([59b92ca](AztecProtocol/aztec-packages@59b92ca)), closes [#7320](AztecProtocol/aztec-packages#7320) ### Miscellaneous * **bb:** Fix double increment ([#7428](AztecProtocol/aztec-packages#7428)) ([7870a58](AztecProtocol/aztec-packages@7870a58)) * **boxes:** Adding an init command for an empty project ([#7398](AztecProtocol/aztec-packages#7398)) ([a6a605d](AztecProtocol/aztec-packages@a6a605d)) * Bump bb to 0.45.1 (noir-lang/noir#5469) ([97ecff5](AztecProtocol/aztec-packages@97ecff5)) * Disable flaky cheat code test ([7b8c2ba](AztecProtocol/aztec-packages@7b8c2ba)) * Document EmbeddedCurvePoint (noir-lang/noir#5468) ([97ecff5](AztecProtocol/aztec-packages@97ecff5)) * Minimize usage of get_row in inverse computation ([#7431](AztecProtocol/aztec-packages#7431)) ([f177887](AztecProtocol/aztec-packages@f177887)) * Private refund cleanup ([#7403](AztecProtocol/aztec-packages#7403)) ([ebec8ff](AztecProtocol/aztec-packages@ebec8ff)) * Replace relative paths to noir-protocol-circuits ([842f6d1](AztecProtocol/aztec-packages@842f6d1)) * Unbundle `check_array_is_initialized` (noir-lang/noir#5451) ([97ecff5](AztecProtocol/aztec-packages@97ecff5)) </details> <details><summary>barretenberg: 0.46.3</summary> ## [0.46.3](AztecProtocol/aztec-packages@barretenberg-v0.46.2...barretenberg-v0.46.3) (2024-07-11) ### Features * **avm:** Calldatacopy and return gadget ([#7415](AztecProtocol/aztec-packages#7415)) ([ec39e4e](AztecProtocol/aztec-packages@ec39e4e)), closes [#7381](AztecProtocol/aztec-packages#7381) [#7211](AztecProtocol/aztec-packages#7211) * **avm:** Make ProverPolynomials::get_row return references ([#7419](AztecProtocol/aztec-packages#7419)) ([108fc5f](AztecProtocol/aztec-packages@108fc5f)) * Integrate new proving systems in e2e ([#6971](AztecProtocol/aztec-packages#6971)) ([723a0c1](AztecProtocol/aztec-packages@723a0c1)) * MSM sorting ([#7351](AztecProtocol/aztec-packages#7351)) ([5cbdc54](AztecProtocol/aztec-packages@5cbdc54)) ### Bug Fixes * **avm:** Fixes AVM full tests and decrease timeout to 35 minutes ([#7438](AztecProtocol/aztec-packages#7438)) ([2a7494b](AztecProtocol/aztec-packages@2a7494b)) * Memory init with no other ops gate counting ([#7427](AztecProtocol/aztec-packages#7427)) ([e7177ba](AztecProtocol/aztec-packages@e7177ba)) ### Miscellaneous * **bb:** Fix double increment ([#7428](AztecProtocol/aztec-packages#7428)) ([7870a58](AztecProtocol/aztec-packages@7870a58)) * Minimize usage of get_row in inverse computation ([#7431](AztecProtocol/aztec-packages#7431)) ([f177887](AztecProtocol/aztec-packages@f177887)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Implements logic for reducing MSM inputs by first summing points which share a scalar. See description in
sorted_msm.hpp
for details of the approach. This is useful when thescalars
input for an MSM contains many repeated values because point addition is much cheaper than scalar multiplication. For example, when using the structured trace, the permutation grand product polynomial is non-zero but constant over 'dead' regions between blocks.Note: For now this work is not integrated into the proving systems. That will be done in a follow on. At that time, considerations will be made for memory consumption (e.g. perhaps memory allocated in pippenger_runtime_state can be repurposed for the MSM sorting logic etc.)