Skip to content

Commit

Permalink
chore(avm): make check_relation safer (#11593)
Browse files Browse the repository at this point in the history 
This requires you to use a TestTraceContainer. Then we do `as_rows()` which most importantly generates the shifted columns.
  • Loading branch information
@fcarreiro
fcarreiro authored Jan 29, 2025
1 parent 83fe192 commit 13863eb
Show file tree
Hide file tree
Showing 6 changed files with 42 additions and 68 deletions.
12 changes: 6 additions & 6 deletions barretenberg/cpp/src/barretenberg/vm2/constraining/relations/alu.test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,26 +19,26 @@ using alu = bb::avm2::alu<FF>;

TEST(AluConstrainingTest, BasicAdd)
{
TestTraceContainer::RowTraceContainer trace = {
auto trace = TestTraceContainer::from_rows({
{ .alu_ia = 1, .alu_ib = 2, .alu_ic = 3, .alu_sel_op_add = 1 },
};
});

check_relation<alu>(trace);
}

TEST(AluConstrainingTest, NegativeSelNonBoolean)
{
TestTraceContainer::RowTraceContainer trace = {
auto trace = TestTraceContainer::from_rows({
// Negative test, this should be a boolean only!
{ .alu_sel_op_add = 23 },
};
});

EXPECT_THROW_WITH_MESSAGE(check_relation<alu>(trace, alu::SR_SEL_ADD_BINARY), "SEL_ADD_BINARY");
}

TEST(AluConstrainingTest, NegativeAdd)
{
TestTraceContainer::RowTraceContainer trace = {
auto trace = TestTraceContainer::from_rows({
{
// Wrong ADD.
.alu_ia = 1,
Expand All @@ -47,7 +47,7 @@ TEST(AluConstrainingTest, NegativeAdd)
// Observe that I'm making subrelation SEL_ADD_BINARY fail too, but we'll only check subrelation ALU_ADD!
.alu_sel_op_add = 1,
},
};
});

EXPECT_THROW_WITH_MESSAGE(check_relation<alu>(trace, alu::SR_ALU_ADD), "ALU_ADD");
}
Expand Down
47 changes: 13 additions & 34 deletions barretenberg/cpp/src/barretenberg/vm2/constraining/relations/bc_decomposition.test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ using tracegen::TestTraceContainer;
using FF = AvmFlavorSettings::FF;
using C = Column;
using bc_decomposition = bb::avm2::bc_decomposition<FF>;
using testing::SizeIs;

std::vector<uint8_t> random_bytes(size_t n)
{
Expand All @@ -38,66 +37,48 @@ TEST(BytecodeDecompositionConstrainingTest, EmptyRow)
{ { C::precomputed_first_row, 1 } },
});

check_relation<bc_decomposition>(trace.as_rows());
check_relation<bc_decomposition>(trace);
}

TEST(BytecodeDecompositionConstrainingTest, SingleBytecode)
{
TestTraceContainer trace({
{ { C::precomputed_first_row, 1 } },
});

TestTraceContainer trace;
BytecodeTraceBuilder builder;
builder.process_decomposition(
{ { .bytecode_id = 1, .bytecode = std::make_shared<std::vector<uint8_t>>(random_bytes(40)) } }, trace);

auto rows = trace.as_rows();
EXPECT_THAT(rows, SizeIs(1 + 40));

check_relation<bc_decomposition>(rows);
EXPECT_EQ(trace.get_num_rows(), 1 + 40);
check_relation<bc_decomposition>(trace);
}

TEST(BytecodeDecompositionConstrainingTest, ShortSingleBytecode)
{
TestTraceContainer trace({
{ { C::precomputed_first_row, 1 } },
});

// Bytecode is shorter than the sliding window.
TestTraceContainer trace;
BytecodeTraceBuilder builder;
builder.process_decomposition(
{ { .bytecode_id = 1, .bytecode = std::make_shared<std::vector<uint8_t>>(random_bytes(5)) } }, trace);

auto rows = trace.as_rows();
EXPECT_THAT(rows, SizeIs(1 + 5));

check_relation<bc_decomposition>(rows);
EXPECT_EQ(trace.get_num_rows(), 1 + 5);
check_relation<bc_decomposition>(trace);
}

