feat: consistent handling of point at infinity in transcript

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

@@ -61,6 +61,10 @@ template <typename T> T convert_from_bn254_frs(std::span<const bb::fr> fr_vec)
         T val;
         val.x = convert_from_bn254_frs<BaseField>(fr_vec.subspan(0, BASE_FIELD_SCALAR_SIZE));
         val.y = convert_from_bn254_frs<BaseField>(fr_vec.subspan(BASE_FIELD_SCALAR_SIZE, BASE_FIELD_SCALAR_SIZE));
+        if (val.x == BaseField::zero() && val.y == BaseField::zero()) {
+            val.self_set_infinity();
+        }
+        ASSERT(val.on_curve());
         return val;
     } else {
         // Array or Univariate
@@ -95,8 +99,19 @@ template <typename T> std::vector<bb::fr> convert_to_bn254_frs(const T& val)
     } else if constexpr (IsAnyOf<T, grumpkin::fr>) {
         return convert_grumpkin_fr_to_bn254_frs(val);
     } else if constexpr (IsAnyOf<T, curve::BN254::AffineElement, curve::Grumpkin::AffineElement>) {
-        auto fr_vec_x = convert_to_bn254_frs(val.x);
-        auto fr_vec_y = convert_to_bn254_frs(val.y);
+        using BaseField = typename T::Fq;
+
+        std::vector<bb::fr> fr_vec_x;
+        std::vector<bb::fr> fr_vec_y;
+        // When encountering a point at infinity we pass a zero point in the proof to ensure that on the receiving size
+        // there are no inconsistencies whenre constructing and hashing.
+        if (val.is_point_at_infinity()) {
+            fr_vec_x = convert_to_bn254_frs(BaseField::zero());
+            fr_vec_y = convert_to_bn254_frs(BaseField::zero());
+        } else {
+            fr_vec_x = convert_to_bn254_frs(val.x);
+            fr_vec_y = convert_to_bn254_frs(val.y);
+        }
         std::vector<bb::fr> fr_vec(fr_vec_x.begin(), fr_vec_x.end());
         fr_vec.insert(fr_vec.end(), fr_vec_y.begin(), fr_vec_y.end());
         return fr_vec;

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

@@ -149,10 +149,25 @@ template <typename G1> class TestAffineElement : public testing::Test {
             EXPECT_EQ(affine_points[i], -result[i]);
         }
     }
+
+    /**
+     * @brief Ensure that the point at inifinity has a fixed value.
+     *
+     */
+    static void test_fixed_point_at_infinity()
+    {
+        using Fq = affine_element::Fq;
+        affine_element P = affine_element::infinity();
+        affine_element Q(Fq::zero(), Fq::zero());
+        Q.x.self_set_msb();
+        affine_element R = affine_element(element::random_element());
+        EXPECT_EQ(P, Q);
+        EXPECT_NE(P, R);
+    }
 };
 
-using TestTypes = testing::Types<bb::g1>;
-// 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);
@@ -176,6 +191,15 @@ TYPED_TEST(TestAffineElement, PointCompression)
     }
 }
 
+TYPED_TEST(TestAffineElement, FixedInfinityPoint)
+{
+    if constexpr (TypeParam::Fq::modulus.data[3] >= 0x4000000000000000ULL) {
+        GTEST_SKIP();
+    } else {
+        TestFixture::test_fixed_point_at_infinity();
+    }
+}
+
 TYPED_TEST(TestAffineElement, PointCompressionUnsafe)
 {
     if constexpr (TypeParam::Fq::modulus.data[3] >= 0x4000000000000000ULL) {

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

@@ -82,7 +82,7 @@ constexpr uint256_t affine_element<Fq, Fr, T>::compress() const noexcept
 
 template <class Fq, class Fr, class T> affine_element<Fq, Fr, T> affine_element<Fq, Fr, T>::infinity()
 {
-    affine_element e;
+    affine_element e{};
     e.self_set_infinity();
     return e;
 }
@@ -105,6 +105,8 @@ template <class Fq, class Fr, class T> constexpr void affine_element<Fq, Fr, T>:
         x.data[3] = Fq::modulus.data[3];
 
     } else {
+        (*this).x = Fq::zero();
+        (*this).y = Fq::zero();
         x.self_set_msb();
     }
 }

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

@@ -515,6 +515,9 @@ template <class Fq, class Fr, class T> constexpr void element<Fq, Fr, T>::self_s
         x.data[2] = Fq::modulus.data[2];
         x.data[3] = Fq::modulus.data[3];
     } else {
+        (*this).x = Fq::zero();
+        (*this).y = Fq::zero();
+        (*this).z = Fq::zero();
         x.self_set_msb();
     }
 }

barretenberg/cpp/src/barretenberg/eccvm/eccvm_transcript.test.cpp

