From 87b13404229296a9f5b1cf813d91adc4223526e2 Mon Sep 17 00:00:00 2001 From: ludamad Date: Tue, 6 Feb 2024 22:32:05 +0000 Subject: [PATCH 01/18] feat: op count timers --- .vscode/settings.json | 3 +- barretenberg/cpp/CMakePresets.json | 27 +++++-- .../cpp/src/barretenberg/common/op_count.cpp | 65 ++++++++++++++-- .../cpp/src/barretenberg/common/op_count.hpp | 75 +++++++++++++++++-- .../barretenberg/ecc/groups/element_impl.hpp | 1 + 5 files changed, 149 insertions(+), 22 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index bb29728a078e..b7aa953af512 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -155,6 +155,5 @@ "C_Cpp.vcpkg.enabled": false, "C_Cpp.default.includePath": [ "barretenberg/cpp/src" - ], - "cmake.sourceDirectory": "/mnt/user-data/adam/aztec-packages/barretenberg/cpp", + ] } diff --git a/barretenberg/cpp/CMakePresets.json b/barretenberg/cpp/CMakePresets.json index d967017743f8..c6c2a0e2eb87 100644 --- a/barretenberg/cpp/CMakePresets.json +++ b/barretenberg/cpp/CMakePresets.json @@ -167,13 +167,23 @@ } }, { - "name": "op-counting", - "displayName": "Release build with operation counts for benchmarks", + "name": "op-count-track", + "displayName": "Release build with operation counts", "description": "Build with op counting", "inherits": "clang16", - "binaryDir": "build-op-counting", + "binaryDir": "build-op-count-track", "environment": { - "CXXFLAGS": "-DBB_USE_OP_COUNT" + "CXXFLAGS": "-DBB_USE_OP_COUNT -DBB_USE_OP_COUNT_TRACK_ONLY" + } + }, + { + "name": "op-count-time", + "displayName": "Release build with time and clock counts", + "description": "Build with op counting", + "inherits": "clang16", + "binaryDir": "build-op-count-time", + "environment": { + "CXXFLAGS": "-DBB_USE_OP_COUNT -DBB_USE_OP_COUNT_TIME_ONLY" } }, { @@ -311,9 +321,14 @@ "configurePreset": "clang16" }, { - "name": "op-counting", + "name": "op-count-time", + "inherits": "default", + "configurePreset": "op-count-time" + }, + { + "name": "op-count-track", "inherits": "default", - "configurePreset": "op-counting" + "configurePreset": "op-count-track" }, { "name": "clang16-dbg", diff --git a/barretenberg/cpp/src/barretenberg/common/op_count.cpp b/barretenberg/cpp/src/barretenberg/common/op_count.cpp index c646c88d829a..d95d46a7788c 100644 --- a/barretenberg/cpp/src/barretenberg/common/op_count.cpp +++ b/barretenberg/cpp/src/barretenberg/common/op_count.cpp @@ -7,7 +7,15 @@ #include namespace bb::detail { -void GlobalOpCountContainer::add_entry(const char* key, std::size_t* count) + +GlobalOpCountContainer::~GlobalOpCountContainer() +{ + // This is useful for printing counts at the end of non-benchmarks. + // See op_count_google_bench.hpp for benchmarks. + // print(); +} + +void GlobalOpCountContainer::add_entry(const char* key, const std::shared_ptr& count) { std::unique_lock lock(mutex); std::stringstream ss; @@ -19,8 +27,16 @@ void GlobalOpCountContainer::print() const { std::cout << "print_op_counts() START" << std::endl; for (const Entry& entry : counts) { - if (*entry.count > 0) { - std::cout << entry.key << "\t" << *entry.count << "\t[thread=" << entry.thread_id << "]" << std::endl; + if (entry.count->count > 0) { + std::cout << entry.key << "\t" << entry.count->count << "\t[thread=" << entry.thread_id << "]" << std::endl; + } + if (entry.count->time > 0) { + std::cout << entry.key << "(t)\t" << static_cast(entry.count->time) / 1000000.0 + << "ms\t[thread=" << entry.thread_id << "]" << std::endl; + } + if (entry.count->cycles > 0) { + std::cout << entry.key << "(c)\t" << entry.count->cycles << "\t[thread=" << entry.thread_id << "]" + << std::endl; } } std::cout << "print_op_counts() END" << std::endl; @@ -30,8 +46,14 @@ std::map GlobalOpCountContainer::get_aggregate_counts( { std::map aggregate_counts; for (const Entry& entry : counts) { - if (*entry.count > 0) { - aggregate_counts[entry.key] += *entry.count; + if (entry.count->count > 0) { + aggregate_counts[entry.key] += entry.count->count; + } + if (entry.count->time > 0) { + aggregate_counts[entry.key + "(t)"] += entry.count->time; + } + if (entry.count->cycles > 0) { + aggregate_counts[entry.key + "(c)"] += entry.count->cycles; } } return aggregate_counts; @@ -41,11 +63,42 @@ void GlobalOpCountContainer::clear() { std::unique_lock lock(mutex); for (Entry& entry : counts) { - *entry.count = 0; + *entry.count = OpStats(); } } // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) GlobalOpCountContainer GLOBAL_OP_COUNTS; + +OpCountCycleReporter::OpCountCycleReporter(OpStats* stats) + : stats(stats) +{ +#if __clang__ && (defined(__x86_64__) || defined(_M_X64) || defined(__i386) || defined(_M_IX86)) + // Don't support any other targets but x86 clang for now, this is a bit lazy but more than fits our needs + cycles = __builtin_ia32_rdtsc(); +#endif +} +OpCountCycleReporter::~OpCountCycleReporter() +{ +#if __clang__ && (defined(__x86_64__) || defined(_M_X64) || defined(__i386) || defined(_M_IX86)) + // Don't support any other targets but x86 clang for now, this is a bit lazy but more than fits our needs + stats->count += 1; + stats->cycles += __builtin_ia32_rdtsc() - cycles; +#endif +} +OpCountTimeReporter::OpCountTimeReporter(OpStats* stats) + : stats(stats) +{ + auto now = std::chrono::high_resolution_clock::now(); + auto now_ns = std::chrono::time_point_cast(now); + time = static_cast(now_ns.time_since_epoch().count()); +} +OpCountTimeReporter::~OpCountTimeReporter() +{ + auto now = std::chrono::high_resolution_clock::now(); + auto now_ns = std::chrono::time_point_cast(now); + stats->count += 1; + stats->time += static_cast(now_ns.time_since_epoch().count()) - time; +} } // namespace bb::detail #endif diff --git a/barretenberg/cpp/src/barretenberg/common/op_count.hpp b/barretenberg/cpp/src/barretenberg/common/op_count.hpp index 57600138fe20..c04a26575aab 100644 --- a/barretenberg/cpp/src/barretenberg/common/op_count.hpp +++ b/barretenberg/cpp/src/barretenberg/common/op_count.hpp @@ -1,12 +1,15 @@ #pragma once +#include #ifndef BB_USE_OP_COUNT // require a semicolon to appease formatters // NOLINTNEXTLINE(cppcoreguidelines-macro-usage) #define BB_OP_COUNT_TRACK() (void)0 // NOLINTNEXTLINE(cppcoreguidelines-macro-usage) #define BB_OP_COUNT_TRACK_NAME(name) (void)0 +#define BB_OP_COUNT_CYCLES() (void)0 +#define BB_OP_COUNT_TIME() (void)0 #else /** * Provides an abstraction that counts operations based on function names. @@ -37,20 +40,27 @@ template struct OperationLabel { char value[N]; }; +struct OpStats { + std::size_t count = 0; + std::size_t time = 0; + std::size_t cycles = 0; +}; + // Contains all statically known op counts struct GlobalOpCountContainer { public: struct Entry { std::string key; std::string thread_id; - std::size_t* count; + std::shared_ptr count; }; + ~GlobalOpCountContainer(); std::mutex mutex; std::vector counts; void print() const; // NOTE: Should be called when other threads aren't active void clear(); - void add_entry(const char* key, std::size_t* count); + void add_entry(const char* key, const std::shared_ptr& count); std::map get_aggregate_counts() const; }; @@ -60,28 +70,77 @@ extern GlobalOpCountContainer GLOBAL_OP_COUNTS; template struct GlobalOpCount { public: // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) - static thread_local std::size_t* thread_local_count; + static thread_local std::shared_ptr stats; + static OpStats* ensure_stats() + { + if (BB_UNLIKELY(stats == nullptr)) { + stats = std::make_shared(); + GLOBAL_OP_COUNTS.add_entry(Op.value, stats); + } + return stats.get(); + } static constexpr void increment_op_count() { +#ifndef BB_USE_OP_COUNT_TIME_ONLY + if (std::is_constant_evaluated()) { + // We do nothing if the compiler tries to run this + return; + } + ensure_stats(); + stats->count++; +#endif + } + static constexpr void add_cycle_time(std::size_t cycles) + { +#ifndef BB_USE_OP_COUNT_TRACK_ONLY if (std::is_constant_evaluated()) { // We do nothing if the compiler tries to run this return; } - if (BB_UNLIKELY(thread_local_count == nullptr)) { - thread_local_count = new std::size_t(); - GLOBAL_OP_COUNTS.add_entry(Op.value, thread_local_count); + ensure_stats(); + stats->cycles += cycles; +#endif + } + static constexpr void add_clock_time(std::size_t time) + { +#ifndef BB_USE_OP_COUNT_TRACK_ONLY + if (std::is_constant_evaluated()) { + // We do nothing if the compiler tries to run this + return; } - (*thread_local_count)++; + ensure_stats(); + stats->time += time; +#endif } }; // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) -template thread_local std::size_t* GlobalOpCount::thread_local_count; +template thread_local std::shared_ptr GlobalOpCount::stats; +// NOLINTNEXTLINE(cppcoreguidelines-special-member-functions) +struct OpCountCycleReporter { + OpStats* stats; + std::size_t cycles; + OpCountCycleReporter(OpStats* stats); + ~OpCountCycleReporter(); +}; +// NOLINTNEXTLINE(cppcoreguidelines-special-member-functions) +struct OpCountTimeReporter { + OpStats* stats; + std::size_t time; + OpCountTimeReporter(OpStats* stats); + ~OpCountTimeReporter(); +}; } // namespace bb::detail // NOLINTNEXTLINE(cppcoreguidelines-macro-usage) #define BB_OP_COUNT_TRACK() bb::detail::GlobalOpCount<__func__>::increment_op_count() // NOLINTNEXTLINE(cppcoreguidelines-macro-usage) #define BB_OP_COUNT_TRACK_NAME(name) bb::detail::GlobalOpCount::increment_op_count() +// NOLINTNEXTLINE(cppcoreguidelines-macro-usage) +#define BB_OP_COUNT_CYCLES() \ + bb::detail::OpCountCycleReporter __bb_op_count_cyles(bb::detail::GlobalOpCount<__func__>::ensure_stats()) +// NOLINTNEXTLINE(cppcoreguidelines-macro-usage) +#define BB_OP_COUNT_TIME() \ + bb::detail::OpCountTimeReporter __bb_op_count_time(bb::detail::GlobalOpCount<__func__>::ensure_stats()) #endif \ No newline at end of file diff --git a/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp b/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp index ec9eba4ffac7..e97884a66bea 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp +++ b/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp @@ -788,6 +788,7 @@ template std::vector> element::batch_mul_with_endomorphism( const std::span>& points, const Fr& exponent) noexcept { + BB_OP_COUNT_TIME(); typedef affine_element affine_element; const size_t num_points = points.size(); From 20a1a1a1ed4726227f2c0aeef3756aad7e6b1162 Mon Sep 17 00:00:00 2001 From: ludamad Date: Thu, 8 Feb 2024 21:57:44 +0000 Subject: [PATCH 02/18] fixes --- .../ecc/groups/affine_element.test.cpp | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) 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 4dd06af4c38f..89386b820e53 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.test.cpp +++ b/barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.test.cpp @@ -129,3 +129,27 @@ TEST(AffineElement, Msgpack) auto [actual, expected] = msgpack_roundtrip(secp256k1::g1::affine_element{ 1, 1 }); EXPECT_EQ(actual, expected); } + +TEST(AffineElement, InfinityMul) +{ + auto result = grumpkin::g1::affine_element::infinity() * grumpkin::fr::random_element(); + EXPECT_TRUE(result.is_point_at_infinity()); +} + +TEST(AffineElement, InfinityBatchMul) +{ + constexpr size_t num_points = 1024; + std::vector affine_points; + for (size_t i = 0; i < num_points; ++i) { + 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); + for (grumpkin::g1::affine_element& el : result) { + EXPECT_TRUE(el.is_point_at_infinity()); + if (!el.is_point_at_infinity()) { + break; // dont spam with errors + } + } +} \ No newline at end of file From 834aa955011e54f00a22630dc5dda2c1c4a13cca Mon Sep 17 00:00:00 2001 From: ludamad Date: Fri, 9 Feb 2024 22:48:13 +0000 Subject: [PATCH 03/18] checkpoint --- .vscode/settings.json | 3 +- barretenberg/cpp/scripts/benchmark_remote.sh | 11 +-- .../cpp/src/barretenberg/common/op_count.cpp | 2 +- .../ecc/fields/field_declarations.hpp | 26 ++++- .../ecc/groups/affine_element.test.cpp | 28 ++++++ .../src/barretenberg/ecc/groups/element.hpp | 2 + .../barretenberg/ecc/groups/element_impl.hpp | 99 +++++++++---------- 7 files changed, 107 insertions(+), 64 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index b7aa953af512..a3fa432542e9 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -155,5 +155,6 @@ "C_Cpp.vcpkg.enabled": false, "C_Cpp.default.includePath": [ "barretenberg/cpp/src" - ] + ], + "cmake.sourceDirectory": "/mnt/user-data/adam/aztec-packages/barretenberg/cpp" } diff --git a/barretenberg/cpp/scripts/benchmark_remote.sh b/barretenberg/cpp/scripts/benchmark_remote.sh index e1d5907eab2d..05a61392584b 100755 --- a/barretenberg/cpp/scripts/benchmark_remote.sh +++ b/barretenberg/cpp/scripts/benchmark_remote.sh @@ -9,20 +9,19 @@ set -eu BENCHMARK=${1:-goblin_bench} COMMAND=${2:-./$BENCHMARK} +PRESET=${3:-clang16} +BUILD_DIR=${4:-build} # Move above script dir. cd $(dirname $0)/.. -# Create lock file -ssh $BB_SSH_KEY $BB_SSH_INSTANCE "touch ~/BENCHMARKING_IN_PROGRESS" - # Configure and build. -cmake --preset clang16 -cmake --build --preset clang16 --target $BENCHMARK +cmake --preset $PRESET +cmake --build --preset $PRESET --target $BENCHMARK source scripts/_benchmark_remote_lock.sh -cd build +cd $BUILD_DIR scp $BB_SSH_KEY ./bin/$BENCHMARK $BB_SSH_INSTANCE:$BB_SSH_CPP_PATH/build ssh $BB_SSH_KEY $BB_SSH_INSTANCE \ "cd $BB_SSH_CPP_PATH/build ; $COMMAND" diff --git a/barretenberg/cpp/src/barretenberg/common/op_count.cpp b/barretenberg/cpp/src/barretenberg/common/op_count.cpp index d95d46a7788c..cbd77c8595d3 100644 --- a/barretenberg/cpp/src/barretenberg/common/op_count.cpp +++ b/barretenberg/cpp/src/barretenberg/common/op_count.cpp @@ -12,7 +12,7 @@ GlobalOpCountContainer::~GlobalOpCountContainer() { // This is useful for printing counts at the end of non-benchmarks. // See op_count_google_bench.hpp for benchmarks. - // print(); + print(); } void GlobalOpCountContainer::add_entry(const char* key, const std::shared_ptr& count) diff --git a/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp b/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp index af92a45ae168..cb75353c09f2 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp +++ b/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp @@ -1,6 +1,7 @@ #pragma once #include "barretenberg/common/assert.hpp" #include "barretenberg/common/compiler_hints.hpp" +#include "barretenberg/common/op_count.hpp" #include "barretenberg/numeric/random/engine.hpp" #include "barretenberg/numeric/uint128/uint128.hpp" #include "barretenberg/numeric/uint256/uint256.hpp" @@ -326,11 +327,25 @@ template struct alignas(32) field { **/ static void split_into_endomorphism_scalars(const field& k, field& k1, field& k2) { + BB_OP_COUNT_TIME(); // if the modulus is a 256-bit integer, we need to use a basis where g1, g2 have been shifted by 2^384 if constexpr (Params::modulus_3 >= 0x4000000000000000ULL) { split_into_endomorphism_scalars_384(k, k1, k2); - return; + } else { + std::pair, std::array> ret = + split_into_endomorphism_scalars_no_shift(k); + k1.data[0] = ret.first[0]; + k1.data[1] = ret.first[1]; + k2.data[0] = ret.second[0]; + k2.data[1] = ret.second[0]; } + } + + static std::pair, std::array> split_into_endomorphism_scalars_no_shift( + const field& k) + { + BB_OP_COUNT_TIME(); + static_assert(Params::modulus_3 < 0x4000000000000000ULL); field input = k.reduce_once(); constexpr field endo_g1 = { Params::endo_g1_lo, Params::endo_g1_mid, Params::endo_g1_hi, 0 }; @@ -369,14 +384,15 @@ template struct alignas(32) field { field t1 = (q2_lo - q1_lo).reduce_once(); field beta = cube_root_of_unity(); field t2 = (t1 * beta + input).reduce_once(); - k2.data[0] = t1.data[0]; - k2.data[1] = t1.data[1]; - k1.data[0] = t2.data[0]; - k1.data[1] = t2.data[1]; + return { + { t2.data[0], t2.data[1] }, + { t1.data[0], t1.data[1] }, + }; } static void split_into_endomorphism_scalars_384(const field& input, field& k1_out, field& k2_out) { + BB_OP_COUNT_TIME(); constexpr field minus_b1f{ Params::endo_minus_b1_lo, 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 89386b820e53..6b99b6d93091 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.test.cpp +++ b/barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.test.cpp @@ -4,6 +4,7 @@ #include "barretenberg/ecc/curves/grumpkin/grumpkin.hpp" #include "barretenberg/ecc/curves/secp256k1/secp256k1.hpp" #include "barretenberg/ecc/curves/secp256r1/secp256r1.hpp" +#include "barretenberg/ecc/groups/element.hpp" #include "barretenberg/serialize/test_helper.hpp" #include @@ -130,6 +131,33 @@ TEST(AffineElement, Msgpack) EXPECT_EQ(actual, expected); } +namespace bb::group_elements { +// Kludge to access mul_without_endomorphism; +class TestElementPrivate { + public: + template + static Element mul_without_endomorphism(const Element& element, const Scalar& scalar) + { + return element.mul_without_endomorphism(scalar); + } + template + static Element mul_with_endomorphism(const Element& element, const Scalar& scalar) + { + return element.mul_with_endomorphism(scalar); + } +}; +} // namespace bb::group_elements + +TEST(AffineElement, EndoMulMatchesNonEndo) +{ + for (int i = 0; i < 100; i++) { + auto x1 = bb::group_elements::element(grumpkin::g1::affine_element::random_element()); + auto r1 = bb::group_elements::TestElementPrivate::mul_without_endomorphism(x1, grumpkin::fr::random_element()); + auto r2 = bb::group_elements::TestElementPrivate::mul_with_endomorphism(x1, grumpkin::fr::random_element()); + EXPECT_EQ(r1, r2); + } +} + TEST(AffineElement, InfinityMul) { auto result = grumpkin::g1::affine_element::infinity() * grumpkin::fr::random_element(); diff --git a/barretenberg/cpp/src/barretenberg/ecc/groups/element.hpp b/barretenberg/cpp/src/barretenberg/ecc/groups/element.hpp index 0c34effbfaa0..35933ec38009 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/groups/element.hpp +++ b/barretenberg/cpp/src/barretenberg/ecc/groups/element.hpp @@ -102,6 +102,8 @@ template class alignas(32) element { Fq z; private: + // For 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; diff --git a/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp b/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp index e97884a66bea..d4739ed59857 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp +++ b/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp @@ -3,6 +3,7 @@ #include "barretenberg/common/thread.hpp" #include "barretenberg/ecc/groups/element.hpp" #include "element.hpp" +#include // NOLINTBEGIN(readability-implicit-bool-conversion, cppcoreguidelines-avoid-c-arrays) namespace bb::group_elements { @@ -599,96 +600,92 @@ element element::mul_without_endomorphism(const Fr& expone const uint256_t converted_scalar(exponent); if (converted_scalar == 0) { - element result{ Fq::zero(), Fq::zero(), Fq::zero() }; - result.self_set_infinity(); - return result; + return element::infinity(); } - element work_element(*this); + element accumulator(*this); const uint64_t maximum_set_bit = converted_scalar.get_msb(); // This is simpler and doublings of infinity should be fast. We should think if we want to defend against the // timing leak here (if used with ECDSA it can sometimes lead to private key compromise) for (uint64_t i = maximum_set_bit - 1; i < maximum_set_bit; --i) { - work_element.self_dbl(); + accumulator.self_dbl(); if (converted_scalar.get_bit(i)) { - work_element += *this; + accumulator += *this; } } - return work_element; + return accumulator; } +namespace detail { +using EndoScalars = std::pair, std::array>; +template struct EndomorphismWnaf { + + static constexpr size_t NUM_WNAF_BITS = 4; + + std::array table; + bool skew = false; + bool endo_skew = false; + + 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 { + BB_OP_COUNT_TIME(); + constexpr size_t NUM_ROUNDS = 32; const Fr converted_scalar = exponent.from_montgomery_form(); - if (converted_scalar.is_zero()) { - element result{ Fq::zero(), Fq::zero(), Fq::zero() }; - result.self_set_infinity(); - return result; + if (converted_scalar.is_zero() || is_point_at_infinity()) { + return element::infinity(); } + static constexpr size_t LOOKUP_SIZE = 8; + std::array lookup_table; - constexpr size_t lookup_size = 8; - constexpr size_t num_rounds = 32; - constexpr size_t num_wnaf_bits = 4; - std::array lookup_table; - - element d2 = element(*this); - d2.self_dbl(); + element d2 = dbl(); lookup_table[0] = element(*this); - for (size_t i = 1; i < lookup_size; ++i) { + for (size_t i = 1; i < LOOKUP_SIZE; ++i) { lookup_table[i] = lookup_table[i - 1] + d2; } - uint64_t wnaf_table[num_rounds * 2]; - Fr endo_scalar; - Fr::split_into_endomorphism_scalars(converted_scalar, endo_scalar, *(Fr*)&endo_scalar.data[2]); // NOLINT - - bool skew = false; - bool endo_skew = false; - - wnaf::fixed_wnaf(&endo_scalar.data[0], &wnaf_table[0], skew, 0, 2, num_wnaf_bits); - wnaf::fixed_wnaf(&endo_scalar.data[2], &wnaf_table[1], endo_skew, 0, 2, num_wnaf_bits); - - element work_element{ T::one_x, T::one_y, Fq::one() }; - work_element.self_set_infinity(); - - uint64_t wnaf_entry = 0; - uint64_t index = 0; - bool sign = false; + detail::EndoScalars endo_scalars = Fr::split_into_endomorphism_scalars_no_shift(converted_scalar); + detail::EndomorphismWnaf wnaf{ endo_scalars }; + element accumulator{ T::one_x, T::one_y, Fq::one() }; + accumulator.self_set_infinity(); Fq beta = Fq::cube_root_of_unity(); - for (size_t i = 0; i < num_rounds * 2; ++i) { - wnaf_entry = wnaf_table[i]; - index = wnaf_entry & 0x0fffffffU; - sign = static_cast((wnaf_entry >> 31) & 1); + for (size_t i = 0; i < NUM_ROUNDS * 2; ++i) { + uint64_t wnaf_entry = wnaf.table[i]; + uint64_t index = wnaf_entry & 0x0fffffffU; + bool sign = static_cast((wnaf_entry >> 31) & 1); const bool is_odd = ((i & 1) == 1); auto to_add = lookup_table[static_cast(index)]; to_add.y.self_conditional_negate(sign ^ is_odd); if (is_odd) { to_add.x *= beta; } - work_element += to_add; + accumulator += to_add; - if (i != ((2 * num_rounds) - 1) && is_odd) { + if (i != ((2 * NUM_ROUNDS) - 1) && is_odd) { for (size_t j = 0; j < 4; ++j) { - work_element.self_dbl(); + accumulator.self_dbl(); } } } - auto temporary = -lookup_table[0]; - if (skew) { - work_element += temporary; + if (wnaf.skew) { + accumulator += -lookup_table[0]; } - - temporary = { lookup_table[0].x * beta, lookup_table[0].y, lookup_table[0].z }; - - if (endo_skew) { - work_element += temporary; + if (wnaf.endo_skew) { + accumulator += element{ lookup_table[0].x * beta, lookup_table[0].y, lookup_table[0].z }; } - return work_element; + return accumulator; } /** From c9c15306a415bb2732984eb76c211fa67377b4a2 Mon Sep 17 00:00:00 2001 From: ludamad Date: Sat, 10 Feb 2024 00:16:05 +0000 Subject: [PATCH 04/18] fix: batch mul --- .../cpp/src/barretenberg/ecc/groups/affine_element.test.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 6b99b6d93091..5a39a042e479 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.test.cpp +++ b/barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.test.cpp @@ -152,8 +152,9 @@ TEST(AffineElement, EndoMulMatchesNonEndo) { for (int i = 0; i < 100; i++) { auto x1 = bb::group_elements::element(grumpkin::g1::affine_element::random_element()); - auto r1 = bb::group_elements::TestElementPrivate::mul_without_endomorphism(x1, grumpkin::fr::random_element()); - auto r2 = bb::group_elements::TestElementPrivate::mul_with_endomorphism(x1, grumpkin::fr::random_element()); + auto f1 = grumpkin::fr::random_element(); + auto r1 = bb::group_elements::TestElementPrivate::mul_without_endomorphism(x1, f1); + auto r2 = bb::group_elements::TestElementPrivate::mul_with_endomorphism(x1, f1); EXPECT_EQ(r1, r2); } } From ee4ed46086b6ace8a93a0d3350978efe8322ecc2 Mon Sep 17 00:00:00 2001 From: ludamad Date: Sat, 10 Feb 2024 00:23:23 +0000 Subject: [PATCH 05/18] fix: mul_with_endo batched --- .../src/barretenberg/ecc/groups/element_impl.hpp | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp b/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp index d4739ed59857..78872bde30b3 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp +++ b/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp @@ -1065,7 +1065,6 @@ std::vector> element::batch_mul_with_endomo num_points, [beta, &lookup_table, &temp_point_vector](size_t start, size_t end) { for (size_t i = start; i < end; ++i) { - temp_point_vector[i] = lookup_table[0][i]; temp_point_vector[i].x *= beta; } @@ -1079,6 +1078,21 @@ std::vector> element::batch_mul_with_endomo /*sequential_copy_ops_per_iteration=*/1); batch_affine_add_internal(&temp_point_vector[0], &work_elements[0]); } + // handle points at infinity explicitly + run_loop_in_parallel_if_effective( + num_points, + [&](size_t start, size_t end) { + for (size_t i = start; i < end; ++i) { + work_elements[i] = points[i].is_infinity() ? work_elements[i].set_infinity() : work_elements[i]; + } + }, + /*finite_field_additions_per_iteration=*/0, + /*finite_field_multiplications_per_iteration=*/1, + /*finite_field_inversions_per_iteration=*/0, + /*group_element_additions_per_iteration=*/0, + /*group_element_doublings_per_iteration=*/0, + /*scalar_multiplications_per_iteration=*/0, + /*sequential_copy_ops_per_iteration=*/1); return work_elements; } From 1718e1342fa73855040e9fccdae0741d95f3ca61 Mon Sep 17 00:00:00 2001 From: ludamad Date: Fri, 9 Feb 2024 19:24:03 -0500 Subject: [PATCH 06/18] Update settings.json --- .vscode/settings.json | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index a3fa432542e9..b7aa953af512 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -155,6 +155,5 @@ "C_Cpp.vcpkg.enabled": false, "C_Cpp.default.includePath": [ "barretenberg/cpp/src" - ], - "cmake.sourceDirectory": "/mnt/user-data/adam/aztec-packages/barretenberg/cpp" + ] } From dd132fcf249eceefbfe694dbabf8fcfa77c75d0b Mon Sep 17 00:00:00 2001 From: ludamad Date: Sat, 10 Feb 2024 00:25:54 +0000 Subject: [PATCH 07/18] revert: timers --- barretenberg/cpp/CMakePresets.json | 27 ++----- .../cpp/src/barretenberg/common/op_count.cpp | 65 ++-------------- .../cpp/src/barretenberg/common/op_count.hpp | 75 ++----------------- 3 files changed, 20 insertions(+), 147 deletions(-) diff --git a/barretenberg/cpp/CMakePresets.json b/barretenberg/cpp/CMakePresets.json index c6c2a0e2eb87..d967017743f8 100644 --- a/barretenberg/cpp/CMakePresets.json +++ b/barretenberg/cpp/CMakePresets.json @@ -167,23 +167,13 @@ } }, { - "name": "op-count-track", - "displayName": "Release build with operation counts", + "name": "op-counting", + "displayName": "Release build with operation counts for benchmarks", "description": "Build with op counting", "inherits": "clang16", - "binaryDir": "build-op-count-track", + "binaryDir": "build-op-counting", "environment": { - "CXXFLAGS": "-DBB_USE_OP_COUNT -DBB_USE_OP_COUNT_TRACK_ONLY" - } - }, - { - "name": "op-count-time", - "displayName": "Release build with time and clock counts", - "description": "Build with op counting", - "inherits": "clang16", - "binaryDir": "build-op-count-time", - "environment": { - "CXXFLAGS": "-DBB_USE_OP_COUNT -DBB_USE_OP_COUNT_TIME_ONLY" + "CXXFLAGS": "-DBB_USE_OP_COUNT" } }, { @@ -321,14 +311,9 @@ "configurePreset": "clang16" }, { - "name": "op-count-time", - "inherits": "default", - "configurePreset": "op-count-time" - }, - { - "name": "op-count-track", + "name": "op-counting", "inherits": "default", - "configurePreset": "op-count-track" + "configurePreset": "op-counting" }, { "name": "clang16-dbg", diff --git a/barretenberg/cpp/src/barretenberg/common/op_count.cpp b/barretenberg/cpp/src/barretenberg/common/op_count.cpp index cbd77c8595d3..c646c88d829a 100644 --- a/barretenberg/cpp/src/barretenberg/common/op_count.cpp +++ b/barretenberg/cpp/src/barretenberg/common/op_count.cpp @@ -7,15 +7,7 @@ #include namespace bb::detail { - -GlobalOpCountContainer::~GlobalOpCountContainer() -{ - // This is useful for printing counts at the end of non-benchmarks. - // See op_count_google_bench.hpp for benchmarks. - print(); -} - -void GlobalOpCountContainer::add_entry(const char* key, const std::shared_ptr& count) +void GlobalOpCountContainer::add_entry(const char* key, std::size_t* count) { std::unique_lock lock(mutex); std::stringstream ss; @@ -27,16 +19,8 @@ void GlobalOpCountContainer::print() const { std::cout << "print_op_counts() START" << std::endl; for (const Entry& entry : counts) { - if (entry.count->count > 0) { - std::cout << entry.key << "\t" << entry.count->count << "\t[thread=" << entry.thread_id << "]" << std::endl; - } - if (entry.count->time > 0) { - std::cout << entry.key << "(t)\t" << static_cast(entry.count->time) / 1000000.0 - << "ms\t[thread=" << entry.thread_id << "]" << std::endl; - } - if (entry.count->cycles > 0) { - std::cout << entry.key << "(c)\t" << entry.count->cycles << "\t[thread=" << entry.thread_id << "]" - << std::endl; + if (*entry.count > 0) { + std::cout << entry.key << "\t" << *entry.count << "\t[thread=" << entry.thread_id << "]" << std::endl; } } std::cout << "print_op_counts() END" << std::endl; @@ -46,14 +30,8 @@ std::map GlobalOpCountContainer::get_aggregate_counts( { std::map aggregate_counts; for (const Entry& entry : counts) { - if (entry.count->count > 0) { - aggregate_counts[entry.key] += entry.count->count; - } - if (entry.count->time > 0) { - aggregate_counts[entry.key + "(t)"] += entry.count->time; - } - if (entry.count->cycles > 0) { - aggregate_counts[entry.key + "(c)"] += entry.count->cycles; + if (*entry.count > 0) { + aggregate_counts[entry.key] += *entry.count; } } return aggregate_counts; @@ -63,42 +41,11 @@ void GlobalOpCountContainer::clear() { std::unique_lock lock(mutex); for (Entry& entry : counts) { - *entry.count = OpStats(); + *entry.count = 0; } } // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) GlobalOpCountContainer GLOBAL_OP_COUNTS; - -OpCountCycleReporter::OpCountCycleReporter(OpStats* stats) - : stats(stats) -{ -#if __clang__ && (defined(__x86_64__) || defined(_M_X64) || defined(__i386) || defined(_M_IX86)) - // Don't support any other targets but x86 clang for now, this is a bit lazy but more than fits our needs - cycles = __builtin_ia32_rdtsc(); -#endif -} -OpCountCycleReporter::~OpCountCycleReporter() -{ -#if __clang__ && (defined(__x86_64__) || defined(_M_X64) || defined(__i386) || defined(_M_IX86)) - // Don't support any other targets but x86 clang for now, this is a bit lazy but more than fits our needs - stats->count += 1; - stats->cycles += __builtin_ia32_rdtsc() - cycles; -#endif -} -OpCountTimeReporter::OpCountTimeReporter(OpStats* stats) - : stats(stats) -{ - auto now = std::chrono::high_resolution_clock::now(); - auto now_ns = std::chrono::time_point_cast(now); - time = static_cast(now_ns.time_since_epoch().count()); -} -OpCountTimeReporter::~OpCountTimeReporter() -{ - auto now = std::chrono::high_resolution_clock::now(); - auto now_ns = std::chrono::time_point_cast(now); - stats->count += 1; - stats->time += static_cast(now_ns.time_since_epoch().count()) - time; -} } // namespace bb::detail #endif diff --git a/barretenberg/cpp/src/barretenberg/common/op_count.hpp b/barretenberg/cpp/src/barretenberg/common/op_count.hpp index c04a26575aab..57600138fe20 100644 --- a/barretenberg/cpp/src/barretenberg/common/op_count.hpp +++ b/barretenberg/cpp/src/barretenberg/common/op_count.hpp @@ -1,15 +1,12 @@ #pragma once -#include #ifndef BB_USE_OP_COUNT // require a semicolon to appease formatters // NOLINTNEXTLINE(cppcoreguidelines-macro-usage) #define BB_OP_COUNT_TRACK() (void)0 // NOLINTNEXTLINE(cppcoreguidelines-macro-usage) #define BB_OP_COUNT_TRACK_NAME(name) (void)0 -#define BB_OP_COUNT_CYCLES() (void)0 -#define BB_OP_COUNT_TIME() (void)0 #else /** * Provides an abstraction that counts operations based on function names. @@ -40,27 +37,20 @@ template struct OperationLabel { char value[N]; }; -struct OpStats { - std::size_t count = 0; - std::size_t time = 0; - std::size_t cycles = 0; -}; - // Contains all statically known op counts struct GlobalOpCountContainer { public: struct Entry { std::string key; std::string thread_id; - std::shared_ptr count; + std::size_t* count; }; - ~GlobalOpCountContainer(); std::mutex mutex; std::vector counts; void print() const; // NOTE: Should be called when other threads aren't active void clear(); - void add_entry(const char* key, const std::shared_ptr& count); + void add_entry(const char* key, std::size_t* count); std::map get_aggregate_counts() const; }; @@ -70,77 +60,28 @@ extern GlobalOpCountContainer GLOBAL_OP_COUNTS; template struct GlobalOpCount { public: // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) - static thread_local std::shared_ptr stats; + static thread_local std::size_t* thread_local_count; - static OpStats* ensure_stats() - { - if (BB_UNLIKELY(stats == nullptr)) { - stats = std::make_shared(); - GLOBAL_OP_COUNTS.add_entry(Op.value, stats); - } - return stats.get(); - } static constexpr void increment_op_count() { -#ifndef BB_USE_OP_COUNT_TIME_ONLY - if (std::is_constant_evaluated()) { - // We do nothing if the compiler tries to run this - return; - } - ensure_stats(); - stats->count++; -#endif - } - static constexpr void add_cycle_time(std::size_t cycles) - { -#ifndef BB_USE_OP_COUNT_TRACK_ONLY if (std::is_constant_evaluated()) { // We do nothing if the compiler tries to run this return; } - ensure_stats(); - stats->cycles += cycles; -#endif - } - static constexpr void add_clock_time(std::size_t time) - { -#ifndef BB_USE_OP_COUNT_TRACK_ONLY - if (std::is_constant_evaluated()) { - // We do nothing if the compiler tries to run this - return; + if (BB_UNLIKELY(thread_local_count == nullptr)) { + thread_local_count = new std::size_t(); + GLOBAL_OP_COUNTS.add_entry(Op.value, thread_local_count); } - ensure_stats(); - stats->time += time; -#endif + (*thread_local_count)++; } }; // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) -template thread_local std::shared_ptr GlobalOpCount::stats; +template thread_local std::size_t* GlobalOpCount::thread_local_count; -// NOLINTNEXTLINE(cppcoreguidelines-special-member-functions) -struct OpCountCycleReporter { - OpStats* stats; - std::size_t cycles; - OpCountCycleReporter(OpStats* stats); - ~OpCountCycleReporter(); -}; -// NOLINTNEXTLINE(cppcoreguidelines-special-member-functions) -struct OpCountTimeReporter { - OpStats* stats; - std::size_t time; - OpCountTimeReporter(OpStats* stats); - ~OpCountTimeReporter(); -}; } // namespace bb::detail // NOLINTNEXTLINE(cppcoreguidelines-macro-usage) #define BB_OP_COUNT_TRACK() bb::detail::GlobalOpCount<__func__>::increment_op_count() // NOLINTNEXTLINE(cppcoreguidelines-macro-usage) #define BB_OP_COUNT_TRACK_NAME(name) bb::detail::GlobalOpCount::increment_op_count() -// NOLINTNEXTLINE(cppcoreguidelines-macro-usage) -#define BB_OP_COUNT_CYCLES() \ - bb::detail::OpCountCycleReporter __bb_op_count_cyles(bb::detail::GlobalOpCount<__func__>::ensure_stats()) -// NOLINTNEXTLINE(cppcoreguidelines-macro-usage) -#define BB_OP_COUNT_TIME() \ - bb::detail::OpCountTimeReporter __bb_op_count_time(bb::detail::GlobalOpCount<__func__>::ensure_stats()) #endif \ No newline at end of file From 5c3783eb04a0d0652e6f974a11f3f93c1e192bc7 Mon Sep 17 00:00:00 2001 From: ludamad Date: Sat, 10 Feb 2024 00:27:05 +0000 Subject: [PATCH 08/18] time --- .../cpp/src/barretenberg/ecc/fields/field_declarations.hpp | 4 ---- barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp | 2 -- 2 files changed, 6 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp b/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp index cb75353c09f2..cf56960d0986 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp +++ b/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp @@ -327,7 +327,6 @@ template struct alignas(32) field { **/ static void split_into_endomorphism_scalars(const field& k, field& k1, field& k2) { - BB_OP_COUNT_TIME(); // if the modulus is a 256-bit integer, we need to use a basis where g1, g2 have been shifted by 2^384 if constexpr (Params::modulus_3 >= 0x4000000000000000ULL) { split_into_endomorphism_scalars_384(k, k1, k2); @@ -344,7 +343,6 @@ template struct alignas(32) field { static std::pair, std::array> split_into_endomorphism_scalars_no_shift( const field& k) { - BB_OP_COUNT_TIME(); static_assert(Params::modulus_3 < 0x4000000000000000ULL); field input = k.reduce_once(); @@ -392,8 +390,6 @@ template struct alignas(32) field { static void split_into_endomorphism_scalars_384(const field& input, field& k1_out, field& k2_out) { - BB_OP_COUNT_TIME(); - constexpr field minus_b1f{ Params::endo_minus_b1_lo, Params::endo_minus_b1_mid, diff --git a/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp b/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp index 78872bde30b3..4780f1dfec21 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp +++ b/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp @@ -637,7 +637,6 @@ template struct EndomorphismWnaf { template element element::mul_with_endomorphism(const Fr& exponent) const noexcept { - BB_OP_COUNT_TIME(); constexpr size_t NUM_ROUNDS = 32; const Fr converted_scalar = exponent.from_montgomery_form(); @@ -785,7 +784,6 @@ template std::vector> element::batch_mul_with_endomorphism( const std::span>& points, const Fr& exponent) noexcept { - BB_OP_COUNT_TIME(); typedef affine_element affine_element; const size_t num_points = points.size(); From 9c08a58fbc7f85fd8fcba213c82d8f97a36d64b9 Mon Sep 17 00:00:00 2001 From: ludamad Date: Sat, 10 Feb 2024 03:42:10 +0000 Subject: [PATCH 09/18] fix --- .../ecc/fields/field_declarations.hpp | 2 +- .../ecc/groups/affine_element.test.cpp | 17 +++++++++++++++++ .../barretenberg/ecc/groups/element_impl.hpp | 3 ++- 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp b/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp index cf56960d0986..3cff3769712f 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp +++ b/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp @@ -336,7 +336,7 @@ template struct alignas(32) field { k1.data[0] = ret.first[0]; k1.data[1] = ret.first[1]; k2.data[0] = ret.second[0]; - k2.data[1] = ret.second[0]; + k2.data[1] = ret.second[1]; } } 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 5a39a042e479..5a2067afd69c 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.test.cpp +++ b/barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.test.cpp @@ -165,6 +165,23 @@ TEST(AffineElement, InfinityMul) EXPECT_TRUE(result.is_point_at_infinity()); } +TEST(AffineElement, BatchMulMatchesMul) +{ + constexpr size_t num_points = 1024; + std::vector affine_points; + for (size_t i = 0; i < num_points; ++i) { + affine_points.emplace_back(grumpkin::g1::affine_element::random_element()); + } + grumpkin::fr exponent = grumpkin::fr::random_element(); + std::vector result = + grumpkin::g1::element::batch_mul_with_endomorphism(affine_points, exponent); + size_t i = 0; + for (grumpkin::g1::affine_element& el : result) { + EXPECT_EQ(el, affine_points[i] * exponent); + i++; + } +} + TEST(AffineElement, InfinityBatchMul) { constexpr size_t num_points = 1024; diff --git a/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp b/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp index 4780f1dfec21..4940d1507e9c 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp +++ b/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp @@ -1081,7 +1081,8 @@ std::vector> element::batch_mul_with_endomo num_points, [&](size_t start, size_t end) { for (size_t i = start; i < end; ++i) { - work_elements[i] = points[i].is_infinity() ? work_elements[i].set_infinity() : work_elements[i]; + work_elements[i] = + points[i].is_point_at_infinity() ? work_elements[i].set_infinity() : work_elements[i]; } }, /*finite_field_additions_per_iteration=*/0, From 9a28126a91e86090e23447e88afaa8844423d60e Mon Sep 17 00:00:00 2001 From: ludamad Date: Sun, 11 Feb 2024 11:39:29 +0000 Subject: [PATCH 10/18] fix: gcc --- .../src/barretenberg/ecc/fields/field_declarations.hpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp b/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp index 3cff3769712f..f3fe75703a97 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp +++ b/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp @@ -335,8 +335,17 @@ template struct alignas(32) field { split_into_endomorphism_scalars_no_shift(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 +#if !defined(__clang__) && defined(__GNUC__) +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Warray-bounds" +#endif k2.data[0] = ret.second[0]; k2.data[1] = ret.second[1]; +#if !defined(__clang__) && defined(__GNUC__) +#pragma GCC diagnostic pop +#endif } } From 2a38c9a15a1b0ca5e59b811b9c9179e292ec4ff2 Mon Sep 17 00:00:00 2001 From: ludamad Date: Sun, 11 Feb 2024 11:48:37 +0000 Subject: [PATCH 11/18] fix --- .../src/barretenberg/ecc/fields/field_declarations.hpp | 2 +- .../cpp/src/barretenberg/ecc/groups/element_impl.hpp | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp b/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp index f3fe75703a97..15bb88d27628 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp +++ b/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp @@ -341,7 +341,7 @@ template struct alignas(32) field { #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Warray-bounds" #endif - k2.data[0] = ret.second[0]; + k2.data[0] = ret.second[0]; // NOLINT k2.data[1] = ret.second[1]; #if !defined(__clang__) && defined(__GNUC__) #pragma GCC diagnostic pop diff --git a/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp b/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp index 4940d1507e9c..fab63b7a7a5b 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp +++ b/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp @@ -954,9 +954,17 @@ std::vector> element::batch_mul_with_endomo uint64_t wnaf_table[num_rounds * 2]; Fr endo_scalar; + // TODO(AD): We should move away from this hack by adapting split_into_endomorphism_scalars_no_shift +#if !defined(__clang__) && defined(__GNUC__) +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Warray-bounds" +#endif // Split single scalar into 2 scalar in endo form Fr::split_into_endomorphism_scalars(converted_scalar, endo_scalar, *(Fr*)&endo_scalar.data[2]); // NOLINT +#if !defined(__clang__) && defined(__GNUC__) +#pragma GCC diagnostic pop +#endif bool skew = false; bool endo_skew = false; From 3aa3ce2e736b014705bdb06fbd6641b88dd312f1 Mon Sep 17 00:00:00 2001 From: ludamad Date: Mon, 12 Feb 2024 10:34:57 +0000 Subject: [PATCH 12/18] fix: inversion of 0 in the field --- .../cpp/src/barretenberg/ecc/groups/element_impl.hpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp b/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp index fab63b7a7a5b..4e5e28123ea9 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp +++ b/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp @@ -919,8 +919,10 @@ std::vector> element::batch_mul_with_endomo num_points, [&temp_point_vector, &lookup_table, &points](size_t start, size_t end) { for (size_t i = start; i < end; ++i) { - temp_point_vector[i] = points[i]; - lookup_table[0][i] = points[i]; + // If the point is at infinity we fix-up the result later + // To avoid 'trying to invert zero in the field' we set the point to 'one' here + temp_point_vector[i] = points[i].is_point_at_infinity() ? affine_element::one() : points[i]; + lookup_table[0][i] = points[i].is_point_at_infinity() ? affine_element::one() : points[i]; } }, /*finite_field_additions_per_iteration=*/0, From b95502f383a51cf0cf3f40ddb17f08c2329e92e6 Mon Sep 17 00:00:00 2001 From: ludamad Date: Mon, 12 Feb 2024 10:41:15 +0000 Subject: [PATCH 13/18] cleanup --- .../cpp/src/barretenberg/ecc/fields/field_declarations.hpp | 1 - 1 file changed, 1 deletion(-) diff --git a/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp b/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp index 15bb88d27628..d0aa109e9657 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp +++ b/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp @@ -1,7 +1,6 @@ #pragma once #include "barretenberg/common/assert.hpp" #include "barretenberg/common/compiler_hints.hpp" -#include "barretenberg/common/op_count.hpp" #include "barretenberg/numeric/random/engine.hpp" #include "barretenberg/numeric/uint128/uint128.hpp" #include "barretenberg/numeric/uint256/uint256.hpp" From 4313e8b212d2164af83e07372f0f4b2706500f66 Mon Sep 17 00:00:00 2001 From: ludamad Date: Mon, 12 Feb 2024 10:47:11 +0000 Subject: [PATCH 14/18] consistency --- .../barretenberg/ecc/groups/element_impl.hpp | 26 +++---------------- 1 file changed, 4 insertions(+), 22 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp b/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp index 4e5e28123ea9..594e35513314 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp +++ b/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp @@ -953,26 +953,8 @@ std::vector> element::batch_mul_with_endomo batch_affine_add_internal(&temp_point_vector[0], &lookup_table[j][0]); } - uint64_t wnaf_table[num_rounds * 2]; - Fr endo_scalar; - - // TODO(AD): We should move away from this hack by adapting split_into_endomorphism_scalars_no_shift -#if !defined(__clang__) && defined(__GNUC__) -#pragma GCC diagnostic push -#pragma GCC diagnostic ignored "-Warray-bounds" -#endif - // Split single scalar into 2 scalar in endo form - Fr::split_into_endomorphism_scalars(converted_scalar, endo_scalar, *(Fr*)&endo_scalar.data[2]); // NOLINT - -#if !defined(__clang__) && defined(__GNUC__) -#pragma GCC diagnostic pop -#endif - bool skew = false; - bool endo_skew = false; - - // Construct wnaf forms from scalars - wnaf::fixed_wnaf(&endo_scalar.data[0], &wnaf_table[0], skew, 0, 2, num_wnaf_bits); - wnaf::fixed_wnaf(&endo_scalar.data[2], &wnaf_table[1], endo_skew, 0, 2, num_wnaf_bits); + detail::EndoScalars endo_scalars = Fr::split_into_endomorphism_scalars_no_shift(converted_scalar); + detail::EndomorphismWnaf wnaf{ endo_scalars }; std::vector work_elements(num_points); @@ -982,7 +964,7 @@ std::vector> element::batch_mul_with_endomo bool sign = 0; // Prepare elements for the first batch addition for (size_t j = 0; j < 2; ++j) { - wnaf_entry = wnaf_table[j]; + wnaf_entry = wnaf.table[j]; index = wnaf_entry & 0x0fffffffU; sign = static_cast((wnaf_entry >> 31) & 1); const bool is_odd = ((j & 1) == 1); @@ -1016,7 +998,7 @@ std::vector> element::batch_mul_with_endomo batch_affine_add_internal(&temp_point_vector[0], &work_elements[0]); // Run through SM logic in wnaf form (excluding the skew) for (size_t j = 2; j < num_rounds * 2; ++j) { - wnaf_entry = wnaf_table[j]; + wnaf_entry = wnaf.table[j]; index = wnaf_entry & 0x0fffffffU; sign = static_cast((wnaf_entry >> 31) & 1); const bool is_odd = ((j & 1) == 1); From 637c531d75dfd46a09b4a2c6e2457101e4d42c08 Mon Sep 17 00:00:00 2001 From: ludamad Date: Mon, 12 Feb 2024 10:49:46 +0000 Subject: [PATCH 15/18] fix consistency pass --- .../src/barretenberg/ecc/groups/element_impl.hpp | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp b/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp index 594e35513314..e801df15194f 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp +++ b/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp @@ -906,10 +906,9 @@ std::vector> element::batch_mul_with_endomo return results; } - constexpr size_t lookup_size = 8; - constexpr size_t num_rounds = 32; - constexpr size_t num_wnaf_bits = 4; - std::array, lookup_size> lookup_table; + constexpr size_t LOOKUP_SIZE = 8; + constexpr size_t NUM_ROUNDS = 32; + std::array, LOOKUP_SIZE> lookup_table; for (auto& table : lookup_table) { table.resize(num_points); } @@ -935,7 +934,7 @@ std::vector> element::batch_mul_with_endomo // Construct lookup table batch_affine_double(&temp_point_vector[0]); - for (size_t j = 1; j < lookup_size; ++j) { + for (size_t j = 1; j < LOOKUP_SIZE; ++j) { run_loop_in_parallel_if_effective( num_points, [j, &lookup_table](size_t start, size_t end) { @@ -997,7 +996,7 @@ std::vector> element::batch_mul_with_endomo // First cycle of addition batch_affine_add_internal(&temp_point_vector[0], &work_elements[0]); // Run through SM logic in wnaf form (excluding the skew) - for (size_t j = 2; j < num_rounds * 2; ++j) { + for (size_t j = 2; j < NUM_ROUNDS * 2; ++j) { wnaf_entry = wnaf.table[j]; index = wnaf_entry & 0x0fffffffU; sign = static_cast((wnaf_entry >> 31) & 1); @@ -1031,7 +1030,7 @@ std::vector> element::batch_mul_with_endomo } // Apply skew for the first endo scalar - if (skew) { + if (wnaf.skew) { run_loop_in_parallel_if_effective( num_points, [&lookup_table, &temp_point_vector](size_t start, size_t end) { @@ -1050,7 +1049,7 @@ std::vector> element::batch_mul_with_endomo batch_affine_add_internal(&temp_point_vector[0], &work_elements[0]); } // Apply skew for the second endo scalar - if (endo_skew) { + if (wnaf.endo_skew) { run_loop_in_parallel_if_effective( num_points, [beta, &lookup_table, &temp_point_vector](size_t start, size_t end) { From a828c5bc15371a4b53cea0cb5c097c2b53e2a714 Mon Sep 17 00:00:00 2001 From: ludamad Date: Mon, 12 Feb 2024 12:14:39 +0000 Subject: [PATCH 16/18] fix: master --- yarn-project/sequencer-client/package.json | 8 -------- 1 file changed, 8 deletions(-) diff --git a/yarn-project/sequencer-client/package.json b/yarn-project/sequencer-client/package.json index 5a1c48d0fa6a..1db7f69a61bc 100644 --- a/yarn-project/sequencer-client/package.json +++ b/yarn-project/sequencer-client/package.json @@ -68,13 +68,5 @@ "types": "./dest/index.d.ts", "engines": { "node": ">=18" - }, - "jest": { - "preset": "ts-jest/presets/default-esm", - "moduleNameMapper": { - "^(\\.{1,2}/.*)\\.[cm]?js$": "$1" - }, - "testRegex": "./src/.*\\.test\\.(js|mjs|ts)$", - "rootDir": "./src" } } From 2c2e5d3beed4aea53817bca2257b40d88b458cd2 Mon Sep 17 00:00:00 2001 From: ludamad Date: Mon, 12 Feb 2024 15:19:35 +0000 Subject: [PATCH 17/18] 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 d0aa109e9657..9eb6d3f1c075 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 5a2067afd69c..8dc10905f5a6 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 35933ec38009..f73f7ab5fe0f 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 099aee803e79..24e45600ae46 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 e7da2ad084aa..c022809208aa 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, From f48445f7e50bff33c6886ffd280eab742dde19cf Mon Sep 17 00:00:00 2001 From: ludamad Date: Mon, 12 Feb 2024 18:33:29 +0000 Subject: [PATCH 18/18] comment fix --- .../cpp/src/barretenberg/ecc/fields/field_declarations.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp b/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp index 9eb6d3f1c075..3e7f433be6e0 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp +++ b/barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp @@ -326,7 +326,7 @@ template struct alignas(32) field { **/ static void split_into_endomorphism_scalars(const field& k, field& k1, field& k2) { - // if the modulus is a 256-bit integer, we need to use a basis where g1, g2 have been shifted by 2^384 + // if the modulus is a >= 255-bit integer, we need to use a basis where g1, g2 have been shifted by 2^384 if constexpr (Params::modulus_3 >= 0x4000000000000000ULL) { split_into_endomorphism_scalars_384(k, k1, k2); } else { @@ -348,7 +348,7 @@ template struct alignas(32) field { } } - // NOTE: this form is only usable if the modulus is not a 256-bit integer, otherwise see + // NOTE: this form is only usable if the modulus is 254 bits or less, 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)