From 26adc55771a204f96e8594f6defde2a4872c88d2 Mon Sep 17 00:00:00 2001 From: Facundo Date: Tue, 29 Oct 2024 17:57:21 +0000 Subject: [PATCH] feat(avm)!: cleanup CALL (#9551) * (Static)CALL now returns just the success bit * Properly returns U1 instead of U8 * Removed FunctionSelector Fixes #8998. Part of #9061. --- avm-transpiler/src/transpile.rs | 48 ++++-------- .../vm/avm/tests/execution.test.cpp | 45 +++++------ .../vm/avm/trace/deserialization.cpp | 5 +- .../barretenberg/vm/avm/trace/execution.cpp | 10 +-- .../src/barretenberg/vm/avm/trace/trace.cpp | 77 ++++--------------- .../src/barretenberg/vm/avm/trace/trace.hpp | 15 +--- .../aztec/src/context/public_context.nr | 52 +++---------- .../src/avm/opcodes/external_calls.test.ts | 63 +++------------ .../src/avm/opcodes/external_calls.ts | 51 +++--------- .../bytecode_serialization.test.ts | 12 --- yarn-project/txe/src/oracle/txe_oracle.ts | 12 +-- .../txe/src/txe_service/txe_service.ts | 13 ++-- 12 files changed, 99 insertions(+), 304 deletions(-) diff --git a/avm-transpiler/src/transpile.rs b/avm-transpiler/src/transpile.rs index 59311033991..799d0017b6c 100644 --- a/avm-transpiler/src/transpile.rs +++ b/avm-transpiler/src/transpile.rs @@ -419,27 +419,34 @@ fn handle_foreign_call( /// Handle an AVM CALL /// (an external 'call' brillig foreign call was encountered) /// Adds the new instruction to the avm instructions list. +// #[oracle(avmOpcodeCall)] +// unconstrained fn call_opcode( +// gas: [Field; 2], // gas allocation: [l2_gas, da_gas] +// address: AztecAddress, +// args: [Field], +// ) -> bool {} fn handle_external_call( avm_instrs: &mut Vec, destinations: &Vec, inputs: &Vec, opcode: AvmOpcode, ) { - if destinations.len() != 2 || inputs.len() != 5 { + if destinations.len() != 1 || inputs.len() != 4 { panic!( - "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; ]`)!", + "Transpiler expects ForeignCall (Static)Call to have 1 destinations and 4 inputs, got {} and {}.", destinations.len(), inputs.len() ); } - let gas = inputs[0]; - let gas_offset_ptr = match gas { + + let gas_offset_ptr = match &inputs[0] { ValueOrArray::HeapArray(HeapArray { pointer, size }) => { - assert!(size == 2, "Call instruction's gas input should be a HeapArray of size 2 (`[l2Gas, daGas]`)"); + assert!( + *size == 2, + "Call instruction's gas input should be a HeapArray of size 2 (`[l2Gas, daGas]`)" + ); pointer } - ValueOrArray::HeapVector(_) => panic!("Call instruction's gas input must be a HeapArray, not a HeapVector. Make sure you are explicitly defining its size as 2 (`[l2Gas, daGas]`)!"), _ => panic!("Call instruction's gas input should be a HeapArray"), }; let address_offset = match &inputs[1] { @@ -450,45 +457,25 @@ fn handle_external_call( // 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 = &inputs[3]; let (args_offset_ptr, args_size_offset) = match args { ValueOrArray::HeapVector(HeapVector { pointer, size }) => (pointer, size), _ => panic!("Call instruction's args input should be a HeapVector input"), }; - let function_selector_offset = match &inputs[4] { - ValueOrArray::MemoryAddress(offset) => offset, - _ => panic!("Call instruction's function selector input should be a basic MemoryAddress",), - }; - let ret_offset_maybe = destinations[0]; - let (ret_offset_ptr, ret_size) = match ret_offset_maybe { - ValueOrArray::HeapArray(HeapArray { pointer, size }) => (pointer, size as u32), - ValueOrArray::HeapVector(_) => panic!("Call instruction's return data must be a HeapArray, not a HeapVector. Make sure you are explicitly defining its size (`let returnData: [Field; ] = ...`)!"), - _ => panic!("Call instruction's returnData destination should be a HeapArray input"), - }; - let success_offset = match &destinations[1] { + let success_offset = match &destinations[0] { ValueOrArray::MemoryAddress(offset) => offset, _ => panic!("Call instruction's success destination should be a basic MemoryAddress",), }; avm_instrs.push(AvmInstruction { opcode, - // * gas offset INDIRECT - // * address offset direct - // * args offset INDIRECT - // * arg size offset direct - // * ret offset INDIRECT - // * (n/a) ret size is an immediate - // * success offset direct - // * selector direct indirect: Some( AddressingModeBuilder::default() .indirect_operand(&gas_offset_ptr) .direct_operand(address_offset) .indirect_operand(&args_offset_ptr) .direct_operand(&args_size_offset) - .indirect_operand(&ret_offset_ptr) .direct_operand(success_offset) - .direct_operand(function_selector_offset) .build(), ), operands: vec![ @@ -496,10 +483,7 @@ fn handle_external_call( AvmOperand::U16 { value: address_offset.to_usize() as u16 }, AvmOperand::U16 { value: args_offset_ptr.to_usize() as u16 }, AvmOperand::U16 { value: args_size_offset.to_usize() as u16 }, - AvmOperand::U16 { value: ret_offset_ptr.to_usize() as u16 }, - AvmOperand::U16 { value: ret_size as u16 }, AvmOperand::U16 { value: success_offset.to_usize() as u16 }, - AvmOperand::U16 { value: function_selector_offset.to_usize() as u16 }, ], ..Default::default() }); diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/tests/execution.test.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/tests/execution.test.cpp index 4837b592ea4..1f00b899d9a 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/tests/execution.test.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/tests/execution.test.cpp @@ -2101,31 +2101,33 @@ TEST_F(AvmExecutionTests, opCallOpcodes) + to_hex(AvmMemoryTag::U32) + "07" // val "01" + - to_hex(OpCode::CALLDATACOPY) + // opcode CALLDATACOPY - "00" // Indirect flag - "0000" // cd_offset - "0001" // copy_size - "0000" // dst_offset - + bytecode_preamble // Load up memory offsets - + to_hex(OpCode::CALL) + // opcode CALL - "003f" // Indirect flag - "0011" // gas offset - "0012" // addr offset - "0013" // args offset - "0014" // args size offset - "0015" // ret offset - "0002" // ret size - "0016" // success offset - "0017" // function_selector_offset - + to_hex(OpCode::RETURN) + // opcode RETURN - "00" // Indirect flag - "0100" // ret offset 8 - "0003"; // ret size 3 (extra read is for the success flag) + to_hex(OpCode::CALLDATACOPY) + // opcode CALLDATACOPY + "00" // Indirect flag + "0000" // cd_offset + "0001" // copy_size + "0000" // dst_offset + + bytecode_preamble // Load up memory offsets + + to_hex(OpCode::CALL) + // opcode CALL + "001f" // Indirect flag + "0011" // gas offset + "0012" // addr offset + "0013" // args offset + "0014" // args size offset + "0016" // success offset + + to_hex(OpCode::RETURNDATACOPY) + // opcode RETURNDATACOPY + "00" // Indirect flag + "0011" // start offset (0) + "0012" // ret size (2) + "0100" // dst offset + + to_hex(OpCode::RETURN) + // opcode RETURN + "00" // Indirect flag + "0100" // ret offset 8 + "0003"; // ret size 3 (extra read is for the success flag) auto bytecode = hex_to_bytes(bytecode_hex); auto instructions = Deserialization::parse(bytecode); - std::vector returndata = {}; + std::vector returndata; // Generate Hint for call operation auto execution_hints = ExecutionHints().with_externalcall_hints({ { @@ -2219,7 +2221,6 @@ TEST_F(AvmExecutionTests, opGetContractInstanceOpcode) TEST_F(AvmExecutionTests, opGetContractInstanceOpcodeBadEnum) { const uint8_t address_byte = 0x42; - const FF address(address_byte); std::string bytecode_hex = to_hex(OpCode::SET_8) + // opcode SET "00" // Indirect flag diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/deserialization.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/deserialization.cpp index c692d13611a..8e203151ded 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/trace/deserialization.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/trace/deserialization.cpp @@ -34,10 +34,7 @@ const std::vector external_call_format = { OperandType::INDIRECT16, /*addrOffset=*/OperandType::UINT16, /*argsOffset=*/OperandType::UINT16, /*argsSize=*/OperandType::UINT16, - /*retOffset=*/OperandType::UINT16, - /*retSize*/ OperandType::UINT16, - /*successOffset=*/OperandType::UINT16, - /*functionSelector=*/OperandType::UINT16 }; + /*successOffset=*/OperandType::UINT16 }; // Contrary to TS, the format does not contain the opcode byte which prefixes any instruction. // The format for OpCode::SET has to be handled separately as it is variable based on the tag. diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/execution.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/execution.cpp index 799f75c6b52..5ab799bf4ef 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/trace/execution.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/trace/execution.cpp @@ -644,10 +644,7 @@ std::vector Execution::gen_trace(std::vector const& calldata, std::get(inst.operands.at(2)), std::get(inst.operands.at(3)), std::get(inst.operands.at(4)), - std::get(inst.operands.at(5)), - std::get(inst.operands.at(6)), - std::get(inst.operands.at(7)), - std::get(inst.operands.at(8))); + std::get(inst.operands.at(5))); break; case OpCode::STATICCALL: trace_builder.op_static_call(std::get(inst.operands.at(0)), @@ -655,10 +652,7 @@ std::vector Execution::gen_trace(std::vector const& calldata, std::get(inst.operands.at(2)), std::get(inst.operands.at(3)), std::get(inst.operands.at(4)), - std::get(inst.operands.at(5)), - std::get(inst.operands.at(6)), - std::get(inst.operands.at(7)), - std::get(inst.operands.at(8))); + std::get(inst.operands.at(5))); break; case OpCode::RETURN: { auto ret = trace_builder.op_return(std::get(inst.operands.at(0)), diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp index 4faac1365fc..a0aa672c22e 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp @@ -2655,10 +2655,7 @@ void AvmTraceBuilder::constrain_external_call(OpCode opcode, uint32_t addr_offset, uint32_t args_offset, uint32_t args_size_offset, - uint32_t ret_offset, - uint32_t ret_size, - uint32_t success_offset, - uint32_t function_selector_offset) + uint32_t success_offset) { ASSERT(opcode == OpCode::CALL || opcode == OpCode::STATICCALL); auto clk = static_cast(main_trace.size()) + 1; @@ -2668,17 +2665,9 @@ void AvmTraceBuilder::constrain_external_call(OpCode opcode, resolved_addr_offset, resolved_args_offset, resolved_args_size_offset, - resolved_ret_offset, - resolved_success_offset, - resolved_function_selector_offset] = Addressing<7>::fromWire(indirect, call_ptr) - .resolve({ gas_offset, - addr_offset, - args_offset, - args_size_offset, - ret_offset, - success_offset, - function_selector_offset }, - mem_trace_builder); + resolved_success_offset] = + Addressing<5>::fromWire(indirect, call_ptr) + .resolve({ gas_offset, addr_offset, args_offset, args_size_offset, success_offset }, mem_trace_builder); // Should read the address next to read_gas as well (tuple of gas values (l2Gas, daGas)) auto read_gas_l2 = constrained_read_from_memory( @@ -2696,7 +2685,7 @@ void AvmTraceBuilder::constrain_external_call(OpCode opcode, gas_trace_builder.constrain_gas(clk, opcode, - /*dyn_gas_multiplier=*/args_size + ret_size, + /*dyn_gas_multiplier=*/args_size, static_cast(hint.l2_gas_used), static_cast(hint.da_gas_used)); @@ -2728,16 +2717,8 @@ void AvmTraceBuilder::constrain_external_call(OpCode opcode, .main_tag_err = FF(static_cast(!tag_match)), }); - // The hint contains the FULL return data. - // TODO: Don't fail if we ask for too much data. - ASSERT(ret_size <= hint.return_data.size()); - - // Write the return data to memory - write_slice_to_memory(resolved_ret_offset, - AvmMemoryTag::FF, - std::vector(hint.return_data.begin(), hint.return_data.begin() + ret_size)); // Write the success flag to memory - write_slice_to_memory(resolved_success_offset, AvmMemoryTag::U8, std::vector{ hint.success }); + write_to_memory(resolved_success_offset, hint.success, AvmMemoryTag::U1); external_call_counter++; // Save return data for later. @@ -2759,33 +2740,18 @@ void AvmTraceBuilder::constrain_external_call(OpCode opcode, * @param addr_offset An index in memory pointing to the target contract address * @param args_offset An index in memory pointing to the first value of the input array for the external call * @param args_size The number of values in the input array for the external call - * @param ret_offset An index in memory pointing to where the first value of the external calls return value should - * be stored. - * @param ret_size The number of values in the return array * @param success_offset An index in memory pointing to where the success flag (U1) of the external call should be * stored - * @param function_selector_offset An index in memory pointing to the function selector of the external call (TEMP) */ void AvmTraceBuilder::op_call(uint16_t indirect, uint32_t gas_offset, uint32_t addr_offset, uint32_t args_offset, uint32_t args_size_offset, - uint32_t ret_offset, - uint32_t ret_size, - uint32_t success_offset, - [[maybe_unused]] uint32_t function_selector_offset) -{ - return constrain_external_call(OpCode::CALL, - indirect, - gas_offset, - addr_offset, - args_offset, - args_size_offset, - ret_offset, - ret_size, - success_offset, - function_selector_offset); + uint32_t success_offset) +{ + return constrain_external_call( + OpCode::CALL, indirect, gas_offset, addr_offset, args_offset, args_size_offset, success_offset); } /** @@ -2798,33 +2764,18 @@ void AvmTraceBuilder::op_call(uint16_t indirect, * @param addr_offset An index in memory pointing to the target contract address * @param args_offset An index in memory pointing to the first value of the input array for the external call * @param args_size The number of values in the input array for the static call - * @param ret_offset An index in memory pointing to where the first value of the static call return value should - * be stored. - * @param ret_size The number of values in the return array * @param success_offset An index in memory pointing to where the success flag (U8) of the static call should be * stored - * @param function_selector_offset An index in memory pointing to the function selector of the external call (TEMP) */ void AvmTraceBuilder::op_static_call(uint16_t indirect, uint32_t gas_offset, uint32_t addr_offset, uint32_t args_offset, uint32_t args_size_offset, - uint32_t ret_offset, - uint32_t ret_size, - uint32_t success_offset, - [[maybe_unused]] uint32_t function_selector_offset) -{ - return constrain_external_call(OpCode::STATICCALL, - indirect, - gas_offset, - addr_offset, - args_offset, - args_size_offset, - ret_offset, - ret_size, - success_offset, - function_selector_offset); + uint32_t success_offset) +{ + return constrain_external_call( + OpCode::STATICCALL, indirect, gas_offset, addr_offset, args_offset, args_size_offset, success_offset); } /** diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.hpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.hpp index 66d373502b4..f1d0b227abd 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.hpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.hpp @@ -133,19 +133,13 @@ class AvmTraceBuilder { uint32_t addr_offset, uint32_t args_offset, uint32_t args_size, - uint32_t ret_offset, - uint32_t ret_size, - uint32_t success_offset, - uint32_t function_selector_offset); + uint32_t success_offset); void op_static_call(uint16_t indirect, uint32_t gas_offset, uint32_t addr_offset, uint32_t args_offset, uint32_t args_size, - uint32_t ret_offset, - uint32_t ret_size, - uint32_t success_offset, - uint32_t function_selector_offset); + uint32_t success_offset); std::vector op_return(uint8_t indirect, uint32_t ret_offset, uint32_t ret_size); // REVERT Opcode (that just call return under the hood for now) std::vector op_revert(uint8_t indirect, uint32_t ret_offset, uint32_t ret_size_offset); @@ -263,10 +257,7 @@ class AvmTraceBuilder { uint32_t addr_offset, uint32_t args_offset, uint32_t args_size_offset, - uint32_t ret_offset, - uint32_t ret_size, - uint32_t success_offset, - [[maybe_unused]] uint32_t function_selector_offset); + uint32_t success_offset); void execute_gasleft(EnvironmentVariable var, uint8_t indirect, uint32_t dst_offset); diff --git a/noir-projects/aztec-nr/aztec/src/context/public_context.nr b/noir-projects/aztec-nr/aztec/src/context/public_context.nr index 83f7a37f067..e7b55e86299 100644 --- a/noir-projects/aztec-nr/aztec/src/context/public_context.nr +++ b/noir-projects/aztec-nr/aztec/src/context/public_context.nr @@ -77,16 +77,10 @@ impl PublicContext { gas_opts: GasOpts, ) -> [Field] { let args = args.push_front(function_selector.to_field()); - // TODO(https://github.com/AztecProtocol/aztec-packages/issues/9061): modify call opcode and remove <0>. - let (_, success) = call::<0>( - gas_for_call(gas_opts), - contract_address, - args, - PUBLIC_DISPATCH_SELECTOR, - ); + let success = call(gas_for_call(gas_opts), contract_address, args); let result_data = returndata_copy(0, returndata_size()); - if success == 0 { + if !success { // Rethrow the revert data. avm_revert(result_data); } @@ -101,16 +95,10 @@ impl PublicContext { gas_opts: GasOpts, ) -> [Field] { let args = args.push_front(function_selector.to_field()); - // TODO(https://github.com/AztecProtocol/aztec-packages/issues/9061): modify call_static opcode and remove <0>. - let (_, success) = call_static::<0>( - gas_for_call(gas_opts), - contract_address, - args, - PUBLIC_DISPATCH_SELECTOR, - ); + let success = call_static(gas_for_call(gas_opts), contract_address, args); let result_data = returndata_copy(0, returndata_size()); - if success == 0 { + if !success { // Rethrow the revert data. avm_revert(result_data); } @@ -277,21 +265,11 @@ unconstrained fn l1_to_l2_msg_exists(msg_hash: Field, msg_leaf_index: Field) -> unconstrained fn send_l2_to_l1_msg(recipient: EthAddress, content: Field) { send_l2_to_l1_msg_opcode(recipient, content) } -unconstrained fn call( - gas: [Field; 2], - address: AztecAddress, - args: [Field], - function_selector: Field, -) -> ([Field; RET_SIZE], u8) { - call_opcode(gas, address, args, function_selector) +unconstrained fn call(gas: [Field; 2], address: AztecAddress, args: [Field]) -> bool { + call_opcode(gas, address, args) } -unconstrained fn call_static( - gas: [Field; 2], - address: AztecAddress, - args: [Field], - function_selector: Field, -) -> ([Field; RET_SIZE], u8) { - call_static_opcode(gas, address, args, function_selector) +unconstrained fn call_static(gas: [Field; 2], address: AztecAddress, args: [Field]) -> bool { + call_static_opcode(gas, address, args) } pub unconstrained fn calldata_copy(cdoffset: u32, copy_size: u32) -> [Field; N] { @@ -417,24 +395,18 @@ unconstrained fn return_opcode(returndata: [Field; N]) {} unconstrained fn revert_opcode(revertdata: [Field]) {} #[oracle(avmOpcodeCall)] -unconstrained fn call_opcode( +unconstrained fn call_opcode( gas: [Field; 2], // gas allocation: [l2_gas, da_gas] address: AztecAddress, args: [Field], - // TODO(5110): consider passing in calldata directly - function_selector: Field, -) -> ([Field; RET_SIZE], u8) {} -// ^ return data ^ success +) -> bool {} #[oracle(avmOpcodeStaticCall)] -unconstrained fn call_static_opcode( +unconstrained fn call_static_opcode( gas: [Field; 2], // gas allocation: [l2_gas, da_gas] address: AztecAddress, args: [Field], - // TODO(5110): consider passing in calldata directly - function_selector: Field, -) -> ([Field; RET_SIZE], u8) {} -// ^ return data ^ success +) -> bool {} #[oracle(avmOpcodeStorageRead)] unconstrained fn storage_read_opcode(storage_slot: Field) -> Field {} 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 7fe8cc7365a..58dd8152391 100644 --- a/yarn-project/simulator/src/avm/opcodes/external_calls.test.ts +++ b/yarn-project/simulator/src/avm/opcodes/external_calls.test.ts @@ -7,7 +7,7 @@ import { mock } from 'jest-mock-extended'; import { type WorldStateDB } from '../../public/public_db_sources.js'; import { type PublicSideEffectTraceInterface } from '../../public/side_effect_trace_interface.js'; import { type AvmContext } from '../avm_context.js'; -import { Field, TypeTag, Uint8, Uint32 } from '../avm_memory_types.js'; +import { Field, TypeTag, Uint1, Uint32 } from '../avm_memory_types.js'; import { markBytecodeAsAvm } from '../bytecode_utils.js'; import { initContext, initPersistableStateManager } from '../fixtures/index.js'; import { type AvmPersistableStateManager } from '../journal/journal.js'; @@ -43,10 +43,7 @@ describe('External Calls', () => { ...Buffer.from('a234', 'hex'), // addrOffset ...Buffer.from('b234', 'hex'), // argsOffset ...Buffer.from('c234', 'hex'), // argsSizeOffset - ...Buffer.from('d234', 'hex'), // retOffset - ...Buffer.from('e234', 'hex'), // retSize ...Buffer.from('f234', 'hex'), // successOffset - ...Buffer.from('f334', 'hex'), // functionSelectorOffset ]); const inst = new Call( /*indirect=*/ 0x1234, @@ -54,10 +51,7 @@ describe('External Calls', () => { /*addrOffset=*/ 0xa234, /*argsOffset=*/ 0xb234, /*argsSizeOffset=*/ 0xc234, - /*retOffset=*/ 0xd234, - /*retSize=*/ 0xe234, /*successOffset=*/ 0xf234, - /*functionSelectorOffset=*/ 0xf334, ); expect(Call.deserialize(buf)).toEqual(inst); @@ -71,25 +65,16 @@ describe('External Calls', () => { const addrOffset = 2; const addr = new Fr(123456n); const argsOffset = 3; - const valueToStore = new Fr(42); - const valueOffset = 0; // 0th entry in calldata to nested call - const slot = new Fr(100); - const slotOffset = 1; // 1st entry in calldata to nested call - const args = [new Field(valueToStore), new Field(slot), new Field(3n)]; + const args = [new Field(1), new Field(2), new Field(3)]; const argsSize = args.length; const argsSizeOffset = 20; - const retOffset = 7; - const retSize = 2; - const expectedRetValue = args.slice(0, retSize); const successOffset = 6; - // const otherContextInstructionsL2GasCost = 780; // Includes the cost of the call itself const otherContextInstructionsBytecode = markBytecodeAsAvm( encodeToBytecode([ new Set(/*indirect=*/ 0, TypeTag.UINT32, 0, /*dstOffset=*/ 0).as(Opcode.SET_8, Set.wireFormat8), new Set(/*indirect=*/ 0, TypeTag.UINT32, argsSize, /*dstOffset=*/ 1).as(Opcode.SET_8, Set.wireFormat8), new CalldataCopy(/*indirect=*/ 0, /*csOffsetAddress=*/ 0, /*copySizeOffset=*/ 1, /*dstOffset=*/ 0), - new SStore(/*indirect=*/ 0, /*srcOffset=*/ valueOffset, /*slotOffset=*/ slotOffset), new Return(/*indirect=*/ 0, /*retOffset=*/ 0, /*size=*/ 2), ]), ); @@ -111,27 +96,14 @@ describe('External Calls', () => { context.machineState.memory.set(argsSizeOffset, new Uint32(argsSize)); context.machineState.memory.setSlice(3, args); - const instruction = new Call( - /*indirect=*/ 0, - gasOffset, - addrOffset, - argsOffset, - argsSizeOffset, - retOffset, - retSize, - successOffset, - /*functionSelectorOffset=*/ 0, - ); + const instruction = new Call(/*indirect=*/ 0, gasOffset, addrOffset, argsOffset, argsSizeOffset, successOffset); await instruction.execute(context); const successValue = context.machineState.memory.get(successOffset); - expect(successValue).toEqual(new Uint8(1n)); - - const retValue = context.machineState.memory.getSlice(retOffset, retSize); - expect(retValue).toEqual(expectedRetValue); + expect(successValue).toEqual(new Uint1(1n)); - // Check that the storage call has been merged into the parent journal - expect(await context.persistableState.peekStorage(addr, slot)).toEqual(valueToStore); + const retValue = context.machineState.nestedReturndata; + expect(retValue).toEqual([new Fr(1n), new Fr(2n)]); expect(context.machineState.l2GasLeft).toBeLessThan(initialL2Gas); expect(context.machineState.daGasLeft).toBeLessThanOrEqual(initialDaGas); @@ -145,8 +117,6 @@ describe('External Calls', () => { const addr = new Fr(123456n); const argsSize = 0; const argsSizeOffset = 20; - const retOffset = 7; - const retSize = 1; const successOffset = 6; const otherContextInstructionsBytecode = markBytecodeAsAvm( @@ -181,18 +151,16 @@ describe('External Calls', () => { addrOffset, /*argsOffset=*/ 0, argsSizeOffset, - retOffset, - retSize, successOffset, - /*functionSelectorOffset=*/ 0, ); await instruction.execute(context); const successValue = context.machineState.memory.get(successOffset); - expect(successValue).toEqual(new Uint8(1n)); + expect(successValue).toEqual(new Uint1(1n)); - const retValue = context.machineState.memory.get(retOffset).toBigInt(); - expect(retValue).toBeLessThan(initialL2Gas); + const retValues = context.machineState.nestedReturndata; + expect(retValues).toHaveLength(1); + expect(retValues[0].toBigInt()).toBeLessThan(initialL2Gas); expect(context.machineState.l2GasLeft).toBeLessThan(initialL2Gas); expect(context.machineState.daGasLeft).toBeLessThanOrEqual(initialDaGas); @@ -208,10 +176,7 @@ describe('External Calls', () => { ...Buffer.from('a234', 'hex'), // addrOffset ...Buffer.from('b234', 'hex'), // argsOffset ...Buffer.from('c234', 'hex'), // argsSizeOffset - ...Buffer.from('d234', 'hex'), // retOffset - ...Buffer.from('e234', 'hex'), // retSize ...Buffer.from('f234', 'hex'), // successOffset - ...Buffer.from('f334', 'hex'), // functionSelectorOffset ]); const inst = new StaticCall( /*indirect=*/ 0x1234, @@ -219,10 +184,7 @@ describe('External Calls', () => { /*addrOffset=*/ 0xa234, /*argsOffset=*/ 0xb234, /*argsSizeOffset=*/ 0xc234, - /*retOffset=*/ 0xd234, - /*retSize=*/ 0xe234, /*successOffset=*/ 0xf234, - /*functionSelectorOffset=*/ 0xf334, ); expect(StaticCall.deserialize(buf)).toEqual(inst); @@ -239,8 +201,6 @@ describe('External Calls', () => { const argsSize = args.length; const argsSizeOffset = 40; - const retOffset = 80; - const retSize = 2; const successOffset = 70; context.machineState.memory.setSlice(gasOffset, gas); @@ -269,10 +229,7 @@ describe('External Calls', () => { addrOffset, argsOffset, argsSizeOffset, - retOffset, - retSize, successOffset, - /*functionSelectorOffset=*/ 0, ); await expect(() => instruction.execute(context)).rejects.toThrow( 'Static call cannot update the state, emit L2->L1 messages or generate logs', diff --git a/yarn-project/simulator/src/avm/opcodes/external_calls.ts b/yarn-project/simulator/src/avm/opcodes/external_calls.ts index fca523f965c..db6b658dade 100644 --- a/yarn-project/simulator/src/avm/opcodes/external_calls.ts +++ b/yarn-project/simulator/src/avm/opcodes/external_calls.ts @@ -1,10 +1,9 @@ -import { FunctionSelector, Gas } from '@aztec/circuits.js'; -import { padArrayEnd } from '@aztec/foundation/collection'; +import { Fr, FunctionSelector, Gas, PUBLIC_DISPATCH_SELECTOR } from '@aztec/circuits.js'; import type { AvmContext } from '../avm_context.js'; import { type AvmContractCallResult } from '../avm_contract_call_result.js'; import { gasLeftToGas } from '../avm_gas.js'; -import { Field, TypeTag, Uint8 } from '../avm_memory_types.js'; +import { type Field, TypeTag, Uint1 } from '../avm_memory_types.js'; import { AvmSimulator } from '../avm_simulator.js'; import { RethrownError } from '../errors.js'; import { Opcode, OperandType } from '../serialization/instruction_serialization.js'; @@ -21,58 +20,39 @@ abstract class ExternalCall extends Instruction { OperandType.UINT16, OperandType.UINT16, OperandType.UINT16, - OperandType.UINT16, - OperandType.UINT16, - OperandType.UINT16, ]; constructor( private indirect: number, - private gasOffset: number /* Unused due to no formal gas implementation at this moment */, + private gasOffset: number, private addrOffset: number, private argsOffset: number, private argsSizeOffset: number, - private retOffset: number, - private retSize: number, private successOffset: number, - // NOTE: Function selector is likely temporary since eventually public contract bytecode will be one - // blob containing all functions, and function selector will become an application-level mechanism - // (e.g. first few bytes of calldata + compiler-generated jump table) - private functionSelectorOffset: number, ) { super(); } public async execute(context: AvmContext) { const memory = context.machineState.memory.track(this.type); - const operands = [ - this.gasOffset, - this.addrOffset, - this.argsOffset, - this.argsSizeOffset, - this.retOffset, - this.successOffset, - this.functionSelectorOffset, - ]; + const operands = [this.gasOffset, this.addrOffset, this.argsOffset, this.argsSizeOffset, this.successOffset]; const addressing = Addressing.fromWire(this.indirect, operands.length); - const [gasOffset, addrOffset, argsOffset, argsSizeOffset, retOffset, successOffset, functionSelectorOffset] = - addressing.resolve(operands, memory); + const [gasOffset, addrOffset, argsOffset, argsSizeOffset, successOffset] = addressing.resolve(operands, memory); memory.checkTags(TypeTag.FIELD, gasOffset, gasOffset + 1); memory.checkTag(TypeTag.FIELD, addrOffset); memory.checkTag(TypeTag.UINT32, argsSizeOffset); - memory.checkTag(TypeTag.FIELD, functionSelectorOffset); const calldataSize = memory.get(argsSizeOffset).toNumber(); memory.checkTagsRange(TypeTag.FIELD, argsOffset, calldataSize); const callAddress = memory.getAs(addrOffset); const calldata = memory.getSlice(argsOffset, calldataSize).map(f => f.toFr()); - const functionSelector = memory.getAs(functionSelectorOffset).toFr(); + const functionSelector = new Fr(PUBLIC_DISPATCH_SELECTOR); // If we are already in a static call, we propagate the environment. const callType = context.environment.isStaticCall ? 'STATICCALL' : this.type; // First we consume the gas for this operation. - context.machineState.consumeGas(this.gasCost(calldataSize + this.retSize)); + context.machineState.consumeGas(this.gasCost(calldataSize)); // Then we consume the gas allocated for the nested call. The excess will be refunded later. // Gas allocation is capped by the amount of gas left in the current context. // We have to do some dancing here because the gas allocation is a field, @@ -106,21 +86,12 @@ abstract class ExternalCall extends Instruction { throw new RethrownError(nestedCallResults.revertReason.message, nestedCallResults.revertReason); } + // Save return/revert data for later. const fullReturnData = nestedCallResults.output; context.machineState.nestedReturndata = fullReturnData; - // We only take as much data as was specified in the return size and pad with zeroes if the return data is smaller - // than the specified size in order to prevent that memory to be left with garbage - const returnData = fullReturnData.slice(0, this.retSize); - const convertedReturnData = padArrayEnd( - returnData.map(f => new Field(f)), - new Field(0), - this.retSize, - ); - - // Write our return data into memory - memory.set(successOffset, new Uint8(success ? 1 : 0)); - memory.setSlice(retOffset, convertedReturnData); + // Write our success flag into memory. + memory.set(successOffset, new Uint1(success ? 1 : 0)); // Refund unused gas context.machineState.refundGas(gasLeftToGas(nestedContext.machineState)); @@ -135,7 +106,7 @@ abstract class ExternalCall extends Instruction { /*avmCallResults=*/ nestedCallResults, ); - memory.assert({ reads: calldataSize + 5, writes: 1 + this.retSize, addressing }); + memory.assert({ reads: calldataSize + 4, writes: 1, addressing }); context.machineState.incrementPc(); } diff --git a/yarn-project/simulator/src/avm/serialization/bytecode_serialization.test.ts b/yarn-project/simulator/src/avm/serialization/bytecode_serialization.test.ts index c344d938779..7662b5300c6 100644 --- a/yarn-project/simulator/src/avm/serialization/bytecode_serialization.test.ts +++ b/yarn-project/simulator/src/avm/serialization/bytecode_serialization.test.ts @@ -84,10 +84,7 @@ describe('Bytecode Serialization', () => { /*addrOffset=*/ 0xa234, /*argsOffset=*/ 0xb234, /*argsSize=*/ 0xc234, - /*retOffset=*/ 0xd234, - /*retSize=*/ 0xe234, /*successOffset=*/ 0xf234, - /*functionSelectorOffset=*/ 0xf334, ), new StaticCall( /*indirect=*/ 0x01, @@ -95,10 +92,7 @@ describe('Bytecode Serialization', () => { /*addrOffset=*/ 0xa234, /*argsOffset=*/ 0xb234, /*argsSize=*/ 0xc234, - /*retOffset=*/ 0xd234, - /*retSize=*/ 0xe234, /*successOffset=*/ 0xf234, - /*functionSelectorOffset=*/ 0xf334, ), ]; const bytecode = Buffer.concat(instructions.map(i => i.serialize())); @@ -122,10 +116,7 @@ describe('Bytecode Serialization', () => { /*addrOffset=*/ 0xa234, /*argsOffset=*/ 0xb234, /*argsSize=*/ 0xc234, - /*retOffset=*/ 0xd234, - /*retSize=*/ 0xe234, /*successOffset=*/ 0xf234, - /*functionSelectorOffset=*/ 0xf334, ), new StaticCall( /*indirect=*/ 0x01, @@ -133,10 +124,7 @@ describe('Bytecode Serialization', () => { /*addrOffset=*/ 0xa234, /*argsOffset=*/ 0xb234, /*argsSize=*/ 0xc234, - /*retOffset=*/ 0xd234, - /*retSize=*/ 0xe234, /*successOffset=*/ 0xf234, - /*functionSelectorOffset=*/ 0xf334, ), ]; diff --git a/yarn-project/txe/src/oracle/txe_oracle.ts b/yarn-project/txe/src/oracle/txe_oracle.ts index e0911adf571..1078e6e329e 100644 --- a/yarn-project/txe/src/oracle/txe_oracle.ts +++ b/yarn-project/txe/src/oracle/txe_oracle.ts @@ -781,24 +781,17 @@ export class TXE implements TypedOracle { // AVM oracles - async avmOpcodeCall( - targetContractAddress: AztecAddress, - functionSelector: FunctionSelector, - args: Fr[], - isStaticCall: boolean, - ) { + async avmOpcodeCall(targetContractAddress: AztecAddress, args: Fr[], isStaticCall: boolean) { // Store and modify env const currentContractAddress = AztecAddress.fromField(this.contractAddress); const currentMessageSender = AztecAddress.fromField(this.msgSender); - const currentFunctionSelector = FunctionSelector.fromField(this.functionSelector.toField()); this.setMsgSender(this.contractAddress); this.setContractAddress(targetContractAddress); - this.setFunctionSelector(functionSelector); const callContext = new CallContext( /* msgSender */ currentContractAddress, targetContractAddress, - functionSelector, + FunctionSelector.fromField(new Fr(PUBLIC_DISPATCH_SELECTOR)), isStaticCall, ); @@ -821,7 +814,6 @@ export class TXE implements TypedOracle { this.setContractAddress(currentContractAddress); this.setMsgSender(currentMessageSender); - this.setFunctionSelector(currentFunctionSelector); return executionResult; } diff --git a/yarn-project/txe/src/txe_service/txe_service.ts b/yarn-project/txe/src/txe_service/txe_service.ts index 07c9fc8b05c..52ceb74f06f 100644 --- a/yarn-project/txe/src/txe_service/txe_service.ts +++ b/yarn-project/txe/src/txe_service/txe_service.ts @@ -218,8 +218,9 @@ export class TXEService { args: ForeignCallArray, ) { const parsedAddress = fromSingle(address); - const parsedSelector = FunctionSelector.fromField(fromSingle(functionSelector)); - const result = await (this.typedOracle as TXE).avmOpcodeCall(parsedAddress, parsedSelector, fromArray(args), false); + const parsedSelector = fromSingle(functionSelector); + const extendedArgs = [parsedSelector, ...fromArray(args)]; + const result = await (this.typedOracle as TXE).avmOpcodeCall(parsedAddress, extendedArgs, false); if (!result.reverted) { throw new ExpectedFailureError('Public call did not revert'); } @@ -730,11 +731,9 @@ export class TXEService { address: ForeignCallSingle, _length: ForeignCallSingle, args: ForeignCallArray, - functionSelector: ForeignCallSingle, ) { const result = await (this.typedOracle as TXE).avmOpcodeCall( fromSingle(address), - FunctionSelector.fromField(fromSingle(functionSelector)), fromArray(args), /* isStaticCall */ false, ); @@ -754,7 +753,7 @@ export class TXEService { } } - return toForeignCallResult([toArray(result.returnValues), toSingle(new Fr(!result.reverted))]); + return toForeignCallResult([toSingle(new Fr(!result.reverted))]); } async avmOpcodeStaticCall( @@ -762,11 +761,9 @@ export class TXEService { address: ForeignCallSingle, _length: ForeignCallSingle, args: ForeignCallArray, - functionSelector: ForeignCallSingle, ) { const result = await (this.typedOracle as TXE).avmOpcodeCall( fromSingle(address), - FunctionSelector.fromField(fromSingle(functionSelector)), fromArray(args), /* isStaticCall */ true, ); @@ -786,6 +783,6 @@ export class TXEService { } } - return toForeignCallResult([toArray(result.returnValues), toSingle(new Fr(!result.reverted))]); + return toForeignCallResult([toSingle(new Fr(!result.reverted))]); } }