Skip to content

Commit 7e7eb85

Browse files
IlyasRidhuanIlyasRidhuan
authored
feat!: remove slice read from CALL (#13729)
Changes the opcode operands of CALL: ### Old `INDIRECT_8, gasOffset, addrOffset, argsOffset, argsSizeOffset` ### New `INDIRECT_16, l2GasOffset, daGasOffset, addrOffset, argsOffset, argsSizeOffset`
1 parent 30a5ea0 commit 7e7eb85

File tree

8 files changed

+124
-74
lines changed

8 files changed

+124
-74
lines changed

‎avm-transpiler/src/transpile.rs

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -594,33 +594,31 @@ fn handle_external_call(
594594
inputs: &[ValueOrArray],
595595
opcode: AvmOpcode,
596596
) {
597-
if !destinations.is_empty() || inputs.len() != 4 {
597+
if !destinations.is_empty() || inputs.len() != 5 {
598598
panic!(
599-
"Transpiler expects ForeignCall (Static)Call to have 0 destinations and 4 inputs, got {} and {}.",
599+
"Transpiler expects ForeignCall (Static)Call to have 0 destinations and 5 inputs, got {} and {}.",
600600
destinations.len(),
601601
inputs.len()
602602
);
603603
}
604604

605-
let gas_offset_ptr = match &inputs[0] {
606-
ValueOrArray::HeapArray(HeapArray { pointer, size }) => {
607-
assert!(
608-
*size == 2,
609-
"Call instruction's gas input should be a HeapArray of size 2 (`[l2Gas, daGas]`)"
610-
);
611-
pointer
612-
}
613-
_ => panic!("Call instruction's gas input should be a HeapArray"),
605+
let l2_gas_offset = match &inputs[0] {
606+
ValueOrArray::MemoryAddress(offset) => offset,
607+
_ => panic!("Call instruction's gas input should be a basic MemoryAddress"),
608+
};
609+
let da_gas_offset = match &inputs[1] {
610+
ValueOrArray::MemoryAddress(offset) => offset,
611+
_ => panic!("Call instruction's gas input should be a basic MemoryAddress"),
614612
};
615-
let address_offset = match &inputs[1] {
613+
let address_offset = match &inputs[2] {
616614
ValueOrArray::MemoryAddress(offset) => offset,
617615
_ => panic!("Call instruction's target address input should be a basic MemoryAddress",),
618616
};
619617
// The args are a slice, and this is represented as a (Field, HeapVector).
620618
// The field is the length (memory address) and the HeapVector has the data and length again.
621619
// This is an ACIR internal representation detail that leaks to the SSA.
622-
// Observe that below, we use `inputs[3]` and therefore skip the length field.
623-
let args = &inputs[3];
620+
// Observe that below, we use `inputs[4]` and therefore skip the length field.
621+
let args = &inputs[4];
624622
let (args_offset_ptr, args_size_offset) = match args {
625623
ValueOrArray::HeapVector(HeapVector { pointer, size }) => (pointer, size),
626624
_ => panic!("Call instruction's args input should be a HeapVector input"),
@@ -630,14 +628,16 @@ fn handle_external_call(
630628
opcode,
631629
indirect: Some(
632630
AddressingModeBuilder::default()
633-
.indirect_operand(gas_offset_ptr)
631+
.direct_operand(l2_gas_offset)
632+
.direct_operand(da_gas_offset)
634633
.direct_operand(address_offset)
635634
.indirect_operand(args_offset_ptr)
636635
.direct_operand(args_size_offset)
637636
.build(),
638637
),
639638
operands: vec![
640-
AvmOperand::U16 { value: gas_offset_ptr.to_usize() as u16 },
639+
AvmOperand::U16 { value: l2_gas_offset.to_usize() as u16 },
640+
AvmOperand::U16 { value: da_gas_offset.to_usize() as u16 },
641641
AvmOperand::U16 { value: address_offset.to_usize() as u16 },
642642
AvmOperand::U16 { value: args_offset_ptr.to_usize() as u16 },
643643
AvmOperand::U16 { value: args_size_offset.to_usize() as u16 },

‎barretenberg/cpp/src/barretenberg/vm2/simulation/lib/serialization.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,11 +40,11 @@ const std::vector<OperandType> three_operand_format16 = {
4040
const std::vector<OperandType> kernel_input_operand_format = { OperandType::INDIRECT8, OperandType::UINT16 };
4141

4242
const std::vector<OperandType> external_call_format = { OperandType::INDIRECT16,
43-
/*gasOffset=*/OperandType::UINT16,
43+
/*l2GasOffset=*/OperandType::UINT16,
44+
/*daGasOffset=*/OperandType::UINT16,
4445
/*addrOffset=*/OperandType::UINT16,
4546
/*argsOffset=*/OperandType::UINT16,
46-
/*argsSizeOffset=*/OperandType::UINT16,
47-
/*successOffset=*/OperandType::UINT16 };
47+
/*argsSizeOffset=*/OperandType::UINT16 };
4848

4949
// Contrary to TS, the format does not contain the WireOpCode byte which prefixes any instruction.
5050
// The format for WireOpCode::SET has to be handled separately as it is variable based on the tag.

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

Lines changed: 30 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,12 @@ impl PublicContext {
8686
) -> [Field] {
8787
let calldata = args.push_front(function_selector.to_field());
8888

89-
call(gas_for_call(gas_opts), contract_address, calldata);
89+
call(
90+
gas_opts.l2_gas.unwrap_or(MAX_FIELD_VALUE),
91+
gas_opts.da_gas.unwrap_or(MAX_FIELD_VALUE),
92+
contract_address,
93+
calldata,
94+
);
9095
// Use success_copy to determine whether the call succeeded
9196
let success = success_copy();
9297

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

110-
call_static(gas_for_call(gas_opts), contract_address, calldata);
115+
call_static(
116+
gas_opts.l2_gas.unwrap_or(MAX_FIELD_VALUE),
117+
gas_opts.da_gas.unwrap_or(MAX_FIELD_VALUE),
118+
contract_address,
119+
calldata,
120+
);
111121
// Use success_copy to determine whether the call succeeded
112122
let success = success_copy();
113123

@@ -245,13 +255,6 @@ impl PublicContext {
245255
}
246256
}
247257

248-
// Helper functions
249-
fn gas_for_call(user_gas: GasOpts) -> [Field; 2] {
250-
// It's ok to use the max possible gas here, because the gas will be
251-
// capped by the gas left in the (STATIC)CALL instruction.
252-
[user_gas.l2_gas.unwrap_or(MAX_FIELD_VALUE), user_gas.da_gas.unwrap_or(MAX_FIELD_VALUE)]
253-
}
254-
255258
// Unconstrained opcode wrappers (do not use directly).
256259
unconstrained fn address() -> AztecAddress {
257260
address_opcode()
@@ -310,12 +313,22 @@ unconstrained fn l1_to_l2_msg_exists(msg_hash: Field, msg_leaf_index: Field) ->
310313
unconstrained fn send_l2_to_l1_msg(recipient: EthAddress, content: Field) {
311314
send_l2_to_l1_msg_opcode(recipient, content)
312315
}
313-
unconstrained fn call(gas: [Field; 2], address: AztecAddress, args: [Field]) {
314-
call_opcode(gas, address, args)
316+
unconstrained fn call(
317+
l2_gas_allocation: Field,
318+
da_gas_allocation: Field,
319+
address: AztecAddress,
320+
args: [Field],
321+
) {
322+
call_opcode(l2_gas_allocation, da_gas_allocation, address, args)
315323
}
316324

317-
unconstrained fn call_static(gas: [Field; 2], address: AztecAddress, args: [Field]) {
318-
call_static_opcode(gas, address, args)
325+
unconstrained fn call_static(
326+
l2_gas_allocation: Field,
327+
da_gas_allocation: Field,
328+
address: AztecAddress,
329+
args: [Field],
330+
) {
331+
call_static_opcode(l2_gas_allocation, da_gas_allocation, address, args)
319332
}
320333

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

442455
#[oracle(avmOpcodeCall)]
443456
unconstrained fn call_opcode(
444-
gas: [Field; 2], // gas allocation: [l2_gas, da_gas]
457+
l2_gas_allocation: Field,
458+
da_gas_allocation: Field,
445459
address: AztecAddress,
446460
args: [Field],
447461
) {}
448462

449463
#[oracle(avmOpcodeStaticCall)]
450464
unconstrained fn call_static_opcode(
451-
gas: [Field; 2], // gas allocation: [l2_gas, da_gas]
465+
l2_gas_allocation: Field,
466+
da_gas_allocation: Field,
452467
address: AztecAddress,
453468
args: [Field],
454469
) {}

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

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ pub contract AvmTest {
1212

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

263262
// Call without capturing a return value since call no longer returns success
264263
call(
265-
gas_allocation,
264+
l2_gas_left - 200_000,
265+
da_gas_left - 200_000,
266266
context.this_address(),
267267
&[selector.to_field()],
268268
);
@@ -540,8 +540,7 @@ pub contract AvmTest {
540540
#[public]
541541
fn nested_call_to_nothing_recovers() {
542542
let garbageAddress = AztecAddress::from_field(42);
543-
let gas = [1, 1];
544-
call(gas, garbageAddress, &[]);
543+
call(1, 1, garbageAddress, &[]);
545544
let success = success_copy();
546545
assert(
547546
!success,

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

Lines changed: 40 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -45,15 +45,17 @@ describe('External Calls', () => {
4545
it('Should (de)serialize correctly', () => {
4646
const buf = Buffer.from([
4747
Call.opcode, // opcode
48-
...Buffer.from('12', 'hex'), // indirect (8 bit)
49-
...Buffer.from('1234', 'hex'), // gasOffset
48+
...Buffer.from('1234', 'hex'), // indirect (16 bit)
49+
...Buffer.from('1234', 'hex'), // l2GasOffset
50+
...Buffer.from('5678', 'hex'), // daGasOffset
5051
...Buffer.from('a234', 'hex'), // addrOffset
5152
...Buffer.from('b234', 'hex'), // argsOffset
5253
...Buffer.from('c234', 'hex'), // argsSizeOffset
5354
]);
5455
const inst = new Call(
55-
/*indirect=*/ 0x12,
56-
/*gasOffset=*/ 0x1234,
56+
/*indirect=*/ 0x1234,
57+
/*l2GasOffset=*/ 0x1234,
58+
/*daGasOffset=*/ 0x5678,
5759
/*addrOffset=*/ 0xa234,
5860
/*argsOffset=*/ 0xb234,
5961
/*argsSizeOffset=*/ 0xc234,
@@ -64,7 +66,8 @@ describe('External Calls', () => {
6466
});
6567

6668
it('Call to non-existent bytecode returns failure', async () => {
67-
const gasOffset = 0;
69+
const l2GasOffset = 0;
70+
const daGasOffset = 1;
6871
const l2Gas = 2e6;
6972
const daGas = 3e6;
7073
const addrOffset = 2;
@@ -84,7 +87,7 @@ describe('External Calls', () => {
8487
context.machineState.memory.set(argsSizeOffset, new Uint32(argsSize));
8588
context.machineState.memory.setSlice(3, args);
8689

87-
const instruction = new Call(/*indirect=*/ 0, gasOffset, addrOffset, argsOffset, argsSizeOffset);
90+
const instruction = new Call(/*indirect=*/ 0, l2GasOffset, daGasOffset, addrOffset, argsOffset, argsSizeOffset);
8891
await instruction.execute(context);
8992

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

106109
it('Should execute a call correctly', async () => {
107-
const gasOffset = 0;
110+
const l2GasOffset = 0;
111+
const daGasOffset = 1;
108112
const l2Gas = 2e6;
109113
const daGas = 3e6;
110114
const addrOffset = 2;
@@ -138,7 +142,7 @@ describe('External Calls', () => {
138142
context.machineState.memory.set(argsSizeOffset, new Uint32(argsSize));
139143
context.machineState.memory.setSlice(3, args);
140144

141-
const instruction = new Call(/*indirect=*/ 0, gasOffset, addrOffset, argsOffset, argsSizeOffset);
145+
const instruction = new Call(/*indirect=*/ 0, l2GasOffset, daGasOffset, addrOffset, argsOffset, argsSizeOffset);
142146
await instruction.execute(context);
143147

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

158162
it('Should cap to available gas if allocated is bigger', async () => {
159-
const gasOffset = 0;
163+
const l2GasOffset = 0;
164+
const daGasOffset = 1;
160165
const l2Gas = 1e9;
161166
const daGas = 1e9;
162167
const addrOffset = 2;
@@ -190,7 +195,14 @@ describe('External Calls', () => {
190195
context.machineState.memory.set(2, new Field(addr));
191196
context.machineState.memory.set(argsSizeOffset, new Uint32(argsSize));
192197

193-
const instruction = new Call(/*indirect=*/ 0, gasOffset, addrOffset, /*argsOffset=*/ 0, argsSizeOffset);
198+
const instruction = new Call(
199+
/*indirect=*/ 0,
200+
l2GasOffset,
201+
daGasOffset,
202+
addrOffset,
203+
/*argsOffset=*/ 0,
204+
argsSizeOffset,
205+
);
194206
await instruction.execute(context);
195207

196208
// Use SuccessCopy to get success
@@ -213,15 +225,17 @@ describe('External Calls', () => {
213225
it('Should (de)serialize correctly', () => {
214226
const buf = Buffer.from([
215227
StaticCall.opcode, // opcode
216-
...Buffer.from('12', 'hex'), // indirect (8 bit)
217-
...Buffer.from('1234', 'hex'), // gasOffset
228+
...Buffer.from('1234', 'hex'), // indirect (16 bit)
229+
...Buffer.from('1234', 'hex'), // l2GasOffset
230+
...Buffer.from('5678', 'hex'), // daGasOffset
218231
...Buffer.from('a234', 'hex'), // addrOffset
219232
...Buffer.from('b234', 'hex'), // argsOffset
220233
...Buffer.from('c234', 'hex'), // argsSizeOffset
221234
]);
222235
const inst = new StaticCall(
223-
/*indirect=*/ 0x12,
224-
/*gasOffset=*/ 0x1234,
236+
/*indirect=*/ 0x1234,
237+
/*l2GasOffset=*/ 0x1234,
238+
/*daGasOffset=*/ 0x5678,
225239
/*addrOffset=*/ 0xa234,
226240
/*argsOffset=*/ 0xb234,
227241
/*argsSizeOffset=*/ 0xc234,
@@ -232,7 +246,8 @@ describe('External Calls', () => {
232246
});
233247

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

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

263-
const instruction = new StaticCall(/*indirect=*/ 0, gasOffset, addrOffset, argsOffset, argsSizeOffset);
279+
const instruction = new StaticCall(
280+
/*indirect=*/ 0,
281+
l2GasOffset,
282+
daGasOffset,
283+
addrOffset,
284+
argsOffset,
285+
argsSizeOffset,
286+
);
264287
await instruction.execute(context);
265288
// Ideally we'd mock the nested call.
266289
expect(context.machineState.collectedRevertInfo?.recursiveRevertReason.message).toMatch(

0 commit comments

Comments
 (0)