From be64eeaba71c96870f5cf1b542225965baeda736 Mon Sep 17 00:00:00 2001 From: Federica Date: Fri, 27 Jan 2023 16:38:36 -0300 Subject: [PATCH 1/4] Fix error variants used in signature.rs --- src/vm/errors/memory_errors.rs | 2 ++ src/vm/errors/vm_errors.rs | 3 ++- src/vm/runners/builtin_runner/signature.rs | 6 +++--- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/vm/errors/memory_errors.rs b/src/vm/errors/memory_errors.rs index 97103e4664..796def2089 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/errors/vm_errors.rs b/src/vm/errors/vm_errors.rs index a25bb93fbf..61cf489dc8 100644 --- a/src/vm/errors/vm_errors.rs +++ b/src/vm/errors/vm_errors.rs @@ -8,8 +8,9 @@ use crate::{ use felt::Felt; use num_bigint::{BigInt, BigUint}; use thiserror::Error; +use std::error::Error; -#[derive(Debug, PartialEq, Error)] +#[derive(Debug, Error)] pub enum VirtualMachineError { #[error("Instruction should be an int")] InvalidInstructionEncoding, diff --git a/src/vm/runners/builtin_runner/signature.rs b/src/vm/runners/builtin_runner/signature.rs index 395bc6abe2..2693be46b8 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 { @@ -104,7 +104,7 @@ impl SignatureBuiltinRunner { -> Result, MemoryError> { let address = match address { MaybeRelocatable::RelocatableValue(address) => *address, - _ => return Err(MemoryError::MissingAccessedAddresses), + _ => return Err(MemoryError::AddressNotRelocatable), }; let address_offset = address.offset.mod_floor(&(cells_per_instance as usize)); From 91eed17c787f3405c8321e692275606738c8eb52 Mon Sep 17 00:00:00 2001 From: Federica Date: Fri, 27 Jan 2023 16:40:18 -0300 Subject: [PATCH 2/4] Fix --- src/vm/errors/memory_errors.rs | 2 +- src/vm/errors/vm_errors.rs | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/vm/errors/memory_errors.rs b/src/vm/errors/memory_errors.rs index 796def2089..80899ab84d 100644 --- a/src/vm/errors/memory_errors.rs +++ b/src/vm/errors/memory_errors.rs @@ -63,5 +63,5 @@ pub enum MemoryError { #[error("Couldn't obtain a mutable accessed offset")] CantGetMutAccessedOffset, #[error("Failed to convert String: {0} to FieldElement")] - FailedStringToFieldElementConversion(String) + FailedStringToFieldElementConversion(String), } diff --git a/src/vm/errors/vm_errors.rs b/src/vm/errors/vm_errors.rs index 61cf489dc8..a25bb93fbf 100644 --- a/src/vm/errors/vm_errors.rs +++ b/src/vm/errors/vm_errors.rs @@ -8,9 +8,8 @@ use crate::{ use felt::Felt; use num_bigint::{BigInt, BigUint}; use thiserror::Error; -use std::error::Error; -#[derive(Debug, Error)] +#[derive(Debug, PartialEq, Error)] pub enum VirtualMachineError { #[error("Instruction should be an int")] InvalidInstructionEncoding, From 8cfce758846bee8ff77e08b3f28d6e141b96d7f8 Mon Sep 17 00:00:00 2001 From: Federica Date: Fri, 27 Jan 2023 16:50:26 -0300 Subject: [PATCH 3/4] Restrict addresses to Relocatable when validating addresses --- src/vm/runners/builtin_runner/mod.rs | 5 ++-- src/vm/runners/builtin_runner/range_check.rs | 4 +-- src/vm/runners/builtin_runner/signature.rs | 13 +++------ src/vm/runners/cairo_runner.rs | 4 +-- src/vm/vm_memory/memory.rs | 30 +++++++++----------- 5 files changed, 22 insertions(+), 34 deletions(-) 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 6239c7af13..4232b5f330 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 2693be46b8..601b602086 100644 --- a/src/vm/runners/builtin_runner/signature.rs +++ b/src/vm/runners/builtin_runner/signature.rs @@ -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::AddressNotRelocatable), - }; - + 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 989355d36b..614bd86fec 100644 --- a/src/vm/runners/cairo_runner.rs +++ b/src/vm/runners/cairo_runner.rs @@ -1516,11 +1516,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] From 2ae7a50465f8759a4ce64eb872d22594da90f9bc Mon Sep 17 00:00:00 2001 From: Federica Date: Fri, 27 Jan 2023 16:57:24 -0300 Subject: [PATCH 4/4] Add Changelog entry --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0205dd866f..9137d49a6a 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`.