Skip to content

chore(avm)!: resolve execution TODOs #19501

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
fcarreiro merged 3 commits into from
Jan 12, 2026
Merged

chore(avm)!: resolve execution TODOs #19501

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
6945ca8
chore(avm)!: resolve execution TODOs
fcarreiro Jan 12, 2026
0d4b48d
fix tests
fcarreiro Jan 12, 2026
b66a324
handle possible UB
fcarreiro Jan 12, 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
2 changes: 1 addition & 1 deletion barretenberg/cpp/src/barretenberg/vm2/common/tagged_value.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class CastException : public TaggedValueException {
{}
};

enum class ValueTag {
enum class ValueTag : uint8_t {
FF = MEM_TAG_FF,
U1 = MEM_TAG_U1,
U8 = MEM_TAG_U8,
Expand Down
54 changes: 30 additions & 24 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 @@ -2,10 +2,12 @@

#include <stdexcept>
#include <string>
#include <type_traits>

#include "barretenberg/common/bb_bench.hpp"
#include "barretenberg/common/log.hpp"
#include "barretenberg/vm2/common/aztec_constants.hpp"
#include "barretenberg/vm2/common/tagged_value.hpp"
#include "barretenberg/vm2/common/to_radix.hpp"
#include "barretenberg/vm2/common/uint1.hpp"
#include "barretenberg/vm2/simulation/events/addressing_event.hpp"
Expand Down Expand Up @@ -417,7 +419,7 @@ void Execution::shr(ContextInterface& context, MemoryAddress a_addr, MemoryAddre
*
* @throws OutOfGasException if the gas limit is exceeded.
*/
void Execution::cast(ContextInterface& context, MemoryAddress src_addr, MemoryAddress dst_addr, uint8_t dst_tag)
void Execution::cast(ContextInterface& context, MemoryAddress src_addr, MemoryAddress dst_addr, MemoryTag dst_tag)
{
BB_BENCH_NAME("Execution::cast");
constexpr auto opcode = ExecutionOpCode::CAST;
Expand All @@ -426,7 +428,7 @@ void Execution::cast(ContextInterface& context, MemoryAddress src_addr, MemoryAd
set_and_validate_inputs(opcode, { val });

get_gas_tracker().consume_gas();
MemoryValue truncated = alu.truncate(val.as_ff(), static_cast<MemoryTag>(dst_tag));
MemoryValue truncated = alu.truncate(val.as_ff(), dst_tag);
memory.set(dst_addr, truncated);
set_output(opcode, truncated);
}
Expand All @@ -436,23 +438,28 @@ void Execution::cast(ContextInterface& context, MemoryAddress src_addr, MemoryAd
*
* @param context The context.
* @param dst_addr The resolved address of the output value.
* @param var_enum The enum value of the environment variable to get.
* @param env_var The enum value of the environment variable to get (as an uint8_t).
* We need to use uint8_t here to manually perform validation.
*
* @throws OutOfGasException if the gas limit is exceeded.
* @throws OpcodeExecutionException if the enum value is invalid.
*/
void Execution::get_env_var(ContextInterface& context, MemoryAddress dst_addr, uint8_t var_enum)
void Execution::get_env_var(ContextInterface& context, MemoryAddress dst_addr, uint8_t env_var_value)
{
BB_BENCH_NAME("Execution::get_env_var");
constexpr auto opcode = ExecutionOpCode::GETENVVAR;
auto& memory = context.get_memory();

get_gas_tracker().consume_gas();

// If env_var_value is not a valid EnvironmentVariable enum value, throw an OpcodeExecutionException.
if (env_var_value > static_cast<uint8_t>(EnvironmentVariable::MAX)) {
throw OpcodeExecutionException("Invalid environment variable enum value");
}

MemoryValue result;

EnvironmentVariable env_var = static_cast<EnvironmentVariable>(var_enum);

@jeanmon jeanmon Jan 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems that is undefined behvior. https://stackoverflow.com/questions/33607809/can-an-out-of-range-enum-conversion-produce-a-value-outside-the-underlying-type

Not created by your PR @fcarreiro though but for this case I think we need keep the uint8_t value guarding by env_var <= EnvironmentVariable::MAX.

I think the tag is fine because it should have been filtered earlier in the read_instruction().

@jeanmon jeanmon Jan 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

https://en.cppreference.com/w/cpp/language/enum.html#Notes

@fcarreiro fcarreiro Jan 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Handled!

switch (env_var) {
switch (static_cast<EnvironmentVariable>(env_var_value)) {
case EnvironmentVariable::ADDRESS:
result = MemoryValue::from<FF>(context.get_address());
break;
Expand Down Expand Up @@ -490,6 +497,7 @@ void Execution::get_env_var(ContextInterface& context, MemoryAddress dst_addr, u
result = MemoryValue::from<uint32_t>(context.gas_left().da_gas);
break;
default:
// We leave this here defensively.
throw OpcodeExecutionException("Invalid environment variable enum value");
}

Expand All @@ -503,19 +511,18 @@ void Execution::get_env_var(ContextInterface& context, MemoryAddress dst_addr, u
*
* @param context The context.
* @param dst_addr The resolved address of the output memory value.
* @param dst_tag The destination tag of the value to set. (as an uint8_t)
* @param dst_tag The destination tag of the value to set.
* @param value The source value to set. (might get truncated)
*
* @throws OutOfGasException if the gas limit is exceeded.
*/
// TODO: My dispatch system makes me have a uint8_t tag. Rethink.
void Execution::set(ContextInterface& context, MemoryAddress dst_addr, uint8_t dst_tag, const FF& value)
void Execution::set(ContextInterface& context, MemoryAddress dst_addr, MemoryTag dst_tag, const FF& value)
{
BB_BENCH_NAME("Execution::set");
get_gas_tracker().consume_gas();

constexpr auto opcode = ExecutionOpCode::SET;
MemoryValue truncated = alu.truncate(value, static_cast<MemoryTag>(dst_tag));
MemoryValue truncated = alu.truncate(value, dst_tag);
context.get_memory().set(dst_addr, truncated);
set_output(opcode, truncated);
}
Expand Down Expand Up @@ -1781,9 +1788,7 @@ EnqueuedCallResult Execution::execute(std::unique_ptr<ContextInterface> enqueued

gas_tracker = execution_components.make_gas_tracker(gas_event, instruction, context);
dispatch_opcode(instruction.get_exec_opcode(), context, resolved_operands);
}
// TODO(fcarreiro): handle this in a better way.
catch (const BytecodeRetrievalError& e) {
} catch (const BytecodeRetrievalError& e) {
vinfo("Bytecode retrieval error:: ", e.what());
error = ExecutionError::BYTECODE_RETRIEVAL;
handle_exceptional_halt(context, e.what());
Expand Down Expand Up @@ -1819,8 +1824,6 @@ EnqueuedCallResult Execution::execute(std::unique_ptr<ContextInterface> enqueued
context.set_pc(context.get_next_pc());
execution_id_manager.increment_execution_id();

// TODO: We set the inputs and outputs here and into the execution event,
// but maybe there's a better way to do this.
events.emit({
.error = error,
.wire_instruction = instruction,
Expand Down Expand Up @@ -1932,14 +1935,9 @@ void Execution::handle_exit_call()
parent_context.set_gas_used(result.gas_used + parent_context.get_gas_used());
parent_context.set_child_context(std::move(child_context));

// TODO(fcarreiro): move somewhere else.
if (parent_context.get_checkpoint_id_at_creation() != merkle_db.get_checkpoint_id()) {
throw std::runtime_error(format("Checkpoint id mismatch: ",
parent_context.get_checkpoint_id_at_creation(),
" != ",
merkle_db.get_checkpoint_id(),
" (gone back to the wrong db/context)"));
}
BB_ASSERT_EQ(parent_context.get_checkpoint_id_at_creation(),
merkle_db.get_checkpoint_id(),
"Checkpoint id mismatch: gone back to the wrong db/context");
}
// Else: was top level. ExecutionResult is already set and that will be returned.
}
Expand Down Expand Up @@ -2144,7 +2142,15 @@ inline void Execution::call_with_operands(void (Execution::*f)(ContextInterface&
BB_ASSERT_DEBUG(resolved_operands.size() == sizeof...(Ts), "Resolved operands size mismatch");
auto operand_indices = std::make_index_sequence<sizeof...(Ts)>{};
[f, this, &context, &resolved_operands]<std::size_t... Is>(std::index_sequence<Is...>) {
(this->*f)(context, resolved_operands.at(Is).to<std::decay_t<Ts>>()...);
// This helper handles operand conversion. In particular it converts enums to their underlying type first.
auto convert_operand = []<typename T>(const Operand& op) -> T {
if constexpr (std::is_enum_v<T>) {
return static_cast<T>(op.to<std::underlying_type_t<T>>());

@jeanmon jeanmon Jan 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Brilliant!

} else {
return op.to<T>();
}
};
(this->*f)(context, convert_operand.template operator()<std::decay_t<Ts>>(resolved_operands.at(Is))...);
}(operand_indices);
}

Expand Down
7 changes: 4 additions & 3 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 @@ -9,6 +9,7 @@
#include "barretenberg/vm2/common/field.hpp"
#include "barretenberg/vm2/common/memory_types.hpp"
#include "barretenberg/vm2/common/opcodes.hpp"
#include "barretenberg/vm2/common/tagged_value.hpp"
#include "barretenberg/vm2/simulation/events/context_events.hpp"
#include "barretenberg/vm2/simulation/events/event_emitter.hpp"
#include "barretenberg/vm2/simulation/events/execution_event.hpp"
Expand Down Expand Up @@ -117,9 +118,9 @@ class Execution : public ExecutionInterface {
void lt(ContextInterface& context, MemoryAddress a_addr, MemoryAddress b_addr, MemoryAddress dst_addr);
void lte(ContextInterface& context, MemoryAddress a_addr, MemoryAddress b_addr, MemoryAddress dst_addr);
void op_not(ContextInterface& context, MemoryAddress src_addr, MemoryAddress dst_addr);
void cast(ContextInterface& context, MemoryAddress src_addr, MemoryAddress dst_addr, uint8_t dst_tag);
void get_env_var(ContextInterface& context, MemoryAddress dst_addr, uint8_t var_enum);
void set(ContextInterface& context, MemoryAddress dst_addr, uint8_t tag, const FF& value);
void cast(ContextInterface& context, MemoryAddress src_addr, MemoryAddress dst_addr, MemoryTag dst_tag);
void get_env_var(ContextInterface& context, MemoryAddress dst_addr, uint8_t env_var_value);
void set(ContextInterface& context, MemoryAddress dst_addr, MemoryTag tag, const FF& value);
void mov(ContextInterface& context, MemoryAddress src_addr, MemoryAddress dst_addr);
void jump(ContextInterface& context, uint32_t loc);
void jumpi(ContextInterface& context, MemoryAddress cond_addr, uint32_t loc);
Expand Down
4 changes: 2 additions & 2 deletions barretenberg/cpp/src/barretenberg/vm2/simulation/gadgets/execution.test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -982,7 +982,7 @@ TEST_F(ExecutionSimulationTest, EmitNullifierCollision)
TEST_F(ExecutionSimulationTest, Set)
{
MemoryAddress dst_addr = 10;
uint8_t dst_tag = static_cast<uint8_t>(MemoryTag::U8);
MemoryTag dst_tag = MemoryTag::U8;
FF value = 7;

EXPECT_CALL(context, get_memory());
Expand All @@ -997,7 +997,7 @@ TEST_F(ExecutionSimulationTest, Cast)
{
MemoryAddress src_addr = 9;
MemoryAddress dst_addr = 10;
uint8_t dst_tag = static_cast<uint8_t>(MemoryTag::U1);
MemoryTag dst_tag = MemoryTag::U1;
MemoryValue value = MemoryValue::from<uint64_t>(7);

EXPECT_CALL(context, get_memory()).WillOnce(ReturnRef(memory));
Expand Down
Loading