Skip to content

Commit

Permalink
feat(avm)!: cleanup CALL (#9551)
Browse files Browse the repository at this point in the history 
* (Static)CALL now returns just the success bit
* Properly returns U1 instead of U8
* Removed FunctionSelector

Fixes #8998. Part of #9061.
  • Loading branch information
@fcarreiro
fcarreiro authored Oct 29, 2024
1 parent 2132bc2 commit 26adc55
Show file tree
Hide file tree
Showing 12 changed files with 99 additions and 304 deletions.
48 changes: 16 additions & 32 deletions avm-transpiler/src/transpile.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<AvmInstruction>,
destinations: &Vec<ValueOrArray>,
inputs: &Vec<ValueOrArray>,
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; <size>]`)!",
"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] {
Expand All @@ -450,56 +457,33 @@ 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; <size>] = ...`)!"),
_ => 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![
AvmOperand::U16 { value: gas_offset_ptr.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 },
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()
});
Expand Down
45 changes: 23 additions & 22 deletions barretenberg/cpp/src/barretenberg/vm/avm/tests/execution.test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<FF> returndata = {};
std::vector<FF> returndata;

// Generate Hint for call operation
auto execution_hints = ExecutionHints().with_externalcall_hints({ {
Expand Down Expand Up @@ -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
Expand Down
5 changes: 1 addition & 4 deletions barretenberg/cpp/src/barretenberg/vm/avm/trace/deserialization.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,7 @@ const std::vector<OperandType> 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.
Expand Down
10 changes: 2 additions & 8 deletions barretenberg/cpp/src/barretenberg/vm/avm/trace/execution.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -644,21 +644,15 @@ std::vector<Row> Execution::gen_trace(std::vector<FF> const& calldata,
std::get<uint16_t>(inst.operands.at(2)),
std::get<uint16_t>(inst.operands.at(3)),
std::get<uint16_t>(inst.operands.at(4)),
std::get<uint16_t>(inst.operands.at(5)),
std::get<uint16_t>(inst.operands.at(6)),
std::get<uint16_t>(inst.operands.at(7)),
std::get<uint16_t>(inst.operands.at(8)));
std::get<uint16_t>(inst.operands.at(5)));
break;
case OpCode::STATICCALL:
trace_builder.op_static_call(std::get<uint16_t>(inst.operands.at(0)),
std::get<uint16_t>(inst.operands.at(1)),
std::get<uint16_t>(inst.operands.at(2)),
std::get<uint16_t>(inst.operands.at(3)),
std::get<uint16_t>(inst.operands.at(4)),
std::get<uint16_t>(inst.operands.at(5)),
std::get<uint16_t>(inst.operands.at(6)),
std::get<uint16_t>(inst.operands.at(7)),
std::get<uint16_t>(inst.operands.at(8)));
std::get<uint16_t>(inst.operands.at(5)));
break;
case OpCode::RETURN: {
auto ret = trace_builder.op_return(std::get<uint8_t>(inst.operands.at(0)),
Expand Down
77 changes: 14 additions & 63 deletions barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<uint32_t>(main_trace.size()) + 1;
Expand All @@ -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(
Expand All @@ -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<uint32_t>(hint.l2_gas_used),
static_cast<uint32_t>(hint.da_gas_used));

Expand Down Expand Up @@ -2728,16 +2717,8 @@ void AvmTraceBuilder::constrain_external_call(OpCode opcode,
.main_tag_err = FF(static_cast<uint32_t>(!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<FF>{ hint.success });
write_to_memory(resolved_success_offset, hint.success, AvmMemoryTag::U1);
external_call_counter++;

// Save return data for later.
Expand All @@ -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);
}

/**
Expand All @@ -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);
}

/**
Expand Down
Loading

0 comments on commit 26adc55

Please sign in to comment.