TEST(BytecodeDecompositionConstrainingTest, MultipleBytecodes)
{
TestTraceContainer trace({
{ { C::precomputed_first_row, 1 } },
});

TestTraceContainer trace;
BytecodeTraceBuilder builder;
builder.process_decomposition(
{ { .bytecode_id = 1, .bytecode = std::make_shared<std::vector<uint8_t>>(random_bytes(40)) },
{ .bytecode_id = 2, .bytecode = std::make_shared<std::vector<uint8_t>>(random_bytes(55)) } },
trace);

auto rows = trace.as_rows();
EXPECT_THAT(rows, SizeIs(1 + 40 + 55));

check_relation<bc_decomposition>(rows);
EXPECT_EQ(trace.get_num_rows(), 1 + 40 + 55);
check_relation<bc_decomposition>(trace);
}

TEST(BytecodeDecompositionConstrainingTest, MultipleBytecodesWithShortOnes)
{
TestTraceContainer trace({
{ { C::precomputed_first_row, 1 } },
});

TestTraceContainer trace;
BytecodeTraceBuilder builder;
builder.process_decomposition(
{ { .bytecode_id = 1, .bytecode = std::make_shared<std::vector<uint8_t>>(random_bytes(40)) },
Expand All @@ -107,10 +88,8 @@ TEST(BytecodeDecompositionConstrainingTest, MultipleBytecodesWithShortOnes)
{ .bytecode_id = 5, .bytecode = std::make_shared<std::vector<uint8_t>>(random_bytes(2)) } },
trace);

auto rows = trace.as_rows();
EXPECT_THAT(rows, SizeIs(1 + 40 + 5 + 10 + 55 + 2));

check_relation<bc_decomposition>(rows);
EXPECT_EQ(trace.get_num_rows(), 1 + 40 + 5 + 10 + 55 + 2);
check_relation<bc_decomposition>(trace);
}

} // namespace
Expand Down
19 changes: 7 additions & 12 deletions barretenberg/cpp/src/barretenberg/vm2/constraining/relations/execution.test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ TEST(ExecutionConstrainingTest, Basic)
});
// clang-format on

check_relation<execution>(trace.as_rows());
check_relation<execution>(trace);
}

TEST(ExecutionConstrainingTest, Continuity)
Expand All @@ -40,7 +40,7 @@ TEST(ExecutionConstrainingTest, Continuity)
});
// clang-format on

check_relation<execution>(trace.as_rows(), execution::SR_TRACE_CONTINUITY_1, execution::SR_TRACE_CONTINUITY_2);
check_relation<execution>(trace, execution::SR_TRACE_CONTINUITY_1, execution::SR_TRACE_CONTINUITY_2);
}

TEST(ExecutionConstrainingTest, ContinuityBrokenFirstRow)
Expand All @@ -54,8 +54,7 @@ TEST(ExecutionConstrainingTest, ContinuityBrokenFirstRow)
});
// clang-format on

EXPECT_THROW_WITH_MESSAGE(check_relation<execution>(trace.as_rows(), execution::SR_TRACE_CONTINUITY_2),
"TRACE_CONTINUITY_2");
EXPECT_THROW_WITH_MESSAGE(check_relation<execution>(trace, execution::SR_TRACE_CONTINUITY_2), "TRACE_CONTINUITY_2");
}

TEST(ExecutionConstrainingTest, ContinuityBrokenInMiddle)
Expand All @@ -69,10 +68,8 @@ TEST(ExecutionConstrainingTest, ContinuityBrokenInMiddle)
});
// clang-format on

EXPECT_THROW_WITH_MESSAGE(check_relation<execution>(trace.as_rows(), execution::SR_TRACE_CONTINUITY_1),
"TRACE_CONTINUITY_1");
EXPECT_THROW_WITH_MESSAGE(check_relation<execution>(trace.as_rows(), execution::SR_TRACE_CONTINUITY_2),
"TRACE_CONTINUITY_2");
EXPECT_THROW_WITH_MESSAGE(check_relation<execution>(trace, execution::SR_TRACE_CONTINUITY_1), "TRACE_CONTINUITY_1");
EXPECT_THROW_WITH_MESSAGE(check_relation<execution>(trace, execution::SR_TRACE_CONTINUITY_2), "TRACE_CONTINUITY_2");
}

