From f69721af501fa34c58a7cf0e775ca59b604d91fd Mon Sep 17 00:00:00 2001 From: fmoletta <99273364+fmoletta@users.noreply.github.com> Date: Fri, 27 Jan 2023 17:53:26 -0300 Subject: [PATCH] Restrict addresses to Relocatable + fix some error variants used in signature.rs (#792) * Fix error variants used in signature.rs * Fix * Restrict addresses to Relocatable when validating addresses * Add Changelog entry --- CHANGELOG.md | 6 ++++ src/vm/errors/memory_errors.rs | 2 ++ src/vm/runners/builtin_runner/mod.rs | 5 ++-- src/vm/runners/builtin_runner/range_check.rs | 4 +-- src/vm/runners/builtin_runner/signature.rs | 17 ++++------- src/vm/runners/cairo_runner.rs | 4 +-- src/vm/vm_memory/memory.rs | 30 +++++++++----------- 7 files changed, 32 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c3b9348679..ef7e61db8b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ #### Upcoming Changes +* Restrict addresses to Relocatable + fix some error variants used in signature.rs [#792](https://github.com/lambdaclass/cairo-rs/pull/792) + * Public Api Changes: + * Change `ValidationRule` inner type to `Box Result, MemoryError>>`. + * Change `validated_addresses` field of `Memory` to `HashSet`. + * Change `validate_memory_cell(&mut self, address: &MaybeRelocatable) -> Result<(), MemoryError>` to `validate_memory_cell(&mut self, addr: &Relocatable) -> Result<(), MemoryError>`. + * Add `VmException` to `CairoRunner::run_from_entrypoint`[#775](https://github.com/lambdaclass/cairo-rs/pull/775) * Public Api Changes: * Change error return type of `CairoRunner::run_from_entrypoint` to `CairoRunError`. diff --git a/src/vm/errors/memory_errors.rs b/src/vm/errors/memory_errors.rs index 97103e4664..80899ab84d 100644 --- a/src/vm/errors/memory_errors.rs +++ b/src/vm/errors/memory_errors.rs @@ -62,4 +62,6 @@ pub enum MemoryError { ErrorVerifyingSignature, #[error("Couldn't obtain a mutable accessed offset")] CantGetMutAccessedOffset, + #[error("Failed to convert String: {0} to FieldElement")] + FailedStringToFieldElementConversion(String), } diff --git a/src/vm/runners/builtin_runner/mod.rs b/src/vm/runners/builtin_runner/mod.rs index 95ebe11ab3..7b10bbcff9 100644 --- a/src/vm/runners/builtin_runner/mod.rs +++ b/src/vm/runners/builtin_runner/mod.rs @@ -447,6 +447,7 @@ impl From for BuiltinRunner { mod tests { use super::*; use crate::hint_processor::builtin_hint_processor::builtin_hint_processor_definition::BuiltinHintProcessor; + use crate::relocatable; use crate::types::instance_definitions::ecdsa_instance_def::EcdsaInstanceDef; use crate::types::instance_definitions::keccak_instance_def::KeccakInstanceDef; use crate::types::program::Program; @@ -1194,9 +1195,7 @@ mod tests { BitwiseBuiltinRunner::new(&BitwiseInstanceDef::default(), true).into(); let mut vm = vm!(); - vm.memory - .validated_addresses - .insert(mayberelocatable!(0, 2)); + vm.memory.validated_addresses.insert(relocatable!(0, 2)); vm.memory.data = vec![vec![ mayberelocatable!(0, 0).into(), diff --git a/src/vm/runners/builtin_runner/range_check.rs b/src/vm/runners/builtin_runner/range_check.rs index 0016ee71f0..255d74b745 100644 --- a/src/vm/runners/builtin_runner/range_check.rs +++ b/src/vm/runners/builtin_runner/range_check.rs @@ -86,9 +86,7 @@ impl RangeCheckBuiltinRunner { pub fn add_validation_rule(&self, memory: &mut Memory) -> Result<(), RunnerError> { let rule: ValidationRule = ValidationRule(Box::new( - |memory: &Memory, - address: &MaybeRelocatable| - -> Result, MemoryError> { + |memory: &Memory, address: &Relocatable| -> Result, MemoryError> { if let MaybeRelocatable::Int(ref num) = memory .get(address)? .ok_or(MemoryError::FoundNonInt)? diff --git a/src/vm/runners/builtin_runner/signature.rs b/src/vm/runners/builtin_runner/signature.rs index 886d907cad..6011d4c2c2 100644 --- a/src/vm/runners/builtin_runner/signature.rs +++ b/src/vm/runners/builtin_runner/signature.rs @@ -56,9 +56,9 @@ impl SignatureBuiltinRunner { let s_string = s.to_str_radix(10); let (r_felt, s_felt) = ( FieldElement::from_dec_str(&r_string) - .map_err(|_| MemoryError::AddressNotRelocatable)?, + .map_err(|_| MemoryError::FailedStringToFieldElementConversion(r_string))?, FieldElement::from_dec_str(&s_string) - .map_err(|_| MemoryError::AddressNotRelocatable)?, + .map_err(|_| MemoryError::FailedStringToFieldElementConversion(s_string))?, ); let signature = Signature { @@ -100,13 +100,8 @@ impl SignatureBuiltinRunner { let signatures = Rc::clone(&self.signatures); let rule: ValidationRule = ValidationRule(Box::new( move |memory: &Memory, - address: &MaybeRelocatable| - -> Result, MemoryError> { - let address = match address { - MaybeRelocatable::RelocatableValue(address) => *address, - _ => return Err(MemoryError::MissingAccessedAddresses), - }; - + address: &Relocatable| + -> Result, MemoryError> { let address_offset = address.offset.mod_floor(&(cells_per_instance as usize)); let mem_addr_sum = memory.get(&(address + 1_i32)); let mem_addr_less = if address.offset > 0 { @@ -122,14 +117,14 @@ impl SignatureBuiltinRunner { (0, Ok(Some(_element)), _) => { let pubkey_addr = address; let msg_addr = address + 1_i32; - (pubkey_addr, msg_addr) + (*pubkey_addr, msg_addr) } (1, _, Ok(Some(_element))) if address.offset > 0 => { let pubkey_addr = address .sub_usize(1) .map_err(|_| MemoryError::EffectiveSizesNotCalled)?; let msg_addr = address; - (pubkey_addr, msg_addr) + (pubkey_addr, *msg_addr) } _ => return Ok(Vec::new()), }; diff --git a/src/vm/runners/cairo_runner.rs b/src/vm/runners/cairo_runner.rs index 24bbd091a7..e428f01150 100644 --- a/src/vm/runners/cairo_runner.rs +++ b/src/vm/runners/cairo_runner.rs @@ -1517,11 +1517,11 @@ mod tests { assert!(vm .memory .validated_addresses - .contains(&MaybeRelocatable::from((2, 0)))); + .contains(&Relocatable::from((2, 0)))); assert!(vm .memory .validated_addresses - .contains(&MaybeRelocatable::from((2, 1)))); + .contains(&Relocatable::from((2, 1)))); assert_eq!(vm.memory.validated_addresses.len(), 2); } diff --git a/src/vm/vm_memory/memory.rs b/src/vm/vm_memory/memory.rs index af0fe1d114..ea721ce928 100644 --- a/src/vm/vm_memory/memory.rs +++ b/src/vm/vm_memory/memory.rs @@ -12,7 +12,7 @@ use std::{ pub struct ValidationRule( #[allow(clippy::type_complexity)] - pub Box Result, MemoryError>>, + pub Box Result, MemoryError>>, ); pub struct Memory { @@ -21,7 +21,7 @@ pub struct Memory { // relocation_rules's keys map to temp_data's indices and therefore begin at // zero; that is, segment_index = -1 maps to key 0, -2 to key 1... pub(crate) relocation_rules: HashMap, - pub validated_addresses: HashSet, + pub validated_addresses: HashSet, validation_rules: HashMap, } @@ -31,7 +31,7 @@ impl Memory { data: Vec::>>::new(), temp_data: Vec::>>::new(), relocation_rules: HashMap::new(), - validated_addresses: HashSet::::new(), + validated_addresses: HashSet::::new(), validation_rules: HashMap::new(), } } @@ -81,7 +81,7 @@ impl Memory { } } }; - self.validate_memory_cell(&MaybeRelocatable::from(key)) + self.validate_memory_cell(&relocatable) } /// Retrieve a value from memory (either normal or temporary) and apply relocation rules @@ -249,27 +249,23 @@ impl Memory { self.validation_rules.insert(segment_index, rule); } - fn validate_memory_cell(&mut self, address: &MaybeRelocatable) -> Result<(), MemoryError> { - if let &MaybeRelocatable::RelocatableValue(ref rel_addr) = address { - if !self.validated_addresses.contains(address) { - for (index, validation_rule) in self.validation_rules.iter() { - if rel_addr.segment_index == *index as isize { - self.validated_addresses - .extend(validation_rule.0(self, address)?); - } + fn validate_memory_cell(&mut self, addr: &Relocatable) -> Result<(), MemoryError> { + if !self.validated_addresses.contains(addr) { + for (index, validation_rule) in self.validation_rules.iter() { + if addr.segment_index == *index as isize { + self.validated_addresses + .extend(validation_rule.0(self, addr)?); } } - Ok(()) - } else { - Err(MemoryError::AddressNotRelocatable) } + Ok(()) } ///Applies validation_rules to the current memory //Should be called during initialization, as None values will raise a FoundNonInt error pub fn validate_existing_memory(&mut self) -> Result<(), MemoryError> { for i in 0..self.data.len() { for j in 0..self.data[i].len() { - self.validate_memory_cell(&MaybeRelocatable::from((i as isize, j)))?; + self.validate_memory_cell(&Relocatable::from((i as isize, j)))?; } } Ok(()) @@ -593,7 +589,7 @@ mod memory_tests { memory.validate_existing_memory().unwrap(); assert!(memory .validated_addresses - .contains(&MaybeRelocatable::from((0, 0)))); + .contains(&Relocatable::from((0, 0)))); } #[test]