feat(avm): improve interpreter errors and tests (#4173)

- Interpreter only handles `AvmInterpreterError` type.
- `AvmMessageCallResult` now has a `revertReason: Error`. It's useful
for testing but might be useful in general. Happy to change otherwise.
- Added `/*param=*/` on calls where params are not clear from context or
variable name.

yarn-project/acir-simulator/src/avm/avm_message_call_result.ts

@@ -6,12 +6,15 @@ import { Fr } from '@aztec/foundation/fields';
 export class AvmMessageCallResult {
   /** - */
   public readonly reverted: boolean;
+  /** - */
+  public readonly revertReason: Error | undefined;
   /** .- */
   public readonly output: Fr[];
 
-  constructor(reverted: boolean, output: Fr[]) {
+  private constructor(reverted: boolean, output: Fr[], revertReason?: Error) {
     this.reverted = reverted;
     this.output = output;
+    this.revertReason = revertReason;
   }
 
   /**
@@ -26,9 +29,10 @@ export class AvmMessageCallResult {
   /**
    * Terminate a call as a revert
    * @param output - Return data ( revert message )
+   * @param reason - Optional reason for revert
    * @returns instance of AvmMessageCallResult
    */
-  public static revert(output: Fr[]): AvmMessageCallResult {
-    return new AvmMessageCallResult(true, output);
+  public static revert(output: Fr[], reason?: Error): AvmMessageCallResult {
+    return new AvmMessageCallResult(true, output, reason);
   }
 }

yarn-project/acir-simulator/src/avm/interpreter/interpreter.test.ts

@@ -8,31 +8,26 @@ import { Add } from '../opcodes/arithmetic.js';
 import { Jump, Return } from '../opcodes/control_flow.js';
 import { Instruction } from '../opcodes/instruction.js';
 import { CalldataCopy } from '../opcodes/memory.js';
-import { AvmInterpreter } from './interpreter.js';
+import { AvmInterpreter, InvalidProgramCounterError } from './interpreter.js';
 
 describe('interpreter', () => {
   it('Should execute a series of instructions', () => {
     const calldata: Fr[] = [new Fr(1), new Fr(2)];
     const stateManager = mock<AvmStateManager>();
 
     const instructions: Instruction[] = [
-      // Copy the first two elements of the calldata to memory regions 0 and 1
-      new CalldataCopy(0, 2, 0),
-      // Add the two together and store the result in memory region 2
-      new Add(0, 1, 2), // 1 + 2
-      // Return the result
-      new Return(2, 1), // [3]
+      new CalldataCopy(/*cdOffset=*/ 0, /*copySize=*/ 2, /*destOffset=*/ 0),
+      new Add(/*aOffset=*/ 0, /*bOffset=*/ 1, /*destOffset=*/ 2),
+      new Return(/*returnOffset=*/ 2, /*copySize=*/ 1),
     ];
 
     const context = new AvmMachineState(calldata);
     const interpreter = new AvmInterpreter(context, stateManager, instructions);
     const avmReturnData = interpreter.run();
 
     expect(avmReturnData.reverted).toBe(false);
-
-    const returnData = avmReturnData.output;
-    expect(returnData.length).toBe(1);
-    expect(returnData).toEqual([new Fr(3)]);
+    expect(avmReturnData.revertReason).toBeUndefined();
+    expect(avmReturnData.output).toEqual([new Fr(3)]);
   });
 
   it('Should revert with an invalid jump', () => {
@@ -49,5 +44,7 @@ describe('interpreter', () => {
     const avmReturnData = interpreter.run();
 
     expect(avmReturnData.reverted).toBe(true);
+    expect(avmReturnData.revertReason).toBeInstanceOf(InvalidProgramCounterError);
+    expect(avmReturnData.output).toHaveLength(0);
   });
 });

yarn-project/acir-simulator/src/avm/interpreter/interpreter.ts

@@ -1,6 +1,7 @@
-// import { AvmContext } from "../avm_machineState.js";
 import { Fr } from '@aztec/foundation/fields';
 
+import { strict as assert } from 'assert';
+
 import { AvmMachineState } from '../avm_machine_state.js';
 import { AvmMessageCallResult } from '../avm_message_call_result.js';
 import { AvmStateManager } from '../avm_state_manager.js';
@@ -16,10 +17,10 @@ export class AvmInterpreter {
   private machineState: AvmMachineState;
   private stateManager: AvmStateManager;
 
-  constructor(machineState: AvmMachineState, stateManager: AvmStateManager, bytecode: Instruction[]) {
+  constructor(machineState: AvmMachineState, stateManager: AvmStateManager, instructions: Instruction[]) {
     this.machineState = machineState;
     this.stateManager = stateManager;
-    this.instructions = bytecode;
+    this.instructions = instructions;
   }
 
   /**
@@ -29,27 +30,30 @@ export class AvmInterpreter {
    *               - any other panic will throw
    */
   run(): AvmMessageCallResult {
+    assert(this.instructions.length > 0);
+
     try {
-      while (!this.machineState.halted && this.machineState.pc < this.instructions.length) {
+      while (!this.machineState.halted) {
         const instruction = this.instructions[this.machineState.pc];
-
-        if (!instruction) {
-          throw new InvalidInstructionError(this.machineState.pc);
-        }
+        assert(!!instruction); // This should never happen
 
         instruction.execute(this.machineState, this.stateManager);
 
         if (this.machineState.pc >= this.instructions.length) {
-          throw new InvalidProgramCounterError(this.machineState.pc, this.instructions.length);
+          throw new InvalidProgramCounterError(this.machineState.pc, /*max=*/ this.instructions.length);
         }
       }
 
       const returnData = this.machineState.getReturnData();
       return AvmMessageCallResult.success(returnData);
-    } catch (e) {
-      // TODO: This should only accept AVM defined errors, anything else SHOULD be thrown upstream
+    } catch (_e) {
+      if (!(_e instanceof AvmInterpreterError)) {
+        throw _e;
+      }
+
+      const revertReason: AvmInterpreterError = _e;
       const revertData = this.machineState.getReturnData();
-      return AvmMessageCallResult.revert(revertData);
+      return AvmMessageCallResult.revert(revertData, revertReason);
     }
   }
 
@@ -64,20 +68,21 @@ export class AvmInterpreter {
 }
 
 /**
- * Error is thrown when the program counter goes to an invalid location.
- * There is no instruction at the provided pc
+ * Avm-specific errors should derive from this
  */
-class InvalidProgramCounterError extends Error {
-  constructor(pc: number, max: number) {
-    super(`Invalid program counter ${pc}, max is ${max}`);
+export abstract class AvmInterpreterError extends Error {
+  constructor(message: string) {
+    super(message);
+    this.name = 'AvmInterpreterError';
   }
 }
 
 /**
- * This assertion should never be hit - there should always be a valid instruction
+ * Error is thrown when the program counter goes to an invalid location.
+ * There is no instruction at the provided pc
  */
-class InvalidInstructionError extends Error {
-  constructor(pc: number) {
-    super(`Invalid instruction at ${pc}`);
+export class InvalidProgramCounterError extends AvmInterpreterError {
+  constructor(pc: number, max: number) {
+    super(`Invalid program counter ${pc}, max is ${max}`);
   }
 }