-
Notifications
You must be signed in to change notification settings - Fork 265
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: basic public reverts #4870
feat: basic public reverts #4870
Conversation
Changes to circuit sizes
🧾 Summary (100% most significant diffs)
Full diff report 👇
|
Benchmark resultsMetrics with a significant change:
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 contracts are deployed in the tx.
Transaction processing duration by data writes.
|
63da191
to
cd1d7e9
Compare
12b6f33
to
f7664c8
Compare
Docs PreviewHey there! 👋 You can check your preview at https://65e9e4c4a034df2910c7f505--aztec-docs-dev.netlify.app |
ca65093
to
f05aa53
Compare
870dca0
to
135449a
Compare
834c8af
to
775f80c
Compare
Looks fine to me. But I think @LeilaWang or @sirasistant should have a quick look over the protocol circuit stuff. |
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.
Looks great! Love the use of utils.inspect
, it will make debug go a lot smoother. Just one question related to reverts in setup/teardown
// halt immediately if the public kernel circuit has reverted. | ||
// return no logs, as they should not go on-chain. | ||
return [kernelOutput, kernelProof, [], result.revertReason]; | ||
} |
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 possible I missed, but I'm just wondering, should there be an else if (kernelOutput.reverted) throw new Error
to handle reverts in the non-revertible phases?
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.
Yes! I created #5038 to track this.
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.
A question and a small issue. LGTM overall!
export function assertRightPadded<T>(arr: T[], emptyElt: T) { | ||
let seenEmpty = false; | ||
for (let i = 0; i < arr.length; i++) { | ||
if (arr[i] === emptyElt) { |
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.
If it's an object it might always be false.
@@ -26,6 +26,7 @@ impl PublicKernelTeardownCircuitPrivateInputs { | |||
fn public_kernel_teardown(self) -> PublicKernelCircuitPublicInputs { | |||
// construct the circuit outputs | |||
let mut public_inputs: PublicKernelCircuitPublicInputsBuilder = unsafe::zeroed(); | |||
common::initialize_reverted_flag(self.previous_kernel, self.public_call, &mut public_inputs); |
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.
Should setup
and teardown
check that the public_call
does not revert?
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.
tx.unencryptedLogs || new TxL2Logs([]), | ||
), | ||
); | ||
const txEffects: TxEffect[] = txs.map(tx => toTxEffect(tx)); |
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.
Nice cleanup!
@@ -148,6 +162,21 @@ export class TxEffect { | |||
return this.toBuffer().toString('hex'); | |||
} | |||
|
|||
[inspect.custom]() { | |||
// print out the non-empty fields |
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.
Yay!
fd51297
to
ab21b51
Compare
ab21b51
to
dd05b6e
Compare
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.26.6</summary> ## [0.26.6](aztec-package-v0.26.5...aztec-package-v0.26.6) (2024-03-08) ### Miscellaneous * **aztec-package:** Synchronize aztec-packages versions </details> <details><summary>barretenberg.js: 0.26.6</summary> ## [0.26.6](barretenberg.js-v0.26.5...barretenberg.js-v0.26.6) (2024-03-08) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-cli: 0.26.6</summary> ## [0.26.6](aztec-cli-v0.26.5...aztec-cli-v0.26.6) (2024-03-08) ### Features * Show bytecode size per function in CLI inspect-contract ([#5059](#5059)) ([cb9fdc6](cb9fdc6)) </details> <details><summary>aztec-packages: 0.26.6</summary> ## [0.26.6](aztec-packages-v0.26.5...aztec-packages-v0.26.6) (2024-03-08) ### Features * Basic public reverts ([#4870](#4870)) ([5cccc78](5cccc78)) * Deploying new inbox ([#5036](#5036)) ([fed729d](fed729d)) * Detect unknown note type ids in compute_note_hash ([#5086](#5086)) ([6206bec](6206bec)) * Easy deployment of protocol contracts in e2e ([#4983](#4983)) ([480161f](480161f)) * IPA documentation ([#4924](#4924)) ([48bd22e](48bd22e)) * Nullifier read requests in public kernel ([#4910](#4910)) ([0e44247](0e44247)) * Show bytecode size per function in CLI inspect-contract ([#5059](#5059)) ([cb9fdc6](cb9fdc6)) * Updating an SMT solver class ([#4981](#4981)) ([4b94d58](4b94d58)) ### Bug Fixes * Canonical contract address ([#5030](#5030)) ([b2af880](b2af880)) * Flaky deployment test ([#5035](#5035)) ([039eafc](039eafc)) * Pull the correct platform image for noir ([#5097](#5097)) ([3342371](3342371)) * Sleep function memory leak ([#5023](#5023)) ([a72cfea](a72cfea)), closes [#4817](#4817) * Storage v2 ([#5027](#5027)) ([fe3190e](fe3190e)) * Update protogalaxy cmake dependencies ([#5066](#5066)) ([507c374](507c374)) ### Miscellaneous * Address warnings in noir test suite ([#4966](#4966)) ([7ef4ef5](7ef4ef5)) * Bootstrap noir natively if nargo is invalid ([#5034](#5034)) ([df089de](df089de)) * Build avm transpiler if we are on mac ([#5039](#5039)) ([c2966b9](c2966b9)) * **ci:** Re-enable certain bb solidity ACIR tests ([#5065](#5065)) ([58e1ff4](58e1ff4)) * Cleanup of prover and verifier instances ([#4959](#4959)) ([f2fdefd](f2fdefd)) * Delete bootstrap scripts from `noir/noir-repo` ([#5044](#5044)) ([add91ca](add91ca)) * Disable `hello_world_example` noir test in aztec-packages CI ([#5061](#5061)) ([1be9243](1be9243)) * Join-split example Part 1 ([#4965](#4965)) ([b9de0f5](b9de0f5)) * Moving RootRollupInputs impl ([#5087](#5087)) ([f3d9f9b](f3d9f9b)) * Remove eccvm functionality to update the op queue and ensure ultra ops are populated through function ([#5084](#5084)) ([77954ab](77954ab)) ### Documentation * Parity circuit naming fixes ([#5076](#5076)) ([c255255](c255255)) </details> <details><summary>barretenberg: 0.26.6</summary> ## [0.26.6](barretenberg-v0.26.5...barretenberg-v0.26.6) (2024-03-08) ### Features * IPA documentation ([#4924](#4924)) ([48bd22e](48bd22e)) * Updating an SMT solver class ([#4981](#4981)) ([4b94d58](4b94d58)) ### Bug Fixes * Storage v2 ([#5027](#5027)) ([fe3190e](fe3190e)) * Update protogalaxy cmake dependencies ([#5066](#5066)) ([507c374](507c374)) ### Miscellaneous * **ci:** Re-enable certain bb solidity ACIR tests ([#5065](#5065)) ([58e1ff4](58e1ff4)) * Cleanup of prover and verifier instances ([#4959](#4959)) ([f2fdefd](f2fdefd)) * Join-split example Part 1 ([#4965](#4965)) ([b9de0f5](b9de0f5)) * Remove eccvm functionality to update the op queue and ensure ultra ops are populated through function ([#5084](#5084)) ([77954ab](77954ab)) </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.26.6</summary> ## [0.26.6](AztecProtocol/aztec-packages@aztec-package-v0.26.5...aztec-package-v0.26.6) (2024-03-08) ### Miscellaneous * **aztec-package:** Synchronize aztec-packages versions </details> <details><summary>barretenberg.js: 0.26.6</summary> ## [0.26.6](AztecProtocol/aztec-packages@barretenberg.js-v0.26.5...barretenberg.js-v0.26.6) (2024-03-08) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-cli: 0.26.6</summary> ## [0.26.6](AztecProtocol/aztec-packages@aztec-cli-v0.26.5...aztec-cli-v0.26.6) (2024-03-08) ### Features * Show bytecode size per function in CLI inspect-contract ([#5059](AztecProtocol/aztec-packages#5059)) ([cb9fdc6](AztecProtocol/aztec-packages@cb9fdc6)) </details> <details><summary>aztec-packages: 0.26.6</summary> ## [0.26.6](AztecProtocol/aztec-packages@aztec-packages-v0.26.5...aztec-packages-v0.26.6) (2024-03-08) ### Features * Basic public reverts ([#4870](AztecProtocol/aztec-packages#4870)) ([5cccc78](AztecProtocol/aztec-packages@5cccc78)) * Deploying new inbox ([#5036](AztecProtocol/aztec-packages#5036)) ([fed729d](AztecProtocol/aztec-packages@fed729d)) * Detect unknown note type ids in compute_note_hash ([#5086](AztecProtocol/aztec-packages#5086)) ([6206bec](AztecProtocol/aztec-packages@6206bec)) * Easy deployment of protocol contracts in e2e ([#4983](AztecProtocol/aztec-packages#4983)) ([480161f](AztecProtocol/aztec-packages@480161f)) * IPA documentation ([#4924](AztecProtocol/aztec-packages#4924)) ([48bd22e](AztecProtocol/aztec-packages@48bd22e)) * Nullifier read requests in public kernel ([#4910](AztecProtocol/aztec-packages#4910)) ([0e44247](AztecProtocol/aztec-packages@0e44247)) * Show bytecode size per function in CLI inspect-contract ([#5059](AztecProtocol/aztec-packages#5059)) ([cb9fdc6](AztecProtocol/aztec-packages@cb9fdc6)) * Updating an SMT solver class ([#4981](AztecProtocol/aztec-packages#4981)) ([4b94d58](AztecProtocol/aztec-packages@4b94d58)) ### Bug Fixes * Canonical contract address ([#5030](AztecProtocol/aztec-packages#5030)) ([b2af880](AztecProtocol/aztec-packages@b2af880)) * Flaky deployment test ([#5035](AztecProtocol/aztec-packages#5035)) ([039eafc](AztecProtocol/aztec-packages@039eafc)) * Pull the correct platform image for noir ([#5097](AztecProtocol/aztec-packages#5097)) ([3342371](AztecProtocol/aztec-packages@3342371)) * Sleep function memory leak ([#5023](AztecProtocol/aztec-packages#5023)) ([a72cfea](AztecProtocol/aztec-packages@a72cfea)), closes [#4817](AztecProtocol/aztec-packages#4817) * Storage v2 ([#5027](AztecProtocol/aztec-packages#5027)) ([fe3190e](AztecProtocol/aztec-packages@fe3190e)) * Update protogalaxy cmake dependencies ([#5066](AztecProtocol/aztec-packages#5066)) ([507c374](AztecProtocol/aztec-packages@507c374)) ### Miscellaneous * Address warnings in noir test suite ([#4966](AztecProtocol/aztec-packages#4966)) ([7ef4ef5](AztecProtocol/aztec-packages@7ef4ef5)) * Bootstrap noir natively if nargo is invalid ([#5034](AztecProtocol/aztec-packages#5034)) ([df089de](AztecProtocol/aztec-packages@df089de)) * Build avm transpiler if we are on mac ([#5039](AztecProtocol/aztec-packages#5039)) ([c2966b9](AztecProtocol/aztec-packages@c2966b9)) * **ci:** Re-enable certain bb solidity ACIR tests ([#5065](AztecProtocol/aztec-packages#5065)) ([58e1ff4](AztecProtocol/aztec-packages@58e1ff4)) * Cleanup of prover and verifier instances ([#4959](AztecProtocol/aztec-packages#4959)) ([f2fdefd](AztecProtocol/aztec-packages@f2fdefd)) * Delete bootstrap scripts from `noir/noir-repo` ([#5044](AztecProtocol/aztec-packages#5044)) ([add91ca](AztecProtocol/aztec-packages@add91ca)) * Disable `hello_world_example` noir test in aztec-packages CI ([#5061](AztecProtocol/aztec-packages#5061)) ([1be9243](AztecProtocol/aztec-packages@1be9243)) * Join-split example Part 1 ([#4965](AztecProtocol/aztec-packages#4965)) ([b9de0f5](AztecProtocol/aztec-packages@b9de0f5)) * Moving RootRollupInputs impl ([#5087](AztecProtocol/aztec-packages#5087)) ([f3d9f9b](AztecProtocol/aztec-packages@f3d9f9b)) * Remove eccvm functionality to update the op queue and ensure ultra ops are populated through function ([#5084](AztecProtocol/aztec-packages#5084)) ([77954ab](AztecProtocol/aztec-packages@77954ab)) ### Documentation * Parity circuit naming fixes ([#5076](AztecProtocol/aztec-packages#5076)) ([c255255](AztecProtocol/aztec-packages@c255255)) </details> <details><summary>barretenberg: 0.26.6</summary> ## [0.26.6](AztecProtocol/aztec-packages@barretenberg-v0.26.5...barretenberg-v0.26.6) (2024-03-08) ### Features * IPA documentation ([#4924](AztecProtocol/aztec-packages#4924)) ([48bd22e](AztecProtocol/aztec-packages@48bd22e)) * Updating an SMT solver class ([#4981](AztecProtocol/aztec-packages#4981)) ([4b94d58](AztecProtocol/aztec-packages@4b94d58)) ### Bug Fixes * Storage v2 ([#5027](AztecProtocol/aztec-packages#5027)) ([fe3190e](AztecProtocol/aztec-packages@fe3190e)) * Update protogalaxy cmake dependencies ([#5066](AztecProtocol/aztec-packages#5066)) ([507c374](AztecProtocol/aztec-packages@507c374)) ### Miscellaneous * **ci:** Re-enable certain bb solidity ACIR tests ([#5065](AztecProtocol/aztec-packages#5065)) ([58e1ff4](AztecProtocol/aztec-packages@58e1ff4)) * Cleanup of prover and verifier instances ([#4959](AztecProtocol/aztec-packages#4959)) ([f2fdefd](AztecProtocol/aztec-packages@f2fdefd)) * Join-split example Part 1 ([#4965](AztecProtocol/aztec-packages#4965)) ([b9de0f5](AztecProtocol/aztec-packages@b9de0f5)) * Remove eccvm functionality to update the op queue and ensure ultra ops are populated through function ([#5084](AztecProtocol/aztec-packages#5084)) ([77954ab](AztecProtocol/aztec-packages@77954ab)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Fix #4971 , Parent issue is #4096
This PR adds basic support for reversions of public functions.
A public function may be simulated locally before the TX is submitted. In this case, the PXE calls out to the node, requesting public simulation.
When this is done, the node will throw an error if the public function would revert.
However, if the function is called with
skipPublicSimulation
, the TX is submitted, and the sequencer will run the public processor, which catchesSimulationErrors
, and includes them in the rollup.Generally this is accomplished by adding a
reverted
boolean to thePublicCircuitPublicInputs
andPublicKernelCircuitPublicInputs
.It is expected that the AVM will set its
reverted
flag to true. In the meantime, while simulating with the acvm, if it throws, we catch, and set the flag on the output totrue
.There is also a
revertedReason
, which is useful for debugging (e.g. when you simulate public functions locally, you want the node to return a meaningful error to you)The meat of the logic changes are in
public_kernel_app_logic.nr
,abstract_phase_manager.ts
, andapp_logic_phase_manager.ts
.The primary test is in
e2e_fees.test.ts
.In the app logic circuit, we now check to see if we have reverted, and forward the flag if so. Additionally, if we have reverted, we do not propagate forward any revertible side effects, and instead leave them all to zero.
Note further, if we get a simulation error in a nested public execution, we do throw, and allow the parent/top-level call to catch the error and return gracefully.
I also added a bunch of inspect functions, and assertions about hashes for sanity checking.
Future work
Presently if a tx reverts but is included, it is not distinguished from other transactions in the block which did not revert. This will be fixed as part of #4972
Further, to test the flow where we privately pay the fee but then have a revert, we need to first tackle #4712 so that we have "non-revertible" logs (unshield is used in fee prep in this case, which emits encrypted logs, which are presently dropped if the transaction reverts).
Additionally, as mentioned in comments below, we do not yet insist that the non-revertible parts of public execution in fact do not revert. This will be done in #5038.
All of those are tracked as part of the parent issue #4096
We will also want to optimize the public kernel circuits, but I am in favor of adding the bare minimum feature set before we start optimizing.