-
Notifications
You must be signed in to change notification settings - Fork 4
remove retries from stwo_run_and_prove #261
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
remove retries from stwo_run_and_prove #261
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Yael-Starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @nitsan-starkware)
crates/stwo_run_and_prove/src/main.rs line 211 at r1 (raw file):
Err(e) => { error!("Proving failed with error: {}", e);
how do you distinct between error in the proof creation and error in the verification?
maybe printing which one happened.
crates/stwo_run_and_prove/src/main.rs line 411 at r1 (raw file):
#[test] fn test_stwo_run_and_prove_proving_failure() { let (output_tempfile, proof_tempfile) = run_with_failed_mock_prover();
why did you remove testing a run with verification failure?
I think there should be one case/test for failure of the prover and one case/test for failure of the verification, no?
7f4c63b to
773400a
Compare
nitsan-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @Yael-Starkware)
crates/stwo_run_and_prove/src/main.rs line 211 at r1 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
how do you distinct between error in the proof creation and error in the verification?
maybe printing which one happened.
Done (I’ll check with the Stwo team if we can deduce the error type from the existence of the file).
crates/stwo_run_and_prove/src/main.rs line 411 at r1 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
why did you remove testing a run with verification failure?
I think there should be one case/test for failure of the prover and one case/test for failure of the verification, no?
As we discussed f2f, there’s no point, because we handle both cases the same way except for the log message.
Yael-Starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Yael-Starkware reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nitsan-starkware)
crates/stwo_run_and_prove/src/main.rs line 217 at r2 (raw file):
", proof was created." }; error!("Proving failed with error: {e}{proof_msg}");
Suggestion:
let proof_msg = if is_file_missing_or_empty(&prove_config.proof_path)? {
error!("Proving failed with error {e}");
} else {
error!("Proof verification failed with error {r}, The failed proof was writen into the proof output file.);
};773400a to
9036bfc
Compare
nitsan-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @Yael-Starkware)
crates/stwo_run_and_prove/src/main.rs line 217 at r2 (raw file):
", proof was created." }; error!("Proving failed with error: {e}{proof_msg}");
Done.
Yael-Starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Yael-Starkware reviewed 1 of 1 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @nitsan-starkware)
Merge activity
|
9036bfc to
2e1aa88
Compare

Type
Description
Breaking changes?
This change is