TEST(ExecutionConstrainingTest, ContinuityBrokenAtTheEnd)
Expand All @@ -85,8 +82,7 @@ TEST(ExecutionConstrainingTest, ContinuityBrokenAtTheEnd)
});
// clang-format on

EXPECT_THROW_WITH_MESSAGE(check_relation<execution>(trace.as_rows(), execution::SR_TRACE_CONTINUITY_1),
"TRACE_CONTINUITY_1");
EXPECT_THROW_WITH_MESSAGE(check_relation<execution>(trace, execution::SR_TRACE_CONTINUITY_1), "TRACE_CONTINUITY_1");
}

TEST(ExecutionConstrainingTest, ContinuityMultipleLast)
Expand All @@ -100,8 +96,7 @@ TEST(ExecutionConstrainingTest, ContinuityMultipleLast)
});
// clang-format on

EXPECT_THROW_WITH_MESSAGE(check_relation<execution>(trace.as_rows(), execution::SR_LAST_IS_LAST),
"LAST_IS_LAST.*row 1");
EXPECT_THROW_WITH_MESSAGE(check_relation<execution>(trace, execution::SR_LAST_IS_LAST), "LAST_IS_LAST.*row 1");
}

} // namespace
Expand Down
18 changes: 9 additions & 9 deletions barretenberg/cpp/src/barretenberg/vm2/constraining/relations/range_check.test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ TEST(RangeCheckConstrainingTest, EmptyRow)
{ { C::precomputed_clk, 1 } },
});

check_relation<range_check>(trace.as_rows());
check_relation<range_check>(trace);
}

TEST(RangeCheckConstrainingTest, IsLteMutuallyExclusive)
Expand All @@ -35,7 +35,7 @@ TEST(RangeCheckConstrainingTest, IsLteMutuallyExclusive)
{ { C::range_check_sel, 1 }, { C::range_check_is_lte_u32, 1 } },
});

check_relation<range_check>(trace.as_rows(), range_check::SR_IS_LTE_MUTUALLY_EXCLUSIVE);
check_relation<range_check>(trace, range_check::SR_IS_LTE_MUTUALLY_EXCLUSIVE);
}

TEST(RangeCheckConstrainingTest, NegativeIsLteMutuallyExclusive)
Expand All @@ -45,7 +45,7 @@ TEST(RangeCheckConstrainingTest, NegativeIsLteMutuallyExclusive)
{ { C::range_check_sel, 1 }, { C::range_check_is_lte_u32, 1 }, { C::range_check_is_lte_u112, 1 } },
});

EXPECT_THROW_WITH_MESSAGE(check_relation<range_check>(trace.as_rows(), range_check::SR_IS_LTE_MUTUALLY_EXCLUSIVE),
EXPECT_THROW_WITH_MESSAGE(check_relation<range_check>(trace, range_check::SR_IS_LTE_MUTUALLY_EXCLUSIVE),
"IS_LTE_MUTUALLY_EXCLUSIVE");
}

Expand All @@ -67,7 +67,7 @@ TEST(RangeCheckConstrainingTest, CheckRecomposition)
{ C::range_check_u16_r7, dynamic_slice_register },
} });

check_relation<range_check>(trace.as_rows(), range_check::SR_CHECK_RECOMPOSITION);
check_relation<range_check>(trace, range_check::SR_CHECK_RECOMPOSITION);
}

TEST(RangeCheckConstrainingTest, NegativeCheckRecomposition)
Expand All @@ -89,7 +89,7 @@ TEST(RangeCheckConstrainingTest, NegativeCheckRecomposition)
{ C::range_check_u16_r7, dynamic_slice_register },
} });

EXPECT_THROW_WITH_MESSAGE(check_relation<range_check>(trace.as_rows(), range_check::SR_CHECK_RECOMPOSITION),
EXPECT_THROW_WITH_MESSAGE(check_relation<range_check>(trace, range_check::SR_CHECK_RECOMPOSITION),
"CHECK_RECOMPOSITION");
}

Expand Down Expand Up @@ -126,7 +126,7 @@ TEST(RangeCheckConstrainingTest, Full)
{ C::range_check_sel_r1_16_bit_rng_lookup, 1 },
} });

