feat(avm): storage (#4673)

## Overview
Works around brillig blowup issue by altering the read and write opcodes
to take in arrays of data. This is potentially just a short term fix.

- Reading and writing to storage now take in arrays, code will not
compile without this change, due to an ssa issue ->[ ISSUE
](#4979)

- Tag checks on memory now just make sure the tag is LESS than uint64,
rather than asserting that the memory tag is uint32, this should be
fine.


- We had to blow up the memory space of the avm to u64 as the entire
noir compiler works with u64s. Arrays will not work unless we either
    - Make the avm 64 bit addressable ( probably fine )
- Make noir 32 bit addressable ( requires alot of buy in )
#4814

---------

Co-authored-by: sirasistant <sirasistant@gmail.com>

avm-transpiler/src/instructions.rs

@@ -7,6 +7,7 @@ use crate::opcodes::AvmOpcode;
 pub const ALL_DIRECT: u8 = 0b00000000;
 pub const ZEROTH_OPERAND_INDIRECT: u8 = 0b00000001;
 pub const FIRST_OPERAND_INDIRECT: u8 = 0b00000010;
+pub const SECOND_OPERAND_INDIRECT: u8 = 0b00000100;
 pub const ZEROTH_FIRST_OPERANDS_INDIRECT: u8 = ZEROTH_OPERAND_INDIRECT | FIRST_OPERAND_INDIRECT;
 
 /// A simple representation of an AVM instruction for the purpose

avm-transpiler/src/transpile.rs

@@ -7,7 +7,7 @@ use acvm::brillig_vm::brillig::{
 
 use crate::instructions::{
     AvmInstruction, AvmOperand, AvmTypeTag, ALL_DIRECT, FIRST_OPERAND_INDIRECT,
-    ZEROTH_OPERAND_INDIRECT,
+    SECOND_OPERAND_INDIRECT, ZEROTH_OPERAND_INDIRECT,
 };
 use crate::opcodes::AvmOpcode;
 use crate::utils::{dbg_print_avm_program, dbg_print_brillig_program};
@@ -257,8 +257,10 @@ fn handle_foreign_call(
         "avmOpcodePoseidon" => {
             handle_single_field_hash_instruction(avm_instrs, function, destinations, inputs)
         }
+        "storageWrite" => emit_storage_write(avm_instrs, destinations, inputs),
+        "storageRead" => emit_storage_read(avm_instrs, destinations, inputs),
         // Getters.
-        _ if inputs.len() == 0 && destinations.len() == 1 => {
+        _ if inputs.is_empty() && destinations.len() == 1 => {
             handle_getter_instruction(avm_instrs, function, destinations, inputs)
         }
         // Anything else.
@@ -361,7 +363,7 @@ fn handle_emit_note_hash_or_nullifier(
         "EMITNOTEHASH"
     };
 
-    if destinations.len() != 0 || inputs.len() != 1 {
+    if !destinations.is_empty() || inputs.len() != 1 {
         panic!(
             "Transpiler expects ForeignCall::{} to have 0 destinations and 1 input, got {} and {}",
             function_name,
@@ -390,6 +392,87 @@ fn handle_emit_note_hash_or_nullifier(
     });
 }
 
+/// Emit a storage write opcode
+/// The current implementation writes an array of values into storage ( contiguous slots in memory )
+fn emit_storage_write(
+    avm_instrs: &mut Vec<AvmInstruction>,
+    destinations: &Vec<ValueOrArray>,
+    inputs: &Vec<ValueOrArray>,
+) {
+    assert!(inputs.len() == 2);
+    assert!(destinations.len() == 1);
+
+    let slot_offset_maybe = inputs[0];
+    let slot_offset = match slot_offset_maybe {
+        ValueOrArray::MemoryAddress(slot_offset) => slot_offset.0,
+        _ => panic!("ForeignCall address destination should be a single value"),
+    };
+
+    let src_offset_maybe = inputs[1];
+    let (src_offset, src_size) = match src_offset_maybe {
+        ValueOrArray::HeapArray(HeapArray { pointer, size }) => (pointer.0, size),
+        _ => panic!("Storage write address inputs should be an array of values"),
+    };
+
+    avm_instrs.push(AvmInstruction {
+        opcode: AvmOpcode::SSTORE,
+        indirect: Some(ZEROTH_OPERAND_INDIRECT),
+        operands: vec![
+            AvmOperand::U32 {
+                value: src_offset as u32,
+            },
+            AvmOperand::U32 {
+                value: src_size as u32,
+            },
+            AvmOperand::U32 {
+                value: slot_offset as u32,
+            },
+        ],
+        ..Default::default()
+    })
+}
+
+/// Emit a storage read opcode
+/// The current implementation reads an array of values from storage ( contiguous slots in memory )
+fn emit_storage_read(
+    avm_instrs: &mut Vec<AvmInstruction>,
+    destinations: &Vec<ValueOrArray>,
+    inputs: &Vec<ValueOrArray>,
+) {
+    // For the foreign calls we want to handle, we do not want inputs, as they are getters
+    assert!(inputs.len() == 2); // output, len - but we dont use this len - its for the oracle
+    assert!(destinations.len() == 1);
+
+    let slot_offset_maybe = inputs[0];
+    let slot_offset = match slot_offset_maybe {
+        ValueOrArray::MemoryAddress(slot_offset) => slot_offset.0,
+        _ => panic!("ForeignCall address destination should be a single value"),
+    };
+
+    let dest_offset_maybe = destinations[0];
+    let (dest_offset, src_size) = match dest_offset_maybe {
+        ValueOrArray::HeapArray(HeapArray { pointer, size }) => (pointer.0, size),
+        _ => panic!("Storage write address inputs should be an array of values"),
+    };
+
+    avm_instrs.push(AvmInstruction {
+        opcode: AvmOpcode::SLOAD,
+        indirect: Some(SECOND_OPERAND_INDIRECT),
+        operands: vec![
+            AvmOperand::U32 {
+                value: slot_offset as u32,
+            },
+            AvmOperand::U32 {
+                value: src_size as u32,
+            },
+            AvmOperand::U32 {
+                value: dest_offset as u32,
+            },
+        ],
+        ..Default::default()
+    })
+}
+
 /// Handle an AVM NULLIFIEREXISTS instruction
 /// (a nullifierExists brillig foreign call was encountered)
 /// Adds the new instruction to the avm instructions list.
@@ -483,7 +566,7 @@ fn handle_send_l2_to_l1_msg(
     destinations: &Vec<ValueOrArray>,
     inputs: &Vec<ValueOrArray>,
 ) {
-    if destinations.len() != 0 || inputs.len() != 2 {
+    if !destinations.is_empty() || inputs.len() != 2 {
         panic!(
             "Transpiler expects ForeignCall::SENDL2TOL1MSG to have 0 destinations and 2 inputs, got {} and {}",
             destinations.len(),

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

@@ -14,18 +14,23 @@ use avm::AVMContext;
 struct Context {
     private: Option<&mut PrivateContext>,
     public: Option<&mut PublicContext>,
+    public_vm: Option<&mut AVMContext>,
 }
 
 impl Context {
     pub fn private(context: &mut PrivateContext) -> Context {
-        Context { private: Option::some(context), public: Option::none() }
+        Context { private: Option::some(context), public: Option::none(), public_vm: Option::none() }
     }
 
     pub fn public(context: &mut PublicContext) -> Context {
-        Context { public: Option::some(context), private: Option::none() }
+        Context { public: Option::some(context), private: Option::none(), public_vm: Option::none() }
+    }
+
+    pub fn public_vm(context: &mut AVMContext) -> Context {
+        Context { public_vm: Option::some(context), public: Option::none(), private: Option::none() }
     }
 
     pub fn none() -> Context {
-        Context { public: Option::none(), private: Option::none() }
+        Context { public: Option::none(), private: Option::none(), public_vm: Option::none() }
     }
 }

noir-projects/aztec-nr/aztec/src/oracle/storage.nr

@@ -12,8 +12,8 @@ pub fn storage_read<N>(storage_slot: Field) -> [Field; N] {
 }
 
 #[oracle(storageWrite)]
-fn storage_write_oracle<N>(_storage_slot: Field, _values: [Field; N]) -> [Field; N] {}
+fn storage_write_oracle<N>(_storage_slot: Field, _values: [Field; N]) -> Field {}
 
 unconstrained pub fn storage_write<N>(storage_slot: Field, fields: [Field; N]) {
-    let _hash = storage_write_oracle(storage_slot, fields);
+    let _ = storage_write_oracle(storage_slot, fields);
 }

noir-projects/noir-contracts/contracts/avm_test_contract/src/main.nr

@@ -1,5 +1,6 @@
 contract AvmTest {
     // Libs
+    use dep::aztec::state_vars::PublicMutable;
     use dep::aztec::protocol_types::{address::{AztecAddress, EthAddress}, constants::L1_TO_L2_MESSAGE_LENGTH};
     use dep::compressed_string::CompressedString;
 
@@ -9,7 +10,21 @@ contract AvmTest {
     #[aztec(private)]
     fn constructor() {}
 
-    // Public-vm macro will prefix avm to the function name for transpilation
+    struct Storage {
+        owner: PublicMutable<AztecAddress>
+    }
+
+    #[aztec(public-vm)]
+    fn setAdmin() {
+        storage.owner.write(context.sender());
+    }
+
+    #[aztec(public-vm)]
+    fn setAndRead() -> pub AztecAddress {
+        storage.owner.write(context.sender());
+        storage.owner.read()
+    }
+
     #[aztec(public-vm)]
     fn addArgsReturn(argA: Field, argB: Field) -> pub Field {
         argA + argB

noir/noir-repo/aztec_macros/src/lib.rs

@@ -727,8 +727,14 @@ fn transform_function(
 /// Transform a function to work with AVM bytecode
 fn transform_vm_function(
     func: &mut NoirFunction,
-    _storage_defined: bool,
+    storage_defined: bool,
 ) -> Result<(), AztecMacroError> {
+    // Create access to storage
+    if storage_defined {
+        let storage = abstract_storage("public_vm", true);
+        func.def.body.0.insert(0, storage);
+    }
+
     // Push Avm context creation to the beginning of the function
     let create_context = create_avm_context()?;
     func.def.body.0.insert(0, create_context);

noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir.rs

@@ -32,7 +32,7 @@ use num_bigint::BigUint;
 /// The Brillig VM does not apply a limit to the memory address space,
 /// As a convention, we take use 64 bits. This means that we assume that
 /// memory has 2^64 memory slots.
-pub(crate) const BRILLIG_MEMORY_ADDRESSING_BIT_SIZE: u32 = 32;
+pub(crate) const BRILLIG_MEMORY_ADDRESSING_BIT_SIZE: u32 = 64;
 
 // Registers reserved in runtime for special purposes.
 pub(crate) enum ReservedRegisters {
@@ -562,6 +562,7 @@ impl BrilligContext {
         bit_size: u32,
     ) {
         self.debug_show.const_instruction(result, constant);
+
         self.push_opcode(BrilligOpcode::Const { destination: result, value: constant, bit_size });
     }
 

noir/noir-repo/test_programs/noir_test_failure/should_fail_suite_with_one_failure/Nargo.toml

@@ -0,0 +1,5 @@
+[package]
+name = "should_fail_with_mismatch"
+type = "bin"
+authors = [""]
+[dependencies]

yarn-project/simulator/src/acvm/oracle/oracle.ts

@@ -253,9 +253,11 @@ export class Oracle {
     return values.map(toACVMField);
   }
 
-  async storageWrite([startStorageSlot]: ACVMField[], values: ACVMField[]): Promise<ACVMField[]> {
-    const newValues = await this.typedOracle.storageWrite(fromACVMField(startStorageSlot), values.map(fromACVMField));
-    return newValues.map(toACVMField);
+  storageWrite([startStorageSlot]: ACVMField[], values: ACVMField[]) {
+    this.typedOracle.storageWrite(fromACVMField(startStorageSlot), values.map(fromACVMField));
+
+    // We return 0 here as we MUST return something, but the value is not used.
+    return '0';
   }
 
   emitEncryptedLog(

yarn-project/simulator/src/acvm/oracle/typed_oracle.ts

@@ -173,7 +173,7 @@ export abstract class TypedOracle {
     throw new Error('Not available.');
   }
 
-  storageWrite(_startStorageSlot: Fr, _values: Fr[]): Promise<Fr[]> {
+  storageWrite(_startStorageSlot: Fr, _values: Fr[]) {
     throw new Error('Not available.');
   }
 

yarn-project/simulator/src/avm/avm_memory_types.ts

@@ -255,6 +255,12 @@ export class TaggedMemory {
     }
   }
 
+  public checkIsValidMemoryOffsetTag(offset: number) {
+    if (this.getTag(offset) > TypeTag.UINT64) {
+      throw TagCheckError.forOffset(offset, TypeTag[this.getTag(offset)], 'UINT64');
+    }
+  }
+
   public static checkIsIntegralTag(tag: TypeTag) {
     if (![TypeTag.UINT8, TypeTag.UINT16, TypeTag.UINT32, TypeTag.UINT64, TypeTag.UINT128].includes(tag)) {
       throw TagCheckError.forTag(TypeTag[tag], 'integral');

yarn-project/simulator/src/avm/avm_simulator.test.ts

@@ -111,6 +111,64 @@ describe('AVM simulator', () => {
       });
     });
 
+    describe('Storage accesses', () => {
+      it('Should set a single value in storage', async () => {
+        // We are setting the owner
+        const slot = 1n;
+        const sender = AztecAddress.fromField(new Fr(1));
+        const address = AztecAddress.fromField(new Fr(420));
+
+        const context = initContext({
+          env: initExecutionEnvironment({ sender, address, storageAddress: address }),
+        });
+        const bytecode = getAvmTestContractBytecode('avm_setAdmin');
+        const results = await new AvmSimulator(context).executeBytecode(bytecode);
+
+        // Get contract function artifact
+        expect(results.reverted).toBe(false);
+
+        // Contract 420 - Storage slot 1 should contain the value 1
+        const worldState = context.persistableState.flush();
+
+        const storageSlot = worldState.currentStorageValue.get(address.toBigInt())!;
+        const adminSlotValue = storageSlot.get(slot);
+        expect(adminSlotValue).toEqual(sender.toField());
+
+        // Tracing
+        const storageTrace = worldState.storageWrites.get(address.toBigInt())!;
+        const slotTrace = storageTrace.get(slot);
+        expect(slotTrace).toEqual([sender.toField()]);
+      });
+
+      it('Should read a value from storage', async () => {
+        // We are setting the owner
+        const sender = AztecAddress.fromField(new Fr(1));
+        const address = AztecAddress.fromField(new Fr(420));
+
+        const context = initContext({
+          env: initExecutionEnvironment({ sender, address, storageAddress: address }),
+        });
+        const bytecode = getAvmTestContractBytecode('avm_setAndRead');
+        const results = await new AvmSimulator(context).executeBytecode(bytecode);
+
+        expect(results.reverted).toBe(false);
+
+        expect(results.output).toEqual([sender.toField()]);
+
+        const worldState = context.persistableState.flush();
+
+        // Test read trace
+        const storageReadTrace = worldState.storageReads.get(address.toBigInt())!;
+        const slotReadTrace = storageReadTrace.get(1n);
+        expect(slotReadTrace).toEqual([sender.toField()]);
+
+        // Test write trace
+        const storageWriteTrace = worldState.storageWrites.get(address.toBigInt())!;
+        const slotWriteTrace = storageWriteTrace.get(1n);
+        expect(slotWriteTrace).toEqual([sender.toField()]);
+      });
+    });
+
     describe('Test env getters from noir contract', () => {
       const testEnvGetter = async (valueName: string, value: any, functionName: string, globalVar: boolean = false) => {
         // Execute

yarn-project/simulator/src/avm/journal/trace.ts

@@ -46,7 +46,7 @@ export class WorldStateAccessTrace {
   }
 
   public tracePublicStorageWrite(storageAddress: Fr, slot: Fr, value: Fr) {
-    // TODO(4805): check if some threshold is reached for max storage writes
+    // TODO: check if some threshold is reached for max storage writes
     // (need access to parent length, or trace needs to be initialized with parent's contents)
     //const traced: TracedPublicStorageWrite = {
     //  callPointer: Fr.ZERO,
@@ -57,6 +57,7 @@ export class WorldStateAccessTrace {
     //  endLifetime: Fr.ZERO,
     //};
     //this.publicStorageWrites.push(traced);
+
     this.journalWrite(storageAddress, slot, value);
     this.incrementAccessCounter();
   }

yarn-project/simulator/src/avm/opcodes/addressing_mode.ts

@@ -1,6 +1,6 @@
 import { strict as assert } from 'assert';
 
-import { TaggedMemory, TypeTag } from '../avm_memory_types.js';
+import { TaggedMemory } from '../avm_memory_types.js';
 
 export enum AddressingMode {
   DIRECT,
@@ -51,7 +51,8 @@ export class Addressing {
     for (const [i, offset] of offsets.entries()) {
       switch (this.modePerOperand[i]) {
         case AddressingMode.INDIRECT:
-          mem.checkTag(TypeTag.UINT32, offset);
+          // NOTE(reviewer): less than equal is a deviation from the spec - i dont see why this shouldnt be possible!
+          mem.checkIsValidMemoryOffsetTag(offset);
           resolved[i] = Number(mem.get(offset).toBigInt());
           break;
         case AddressingMode.DIRECT:

yarn-project/simulator/src/avm/opcodes/external_calls.test.ts

@@ -70,7 +70,7 @@ describe('External Calls', () => {
       const successOffset = 7;
       const otherContextInstructionsBytecode = encodeToBytecode([
         new CalldataCopy(/*indirect=*/ 0, /*csOffset=*/ 0, /*copySize=*/ argsSize, /*dstOffset=*/ 0),
-        new SStore(/*indirect=*/ 0, /*srcOffset=*/ 0, /*slotOffset=*/ 0),
+        new SStore(/*indirect=*/ 0, /*srcOffset=*/ 0, /*size=*/ 1, /*slotOffset=*/ 0),
         new Return(/*indirect=*/ 0, /*retOffset=*/ 0, /*size=*/ 2),
       ]);
 
@@ -159,7 +159,7 @@ describe('External Calls', () => {
 
       const otherContextInstructions: Instruction[] = [
         new CalldataCopy(/*indirect=*/ 0, /*csOffset=*/ 0, /*copySize=*/ argsSize, /*dstOffset=*/ 0),
-        new SStore(/*indirect=*/ 0, /*srcOffset=*/ 1, /*slotOffset=*/ 0),
+        new SStore(/*indirect=*/ 0, /*srcOffset=*/ 1, /*size=*/ 1, /*slotOffset=*/ 0),
       ];
 
       const otherContextInstructionsBytecode = encodeToBytecode(otherContextInstructions);