Skip to content

Commit

Permalink
feat: flavor refactor, reduce duplication (#3407)
Browse files Browse the repository at this point in the history
Reduces duplication in Flavor classes by using a macro that combines
pointer_view with struct fields (along with a new get_all getter that is
perhaps more ergonomic) and subclassing. Note that this is a stepping
stone, ideally AztecProtocol/barretenberg#788
is used instead of multiple inheritance
Also resolves AztecProtocol/barretenberg#784
- Introduces a RefVector class, like `std::vector<T&>` (which is
impossible). It is backed by a pointer, and gives operator[] and
iterator semantics on the underlying derefenced pointer. Compare
std::vector<std::reference_wrapper<T>>, which would operate more like
actual pointers (you change the reference, not the value held by the
reference).
- Introduces a get_all alternative to pointer_view that uses RefVector,
update other types to use RefVector, get rid of the HandleType in flavor
entity classes
- Removes some flavor base classes that didn't do much after array
backing removal. Now we get rid of virtual base class methods here, but
I don't think it makes sense to do polymorphism in two ways. We can
instead use concepts if we want to constrain this with a static assert
- Introduces macros that define flavor classes in flavor_macros.hpp.
These remove duplication of pointer view and normal listing.
- Use inheritance to remove duplication in the larger AllEntities
classes in GoblinUltra and GoblinTranslator
- Removes modernize-use-nodiscard - too noisy a linter setting, I doubt
we'd hit a bug that it would fix (i.e. if you return error codes it can
be useful to not ignore them).

---------

Co-authored-by: ludamad <adam@aztecprotocol.com>
  • Loading branch information
ludamad and ludamad0 authored Nov 30, 2023
1 parent 798565d commit 8d6b013
Show file tree
Hide file tree
Showing 30 changed files with 2,126 additions and 3,173 deletions.
4 changes: 4 additions & 0 deletions barretenberg/cpp/.clangd
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@ Diagnostics:
- google-explicit-constructor
# Not honouring.
- cppcoreguidelines-owning-memory
# "This check is deprecated since it’s no longer part of the CERT standard. It will be removed in clang-tidy version 19."
- cert-dc21-cpp
# Noisy. As we don't need to return error types or raw allocations, really unlikely we'd cause problems by ignoring a return type.
- modernize-use-nodiscard

