From 2c2e5d3beed4aea53817bca2257b40d88b458cd2 Mon Sep 17 00:00:00 2001 From: ludamad Date: Mon, 12 Feb 2024 15:19:35 +0000 Subject: [PATCH] fix: review feedback --- .../ecc/fields/field_declarations.hpp | 12 +++-- .../ecc/groups/affine_element.test.cpp | 22 ++++++--- .../src/barretenberg/ecc/groups/element.hpp | 8 ++-- .../barretenberg/ecc/groups/element_impl.hpp | 47 +++++++++++++------ .../cpp/src/barretenberg/ecc/groups/wnaf.hpp | 13 +++++ 5 files changed, 72 insertions(+), 30 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp b/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp index d0aa109e965..9eb6d3f1c07 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp +++ b/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp @@ -330,12 +330,12 @@ template struct alignas(32) field { if constexpr (Params::modulus_3 >= 0x4000000000000000ULL) { split_into_endomorphism_scalars_384(k, k1, k2); } else { - std::pair, std::array> ret = - split_into_endomorphism_scalars_no_shift(k); + std::pair, std::array> ret = split_into_endomorphism_scalars(k); k1.data[0] = ret.first[0]; k1.data[1] = ret.first[1]; - // TODO(AD): We should move away from this hack by adapting split_into_endomorphism_scalars_no_shift + // TODO(https://github.com/AztecProtocol/barretenberg/issues/851): We should move away from this hack by + // returning pair of uint64_t[2] instead of a half-set field #if !defined(__clang__) && defined(__GNUC__) #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Warray-bounds" @@ -348,8 +348,10 @@ template struct alignas(32) field { } } - static std::pair, std::array> split_into_endomorphism_scalars_no_shift( - const field& k) + // NOTE: this form is only usable if the modulus is not a 256-bit integer, otherwise see + // split_into_endomorphism_scalars_384. + // TODO(https://github.com/AztecProtocol/barretenberg/issues/851): Unify these APIs. + static std::pair, std::array> split_into_endomorphism_scalars(const field& k) { static_assert(Params::modulus_3 < 0x4000000000000000ULL); field input = k.reduce_once(); diff --git a/barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.test.cpp b/barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.test.cpp index 5a2067afd69..8dc10905f5a 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.test.cpp +++ b/barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.test.cpp @@ -132,7 +132,9 @@ TEST(AffineElement, Msgpack) } namespace bb::group_elements { -// Kludge to access mul_without_endomorphism; +// mul_with_endomorphism and mul_without_endomorphism are private in affine_element. +// We could make those public to test or create other public utilities, but to keep the API intact we +// instead mark TestElementPrivate as a friend class so that our test functions can have access. class TestElementPrivate { public: template @@ -148,7 +150,8 @@ class TestElementPrivate { }; } // namespace bb::group_elements -TEST(AffineElement, EndoMulMatchesNonEndo) +// Our endomorphism-specialized multiplication should match our generic multiplication +TEST(AffineElement, MulWithEndomorphismMatchesMulWithoutEndomorphism) { for (int i = 0; i < 100; i++) { auto x1 = bb::group_elements::element(grumpkin::g1::affine_element::random_element()); @@ -159,19 +162,23 @@ TEST(AffineElement, EndoMulMatchesNonEndo) } } -TEST(AffineElement, InfinityMul) +// Multiplication of a point at infinity by a scalar should be a point at infinity +TEST(AffineElement, InfinityMulByScalarIsInfinity) { auto result = grumpkin::g1::affine_element::infinity() * grumpkin::fr::random_element(); EXPECT_TRUE(result.is_point_at_infinity()); } -TEST(AffineElement, BatchMulMatchesMul) +// Batched multiplication of points should match +TEST(AffineElement, BatchMulMatchesNonBatchMul) { - constexpr size_t num_points = 1024; + constexpr size_t num_points = 512; std::vector affine_points; - for (size_t i = 0; i < num_points; ++i) { + for (size_t i = 0; i < num_points - 1; ++i) { affine_points.emplace_back(grumpkin::g1::affine_element::random_element()); } + // Include a point at infinity to test the mixed infinity + non-infinity case + affine_points.emplace_back(grumpkin::g1::affine_element::infinity()); grumpkin::fr exponent = grumpkin::fr::random_element(); std::vector result = grumpkin::g1::element::batch_mul_with_endomorphism(affine_points, exponent); @@ -182,7 +189,8 @@ TEST(AffineElement, BatchMulMatchesMul) } } -TEST(AffineElement, InfinityBatchMul) +// Batched multiplication of a point at infinity by a scalar should result in points at infinity +TEST(AffineElement, InfinityBatchMulByScalarIsInfinity) { constexpr size_t num_points = 1024; std::vector affine_points; diff --git a/barretenberg/cpp/src/barretenberg/ecc/groups/element.hpp b/barretenberg/cpp/src/barretenberg/ecc/groups/element.hpp index 35933ec3800..f73f7ab5fe0 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/groups/element.hpp +++ b/barretenberg/cpp/src/barretenberg/ecc/groups/element.hpp @@ -95,17 +95,17 @@ template class alignas(32) element { const std::span>& second_group, const std::span>& results) noexcept; static std::vector> batch_mul_with_endomorphism( - const std::span>& points, const Fr& exponent) noexcept; + const std::span>& points, const Fr& scalar) noexcept; Fq x; Fq y; Fq z; private: - // For access to mul_without_endomorphism + // For test access to mul_without_endomorphism friend class TestElementPrivate; - element mul_without_endomorphism(const Fr& exponent) const noexcept; - element mul_with_endomorphism(const Fr& exponent) const noexcept; + element mul_without_endomorphism(const Fr& scalar) const noexcept; + element mul_with_endomorphism(const Fr& scalar) const noexcept; template > static element random_coordinates_on_curve(numeric::RNG* engine = nullptr) noexcept; diff --git a/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp b/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp index 099aee803e7..24e45600ae4 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp +++ b/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp @@ -595,9 +595,9 @@ element element::random_element(numeric::RNG* engine) noex } template -element element::mul_without_endomorphism(const Fr& exponent) const noexcept +element element::mul_without_endomorphism(const Fr& scalar) const noexcept { - const uint256_t converted_scalar(exponent); + const uint256_t converted_scalar(scalar); if (converted_scalar == 0) { return element::infinity(); @@ -617,30 +617,49 @@ element element::mul_without_endomorphism(const Fr& expone } namespace detail { +// Represents the result of using EndoScalars = std::pair, std::array>; -template struct EndomorphismWnaf { +/** + * @brief Handles the WNAF computation for scalars that are split using an endomorphism, + * achieved through `split_into_endomorphism_scalars`. It facilitates efficient computation of elliptic curve + * point multiplication by optimizing the representation of these scalars. + * + * @tparam Element The data type of elements in the elliptic curve. + * @tparam NUM_ROUNDS The number of computation rounds for WNAF. + */ +template struct EndomorphismWnaf { + // NUM_WNAF_BITS: Number of bits per window in the WNAF representation. static constexpr size_t NUM_WNAF_BITS = 4; - + // table: Stores the WNAF representation of the scalars. std::array table; + // skew and endo_skew: Indicate if our original scalar is even or odd. bool skew = false; bool endo_skew = false; + /** + * @param scalars A pair of 128-bit scalars (as two uint64_t arrays), split using an endomorphism. + */ EndomorphismWnaf(const EndoScalars& scalars) { wnaf::fixed_wnaf(&scalars.first[0], &table[0], skew, 0, 2, NUM_WNAF_BITS); wnaf::fixed_wnaf(&scalars.second[0], &table[1], endo_skew, 0, 2, NUM_WNAF_BITS); } }; + } // namespace detail template -element element::mul_with_endomorphism(const Fr& exponent) const noexcept +element element::mul_with_endomorphism(const Fr& scalar) const noexcept { + // Consider the infinity flag, return infinity if set + if (is_point_at_infinity()) { + return element::infinity(); + } constexpr size_t NUM_ROUNDS = 32; - const Fr converted_scalar = exponent.from_montgomery_form(); + const Fr converted_scalar = scalar.from_montgomery_form(); - if (converted_scalar.is_zero() || is_point_at_infinity()) { + if (converted_scalar.is_zero()) { return element::infinity(); } static constexpr size_t LOOKUP_SIZE = 8; @@ -652,7 +671,7 @@ element element::mul_with_endomorphism(const Fr& exponent) lookup_table[i] = lookup_table[i - 1] + d2; } - detail::EndoScalars endo_scalars = Fr::split_into_endomorphism_scalars_no_shift(converted_scalar); + detail::EndoScalars endo_scalars = Fr::split_into_endomorphism_scalars(converted_scalar); detail::EndomorphismWnaf wnaf{ endo_scalars }; element accumulator{ T::one_x, T::one_y, Fq::one() }; accumulator.self_set_infinity(); @@ -771,18 +790,18 @@ void element::batch_affine_add(const std::span> Vector of new points where each point is exponentâ‹…points[i] */ template std::vector> element::batch_mul_with_endomorphism( - const std::span>& points, const Fr& exponent) noexcept + const std::span>& points, const Fr& scalar) noexcept { BB_OP_COUNT_TIME(); typedef affine_element affine_element; @@ -883,7 +902,7 @@ std::vector> element::batch_mul_with_endomo /*finite_field_multiplications_per_iteration=*/6); }; // Compute wnaf for scalar - const Fr converted_scalar = exponent.from_montgomery_form(); + const Fr converted_scalar = scalar.from_montgomery_form(); // If the scalar is zero, just set results to the point at infinity if (converted_scalar.is_zero()) { @@ -953,7 +972,7 @@ std::vector> element::batch_mul_with_endomo batch_affine_add_internal(&temp_point_vector[0], &lookup_table[j][0]); } - detail::EndoScalars endo_scalars = Fr::split_into_endomorphism_scalars_no_shift(converted_scalar); + detail::EndoScalars endo_scalars = Fr::split_into_endomorphism_scalars(converted_scalar); detail::EndomorphismWnaf wnaf{ endo_scalars }; std::vector work_elements(num_points); diff --git a/barretenberg/cpp/src/barretenberg/ecc/groups/wnaf.hpp b/barretenberg/cpp/src/barretenberg/ecc/groups/wnaf.hpp index e7da2ad084a..c022809208a 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/groups/wnaf.hpp +++ b/barretenberg/cpp/src/barretenberg/ecc/groups/wnaf.hpp @@ -156,6 +156,19 @@ inline void fixed_wnaf_packed( wnaf[0] = ((slice + predicate) >> 1UL) | (point_index); } +/** + * @brief Performs fixed-window non-adjacent form (WNAF) computation for scalar multiplication. + * + * WNAF is a method for representing integers which optimizes the number of non-zero terms, which in turn optimizes + * the number of point doublings in scalar multiplication, in turn aiding efficiency. + * + * @param scalar Pointer to 128-bit scalar for which WNAF is to be computed. + * @param wnaf Pointer to num_points+1 size array where the computed WNAF will be stored. + * @param skew_map Reference to a boolean variable which will be set based on the least significant bit of the scalar. + * @param point_index The index of the point being computed in the context of multiple point multiplication. + * @param num_points The number of points being computed in parallel. + * @param wnaf_bits The number of bits to use in each window of the WNAF representation. + */ inline void fixed_wnaf(const uint64_t* scalar, uint64_t* wnaf, bool& skew_map,