@@ -26,12 +26,10 @@ class ECCVMTranscriptTests : public ::testing::Test {
      *
      * @return TranscriptManifest
      */
-    TranscriptManifest construct_eccvm_honk_manifest(size_t circuit_size, size_t log_ipa_poly_degree)
+    TranscriptManifest construct_eccvm_honk_manifest(size_t circuit_size)
     {
         TranscriptManifest manifest_expected;
         auto log_n = numeric::get_msb(circuit_size);
-        ASSERT(log_n == log_ipa_poly_degree);
-
         size_t MAX_PARTIAL_RELATION_LENGTH = Flavor::BATCHED_RELATION_PARTIAL_LENGTH;
         // Size of types is number of bb::frs needed to represent the type
         size_t frs_per_Fr = bb::field_conversion::calc_num_bn254_frs<FF>();
@@ -252,8 +250,7 @@ TEST_F(ECCVMTranscriptTests, ProverManifestConsistency)
     auto proof = prover.construct_proof();
 
     // Check that the prover generated manifest agrees with the manifest hard coded in this suite
-    auto manifest_expected =
-        this->construct_eccvm_honk_manifest(prover.key->circuit_size, prover.sumcheck_output.challenge.size());
+    auto manifest_expected = this->construct_eccvm_honk_manifest(prover.key->circuit_size);
     auto prover_manifest = prover.transcript->get_manifest();
     // Note: a manifest can be printed using manifest.print()
     for (size_t round = 0; round < manifest_expected.size(); ++round) {

barretenberg/cpp/src/barretenberg/eccvm_recursion/eccvm_recursive_verifier.cpp

@@ -38,12 +38,6 @@ template <typename Flavor> void ECCVMRecursiveVerifier_<Flavor>::verify_proof(co
 
     for (auto [comm, label] : zip_view(commitments.get_wires(), commitment_labels.get_wires())) {
         comm = transcript->template receive_from_prover<Commitment>(label);
-        // TODO(https://github.com/AztecProtocol/barretenberg/issues/1017): This is a hack to ensure zero commitments
-        // are still on curve as the transcript doesn't currently support a point at infinity representation for
-        // cycle_group
-        if (!comm.get_value().on_curve()) {
-            comm.set_point_at_infinity(true);
-        }
     }
 
     // Get challenge for sorted list batching and wire four memory records

barretenberg/cpp/src/barretenberg/stdlib/honk_recursion/transcript/transcript.test.cpp

@@ -179,4 +179,79 @@ TEST(RecursiveHonkTranscript, ReturnValuesMatch)
     EXPECT_EQ(static_cast<FF>(native_alpha), stdlib_alpha.get_value());
     EXPECT_EQ(static_cast<FF>(native_beta), stdlib_beta.get_value());
 }
+
+/**
+ * @brief Ensure that when encountering an infinity commitment results stay consistent in the recursive and native case
+ * for Grumpkin and the native and stdlib transcripts produce the same challenge.
+ * @todo(https://github.com/AztecProtocol/barretenberg/issues/1064)  Add more transcript tests for both curves
+ */
+TEST(RecursiveTranscript, InfinityConsistencyGrumpkin)
+{
+    using NativeCurve = curve::Grumpkin;
+    using NativeCommitment = typename NativeCurve::AffineElement;
+    using NativeFF = NativeCurve::ScalarField;
+
+    using FF = bigfield<Builder, bb::Bn254FqParams>;
+    using Commitment = cycle_group<Builder>;
+
+    Builder builder;
+
+    NativeCommitment infinity = NativeCommitment::infinity();
+
+    NativeTranscript prover_transcript;
+    prover_transcript.send_to_verifier("infinity", infinity);
+    NativeFF challenge = prover_transcript.get_challenge<NativeFF>("challenge");
+    auto proof_data = prover_transcript.proof_data;
+
+    NativeTranscript verifier_transcript(proof_data);
+    verifier_transcript.receive_from_prover<NativeCommitment>("infinity");
+    auto verifier_challenge = verifier_transcript.get_challenge<NativeFF>("challenge");
+
+    StdlibProof<Builder> stdlib_proof = bb::convert_proof_to_witness(&builder, proof_data);
+    StdlibTranscript stdlib_transcript{ stdlib_proof };
+    auto stdlib_infinity = stdlib_transcript.receive_from_prover<Commitment>("infinity");
+    EXPECT_TRUE(stdlib_infinity.is_point_at_infinity().get_value());
+    auto stdlib_challenge = stdlib_transcript.get_challenge<FF>("challenge");
+
+    EXPECT_EQ(challenge, verifier_challenge);
+    EXPECT_EQ(verifier_challenge, NativeFF(stdlib_challenge.get_value() % FF::modulus));
+}
+
+/**
+ * @brief Ensure that when encountering an infinity commitment results stay consistent in the recursive and native case
+ * for BN254 and the native and stdlib transcripts produce the same challenge.
+ * @todo(https://github.com/AztecProtocol/barretenberg/issues/1064)  Add more transcript tests for both curves
+ */
+TEST(RecursiveTranscript, InfinityConsistencyBN254)
+{
+    using NativeCurve = curve::BN254;
+    using NativeCommitment = typename NativeCurve::AffineElement;
+    using NativeFF = NativeCurve::ScalarField;
+
+    using FF = field_t<Builder>;
+    using BF = bigfield<Builder, bb::Bn254FqParams>;
+    using Commitment = element<Builder, BF, FF, bb::g1>;
+
+    Builder builder;
+
+    NativeCommitment infinity = NativeCommitment::infinity();
+
+    NativeTranscript prover_transcript;
+    prover_transcript.send_to_verifier("infinity", infinity);
+    NativeFF challenge = prover_transcript.get_challenge<NativeFF>("challenge");
+    auto proof_data = prover_transcript.proof_data;
+
+    NativeTranscript verifier_transcript(proof_data);
+    verifier_transcript.receive_from_prover<NativeCommitment>("infinity");
+    auto verifier_challenge = verifier_transcript.get_challenge<NativeFF>("challenge");
+
+    StdlibProof<Builder> stdlib_proof = bb::convert_proof_to_witness(&builder, proof_data);
+    StdlibTranscript stdlib_transcript{ stdlib_proof };
+    auto stdlib_commitment = stdlib_transcript.receive_from_prover<Commitment>("infinity");
+    EXPECT_TRUE(stdlib_commitment.is_point_at_infinity().get_value());
+    auto stdlib_challenge = stdlib_transcript.get_challenge<FF>("challenge");
+
+    EXPECT_EQ(challenge, verifier_challenge);
+    EXPECT_EQ(verifier_challenge, stdlib_challenge.get_value());
+}
 } // namespace bb::stdlib::recursion::honk

barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field_conversion.hpp

@@ -70,6 +70,8 @@ template <typename Builder, typename T> constexpr size_t calc_num_bn254_frs()
  * @param builder
  * @param fr_vec
  * @return T
+ * @todo https://github.com/AztecProtocol/barretenberg/issues/1065  optimise validate_on_curve and check points
+ * reconstructed from the transcript
  */
 template <typename Builder, typename T> T convert_from_bn254_frs(Builder& builder, std::span<const fr<Builder>> fr_vec)
 {
@@ -78,7 +80,7 @@ template <typename Builder, typename T> T convert_from_bn254_frs(Builder& builde
         return fr_vec[0];
     } else if constexpr (IsAnyOf<T, fq<Builder>>) {
         ASSERT(fr_vec.size() == 2);
-        fq<Builder> result(fr_vec[0], fr_vec[1], 0, 0);
+        fq<Builder> result(fr_vec[0], fr_vec[1]);
         return result;
     } else if constexpr (IsAnyOf<T, bn254_element<Builder>>) {
         using BaseField = fq<Builder>;
@@ -88,16 +90,19 @@ template <typename Builder, typename T> T convert_from_bn254_frs(Builder& builde
         result.x = convert_from_bn254_frs<Builder, BaseField>(builder, fr_vec.subspan(0, BASE_FIELD_SCALAR_SIZE));
         result.y = convert_from_bn254_frs<Builder, BaseField>(
             builder, fr_vec.subspan(BASE_FIELD_SCALAR_SIZE, BASE_FIELD_SCALAR_SIZE));
+
+        result.set_point_at_infinity(fr_vec[0].is_zero() && fr_vec[1].is_zero() && fr_vec[2].is_zero() &&
+                                     fr_vec[3].is_zero());
         return result;
     } else if constexpr (IsAnyOf<T, grumpkin_element<Builder>>) {
         using BaseField = fr<Builder>;
         constexpr size_t BASE_FIELD_SCALAR_SIZE = calc_num_bn254_frs<Builder, BaseField>();
         ASSERT(fr_vec.size() == 2 * BASE_FIELD_SCALAR_SIZE);
-        grumpkin_element<Builder> result(
-            convert_from_bn254_frs<Builder, fr<Builder>>(builder, fr_vec.subspan(0, BASE_FIELD_SCALAR_SIZE)),
-            convert_from_bn254_frs<Builder, fr<Builder>>(
-                builder, fr_vec.subspan(BASE_FIELD_SCALAR_SIZE, BASE_FIELD_SCALAR_SIZE)),
-            false);
+        fr<Builder> x =
+            convert_from_bn254_frs<Builder, fr<Builder>>(builder, fr_vec.subspan(0, BASE_FIELD_SCALAR_SIZE));
+        fr<Builder> y = convert_from_bn254_frs<Builder, fr<Builder>>(
+            builder, fr_vec.subspan(BASE_FIELD_SCALAR_SIZE, BASE_FIELD_SCALAR_SIZE));
+        grumpkin_element<Builder> result(x, y, x.is_zero() && y.is_zero());
         return result;
     } else {
         // Array or Univariate

barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.cpp

@@ -38,6 +38,11 @@ cycle_group<Builder>::cycle_group(field_t _x, field_t _y, bool_t is_infinity)
     } else {
         context = is_infinity.get_context();
     }
+
+    // TODO(https://github.com/AztecProtocol/barretenberg/issues/1067): This ASSERT is missing in the constructor but
+    // causes schnorr acir test to fail due to a bad input (a public key that has x and y coordinate set to 0).
+    // Investigate this to be able to enable the test.
+    // ASSERT(get_value().on_curve());
 }
 
 /**