-
Notifications
You must be signed in to change notification settings - Fork 120
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
Retry Zcash sprout and sapling parameters download #3306
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3306 +/- ##
==========================================
- Coverage 77.30% 77.29% -0.02%
==========================================
Files 265 265
Lines 31347 31380 +33
==========================================
+ Hits 24234 24254 +20
- Misses 7113 7126 +13 |
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.
Logging the error is a blocker for merging this PR, because I've used it a lot to diagnose download errors.
Did you also want advice about simplifying the code?
These changes are optional:
Groth16Parameters::new
is getting really long. Can you split out the individual sapling and sprout downloads into separate methods?
If we do that change, we can simplify the loop to just handle the error case, because the success case doesn't need any extra code:
while let Err(error) = Groth16Parameters::download_sapling_parameters() {
sapling_retries += 1;
if sapling_retries >= PARAMETER_DOWNLOAD_MAX_RETRIES {
panic!(
"error downloading Sapling parameter files after {} retries. {:?} {}",
PARAMETER_DOWNLOAD_MAX_RETRIES,
error,
Groth16Parameters::failure_hint(),
);
} else {
tracing::info!(?error, "error downloading Zcash Sapling parameters, retrying");
}
}
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.
Looks good, but we should check that the paths are the ones we expect, otherwise loading might fail. (This is a workaround for the librustzcash
API, we should make it easier to use in future.)
See PR oxarbitrage#162 for a commit that adds the asserts back in.
Assert the parameters were downloaded to the expected paths
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.
Looks good, thanks!
Motivation
We want to retry a few times the Sprout and Sapling parameter download if they fail for whatever reason.
Fixes #3239
Solution
Retry the downloads if they fail. Number of retries is defined by a constant(currently = 3).
The code can be maybe improved a bit to make it shorter but i think it should work as it is too.
Review
Anyone can review, the code is pretty simple.
Reviewer Checklist