Signed functions for felts and sqrt

CHANGELOG.md

@@ -2,6 +2,11 @@
 
 #### Upcoming Changes
 
+* fix: Fix felt sqrt and Signed impl [#1150](https://github.com/lambdaclass/cairo-rs/pull/1150)
+
+  * BREAKING: Fix `Felt252` methods `abs`, `signum`, `is_positive`, `is_negative` and `sqrt`
+  * BREAKING: Remove function `math_utils::sqrt`(Now moved to `Felt252::sqrt`)
+
 * feat: Add method `CairoRunner::initialize_function_runner_cairo_1` [#1151](https://github.com/lambdaclass/cairo-rs/pull/1151)
 
   * Add method `pub fn initialize_function_runner_cairo_1(

felt/src/bigint_felt.rs

@@ -183,7 +183,7 @@ impl FeltOps for FeltBigInt<FIELD_HIGH, FIELD_LOW> {
     }
 
     fn to_signed_felt(&self) -> BigInt {
-        if self.is_negative() {
+        if self.val > *SIGNED_FELT_MAX {
             BigInt::from_biguint(num_bigint::Sign::Minus, &*CAIRO_PRIME_BIGUINT - &self.val)
         } else {
             self.val.clone().into()
@@ -198,12 +198,6 @@ impl FeltOps for FeltBigInt<FIELD_HIGH, FIELD_LOW> {
         self.val.clone()
     }
 
-    fn sqrt(&self) -> FeltBigInt<FIELD_HIGH, FIELD_LOW> {
-        FeltBigInt {
-            val: self.val.sqrt(),
-        }
-    }
-
     fn bits(&self) -> u64 {
         self.val.bits()
     }
@@ -659,11 +653,7 @@ impl Integer for FeltBigInt<FIELD_HIGH, FIELD_LOW> {
 
 impl Signed for FeltBigInt<FIELD_HIGH, FIELD_LOW> {
     fn abs(&self) -> Self {
-        if self.is_negative() {
-            self.neg()
-        } else {
-            self.clone()
-        }
+        self.clone()
     }
 
     fn abs_sub(&self, other: &Self) -> Self {
@@ -677,15 +667,13 @@ impl Signed for FeltBigInt<FIELD_HIGH, FIELD_LOW> {
     fn signum(&self) -> Self {
         if self.is_zero() {
             FeltBigInt::zero()
-        } else if self.is_positive() {
-            FeltBigInt::one()
         } else {
-            FeltBigInt::max_value()
+            FeltBigInt::one()
         }
     }
 
     fn is_positive(&self) -> bool {
-        !self.is_zero() && self.val < *SIGNED_FELT_MAX
+        !self.is_zero()
     }
 
     fn is_negative(&self) -> bool {

felt/src/lib.rs

@@ -90,8 +90,6 @@ pub(crate) trait FeltOps {
     /// ```
     fn to_biguint(&self) -> BigUint;
 
-    fn sqrt(&self) -> Self;
-
     fn bits(&self) -> u64;
 
     fn prime() -> BigUint;
@@ -217,10 +215,38 @@ impl Felt252 {
         self.value.to_biguint()
     }
     pub fn sqrt(&self) -> Self {
-        Self {
-            value: self.value.sqrt(),
+        // Based on Tonelli-Shanks' algorithm for finding square roots
+        // and sympy's library implementation of said algorithm.
+        if self.is_zero() || self.is_one() {
+            return self.clone();
+        }
+
+        let max_felt = Felt252::max_value();
+        let trailing_prime = Felt252::max_value() >> 192; // 0x800000000000011
+
+        let a = self.pow(&trailing_prime);
+        let d = (&Felt252::new(3_i32)).pow(&trailing_prime);
+        let mut m = Felt252::zero();
+        let mut exponent = Felt252::one() << 191_u32;
+        let mut adm;
+        for i in 0..192_u32 {
+            adm = &a * &(&d).pow(&m);
+            adm = (&adm).pow(&exponent);
+            exponent >>= 1;
+            // if adm ≡ -1 (mod CAIRO_PRIME)
+            if adm == max_felt {
+                m += Felt252::one() << i;
+            }
+        }
+        let root_1 = self.pow(&((trailing_prime + 1_u32) >> 1)) * (&d).pow(&(m >> 1));
+        let root_2 = &max_felt - &root_1 + 1_usize;
+        if root_1 < root_2 {
+            root_1
+        } else {
+            root_2
         }
     }
+
     pub fn bits(&self) -> u64 {
         self.value.bits()
     }
@@ -1282,32 +1308,17 @@ mod test {
             prop_assert_eq!(x, &(&one * x));
         }
 
-        // Signedness has ambiguous meaning and currently there's a mismatch between
-        // the implementation and test's interpretations
-        // See: https://github.com/lambdaclass/cairo-rs/issues/1076
-        // WIP fix: https://github.com/lambdaclass/cairo-rs/pull/1150
         #[test]
-        #[ignore]
-        fn non_zero_felt_is_always_positive(x in nonzero_felt252()) {
+        fn felt_is_always_positive(x in any::<Felt252>()) {
             prop_assert!(x.is_positive())
         }
 
-        // Signedness has ambiguous meaning and currently there's a mismatch
-        // between the implementation and test's interpretations
-        // See: https://github.com/lambdaclass/cairo-rs/issues/1076
-        // WIP fix: https://github.com/lambdaclass/cairo-rs/pull/1150
         #[test]
-        #[ignore]
         fn felt_is_never_negative(x in any::<Felt252>()) {
             prop_assert!(!x.is_negative())
         }
 
-        // Signedness has ambiguous meaning and currently there's a mismatch between
-        // the implementation and test's interpretations
-        // See: https://github.com/lambdaclass/cairo-rs/issues/1076
-        // WIP fix: https://github.com/lambdaclass/cairo-rs/pull/1150
         #[test]
-        #[ignore]
         fn non_zero_felt_signum_is_always_one(ref x in nonzero_felt252()) {
             let one = Felt252::one();
             prop_assert_eq!(x.signum(), one)
@@ -1320,12 +1331,7 @@ mod test {
             prop_assert_eq!(x.abs_sub(&y), expected_abs_sub)
         }
 
-        // Signedness has ambiguous meaning and currently there's a mismatch
-        // between the implementation and test's interpretations
-        // See: https://github.com/lambdaclass/cairo-rs/issues/1076
-        // WIP fix: https://github.com/lambdaclass/cairo-rs/pull/1150
         #[test]
-        #[ignore]
         fn abs(x in any::<Felt252>()) {
             prop_assert_eq!(&x, &x.abs())
         }
@@ -1348,14 +1354,12 @@ mod test {
             prop_assert!(sqrt < p, "{}", sqrt);
         }
 
-        // There is a known bug in this implementation where the squared root
-        // we compute is that of the underlying integer rather than that of the
-        // field element.
-        // See: https://github.com/lambdaclass/cairo-rs/issues/1076
-        // WIP fix: https://github.com/lambdaclass/cairo-rs/pull/1150
         #[test]
-        #[ignore]
         fn sqrt_is_inv_square(x in any::<Felt252>()) {
+            // In the regular use case the number upon which sqrt is called will always be a felt (aka a numer lower than cairo_prime)
+            // In order to get a number for which we know its root it is not a good enough solution to square the felt (as we might get something bigger than cairo prime)
+            // We can instead use the square root (using the biguint implementation), to guarantee that its square will be inside the felt range
+            let x = Felt252::from(x.to_biguint().sqrt());
             prop_assert_eq!((&x * &x).sqrt(), x);
         }
 

src/hint_processor/builtin_hint_processor/ec_utils.rs

@@ -17,8 +17,6 @@ use num_bigint::ToBigInt;
 use num_traits::{Bounded, Num, One, Pow, ToPrimitive, Zero};
 use sha2::{Digest, Sha256};
 
-use crate::math_utils::sqrt;
-
 use super::hint_utils::get_ptr_from_var_name;
 
 #[derive(Debug, PartialEq)]
@@ -207,7 +205,7 @@ lazy_static! {
 fn recover_y(x: &BigUint) -> Option<BigUint> {
     let y_squared: BigUint = x.modpow(&BigUint::from(3_u32), &CAIRO_PRIME) + ALPHA * x + &*BETA;
     if is_quad_residue(&y_squared) {
-        Some(sqrt(&Felt252::from(y_squared)).to_biguint())
+        Some(Felt252::from(y_squared).sqrt().to_biguint())
     } else {
         None
     }

src/hint_processor/builtin_hint_processor/find_element_hint.rs

@@ -1,5 +1,4 @@
 use crate::stdlib::{collections::HashMap, prelude::*};
-
 use crate::{
     hint_processor::{
         builtin_hint_processor::hint_utils::{
@@ -14,7 +13,8 @@ use crate::{
     vm::{errors::hint_errors::HintError, vm_core::VirtualMachine},
 };
 use felt::Felt252;
-use num_traits::{Signed, ToPrimitive};
+use num_traits::Signed;
+use num_traits::ToPrimitive;
 
 pub fn find_element(
     vm: &mut VirtualMachine,
@@ -51,10 +51,6 @@ pub fn find_element(
         exec_scopes.delete_variable("find_element_index");
         Ok(())
     } else {
-        if n_elms.is_negative() {
-            return Err(HintError::ValueOutOfRange(n_elms.into_owned()));
-        }
-
         if let Ok(find_element_max_size) = exec_scopes.get_ref::<Felt252>("find_element_max_size") {
             if n_elms.as_ref() > find_element_max_size {
                 return Err(HintError::FindElemMaxSize(
@@ -103,10 +99,6 @@ pub fn search_sorted_lower(
         return Err(HintError::ValueOutOfRange(elm_size.into_owned()));
     }
 
-    if n_elms.is_negative() {
-        return Err(HintError::ValueOutOfRange(n_elms.into_owned()));
-    }
-
     if let Ok(find_element_max_size) = find_element_max_size {
         if n_elms.as_ref() > &find_element_max_size {
             return Err(HintError::FindElemMaxSize(
@@ -340,7 +332,7 @@ mod tests {
         )]));
         assert_matches!(
             run_hint!(vm, ids_data, hint_code::FIND_ELEMENT),
-            Err(HintError::ValueOutOfRange(x)) if x == Felt252::new(-1)
+            Err(HintError::Math(MathError::Felt252ToI32Conversion(x))) if x == Felt252::new(-1)
         );
     }
 
@@ -427,19 +419,6 @@ mod tests {
         );
     }
 
-    #[test]
-    #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
-    fn search_sorted_lower_negative_elm_size() {
-        let (mut vm, ids_data) = init_vm_ids_data(HashMap::from([(
-            "elm_size".to_string(),
-            MaybeRelocatable::Int(Felt252::new(-1)),
-        )]));
-        assert_matches!(
-            run_hint!(vm, ids_data, hint_code::SEARCH_SORTED_LOWER),
-            Err(HintError::ValueOutOfRange(x)) if x == Felt252::new(-1)
-        );
-    }
-
     #[test]
     #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
     fn search_sorted_lower_not_int_n_elms() {
@@ -454,19 +433,6 @@ mod tests {
         );
     }
 
-    #[test]
-    #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
-    fn search_sorted_lower_negative_n_elms() {
-        let (mut vm, ids_data) = init_vm_ids_data(HashMap::from([(
-            "n_elms".to_string(),
-            MaybeRelocatable::Int(Felt252::new(-1)),
-        )]));
-        assert_matches!(
-            run_hint!(vm, ids_data, hint_code::SEARCH_SORTED_LOWER),
-            Err(HintError::ValueOutOfRange(x)) if x == Felt252::new(-1)
-        );
-    }
-
     #[test]
     #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
     fn search_sorted_lower_empty_scope() {

src/hint_processor/builtin_hint_processor/math_utils.rs

@@ -374,20 +374,17 @@ pub fn is_positive(
     ap_tracking: &ApTracking,
 ) -> Result<(), HintError> {
     let value = get_integer_from_var_name("value", vm, ids_data, ap_tracking)?;
+    let value_as_int = value.to_signed_felt();
     let range_check_builtin = vm.get_range_check_builtin()?;
     //Main logic (assert a is positive)
     match &range_check_builtin._bound {
-        Some(bound) if &value.abs() > bound => {
+        Some(bound) if value_as_int.abs() > bound.to_bigint() => {
             return Err(HintError::ValueOutsideValidRange(value.into_owned()))
         }
         _ => {}
     };
 
-    let result = if value.is_positive() {
-        Felt252::one()
-    } else {
-        Felt252::zero()
-    };
+    let result = Felt252::from(value_as_int.is_positive() as u8);
     insert_value_from_var_name("is_positive", result, vm, ids_data, ap_tracking)
 }
 
@@ -665,11 +662,11 @@ pub fn is_quad_residue(
     if x.is_zero() || x.is_one() {
         insert_value_from_var_name("y", x.as_ref().clone(), vm, ids_data, ap_tracking)
     } else if Pow::pow(x.as_ref(), &(Felt252::max_value() >> 1)).is_one() {
-        insert_value_from_var_name("y", crate::math_utils::sqrt(&x), vm, ids_data, ap_tracking)
+        insert_value_from_var_name("y", &x.sqrt(), vm, ids_data, ap_tracking)
     } else {
         insert_value_from_var_name(
             "y",
-            crate::math_utils::sqrt(&(x.as_ref() / Felt252::new(3_i32))),
+            (x.as_ref() / Felt252::new(3_i32)).sqrt(),
             vm,
             ids_data,
             ap_tracking,
@@ -2357,9 +2354,9 @@ mod tests {
             if x.is_zero() || x.is_one() {
                 assert_eq!(vm.get_integer(Relocatable::from((1, 0))).unwrap().as_ref(), x);
             } else if x.pow(&(Felt252::max_value() >> 1)).is_one() {
-                assert_eq!(vm.get_integer(Relocatable::from((1, 0))).unwrap().into_owned(), crate::math_utils::sqrt(x));
+                assert_eq!(vm.get_integer(Relocatable::from((1, 0))).unwrap().into_owned(), x.sqrt());
             } else {
-                assert_eq!(vm.get_integer(Relocatable::from((1, 0))).unwrap().into_owned(), crate::math_utils::sqrt(&(x / Felt252::new(3))));
+                assert_eq!(vm.get_integer(Relocatable::from((1, 0))).unwrap().into_owned(), (x / Felt252::new(3)).sqrt());
             }
         }
     }