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

core: not able to run when execution_base segment is not 1 #1908

Open
ClementWalter opened this issue Jan 4, 2025 · 6 comments
Open

core: not able to run when execution_base segment is not 1 #1908

ClementWalter opened this issue Jan 4, 2025 · 6 comments
Labels
bug Something isn't working

Comments

@ClementWalter
Copy link

ClementWalter commented Jan 4, 2025

Describe the bug

It's possible to initialize the runner with self.execution_base != (1, 0) as the initialize_segments function just adds segments without constraint on already existing segments

pub fn initialize_segments(&mut self, program_base: Option<Relocatable>) {
    self.program_base = match program_base {
        Some(base) => Some(base),
        None => Some(self.vm.add_memory_segment()),
    };
    self.execution_base = Some(self.vm.add_memory_segment());

In this context, runner.initial_(f/a)p are properly targeting the right segment but this info is dropped during the initialization of the RunContext

pub fn initialize_vm(&mut self) -> Result<(), RunnerError> {
    self.vm.run_context.pc = *self.initial_pc.as_ref().ok_or(RunnerError::NoPC)?;
    self.vm.run_context.ap = self.initial_ap.as_ref().ok_or(RunnerError::NoAP)?.offset;
    self.vm.run_context.fp = self.initial_fp.as_ref().ok_or(RunnerError::NoFP)?.offset;

which actually has a hard-coded 1

impl RunContext {
    pub fn get_ap(&self) -> Relocatable {
        Relocatable::from((1, self.ap))
    }
    pub fn get_fp(&self) -> Relocatable {
        Relocatable::from((1, self.fp))
    }

All together, this makes the VM unable to run since ap and fp are pointing to the wrong segment.

Since there is no reason to enforce the execution segment to be 1, the easiest and most reasonable fix to me is to add a execution_base_segment_index to the RunContext and to update the get_ap and get_fp methods.

@ClementWalter ClementWalter added the bug Something isn't working label Jan 4, 2025
@gabrielbosio
Copy link
Collaborator

Hi @ClementWalter, we can document the initialization functions to clarify that they are meant to be called only once.

@ClementWalter
Copy link
Author

ClementWalter commented Jan 6, 2025

@gabrielbosio sure you can document, but I don't think enforcing execution_base = 1 is a correct behavior for the VM, see the reference python implementation https://github.com/starkware-libs/cairo-lang/blob/master/src/starkware/cairo/lang/vm/vm_core.py#L31-L40

ap and fp are relocatable and could point to any segment

@gabrielbosio
Copy link
Collaborator

gabrielbosio commented Jan 6, 2025

@ClementWalter While it's not explicit that in the VM execution_base == 1 must be true, if you try to generate a Cairo Pie using execution_base != 1, the validation will fail: https://github.com/starkware-libs/cairo-lang/blob/8e11b8cc65ae1d0959328b1b4a40b92df8b58595/src/starkware/cairo/lang/vm/cairo_pie.py#L68

@ClementWalter
Copy link
Author

it's true that Cairo PIE enforces the segments orders (0 and 1) but it's because the goal of PIEe is to concatenate runs. This is a special use case of the VM, not the spec. The VM itself doesn't care about segment orders. As a matter of fact, the reference implementation allowed program and execution to be anything, but enforced this at the PIE level. I don't see good reason to depart from this.

@gabrielbosio
Copy link
Collaborator

From an example of the Cairo 0 docs:

Observe that the value of the ap register is the relocatable value 1:4. Usually, segment 1 is the execution segment (recall that ap starts in the execution segment).

I'm curious about what's the context where execution_base segment is needed to be different than 1. Is there a specific case that needs this?

@ClementWalter
Copy link
Author

it's just not part of the spec. it's like if builtins segments were expected to have constant indexes. It would work, but it's not part of the spec. As a real world use case, it happened to me because we're pre filling the memory with a lot of data even before the runner starts. It should be an issue because the segments indexes don't matter. But now it's one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants