Skip to content

fix: resolve misc bugs handling phases in avm witgen #11218

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 5 commits into from
Jan 16, 2025
Merged

fix: resolve misc bugs handling phases in avm witgen #11218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cec5337
fix: resolve misc bugs handling phases in avm witgen
Jan 14, 2025
5a39dcb
fix test fixture
Jan 14, 2025
f4d0386
fix
Jan 14, 2025
a1407f3
Merge branch 'master' into db/avm-witgen-phase-bugs
Jan 15, 2025
f8a0cf3
fixes
Jan 15, 2025
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
62 changes: 50 additions & 12 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 @@ -403,6 +403,7 @@ std::vector<Row> Execution::gen_trace(AvmPublicInputs const& public_inputs,

// Loop over all the public call requests
auto const phases = { TxExecutionPhase::SETUP, TxExecutionPhase::APP_LOGIC, TxExecutionPhase::TEARDOWN };
size_t enqueued_call_hint_index = 0;
for (auto phase : phases) {
const auto public_call_requests = phase == TxExecutionPhase::SETUP ? setup_call_requests
: phase == TxExecutionPhase::APP_LOGIC ? app_logic_call_requests
Expand Down Expand Up @@ -447,11 +448,15 @@ std::vector<Row> Execution::gen_trace(AvmPublicInputs const& public_inputs,
auto public_call_request = public_call_requests.at(i);
trace_builder.set_public_call_request(public_call_request);
// At the start of each enqueued call, we read the enqueued call hints
auto enqueued_call_hint = execution_hints.enqueued_call_hints.at(i);
auto enqueued_call_hint = execution_hints.enqueued_call_hints.at(enqueued_call_hint_index++);
ASSERT(public_call_request.contract_address == enqueued_call_hint.contract_address);
// Execute!
phase_error =
Execution::execute_enqueued_call(trace_builder, enqueued_call_hint, returndata, apply_e2e_assertions);
phase_error = Execution::execute_enqueued_call(phase,
trace_builder,
enqueued_call_hint,
public_inputs.gas_settings.teardown_gas_limits,
returndata,
apply_e2e_assertions);

if (!is_ok(phase_error)) {
info("Phase ", to_name(phase), " reverted.");
Expand All @@ -468,6 +473,13 @@ std::vector<Row> Execution::gen_trace(AvmPublicInputs const& public_inputs,
throw std::runtime_error("A revert was encountered in the SETUP phase, killing the entire TX");
break;
}
vinfo("Ended phase ",
to_name(phase),
" with ",
trace_builder.get_l2_gas_left(),
" L2 gas left and ",
trace_builder.get_da_gas_left(),
" DA gas left");
}

if (apply_e2e_assertions) {
Expand All @@ -493,17 +505,31 @@ std::vector<Row> Execution::gen_trace(AvmPublicInputs const& public_inputs,
* @returns the error/result of the enqueued call
*
*/
AvmError Execution::execute_enqueued_call(AvmTraceBuilder& trace_builder,
AvmError Execution::execute_enqueued_call(TxExecutionPhase& phase,
AvmTraceBuilder& trace_builder,
AvmEnqueuedCallHint& enqueued_call_hint,
Gas const teardown_gas_limits,
std::vector<FF>& returndata,
bool check_bytecode_membership)
{
AvmError error = AvmError::NO_ERROR;

// save gas before phase for use after teardown (teardown shouldn't affect end gas)
const auto l2_gas_left_before_enqueued_call = trace_builder.get_l2_gas_left();
const auto da_gas_left_before_enqueued_call = trace_builder.get_da_gas_left();

// TODO(dbanks12): use this below for teardown instead of raw limits.l2_gas
// auto const teardown_allocated_l2_gas = std::min(
// teardown_gas_limits.l2_gas,
// static_cast<uint32_t>(MAX_L2_GAS_PER_TX_PUBLIC_PORTION));

// These hints help us to set up first call ctx
auto context_id = trace_builder.next_context_id;
uint32_t l2_gas_allocated_to_enqueued_call = trace_builder.get_l2_gas_left();
uint32_t da_gas_allocated_to_enqueued_call = trace_builder.get_da_gas_left();
uint32_t l2_gas_allocated_to_enqueued_call =
phase == TxExecutionPhase::TEARDOWN ? teardown_gas_limits.l2_gas : l2_gas_left_before_enqueued_call;
uint32_t da_gas_allocated_to_enqueued_call =
phase == TxExecutionPhase::TEARDOWN ? teardown_gas_limits.da_gas : da_gas_left_before_enqueued_call;
;
trace_builder.current_ext_call_ctx = AvmTraceBuilder::ExtCallCtx{
.context_id = context_id,
.parent_id = 0,
Expand All @@ -513,10 +539,10 @@ AvmError Execution::execute_enqueued_call(AvmTraceBuilder& trace_builder,
.nested_returndata = {},
.last_pc = 0,
.success_offset = 0,
.start_l2_gas_left = l2_gas_allocated_to_enqueued_call,
.start_da_gas_left = da_gas_allocated_to_enqueued_call,
.l2_gas_left = l2_gas_allocated_to_enqueued_call,
.da_gas_left = da_gas_allocated_to_enqueued_call,
.start_l2_gas_left = l2_gas_left_before_enqueued_call,
.start_da_gas_left = da_gas_left_before_enqueued_call,
.l2_gas_left = l2_gas_left_before_enqueued_call,
.da_gas_left = da_gas_left_before_enqueued_call,
.internal_return_ptr_stack = {},
};
trace_builder.next_context_id++;
Expand All @@ -534,7 +560,11 @@ AvmError Execution::execute_enqueued_call(AvmTraceBuilder& trace_builder,
// This error was encountered before any opcodes were executed, but
// we need at least one row in the execution trace to then mutate and say "it halted and consumed all gas!"
trace_builder.op_add(0, 0, 0, 0, OpCode::ADD_8);
trace_builder.handle_exceptional_halt();
if (phase == TxExecutionPhase::TEARDOWN) {
trace_builder.handle_end_of_teardown(l2_gas_left_before_enqueued_call, da_gas_left_before_enqueued_call);
} else {
trace_builder.handle_exceptional_halt();
}
return AvmError::FAILED_BYTECODE_RETRIEVAL;
}

Expand Down Expand Up @@ -1125,7 +1155,12 @@ AvmError Execution::execute_enqueued_call(AvmTraceBuilder& trace_builder,
" IC: ",
error_ic);

trace_builder.handle_exceptional_halt();
// For nested calls or non-teardown-enqueued-calls, handle the exceptional halt
// (consume all gas). Don't do it for teardown enqueued call because gas needs
// to be reset after teardown (teardown doesn't count towards end-gas).
if (!is_top_level || phase != TxExecutionPhase::TEARDOWN) {
trace_builder.handle_exceptional_halt();
}

if (is_top_level) {
break;
Expand All @@ -1141,6 +1176,9 @@ AvmError Execution::execute_enqueued_call(AvmTraceBuilder& trace_builder,
error = AvmError::NO_ERROR;
}
}
if (phase == TxExecutionPhase::TEARDOWN) {
trace_builder.handle_end_of_teardown(l2_gas_left_before_enqueued_call, da_gas_left_before_enqueued_call);
Copy link
Contributor

@jeanmon jeanmon Jan 15, 2025

Choose a reason for hiding this comment

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

Nitpick: Should not we perform this in the caller? Ideally, I would not have "leaked" the phases into this routine. (Handling exceptional halt above depends already on the phase though.)

Copy link
Contributor Author

@dbanks12 dbanks12 Jan 15, 2025

Choose a reason for hiding this comment

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

That's what I started doing at first, but it got ugly that way too.... Either we expose phase to this function, or the parent function needs to start caring about gas allocations and whether the nested call exceptionally halted (vs return or revert opcode).

I'll think about it more, but I'll merge this since it is a bugfix afterall.

Copy link
Contributor

@jeanmon jeanmon Jan 16, 2025

Choose a reason for hiding this comment

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

Sure. If you think the alternative is uglier, let us keep it this way.

}
return error;
}

Expand Down
4 changes: 3 additions & 1 deletion barretenberg/cpp/src/barretenberg/vm/avm/trace/execution.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,10 @@ class Execution {
ExecutionHints const& execution_hints,
bool apply_e2e_assertions = false);

static AvmError execute_enqueued_call(AvmTraceBuilder& trace_builder,
static AvmError execute_enqueued_call(TxExecutionPhase& phase,
AvmTraceBuilder& trace_builder,
AvmEnqueuedCallHint& enqueued_call_hint,
Gas const teardown_gas_limits,
std::vector<FF>& returndata,
bool check_bytecode_membership);

Expand Down
4 changes: 2 additions & 2 deletions barretenberg/cpp/src/barretenberg/vm/avm/trace/gas_trace.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,8 @@ void AvmGasTraceBuilder::constrain_gas_for_halt(bool exceptional_halt,
uint32_t l2_gas_allocated_to_nested_call,
uint32_t da_gas_allocated_to_nested_call)
{
debug("Resetting to parent's L2 gas left (", parent_l2_gas_left, ") before consuming gas allocated to nested call");
debug("Resetting to parent's DA gas left (", parent_da_gas_left, ") before consuming gas allocated to nested call");
debug("Resetting to parent's L2 gas left (", parent_l2_gas_left, ") before consuming gas used by nested call");
debug("Resetting to parent's DA gas left (", parent_da_gas_left, ") before consuming gas used by nested call");
// how much gas did the nested call consume
auto l2_gas_consumed = l2_gas_allocated_to_nested_call - remaining_l2_gas;
auto da_gas_consumed = da_gas_allocated_to_nested_call - remaining_da_gas;
Expand Down
16 changes: 16 additions & 0 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 @@ -429,6 +429,22 @@ void AvmTraceBuilder::handle_exceptional_halt()
}
}

void AvmTraceBuilder::handle_end_of_teardown(uint32_t pre_teardown_l2_gas_left, uint32_t pre_teardown_da_gas_left)
{
vinfo("Handling end of teardown");

// modify the last row of the gas trace to reset back to pre-teardown gas
// since gas used by teardown doesn't contribute to end-gas
gas_trace_builder.constrain_gas_for_halt(/*exceptional_halt=*/true, // not really an exceptional halt
pre_teardown_l2_gas_left,
pre_teardown_da_gas_left,
/*l2_gas_allocated_to_nested_call=*/0,
/*da_gas_allocated_to_nested_call=*/0);

// max out the pc to signify "done"
pc = UINT32_MAX;
}

/**
* @brief Loads a value from memory into a given intermediate register at a specified clock cycle.
* Handles both direct and indirect memory access.
Expand Down
1 change: 1 addition & 0 deletions barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ class AvmTraceBuilder {
void pad_trees();
void allocate_gas_for_call(uint32_t l2_gas, uint32_t da_gas);
void handle_exceptional_halt();
void handle_end_of_teardown(uint32_t pre_teardown_l2_gas_left, uint32_t pre_teardown_da_gas_left);

// These are used for testing only.
AvmTraceBuilder& set_range_check_required(bool required)
Expand Down
Loading
Loading