From dc09d8b36c922f0cc98efcda73593831e5d9a707 Mon Sep 17 00:00:00 2001 From: Federica Date: Mon, 13 Feb 2023 17:15:25 -0300 Subject: [PATCH 1/8] Remove error --- src/vm/vm_core.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/vm/vm_core.rs b/src/vm/vm_core.rs index 224eae31a7..7d21007ad1 100644 --- a/src/vm/vm_core.rs +++ b/src/vm/vm_core.rs @@ -146,12 +146,7 @@ impl VirtualMachine { }; let imm_addr = &self.run_context.pc + 1_i32; - - if let Ok(optional_imm) = self.memory.get(&imm_addr) { - Ok((encoding_ref, optional_imm)) - } else { - Err(VirtualMachineError::InvalidInstructionEncoding) - } + Ok((encoding_ref, self.memory.get(&imm_addr).ok().flatten())) } fn update_fp( From 13f3c201cff7830897de41ccf8eedc35f7fe6166 Mon Sep 17 00:00:00 2001 From: Federica Date: Tue, 14 Feb 2023 10:13:55 -0300 Subject: [PATCH 2/8] Check error types used --- src/vm/errors/memory_errors.rs | 2 ++ src/vm/errors/vm_errors.rs | 2 ++ src/vm/vm_core.rs | 11 +++++------ 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/vm/errors/memory_errors.rs b/src/vm/errors/memory_errors.rs index 27a899a661..f0303be1e5 100644 --- a/src/vm/errors/memory_errors.rs +++ b/src/vm/errors/memory_errors.rs @@ -75,4 +75,6 @@ pub enum MemoryError { MsgNonInt(Relocatable), #[error("Failed to convert String: {0} to FieldElement")] FailedStringToFieldElementConversion(String), + #[error("Failed to fetch {0} return values, ap is only {1}")] + FailedToGetReturnValues(usize, Relocatable), } diff --git a/src/vm/errors/vm_errors.rs b/src/vm/errors/vm_errors.rs index a0a76a6efd..9694b3e43d 100644 --- a/src/vm/errors/vm_errors.rs +++ b/src/vm/errors/vm_errors.rs @@ -144,6 +144,8 @@ pub enum VirtualMachineError { NegBuiltinBase, #[error("Security Error: Invalid Memory Value: temporary address not relocated: {0}")] InvalidMemoryValueTemporaryAddress(Relocatable), + #[error("accessed_addresses is None.")] + MissingAccessedAddresses, #[error(transparent)] Other(Box), } diff --git a/src/vm/vm_core.rs b/src/vm/vm_core.rs index 7d21007ad1..28edf0f425 100644 --- a/src/vm/vm_core.rs +++ b/src/vm/vm_core.rs @@ -332,10 +332,9 @@ impl VirtualMachine { ) -> Result, VirtualMachineError> { for (_, builtin) in self.builtin_runners.iter() { if builtin.base() == address.segment_index { - match builtin.deduce_memory_cell(address, &self.memory) { - Ok(maybe_reloc) => return Ok(maybe_reloc), - Err(error) => return Err(VirtualMachineError::RunnerError(error)), - }; + return builtin + .deduce_memory_cell(address, &self.memory) + .map_err(VirtualMachineError::RunnerError); } } Ok(None) @@ -749,7 +748,7 @@ impl VirtualMachine { } self.accessed_addresses .as_mut() - .ok_or(VirtualMachineError::RunNotFinished)? + .ok_or(VirtualMachineError::MissingAccessedAddresses)? .extend((0..len).map(|i: usize| base + i)); Ok(()) } @@ -900,7 +899,7 @@ impl VirtualMachine { .run_context .get_ap() .sub_usize(n_ret) - .map_err(|_| MemoryError::NumOutOfBounds)?; + .map_err(|_| MemoryError::FailedToGetReturnValues(n_ret, self.get_ap()))?; self.memory.get_continuous_range(&addr.into(), n_ret) } From d88f16dcae30e9af02d09f6b75a6bae98f0d4154 Mon Sep 17 00:00:00 2001 From: Federica Date: Tue, 14 Feb 2023 10:54:12 -0300 Subject: [PATCH 3/8] Remove PureValueError --- src/vm/errors/vm_errors.rs | 8 +++-- src/vm/vm_core.rs | 66 ++++++++++++++------------------------ 2 files changed, 30 insertions(+), 44 deletions(-) diff --git a/src/vm/errors/vm_errors.rs b/src/vm/errors/vm_errors.rs index 9694b3e43d..9b6e956622 100644 --- a/src/vm/errors/vm_errors.rs +++ b/src/vm/errors/vm_errors.rs @@ -32,6 +32,12 @@ pub enum VirtualMachineError { UnconstrainedResJumpRel, #[error("Res.UNCONSTRAINED cannot be used with Opcode.ASSERT_EQ")] UnconstrainedResAssertEq, + #[error("An integer value as Res cannot be used with PcUpdate.JUMP_REL")] + JumpRelNotInt, + #[error( + "Failed to compute Res.MUL: Could not complete computation of non pure values {0} * {1}" + )] + ComputeResRelocatableMul(MaybeRelocatable, MaybeRelocatable), #[error("Couldn't compute operand {0} at address {1}")] FailedToComputeOperands(String, Relocatable), #[error("An ASSERT_EQ instruction failed: {0} != {1}.")] @@ -42,8 +48,6 @@ pub enum VirtualMachineError { CantWriteReturnFp(MaybeRelocatable, MaybeRelocatable), #[error("Couldn't get or load dst")] NoDst, - #[error("Pure Value Error")] - PureValue, #[error("Invalid res value: {0}")] InvalidRes(i64), #[error("Invalid opcode value: {0}")] diff --git a/src/vm/vm_core.rs b/src/vm/vm_core.rs index 28edf0f425..032edf3d8d 100644 --- a/src/vm/vm_core.rs +++ b/src/vm/vm_core.rs @@ -173,16 +173,16 @@ impl VirtualMachine { instruction: &Instruction, operands: &Operands, ) -> Result<(), VirtualMachineError> { - let new_ap: Relocatable = match instruction.ap_update { - ApUpdate::Add => match operands.res.clone() { - Some(res) => self.run_context.get_ap().add_maybe(&res)?, + let new_ap_offset: usize = match instruction.ap_update { + ApUpdate::Add => match &operands.res { + Some(res) => self.run_context.get_ap().add_maybe(res)?.offset, None => return Err(VirtualMachineError::UnconstrainedResAdd), }, - ApUpdate::Add1 => self.run_context.get_ap() + 1_i32, - ApUpdate::Add2 => self.run_context.get_ap() + 2_i32, + ApUpdate::Add1 => self.run_context.ap + 1, + ApUpdate::Add2 => self.run_context.ap + 2, ApUpdate::Regular => return Ok(()), }; - self.run_context.ap = new_ap.offset; + self.run_context.ap = new_ap_offset; Ok(()) } @@ -201,11 +201,11 @@ impl VirtualMachine { Some(res) => match res { MaybeRelocatable::Int(num_res) => self.run_context.pc.add_int(&num_res)?, - _ => return Err(VirtualMachineError::PureValue), + _ => return Err(VirtualMachineError::JumpRelNotInt), }, None => return Err(VirtualMachineError::UnconstrainedResJumpRel), }, - PcUpdate::Jnz => match VirtualMachine::is_zero(&operands.dst)? { + PcUpdate::Jnz => match VirtualMachine::is_zero(&operands.dst) { true => self.run_context.pc + instruction.size(), false => (self.run_context.pc.add_maybe(&operands.op1))?, }, @@ -227,11 +227,10 @@ impl VirtualMachine { /// Returns true if the value is zero /// Used for JNZ instructions - fn is_zero(addr: &MaybeRelocatable) -> Result { + fn is_zero(addr: &MaybeRelocatable) -> bool { match addr { - MaybeRelocatable::Int(num) => Ok(num.is_zero()), - MaybeRelocatable::RelocatableValue(rel_value) if rel_value.offset > 0 => Ok(false), - _ => Err(VirtualMachineError::PureValue), + MaybeRelocatable::Int(num) => num.is_zero(), + _ => false, } } @@ -355,7 +354,10 @@ impl VirtualMachine { { return Ok(Some(MaybeRelocatable::Int(num_op0 * num_op1))); } - Err(VirtualMachineError::PureValue) + Err(VirtualMachineError::ComputeResRelocatableMul( + op0.clone(), + op1.clone(), + )) } Res::Unconstrained => Ok(None), } @@ -367,15 +369,10 @@ impl VirtualMachine { res: Option<&MaybeRelocatable>, ) -> Option { match instruction.opcode { - Opcode::AssertEq => { - if let Some(res_addr) = res { - return Some(res_addr.clone()); - } - } - Opcode::Call => return Some(MaybeRelocatable::from(self.run_context.get_fp())), - _ => (), - }; - None + Opcode::AssertEq => res.map(Clone::clone), + Opcode::Call => Some(self.get_fp().into()), + _ => None, + } } fn opcode_assertions( @@ -1616,7 +1613,7 @@ mod tests { let mut vm = vm!(); assert_matches!( vm.update_pc(&instruction, &operands), - Err::<(), VirtualMachineError>(VirtualMachineError::PureValue) + Err::<(), VirtualMachineError>(VirtualMachineError::JumpRelNotInt) ); } @@ -1763,28 +1760,13 @@ mod tests { #[test] fn is_zero_int_value() { let value = MaybeRelocatable::Int(Felt::new(1)); - assert_matches!( - VirtualMachine::is_zero(&value), - Ok::(false) - ); + assert!(!VirtualMachine::is_zero(&value)); } #[test] fn is_zero_relocatable_value() { let value = MaybeRelocatable::from((1, 2)); - assert_matches!( - VirtualMachine::is_zero(&value), - Ok::(false) - ); - } - - #[test] - fn is_zero_relocatable_value_negative() { - let value = MaybeRelocatable::from((1, 0)); - assert_matches!( - VirtualMachine::is_zero(&value), - Err::(VirtualMachineError::PureValue) - ); + assert!(!VirtualMachine::is_zero(&value)); } #[test] @@ -2306,7 +2288,7 @@ mod tests { let op0 = MaybeRelocatable::from((2, 6)); assert_matches!( vm.compute_res(&instruction, &op0, &op1), - Err::, VirtualMachineError>(VirtualMachineError::PureValue) + Err(VirtualMachineError::ComputeResRelocatableMul(x, y)) if x == op0 && y == op1 ); } @@ -3533,7 +3515,7 @@ mod tests { fn get_return_values_fails_when_ap_is_0() { let mut vm = vm!(); vm.memory = memory![((1, 0), 1), ((1, 1), 2), ((1, 2), 3), ((1, 3), 4)]; - assert_matches!(vm.get_return_values(3), Err(MemoryError::NumOutOfBounds)); + assert_matches!(vm.get_return_values(3), Err(MemoryError::FailedToGetReturnValues(x, y)) if x == 3 && y == Relocatable::from((1,0))); } /* From a20507d85b86117f469b4deaed1e8a10da82652d Mon Sep 17 00:00:00 2001 From: Federica Date: Tue, 14 Feb 2023 11:17:33 -0300 Subject: [PATCH 4/8] Simplify logic --- src/vm/vm_core.rs | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/src/vm/vm_core.rs b/src/vm/vm_core.rs index 032edf3d8d..02d06f4f4b 100644 --- a/src/vm/vm_core.rs +++ b/src/vm/vm_core.rs @@ -200,7 +200,6 @@ impl VirtualMachine { PcUpdate::JumpRel => match operands.res.clone() { Some(res) => match res { MaybeRelocatable::Int(num_res) => self.run_context.pc.add_int(&num_res)?, - _ => return Err(VirtualMachineError::JumpRelNotInt), }, None => return Err(VirtualMachineError::UnconstrainedResJumpRel), @@ -260,19 +259,16 @@ impl VirtualMachine { } } Res::Mul => { - if let (Some(dst_addr), Some(op1_addr)) = (dst, op1) { - if let ( - MaybeRelocatable::Int(num_dst), - MaybeRelocatable::Int(ref num_op1_ref), - ) = (dst_addr, op1_addr) - { - let num_op1 = Clone::clone(num_op1_ref); - if num_op1 != Felt::zero() { - return Ok(( - Some(MaybeRelocatable::Int(num_dst / num_op1)), - Some(dst_addr.clone()), - )); - } + if let ( + Some(MaybeRelocatable::Int(num_dst)), + Some(MaybeRelocatable::Int(num_op1)), + ) = (dst, op1) + { + if !num_op1.is_zero() { + return Ok(( + Some(MaybeRelocatable::Int(num_dst / num_op1)), + dst.map(Clone::clone), + )); } } } From 9a4fc569ee5c69472d2fe98b03c4ddd142f5284b Mon Sep 17 00:00:00 2001 From: Federica Date: Tue, 14 Feb 2023 11:34:57 -0300 Subject: [PATCH 5/8] Simplify logic deduce_op1 --- src/vm/vm_core.rs | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/src/vm/vm_core.rs b/src/vm/vm_core.rs index 02d06f4f4b..991aa83b13 100644 --- a/src/vm/vm_core.rs +++ b/src/vm/vm_core.rs @@ -291,30 +291,22 @@ impl VirtualMachine { ) -> Result<(Option, Option), VirtualMachineError> { if let Opcode::AssertEq = instruction.opcode { match instruction.res { - Res::Op1 => { - if let Some(dst_addr) = dst { - return Ok((Some(dst_addr.clone()), Some(dst_addr.clone()))); - } - } + Res::Op1 => return Ok((dst.cloned(), dst.cloned())), Res::Add => { - if let (Some(dst_addr), Some(op0_addr)) = (dst, op0) { - return Ok((Some(dst_addr.sub(&op0_addr)?), Some(dst_addr.clone()))); - } + return Ok(( + dst.zip(op0).and_then(|(dst, op0)| dst.sub(&op0).ok()), + dst.cloned(), + )) } - Res::Mul => { - if let (Some(dst_addr), Some(op0_addr)) = (dst, op0) { - if let (MaybeRelocatable::Int(num_dst), MaybeRelocatable::Int(num_op0)) = - (dst_addr, op0_addr) - { - if num_op0 != Felt::zero() { - return Ok(( - Some(MaybeRelocatable::Int(num_dst / num_op0)), - Some(dst_addr.clone()), - )); - } - } + Res::Mul => match (dst, op0) { + ( + Some(MaybeRelocatable::Int(num_dst)), + Some(MaybeRelocatable::Int(num_op0)), + ) if num_op0.is_zero() => { + return Ok((Some(MaybeRelocatable::Int(num_dst / num_op0)), dst.cloned())) } - } + _ => (), + }, _ => (), }; }; From d13878eed92ba25df66e7b654def440278ed8c6e Mon Sep 17 00:00:00 2001 From: Federica Date: Tue, 14 Feb 2023 11:41:08 -0300 Subject: [PATCH 6/8] Fix --- src/vm/vm_core.rs | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/src/vm/vm_core.rs b/src/vm/vm_core.rs index 991aa83b13..6e3c3ddc74 100644 --- a/src/vm/vm_core.rs +++ b/src/vm/vm_core.rs @@ -302,7 +302,7 @@ impl VirtualMachine { ( Some(MaybeRelocatable::Int(num_dst)), Some(MaybeRelocatable::Int(num_op0)), - ) if num_op0.is_zero() => { + ) if !num_op0.is_zero() => { return Ok((Some(MaybeRelocatable::Int(num_dst / num_op0)), dst.cloned())) } _ => (), @@ -357,7 +357,7 @@ impl VirtualMachine { res: Option<&MaybeRelocatable>, ) -> Option { match instruction.opcode { - Opcode::AssertEq => res.map(Clone::clone), + Opcode::AssertEq => res.cloned(), Opcode::Call => Some(self.get_fp().into()), _ => None, } @@ -369,20 +369,14 @@ impl VirtualMachine { operands: &Operands, ) -> Result<(), VirtualMachineError> { match instruction.opcode { - Opcode::AssertEq => { - match &operands.res { - None => return Err(VirtualMachineError::UnconstrainedResAssertEq), - Some(res) => { - if res != &operands.dst { - return Err(VirtualMachineError::DiffAssertValues( - operands.dst.clone(), - res.clone(), - )); - }; - } - }; - Ok(()) - } + Opcode::AssertEq => match &operands.res { + None => Err(VirtualMachineError::UnconstrainedResAssertEq), + Some(res) if res != &operands.dst => Err(VirtualMachineError::DiffAssertValues( + operands.dst.clone(), + res.clone(), + )), + _ => Ok(()), + }, Opcode::Call => { let return_pc = MaybeRelocatable::from(self.run_context.pc + instruction.size()); if operands.op0 != return_pc { From fd91ceec6745aa2080ecc6012d2d1d3a56b446a9 Mon Sep 17 00:00:00 2001 From: Federica Date: Tue, 14 Feb 2023 11:51:34 -0300 Subject: [PATCH 7/8] Simplify step --- src/types/relocatable.rs | 15 ++++++++++++++- src/vm/vm_core.rs | 6 ++---- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/types/relocatable.rs b/src/types/relocatable.rs index 731481afca..f9ba753146 100644 --- a/src/types/relocatable.rs +++ b/src/types/relocatable.rs @@ -7,7 +7,7 @@ use num_traits::{FromPrimitive, ToPrimitive, Zero}; use serde::{Deserialize, Serialize}; use std::{ fmt::{self, Display}, - ops::Add, + ops::{Add, AddAssign}, }; #[derive(Eq, Hash, PartialEq, PartialOrd, Clone, Copy, Debug, Serialize, Deserialize)] @@ -95,6 +95,12 @@ impl Add for Relocatable { } } +impl AddAssign for Relocatable { + fn add_assign(&mut self, rhs: usize) { + self.offset += rhs + } +} + impl Add for Relocatable { type Output = Relocatable; fn add(self, other: i32) -> Self { @@ -825,4 +831,11 @@ mod tests { String::from("6") ) } + + #[test] + fn relocatable_add_assign_usize() { + let mut addr = Relocatable::from((1, 0)); + addr += 1; + assert_eq!(addr, Relocatable::from((1, 1))) + } } diff --git a/src/vm/vm_core.rs b/src/vm/vm_core.rs index 6e3c3ddc74..85e3ebed96 100644 --- a/src/vm/vm_core.rs +++ b/src/vm/vm_core.rs @@ -22,7 +22,7 @@ use crate::{ }; use felt::Felt; use num_traits::{ToPrimitive, Zero}; -use std::{any::Any, borrow::Cow, collections::HashMap, ops::Add}; +use std::{any::Any, borrow::Cow, collections::HashMap}; const MAX_TRACEBACK_ENTRIES: u32 = 20; @@ -485,9 +485,7 @@ impl VirtualMachine { if !self.skip_instruction_execution { self.run_instruction(instruction)?; } else { - let pc = &self.get_pc().clone(); - let size = instruction.size(); - self.set_pc(pc.add(size)); + self.run_context.pc += instruction.size(); self.skip_instruction_execution = false; } Ok(()) From 2a8cb729080ac524da0a54f56e7e631ac09bcfbc Mon Sep 17 00:00:00 2001 From: Federica Date: Tue, 14 Feb 2023 12:02:56 -0300 Subject: [PATCH 8/8] Simplify deduce_op1 --- src/vm/vm_core.rs | 56 ++++++++++++++++++----------------------------- 1 file changed, 21 insertions(+), 35 deletions(-) diff --git a/src/vm/vm_core.rs b/src/vm/vm_core.rs index 85e3ebed96..8ba2071c21 100644 --- a/src/vm/vm_core.rs +++ b/src/vm/vm_core.rs @@ -243,41 +243,27 @@ impl VirtualMachine { op1: Option<&MaybeRelocatable>, ) -> Result<(Option, Option), VirtualMachineError> { match instruction.opcode { - Opcode::Call => { - return Ok(( - Some(MaybeRelocatable::from( - self.run_context.pc + instruction.size(), - )), - None, - )) - } - Opcode::AssertEq => { - match instruction.res { - Res::Add => { - if let (Some(dst_addr), Some(op1_addr)) = (dst, op1) { - return Ok((Some(dst_addr.sub(op1_addr)?), Some(dst_addr.clone()))); - } - } - Res::Mul => { - if let ( - Some(MaybeRelocatable::Int(num_dst)), - Some(MaybeRelocatable::Int(num_op1)), - ) = (dst, op1) - { - if !num_op1.is_zero() { - return Ok(( - Some(MaybeRelocatable::Int(num_dst / num_op1)), - dst.map(Clone::clone), - )); - } - } - } - _ => (), - }; - } - _ => (), - }; - Ok((None, None)) + Opcode::Call => Ok(( + Some(MaybeRelocatable::from( + self.run_context.pc + instruction.size(), + )), + None, + )), + Opcode::AssertEq => match (&instruction.res, dst, op1) { + (Res::Add, Some(dst_addr), Some(op1_addr)) => { + Ok((Some(dst_addr.sub(op1_addr)?), dst.cloned())) + } + ( + Res::Mul, + Some(MaybeRelocatable::Int(num_dst)), + Some(MaybeRelocatable::Int(num_op1)), + ) if !num_op1.is_zero() => { + Ok((Some(MaybeRelocatable::Int(num_dst / num_op1)), dst.cloned())) + } + _ => Ok((None, None)), + }, + _ => Ok((None, None)), + } } /// Returns a tuple (deduced_op1, deduced_res).