fix: mul with endomorphism (#4538)

Previously, batch_mul_with_endomorphism and mul_with_endomorphism did
not consider points at infinity. This caused either invert-0 errors or
incorrect results when the point at infinity was multiplied by any
scalar.

The inconsistency between getting invert-0 errors is probably something
worth following up on.

---------

Co-authored-by: ludamad <adam@aztecprotocol.com>

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"

barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp

@@ -326,11 +326,34 @@ template <class Params_> 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);
-            return;
+        } else {
+            std::pair<std::array<uint64_t, 2>, std::array<uint64_t, 2>> ret = split_into_endomorphism_scalars(k);
+            k1.data[0] = ret.first[0];
+            k1.data[1] = ret.first[1];
+
+            // 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"
+#endif
+            k2.data[0] = ret.second[0]; // NOLINT
+            k2.data[1] = ret.second[1];
+#if !defined(__clang__) && defined(__GNUC__)
+#pragma GCC diagnostic pop
+#endif
         }
+    }
+
+    // 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<uint64_t, 2>, std::array<uint64_t, 2>> split_into_endomorphism_scalars(const field& k)
+    {
+        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,15 +392,14 @@ template <class Params_> 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)
     {
-
         constexpr field minus_b1f{
             Params::endo_minus_b1_lo,
             Params::endo_minus_b1_mid,

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 <fstream>
 
@@ -129,3 +130,80 @@ TEST(AffineElement, Msgpack)
     auto [actual, expected] = msgpack_roundtrip(secp256k1::g1::affine_element{ 1, 1 });
     EXPECT_EQ(actual, expected);
 }
+
+namespace bb::group_elements {
+// 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 <typename Element, typename Scalar>
+    static Element mul_without_endomorphism(const Element& element, const Scalar& scalar)
+    {
+        return element.mul_without_endomorphism(scalar);
+    }
+    template <typename Element, typename Scalar>
+    static Element mul_with_endomorphism(const Element& element, const Scalar& scalar)
+    {
+        return element.mul_with_endomorphism(scalar);
+    }
+};
+} // namespace bb::group_elements
+
+// 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());
+        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);
+    }
+}
+
+// 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());
+}
+
+// Batched multiplication of points should match
+TEST(AffineElement, BatchMulMatchesNonBatchMul)
+{
+    constexpr size_t num_points = 512;
+    std::vector<grumpkin::g1::affine_element> affine_points;
+    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<grumpkin::g1::affine_element> 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++;
+    }
+}
+
+// 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<grumpkin::g1::affine_element> 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<grumpkin::g1::affine_element> 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
+        }
+    }
+}

barretenberg/cpp/src/barretenberg/ecc/groups/element.hpp

@@ -95,15 +95,17 @@ template <class Fq, class Fr, class Params> class alignas(32) element {
                                  const std::span<affine_element<Fq, Fr, Params>>& second_group,
                                  const std::span<affine_element<Fq, Fr, Params>>& results) noexcept;
     static std::vector<affine_element<Fq, Fr, Params>> batch_mul_with_endomorphism(
-        const std::span<affine_element<Fq, Fr, Params>>& points, const Fr& exponent) noexcept;
+        const std::span<affine_element<Fq, Fr, Params>>& points, const Fr& scalar) noexcept;
 
     Fq x;
     Fq y;
     Fq z;
 
   private:
-    element mul_without_endomorphism(const Fr& exponent) const noexcept;
-    element mul_with_endomorphism(const Fr& exponent) const noexcept;
+    // For test access to mul_without_endomorphism
+    friend class TestElementPrivate;
+    element mul_without_endomorphism(const Fr& scalar) const noexcept;
+    element mul_with_endomorphism(const Fr& scalar) const noexcept;
 
     template <typename = typename std::enable_if<Params::can_hash_to_curve>>
     static element random_coordinates_on_curve(numeric::RNG* engine = nullptr) noexcept;