Skip to content
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

Download sprout parameters in zcash_proofs #459

Merged
merged 29 commits into from
Aug 4, 2022

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Nov 22, 2021

Motivation

Zebra wants to use zcash_proofs to download both Sprout and Sapling parameters.

Changes

Add sprout:

  • Add a download_sprout_parameters function.
  • Stream sprout and sapling downloads to disk, rather than loading the whole file into RAM.

Functionality changes:

  • If the parameter files already exist, check them instead of starting a download.
  • Allow the caller to specify a download timeout.
  • Check file sizes as well as file hashes (useful for debugging).
  • Remove downloaded files on error (but leave existing files alone) .

Interface changes:

  • Add download_sapling_parameters and deprecate download_parameters. This avoids confusion between sprout and sapling downloads, while maintaining backward compatibility.
  • Make the Sprout and Sapling parameter file names public. This makes it easier for callers to supply the correct paths to load_parameters.

Bug fixes:

  • Add a missing feature dependency: download_params -> directories. This makes sure download_parameters will work if it's the only feature specified.

Tweaks:

  • Refactor file loads to use the same verifying function as downloads.

@teor2345 teor2345 marked this pull request as ready for review November 23, 2021 22:34
zcash_proofs/src/lib.rs Outdated Show resolved Hide resolved
@teor2345 teor2345 changed the title Download sprout parameters Download sprout parameters in zcash_proofs Nov 24, 2021
@teor2345 teor2345 marked this pull request as draft November 24, 2021 04:52
@teor2345

This comment was marked as resolved.

Copy link

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@teor2345 asked me to take a look. It seems good.

I think it would improve usability to delete downloaded files on errors, so that the user won't need to manually delete them. But it's not a big deal.

@teor2345
Copy link
Contributor Author

I think it would improve usability to delete downloaded files on errors, so that the user won't need to manually delete them. But it's not a big deal.

Sounds like a good idea, I'll see if I can make it happen today.

I was concerned we could accidentally delete files we haven't created. But now that we're handling existing and downloaded files separately, we can just do the deletes for downloaded files.

@teor2345 teor2345 marked this pull request as ready for review November 25, 2021 01:01
@teor2345
Copy link
Contributor Author

Chaining the Sprout requests is failing, probably because the second connection times out.

This is fixed now.

Tested using:

RUSTFLAGS="--cfg always_download" cargo run --release --features=download-params --example download-sprout-and-sapling-params

@teor2345
Copy link
Contributor Author

I also checked the code with clippy using:

cargo clippy --all-features --no-deps --lib --examples

@nuttycom nuttycom changed the base branch from non-consensus-changes-on-branchid-37519621 to master April 14, 2022 15:54
@teor2345 teor2345 force-pushed the download-sprout-params branch from ec79b5d to 6d75718 Compare May 11, 2022 23:08
@teor2345
Copy link
Contributor Author

@nuttycom @str4d I rebased on master, fixed a clippy warning, and updated this PR.

Copy link
Contributor

@defuse defuse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK with comments/questions

zcash_proofs/src/lib.rs Show resolved Hide resolved
zcash_proofs/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

zcash_proofs/src/downloadreader.rs Outdated Show resolved Hide resolved
zcash_proofs/src/lib.rs Show resolved Hide resolved
zcash_proofs/src/lib.rs Outdated Show resolved Hide resolved
zcash_proofs/src/lib.rs Outdated Show resolved Hide resolved
@teor2345
Copy link
Contributor Author

Thanks for the review, everything should be rebased and fixed now.

I ran cargo test locally, and everything passed.

@teor2345
Copy link
Contributor Author

I forgot to test with and without features, I pushed some minor fixes.

@codecov
Copy link

codecov bot commented Jul 29, 2022

Codecov Report

Merging #459 (c0f20b3) into main (8101b1a) will decrease coverage by 0.39%.
The diff coverage is 26.00%.

@@            Coverage Diff             @@
##             main     #459      +/-   ##
==========================================
- Coverage   51.51%   51.11%   -0.40%     
==========================================
  Files          91       94       +3     
  Lines        8500     8897     +397     
==========================================
+ Hits         4379     4548     +169     
- Misses       4121     4349     +228     
Impacted Files Coverage Δ
zcash_proofs/src/lib.rs 23.33% <21.73%> (-11.45%) ⬇️
zcash_proofs/src/hashreader.rs 43.75% <75.00%> (-2.41%) ⬇️
zcash_client_backend/src/keys.rs 54.70% <0.00%> (-16.73%) ⬇️
zcash_primitives/src/sapling/note_encryption.rs 64.08% <0.00%> (-9.36%) ⬇️
zcash_client_sqlite/src/chain.rs 89.44% <0.00%> (-4.58%) ⬇️
components/zcash_address/src/encoding.rs 91.37% <0.00%> (-3.45%) ⬇️
zcash_client_sqlite/src/wallet/transact.rs 87.44% <0.00%> (-3.22%) ⬇️
zcash_client_backend/src/data_api/wallet.rs 18.06% <0.00%> (-3.17%) ⬇️
zcash_history/src/tree.rs 55.94% <0.00%> (-3.09%) ⬇️
... and 57 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8101b1a...c0f20b3. Read the comment docs.

daira
daira previously requested changes Jul 31, 2022
Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of constants that are not always compiled-in is blocking.

Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants