Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix MemorySegmentManager::add_zero_segment #1818

Merged
merged 10 commits into from
Aug 8, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

#### Upcoming Changes

* fix(BREAKING): [#1818](https://github.com/lambdaclass/cairo-vm/pull/1818):
* Fix `MemorySegmentManager::add_zero_segment` function when resizing a segment
* Signature change: `MemorySegmentManager::get_memory_holes` now receives `builtin_segment_indexes: HashSet<usize>`

* fix(BREAKING): Replace `CairoRunner` method `initialize_all_builtins` with `initialize_program_builtins`. Now it only initializes program builtins instead of all of them

#### [1.0.0] - 2024-08-01
Expand Down
12 changes: 7 additions & 5 deletions vm/src/vm/runners/cairo_runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -791,14 +791,16 @@ impl CairoRunner {

/// Count the number of holes present in the segments.
pub fn get_memory_holes(&self) -> Result<usize, MemoryError> {
let output_builtin_index = self
// Grab builtin segment indexes, except for the output builtin
let builtin_segment_indexes: HashSet<usize> = 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)
.filter(|b| b.name() != BuiltinName::output)
.map(|b| b.base())
.collect();

self.vm.segments.get_memory_holes(builtin_segment_indexes)
}

/// Check if there are enough trace cells to fill the entire diluted checks.
Expand Down
45 changes: 25 additions & 20 deletions vm/src/vm/vm_memory/memory_segments.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::stdlib::collections::HashSet;
use core::cmp::max;
use core::fmt;

Expand Down Expand Up @@ -200,27 +201,18 @@
}

/// 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
/// # Parameters
/// - `builtin_segment_indexes`: Set representing the segments indexes of the builtins initialized in the VM, except for the output builtin.
pub fn get_memory_holes(
&self,
builtin_count: usize,
output_builtin_index: Option<usize>,
builtin_segment_indexes: HashSet<usize>,
) -> Result<usize, MemoryError> {
let data = &self.memory.data;
let mut memory_holes = 0;
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
&& !output_segment_index.is_some_and(|output_index| output_index == i)
{
if builtin_segment_indexes.contains(&i) {
continue;
}
let accessed_amount =
Expand Down Expand Up @@ -293,8 +285,9 @@
if self.zero_segment_index.is_zero() {
self.zero_segment_index = self.add().segment_index as usize;
}

// Fil zero segment with zero values until size is reached
for _ in 0..self.zero_segment_size.saturating_sub(size) {
for _ in 0..(size.saturating_sub(self.zero_segment_size)) {

Check warning on line 290 in vm/src/vm/vm_memory/memory_segments.rs

View check run for this annotation

Codecov / codecov/patch

vm/src/vm/vm_memory/memory_segments.rs#L290

Added line #L290 was not covered by tests
// As zero_segment_index is only accessible to the segment manager
// we can asume that it is always valid and index direcly into it
self.memory.data[self.zero_segment_index].push(MemoryCell::new(Felt252::ZERO.into()))
Expand Down Expand Up @@ -690,7 +683,7 @@
.memory
.mark_as_accessed((0, 0).into());
assert_eq!(
memory_segment_manager.get_memory_holes(0, None),
memory_segment_manager.get_memory_holes(HashSet::new()),
Err(MemoryError::MissingSegmentUsedSizes),
);
}
Expand All @@ -707,7 +700,7 @@
.mark_as_accessed((0, i).into());
}
assert_eq!(
memory_segment_manager.get_memory_holes(0, None),
memory_segment_manager.get_memory_holes(HashSet::new()),
Err(MemoryError::SegmentHasMoreAccessedAddressesThanSize(
Box::new((0, 3, 2))
)),
Expand All @@ -719,15 +712,21 @@
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, None), Ok(0),);
assert_eq!(
memory_segment_manager.get_memory_holes(HashSet::new()),
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, None), Ok(0),);
assert_eq!(
memory_segment_manager.get_memory_holes(HashSet::new()),
Ok(0),
);
}

#[test]
Expand All @@ -750,7 +749,10 @@
.memory
.mark_as_accessed((0, i).into());
}
assert_eq!(memory_segment_manager.get_memory_holes(0, None), Ok(2),);
assert_eq!(
memory_segment_manager.get_memory_holes(HashSet::new()),
Ok(2),
);
}

#[test]
Expand All @@ -775,7 +777,10 @@
.memory
.mark_as_accessed((0, i).into());
}
assert_eq!(memory_segment_manager.get_memory_holes(0, None), Ok(7),);
assert_eq!(
memory_segment_manager.get_memory_holes(HashSet::new()),
Ok(7),
);
}

#[test]
Expand Down
Loading