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

fix: small fixes to avm witgen #10749

Merged
merged 2 commits into from
Dec 18, 2024
Merged

fix: small fixes to avm witgen #10749

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
c37b739
fix: add tree snapshots to ctx
IlyasRidhuan Dec 15, 2024
e81f2ad
fix: change limit counters
IlyasRidhuan Dec 16, 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
1 change: 1 addition & 0 deletions barretenberg/cpp/src/barretenberg/vm/avm/trace/errors.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ enum class AvmError : uint32_t {
DUPLICATE_NULLIFIER,
SIDE_EFFECT_LIMIT_REACHED,
OUT_OF_GAS,
STATIC_CALL_ALTERATION
};

} // namespace bb::avm_trace
4 changes: 4 additions & 0 deletions barretenberg/cpp/src/barretenberg/vm/avm/trace/gadgets/merkle_tree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,8 @@ FF AvmMerkleTreeTraceBuilder::perform_storage_write([[maybe_unused]] uint32_t cl
// Update the low leaf
tree_snapshots.public_data_tree.root =
unconstrained_update_leaf_index(low_preimage_hash, static_cast<uint64_t>(low_index), low_path);
// Update the set of writes to unique slots
public_data_unique_writes.insert(slot);
return tree_snapshots.public_data_tree.root;
}
// Check the low leaf conditions (i.e. the slot is sandwiched by the low nullifier, or the new slot is a max
Expand All @@ -172,6 +174,8 @@ FF AvmMerkleTreeTraceBuilder::perform_storage_write([[maybe_unused]] uint32_t cl
// Insert the new leaf into the tree
tree_snapshots.public_data_tree.root = unconstrained_update_leaf_index(leaf_preimage_hash, index, insertion_path);
tree_snapshots.public_data_tree.size++;
// Update the set of writes to unique slots
public_data_unique_writes.insert(slot);
return tree_snapshots.public_data_tree.root;
}

Expand Down
8 changes: 8 additions & 0 deletions barretenberg/cpp/src/barretenberg/vm/avm/trace/gadgets/merkle_tree.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include "barretenberg/vm/avm/trace/gadgets/poseidon2.hpp"
#include "barretenberg/vm/avm/trace/public_inputs.hpp"

