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

fix: mul with endomorphism #4538

Merged
merged 26 commits into from
Feb 12, 2024
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
87b1340
feat: op count timers
ludamad0 Feb 6, 2024
20a1a1a
fixes
ludamad0 Feb 8, 2024
4c9bcc5
Merge branch 'ad/feat/op-count-timers' into infinity
ludamad0 Feb 8, 2024
834aa95
checkpoint
ludamad0 Feb 9, 2024
c9c1530
fix: batch mul
ludamad0 Feb 10, 2024
ee4ed46
fix: mul_with_endo batched
ludamad0 Feb 10, 2024
1718e13
Update settings.json
ludamad Feb 10, 2024
dd132fc
revert: timers
ludamad0 Feb 10, 2024
4c31fc9
Merge remote-tracking branch 'origin/ad/fix/endo-infinite' into ad/fi…
ludamad0 Feb 10, 2024
5c3783e
time
ludamad0 Feb 10, 2024
9c08a58
fix
ludamad0 Feb 10, 2024
9a28126
fix: gcc
ludamad0 Feb 11, 2024
2a38c9a
fix
ludamad0 Feb 11, 2024
f1396c9
Merge remote-tracking branch 'origin/master' into ad/fix/endo-infinite
ludamad0 Feb 12, 2024
3aa3ce2
fix: inversion of 0 in the field
ludamad0 Feb 12, 2024
b95502f
cleanup
ludamad0 Feb 12, 2024
4313e8b
consistency
ludamad0 Feb 12, 2024
637c531
fix consistency pass
ludamad0 Feb 12, 2024
07c6c59
Merge branch 'master' into ad/fix/endo-infinite
ludamad Feb 12, 2024
58693b9
Merge branch 'master' into ad/fix/endo-infinite
ludamad Feb 12, 2024
a828c5b
fix: master
ludamad Feb 12, 2024
4bfd00b
Merge remote-tracking branch 'origin/fix/master' into ad/fix/endo-inf…
ludamad0 Feb 12, 2024
3347344
Merge remote-tracking branch 'origin/ad/fix/endo-infinite' into ad/fi…
ludamad0 Feb 12, 2024
2c2e5d3
fix: review feedback
ludamad0 Feb 12, 2024
161262e
Merge branch 'master' into ad/fix/endo-infinite
ludamad Feb 12, 2024
f48445f
comment fix
ludamad0 Feb 12, 2024
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
11 changes: 5 additions & 6 deletions barretenberg/cpp/scripts/benchmark_remote.sh
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,19 @@ set -eu

BENCHMARK=${1:-goblin_bench}
COMMAND=${2:-./$BENCHMARK}
PRESET=${3:-clang16}
BUILD_DIR=${4:-build}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just lets us remote benchmark other presets + build folders


# 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"
Original file line number Diff line number Diff line change
Expand Up @@ -329,8 +329,29 @@ template <class Params_> struct alignas(32) field {
// 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<uint64_t, 2>, std::array<uint64_t, 2>> ret =
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]; // NOLINT
k2.data[1] = ret.second[1];
#if !defined(__clang__) && defined(__GNUC__)
#pragma GCC diagnostic pop
#endif
}
}

static std::pair<std::array<uint64_t, 2>, std::array<uint64_t, 2>> split_into_endomorphism_scalars_no_shift(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it called "no_shift"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was trying to find a good name as is the counter part to this case technically, but I may be off the mark
Screenshot 2024-02-12 at 1 41 23 PM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll just call it split_into_endomorphism_scalars, though, I think. If it was ever misused the static assert will catch it

Copy link
Contributor

Choose a reason for hiding this comment

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

You can specify that it works for values less than 256 bits if you want, but shift means something else, so it's a bit confusing

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 };
Expand Down Expand Up @@ -369,15 +390,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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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>

Expand Down Expand Up @@ -129,3 +130,72 @@ TEST(AffineElement, Msgpack)
auto [actual, expected] = msgpack_roundtrip(secp256k1::g1::affine_element{ 1, 1 });
EXPECT_EQ(actual, expected);
}

namespace bb::group_elements {
// Kludge to access mul_without_endomorphism;
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

TEST(AffineElement, EndoMulMatchesNonEndo)
Copy link
Contributor

@Rumata888 Rumata888 Feb 12, 2024

Choose a reason for hiding this comment

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

Could you please expand the names of the tests? If a person has no context, they have to look inside the test to understand what's happening

{
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);
}
}

TEST(AffineElement, InfinityMul)
{
auto result = grumpkin::g1::affine_element::infinity() * grumpkin::fr::random_element();
EXPECT_TRUE(result.is_point_at_infinity());
}

TEST(AffineElement, BatchMulMatchesMul)
{
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::random_element());
}
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++;
}
}

TEST(AffineElement, InfinityBatchMul)
{
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
}
}
}
2 changes: 2 additions & 0 deletions barretenberg/cpp/src/barretenberg/ecc/groups/element.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ template <class Fq, class Fr, class Params> 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;

Expand Down
Loading
Loading