fix: Include revertdata in Avm simulation errors for failures in nonr…

…evertible (#9744) Fixes #9727 errors after #9552
AztecProtocol · Nov 6, 2024 · 3f52f71 · 3f52f71
1 parent 62c672c
commit 3f52f71
Show file tree

Hide file tree

Showing 10 changed files with 38 additions and 30 deletions.
diff --git a/scripts/ci/get_e2e_jobs.sh b/scripts/ci/get_e2e_jobs.sh
@@ -23,10 +23,13 @@ allow_list=(
   "e2e_avm_simulator"
   "e2e_block_building"
   "e2e_cross_chain_messaging"
+  "e2e_crowdfunding_and_claim"
   "e2e_deploy_contract"
   "e2e_fees"
+  "e2e_fees_failures"
   "e2e_fees_gas_estimation"
   "e2e_fees_private_payments"
+  "e2e_fees_private_refunds"
   "e2e_max_block_number"
   "e2e_nested_contract"
   "e2e_ordering"

diff --git a/yarn-project/circuit-types/src/simulation_error.ts b/yarn-project/circuit-types/src/simulation_error.ts
@@ -1,4 +1,4 @@
-import { type AztecAddress, type FunctionSelector } from '@aztec/circuits.js';
+import { type AztecAddress, Fr, type FunctionSelector } from '@aztec/circuits.js';
 import { type OpcodeLocation } from '@aztec/foundation/abi';
 
 /**
@@ -68,6 +68,7 @@ export class SimulationError extends Error {
   constructor(
     private originalMessage: string,
     private functionErrorStack: FailingFunction[],
+    public revertData: Fr[] = [],
     private noirErrorStack?: NoirCallStack,
     options?: ErrorOptions,
   ) {
@@ -202,10 +203,16 @@ export class SimulationError extends Error {
       originalMessage: this.originalMessage,
       functionErrorStack: this.functionErrorStack,
       noirErrorStack: this.noirErrorStack,
+      revertData: this.revertData.map(fr => fr.toString()),
     };
   }
 
   static fromJSON(obj: ReturnType<SimulationError['toJSON']>) {
-    return new SimulationError(obj.originalMessage, obj.functionErrorStack, obj.noirErrorStack);
+    return new SimulationError(
+      obj.originalMessage,
+      obj.functionErrorStack,
+      obj.revertData.map(serializedFr => Fr.fromString(serializedFr)),
+      obj.noirErrorStack,
+    );
   }
 }
diff --git a/yarn-project/end-to-end/scripts/e2e_test_config.yml b/yarn-project/end-to-end/scripts/e2e_test_config.yml
@@ -30,8 +30,7 @@ tests:
   e2e_card_game: {}
   e2e_cheat_codes: {}
   e2e_cross_chain_messaging: {}
-  # TODO reenable in https://github.com/AztecProtocol/aztec-packages/pull/9727
-  # e2e_crowdfunding_and_claim: {}
+  e2e_crowdfunding_and_claim: {}
   e2e_deploy_contract: {}
   e2e_devnet_smoke: {}
   docs_examples:
@@ -42,18 +41,16 @@ tests:
   # TODO(https://github.com/AztecProtocol/aztec-packages/issues/9488): reenable
   # e2e_fees_dapp_subscription:
   #   test_path: "e2e_fees/dapp_subscription.test.ts"
-  # TODO reenable in https://github.com/AztecProtocol/aztec-packages/pull/9727
-  # e2e_fees_failures:
-  #   test_path: 'e2e_fees/failures.test.ts'
+  e2e_fees_failures:
+    test_path: 'e2e_fees/failures.test.ts'
   e2e_fees_fee_juice_payments:
     test_path: 'e2e_fees/fee_juice_payments.test.ts'
   e2e_fees_gas_estimation:
     test_path: 'e2e_fees/gas_estimation.test.ts'
   e2e_fees_private_payments:
     test_path: 'e2e_fees/private_payments.test.ts'
-  # TODO reenable in https://github.com/AztecProtocol/aztec-packages/pull/9727
-  # e2e_fees_private_refunds:
-  #   test_path: 'e2e_fees/private_refunds.test.ts'
+  e2e_fees_private_refunds:
+    test_path: 'e2e_fees/private_refunds.test.ts'
   e2e_keys: {}
   e2e_l1_with_wall_time: {}
   e2e_lending_contract: {}

diff --git a/yarn-project/end-to-end/src/e2e_crowdfunding_and_claim.test.ts b/yarn-project/end-to-end/src/e2e_crowdfunding_and_claim.test.ts
@@ -346,7 +346,7 @@ describe('e2e_crowdfunding_and_claim', () => {
     donorWallets[1].setScopes([donorWallets[1].getAddress(), crowdfundingContract.address]);
 
     await expect(donorWallets[1].simulateTx(request, true, operatorWallet.getAddress())).rejects.toThrow(
-      'Assertion failed: Users cannot set msg_sender in first call',
+      'Circuit execution failed: Users cannot set msg_sender in first call',
     );
   });
 

diff --git a/yarn-project/pxe/src/pxe_service/error_enriching.ts b/yarn-project/pxe/src/pxe_service/error_enriching.ts
@@ -54,7 +54,6 @@ export async function enrichSimulationError(err: SimulationError, db: PxeDatabas
 
 export async function enrichPublicSimulationError(
   err: SimulationError,
-  revertData: Fr[],
   contractDataOracle: ContractDataOracle,
   db: PxeDatabase,
   logger: DebugLogger,
@@ -70,7 +69,7 @@ export async function enrichPublicSimulationError(
     originalFailingFunction.contractAddress,
     FunctionSelector.fromField(new Fr(PUBLIC_DISPATCH_SELECTOR)),
   );
-  const assertionMessage = resolveAssertionMessageFromRevertData(revertData, artifact);
+  const assertionMessage = resolveAssertionMessageFromRevertData(err.revertData, artifact);
   if (assertionMessage) {
     err.setOriginalMessage(err.getOriginalMessage() + `${assertionMessage}`);
   }

diff --git a/yarn-project/pxe/src/pxe_service/pxe_service.ts b/yarn-project/pxe/src/pxe_service/pxe_service.ts
@@ -781,18 +781,20 @@ export class PXEService implements PXE {
    * @param tx - The transaction to be simulated.
    */
   async #simulatePublicCalls(tx: Tx) {
-    const result = await this.node.simulatePublicCalls(tx);
-    if (result.revertReason) {
-      await enrichPublicSimulationError(
-        result.revertReason,
-        result.publicReturnValues[0].values || [],
-        this.contractDataOracle,
-        this.db,
-        this.log,
-      );
-      throw result.revertReason;
+    // Simulating public calls can throw if the TX fails in a phase that doesn't allow reverts (setup)
+    // Or return as reverted if it fails in a phase that allows reverts (app logic, teardown)
+    try {
+      const result = await this.node.simulatePublicCalls(tx);
+      if (result.revertReason) {
+        throw result.revertReason;
+      }
+      return result;
+    } catch (err) {
+      if (err instanceof SimulationError) {
+        await enrichPublicSimulationError(err, this.contractDataOracle, this.db, this.log);
+      }
+      throw err;
     }
-    return result;
   }
 
   async #profileKernelProver(

diff --git a/yarn-project/simulator/src/common/errors.ts b/yarn-project/simulator/src/common/errors.ts
@@ -1,4 +1,5 @@
 import { type FailingFunction, type NoirCallStack, SimulationError } from '@aztec/circuit-types';
+import { type Fr } from '@aztec/circuits.js';
 
 /**
  * An error that occurred during the execution of a function.
@@ -47,7 +48,7 @@ export function traverseCauseChain(error: Error, callback: (error: Error) => voi
  * @param error - The error thrown during execution.
  * @returns - A simulation error.
  */
-export function createSimulationError(error: Error): SimulationError {
+export function createSimulationError(error: Error, revertData?: Fr[]): SimulationError {
   let rootCause = error;
   let noirCallStack: NoirCallStack | undefined = undefined;
   const aztecCallStack: FailingFunction[] = [];
@@ -62,5 +63,5 @@ export function createSimulationError(error: Error): SimulationError {
     }
   });
 
-  return new SimulationError(rootCause.message, aztecCallStack, noirCallStack, { cause: rootCause });
+  return new SimulationError(rootCause.message, aztecCallStack, revertData, noirCallStack, { cause: rootCause });
 }
diff --git a/yarn-project/simulator/src/public/side_effect_trace.ts b/yarn-project/simulator/src/public/side_effect_trace.ts
@@ -361,7 +361,9 @@ export class PublicSideEffectTrace implements PublicSideEffectTraceInterface {
       calldata: avmEnvironment.calldata,
       returnValues: avmCallResults.output,
       reverted: avmCallResults.reverted,
-      revertReason: avmCallResults.revertReason ? createSimulationError(avmCallResults.revertReason) : undefined,
+      revertReason: avmCallResults.revertReason
+        ? createSimulationError(avmCallResults.revertReason, avmCallResults.output)
+        : undefined,
 
       contractStorageReads: this.contractStorageReads,
       contractStorageUpdateRequests: this.contractStorageUpdateRequests,

diff --git a/yarn-project/txe/src/oracle/txe_oracle.ts b/yarn-project/txe/src/oracle/txe_oracle.ts
@@ -695,7 +695,6 @@ export class TXE implements TypedOracle {
       if (executionResult.revertReason && executionResult.revertReason instanceof SimulationError) {
         await enrichPublicSimulationError(
           executionResult.revertReason,
-          executionResult.returnValues,
           this.contractDataOracle,
           this.txeDatabase,
           this.logger,

diff --git a/yarn-project/txe/src/txe_service/txe_service.ts b/yarn-project/txe/src/txe_service/txe_service.ts
@@ -743,7 +743,6 @@ export class TXEService {
       if (result.revertReason && result.revertReason instanceof SimulationError) {
         await enrichPublicSimulationError(
           result.revertReason,
-          result.returnValues,
           (this.typedOracle as TXE).getContractDataOracle(),
           (this.typedOracle as TXE).getTXEDatabase(),
           this.logger,
@@ -774,7 +773,6 @@ export class TXEService {
       if (result.revertReason && result.revertReason instanceof SimulationError) {
         await enrichPublicSimulationError(
           result.revertReason,
-          result.returnValues,
           (this.typedOracle as TXE).getContractDataOracle(),
           (this.typedOracle as TXE).getTXEDatabase(),
           this.logger,