feat(starknet_os): add OsOutput and complete run_os function

[TzahiTaub]

No description provided.

[reviewable-StarkWare]

This change is 

[TzahiTaub]

• feat(starknet_os): add OsOutput and complete run_os function #5020 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[TzahiTaub]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)

---

crates/starknet_os/src/io/os_output.rs line 0 at r1 (raw file):
Based on the snos output file. There is no need for deserializing the output - the python side does it by itself.

[dorimedini-starkware]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @TzahiTaub)

---

crates/starknet_os/src/io/os_output.rs line at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Based on the snos output file. There is no need for deserializing the output - the python side does it by itself.

well, can you add a TODO to define a struct for it?
not useful now, but when this crate is used as a library for running the OS it will be necessary

---

crates/starknet_os/src/io/os_output.rs line 8 at r1 (raw file):

use cairo_vm::vm::runners::cairo_pie::CairoPie;
use cairo_vm::vm::vm_core::VirtualMachine;
use cairo_vm::Felt252;

same type, Felt252 is just a re-export

Suggestion:

use starknet_types_core::felt::Felt;

---

crates/starknet_os/src/io/os_output.rs line 36 at r1 (raw file):

    let builtin_end_ptrs = vm
        .get_return_values(n_builtins)
        .map_err(|err| StarknetOsError::VirtualMachineError(err.into()))?;

don't we have a Memory variant in our error enum? get_return_values returns a MemoryError error type

Code quote:

        .get_return_values(n_builtins)
        .map_err(|err| StarknetOsError::VirtualMachineError(err.into()))?;

---

crates/starknet_os/src/io/os_output.rs line 41 at r1 (raw file):

    let output_size = builtin_end_ptrs
        .iter()
        .find_map(|ptr| {

why find_map and not the first value?

Code quote:

    let output_size = builtin_end_ptrs
        .iter()
        .find_map(|ptr| {

---

crates/starknet_os/src/io/os_output.rs line 95 at r1 (raw file):

        .collect();

    raw_output

this way, you don't need explicit type hinting for the collect()

Suggestion:

    Ok(range_of_output
        .iter()
        .map(|x| match x {
            Some(cow) => match **cow {
                MaybeRelocatable::Int(val) => Ok(val),
                MaybeRelocatable::RelocatableValue(val) => {
                    Err(StarknetOsError::VirtualMachineError(
                        VirtualMachineError::ExpectedIntAtRange(Box::new(Some(val.into()))),
                    ))
                }
            },
            None => Err(StarknetOsError::VirtualMachineError(
                MemoryError::MissingMemoryCells(BuiltinName::output.into()).into(),
            )),
        })
        .collect())

---

crates/starknet_os/src/runner.rs line 77 at r1 (raw file):

    let os_output = get_run_output(&cairo_runner.vm)?;

    log::debug!("output: {}", serde_json::to_string_pretty(&os_output).unwrap());

won't this bloat our logs with random integers?

Code quote:

 log::debug!("output: {}", serde_json::to_string_pretty(&os_output).unwrap());

---

crates/starknet_os/src/runner.rs line 87 at r1 (raw file):

        .map_err(|e| StarknetOsError::VirtualMachineError(e.into()))?;

    // Parse the Cairo VM output

non blocking...

Suggestion:

// Parse the Cairo VM output.

[TzahiTaub]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/starknet_os/src/runner.rs line 77 at r1 (raw file):

Previously, dorimedini-starkware wrote…

won't this bloat our logs with random integers?

Right, this made sense when the os_output was a struct and not raw data.

---

crates/starknet_os/src/io/os_output.rs line 8 at r1 (raw file):

Previously, dorimedini-starkware wrote…

same type, Felt252 is just a re-export

Done.

---

crates/starknet_os/src/io/os_output.rs line 36 at r1 (raw file):

Previously, dorimedini-starkware wrote…

don't we have a Memory variant in our error enum? get_return_values returns a MemoryError error type

The cairo-vm VirtualMachineError has a Memory variant, and this is what we'll raise (this is what the into does from MemoryError to the VirtualMachineError - Memory variant.

---

crates/starknet_os/src/io/os_output.rs line 41 at r1 (raw file):

Previously, dorimedini-starkware wrote…

why find_map and not the first value?

I'm just not sure if the order of the builtins is some commitment or it can change, so I prefer to find it. They also did it in snos (if you ask because you compared) - but in a weird way - they read cell 0, and then assert the segment index of the Relocatable in cell 0, is the same as of the output_basefrom above.

---

crates/starknet_os/src/io/os_output.rs line 95 at r1 (raw file):

Previously, dorimedini-starkware wrote…

this way, you don't need explicit type hinting for the collect()

It's OK without the Ok.

[dorimedini-starkware]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)

---

crates/starknet_os/src/runner.rs line 77 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Right, this made sense when the os_output was a struct and not raw data.

maybe add a TODO to log the output once we structify it in rust