chore(avm): ECC pre-audit - normalise infinity points

barretenberg/cpp/pil/vm2/ecc.pil

@@ -22,6 +22,7 @@ namespace ecc;
     add_op * (1 - add_op) = 0;
 
     // When the sel is on, we are either doubling, adding or handling the edge case
+    #[OP_CHECK]
     sel = double_op + add_op + INFINITY_PRED;
 
     // Point P in affine form

barretenberg/cpp/pil/vm2/ecc_mem.pil

@@ -47,7 +47,9 @@ include "gt.pil";
  * N.B This subtrace writes the values within a single row (i.e. 3 output columns)
  *
  * This subtrace is connected to the ECC subtrace via a lookup. ECC is used by
- * other subtraces internally (e.g., address derivation)
+ * other subtraces internally (e.g., address derivation). We re-map any input infinity points to (0, 0)
+ * so ECC correctly manages edge cases. Resulting infinity points are guaranteed to be (0, 0) by the
+ * ECC subtrace (see #[OUTPUT_X_COORD] and #[OUTPUT_Y_COORD]).
  */
 
 namespace ecc_add_mem;
@@ -127,10 +129,22 @@ namespace ecc_add_mem;
     pol commit sel_should_exec; // @boolean (by definition)
     sel_should_exec = sel * (1 - err);
 
+    // Needs to be committed columns as they are used in the lookups
+    pol commit p_x_n, p_y_n;
+    pol commit q_x_n, q_y_n;
+
+    // We re-map input infinity points to (0, 0) for ecc.pil. Output infinities are already constrained to be (0, 0) in the subtrace.
+    // Note that we cannot use p_x, p_y, etc. because they are read from memory in execution's #[DISPATCH_TO_ECC_ADD].
+    sel_should_exec * (p_x_n - (1 - p_is_inf) * p_x - p_is_inf * ecc.INFINITY_X) = 0;
+    sel_should_exec * (p_y_n - (1 - p_is_inf) * p_y - p_is_inf * ecc.INFINITY_Y) = 0;
+    sel_should_exec * (q_x_n - (1 - q_is_inf) * q_x - q_is_inf * ecc.INFINITY_X) = 0;
+    sel_should_exec * (q_y_n - (1 - q_is_inf) * q_y - q_is_inf * ecc.INFINITY_Y) = 0;
+
+
     #[INPUT_OUTPUT_ECC_ADD]
     sel_should_exec {
-        p_x, p_y, p_is_inf,
-        q_x, q_y, q_is_inf,
+        p_x_n, p_y_n, p_is_inf,
+        q_x_n, q_y_n, q_is_inf,
         res_x, res_y, res_is_inf
     } in
     ecc.sel {

barretenberg/cpp/src/barretenberg/avm_fuzzer/harness/ecc.fuzzer.cpp

@@ -12,7 +12,6 @@
 #include "barretenberg/vm2/common/aztec_types.hpp"
 #include "barretenberg/vm2/common/field.hpp"
 #include "barretenberg/vm2/common/memory_types.hpp"
-#include "barretenberg/vm2/common/standard_affine_point.hpp"
 #include "barretenberg/vm2/constraining/testing/check_relation.hpp"
 #include "barretenberg/vm2/generated/columns.hpp"
 #include "barretenberg/vm2/generated/relations/scalar_mul.hpp"
@@ -41,7 +40,6 @@ using namespace bb::avm2::tracegen;
 using namespace bb::avm2::constraining;
 
 using avm2::AffinePoint;
-using StandardAffinePoint = avm2::StandardAffinePoint<AffinePoint>;
 using bb::avm2::EmbeddedCurvePoint;
 using bb::avm2::FF;
 using bb::avm2::MemoryAddress;
@@ -227,10 +225,8 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size)
     const EccFuzzerInput input = EccFuzzerInput::from_buffer(data);
     bool error = false;
 
-    EmbeddedCurvePoint point_p =
-        input.p.is_point_at_infinity() ? EmbeddedCurvePoint::infinity() : EmbeddedCurvePoint(input.p);
-    EmbeddedCurvePoint point_q =
-        input.q.is_point_at_infinity() ? EmbeddedCurvePoint::infinity() : EmbeddedCurvePoint(input.q);
+    EmbeddedCurvePoint point_p = EmbeddedCurvePoint(input.p);
+    EmbeddedCurvePoint point_q = EmbeddedCurvePoint(input.q);
 
     // Set up gadgets and event emitters
     DeduplicatingEventEmitter<RangeCheckEvent> range_check_emitter;
@@ -270,8 +266,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size)
         error = true;
     }
     if (!error) {
-        // StandardAffinePoint, unlike AffinePoint, normalises infinity to (0, 0) to match EmbeddedCurvePoint
-        StandardAffinePoint expected_result = StandardAffinePoint(input.p) + StandardAffinePoint(input.q);
+        EmbeddedCurvePoint expected_result = point_p + point_q;
 
         // Verify output in memory
         MemoryValue res_x = mem->get(input.addresses[6]);
@@ -285,7 +280,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size)
         BB_ASSERT(result_point.is_infinity() == expected_result.is_infinity(), "Result infinity flag mismatch");
 
         // Non mem-aware ecmul result:
-        expected_result = StandardAffinePoint(input.p) * input.scalar;
+        expected_result = point_p * input.scalar;
 
         BB_ASSERT(scalar_mul_result.x() == expected_result.x(), "Mul result x-coordinate mismatch");
         BB_ASSERT(scalar_mul_result.y() == expected_result.y(), "Mul result y-coordinate mismatch");

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

@@ -11,6 +11,11 @@ namespace bb::avm2 {
  * 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.
  */
 template <typename AffinePoint> class StandardAffinePoint {
   public:

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

@@ -24,6 +24,7 @@
 #include "barretenberg/vm2/testing/fixtures.hpp"
 #include "barretenberg/vm2/testing/macros.hpp"
 #include "barretenberg/vm2/tracegen/ecc_trace.hpp"
+#include "barretenberg/vm2/tracegen/execution_trace.hpp"
 #include "barretenberg/vm2/tracegen/test_trace_container.hpp"
 #include "barretenberg/vm2/tracegen/to_radix_trace.hpp"
 
@@ -796,6 +797,54 @@ TEST(ScalarMulConstrainingTest, MulAddInteractionsInfinity)
     check_relation<ecc>(trace);
 }
 
+TEST(ScalarMulConstrainingTest, MulAddInteractionsInfinityRep)
+{
+    EccTraceBuilder builder;
+
+    EventEmitter<EccAddEvent> ecc_add_event_emitter;
+    EventEmitter<ScalarMulEvent> scalar_mul_event_emitter;
+    NoopEventEmitter<EccAddMemoryEvent> ecc_add_memory_event_emitter;
+
+    StrictMock<MockExecutionIdManager> execution_id_manager;
+    StrictMock<MockGreaterThan> gt;
+    PureToRadix to_radix_simulator = PureToRadix();
+    EccSimulator ecc_simulator(execution_id_manager,
+                               gt,
+                               to_radix_simulator,
+                               ecc_add_event_emitter,
+                               scalar_mul_event_emitter,
+                               ecc_add_memory_event_emitter);
+
+    EmbeddedCurvePoint inf = EmbeddedCurvePoint::infinity();
+    // EmbeddedCurvePoint preserves raw coordinates (see StandardAffinePointTest)
+    EmbeddedCurvePoint inf_bb = EmbeddedCurvePoint(avm2::AffinePoint::infinity());
+    EmbeddedCurvePoint inf_alt = EmbeddedCurvePoint(1, 2, true);
+
+    EmbeddedCurvePoint result = ecc_simulator.scalar_mul(inf_bb, FF(10));
+    ASSERT_TRUE(result.is_infinity());
+    result = ecc_simulator.scalar_mul(inf_alt, FF(10));
+    ASSERT_TRUE(result.is_infinity());
+
+    TestTraceContainer trace({
+        { { C::precomputed_first_row, 1 } },
+    });
+
+    auto scalar_mul_events = scalar_mul_event_emitter.dump_events();
+    // Infinity points should be normalised to (0, 0) for any lookups into ecc.pil
+    for (auto& event : scalar_mul_events) {
+        EXPECT_EQ(event.point.x(), inf.x());
+        EXPECT_EQ(event.point.y(), inf.y());
+    }
+
+    builder.process_scalar_mul(scalar_mul_events, trace);
+    builder.process_add(ecc_add_event_emitter.dump_events(), trace);
+
+    check_interaction<EccTraceBuilder, lookup_scalar_mul_double_settings, lookup_scalar_mul_add_settings>(trace);
+
+    check_relation<scalar_mul>(trace);
+    check_relation<ecc>(trace);
+}
+
 TEST(ScalarMulConstrainingTest, NegativeMulAddInteractions)
 {
     EccTraceBuilder builder;
@@ -1295,5 +1344,95 @@ TEST(EccAddMemoryConstrainingTest, EccAddMemoryPointError)
     check_relation<mem_aware_ecc>(trace);
 }
 
+TEST(EccAddMemoryConstrainingTest, InfinityRepresentations)
+{
+    EccTraceBuilder builder;
+    MemoryStore memory;
+
+    EventEmitter<EccAddEvent> ecc_add_event_emitter;
+    EventEmitter<ScalarMulEvent> scalar_mul_event_emitter;
+    EventEmitter<EccAddMemoryEvent> ecc_add_memory_event_emitter;
+
+    StrictMock<MockExecutionIdManager> execution_id_manager;
+    EXPECT_CALL(execution_id_manager, get_execution_id)
+        .WillRepeatedly(Return(0)); // Use a fixed execution ID for the test
+    PureGreaterThan gt;
+    PureToRadix to_radix_simulator = PureToRadix();
+    EccSimulator ecc_simulator(execution_id_manager,
+                               gt,
+                               to_radix_simulator,
+                               ecc_add_event_emitter,
+                               scalar_mul_event_emitter,
+                               ecc_add_memory_event_emitter);
+    MemoryAddress dst_address = 5;
+
+    // Point P is infinity
+    EmbeddedCurvePoint inf = EmbeddedCurvePoint::infinity();
+    // EmbeddedCurvePoint preserves raw coordinates (see StandardAffinePointTest)
+    EmbeddedCurvePoint inf_bb = EmbeddedCurvePoint(avm2::AffinePoint::infinity());
+    EmbeddedCurvePoint inf_alt = EmbeddedCurvePoint(1, 2, true);
+    TestTraceContainer trace;
+
+    // Internal add() expects normalized points:
+    EXPECT_THROW_WITH_MESSAGE(ecc_simulator.add(inf, inf_alt), "normalized");
+
+    // Coordinates are normalized in tracegen, so even though inf_bb and inf_alt have different coordinates, the circuit
+    // correctly assigns double_op = true when doubling inf:
+    ecc_simulator.add(memory, inf, inf_alt, dst_address);
+    // As above for the noir (0, 0) and bb (x, 0) inf reps:
+    ecc_simulator.add(memory, inf, inf_bb, dst_address + 3);
+
+    builder.process_add(ecc_add_event_emitter.dump_events(), trace);
+    check_relation<ecc>(trace);
+    EXPECT_EQ(trace.get(C::ecc_double_op, 0), 1);
+
+    // Set memory reads:
+    trace.set(0,
+              { { // Execution
+                  { C::execution_sel, 1 },
+                  { C::execution_sel_exec_dispatch_ecc_add, 1 },
+                  { C::execution_rop_6_, dst_address },
+                  { C::execution_register_0_, inf.x() },
+                  { C::execution_register_1_, inf.y() },
+                  { C::execution_register_2_, inf.is_infinity() ? 1 : 0 },
+                  { C::execution_register_3_, inf_alt.x() },
+                  { C::execution_register_4_, inf_alt.y() },
+                  { C::execution_register_5_, inf_alt.is_infinity() ? 1 : 0 },
+                  // GT - dst out of range check
+                  { C::gt_sel, 1 },
+                  { C::gt_input_a, dst_address + 2 }, // highest write address is dst_address + 2
+                  { C::gt_input_b, AVM_HIGHEST_MEM_ADDRESS },
+                  { C::gt_res, 0 } } });
+    trace.set(1,
+              { { // Execution
+                  { C::execution_sel, 1 },
+                  { C::execution_sel_exec_dispatch_ecc_add, 1 },
+                  { C::execution_rop_6_, dst_address + 3 },
+                  { C::execution_register_0_, inf.x() },
+                  { C::execution_register_1_, inf.y() },
+                  { C::execution_register_2_, inf.is_infinity() ? 1 : 0 },
+                  { C::execution_register_3_, inf_bb.x() },
+                  { C::execution_register_4_, inf_bb.y() },
+                  { C::execution_register_5_, inf_bb.is_infinity() ? 1 : 0 },
+                  // GT - dst out of range check
+                  { C::gt_sel, 1 },
+                  { C::gt_input_a, dst_address + 5 },
+                  { C::gt_input_b, AVM_HIGHEST_MEM_ADDRESS },
+                  { C::gt_res, 0 } } });
+
+    builder.process_add_with_memory(ecc_add_memory_event_emitter.dump_events(), trace);
+
+    // The original coordinates are stored in memory for the read...
+    EXPECT_EQ(trace.get(C::ecc_add_mem_q_x, 1), inf_bb.x());
+    EXPECT_EQ(trace.get(C::ecc_add_mem_q_y, 1), inf_bb.y());
+    // ...but normalised coordinates are sent to the ecc subtrace:
+    EXPECT_EQ(trace.get(C::ecc_add_mem_q_x_n, 1), 0);
+    EXPECT_EQ(trace.get(C::ecc_add_mem_q_y_n, 1), 0);
+    check_relation<mem_aware_ecc>(trace);
+    check_relation<ecc>(trace);
+    check_all_interactions<EccTraceBuilder>(trace);
+    check_interaction<tracegen::ExecutionTraceBuilder, bb::avm2::perm_execution_dispatch_to_ecc_add_settings>(trace);
+}
+
 } // namespace
 } // namespace bb::avm2::constraining