#include <unordered_set>
#include <vector>
namespace bb::avm_trace {

Expand Down Expand Up @@ -42,6 +43,12 @@ class AvmMerkleTreeTraceBuilder {
FF compute_public_tree_leaf_slot(uint32_t clk, FF contract_address, FF leaf_index);

TreeSnapshots& get_tree_snapshots() { return tree_snapshots; }
void restore_tree_state(TreeSnapshots& tree_snapshots, std::unordered_set<FF>& public_data_unique_writes)
{
this->tree_snapshots = tree_snapshots;
this->public_data_unique_writes = public_data_unique_writes;
}
std::unordered_set<FF> get_public_data_unique_writes() { return public_data_unique_writes; }

// Public Data Tree
bool perform_storage_read(uint32_t clk,
Expand Down Expand Up @@ -120,6 +127,7 @@ class AvmMerkleTreeTraceBuilder {
std::vector<MerkleEntry> merkle_check_trace;
TreeSnapshots non_revertible_tree_snapshots;
TreeSnapshots tree_snapshots;
std::unordered_set<FF> public_data_unique_writes;
MerkleEntry compute_root_from_path(uint32_t clk,
const FF& leaf_value,
const uint64_t leaf_index,
Expand Down
61 changes: 47 additions & 14 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 @@ -100,7 +100,9 @@ uint32_t finalize_rng_chks_for_testing(std::vector<Row>& main_trace,
auto old_size = main_trace.size();
for (auto const& clk : custom_clk) {
if (clk >= old_size) {
main_trace.push_back(Row{ .main_clk = FF(clk) });
Row row{};
row.main_clk = clk;
main_trace.push_back(row);
}
}

Expand Down Expand Up @@ -219,6 +221,19 @@ uint32_t AvmTraceBuilder::get_inserted_note_hashes_count()
public_inputs.start_tree_snapshots.note_hash_tree.size;
}

uint32_t AvmTraceBuilder::get_inserted_nullifiers_count()
{
return merkle_tree_trace_builder.get_tree_snapshots().nullifier_tree.size -
public_inputs.start_tree_snapshots.nullifier_tree.size;
}

// Keeping track of the public data writes isn't as simple as comparing sizes
// because of updates to leaves that were already in the tree and state squashing
uint32_t AvmTraceBuilder::get_public_data_writes_count()
{
return static_cast<uint32_t>(merkle_tree_trace_builder.get_public_data_unique_writes().size());
}

void AvmTraceBuilder::insert_private_state(const std::vector<FF>& siloed_nullifiers,
const std::vector<FF>& unique_note_hashes)
{
Expand Down Expand Up @@ -305,6 +320,7 @@ void AvmTraceBuilder::pay_fee()
throw std::runtime_error("Not enough balance for fee payer to pay for transaction");
}

// TS equivalent:
// writeStorage(FEE_JUICE_ADDRESS, balance_slot, updated_balance);
PublicDataWriteTreeHint write_hint = execution_hints.storage_write_hints.at(storage_write_counter++);
ASSERT(write_hint.new_leaf_preimage.value == updated_balance);
Expand Down Expand Up @@ -2818,15 +2834,17 @@ AvmError AvmTraceBuilder::op_sload(uint8_t indirect, uint32_t slot_offset, uint3

AvmError AvmTraceBuilder::op_sstore(uint8_t indirect, uint32_t src_offset, uint32_t slot_offset)
{
// We keep the first encountered error
AvmError error = AvmError::NO_ERROR;
auto clk = static_cast<uint32_t>(main_trace.size()) + 1;
uint32_t unique_public_data_slot_writes = get_public_data_writes_count();
AvmError error = current_ext_call_ctx.is_static_call ? AvmError::STATIC_CALL_ALTERATION
: unique_public_data_slot_writes >= MAX_PUBLIC_DATA_UPDATE_REQUESTS_PER_TX
? AvmError::SIDE_EFFECT_LIMIT_REACHED
: AvmError::NO_ERROR;

if (storage_write_counter >= MAX_PUBLIC_DATA_UPDATE_REQUESTS_PER_TX) {
if (!is_ok(error)) {
// NOTE: the circuit constraint for this limit should only be applied
Comment on lines +2838 to 2845
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be able to remove error = ... SIDE..LIMIT.. below

// for the storage writes performed by this opcode. An exception should before
// made for the fee juice storage write made after teardown.
error = AvmError::SIDE_EFFECT_LIMIT_REACHED;
auto row = Row{
.main_clk = clk,
.main_internal_return_ptr = internal_return_ptr,
Expand Down Expand Up @@ -3005,8 +3023,11 @@ AvmError AvmTraceBuilder::op_emit_note_hash(uint8_t indirect, uint32_t note_hash
{
auto const clk = static_cast<uint32_t>(main_trace.size()) + 1;
uint32_t inserted_note_hashes_count = get_inserted_note_hashes_count();
if (inserted_note_hashes_count >= MAX_NOTE_HASHES_PER_TX) {
AvmError error = AvmError::SIDE_EFFECT_LIMIT_REACHED;
AvmError error = current_ext_call_ctx.is_static_call ? AvmError::STATIC_CALL_ALTERATION
: inserted_note_hashes_count >= MAX_NOTE_HASHES_PER_TX ? AvmError::SIDE_EFFECT_LIMIT_REACHED
: AvmError::NO_ERROR;

if (!is_ok(error)) {
auto row = Row{
.main_clk = clk,
.main_internal_return_ptr = internal_return_ptr,
Expand All @@ -3019,8 +3040,12 @@ AvmError AvmTraceBuilder::op_emit_note_hash(uint8_t indirect, uint32_t note_hash
return error;
}

auto [row, error] = create_kernel_output_opcode(indirect, clk, note_hash_offset);
auto [row, output_error] = create_kernel_output_opcode(indirect, clk, note_hash_offset);
row.main_sel_op_emit_note_hash = FF(1);
if (is_ok(error)) {
error = output_error;
}

row.main_op_err = FF(static_cast<uint32_t>(!is_ok(error)));

AppendTreeHint note_hash_write_hint = execution_hints.note_hash_write_hints.at(note_hash_write_counter++);
Expand Down Expand Up @@ -3157,12 +3182,14 @@ AvmError AvmTraceBuilder::op_nullifier_exists(uint8_t indirect,

AvmError AvmTraceBuilder::op_emit_nullifier(uint8_t indirect, uint32_t nullifier_offset)
{
// We keep the first encountered error
AvmError error = AvmError::NO_ERROR;
auto const clk = static_cast<uint32_t>(main_trace.size()) + 1;

if (nullifier_write_counter >= MAX_NULLIFIERS_PER_TX) {
error = AvmError::SIDE_EFFECT_LIMIT_REACHED;
uint32_t inserted_nullifier_count = get_inserted_nullifiers_count();
AvmError error = current_ext_call_ctx.is_static_call ? AvmError::STATIC_CALL_ALTERATION
: inserted_nullifier_count >= MAX_NULLIFIERS_PER_TX ? AvmError::SIDE_EFFECT_LIMIT_REACHED
: AvmError::NO_ERROR;

if (!is_ok(error)) {
auto row = Row{
.main_clk = clk,
.main_internal_return_ptr = internal_return_ptr,
Expand Down Expand Up @@ -3754,7 +3781,9 @@ AvmError AvmTraceBuilder::constrain_external_call(OpCode opcode,

// We push the current ext call ctx onto the stack and initialize a new one
current_ext_call_ctx.last_pc = pc;
current_ext_call_ctx.success_offset = resolved_success_offset,
current_ext_call_ctx.success_offset = resolved_success_offset;
current_ext_call_ctx.tree_snapshot = merkle_tree_trace_builder.get_tree_snapshots();
current_ext_call_ctx.public_data_unique_writes = merkle_tree_trace_builder.get_public_data_unique_writes();
external_call_ctx_stack.emplace(current_ext_call_ctx);

// Ext Ctx setup
Expand All @@ -3771,6 +3800,7 @@ AvmError AvmTraceBuilder::constrain_external_call(OpCode opcode,
current_ext_call_ctx = ExtCallCtx{
.context_id = static_cast<uint8_t>(clk),
.parent_id = current_ext_call_ctx.context_id,
.is_static_call = opcode == OpCode::STATICCALL,
.contract_address = read_addr.val,
.calldata = calldata,
.nested_returndata = {},
Expand All @@ -3781,6 +3811,7 @@ AvmError AvmTraceBuilder::constrain_external_call(OpCode opcode,
.l2_gas_left = l2_gas_allocated_to_nested_call,
.da_gas_left = da_gas_allocated_to_nested_call,
.internal_return_ptr_stack = {},
.tree_snapshot = {},
};

allocate_gas_for_call(l2_gas_allocated_to_nested_call, da_gas_allocated_to_nested_call);
Expand Down Expand Up @@ -4079,6 +4110,8 @@ ReturnDataError AvmTraceBuilder::op_revert(uint8_t indirect, uint32_t ret_offset
set_call_ptr(static_cast<uint8_t>(current_ext_call_ctx.context_id));
write_to_memory(
current_ext_call_ctx.success_offset, /*success=*/FF::one(), AvmMemoryTag::U1, /*fix_pc=*/false);
merkle_tree_trace_builder.restore_tree_state(current_ext_call_ctx.tree_snapshot,
current_ext_call_ctx.public_data_unique_writes);
}
}

Expand All @@ -4091,7 +4124,7 @@ ReturnDataError AvmTraceBuilder::op_revert(uint8_t indirect, uint32_t ret_offset
.main_internal_return_ptr = FF(internal_return_ptr),
.main_op_err = FF(static_cast<uint32_t>(!is_ok(error))),
.main_pc = pc,
.main_sel_op_external_return = 1,
.main_sel_op_external_revert = 1,
});

pc = is_top_level ? UINT32_MAX : current_ext_call_ctx.last_pc;
Expand Down
5 changes: 5 additions & 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 @@ -266,6 +266,7 @@ class AvmTraceBuilder {
struct ExtCallCtx {
uint32_t context_id; // This is the unique id of the ctx, we'll use the clk
uint32_t parent_id;
bool is_static_call = false;
FF contract_address{};
std::vector<FF> calldata;
std::vector<FF> nested_returndata;
Expand All @@ -276,6 +277,8 @@ class AvmTraceBuilder {
uint32_t l2_gas_left; // as of start of latest nested call
uint32_t da_gas_left; // as of start of latest nested call
std::stack<uint32_t> internal_return_ptr_stack;
TreeSnapshots tree_snapshot; // This is the tree state at the time of the call
std::unordered_set<FF> public_data_unique_writes;
};

ExtCallCtx current_ext_call_ctx{};
Expand Down Expand Up @@ -367,6 +370,8 @@ class AvmTraceBuilder {
IntermRegister reg,
AvmMemTraceBuilder::MemOpOwner mem_op_owner = AvmMemTraceBuilder::MAIN);
uint32_t get_inserted_note_hashes_count();
uint32_t get_inserted_nullifiers_count();
uint32_t get_public_data_writes_count();
FF get_tx_hash() const { return public_inputs.previous_non_revertible_accumulated_data.nullifiers[0]; }

// TODO: remove these once everything is constrained.
Expand Down
Loading