diff --git a/CHANGELOG.md b/CHANGELOG.md index 49518594bb..2a028ea8f6 100644 --- a/CHANGELOG.md +++ b/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( diff --git a/felt/src/bigint_felt.rs b/felt/src/bigint_felt.rs index f4db9977b9..72fd12cccb 100644 --- a/felt/src/bigint_felt.rs +++ b/felt/src/bigint_felt.rs @@ -183,7 +183,7 @@ impl FeltOps for FeltBigInt { } 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 { self.val.clone() } - fn sqrt(&self) -> FeltBigInt { - FeltBigInt { - val: self.val.sqrt(), - } - } - fn bits(&self) -> u64 { self.val.bits() } @@ -670,11 +664,7 @@ impl Integer for FeltBigInt { impl Signed for FeltBigInt { fn abs(&self) -> Self { - if self.is_negative() { - self.neg() - } else { - self.clone() - } + self.clone() } fn abs_sub(&self, other: &Self) -> Self { @@ -688,15 +678,13 @@ impl Signed for FeltBigInt { 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 { diff --git a/felt/src/lib.rs b/felt/src/lib.rs index d2a04df56a..6d9f09b9e2 100644 --- a/felt/src/lib.rs +++ b/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; @@ -228,10 +226,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() } @@ -1377,32 +1403,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::()) { 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::()) { 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) @@ -1415,12 +1426,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::()) { prop_assert_eq!(&x, &x.abs()) } @@ -1443,15 +1449,16 @@ 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::()) { - prop_assert_eq!((&x * &x).sqrt(), x); + let x_sq = &x * &x; + let sqrt = x_sq.sqrt(); + + if sqrt != x { + prop_assert_eq!(Felt252::max_value() - sqrt + 1_usize, x); + } else { + prop_assert_eq!(sqrt, x); + } } #[test] diff --git a/src/hint_processor/builtin_hint_processor/ec_utils.rs b/src/hint_processor/builtin_hint_processor/ec_utils.rs index e995b279a1..2481a18e05 100644 --- a/src/hint_processor/builtin_hint_processor/ec_utils.rs +++ b/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 { 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 } diff --git a/src/hint_processor/builtin_hint_processor/find_element_hint.rs b/src/hint_processor/builtin_hint_processor/find_element_hint.rs index 235ba2e080..a435da628e 100644 --- a/src/hint_processor/builtin_hint_processor/find_element_hint.rs +++ b/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::("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() { diff --git a/src/hint_processor/builtin_hint_processor/math_utils.rs b/src/hint_processor/builtin_hint_processor/math_utils.rs index 948e71d69e..c7bb66058e 100644 --- a/src/hint_processor/builtin_hint_processor/math_utils.rs +++ b/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()); } } } diff --git a/src/hint_processor/cairo_1_hint_processor/hint_processor.rs b/src/hint_processor/cairo_1_hint_processor/hint_processor.rs index ca5b03ef31..c436dd823b 100644 --- a/src/hint_processor/cairo_1_hint_processor/hint_processor.rs +++ b/src/hint_processor/cairo_1_hint_processor/hint_processor.rs @@ -263,9 +263,9 @@ impl Cairo1HintProcessor { value: &ResOperand, dst: &CellRef, ) -> Result<(), HintError> { - let value = res_operand_get_val(vm, value)?; + let value = res_operand_get_val(vm, value)?.to_biguint(); let result = value.sqrt(); - vm.insert_value(cell_ref_to_relocatable(dst, vm)?, result) + vm.insert_value(cell_ref_to_relocatable(dst, vm)?, Felt252::from(result)) .map_err(HintError::from) } diff --git a/src/math_utils.rs b/src/math_utils.rs index ae8ba6668c..d0f474e3c6 100644 --- a/src/math_utils.rs +++ b/src/math_utils.rs @@ -6,7 +6,7 @@ use felt::Felt252; use num_bigint::{BigInt, BigUint, RandBigInt}; use num_integer::Integer; use num_prime::nt_funcs::is_prime; -use num_traits::{Bounded, One, Pow, Signed, Zero}; +use num_traits::{One, Signed, Zero}; use rand::{rngs::SmallRng, SeedableRng}; ///Returns the integer square root of the nonnegative integer n. ///This is the floor of the exact square root of n. @@ -153,38 +153,6 @@ pub fn ec_double_slope(point: &(BigInt, BigInt), alpha: &BigInt, prime: &BigInt) ) } -pub fn sqrt(n: &Felt252) -> Felt252 { - // Based on Tonelli-Shanks' algorithm for finding square roots - // and sympy's library implementation of said algorithm. - if n.is_zero() || n.is_one() { - return n.clone(); - } - - let max_felt = Felt252::max_value(); - let trailing_prime = Felt252::max_value() >> 192; // 0x800000000000011 - let a = n.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 = n.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 - } -} - // Adapted from sympy _sqrt_prime_power with k == 1 pub fn sqrt_prime_power(a: &BigUint, p: &BigUint) -> Option { if p.is_zero() || !is_prime(p, None).probably() { @@ -727,22 +695,6 @@ mod tests { assert_matches!(safe_div_bigint(&x, &y), Err(MathError::DividedByZero)) } - #[test] - #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] - fn test_sqrt() { - let n = Felt252::from_str_radix( - "99957092485221722822822221624080199277265330641980989815386842231144616633668", - 10, - ) - .unwrap(); - let expected_sqrt = Felt252::from_str_radix( - "205857351767627712295703269674687767888261140702556021834663354704341414042", - 10, - ) - .unwrap(); - assert_eq!(sqrt(&n), expected_sqrt); - } - #[test] #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] fn test_sqrt_prime_power() { @@ -845,20 +797,6 @@ mod tests { #[cfg(not(target_arch = "wasm32"))] proptest! { - #[test] - // Test for sqrt of a quadratic residue. Result should be the minimum root. - fn sqrt_felt_test(ref x in any::<[u8; 32]>()) { - let x = &Felt252::from_bytes_be(x); - let x_sq = x * x; - let sqrt = sqrt(&x_sq); - - if &sqrt != x { - prop_assert_eq!(&(Felt252::max_value() - sqrt + 1_usize), x); - } else { - prop_assert_eq!(&sqrt, x); - } - } - #[test] // Test for sqrt_prime_power_ of a quadratic residue. Result should be the minimum root. fn sqrt_prime_power_using_random_prime(ref x in any::<[u8; 38]>(), ref y in any::()) {