--- # this divider is necessary
# Disable some checks for Google Test/Bench
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ template <typename Flavor, typename Relation> void execute_relation(::benchmark:
auto params = proof_system::RelationParameters<FF>::get_random();

// Extract an array containing all the polynomial evaluations at a given row i
AllValues new_value;
AllValues new_value{};
// Define the appropriate SumcheckArrayOfValuesOverSubrelations type for this relation and initialize to zero
SumcheckArrayOfValuesOverSubrelations accumulator;
// Evaluate each constraint in the relation and check that each is satisfied
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#pragma once
#include "barretenberg/common/ref_vector.hpp"
#include "barretenberg/common/zip_view.hpp"
#include "barretenberg/polynomials/polynomial.hpp"
namespace proof_system::honk::pcs::zeromorph {
Expand Down Expand Up @@ -321,7 +322,8 @@ template <typename Curve> class ZeroMorphProver_ {
auto& transcript,
const std::vector<std::span<FF>>& concatenated_polynomials = {},
const std::vector<FF>& concatenated_evaluations = {},
const std::vector<std::vector<std::span<FF>>>& concatenation_groups = {})
// TODO(https://github.com/AztecProtocol/barretenberg/issues/743) remove span
const std::vector<RefVector<std::span<FF>>>& concatenation_groups = {})
{
// Generate batching challenge \rho and powers 1,...,\rho^{m-1}
FF rho = transcript.get_challenge("rho");
Expand Down Expand Up @@ -513,14 +515,14 @@ template <typename Curve> class ZeroMorphVerifier_ {
* @param concatenation_groups_commitments
* @return Commitment
*/
static Commitment compute_C_Z_x(std::vector<Commitment> f_commitments,
std::vector<Commitment> g_commitments,
static Commitment compute_C_Z_x(const std::vector<Commitment>& f_commitments,
const std::vector<Commitment>& g_commitments,
std::vector<Commitment>& C_q_k,
FF rho,
FF batched_evaluation,
FF x_challenge,
std::vector<FF> u_challenge,
const std::vector<std::vector<Commitment>>& concatenation_groups_commitments = {})
const std::vector<RefVector<Commitment>>& concatenation_groups_commitments = {})
{
size_t log_N = C_q_k.size();
size_t N = 1 << log_N;
Expand Down Expand Up @@ -611,7 +613,7 @@ template <typename Curve> class ZeroMorphVerifier_ {
* @brief Utility for native batch multiplication of group elements
* @note This is used only for native verification and is not optimized for efficiency
*/
static Commitment batch_mul_native(std::vector<Commitment> points, std::vector<FF> scalars)
static Commitment batch_mul_native(const std::vector<Commitment>& points, const std::vector<FF>& scalars)
{
auto result = points[0] * scalars[0];
for (size_t idx = 1; idx < scalars.size(); ++idx) {
Expand All @@ -637,7 +639,7 @@ template <typename Curve> class ZeroMorphVerifier_ {
auto&& shifted_evaluations,
auto& multivariate_challenge,
auto& transcript,
const std::vector<std::vector<Commitment>>& concatenation_group_commitments = {},
const std::vector<RefVector<Commitment>>& concatenation_group_commitments = {},
const std::vector<FF>& concatenated_evaluations = {})
{
size_t log_N = multivariate_challenge.size();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ template <class Curve> class ZeroMorphWithConcatenationTest : public CommitmentT
prover_transcript,
concatenated_polynomials_views,
c_evaluations,
concatenation_groups_views);
to_vector_of_ref_vectors(concatenation_groups_views));

auto verifier_transcript = BaseTranscript<Fr>::verifier_init_empty(prover_transcript);

Expand All @@ -257,7 +257,7 @@ template <class Curve> class ZeroMorphWithConcatenationTest : public CommitmentT
w_evaluations, // shifted
u_challenge,
verifier_transcript,
concatenation_groups_commitments,
to_vector_of_ref_vectors(concatenation_groups_commitments),
c_evaluations);

verified = this->vk()->pairing_check(pairing_points[0], pairing_points[1]);
Expand Down
27 changes: 0 additions & 27 deletions barretenberg/cpp/src/barretenberg/common/constexpr_utils.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@
*
* constexpr_for : loop over a range , where the size_t iterator `i` is a constexpr variable
* constexpr_find : find if an element is in an array
* concatenate_arrays : smoosh multiple std::array objects into a single std::array
*
*/
namespace barretenberg {

Expand Down Expand Up @@ -121,31 +119,6 @@ template <const auto& container, auto key> constexpr bool constexpr_find()
return found;
}

/**
* @brief merges multiple std::arrays into a single array.
* Array lengths can be different but array type must match
* Method is constexpr and should concat constexpr arrays at compile time
*
* @tparam Type the array type
* @tparam sizes template parameter pack of size_t value params
* @param arrays
* @return constexpr auto
*
* @details template params should be autodeducted. Example use case:
*
* ```
* std::array<int, 2> a{1, 2};
* std::array<int, 3> b{1,3, 5};
* std::array<int, 5> c = concatenate(a, b);
* ```
*/
template <typename Type, std::size_t... sizes>
constexpr auto concatenate_arrays(const std::array<Type, sizes>&... arrays)
{
return std::apply([](auto... elems) -> std::array<Type, (sizes + ...)> { return { { elems... } }; },
std::tuple_cat(std::tuple_cat(arrays)...));
}

/**
* @brief Create a constexpr array object whose elements contain a default value
*
Expand Down
140 changes: 140 additions & 0 deletions barretenberg/cpp/src/barretenberg/common/ref_array.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
#include "barretenberg/common/assert.hpp"
#include <array>
#include <cstddef>
#include <initializer_list>
#include <iterator>
#include <stdexcept>

// TODO(https://github.com/AztecProtocol/barretenberg/issues/794) namespace this once convenient
/**
* @brief A template class for a reference array. Behaves as if std::array<T&, N> was possible.
*
* This class provides a fixed-size array of pointers to elements of type T, exposed as references.
* It offers random access to its elements and provides an iterator class
* for traversal.
*
* @tparam T The type of elements stored in the array.
* @tparam N The size of the array.
*/
template <typename T, std::size_t N> class RefArray {
public:
RefArray(const std::array<T*, N>& ptr_array)
{
std::size_t i = 0;
for (T& elem : ptr_array) {
storage[i++] = &elem;
}
}
RefArray(std::initializer_list<T&> init)
{
if (init.size() != N) {
throw std::invalid_argument("Initializer list size does not match RefArray size");
}
std::size_t i = 0;
for (auto& elem : init) {
storage[i++] = &elem;
}
}

T& operator[](std::size_t idx) const
{
ASSERT(idx < N);
return *storage[idx];
}

/**
* @brief Nested iterator class for RefArray, based on indexing into the pointer array.
* Provides semantics similar to what would be expected if std::array<T&, N> was possible.
*/
class iterator {
public:
/**
* @brief Constructs an iterator for a given RefArray object.
*
* @param array Pointer to the RefArray object.
* @param pos The starting position in the array.
*/
iterator(RefArray const* array, std::size_t pos)
: array(array)
, pos(pos)
{}

T& operator*() const { return (*array)[pos]; }

iterator& operator++()
{
pos++;
return *this;
}

iterator operator++(int)
{
iterator temp = *this;
++(*this);
return temp;
}

bool operator==(iterator const& other) const { return pos == other.pos; }
bool operator!=(iterator const& other) const { return pos != other.pos; }

private:
RefArray const* array;
std::size_t pos;
};

/**
* @brief Returns an iterator to the beginning of the RefArray.
*
* @return An iterator to the first element.
*/
iterator begin() const { return iterator(this, 0); }
/**
* @brief Returns an iterator to the end of the RefArray.
*
* @return An iterator to the element following the last element.
*/
iterator end() const { return iterator(this, N); }

private:
// We are making a high-level array, for simplicity having a C array as backing makes sense.
// NOLINTNEXTLINE(cppcoreguidelines-avoid-c-arrays)
T* storage[N];
};

/**
* @brief Deduction guide for the RefArray class.
* Allows for RefArray {a, b, c} without explicit template params.
*/
template <typename T, typename... Ts> RefArray(T&, Ts&...) -> RefArray<T, 1 + sizeof...(Ts)>;

/**
* @brief Concatenates multiple RefArray objects into a single RefArray.
*
* This function takes multiple RefArray objects as input and concatenates them into a single
* RefArray.
* @tparam T The type of elements in the RefArray.
* @tparam Ns The sizes of the input RefArrays.
* @param ref_arrays The RefArray objects to be concatenated.
* @return RefArray object containing all elements from the input arrays.
*/
template <typename T, std::size_t... Ns> RefArray<T, (Ns + ...)> concatenate(const RefArray<T, Ns>&... ref_arrays)
{
// Fold expression to calculate the total size of the new array using fold expression
constexpr std::size_t TotalSize = (Ns + ...);
std::array<T*, TotalSize> concatenated;

std::size_t offset = 0;
// Copies elements from a given RefArray to the concatenated array
auto copy_into = [&](const auto& ref_array, std::size_t& offset) {
for (std::size_t i = 0; i < ref_array.size(); ++i) {
concatenated[offset + i] = &ref_array[i];
}
offset += ref_array.size();
};

// Fold expression to copy elements from each input RefArray to the concatenated array
(..., copy_into(ref_arrays, offset));

return RefArray<T, TotalSize>{ concatenated };
}
Loading

0 comments on commit 8d6b013

Please sign in to comment.