Skip to content

Commit 0038711

Browse files
notleshwhichqua
andauthored
fix: Always use a normal segment in first SegmentArena segment (#1845)
Co-authored-by: Whichqua <[email protected]>
1 parent 368e3fb commit 0038711

File tree

8 files changed

+519
-10
lines changed

8 files changed

+519
-10
lines changed

CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22

33
#### Upcoming Changes
44

5+
* fix: Always use a normal segment in first SegmentArena segment [#1845](https://github.com/lambdaclass/cairo-vm/pull/1845)
6+
57
* chore: update cairo-lang dependencies to 2.12.0-dev.0 #[2040](https://github.com/lambdaclass/cairo-vm/pull/2040)
68

79
* feat: add get_current_step getter [#2034](https://github.com/lambdaclass/cairo-vm/pull/2034)

Makefile

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,10 @@ KZG_DA_CAIRO0_HINTS_DIR=cairo_programs/cairo-0-kzg-da-hints
105105
KZG_DA_CAIRO0_HINTS_FILES:=$(wildcard $(KZG_DA_CAIRO0_HINTS_DIR)/*.cairo)
106106
COMPILED_KZG_DA_CAIRO0_HINTS:=$(patsubst $(KZG_DA_CAIRO0_HINTS_DIR)/%.cairo, $(KZG_DA_CAIRO0_HINTS_DIR)/%.json, $(KZG_DA_CAIRO0_HINTS_FILES))
107107

108+
SEGMENT_ARENA_CAIRO0_HINTS_DIR=cairo_programs/segment_arena
109+
SEGMENT_ARENA_CAIRO0_HINTS_FILES:=$(wildcard $(SEGMENT_ARENA_CAIRO0_HINTS_DIR)/*.cairo)
110+
COMPILED_SEGMENT_ARENA_CAIRO0_HINTS:=$(patsubst $(SEGMENT_ARENA_CAIRO0_HINTS_DIR)/%.cairo, $(SEGMENT_ARENA_CAIRO0_HINTS_DIR)/%.json, $(SEGMENT_ARENA_CAIRO0_HINTS_FILES))
111+
108112
PRINT_TEST_DIR=cairo_programs/print_feature
109113
PRINT_TEST_FILES:=$(wildcard $(PRINT_TEST_DIR)/*.cairo)
110114
COMPILED_PRINT_TESTS:=$(patsubst $(PRINT_TEST_DIR)/%.cairo, $(PRINT_TEST_DIR)/%.json, $(PRINT_TEST_FILES))
@@ -254,7 +258,7 @@ run:
254258
check:
255259
cargo check
256260

257-
cairo_test_programs: $(COMPILED_TESTS) $(COMPILED_BAD_TESTS) $(COMPILED_NORETROCOMPAT_TESTS) $(COMPILED_PRINT_TESTS) $(COMPILED_MOD_BUILTIN_TESTS) $(COMPILED_SECP_CAIRO0_HINTS) $(COMPILED_KZG_DA_CAIRO0_HINTS)
261+
cairo_test_programs: $(COMPILED_TESTS) $(COMPILED_BAD_TESTS) $(COMPILED_NORETROCOMPAT_TESTS) $(COMPILED_PRINT_TESTS) $(COMPILED_MOD_BUILTIN_TESTS) $(COMPILED_SECP_CAIRO0_HINTS) $(COMPILED_KZG_DA_CAIRO0_HINTS) $(COMPILED_SEGMENT_ARENA_CAIRO0_HINTS)
258262
cairo_proof_programs: $(COMPILED_PROOF_TESTS) $(COMPILED_MOD_BUILTIN_PROOF_TESTS) $(COMPILED_STWO_EXCLUSIVE_TESTS)
259263
cairo_bench_programs: $(COMPILED_BENCHES)
260264
cairo_1_test_contracts: $(CAIRO_1_COMPILED_CASM_CONTRACTS)
Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,172 @@
1+
// This test contains parts imported from cairo-lang
2+
// https://github.com/starkware-libs/cairo-lang/blob/master/src/starkware/starknet/builtins/segment_arena/segment_arena_test.cairo
3+
4+
from starkware.cairo.common.alloc import alloc
5+
6+
// Represents the information about a single segment allocated by the arena.
7+
struct SegmentInfo {
8+
// A pointer to the first element of this segment.
9+
start: felt*,
10+
// A pointer to the end of this segment (the first unused element).
11+
end: felt*,
12+
// A sequential id, assigned to the segment when it is finalized.
13+
// This value is used to guarantee that 'end' is not assigned twice.
14+
finalization_index: felt,
15+
}
16+
17+
// Represents the status of the segment arena.
18+
struct SegmentArenaBuiltin {
19+
// A pointer to a list of SegmentInfo. infos[i] contains information about the i-th segment
20+
// (ordered by construction).
21+
// The value is fixed during the execution of an entry point.
22+
infos: SegmentInfo*,
23+
// The number of segments that were created so far.
24+
n_segments: felt,
25+
// The number of segments that were finalized so far.
26+
n_finalized: felt,
27+
}
28+
29+
// Constructs a new segment for the segment arena builtin and initializes it with an empty instance
30+
// of `SegmentArenaBuiltin`.
31+
func new_arena() -> SegmentArenaBuiltin* {
32+
let (segment_arena: SegmentArenaBuiltin*) = alloc();
33+
assert segment_arena[0] = SegmentArenaBuiltin(
34+
infos=cast(nondet %{ segments.add() %}, SegmentInfo*), n_segments=0, n_finalized=0
35+
);
36+
return &segment_arena[1];
37+
}
38+
39+
// Validates the segment arena builtin.
40+
//
41+
// In particular, relocates the temporary segments such that the start of segment i is strictly
42+
// larger than the end of segment i+1.
43+
func validate_segment_arena(segment_arena: SegmentArenaBuiltin*) {
44+
tempvar n_segments = segment_arena.n_segments;
45+
tempvar n_finalized = segment_arena.n_finalized;
46+
// The following line should follow from the fact that every allocated segment
47+
// must be finalized exactly once.
48+
// We keep it both as a sanity check and since Sierra compilation is not proven yet.
49+
assert n_segments = n_finalized;
50+
51+
if (n_segments == 0) {
52+
return ();
53+
}
54+
55+
// The following call also implies that n_segments > 0.
56+
_verify_continuity(infos=segment_arena.infos, n_segments_minus_one=n_segments - 1);
57+
return ();
58+
}
59+
60+
// Helper function for validate_segment_arena.
61+
func _verify_continuity(infos: SegmentInfo*, n_segments_minus_one: felt) {
62+
if (n_segments_minus_one == 0) {
63+
// If there is only one segment left, there is no need to check anything.
64+
return ();
65+
}
66+
67+
// Enforce an empty cell between two consecutive segments so that the start of a segment
68+
// is strictly bigger than the end of the previous segment.
69+
// This is required for proving the soundness of this construction, in the case where a segment
70+
// has length zero.
71+
72+
// Note: the following code was copied from relocate_segment() for efficiency reasons.
73+
let src_ptr = infos[1].start;
74+
let dest_ptr = infos[0].end + 1;
75+
%{ memory.add_relocation_rule(src_ptr=ids.src_ptr, dest_ptr=ids.dest_ptr) %}
76+
assert src_ptr = dest_ptr;
77+
78+
return _verify_continuity(infos=&infos[1], n_segments_minus_one=n_segments_minus_one - 1);
79+
}
80+
81+
82+
// Creates a new segment using the segment arena.
83+
func new_segment{segment_arena: SegmentArenaBuiltin*}() -> felt* {
84+
let prev_segment_arena = &segment_arena[-1];
85+
tempvar n_segments = prev_segment_arena.n_segments;
86+
tempvar infos = prev_segment_arena.infos;
87+
88+
%{
89+
if 'segment_index_to_arena_index' not in globals():
90+
# A map from the relocatable value segment index to the index in the arena.
91+
segment_index_to_arena_index = {}
92+
93+
# The segment is placed at the end of the arena.
94+
index = ids.n_segments
95+
96+
# Create a segment or a temporary segment.
97+
start = segments.add_temp_segment() if index > 0 else segments.add()
98+
99+
# Update 'SegmentInfo::start' and 'segment_index_to_arena_index'.
100+
ids.prev_segment_arena.infos[index].start = start
101+
segment_index_to_arena_index[start.segment_index] = index
102+
%}
103+
assert segment_arena[0] = SegmentArenaBuiltin(
104+
infos=infos, n_segments=n_segments + 1, n_finalized=prev_segment_arena.n_finalized
105+
);
106+
let segment_arena = &segment_arena[1];
107+
return infos[n_segments].start;
108+
}
109+
110+
// Finalizes a given segment and returns the corresponding start.
111+
func finalize_segment{segment_arena: SegmentArenaBuiltin*}(segment_end: felt*) -> felt* {
112+
let prev_segment_arena = &segment_arena[-1];
113+
tempvar n_segments = prev_segment_arena.n_segments;
114+
tempvar n_finalized = prev_segment_arena.n_finalized;
115+
116+
// Guess the index of the segment.
117+
tempvar index = nondet %{ segment_index_to_arena_index[ids.segment_end.segment_index] %};
118+
119+
// Write segment_end in the manager.
120+
tempvar infos: SegmentInfo* = prev_segment_arena.infos;
121+
tempvar segment_info: SegmentInfo* = &infos[index];
122+
// Writing n_finalized to 'finalization_index' guarantees 'segment_info.end' was not assigned
123+
// a value before.
124+
assert segment_info.finalization_index = n_finalized;
125+
assert segment_info.end = segment_end;
126+
127+
assert segment_arena[0] = SegmentArenaBuiltin(
128+
infos=infos, n_segments=n_segments, n_finalized=n_finalized + 1
129+
);
130+
131+
let segment_arena = &segment_arena[1];
132+
return segment_info.start;
133+
}
134+
135+
func test_segment_arena() -> (felt*, SegmentInfo*) {
136+
alloc_locals;
137+
138+
local segment_arena_start: SegmentArenaBuiltin* = new_arena();
139+
let segment_arena = segment_arena_start;
140+
141+
with segment_arena {
142+
let segment0 = new_segment();
143+
let segment1 = new_segment();
144+
let segment2 = new_segment();
145+
146+
assert segment0[0] = 1;
147+
assert segment0[1] = 2;
148+
149+
assert segment1[0] = 3;
150+
assert segment1[1] = 4;
151+
152+
assert segment2[0] = 5;
153+
154+
assert finalize_segment(segment0 + 2) = segment0;
155+
assert finalize_segment(segment1 + 2) = segment1;
156+
157+
let segment3 = new_segment();
158+
159+
assert segment3[0] = 6;
160+
assert segment3[1] = 7;
161+
162+
assert finalize_segment(segment3 + 2) = segment3;
163+
assert finalize_segment(segment2 + 1) = segment2;
164+
}
165+
validate_segment_arena(segment_arena=&segment_arena[-1]);
166+
return (segment0, segment_arena_start[-1].infos);
167+
}
168+
169+
func main{}() {
170+
let (_segment0, _infos) = test_segment_arena();
171+
ret;
172+
}

vm/src/hint_processor/builtin_hint_processor/segments.rs

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,35 @@ pub fn relocate_segment(
1919
ap_tracking: &ApTracking,
2020
) -> Result<(), HintError> {
2121
let src_ptr = get_ptr_from_var_name("src_ptr", vm, ids_data, ap_tracking)?;
22-
#[cfg(not(feature = "extensive_hints"))]
23-
let dest_ptr = get_ptr_from_var_name("dest_ptr", vm, ids_data, ap_tracking)?;
24-
#[cfg(feature = "extensive_hints")]
25-
let dest_ptr = crate::hint_processor::builtin_hint_processor::hint_utils::get_maybe_relocatable_from_var_name("dest_ptr", vm, ids_data, ap_tracking)?;
26-
22+
// Bugfix: this is a workaround for an issue in the vm related to the way it computes
23+
// `dest_ptr` in `segment_arena.cairo`. For some reason it sets `dest_ptr` to 0 (felt) instead
24+
// of a valid relocatable value.
25+
// As this hint is used in other places, we need to determine if we are in `segments_arena`
26+
// first. We check this by determining if the "infos" variable is declared.
27+
// If it is the case, we simply recompute `dest_ptr` manually.
28+
let dest_ptr = if let Ok(infos) = get_ptr_from_var_name("infos", vm, ids_data, ap_tracking) {
29+
let infos_0_end = vm.get_relocatable((infos + 1)?)?;
30+
31+
#[cfg(not(feature = "extensive_hints"))]
32+
{
33+
(infos_0_end + 1u32)?
34+
}
35+
36+
#[cfg(feature = "extensive_hints")]
37+
{
38+
crate::types::relocatable::MaybeRelocatable::RelocatableValue((infos_0_end + 1u32)?)
39+
}
40+
} else {
41+
#[cfg(not(feature = "extensive_hints"))]
42+
{
43+
get_ptr_from_var_name("dest_ptr", vm, ids_data, ap_tracking)?
44+
}
45+
46+
#[cfg(feature = "extensive_hints")]
47+
{
48+
crate::hint_processor::builtin_hint_processor::hint_utils::get_maybe_relocatable_from_var_name("dest_ptr", vm, ids_data, ap_tracking)?
49+
}
50+
};
2751
vm.add_relocation_rule(src_ptr, dest_ptr)
2852
.map_err(HintError::Memory)?;
2953
Ok(())

vm/src/hint_processor/cairo_1_hint_processor/dict_manager.rs

Lines changed: 97 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ impl DictManagerExecScope {
5757

5858
/// Allocates a new segment for a new dictionary and return the start of the segment.
5959
pub fn new_default_dict(&mut self, vm: &mut VirtualMachine) -> Result<Relocatable, HintError> {
60-
let dict_segment = if self.use_temporary_segments {
60+
let dict_segment = if self.use_temporary_segments && !self.trackers.is_empty() {
6161
vm.add_temporary_segment()
6262
} else {
6363
vm.add_memory_segment()
@@ -123,9 +123,28 @@ impl DictManagerExecScope {
123123
/// Relocates all dictionaries into a single segment
124124
/// Does nothing if use_temporary_segments is set to false
125125
pub fn relocate_all_dictionaries(&mut self, vm: &mut VirtualMachine) -> Result<(), HintError> {
126-
if self.use_temporary_segments {
127-
let mut prev_end = vm.add_memory_segment();
128-
for tracker in &self.trackers {
126+
// We expect the first segment to be a normal one, which doesn't require relocation. So
127+
// there is nothing to do unless there are at least two segments.
128+
if self.use_temporary_segments && !self.trackers.is_empty() {
129+
let first_segment = self.trackers.first().ok_or(HintError::CustomHint(
130+
"Trackers must have a first element".into(),
131+
))?;
132+
if first_segment.start.segment_index < 0 {
133+
return Err(HintError::CustomHint(
134+
"First dict segment should not be temporary"
135+
.to_string()
136+
.into_boxed_str(),
137+
));
138+
}
139+
let mut prev_end = first_segment.end.unwrap_or_default();
140+
for tracker in &self.trackers[1..] {
141+
if tracker.start.segment_index >= 0 {
142+
return Err(HintError::CustomHint(
143+
"Dict segment should be temporary"
144+
.to_string()
145+
.into_boxed_str(),
146+
));
147+
}
129148
#[cfg(feature = "extensive_hints")]
130149
{
131150
vm.add_relocation_rule(
@@ -220,3 +239,77 @@ impl DictSquashExecScope {
220239
self.current_access_indices()?.pop()
221240
}
222241
}
242+
243+
#[cfg(test)]
244+
mod tests {
245+
use super::*;
246+
use crate::stdlib::collections::HashMap;
247+
use crate::types::relocatable::Relocatable;
248+
use crate::vm::vm_core::VirtualMachine;
249+
250+
/// Test for relocate_all_dictionaries error cases
251+
#[test]
252+
fn test_relocate_all_dictionaries_errors() {
253+
let mut vm = VirtualMachine::new(false, false);
254+
255+
// Test 1: First segment is a temporary segment (should error)
256+
{
257+
let mut dict_manager = DictManagerExecScope::new(true);
258+
let first_dict_start = Relocatable::from((-1, 0)); // Temporary segment
259+
260+
dict_manager.trackers.push(DictTrackerExecScope {
261+
data: HashMap::default(),
262+
start: first_dict_start,
263+
end: Some(Relocatable::from((-1, 10))),
264+
});
265+
266+
let result = dict_manager.relocate_all_dictionaries(&mut vm);
267+
assert!(matches!(
268+
result,
269+
Err(HintError::CustomHint(_)) if result.unwrap_err().to_string().contains("First dict segment should not be temporary")
270+
));
271+
}
272+
273+
// Test 2: Non-temporary dictionary segment
274+
{
275+
let mut dict_manager = DictManagerExecScope::new(true);
276+
let first_dict_start = Relocatable::from((0, 0)); // Non-temporary segment
277+
let second_dict_start = Relocatable::from((1, 0)); // Non-temporary segment
278+
279+
dict_manager.trackers.push(DictTrackerExecScope {
280+
data: HashMap::default(),
281+
start: first_dict_start,
282+
end: Some(Relocatable::from((0, 10))),
283+
});
284+
dict_manager.trackers.push(DictTrackerExecScope {
285+
data: HashMap::default(),
286+
start: second_dict_start,
287+
end: Some(Relocatable::from((1, 10))),
288+
});
289+
290+
let result = dict_manager.relocate_all_dictionaries(&mut vm);
291+
assert!(matches!(
292+
result,
293+
Err(HintError::CustomHint(_)) if result.unwrap_err().to_string().contains("Dict segment should be temporary")
294+
));
295+
}
296+
}
297+
298+
/// Test for relocate_all_dictionaries when no temporary segments
299+
#[test]
300+
fn test_relocate_all_dictionaries_no_temporary_segments() {
301+
let mut vm = VirtualMachine::new(false, false);
302+
let mut dict_manager = DictManagerExecScope::new(false);
303+
304+
// Adding some trackers should not cause any errors
305+
dict_manager.trackers.push(DictTrackerExecScope {
306+
data: HashMap::default(),
307+
start: Relocatable::from((0, 0)),
308+
end: Some(Relocatable::from((0, 10))),
309+
});
310+
311+
// Should not error and essentially do nothing
312+
let result = dict_manager.relocate_all_dictionaries(&mut vm);
313+
assert!(result.is_ok());
314+
}
315+
}

vm/src/tests/cairo_run_test.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
use crate::{tests::*, types::layout_name::LayoutName};
2+
23
#[cfg(feature = "mod_builtin")]
34
use crate::{
45
utils::test_utils::Program,

vm/src/tests/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ mod run_deprecated_contract_class_simplified;
3636
mod cairo_1_run_from_entrypoint_tests;
3737
mod cairo_run_test;
3838
mod pedersen_test;
39+
mod segment_arena_test;
3940
mod struct_test;
4041

4142
mod cairo_pie_test;

0 commit comments

Comments
 (0)