Fix BuiltinRunner::final_stack and remove quick fix (#778)

* Save progress * Restrict builtin methods argumnets to avoid mutability issues * Clippy * Remove old comment * Update changelog entry * Add missing .rev() * Fix changelog * Update CHANGELOG.md Co-authored-by: Mario Rugiero <[email protected]> * add parameter to keccak constructor for clippy * Add method `CairoRunner::get_builtins_final_stack` (#780) * Add method get_builtins_final_stack * Add tests * Fix extra line Co-authored-by: Mario Rugiero <[email protected]> Co-authored-by: Juan Rigada <[email protected]> Co-authored-by: jrigada <[email protected]>
lambdaclass · Jan 27, 2023 · 195f9ce · 195f9ce
1 parent 639ab22
commit 195f9ce
Show file tree

Hide file tree

Showing 12 changed files with 448 additions and 367 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,15 @@
 
 #### Upcoming Changes
 
+* Fix `BuiltinRunner::final_stack` and remove quick fix [#778](https://github.com/lambdaclass/cairo-rs/pull/778)
+    * Public Api changes:
+        * Various changes to public `BuiltinRunner` method's signatures:
+            * `final_stack(&self, vm: &VirtualMachine, pointer: Relocatable) -> Result<(Relocatable, usize), RunnerError>` to `final_stack(&mut self, segments: &MemorySegmentManager, memory: &Memory, pointer: Relocatable) -> Result<Relocatable,RunnerError>`.
+            * `get_used_cells(&self, vm: &VirtualMachine) -> Result<usize, MemoryError>` to  `get_used_cells(&self, segments: &MemorySegmentManager) -> Result<usize, MemoryError>`.
+            * `get_used_instances(&self, vm: &VirtualMachine) -> Result<usize, MemoryError>` to `get_used_instances(&self, segments: &MemorySegmentManager) -> Result<usize, MemoryError>`.
+    * Bugfixes:
+        * `BuiltinRunner::final_stack` now updates the builtin's stop_ptr instead of returning it. This replaces the bugfix on PR #768.
+
 #### [0.1.3] - 2023-01-26
 * Add secure_run flag + integrate verify_secure_runner into cairo-run [#771](https://github.com/lambdaclass/cairo-rs/pull/777)
     * Public Api changes:

diff --git a/src/types/instance_definitions/builtins_instance_def.rs b/src/types/instance_definitions/builtins_instance_def.rs
@@ -84,7 +84,7 @@ impl BuiltinsInstanceDef {
             _ecdsa: None,
             bitwise: Some(BitwiseInstanceDef::new(16)),
             ec_op: None,
-            keccak: Some(KeccakInstanceDef::new(2048)),
+            keccak: Some(KeccakInstanceDef::new(2048, vec![200; 8])),
         }
     }
 

diff --git a/src/types/instance_definitions/keccak_instance_def.rs b/src/types/instance_definitions/keccak_instance_def.rs
@@ -17,9 +17,10 @@ impl Default for KeccakInstanceDef {
 }
 
 impl KeccakInstanceDef {
-    pub(crate) fn new(_ratio: u32) -> Self {
+    pub(crate) fn new(_ratio: u32, _state_rep: Vec<u32>) -> Self {
         Self {
             _ratio,
+            _state_rep,
             ..Default::default()
         }
     }
@@ -56,7 +57,7 @@ mod tests {
             _state_rep: vec![200; 8],
             _instance_per_component: 16,
         };
-        assert_eq!(KeccakInstanceDef::new(2048), builtin_instance);
+        assert_eq!(KeccakInstanceDef::new(2048, vec![200; 8]), builtin_instance);
     }
 
     #[test]

diff --git a/src/vm/runners/builtin_runner/bitwise.rs b/src/vm/runners/builtin_runner/bitwise.rs
@@ -22,20 +22,20 @@ pub struct BitwiseBuiltinRunner {
     pub(crate) n_input_cells: u32,
     bitwise_builtin: BitwiseInstanceDef,
     pub(crate) stop_ptr: Option<usize>,
-    pub(crate) _included: bool,
+    pub(crate) included: bool,
     instances_per_component: u32,
 }
 
 impl BitwiseBuiltinRunner {
-    pub(crate) fn new(instance_def: &BitwiseInstanceDef, include: bool) -> Self {
+    pub(crate) fn new(instance_def: &BitwiseInstanceDef, included: bool) -> Self {
         BitwiseBuiltinRunner {
             base: 0,
             ratio: instance_def.ratio,
             cells_per_instance: CELLS_PER_BITWISE,
             n_input_cells: INPUT_CELLS_PER_BITWISE,
             bitwise_builtin: instance_def.clone(),
             stop_ptr: None,
-            _included: include,
+            included,
             instances_per_component: 1,
         }
     }
@@ -49,7 +49,7 @@ impl BitwiseBuiltinRunner {
     }
 
     pub fn initial_stack(&self) -> Vec<MaybeRelocatable> {
-        if self._included {
+        if self.included {
             vec![MaybeRelocatable::from((self.base, 0))]
         } else {
             vec![]
@@ -124,9 +124,9 @@ impl BitwiseBuiltinRunner {
         (self.base, self.stop_ptr)
     }
 
-    pub fn get_used_cells(&self, vm: &VirtualMachine) -> Result<usize, MemoryError> {
+    pub fn get_used_cells(&self, segments: &MemorySegmentManager) -> Result<usize, MemoryError> {
         let base = self.base();
-        vm.segments
+        segments
             .get_segment_used_size(
                 base.try_into()
                     .map_err(|_| MemoryError::AddressInTemporarySegment(base))?,
@@ -144,7 +144,7 @@ impl BitwiseBuiltinRunner {
         if vm.current_step < min_step {
             Err(MemoryError::InsufficientAllocatedCells)
         } else {
-            let used = self.get_used_cells(vm)?;
+            let used = self.get_used_cells(&vm.segments)?;
             let size = cells_per_instance as usize
                 * safe_div_usize(vm.current_step, ratio)
                     .map_err(|_| MemoryError::InsufficientAllocatedCells)?;
@@ -174,40 +174,43 @@ impl BitwiseBuiltinRunner {
     }
 
     pub fn final_stack(
-        &self,
-        vm: &VirtualMachine,
+        &mut self,
+        segments: &MemorySegmentManager,
+        memory: &Memory,
         pointer: Relocatable,
-    ) -> Result<(Relocatable, usize), RunnerError> {
-        if self._included {
-            if let Ok(stop_pointer) =
-                vm.get_relocatable(&(pointer.sub_usize(1)).map_err(|_| RunnerError::FinalStack)?)
+    ) -> Result<Relocatable, RunnerError> {
+        if self.included {
+            if let Ok(stop_pointer) = memory
+                .get_relocatable(&(pointer.sub_usize(1)).map_err(|_| RunnerError::FinalStack)?)
             {
                 if self.base() != stop_pointer.segment_index {
                     return Err(RunnerError::InvalidStopPointer("bitwise".to_string()));
                 }
                 let stop_ptr = stop_pointer.offset;
                 let num_instances = self
-                    .get_used_instances(vm)
+                    .get_used_instances(segments)
                     .map_err(|_| RunnerError::FinalStack)?;
                 let used_cells = num_instances * self.cells_per_instance as usize;
                 if stop_ptr != used_cells {
                     return Err(RunnerError::InvalidStopPointer("bitwise".to_string()));
                 }
-                Ok((
-                    pointer.sub_usize(1).map_err(|_| RunnerError::FinalStack)?,
-                    stop_ptr,
-                ))
+                self.stop_ptr = Some(stop_ptr);
+                Ok(pointer.sub_usize(1).map_err(|_| RunnerError::FinalStack)?)
             } else {
                 Err(RunnerError::FinalStack)
             }
         } else {
             let stop_ptr = self.base() as usize;
-            Ok((pointer, stop_ptr))
+            self.stop_ptr = Some(stop_ptr);
+            Ok(pointer)
         }
     }
 
-    pub fn get_used_instances(&self, vm: &VirtualMachine) -> Result<usize, MemoryError> {
-        let used_cells = self.get_used_cells(vm)?;
+    pub fn get_used_instances(
+        &self,
+        segments: &MemorySegmentManager,
+    ) -> Result<usize, MemoryError> {
+        let used_cells = self.get_used_cells(segments)?;
         Ok(div_ceil(used_cells, self.cells_per_instance as usize))
     }
 }
@@ -238,12 +241,12 @@ mod tests {
 
         vm.segments.segment_used_sizes = Some(vec![1]);
 
-        assert_eq!(builtin.get_used_instances(&vm), Ok(1));
+        assert_eq!(builtin.get_used_instances(&vm.segments), Ok(1));
     }
 
     #[test]
     fn final_stack() {
-        let builtin = BitwiseBuiltinRunner::new(&BitwiseInstanceDef::new(10), true);
+        let mut builtin = BitwiseBuiltinRunner::new(&BitwiseInstanceDef::new(10), true);
 
         let mut vm = vm!();
 
@@ -259,14 +262,16 @@ mod tests {
         let pointer = Relocatable::from((2, 2));
 
         assert_eq!(
-            builtin.final_stack(&vm, pointer).unwrap(),
-            (Relocatable::from((2, 1)), 0)
+            builtin
+                .final_stack(&vm.segments, &vm.memory, pointer)
+                .unwrap(),
+            Relocatable::from((2, 1))
         );
     }
 
     #[test]
     fn final_stack_error_stop_pointer() {
-        let builtin = BitwiseBuiltinRunner::new(&BitwiseInstanceDef::new(10), true);
+        let mut builtin = BitwiseBuiltinRunner::new(&BitwiseInstanceDef::new(10), true);
 
         let mut vm = vm!();
 
@@ -282,14 +287,14 @@ mod tests {
         let pointer = Relocatable::from((2, 2));
 
         assert_eq!(
-            builtin.final_stack(&vm, pointer),
+            builtin.final_stack(&vm.segments, &vm.memory, pointer),
             Err(RunnerError::InvalidStopPointer("bitwise".to_string()))
         );
     }
 
     #[test]
-    fn final_stack_error_when_not_included() {
-        let builtin = BitwiseBuiltinRunner::new(&BitwiseInstanceDef::new(10), false);
+    fn final_stack_error_when_notincluded() {
+        let mut builtin = BitwiseBuiltinRunner::new(&BitwiseInstanceDef::new(10), false);
 
         let mut vm = vm!();
 
@@ -305,14 +310,16 @@ mod tests {
         let pointer = Relocatable::from((2, 2));
 
         assert_eq!(
-            builtin.final_stack(&vm, pointer).unwrap(),
-            (Relocatable::from((2, 2)), 0)
+            builtin
+                .final_stack(&vm.segments, &vm.memory, pointer)
+                .unwrap(),
+            Relocatable::from((2, 2))
         );
     }
 
     #[test]
     fn final_stack_error_non_relocatable() {
-        let builtin = BitwiseBuiltinRunner::new(&BitwiseInstanceDef::new(10), true);
+        let mut builtin = BitwiseBuiltinRunner::new(&BitwiseInstanceDef::new(10), true);
 
         let mut vm = vm!();
 
@@ -328,7 +335,7 @@ mod tests {
         let pointer = Relocatable::from((2, 2));
 
         assert_eq!(
-            builtin.final_stack(&vm, pointer),
+            builtin.final_stack(&vm.segments, &vm.memory, pointer),
             Err(RunnerError::FinalStack)
         );
     }
@@ -524,7 +531,7 @@ mod tests {
         let vm = vm!();
 
         assert_eq!(
-            builtin.get_used_cells(&vm),
+            builtin.get_used_cells(&vm.segments),
             Err(MemoryError::MissingSegmentUsedSizes)
         );
     }
@@ -538,7 +545,7 @@ mod tests {
         let mut vm = vm!();
 
         vm.segments.segment_used_sizes = Some(vec![0]);
-        assert_eq!(builtin.get_used_cells(&vm), Ok(0));
+        assert_eq!(builtin.get_used_cells(&vm.segments), Ok(0));
     }
 
     #[test]
@@ -550,7 +557,7 @@ mod tests {
         let mut vm = vm!();
 
         vm.segments.segment_used_sizes = Some(vec![4]);
-        assert_eq!(builtin.get_used_cells(&vm), Ok(4));
+        assert_eq!(builtin.get_used_cells(&vm.segments), Ok(4));
     }
 
     #[test]