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

refactor(bb): more graceful pippenger on non-powers-of-2 #8279

Merged
merged 27 commits into from
Aug 31, 2024

Conversation

ludamad
Copy link
Collaborator

@ludamad ludamad commented Aug 29, 2024

Pippenger was 50% slow if you had a power of 2 minus 1 vs the same power of 2
Before

----------------------------------------------------------------------------------------------
Benchmark                                                    Time             CPU   Iterations
----------------------------------------------------------------------------------------------
bench_commit_random<curve::BN254>/22                      1438 ms         1313 ms            1
bench_commit_random_non_power_of_2<curve::BN254>/22       1583 ms         1422 ms            1

After

----------------------------------------------------------------------------------------------
Benchmark                                                    Time             CPU   Iterations
----------------------------------------------------------------------------------------------
bench_commit_random<curve::BN254>/22                      1436 ms         1303 ms            1
bench_commit_random_non_power_of_2<curve::BN254>/22       1438 ms         1266 ms            1

wnaf_second_half(&T0.data[2], j);
}
for (uint64_t j = defined_scalars; j < num_initial_points_per_thread; j++) {
// If we are trying to use a non-power-of-2
Copy link
Collaborator Author

@ludamad ludamad Aug 30, 2024

Choose a reason for hiding this comment

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

This could be simpler (in terms of operations, not necessarily code), I just took the obviously correct path as I didn't want to spend too long on it

Copy link
Contributor

Choose a reason for hiding this comment

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

Either waway, thanks for clarifying it a bit :)

@ludamad ludamad marked this pull request as ready for review August 30, 2024 00:56
@ludamad ludamad enabled auto-merge (squash) August 30, 2024 21:14
Copy link
Contributor

@maramihali maramihali left a comment

Choose a reason for hiding this comment

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

Lgtm! Thanks for clarifying code as well, just left some small comments:)

FROM +preset-release-assert-test
COPY --dir ./srs_db/+build/. srs_db
# limit hardware concurrency, if provided
IF [ "$HARDWARE_CONCURRENCY" != "" ]
ENV HARDWARE_CONCURRENCY=$hardware_concurrency
END
RUN cd build && GTEST_COLOR=1 ctest -j$(nproc) --output-on-failure
RUN cd build && ./bin/stdlib_client_ivc_verifier_tests --gtest_filter=ClientIVCRecursionTests.ClientTubeBase #GTEST_COLOR=1 ctest -j$(nproc) --output-on-failure
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this only running some of the tube tests now? Is this intended?

@@ -129,6 +129,22 @@ template <typename Curve> void bench_commit_random(::benchmark::State& state)
key->commit(polynomial);
}
}
// Commit to a polynomial with dense random nonzero entries but NOT our happiest case of an exact power of 2
// Note this used to be a 50% regression just subtracting a power of 2 by 1.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Note this used to be a 50% regression just subtracting a power of 2 by 1.
// Note this used to be a 50% regression just having a polynomial of size 2^n - 1 for any n.

maybe something like this is a bit more clear.

@@ -34,6 +35,17 @@ template <class Curve> class CommitmentKey {
using Fr = typename Curve::ScalarField;
using Commitment = typename Curve::AffineElement;
using G1 = typename Curve::AffineElement;
static constexpr size_t EXTRA_SRS_POINTS_FOR_ECCVM_IPA = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Iirc it’s not strictly an ECCVM thing but generally applies for IPA?

wnaf_second_half(&T0.data[2], j);
}
for (uint64_t j = defined_scalars; j < num_initial_points_per_thread; j++) {
// If we are trying to use a non-power-of-2
Copy link
Contributor

Choose a reason for hiding this comment

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

Either waway, thanks for clarifying it a bit :)

@ludamad ludamad merged commit 104ea85 into master Aug 31, 2024
37 checks passed
@ludamad ludamad deleted the ad/pippenger-edge-case-smoothing branch August 31, 2024 09:31
@ludamad
Copy link
Collaborator Author

ludamad commented Aug 31, 2024

Ah the test change needs to be reverted

