feat: affine_element read/write with proper handling of point at infi…

…nity (#6963)

Please read [contributing guidelines](CONTRIBUTING.md) and remove this
line.

---------

Co-authored-by: codygunton <[email protected]>
Co-authored-by: ludamad <[email protected]>
Co-authored-by: ludamad <[email protected]>

.github/workflows/ci.yml

@@ -35,7 +35,7 @@ jobs:
 
   changes:
     runs-on: ubuntu-20.04
-    # Required permissions
+    # Required permissions.
     permissions:
       pull-requests: read
     # Set job outputs to values from filter step

barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_builder.test.cpp

@@ -65,7 +65,7 @@ TEST(ultra_circuit_constructor, create_gates_from_plookup_accumulators)
 
         grumpkin::g1::affine_element base_point = plookup::fixed_base::table::LHS_GENERATOR_POINT;
         std::vector<uint8_t> input_buf;
-        serialize::write(input_buf, base_point);
+        write(input_buf, base_point);
         const auto offset_generators =
             grumpkin::g1::derive_generators(input_buf, plookup::fixed_base::table::NUM_TABLES_PER_LO_MULTITABLE);
 

barretenberg/cpp/src/barretenberg/crypto/ecdsa/c_bind.cpp

@@ -8,7 +8,7 @@ WASM_EXPORT void ecdsa__compute_public_key(uint8_t const* private_key, uint8_t*
 {
     auto priv_key = from_buffer<secp256k1::fr>(private_key);
     secp256k1::g1::affine_element pub_key = secp256k1::g1::one * priv_key;
-    serialize::write(public_key_buf, pub_key);
+    write(public_key_buf, pub_key);
 }
 
 WASM_EXPORT void ecdsa__construct_signature(uint8_t const* message,
@@ -45,7 +45,7 @@ WASM_EXPORT void ecdsa__recover_public_key_from_signature(uint8_t const* message
     ecdsa_signature sig = { r, s, v };
     auto recovered_pub_key = ecdsa_recover_public_key<Sha256Hasher, secp256k1::fq, secp256k1::fr, secp256k1::g1>(
         std::string((char*)message, msg_len), sig);
-    serialize::write(output_pub_key, recovered_pub_key);
+    write(output_pub_key, recovered_pub_key);
 }
 
 WASM_EXPORT bool ecdsa__verify_signature(uint8_t const* message,

barretenberg/cpp/src/barretenberg/crypto/pedersen_commitment/c_bind.cpp

@@ -11,5 +11,5 @@ WASM_EXPORT void pedersen_commit(fr::vec_in_buf inputs_buffer, grumpkin::g1::aff
     read(inputs_buffer, to_commit);
     grumpkin::g1::affine_element pedersen_commitment = crypto::pedersen_commitment::commit_native(to_commit);
 
-    serialize::write(output, pedersen_commitment);
+    write(output, pedersen_commitment);
 }

barretenberg/cpp/src/barretenberg/crypto/schnorr/c_bind.cpp

@@ -13,15 +13,15 @@ WASM_EXPORT void schnorr_compute_public_key(uint8_t const* private_key, uint8_t*
 {
     auto priv_key = from_buffer<grumpkin::fr>(private_key);
     grumpkin::g1::affine_element pub_key = grumpkin::g1::one * priv_key;
-    serialize::write(public_key_buf, pub_key);
+    write(public_key_buf, pub_key);
 }
 
 WASM_EXPORT void schnorr_negate_public_key(uint8_t const* public_key_buffer, uint8_t* output)
 {
     // Negate the public key (effectively negating the y-coordinate of the public key) and return the resulting public
     // key.
     auto account_public_key = from_buffer<grumpkin::g1::affine_element>(public_key_buffer);
-    serialize::write(output, -account_public_key);
+    write(output, -account_public_key);
 }
 
 WASM_EXPORT void schnorr_construct_signature(uint8_t const* message_buf,
@@ -75,10 +75,10 @@ WASM_EXPORT void schnorr_multisig_validate_and_combine_signer_pubkeys(uint8_t co
     auto combined_key = multisig::validate_and_combine_signer_pubkeys(pubkeys);
 
     if (combined_key) {
-        serialize::write(combined_key_buf, *combined_key);
+        write(combined_key_buf, *combined_key);
         *success = true;
     } else {
-        serialize::write(combined_key_buf, affine_element::one());
+        write(combined_key_buf, affine_element::one());
         *success = false;
     }
 }

barretenberg/cpp/src/barretenberg/crypto/schnorr/multisig.hpp

@@ -166,10 +166,10 @@ template <typename G1, typename HashRegNon, typename HashSig = Blake2sHasher> cl
             domain_separator_nonce.begin(), domain_separator_nonce.end(), std::back_inserter(nonce_challenge_buffer));
 
         // write the group generator
-        serialize::write(nonce_challenge_buffer, G1::affine_one);
+        write(nonce_challenge_buffer, G1::affine_one);
 
         // write X
-        serialize::write(nonce_challenge_buffer, aggregate_pubkey);
+        write(nonce_challenge_buffer, aggregate_pubkey);
 
         // we slightly deviate from the protocol when including 'm', since the length of 'm' is variable
         // by writing a prefix and a suffix, we prevent the message from being interpreted as coming from a different
@@ -188,8 +188,8 @@ template <typename G1, typename HashRegNon, typename HashSig = Blake2sHasher> cl
 
         // write  {(R1, S1), ..., (Rn, Sn)}
         for (const auto& nonce : round_1_nonces) {
-            serialize::write(nonce_challenge_buffer, nonce.R);
-            serialize::write(nonce_challenge_buffer, nonce.S);
+            write(nonce_challenge_buffer, nonce.R);
+            write(nonce_challenge_buffer, nonce.S);
         }
 
         // uses the different hash function for proper domain separation

barretenberg/cpp/src/barretenberg/crypto/schnorr/proof_of_possession.hpp

@@ -106,14 +106,14 @@ template <typename G1, typename Hash> struct SchnorrProofOfPossession {
         std::copy(domain_separator_pop.begin(), domain_separator_pop.end(), std::back_inserter(challenge_buf));
 
         // write the group generator
-        serialize::write(challenge_buf, G1::affine_one);
+        write(challenge_buf, G1::affine_one);
 
         // write X twice as per the spec
-        serialize::write(challenge_buf, public_key);
-        serialize::write(challenge_buf, public_key);
+        write(challenge_buf, public_key);
+        write(challenge_buf, public_key);
 
         // write R
-        serialize::write(challenge_buf, R);
+        write(challenge_buf, R);
 
         // generate the raw bits of H_reg(X,X,R)
         return Hash::hash(challenge_buf);

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

@@ -1,4 +1,5 @@
 #pragma once
+#include "barretenberg/common/serialize.hpp"
 #include "barretenberg/ecc/curves/bn254/fq2.hpp"
 #include "barretenberg/numeric/uint256/uint256.hpp"
 #include "barretenberg/serialize/msgpack.hpp"
@@ -101,15 +102,18 @@ template <typename Fq_, typename Fr_, typename Params> class alignas(64) affine_
      * @warning This will need to be updated if we serialize points over composite-order fields other than fq2!
      *
      */
-    static void serialize_to_buffer(const affine_element& value, uint8_t* buffer)
+    static void serialize_to_buffer(const affine_element& value, uint8_t* buffer, bool write_x_first = false)
     {
+        using namespace serialize;
         if (value.is_point_at_infinity()) {
             // 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));
+            // Note: for historic reasons we will need to redo downstream hashes if we want this to always be written in
+            // the same order in our various serialization flows
+            write(buffer, write_x_first ? value.x : value.y);
+            write(buffer, write_x_first ? value.y : value.x);
         }
     }
 
@@ -125,8 +129,9 @@ template <typename Fq_, typename Fr_, typename Params> class alignas(64) affine_
      *
      * @warning This will need to be updated if we serialize points over composite-order fields other than fq2!
      */
-    static affine_element serialize_from_buffer(uint8_t* buffer)
+    static affine_element serialize_from_buffer(const uint8_t* buffer, bool write_x_first = false)
     {
+        using namespace serialize;
         // 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
@@ -136,8 +141,10 @@ template <typename Fq_, typename Fr_, typename Params> class alignas(64) affine_
             return affine_element::infinity();
         }
         affine_element result;
-        result.y = Fq::serialize_from_buffer(buffer);
-        result.x = Fq::serialize_from_buffer(buffer + sizeof(Fq));
+        // Note: for historic reasons we will need to redo downstream hashes if we want this to always be read in the
+        // same order in our various serialization flows
+        read(buffer, write_x_first ? result.x : result.y);
+        read(buffer, write_x_first ? result.y : result.x);
         return result;
     }
 
@@ -160,11 +167,27 @@ template <typename Fq_, typename Fr_, typename Params> class alignas(64) affine_
     }
     Fq x;
     Fq y;
-
-    // for serialization: update with new fields
-    // TODO(https://github.com/AztecProtocol/barretenberg/issues/908) point at inifinty isn't handled
-    MSGPACK_FIELDS(x, y);
 };
+
+template <typename B, typename Fq_, typename Fr_, typename Params>
+inline void read(B& it, group_elements::affine_element<Fq_, Fr_, Params>& element)
+{
+    using namespace serialize;
+    std::array<uint8_t, sizeof(element)> buffer;
+    read(it, buffer);
+    element = group_elements::affine_element<Fq_, Fr_, Params>::serialize_from_buffer(
+        buffer.data(), /* use legacy field order */ true);
+}
+
+template <typename B, typename Fq_, typename Fr_, typename Params>
+inline void write(B& it, group_elements::affine_element<Fq_, Fr_, Params> const& element)
+{
+    using namespace serialize;
+    std::array<uint8_t, sizeof(element)> buffer;
+    group_elements::affine_element<Fq_, Fr_, Params>::serialize_to_buffer(
+        element, buffer.data(), /* use legacy field order */ true);
+    write(it, buffer);
+}
 } // namespace bb::group_elements
 
 #include "./affine_element_impl.hpp"

barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.test.cpp

@@ -66,6 +66,29 @@ template <typename G1> class TestAffineElement : public testing::Test {
         }
     }
 
+    static void test_read_and_write()
+    {
+        // a generic point
+        {
+            affine_element P = affine_element(element::random_element());
+            [[maybe_unused]] affine_element R;
+
+            std::vector<uint8_t> v(sizeof(R));
+            uint8_t* ptr = v.data();
+            write(ptr, P);
+            ASSERT_TRUE(P.on_curve());
+
+            // // Reset to start?
+            // ptr = v.data();
+
+            const uint8_t* read_ptr = v.data();
+            // good read
+            read(read_ptr, R);
+            ASSERT_TRUE(R.on_curve());
+            ASSERT_TRUE(P == R);
+        }
+    }
+
     static void test_point_compression()
     {
         for (size_t i = 0; i < 10; i++) {
@@ -110,17 +133,6 @@ template <typename G1> class TestAffineElement : public testing::Test {
         EXPECT_NE(P < Q, Q < P);
     }
 
-    /**
-     * @brief Check that msgpack encoding is consistent with decoding
-     *
-     */
-    static void test_msgpack_roundtrip()
-    {
-        // TODO(https://github.com/AztecProtocol/barretenberg/issues/908) point at inifinty isn't handled
-        auto [actual, expected] = msgpack_roundtrip(affine_element{ 1, 1 });
-        EXPECT_EQ(actual, expected);
-    }
-
     /**
      * @brief A regression test to make sure the -1 case is covered
      *
@@ -139,11 +151,17 @@ template <typename G1> class TestAffineElement : public testing::Test {
     }
 };
 
-using TestTypes = testing::Types<bb::g1, grumpkin::g1, secp256k1::g1, secp256r1::g1>;
+using TestTypes = testing::Types<bb::g1>;
+// using TestTypes = testing::Types<bb::g1, grumpkin::g1, secp256k1::g1, secp256r1::g1>;
 } // namespace
 
 TYPED_TEST_SUITE(TestAffineElement, TestTypes);
 
+TYPED_TEST(TestAffineElement, ReadWrite)
+{
+    TestFixture::test_read_and_write();
+}
+
 TYPED_TEST(TestAffineElement, ReadWriteBuffer)
 {
     TestFixture::test_read_write_buffer();
@@ -172,11 +190,6 @@ TYPED_TEST(TestAffineElement, InfinityOrderingRegression)
     TestFixture::test_infinity_ordering_regression();
 }
 
-TYPED_TEST(TestAffineElement, Msgpack)
-{
-    TestFixture::test_msgpack_roundtrip();
-}
-
 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

barretenberg/cpp/src/barretenberg/examples/join_split/join_split_tx.cpp

@@ -25,7 +25,7 @@ void write(std::vector<uint8_t>& buf, join_split_tx const& tx)
     write(buf, tx.account_required);
     write(buf, tx.account_note_index);
     write(buf, tx.account_note_path);
-    serialize::write(buf, tx.signing_pub_key);
+    write(buf, tx.signing_pub_key);
 
     write(buf, tx.backward_link);
     write(buf, tx.allow_chain);

barretenberg/cpp/src/barretenberg/examples/join_split/notes/native/value/value_note.hpp

@@ -53,7 +53,7 @@ inline void write(std::vector<uint8_t>& buf, value_note const& note)
     write(buf, note.value);
     write(buf, note.asset_id);
     write(buf, note.account_required);
-    serialize::write(buf, note.owner);
+    write(buf, note.owner);
     write(buf, note.secret);
     write(buf, note.creator_pubkey);
     write(buf, note.input_nullifier);

barretenberg/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp

@@ -85,7 +85,7 @@ TYPED_TEST(ultra_plonk_composer, create_gates_from_plookup_accumulators)
 
         grumpkin::g1::affine_element base_point = plookup::fixed_base::table::LHS_GENERATOR_POINT;
         std::vector<uint8_t> input_buf;
-        serialize::write(input_buf, base_point);
+        write(input_buf, base_point);
         const auto offset_generators =
             grumpkin::g1::derive_generators(input_buf, plookup::fixed_base::table::NUM_TABLES_PER_LO_MULTITABLE);
 

barretenberg/cpp/src/barretenberg/polynomials/univariate.hpp

@@ -686,4 +686,4 @@ template <typename T, typename U, std::size_t N> std::array<T, N> array_to_array
 namespace std {
 template <typename T, size_t N> struct tuple_size<bb::Univariate<T, N>> : std::integral_constant<std::size_t, N> {};
 
-} // namespace std
+} // namespace std

barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/plookup_tables/fixed_base/fixed_base.cpp

@@ -56,7 +56,7 @@ template <size_t num_bits> table::fixed_base_scalar_mul_tables table::generate_t
     result.reserve(NUM_TABLES);
 
     std::vector<uint8_t> input_buf;
-    serialize::write(input_buf, input);
+    write(input_buf, input);
     const auto offset_generators = grumpkin::g1::derive_generators(input_buf, NUM_TABLES);
 
     grumpkin::g1::element accumulator = input;
@@ -87,7 +87,7 @@ grumpkin::g1::affine_element table::generate_generator_offset(const grumpkin::g1
     constexpr size_t NUM_TABLES = get_num_tables_per_multi_table<num_table_bits>();
 
     std::vector<uint8_t> input_buf;
-    serialize::write(input_buf, input);
+    write(input_buf, input);
     const auto offset_generators = grumpkin::g1::derive_generators(input_buf, NUM_TABLES);
     grumpkin::g1::element acc = grumpkin::g1::point_at_infinity;
     for (const auto& gen : offset_generators) {

barretenberg/cpp/src/barretenberg/ultra_honk/ultra_composer.test.cpp

@@ -169,7 +169,7 @@ TEST_F(UltraHonkComposerTests, create_gates_from_plookup_accumulators)
 
         grumpkin::g1::affine_element base_point = plookup::fixed_base::table::LHS_GENERATOR_POINT;
         std::vector<uint8_t> input_buf;
-        serialize::write(input_buf, base_point);
+        write(input_buf, base_point);
         const auto offset_generators =
             grumpkin::g1::derive_generators(input_buf, plookup::fixed_base::table::NUM_TABLES_PER_LO_MULTITABLE);