From df2ec88ee985b0ec7f9c82923eca1d31ba4290b4 Mon Sep 17 00:00:00 2001 From: Ori Ziv Date: Mon, 27 May 2024 12:52:39 +0300 Subject: [PATCH] Audit responses. commit-id:f3de9727 --- corelib/src/test/integer_test.cairo | 36 ++++++++++--- .../src/core_libfunc_ap_change.rs | 6 +-- .../src/core_libfunc_cost_base.rs | 6 +-- .../src/invocations/int/bounded.rs | 34 ++++++------- .../src/extensions/modules/bounded_int.rs | 50 +++++++++++-------- .../extensions/modules/starknet/syscalls.rs | 27 +++++++--- tests/e2e_test_data/libfuncs/bounded_int | 42 ++++++++++++++++ 7 files changed, 140 insertions(+), 61 deletions(-) diff --git a/corelib/src/test/integer_test.cairo b/corelib/src/test/integer_test.cairo index a1345d06a18..887484ebda9 100644 --- a/corelib/src/test/integer_test.cairo +++ b/corelib/src/test/integer_test.cairo @@ -2006,13 +2006,20 @@ mod bounded_int { a: T1, b: T2 ) -> (DRR::DivT, DRR::RemT) implicits(RangeCheck) nopanic; + /// Same as `bounded_int_div_rem`, but unwraps the result into felt252s. + fn bounded_int_div_rem_unwrapped>( + a: T1, b: T2 + ) -> (felt252, felt252) { + let (q, r) = bounded_int_div_rem(a, b); + (upcast(q), upcast(r)) + } + impl SmallNumDivRemRes of DivRemRes, BoundedInt<3, 8>> { type DivT = BoundedInt<16, 85>; type RemT = BoundedInt<0, 7>; } fn div_rem_helper(a: u128, b: u128) -> (felt252, felt252) { - let (q, r) = bounded_int_div_rem(bi_value::<128, 255>(a), bi_value::<3, 8>(b)); - (upcast(q), upcast(r)) + bounded_int_div_rem_unwrapped(bi_value::<128, 255>(a), bi_value::<3, 8>(b)) } #[test] @@ -2028,8 +2035,7 @@ mod bounded_int { type RemT = BoundedInt<0, 0xfffffffffffffffffffffffffffffffe>; } fn div_rem_wide_helper(a: u128, b: u128) -> (felt252, felt252) { - let (q, r) = bounded_int_div_rem(a, bi_value::<1, 0xffffffffffffffffffffffffffffffff>(b)); - (upcast(q), upcast(r)) + bounded_int_div_rem_unwrapped(a, bi_value::<1, 0xffffffffffffffffffffffffffffffff>(b)) } #[test] @@ -2057,8 +2063,7 @@ mod bounded_int { >( a: BoundedInt ) -> (felt252, felt252) { - let (q, r) = bounded_int_div_rem::>(upcast(a), bi_const::()); - (upcast(q), upcast(r)) + bounded_int_div_rem_unwrapped::>(upcast(a), bi_const::()) } const POW_2_124: felt252 = 0x10000000000000000000000000000000; @@ -2071,6 +2076,17 @@ mod bounded_int { impl U251Pow128DivRemRes = helpers::DivRemResImpl; + // Test an extreme case where BoundedIntDivRemAlgorithm::KnownSmallLhs is used, + // and `min{b, q} = lhs_upper_sqrt - 1`. + type MaxRootLhs = + BoundedInt<1, 0x1000000000000000000000000000001000000000000000000000000000001>; + type MaxRootRhs = + BoundedInt<0x20000000000000000000000000000, { 0x100000000000000000000000000000000 - 1 }>; + impl MaxRootDivRemRes of DivRemRes { + type DivT = BoundedInt<0, 0x80000000000000000000000000000080>; + type RemT = BoundedInt<0, { 0x100000000000000000000000000000000 - 2 }>; + } + #[test] fn test_div_rem_small_quotient() { assert!(div_rem_small_quotient_helper::(bi_const::<0>()) == (0, 0)); @@ -2084,6 +2100,14 @@ mod bounded_int { assert!( div_rem_small_quotient_helper::(dividend) == (POW_2_123, POW_2_123) ); + assert!( + bounded_int_div_rem_unwrapped::< + MaxRootLhs, MaxRootRhs + >( + 0x1000000000000000000000000000001000000000000000000000000000000, + 0x1000000000000000000000000000000 + ) == (0x1000000000000000000000000000001, 0) + ); } trait BIConstrain { diff --git a/crates/cairo-lang-sierra-ap-change/src/core_libfunc_ap_change.rs b/crates/cairo-lang-sierra-ap-change/src/core_libfunc_ap_change.rs index a8fa6574500..44e72ce1164 100644 --- a/crates/cairo-lang-sierra-ap-change/src/core_libfunc_ap_change.rs +++ b/crates/cairo-lang-sierra-ap-change/src/core_libfunc_ap_change.rs @@ -372,10 +372,10 @@ pub fn core_libfunc_ap_change( | BoundedIntConcreteLibfunc::Mul(_) => vec![ApChange::Known(0)], BoundedIntConcreteLibfunc::DivRem(libfunc) => { vec![ApChange::Known( - match BoundedIntDivRemAlgorithm::new(&libfunc.lhs, &libfunc.rhs).unwrap() { + match BoundedIntDivRemAlgorithm::try_new(&libfunc.lhs, &libfunc.rhs).unwrap() { BoundedIntDivRemAlgorithm::KnownSmallRhs => 5, - BoundedIntDivRemAlgorithm::KnownSmallQuotient(_) => 6, - BoundedIntDivRemAlgorithm::KnownSmallLhs(_) => 7, + BoundedIntDivRemAlgorithm::KnownSmallQuotient { .. } => 6, + BoundedIntDivRemAlgorithm::KnownSmallLhs { .. } => 7, }, )] } diff --git a/crates/cairo-lang-sierra-gas/src/core_libfunc_cost_base.rs b/crates/cairo-lang-sierra-gas/src/core_libfunc_cost_base.rs index d7b954321c2..e53fcb14de8 100644 --- a/crates/cairo-lang-sierra-gas/src/core_libfunc_cost_base.rs +++ b/crates/cairo-lang-sierra-gas/src/core_libfunc_cost_base.rs @@ -458,14 +458,14 @@ pub fn core_libfunc_cost( | BoundedIntConcreteLibfunc::Mul(_) => vec![ConstCost::steps(0).into()], BoundedIntConcreteLibfunc::DivRem(libfunc) => { vec![ - match BoundedIntDivRemAlgorithm::new(&libfunc.lhs, &libfunc.rhs).unwrap() { + match BoundedIntDivRemAlgorithm::try_new(&libfunc.lhs, &libfunc.rhs).unwrap() { BoundedIntDivRemAlgorithm::KnownSmallRhs => { ConstCost { steps: 7, holes: 0, range_checks: 3 } } - BoundedIntDivRemAlgorithm::KnownSmallQuotient(_) => { + BoundedIntDivRemAlgorithm::KnownSmallQuotient { .. } => { ConstCost { steps: 9, holes: 0, range_checks: 4 } } - BoundedIntDivRemAlgorithm::KnownSmallLhs(_) => { + BoundedIntDivRemAlgorithm::KnownSmallLhs { .. } => { ConstCost { steps: 11, holes: 0, range_checks: 4 } } } diff --git a/crates/cairo-lang-sierra-to-casm/src/invocations/int/bounded.rs b/crates/cairo-lang-sierra-to-casm/src/invocations/int/bounded.rs index e8eed5bd526..2ac183d4afb 100644 --- a/crates/cairo-lang-sierra-to-casm/src/invocations/int/bounded.rs +++ b/crates/cairo-lang-sierra-to-casm/src/invocations/int/bounded.rs @@ -51,13 +51,13 @@ pub fn build_div_rem( ) -> Result { let [range_check, a, b] = builder.try_get_single_cells()?; - let alg = BoundedIntDivRemAlgorithm::new(lhs, rhs).unwrap(); + let alg = BoundedIntDivRemAlgorithm::try_new(lhs, rhs).unwrap(); let mut casm_builder = CasmBuilder::default(); let rc_slack = match &alg { BoundedIntDivRemAlgorithm::KnownSmallRhs => 2, - BoundedIntDivRemAlgorithm::KnownSmallQuotient(_) => 3, - BoundedIntDivRemAlgorithm::KnownSmallLhs(_) => 4, + BoundedIntDivRemAlgorithm::KnownSmallQuotient { .. } + | BoundedIntDivRemAlgorithm::KnownSmallLhs { .. } => 3, }; add_input_variables! {casm_builder, buffer(rc_slack) range_check; @@ -72,10 +72,10 @@ pub fn build_div_rem( let (q_is_small, b_or_q_bound_rc_value) = match alg { BoundedIntDivRemAlgorithm::KnownSmallRhs => (None, None), - BoundedIntDivRemAlgorithm::KnownSmallQuotient(_) => { + BoundedIntDivRemAlgorithm::KnownSmallQuotient { .. } => { (None, Some(casm_builder.alloc_var(false))) } - BoundedIntDivRemAlgorithm::KnownSmallLhs(_) => { + BoundedIntDivRemAlgorithm::KnownSmallLhs { .. } => { (Some(casm_builder.alloc_var(false)), Some(casm_builder.alloc_var(false))) } }; @@ -113,32 +113,26 @@ pub fn build_div_rem( // For this case `q + 1 <= q_max + 1 <= 2**128` and `b < rhs.upper` therefore // `(q + 1) * b < 2**128 * rhs.upper <= prime`. } - BoundedIntDivRemAlgorithm::KnownSmallQuotient(q_bound) => { + BoundedIntDivRemAlgorithm::KnownSmallQuotient { q_upper_bound } => { let b_or_q_bound_rc_value = b_or_q_bound_rc_value.unwrap(); // For this case `(q + 1) <= q_bound`, and `b < rhs.upper <= 2**128` therefore // `(q + 1) * b < q_bound * 2**128 < prime`. casm_build_extend! {casm_builder, - const u128_bound_minus_q_upper = (BigInt::one().shl(128) - q_bound) as BigInt; + const u128_bound_minus_q_upper = (BigInt::one().shl(128) - q_upper_bound) as BigInt; assert b_or_q_bound_rc_value = q + u128_bound_minus_q_upper; assert b_or_q_bound_rc_value = *(range_check++); } } - BoundedIntDivRemAlgorithm::KnownSmallLhs(lhs_upper_sqrt) => { + BoundedIntDivRemAlgorithm::KnownSmallLhs { lhs_upper_sqrt } => { let q_is_small = q_is_small.unwrap(); let b_or_q_bound_rc_value = b_or_q_bound_rc_value.unwrap(); casm_build_extend! {casm_builder, - // For this case we know that `(lhs_upper_sqrt + 1) * 2**128 < prime`. - // First note that `(q + 1) * b <= (min(q, b) + 1) * max(q, b)`, since: - // ``` - // max((min(q, b) + 1) * max(q, b), (max(q, b) + 1) * min(q, b)) == - // max(min(q, b) * max(q, b) + max(q, b), max(q, b) * min(q, b) + min(q, b)) == - // min(q, b) * max(q, b) + max(max(q, b), min(q, b)) == - // min(q, b) * max(q, b) + max(q, b) == (min(q, b) + 1) * max(q, b) - // ``` - // Since `b * q < lhs.upper`, `min(b, q) <= sqrt(lhs.upper)`. - // Since `b` and `q` are less than 2**128, `max(b, q) < 2**128`. - // Therefore `(min(q, b) + 1) * max(q, b) <= (lhs_upper_sqrt + 1) * 2**128 < prime`. - // We can now guess which whether `b` or `q` is small enough and verify. + // Note that for a honest prover, `min{q, b} < root`, as otherwise + // `lhs_upper > a >= q * b >= root ** 2` (and on the other hand, + // by the definition of `root`: `root ** 2 >= lhs_upper`). + // Therefore we require `min{q, b} < root`, which guarantees that: + // q * b + r < min{q, b} * max{q, b} + 2**128 <= + // (root - 1) * 2**128 + 2**128 <= root * 2**128 < prime. const limiter_bound = lhs_upper_sqrt.clone(); hint TestLessThan {lhs: q, rhs: limiter_bound} into {dst: q_is_small}; const u128_bound_minus_limiter_bound = (BigInt::one().shl(128) - lhs_upper_sqrt) as BigInt; diff --git a/crates/cairo-lang-sierra/src/extensions/modules/bounded_int.rs b/crates/cairo-lang-sierra/src/extensions/modules/bounded_int.rs index 1c4c783e9a3..fde1ed9a3df 100644 --- a/crates/cairo-lang-sierra/src/extensions/modules/bounded_int.rs +++ b/crates/cairo-lang-sierra/src/extensions/modules/bounded_int.rs @@ -168,7 +168,7 @@ impl NamedLibfunc for BoundedIntDivRemLibfunc { return Err(SpecializationError::UnsupportedGenericArg); } // Making sure the algorithm is runnable. - if BoundedIntDivRemAlgorithm::new(&lhs_range, &rhs_range).is_none() { + if BoundedIntDivRemAlgorithm::try_new(&lhs_range, &rhs_range).is_none() { return Err(SpecializationError::UnsupportedGenericArg); } let quotient_min = lhs_range.lower / (&rhs_range.upper - 1); @@ -226,34 +226,43 @@ pub enum BoundedIntDivRemAlgorithm { /// The rhs is small enough to be multiplied by `2**128` without wraparound. KnownSmallRhs, /// The quotient is small enough to be multiplied by `2**128` without wraparound. - KnownSmallQuotient(BigInt), + KnownSmallQuotient { q_upper_bound: BigInt }, /// The lhs is small enough so that its square root plus 1 can be multiplied by `2**128` /// without wraparound. - KnownSmallLhs(BigInt), + /// `lhs_upper_sqrt` is the square root of the upper bound of the lhs, rounded up. + KnownSmallLhs { lhs_upper_sqrt: BigInt }, } impl BoundedIntDivRemAlgorithm { /// Returns the algorithm to use for division and remainder of bounded integers. /// Fails if the div_rem of the ranges is not supported yet. /// /// Assumption: `lhs` is non-negative and `rhs` is positive. - pub fn new(lhs: &Range, rhs: &Range) -> Option { + pub fn try_new(lhs: &Range, rhs: &Range) -> Option { let prime = Felt252::prime().to_bigint().unwrap(); let q_max = (&lhs.upper - 1) / &rhs.lower; let u128_limit = BigInt::one().shl(128); // `q` is range checked in all algorithm variants, so `q_max` must be smaller than `2**128`. require(q_max < u128_limit)?; - // `r` is range checked in all algorithm variants, so `lhs.upper` must be at most `2**128`. - require(rhs.upper <= u128_limit)?; + // `r` is range checked in all algorithm variants, so `rhs.upper` must be at most + // `2**128 + 1`. + require(rhs.upper <= &u128_limit + 1)?; if &rhs.upper * &u128_limit < prime { return Some(Self::KnownSmallRhs); } let q_upper_bound = q_max + 1; if &q_upper_bound * &u128_limit < prime { - return Some(Self::KnownSmallQuotient(q_upper_bound)); + return Some(Self::KnownSmallQuotient { q_upper_bound }); } - let root = lhs.upper.sqrt(); - if (&root + 1) * &u128_limit < prime { - return Some(Self::KnownSmallLhs(root)); + let mut lhs_upper_sqrt = lhs.upper.sqrt(); + // Round lhs_upper_sqrt up. + if lhs_upper_sqrt.pow(2) != lhs.upper { + lhs_upper_sqrt += 1; + } + if &lhs_upper_sqrt * &u128_limit < prime { + // Make sure `lhs_upper_sqrt < 2**128`, since the value bounded by root is range + // checked. + require(lhs_upper_sqrt < u128_limit)?; + return Some(Self::KnownSmallLhs { lhs_upper_sqrt }); } // No algorithm found. None @@ -280,15 +289,12 @@ impl NamedLibfunc for BoundedIntConstrainLibfunc { _ => Err(SpecializationError::WrongNumberOfGenericArgs), }?; let range = Range::from_type(context, ty.clone())?; - let under_range = Range::half_open(range.lower, boundary.clone()); - let over_range = Range::half_open(boundary.clone(), range.upper); - require( - under_range.size() >= BigInt::one() - && over_range.size() >= BigInt::one() - && under_range.is_small_range() - && over_range.is_small_range(), - ) - .ok_or(SpecializationError::UnsupportedGenericArg)?; + require(&range.lower < boundary && boundary < &range.upper) + .ok_or(SpecializationError::UnsupportedGenericArg)?; + let low_range = Range::half_open(range.lower, boundary.clone()); + let high_range = Range::half_open(boundary.clone(), range.upper); + require(low_range.is_small_range() && high_range.is_small_range()) + .ok_or(SpecializationError::UnsupportedGenericArg)?; let range_check_type = context.get_concrete_type(RangeCheckType::id(), &[])?; let branch_signature = |rng: Range| { Ok(BranchSignature { @@ -307,7 +313,7 @@ impl NamedLibfunc for BoundedIntConstrainLibfunc { ParamSignature::new(range_check_type.clone()).with_allow_add_const(), ParamSignature::new(ty.clone()), ], - branch_signatures: vec![branch_signature(under_range)?, branch_signature(over_range)?], + branch_signatures: vec![branch_signature(low_range)?, branch_signature(high_range)?], fallthrough: Some(0), }) } @@ -349,8 +355,8 @@ fn specialize_helper( Range::from_type(context, lhs.clone())?, Range::from_type(context, rhs.clone())?, ); - Ok(LibfuncSignature::new_non_branch( - vec![lhs, rhs], + Ok(LibfuncSignature::new_non_branch_ex( + vec![ParamSignature::new(lhs), ParamSignature::new(rhs).with_allow_const()], vec![OutputVarInfo { ty: bounded_int_ty(context, min_result, max_result)?, ref_info: OutputVarReferenceInfo::Deferred(DeferredOutputKind::Generic), diff --git a/crates/cairo-lang-sierra/src/extensions/modules/starknet/syscalls.rs b/crates/cairo-lang-sierra/src/extensions/modules/starknet/syscalls.rs index 58723905430..2a50d8ecff9 100644 --- a/crates/cairo-lang-sierra/src/extensions/modules/starknet/syscalls.rs +++ b/crates/cairo-lang-sierra/src/extensions/modules/starknet/syscalls.rs @@ -12,7 +12,7 @@ use crate::extensions::lib_func::{ SierraApChange, SignatureSpecializationContext, }; use crate::extensions::modules::get_u256_type; -use crate::extensions::utils::{fixed_size_array_ty, reinterpret_cast_signature}; +use crate::extensions::utils::fixed_size_array_ty; use crate::extensions::{ NamedType, NoGenericArgsGenericLibfunc, NoGenericArgsGenericType, OutputVarReferenceInfo, SpecializationError, @@ -207,9 +207,15 @@ impl NoGenericArgsGenericLibfunc for Sha256StateHandleInitLibfunc { &self, context: &dyn SignatureSpecializationContext, ) -> Result { - Ok(reinterpret_cast_signature( - sha256_state_handle_unwrapped_type(context)?, - context.get_concrete_type(Sha256StateHandleType::id(), &[])?, + Ok(LibfuncSignature::new_non_branch_ex( + vec![ + ParamSignature::new(sha256_state_handle_unwrapped_type(context)?).with_allow_all(), + ], + vec![OutputVarInfo { + ty: context.get_concrete_type(Sha256StateHandleType::id(), &[])?, + ref_info: OutputVarReferenceInfo::Deferred(DeferredOutputKind::Generic), + }], + SierraApChange::Known { new_vars_only: false }, )) } } @@ -224,9 +230,16 @@ impl NoGenericArgsGenericLibfunc for Sha256StateHandleDigestLibfunc { &self, context: &dyn SignatureSpecializationContext, ) -> Result { - Ok(reinterpret_cast_signature( - context.get_concrete_type(Sha256StateHandleType::id(), &[])?, - sha256_state_handle_unwrapped_type(context)?, + Ok(LibfuncSignature::new_non_branch_ex( + vec![ + ParamSignature::new(context.get_concrete_type(Sha256StateHandleType::id(), &[])?) + .with_allow_all(), + ], + vec![OutputVarInfo { + ty: sha256_state_handle_unwrapped_type(context)?, + ref_info: OutputVarReferenceInfo::Deferred(DeferredOutputKind::Generic), + }], + SierraApChange::Known { new_vars_only: false }, )) } } diff --git a/tests/e2e_test_data/libfuncs/bounded_int b/tests/e2e_test_data/libfuncs/bounded_int index f5c0b94097f..321cf301b89 100644 --- a/tests/e2e_test_data/libfuncs/bounded_int +++ b/tests/e2e_test_data/libfuncs/bounded_int @@ -115,6 +115,48 @@ test::foo@0([0]: i8, [1]: i8) -> (BoundedInt<-16256, 16384>); //! > ========================================================================== +//! > bounded_int_add with const libfunc + +//! > test_runner_name +SmallE2ETestRunner + +//! > cairo +extern type BoundedInt; +const MIN_I8: felt252 = -128; +const MAX_I8: felt252 = 127; +type AddType = BoundedInt<{MIN_I8 + MIN_I8}, {MAX_I8 + MAX_I8}>; + +extern fn bounded_int_add(a: T1, b: T2) -> AddType nopanic; + +fn foo(a: i8) -> AddType { + bounded_int_add(a, 50_i8) +} + +//! > casm +[ap + 0] = [fp + -3] + 50, ap++; +ret; + +//! > function_costs +test::foo: OrderedHashMap({Const: 100}) + +//! > sierra_code +type i8 = i8 [storable: true, drop: true, dup: true, zero_sized: false]; +type BoundedInt<-256, 254> = BoundedInt<-256, 254> [storable: true, drop: true, dup: true, zero_sized: false]; +type Const = Const [storable: false, drop: false, dup: false, zero_sized: false]; + +libfunc const_as_immediate> = const_as_immediate>; +libfunc bounded_int_add = bounded_int_add; +libfunc store_temp> = store_temp>; + +const_as_immediate>() -> ([1]); // 0 +bounded_int_add([0], [1]) -> ([2]); // 1 +store_temp>([2]) -> ([2]); // 2 +return([2]); // 3 + +test::foo@0([0]: i8) -> (BoundedInt<-256, 254>); + +//! > ========================================================================== + //! > bounded_int_div_rem libfunc //! > test_runner_name