chore: sanitise inf = (0,0) rep in StandardAffinePoint class

Author: MirandaWood

barretenberg/cpp/src/barretenberg/vm2/common/standard_affine_point.hpp

@@ -9,17 +9,15 @@ namespace bb::avm2 {
  * AVM bytecode expects the representation of points to be triplets, the two coordinates and an is_infinity boolean.
  * Furthermore, its representation of infinity is inherited from noir's and is expected to be 0,0,true.
  * BB, however, uses only the two coordinates to represent points. Infinity in barretenberg is represented as (P+1)/2,0.
- * This class is a wrapper of the BB representation, needed to operate with points, that allows to extract the standard
- * representation that AVM bytecode expects.
- * NOTE: When constructing infinity from BB's two element representation, is_infinity() will be true but the coordinates
- * will remain (P+1)/2,0.
- * NOTE: When constructing infinity via BaseFields, input coordinates are maintained and can be any values, so may
- * mismatch the underlying AffinePoint. Always check is_infinity() before ECC operations on coordinates. See test
- * InfinityPreservesRawCoordinates for an example.
+ * This class is a wrapper of the BB representation, needed to operate with points, that allows us to extract the
+ * standard representation that AVM bytecode expects.
+ *
+ * NOTE: When constructing infinity from BB's two element representation, we keep the original AffinePoint so operations
+ * can use it in the background, but set extractable coordinates to be our represention of (0,0).
+ * NOTE: When constructing infinity via BaseFields, input coordinates are overwritten to our representation of (0,0)
+ * if the input is_infinity is true, so will mismatch the underlying AffinePoint's coordinates.
  *
  * TODO(#AVM-266): Remove is_infinity flag from point representation.
- * Now that the is_inf flag has been removed from noir (noir-lang/noir/#11926) we should consider a point to be
- * infinity iff its coordinates are (0, 0). This likely means changing our handling of underlying coordinates below.
  */
 template <typename AffinePoint> class StandardAffinePoint {
   public:
@@ -30,14 +28,16 @@ template <typename AffinePoint> class StandardAffinePoint {
 
     constexpr StandardAffinePoint(AffinePoint val) noexcept
         : point(val)
-        , x_coord(val.x)
-        , y_coord(val.y)
+        , x_coord(val.is_point_at_infinity() ? zero : val.x)
+        , y_coord(val.is_point_at_infinity() ? zero : val.y)
     {}
 
+    // TODO(MW): Do we want to silently discard input x, y if is_infinity = true?
+    // Or, discard is_infinity and set point to AffinePoint::infinity() iff x = y = 0?
     constexpr StandardAffinePoint(BaseField x, BaseField y, bool is_infinity) noexcept
         : point(is_infinity ? AffinePoint::infinity() : AffinePoint(x, y))
-        , x_coord(x)
-        , y_coord(y)
+        , x_coord(is_infinity ? zero : x)
+        , y_coord(is_infinity ? zero : y)
     {}
 
     constexpr StandardAffinePoint operator+(const StandardAffinePoint& other) const noexcept
@@ -80,7 +80,7 @@ template <typename AffinePoint> class StandardAffinePoint {
 
     // Always returns the raw coordinates, when an operation results in infinity these will be (0,0).
     // If a point at infinity is constructed with non-zero coordinates, we likely want to preserve those.
-    // TODO(#AVM-266): Now that Noir uses (0, 0) <==> is_infinite, the above may not be true.
+    // TODO(MW): Clarify - docs here no longer true
     constexpr const BaseField& x() const noexcept { return x_coord; }
 
     constexpr const BaseField& y() const noexcept { return y_coord; }
@@ -98,6 +98,7 @@ template <typename AffinePoint> class StandardAffinePoint {
     }
 
   private:
+    // TODO(MW): Clarify - docs here no longer true
     // The affine point for operations, this will always match the raw coordinates unless the point is infinity.
     // In that case, the point will be set to barretenberg's infinity representation - which is not (0,0).
     AffinePoint point;

barretenberg/cpp/src/barretenberg/vm2/common/standard_affine_point.test.cpp

@@ -10,18 +10,24 @@ using EmbeddedCurvePoint = StandardAffinePoint<grumpkin::g1::affine_element>;
 using Fr = grumpkin::fr;
 using Fq = grumpkin::fq;
 
-TEST(StandardAffinePointTest, InfinityPreservesRawCoordinates)
+TEST(StandardAffinePointTest, InfinityDiscardsRawCoordinates)
 {
+    // NOTE: As of #AVM-248, we moved from preserving raw coordinates in
+    // infinity points to our (0,0) representation when using x() and y().
+    // The underlying AffinePoint is set to AffinePoint::infinity() for
+    // bb operations.
+
     // When constructing an infinity point with non-zero coordinates,
-    // x() and y() should return the raw coordinates
+    // x() and y() should return our standard representation.
     Fq raw_x = 1;
     Fq raw_y = 2;
 
+    // Note that raw x and y are silently discarded.
     EmbeddedCurvePoint point(raw_x, raw_y, /*is_infinity=*/true);
 
     EXPECT_TRUE(point.is_infinity());
-    EXPECT_EQ(point.x(), raw_x);
-    EXPECT_EQ(point.y(), raw_y);
+    EXPECT_TRUE(point.x().is_zero());
+    EXPECT_TRUE(point.y().is_zero());
 }
 
 TEST(StandardAffinePointTest, NormalPointCoordinates)
@@ -80,18 +86,16 @@ TEST(StandardAffinePointTest, StaticInfinityHasZeroCoordinates)
     EXPECT_TRUE(inf.y().is_zero());
 }
 
-TEST(StandardAffinePointTest, NegatingInfinityPreservesRawCoordinates)
+TEST(StandardAffinePointTest, NegatingInfinity)
 {
-    // Negating an infinity point should preserve its raw coordinates
-    Fq raw_x = 1;
-    Fq raw_y = 2;
-    EmbeddedCurvePoint inf(raw_x, raw_y, /*is_infinity=*/true);
+    // Negating an infinity point should return (0,0,true)
+    EmbeddedCurvePoint inf(0, 0, /*is_infinity=*/true);
 
     auto neg_inf = -inf;
 
     EXPECT_TRUE(neg_inf.is_infinity());
-    EXPECT_EQ(neg_inf.x(), raw_x);
-    EXPECT_EQ(neg_inf.y(), raw_y);
+    EXPECT_TRUE(neg_inf.x().is_zero());
+    EXPECT_TRUE(neg_inf.y().is_zero());
 }
 
 } // namespace

barretenberg/cpp/src/barretenberg/vm2/constraining/relations/ecc.test.cpp

@@ -1374,14 +1374,13 @@ TEST(EccAddMemoryConstrainingTest, InfinityRepresentations)
 
     // Point P is infinity
     EmbeddedCurvePoint inf = EmbeddedCurvePoint::infinity();
-    // EmbeddedCurvePoint preserves raw coordinates (see StandardAffinePointTest)
+    // EmbeddedCurvePoint always sets extractable coordinates as (0,0) and the underlying point as
+    // AffinePoint::infinity() for input infinity points.
     EmbeddedCurvePoint inf_bb = EmbeddedCurvePoint(avm2::AffinePoint::infinity());
-    EmbeddedCurvePoint inf_alt = EmbeddedCurvePoint(1, 2, true);
+    EmbeddedCurvePoint inf_alt = EmbeddedCurvePoint(0, 7, true);
+    EXPECT_EQ(inf_bb, inf_alt);
     TestTraceContainer trace;
 
-    // Internal add() expects normalized points:
-    EXPECT_THROW_WITH_MESSAGE(ecc_simulator.add(inf, inf_alt), "normalized");
-
     // The circuit correctly assigns double_op = true when doubling inf:
     ecc_simulator.add(memory, inf, inf_bb, dst_address);
 
@@ -1415,7 +1414,7 @@ TEST(EccAddMemoryConstrainingTest, InfinityRepresentations)
     trace.set(C::ecc_add_mem_p_is_inf, 0, 0);
     EXPECT_THROW_WITH_MESSAGE(check_relation<mem_aware_ecc>(trace, mem_aware_ecc::SR_P_CURVE_EQN), "P_CURVE_EQN");
 
-    // If is_if is set, the coordinates must be (0, 0):
+    // If is_inf is set, the coordinates must be (0, 0):
     trace.set(C::ecc_add_mem_q_x, 0, 1);
     trace.set(C::ecc_add_mem_q_y, 0, 2);
     EXPECT_THROW_WITH_MESSAGE(check_relation<mem_aware_ecc>(trace, mem_aware_ecc::SR_Q_INF_X_CHECK), "Q_INF_X_CHECK");

barretenberg/cpp/src/barretenberg/vm2/simulation/gadgets/ecc.cpp

@@ -18,7 +18,7 @@ class InternalEccException : public std::runtime_error {
  * @brief Adds Grumpkin curve points P and Q and emits an EccAddEvent.
  *  Corresponds to the non-memory aware subtrace ecc.pil.
  *
- * @throws Unexpected exception if points are not on the curve or not normalized.
+ * @throws Unexpected exception if points are not on the curve.
  *  Note: This function assumes that the points p and q are on the curve. You should only use this function internally
  * if you can guarantee this. Otherwise it is called via the opcode ECADD, see the overloaded function Ecc::add (which
  * performs the curve check).
@@ -32,14 +32,6 @@ EmbeddedCurvePoint Ecc::add(const EmbeddedCurvePoint& p, const EmbeddedCurvePoin
     // Check if points are on the curve. These will throw an unexpected exception if they fail.
     BB_ASSERT(p.on_curve(), "Point p is not on the curve");
     BB_ASSERT(q.on_curve(), "Point q is not on the curve");
-    // TODO(#AVM-266): Remove is_infinity flag from point representation.
-    // Check if the points are normalized (infinity points must be (0, 0, true)).
-    if (p.is_infinity()) {
-        BB_ASSERT((p.x() == 0) && (p.y() == 0), "Point p is not normalized");
-    }
-    if (q.is_infinity()) {
-        BB_ASSERT((q.x() == 0) && (q.y() == 0), "Point q is not normalized");
-    }
 
     EmbeddedCurvePoint result = p + q;
     add_events.emit({ .p = p, .q = q, .result = result });
@@ -71,14 +63,11 @@ EmbeddedCurvePoint Ecc::scalar_mul(const EmbeddedCurvePoint& point, const FF& sc
     // Emits ToRadixEvent, see #[TO_RADIX] in scalar_mul.pil.
     auto bits = to_radix.to_le_bits(scalar, 254).first;
 
-    // Normalize input infinity point (infinity points must be (0, 0, true)).
-    EmbeddedCurvePoint point_input = point.is_infinity() ? EmbeddedCurvePoint::infinity() : point;
-
     // First iteration does conditional assignment instead of addition. Note: in circuit we perform reverse aggregation,
     // so the corresponding constraints for below are gated by 'end'.
 
     // See 'Temp Computation' section in scalar_mul.pil.
-    EmbeddedCurvePoint temp = point_input;
+    EmbeddedCurvePoint temp = point;
     bool bit = bits[0];
 
     // See 'Result Computation' section in scalar_mul.pil.
@@ -95,10 +84,8 @@ EmbeddedCurvePoint Ecc::scalar_mul(const EmbeddedCurvePoint& point, const FF& sc
         }
         intermediate_states[i] = { result, temp, bit };
     }
-    scalar_mul_events.emit({ .point = point_input,
-                             .scalar = scalar,
-                             .intermediate_states = std::move(intermediate_states),
-                             .result = result });
+    scalar_mul_events.emit(
+        { .point = point, .scalar = scalar, .intermediate_states = std::move(intermediate_states), .result = result });
     return result;
 }
 
@@ -136,20 +123,14 @@ void Ecc::add(MemoryInterface& memory,
             throw InternalEccException("dst address out of range");
         }
 
-        // TODO(#AVM-266): Note: bb infinity points (is_inf=true with x=(P+1)/2, y=0) pass on_curve() without
-        // throwing here, but would throw in the circuit. We assume here input points follow the Noir convention
-        // of (x=0, y=0) <==> is_infinity.
+        // TODO(#AVM-266): Remove is_infinity flag from point representation. We assume here input
+        // points follow the Noir convention of (x=0, y=0) <==> is_infinity.
         if (!p.on_curve() || !q.on_curve()) {
             throw InternalEccException("One of the points is not on the curve");
         }
 
-        // Normalize input infinity points.
-        EmbeddedCurvePoint p_input = p.is_infinity() ? EmbeddedCurvePoint::infinity() : p;
-        EmbeddedCurvePoint q_input = q.is_infinity() ? EmbeddedCurvePoint::infinity() : q;
-
         // Emits EccAddEvent, see #[INPUT_OUTPUT_ECC_ADD] in ecc_mem.pil.
-        EmbeddedCurvePoint result =
-            add(p_input, q_input); // Cannot throw since we have checked on_curve() and normalized.
+        EmbeddedCurvePoint result = add(p, q); // Cannot throw since we have checked on_curve().
 
         // Emits MemoryEvents, see #[WRITE_MEM_i] for i = 0, 1, 2,  in ecc_mem.pil.
         memory.set(dst_address, MemoryValue::from<FF>(result.x()));

barretenberg/cpp/src/barretenberg/vm2/tracegen/ecc_trace.cpp

@@ -299,14 +299,6 @@ void EccTraceBuilder::process_add_with_memory(
 
         bool error = dst_out_of_range_err || !p_is_on_curve || !q_is_on_curve;
 
-        // TODO(#AVM-266): Remove is_infinity flag from point representation. For now, we derive is_inf by
-        // checking coordinates in-circuit and ignoring the flag read from memory. Below, we do the 'reverse'
-        // and set derive coordinates as (0, 0) if is_inf is true. This allows us to handle bb inf points until
-        // the flag is removed.
-        // Normalized points, ensures that input infinity points are represented by (0, 0) in the ecc subtrace.
-        EmbeddedCurvePoint p_n = event.p.is_infinity() ? EmbeddedCurvePoint::infinity() : event.p;
-        EmbeddedCurvePoint q_n = event.q.is_infinity() ? EmbeddedCurvePoint::infinity() : event.q;
-
         trace.set(
             row,
             { {
@@ -331,14 +323,14 @@ void EccTraceBuilder::process_add_with_memory(
                 { C::ecc_add_mem_dst_addr_1_, dst_addr + 1 },
                 { C::ecc_add_mem_dst_addr_2_, dst_addr + 2 },
                 // Input - Point P
-                { C::ecc_add_mem_p_x, p_n.x() },
-                { C::ecc_add_mem_p_y, p_n.y() },
+                { C::ecc_add_mem_p_x, event.p.x() },
+                { C::ecc_add_mem_p_y, event.p.y() },
                 { C::ecc_add_mem_p_is_inf,
                   event.p.is_infinity() ? 1 : 0 }, // TODO(#AVM-266): If committed, Will be p.x() == 0 && p.y() == 0
                 { C::ecc_add_mem_p_is_inf_, event.p.is_infinity() ? 1 : 0 }, // TODO(#AVM-266): Remove is_infinity flag
                 // Input - Point Q
-                { C::ecc_add_mem_q_x, q_n.x() },
-                { C::ecc_add_mem_q_y, q_n.y() },
+                { C::ecc_add_mem_q_x, event.q.x() },
+                { C::ecc_add_mem_q_y, event.q.y() },
                 { C::ecc_add_mem_q_is_inf,
                   event.q.is_infinity() ? 1 : 0 }, // TODO(#AVM-266): If committed, Will be q.x() == 0 && q.y() == 0
                 { C::ecc_add_mem_q_is_inf_, event.q.is_infinity() ? 1 : 0 }, // TODO(#AVM-266): Remove is_infinity flag