Always use a normal segment for first SegmentArena segment #1845

notlesh · 2024-09-25T21:24:27Z

(Note that this is basically a clone of Moonsong-Labs#45. We'd love to get this accepted upstream if possible.)

Problem: Starknet OS expects SegmentArena segments to all be temporary except the first segment, which must be normal. This is so that relocation rules for all segments can chain back into the first.

Solution: Use temporary segments only if it's not the first.

Description

The Starknet OS verifies the continuity of the segments in SegmentArenas here, which requires relocation rules to be properly set up.

The requirements for this to work are that each src_ptr is a temporary segment and each dest_ptr is a non-temporary segment (or a temporary segment with an existing rule mapping it to a non-temporary segment).

cairo-vm uses a number of hints to accomplish this, and these are injected by the sierra-to-casm compiler. However, it relies on one additional hint which is not injected anywhere in the case of SNOS.

This hint seems to ensure the same condition needed in SNOS: that the initial segment is a normal segment and that all other segments are temporary segments with relocation rules pointing back to the initial one. However, it creates the normal segment inside this last hint rather than when the SegmentArena is initialized.

SNOS handles this by simply allocating the initial segment as a normal one as explained here. This seems to alleviate the need to later recreate the segment in relocate_all_segments(), allowing the implementation to work in SNOS without the hint mentioned above.

Checklist

Linked to Github Issue
Unit tests added
Integration tests added.
This change requires new documentation.
- Documentation has been added/updated.
- CHANGELOG has been updated.

pefontana · 2024-09-27T22:14:20Z

Hi @notlesh
Thanks for the contribution!
There is just a minor error in the CI in the make check-fmt check
Also, would it be possible to add a small program or a little test for this fix?

notlesh · 2024-09-29T18:28:22Z

Hi @notlesh Thanks for the contribution! There is just a minor error in the CI in the make check-fmt check Also, would it be possible to add a small program or a little test for this fix?

I've wanted to produce a better way to test this, but it's complicated because the failure occurs in a different repo.

One idea for a test case is to show that the dictionaries do not have a temporary segment as their first segment prior to the RelocateAllDictionaries cheatcode being called -- this is fundamentally what was broken and what is fixed here.

This should act as a regression test (reproducing the problem and demonstrating that it's fixed) but it doesn't reproduce the exact error. Let me know what you think and if you want more details or want to know how you can reproduce the exact error yourself.

pefontana · 2024-10-02T19:46:32Z

This should act as a regression test (reproducing the problem and demonstrating that it's fixed) but it doesn't reproduce the exact error. Let me know what you think and if you want more details or want to know how you can reproduce the exact error yourself.

What do you mean by it doesn't reproduce the exact error? Can we add some Cairo program that plays with dictionaries triggering the error? O maybe just add some unit tests

Also, there are merge conflicts that need to let me approve the CI run, can you solve them?

notlesh · 2025-01-03T22:05:37Z

What do you mean by it doesn't reproduce the exact error? Can we add some Cairo program that plays with dictionaries triggering the error? O maybe just add some unit tests

Also, there are merge conflicts that need to let me approve the CI run, can you solve them?

Sorry for the long delay, I'd like to push this forward soon (resolve conflicts and add a test). In the meantime, I was wondering if you could point me to an example of the sort of test you have in mind, as I'm unsure of where to start with this.

gabrielbosio · 2025-01-08T19:37:41Z

Let me know what you think and if you want more details or want to know how you can reproduce the exact error yourself.

Hi, @notlesh! To test this, I think it would be helpful to know what's the code that's being problematic to see why is this scenario difficult to reproduce, because I'm not sure if there's a Cairo program that can be used for testing because the hint that calls new_default_dict already enforces the temporary segments to be all of the segments except the first one.
Once we have the code we can think of something smaller like a unit test.

notlesh · 2025-01-08T22:59:15Z

Let me know what you think and if you want more details or want to know how you can reproduce the exact error yourself.

Hi, @notlesh! To test this, I think it would be helpful to know what's the code that's being problematic to see why is this scenario difficult to reproduce, because I'm not sure if there's a Cairo program that can be used for testing because the hint that calls new_default_dict already enforces the temporary segments to be all of the segments except the first one. Once we have the code we can think of something smaller like a unit test.

Hey, @gabrielbosio , thanks for taking a look.

The code that is failing is the first link mentioned in the description, specifically the hint that calls memory.add_relocation_rule():

// Helper function for validate_segment_arena.
func _verify_continuity(infos: SegmentInfo*, n_segments_minus_one: felt) {
    if (n_segments_minus_one == 0) {
        // If there is only one segment left, there is no need to check anything.
        return ();
    }

    // Enforce an empty cell between two consecutive segments so that the start of a segment
    // is strictly bigger than the end of the previous segment.
    // This is required for proving the soundness of this construction, in the case where a segment
    // has length zero.

    // Note: the following code was copied from relocate_segment() for efficiency reasons.
    let src_ptr = infos[1].start;
    let dest_ptr = infos[0].end + 1;
    %{ memory.add_relocation_rule(src_ptr=ids.src_ptr, dest_ptr=ids.dest_ptr) %} // <--- FAILS HERE
    assert src_ptr = dest_ptr;

    return _verify_continuity(infos=&infos[1], n_segments_minus_one=n_segments_minus_one - 1);
}

This fails because src_ptr must be a temporary segment. cairo-vm currently does this, basically:

Allocate everything as a temporary segment during execution
At the end, in a hint injected by the Sierra-CASM compiler (IIRC), this is fixed by creating a single normal segment before processing all of the other segments

The current code works so long as this hint is called, but that's not possible in Cairo0 code -- which the OS is written in, hence the reason it doesn't work for SNOS.

This PR fixes this by ensuring that the required normal segment is allocated up front instead of fixed later on, which I think works for all cases.

Let me know if this is any clearer than it was, I know the problem is pretty detailed. I'd also be happy to jump on a call and walk through it with you. Thanks again!

gabrielbosio · 2025-01-10T15:20:19Z

This PR fixes this by ensuring that the required normal segment is allocated up front instead of fixed later on, which I think works for all cases.

To make sure that this works for all cases, I'm thinking of adding at least two tests:

Check that this works for Cairo 0. This could be achieved by calling the validate_segment_arena without failing.
Check that this works for Cairo 1. This could be achieved with a Cairo program that uses the alloc_felt_252_dict hint. For example, this program uses it:
```
use dict::{felt252_dict_entry_finalize, Felt252DictTrait};

fn main() -> (felt252, felt252) {
    let mut dict = felt252_dict_new::<felt252>();
    dict.insert(1, 64);
    let val = dict.get(1);
    dict.insert(1, 75);
    let val2 = dict.get(1);
    dict.squash();
    return (val, val2);
}
```
And then run this program and check the segments the hint creates are the same than
main. I've run this program in both main and this branch and they produced the same segments. The segments created by the hint were the ones that had index >= 6 so I think you can compare those instead of the entire memory.

gabrielbosio · 2025-01-10T17:04:37Z

Also, there's an error from clippy

gabrielbosio · 2025-01-20T14:53:09Z

@notlesh any news on this?

whichqua · 2025-03-19T10:43:39Z

Hey @gabrielbosio, I am taking on the remaining parts of this task, specifically ensuring our fix is tested appropriately.

Initial takeaways

For Cairo 1, there are already existing tests that use the alloc_felt_252_dict as seen here and here. The tests are still passing and there is not much we are gonna add on this.
For Cairo 0, we are going to add a test and this is not straight forward but I do have an attempt.

Our Cairo0 approach:

We have added a Cairo0 test that creates some segments and runs some transformations on these segments as seen on this file which is based on this test on the cairo-lang repo.
This test has some hints that are not available on cairo-vm, and hence my (currently failing) test is a little more customized as seen here.
I would like to hear your opinion on this, specifically if this approach is what your team expects.

Let me know your thoughts.

FrancoGiachetta · 2025-03-19T15:12:55Z

vm/src/tests/cairo_run_test.rs

@@ -1,4 +1,13 @@
-use crate::{tests::*, types::layout_name::LayoutName};
+use std::rc::Rc;


Hi there, I know this is still in process but this line might lead to a criptic error when running wasm tests. To save you the headache Instead of using std, try with crate::stdlib::rc::Rc.

Resolve in e4ef596

gabrielbosio

Hi @whichqua I think the proposal makes sense, broadly speaking. I left some suggestions and comments regarding specific details.

gabrielbosio · 2025-03-19T15:05:05Z

cairo_programs/test_segment_arena.cairo

@@ -0,0 +1,170 @@
+from starkware.cairo.common.alloc import alloc


If this test file is based on this one I would add a comment with a link to that file.

Resolved in 3c2c6bc

gabrielbosio · 2025-03-19T15:05:22Z

cairo_programs/test_segment_arena.cairo

+
+func main{}() {
+    let (res, segments) = test_segment_arena();
+    // assert res[0] = 1;


Why is this commented?

I need to do the actual assertions here.

Resolved in 3c2c6bc

gabrielbosio · 2025-03-19T15:06:46Z

cairo_programs/test_segment_arena.cairo

+    let (res, segments) = test_segment_arena();
+    // assert res[0] = 1;
+    ret;
+}


Please add a final newline

gabrielbosio · 2025-03-19T15:10:38Z

vm/src/tests/cairo_run_test.rs

+        proof_mode: false,
+        ..Default::default()
+    };
+    cairo_run(program_data, &cairo_run_config, &mut hint_executor).expect("Execution failed");


Given that the Python test used as reference runs some assertions after running the program, I think it would be nice if this test includes those assertions too.

Sure, I will have that.

Resolved in 3c2c6bc

gabrielbosio · 2025-03-19T15:11:41Z

vm/src/tests/cairo_run_test.rs

+    use crate::any_box;
+    use crate::hint_processor::builtin_hint_processor::hint_utils::insert_value_into_ap;
+    use crate::hint_processor::hint_processor_definition::HintReference;
+    use crate::serde::deserialize_program::ApTracking;
+    use crate::types::exec_scope::ExecutionScopes;
+    use crate::vm::errors::hint_errors::HintError;
+    use crate::vm::vm_core::VirtualMachine;
+    use crate::Felt252;
+    use indoc::indoc;
+    use std::collections::HashMap;


Please move these imports outside the test function

Resolved in 3c2c6bc

gabrielbosio · 2025-03-19T15:13:23Z

vm/src/tests/cairo_run_test.rs

+    const SEGMENTS_ADD: &str = "memory[ap] = to_felt_or_relocatable(segments.add())";
+
+    fn segments_add(
+        vm: &mut VirtualMachine,
+        _exec_scopes: &mut ExecutionScopes,
+        _ids_data: &HashMap<String, HintReference>,
+        _ap_tracking: &ApTracking,
+        _constants: &HashMap<String, Felt252>,
+    ) -> Result<(), HintError> {
+        let segment = vm.add_memory_segment();
+        insert_value_into_ap(vm, segment)
+    }
+
+    const SETUP_SEGMENT_INDEX: &str = indoc! {r#"if 'segment_index_to_arena_index' not in globals():
+            # A map from the relocatable value segment index to the index in the arena.
+            segment_index_to_arena_index = {}
+
+        # The segment is placed at the end of the arena.
+        index = ids.n_segments
+
+        # Create a segment or a temporary segment.
+        start = segments.add_temp_segment() if index > 0 else segments.add()
+
+        # Update 'SegmentInfo::start' and 'segment_index_to_arena_index'.
+        ids.prev_segment_arena.infos[index].start = start
+        segment_index_to_arena_index[start.segment_index] = index"#};
+
+    fn get_variable_from_root_exec_scope<T>(
+        exec_scopes: &ExecutionScopes,
+        name: &str,
+    ) -> Result<T, HintError>
+    where
+        T: Clone + 'static,
+    {
+        exec_scopes.data[0]
+            .get(name)
+            .and_then(|var| var.downcast_ref::<T>().cloned())
+            .ok_or(HintError::VariableNotInScopeError(
+                name.to_string().into_boxed_str(),
+            ))
+    }
+    fn setup_segment_index(
+        vm: &mut VirtualMachine,
+        exec_scopes: &mut ExecutionScopes,
+        ids_data: &HashMap<String, HintReference>,
+        ap_tracking: &ApTracking,
+        _constants: &HashMap<String, Felt252>,
+    ) -> Result<(), HintError> {
+        let mut segment_index_to_arena_index: HashMap<isize, Felt252> =
+            get_variable_from_root_exec_scope(exec_scopes, "segment_index_to_arena_index")
+                .unwrap_or_default();
+        let n_segments = get_integer_from_var_name("n_segments", vm, ids_data, ap_tracking)?;
+        let start = if n_segments > Felt252::ZERO {
+            vm.add_temporary_segment()
+        } else {
+            vm.add_memory_segment()
+        };
+
+        let prev_segment_arena_ptr =
+            get_ptr_from_var_name("prev_segment_arena", vm, ids_data, ap_tracking)?;
+
+        let infos_ptr = vm.get_relocatable(prev_segment_arena_ptr).unwrap();
+
+        let infos_start_ptr: crate::types::relocatable::Relocatable =
+            vm.load_data(infos_ptr, &[start.into()]).unwrap();
+
+        vm.insert_value(infos_start_ptr, start)?;
+
+        exec_scopes.insert_value("index", n_segments);
+        exec_scopes.insert_value("start", start);
+
+        segment_index_to_arena_index.insert(start.segment_index, n_segments);
+
+        exec_scopes.data[0].insert(
+            "segment_index_to_arena_index".to_string(),
+            any_box!(segment_index_to_arena_index),
+        );
+
+        Ok(())
+    }
+


I suggest to move these hints to a helper file in the same directory than this file.

I ended up moving the test into its own file as its pretty different with everything else in that file.
See 3c2c6bc

vm/src/hint_processor/builtin_hint_processor/segments.rs

whichqua · 2025-03-20T13:48:34Z

@gabrielbosio This is ready for review.

vm/src/hint_processor/cairo_1_hint_processor/dict_manager.rs

whichqua · 2025-03-20T16:46:15Z

@gabrielbosio @FrancoGiachetta Would be nice if we had the workflows running.

whichqua · 2025-03-21T18:40:53Z

@gabrielbosio I need your thoughts how to approach this failure. The hints are custom and are not implemented on the vm.

FrancoGiachetta · 2025-03-21T19:02:05Z

There's a way to add custom hints and add them during the vm initialization (which might suit your case since it's a test). Check this link. It's an example of how to use it, it is about adding a hint which just prints a value.

whichqua · 2025-03-24T07:10:16Z

@FrancoGiachetta That is not the problem here. Our test contains a hint that is not in the vm

FrancoGiachetta · 2025-03-24T13:11:18Z

Yes, I understand that. Custom hints are an easy way of defining hints that are no implemented in the vm. I think that should be the solution to the issue here.

whichqua · 2025-03-24T13:14:55Z

@FrancoGiachetta The hints themselves are already implemented here

FrancoGiachetta · 2025-03-24T13:26:28Z

Mmm, I see. Perhaps it's not well formated?

whichqua · 2025-03-24T13:31:08Z

I dont think so, I can successfully run make test without any problem. The failures come from this workflow.

FrancoGiachetta · 2025-03-24T14:19:55Z

Okay. The issue seems to be that since your program is in cairo_programs/ it's being taken into account by that workflow. Since it uses the cairo-vm-cli to run it, that custom hints are not defined. I can only suggest you to move that code to a different location to see if that solves the problem. Create a new folder inside cairo-programs/ an place you program there.
Let me know if that helps.

Makefile

whichqua · 2025-03-26T14:15:36Z

@gabrielbosio This should be good right?

Co-authored-by: Stephen Shelton <[email protected]> Co-authored-by: Geoffrey Mureithi <[email protected]>

gabrielbosio · 2025-03-26T22:03:21Z

@whichqua looks good, I'm waiting for the CI in a clone of this PR (#2029) to make sure the jobs that fail here pass in the clone.

gabrielbosio · 2025-03-26T22:33:35Z

@whichqua I've run the CI and the codecov job fails. It seems that you need to cover the cases where relocate_all_dictionaries returns Err.
Here's the link to the codecov analysis.

gabrielbosio · 2025-03-27T15:09:32Z

Also, all the commits in this PR must be signed before merging.

whichqua · 2025-03-27T15:51:58Z

It seems that you need to cover the cases where relocate_all_dictionaries returns Err.

I think I can add unit tests on the called methods. Would that be enough?

gabrielbosio · 2025-03-28T14:09:17Z

I think it would 👍

Co-authored-by: notlesh <[email protected]>

whichqua · 2025-04-01T14:31:30Z

@gabrielbosio I had to work though the resigning of some past commits but ended up settling for a force push. Do you need @notlesh to also sign in this case?

whichqua · 2025-04-02T16:04:02Z

Any updates on this @gabrielbosio?

gabrielbosio · 2025-04-04T14:15:39Z

Do you need @notlesh to also sign in this case?

It seems it's enough with what you did. I'm running the CI jobs in #2048 to check if coverage is OK.

whichqua · 2025-04-04T15:06:27Z

@gabrielbosio Seems everything is ✅

notlesh requested review from igaray, Oppen, fmoletta, juanbono and pefontana as code owners September 25, 2024 21:24

notlesh mentioned this pull request Jan 3, 2025

chore: SNOS dependency customizations create duplicates in crate tree keep-starknet-strange/snos#458

Open

notlesh requested a review from gabrielbosio as a code owner January 8, 2025 17:50

whichqua requested review from yuvalsw and noaov1 as code owners March 19, 2025 10:17

FrancoGiachetta reviewed Mar 19, 2025

View reviewed changes

gabrielbosio requested changes Mar 19, 2025

View reviewed changes

whichqua reviewed Mar 20, 2025

View reviewed changes

vm/src/hint_processor/builtin_hint_processor/segments.rs Show resolved Hide resolved

FrancoGiachetta requested changes Mar 20, 2025

View reviewed changes

vm/src/hint_processor/cairo_1_hint_processor/dict_manager.rs Outdated Show resolved Hide resolved

FrancoGiachetta reviewed Mar 25, 2025

View reviewed changes

Makefile Outdated Show resolved Hide resolved

whichqua reviewed Mar 25, 2025

View reviewed changes

Makefile Outdated Show resolved Hide resolved

gabrielbosio added a commit that referenced this pull request Mar 26, 2025

Always use a normal segment for first SegmentArena segment #1845

399b074

Co-authored-by: Stephen Shelton <[email protected]> Co-authored-by: Geoffrey Mureithi <[email protected]>

gabrielbosio mentioned this pull request Mar 26, 2025

[DNM] Always use a normal segment for first SegmentArena segment #2029

Closed

fix: Always use a normal segment in first SegmentArena segment

b8f841e

Co-authored-by: notlesh <[email protected]>

whichqua force-pushed the notlesh/segment-arena-relocation-fix-upstream branch from 9557d56 to b8f841e Compare April 1, 2025 09:57

Merge branch 'main' into notlesh/segment-arena-relocation-fix-upstream

685fc69

gabrielbosio mentioned this pull request Apr 4, 2025

[DNM] Segment arena relocation fix upstream #2048

Closed

gabrielbosio approved these changes Apr 4, 2025

View reviewed changes

FrancoGiachetta approved these changes Apr 4, 2025

View reviewed changes

FrancoGiachetta added this pull request to the merge queue Apr 4, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 4, 2025

FrancoGiachetta added this pull request to the merge queue Apr 4, 2025

Merged via the queue into lambdaclass:main with commit 0038711 Apr 4, 2025
172 of 184 checks passed

whichqua deleted the notlesh/segment-arena-relocation-fix-upstream branch April 8, 2025 12:14

		@@ -1,4 +1,13 @@
		use crate::{tests::*, types::layout_name::LayoutName};
		use std::rc::Rc;

		@@ -0,0 +1,170 @@
		from starkware.cairo.common.alloc import alloc

Always use a normal segment for first SegmentArena segment #1845

Always use a normal segment for first SegmentArena segment #1845

Conversation

notlesh commented Sep 25, 2024 • edited Loading

Description

Checklist

pefontana commented Sep 27, 2024 • edited Loading

notlesh commented Sep 29, 2024

pefontana commented Oct 2, 2024

notlesh commented Jan 3, 2025

gabrielbosio commented Jan 8, 2025

notlesh commented Jan 8, 2025

gabrielbosio commented Jan 10, 2025

gabrielbosio commented Jan 10, 2025

gabrielbosio commented Jan 20, 2025

whichqua commented Mar 19, 2025 • edited Loading

Initial takeaways

Our Cairo0 approach:

FrancoGiachetta Mar 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielbosio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whichqua commented Mar 20, 2025

whichqua commented Mar 20, 2025

whichqua commented Mar 21, 2025

FrancoGiachetta commented Mar 21, 2025 • edited Loading

whichqua commented Mar 24, 2025

FrancoGiachetta commented Mar 24, 2025

whichqua commented Mar 24, 2025

FrancoGiachetta commented Mar 24, 2025

whichqua commented Mar 24, 2025

FrancoGiachetta commented Mar 24, 2025 • edited Loading

whichqua commented Mar 26, 2025

gabrielbosio commented Mar 26, 2025

gabrielbosio commented Mar 26, 2025

gabrielbosio commented Mar 27, 2025

whichqua commented Mar 27, 2025

gabrielbosio commented Mar 28, 2025

whichqua commented Apr 1, 2025

whichqua commented Apr 2, 2025

gabrielbosio commented Apr 4, 2025

whichqua commented Apr 4, 2025

notlesh commented Sep 25, 2024 •

edited

Loading

pefontana commented Sep 27, 2024 •

edited

Loading

whichqua commented Mar 19, 2025 •

edited

Loading

FrancoGiachetta Mar 19, 2025 •

edited

Loading

FrancoGiachetta commented Mar 21, 2025 •

edited

Loading

FrancoGiachetta commented Mar 24, 2025 •

edited

Loading