Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify Memory::get return type to Option #852

Merged
merged 8 commits into from
Feb 23, 2023
Merged
42 changes: 24 additions & 18 deletions src/hint_processor/builtin_hint_processor/blake2s_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,9 @@ mod tests {
//Execute the hint
assert_matches!(
run_hint!(vm, ids_data, hint_code),
Err(HintError::Internal(VirtualMachineError::ExpectedInteger(x))) if x == MaybeRelocatable::from((2,0))
Err(HintError::Internal(VirtualMachineError::UnknownMemoryCell(
x
))) if x == Relocatable::from((2, 0))
);
}

Expand Down Expand Up @@ -335,7 +337,7 @@ mod tests {
//Execute the hint
assert_matches!(
run_hint!(vm, ids_data, hint_code),
Err(HintError::Internal(VirtualMachineError::ExpectedInteger(x))) if x == MaybeRelocatable::from((2,0))
Err(HintError::Internal(VirtualMachineError::ExpectedInteger(x))) if x == Relocatable::from((2,0))
);
}

Expand Down Expand Up @@ -449,10 +451,11 @@ mod tests {
((2, 6), 0),
((2, 7), 0)
];
assert_eq!(
vm.segments.memory.get(&MaybeRelocatable::from((2, 8))),
Ok(None)
);
assert!(vm
.segments
.memory
.get(&MaybeRelocatable::from((2, 8)))
.is_none());
}

#[test]
Expand Down Expand Up @@ -480,10 +483,11 @@ mod tests {
((2, 6), 0),
((2, 7), 0)
];
assert_eq!(
vm.segments.memory.get(&MaybeRelocatable::from((2, 8))),
Ok(None)
);
assert!(vm
.segments
.memory
.get(&MaybeRelocatable::from((2, 8)))
.is_none());
}

#[test]
Expand Down Expand Up @@ -511,10 +515,11 @@ mod tests {
((2, 6), 0),
((2, 7), 0)
];
assert_eq!(
vm.segments.memory.get(&MaybeRelocatable::from((2, 8))),
Ok(None)
);
assert!(vm
.segments
.memory
.get(&MaybeRelocatable::from((2, 8)))
.is_none());
}

#[test]
Expand Down Expand Up @@ -542,9 +547,10 @@ mod tests {
((2, 6), 0),
((2, 7), 20)
];
assert_eq!(
vm.segments.memory.get(&MaybeRelocatable::from((2, 8))),
Ok(None)
);
assert!(vm
.segments
.memory
.get(&MaybeRelocatable::from((2, 8)))
.is_none());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -801,7 +801,9 @@ mod tests {
let ids_data = non_continuous_ids_data![("keccak_state", -7), ("high", -3), ("low", -2)];
assert_matches!(
run_hint!(vm, ids_data, hint_code),
Err(HintError::Internal(VirtualMachineError::NoneInMemoryRange))
Err(HintError::Internal(VirtualMachineError::UnknownMemoryCell(
_
)))
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,6 @@ mod tests {
.memory
.get(&MaybeRelocatable::from((1, 1)))
.unwrap()
.unwrap()
.as_ref(),
&MaybeRelocatable::from(12)
);
Expand Down
41 changes: 5 additions & 36 deletions src/hint_processor/builtin_hint_processor/keccak_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,8 @@ use crate::{
hint_processor_definition::HintReference,
},
serde::deserialize_program::ApTracking,
types::{
exec_scope::ExecutionScopes,
relocatable::{MaybeRelocatable, Relocatable},
},
vm::{
errors::{hint_errors::HintError, vm_errors::VirtualMachineError},
vm_core::VirtualMachine,
},
types::{exec_scope::ExecutionScopes, relocatable::Relocatable},
vm::{errors::hint_errors::HintError, vm_core::VirtualMachine},
};
use felt::Felt;
use num_traits::{One, Signed, ToPrimitive};
Expand Down Expand Up @@ -150,27 +144,12 @@ pub fn unsafe_keccak_finalize(
offset: keccak_state_ptr.offset + 1,
})?;

