feat: basic public reverts (#4870)

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
catches `SimulationErrors`, and includes them in the rollup.

Generally this is accomplished by adding a `reverted` boolean to the
`PublicCircuitPublicInputs` and `PublicKernelCircuitPublicInputs`.

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 to `true`.

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`, and `app_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.

.vscode/settings.json

@@ -126,7 +126,9 @@
     "editor.defaultFormatter": "esbenp.prettier-vscode"
   },
   // Now when a new file is pasted in /yellow-paper/docs/readme.md, the image file is created at /yellow-paper/docs/images/readme/image.png.
-  "markdown.copyFiles.destination": {"/yellow-paper/**/*": "images/${documentBaseName}/"},
+  "markdown.copyFiles.destination": {
+    "/yellow-paper/**/*": "images/${documentBaseName}/"
+  },
   ///////////////////////////////////////
   // C++/Circuits settings
   ///////////////////////////////////////
@@ -153,11 +155,9 @@
   "C_Cpp.default.enableConfigurationSquiggles": false,
   "C_Cpp.formatting": "disabled",
   "C_Cpp.vcpkg.enabled": false,
-  "C_Cpp.default.includePath": [
-    "barretenberg/cpp/src"
-  ],
+  "C_Cpp.default.includePath": ["barretenberg/cpp/src"],
   "rust-analyzer.linkedProjects": [
     "noir/noir-repo/Cargo.toml",
     "avm-transpiler/Cargo.toml"
-  ] 
+  ]
 }

docs/docs/developers/tutorials/testing.md

@@ -130,18 +130,16 @@ Keep in mind that public function calls behave as in EVM blockchains, in that th
 
 #### A public call fails on the sequencer
 
-We can ignore a local simulation error for a public function via the `skipPublicSimulation`. This will submit a failing call to the sequencer, who will then reject it and drop the transaction.
+We can ignore a local simulation error for a public function via the `skipPublicSimulation`. This will submit a failing call to the sequencer, who will include the transaction, but without any side effects from our application logic.
 
-#include_code pub-dropped /yarn-project/end-to-end/src/guides/dapp_testing.test.ts typescript
-
-If you run the snippet above, you'll see the following error in the Sandbox logs:
+#include_code pub-reverted /yarn-project/end-to-end/src/guides/dapp_testing.test.ts typescript
 
 ```
 WARN Error processing tx 06dc87c4d64462916ea58426ffcfaf20017880b353c9ec3e0f0ee5fab3ea923f: Assertion failed: Balance too low.
 ```
 
 :::info
-In the near future, transactions where a public function call fails will get mined but flagged as reverted, instead of dropped.
+Presently, the transaction is included, but no additional information is included in the block to mark it as reverted. This will change in the near future.
 :::
 
 ### State

l1-contracts/src/core/libraries/ConstantsGen.sol

@@ -114,7 +114,7 @@ library Constants {
   uint256 internal constant PARTIAL_STATE_REFERENCE_LENGTH = 8;
   uint256 internal constant PRIVATE_CALL_STACK_ITEM_LENGTH = 223;
   uint256 internal constant PRIVATE_CIRCUIT_PUBLIC_INPUTS_LENGTH = 218;
-  uint256 internal constant PUBLIC_CIRCUIT_PUBLIC_INPUTS_LENGTH = 194;
+  uint256 internal constant PUBLIC_CIRCUIT_PUBLIC_INPUTS_LENGTH = 195;
   uint256 internal constant STATE_REFERENCE_LENGTH = 10;
   uint256 internal constant TX_CONTEXT_DATA_LENGTH = 11;
   uint256 internal constant TX_REQUEST_LENGTH = 17;

noir-projects/aztec-nr/aztec/src/context/private_context.nr

@@ -465,7 +465,8 @@ impl PrivateContext {
                 unencrypted_logs_hash: [0; NUM_FIELDS_PER_SHA256],
                 unencrypted_log_preimages_length: 0,
                 historical_header: Header::empty(),
-                prover_address: AztecAddress::zero()
+                prover_address: AztecAddress::zero(),
+                reverted: false
             },
             is_execution_request: true
         };

noir-projects/aztec-nr/aztec/src/context/public_context.nr

@@ -157,7 +157,8 @@ impl PublicContext {
             unencrypted_logs_hash,
             unencrypted_log_preimages_length,
             historical_header: self.inputs.historical_header,
-            prover_address: self.prover_address
+            prover_address: self.prover_address,
+            reverted: false
         };
         pub_circuit_pub_inputs
     }

