feat: derive inf from coordinates in exec, some docs

Author: MirandaWood

barretenberg/cpp/pil/vm2/bytecode/address_derivation.pil

@@ -186,7 +186,7 @@ namespace address_derivation;
 
     // TODO(#AVM-266): Remove infinity flags from point representation. Note that we may still need to use
     // precomputed.zero in the hash preimages until address derivation removes them:
-    // TODO(#7529): Remove all the 0s for is_infinity when removed from public_keys.nr
+    // TODO(#7529)/TODO(F-553): Remove all the 0s for is_infinity when removed from public_keys.nr
     // https://github.com/AztecProtocol/aztec-packages/issues/7529
     // TODO(#14031): Compress keys in public_keys_hash
     // https://github.com/AztecProtocol/aztec-packages/issues/14031

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

@@ -80,6 +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.
     constexpr const BaseField& x() const noexcept { return x_coord; }
 
     constexpr const BaseField& y() const noexcept { return y_coord; }

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

@@ -1,3 +1,4 @@
+#include <cstdio>
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
@@ -1388,29 +1389,43 @@ TEST(EccAddMemoryConstrainingTest, InfinityRepresentations)
     check_relation<ecc>(trace);
     EXPECT_EQ(trace.get(C::ecc_double_op, 0), 1);
 
+    ecc_simulator.add(memory, inf, inf_bb, dst_address);
+
+    // 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 + 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 + 2 },
+                  { 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 derived is_inf column must be true if the coordinates are (0, 0):
+    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):
+    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");
+    EXPECT_THROW_WITH_MESSAGE(check_relation<mem_aware_ecc>(trace, mem_aware_ecc::SR_Q_INF_Y_CHECK), "Q_INF_Y_CHECK");
+
     // TODO(#AVM-266): Rework test when is_infinity flag is removed from point representations.
     // We now derive is_inf from coordinates, whereas previously we remapped coordinates /from/ is_inf.
     // The below test no longer makes sense since it checks we store non-(0,0) coordinates for an inf
     // point, which we do not allow.
 
-    // // 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 + 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 + 2 },
-    //               { 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...
@@ -1422,7 +1437,8 @@ TEST(EccAddMemoryConstrainingTest, InfinityRepresentations)
     // 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);
+    // check_interaction<tracegen::ExecutionTraceBuilder,
+    // bb::avm2::perm_execution_dispatch_to_ecc_add_settings>(trace);
 }
 
 } // namespace

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

@@ -136,11 +136,10 @@ 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.
         if (!p.on_curve() || !q.on_curve()) {
-            // TODO(#AVM-266): Note: We now use this to enforce (X, Y) == (0, 0) ==> is_inf
-            // until is_inf is removed. This means a bb inf point (!= (0, 0)) will
-            // not throw here but would throw in the circuit. Since we remap such points to
-            // (0, 0) below, it shouldn't reach the circuit at all.
             throw InternalEccException("One of the points is not on the curve");
         }
 

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

@@ -1505,19 +1505,26 @@ void Execution::ecc_add(ContextInterface& context,
     // Read the points from memory.
     const auto& p_x = memory.get(p_x_addr);
     const auto& p_y = memory.get(p_y_addr);
+    // TODO(#AVM-266): Remove infinity flags from point representation, the below is currently ignored in-circuit.
     const auto& p_inf = memory.get(p_inf_addr);
 
     const auto& q_x = memory.get(q_x_addr);
     const auto& q_y = memory.get(q_y_addr);
+    // TODO(#AVM-266): Remove infinity flags from point representation, the below is currently ignored in-circuit.
     const auto& q_inf = memory.get(q_inf_addr);
 
     set_and_validate_inputs(opcode, { p_x, p_y, p_inf, q_x, q_y, q_inf });
     get_gas_tracker().consume_gas();
 
     // Once inputs are tag checked the conversion to EmbeddedCurvePoint is safe, on curve checks are done in the add
-    // method.
-    EmbeddedCurvePoint p = EmbeddedCurvePoint(p_x.as_ff(), p_y.as_ff(), p_inf == MemoryValue::from<uint1_t>(1));
-    EmbeddedCurvePoint q = EmbeddedCurvePoint(q_x.as_ff(), q_y.as_ff(), q_inf == MemoryValue::from<uint1_t>(1));
+    // method. TODO(#AVM-266): We derive is_infinity from coordinates using the Noir convention of (x=0, y=0) <==>
+    // is_infinity. The flag will be removed in future.
+    const FF p_x_ff = p_x.as_ff();
+    const FF p_y_ff = p_y.as_ff();
+    EmbeddedCurvePoint p = EmbeddedCurvePoint(p_x_ff, p_y_ff, (p_x_ff == FF::zero()) && (p_y_ff == FF::zero()));
+    const FF q_x_ff = q_x.as_ff();
+    const FF q_y_ff = q_y.as_ff();
+    EmbeddedCurvePoint q = EmbeddedCurvePoint(q_x_ff, q_y_ff, (q_x_ff == FF::zero()) && (q_y_ff == FF::zero()));
 
     try {
         embedded_curve.add(memory, p, q, dst_addr);