check_relation<range_check>(trace.as_rows());
check_relation<range_check>(trace);
}

TEST(RangeCheckConstrainingTest, NegativeMissingLookup)
Expand Down Expand Up @@ -162,7 +162,7 @@ TEST(RangeCheckConstrainingTest, NegativeMissingLookup)
{ C::range_check_sel_r1_16_bit_rng_lookup, 0 }, // BAD! SHOULD BE 1
} });

EXPECT_THROW_WITH_MESSAGE(check_relation<range_check>(trace.as_rows()), "Relation range_check");
EXPECT_THROW_WITH_MESSAGE(check_relation<range_check>(trace), "Relation range_check");
}

TEST(RangeCheckConstrainingTest, WithTracegen)
Expand All @@ -187,7 +187,7 @@ TEST(RangeCheckConstrainingTest, WithTracegen)
},
trace);

check_relation<range_check>(trace.as_rows());
check_relation<range_check>(trace);
}

TEST(RangeCheckConstrainingTest, NegativeWithTracegen)
Expand All @@ -210,7 +210,7 @@ TEST(RangeCheckConstrainingTest, NegativeWithTracegen)
},
trace);

EXPECT_THROW_WITH_MESSAGE(check_relation<range_check>(trace.as_rows()), "Relation range_check");
EXPECT_THROW_WITH_MESSAGE(check_relation<range_check>(trace), "Relation range_check");
}

} // namespace
Expand Down
6 changes: 2 additions & 4 deletions barretenberg/cpp/src/barretenberg/vm2/constraining/relations/sha256.test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ TEST(Sha256ConstrainingTest, EmptyRow)
{ { C::precomputed_clk, 1 } },
});

check_relation<sha256>(trace.as_rows());
check_relation<sha256>(trace);
}

// This test imports a bunch of external code since hand-generating the sha256 trace is a bit laborious atm.
Expand Down Expand Up @@ -71,9 +71,7 @@ TEST(Sha256ConstrainingTest, Basic)
const auto sha256_event_container = sha256_event_emitter.dump_events();
builder.process(sha256_event_container);

TestTraceContainer::RowTraceContainer rows = trace.as_rows();

check_relation<sha256>(rows);
check_relation<sha256>(trace);
}

} // namespace
Expand Down
8 changes: 5 additions & 3 deletions barretenberg/cpp/src/barretenberg/vm2/constraining/testing/check_relation.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <stdexcept>

#include "barretenberg/common/log.hpp"
#include "barretenberg/vm2/tracegen/test_trace_container.hpp"

namespace bb::avm2::constraining {

Expand All @@ -28,13 +29,14 @@ void check_relation_internal(const Trace& trace, std::span<size_t> subrelations)
}
}

template <typename Relation, typename Trace, typename... Ts> void check_relation(const Trace& trace, Ts... subrelation)
template <typename Relation, typename... Ts>
void check_relation(const tracegen::TestTraceContainer& trace, Ts... subrelation)
{
std::array<size_t, sizeof...(Ts)> subrelations = { subrelation... };
check_relation_internal<Relation>(trace, subrelations);
check_relation_internal<Relation>(trace.as_rows(), subrelations);
}

template <typename Relation, typename Trace> void check_relation(const Trace& trace)
template <typename Relation> void check_relation(const tracegen::TestTraceContainer& trace)
{
auto subrelations = std::make_index_sequence<Relation::SUBRELATION_PARTIAL_LENGTHS.size()>();
[&]<size_t... Is>(std::index_sequence<Is...>) { check_relation<Relation>(trace, Is...); }(subrelations);
Expand Down

1 comment on commit 13863eb

@AztecBot
Copy link
Collaborator

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'C++ Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.05.

Benchmark suite Current: 13863eb Previous: 570cdba Ratio
nativeClientIVCBench/Ambient_17_in_20/6 20342.591107999993 ms/iter 18883.298175999982 ms/iter 1.08
nativeconstruct_proof_ultrahonk_power_of_2/20 4642.629907000014 ms/iter 4149.315693000006 ms/iter 1.12
Goblin::merge(t) 141495750 ns/iter 132750675 ns/iter 1.07

This comment was automatically generated by workflow using github-action-benchmark.

CC: @ludamad @codygunton

Please sign in to comment.