noir-projects/noir-protocol-circuits/crates/public-kernel-lib/src/common.nr

@@ -39,6 +39,14 @@ pub fn validate_inputs(public_call: PublicCallData) {
     assert(public_call.bytecode_hash != 0, "Bytecode hash cannot be zero");
 }
 
+pub fn initialize_reverted_flag(
+    previous_kernel: PublicKernelData,
+    public_call: PublicCallData,
+    circuit_outputs: &mut PublicKernelCircuitPublicInputsBuilder
+) {
+    circuit_outputs.reverted = previous_kernel.public_inputs.reverted | public_call.call_stack_item.public_inputs.reverted;
+}
+
 pub fn initialize_end_values(
     previous_kernel: PublicKernelData,
     circuit_outputs: &mut PublicKernelCircuitPublicInputsBuilder
@@ -50,21 +58,23 @@ pub fn initialize_end_values(
     // functions within this circuit:
     let start = previous_kernel.public_inputs.end;
 
-    circuit_outputs.end.new_note_hashes = array_to_bounded_vec(start.new_note_hashes);
-    circuit_outputs.end.new_nullifiers = array_to_bounded_vec(start.new_nullifiers);
+    if circuit_outputs.reverted == false {
+        circuit_outputs.end.new_note_hashes = array_to_bounded_vec(start.new_note_hashes);
+        circuit_outputs.end.new_nullifiers = array_to_bounded_vec(start.new_nullifiers);
 
-    circuit_outputs.end.private_call_stack = array_to_bounded_vec(start.private_call_stack);
-    circuit_outputs.end.public_call_stack = array_to_bounded_vec(start.public_call_stack);
-    circuit_outputs.end.new_l2_to_l1_msgs = array_to_bounded_vec(start.new_l2_to_l1_msgs);
+        circuit_outputs.end.private_call_stack = array_to_bounded_vec(start.private_call_stack);
+        circuit_outputs.end.public_call_stack = array_to_bounded_vec(start.public_call_stack);
+        circuit_outputs.end.new_l2_to_l1_msgs = array_to_bounded_vec(start.new_l2_to_l1_msgs);
 
-    circuit_outputs.end.public_data_update_requests = array_to_bounded_vec(start.public_data_update_requests);
-    circuit_outputs.end.public_data_reads = array_to_bounded_vec(start.public_data_reads);
+        circuit_outputs.end.public_data_update_requests = array_to_bounded_vec(start.public_data_update_requests);
+        circuit_outputs.end.public_data_reads = array_to_bounded_vec(start.public_data_reads);
 
-    // Public kernel does not modify encrypted logs values --> we just copy them to output
-    circuit_outputs.end.encrypted_logs_hash = start.encrypted_logs_hash;
-    circuit_outputs.end.encrypted_log_preimages_length = start.encrypted_log_preimages_length;
+        // Public kernel does not modify encrypted logs values --> we just copy them to output
+        circuit_outputs.end.encrypted_logs_hash = start.encrypted_logs_hash;
+        circuit_outputs.end.encrypted_log_preimages_length = start.encrypted_log_preimages_length;
 
-    circuit_outputs.end.new_contracts = array_to_bounded_vec(previous_kernel.public_inputs.end.new_contracts);
+        circuit_outputs.end.new_contracts = array_to_bounded_vec(previous_kernel.public_inputs.end.new_contracts);
+    }
 
     let start_non_revertible = previous_kernel.public_inputs.end_non_revertible;
     circuit_outputs.end_non_revertible.new_note_hashes = array_to_bounded_vec(start_non_revertible.new_note_hashes);

noir-projects/noir-protocol-circuits/crates/public-kernel-lib/src/public_kernel_app_logic.nr

@@ -2,6 +2,7 @@ use dep::types::abis::public_call_data::PublicCallData;
 use dep::types::abis::kernel_data::{PublicKernelData};
 use dep::types::PublicKernelCircuitPublicInputs;
 use dep::types::abis::kernel_circuit_public_inputs::PublicKernelCircuitPublicInputsBuilder;
+use dep::types::utils::arrays::array_to_bounded_vec;
 use crate::common;
 use dep::std::unsafe;
 
@@ -22,6 +23,7 @@ impl PublicKernelAppLogicCircuitPrivateInputs {
     fn public_kernel_app_logic(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);
 
         // initialise the end state with our provided previous kernel state
         common::initialize_end_values(self.previous_kernel, &mut public_inputs);
@@ -32,18 +34,31 @@ impl PublicKernelAppLogicCircuitPrivateInputs {
         // validate the inputs unique to having a previous public kernel
         self.validate_inputs();
 
-        // Pops the item from the call stack and validates it against the current execution.
-        let call_request = public_inputs.end.public_call_stack.pop();
-        common::validate_call_against_request(self.public_call, call_request);
-
-        common::update_public_end_values(self.public_call, &mut public_inputs);
-
-        common::accumulate_unencrypted_logs(
-            self.public_call,
-            self.previous_kernel.public_inputs.end.unencrypted_logs_hash,
-            self.previous_kernel.public_inputs.end.unencrypted_log_preimages_length,
-            &mut public_inputs
-        );
+        if public_inputs.reverted == false {
+            // Pops the item from the call stack and validates it against the current execution.
+            let call_request = public_inputs.end.public_call_stack.pop();
+            common::validate_call_against_request(self.public_call, call_request);
+            common::update_public_end_values(self.public_call, &mut public_inputs);
+            common::accumulate_unencrypted_logs(
+                self.public_call,
+                self.previous_kernel.public_inputs.end.unencrypted_logs_hash,
+                self.previous_kernel.public_inputs.end.unencrypted_log_preimages_length,
+                &mut public_inputs
+            );
+        } else {
+            let mut remaining_calls = array_to_bounded_vec(self.previous_kernel.public_inputs.end.public_call_stack);
+            let reverted_call_request = remaining_calls.pop();
+            // even though we reverted, we still need to make sure the correct call was made
+            // but don't do the full `validate_call_against_request` because 
+            // that makes a bunch of assertions that we don't want to make
+            // e.g. that msg_sender is self in the case of internal.
+            // We don't want to make those checks because we already know we reverted,
+            // and we aren't updating the public end values, so we want this kernel circuit to solve.
+            // So just check that the call request is the same as the one we expected.
+            assert(
+                reverted_call_request.hash == self.public_call.call_stack_item.hash(), "calculated public_kernel_inputs_hash does not match provided public_kernel_inputs_hash at the top of the call stack"
+            );
+        }
 
         public_inputs.to_inner()
     }

noir-projects/noir-protocol-circuits/crates/public-kernel-lib/src/public_kernel_setup.nr

@@ -38,6 +38,7 @@ impl PublicKernelSetupCircuitPrivateInputs {
     fn public_kernel_setup(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);
 
         // initialise the end state with our provided previous kernel state
         common::initialize_end_values(self.previous_kernel, &mut public_inputs);

noir-projects/noir-protocol-circuits/crates/public-kernel-lib/src/public_kernel_teardown.nr

@@ -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);
 
         // initialise the end state with our provided previous kernel state
         common::initialize_end_values(self.previous_kernel, &mut public_inputs);
@@ -42,12 +43,14 @@ impl PublicKernelTeardownCircuitPrivateInputs {
 
         common::update_public_end_non_revertible_values(self.public_call, &mut public_inputs);
 
-        common::accumulate_unencrypted_logs(
-            self.public_call,
-            self.previous_kernel.public_inputs.end.unencrypted_logs_hash,
-            self.previous_kernel.public_inputs.end.unencrypted_log_preimages_length,
-            &mut public_inputs
-        );
+        if public_inputs.reverted == false {
+            common::accumulate_unencrypted_logs(
+                self.public_call,
+                self.previous_kernel.public_inputs.end.unencrypted_logs_hash,
+                self.previous_kernel.public_inputs.end.unencrypted_log_preimages_length,
+                &mut public_inputs
+            );
+        }
 
         let mut output = public_inputs.to_inner();
 

noir-projects/noir-protocol-circuits/crates/types/src/abis/kernel_circuit_public_inputs/public_kernel_circuit_public_inputs.nr

@@ -13,4 +13,5 @@ struct PublicKernelCircuitPublicInputs {
     needs_setup: bool,
     needs_app_logic: bool,
     needs_teardown: bool,
+    reverted: bool,
 }

noir-projects/noir-protocol-circuits/crates/types/src/abis/kernel_circuit_public_inputs/public_kernel_circuit_public_inputs_builder.nr

@@ -11,6 +11,7 @@ struct PublicKernelCircuitPublicInputsBuilder {
     end_non_revertible: AccumulatedNonRevertibleDataBuilder,
     end: AccumulatedRevertibleDataBuilder,
     constants: CombinedConstantData,
+    reverted: bool,
 }
 
 impl PublicKernelCircuitPublicInputsBuilder {
@@ -24,7 +25,8 @@ impl PublicKernelCircuitPublicInputsBuilder {
             constants: self.constants,
             needs_setup: end_non_revertible.needs_setup(),
             needs_app_logic: end.needs_app_logic(),
-            needs_teardown: end_non_revertible.needs_teardown()
+            needs_teardown: end_non_revertible.needs_teardown(),
+            reverted: self.reverted
         }
     }
 }

noir-projects/noir-protocol-circuits/crates/types/src/abis/public_call_stack_item.nr

@@ -69,7 +69,7 @@ mod tests {
         let call_stack_item = PublicCallStackItem { contract_address, public_inputs, is_execution_request: true, function_data };
 
         // Value from public_call_stack_item.test.ts "Computes a callstack item request hash" test
-        assert_eq(call_stack_item.hash(), 0x2812dfeffdb7553fbbdd27c03fbdf61e3aa9bab3209db39f78838508ad892803);
+        assert_eq(call_stack_item.hash(), 0x2f3c7c0a0a0611feaba860e7c1022b18b6a9da1db41e3b4a65e535553e371765);
     }
 
     #[test]
@@ -86,6 +86,6 @@ mod tests {
         let call_stack_item = PublicCallStackItem { contract_address, public_inputs, is_execution_request: false, function_data };
 
         // Value from public_call_stack_item.test.ts "Computes a callstack item hash" test
-        assert_eq(call_stack_item.hash(), 0x1f71c0d6bd03e409df694549b6aa83d706cfe55427152e6ec443ec64fa62d3a0);
+        assert_eq(call_stack_item.hash(), 0x21a57c25d064f611e94bcbd6729cdf7d4194c98e8c58ad4703c6315dfbd7e1d9);
     }
 }

noir-projects/noir-protocol-circuits/crates/types/src/abis/public_circuit_public_inputs.nr

@@ -38,6 +38,8 @@ struct PublicCircuitPublicInputs{
     historical_header: Header,
 
     prover_address: AztecAddress,
+
+    reverted: bool,
 }
 
 impl Eq for PublicCircuitPublicInputs {
@@ -73,6 +75,7 @@ impl Serialize<PUBLIC_CIRCUIT_PUBLIC_INPUTS_LENGTH> for PublicCircuitPublicInput
         fields.push(self.unencrypted_log_preimages_length);
         fields.extend_from_array(self.historical_header.serialize());
         fields.push(self.prover_address.to_field());
+        fields.push(self.reverted as Field);
         fields.storage
     }
 }
@@ -95,6 +98,7 @@ impl Deserialize<PUBLIC_CIRCUIT_PUBLIC_INPUTS_LENGTH> for PublicCircuitPublicInp
             unencrypted_log_preimages_length: reader.read(),
             historical_header: reader.read_struct(Header::deserialize),
             prover_address: reader.read_struct(AztecAddress::deserialize),
+            reverted: reader.read() as bool,
         };
 
         reader.finish();
