Skip to content
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

Jump to bottom

feat(avm)!: cleanup CALL #9551

Merged
merged 3 commits into from
Oct 29, 2024
+99 −304
Merged

feat(avm)!: cleanup CALL #9551

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f8b5b3b
feat(avm)!: cleanup CALL
fcarreiro Oct 29, 2024
0823f19
cpp side
fcarreiro Oct 29, 2024
b6ad77f
txe
fcarreiro Oct 29, 2024
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
48 changes: 16 additions & 32 deletions avm-transpiler/src/transpile.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading