Use ProjectivePoint from types-rs in ec_op builtin impl (#1532)

* First draft * Fixes * Fix example * Fixes * Update version * Update version * Add changelog * Update types-rs version * Update lock * Remove FIXME * Remove allows * Apply suggestions * Update vm/src/vm/runners/builtin_runner/ec_op.rs Co-authored-by: Mario Rugiero <mario.rugiero@lambdaclass.com> --------- Co-authored-by: Mario Rugiero <mario.rugiero@lambdaclass.com>
lambdaclass · Jan 5, 2024 · 42a652c · 42a652c
1 parent 88a5bf1
commit 42a652c
Show file tree

Hide file tree

Showing 7 changed files with 85 additions and 71 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,8 @@
 
 #### Upcoming Changes
 
+* feat: Use `ProjectivePoint` from types-rs in ec_op builtin impl [#1532](https://github.com/lambdaclass/cairo-vm/pull/1532)
+
 * feat(BREAKING): Replace `cairo-felt` crate with `starknet-types-core` (0.0.5) [#1408](https://github.com/lambdaclass/cairo-vm/pull/1408)
 
 * feat(BREAKING): Add Cairo 1 proof mode compilation and execution [#1517] (https://github.com/lambdaclass/cairo-vm/pull/1517)

diff --git a/Cargo.lock b/Cargo.lock
diff --git a/fuzzer/Cargo.lock b/fuzzer/Cargo.lock
diff --git a/vm/Cargo.toml b/vm/Cargo.toml
@@ -61,7 +61,7 @@ keccak = { workspace = true }
 hashbrown = { workspace = true }
 anyhow = { workspace = true }
 thiserror-no-std = { workspace = true }
-starknet-types-core = { version = "0.0.5", default-features = false, features = ["serde"] }
+starknet-types-core = { version = "0.0.6", default-features = false, features = ["serde", "curve", "num-traits"] }
 
 # only for std
 num-prime = { version = "0.4.3", features = ["big-int"], optional = true }

diff --git a/vm/src/math_utils/mod.rs b/vm/src/math_utils/mod.rs
@@ -48,12 +48,13 @@ pub fn pow2_const_nz(n: u32) -> &'static NonZeroFelt {
 /// # Examples
 ///
 /// ```
-/// # use crate::Felt252;
-/// let positive = Felt252::new(5);
-/// assert_eq!(signed_felt(positive), Felt252::from(5));
+/// # use cairo_vm::{Felt252, math_utils::signed_felt};
+/// # use num_bigint::BigInt;
+/// let positive = Felt252::from(5);
+/// assert_eq!(signed_felt(positive), BigInt::from(5));
 ///
-/// let negative = Felt252::max_value();
-/// assert_eq!(signed_felt(negative), Felt252::from(-1));
+/// let negative = Felt252::MAX;
+/// assert_eq!(signed_felt(negative), BigInt::from(-1));
 /// ```
 
 pub fn signed_felt(felt: Felt252) -> BigInt {

diff --git a/vm/src/vm/errors/runner_errors.rs b/vm/src/vm/errors/runner_errors.rs
@@ -2,7 +2,6 @@
 #![allow(clippy::explicit_auto_deref)]
 
 use crate::stdlib::{collections::HashSet, prelude::*};
-
 use thiserror_no_std::Error;
 
 use super::{memory_errors::MemoryError, trace_errors::TraceError};
@@ -101,6 +100,8 @@ pub enum RunnerError {
     StrippedProgramNoMain,
     #[error(transparent)]
     Trace(#[from] TraceError),
+    #[error("EcOp builtin: Invalid Point")]
+    InvalidPoint,
 }
 
 #[cfg(test)]

diff --git a/vm/src/vm/runners/builtin_runner/ec_op.rs b/vm/src/vm/runners/builtin_runner/ec_op.rs
@@ -1,19 +1,16 @@
-use crate::math_utils::{ec_add, ec_double};
 use crate::stdlib::{borrow::Cow, prelude::*};
 use crate::stdlib::{cell::RefCell, collections::HashMap};
 use crate::types::instance_definitions::ec_op_instance_def::{
     EcOpInstanceDef, CELLS_PER_EC_OP, INPUT_CELLS_PER_EC_OP,
 };
 use crate::types::relocatable::{MaybeRelocatable, Relocatable};
-use crate::utils::{bigint_to_felt, felt_to_bigint, CAIRO_PRIME};
 use crate::vm::errors::memory_errors::MemoryError;
 use crate::vm::errors::runner_errors::RunnerError;
 use crate::vm::vm_memory::memory::Memory;
 use crate::vm::vm_memory::memory_segments::MemorySegmentManager;
 use crate::Felt252;
-use num_bigint::{BigInt, ToBigInt};
 use num_integer::{div_ceil, Integer};
-use num_traits::{One, Zero};
+use starknet_types_core::curve::ProjectivePoint;
 
 use super::EC_OP_BUILTIN_NAME;
 
@@ -51,7 +48,6 @@ impl EcOpBuiltinRunner {
         y.pow(2_u32) == (x.pow(3_u32) + alpha * x) + beta
     }
 
-    #[allow(deprecated)]
     ///Returns the result of the EC operation P + m * Q.
     /// where P = (p_x, p_y), Q = (q_x, q_y) are points on the elliptic curve defined as
     /// y^2 = x^3 + alpha * x + beta (mod prime).
@@ -62,31 +58,29 @@ impl EcOpBuiltinRunner {
         partial_sum: (Felt252, Felt252),
         doubled_point: (Felt252, Felt252),
         m: &Felt252,
-        alpha: &BigInt,
-        prime: &BigInt,
         height: u32,
-    ) -> Result<(BigInt, BigInt), RunnerError> {
-        let mut slope = felt_to_bigint(*m);
-        let mut partial_sum_b = (felt_to_bigint(partial_sum.0), felt_to_bigint(partial_sum.1));
-        let mut doubled_point_b = (
-            felt_to_bigint(doubled_point.0),
-            felt_to_bigint(doubled_point.1),
-        );
-        for _ in 0..height {
-            if (doubled_point_b.0.clone() - partial_sum_b.0.clone()).is_zero() {
-                #[allow(deprecated)]
+    ) -> Result<(Felt252, Felt252), RunnerError> {
+        let slope = m.to_biguint();
+        let mut partial_sum_b = ProjectivePoint::from_affine(partial_sum.0, partial_sum.1)
+            .map_err(|_| RunnerError::PointNotOnCurve(Box::new(partial_sum)))?;
+        let mut doubled_point_b = ProjectivePoint::from_affine(doubled_point.0, doubled_point.1)
+            .map_err(|_| RunnerError::PointNotOnCurve(Box::new(doubled_point)))?;
+        for i in 0..(height as u64).min(slope.bits()) {
+            if partial_sum_b.x() * doubled_point_b.z() == partial_sum_b.z() * doubled_point_b.x() {
                 return Err(RunnerError::EcOpSameXCoordinate(
                     Self::format_ec_op_error(partial_sum_b, slope, doubled_point_b)
                         .into_boxed_str(),
                 ));
             };
-            if !(slope.clone() & &BigInt::one()).is_zero() {
-                partial_sum_b = ec_add(partial_sum_b, doubled_point_b.clone(), prime)?;
+            if slope.bit(i) {
+                partial_sum_b += &doubled_point_b;
             }
-            doubled_point_b = ec_double(doubled_point_b, alpha, prime)?;
-            slope = slope.clone() >> 1_u32;
+            doubled_point_b = doubled_point_b.double();
         }
-        Ok(partial_sum_b)
+        partial_sum_b
+            .to_affine()
+            .map(|p| (p.x(), p.y()))
+            .map_err(|_| RunnerError::InvalidPoint)
     }
 
     pub fn initialize_segments(&mut self, segments: &mut MemorySegmentManager) {
@@ -179,17 +173,12 @@ impl EcOpBuiltinRunner {
                 ))));
             };
         }
-        let prime = CAIRO_PRIME.to_bigint().unwrap();
         let result = EcOpBuiltinRunner::ec_op_impl(
             (input_cells[0].to_owned(), input_cells[1].to_owned()),
             (input_cells[2].to_owned(), input_cells[3].to_owned()),
             input_cells[4],
-            #[allow(deprecated)]
-            &felt_to_bigint(alpha),
-            &prime,
             self.ec_op_builtin.scalar_height,
         )?;
-        let result = (bigint_to_felt(&result.0)?, bigint_to_felt(&result.1)?);
         self.cache.borrow_mut().insert(x_addr, result.0);
         self.cache.borrow_mut().insert(
             (x_addr + 1usize)
@@ -259,10 +248,12 @@ impl EcOpBuiltinRunner {
     }
 
     pub fn format_ec_op_error(
-        p: (num_bigint::BigInt, num_bigint::BigInt),
-        m: num_bigint::BigInt,
-        q: (num_bigint::BigInt, num_bigint::BigInt),
+        p: ProjectivePoint,
+        m: num_bigint::BigUint,
+        q: ProjectivePoint,
     ) -> String {
+        let p = p.to_affine().map(|p| (p.x(), p.y())).unwrap_or_default();
+        let q = q.to_affine().map(|q| (q.x(), q.y())).unwrap_or_default();
         format!("Cannot apply EC operation: computation reached two points with the same x coordinate. \n
     Attempting to compute P + m * Q where:\n
     P = {p:?} \n
@@ -278,7 +269,7 @@ mod tests {
     use crate::serde::deserialize_program::BuiltinName;
     use crate::stdlib::collections::HashMap;
     use crate::types::program::Program;
-    use crate::utils::{test_utils::*, CAIRO_PRIME};
+    use crate::utils::test_utils::*;
     use crate::vm::errors::cairo_run_errors::CairoRunError;
     use crate::vm::errors::vm_errors::VirtualMachineError;
     use crate::vm::runners::cairo_runner::CairoRunner;
@@ -548,18 +539,15 @@ mod tests {
             felt_hex!("0x5668060aa49730b7be4801df46ec62de53ecd11abe43a32873000c36e8dc1f"),
         );
         let m = Felt252::from(34);
-        let alpha = bigint!(1);
         let height = 256;
-        let prime = (*CAIRO_PRIME).clone().into();
-        let result =
-            EcOpBuiltinRunner::ec_op_impl(partial_sum, doubled_point, &m, &alpha, &prime, height);
+        let result = EcOpBuiltinRunner::ec_op_impl(partial_sum, doubled_point, &m, height);
         assert_eq!(
             result,
             Ok((
-                bigint_str!(
+                felt_str!(
                     "1977874238339000383330315148209250828062304908491266318460063803060754089297"
                 ),
-                bigint_str!(
+                felt_str!(
                     "2969386888251099938335087541720168257053975603483053253007176033556822156706"
                 )
             ))
@@ -578,18 +566,15 @@ mod tests {
             felt_hex!("0x5668060aa49730b7be4801df46ec62de53ecd11abe43a32873000c36e8dc1f"),
         );
         let m = Felt252::from(34);
-        let alpha = bigint!(1);
         let height = 256;
-        let prime = (*CAIRO_PRIME).clone().into();
-        let result =
-            EcOpBuiltinRunner::ec_op_impl(partial_sum, doubled_point, &m, &alpha, &prime, height);
+        let result = EcOpBuiltinRunner::ec_op_impl(partial_sum, doubled_point, &m, height);
         assert_eq!(
             result,
             Ok((
-                bigint_str!(
+                felt_str!(
                     "2778063437308421278851140253538604815869848682781135193774472480292420096757"
                 ),
-                bigint_str!(
+                felt_str!(
                     "3598390311618116577316045819420613574162151407434885460365915347732568210029"
                 )
             ))
@@ -598,26 +583,25 @@ mod tests {
 
     #[test]
     #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
-    #[allow(deprecated)]
     fn compute_ec_op_invalid_same_x_coordinate() {
-        let partial_sum = (Felt252::ONE, Felt252::from(9));
-        let doubled_point = (Felt252::ONE, Felt252::from(12));
+        let partial_sum = (
+            felt_hex!("0x6f0a1ddaf19c44781c8946db396f494a10ffab183c2d8cf6c4cd321a8d87fd9"),
+            felt_hex!("0x4afa52a9ef8c023d3385fddb6e1d78d57b0693b9b02d45d0f939b526d474c39"),
+        );
+        let doubled_point = (
+            felt_hex!("0x6f0a1ddaf19c44781c8946db396f494a10ffab183c2d8cf6c4cd321a8d87fd9"),
+            felt_hex!("0x4afa52a9ef8c023d3385fddb6e1d78d57b0693b9b02d45d0f939b526d474c39"),
+        );
         let m = Felt252::from(34);
-        let alpha = bigint!(1);
         let height = 256;
-        let prime = (*CAIRO_PRIME).clone().into();
-        let result =
-            EcOpBuiltinRunner::ec_op_impl(partial_sum, doubled_point, &m, &alpha, &prime, height);
+        let result = EcOpBuiltinRunner::ec_op_impl(partial_sum, doubled_point, &m, height);
         assert_eq!(
             result,
             Err(RunnerError::EcOpSameXCoordinate(
                 EcOpBuiltinRunner::format_ec_op_error(
-                    (felt_to_bigint(partial_sum.0), felt_to_bigint(partial_sum.1)),
-                    felt_to_bigint(m),
-                    (
-                        felt_to_bigint(doubled_point.0),
-                        felt_to_bigint(doubled_point.1)
-                    )
+                    ProjectivePoint::from_affine(partial_sum.0, partial_sum.1).unwrap(),
+                    m.to_biguint(),
+                    ProjectivePoint::from_affine(doubled_point.0, doubled_point.1).unwrap(),
                 )
                 .into_boxed_str()
             ))