Skip to content

feat!: remove slice read from CALL #13729

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
IlyasRidhuan merged 1 commit into from
Apr 23, 2025
Merged

feat!: remove slice read from CALL #13729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 16 additions & 16 deletions avm-transpiler/src/transpile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,33 +594,31 @@ fn handle_external_call(
inputs: &[ValueOrArray],
opcode: AvmOpcode,
) {
if !destinations.is_empty() || inputs.len() != 4 {
if !destinations.is_empty() || inputs.len() != 5 {
panic!(
"Transpiler expects ForeignCall (Static)Call to have 0 destinations and 4 inputs, got {} and {}.",
"Transpiler expects ForeignCall (Static)Call to have 0 destinations and 5 inputs, got {} and {}.",
destinations.len(),
inputs.len()
);
}

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]`)"
);
pointer
}
_ => panic!("Call instruction's gas input should be a HeapArray"),
let l2_gas_offset = match &inputs[0] {
ValueOrArray::MemoryAddress(offset) => offset,
_ => panic!("Call instruction's gas input should be a basic MemoryAddress"),
};
let da_gas_offset = match &inputs[1] {
ValueOrArray::MemoryAddress(offset) => offset,
_ => panic!("Call instruction's gas input should be a basic MemoryAddress"),
};
let address_offset = match &inputs[1] {
let address_offset = match &inputs[2] {
ValueOrArray::MemoryAddress(offset) => offset,
_ => panic!("Call instruction's target address input should be a basic MemoryAddress",),
};
// 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];
// Observe that below, we use `inputs[4]` and therefore skip the length field.
let args = &inputs[4];
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"),
Expand All @@ -630,14 +628,16 @@ fn handle_external_call(
opcode,
indirect: Some(
AddressingModeBuilder::default()
.indirect_operand(gas_offset_ptr)
.direct_operand(l2_gas_offset)
.direct_operand(da_gas_offset)
.direct_operand(address_offset)
.indirect_operand(args_offset_ptr)
.direct_operand(args_size_offset)
.build(),
),
operands: vec![
AvmOperand::U16 { value: gas_offset_ptr.to_usize() as u16 },
AvmOperand::U16 { value: l2_gas_offset.to_usize() as u16 },
AvmOperand::U16 { value: da_gas_offset.to_usize() as u16 },
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 },
Expand Down
6 changes: 3 additions & 3 deletions barretenberg/cpp/src/barretenberg/vm2/simulation/lib/serialization.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,11 @@ const std::vector<OperandType> three_operand_format16 = {
const std::vector<OperandType> kernel_input_operand_format = { OperandType::INDIRECT8, OperandType::UINT16 };

const std::vector<OperandType> external_call_format = { OperandType::INDIRECT16,
/*gasOffset=*/OperandType::UINT16,
/*l2GasOffset=*/OperandType::UINT16,
/*daGasOffset=*/OperandType::UINT16,
/*addrOffset=*/OperandType::UINT16,
/*argsOffset=*/OperandType::UINT16,
/*argsSizeOffset=*/OperandType::UINT16,
/*successOffset=*/OperandType::UINT16 };
/*argsSizeOffset=*/OperandType::UINT16 };

// Contrary to TS, the format does not contain the WireOpCode byte which prefixes any instruction.
// The format for WireOpCode::SET has to be handled separately as it is variable based on the tag.
Expand Down
45 changes: 30 additions & 15 deletions noir-projects/aztec-nr/aztec/src/context/public_context.nr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,12 @@ impl PublicContext {
) -> [Field] {
let calldata = args.push_front(function_selector.to_field());

call(gas_for_call(gas_opts), contract_address, calldata);
call(
gas_opts.l2_gas.unwrap_or(MAX_FIELD_VALUE),
gas_opts.da_gas.unwrap_or(MAX_FIELD_VALUE),
contract_address,
calldata,
);
// Use success_copy to determine whether the call succeeded
let success = success_copy();

Expand All @@ -107,7 +112,12 @@ impl PublicContext {
) -> [Field] {
let calldata = args.push_front(function_selector.to_field());

call_static(gas_for_call(gas_opts), contract_address, calldata);
call_static(
gas_opts.l2_gas.unwrap_or(MAX_FIELD_VALUE),
gas_opts.da_gas.unwrap_or(MAX_FIELD_VALUE),
contract_address,
calldata,
);
// Use success_copy to determine whether the call succeeded
let success = success_copy();

Expand Down Expand Up @@ -245,13 +255,6 @@ impl PublicContext {
}
}

// Helper functions
fn gas_for_call(user_gas: GasOpts) -> [Field; 2] {
// It's ok to use the max possible gas here, because the gas will be
// capped by the gas left in the (STATIC)CALL instruction.
[user_gas.l2_gas.unwrap_or(MAX_FIELD_VALUE), user_gas.da_gas.unwrap_or(MAX_FIELD_VALUE)]
}

// Unconstrained opcode wrappers (do not use directly).
unconstrained fn address() -> AztecAddress {
address_opcode()
Expand Down Expand Up @@ -310,12 +313,22 @@ 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]) {
call_opcode(gas, address, args)
unconstrained fn call(
l2_gas_allocation: Field,
da_gas_allocation: Field,
address: AztecAddress,
args: [Field],
) {
call_opcode(l2_gas_allocation, da_gas_allocation, address, args)
}

unconstrained fn call_static(gas: [Field; 2], address: AztecAddress, args: [Field]) {
call_static_opcode(gas, address, args)
unconstrained fn call_static(
l2_gas_allocation: Field,
da_gas_allocation: Field,
address: AztecAddress,
args: [Field],
) {
call_static_opcode(l2_gas_allocation, da_gas_allocation, address, args)
}

pub unconstrained fn calldata_copy<let N: u32>(cdoffset: u32, copy_size: u32) -> [Field; N] {
Expand Down Expand Up @@ -441,14 +454,16 @@ unconstrained fn revert_opcode(revertdata: [Field]) {}

#[oracle(avmOpcodeCall)]
unconstrained fn call_opcode(
gas: [Field; 2], // gas allocation: [l2_gas, da_gas]
l2_gas_allocation: Field,
da_gas_allocation: Field,
address: AztecAddress,
args: [Field],
) {}

#[oracle(avmOpcodeStaticCall)]
unconstrained fn call_static_opcode(
gas: [Field; 2], // gas allocation: [l2_gas, da_gas]
l2_gas_allocation: Field,
da_gas_allocation: Field,
address: AztecAddress,
args: [Field],
) {}
Expand Down
9 changes: 4 additions & 5 deletions noir-projects/noir-contracts/contracts/test/avm_test_contract/src/main.nr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ pub contract AvmTest {

// Libs
use dep::aztec::context::gas::GasOpts;
use dep::aztec::context::public_context::{call, gas_for_call, returndata_size, success_copy};
use dep::aztec::context::public_context::{call, returndata_size, success_copy};
use dep::aztec::macros::{functions::{private, public}, storage::storage};
use dep::aztec::oracle::get_contract_instance::{
get_contract_instance_class_id_avm, get_contract_instance_deployer_avm,
Expand Down Expand Up @@ -257,12 +257,12 @@ pub contract AvmTest {
// since it will all be consumed on exceptional halt.
let l2_gas_left = context.l2_gas_left();
let da_gas_left = context.da_gas_left();
let gas_allocation: [Field; 2] = [l2_gas_left - 200_000, da_gas_left - 200_000];
let selector = FunctionSelector::from_signature("divide_by_zero()");

// Call without capturing a return value since call no longer returns success
call(
gas_allocation,
l2_gas_left - 200_000,
da_gas_left - 200_000,
context.this_address(),
&[selector.to_field()],
);
Expand Down Expand Up @@ -540,8 +540,7 @@ pub contract AvmTest {
#[public]
fn nested_call_to_nothing_recovers() {
let garbageAddress = AztecAddress::from_field(42);
let gas = [1, 1];
call(gas, garbageAddress, &[]);
call(1, 1, garbageAddress, &[]);
let success = success_copy();
assert(
!success,
Expand Down
57 changes: 40 additions & 17 deletions yarn-project/simulator/src/public/avm/opcodes/external_calls.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,17 @@ describe('External Calls', () => {
it('Should (de)serialize correctly', () => {
const buf = Buffer.from([
Call.opcode, // opcode
...Buffer.from('12', 'hex'), // indirect (8 bit)
...Buffer.from('1234', 'hex'), // gasOffset
...Buffer.from('1234', 'hex'), // indirect (16 bit)
...Buffer.from('1234', 'hex'), // l2GasOffset
...Buffer.from('5678', 'hex'), // daGasOffset
...Buffer.from('a234', 'hex'), // addrOffset
...Buffer.from('b234', 'hex'), // argsOffset
...Buffer.from('c234', 'hex'), // argsSizeOffset
]);
const inst = new Call(
/*indirect=*/ 0x12,
/*gasOffset=*/ 0x1234,
/*indirect=*/ 0x1234,
/*l2GasOffset=*/ 0x1234,
/*daGasOffset=*/ 0x5678,
/*addrOffset=*/ 0xa234,
/*argsOffset=*/ 0xb234,
/*argsSizeOffset=*/ 0xc234,
Expand All @@ -64,7 +66,8 @@ describe('External Calls', () => {
});

it('Call to non-existent bytecode returns failure', async () => {
const gasOffset = 0;
const l2GasOffset = 0;
const daGasOffset = 1;
const l2Gas = 2e6;
const daGas = 3e6;
const addrOffset = 2;
Expand All @@ -84,7 +87,7 @@ 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);
const instruction = new Call(/*indirect=*/ 0, l2GasOffset, daGasOffset, addrOffset, argsOffset, argsSizeOffset);
await instruction.execute(context);

// Use SuccessCopy to get success
Expand All @@ -104,7 +107,8 @@ describe('External Calls', () => {
});

it('Should execute a call correctly', async () => {
const gasOffset = 0;
const l2GasOffset = 0;
const daGasOffset = 1;
const l2Gas = 2e6;
const daGas = 3e6;
const addrOffset = 2;
Expand Down Expand Up @@ -138,7 +142,7 @@ 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);
const instruction = new Call(/*indirect=*/ 0, l2GasOffset, daGasOffset, addrOffset, argsOffset, argsSizeOffset);
await instruction.execute(context);

// Use SuccessCopy to get success
Expand All @@ -156,7 +160,8 @@ describe('External Calls', () => {
});

it('Should cap to available gas if allocated is bigger', async () => {
const gasOffset = 0;
const l2GasOffset = 0;
const daGasOffset = 1;
const l2Gas = 1e9;
const daGas = 1e9;
const addrOffset = 2;
Expand Down Expand Up @@ -190,7 +195,14 @@ describe('External Calls', () => {
context.machineState.memory.set(2, new Field(addr));
context.machineState.memory.set(argsSizeOffset, new Uint32(argsSize));

const instruction = new Call(/*indirect=*/ 0, gasOffset, addrOffset, /*argsOffset=*/ 0, argsSizeOffset);
const instruction = new Call(
/*indirect=*/ 0,
l2GasOffset,
daGasOffset,
addrOffset,
/*argsOffset=*/ 0,
argsSizeOffset,
);
await instruction.execute(context);

// Use SuccessCopy to get success
Expand All @@ -213,15 +225,17 @@ describe('External Calls', () => {
it('Should (de)serialize correctly', () => {
const buf = Buffer.from([
StaticCall.opcode, // opcode
...Buffer.from('12', 'hex'), // indirect (8 bit)
...Buffer.from('1234', 'hex'), // gasOffset
...Buffer.from('1234', 'hex'), // indirect (16 bit)
...Buffer.from('1234', 'hex'), // l2GasOffset
...Buffer.from('5678', 'hex'), // daGasOffset
...Buffer.from('a234', 'hex'), // addrOffset
...Buffer.from('b234', 'hex'), // argsOffset
...Buffer.from('c234', 'hex'), // argsSizeOffset
]);
const inst = new StaticCall(
/*indirect=*/ 0x12,
/*gasOffset=*/ 0x1234,
/*indirect=*/ 0x1234,
/*l2GasOffset=*/ 0x1234,
/*daGasOffset=*/ 0x5678,
/*addrOffset=*/ 0xa234,
/*argsOffset=*/ 0xb234,
/*argsSizeOffset=*/ 0xc234,
Expand All @@ -232,7 +246,8 @@ describe('External Calls', () => {
});

it('Should fail if a static call attempts to touch storage', async () => {
const gasOffset = 0;
const l2GasOffset = 0;
const daGasOffset = 1;
const gas = [new Field(0n), new Field(0n), new Field(0n)];
const addrOffset = 10;
const addr = new Field(123456n);
Expand All @@ -242,7 +257,8 @@ describe('External Calls', () => {
const argsSize = args.length;
const argsSizeOffset = 60;

context.machineState.memory.setSlice(gasOffset, gas);
context.machineState.memory.set(l2GasOffset, gas[0]);
context.machineState.memory.set(daGasOffset, gas[1]);
context.machineState.memory.set(addrOffset, addr);
context.machineState.memory.set(argsSizeOffset, new Uint32(argsSize));
context.machineState.memory.setSlice(argsOffset, args);
Expand All @@ -260,7 +276,14 @@ describe('External Calls', () => {
const contractInstance = await makeContractInstanceFromClassId(contractClass.id);
mockGetContractInstance(contractsDB, contractInstance);

const instruction = new StaticCall(/*indirect=*/ 0, gasOffset, addrOffset, argsOffset, argsSizeOffset);
const instruction = new StaticCall(
/*indirect=*/ 0,
l2GasOffset,
daGasOffset,
addrOffset,
argsOffset,
argsSizeOffset,
);
await instruction.execute(context);
// Ideally we'd mock the nested call.
expect(context.machineState.collectedRevertInfo?.recursiveRevertReason.message).toMatch(
Expand Down
Loading