chore: docs, clear out old todos

Author: MirandaWood

barretenberg/cpp/pil/vm2/ecc.pil

@@ -2,16 +2,11 @@
 /**
  * This subtrace supports point addition over the Grumpkin curve.
  * Given two points, P & Q, this trace computes R = P + Q.
- * PRECONDITIONS: The only assumption here is that the inputs P & Q are points on the Grumpkin curve (note that the Point at Infinity = (0, 0) is considered on the curve):
- * Grumpkin Curve Eqn in SW form:  Y^2 = X^3 − 17.
+ * PRECONDITIONS: This trace assumes that the inputs P & Q are points on the Grumpkin curve and infinity points are correctly
+ * flagged with p_is_inf and/or q_is_inf (note that the Point at Infinity = (0, 0) is considered on the curve):
+ *      Grumpkin Curve Eqn in SW form:  Y^2 = X^3 − 17.
  * Note: Grumpkin forms a 2-cycle with BN254, i.e the base field of one is the scalar field of the other and vice-versa.
  *
- *
- * MW NOTE: USING APPROACH 1 BELOW:
- * Note that once TODO(#AVM-266) is complete, is_inf will no longer be part of our point representation and we must either:
- *  - continue to rely on the 'calling' trace to inject a constrained is_inf, or
- *  - derive is_inf (<==> (X, Y) == (INFINITY_X, INFINITY_Y)) within this trace.
- *
  * USAGE: This is a non-memory aware subtrace used to constrain point addition as defined above. Each point can be looked up
  *        by coordinates (lookup as defined in ecc_mem.pil):
  *        #[INPUT_OUTPUT_ECC_ADD]
@@ -29,8 +24,9 @@
  *          ecc.r_x, ecc.r_y // Point R
  *        };
  *
- * MW NOTE: For now, the calling trace MUST constrain that p_is_inf, q_is_inf above are correct. This is so if we have a calling
- *          trace in which we know inf would never be an input we can simply use precomputed.zero and avoid wasting gates on deriving is_inf.
+ * NOTE: For now, the calling trace MUST constrain that p_is_inf, q_is_inf above are correct. This is so if we have a calling
+ *       trace in which we know inf would never be an input we can simply use precomputed.zero and avoid wasting gates on deriving is_inf.
+ *       This follows the same logic for points being on the curve.
  *
  * TRACE SHAPE: 1 single row per computation (P + Q = R).
  *

barretenberg/cpp/pil/vm2/ecc_mem.pil

@@ -4,23 +4,20 @@ include "gt.pil";
 include "memory.pil";
 include "precomputed.pil";
 
-// TODO(#AVM-266): documentation
-
 /**
  * This handles the memory writes when the ECADD opcode is executed by user code.
  * Given two points, P & Q, this trace constrains that both exist on the Grumpkin curve
  * and the claimed result point, R = P + Q, is written to memory addresses within range.
  * A point exists on the curve if it satisfies the curve equation (SW form: Y^2 = X^3 − 17)
- * or is the point at infinity, represented by (X, Y) = (0, 0).
+ * or is the point at infinity, represented by (X, Y) = (INFINITY_X, INFINITY_Y) = (0, 0).
  * The reads of P and Q are handled by the registers in the execution trace. The correctness
  * of point addition is handled by the ECC subtrace.
  *
  * This trace writes the resulting embedded curve point to the addresses {dst,
- * dst + 1, and dst + 2 }. Embedded curve points consist of the tuple of types
- * {x: FF, y: FF, is_inf: U1 }.
+ * dst + 1 }. Embedded curve points consist of the tuple of types {x: FF, y: FF }.
  *
- * PRECONDITIONS: Input point coordinates have tag checked FF coordinates and U1 is_inf
- * flag (see Memory I/O below for details).
+ * PRECONDITIONS: Input point coordinates have tag checked FF coordinates (see Memory I/O
+ * below for details).
  *
  * USAGE: Dispatching lookup defined in execution.pil:
  *        #[DISPATCH_TO_ECC_ADD]
@@ -85,17 +82,13 @@ include "precomputed.pil";
  *
  * This subtrace is connected to the ECC subtrace via a lookup. ECC is used by other subtraces internally
  * (e.g. address derivation). Now that the is_inf flag has been removed from noir (noir-lang/noir/#11926/) we consider
- * a point to be infinity iff its coordinates are (0, 0); the noir standard representation.
+ * a point to be infinity iff its coordinates are (0, 0); the noir standard representation (see relations #[P/Q_INF_X/Y_CHECK]).
  *
  * Note that the point at infinity, O, does not have valid coordinates (a property of SW curves like Grumpkin). We represent it
  * as (0, 0) but any (ecc.INFINITY_X, ecc.INFINITY_Y) not satisfying the curve equation will be correctly handled in this trace.
  *
- * For now, we simply ignore any is_inf flags coming from memory (assigning and not reading the placeholder is_inf_), but
- * will eventually remove it from the operands TODO(#AVM-266).
- *
- * MW NOTE: Still using p_is_inf, q_is_inf just as flags to tell ecc.pil how to treat the points.
- * Until #AVM-266, the ECC subtrace still requires a correctly constrained is_inf flag for each point. We derive it within
- * this circuit by enforcing (0, 0) <==> is_inf for input points P and Q:
+ * The ECC subtrace requires a correctly constrained is_inf flag for each point. We derive it within this circuit by enforcing
+ * (0, 0) <==> is_inf for input points P and Q:
  *  - (0, 0) ==> is_inf by #[P/Q_ON_CURVE_CHECK] (zero coordinates will fail this relation unless is_inf is set correctly).
  *  - is_inf ==> (0, 0) by #[P/Q_INF_X/Y_CHECK].
  * Resulting infinity points are guaranteed to be (0, 0) by the ECC subtrace (see #[OUTPUT_X_COORD] and #[OUTPUT_Y_COORD]).
@@ -185,9 +178,7 @@ namespace ecc_add_mem;
     pol commit sel_should_exec; // @boolean (by definition)
     sel_should_exec = sel * (1 - err);
 
-    // TODO(#AVM-266): For now, we ensure that the flag being set means that the coordinates are set to our infinity
-    // representation: (INFINITY_X, INFINITY_Y) = (0, 0). The reverse ((INFINITY_X, INFINITY_Y) ==> is_inf)
-    // is constrained by #[P/Q_ON_CURVE_CHECK]. Output infinities are already constrained to be (0, 0) in the subtrace.
+    // Constrains that the infinity flags required for the ecc trace have been set correctly:
     #[P_INF_X_CHECK]
     sel * p_is_inf * (p_x - ecc.INFINITY_X) = 0;
     #[P_INF_Y_CHECK]

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

@@ -149,7 +149,7 @@ void EccTraceBuilder::process_add(const simulation::EventEmitterInterface<simula
 
                       // Witness for add operation
                       { C::ecc_add_op, add_predicate },
-                      // This is a witness for the result(r) being the point at infinity.
+                      // This is a witness for the result (r) being the point at infinity.
                       { C::ecc_result_infinity, result_is_infinity },
                       // The computed 'slope' between points P and Q.
                       { C::ecc_lambda, lambda },
@@ -252,8 +252,6 @@ void EccTraceBuilder::process_scalar_mul(
  * @brief Process the ECC add memory events and populate the relevant columns in the trace.
  *  Corresponds to the memory aware subtrace ecc_mem.pil.
  *
- * TODO(#AVM-266): Remove is_infinity flag from point representation.
- *
  * @param events The container of ECC add memory events to process.
  * @param trace The trace container.
  */
@@ -273,9 +271,9 @@ void EccTraceBuilder::process_add_with_memory(
     // If there is no error, the trace constrains the correctness of the add result R with the ecc subtrace (see
     // process_add) via separate add events and the memory reads of the input points with the execution trace's
     // #[DISPATCH_TO_ECC_ADD]. The writes are handled in the trace with a permutation to memory (see #[WRITE_MEM_i]
-    // and interactions perm_ecc_mem_write_mem_i for i = 0, 1, 2).
+    // and interactions perm_ecc_mem_write_mem_i for i = 0, 1).
 
-    // If there is an error, the event has an empty result point (0, 0, false), the add/write lookups/permutations are
+    // If there is an error, the event has an empty result point (0, 0), the add/write lookups/permutations are
     // skipped, and the error flag is set to 1. This flag is checked against sel_opcode_error in #[DISPATCH_TO_ECC_ADD].
     for (const auto& event : events) {
         // Address cast to uint64_t to capture possible overflow.
@@ -297,43 +295,41 @@ void EccTraceBuilder::process_add_with_memory(
 
         bool error = dst_out_of_range_err || !p_is_on_curve || !q_is_on_curve;
 
-        trace.set(
-            row,
-            { {
-                { C::ecc_add_mem_sel, 1 },
-                { C::ecc_add_mem_execution_clk, event.execution_clk },
-                { C::ecc_add_mem_space_id, event.space_id },
-                // Error handling - dst out of range
-                { C::ecc_add_mem_max_mem_addr, AVM_HIGHEST_MEM_ADDRESS },
-                { C::ecc_add_mem_sel_dst_out_of_range_err, dst_out_of_range_err ? 1 : 0 },
-                // Error handling - p is not on curve
-                { C::ecc_add_mem_sel_p_not_on_curve_err, !p_is_on_curve ? 1 : 0 },
-                { C::ecc_add_mem_p_is_on_curve_eqn, p_is_on_curve_eqn },
-                { C::ecc_add_mem_p_is_on_curve_eqn_inv, p_is_on_curve_eqn_inv },
-                // Error handling - q is not on curve
-                { C::ecc_add_mem_sel_q_not_on_curve_err, !q_is_on_curve ? 1 : 0 },
-                { C::ecc_add_mem_q_is_on_curve_eqn, q_is_on_curve_eqn },
-                { C::ecc_add_mem_q_is_on_curve_eqn_inv, q_is_on_curve_eqn_inv },
-                // Consolidated error
-                { C::ecc_add_mem_err, error ? 1 : 0 },
-                // Memory Writes
-                { C::ecc_add_mem_dst_addr_0_, dst_addr },
-                { C::ecc_add_mem_dst_addr_1_, dst_addr + 1 },
-                // Input - Point P
-                { 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
-                // Input - Point Q
-                { 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
-                // Output
-                { C::ecc_add_mem_sel_should_exec, error ? 0 : 1 },
-                { C::ecc_add_mem_res_x, event.result.x() },
-                { C::ecc_add_mem_res_y, event.result.y() },
-            } });
+        trace.set(row,
+                  { {
+                      { C::ecc_add_mem_sel, 1 },
+                      { C::ecc_add_mem_execution_clk, event.execution_clk },
+                      { C::ecc_add_mem_space_id, event.space_id },
+                      // Error handling - dst out of range
+                      { C::ecc_add_mem_max_mem_addr, AVM_HIGHEST_MEM_ADDRESS },
+                      { C::ecc_add_mem_sel_dst_out_of_range_err, dst_out_of_range_err ? 1 : 0 },
+                      // Error handling - p is not on curve
+                      { C::ecc_add_mem_sel_p_not_on_curve_err, !p_is_on_curve ? 1 : 0 },
+                      { C::ecc_add_mem_p_is_on_curve_eqn, p_is_on_curve_eqn },
+                      { C::ecc_add_mem_p_is_on_curve_eqn_inv, p_is_on_curve_eqn_inv },
+                      // Error handling - q is not on curve
+                      { C::ecc_add_mem_sel_q_not_on_curve_err, !q_is_on_curve ? 1 : 0 },
+                      { C::ecc_add_mem_q_is_on_curve_eqn, q_is_on_curve_eqn },
+                      { C::ecc_add_mem_q_is_on_curve_eqn_inv, q_is_on_curve_eqn_inv },
+                      // Consolidated error
+                      { C::ecc_add_mem_err, error ? 1 : 0 },
+                      // Memory Writes
+                      { C::ecc_add_mem_dst_addr_0_, dst_addr },
+                      { C::ecc_add_mem_dst_addr_1_, dst_addr + 1 },
+                      // Input - Point P
+                      { C::ecc_add_mem_p_x, event.p.x() },
+                      { C::ecc_add_mem_p_y, event.p.y() },
+                      // Input - Point Q
+                      { C::ecc_add_mem_q_x, event.q.x() },
+                      { C::ecc_add_mem_q_y, event.q.y() },
+                      // Input - Infinity flags required for ECC trace
+                      { C::ecc_add_mem_p_is_inf, event.p.is_infinity() ? 1 : 0 },
+                      { C::ecc_add_mem_q_is_inf, event.q.is_infinity() ? 1 : 0 },
+                      // Output
+                      { C::ecc_add_mem_sel_should_exec, error ? 0 : 1 },
+                      { C::ecc_add_mem_res_x, event.result.x() },
+                      { C::ecc_add_mem_res_y, event.result.y() },
+                  } });
 
         row++;
     }