// this is not very nice code, we should consider adding the sub() method for Relocatable's
let maybe_rel_start_ptr = MaybeRelocatable::RelocatableValue(start_ptr);
let maybe_rel_end_ptr = MaybeRelocatable::RelocatableValue(end_ptr);

let n_elems = maybe_rel_end_ptr
.sub(&maybe_rel_start_ptr)?
.get_int_ref()?
.to_usize()
.ok_or(VirtualMachineError::BigintToUsizeFail)?;
let n_elems = end_ptr.sub(&start_ptr)?;

let mut keccak_input = Vec::new();
let range = vm
.get_range(&maybe_rel_start_ptr, n_elems)
.map_err(VirtualMachineError::MemoryError)?;

check_no_nones_in_range(&range)?;

for maybe_reloc_word in range.into_iter() {
let word = maybe_reloc_word.ok_or(VirtualMachineError::ExpectedIntAtRange(None))?;
let word = word.get_int_ref()?;
let range = vm.get_integer_range(start_ptr, n_elems)?;

for word in range.into_iter() {
let mut bytes = word.to_bytes_be();
let mut bytes = {
let n_word_bytes = &bytes.len();
Expand Down Expand Up @@ -208,13 +187,3 @@ pub(crate) fn left_pad_u64(bytes_vector: &mut [u64], n_zeros: usize) -> Vec<u64>

res
}

fn check_no_nones_in_range<T>(range: &Vec<Option<T>>) -> Result<(), VirtualMachineError> {
for memory_cell in range {
memory_cell
.as_ref()
.ok_or(VirtualMachineError::NoneInMemoryRange)?;
}

Ok(())
}
1 change: 0 additions & 1 deletion src/hint_processor/builtin_hint_processor/set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,6 @@ mod tests {
.memory
.get(&MaybeRelocatable::from((1, 0)))
.unwrap()
.unwrap()
.as_ref(),
&MaybeRelocatable::Int(Felt::zero())
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use crate::{
hint_processor_definition::HintReference,
},
serde::deserialize_program::ApTracking,
types::{exec_scope::ExecutionScopes, relocatable::MaybeRelocatable},
types::exec_scope::ExecutionScopes,
vm::{
errors::{hint_errors::HintError, vm_errors::VirtualMachineError},
vm_core::VirtualMachine,
Expand Down Expand Up @@ -268,7 +268,7 @@ pub fn squash_dict(
let key_addr = address + DICT_ACCESS_SIZE * i;
let key = vm
.get_integer(key_addr)
.map_err(|_| VirtualMachineError::ExpectedInteger(MaybeRelocatable::from(key_addr)))?;
.map_err(|_| VirtualMachineError::ExpectedInteger(key_addr))?;
access_indices
.entry(key.into_owned())
.or_default()
Expand Down Expand Up @@ -297,6 +297,7 @@ pub fn squash_dict(
#[cfg(test)]
mod tests {
use super::*;
use crate::types::relocatable::MaybeRelocatable;
use crate::vm::vm_memory::memory_segments::MemorySegmentManager;
use crate::{
any_box,
Expand Down
10 changes: 4 additions & 6 deletions src/hint_processor/hint_processor_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,8 @@ pub fn get_maybe_relocatable_from_reference(
}
//Then calculate address
let var_addr = compute_addr_from_reference(hint_reference, vm, ap_tracking)?;

if hint_reference.dereference {
vm.get_maybe(&var_addr).ok()?
vm.get_maybe(&var_addr)
} else {
Some(MaybeRelocatable::from(var_addr))
}
Expand All @@ -100,8 +99,7 @@ pub fn compute_addr_from_reference(
hint_ap_tracking,
&hint_reference.offset1,
)?
.get_relocatable()
.ok()?
.get_relocatable()?
} else {
return None;
};
Expand All @@ -117,7 +115,7 @@ pub fn compute_addr_from_reference(
&hint_reference.offset2,
)?;

Some(offset1 + value.get_int_ref().ok()?.to_usize()?)
Some(offset1 + value.get_int_ref()?.to_usize()?)
}
OffsetValue::Value(value) => Some(offset1 + *value),
_ => None,
Expand Down Expand Up @@ -172,7 +170,7 @@ fn get_offset_value_reference(
}

if *deref {
vm.get_maybe(&(base_addr + *offset)).ok()?
vm.get_maybe(&(base_addr + *offset))
} else {
Some((base_addr + *offset).into())
}
Expand Down
25 changes: 9 additions & 16 deletions src/types/relocatable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ impl Relocatable {
pub fn add_maybe(&self, other: &MaybeRelocatable) -> Result<Relocatable, VirtualMachineError> {
let num_ref = other
.get_int_ref()
.map_err(|_| VirtualMachineError::RelocatableAdd)?;
.ok_or(VirtualMachineError::RelocatableAdd)?;

let big_offset: Felt = num_ref + self.offset;
let new_offset = big_offset
Expand Down Expand Up @@ -312,20 +312,18 @@ impl MaybeRelocatable {
}

//Returns reference to Felt inside self if Int variant or Error if RelocatableValue variant
pub fn get_int_ref(&self) -> Result<&Felt, VirtualMachineError> {
pub fn get_int_ref(&self) -> Option<&Felt> {
match self {
MaybeRelocatable::Int(num) => Ok(num),
MaybeRelocatable::RelocatableValue(_) => {
Err(VirtualMachineError::ExpectedInteger(self.clone()))
}
MaybeRelocatable::Int(num) => Some(num),
MaybeRelocatable::RelocatableValue(_) => None,
}
}

//Returns reference to Relocatable inside self if Relocatable variant or Error if Int variant
pub fn get_relocatable(&self) -> Result<Relocatable, VirtualMachineError> {
pub fn get_relocatable(&self) -> Option<Relocatable> {
match self {
MaybeRelocatable::RelocatableValue(rel) => Ok(*rel),
MaybeRelocatable::Int(_) => Err(VirtualMachineError::ExpectedRelocatable(self.clone())),
MaybeRelocatable::RelocatableValue(rel) => Some(*rel),
MaybeRelocatable::Int(_) => None,
}
}
}
Expand Down Expand Up @@ -798,14 +796,9 @@ mod tests {
fn get_relocatable_test() {
assert_matches!(
mayberelocatable!(1, 2).get_relocatable(),
Ok::<Relocatable, VirtualMachineError>(x) if x == relocatable!(1, 2)
Some(x) if x == relocatable!(1, 2)
);
assert_matches!(
mayberelocatable!(3).get_relocatable(),
Err::<Relocatable, VirtualMachineError>(VirtualMachineError::ExpectedRelocatable(
x
)) if x == mayberelocatable!(3)
)
assert_matches!(mayberelocatable!(3).get_relocatable(), None)
}

#[test]
Expand Down
10 changes: 2 additions & 8 deletions src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,19 +163,13 @@ pub mod test_utils {
macro_rules! check_memory_address {
($mem:expr, ($si:expr, $off:expr), ($sival:expr, $offval: expr)) => {
assert_eq!(
$mem.get(&mayberelocatable!($si, $off))
.unwrap()
.unwrap()
.as_ref(),
$mem.get(&mayberelocatable!($si, $off)).unwrap().as_ref(),
&mayberelocatable!($sival, $offval)
)
};
($mem:expr, ($si:expr, $off:expr), $val:expr) => {
assert_eq!(
$mem.get(&mayberelocatable!($si, $off))
.unwrap()
.unwrap()
.as_ref(),
$mem.get(&mayberelocatable!($si, $off)).unwrap().as_ref(),
&mayberelocatable!($val)
)
};
Expand Down
10 changes: 6 additions & 4 deletions src/vm/errors/memory_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ pub enum MemoryError {
UnallocatedSegment(usize, usize),
#[error("Memory addresses must be relocatable")]
AddressNotRelocatable,
#[error("Range-check validation failed, number is out of valid range")]
NumOutOfBounds,
#[error("Range-check validation failed, encountered non-int value")]
FoundNonInt,
#[error("Range-check validation failed, number {0} is out of valid range [0, {1}]")]
RangeCheckNumOutOfBounds(Felt, Felt),
#[error("Range-check validation failed, encountered non-int value at address {0}")]
RangeCheckFoundNonInt(Relocatable),
#[error("Inconsistent memory assignment at address {0:?}. {1:?} != {2:?}")]
InconsistentMemory(MaybeRelocatable, MaybeRelocatable, MaybeRelocatable),
#[error("compute_effective_sizes should be called before relocate_segments")]
Expand Down Expand Up @@ -77,6 +77,8 @@ pub enum MemoryError {
FailedToGetReturnValues(usize, Relocatable),
#[error(transparent)]
InsufficientAllocatedCells(#[from] InsufficientAllocatedCellsError),
#[error("Accessed address {0} has higher offset than the maximal offset {1} encountered in the memory segment.")]
AccessedAddressOffsetBiggerThanSegmentSize(Relocatable, usize),
}

#[derive(Debug, PartialEq, Eq, Error)]
Expand Down
6 changes: 4 additions & 2 deletions src/vm/errors/vm_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,9 @@ pub enum VirtualMachineError {
#[error("Failed to retrieve value from address {0}")]
MemoryGet(MaybeRelocatable),
#[error("Expected integer at address {0}")]
ExpectedInteger(MaybeRelocatable),
ExpectedInteger(Relocatable),
#[error("Expected relocatable at address {0}")]
ExpectedRelocatable(MaybeRelocatable),
ExpectedRelocatable(Relocatable),
#[error("Value: {0} should be positive")]
ValueNotPositive(Felt),
#[error("Div out of range: 0 < {0} <= {1}")]
Expand Down Expand Up @@ -150,6 +150,8 @@ pub enum VirtualMachineError {
InvalidMemoryValueTemporaryAddress(Relocatable),
#[error("accessed_addresses is None.")]
MissingAccessedAddresses,
#[error("Unknown memory cell at address {0}")]
UnknownMemoryCell(Relocatable),
#[error(transparent)]
Other(Box<dyn Error>),
}
9 changes: 3 additions & 6 deletions src/vm/runners/builtin_runner/bitwise.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,9 @@ impl BitwiseBuiltinRunner {

let num_x = memory.get(&x_addr);
let num_y = memory.get(&y_addr);
if let (
Ok(Some(MaybeRelocatable::Int(ref num_x))),
Ok(Some(MaybeRelocatable::Int(ref num_y))),
) = (
num_x.as_ref().map(|x| x.as_ref().map(|x| x.as_ref())),
num_y.as_ref().map(|x| x.as_ref().map(|x| x.as_ref())),
if let (Some(MaybeRelocatable::Int(ref num_x)), Some(MaybeRelocatable::Int(ref num_y))) = (
num_x.as_ref().map(|x| x.as_ref()),
num_y.as_ref().map(|x| x.as_ref()),
) {
if num_x.bits() > self.bitwise_builtin.total_n_bits as u64 {
return Err(RunnerError::IntegerBiggerThanPowerOfTwo(
Expand Down
5 changes: 1 addition & 4 deletions src/vm/runners/builtin_runner/ec_op.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,7 @@ impl EcOpBuiltinRunner {
//If an input cell is not filled, return None
let mut input_cells = Vec::<Cow<Felt>>::with_capacity(self.n_input_cells as usize);
for i in 0..self.n_input_cells as usize {
match memory
.get(&instance.add_usize(i))
.map_err(RunnerError::FailedMemoryGet)?
{
match memory.get(&instance.add_usize(i)) {
None => return Ok(None),
Some(addr) => {
input_cells.push(match addr {
Expand Down
6 changes: 3 additions & 3 deletions src/vm/runners/builtin_runner/hash.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,9 @@ impl HashBuiltinRunner {
segment_index: address.segment_index,
offset: address.offset - 2,
}));
if let (Ok(Some(MaybeRelocatable::Int(num_a))), Ok(Some(MaybeRelocatable::Int(num_b)))) = (
num_a.as_ref().map(|x| x.as_ref().map(|x| x.as_ref())),
num_b.as_ref().map(|x| x.as_ref().map(|x| x.as_ref())),
if let (Some(MaybeRelocatable::Int(num_a)), Some(MaybeRelocatable::Int(num_b))) = (
num_a.as_ref().map(|x| x.as_ref()),
num_b.as_ref().map(|x| x.as_ref()),
) {
self.verified_addresses.borrow_mut().push(address);

Expand Down
Loading