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.

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

feat: flavor refactor, reduce duplication #3407

Merged
merged 59 commits into from
Nov 30, 2023
Merged
Show file tree
Hide file tree
Changes from 49 commits
Commits
Show all changes
59 commits
Select commit Hold shift + click to select a range
040a2b2
towards more succinct flavor classes
ludamad0 Nov 21, 2023
40e2a38
progress
ludamad0 Nov 22, 2023
aaabc58
Merge branch 'master' of github.com:AztecProtocol/aztec-packages into…
ludamad0 Nov 22, 2023
a64c2de
working ref array
ludamad0 Nov 22, 2023
220999e
Message pointer views
ludamad0 Nov 22, 2023
e956e96
Merge github.com:AztecProtocol/aztec-packages into feat/flavor-refactor
ludamad0 Nov 22, 2023
750d5a8
honk building
ludamad0 Nov 22, 2023
82e0b57
it builds
ludamad0 Nov 22, 2023
ebf7527
Merge
ludamad0 Nov 22, 2023
12ad654
Remove unused variables
ludamad0 Nov 22, 2023
c83cf01
initializer
ludamad0 Nov 23, 2023
11cfe94
fix eccvm base to_be_shifted
ludamad0 Nov 23, 2023
dd67fed
towards fixed goblin tests
ludamad0 Nov 24, 2023
8884e24
Affine element simplif
ludamad0 Nov 24, 2023
24f1dc3
debug print
ludamad0 Nov 24, 2023
d398e2c
stash debug_helpers.py
ludamad0 Nov 25, 2023
907e437
update with field detection
ludamad0 Nov 25, 2023
7621f6f
debug helpers
ludamad0 Nov 25, 2023
273d17f
lldb helpers
ludamad0 Nov 26, 2023
bb923d9
Initialize all members
ludamad0 Nov 27, 2023
f56bec6
Merge remote-tracking branch 'origin/master' into feat/flavor-refactor
ludamad0 Nov 27, 2023
9bb3354
feat: fix to be shifted and unshifted
ludamad0 Nov 27, 2023
560936e
Remove print() that was added. Fix to_be_shifted in goblin translator.
ludamad0 Nov 28, 2023
3d10141
Deduplication
ludamad0 Nov 28, 2023
3dd704c
Update
ludamad0 Nov 28, 2023
b716eef
Corrections
ludamad0 Nov 28, 2023
a84cd10
Merge branch 'master' into feat/flavor-refactor
ludamad Nov 28, 2023
821dfe9
Temp push
ludamad0 Nov 28, 2023
73af46a
Revert until working.
ludamad0 Nov 28, 2023
407fdb7
Merge remote-tracking branch 'origin/feat/flavor-refactor' into feat/…
ludamad0 Nov 28, 2023
490cb77
Revert "Revert until working."
ludamad0 Nov 28, 2023
9d8be5a
Comment
ludamad0 Nov 28, 2023
b3f1d7d
Revert "Revert "Revert until working.""
ludamad0 Nov 28, 2023
da73120
Revert "Revert "Revert "Revert until working."""
ludamad0 Nov 28, 2023
7a4ced3
test fixes
ludamad0 Nov 28, 2023
87714ee
reinstate tests
ludamad0 Nov 28, 2023
4fb3e68
Remove files
ludamad0 Nov 28, 2023
f4ee388
Reverts
ludamad0 Nov 28, 2023
6134f35
Reverts
ludamad0 Nov 28, 2023
cfd5d27
Flavor comment
ludamad0 Nov 28, 2023
0cc0385
Merge branch 'master' into feat/flavor-refactor
ludamad Nov 28, 2023
0d83433
ix: react to generated flavors
ludamad0 Nov 28, 2023
0d19f90
fix
ludamad0 Nov 29, 2023
f4dae28
Merge branch 'master' into feat/flavor-refactor
ludamad Nov 29, 2023
e2cf3b5
revert commenting
ludamad Nov 29, 2023
b939729
Update relation_correctness.test.cpp
ludamad Nov 29, 2023
5ff03e7
Clang formatting
ludamad0 Nov 29, 2023
a4abe50
notes on TODO's to namespace utilities
ludamad0 Nov 29, 2023
638c020
typo
ludamad0 Nov 29, 2023
7f0e26c
Merge branch 'master' of github.com:AztecProtocol/aztec-packages into…
ludamad0 Nov 29, 2023
ad27d83
Update affine_element.hpp
ludamad Nov 29, 2023
8dbeb8c
fix undefined behaviour
ludamad0 Nov 29, 2023
bbfdc84
review feedback
ludamad0 Nov 29, 2023
bb25da5
Merge remote-tracking branch 'origin/feat/flavor-refactor' into feat/…
ludamad0 Nov 29, 2023
7f0006d
review feedback for concatenate
ludamad0 Nov 29, 2023
699fcc7
fix build and semantics
ludamad0 Nov 29, 2023
2e1bac2
Update ref_vector.hpp
ludamad Nov 30, 2023
28be820
Format
ludamad0 Nov 30, 2023
6289427
Revert "fix undefined behaviour"
ludamad0 Nov 30, 2023
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
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{};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

GCC rightfully caught this is no longer initialized. Plan to do a pass on this with AztecProtocol/barretenberg#793

// 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,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We pass vector by value unnecessarily, causing full new vector allocations

Copy link
Contributor

Choose a reason for hiding this comment

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

Eek, thanks for catching and fixing.

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 = {})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The new shape here is vector<RefVector<>>. This required some adapting in the tests

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));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to_vector_of_ref_vectors takes vector of vector to vector of RefVector


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.
*
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not currently used, but originally I was using it and maybe it will come in handy? I thought that having compile time sized arrays was not worth the type complexity vs extra std::vector allocations/dynamicness, but in tight loops it certainly could be

* 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