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

fix: Reallocate commitment key to avoid pippenger error #11249

Merged
merged 7 commits into from
Jan 16, 2025

Conversation

lucasxia01
Copy link
Contributor

@lucasxia01 lucasxia01 commented Jan 15, 2025

Fixes recent bug introduced by the SmallSubgroupIPA work which added an edge case where we always commit to polynomials of some fixed degree (of 259 or whatever). Pippenger was set up to work for circuit_size amount of points, which could be lower than the SmallSubgroupIPA poly sizes, so it led to buffer overflows.

Fixes it by reallocating commitment key if necessary in SmallSubgroupIPA prover. Also adds an assert to commit() to check for any potential overflows.

@lucasxia01 lucasxia01 marked this pull request as ready for review January 15, 2025 19:00
@lucasxia01 lucasxia01 self-assigned this Jan 15, 2025
@@ -90,6 +91,9 @@ template <class Curve> class CommitmentKey {
PROFILE_THIS_NAME("commit");
// We must have a power-of-2 SRS points *after* subtracting by start_index.
size_t dyadic_poly_size = numeric::round_up_power_2(polynomial.size());
info("dyadic_poly_size: ", dyadic_poly_size);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

will delete

@@ -90,6 +91,9 @@ template <class Curve> class CommitmentKey {
PROFILE_THIS_NAME("commit");
// We must have a power-of-2 SRS points *after* subtracting by start_index.
size_t dyadic_poly_size = numeric::round_up_power_2(polynomial.size());
info("dyadic_poly_size: ", dyadic_poly_size);
info("dyadic_size: ", dyadic_size);
ASSERT(dyadic_poly_size <= dyadic_size && "Polynomial size exceeds commitment key size.");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now we throw an error if we try to commit to too large of a polynomial

@@ -135,6 +135,10 @@ template <typename Flavor> class SmallSubgroupIPAProver {
, batched_quotient(QUOTIENT_LENGTH)

{
// Reallocate the commitment key if necessary. This is an edge case with SmallSubgroupIPA
Copy link
Contributor Author

Choose a reason for hiding this comment

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

could add more to this comment

@@ -77,6 +77,7 @@ template <class Curve> class CommitmentKey {
CommitmentKey(const size_t num_points, std::shared_ptr<srs::factories::ProverCrs<Curve>> prover_crs)
: pippenger_runtime_state(num_points)
, srs(prover_crs)
, dyadic_size(get_num_needed_srs_points(num_points))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

used to not be set for plonk, led to errors because the assert would fail.

@@ -135,6 +135,10 @@ template <typename Flavor> class SmallSubgroupIPAProver {
, batched_quotient(QUOTIENT_LENGTH)

{
// Reallocate the commitment key if necessary. This is an edge case with SmallSubgroupIPA
if (commitment_key->dyadic_size < SUBGROUP_SIZE + 3) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose this is safer than catching it in pippenger - but can we also add a check there? Approving though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean? the assert in commit() should serve that purpose right

@lucasxia01 lucasxia01 enabled auto-merge (squash) January 16, 2025 20:33
@lucasxia01 lucasxia01 merged commit 8fc2011 into master Jan 16, 2025
25 checks passed
@lucasxia01 lucasxia01 deleted the lx/fix-small-subgroup-ipa-commitment-error branch January 16, 2025 20:48
TomAFrench added a commit that referenced this pull request Jan 16, 2025
* master: (395 commits)
  chore: Log correlation in traces in google cloud (#11276)
  fix: Reallocate commitment key to avoid pippenger error (#11249)
  feat: archive public testnet tx data (#11192)
  chore: remove warnings from types and rollup lib crates (#11269)
  fix: Faster polling times for archiver and sequencer (#11262)
  chore: Remove references to padding txs (#11264)
  chore(avm): calldata, returndata slices out of range padded with zeros (#11265)
  feat: process note logs in aztec-nr (#10651)
  feat: track block building helpers (#11190)
  chore: silence circuit return values in CI (#11259)
  chore: Demote error closing forks to warn (#11263)
  chore:  move shared pcs functionality to internal library in solidity and small refactorings in sumcheck (#11230)
  chore: delete external-ci-approved.yml (#11258)
  feat: reenable constrained config for roots (#10605)
  feat(docs): algolia->typesense (#11034)
  fix: references to a3 in docs (#11256)
  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
  ...
rahul-kothari pushed a commit that referenced this pull request Jan 17, 2025
🤖 I have created a release *beep* *boop*
---


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

##
[0.71.0](aztec-package-v0.70.0...aztec-package-v0.71.0)
(2025-01-17)


### Features

* Track block building helpers
([#11190](#11190))
([a749dc1](a749dc1)),
closes
[#11184](#11184)
</details>

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

##
[0.71.0](barretenberg.js-v0.70.0...barretenberg.js-v0.71.0)
(2025-01-17)


### Miscellaneous

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

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

##
[0.71.0](aztec-packages-v0.70.0...aztec-packages-v0.71.0)
(2025-01-17)


### ⚠ BREAKING CHANGES

* `loop` statements (only frontend)
(noir-lang/noir#7092)
* Include kind in `StructDefinition::generics` and fix derivation of Eq
in structs with numeric generics
(noir-lang/noir#7076)
* Attestation collection times out based on sequencer timetable
([#11248](#11248))

### Features

* `loop` statements (only frontend)
(noir-lang/noir#7092)
([a964cd0](a964cd0))
* Add `ConstrainNotEqual` instruction
(noir-lang/noir#7032)
([a964cd0](a964cd0))
* Archive public testnet tx data
([#11192](#11192))
([66f2014](66f2014))
* Backup proof failures to google cloud storage
([#11255](#11255))
([b4775fd](b4775fd)),
closes
[#11062](#11062)
* **docs:** Algolia-&gt;typesense
([#11034](#11034))
([d254f49](d254f49))
* Improve PXE contract DB capabilities
([#11303](#11303))
([fab5570](fab5570))
* **LSP:** Auto-import trait reexport if trait is not visible
(noir-lang/noir#7079)
([a964cd0](a964cd0))
* Process note logs in aztec-nr
([#10651](#10651))
([708139d](708139d))
* Reenable constrained config for roots
([#10605](#10605))
([a6ebc2e](a6ebc2e))
* **spartan:** Add extra accounts
([#11300](#11300))
([7782836](7782836))
* **ssa:** Treat globals as constant in a function's DFG
(noir-lang/noir#7040)
([a964cd0](a964cd0))
* Track block building helpers
([#11190](#11190))
([a749dc1](a749dc1)),
closes
[#11184](#11184)


### Bug Fixes

* Allow implicit associated types on integer type kinds
(noir-lang/noir#7078)
([a964cd0](a964cd0))
* Do not remove memory blocks used as brillig input
(noir-lang/noir#7073)
([a964cd0](a964cd0))
* Ensure 'docker info' works before preceding
([#11286](#11286))
([0b0e81a](0b0e81a))
* Fail in proxy deployment should fail the step
([#11308](#11308))
([b780d75](b780d75))
* Faster polling times for archiver and sequencer
([#11262](#11262))
([d70511e](d70511e))
* Https://github.com/AztecProtocol/aztec-packages/issues/8939
([66f2014](66f2014))
* Idempotent deploy-l1-contracts with initial validators
([#11284](#11284))
([3a3f9c0](3a3f9c0)),
closes
[#11283](#11283)
* Include kind in `StructDefinition::generics` and fix derivation of Eq
in structs with numeric generics
(noir-lang/noir#7076)
([a964cd0](a964cd0))
* Legacy runner start
([#11291](#11291))
([0b2a619](0b2a619))
* Reallocate commitment key to avoid pippenger error
([#11249](#11249))
([8fc2011](8fc2011))
* References to a3 in docs
([#11256](#11256))
([caf88fa](caf88fa))
* Remove bb path override in cli-wallet
([#11280](#11280))
([a6a226e](a6a226e))
* Resolve misc bugs handling phases in avm witgen
([#11218](#11218))
([29bc4bd](29bc4bd))
* Sequencer timetable accounts for spare time
([#11221](#11221))
([f1b9211](f1b9211))
* Validator ignores block limits during reexec
([#11288](#11288))
([920a521](920a521))


### Miscellaneous

* Add circuit input checks to bootstrap.sh
([#11261](#11261))
([a718b15](a718b15))
* Add regression test for
[#6530](#6530)
(noir-lang/noir#7089)
([a964cd0](a964cd0))
* Add test for isuee
[#7090](#7090)
(noir-lang/noir#7091)
([a964cd0](a964cd0))
* Allow passing custom conditions to inlining pass
(noir-lang/noir#7083)
([a964cd0](a964cd0))
* Attestation collection times out based on sequencer timetable
([#11248](#11248))
([946a418](946a418))
* **avm:** Calldata, returndata slices out of range padded with zeros
([#11265](#11265))
([a469c94](a469c94)),
closes
[#10933](#10933)
* Delete external-ci-approved.yml
([#11258](#11258))
([642bce6](642bce6))
* Demote error closing forks to warn
([#11263](#11263))
([a5b7a6a](a5b7a6a))
* Do not make new instruction if it hasn't changed
(noir-lang/noir#7069)
([a964cd0](a964cd0))
* Ensure devnet has unproven config
([#11302](#11302))
([085f782](085f782))
* Fixing `[@safety](https://github.com/safety)` warnings
([#11094](#11094))
([5de24e0](5de24e0))
* Log correlation in traces in google cloud
([#11276](#11276))
([fbcc8ef](fbcc8ef)),
closes
[#11019](#11019)
[#10937](#10937)
* Mark `noir-edwards` as expected to compile
(noir-lang/noir#7085)
([a964cd0](a964cd0))
* Move shared pcs functionality to internal library in solidity and
small refactorings in sumcheck
([#11230](#11230))
([507ae9d](507ae9d))
* Reduce the number of provers in rc-1
([#11296](#11296))
([92e40ff](92e40ff))
* Remove references to padding txs
([#11264](#11264))
([32408f6](32408f6))
* Remove warnings from types and rollup lib crates
([#11269](#11269))
([9f389a7](9f389a7))
* Replace relative paths to noir-protocol-circuits
([8ece166](8ece166))
* Replace relative paths to noir-protocol-circuits
([be42305](be42305))
* Retry deploys
([#11252](#11252))
([23cfbb4](23cfbb4))
* Set failed proof store for spartan deployments
([#11282](#11282))
([f787a52](f787a52))
* Silence "Updated proven chain" log
([#11250](#11250))
([44bd79b](44bd79b))
* Silence circuit return values in CI
([#11259](#11259))
([db3d860](db3d860))
* Stable masternet images
([#11289](#11289))
([07fabe8](07fabe8))
</details>

<details><summary>barretenberg: 0.71.0</summary>

##
[0.71.0](barretenberg-v0.70.0...barretenberg-v0.71.0)
(2025-01-17)


### Bug Fixes

* Reallocate commitment key to avoid pippenger error
([#11249](#11249))
([8fc2011](8fc2011))
* Resolve misc bugs handling phases in avm witgen
([#11218](#11218))
([29bc4bd](29bc4bd))


### Miscellaneous

* **avm:** Calldata, returndata slices out of range padded with zeros
([#11265](#11265))
([a469c94](a469c94)),
closes
[#10933](#10933)
* Move shared pcs functionality to internal library in solidity and
small refactorings in sumcheck
([#11230](#11230))
([507ae9d](507ae9d))
</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 Jan 18, 2025
🤖 I have created a release *beep* *boop*
---


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

##
[0.71.0](AztecProtocol/aztec-packages@aztec-package-v0.70.0...aztec-package-v0.71.0)
(2025-01-17)


### Features

* Track block building helpers
([#11190](AztecProtocol/aztec-packages#11190))
([a749dc1](AztecProtocol/aztec-packages@a749dc1)),
closes
[#11184](AztecProtocol/aztec-packages#11184)
</details>

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

##
[0.71.0](AztecProtocol/aztec-packages@barretenberg.js-v0.70.0...barretenberg.js-v0.71.0)
(2025-01-17)


### Miscellaneous

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

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

##
[0.71.0](AztecProtocol/aztec-packages@aztec-packages-v0.70.0...aztec-packages-v0.71.0)
(2025-01-17)


### ⚠ BREAKING CHANGES

* `loop` statements (only frontend)
(noir-lang/noir#7092)
* Include kind in `StructDefinition::generics` and fix derivation of Eq
in structs with numeric generics
(noir-lang/noir#7076)
* Attestation collection times out based on sequencer timetable
([#11248](AztecProtocol/aztec-packages#11248))

### Features

* `loop` statements (only frontend)
(noir-lang/noir#7092)
([a964cd0](AztecProtocol/aztec-packages@a964cd0))
* Add `ConstrainNotEqual` instruction
(noir-lang/noir#7032)
([a964cd0](AztecProtocol/aztec-packages@a964cd0))
* Archive public testnet tx data
([#11192](AztecProtocol/aztec-packages#11192))
([66f2014](AztecProtocol/aztec-packages@66f2014))
* Backup proof failures to google cloud storage
([#11255](AztecProtocol/aztec-packages#11255))
([b4775fd](AztecProtocol/aztec-packages@b4775fd)),
closes
[#11062](AztecProtocol/aztec-packages#11062)
* **docs:** Algolia-&gt;typesense
([#11034](AztecProtocol/aztec-packages#11034))
([d254f49](AztecProtocol/aztec-packages@d254f49))
* Improve PXE contract DB capabilities
([#11303](AztecProtocol/aztec-packages#11303))
([fab5570](AztecProtocol/aztec-packages@fab5570))
* **LSP:** Auto-import trait reexport if trait is not visible
(noir-lang/noir#7079)
([a964cd0](AztecProtocol/aztec-packages@a964cd0))
* Process note logs in aztec-nr
([#10651](AztecProtocol/aztec-packages#10651))
([708139d](AztecProtocol/aztec-packages@708139d))
* Reenable constrained config for roots
([#10605](AztecProtocol/aztec-packages#10605))
([a6ebc2e](AztecProtocol/aztec-packages@a6ebc2e))
* **spartan:** Add extra accounts
([#11300](AztecProtocol/aztec-packages#11300))
([7782836](AztecProtocol/aztec-packages@7782836))
* **ssa:** Treat globals as constant in a function's DFG
(noir-lang/noir#7040)
([a964cd0](AztecProtocol/aztec-packages@a964cd0))
* Track block building helpers
([#11190](AztecProtocol/aztec-packages#11190))
([a749dc1](AztecProtocol/aztec-packages@a749dc1)),
closes
[#11184](AztecProtocol/aztec-packages#11184)


### Bug Fixes

* Allow implicit associated types on integer type kinds
(noir-lang/noir#7078)
([a964cd0](AztecProtocol/aztec-packages@a964cd0))
* Do not remove memory blocks used as brillig input
(noir-lang/noir#7073)
([a964cd0](AztecProtocol/aztec-packages@a964cd0))
* Ensure 'docker info' works before preceding
([#11286](AztecProtocol/aztec-packages#11286))
([0b0e81a](AztecProtocol/aztec-packages@0b0e81a))
* Fail in proxy deployment should fail the step
([#11308](AztecProtocol/aztec-packages#11308))
([b780d75](AztecProtocol/aztec-packages@b780d75))
* Faster polling times for archiver and sequencer
([#11262](AztecProtocol/aztec-packages#11262))
([d70511e](AztecProtocol/aztec-packages@d70511e))
* Https://github.com/AztecProtocol/aztec-packages/issues/8939
([66f2014](AztecProtocol/aztec-packages@66f2014))
* Idempotent deploy-l1-contracts with initial validators
([#11284](AztecProtocol/aztec-packages#11284))
([3a3f9c0](AztecProtocol/aztec-packages@3a3f9c0)),
closes
[#11283](AztecProtocol/aztec-packages#11283)
* Include kind in `StructDefinition::generics` and fix derivation of Eq
in structs with numeric generics
(noir-lang/noir#7076)
([a964cd0](AztecProtocol/aztec-packages@a964cd0))
* Legacy runner start
([#11291](AztecProtocol/aztec-packages#11291))
([0b2a619](AztecProtocol/aztec-packages@0b2a619))
* Reallocate commitment key to avoid pippenger error
([#11249](AztecProtocol/aztec-packages#11249))
([8fc2011](AztecProtocol/aztec-packages@8fc2011))
* References to a3 in docs
([#11256](AztecProtocol/aztec-packages#11256))
([caf88fa](AztecProtocol/aztec-packages@caf88fa))
* Remove bb path override in cli-wallet
([#11280](AztecProtocol/aztec-packages#11280))
([a6a226e](AztecProtocol/aztec-packages@a6a226e))
* Resolve misc bugs handling phases in avm witgen
([#11218](AztecProtocol/aztec-packages#11218))
([29bc4bd](AztecProtocol/aztec-packages@29bc4bd))
* Sequencer timetable accounts for spare time
([#11221](AztecProtocol/aztec-packages#11221))
([f1b9211](AztecProtocol/aztec-packages@f1b9211))
* Validator ignores block limits during reexec
([#11288](AztecProtocol/aztec-packages#11288))
([920a521](AztecProtocol/aztec-packages@920a521))


### Miscellaneous

* Add circuit input checks to bootstrap.sh
([#11261](AztecProtocol/aztec-packages#11261))
([a718b15](AztecProtocol/aztec-packages@a718b15))
* Add regression test for
[#6530](AztecProtocol/aztec-packages#6530)
(noir-lang/noir#7089)
([a964cd0](AztecProtocol/aztec-packages@a964cd0))
* Add test for isuee
[#7090](AztecProtocol/aztec-packages#7090)
(noir-lang/noir#7091)
([a964cd0](AztecProtocol/aztec-packages@a964cd0))
* Allow passing custom conditions to inlining pass
(noir-lang/noir#7083)
([a964cd0](AztecProtocol/aztec-packages@a964cd0))
* Attestation collection times out based on sequencer timetable
([#11248](AztecProtocol/aztec-packages#11248))
([946a418](AztecProtocol/aztec-packages@946a418))
* **avm:** Calldata, returndata slices out of range padded with zeros
([#11265](AztecProtocol/aztec-packages#11265))
([a469c94](AztecProtocol/aztec-packages@a469c94)),
closes
[#10933](AztecProtocol/aztec-packages#10933)
* Delete external-ci-approved.yml
([#11258](AztecProtocol/aztec-packages#11258))
([642bce6](AztecProtocol/aztec-packages@642bce6))
* Demote error closing forks to warn
([#11263](AztecProtocol/aztec-packages#11263))
([a5b7a6a](AztecProtocol/aztec-packages@a5b7a6a))
* Do not make new instruction if it hasn't changed
(noir-lang/noir#7069)
([a964cd0](AztecProtocol/aztec-packages@a964cd0))
* Ensure devnet has unproven config
([#11302](AztecProtocol/aztec-packages#11302))
([085f782](AztecProtocol/aztec-packages@085f782))
* Fixing `[@safety](https://github.com/safety)` warnings
([#11094](AztecProtocol/aztec-packages#11094))
([5de24e0](AztecProtocol/aztec-packages@5de24e0))
* Log correlation in traces in google cloud
([#11276](AztecProtocol/aztec-packages#11276))
([fbcc8ef](AztecProtocol/aztec-packages@fbcc8ef)),
closes
[#11019](AztecProtocol/aztec-packages#11019)
[#10937](AztecProtocol/aztec-packages#10937)
* Mark `noir-edwards` as expected to compile
(noir-lang/noir#7085)
([a964cd0](AztecProtocol/aztec-packages@a964cd0))
* Move shared pcs functionality to internal library in solidity and
small refactorings in sumcheck
([#11230](AztecProtocol/aztec-packages#11230))
([507ae9d](AztecProtocol/aztec-packages@507ae9d))
* Reduce the number of provers in rc-1
([#11296](AztecProtocol/aztec-packages#11296))
([92e40ff](AztecProtocol/aztec-packages@92e40ff))
* Remove references to padding txs
([#11264](AztecProtocol/aztec-packages#11264))
([32408f6](AztecProtocol/aztec-packages@32408f6))
* Remove warnings from types and rollup lib crates
([#11269](AztecProtocol/aztec-packages#11269))
([9f389a7](AztecProtocol/aztec-packages@9f389a7))
* Replace relative paths to noir-protocol-circuits
([8ece166](AztecProtocol/aztec-packages@8ece166))
* Replace relative paths to noir-protocol-circuits
([be42305](AztecProtocol/aztec-packages@be42305))
* Retry deploys
([#11252](AztecProtocol/aztec-packages#11252))
([23cfbb4](AztecProtocol/aztec-packages@23cfbb4))
* Set failed proof store for spartan deployments
([#11282](AztecProtocol/aztec-packages#11282))
([f787a52](AztecProtocol/aztec-packages@f787a52))
* Silence "Updated proven chain" log
([#11250](AztecProtocol/aztec-packages#11250))
([44bd79b](AztecProtocol/aztec-packages@44bd79b))
* Silence circuit return values in CI
([#11259](AztecProtocol/aztec-packages#11259))
([db3d860](AztecProtocol/aztec-packages@db3d860))
* Stable masternet images
([#11289](AztecProtocol/aztec-packages#11289))
([07fabe8](AztecProtocol/aztec-packages@07fabe8))
</details>

<details><summary>barretenberg: 0.71.0</summary>

##
[0.71.0](AztecProtocol/aztec-packages@barretenberg-v0.70.0...barretenberg-v0.71.0)
(2025-01-17)


### Bug Fixes

* Reallocate commitment key to avoid pippenger error
([#11249](AztecProtocol/aztec-packages#11249))
([8fc2011](AztecProtocol/aztec-packages@8fc2011))
* Resolve misc bugs handling phases in avm witgen
([#11218](AztecProtocol/aztec-packages#11218))
([29bc4bd](AztecProtocol/aztec-packages@29bc4bd))


### Miscellaneous

* **avm:** Calldata, returndata slices out of range padded with zeros
([#11265](AztecProtocol/aztec-packages#11265))
([a469c94](AztecProtocol/aztec-packages@a469c94)),
closes
[#10933](AztecProtocol/aztec-packages#10933)
* Move shared pcs functionality to internal library in solidity and
small refactorings in sumcheck
([#11230](AztecProtocol/aztec-packages#11230))
([507ae9d](AztecProtocol/aztec-packages@507ae9d))
</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