fix: fail transaction if we revert in setup or teardown

cspell.json

@@ -38,6 +38,7 @@
     "chainsafe",
     "cheatcode",
     "cheatcodes",
+    "checkpointed",
     "checksummed",
     "cimg",
     "ciphertext",

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

@@ -35,6 +35,10 @@ pub fn validate_inputs(public_call: PublicCallData) {
     assert(public_call.bytecode_hash != 0, "Bytecode hash cannot be zero");
 }
 
+pub fn validate_public_call_non_revert(public_call: PublicCallData) {
+    assert(public_call.call_stack_item.public_inputs.reverted == false, "Public call cannot be reverted");
+}
+
 pub fn initialize_reverted_flag(
     previous_kernel: PublicKernelData,
     public_call: PublicCallData,

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

@@ -38,6 +38,8 @@ impl PublicKernelSetupCircuitPrivateInputs {
     fn public_kernel_setup(self) -> PublicKernelCircuitPublicInputs {
         // construct the circuit outputs
         let mut public_inputs: PublicKernelCircuitPublicInputsBuilder = unsafe::zeroed();
+        // since this phase is non-revertible, we must assert the public call did not revert
+        common::validate_public_call_non_revert(self.public_call);
         common::initialize_reverted_flag(self.previous_kernel, self.public_call, &mut public_inputs);
 
         // initialise the end state with our provided previous kernel state
@@ -523,4 +525,12 @@ mod tests {
         assert_eq(request_context.counter, request_1.counter);
         assert_eq(request_context.contract_address, storage_contract_address);
     }
+
+    #[test(should_fail_with="Public call cannot be reverted")]
+    fn fails_if_public_call_reverted() {
+        let mut builder = PublicKernelSetupCircuitPrivateInputsBuilder::new();
+        builder.public_call.public_inputs.reverted = true;
+
+        builder.failed();
+    }
 }

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

@@ -26,6 +26,8 @@ impl PublicKernelTeardownCircuitPrivateInputs {
     fn public_kernel_teardown(self) -> PublicKernelCircuitPublicInputs {
         // construct the circuit outputs
         let mut public_inputs: PublicKernelCircuitPublicInputsBuilder = unsafe::zeroed();
+        // since this phase is non-revertible, we must assert the public call did not revert
+        common::validate_public_call_non_revert(self.public_call);
         common::initialize_reverted_flag(self.previous_kernel, self.public_call, &mut public_inputs);
 
         // initialise the end state with our provided previous kernel state
@@ -390,4 +392,12 @@ mod tests {
         let expected_unencrypted_logs_hash = compute_logs_hash(prev_unencrypted_logs_hash, unencrypted_logs_hash);
         assert_eq(public_inputs.end.unencrypted_logs_hash, expected_unencrypted_logs_hash);
     }
+
+    #[test(should_fail_with="Public call cannot be reverted")]
+    fn fails_if_public_call_reverted() {
+        let mut builder = PublicKernelTeardownCircuitPrivateInputsBuilder::new();
+        builder.public_call.public_inputs.reverted = true;
+
+        builder.failed();
+    }
 }

yarn-project/aztec.js/src/contract/sent_tx.ts

@@ -64,7 +64,9 @@ export class SentTx {
     }
     const receipt = await this.waitForReceipt(opts);
     if (receipt.status !== TxStatus.MINED) {
-      throw new Error(`Transaction ${await this.getTxHash()} was ${receipt.status}`);
+      throw new Error(
+        `Transaction ${await this.getTxHash()} was ${receipt.status}. Reason: ${receipt.error ?? 'unknown'}`,
+      );
     }
     if (opts?.debug) {
       const txHash = await this.getTxHash();

yarn-project/aztec.js/src/fee/public_fee_payment_method.ts

@@ -16,16 +16,16 @@ export class PublicFeePaymentMethod implements FeePaymentMethod {
     /**
      * The asset used to pay the fee.
      */
-    private asset: AztecAddress,
+    protected asset: AztecAddress,
     /**
      * Address which will hold the fee payment.
      */
-    private paymentContract: AztecAddress,
+    protected paymentContract: AztecAddress,
 
     /**
      * An auth witness provider to authorize fee payments
      */
-    private wallet: AccountWallet,
+    protected wallet: AccountWallet,
   ) {}
 
   /**

yarn-project/end-to-end/src/e2e_fees.test.ts

@@ -4,14 +4,17 @@ import {
   DebugLogger,
   ExtendedNote,
   Fr,
+  FunctionCall,
   FunctionSelector,
   Note,
   PrivateFeePaymentMethod,
   PublicFeePaymentMethod,
   TxHash,
   Wallet,
+  computeAuthWitMessageHash,
   computeMessageSecretHash,
 } from '@aztec/aztec.js';
+import { FunctionData } from '@aztec/circuits.js';
 import { ContractArtifact, decodeFunctionSignature } from '@aztec/foundation/abi';
 import {
   TokenContract as BananaCoin,
@@ -506,6 +509,114 @@ describe('e2e_fees', () => {
     });
   });
 
+  it('fails transaction that error in setup', async () => {
+    const OutrageousPublicAmountAliceDoesNotHave = 10000n;
+    // const PublicMintedAlicePublicBananas = 1000n;
+    const FeeAmount = 1n;
+    const RefundAmount = 2n;
+    const MaxFee = FeeAmount + RefundAmount;
+    const { wallets } = e2eContext;
+
+    // simulation throws an error when setup fails
+    await expect(
+      bananaCoin.methods
+        .transfer_public(aliceAddress, sequencerAddress, OutrageousPublicAmountAliceDoesNotHave, 0)
+        .send({
+          fee: {
+            maxFee: MaxFee,
+            paymentMethod: new BuggedSetupFeePaymentMethod(bananaCoin.address, bananaFPC.address, wallets[0]),
+          },
+        })
+        .wait(),
+    ).rejects.toThrow(/Message not authorized by account 'is_valid == true'/);
+
+    // so does the sequencer
+    await expect(
+      bananaCoin.methods
+        .transfer_public(aliceAddress, sequencerAddress, OutrageousPublicAmountAliceDoesNotHave, 0)
+        .send({
+          skipPublicSimulation: true,
+          fee: {
+            maxFee: MaxFee,
+            paymentMethod: new BuggedSetupFeePaymentMethod(bananaCoin.address, bananaFPC.address, wallets[0]),
+          },
+        })
+        .wait(),
+    ).rejects.toThrow(/Transaction [0-9a-f]{64} was dropped\. Reason: Tx dropped by P2P node\./);
+  });
+
+  it('fails transaction that error in teardown', async () => {
+    /**
+     * We trigger an error in teardown by having the FPC authorize a transfer of its entire balance to Alice
+     * as part of app logic. This will cause the FPC to not have enough funds to pay the refund back to Alice.
+     */
+
+    const PublicMintedAlicePublicBananas = 1000n;
+    const FeeAmount = 1n;
+    const RefundAmount = 2n;
+    const MaxFee = FeeAmount + RefundAmount;
+    const { wallets } = e2eContext;
+
+    const [initialAlicePrivateBananas, initialFPCPrivateBananas] = await bananaPrivateBalances(
+      aliceAddress,
+      bananaFPC.address,
+    );
+    const [initialAlicePublicBananas, initialFPCPublicBananas] = await bananaPublicBalances(
+      aliceAddress,
+      bananaFPC.address,
+    );
+    const [initialAliceGas, initialFPCGas, initialSequencerGas] = await gasBalances(
+      aliceAddress,
+      bananaFPC.address,
+      sequencerAddress,
+    );
+
+    await bananaCoin.methods.mint_public(aliceAddress, PublicMintedAlicePublicBananas).send().wait();
+
+    await expect(
+      bananaCoin.methods
+        .mint_public(aliceAddress, 1n) // random operation
+        .send({
+          fee: {
+            maxFee: MaxFee,
+            paymentMethod: new BuggedTeardownFeePaymentMethod(bananaCoin.address, bananaFPC.address, wallets[0]),
+          },
+        })
+        .wait(),
+    ).rejects.toThrow(/invalid nonce/);
+
+    // node also drops
+    await expect(
+      bananaCoin.methods
+        .mint_public(aliceAddress, 1n) // random operation
+        .send({
+          skipPublicSimulation: true,
+          fee: {
+            maxFee: MaxFee,
+            paymentMethod: new BuggedTeardownFeePaymentMethod(bananaCoin.address, bananaFPC.address, wallets[0]),
+          },
+        })
+        .wait(),
+    ).rejects.toThrow(/Transaction [0-9a-f]{64} was dropped\. Reason: Tx dropped by P2P node\./);
+
+    // nothing happened
+    await expectMapping(
+      bananaPrivateBalances,
+      [aliceAddress, bananaFPC.address, sequencerAddress],
+      [initialAlicePrivateBananas, initialFPCPrivateBananas, 0n],
+    );
+    await expectMapping(
+      bananaPublicBalances,
+      [aliceAddress, bananaFPC.address, sequencerAddress],
+      [initialAlicePublicBananas + PublicMintedAlicePublicBananas, initialFPCPublicBananas, 0n],
+    );
+    await expectMapping(
+      gasBalances,
+      [aliceAddress, bananaFPC.address, sequencerAddress],
+      [initialAliceGas, initialFPCGas, initialSequencerGas],
+    );
+  });
+
   function logFunctionSignatures(artifact: ContractArtifact, logger: DebugLogger) {
     artifact.functions.forEach(fn => {
       const sig = decodeFunctionSignature(fn.name, fn.parameters);
@@ -543,3 +654,81 @@ describe('e2e_fees', () => {
     await e2eContext.wallets[accountIndex].addNote(extendedNote);
   };
 });
+
+class BuggedSetupFeePaymentMethod extends PublicFeePaymentMethod {
+  getFunctionCalls(maxFee: Fr): Promise<FunctionCall[]> {
+    const nonce = Fr.random();
+    const messageHash = computeAuthWitMessageHash(this.paymentContract, {
+      args: [this.wallet.getAddress(), this.paymentContract, maxFee, nonce],
+      functionData: new FunctionData(
+        FunctionSelector.fromSignature('transfer_public((Field),(Field),Field,Field)'),
+        false,
+        false,
+        false,
+      ),
+      to: this.asset,
+    });
+
+    const tooMuchFee = new Fr(maxFee.toBigInt() * 2n);
+
+    return Promise.resolve([
+      this.wallet.setPublicAuth(messageHash, true).request(),
+      {
+        to: this.getPaymentContract(),
+        functionData: new FunctionData(
+          FunctionSelector.fromSignature('fee_entrypoint_public(Field,(Field),Field)'),
+          false,
+          true,
+          false,
+        ),
+        args: [tooMuchFee, this.asset, nonce],
+      },
+    ]);
+  }
+}
+
+class BuggedTeardownFeePaymentMethod extends PublicFeePaymentMethod {
+  async getFunctionCalls(maxFee: Fr): Promise<FunctionCall[]> {
+    // authorize the FPC to take the max fee from Alice
+    const nonce = Fr.random();
+    const messageHash1 = computeAuthWitMessageHash(this.paymentContract, {
+      args: [this.wallet.getAddress(), this.paymentContract, maxFee, nonce],
+      functionData: new FunctionData(
+        FunctionSelector.fromSignature('transfer_public((Field),(Field),Field,Field)'),
+        false,
+        false,
+        false,
+      ),
+      to: this.asset,
+    });
+
+    // authorize the FPC to take the maxFee
+    // do this first because we only get 2 feepayload calls
+    await this.wallet.setPublicAuth(messageHash1, true).send().wait();
+
+    return Promise.resolve([
+      // in this, we're actually paying the fee in setup
+      {
+        to: this.getPaymentContract(),
+        functionData: new FunctionData(
+          FunctionSelector.fromSignature('fee_entrypoint_public(Field,(Field),Field)'),
+          false,
+          true,
+          false,
+        ),
+        args: [maxFee, this.asset, nonce],
+      },
+      // and trying to take a little extra in teardown, but specify a bad nonce
+      {
+        to: this.asset,
+        functionData: new FunctionData(
+          FunctionSelector.fromSignature('transfer_public((Field),(Field),Field,Field)'),
+          false,
+          false,
+          false,
+        ),
+        args: [this.wallet.getAddress(), this.paymentContract, new Fr(1), Fr.random()],
+      },
+    ]);
+  }
+}

yarn-project/sequencer-client/src/sequencer/abstract_phase_manager.ts

@@ -59,7 +59,6 @@ import { getVerificationKeys } from '../mocks/verification_keys.js';
 import { PublicProver } from '../prover/index.js';
 import { PublicKernelCircuitSimulator } from '../simulator/index.js';
 import { HintsBuilder } from './hints_builder.js';
-import { FailedTx } from './processed_tx.js';
 import { lastSideEffectCounter } from './utils.js';
 
 export enum PublicKernelPhase {
@@ -115,7 +114,6 @@ export abstract class AbstractPhaseManager {
      */
     revertReason: SimulationError | undefined;
   }>;
-  abstract rollback(tx: Tx, err: unknown): Promise<FailedTx>;
 
   public static extractEnqueuedPublicCallsByPhase(
     publicInputs: PrivateKernelTailCircuitPublicInputs,
@@ -220,8 +218,18 @@ export abstract class AbstractPhaseManager {
 
         const result = isExecutionRequest ? await simulator(current, this.globalVariables) : current;
 
-        newUnencryptedFunctionLogs.push(result.unencryptedLogs);
         const functionSelector = result.execution.functionData.selector.toString();
+        if (result.reverted && !PhaseIsRevertible[this.phase]) {
+          this.log.debug(
+            `Simulation error on ${result.execution.contractAddress.toString()}:${functionSelector} with reason: ${
+              result.revertReason
+            }`,
+          );
+          throw result.revertReason;
+        }
+
+        newUnencryptedFunctionLogs.push(result.unencryptedLogs);
+
         this.log.debug(
           `Running public kernel circuit for ${result.execution.contractAddress.toString()}:${functionSelector}`,
         );
@@ -230,14 +238,23 @@ export abstract class AbstractPhaseManager {
 
         [kernelOutput, kernelProof] = await this.runKernelCircuit(kernelOutput, kernelProof, callData);
 
-        if (kernelOutput.reverted && this.phase === PublicKernelPhase.APP_LOGIC) {
+        // sanity check. Note we can't expect them to just be equal, because e.g.
+        // if the simulator reverts in app logic, it "resets" and result.reverted will be false when we run teardown,
+        // but the kernel carries the reverted flag forward. But if the simulator reverts, so should the kernel.
+        if (result.reverted && !kernelOutput.reverted) {
+          throw new Error(
+            `Public kernel circuit did not revert on ${result.execution.contractAddress.toString()}:${functionSelector}, but simulator did.`,
+          );
+        }
+
+        // We know the phase is revertible due to the above check.
+        // So safely return the revert reason and the kernel output (which has had its revertible side effects dropped)
+        if (result.reverted) {
           this.log.debug(
             `Reverting on ${result.execution.contractAddress.toString()}:${functionSelector} with reason: ${
               result.revertReason
             }`,
           );
-          // halt immediately if the public kernel circuit has reverted.
-          // return no logs, as they should not go on-chain.
           return [kernelOutput, kernelProof, [], result.revertReason];
         }