Skip to content

fix(avm): relax constraints for leaf_index kernel opcodes #8288

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
IlyasRidhuan merged 4 commits into from
Sep 3, 2024
Merged

fix(avm): relax constraints for leaf_index kernel opcodes #8288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3bfa1e4
fix(avm): relax constraints for leaf_index kernel opcodes
IlyasRidhuan Aug 29, 2024
e8d2772
fix(avm): handling incorrect leaf values
IlyasRidhuan Sep 1, 2024
4f54b7f
fix(avm): update tests
IlyasRidhuan Sep 2, 2024
47767d0
fix(avm): undefined leaf index returns zero
IlyasRidhuan Sep 2, 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
7 changes: 4 additions & 3 deletions barretenberg/cpp/pil/avm/kernel.pil
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,8 @@ namespace main(256);

// When we encounter a state writing opcode
// We increment the side effect counter by 1
#[SIDE_EFFECT_COUNTER_INCREMENT]
KERNEL_OUTPUT_SELECTORS * (side_effect_counter' - (side_effect_counter + 1)) = 0;
//#[SIDE_EFFECT_COUNTER_INCREMENT]
//KERNEL_OUTPUT_SELECTORS * (side_effect_counter' - (side_effect_counter + 1)) = 0;

//===== LOOKUPS INTO THE PUBLIC INPUTS ===========================================
pol KERNEL_INPUT_SELECTORS = sel_op_address + sel_op_storage_address + sel_op_sender
Expand All @@ -182,8 +182,9 @@ namespace main(256);
#[KERNEL_OUTPUT_ACTIVE_CHECK]
KERNEL_OUTPUT_SELECTORS * (1 - sel_q_kernel_output_lookup) = 0;

// TODO(#8287): Reintroduce constraints
#[KERNEL_OUTPUT_LOOKUP]
sel_q_kernel_output_lookup {kernel_out_offset, ia, side_effect_counter, ib} in sel_kernel_out {clk, kernel_value_out, kernel_side_effect_out, kernel_metadata_out};
sel_q_kernel_output_lookup {kernel_out_offset, /*ia,*/ /*side_effect_counter,*/ ib } in sel_kernel_out {clk, /*kernel_value_out,*/ /*kernel_side_effect_out,*/ kernel_metadata_out};
Copy link
Contributor Author

@IlyasRidhuan IlyasRidhuan Sep 2, 2024

Choose a reason for hiding this comment

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

Had to turn off the kernel_value_out and side_effect parts of the lookup


#[LOOKUP_INTO_KERNEL]
sel_q_kernel_lookup { main.ia, kernel_in_offset } in sel_kernel_inputs { kernel_inputs, clk };
36 changes: 17 additions & 19 deletions barretenberg/cpp/src/barretenberg/vm/avm/generated/flavor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -774,24 +774,23 @@ AvmFlavor::AllConstRefValues::AllConstRefValues(
, main_nullifier_non_exists_write_offset_shift(il[766])
, main_pc_shift(il[767])
, main_sel_execution_row_shift(il[768])
, main_side_effect_counter_shift(il[769])
, main_sload_write_offset_shift(il[770])
, main_sstore_write_offset_shift(il[771])
, mem_glob_addr_shift(il[772])
, mem_rw_shift(il[773])
, mem_sel_mem_shift(il[774])
, mem_tag_shift(il[775])
, mem_tsp_shift(il[776])
, mem_val_shift(il[777])
, slice_addr_shift(il[778])
, slice_clk_shift(il[779])
, slice_cnt_shift(il[780])
, slice_col_offset_shift(il[781])
, slice_sel_cd_cpy_shift(il[782])
, slice_sel_mem_active_shift(il[783])
, slice_sel_return_shift(il[784])
, slice_sel_start_shift(il[785])
, slice_space_id_shift(il[786])
, main_sload_write_offset_shift(il[769])
, main_sstore_write_offset_shift(il[770])
, mem_glob_addr_shift(il[771])
, mem_rw_shift(il[772])
, mem_sel_mem_shift(il[773])
, mem_tag_shift(il[774])
, mem_tsp_shift(il[775])
, mem_val_shift(il[776])
, slice_addr_shift(il[777])
, slice_clk_shift(il[778])
, slice_cnt_shift(il[779])
, slice_col_offset_shift(il[780])
, slice_sel_cd_cpy_shift(il[781])
, slice_sel_mem_active_shift(il[782])
, slice_sel_return_shift(il[783])
, slice_sel_start_shift(il[784])
, slice_space_id_shift(il[785])
{}

AvmFlavor::ProverPolynomials::ProverPolynomials(ProvingKey& proving_key)
Expand Down Expand Up @@ -1577,7 +1576,6 @@ AvmFlavor::AllConstRefValues AvmFlavor::ProverPolynomials::get_row(size_t row_id
main_nullifier_non_exists_write_offset_shift[row_idx],
main_pc_shift[row_idx],
main_sel_execution_row_shift[row_idx],
main_side_effect_counter_shift[row_idx],
main_sload_write_offset_shift[row_idx],
main_sstore_write_offset_shift[row_idx],
mem_glob_addr_shift[row_idx],
Expand Down
8 changes: 4 additions & 4 deletions barretenberg/cpp/src/barretenberg/vm/avm/generated/flavor.hpp
View file
Open in desktop

Large diffs are not rendered by default.

19 changes: 5 additions & 14 deletions barretenberg/cpp/src/barretenberg/vm/avm/generated/relations/kernel.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ template <typename FF_> class kernelImpl {
public:
using FF = FF_;

static constexpr std::array<size_t, 44> SUBRELATION_PARTIAL_LENGTHS = { 3, 3, 4, 4, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3,
static constexpr std::array<size_t, 43> SUBRELATION_PARTIAL_LENGTHS = { 3, 3, 4, 4, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3,
3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 4, 3, 3, 3,
3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3 };
3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3 };

template <typename ContainerOverSubrelations, typename AllEntities>
void static accumulate(ContainerOverSubrelations& evals,
Expand Down Expand Up @@ -363,22 +363,15 @@ template <typename FF_> class kernelImpl {
}
{
using Accumulator = typename std::tuple_element_t<41, ContainerOverSubrelations>;
auto tmp = (main_KERNEL_OUTPUT_SELECTORS *
(new_term.main_side_effect_counter_shift - (new_term.main_side_effect_counter + FF(1))));
auto tmp = (main_KERNEL_INPUT_SELECTORS * (FF(1) - new_term.main_sel_q_kernel_lookup));
tmp *= scaling_factor;
std::get<41>(evals) += typename Accumulator::View(tmp);
}
{
using Accumulator = typename std::tuple_element_t<42, ContainerOverSubrelations>;
auto tmp = (main_KERNEL_INPUT_SELECTORS * (FF(1) - new_term.main_sel_q_kernel_lookup));
tmp *= scaling_factor;
std::get<42>(evals) += typename Accumulator::View(tmp);
}
{
using Accumulator = typename std::tuple_element_t<43, ContainerOverSubrelations>;
auto tmp = (main_KERNEL_OUTPUT_SELECTORS * (FF(1) - new_term.main_sel_q_kernel_output_lookup));
tmp *= scaling_factor;
std::get<43>(evals) += typename Accumulator::View(tmp);
std::get<42>(evals) += typename Accumulator::View(tmp);
}
}
};
Expand Down Expand Up @@ -453,10 +446,8 @@ template <typename FF> class kernel : public Relation<kernelImpl<FF>> {
case 39:
return "SSTORE_KERNEL_OUTPUT";
case 41:
return "SIDE_EFFECT_COUNTER_INCREMENT";
case 42:
return "KERNEL_INPUT_ACTIVE_CHECK";
case 43:
case 42:
return "KERNEL_OUTPUT_ACTIVE_CHECK";
}
return std::to_string(index);
Expand Down
10 changes: 1 addition & 9 deletions barretenberg/cpp/src/barretenberg/vm/avm/generated/relations/kernel_output_lookup.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ class kernel_output_lookup_lookup_settings {
static constexpr size_t WRITE_TERMS = 1;
static constexpr size_t READ_TERM_TYPES[READ_TERMS] = { 0 };
static constexpr size_t WRITE_TERM_TYPES[WRITE_TERMS] = { 0 };
static constexpr size_t LOOKUP_TUPLE_SIZE = 4;
static constexpr size_t LOOKUP_TUPLE_SIZE = 2;
static constexpr size_t INVERSE_EXISTS_POLYNOMIAL_DEGREE = 4;
static constexpr size_t READ_TERM_DEGREE = 0;
static constexpr size_t WRITE_TERM_DEGREE = 0;
Expand All @@ -40,12 +40,8 @@ class kernel_output_lookup_lookup_settings {
in.main_sel_q_kernel_output_lookup,
in.main_sel_kernel_out,
in.main_kernel_out_offset,
in.main_ia,
in.main_side_effect_counter,
in.main_ib,
in.main_clk,
in.main_kernel_value_out,
in.main_kernel_side_effect_out,
in.main_kernel_metadata_out);
}

Expand All @@ -56,12 +52,8 @@ class kernel_output_lookup_lookup_settings {
in.main_sel_q_kernel_output_lookup,
in.main_sel_kernel_out,
in.main_kernel_out_offset,
in.main_ia,
in.main_side_effect_counter,
in.main_ib,
in.main_clk,
in.main_kernel_value_out,
in.main_kernel_side_effect_out,
in.main_kernel_metadata_out);
}
};
Expand Down
13 changes: 9 additions & 4 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 @@ -1771,7 +1771,8 @@ TEST_F(AvmExecutionTests, kernelOutputEmitOpcodes)
auto emit_note_hash_kernel_out_row = std::ranges::find_if(
trace.begin(), trace.end(), [&](Row r) { return r.main_clk == emit_note_hash_out_offset; });
EXPECT_EQ(emit_note_hash_kernel_out_row->main_kernel_value_out, 1);
EXPECT_EQ(emit_note_hash_kernel_out_row->main_kernel_side_effect_out, 0);
// TODO(#8287)
// EXPECT_EQ(emit_note_hash_kernel_out_row->main_kernel_side_effect_out, 0);
feed_output(emit_note_hash_out_offset, 1, 0, 0);

// CHECK EMIT NULLIFIER
Expand Down Expand Up @@ -2174,7 +2175,9 @@ TEST_F(AvmExecutionTests, kernelOutputHashExistsOpcodes)
std::vector<FF> returndata = {};

// Generate Hint for hash exists operation
auto execution_hints = ExecutionHints().with_storage_value_hints({ { 0, 1 }, { 1, 1 }, { 2, 1 } });
auto execution_hints = ExecutionHints()
.with_storage_value_hints({ { 0, 1 }, { 1, 1 }, { 2, 1 } })
.with_note_hash_exists_hints({ { 0, 1 }, { 1, 1 }, { 2, 1 } });

auto trace = Execution::gen_trace(instructions, returndata, calldata, public_inputs_vec, execution_hints);

Expand Down Expand Up @@ -2202,7 +2205,8 @@ TEST_F(AvmExecutionTests, kernelOutputHashExistsOpcodes)
auto nullifier_out_row = std::ranges::find_if(
trace.begin(), trace.end(), [&](Row r) { return r.main_clk == START_NULLIFIER_EXISTS_OFFSET; });
EXPECT_EQ(nullifier_out_row->main_kernel_value_out, 1); // value
EXPECT_EQ(nullifier_out_row->main_kernel_side_effect_out, 1);
// TODO(#8287)
// EXPECT_EQ(nullifier_out_row->main_kernel_side_effect_out, 1);
EXPECT_EQ(nullifier_out_row->main_kernel_metadata_out, 1); // exists
feed_output(START_NULLIFIER_EXISTS_OFFSET, 1, 1, 1);

Expand All @@ -2216,7 +2220,8 @@ TEST_F(AvmExecutionTests, kernelOutputHashExistsOpcodes)
auto msg_out_row = std::ranges::find_if(
trace.begin(), trace.end(), [&](Row r) { return r.main_clk == START_L1_TO_L2_MSG_EXISTS_WRITE_OFFSET; });
EXPECT_EQ(msg_out_row->main_kernel_value_out, 1); // value
EXPECT_EQ(msg_out_row->main_kernel_side_effect_out, 2);
// TODO(#8287)
// EXPECT_EQ(msg_out_row->main_kernel_side_effect_out, 2);
EXPECT_EQ(msg_out_row->main_kernel_metadata_out, 1); // exists
feed_output(START_L1_TO_L2_MSG_EXISTS_WRITE_OFFSET, 1, 2, 1);

Expand Down
9 changes: 6 additions & 3 deletions barretenberg/cpp/src/barretenberg/vm/avm/tests/kernel.test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1217,14 +1217,16 @@ TEST_F(AvmKernelOutputPositiveTests, kernelNoteHashExists)

auto direct_apply_opcodes = [=](AvmTraceBuilder& trace_builder) {
trace_builder.op_set(0, static_cast<uint128_t>(value), value_offset, AvmMemoryTag::FF);
trace_builder.op_note_hash_exists(/*indirect*/ false, value_offset, metadata_offset);
// TODO(#8287): Leaf index isnt constrained properly so we just set it to 0
trace_builder.op_note_hash_exists(/*indirect*/ false, value_offset, 0, metadata_offset);
};
// TODO: fix
auto indirect_apply_opcodes = [=](AvmTraceBuilder& trace_builder) {
trace_builder.op_set(0, static_cast<uint128_t>(value), value_offset, AvmMemoryTag::FF);
trace_builder.op_set(0, value_offset, indirect_value_offset, AvmMemoryTag::U32);
trace_builder.op_set(0, metadata_offset, indirect_metadata_offset, AvmMemoryTag::U32);
trace_builder.op_note_hash_exists(/*indirect*/ 3, indirect_value_offset, indirect_metadata_offset);
// TODO(#8287): Leaf index isnt constrained properly so we just set it to 0
trace_builder.op_note_hash_exists(/*indirect*/ 3, indirect_value_offset, 0, indirect_metadata_offset);
};
auto checks = [=](bool indirect, const std::vector<Row>& trace) {
auto row = std::ranges::find_if(
Expand Down Expand Up @@ -1343,7 +1345,8 @@ TEST_F(AvmKernelOutputPositiveTests, kernelL1ToL2MsgExists)

auto apply_opcodes = [=](AvmTraceBuilder& trace_builder) {
trace_builder.op_set(0, static_cast<uint128_t>(value), value_offset, AvmMemoryTag::FF);
trace_builder.op_l1_to_l2_msg_exists(/*indirect*/ false, value_offset, metadata_offset);
// TODO(#8287): Leaf index isnt constrained properly so we just set it to 0
trace_builder.op_l1_to_l2_msg_exists(/*indirect*/ false, value_offset, 0, metadata_offset);
};
auto checks = [=]([[maybe_unused]] bool indirect, const std::vector<Row>& trace) {
auto row = std::ranges::find_if(
Expand Down
6 changes: 2 additions & 4 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 @@ -650,8 +650,7 @@ std::vector<Row> Execution::gen_trace(std::vector<Instruction> const& instructio
case OpCode::NOTEHASHEXISTS:
trace_builder.op_note_hash_exists(std::get<uint8_t>(inst.operands.at(0)),
std::get<uint32_t>(inst.operands.at(1)),
// TODO: leaf offset exists
// std::get<uint32_t>(inst.operands.at(2))
std::get<uint32_t>(inst.operands.at(2)),
std::get<uint32_t>(inst.operands.at(3)));
break;
case OpCode::EMITNOTEHASH:
Expand All @@ -673,8 +672,7 @@ std::vector<Row> Execution::gen_trace(std::vector<Instruction> const& instructio
case OpCode::L1TOL2MSGEXISTS:
trace_builder.op_l1_to_l2_msg_exists(std::get<uint8_t>(inst.operands.at(0)),
std::get<uint32_t>(inst.operands.at(1)),
// TODO: leaf offset exists
// std::get<uint32_t>(inst.operands.at(2))
std::get<uint32_t>(inst.operands.at(2)),
std::get<uint32_t>(inst.operands.at(3)));
break;
case OpCode::GETCONTRACTINSTANCE:
Expand Down
11 changes: 9 additions & 2 deletions barretenberg/cpp/src/barretenberg/vm/avm/trace/execution_hints.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,15 @@ struct ExecutionHints {
{
std::unordered_map<uint32_t, FF> hints_map;
push_vec_into_map(hints_map, storage_value_hints);
push_vec_into_map(hints_map, note_hash_exists_hints);
push_vec_into_map(hints_map, nullifier_exists_hints);
return hints_map;
}

// Leaf index -> exists
std::unordered_map<uint32_t, FF> get_leaf_index_hints() const
Copy link
Contributor Author

@IlyasRidhuan IlyasRidhuan Sep 2, 2024

Choose a reason for hiding this comment

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

Splitting out leaf index hints since they now use the leaf index instead of side effect counters which would likely collide in a single map

{
std::unordered_map<uint32_t, FF> hints_map;
push_vec_into_map(hints_map, note_hash_exists_hints);
push_vec_into_map(hints_map, l1_to_l2_message_exists_hints);
return hints_map;
}
Expand Down Expand Up @@ -161,4 +168,4 @@ struct ExecutionHints {
{}
};

} // namespace bb::avm_trace
} // namespace bb::avm_trace
22 changes: 20 additions & 2 deletions barretenberg/cpp/src/barretenberg/vm/avm/trace/kernel_trace.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,16 @@ void AvmKernelTraceBuilder::op_note_hash_exists(uint32_t clk,
{

uint32_t offset = START_NOTE_HASH_EXISTS_WRITE_OFFSET + note_hash_exists_offset;
perform_kernel_output_lookup(offset, side_effect_counter, note_hash, FF(result));
// TODO(#8287)Lookups are heavily underconstrained atm
if (result == 1) {
Copy link
Contributor Author

@IlyasRidhuan IlyasRidhuan Sep 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This whole section is a bit messy atm while the lookups are still "half-on". Will be resolved when we clean up the hints/public inputs stuff and if we give the trace.cpp access to the public inputs as well

perform_kernel_output_lookup(offset, side_effect_counter, note_hash, FF(result));
} else {
// if the note_hash does NOT exist, the public inputs already contains the correct output value (i.e. the
// actual value at the index), so we don't try to overwrite the value
std::get<KERNEL_OUTPUTS_SIDE_EFFECT_COUNTER>(public_inputs)[offset] = side_effect_counter;
std::get<KERNEL_OUTPUTS_METADATA>(public_inputs)[offset] = FF(result);
kernel_output_selector_counter[offset]++;
}
note_hash_exists_offset++;

KernelTraceEntry entry = {
Expand Down Expand Up @@ -245,7 +254,16 @@ void AvmKernelTraceBuilder::op_l1_to_l2_msg_exists(uint32_t clk,
uint32_t result)
{
uint32_t offset = START_L1_TO_L2_MSG_EXISTS_WRITE_OFFSET + l1_to_l2_msg_exists_offset;
perform_kernel_output_lookup(offset, side_effect_counter, message, FF(result));
// TODO(#8287)Lookups are heavily underconstrained atm
if (result == 1) {
perform_kernel_output_lookup(offset, side_effect_counter, message, FF(result));
} else {
// if the l1_to_l2_msg_exists is false, the public inputs already contains the correct output value (i.e. the
// actual value at the index), so we don't try to overwrite the value
std::get<KERNEL_OUTPUTS_SIDE_EFFECT_COUNTER>(public_inputs)[offset] = side_effect_counter;
std::get<KERNEL_OUTPUTS_METADATA>(public_inputs)[offset] = FF(result);
kernel_output_selector_counter[offset]++;
}
l1_to_l2_msg_exists_offset++;

KernelTraceEntry entry = {
Expand Down
Loading