@@ -122,5 +126,5 @@ fn empty_hash() {
     let hash = inputs.hash();
 
     // Value from public_circuit_public_inputs.test.ts "computes empty item hash" test
-    assert_eq(hash, 0x0d43290c164ebc3d80d4d17f1939482d9d01ad503cebceb8c665d2bd96597a68);
+    assert_eq(hash, 0x022c1c742521fb12c0d40c223b953d6499be7de29d6cc62f80ae141bbf4cd9a3);
 }

noir-projects/noir-protocol-circuits/crates/types/src/constants.nr

@@ -168,7 +168,7 @@ global PRIVATE_CALL_STACK_ITEM_LENGTH: u64 = 223;
 // constant as well PRIVATE_CALL_STACK_ITEM_LENGTH
 global PRIVATE_CIRCUIT_PUBLIC_INPUTS_LENGTH: u64 = 218;
 // Change this ONLY if you have changed the PublicCircuitPublicInputs structure.
-global PUBLIC_CIRCUIT_PUBLIC_INPUTS_LENGTH: u64 = 194;
+global PUBLIC_CIRCUIT_PUBLIC_INPUTS_LENGTH: u64 = 195;
 global STATE_REFERENCE_LENGTH: u64 = 10; // 2 for snap + 8 for partial
 global TX_CONTEXT_DATA_LENGTH: u64 = 11;
 global TX_REQUEST_LENGTH: u64 = 17;

