Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@ namespace bb {
template <typename Params, typename View>
using GetParameterView = std::conditional_t<IsField<typename Params::DataType>, typename Params::DataType, View>;

template <typename T, size_t subrelation_idx>
template <typename T>
concept HasSubrelationLinearlyIndependentMember = requires(T) {
{
std::get<subrelation_idx>(T::SUBRELATION_LINEARLY_INDEPENDENT)
std::get<0>(T::SUBRELATION_LINEARLY_INDEPENDENT)
} -> std::convertible_to<bool>;
};

Expand All @@ -41,7 +41,7 @@ concept HasParameterLengthAdjustmentsMember = requires { T::TOTAL_LENGTH_ADJUSTM
*/
template <typename Relation, size_t subrelation_index> constexpr bool subrelation_is_linearly_independent()
{
if constexpr (HasSubrelationLinearlyIndependentMember<Relation, subrelation_index>) {
if constexpr (HasSubrelationLinearlyIndependentMember<Relation>) {
return std::get<subrelation_index>(Relation::SUBRELATION_LINEARLY_INDEPENDENT);
} else {
return true;
Expand Down
3 changes: 3 additions & 0 deletions barretenberg/cpp/src/barretenberg/vm2/common/stats.test.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
// This is a hack needed to be able to use files that use stats (e.g., constraining/polynomials.cpp) in testing in vm2
// It can be removed once VM1 is removed and stats are moved to VM2.
#include "barretenberg/vm/stats.cpp"
93 changes: 93 additions & 0 deletions barretenberg/cpp/src/barretenberg/vm2/constraining/polynomials.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
#include "barretenberg/vm2/constraining/polynomials.hpp"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved from proving_helper.


#include <cstdint>

#include "barretenberg/common/thread.hpp"
#include "barretenberg/vm/stats.hpp"
#include "barretenberg/vm2/common/constants.hpp"
#include "barretenberg/vm2/generated/columns.hpp"

namespace bb::avm2::constraining {

AvmProver::ProverPolynomials compute_polynomials(tracegen::TraceContainer& trace)
{
AvmProver::ProverPolynomials polys;

// Polynomials that will be shifted need special care.
AVM_TRACK_TIME("proving/init_polys_to_be_shifted", ({
auto to_be_shifted = polys.get_to_be_shifted();

// TODO: cannot parallelize because Polynomial construction uses parallelism.
for (size_t i = 0; i < to_be_shifted.size(); i++) {
auto& poly = to_be_shifted[i];
// WARNING! Column-Polynomials order matters!
Column col = static_cast<Column>(TO_BE_SHIFTED_COLUMNS_ARRAY.at(i));
// We need at least 2 rows for the shifted columns.
uint32_t num_rows = std::max<uint32_t>(trace.get_column_rows(col), 2);

poly = AvmProver::Polynomial(
/*memory size*/
num_rows - 1,
/*largest possible index*/ CIRCUIT_SUBGROUP_SIZE,
/*make shiftable with offset*/ 1);
}
}));

// Catch-all with fully formed polynomials
// Note: derived polynomials (i.e., inverses) are not in the trace at this point, because they can only
// be computed after committing to the other witnesses. Therefore, they will be initialized as empty
// and they will be not set below. The derived polynomials will be reinitialized and set in the prover
// itself mid-proving. (TO BE DONE!).
//
// NOTE FOR SELF: however, the counts will be known here and the inv have the same size?
// think about it and check the formula.
AVM_TRACK_TIME("proving/init_polys_unshifted", ({
auto unshifted = polys.get_unshifted();

// Derived polynomials will be empty.
bb::parallel_for(unshifted.size(), [&](size_t i) {
auto& poly = unshifted[i];
// FIXME: this is a bad way to check if the polynomial is already initialized.
// It could be that it has been initialized, but it's all zeroes.
if (!poly.is_empty()) {
// Already initialized above.
return;
}

// WARNING! Column-Polynomials order matters!
Column col = static_cast<Column>(i);
const auto num_rows = trace.get_column_rows(col);
poly = AvmProver::Polynomial::create_non_parallel_zero_init(num_rows, CIRCUIT_SUBGROUP_SIZE);
});
}));

AVM_TRACK_TIME("proving/set_polys_unshifted", ({
auto unshifted = polys.get_unshifted();

// TODO: We are now visiting per-column. Profile if per-row is better.
// This would need changes to the trace container.
bb::parallel_for(unshifted.size(), [&](size_t i) {
// WARNING! Column-Polynomials order matters!
auto& poly = unshifted[i];
Column col = static_cast<Column>(i);

trace.visit_column(col, [&](size_t row, const AvmProver::FF& value) {
// We use `at` because we are sure the row exists and the value is non-zero.
poly.at(row) = value;
});
// We free columns as we go.
// TODO: If we merge the init with the setting, this would be even more memory efficient.
trace.clear_column(col);
Comment on lines +74 to +80
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this file was just moved, but I'd make a comment with a warning that this function kills the trace by clearing the columns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. In the hpp.

});
}));

AVM_TRACK_TIME("proving/set_polys_shifted", ({
for (auto [shifted, to_be_shifted] : zip_view(polys.get_shifted(), polys.get_to_be_shifted())) {
shifted = to_be_shifted.shifted();
}
}));

return polys;
}

} // namespace bb::avm2::constraining
11 changes: 11 additions & 0 deletions barretenberg/cpp/src/barretenberg/vm2/constraining/polynomials.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
#pragma once

#include "barretenberg/vm2/generated/prover.hpp"
#include "barretenberg/vm2/tracegen/trace_container.hpp"

namespace bb::avm2::constraining {

// Computes the polynomials from the trace, and destroys it in the process.
Copy link
Contributor

Choose a reason for hiding this comment

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

@fcarreiro : If this function destroys the trace, cannot we use std::move mechanism to make it clear that ownership is passed to the function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried, but this function is used in a context where the caller cannot bind to an && reference. A bit complicated. I might make some changes at some point but it was not trivial.

AvmProver::ProverPolynomials compute_polynomials(tracegen::TraceContainer& trace);

} // namespace bb::avm2::constraining
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@
#include "barretenberg/vm2/constraining/testing/check_relation.hpp"
#include "barretenberg/vm2/generated/flavor_settings.hpp"
#include "barretenberg/vm2/generated/relations/execution.hpp"
#include "barretenberg/vm2/generated/relations/lookups_execution.hpp"
#include "barretenberg/vm2/generated/relations/perms_execution.hpp"
#include "barretenberg/vm2/testing/macros.hpp"
#include "barretenberg/vm2/tracegen/lib/permutation_builder.hpp"
#include "barretenberg/vm2/tracegen/test_trace_container.hpp"

namespace bb::avm2::constraining {
Expand All @@ -16,19 +19,34 @@ using tracegen::TestTraceContainer;
using FF = AvmFlavorSettings::FF;
using C = Column;
using execution = bb::avm2::execution<FF>;
using perm_dummy_dynamic_relation = perm_dummy_dynamic_relation<FF>;

TEST(ExecutionConstrainingTest, Basic)
{
// clang-format off
TestTraceContainer trace({
{{ C::execution_sel, 1 }, {C::execution_clk, 0}, { C::execution_pc, 0 }},
{{ C::execution_sel, 1 }, {C::execution_clk, 1}, { C::execution_pc, 20 }, { C::execution_last, 1 }}
{{ C::execution_sel, 1 }, {C::execution_clk, 1}, { C::execution_pc, 20 }, { C::execution_last, 1 }}
});
// clang-format on

check_relation<execution>(trace);
}

// This is just a PoC.
TEST(ExecutionConstrainingTest, Permutation)
Copy link
Contributor

Choose a reason for hiding this comment

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

@fcarreiro Do you have any similar test for dynamic lookups?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't write one but it should be the same just changing the permutation for the lookupintodynamic.

{
// clang-format off
TestTraceContainer trace({
{{ C::precomputed_first_row, 1 }},
{{ C::execution_sel, 1 }, {C::execution_op1, 1}, {C::execution_op2, 2}, {C::execution_op3, 3}, {C::execution_op4, 4}}
});
// clang-format on

tracegen::PermutationBuilder<perm_dummy_dynamic_relation::Settings>().process(trace);
check_interaction<perm_dummy_dynamic_relation>(trace);
}

TEST(ExecutionConstrainingTest, Continuity)
{
// clang-format off
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
#include "barretenberg/vm2/constraining/testing/check_relation.hpp"

namespace bb::avm2::constraining::detail {

const RelationParameters<FF>& get_test_params()
{
static RelationParameters<FF> params = {
.eta = 0,
.beta = FF::random_element(),
.gamma = FF::random_element(),
.public_input_delta = 0,
.lookup_grand_product_delta = 0,
.beta_sqr = 0,
.beta_cube = 0,
.eccvm_set_permutation_delta = 0,
};
return params;
}

} // namespace bb::avm2::constraining::detail
Original file line number Diff line number Diff line change
@@ -1,23 +1,42 @@
#pragma once

#include <array>
#include <cstdlib>
#include <span>
#include <stdexcept>

#include "barretenberg/common/log.hpp"
#include "barretenberg/honk/proof_system/logderivative_library.hpp"
#include "barretenberg/relations/relation_parameters.hpp"
#include "barretenberg/relations/relation_types.hpp"
#include "barretenberg/vm2/constraining/polynomials.hpp"
#include "barretenberg/vm2/tracegen/test_trace_container.hpp"

namespace bb::avm2::constraining {
namespace detail {
Copy link
Contributor

Choose a reason for hiding this comment

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

why the detail namespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's common in C++ when you want to hide/indicate that this should not be used externally, but you still need to keep it in a header file (templates, etc). In header files you should not use anonymous namespaces, which is what you'd do in cpp files.


template <typename Relation, typename Trace>
void check_relation_internal(const Trace& trace, std::span<size_t> subrelations)
const RelationParameters<FF>& get_test_params();

template <typename Relation> constexpr bool subrelation_is_linearly_independent(size_t subrelation_index)
{
if constexpr (HasSubrelationLinearlyIndependentMember<Relation>) {
return Relation::SUBRELATION_LINEARLY_INDEPENDENT[subrelation_index];
} else {
return true;
}
}

template <typename Relation, typename Trace, typename RowGetter>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I needed to template how to get the row, because it works differently on traces and polynomials.

void check_relation_internal(const Trace& trace, std::span<const size_t> subrelations, RowGetter get_row)
{
typename Relation::SumcheckArrayOfValuesOverSubrelations result{};

// Accumulate the trace over the subrelations and check the result
// if the subrelation is linearly independent.
for (size_t r = 0; r < trace.size(); ++r) {
Relation::accumulate(result, trace.at(r), {}, 1);
Relation::accumulate(result, get_row(trace, r), get_test_params(), 1);
for (size_t j : subrelations) {
if (!result[j].is_zero()) {
if (subrelation_is_linearly_independent<Relation>(j) && !result[j].is_zero()) {
throw std::runtime_error(format("Relation ",
Relation::NAME,
", subrelation ",
Expand All @@ -27,13 +46,26 @@ void check_relation_internal(const Trace& trace, std::span<size_t> subrelations)
}
}
}
// For all subrelations, the result should be zero at the end of the trace.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this to work both with linearly dependent or independent subrelations. The dependent ones are only checked at the end.

for (size_t j : subrelations) {
if (!result[j].is_zero()) {
throw std::runtime_error(format("Relation ",
Relation::NAME,
", subrelation ",
Relation::get_subrelation_label(j),
" is non-zero at end of trace"));
}
}
}

} // namespace detail

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.as_rows(), subrelations);
detail::check_relation_internal<Relation>(
trace.as_rows(), subrelations, [](const auto& trace, size_t r) { return trace.at(r); });
}

template <typename Relation> void check_relation(const tracegen::TestTraceContainer& trace)
Expand All @@ -42,4 +74,27 @@ template <typename Relation> void check_relation(const tracegen::TestTraceContai
[&]<size_t... Is>(std::index_sequence<Is...>) { check_relation<Relation>(trace, Is...); }(subrelations);
}

// Computes logderiv inverses and checks the lookup or permutation.
template <typename Lookup> void check_interaction(const tracegen::TestTraceContainer& trace)
{
using Settings = typename Lookup::Settings;
if (trace.get_column_rows(Settings::INVERSES) == 0) {
std::cerr << "Inverses for " << Lookup::NAME
<< " are unset. Did you forget to run a lookup/permutation builder?" << std::endl;
abort();
}
// We copy the trace because constructing the polynomials destroys it.
auto trace_copy = trace;
const auto num_rows = trace.get_num_rows();
// We compute the polys and the real inverses.
auto polys = constraining::compute_polynomials(trace_copy);
bb::compute_logderivative_inverse<FF, Lookup>(polys, detail::get_test_params(), num_rows);
// Finally we check the interaction.
[&]<size_t... Is>(std::index_sequence<Is...>) {
constexpr std::array<size_t, sizeof...(Is)> subrels = { Is... };
detail::check_relation_internal<Lookup>(
polys, subrels, [](const auto& polys, size_t r) { return polys.get_row(r); });
}(std::make_index_sequence<Lookup::SUBRELATION_PARTIAL_LENGTHS.size()>());
Comment on lines +92 to +97
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you comment here a bit more about the the use of the elipses and what Is is? Also why do you need to create a lambda and then call it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately that would require a course in C++ metaprogramming. The person attempting to understand this code will either understand it, or should not modify it 😆 . I can try to explain over huddle some day but really there is no short explanation that would fit in a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

}

} // namespace bb::avm2::constraining
Original file line number Diff line number Diff line change
Expand Up @@ -72,15 +72,24 @@ template <typename FF_>
class lookup_bytecode_bytes_are_bytes_relation
: public GenericLookupRelation<lookup_bytecode_bytes_are_bytes_lookup_settings, FF_> {
public:
using Settings = lookup_bytecode_bytes_are_bytes_lookup_settings;
static constexpr std::string_view NAME = lookup_bytecode_bytes_are_bytes_lookup_settings::NAME;

template <typename AllEntities> inline static bool skip(const AllEntities& in)
{
return in.bc_decomposition_sel.is_zero() && in.precomputed_sel_range_8.is_zero();
}

static std::string get_subrelation_label(size_t index)
{
if (index == 0) {
return "INVERSES_ARE_CORRECT";
} else if (index == 1) {
return "ACCUMULATION_IS_CORRECT";
}
return std::to_string(index);
}
};
template <typename FF_>
using lookup_bytecode_bytes_are_bytes = GenericLookup<lookup_bytecode_bytes_are_bytes_lookup_settings, FF_>;

/////////////////// lookup_bytecode_to_read_unary ///////////////////

Expand Down Expand Up @@ -149,14 +158,23 @@ template <typename FF_>
class lookup_bytecode_to_read_unary_relation
: public GenericLookupRelation<lookup_bytecode_to_read_unary_lookup_settings, FF_> {
public:
using Settings = lookup_bytecode_to_read_unary_lookup_settings;
static constexpr std::string_view NAME = lookup_bytecode_to_read_unary_lookup_settings::NAME;

template <typename AllEntities> inline static bool skip(const AllEntities& in)
{
return in.bc_decomposition_sel.is_zero() && in.precomputed_sel_unary.is_zero();
}

static std::string get_subrelation_label(size_t index)
{
if (index == 0) {
return "INVERSES_ARE_CORRECT";
} else if (index == 1) {
return "ACCUMULATION_IS_CORRECT";
}
return std::to_string(index);
}
};
template <typename FF_>
using lookup_bytecode_to_read_unary = GenericLookup<lookup_bytecode_to_read_unary_lookup_settings, FF_>;

} // namespace bb::avm2
Loading
Loading