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.

Sign up for GitHub

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: g2.Serialize sporadic failure #4626

Merged
merged 3 commits into from
Feb 15, 2024
Merged

fix: g2.Serialize sporadic failure #4626

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ee89aa9
fix: g2.Serialize sporadic failure
ludamad0 Feb 15, 2024
0193ca9
consistency
ludamad0 Feb 15, 2024
8a34999
comment++
ludamad0 Feb 15, 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
4 changes: 2 additions & 2 deletions barretenberg/cpp/dockerfiles/interactive/Dockerfile.msan.ubuntu
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ RUN cmake -G Ninja -S llvm-project/runtimes -B llvm-project/build \
RUN ninja -C llvm-project/build cxx cxxabi
RUN ninja -C llvm-project/build install-cxx install-cxxabi

ENV MSAN_CFLAGS="-std=c++20 -fsanitize=memory -nostdinc++ -I/opt/include -I/opt/include/c++/v1"
ENV MSAN_LFLAGS="-fsanitize=memory -stdlib=libc++ -L/opt/lib -lc++abi -Wl,-rpath,/opt/lib"
ENV MSAN_CFLAGS="-std=c++20 -fsanitize=memory -fsanitize-memory-track-origins -nostdinc++ -I/opt/include -I/opt/include/c++/v1"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It worked before, and this is 2x slower, but boy is it useful seeing exactly where the problem memory was allocated. Worth it.

ENV MSAN_LFLAGS="-fsanitize=memory -fsanitize-memory-track-origins -stdlib=libc++ -L/opt/lib -lc++abi -Wl,-rpath,/opt/lib"

WORKDIR /usr/src/barretenberg/cpp

Expand Down
54 changes: 15 additions & 39 deletions barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#include "barretenberg/ecc/curves/bn254/fq2.hpp"
#include "barretenberg/numeric/uint256/uint256.hpp"
#include "barretenberg/serialize/msgpack.hpp"
#include <cstring>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for memset

#include <type_traits>
#include <vector>

Expand All @@ -21,7 +22,7 @@ template <typename Fq_, typename Fr_, typename Params> class alignas(64) affine_
affine_element() noexcept = default;
~affine_element() noexcept = default;

constexpr affine_element(const Fq& a, const Fq& b) noexcept;
constexpr affine_element(const Fq& x, const Fq& y) noexcept;

constexpr affine_element(const affine_element& other) noexcept = default;

Expand Down Expand Up @@ -103,13 +104,9 @@ template <typename Fq_, typename Fr_, typename Params> class alignas(64) affine_
static void serialize_to_buffer(const affine_element& value, uint8_t* buffer)
{
if (value.is_point_at_infinity()) {
if constexpr (Fq::modulus.get_msb() == 255) {
write(buffer, uint256_t(0));
write(buffer, Fq::modulus);
} else {
write(buffer, uint256_t(0));
write(buffer, uint256_t(1) << 255);
}
// if we are infinity, just set all buffer bits to 1
// we only need this case because the below gets mangled converting from montgomery for infinity points
memset(buffer, 255, sizeof(Fq) * 2);
} else {
Fq::serialize_to_buffer(value.y, buffer);
Fq::serialize_to_buffer(value.x, buffer + sizeof(Fq));
Expand All @@ -130,38 +127,17 @@ template <typename Fq_, typename Fr_, typename Params> class alignas(64) affine_
*/
static affine_element serialize_from_buffer(uint8_t* buffer)
{
affine_element result;

// need to read a raw uint256_t to avoid reductions so we can check whether the point is the point at infinity
auto raw_x = from_buffer<uint256_t>(buffer + sizeof(Fq));

if constexpr (Fq::modulus.get_msb() == 255) {
if (raw_x == Fq::modulus) {
result.y = Fq::zero();
result.x.data[0] = raw_x.data[0];
result.x.data[1] = raw_x.data[1];
result.x.data[2] = raw_x.data[2];
result.x.data[3] = raw_x.data[3];
} else {
result.y = Fq::serialize_from_buffer(buffer);
result.x = Fq(raw_x);
}
} else {
if (raw_x.get_msb() == 255) {
result.y = Fq::zero();
result.x = Fq::zero();
result.self_set_infinity();
} else {
// conditional here to avoid reading the same data twice in case of a field of prime order
if constexpr (std::is_same<Fq, fq2>::value) {
result.y = Fq::serialize_from_buffer(buffer);
result.x = Fq::serialize_from_buffer(buffer + sizeof(Fq));
} else {
result.y = Fq::serialize_from_buffer(buffer);
result.x = Fq(raw_x);
}
}
// Does the buffer consist entirely of set bits? If so, we have a point at infinity
// Note that if it isn't, this loop should end early.
// We only need this case because the below gets mangled converting to montgomery for infinity points
bool is_point_at_infinity =
std::all_of(buffer, buffer + sizeof(Fq) * 2, [](uint8_t val) { return val == 255; });
if (is_point_at_infinity) {
return affine_element::infinity();
}
affine_element result;
result.y = Fq::serialize_from_buffer(buffer);
result.x = Fq::serialize_from_buffer(buffer + sizeof(Fq));
return result;
}

Expand Down
6 changes: 3 additions & 3 deletions barretenberg/cpp/src/barretenberg/ecc/groups/affine_element_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@

namespace bb::group_elements {
template <class Fq, class Fr, class T>
constexpr affine_element<Fq, Fr, T>::affine_element(const Fq& a, const Fq& b) noexcept
: x(a)
, y(b)
constexpr affine_element<Fq, Fr, T>::affine_element(const Fq& x, const Fq& y) noexcept
: x(x)
, y(y)
{}

template <class Fq, class Fr, class T>
Expand Down
Loading