TomAFrench added a commit that referenced this pull request Sep 1, 2024
* master:
  git subrepo push --branch=master noir-projects/aztec-nr
  git_subrepo.sh: Fix parent in .gitrepo file. [skip ci]
  chore: replace relative paths to noir-protocol-circuits
  git subrepo push --branch=master barretenberg
  chore(revert): earthfile accidental change (#8309)
  chore: fix warnings in `avm-transpiler` (#8307)
  refactor(bb): more graceful pippenger on non-powers-of-2 (#8279)
  git subrepo push --branch=master noir-projects/aztec-nr
  git_subrepo.sh: Fix parent in .gitrepo file. [skip ci]
  chore: replace relative paths to noir-protocol-circuits
  git subrepo push --branch=master barretenberg
  feat: Removing `is_dev_net` flag (#8275)
PhilWindle pushed a commit that referenced this pull request Sep 2, 2024
🤖 I have created a release *beep* *boop*
---


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

##
[0.52.0](aztec-package-v0.51.1...aztec-package-v0.52.0)
(2024-09-01)


### Miscellaneous

* **aztec-package:** Synchronize aztec-packages versions
</details>

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

##
[0.52.0](barretenberg.js-v0.51.1...barretenberg.js-v0.52.0)
(2024-09-01)


### Miscellaneous

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

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

##
[0.52.0](aztec-packages-v0.51.1...aztec-packages-v0.52.0)
(2024-09-01)


### ⚠ BREAKING CHANGES

* Check unused generics are bound
(noir-lang/noir#5840)

### Features

* Add `Expr::as_assert` (noir-lang/noir#5857)
([cf5b667](cf5b667))
* Add `Expr::resolve` and `TypedExpr::as_function_definition`
(noir-lang/noir#5859)
([cf5b667](cf5b667))
* Add `FunctionDef::body` (noir-lang/noir#5825)
([cf5b667](cf5b667))
* Add `FunctionDef::has_named_attribute`
(noir-lang/noir#5870)
([cf5b667](cf5b667))
* Add `Type::as_string` (noir-lang/noir#5871)
([cf5b667](cf5b667))
* Clarify state in Protogalaxy 3
([#8181](#8181))
([4a9bb9d](4a9bb9d))
* LSP signature help for assert and assert_eq
(noir-lang/noir#5862)
([cf5b667](cf5b667))
* **meta:** Comptime keccak
(noir-lang/noir#5854)
([cf5b667](cf5b667))
* **optimization:** Avoid merging identical (by ID) arrays
(noir-lang/noir#5853)
([cf5b667](cf5b667))
* **perf:** Simplify poseidon2 cache zero-pad
(noir-lang/noir#5869)
([cf5b667](cf5b667))
* Populate epoch 0 from initial validator set
([#8286](#8286))
([cbdec54](cbdec54))
* Remove unnecessary copying of vector size during reversal
(noir-lang/noir#5852)
([cf5b667](cf5b667))
* Removing `is_dev_net` flag
([#8275](#8275))
([fc1f307](fc1f307))
* Show backtrace on comptime assertion failures
(noir-lang/noir#5842)
([cf5b667](cf5b667))
* Simplify constant calls to `poseidon2_permutation`, `schnorr_verify`
and `embedded_curve_add` (noir-lang/noir#5140)
([cf5b667](cf5b667))
* Sync from aztec-packages (noir-lang/noir#5790)
([cf5b667](cf5b667))
* Warn on unused imports (noir-lang/noir#5847)
([cf5b667](cf5b667))


### Bug Fixes

* Check unused generics are bound
(noir-lang/noir#5840)
([cf5b667](cf5b667))
* Enforce parity of sequencer tx validation and node tx validation
([#7951](#7951))
([c7eaf92](c7eaf92))
* Make simulations validate resulting tx by default
([#8157](#8157))
([f5e388d](f5e388d))
* **nargo:** Resolve Brillig assertion payloads
(noir-lang/noir#5872)
([cf5b667](cf5b667))
* Prevent honk proof from getting stale inputs on syncs
([#8293](#8293))
([2598108](2598108))
* Remove fee juice mint public
([#8260](#8260))
([2395af3](2395af3))
* **sha256:** Add extra checks against message size when constructing
msg blocks (noir-lang/noir#5861)
([cf5b667](cf5b667))
* **sha256:** Fix upper bound when building msg block and delay final
block compression under certain cases
(noir-lang/noir#5838)
([cf5b667](cf5b667))
* **sha256:** Perform compression per block and utilize ROM instead of
RAM when setting up the message block
(noir-lang/noir#5760)
([cf5b667](cf5b667))


### Miscellaneous

* Add documentation to `to_be_bytes`, etc.
(noir-lang/noir#5843)
([cf5b667](cf5b667))
* Add missing cases to arithmetic generics
(noir-lang/noir#5841)
([cf5b667](cf5b667))
* Add test to reproduce
[#8306](#8306)
([41d418c](41d418c))
* Alert slack on Sepolia test
([#8263](#8263))
([6194b94](6194b94))
* **bb:** Make compile on stock mac clang
([#8278](#8278))
([7af80ff](7af80ff))
* **bb:** More graceful pippenger on non-powers-of-2
([#8279](#8279))
([104ea85](104ea85))
* Bump noir-bignum to 0.3.2
([#8276](#8276))
([4c6fe1a](4c6fe1a))
* **ci:** Try to debug 'command brotli not found'
([#8305](#8305))
([9ee8dd6](9ee8dd6))
* Don't require empty `Prover.toml` for programs with zero arguments but
a return value (noir-lang/noir#5845)
([cf5b667](cf5b667))
* Fix a bunch of generics issues in aztec-nr
([#8295](#8295))
([6e84970](6e84970))
* Fix more issues with generics
([#8302](#8302))
([4e2ce80](4e2ce80))
* Fix warnings in `avm-transpiler`
([#8307](#8307))
([359fe05](359fe05))
* Introduce the Visitor pattern
(noir-lang/noir#5868)
([cf5b667](cf5b667))
* **perf:** Simplify poseidon2 algorithm
(noir-lang/noir#5811)
([cf5b667](cf5b667))
* **perf:** Update to stdlib keccak for reduced Brillig code size
(noir-lang/noir#5827)
([cf5b667](cf5b667))
* Redo typo PR by nnsW3 (noir-lang/noir#5834)
([cf5b667](cf5b667))
* Renaming around Protogalaxy Prover
([#8272](#8272))
([be2169d](be2169d))
* Replace relative paths to noir-protocol-circuits
([56e3fbf](56e3fbf))
* Replace relative paths to noir-protocol-circuits
([1b245c4](1b245c4))
* Replace relative paths to noir-protocol-circuits
([9c3bc43](9c3bc43))
* **revert:** Earthfile accidental change
([#8309](#8309))
([2d3e0b6](2d3e0b6))
* Underconstrained check in parallel
(noir-lang/noir#5848)
([cf5b667](cf5b667))


### Documentation

* **bb:** Transcript spec
([#8301](#8301))
([18abf37](18abf37))
</details>

<details><summary>barretenberg: 0.52.0</summary>

##
[0.52.0](barretenberg-v0.51.1...barretenberg-v0.52.0)
(2024-09-01)


### Features

* Clarify state in Protogalaxy 3
([#8181](#8181))
([4a9bb9d](4a9bb9d))


### Bug Fixes

* Prevent honk proof from getting stale inputs on syncs
([#8293](#8293))
([2598108](2598108))


### Miscellaneous

* **bb:** Make compile on stock mac clang
([#8278](#8278))
([7af80ff](7af80ff))
* **bb:** More graceful pippenger on non-powers-of-2
([#8279](#8279))
([104ea85](104ea85))
* Renaming around Protogalaxy Prover
([#8272](#8272))
([be2169d](be2169d))
* **revert:** Earthfile accidental change
([#8309](#8309))
([2d3e0b6](2d3e0b6))


### Documentation

* **bb:** Transcript spec
([#8301](#8301))
([18abf37](18abf37))
</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 3, 2024
🤖 I have created a release *beep* *boop*
---


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

##
[0.52.0](AztecProtocol/aztec-packages@aztec-package-v0.51.1...aztec-package-v0.52.0)
(2024-09-01)


### Miscellaneous

* **aztec-package:** Synchronize aztec-packages versions
</details>

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

##
[0.52.0](AztecProtocol/aztec-packages@barretenberg.js-v0.51.1...barretenberg.js-v0.52.0)
(2024-09-01)


### Miscellaneous

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

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

##
[0.52.0](AztecProtocol/aztec-packages@aztec-packages-v0.51.1...aztec-packages-v0.52.0)
(2024-09-01)


### ⚠ BREAKING CHANGES

* Check unused generics are bound
(noir-lang/noir#5840)

### Features

* Add `Expr::as_assert` (noir-lang/noir#5857)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))
* Add `Expr::resolve` and `TypedExpr::as_function_definition`
(noir-lang/noir#5859)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))
* Add `FunctionDef::body` (noir-lang/noir#5825)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))
* Add `FunctionDef::has_named_attribute`
(noir-lang/noir#5870)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))
* Add `Type::as_string` (noir-lang/noir#5871)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))
* Clarify state in Protogalaxy 3
([#8181](AztecProtocol/aztec-packages#8181))
([4a9bb9d](AztecProtocol/aztec-packages@4a9bb9d))
* LSP signature help for assert and assert_eq
(noir-lang/noir#5862)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))
* **meta:** Comptime keccak
(noir-lang/noir#5854)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))
* **optimization:** Avoid merging identical (by ID) arrays
(noir-lang/noir#5853)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))
* **perf:** Simplify poseidon2 cache zero-pad
(noir-lang/noir#5869)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))
* Populate epoch 0 from initial validator set
([#8286](AztecProtocol/aztec-packages#8286))
([cbdec54](AztecProtocol/aztec-packages@cbdec54))
* Remove unnecessary copying of vector size during reversal
(noir-lang/noir#5852)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))
* Removing `is_dev_net` flag
([#8275](AztecProtocol/aztec-packages#8275))
([fc1f307](AztecProtocol/aztec-packages@fc1f307))
* Show backtrace on comptime assertion failures
(noir-lang/noir#5842)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))
* Simplify constant calls to `poseidon2_permutation`, `schnorr_verify`
and `embedded_curve_add` (noir-lang/noir#5140)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))
* Sync from aztec-packages (noir-lang/noir#5790)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))
* Warn on unused imports (noir-lang/noir#5847)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))


### Bug Fixes

* Check unused generics are bound
(noir-lang/noir#5840)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))
* Enforce parity of sequencer tx validation and node tx validation
([#7951](AztecProtocol/aztec-packages#7951))
([c7eaf92](AztecProtocol/aztec-packages@c7eaf92))
* Make simulations validate resulting tx by default
([#8157](AztecProtocol/aztec-packages#8157))
([f5e388d](AztecProtocol/aztec-packages@f5e388d))
* **nargo:** Resolve Brillig assertion payloads
(noir-lang/noir#5872)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))
* Prevent honk proof from getting stale inputs on syncs
([#8293](AztecProtocol/aztec-packages#8293))
([2598108](AztecProtocol/aztec-packages@2598108))
* Remove fee juice mint public
([#8260](AztecProtocol/aztec-packages#8260))
([2395af3](AztecProtocol/aztec-packages@2395af3))
* **sha256:** Add extra checks against message size when constructing
msg blocks (noir-lang/noir#5861)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))
* **sha256:** Fix upper bound when building msg block and delay final
block compression under certain cases
(noir-lang/noir#5838)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))
* **sha256:** Perform compression per block and utilize ROM instead of
RAM when setting up the message block
(noir-lang/noir#5760)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))


### Miscellaneous

* Add documentation to `to_be_bytes`, etc.
(noir-lang/noir#5843)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))
* Add missing cases to arithmetic generics
(noir-lang/noir#5841)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))
* Add test to reproduce
[#8306](AztecProtocol/aztec-packages#8306)
([41d418c](AztecProtocol/aztec-packages@41d418c))
* Alert slack on Sepolia test
([#8263](AztecProtocol/aztec-packages#8263))
([6194b94](AztecProtocol/aztec-packages@6194b94))
* **bb:** Make compile on stock mac clang
([#8278](AztecProtocol/aztec-packages#8278))
([7af80ff](AztecProtocol/aztec-packages@7af80ff))
* **bb:** More graceful pippenger on non-powers-of-2
([#8279](AztecProtocol/aztec-packages#8279))
([104ea85](AztecProtocol/aztec-packages@104ea85))
* Bump noir-bignum to 0.3.2
([#8276](AztecProtocol/aztec-packages#8276))
([4c6fe1a](AztecProtocol/aztec-packages@4c6fe1a))
* **ci:** Try to debug 'command brotli not found'
([#8305](AztecProtocol/aztec-packages#8305))
([9ee8dd6](AztecProtocol/aztec-packages@9ee8dd6))
* Don't require empty `Prover.toml` for programs with zero arguments but
a return value (noir-lang/noir#5845)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))
* Fix a bunch of generics issues in aztec-nr
([#8295](AztecProtocol/aztec-packages#8295))
([6e84970](AztecProtocol/aztec-packages@6e84970))
* Fix more issues with generics
([#8302](AztecProtocol/aztec-packages#8302))
([4e2ce80](AztecProtocol/aztec-packages@4e2ce80))
* Fix warnings in `avm-transpiler`
([#8307](AztecProtocol/aztec-packages#8307))
([359fe05](AztecProtocol/aztec-packages@359fe05))
* Introduce the Visitor pattern
(noir-lang/noir#5868)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))
* **perf:** Simplify poseidon2 algorithm
(noir-lang/noir#5811)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))
* **perf:** Update to stdlib keccak for reduced Brillig code size
(noir-lang/noir#5827)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))
* Redo typo PR by nnsW3 (noir-lang/noir#5834)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))
* Renaming around Protogalaxy Prover
([#8272](AztecProtocol/aztec-packages#8272))
([be2169d](AztecProtocol/aztec-packages@be2169d))
* Replace relative paths to noir-protocol-circuits
([56e3fbf](AztecProtocol/aztec-packages@56e3fbf))
* Replace relative paths to noir-protocol-circuits
([1b245c4](AztecProtocol/aztec-packages@1b245c4))
* Replace relative paths to noir-protocol-circuits
([9c3bc43](AztecProtocol/aztec-packages@9c3bc43))
* **revert:** Earthfile accidental change
([#8309](AztecProtocol/aztec-packages#8309))
([2d3e0b6](AztecProtocol/aztec-packages@2d3e0b6))
* Underconstrained check in parallel
(noir-lang/noir#5848)
([cf5b667](AztecProtocol/aztec-packages@cf5b667))


### Documentation

* **bb:** Transcript spec
([#8301](AztecProtocol/aztec-packages#8301))
([18abf37](AztecProtocol/aztec-packages@18abf37))
</details>

<details><summary>barretenberg: 0.52.0</summary>

##
[0.52.0](AztecProtocol/aztec-packages@barretenberg-v0.51.1...barretenberg-v0.52.0)
(2024-09-01)


### Features

* Clarify state in Protogalaxy 3
([#8181](AztecProtocol/aztec-packages#8181))
([4a9bb9d](AztecProtocol/aztec-packages@4a9bb9d))


### Bug Fixes

* Prevent honk proof from getting stale inputs on syncs
([#8293](AztecProtocol/aztec-packages#8293))
([2598108](AztecProtocol/aztec-packages@2598108))


### Miscellaneous

* **bb:** Make compile on stock mac clang
([#8278](AztecProtocol/aztec-packages#8278))
([7af80ff](AztecProtocol/aztec-packages@7af80ff))
* **bb:** More graceful pippenger on non-powers-of-2
([#8279](AztecProtocol/aztec-packages#8279))
([104ea85](AztecProtocol/aztec-packages@104ea85))
* Renaming around Protogalaxy Prover
([#8272](AztecProtocol/aztec-packages#8272))
([be2169d](AztecProtocol/aztec-packages@be2169d))
* **revert:** Earthfile accidental change
([#8309](AztecProtocol/aztec-packages#8309))
([2d3e0b6](AztecProtocol/aztec-packages@2d3e0b6))


### Documentation

* **bb:** Transcript spec
([#8301](AztecProtocol/aztec-packages#8301))
([18abf37](AztecProtocol/aztec-packages@18abf37))
</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.

2 participants