From 95eadd67a72a286f07876f80586e6d57605d0af5 Mon Sep 17 00:00:00 2001 From: Facundo Date: Tue, 16 Apr 2024 15:37:44 +0100 Subject: [PATCH] feat(avm): take sizeOffset in CALL (#5763) CC @Thunkar. --- avm-transpiler/src/transpile.rs | 38 ++++++++++++------- .../public-vm/gen/_instruction-set.mdx | 6 +-- .../InstructionSet/InstructionSet.js | 7 ++-- .../aztec-nr/aztec/src/context/avm_context.nr | 16 ++++---- .../src/avm/opcodes/external_calls.test.ts | 22 +++++++---- .../src/avm/opcodes/external_calls.ts | 13 ++++--- 6 files changed, 61 insertions(+), 41 deletions(-) diff --git a/avm-transpiler/src/transpile.rs b/avm-transpiler/src/transpile.rs index 91345424a3c..581438c2ccb 100644 --- a/avm-transpiler/src/transpile.rs +++ b/avm-transpiler/src/transpile.rs @@ -2,7 +2,7 @@ use acvm::acir::brillig::Opcode as BrilligOpcode; use acvm::acir::circuit::brillig::Brillig; use acvm::brillig_vm::brillig::{ - BinaryFieldOp, BinaryIntOp, BlackBoxOp, HeapArray, MemoryAddress, ValueOrArray, + BinaryFieldOp, BinaryIntOp, BlackBoxOp, HeapArray, HeapVector, MemoryAddress, ValueOrArray, }; use acvm::FieldElement; @@ -380,16 +380,16 @@ fn handle_external_call( inputs: &Vec, opcode: AvmOpcode, ) { - if destinations.len() != 2 || inputs.len() != 4 { + if destinations.len() != 2 || inputs.len() != 5 { panic!( - "Transpiler expects ForeignCall (Static)Call to have 2 destinations and 4 inputs, got {} and {}. + "Transpiler expects ForeignCall (Static)Call to have 2 destinations and 5 inputs, got {} and {}. Make sure your call instructions's input/return arrays have static length (`[Field; ]`)!", destinations.len(), inputs.len() ); } - let gas_offset_maybe = inputs[0]; - let gas_offset = match gas_offset_maybe { + let gas = inputs[0]; + let gas_offset = match gas { ValueOrArray::HeapArray(HeapArray { pointer, size }) => { assert!(size == 3, "Call instruction's gas input should be a HeapArray of size 3 (`[l1Gas, l2Gas, daGas]`)"); pointer.0 as u32 @@ -401,13 +401,16 @@ fn handle_external_call( ValueOrArray::MemoryAddress(offset) => offset.to_usize() as u32, _ => panic!("Call instruction's target address input should be a basic MemoryAddress",), }; - let args_offset_maybe = inputs[2]; - let (args_offset, args_size) = match args_offset_maybe { - ValueOrArray::HeapArray(HeapArray { pointer, size }) => (pointer.0 as u32, size as u32), - ValueOrArray::HeapVector(_) => panic!("Call instruction's args must be a HeapArray, not a HeapVector. Make sure you are explicitly defining its size (`[arg0, arg1, ... argN]`)!"), - _ => panic!("Call instruction's args input should be a HeapArray input"), + // The args are a slice, and this is represented as a (Field, HeapVector). + // The field is the length (memory address) and the HeapVector has the data and length again. + // This is an ACIR internal representation detail that leaks to the SSA. + // Observe that below, we use `inputs[3]` and therefore skip the length field. + let args = inputs[3]; + let (args_offset, args_size_offset) = match args { + ValueOrArray::HeapVector(HeapVector { pointer, size }) => (pointer.0 as u32, size.0 as u32), + _ => panic!("Call instruction's args input should be a HeapVector input"), }; - let temporary_function_selector_offset = match &inputs[3] { + let temporary_function_selector_offset = match &inputs[4] { ValueOrArray::MemoryAddress(offset) => offset.to_usize() as u32, _ => panic!( "Call instruction's temporary function selector input should be a basic MemoryAddress", @@ -426,14 +429,23 @@ fn handle_external_call( }; avm_instrs.push(AvmInstruction { opcode: opcode, - indirect: Some(0b01101), // (left to right) selector direct, ret offset INDIRECT, args offset INDIRECT, address offset direct, gas offset INDIRECT + // (left to right) + // * selector direct + // * ret offset INDIRECT + // * arg size offset direct + // * args offset INDIRECT + // * address offset direct + // * gas offset INDIRECT + indirect: Some(0b010101), operands: vec![ AvmOperand::U32 { value: gas_offset }, AvmOperand::U32 { value: address_offset, }, AvmOperand::U32 { value: args_offset }, - AvmOperand::U32 { value: args_size }, + AvmOperand::U32 { + value: args_size_offset, + }, AvmOperand::U32 { value: ret_offset }, AvmOperand::U32 { value: ret_size }, AvmOperand::U32 { diff --git a/docs/docs/protocol-specs/public-vm/gen/_instruction-set.mdx b/docs/docs/protocol-specs/public-vm/gen/_instruction-set.mdx index 43dfced8b9a..0467dcd11df 100644 --- a/docs/docs/protocol-specs/public-vm/gen/_instruction-set.mdx +++ b/docs/docs/protocol-specs/public-vm/gen/_instruction-set.mdx @@ -1700,7 +1700,7 @@ Call into another contract - **gasOffset**: offset to three words containing `{l1GasLeft, l2GasLeft, daGasLeft}`: amount of gas to provide to the callee - **addrOffset**: address of the contract to call - **argsOffset**: memory offset to args (will become the callee's calldata) - - **argsSize**: number of words to pass via callee's calldata + - **argsSizeOffset**: memory offset for the number of words to pass via callee's calldata - **retOffset**: destination memory offset specifying where to store the data returned from the callee - **retSize**: number of words to copy from data returned by callee - **successOffset**: destination memory offset specifying where to store the call's success (0: failure, 1: success) @@ -1748,7 +1748,7 @@ Call into another contract, disallowing World State and Accrued Substate modific - **gasOffset**: offset to three words containing `{l1GasLeft, l2GasLeft, daGasLeft}`: amount of gas to provide to the callee - **addrOffset**: address of the contract to call - **argsOffset**: memory offset to args (will become the callee's calldata) - - **argsSize**: number of words to pass via callee's calldata + - **argsSizeOffset**: memory offset for the number of words to pass via callee's calldata - **retOffset**: destination memory offset specifying where to store the data returned from the callee - **retSize**: number of words to copy from data returned by callee - **successOffset**: destination memory offset specifying where to store the call's success (0: failure, 1: success) @@ -1793,7 +1793,7 @@ Call into another contract, but keep the caller's `sender` and `storageAddress` - **gasOffset**: offset to three words containing `{l1GasLeft, l2GasLeft, daGasLeft}`: amount of gas to provide to the callee - **addrOffset**: address of the contract to call - **argsOffset**: memory offset to args (will become the callee's calldata) - - **argsSize**: number of words to pass via callee's calldata + - **argsSizeOffset**: memory offset for the number of words to pass via callee's calldata - **retOffset**: destination memory offset specifying where to store the data returned from the callee - **retSize**: number of words to copy from data returned by callee - **successOffset**: destination memory offset specifying where to store the call's success (0: failure, 1: success) diff --git a/docs/src/preprocess/InstructionSet/InstructionSet.js b/docs/src/preprocess/InstructionSet/InstructionSet.js index 936a15b0c23..c6c3721963d 100644 --- a/docs/src/preprocess/InstructionSet/InstructionSet.js +++ b/docs/src/preprocess/InstructionSet/InstructionSet.js @@ -38,10 +38,9 @@ const CALL_INSTRUCTION_ARGS = [ description: "memory offset to args (will become the callee's calldata)", }, { - name: "argsSize", - description: "number of words to pass via callee's calldata", - mode: "immediate", - type: "u32", + name: "argsSizeOffset", + description: + "memory offset for the number of words to pass via callee's calldata", }, { name: "retOffset", diff --git a/noir-projects/aztec-nr/aztec/src/context/avm_context.nr b/noir-projects/aztec-nr/aztec/src/context/avm_context.nr index 3a8dd4bb9bf..504dec77522 100644 --- a/noir-projects/aztec-nr/aztec/src/context/avm_context.nr +++ b/noir-projects/aztec-nr/aztec/src/context/avm_context.nr @@ -63,7 +63,7 @@ impl AvmContext { call( gas_for_call(gas), contract_address, - args, + args.as_slice(), temporary_function_selector ) } @@ -78,7 +78,7 @@ impl AvmContext { call_static( gas_for_call(gas), contract_address, - args, + args.as_slice(), temporary_function_selector ) } @@ -149,7 +149,7 @@ impl PublicContextInterface for AvmContext { let results = call( gas_for_call(gas_opts), contract_address, - args, + args.as_slice(), temporary_function_selector.to_field() ); let data_to_return: [Field; RETURNS_COUNT] = results.0; @@ -169,7 +169,7 @@ impl PublicContextInterface for AvmContext { let (data_to_return, success): ([Field; RETURNS_COUNT], u8) = call_static( gas_for_call(gas_opts), contract_address, - args, + args.as_slice(), temporary_function_selector.to_field() ); @@ -296,20 +296,20 @@ fn l1_to_l2_msg_exists(msg_hash: Field, msg_leaf_index: Field) -> u8 {} fn send_l2_to_l1_msg(recipient: EthAddress, content: Field) {} #[oracle(avmOpcodeCall)] -fn call( +fn call( gas: [Field; 3], // gas allocation: [l1_gas, l2_gas, da_gas] address: AztecAddress, - args: [Field; ARGS_COUNT], + args: [Field], // TODO(5110): consider passing in calldata directly temporary_function_selector: Field ) -> ([Field; RET_SIZE], u8) {} // ^ return data ^ success #[oracle(avmOpcodeStaticCall)] -fn call_static( +fn call_static( gas: [Field; 3], // gas allocation: [l1_gas, l2_gas, da_gas] address: AztecAddress, - args: [Field; ARGS_COUNT], + args: [Field], // TODO(5110): consider passing in calldata directly temporary_function_selector: Field ) -> ([Field; RET_SIZE], u8) {} diff --git a/yarn-project/simulator/src/avm/opcodes/external_calls.test.ts b/yarn-project/simulator/src/avm/opcodes/external_calls.test.ts index c9acf97b61e..29c6626fa8d 100644 --- a/yarn-project/simulator/src/avm/opcodes/external_calls.test.ts +++ b/yarn-project/simulator/src/avm/opcodes/external_calls.test.ts @@ -6,7 +6,7 @@ import { mock } from 'jest-mock-extended'; import { type CommitmentsDB, type PublicContractsDB, type PublicStateDB } from '../../index.js'; import { markBytecodeAsAvm } from '../../public/transitional_adaptors.js'; import { type AvmContext } from '../avm_context.js'; -import { Field, Uint8 } from '../avm_memory_types.js'; +import { Field, Uint8, Uint32 } from '../avm_memory_types.js'; import { adjustCalldataIndex, initContext } from '../fixtures/index.js'; import { HostStorage } from '../journal/host_storage.js'; import { AvmPersistableStateManager } from '../journal/journal.js'; @@ -36,7 +36,7 @@ describe('External Calls', () => { ...Buffer.from('12345678', 'hex'), // gasOffset ...Buffer.from('a2345678', 'hex'), // addrOffset ...Buffer.from('b2345678', 'hex'), // argsOffset - ...Buffer.from('c2345678', 'hex'), // argsSize + ...Buffer.from('c2345678', 'hex'), // argsSizeOffset ...Buffer.from('d2345678', 'hex'), // retOffset ...Buffer.from('e2345678', 'hex'), // retSize ...Buffer.from('f2345678', 'hex'), // successOffset @@ -47,7 +47,7 @@ describe('External Calls', () => { /*gasOffset=*/ 0x12345678, /*addrOffset=*/ 0xa2345678, /*argsOffset=*/ 0xb2345678, - /*argsSize=*/ 0xc2345678, + /*argsSizeOffset=*/ 0xc2345678, /*retOffset=*/ 0xd2345678, /*retSize=*/ 0xe2345678, /*successOffset=*/ 0xf2345678, @@ -68,6 +68,7 @@ describe('External Calls', () => { const argsOffset = 4; const args = [new Field(1n), new Field(2n), new Field(3n)]; const argsSize = args.length; + const argsSizeOffset = 20; const retOffset = 8; const retSize = 2; const successOffset = 7; @@ -93,6 +94,7 @@ describe('External Calls', () => { context.machineState.memory.set(1, new Field(l2Gas)); context.machineState.memory.set(2, new Field(daGas)); context.machineState.memory.set(3, new Field(addr)); + context.machineState.memory.set(argsSizeOffset, new Uint32(argsSize)); context.machineState.memory.setSlice(4, args); jest .spyOn(context.persistableState.hostStorage.contractsDb, 'getBytecode') @@ -103,7 +105,7 @@ describe('External Calls', () => { gasOffset, addrOffset, argsOffset, - argsSize, + argsSizeOffset, retOffset, retSize, successOffset, @@ -145,6 +147,7 @@ describe('External Calls', () => { const argsOffset = 4; const args = [new Field(1n), new Field(2n), new Field(3n)]; const argsSize = args.length; + const argsSizeOffset = 20; const retOffset = 8; const retSize = 2; const successOffset = 7; @@ -153,6 +156,7 @@ describe('External Calls', () => { context.machineState.memory.set(1, new Field(l2Gas)); context.machineState.memory.set(2, new Field(daGas)); context.machineState.memory.set(3, new Field(addr)); + context.machineState.memory.set(argsSizeOffset, new Uint32(argsSize)); context.machineState.memory.setSlice(4, args); jest @@ -164,7 +168,7 @@ describe('External Calls', () => { gasOffset, addrOffset, argsOffset, - argsSize, + argsSizeOffset, retOffset, retSize, successOffset, @@ -183,7 +187,7 @@ describe('External Calls', () => { ...Buffer.from('12345678', 'hex'), // gasOffset ...Buffer.from('a2345678', 'hex'), // addrOffset ...Buffer.from('b2345678', 'hex'), // argsOffset - ...Buffer.from('c2345678', 'hex'), // argsSize + ...Buffer.from('c2345678', 'hex'), // argsSizeOffset ...Buffer.from('d2345678', 'hex'), // retOffset ...Buffer.from('e2345678', 'hex'), // retSize ...Buffer.from('f2345678', 'hex'), // successOffset @@ -194,7 +198,7 @@ describe('External Calls', () => { /*gasOffset=*/ 0x12345678, /*addrOffset=*/ 0xa2345678, /*argsOffset=*/ 0xb2345678, - /*argsSize=*/ 0xc2345678, + /*argsSizeOffset=*/ 0xc2345678, /*retOffset=*/ 0xd2345678, /*retSize=*/ 0xe2345678, /*successOffset=*/ 0xf2345678, @@ -214,12 +218,14 @@ describe('External Calls', () => { const args = [new Field(1n), new Field(2n), new Field(3n)]; const argsSize = args.length; + const argsSizeOffset = 20; const retOffset = 8; const retSize = 2; const successOffset = 7; context.machineState.memory.set(0, gas); context.machineState.memory.set(1, addr); + context.machineState.memory.set(argsSizeOffset, new Uint32(argsSize)); context.machineState.memory.setSlice(2, args); const otherContextInstructions: Instruction[] = [ @@ -243,7 +249,7 @@ describe('External Calls', () => { gasOffset, addrOffset, argsOffset, - argsSize, + argsSizeOffset, retOffset, retSize, successOffset, diff --git a/yarn-project/simulator/src/avm/opcodes/external_calls.ts b/yarn-project/simulator/src/avm/opcodes/external_calls.ts index 2d25634055a..fd54c6dbfe6 100644 --- a/yarn-project/simulator/src/avm/opcodes/external_calls.ts +++ b/yarn-project/simulator/src/avm/opcodes/external_calls.ts @@ -36,7 +36,7 @@ abstract class ExternalCall extends Instruction { private gasOffset: number /* Unused due to no formal gas implementation at this moment */, private addrOffset: number, private argsOffset: number, - private argsSize: number, + private argsSizeOffset: number, private retOffset: number, private retSize: number, private successOffset: number, @@ -50,20 +50,23 @@ abstract class ExternalCall extends Instruction { public async execute(context: AvmContext) { const memory = context.machineState.memory.track(this.type); - const [gasOffset, addrOffset, argsOffset, retOffset, successOffset] = Addressing.fromWire(this.indirect).resolve( - [this.gasOffset, this.addrOffset, this.argsOffset, this.retOffset, this.successOffset], + const [gasOffset, addrOffset, argsOffset, argsSizeOffset, retOffset, successOffset] = Addressing.fromWire( + this.indirect, + ).resolve( + [this.gasOffset, this.addrOffset, this.argsOffset, this.argsSizeOffset, this.retOffset, this.successOffset], memory, ); const callAddress = memory.getAs(addrOffset); - const calldata = memory.getSlice(argsOffset, this.argsSize).map(f => f.toFr()); + const calldataSize = memory.get(argsSizeOffset).toNumber(); + const calldata = memory.getSlice(argsOffset, calldataSize).map(f => f.toFr()); const l1Gas = memory.get(gasOffset).toNumber(); const l2Gas = memory.getAs(gasOffset + 1).toNumber(); const daGas = memory.getAs(gasOffset + 2).toNumber(); const functionSelector = memory.getAs(this.temporaryFunctionSelectorOffset).toFr(); const allocatedGas = { l1Gas, l2Gas, daGas }; - const memoryOperations = { reads: this.argsSize + 5, writes: 1 + this.retSize, indirect: this.indirect }; + const memoryOperations = { reads: calldataSize + 6, writes: 1 + this.retSize, indirect: this.indirect }; const totalGas = sumGas(this.gasCost(memoryOperations), allocatedGas); context.machineState.consumeGas(totalGas);