noir-projects/noir-protocol-circuits/crates/types/src/tests/kernel_data_builder.nr

@@ -35,7 +35,8 @@ struct PreviousKernelDataBuilder {
     vk_index: u32,
     vk_path: [Field; VK_TREE_HEIGHT],
     sideffect_counter: u32,
-    min_revertible_side_effect_counter: u32
+    min_revertible_side_effect_counter: u32,
+    reverted: bool
 }
 
 impl PreviousKernelDataBuilder {
@@ -69,7 +70,8 @@ impl PreviousKernelDataBuilder {
             vk_index: 0,
             vk_path: [0; VK_TREE_HEIGHT],
             sideffect_counter: 2,
-            min_revertible_side_effect_counter: 2
+            min_revertible_side_effect_counter: 2,
+            reverted: false
         }
     }
 
@@ -144,6 +146,11 @@ impl PreviousKernelDataBuilder {
         first_nullifier + nullifier_index as Field
     }
 
+    fn get_mock_nullifier_value_non_revertible(self, nullifier_index: u64) -> Field {
+        let first_nullifier = self.end_non_revertible.new_nullifiers.get(0);
+        first_nullifier.value + nullifier_index as Field
+    }
+
     pub fn append_new_nullifiers_from_private(&mut self, num_extra_nullifier: u64) {
         // in private kernel, the nullifiers have not yet been partitioned
         // (that is part of the job of the private kernel tail)
@@ -169,7 +176,7 @@ impl PreviousKernelDataBuilder {
             if i <= num_extra_nullifier {
                 self.end.new_nullifiers.push(
                     SideEffectLinkedToNoteHash {
-                    value: self.get_mock_nullifier_value(index_offset + i),
+                    value: self.get_mock_nullifier_value_non_revertible(index_offset + i),
                     note_hash: 0,
                     counter: self.next_sideffect_counter()
                 }
@@ -292,7 +299,8 @@ impl PreviousKernelDataBuilder {
             constants: CombinedConstantData { historical_header: self.historical_header, tx_context: self.tx_context },
             needs_setup: end_non_revertible.needs_setup(),
             needs_app_logic: end.needs_app_logic(),
-            needs_teardown: end_non_revertible.needs_teardown()
+            needs_teardown: end_non_revertible.needs_teardown(),
+            reverted: self.reverted
         };
 
         PublicKernelData { public_inputs, proof: self.proof, vk: self.vk, vk_index: self.vk_index, vk_path: self.vk_path }

noir-projects/noir-protocol-circuits/crates/types/src/tests/public_circuit_public_inputs_builder.nr

@@ -27,6 +27,7 @@ struct PublicCircuitPublicInputsBuilder {
     unencrypted_log_preimages_length: Field,
     historical_header: Header,
     prover_address: AztecAddress,
+    reverted: bool,
 }
 
 impl PublicCircuitPublicInputsBuilder {
@@ -51,7 +52,8 @@ impl PublicCircuitPublicInputsBuilder {
             unencrypted_logs_hash: self.unencrypted_logs_hash,
             unencrypted_log_preimages_length: self.unencrypted_log_preimages_length,
             historical_header: self.historical_header,
-            prover_address: self.prover_address
+            prover_address: self.prover_address,
+            reverted: self.reverted
         }
     }
 }

yarn-project/aztec-node/src/aztec-node/server.ts

@@ -52,6 +52,7 @@ import {
   SequencerClient,
   WASMSimulator,
   getGlobalVariableBuilder,
+  partitionReverts,
 } from '@aztec/sequencer-client';
 import { ContractClassPublic, ContractInstanceWithAddress } from '@aztec/types/contracts';
 import {
@@ -609,10 +610,16 @@ export class AztecNodeService implements AztecNode {
       new WASMSimulator(),
     );
     const processor = await publicProcessorFactory.create(prevHeader, newGlobalVariables);
-    const [, failedTxs] = await processor.process([tx]);
+    const [processedTxs, failedTxs] = await processor.process([tx]);
     if (failedTxs.length) {
+      this.log.warn(`Simulated tx ${tx.getTxHash()} fails: ${failedTxs[0].error}`);
       throw failedTxs[0].error;
     }
+    const { reverted } = partitionReverts(processedTxs);
+    if (reverted.length) {
+      this.log.warn(`Simulated tx ${tx.getTxHash()} reverts: ${reverted[0].revertReason}`);
+      throw reverted[0].revertReason;
+    }
     this.log.info(`Simulated tx ${tx.getTxHash()} succeeds`);
   }