-
Notifications
You must be signed in to change notification settings - Fork 236
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: track side effects in public #5129
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 processing duration by data writes.
|
bdced68
to
e48a48a
Compare
Changes to circuit sizes
🧾 Summary (100% most significant diffs)
Full diff report 👇
|
f264ef7
to
6c982a1
Compare
114c536
to
1146e5d
Compare
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.
Changes seem okay, but I'm definitely a bit hazy on the interplay among the different counters. Docs here (or in the code) would be really helpful.
noir-projects/noir-protocol-circuits/crates/types/src/tests/public_call_data_builder.nr
Show resolved
Hide resolved
let max = 0; | ||
for (const sideEffect of sideEffectCounters) { | ||
if (sideEffect instanceof CallRequest) { | ||
// look at both start and end counters because for enqueued public calls start > 0 while 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.
Can you link an issue for fixing end === 0
? I believe its what you're referring to in future work in your comment in the PR.
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.
Oh the comment isn't that great. What I'm saying is that public functions enqueued from private become CallRequests where start_side_effect_counter
is the time that they were enqueued and end_side_effect_counter
is 0 (they haven't run) while public calls in public execution are fully formed.
@@ -10,5 +10,6 @@ struct PrivateContextInputs { | |||
call_context : CallContext, | |||
historical_header: Header, | |||
private_global_variables: PrivateGlobalVariables, | |||
start_side_effect_counter: u32, |
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 did we need to add this here instead of continuing to use the one in CallContext
? I'm struggling a bit with how they are different, and why the one in CallContext
got renamed to just side_effect_counter
.
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.
@LeilaWang came with the suggestion to remove it from CallContext
entirely, which I've attempted to do in #5177
Why did we need to add this here instead of continuing to use the one in CallContext?
The value in call_context
is either the start_side_effect_counter
for private calls and public calls in public execution, while it is the enqueued_side_effect_counter
for enqueued public calls.
The real start_side_effect_counter
is the one returned by context.finish()
and for enqueued calls it will be different than what's in call_context
(I enqueue it at 5 but run the function at 20). Because of the hashing that goes on in the kernel I can't modify call_context.side_effect_counter
for enqueued calls.
In #5177 we remove call_context.side_effect_counter
and rely on public inputs.
1146e5d
to
6331507
Compare
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.
🚀
…5156) This PR fixes an issue where the phase manager was incorrectly extracting setup and teardown enqueued public function calls when there was no app logic in between them. Fix #5150 Stacked on top of #5129 Co-authored-by: Mitchell Tracy <[email protected]>
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.28.0</summary> ## [0.28.0](aztec-package-v0.27.2...aztec-package-v0.28.0) (2024-03-14) ### ⚠ BREAKING CHANGES * Support contracts with no constructor ([#5175](#5175)) ### Features * Support contracts with no constructor ([#5175](#5175)) ([df7fa32](df7fa32)) </details> <details><summary>barretenberg.js: 0.28.0</summary> ## [0.28.0](barretenberg.js-v0.27.2...barretenberg.js-v0.28.0) (2024-03-14) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-cli: 0.28.0</summary> ## [0.28.0](aztec-cli-v0.27.2...aztec-cli-v0.28.0) (2024-03-14) ### ⚠ BREAKING CHANGES * Support contracts with no constructor ([#5175](#5175)) ### Features * Support contracts with no constructor ([#5175](#5175)) ([df7fa32](df7fa32)) </details> <details><summary>aztec-packages: 0.28.0</summary> ## [0.28.0](aztec-packages-v0.27.2...aztec-packages-v0.28.0) (2024-03-14) ### ⚠ BREAKING CHANGES * Support contracts with no constructor ([#5175](#5175)) ### Features * **avm-simulator:** Euclidean and field div ([#5181](#5181)) ([037a38f](037a38f)) * Isolate Plonk dependencies ([#5068](#5068)) ([5cbbd7d](5cbbd7d)) * New brillig field operations and refactor of binary operations ([#5208](#5208)) ([eb69504](eb69504)) * Parallelize linearly dependent contribution in PG ([#4742](#4742)) ([d1799ae](d1799ae)) * Parity circuits ([#5082](#5082)) ([335c46e](335c46e)) * Support contracts with no constructor ([#5175](#5175)) ([df7fa32](df7fa32)) * Track side effects in public ([#5129](#5129)) ([d666f6f](d666f6f)), closes [#5185](#5185) * Update SMT Circuit class and add gate relaxation functionality ([#5176](#5176)) ([5948996](5948996)) ### Bug Fixes * **avm-transpiler:** FDIV and U128 test case ([#5200](#5200)) ([6977e81](6977e81)) * Barretenberg-acir-tests-bb.js thru version bump ([#5216](#5216)) ([9298f93](9298f93)) * Do not release docs on every commit to master ([#5214](#5214)) ([c34a299](c34a299)) * Fail transaction if we revert in setup or teardown ([#5093](#5093)) ([db9a960](db9a960)) * Intermittent invert 0 in Goblin ([#5189](#5189)) ([6c70624](6c70624)) * Point docs links to current tag if available ([#5219](#5219)) ([0e9c7c7](0e9c7c7)) * Remove embedded srs ([#5173](#5173)) ([cfd673d](cfd673d)) * Split setup/teardown functions when there's no public app logic ([#5156](#5156)) ([2ee13b3](2ee13b3)) * Validate EthAddress size in aztec-nr ([#5198](#5198)) ([201c5e1](201c5e1)) ### Miscellaneous * Add dependency instructions to bberg README ([#5187](#5187)) ([850febc](850febc)) * **avm-simulator:** Make sure we support Map storage ([#5207](#5207)) ([08835f9](08835f9)) * **avm-simulator:** Restructure contract storage tests ([#5194](#5194)) ([fcdd1cc](fcdd1cc)) * **docs:** Add details to getting started contract deployment ([#5220](#5220)) ([5c267ae](5c267ae)) * Moving wit comms and witness and comm labels from instance to oink ([#5199](#5199)) ([19eb7f9](19eb7f9)) * Oink ([#5210](#5210)) ([321f149](321f149)) * Pull noir ([#5193](#5193)) ([aa90f6e](aa90f6e)) * Trying to fix intermitent ci failure for boxes ([#5182](#5182)) ([f988cb8](f988cb8)) ### Documentation * **yellow-paper:** Add pseudocode for verifying broadcasted functions in contract deployment ([#4431](#4431)) ([8bdb921](8bdb921)) </details> <details><summary>barretenberg: 0.28.0</summary> ## [0.28.0](barretenberg-v0.27.2...barretenberg-v0.28.0) (2024-03-14) ### Features * **avm-simulator:** Euclidean and field div ([#5181](#5181)) ([037a38f](037a38f)) * Isolate Plonk dependencies ([#5068](#5068)) ([5cbbd7d](5cbbd7d)) * New brillig field operations and refactor of binary operations ([#5208](#5208)) ([eb69504](eb69504)) * Parallelize linearly dependent contribution in PG ([#4742](#4742)) ([d1799ae](d1799ae)) * Update SMT Circuit class and add gate relaxation functionality ([#5176](#5176)) ([5948996](5948996)) ### Bug Fixes * Barretenberg-acir-tests-bb.js thru version bump ([#5216](#5216)) ([9298f93](9298f93)) * Intermittent invert 0 in Goblin ([#5189](#5189)) ([6c70624](6c70624)) * Remove embedded srs ([#5173](#5173)) ([cfd673d](cfd673d)) ### Miscellaneous * Add dependency instructions to bberg README ([#5187](#5187)) ([850febc](850febc)) * Moving wit comms and witness and comm labels from instance to oink ([#5199](#5199)) ([19eb7f9](19eb7f9)) * Oink ([#5210](#5210)) ([321f149](321f149)) </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.28.0</summary> ## [0.28.0](AztecProtocol/aztec-packages@aztec-package-v0.27.2...aztec-package-v0.28.0) (2024-03-14) ### ⚠ BREAKING CHANGES * Support contracts with no constructor ([#5175](AztecProtocol/aztec-packages#5175)) ### Features * Support contracts with no constructor ([#5175](AztecProtocol/aztec-packages#5175)) ([df7fa32](AztecProtocol/aztec-packages@df7fa32)) </details> <details><summary>barretenberg.js: 0.28.0</summary> ## [0.28.0](AztecProtocol/aztec-packages@barretenberg.js-v0.27.2...barretenberg.js-v0.28.0) (2024-03-14) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-cli: 0.28.0</summary> ## [0.28.0](AztecProtocol/aztec-packages@aztec-cli-v0.27.2...aztec-cli-v0.28.0) (2024-03-14) ### ⚠ BREAKING CHANGES * Support contracts with no constructor ([#5175](AztecProtocol/aztec-packages#5175)) ### Features * Support contracts with no constructor ([#5175](AztecProtocol/aztec-packages#5175)) ([df7fa32](AztecProtocol/aztec-packages@df7fa32)) </details> <details><summary>aztec-packages: 0.28.0</summary> ## [0.28.0](AztecProtocol/aztec-packages@aztec-packages-v0.27.2...aztec-packages-v0.28.0) (2024-03-14) ### ⚠ BREAKING CHANGES * Support contracts with no constructor ([#5175](AztecProtocol/aztec-packages#5175)) ### Features * **avm-simulator:** Euclidean and field div ([#5181](AztecProtocol/aztec-packages#5181)) ([037a38f](AztecProtocol/aztec-packages@037a38f)) * Isolate Plonk dependencies ([#5068](AztecProtocol/aztec-packages#5068)) ([5cbbd7d](AztecProtocol/aztec-packages@5cbbd7d)) * New brillig field operations and refactor of binary operations ([#5208](AztecProtocol/aztec-packages#5208)) ([eb69504](AztecProtocol/aztec-packages@eb69504)) * Parallelize linearly dependent contribution in PG ([#4742](AztecProtocol/aztec-packages#4742)) ([d1799ae](AztecProtocol/aztec-packages@d1799ae)) * Parity circuits ([#5082](AztecProtocol/aztec-packages#5082)) ([335c46e](AztecProtocol/aztec-packages@335c46e)) * Support contracts with no constructor ([#5175](AztecProtocol/aztec-packages#5175)) ([df7fa32](AztecProtocol/aztec-packages@df7fa32)) * Track side effects in public ([#5129](AztecProtocol/aztec-packages#5129)) ([d666f6f](AztecProtocol/aztec-packages@d666f6f)), closes [#5185](AztecProtocol/aztec-packages#5185) * Update SMT Circuit class and add gate relaxation functionality ([#5176](AztecProtocol/aztec-packages#5176)) ([5948996](AztecProtocol/aztec-packages@5948996)) ### Bug Fixes * **avm-transpiler:** FDIV and U128 test case ([#5200](AztecProtocol/aztec-packages#5200)) ([6977e81](AztecProtocol/aztec-packages@6977e81)) * Barretenberg-acir-tests-bb.js thru version bump ([#5216](AztecProtocol/aztec-packages#5216)) ([9298f93](AztecProtocol/aztec-packages@9298f93)) * Do not release docs on every commit to master ([#5214](AztecProtocol/aztec-packages#5214)) ([c34a299](AztecProtocol/aztec-packages@c34a299)) * Fail transaction if we revert in setup or teardown ([#5093](AztecProtocol/aztec-packages#5093)) ([db9a960](AztecProtocol/aztec-packages@db9a960)) * Intermittent invert 0 in Goblin ([#5189](AztecProtocol/aztec-packages#5189)) ([6c70624](AztecProtocol/aztec-packages@6c70624)) * Point docs links to current tag if available ([#5219](AztecProtocol/aztec-packages#5219)) ([0e9c7c7](AztecProtocol/aztec-packages@0e9c7c7)) * Remove embedded srs ([#5173](AztecProtocol/aztec-packages#5173)) ([cfd673d](AztecProtocol/aztec-packages@cfd673d)) * Split setup/teardown functions when there's no public app logic ([#5156](AztecProtocol/aztec-packages#5156)) ([2ee13b3](AztecProtocol/aztec-packages@2ee13b3)) * Validate EthAddress size in aztec-nr ([#5198](AztecProtocol/aztec-packages#5198)) ([201c5e1](AztecProtocol/aztec-packages@201c5e1)) ### Miscellaneous * Add dependency instructions to bberg README ([#5187](AztecProtocol/aztec-packages#5187)) ([850febc](AztecProtocol/aztec-packages@850febc)) * **avm-simulator:** Make sure we support Map storage ([#5207](AztecProtocol/aztec-packages#5207)) ([08835f9](AztecProtocol/aztec-packages@08835f9)) * **avm-simulator:** Restructure contract storage tests ([#5194](AztecProtocol/aztec-packages#5194)) ([fcdd1cc](AztecProtocol/aztec-packages@fcdd1cc)) * **docs:** Add details to getting started contract deployment ([#5220](AztecProtocol/aztec-packages#5220)) ([5c267ae](AztecProtocol/aztec-packages@5c267ae)) * Moving wit comms and witness and comm labels from instance to oink ([#5199](AztecProtocol/aztec-packages#5199)) ([19eb7f9](AztecProtocol/aztec-packages@19eb7f9)) * Oink ([#5210](AztecProtocol/aztec-packages#5210)) ([321f149](AztecProtocol/aztec-packages@321f149)) * Pull noir ([#5193](AztecProtocol/aztec-packages#5193)) ([aa90f6e](AztecProtocol/aztec-packages@aa90f6e)) * Trying to fix intermitent ci failure for boxes ([#5182](AztecProtocol/aztec-packages#5182)) ([f988cb8](AztecProtocol/aztec-packages@f988cb8)) ### Documentation * **yellow-paper:** Add pseudocode for verifying broadcasted functions in contract deployment ([#4431](AztecProtocol/aztec-packages#4431)) ([8bdb921](AztecProtocol/aztec-packages@8bdb921)) </details> <details><summary>barretenberg: 0.28.0</summary> ## [0.28.0](AztecProtocol/aztec-packages@barretenberg-v0.27.2...barretenberg-v0.28.0) (2024-03-14) ### Features * **avm-simulator:** Euclidean and field div ([#5181](AztecProtocol/aztec-packages#5181)) ([037a38f](AztecProtocol/aztec-packages@037a38f)) * Isolate Plonk dependencies ([#5068](AztecProtocol/aztec-packages#5068)) ([5cbbd7d](AztecProtocol/aztec-packages@5cbbd7d)) * New brillig field operations and refactor of binary operations ([#5208](AztecProtocol/aztec-packages#5208)) ([eb69504](AztecProtocol/aztec-packages@eb69504)) * Parallelize linearly dependent contribution in PG ([#4742](AztecProtocol/aztec-packages#4742)) ([d1799ae](AztecProtocol/aztec-packages@d1799ae)) * Update SMT Circuit class and add gate relaxation functionality ([#5176](AztecProtocol/aztec-packages#5176)) ([5948996](AztecProtocol/aztec-packages@5948996)) ### Bug Fixes * Barretenberg-acir-tests-bb.js thru version bump ([#5216](AztecProtocol/aztec-packages#5216)) ([9298f93](AztecProtocol/aztec-packages@9298f93)) * Intermittent invert 0 in Goblin ([#5189](AztecProtocol/aztec-packages#5189)) ([6c70624](AztecProtocol/aztec-packages@6c70624)) * Remove embedded srs ([#5173](AztecProtocol/aztec-packages#5173)) ([cfd673d](AztecProtocol/aztec-packages@cfd673d)) ### Miscellaneous * Add dependency instructions to bberg README ([#5187](AztecProtocol/aztec-packages#5187)) ([850febc](AztecProtocol/aztec-packages@850febc)) * Moving wit comms and witness and comm labels from instance to oink ([#5199](AztecProtocol/aztec-packages#5199)) ([19eb7f9](AztecProtocol/aztec-packages@19eb7f9)) * Oink ([#5210](AztecProtocol/aztec-packages#5210)) ([321f149](AztecProtocol/aztec-packages@321f149)) </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.28.0</summary> ## [0.28.0](aztec-package-v0.27.2...aztec-package-v0.28.0) (2024-03-14) ### ⚠ BREAKING CHANGES * Support contracts with no constructor ([#5175](#5175)) ### Features * Support contracts with no constructor ([#5175](#5175)) ([df7fa32](df7fa32)) </details> <details><summary>barretenberg.js: 0.28.0</summary> ## [0.28.0](barretenberg.js-v0.27.2...barretenberg.js-v0.28.0) (2024-03-14) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-cli: 0.28.0</summary> ## [0.28.0](aztec-cli-v0.27.2...aztec-cli-v0.28.0) (2024-03-14) ### ⚠ BREAKING CHANGES * Support contracts with no constructor ([#5175](#5175)) ### Features * Support contracts with no constructor ([#5175](#5175)) ([df7fa32](df7fa32)) </details> <details><summary>aztec-packages: 0.28.0</summary> ## [0.28.0](aztec-packages-v0.27.2...aztec-packages-v0.28.0) (2024-03-14) ### ⚠ BREAKING CHANGES * Support contracts with no constructor ([#5175](#5175)) ### Features * **avm-simulator:** Euclidean and field div ([#5181](#5181)) ([037a38f](037a38f)) * Isolate Plonk dependencies ([#5068](#5068)) ([5cbbd7d](5cbbd7d)) * New brillig field operations and refactor of binary operations ([#5208](#5208)) ([eb69504](eb69504)) * Parallelize linearly dependent contribution in PG ([#4742](#4742)) ([d1799ae](d1799ae)) * Parity circuits ([#5082](#5082)) ([335c46e](335c46e)) * Support contracts with no constructor ([#5175](#5175)) ([df7fa32](df7fa32)) * Track side effects in public ([#5129](#5129)) ([d666f6f](d666f6f)), closes [#5185](#5185) * Update SMT Circuit class and add gate relaxation functionality ([#5176](#5176)) ([5948996](5948996)) ### Bug Fixes * **avm-transpiler:** FDIV and U128 test case ([#5200](#5200)) ([6977e81](6977e81)) * Barretenberg-acir-tests-bb.js thru version bump ([#5216](#5216)) ([9298f93](9298f93)) * Do not release docs on every commit to master ([#5214](#5214)) ([c34a299](c34a299)) * Fail transaction if we revert in setup or teardown ([#5093](#5093)) ([db9a960](db9a960)) * Intermittent invert 0 in Goblin ([#5189](#5189)) ([6c70624](6c70624)) * Point docs links to current tag if available ([#5219](#5219)) ([0e9c7c7](0e9c7c7)) * Remove embedded srs ([#5173](#5173)) ([cfd673d](cfd673d)) * Split setup/teardown functions when there's no public app logic ([#5156](#5156)) ([2ee13b3](2ee13b3)) * Validate EthAddress size in aztec-nr ([#5198](#5198)) ([201c5e1](201c5e1)) ### Miscellaneous * Add dependency instructions to bberg README ([#5187](#5187)) ([850febc](850febc)) * **avm-simulator:** Make sure we support Map storage ([#5207](#5207)) ([08835f9](08835f9)) * **avm-simulator:** Restructure contract storage tests ([#5194](#5194)) ([fcdd1cc](fcdd1cc)) * **docs:** Add details to getting started contract deployment ([#5220](#5220)) ([5c267ae](5c267ae)) * Moving wit comms and witness and comm labels from instance to oink ([#5199](#5199)) ([19eb7f9](19eb7f9)) * Oink ([#5210](#5210)) ([321f149](321f149)) * Pull noir ([#5193](#5193)) ([aa90f6e](aa90f6e)) * Trying to fix intermitent ci failure for boxes ([#5182](#5182)) ([f988cb8](f988cb8)) ### Documentation * **yellow-paper:** Add pseudocode for verifying broadcasted functions in contract deployment ([#4431](#4431)) ([8bdb921](8bdb921)) </details> <details><summary>barretenberg: 0.28.0</summary> ## [0.28.0](barretenberg-v0.27.2...barretenberg-v0.28.0) (2024-03-14) ### Features * **avm-simulator:** Euclidean and field div ([#5181](#5181)) ([037a38f](037a38f)) * Isolate Plonk dependencies ([#5068](#5068)) ([5cbbd7d](5cbbd7d)) * New brillig field operations and refactor of binary operations ([#5208](#5208)) ([eb69504](eb69504)) * Parallelize linearly dependent contribution in PG ([#4742](#4742)) ([d1799ae](d1799ae)) * Update SMT Circuit class and add gate relaxation functionality ([#5176](#5176)) ([5948996](5948996)) ### Bug Fixes * Barretenberg-acir-tests-bb.js thru version bump ([#5216](#5216)) ([9298f93](9298f93)) * Intermittent invert 0 in Goblin ([#5189](#5189)) ([6c70624](6c70624)) * Remove embedded srs ([#5173](#5173)) ([cfd673d](cfd673d)) ### Miscellaneous * Add dependency instructions to bberg README ([#5187](#5187)) ([850febc](850febc)) * Moving wit comms and witness and comm labels from instance to oink ([#5199](#5199)) ([19eb7f9](19eb7f9)) * Oink ([#5210](#5210)) ([321f149](321f149)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This PR adds start/end side effect counters to private & public context inputs.
This solves #4958 by assigning the correct side effect counters to notes created in setup/teardown so that when they're recombined into a single array they can be sorted in order of creation. A few e2e tests were also added to validate the change.
Next steps:
context.side_effect_counter
to the called function'send_side_effect_counter + 1
(just like the private context)call_context.side_effect_counter
to justcall_context.counter
Fix #4958
Fix #5185