From d33589af2734b33e5f93b009aad739aa3a207dd0 Mon Sep 17 00:00:00 2001 From: fmoletta <99273364+fmoletta@users.noreply.github.com> Date: Wed, 31 Jul 2024 13:13:22 -0300 Subject: [PATCH] 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 | 5 +++++ vm/src/vm/runners/cairo_runner.rs | 12 +++++++---- vm/src/vm/vm_memory/memory_segments.rs | 29 ++++++++++++++------------ 3 files changed, 29 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 35de4edb75..ef644036ae 100644 --- a/CHANGELOG.md +++ b/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`) 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 diff --git a/vm/src/vm/runners/cairo_runner.rs b/vm/src/vm/runners/cairo_runner.rs index c6cb1ae141..5bb9305939 100644 --- a/vm/src/vm/runners/cairo_runner.rs +++ b/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 { - 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. diff --git a/vm/src/vm/vm_memory/memory_segments.rs b/vm/src/vm/vm_memory/memory_segments.rs index 71b727c1cc..b0fa04c327 100644 --- a/vm/src/vm/vm_memory/memory_segments.rs +++ b/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, ) -> Result { 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,7 +719,7 @@ 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] @@ -724,7 +727,7 @@ mod tests { 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]