Skip to content

feat: specify initial value for exec_scopes#1772

Merged
juanbono merged 4 commits intolambdaclass:mainfrom
Moonsong-Labs:msl/init-vm-variables-in-cairo-run
May 28, 2024
Merged

feat: specify initial value for exec_scopes#1772
juanbono merged 4 commits intolambdaclass:mainfrom
Moonsong-Labs:msl/init-vm-variables-in-cairo-run

Conversation

@odesenfans
Copy link
Contributor

@odesenfans odesenfans commented May 23, 2024

Context: bootloader and Starknet OS support.

Added a cairo_run_program_with_initial_scope function that allows to run a program with a customized execution scope. cairo_run_program now calls this function with an empty execution scope.

We need a way to set initial variables inside the execution scopes to pass bootloader/OS arguments. The Python VM relies on a program input file and uses hints to deserialize this input. In most cases we do not want to have to deserialize these input types, but rather pass them directly as variables inside the root execution scope.

Possible alternatives:

  1. Pass init values inside CairoRunConfig.
  2. Support a more generic string argument for program input, at the expense of having to make all input structures deserializable. This has proven to be complex because some bootloader structures rely on Program and CairoPie, so I'd prefer not to go down that road.

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.

Copy link
Contributor

@fmoletta fmoletta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a very specific behaviour, I would rather have it added to the CairoRunConfig struct, so users can just ignore it (aka leave it to the Default impl to handle).

@odesenfans
Copy link
Contributor Author

This seems like a very specific behaviour, I would rather have it added to the CairoRunConfig struct, so users can just ignore it (aka leave it to the Default impl to handle).

One issue with that is that we get a &CairoRunConfig as argument so we can't move the exec_scopes field out, and ExecutionScopes is not Clone because of the Box<dyn Any> values inside it. A solution could be to pass an owned CairoRunConfig directly to cairo_run. There is no particular reason to keep that structure around after the run. CairoRunConfig is not Clone so this is not an issue, but it could be slightly annoying for existing code that reads values from CairoRunConfig after the call to cairo_run.

What do you think @fmoletta ?

@Oppen
Copy link
Contributor

Oppen commented May 23, 2024

It's not clear to me that you can't do this via a regular hint that you run as part of your program load process. Is there something getting in the way of doing that?

@odesenfans
Copy link
Contributor Author

It's not clear to me that you can't do this via a regular hint that you run as part of your program load process. Is there something getting in the way of doing that?

@Oppen yes. You need to load these initial variables from somewhere. You could say "let's load it from the file system" but a) that involves some useless serialization/deserialization which is not that easy with types like Program and CairoPie, b) without other inputs the hint needs to use a hardcoded path, which is not ideal and c) I feel like the ability to build your inputs programmatically and pass them as needed is not that unreasonable a request for the OS and bootloader.

Anecdotally, this feature exists for the Python VM (see here).

@odesenfans odesenfans force-pushed the msl/init-vm-variables-in-cairo-run branch from 33a55b8 to feb7b24 Compare May 24, 2024 12:11
@fmoletta
Copy link
Contributor

fmoletta commented May 28, 2024

This seems like a very specific behaviour, I would rather have it added to the CairoRunConfig struct, so users can just ignore it (aka leave it to the Default impl to handle).

One issue with that is that we get a &CairoRunConfig as argument so we can't move the exec_scopes field out, and ExecutionScopes is not Clone because of the Box<dyn Any> values inside it. A solution could be to pass an owned CairoRunConfig directly to cairo_run. There is no particular reason to keep that structure around after the run. CairoRunConfig is not Clone so this is not an issue, but it could be slightly annoying for existing code that reads values from CairoRunConfig after the call to cairo_run.

What do you think @fmoletta ?

Oh, then we could go with the current solution and rename cairo_run_program to cairo_run_program_with_initial_scope and add a cairo_run_program that calls it so we can keep the cairo_run_program signature as it is now

Context: bootloader and Starknet OS support.

Added an `exec_scopes` parameter to `cairo_run_program` to pass
variables to the hints of the program being run.

We need a way to set initial variables inside the execution scopes to
pass bootloader/OS arguments. The Python VM relies on a program input file
and uses hints to deserialize this input. In most cases we do not want
to have to deserialize these input types, but rather pass them directly
as variables inside the root execution scope.

We did not extend this feature to `cairo_run` to keep the PR minimal for
discussion, TBD if it is desirable.
@odesenfans odesenfans force-pushed the msl/init-vm-variables-in-cairo-run branch from feb7b24 to 45e929e Compare May 28, 2024 15:22
@odesenfans
Copy link
Contributor Author

Oh, then we could go with the current solution and rename cairo_run_program to cairo_run_program_with_initial_scope and add a cairo_run_program that calls it so we can keep the cairo_run_program signature as it is now

Done: 45e929e

@juanbono juanbono enabled auto-merge May 28, 2024 15:56
@juanbono juanbono added this pull request to the merge queue May 28, 2024
Merged via the queue into lambdaclass:main with commit 05352b1 May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants