Skip to content

Commit

Permalink
Add MathError for math operations (#855)
Browse files Browse the repository at this point in the history
* Use only option for Memory.get

* Fix some tests + refactor range_check validation

* use proper error for get_memory_holes

* Move MaybeRelocatable methods get_int_ref & get_reloctable to Option

* Fix tests

* Clippy

* Fix `CairoRunner::write_output` so that it prints missing and relocatable values (#853)

* Print relocatables & missing members in write_output

* Add test

* Move errors outputed by math_utils to MathError

* Start moving relocatable operations to MathError

* Fix tests

* Remove math-related errors from vm error

* Move conversion errors to MathError

* Move type conversions to MathError

* Remove unused errors

* Clippy

* Clippy

* Simplify addition

* Simplify addition

* Clippy

* Add math_errors.rs

* Check for overflows in relocatable operations (#859)

* Catch possible overflows in Relocatable::add

* Move sub implementations to trait impl

* Swap sub_usize for - operator

* Vheck possible overflows in Add<i32>

* Fix should_panic test

* remove referenced add

* Replace Relocatable methods for trait implementations

* Catch overflows in mayberelocatable operations

* Fix keccak

* Clippy
  • Loading branch information
fmoletta authored Mar 6, 2023
1 parent d5a7bea commit 1de3e2b
Show file tree
Hide file tree
Showing 37 changed files with 446 additions and 466 deletions.
22 changes: 11 additions & 11 deletions src/hint_processor/builtin_hint_processor/blake2s_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,10 @@ output_ptr should point to the middle of an instance, right after initial_state,
which should all have a value at this point, and right before the output portion which will be
written by this function.*/
fn compute_blake2s_func(vm: &mut VirtualMachine, output_ptr: Relocatable) -> Result<(), HintError> {
let h = get_fixed_size_u32_array::<8>(&vm.get_integer_range(output_ptr.sub_usize(26)?, 8)?)?;
let message =
get_fixed_size_u32_array::<16>(&vm.get_integer_range(output_ptr.sub_usize(18)?, 16)?)?;
let t = felt_to_u32(vm.get_integer(output_ptr.sub_usize(2)?)?.as_ref())?;
let f = felt_to_u32(vm.get_integer(output_ptr.sub_usize(1)?)?.as_ref())?;
let h = get_fixed_size_u32_array::<8>(&vm.get_integer_range((output_ptr - 26)?, 8)?)?;
let message = get_fixed_size_u32_array::<16>(&vm.get_integer_range((output_ptr - 18)?, 16)?)?;
let t = felt_to_u32(vm.get_integer((output_ptr - 2)?)?.as_ref())?;
let f = felt_to_u32(vm.get_integer((output_ptr - 1)?)?.as_ref())?;
let new_state =
get_maybe_relocatable_array_from_u32(&blake2s_compress(&h, &message, t, 0, f, 0));
vm.load_data(output_ptr, &new_state)
Expand Down Expand Up @@ -155,7 +154,7 @@ pub fn blake2s_add_uint256(
}
//Insert second batch of data
let data = get_maybe_relocatable_array_from_felt(&inner_data);
vm.load_data(data_ptr + 4, &data)
vm.load_data((data_ptr + 4)?, &data)
.map_err(HintError::Memory)?;
Ok(())
}
Expand Down Expand Up @@ -198,15 +197,15 @@ pub fn blake2s_add_uint256_bigend(
}
//Insert second batch of data
let data = get_maybe_relocatable_array_from_felt(&inner_data);
vm.load_data(data_ptr + 4, &data)
vm.load_data((data_ptr + 4)?, &data)
.map_err(HintError::Memory)?;
Ok(())
}

#[cfg(test)]
mod tests {
use super::*;
use crate::vm::errors::vm_errors::VirtualMachineError;
use crate::types::errors::math_errors::MathError;
use crate::vm::vm_memory::memory_segments::MemorySegmentManager;
use crate::{
any_box,
Expand Down Expand Up @@ -238,9 +237,10 @@ mod tests {
//Execute the hint
assert_matches!(
run_hint!(vm, ids_data, hint_code),
Err(HintError::Internal(VirtualMachineError::CantSubOffset(
5, 26
)))
Err(HintError::Math(MathError::RelocatableSubNegOffset(
x,
y
))) if x == relocatable!(2,5) && y == 26
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,15 @@ use crate::{
hint_processor_definition::HintReference,
},
serde::deserialize_program::ApTracking,
types::relocatable::MaybeRelocatable,
types::{errors::math_errors::MathError, relocatable::MaybeRelocatable},
vm::{
errors::{hint_errors::HintError, vm_errors::VirtualMachineError},
vm_core::VirtualMachine,
},
};
use felt::Felt;
use num_traits::{ToPrimitive, Zero};
use std::{borrow::Cow, collections::HashMap, ops::Add};
use std::{borrow::Cow, collections::HashMap};

// Constants in package "starkware.cairo.common.cairo_keccak.keccak".
const BYTES_IN_WORD: &str = "starkware.cairo.common.cairo_keccak.keccak.BYTES_IN_WORD";
Expand Down Expand Up @@ -53,7 +53,7 @@ pub fn keccak_write_args(
.map_err(HintError::Memory)?;

let high_args: Vec<_> = high_args.into_iter().map(MaybeRelocatable::from).collect();
vm.write_arg(inputs_ptr.add(2_i32), &high_args)
vm.write_arg((inputs_ptr + 2_i32)?, &high_args)
.map_err(HintError::Memory)?;

Ok(())
Expand Down Expand Up @@ -144,7 +144,7 @@ pub fn block_permutation(

let keccak_state_size_felts = keccak_state_size_felts.to_usize().unwrap();
let values = vm.get_range(
keccak_ptr.sub_usize(keccak_state_size_felts)?,
(keccak_ptr - keccak_state_size_felts)?,
keccak_state_size_felts,
);

Expand Down Expand Up @@ -233,9 +233,9 @@ pub(crate) fn maybe_reloc_vec_to_u64_array(
.iter()
.map(|n| match n {
Some(Cow::Owned(MaybeRelocatable::Int(ref num)))
| Some(Cow::Borrowed(MaybeRelocatable::Int(ref num))) => {
num.to_u64().ok_or(VirtualMachineError::BigintToU64Fail)
}
| Some(Cow::Borrowed(MaybeRelocatable::Int(ref num))) => num
.to_u64()
.ok_or_else(|| MathError::FeltToU64Conversion(num.clone()).into()),
_ => Err(VirtualMachineError::ExpectedIntAtRange(
n.as_ref().map(|x| x.as_ref().to_owned()),
)),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ pub fn dict_write(
let tracker = dict.get_tracker_mut(dict_ptr)?;
//dict_ptr is a pointer to a struct, with the ordered fields (key, prev_value, new_value),
//dict_ptr.prev_value will be equal to dict_ptr + 1
let dict_ptr_prev_value = dict_ptr + 1_i32;
let dict_ptr_prev_value = (dict_ptr + 1_i32)?;
//Tracker set to track next dictionary entry
tracker.current_ptr.offset += DICT_ACCESS_SIZE;
//Get previous value
Expand Down
13 changes: 5 additions & 8 deletions src/hint_processor/builtin_hint_processor/find_element_hint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,8 @@ use crate::{
hint_processor_utils::felt_to_usize,
},
serde::deserialize_program::ApTracking,
types::exec_scope::ExecutionScopes,
vm::{
errors::{hint_errors::HintError, vm_errors::VirtualMachineError},
vm_core::VirtualMachine,
},
types::{errors::math_errors::MathError, exec_scope::ExecutionScopes},
vm::{errors::hint_errors::HintError, vm_core::VirtualMachine},
};
use felt::Felt;
use num_traits::{Signed, ToPrimitive};
Expand All @@ -39,7 +36,7 @@ pub fn find_element(
if let Some(find_element_index_value) = find_element_index {
let find_element_index_usize = felt_to_usize(&find_element_index_value)?;
let found_key = vm
.get_integer(array_start + (elm_size * find_element_index_usize))
.get_integer((array_start + (elm_size * find_element_index_usize))?)
.map_err(|_| HintError::KeyNotFound)?;

if found_key.as_ref() != key.as_ref() {
Expand Down Expand Up @@ -67,11 +64,11 @@ pub fn find_element(
}
let n_elms_iter: i32 = n_elms
.to_i32()
.ok_or_else(|| VirtualMachineError::OffsetExceeded(n_elms.into_owned()))?;
.ok_or_else(|| MathError::FeltToI32Conversion(n_elms.into_owned()))?;

for i in 0..n_elms_iter {
let iter_key = vm
.get_integer(array_start + (elm_size * i as usize))
.get_integer((array_start + (elm_size * i as usize))?)
.map_err(|_| HintError::KeyNotFound)?;

if iter_key.as_ref() == key.as_ref() {
Expand Down
2 changes: 1 addition & 1 deletion src/hint_processor/builtin_hint_processor/keccak_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ pub fn unsafe_keccak_finalize(
offset: keccak_state_ptr.offset + 1,
})?;

let n_elems = end_ptr.sub(&start_ptr)?;
let n_elems = (end_ptr - start_ptr)?;

let mut keccak_input = Vec::new();
let range = vm.get_integer_range(start_ptr, n_elems)?;
Expand Down
6 changes: 3 additions & 3 deletions src/hint_processor/builtin_hint_processor/math_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,10 @@ pub fn assert_le_felt(
let (q_0, r_0) = (lengths_and_indices[0].0).div_mod_floor(prime_over_3_high);
let (q_1, r_1) = (lengths_and_indices[1].0).div_mod_floor(prime_over_2_high);

vm.insert_value(range_check_ptr + 1_i32, q_0)?;
vm.insert_value((range_check_ptr + 1_i32)?, q_0)?;
vm.insert_value(range_check_ptr, r_0)?;
vm.insert_value(range_check_ptr + 3_i32, q_1)?;
vm.insert_value(range_check_ptr + 2_i32, r_1)?;
vm.insert_value((range_check_ptr + 3_i32)?, q_1)?;
vm.insert_value((range_check_ptr + 2_i32)?, r_1)?;
Ok(())
}

Expand Down
4 changes: 3 additions & 1 deletion src/hint_processor/builtin_hint_processor/pow_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ pub fn pow(
ap_tracking: &ApTracking,
) -> Result<(), HintError> {
let prev_locs_exp = vm
.get_integer(get_relocatable_from_var_name("prev_locs", vm, ids_data, ap_tracking)? + 4_i32)
.get_integer(
(get_relocatable_from_var_name("prev_locs", vm, ids_data, ap_tracking)? + 4_i32)?,
)
.map_err(|_| {
HintError::IdentifierHasNoMember("prev_locs".to_string(), "exp".to_string())
})?;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@ impl BigInt3<'_> {
d0: vm.get_integer(addr).map_err(|_| {
HintError::IdentifierHasNoMember(name.to_string(), "d0".to_string())
})?,
d1: vm.get_integer(addr + 1).map_err(|_| {
d1: vm.get_integer((addr + 1)?).map_err(|_| {
HintError::IdentifierHasNoMember(name.to_string(), "d1".to_string())
})?,
d2: vm.get_integer(addr + 2).map_err(|_| {
d2: vm.get_integer((addr + 2)?).map_err(|_| {
HintError::IdentifierHasNoMember(name.to_string(), "d2".to_string())
})?,
})
Expand Down Expand Up @@ -91,7 +91,7 @@ pub fn bigint_to_uint256(
) -> Result<(), HintError> {
let x_struct = get_relocatable_from_var_name("x", vm, ids_data, ap_tracking)?;
let d0 = vm.get_integer(x_struct)?;
let d1 = vm.get_integer(x_struct + 1_i32)?;
let d1 = vm.get_integer((x_struct + 1_i32)?)?;
let d0 = d0.as_ref();
let d1 = d1.as_ref();
let base_86 = constants
Expand Down
4 changes: 2 additions & 2 deletions src/hint_processor/builtin_hint_processor/secp/ec_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ impl EcPoint<'_> {
let point_addr = get_relocatable_from_var_name(name, vm, ids_data, ap_tracking)?;
Ok(EcPoint {
x: BigInt3::from_base_addr(point_addr, &format!("{}.x", name), vm)?,
y: BigInt3::from_base_addr(point_addr + 3, &format!("{}.y", name), vm)?,
y: BigInt3::from_base_addr((point_addr + 3)?, &format!("{}.y", name), vm)?,
})
}
}
Expand Down Expand Up @@ -71,7 +71,7 @@ pub fn ec_negate(
.to_bigint();

//ids.point
let point_y = get_relocatable_from_var_name("point", vm, ids_data, ap_tracking)? + 3i32;
let point_y = (get_relocatable_from_var_name("point", vm, ids_data, ap_tracking)? + 3i32)?;
let y_bigint3 = BigInt3::from_base_addr(point_y, "point.y", vm)?;
let y = pack(y_bigint3);
let value = (-y).mod_floor(&secp_p);
Expand Down
8 changes: 3 additions & 5 deletions src/hint_processor/builtin_hint_processor/secp/signature.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ pub fn get_point_from_x(
#[cfg(test)]
mod tests {
use super::*;
use crate::types::errors::math_errors::MathError;
use crate::vm::vm_memory::memory_segments::MemorySegmentManager;
use crate::{
any_box,
Expand All @@ -160,10 +161,7 @@ mod tests {
},
types::{exec_scope::ExecutionScopes, relocatable::MaybeRelocatable},
utils::test_utils::*,
vm::{
errors::{memory_errors::MemoryError, vm_errors::VirtualMachineError},
vm_memory::memory::Memory,
},
vm::{errors::memory_errors::MemoryError, vm_memory::memory::Memory},
};
use assert_matches::assert_matches;
use num_traits::Zero;
Expand Down Expand Up @@ -222,7 +220,7 @@ mod tests {
.collect()
),
Err(
HintError::Internal(VirtualMachineError::SafeDivFailBigInt(
HintError::Math(MathError::SafeDivFailBigInt(
x,
y,
)
Expand Down
31 changes: 16 additions & 15 deletions src/hint_processor/builtin_hint_processor/set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,8 @@ use crate::{
hint_processor_definition::HintReference,
},
serde::deserialize_program::ApTracking,
types::relocatable::MaybeRelocatable,
vm::{
errors::{hint_errors::HintError, vm_errors::VirtualMachineError},
vm_core::VirtualMachine,
},
types::{errors::math_errors::MathError, relocatable::MaybeRelocatable},
vm::{errors::hint_errors::HintError, vm_core::VirtualMachine},
};
use felt::Felt;
use num_traits::{One, ToPrimitive, Zero};
Expand All @@ -22,14 +19,18 @@ pub fn set_add(
ap_tracking: &ApTracking,
) -> Result<(), HintError> {
let set_ptr = get_ptr_from_var_name("set_ptr", vm, ids_data, ap_tracking)?;
let elm_size = get_integer_from_var_name("elm_size", vm, ids_data, ap_tracking)?
.to_usize()
.ok_or(VirtualMachineError::BigintToUsizeFail)?;
let elm_size =
get_integer_from_var_name("elm_size", vm, ids_data, ap_tracking).and_then(|x| {
x.to_usize()
.ok_or_else(|| MathError::FeltToUsizeConversion(x.into_owned()).into())
})?;
let elm_ptr = get_ptr_from_var_name("elm_ptr", vm, ids_data, ap_tracking)?;
let set_end_ptr = get_ptr_from_var_name("set_end_ptr", vm, ids_data, ap_tracking)?;

if elm_size.is_zero() {
Err(VirtualMachineError::ValueNotPositive(Felt::new(elm_size)))?;
Err(HintError::AssertionFailed(String::from(
"assert ids.elm_size > 0",
)))?;
}
let elm = vm.get_range(elm_ptr, elm_size);

Expand All @@ -40,10 +41,10 @@ pub fn set_add(
));
}

let range_limit = set_end_ptr.sub(&set_ptr)?;
let range_limit = (set_end_ptr - set_ptr)?;

for i in (0..range_limit).step_by(elm_size) {
let set_iter = vm.get_range(set_ptr + i, elm_size);
let set_iter = vm.get_range((set_ptr + i)?, elm_size);

if set_iter == elm {
insert_value_from_var_name(
Expand Down Expand Up @@ -154,7 +155,7 @@ mod tests {
let (mut vm, ids_data) = init_vm_ids_data(None, Some(-2), None, None);
assert_matches!(
run_hint!(vm, ids_data, HINT_CODE),
Err(HintError::Internal(VirtualMachineError::BigintToUsizeFail))
Err(HintError::Math(MathError::FeltToUsizeConversion(_)))
);
}

Expand All @@ -163,9 +164,9 @@ mod tests {
let (mut vm, ids_data) = init_vm_ids_data(None, Some(0), None, None);
assert_matches!(
run_hint!(vm, ids_data, HINT_CODE),
Err(HintError::Internal(VirtualMachineError::ValueNotPositive(
int
))) if int.is_zero()
Err(HintError::AssertionFailed(
m
)) if m == *"assert ids.elm_size > 0"
);
}
#[test]
Expand Down
2 changes: 1 addition & 1 deletion src/hint_processor/builtin_hint_processor/sha256_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ pub fn sha256_main(
let mut message: Vec<u8> = Vec::with_capacity(4 * SHA256_INPUT_CHUNK_SIZE_FELTS);

for i in 0..SHA256_INPUT_CHUNK_SIZE_FELTS {
let input_element = vm.get_integer(input_ptr + i)?;
let input_element = vm.get_integer((input_ptr + i)?)?;
let bytes = felt_to_u32(input_element.as_ref())?.to_be_bytes();
message.extend(bytes);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ pub fn squash_dict_inner_continue_loop(
};
//loop_temps.delta_minus1 = loop_temps + 3 as it is the fourth field of the struct
//Insert loop_temps.delta_minus1 into memory
let should_continue_addr = loop_temps_addr + 3_i32;
let should_continue_addr = (loop_temps_addr + 3_i32)?;
vm.insert_value(should_continue_addr, should_continue)
.map_err(HintError::Memory)
}
Expand Down Expand Up @@ -265,7 +265,7 @@ pub fn squash_dict(
//A map from key to the list of indices accessing it.
let mut access_indices = HashMap::<Felt, Vec<Felt>>::new();
for i in 0..n_accesses_usize {
let key_addr = address + DICT_ACCESS_SIZE * i;
let key_addr = (address + DICT_ACCESS_SIZE * i)?;
let key = vm
.get_integer(key_addr)
.map_err(|_| MemoryError::ExpectedInteger(key_addr))?;
Expand Down
Loading

1 comment on commit 1de3e2b

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.30.

Benchmark suite Current: 1de3e2b Previous: d5a7bea Ratio
cairo_run(cairo_programs/benchmarks/compare_arrays_200000.json) 884503786 ns/iter (± 28972997) 637231489 ns/iter (± 1969874) 1.39
cairo_run(cairo_programs/benchmarks/factorial_multirun.json) 329271401 ns/iter (± 12461198) 240630877 ns/iter (± 664836) 1.37
cairo_run(cairo_programs/benchmarks/fibonacci_1000_multirun.json) 139728837 ns/iter (± 7714931) 101045574 ns/iter (± 351582) 1.38
cairo_run(cairo_programs/benchmarks/integration_builtins.json) 537004106 ns/iter (± 23605098) 386937505 ns/iter (± 1718909) 1.39
cairo_run(cairo_programs/benchmarks/linear_search.json) 103559057 ns/iter (± 6181470) 79573059 ns/iter (± 5279566) 1.30
cairo_run(cairo_programs/benchmarks/keccak_integration_benchmark.json) 1682060789 ns/iter (± 48924206) 1240800087 ns/iter (± 8607617) 1.36
cairo_run(cairo_programs/benchmarks/secp_integration_benchmark.json) 1869022503 ns/iter (± 80441664) 1348564409 ns/iter (± 2660517) 1.39
cairo_run(cairo_programs/benchmarks/blake2s_integration_benchmark.json) 1534652558 ns/iter (± 71555462) 1082080349 ns/iter (± 8505744) 1.42
cairo_run(cairo_programs/benchmarks/dict_integration_benchmark.json) 1000563302 ns/iter (± 32632253) 757723182 ns/iter (± 8971950) 1.32
cairo_run(cairo_programs/benchmarks/math_integration_benchmark.json) 493033960 ns/iter (± 24944368) 377129902 ns/iter (± 871064) 1.31
cairo_run(cairo_programs/benchmarks/math_cmp_and_pow_integration_benchmark.json) 23903260 ns/iter (± 1455740) 18089601 ns/iter (± 107357) 1.32
cairo_run(cairo_programs/benchmarks/operations_with_data_structures_benchmarks.json) 2213835247 ns/iter (± 67634907) 1610375764 ns/iter (± 30312668) 1.37
cairo_run(cairo_programs/benchmarks/uint256_integration_benchmark.json) 1502525606 ns/iter (± 61358205) 1081656715 ns/iter (± 3755716) 1.39
cairo_run(cairo_programs/benchmarks/set_integration_benchmark.json) 169453151 ns/iter (± 10920855) 121522873 ns/iter (± 418889) 1.39

This comment was automatically generated by workflow using github-action-benchmark.

CC: @unbalancedparentheses

Please sign in to comment.