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

feat: MSM sorting #7351

Merged
merged 24 commits into from
Jul 11, 2024
Merged

feat: MSM sorting #7351

merged 24 commits into from
Jul 11, 2024

Conversation

ledwards2225
Copy link
Contributor

@ledwards2225 ledwards2225 commented Jul 4, 2024

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 the scalars 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.)

@ledwards2225 ledwards2225 marked this pull request as ready for review July 8, 2024 21:38
@ledwards2225 ledwards2225 self-assigned this Jul 8, 2024
EXPECT_EQ(msm_result, expected_msm_result);
}

TYPED_TEST(SortedMsmTests, ReduceMsmInputsSimple)
Copy link
Contributor

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?

Copy link
Contributor Author

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)
Copy link
Contributor

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

Copy link
Contributor Author

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);
Copy link
Contributor

@Rumata888 Rumata888 Jul 10, 2024

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

#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

Copy link
Contributor Author

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()

Copy link
Contributor

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

Copy link
Contributor

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,
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done

Copy link
Contributor

@Rumata888 Rumata888 left a 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

@ledwards2225
Copy link
Contributor Author

@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.

@ledwards2225 ledwards2225 enabled auto-merge (squash) July 11, 2024 03:32
@ledwards2225 ledwards2225 merged commit 5cbdc54 into master Jul 11, 2024
33 checks passed
@ledwards2225 ledwards2225 deleted the lde/msm_sorting branch July 11, 2024 03:57
rahul-kothari pushed a commit that referenced this pull request Jul 11, 2024
🤖 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).
AztecBot added a commit to AztecProtocol/barretenberg that referenced this pull request Jul 12, 2024
🤖 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).
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 this pull request may close these issues.

4 participants