Add `scarb prove` #1900

DelevoXDG · 2025-01-18T05:05:01Z

Add scarb execute extension

Change output path from `scarb-execute/package_N` to `scarb-execute/package/executionN`

…n logic

- Add `UiPrinter` - Cannot use `Ui` directly due to circular dependency

DelevoXDG · 2025-01-21T12:33:22Z

extensions/scarb-prove/src/main.rs

+    /// Execute the program before proving.
+    #[arg(long, conflicts_with_all = ["execution_id", "pub_input_file", "priv_input_file"])]
+    execute: bool,


Do we want to have the execute mode on by default?

And if not, do we want to change the error message to default to --execute or --execution mode when no args are provided?

Currently, it's

error: the following required arguments were not provided: --pub-input-file <PUB_INPUT_FILE> --priv-input-file <PRIV_INPUT_FILE> Usage: scarb-prove --verbosity <VERBOSITY> --pub-input-file <PUB_INPUT_FILE> --priv-input-file <PRIV_INPUT_FILE> For more information, try '--help'.

Was there any discussion about this topic?
My understanding is that you technically could want to just prove some previously executed code, but am not sure about implication of this (how do you make sure the code you're proving is THE code?).

DelevoXDG · 2025-01-21T12:42:45Z

extensions/scarb-prove/src/main.rs

+    let output = cmd.output_and_stream(&printer)?;
+    extract_execution_id(&output)


I know this is not ideal, but the original approach discussed with use of env::set_var seems problematic to me due to this function now being unsafe: rust-lang/rust#124636

my concern is: how stable is that? can't we get it any other way? it seems to me that something like extracting execution id could be done better than parsing the scarb execute output 🤔

DelevoXDG · 2025-01-21T12:46:33Z

extensions/scarb-prove/src/main.rs

+#[derive(Parser, Clone, Debug)]
+struct InputFileArgs {
+    /// AIR public input path.
+    #[arg(long, required_unless_present_any = ["execution_id", "execute"], conflicts_with_all = ["execution_id", "execute"])]
+    pub_input_file: Option<Utf8PathBuf>,
+
+    /// AIR private input path.
+    #[arg(long, required_unless_present_any = ["execution_id", "execute"], conflicts_with_all = ["execution_id", "execute"])]
+    priv_input_file: Option<Utf8PathBuf>,
+}


We initially discussed whether such approach should be available, but I still wonder if we should support package-agnostic files in Scarb, as it creates a lot of noise in a sense of args available for a given subcommand.

any additional context on that? 🙏

DelevoXDG · 2025-01-21T12:52:04Z

extensions/scarb-prove/src/main.rs

+#[derive(Parser, Clone, Debug)]
+struct ExecuteArgs {
+    /// Do not build the package before execution.
+    #[arg(long, requires = "execute")]
+    no_build: bool,
+
+    /// Arguments to pass to the program during execution.
+    #[arg(long, requires = "execute")]
+    arguments: Option<String>,
+
+    /// Arguments to the executable function from a file.
+    #[arg(long, conflicts_with = "arguments")]
+    arguments_file: Option<String>,
+
+    /// Target for execution.
+    #[arg(long, requires = "execute")]
+    target: Option<String>,
+}


--execute flag was requested initially for convenience, but I wonder whether we should really have it, as we it forces us to populate scarb prove with bunch of arguments related to scarb execute (and one more is coming when stwo prover would support cairo-pie).

Maybe it's better to output a warning, similarly to how scarb execute does it?

package has not been compiled, file does not exist: `{filename}` help: run `scarb build` to compile the package

This way all tricky logic related to extracting scarb execute output would be eliminated.

Maybe not necessarily something we want to address as a part of this PR, but something we should possibly discuss 🤔

I'm not sure I follow, sorry. Can you give me an example of problematic things related to this?
I guess that if we want to --execute we should probably allow to set all the possible arguments, right? and having an option to just --execute seems cool enough, but again - maybe I'm just missing something

THenry14 · 2025-01-22T13:53:10Z

.github/workflows/ci.yml

+            -p scarb-ui \
+            -p test-for-each-example \
+            -p xtask
+      - run: cargo clippy --all-targets --all-features --workspace --exclude scarb-prove -- --no-deps


let's create an issue to remove all this workarounds after we're able to use rust stable for all packages and add a todo in here

THenry14 · 2025-01-22T13:54:04Z

Cargo.toml

@@ -137,7 +138,7 @@ tar = "0.4"
 target-triple = "0.1"
 tempfile = "3"
 test-case = "3"
-thiserror = "2"
+thiserror = "2.0.10"


any specific reason to pin that?

THenry14 · 2025-01-22T14:18:36Z

extensions/scarb-prove/src/main.rs

+    /// Execute the program before proving.
+    #[arg(long, conflicts_with_all = ["execution_id", "pub_input_file", "priv_input_file"])]
+    execute: bool,


Was there any discussion about this topic?
My understanding is that you technically could want to just prove some previously executed code, but am not sure about implication of this (how do you make sure the code you're proving is THE code?).

THenry14 · 2025-01-22T14:24:03Z

extensions/scarb-prove/src/main.rs

+    /// Target for execution.
+    #[arg(long, requires = "execute")]
+    target: Option<String>,


shouldn't this be ExecutionTarget ?

THenry14 · 2025-01-22T14:27:23Z

extensions/scarb-prove/src/main.rs

+    /// Arguments to pass to the program during execution.
+    #[arg(long, requires = "execute")]
+    arguments: Option<String>,
+
+    /// Arguments to the executable function from a file.
+    #[arg(long, conflicts_with = "arguments")]
+    arguments_file: Option<String>,


Why not ProgramArguments instead?

THenry14 · 2025-01-22T14:30:14Z

extensions/scarb-prove/src/main.rs

+#[derive(Parser, Clone, Debug)]
+struct ExecuteArgs {
+    /// Do not build the package before execution.
+    #[arg(long, requires = "execute")]
+    no_build: bool,
+
+    /// Arguments to pass to the program during execution.
+    #[arg(long, requires = "execute")]
+    arguments: Option<String>,
+
+    /// Arguments to the executable function from a file.
+    #[arg(long, conflicts_with = "arguments")]
+    arguments_file: Option<String>,
+
+    /// Target for execution.
+    #[arg(long, requires = "execute")]
+    target: Option<String>,
+}


I'm not sure I follow, sorry. Can you give me an example of problematic things related to this?
I guess that if we want to --execute we should probably allow to set all the possible arguments, right? and having an option to just --execute seems cool enough, but again - maybe I'm just missing something

THenry14 · 2025-01-22T14:57:55Z

extensions/scarb-prove/src/main.rs

+#[derive(Parser, Clone, Debug)]
+struct InputFileArgs {
+    /// AIR public input path.
+    #[arg(long, required_unless_present_any = ["execution_id", "execute"], conflicts_with_all = ["execution_id", "execute"])]
+    pub_input_file: Option<Utf8PathBuf>,
+
+    /// AIR private input path.
+    #[arg(long, required_unless_present_any = ["execution_id", "execute"], conflicts_with_all = ["execution_id", "execute"])]
+    priv_input_file: Option<Utf8PathBuf>,
+}


any additional context on that? 🙏

THenry14 · 2025-01-22T15:02:48Z

extensions/scarb-prove/src/main.rs

+    let output = cmd.output_and_stream(&printer)?;
+    extract_execution_id(&output)


my concern is: how stable is that? can't we get it any other way? it seems to me that something like extracting execution id could be done better than parsing the scarb execute output 🤔

FroyaTheHen and others added 22 commits January 15, 2025 08:53

feat: executable command

c658194

Add scarb execute extension

Initial cairo-prove

af5414d

Support prover config; Restructure args

ccc3aa3

Restructure cairo-execute output directories

972acdd

Change output path from `scarb-execute/package_N` to `scarb-execute/package/executionN`

Refactor; Improve error messages; Improve args

6036329

Add tests

0a404e4

Refactor

55525ed

Format

0da153d

Add --execute flag to run execution before prove

cb4b7fb

Refactor: add helper functions; Simplify execution_number extractio…

3946387

…n logic

Improve error messages in line with scarb execute changes

951e3d7

use lossy utf8 conversion

ff83e91

misc: rename test

3c31623

rename all "execution number" mentions to execution_id

5bb3424

Add verbosity settings

01070f1

fix tests (--execution-id)

e02f52d

Disable scarb execute output for quiet verbosity

e82596a

Stream AND write output of subcommand on the fly

39f1f70

Format

de0d76d

improve approach to streaming stdout of nested commands

72e223b

- Add `UiPrinter` - Cannot use `Ui` directly due to circular dependency

refactor: rename scarb-cairo-{execute,prove} to scarb-{execute,prove}

2ab30cd

finish scarb-cairo-{execute,prove} -> scarb-{execute,prove}`

e6556de

DelevoXDG changed the base branch from maicektr/executable to main January 18, 2025 05:05

DelevoXDG added 3 commits January 18, 2025 06:20

Merge branch 'main' into zdobnikau/stwo-prove

c7e0dc6

format

242e179

lock

48a1399

DelevoXDG changed the title ~~Zdobnikau/stwo prove~~ Add scarb prove Jan 18, 2025

DelevoXDG added 3 commits January 20, 2025 23:40

update ci

375195c

fix scarb execute tests

a9e7e28

--exclude not supported by rustfmt

d19a6e1

DelevoXDG added 11 commits January 21, 2025 12:06

install components in check-rust-scarb-prove

03593e3

Fix build in test-scarb-prove

e9e2a15

fmt

60de69f

fix check-rust

e42b960

fix test-scarb-prove

eea251e

fmt

21a899c

remove config.toml

a1e1fe2

fix clippy ci

fa3ddf4

fix tests

6936618

fix ci

a3ce3a4

support arguments file

d783d03

DelevoXDG commented Jan 21, 2025

View reviewed changes

DelevoXDG requested a review from THenry14 January 21, 2025 12:58

DelevoXDG marked this pull request as ready for review January 21, 2025 13:00

DelevoXDG requested a review from a team as a code owner January 21, 2025 13:00

DelevoXDG added 3 commits January 21, 2025 15:00

fmt

57839d4

fmt

1eb0f34

update extract_execution_id

4218467

THenry14 reviewed Jan 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `scarb prove` #1900

Add `scarb prove` #1900

DelevoXDG commented Jan 18, 2025 •

edited

Loading

DelevoXDG Jan 21, 2025

DelevoXDG Jan 21, 2025 •

edited

Loading

THenry14 Jan 22, 2025

DelevoXDG Jan 21, 2025

THenry14 Jan 22, 2025

DelevoXDG Jan 21, 2025

THenry14 Jan 22, 2025

DelevoXDG Jan 21, 2025

THenry14 Jan 22, 2025

THenry14 Jan 22, 2025

THenry14 Jan 22, 2025

THenry14 Jan 22, 2025

THenry14 Jan 22, 2025

THenry14 Jan 22, 2025

THenry14 Jan 22, 2025

THenry14 Jan 22, 2025

THenry14 Jan 22, 2025

		let output = cmd.output_and_stream(&printer)?;
		extract_execution_id(&output)

Add scarb prove #1900

Are you sure you want to change the base?

Add scarb prove #1900

Conversation

DelevoXDG commented Jan 18, 2025 • edited Loading

Choose a reason for hiding this comment

DelevoXDG Jan 21, 2025 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `scarb prove` #1900

Add `scarb prove` #1900

DelevoXDG commented Jan 18, 2025 •

edited

Loading

DelevoXDG Jan 21, 2025 •

edited

Loading