fix: Don't assume output builtin is first when counting memory holes (#…

…1811)

* Dont asume output builtin is first when counting memory holes

* Add changelog entry

* Fix + doc

* Fix typo

CHANGELOG.md

@@ -2,6 +2,11 @@
 
 #### Upcoming Changes
 
+* fix(BREAKING): Don't assume output builtin is first when counting memory holes
+
+  * Logic change: Memory hole counting no longer asumes that the output builtin ocuppies the first builtin segment if present
+  * Signature change: `MemorySegmentManager` method `get_memory_holes` now receives the index of the output builtin (as an `Option<usize>`) instead of the boolean argument `has_output_builtin`[#1807](https://github.com/lambdaclass/cairo-vm/pull/1811)
+
 * fix: ambiguous keccak module name use on external contexts
 
 #### [1.0.0-rc6] - 2024-07-22

vm/src/vm/runners/cairo_runner.rs

@@ -817,10 +817,14 @@ impl CairoRunner {
 
     /// Count the number of holes present in the segments.
     pub fn get_memory_holes(&self) -> Result<usize, MemoryError> {
-        self.vm.segments.get_memory_holes(
-            self.vm.builtin_runners.len(),
-            self.program.builtins.contains(&BuiltinName::output),
-        )
+        let output_builtin_index = self
+            .vm
+            .builtin_runners
+            .iter()
+            .position(|b| b.name() == BuiltinName::output);
+        self.vm
+            .segments
+            .get_memory_holes(self.vm.builtin_runners.len(), output_builtin_index)
     }
 
     /// Check if there are enough trace cells to fill the entire diluted checks.

vm/src/vm/vm_memory/memory_segments.rs

@@ -199,25 +199,28 @@ impl MemorySegmentManager {
         }
     }
 
+    /// Counts the memory holes (aka unaccessed memory cells) in memory
+    /// Receives the amount of builtins in the vm and the position of the output builtin within the builtins list if present
     pub fn get_memory_holes(
         &self,
         builtin_count: usize,
-        has_output_builtin: bool,
+        output_builtin_index: Option<usize>,
     ) -> Result<usize, MemoryError> {
         let data = &self.memory.data;
         let mut memory_holes = 0;
-        let builtin_segments_start = if has_output_builtin {
-            2 // program segment + execution segment + output segment
-        } else {
-            1 // program segment + execution segment
-        };
+        let builtin_segments_start = 1; // program segment + execution segment
         let builtin_segments_end = builtin_segments_start + builtin_count;
+        let output_segment_index =
+            output_builtin_index.map(|i| i + 2 /*program segment + execution segment*/);
         // Count the memory holes for each segment by substracting the amount of accessed_addresses from the segment's size
         // Segments without accesses addresses are not accounted for when counting memory holes
         for i in 0..data.len() {
             // Instead of marking all of the builtin segment's address as accessed, we just skip them when counting memory holes
             // Output builtin is extempt from this behaviour
-            if i > builtin_segments_start && i <= builtin_segments_end {
+            if i > builtin_segments_start
+                && i <= builtin_segments_end
+                && !output_segment_index.is_some_and(|output_index| output_index == i)
+            {
                 continue;
             }
             let accessed_amount =
@@ -687,7 +690,7 @@ mod tests {
             .memory
             .mark_as_accessed((0, 0).into());
         assert_eq!(
-            memory_segment_manager.get_memory_holes(0, false),
+            memory_segment_manager.get_memory_holes(0, None),
             Err(MemoryError::MissingSegmentUsedSizes),
         );
     }
@@ -704,7 +707,7 @@ mod tests {
                 .mark_as_accessed((0, i).into());
         }
         assert_eq!(
-            memory_segment_manager.get_memory_holes(0, false),
+            memory_segment_manager.get_memory_holes(0, None),
             Err(MemoryError::SegmentHasMoreAccessedAddressesThanSize(
                 Box::new((0, 3, 2))
             )),
@@ -716,15 +719,15 @@ mod tests {
     fn get_memory_holes_empty() {
         let mut memory_segment_manager = MemorySegmentManager::new();
         memory_segment_manager.segment_used_sizes = Some(Vec::new());
-        assert_eq!(memory_segment_manager.get_memory_holes(0, false), Ok(0),);
+        assert_eq!(memory_segment_manager.get_memory_holes(0, None), Ok(0),);
     }
 
     #[test]
     #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
     fn get_memory_holes_empty2() {
         let mut memory_segment_manager = MemorySegmentManager::new();
         memory_segment_manager.segment_used_sizes = Some(vec![4]);
-        assert_eq!(memory_segment_manager.get_memory_holes(0, false), Ok(0),);
+        assert_eq!(memory_segment_manager.get_memory_holes(0, None), Ok(0),);
     }
 
     #[test]
@@ -747,7 +750,7 @@ mod tests {
                 .memory
                 .mark_as_accessed((0, i).into());
         }
-        assert_eq!(memory_segment_manager.get_memory_holes(0, false), Ok(2),);
+        assert_eq!(memory_segment_manager.get_memory_holes(0, None), Ok(2),);
     }
 
     #[test]
@@ -772,7 +775,7 @@ mod tests {
                 .memory
                 .mark_as_accessed((0, i).into());
         }
-        assert_eq!(memory_segment_manager.get_memory_holes(0, false), Ok(7),);
+        assert_eq!(memory_segment_manager.get_memory_holes(0, None), Ok(7),);
     }
 
     #[test]