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 Zcash Sapling parameters and load them from cached files #3057

Merged
merged 25 commits into from
Nov 19, 2021

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Nov 15, 2021

Motivation

We want to avoid linking half a gigabyte of Zcash parameters into the zebrad executable, tests, and build products.

This might also save us some redundant copies of the parameters crates, if they are duplicated for dev and release builds.

This is the Sapling refactor part of ticket #3037.

Solution

Zebra:

  • Load Sapling parameters from the downloaded files, rather than crates
  • When Zebra is initialised, spawn a task to download parameters
  • Mention zebrad download and the cached parameters directory in download error messages

CI:

  • Download Zcash parameters in Docker using a new zebrad download command
  • Download and cache Zcash parameters in GitHub CI using zebra-consensus examples, like zcash_proofs does

TODO in the next PR:

  • Implement Sprout parameter downloads in zcash_proofs
  • Load Sprout parameters from the downloaded files (already implemented in zcash_proofs, just needs glue)

Review

@jvff is reviewing this PR.

Reviewer Checklist

  • Code implements Specs and Designs
  • Existing tests pass
  • CI changes pass reliably

Follow Up Work

@teor2345 teor2345 added A-dependencies Area: Dependency file updates NU-1 Sapling Network Upgrade: Sapling specific tasks NU Sprout Network Upgrade: Sprout specific tasks (before Overwinter) A-devops Area: Pipelines, CI/CD and Dockerfiles C-enhancement Category: This is an improvement labels Nov 15, 2021
@teor2345 teor2345 requested review from jvff and oxarbitrage November 15, 2021 03:01
@teor2345 teor2345 self-assigned this Nov 15, 2021
@str4d
Copy link
Contributor

str4d commented Nov 15, 2021

ICYMI, we have a download_params function in the zcash_proofs crate that emulates fetch-params.sh. Limitations:

  • It currently only fetches Sapling parameters (as that is all our Rust crates supported making transactions with), not Sprout (as they were too large to just fetch inline all the time).
  • It doesn't include the IPFS fetching logic (pulling from there instead of HTTPS if ipfs is installed locally).
  • It hardcodes minreq as the fetcher (as we just needed a simple self-contained HTTP stack), whereas good probably want an async-compatible function that can use whatever HTTP stack you're already using elsewhere.

I'd be interested in collaborating on extending that Rust function with additional functionality, or building out a separate dependency for it.

jvff
jvff previously approved these changes Nov 15, 2021
@teor2345
Copy link
Contributor Author

teor2345 commented Nov 15, 2021

ICYMI, we have a download_params function in the zcash_proofs crate that emulates fetch-params.sh. Limitations:

Thanks, I had missed download_parameters:
https://github.com/zcash/librustzcash/blob/85780f994d51abb965cbf4b981f8730ff0554607/zcash_proofs/src/lib.rs#L68

It looks like our code is an uncredited copy of that method 😞

  • It currently only fetches Sapling parameters (as that is all our Rust crates supported making transactions with), not Sprout (as they were too large to just fetch inline all the time).

We need to fetch Sprout to close ticket #3037, so I'm happy to submit a patch with those changes.

  • It doesn't include the IPFS fetching logic (pulling from there instead of HTTPS if ipfs is installed locally).

IPFS is out of scope for Zebra for NU5 mainnet activation.

  • It hardcodes minreq as the fetcher (as we just needed a simple self-contained HTTP stack), whereas good probably want an async-compatible function that can use whatever HTTP stack you're already using elsewhere.

We're trying to minimise our scope before Zebra for NU5 mainnet activation. So I don't really want to rewrite working code.

We can just spawn a blocking async task for these downloads, and the executor will put it on its own thread. Then we can open a ticket for an async rewrite that uses reqwest.

(We haven't explicitly picked a HTTP stack yet. But some of Zebra's dependencies use reqwest.)

I'd be interested in collaborating on extending that Rust function with additional functionality, or building out a separate dependency for it.

We don't really have time to split dependencies right now. But I'd be happy to work on a Sprout download.

@teor2345 teor2345 changed the title Load Zcash Sapling & Sprout parameters from files Download Zcash Sapling & Sprout parameters, and load them from files Nov 16, 2021
@teor2345 teor2345 force-pushed the download-zcash-params branch from 4a1da00 to 5e3bb5e Compare November 17, 2021 00:20
@teor2345 teor2345 changed the title Download Zcash Sapling & Sprout parameters, and load them from files Download Zcash Sapling parameters, and load them from files Nov 17, 2021
@teor2345 teor2345 changed the title Download Zcash Sapling parameters, and load them from files Download Zcash Sapling parameters, and load them from cached files Nov 17, 2021
@teor2345 teor2345 marked this pull request as ready for review November 17, 2021 00:30
@teor2345 teor2345 changed the title Download Zcash Sapling parameters, and load them from cached files Download Zcash Sapling parameters and load them from cached files Nov 17, 2021
@teor2345 teor2345 removed the request for review from oxarbitrage November 17, 2021 00:31
@teor2345 teor2345 force-pushed the download-zcash-params branch 2 times, most recently from 54c60fa to b077e70 Compare November 17, 2021 03:56
@teor2345 teor2345 requested a review from jvff November 17, 2021 04:02
@teor2345
Copy link
Contributor Author

it seems like building Zebra is too slow for the time limits on these tests.

That's weird, because all the built crates should just get re-used. Maybe there is some minor difference between cargo run and cargo test that's causing a rebuilt.

I might have to make a "test" that just downloads the parameter files. That should be much smaller than all of zebrad. And it should have exactly the same build settings.

@str4d
Copy link
Contributor

str4d commented Nov 17, 2021

You might find this logic interesting, which we use in our CI to cache the parameters in GH Actions workers: https://github.com/zcash/librustzcash/blob/master/.github/workflows/ci.yml#L20-L33

@teor2345 teor2345 force-pushed the download-zcash-params branch from 2063ece to 19bd11c Compare November 18, 2021 02:30
@teor2345
Copy link
Contributor Author

You might find this logic interesting, which we use in our CI to cache the parameters in GH Actions workers: https://github.com/zcash/librustzcash/blob/master/.github/workflows/ci.yml#L20-L33

Thanks, this was really helpful, I've used similar logic in our CI.

jvff
jvff previously approved these changes Nov 18, 2021
Copy link
Contributor

@jvff jvff left a comment

Choose a reason for hiding this comment

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

Looks good, I just added some optional suggestions.

This command isn't required for nomrmal usage.
But it's useful when testing, or launching multiple Zebra instances.
This avoids re-compining Zebra with and without coverage.
```sh
fastmod SaplingParams SaplingParameters zebra*
fastmod Groth16Params Groth16Parameters zebra*
fastmod PARAMS GROTH16_PARAMETERS zebra*
fastmod params_folder directory zebra*
```

And a manual variable name tweak.
@teor2345 teor2345 force-pushed the download-zcash-params branch from 3ec1bff to e3f4bda Compare November 19, 2021 04:15
Copy link
Contributor

@jvff jvff left a comment

Choose a reason for hiding this comment

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

Looks great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Area: Dependency file updates A-devops Area: Pipelines, CI/CD and Dockerfiles C-enhancement Category: This is an improvement NU Sprout Network Upgrade: Sprout specific tasks (before Overwinter) NU-1 Sapling Network Upgrade: Sapling specific tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants