Skip to content

chore: fix ordering of external call arg comments #21806

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
dbanks12 merged 2 commits into from
Mar 20, 2026
Merged

chore: fix ordering of external call arg comments #21806

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3e18a40
chore: fix ordering of external call arg comments
dbanks12 Mar 19, 2026
1ad4255
address pr comments
dbanks12 Mar 20, 2026
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
4 changes: 2 additions & 2 deletions barretenberg/cpp/pil/vm2/opcodes/external_call.pil
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ include "../gt.pil";
// - l2_gas: allocated L2 gas (register[0])
// - da_gas: allocated DA gas (register[1])
// - contract_address: address of the contract to call (register[2])
// - cd_size: size of the calldata to copy (register[3])
// - cd_offset: offset of the calldata to copy (rop[4])
// - args_size: size of the args to pass (register[3])
// - args_offset: offset of the args to pass (rop[4])
//
// We only use register[0] and register[1] here.
//
Expand Down
4 changes: 2 additions & 2 deletions barretenberg/cpp/src/barretenberg/vm2/common/instruction_spec.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,14 +706,14 @@ const std::unordered_map<ExecutionOpCode, ExecInstructionSpec>& get_exec_instruc
.register_info = RegisterInfo().add_inputs({ /*l2_gas*/ ValueTag::U32,
/*da_gas*/ ValueTag::U32,
/*contract_address*/ ValueTag::FF,
/*cd_size*/ ValueTag::U32 }) } },
/*args_size*/ ValueTag::U32 }) } },
{ ExecutionOpCode::STATICCALL,
{ .num_addresses = 5,
.gas_cost = { .opcode_gas = AVM_STATICCALL_BASE_L2_GAS, .base_da = 0, .dyn_l2 = 0, .dyn_da = 0 },
.register_info = RegisterInfo().add_inputs({ /*l2_gas*/ ValueTag::U32,
/*da_gas*/ ValueTag::U32,
/*contract_address*/ ValueTag::FF,
/*cd_size*/ ValueTag::U32 }) } },
/*args_size*/ ValueTag::U32 }) } },
{ ExecutionOpCode::RETURN,
{ .num_addresses = 2,
.gas_cost = { .opcode_gas = AVM_RETURN_BASE_L2_GAS, .base_da = 0, .dyn_l2 = 0, .dyn_da = 0 },
Expand Down
44 changes: 22 additions & 22 deletions barretenberg/cpp/src/barretenberg/vm2/simulation/gadgets/execution.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,22 +561,22 @@ void Execution::mov(ContextInterface& context, MemoryAddress src_addr, MemoryAdd
* @param l2_gas_offset The resolved address of the allocated L2 gas value.
* @param da_gas_offset The resolved address of the allocated DA gas value.
* @param addr The resolved address of the contract address.
* @param cd_size_offset The resolved address of the calldata size value.
* @param cd_offset The resolved address of the calldata offset value.
* @param args_size_offset The resolved address of the args size value.
* @param args_offset The resolved address of the args offset value.
*
* @throws RegisterValidationException if the tags of the input values do not match the expected tags:
* - l2_gas_offset memory value tag: U32
* - da_gas_offset memory value tag: U32
* - addr memory value tag: FF
* - cd_size_offset memory value tag: U32
* - args_size_offset memory value tag: U32
* @throws OutOfGasException if the gas limit is exceeded.
*/
void Execution::call(ContextInterface& context,
MemoryAddress l2_gas_offset,
MemoryAddress da_gas_offset,
MemoryAddress addr,
MemoryAddress cd_size_offset,
MemoryAddress cd_offset)
MemoryAddress args_size_offset,
MemoryAddress args_offset)
{
BB_BENCH_NAME("Execution::call");
constexpr auto opcode = ExecutionOpCode::CALL;
Expand All @@ -586,22 +586,22 @@ void Execution::call(ContextInterface& context,
const auto& allocated_l2_gas_read = memory.get(l2_gas_offset);
const auto& allocated_da_gas_read = memory.get(da_gas_offset);
const auto& contract_address = memory.get(addr);
// Cd offset loads are deferred to calldatacopy
const auto& cd_size = memory.get(cd_size_offset);
// Args offset load is deferred to calldatacopy
const auto& args_size = memory.get(args_size_offset);

set_and_validate_inputs(opcode, { allocated_l2_gas_read, allocated_da_gas_read, contract_address, cd_size });
set_and_validate_inputs(opcode, { allocated_l2_gas_read, allocated_da_gas_read, contract_address, args_size });

get_gas_tracker().consume_gas(); // Base gas.
Gas gas_limit = get_gas_tracker().compute_gas_limit_for_call(
Gas{ .l2_gas = allocated_l2_gas_read.as<uint32_t>(), .da_gas = allocated_da_gas_read.as<uint32_t>() });

// Tag check contract address + cd_size
// Tag check contract address + args_size
auto nested_context = context_provider.make_nested_context(contract_address,
/*msg_sender=*/context.get_address(),
/*transaction_fee=*/context.get_transaction_fee(),
/*parent_context=*/context,
/*cd_offset_address=*/cd_offset,
/*cd_size=*/cd_size.as<uint32_t>(),
/*cd_offset_address=*/args_offset,
/*cd_size=*/args_size.as<uint32_t>(),
/*is_static=*/context.get_is_static(),
/*gas_limit=*/gas_limit,
/*phase=*/context.get_phase());
Expand All @@ -621,22 +621,22 @@ void Execution::call(ContextInterface& context,
* @param l2_gas_offset The resolved address of the allocated L2 gas value.
* @param da_gas_offset The resolved address of the allocated DA gas value.
* @param addr The resolved address of the contract address.
* @param cd_size_offset The resolved address of the calldata size value.
* @param cd_offset The resolved address of the calldata offset value.
* @param args_size_offset The resolved address of the args size value.
* @param args_offset The resolved address of the args offset value.
*
* @throws RegisterValidationException if the tags of the input values do not match the expected tags:
* - l2_gas_offset memory value tag: U32
* - da_gas_offset memory value tag: U32
* - addr memory value tag: FF
* - cd_size_offset memory value tag: U32
* - args_size_offset memory value tag: U32
* @throws OutOfGasException if the gas limit is exceeded.
*/
void Execution::static_call(ContextInterface& context,
MemoryAddress l2_gas_offset,
MemoryAddress da_gas_offset,
MemoryAddress addr,
MemoryAddress cd_size_offset,
MemoryAddress cd_offset)
MemoryAddress args_size_offset,
MemoryAddress args_offset)
{
BB_BENCH_NAME("Execution::static_call");
constexpr auto opcode = ExecutionOpCode::STATICCALL;
Expand All @@ -646,22 +646,22 @@ void Execution::static_call(ContextInterface& context,
const auto& allocated_l2_gas_read = memory.get(l2_gas_offset);
const auto& allocated_da_gas_read = memory.get(da_gas_offset);
const auto& contract_address = memory.get(addr);
// Cd offset loads are deferred to calldatacopy
const auto& cd_size = memory.get(cd_size_offset);
// Args offset load is deferred to calldatacopy
const auto& args_size = memory.get(args_size_offset);

set_and_validate_inputs(opcode, { allocated_l2_gas_read, allocated_da_gas_read, contract_address, cd_size });
set_and_validate_inputs(opcode, { allocated_l2_gas_read, allocated_da_gas_read, contract_address, args_size });

get_gas_tracker().consume_gas(); // Base gas.
Gas gas_limit = get_gas_tracker().compute_gas_limit_for_call(
Gas{ .l2_gas = allocated_l2_gas_read.as<uint32_t>(), .da_gas = allocated_da_gas_read.as<uint32_t>() });

// Tag check contract address + cd_size
// Tag check contract address + args_size
auto nested_context = context_provider.make_nested_context(contract_address,
/*msg_sender=*/context.get_address(),
/*transaction_fee=*/context.get_transaction_fee(),
/*parent_context=*/context,
/*cd_offset_address=*/cd_offset,
/*cd_size=*/cd_size.as<uint32_t>(),
/*cd_offset_address=*/args_offset,
/*cd_size=*/args_size.as<uint32_t>(),
/*is_static=*/true,
/*gas_limit=*/gas_limit,
/*phase=*/context.get_phase());
Expand Down
8 changes: 4 additions & 4 deletions barretenberg/cpp/src/barretenberg/vm2/simulation/gadgets/execution.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,14 +128,14 @@ class Execution : public ExecutionInterface {
MemoryAddress l2_gas_offset,
MemoryAddress da_gas_offset,
MemoryAddress addr,
MemoryAddress cd_size_offset,
MemoryAddress cd_offset);
MemoryAddress args_size_offset,
MemoryAddress args_offset);
void static_call(ContextInterface& context,
MemoryAddress l2_gas_offset,
MemoryAddress da_gas_offset,
MemoryAddress addr,
MemoryAddress cd_size_offset,
MemoryAddress cd_offset);
MemoryAddress args_size_offset,
MemoryAddress args_offset);
void ret(ContextInterface& context, MemoryAddress ret_size_offset, MemoryAddress ret_offset);
void revert(ContextInterface& context, MemoryAddress rev_size_offset, MemoryAddress rev_offset);
void cd_copy(ContextInterface& context,
Expand Down
4 changes: 2 additions & 2 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 @@ -52,8 +52,8 @@ const std::vector<OperandType> external_call_format = { OperandType::INDIRECT16,
/*l2GasOffset=*/OperandType::UINT16,
/*daGasOffset=*/OperandType::UINT16,
/*addrOffset=*/OperandType::UINT16,
/*argsOffset=*/OperandType::UINT16,
Copy link
Contributor

@jeanmon jeanmon Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch and you are right about the ordering.
However, in execution.cpp or instruction_spec.cpp we use another notation. Instead of "args", we use "cd" terminology. Maybe it is the occasion to align to "cd" terminology.

/*argsSizeOffset=*/OperandType::UINT16 };
/*argsSizeOffset=*/OperandType::UINT16,
/*argsOffset=*/OperandType::UINT16 };

namespace {
// Contrary to TS, the format does not contain the WireOpCode byte which prefixes any instruction.
Expand Down
2 changes: 1 addition & 1 deletion yarn-project/simulator/src/public/avm/opcodes/external_calls.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ abstract class ExternalCall extends Instruction {
OperandType.UINT16, // L2 gas offset
OperandType.UINT16, // DA gas offset
OperandType.UINT16, // Address offset
OperandType.UINT16, // Args offset
OperandType.UINT16, // Args size offset
OperandType.UINT16, // Args offset
];

constructor(
Expand Down
Loading