fix(revert): "feat(avm): storage"

avm-transpiler/src/instructions.rs

@@ -7,7 +7,6 @@ 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,
-    SECOND_OPERAND_INDIRECT, ZEROTH_OPERAND_INDIRECT,
+    ZEROTH_OPERAND_INDIRECT,
 };
 use crate::opcodes::AvmOpcode;
 use crate::utils::{dbg_print_avm_program, dbg_print_brillig_program};
@@ -257,10 +257,8 @@ 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.is_empty() && destinations.len() == 1 => {
+        _ if inputs.len() == 0 && destinations.len() == 1 => {
             handle_getter_instruction(avm_instrs, function, destinations, inputs)
         }
         // Anything else.
@@ -363,7 +361,7 @@ fn handle_emit_note_hash_or_nullifier(
         "EMITNOTEHASH"
     };
 
-    if !destinations.is_empty() || inputs.len() != 1 {
+    if destinations.len() != 0 || inputs.len() != 1 {
         panic!(
             "Transpiler expects ForeignCall::{} to have 0 destinations and 1 input, got {} and {}",
             function_name,
@@ -392,87 +390,6 @@ 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.
@@ -566,7 +483,7 @@ fn handle_send_l2_to_l1_msg(
     destinations: &Vec<ValueOrArray>,
     inputs: &Vec<ValueOrArray>,
 ) {
-    if !destinations.is_empty() || inputs.len() != 2 {
+    if destinations.len() != 0 || 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,23 +14,18 @@ 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(), public_vm: Option::none() }
+        Context { private: Option::some(context), public: Option::none() }
     }
 
     pub fn public(context: &mut PublicContext) -> Context {
-        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() }
+        Context { public: Option::some(context), private: Option::none() }
     }
 
     pub fn none() -> Context {
-        Context { public: Option::none(), private: Option::none(), public_vm: Option::none() }
+        Context { public: Option::none(), private: 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 {}
+fn storage_write_oracle<N>(_storage_slot: Field, _values: [Field; N]) -> [Field; N] {}
 
 unconstrained pub fn storage_write<N>(storage_slot: Field, fields: [Field; N]) {
-    let _ = storage_write_oracle(storage_slot, fields);
+    let _hash = storage_write_oracle(storage_slot, fields);
 }

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

@@ -1,6 +1,5 @@
 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;
 
@@ -10,21 +9,7 @@ contract AvmTest {
     #[aztec(private)]
     fn constructor() {}
 
-    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()
-    }
-
+    // Public-vm macro will prefix avm to the function name for transpilation
     #[aztec(public-vm)]
     fn addArgsReturn(argA: Field, argB: Field) -> pub Field {
         argA + argB

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

@@ -727,14 +727,8 @@ 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 = 64;
+pub(crate) const BRILLIG_MEMORY_ADDRESSING_BIT_SIZE: u32 = 32;
 
 // Registers reserved in runtime for special purposes.
 pub(crate) enum ReservedRegisters {
@@ -562,7 +562,6 @@ impl BrilligContext {
         bit_size: u32,
     ) {
         self.debug_show.const_instruction(result, constant);
-
         self.push_opcode(BrilligOpcode::Const { destination: result, value: constant, bit_size });
     }
 

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

@@ -253,11 +253,9 @@ export class Oracle {
     return values.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';
+  async storageWrite([startStorageSlot]: ACVMField[], values: ACVMField[]): Promise<ACVMField[]> {
+    const newValues = await this.typedOracle.storageWrite(fromACVMField(startStorageSlot), values.map(fromACVMField));
+    return newValues.map(toACVMField);
   }
 
   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[]) {
+  storageWrite(_startStorageSlot: Fr, _values: Fr[]): Promise<Fr[]> {
     throw new Error('Not available.');
   }
 

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

@@ -255,12 +255,6 @@ 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,64 +111,6 @@ 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: check if some threshold is reached for max storage writes
+    // TODO(4805): 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,7 +57,6 @@ 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 } from '../avm_memory_types.js';
+import { TaggedMemory, TypeTag } from '../avm_memory_types.js';
 
 export enum AddressingMode {
   DIRECT,
@@ -51,8 +51,7 @@ export class Addressing {
     for (const [i, offset] of offsets.entries()) {
       switch (this.modePerOperand[i]) {
         case AddressingMode.INDIRECT:
-          // NOTE(reviewer): less than equal is a deviation from the spec - i dont see why this shouldnt be possible!
-          mem.checkIsValidMemoryOffsetTag(offset);
+          mem.checkTag(TypeTag.UINT32, 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, /*size=*/ 1, /*slotOffset=*/ 0),
+        new SStore(/*indirect=*/ 0, /*srcOffset=*/ 0, /*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, /*size=*/ 1, /*slotOffset=*/ 0),
+        new SStore(/*indirect=*/ 0, /*srcOffset=*/ 1, /*slotOffset=*/ 0),
       ];
 
       const otherContextInstructionsBytecode = encodeToBytecode(otherContextInstructions);