chore(avm-simulator): formatting and fixes (#5092)

* Logging * `Call` and `StaticCall` were returning Field while per YP should be Uint8. * I'm also moving the RETURN and REVERT opcodes to `external_calls.ts` (from `control_flow.ts`). Relates to #4313, #4127.
AztecProtocol · Mar 11, 2024 · b3fa084 · b3fa084
1 parent 07dd821
commit b3fa084
Show file tree

Hide file tree

Showing 11 changed files with 199 additions and 170 deletions.
diff --git a/avm-transpiler/src/opcodes.rs b/avm-transpiler/src/opcodes.rs
@@ -3,26 +3,21 @@
 #[derive(Copy, Clone)]
 pub enum AvmOpcode {
     // Compute
-    // Compute - Arithmetic
     ADD,
     SUB,
     MUL,
     DIV,
-    // Compute - Comparators
     EQ,
     LT,
     LTE,
-    // Compute - Bitwise
     AND,
     OR,
     XOR,
     NOT,
     SHL,
     SHR,
-    // Compute - Type Conversions
     CAST,
-
-    // Execution Environment
+    // Execution environment
     ADDRESS,
     STORAGEADDRESS,
     ORIGIN,
@@ -32,7 +27,6 @@ pub enum AvmOpcode {
     FEEPERL2GAS,
     FEEPERDAGAS,
     CONTRACTCALLDEPTH,
-    // Execution Environment - Globals
     CHAINID,
     VERSION,
     BLOCKNUMBER,
@@ -41,50 +35,42 @@ pub enum AvmOpcode {
     BLOCKL1GASLIMIT,
     BLOCKL2GASLIMIT,
     BLOCKDAGASLIMIT,
-    // Execution Environment - Calldata
     CALLDATACOPY,
-
-    // Machine State
-    // Machine State - Gas
+    // Gas
     L1GASLEFT,
     L2GASLEFT,
     DAGASLEFT,
-    // Machine State - Internal Control Flow
+    // Control flow
     JUMP,
     JUMPI,
     INTERNALCALL,
     INTERNALRETURN,
-    // Machine State - Memory
+    // Memory
     SET,
     MOV,
     CMOV,
-
-    // World State
-    SLOAD,           // Public Storage
-    SSTORE,          // Public Storage
-    NOTEHASHEXISTS,  // Notes & Nullifiers
-    EMITNOTEHASH,    // Notes & Nullifiers
-    NULLIFIEREXISTS, // Notes & Nullifiers
-    EMITNULLIFIER,   // Notes & Nullifiers
-    L1TOL2MSGEXISTS, // Messages
-    HEADERMEMBER,    // Archive tree & Headers
-
-    // Accrued Substate
+    // World state
+    SLOAD,
+    SSTORE,
+    NOTEHASHEXISTS,
+    EMITNOTEHASH,
+    NULLIFIEREXISTS,
+    EMITNULLIFIER,
+    L1TOL2MSGEXISTS,
+    HEADERMEMBER,
     EMITUNENCRYPTEDLOG,
     SENDL2TOL1MSG,
-
-    // Control Flow - Contract Calls
+    // External calls
     CALL,
     STATICCALL,
     DELEGATECALL,
     RETURN,
     REVERT,
-
     // Gadgets
     KECCAK,
     POSEIDON,
-    SHA256,
-    PEDERSEN,
+    SHA256,   // temp - may be removed, but alot of contracts rely on it
+    PEDERSEN, // temp - may be removed, but alot of contracts rely on it
 }
 
 impl AvmOpcode {

diff --git a/yarn-project/simulator/src/avm/avm_memory_types.ts b/yarn-project/simulator/src/avm/avm_memory_types.ts
@@ -1,5 +1,6 @@
 import { toBufferBE } from '@aztec/foundation/bigint-buffer';
 import { Fr } from '@aztec/foundation/fields';
+import { DebugLogger, createDebugLogger } from '@aztec/foundation/log';
 
 import { strict as assert } from 'assert';
 
@@ -28,6 +29,10 @@ export abstract class MemoryValue {
   public toFr(): Fr {
     return new Fr(this.toBigInt());
   }
+
+  public toString(): string {
+    return `${this.constructor.name}(0x${this.toBigInt().toString(16)})`;
+  }
 }
 
 /** IntegralValue gathers the common operations for all integral memory types. */
@@ -189,6 +194,8 @@ export enum TypeTag {
 
 // TODO: Consider automatic conversion when getting undefined values.
 export class TaggedMemory {
+  static readonly log: DebugLogger = createDebugLogger('aztec:avm_simulator:memory');
+
   // FIXME: memory should be 2^32, but TS doesn't allow for arrays that big.
   static readonly MAX_MEMORY_SIZE = Number((1n << 32n) - 2n);
   private _mem: MemoryValue[];
@@ -200,25 +207,29 @@ export class TaggedMemory {
 
   public get(offset: number): MemoryValue {
     assert(offset < TaggedMemory.MAX_MEMORY_SIZE);
-    return this.getAs<MemoryValue>(offset);
+    const value = this.getAs<MemoryValue>(offset);
+    return value;
   }
 
   public getAs<T>(offset: number): T {
     assert(offset < TaggedMemory.MAX_MEMORY_SIZE);
     const word = this._mem[offset];
+    TaggedMemory.log(`get(${offset}) = ${word}`);
     return word as T;
   }
 
   public getSlice(offset: number, size: number): MemoryValue[] {
     assert(offset < TaggedMemory.MAX_MEMORY_SIZE);
     assert(offset + size < TaggedMemory.MAX_MEMORY_SIZE);
-    return this._mem.slice(offset, offset + size);
+    const value = this._mem.slice(offset, offset + size);
+    TaggedMemory.log(`getSlice(${offset}, ${size}) = ${value}`);
+    return value;
   }
 
   public getSliceAs<T>(offset: number, size: number): T[] {
     assert(offset < TaggedMemory.MAX_MEMORY_SIZE);
     assert(offset + size < TaggedMemory.MAX_MEMORY_SIZE);
-    return this._mem.slice(offset, offset + size) as T[];
+    return this.getSlice(offset, size) as T[];
   }
 
   public getSliceTags(offset: number, size: number): TypeTag[] {
@@ -230,6 +241,7 @@ export class TaggedMemory {
   public set(offset: number, v: MemoryValue) {
     assert(offset < TaggedMemory.MAX_MEMORY_SIZE);
     this._mem[offset] = v;
+    TaggedMemory.log(`set(${offset}, ${v})`);
   }
 
   public setSlice(offset: number, vs: MemoryValue[]) {
@@ -240,6 +252,7 @@ export class TaggedMemory {
       this._mem.length = offset + vs.length;
     }
     this._mem.splice(offset, vs.length, ...vs);
+    TaggedMemory.log(`setSlice(${offset}, ${vs})`);
   }
 
   public getTag(offset: number): TypeTag {
@@ -327,4 +340,24 @@ export class TaggedMemory {
         throw new Error(`${TypeTag[tag]} is not a valid integral type.`);
     }
   }
+
+  // Does not truncate. Type constructor will check that it fits.
+  public static buildFromTagOrDie(v: bigint | number, tag: TypeTag): MemoryValue {
+    switch (tag) {
+      case TypeTag.UINT8:
+        return new Uint8(v);
+      case TypeTag.UINT16:
+        return new Uint16(v);
+      case TypeTag.UINT32:
+        return new Uint32(v);
+      case TypeTag.UINT64:
+        return new Uint64(v);
+      case TypeTag.UINT128:
+        return new Uint128(v);
+      case TypeTag.FIELD:
+        return new Field(v);
+      default:
+        throw new Error(`${TypeTag[tag]} is not a valid integral type.`);
+    }
+  }
 }
diff --git a/yarn-project/simulator/src/avm/avm_simulator.test.ts b/yarn-project/simulator/src/avm/avm_simulator.test.ts
@@ -23,7 +23,7 @@ import { encodeToBytecode } from './serialization/bytecode_serialization.js';
 function getAvmTestContractBytecode(functionName: string): Buffer {
   const artifact = AvmTestContractArtifact.functions.find(f => f.name === functionName)!;
   assert(
-    !!artifact.bytecode,
+    !!artifact?.bytecode,
     `No bytecode found for function ${functionName}. Try re-running bootstraph.sh on the repository root.`,
   );
   return Buffer.from(artifact.bytecode, 'base64');

diff --git a/yarn-project/simulator/src/avm/avm_simulator.ts b/yarn-project/simulator/src/avm/avm_simulator.ts
@@ -9,9 +9,13 @@ import type { Instruction } from './opcodes/index.js';
 import { decodeFromBytecode } from './serialization/bytecode_serialization.js';
 
 export class AvmSimulator {
-  private log: DebugLogger = createDebugLogger('aztec:avm_simulator');
+  private log: DebugLogger;
 
-  constructor(private context: AvmContext) {}
+  constructor(private context: AvmContext) {
+    this.log = createDebugLogger(
+      `aztec:avm_simulator:core(f:${context.environment.temporaryFunctionSelector.toString()})`,
+    );
+  }
 
   /**
    * Fetch the bytecode and execute it in the current context.
@@ -52,7 +56,10 @@ export class AvmSimulator {
       // continuing until the machine state signifies a halt
       while (!this.context.machineState.halted) {
         const instruction = instructions[this.context.machineState.pc];
-        assert(!!instruction); // This should never happen
+        assert(
+          !!instruction,
+          'AVM attempted to execute non-existent instruction. This should never happen (invalid bytecode or AVM simulator bug)!',
+        );
 
         this.log.debug(`@${this.context.machineState.pc} ${instruction.toString()}`);
         // Execute the instruction.

diff --git a/yarn-project/simulator/src/avm/opcodes/arithmetic.ts b/yarn-project/simulator/src/avm/opcodes/arithmetic.ts
@@ -11,6 +11,8 @@ export class Add extends ThreeOperandInstruction {
   }
 
   async execute(context: AvmContext): Promise<void> {
+    context.machineState.memory.checkTags(this.inTag, this.aOffset, this.bOffset);
+
     const a = context.machineState.memory.get(this.aOffset);
     const b = context.machineState.memory.get(this.bOffset);
 

diff --git a/yarn-project/simulator/src/avm/opcodes/comparators.ts b/yarn-project/simulator/src/avm/opcodes/comparators.ts
@@ -1,4 +1,5 @@
 import type { AvmContext } from '../avm_context.js';
+import { TaggedMemory } from '../avm_memory_types.js';
 import { Opcode } from '../serialization/instruction_serialization.js';
 import { ThreeOperandInstruction } from './instruction_impl.js';
 
@@ -16,8 +17,7 @@ export class Eq extends ThreeOperandInstruction {
     const a = context.machineState.memory.get(this.aOffset);
     const b = context.machineState.memory.get(this.bOffset);
 
-    // Result will be of the same type as 'a'.
-    const dest = a.build(a.equals(b) ? 1n : 0n);
+    const dest = TaggedMemory.buildFromTagOrDie(a.equals(b) ? 1n : 0n, this.inTag);
     context.machineState.memory.set(this.dstOffset, dest);
 
     context.machineState.incrementPc();
@@ -38,8 +38,7 @@ export class Lt extends ThreeOperandInstruction {
     const a = context.machineState.memory.get(this.aOffset);
     const b = context.machineState.memory.get(this.bOffset);
 
-    // Result will be of the same type as 'a'.
-    const dest = a.build(a.lt(b) ? 1n : 0n);
+    const dest = TaggedMemory.buildFromTagOrDie(a.lt(b) ? 1n : 0n, this.inTag);
     context.machineState.memory.set(this.dstOffset, dest);
 
     context.machineState.incrementPc();
@@ -60,8 +59,7 @@ export class Lte extends ThreeOperandInstruction {
     const a = context.machineState.memory.get(this.aOffset);
     const b = context.machineState.memory.get(this.bOffset);
 
-    // Result will be of the same type as 'a'.
-    const dest = a.build(a.equals(b) || a.lt(b) ? 1n : 0n);
+    const dest = TaggedMemory.buildFromTagOrDie(a.lt(b) || a.equals(b) ? 1n : 0n, this.inTag);
     context.machineState.memory.set(this.dstOffset, dest);
 
     context.machineState.incrementPc();

diff --git a/yarn-project/simulator/src/avm/opcodes/control_flow.test.ts b/yarn-project/simulator/src/avm/opcodes/control_flow.test.ts
@@ -1,10 +1,8 @@
-import { Fr } from '@aztec/foundation/fields';
-
 import { AvmContext } from '../avm_context.js';
-import { Field, Uint16 } from '../avm_memory_types.js';
+import { Uint16 } from '../avm_memory_types.js';
 import { InstructionExecutionError } from '../errors.js';
 import { initContext } from '../fixtures/index.js';
-import { InternalCall, InternalReturn, Jump, JumpI, Return, Revert } from './control_flow.js';
+import { InternalCall, InternalReturn, Jump, JumpI } from './control_flow.js';
 
 describe('Control Flow Opcodes', () => {
   let context: AvmContext;
@@ -82,7 +80,7 @@ describe('Control Flow Opcodes', () => {
     });
   });
 
-  describe('INTERNALCALL and RETURN', () => {
+  describe('INTERNALCALL and INTERNALRETURN', () => {
     it('INTERNALCALL Should (de)serialize correctly', () => {
       const buf = Buffer.from([
         InternalCall.opcode, // opcode
@@ -151,68 +149,4 @@ describe('Control Flow Opcodes', () => {
       }
     });
   });
-
-  describe('RETURN', () => {
-    it('Should (de)serialize correctly', () => {
-      const buf = Buffer.from([
-        Return.opcode, // opcode
-        0x01, // indirect
-        ...Buffer.from('12345678', 'hex'), // returnOffset
-        ...Buffer.from('a2345678', 'hex'), // copySize
-      ]);
-      const inst = new Return(/*indirect=*/ 0x01, /*returnOffset=*/ 0x12345678, /*copySize=*/ 0xa2345678);
-
-      expect(Return.deserialize(buf)).toEqual(inst);
-      expect(inst.serialize()).toEqual(buf);
-    });
-
-    it('Should return data from the return opcode', async () => {
-      const returnData = [new Fr(1n), new Fr(2n), new Fr(3n)];
-
-      context.machineState.memory.set(0, new Field(1n));
-      context.machineState.memory.set(1, new Field(2n));
-      context.machineState.memory.set(2, new Field(3n));
-
-      const instruction = new Return(/*indirect=*/ 0, /*returnOffset=*/ 0, returnData.length);
-      await instruction.execute(context);
-
-      expect(context.machineState.halted).toBe(true);
-      expect(context.machineState.getResults()).toEqual({
-        reverted: false,
-        output: returnData,
-      });
-    });
-  });
-
-  describe('REVERT', () => {
-    it('Should (de)serialize correctly', () => {
-      const buf = Buffer.from([
-        Revert.opcode, // opcode
-        0x01, // indirect
-        ...Buffer.from('12345678', 'hex'), // returnOffset
-        ...Buffer.from('a2345678', 'hex'), // retSize
-      ]);
-      const inst = new Revert(/*indirect=*/ 0x01, /*returnOffset=*/ 0x12345678, /*retSize=*/ 0xa2345678);
-
-      expect(Revert.deserialize(buf)).toEqual(inst);
-      expect(inst.serialize()).toEqual(buf);
-    });
-
-    it('Should return data and revert from the revert opcode', async () => {
-      const returnData = [new Fr(1n), new Fr(2n), new Fr(3n)];
-
-      context.machineState.memory.set(0, new Field(1n));
-      context.machineState.memory.set(1, new Field(2n));
-      context.machineState.memory.set(2, new Field(3n));
-
-      const instruction = new Revert(/*indirect=*/ 0, /*returnOffset=*/ 0, returnData.length);
-      await instruction.execute(context);
-
-      expect(context.machineState.halted).toBe(true);
-      expect(context.machineState.getResults()).toEqual({
-        reverted: true,
-        output: returnData,
-      });
-    });
-  });
 });