Skip to content

Commit

Permalink
fix: g2.Serialize sporadic failure (#4626)
Browse files Browse the repository at this point in the history
There was a long-standing bug that only occurred occasionally. Memory
sanitizer found this nicely. The crux is that g2 uses field2 which is
NOT the same size as uint256_t. Simplified the code to just either be
normal serialization or a buffer of all set bits.

---------

Co-authored-by: ludamad <adam@aztecprotocol.com>
  • Loading branch information
ludamad and ludamad0 committed Feb 15, 2024
1 parent cdf67ea commit c9e6bb1
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 44 deletions.
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"
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
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>
#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
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

0 comments on commit c9e6bb1

Please sign in to comment.