barretenberg/cpp/src/barretenberg/vm2/generated/flavor_variables.hpp

@@ -145,10 +145,10 @@ namespace bb::avm2 {
 
 struct AvmFlavorVariables {
     static constexpr size_t NUM_PRECOMPUTED_ENTITIES = 123;
-    static constexpr size_t NUM_WITNESS_ENTITIES = 3059;
+    static constexpr size_t NUM_WITNESS_ENTITIES = 3063;
     static constexpr size_t NUM_SHIFTED_ENTITIES = 345;
-    static constexpr size_t NUM_WIRES = 2593;
-    static constexpr size_t NUM_ALL_ENTITIES = 3527;
+    static constexpr size_t NUM_WIRES = 2597;
+    static constexpr size_t NUM_ALL_ENTITIES = 3531;
 
     // Need to be templated for recursive verifier
     template <typename FF_>

barretenberg/cpp/src/barretenberg/vm2/generated/relations/ecc.hpp

@@ -36,6 +36,7 @@ template <typename FF> class ecc : public Relation<eccImpl<FF>> {
     static constexpr const std::string_view NAME = "ecc";
 
     // Subrelation indices constants, to be used in tests.
+    static constexpr size_t SR_OP_CHECK = 3;
     static constexpr size_t SR_DOUBLE_PRED = 11;
     static constexpr size_t SR_INFINITY_RESULT = 12;
     static constexpr size_t SR_COMPUTED_LAMBDA = 14;
@@ -46,6 +47,8 @@ template <typename FF> class ecc : public Relation<eccImpl<FF>> {
     static std::string get_subrelation_label(size_t index)
     {
         switch (index) {
+        case SR_OP_CHECK:
+            return "OP_CHECK";
         case SR_DOUBLE_PRED:
             return "DOUBLE_PRED";
         case SR_INFINITY_RESULT:

barretenberg/cpp/src/barretenberg/vm2/generated/relations/ecc_impl.hpp

@@ -43,7 +43,7 @@ void eccImpl<FF_>::accumulate(ContainerOverSubrelations& evals,
         auto tmp = static_cast<View>(in.get(C::ecc_add_op)) * (FF(1) - static_cast<View>(in.get(C::ecc_add_op)));
         std::get<2>(evals) += (tmp * scaling_factor);
     }
-    {
+    { // OP_CHECK
         using View = typename std::tuple_element_t<3, ContainerOverSubrelations>::View;
         auto tmp = (static_cast<View>(in.get(C::ecc_sel)) -
                     (static_cast<View>(in.get(C::ecc_double_op)) + static_cast<View>(in.get(C::ecc_add_op)) +

barretenberg/cpp/src/barretenberg/vm2/generated/relations/ecc_mem.hpp

@@ -14,7 +14,8 @@ template <typename FF_> class ecc_memImpl {
   public:
     using FF = FF_;
 
-    static constexpr std::array<size_t, 12> SUBRELATION_PARTIAL_LENGTHS = { 3, 3, 3, 3, 3, 3, 6, 5, 6, 5, 4, 3 };
+    static constexpr std::array<size_t, 16> SUBRELATION_PARTIAL_LENGTHS = { 3, 3, 3, 3, 3, 3, 6, 5,
+                                                                            6, 5, 4, 3, 4, 4, 4, 4 };
 
     template <typename AllEntities> inline static bool skip(const AllEntities& in)
     {

barretenberg/cpp/src/barretenberg/vm2/generated/relations/ecc_mem_impl.hpp

@@ -16,6 +16,8 @@ void ecc_memImpl<FF_>::accumulate(ContainerOverSubrelations& evals,
     using C = ColumnAndShifts;
 
     const auto constants_AVM_HIGHEST_MEM_ADDRESS = FF(4294967295UL);
+    const auto ecc_INFINITY_X = FF(0);
+    const auto ecc_INFINITY_Y = FF(0);
     const auto ecc_add_mem_P_X3 = in.get(C::ecc_add_mem_p_x) * in.get(C::ecc_add_mem_p_x) * in.get(C::ecc_add_mem_p_x);
     const auto ecc_add_mem_P_Y2 = in.get(C::ecc_add_mem_p_y) * in.get(C::ecc_add_mem_p_y);
     const auto ecc_add_mem_Q_X3 = in.get(C::ecc_add_mem_q_x) * in.get(C::ecc_add_mem_q_x) * in.get(C::ecc_add_mem_q_x);
@@ -110,6 +112,42 @@ void ecc_memImpl<FF_>::accumulate(ContainerOverSubrelations& evals,
              static_cast<View>(in.get(C::ecc_add_mem_sel)) * (FF(1) - static_cast<View>(in.get(C::ecc_add_mem_err))));
         std::get<11>(evals) += (tmp * scaling_factor);
     }
+    {
+        using View = typename std::tuple_element_t<12, ContainerOverSubrelations>::View;
+        auto tmp = static_cast<View>(in.get(C::ecc_add_mem_sel_should_exec)) *
+                   ((static_cast<View>(in.get(C::ecc_add_mem_p_x_n)) -
+                     (FF(1) - static_cast<View>(in.get(C::ecc_add_mem_p_is_inf))) *
+                         static_cast<View>(in.get(C::ecc_add_mem_p_x))) -
+                    static_cast<View>(in.get(C::ecc_add_mem_p_is_inf)) * CView(ecc_INFINITY_X));
+        std::get<12>(evals) += (tmp * scaling_factor);
+    }
+    {
+        using View = typename std::tuple_element_t<13, ContainerOverSubrelations>::View;
+        auto tmp = static_cast<View>(in.get(C::ecc_add_mem_sel_should_exec)) *
+                   ((static_cast<View>(in.get(C::ecc_add_mem_p_y_n)) -
+                     (FF(1) - static_cast<View>(in.get(C::ecc_add_mem_p_is_inf))) *
+                         static_cast<View>(in.get(C::ecc_add_mem_p_y))) -
+                    static_cast<View>(in.get(C::ecc_add_mem_p_is_inf)) * CView(ecc_INFINITY_Y));
+        std::get<13>(evals) += (tmp * scaling_factor);
+    }
+    {
+        using View = typename std::tuple_element_t<14, ContainerOverSubrelations>::View;
+        auto tmp = static_cast<View>(in.get(C::ecc_add_mem_sel_should_exec)) *
+                   ((static_cast<View>(in.get(C::ecc_add_mem_q_x_n)) -
+                     (FF(1) - static_cast<View>(in.get(C::ecc_add_mem_q_is_inf))) *
+                         static_cast<View>(in.get(C::ecc_add_mem_q_x))) -
+                    static_cast<View>(in.get(C::ecc_add_mem_q_is_inf)) * CView(ecc_INFINITY_X));
+        std::get<14>(evals) += (tmp * scaling_factor);
+    }
+    {
+        using View = typename std::tuple_element_t<15, ContainerOverSubrelations>::View;
+        auto tmp = static_cast<View>(in.get(C::ecc_add_mem_sel_should_exec)) *
+                   ((static_cast<View>(in.get(C::ecc_add_mem_q_y_n)) -
+                     (FF(1) - static_cast<View>(in.get(C::ecc_add_mem_q_is_inf))) *
+                         static_cast<View>(in.get(C::ecc_add_mem_q_y))) -
+                    static_cast<View>(in.get(C::ecc_add_mem_q_is_inf)) * CView(ecc_INFINITY_Y));
+        std::get<15>(evals) += (tmp * scaling_factor);
+    }
 }
 
 } // namespace bb::avm2

barretenberg/cpp/src/barretenberg/vm2/generated/relations/lookups_ecc_mem.hpp

@@ -48,8 +48,8 @@ struct lookup_ecc_mem_input_output_ecc_add_settings_ {
     static constexpr Column COUNTS = Column::lookup_ecc_mem_input_output_ecc_add_counts;
     static constexpr Column INVERSES = Column::lookup_ecc_mem_input_output_ecc_add_inv;
     static constexpr std::array<ColumnAndShifts, LOOKUP_TUPLE_SIZE> SRC_COLUMNS = {
-        ColumnAndShifts::ecc_add_mem_p_x,   ColumnAndShifts::ecc_add_mem_p_y,   ColumnAndShifts::ecc_add_mem_p_is_inf,
-        ColumnAndShifts::ecc_add_mem_q_x,   ColumnAndShifts::ecc_add_mem_q_y,   ColumnAndShifts::ecc_add_mem_q_is_inf,
+        ColumnAndShifts::ecc_add_mem_p_x_n, ColumnAndShifts::ecc_add_mem_p_y_n, ColumnAndShifts::ecc_add_mem_p_is_inf,
+        ColumnAndShifts::ecc_add_mem_q_x_n, ColumnAndShifts::ecc_add_mem_q_y_n, ColumnAndShifts::ecc_add_mem_q_is_inf,
         ColumnAndShifts::ecc_add_mem_res_x, ColumnAndShifts::ecc_add_mem_res_y, ColumnAndShifts::ecc_add_mem_res_is_inf
     };
     static constexpr std::array<ColumnAndShifts, LOOKUP_TUPLE_SIZE> DST_COLUMNS = {