add save_debug_data to stwo_run_and_prove

[nitsan-starkware]

Type

• feature
• bugfix
• dev (no functional changes, no API changes)
• fmt (formatting, renaming)
• build
• docs
• testing

Description

Breaking changes?

• yes
• no

---

This change is 

[nitsan-starkware]

• add save_debug_data to stwo_run_and_prove #262 👈 (View in Graphite)
• remove retries from stwo_run_and_prove #261
• main

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

[nitsan-starkware]

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @Yael-Starkware)

---

.github/workflows/upload_artifacts_workflow.yml line 6 at r2 (raw file):

  push:
    branches:
      - nitsan/add_save_debug_data

Change back to main

[Yael-Starkware]

Reviewable status: 0 of 4 files reviewed, 8 unresolved discussions (waiting on @nitsan-starkware)

---

crates/stwo_run_and_prove/src/main.rs line 168 at r3 (raw file):

/// Runs the program and generates a proof for it, then saves the proof to the given path.
/// If `debug_data_dir` is provided, and there is a proving error or the `save_debug_data` flag is

if the save_debug_data is true, does it save it also in case of success?
is this a requirement from Gil/Yuval?

makes more sense to me that the directory should be provided only if save_debug_data=true

---

crates/stwo_run_and_prove/src/main.rs line 204 at r3 (raw file):

    if let Some(data_dir) = debug_data_dir
        && (result.is_err() || save_debug_data)

depending on your answer to the question above regarding requirements,
I think you should first check is_error && save_debug_data and then check if there is a directory.

Code quote:

    if let Some(data_dir) = debug_data_dir
        && (result.is_err() || save_debug_data)

---

crates/stwo_run_and_prove/src/main.rs line 213 at r3 (raw file):

            sonic_rs::to_string_pretty(&prover_input)?,
        )
        .map_err(|e| StwoRunAndProveError::from((e, data_dir)))?;

can't you forward the error with "?" ?

Code quote:

        .map_err(|e| StwoRunAndProveError::from((e, data_dir)))?;

---

crates/stwo_run_and_prove/src/main.rs line 477 at r3 (raw file):

    #[test]
    fn test_stwo_run_and_prove() {
        let (output_tempfile, proof_tempfile, _) = run_with_successful_mock_prover();

Suggestion:

let (output_tempfile, proof_tempfile) = run_with_successful_mock_prover();

---

crates/stwo_run_and_prove/src/main.rs line 529 at r3 (raw file):

            serde_json::from_slice(&expected_prover_input_content)
                .expect("Failed to parse expected prover input JSON");
        normalize_json(&mut expected_paover_input_json);

the 2 paragraphs are the same, please extract to a function.

Code quote:

        let prover_input_file = debug_data_tempdir.path().join(PROVER_INPUT_FILE_NAME);
        let prover_input_content =
            std::fs::read(prover_input_file).expect("Failed to read prover input file");
        let mut prover_input_json: serde_json::Value =
            serde_json::from_slice(&prover_input_content)
                .expect("Failed to parse prover input JSON");
        normalize_json(&mut prover_input_json);

        let expected_prover_input_file = get_path(EXPECTED_PROVER_INPUT_FILE_NAME);
        let expected_prover_input_content = std::fs::read(expected_prover_input_file)
            .expect("Failed to read expected prover input file");
        let mut expected_paover_input_json: Value =
            serde_json::from_slice(&expected_prover_input_content)
                .expect("Failed to parse expected prover input JSON");
        normalize_json(&mut expected_paover_input_json);

---

crates/stwo_run_and_prove/src/main.rs line 530 at r3 (raw file):

                .expect("Failed to parse expected prover input JSON");
        normalize_json(&mut expected_paover_input_json);

I think you can use assert_json_diff crate to compare the jsons without normalize
assert_json_diff::assert_json_matches_no_panic

---

crates/stwo_run_and_prove/src/main.rs line 532 at r3 (raw file):

        assert_eq!(
            prover_input_json, expected_paover_input_json,

Suggestion:

prover_input_json, expected_prover_input_json,

[nitsan-starkware]

Reviewable status: 0 of 4 files reviewed, 8 unresolved discussions (waiting on @Yael-Starkware)

---

crates/stwo_run_and_prove/src/main.rs line 168 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

if the save_debug_data is true, does it save it also in case of success?
is this a requirement from Gil/Yuval?

makes more sense to me that the directory should be provided only if save_debug_data=true

We have two cases of saving debug data -

1. Attempt_idx > 0. In this case we will send from the Python code save_debug_data=true.
2. An error in the current binary run, of one of the types that we can catch. In this case save_debug_data=false, but we till want to get a debug_data_dir to save the data to.

---

crates/stwo_run_and_prove/src/main.rs line 204 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

depending on your answer to the question above regarding requirements,
I think you should first check is_error && save_debug_data and then check if there is a directory.

Same answer :)

---

crates/stwo_run_and_prove/src/main.rs line 213 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

can't you forward the error with "?" ?

I want to add the dir path to the error. In this way it will use:

impl From<(std::io::Error, PathBuf)> for StwoRunAndProveError {
fn from((source, path): (std::io::Error, PathBuf)) -> Self {
StwoRunAndProveError::PathIO { path, source }
}
}

So we will get:
IO error on file '<data_dir>': No such file or directory

---

crates/stwo_run_and_prove/src/main.rs line 529 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

the 2 paragraphs are the same, please extract to a function.

Done.

---

crates/stwo_run_and_prove/src/main.rs line 530 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

I think you can use assert_json_diff crate to compare the jsons without normalize
assert_json_diff::assert_json_matches_no_panic

Great, thanks! (why no panic in tests?)

---

crates/stwo_run_and_prove/src/main.rs line 477 at r3 (raw file):

    #[test]
    fn test_stwo_run_and_prove() {
        let (output_tempfile, proof_tempfile, _) = run_with_successful_mock_prover();

How should it work?
run_with_successful_mock_prover returns (TempPath, TempPath, TempDir)

---

crates/stwo_run_and_prove/src/main.rs line 532 at r3 (raw file):

        assert_eq!(
            prover_input_json, expected_paover_input_json,

Done.

[nitsan-starkware]

Reviewable status: 0 of 6 files reviewed, 7 unresolved discussions (waiting on @Yael-Starkware)

---

crates/stwo_run_and_prove/src/main.rs line 530 at r3 (raw file):

Previously, nitsan-starkware (Nitsan Davidovich) wrote…

Great, thanks! (why no panic in tests?)

Oh actually it is not working because of the structure of the prover input, it's not just the order of the elements that different, but also the indexes.
With assert_json_diff the assert fails with errors like:
json atoms at path ".public_memory_addresses[29]" are not equal:
lhs:
5
rhs:
30
json atoms at path ".public_memory_addresses[4]" are not equal:
lhs:
62
rhs:
5
etc.

[Yael-Starkware]

@Yael-Starkware reviewed all commit messages.
Reviewable status: 0 of 6 files reviewed, 5 unresolved discussions (waiting on @nitsan-starkware)

---

crates/stwo_run_and_prove/src/main.rs line 213 at r3 (raw file):

Previously, nitsan-starkware (Nitsan Davidovich) wrote…

I want to add the dir path to the error. In this way it will use:

impl From<(std::io::Error, PathBuf)> for StwoRunAndProveError {
fn from((source, path): (std::io::Error, PathBuf)) -> Self {
StwoRunAndProveError::PathIO { path, source }
}
}

So we will get:
IO error on file '<data_dir>': No such file or directory

1. doesnt the std error print the path?

2. if not, can you do something like that?

#[derive(Debug, Error)] enum StwoRunAndProveError {
#[error("IO error on file '{1:?}': {0}")]
PathIO(std::io::Error, PathBuf),

.....
.map_err(|e| StwoRunAndProveError::PathIO(e, data_dir))?

---

crates/stwo_run_and_prove/src/main.rs line 477 at r3 (raw file):

Previously, nitsan-starkware (Nitsan Davidovich) wrote…

How should it work?
run_with_successful_mock_prover returns (TempPath, TempPath, TempDir)

remove it also from the function return value.

---

crates/stwo_run_and_prove/resources/expected_prover_input.json line 1 at r5 (raw file):

{

consider adding test_ prefix to the file name.

---

crates/stwo_run_and_prove/src/main.rs line 63 at r5 (raw file):

    #[clap(
        long = "save_debug_data",
        help = "Should save the CairoRunner and ProverInput to files."

Suggestion:

help = "Should save the CairoRunner and ProverInput to files `debug_data_dir` in for both success and failure."

[nitsan-starkware]

Reviewable status: 0 of 6 files reviewed, 5 unresolved discussions (waiting on @Yael-Starkware)

---

crates/stwo_run_and_prove/src/main.rs line 213 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

1. doesnt the std error print the path?

2. if not, can you do something like that?

#[derive(Debug, Error)] enum StwoRunAndProveError {
#[error("IO error on file '{1:?}': {0}")]
PathIO(std::io::Error, PathBuf),

.....
.map_err(|e| StwoRunAndProveError::PathIO(e, data_dir))?

Done, thanks!

---

crates/stwo_run_and_prove/src/main.rs line 477 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

remove it also from the function return value.

lol done

---

crates/stwo_run_and_prove/resources/expected_prover_input.json line 1 at r5 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

consider adding test_ prefix to the file name.

Do you think I should add it to all the files in the resources dir?

---

crates/stwo_run_and_prove/src/main.rs line 63 at r5 (raw file):

    #[clap(
        long = "save_debug_data",
        help = "Should save the CairoRunner and ProverInput to files."

Done.

[Yael-Starkware]

@Yael-Starkware reviewed 3 of 6 files at r4, 1 of 3 files at r5, 2 of 3 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @gilbens-starkware and @nitsan-starkware)

---

crates/stwo_run_and_prove/resources/expected_prover_input.json line 1 at r5 (raw file):

Previously, nitsan-starkware (Nitsan Davidovich) wrote…

Do you think I should add it to all the files in the resources dir?

are they all for testing?
if so, you can leave it as is.

---

.github/workflows/check.yml line 36 at r7 (raw file):

        with:
          submodules: "true"
      - run: cargo tree

please remove

Code quote:

 - run: cargo tree

---

crates/stwo_run_and_prove/src/main.rs line 61 at r7 (raw file):

    #[clap(
        long = "save_debug_data",
        help = "Should save the ProverInput to file in `debug_data_dir` for both success and failure."

Suggestion:

help = "Should save the ProverInput to a file in `debug_data_dir` for both success and failure."

---

crates/stwo_run_and_prove/src/main.rs line 102 at r7 (raw file):

    }
}

Yay!

---

crates/stwo_run_and_prove/src/main.rs line 432 at r7 (raw file):

                        let b = b.as_f64().unwrap();
                        a.partial_cmp(&b).unwrap_or(Ordering::Equal)
                    });

can you explain the sorting?
doesn't the order have a meaning for some of the arrays?
does the cairo vm run result in different prover_input? why? what is not deterministic there?

Code quote:

                    arr.sort_by(|a, b| {
                        let a = a.as_f64().unwrap();
                        let b = b.as_f64().unwrap();
                        a.partial_cmp(&b).unwrap_or(Ordering::Equal)
                    });

---

crates/stwo_run_and_prove/src/main.rs line 446 at r7 (raw file):

    /// Reads the JSON content from the given file path and normalizes it.
    fn get_json_content(file_path: &PathBuf, file_name: &str) -> serde_json::Value {

Suggestion:

get_json_normalized_content(

---

crates/stwo_run_and_prove/src/main.rs line 521 at r7 (raw file):

            it's absolutely necessary, and update all the places that call this binary accordingly."
        );
    }

I think it is safer to check the actual file name that stwo_run_and_prove stores, than checking the constant that may or may not be used.

you can add this check with the log msg to the previous test.

Code quote:

    #[test]
    fn test_debug_data_file_name() {
        // DO NOT CHANGE THIS VALUE UNLESS IT WAS CHANGED IN ALL THE PLACES THAT CALL THIS BINARY
        let expected_prover_input_file_name: &str = "prover_input.json";

        assert_eq!(
            PROVER_INPUT_FILE_NAME, expected_prover_input_file_name,
            "The PROVER_INPUT_FILE_NAME constant value has changed. This change will break all the
            places that call this binary. Such a change should not happen often, so please make sure
            it's absolutely necessary, and update all the places that call this binary accordingly."
        );
    }

---

Cargo.toml line 13 at r7 (raw file):

anyhow = "1.0.98"
bincode = { version = "2.0.1", features = ["serde"] }
cairo-air = { git = "https://github.com/starkware-libs/stwo-cairo", rev = "e2422a76e3837976cec08c76efdd12825422ac2b" }

please get an approval from @gilbens-starkware that this is a stable version we can use in dev

Code quote:

e2422a76e3837976cec08c76efdd12825422ac2b

[nitsan-starkware]

Reviewable status: 4 of 7 files reviewed, 8 unresolved discussions (waiting on @gilbens-starkware and @Yael-Starkware)

---

Cargo.toml line 13 at r7 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

please get an approval from @gilbens-starkware that this is a stable version we can use in dev

Done.

---

crates/stwo_run_and_prove/src/main.rs line 432 at r7 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

can you explain the sorting?
doesn't the order have a meaning for some of the arrays?
does the cairo vm run result in different prover_input? why? what is not deterministic there?

I didn’t dive into this resolution. I just noticed that the content is actually the same in the results of the two runs, but the indexes in public_memory_addresses are different. I’ll explore this and will update.

---

crates/stwo_run_and_prove/src/main.rs line 521 at r7 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

I think it is safer to check the actual file name that stwo_run_and_prove stores, than checking the constant that may or may not be used.

you can add this check with the log msg to the previous test.

Done.

---

.github/workflows/check.yml line 36 at r7 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

please remove

Done, thanks

---

crates/stwo_run_and_prove/resources/expected_prover_input.json line 1 at r5 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

are they all for testing?
if so, you can leave it as is.

Yes

---

crates/stwo_run_and_prove/src/main.rs line 61 at r7 (raw file):

    #[clap(
        long = "save_debug_data",
        help = "Should save the ProverInput to file in `debug_data_dir` for both success and failure."

Done.

---

crates/stwo_run_and_prove/src/main.rs line 446 at r7 (raw file):

    /// Reads the JSON content from the given file path and normalizes it.
    fn get_json_content(file_path: &PathBuf, file_name: &str) -> serde_json::Value {

Done.

[Yael-Starkware]

Reviewable status: 4 of 7 files reviewed, 6 unresolved discussions (waiting on @gilbens-starkware and @nitsan-starkware)

---

crates/stwo_run_and_prove/src/main.rs line 291 at r8 (raw file):

}

fn is_file_exists(path: &PathBuf) -> bool {

Suggestion:

fn file_exists(path: &PathBuf) -> bool {

---

crates/stwo_run_and_prove/src/main.rs line 500 at r8 (raw file):

            "Prover input file was not created in the debug data directory, or was created with an
            incorrect name, after running with a proving failure",
        );

Suggestion:

        assert!(
            is_file_exists(&debug_data_tempdir.path().join(PROVER_INPUT_FILE_NAME)),
            "Prover input file was not created in the debug data directory, or was created with an
            incorrect name, after running with a proving failure. NOTE: if the file name has changed, it may break external dependencies.",
        );

[nitsan-starkware]

Reviewable status: 4 of 7 files reviewed, 5 unresolved discussions (waiting on @gilbens-starkware and @Yael-Starkware)

---

crates/stwo_run_and_prove/src/main.rs line 291 at r8 (raw file):

}

fn is_file_exists(path: &PathBuf) -> bool {

Done.

[nitsan-starkware]

Reviewable status: 4 of 7 files reviewed, 5 unresolved discussions (waiting on @gilbens-starkware and @Yael-Starkware)

---

crates/stwo_run_and_prove/src/main.rs line 500 at r8 (raw file):

            "Prover input file was not created in the debug data directory, or was created with an
            incorrect name, after running with a proving failure",
        );

Done.

[nitsan-starkware]

Reviewable status: 4 of 7 files reviewed, 5 unresolved discussions (waiting on @gilbens-starkware and @Yael-Starkware)

---

crates/stwo_run_and_prove/src/main.rs line 432 at r7 (raw file):

Previously, nitsan-starkware (Nitsan Davidovich) wrote…

I didn’t dive into this resolution. I just noticed that the content is actually the same in the results of the two runs, but the indexes in public_memory_addresses are different. I’ll explore this and will update.

Updating that the public memory addresses order is not deterministic because the VM run returns it as Hash-map (I can explain this further f2f).
So the normalization is necessary, but I changed it to sort only the public memory array.

[Yael-Starkware]

@Yael-Starkware reviewed 2 of 3 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nitsan-starkware)

[nitsan-starkware]

Merge activity

• Dec 17, 2:27 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• Dec 17, 2:28 PM UTC: @nitsan-starkware merged this pull request with Graphite.