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

Remove the sapling params from the published Rust crate, and download them from GitHub at build time #3951

Merged
merged 1 commit into from
Jun 8, 2023
Merged

Remove the sapling params from the published Rust crate, and download them from GitHub at build time #3951

merged 1 commit into from
Jun 8, 2023

Conversation

andreacorbellini
Copy link
Contributor

Summary

crates.io currently does not allow crates bigger than 10 MB. Our ironfish crate is currently almost 90 MB due to the fact that the sapling params are shipped together with the crate. Due to this limitation, it is not possible to publish our ironfish crate with the sapling params included in it. This PR reduces the crate size from 90 MB to 48 KB.

This PR makes the sapling params optional, and excludes them from the Rust crate to be published on crates.io (or other Cargo registries). When the sapling params are not present, they are fetched from GitHub, more specifically from the master branch of the iron-fish/ironfish repository. The integrity of sapling param files is checked using their SHA-512 hash.

The build experience for people using the Git repository does not change (no files are downloaded).

The runtime experience is the same for people using the Git repository or using the Cargo crate (the resulting binary is the same).

Alternatives considered

  • Asking the crates.io admins for an exception: search shows that admins are willing to lift the limit by a tiny increment (e.g. 10 MB -> 15 MB), but 90 MB is way too much.
  • Fetching the param files at runtime. This has been briefly discussed on Slack, but it would be more complex and raise more eyebrows than fetching the files at build time.

Future improvements

Fetch files from *.ironfish.network instead of GitHub.

Testing Plan

  • Trying to build the crate from the Git repository should not download any file:

    $ rm -r target/*
    $ cd ironfish-rust && cargo build
    ...
        Finished dev [unoptimized + debuginfo] target(s) in 25.84s
    

    (No warnings displayed)

  • Trying to build the crate without the sapling params should result in the files to be downloaded:

    $ rm -r target/*
    $ rm ironfish-rust/src/sapling_params/*.params
    $ cargo build -p ironfish
    warning: fetching sapling-mint.params from GitHub
    warning: fetching sapling-output.params from GitHub
    warning: fetching sapling-spend.params from GitHub
        Finished dev [unoptimized + debuginfo] target(s) in 28.92s
    

    (Warnings displayed)

  • Multiple builds should not re-run the build script:

    $ rm -r target/*
    $ rm ironfish-rust/src/sapling_params/*.params
    $ cargo build -p ironfish
    warning: fetching sapling-mint.params from GitHub
    warning: fetching sapling-output.params from GitHub
    warning: fetching sapling-spend.params from GitHub
        Finished dev [unoptimized + debuginfo] target(s) in 28.92s
    $ cargo build -p ironfish
    warning: fetching sapling-mint.params from GitHub
    warning: fetching sapling-output.params from GitHub
    warning: fetching sapling-spend.params from GitHub
        Finished dev [unoptimized + debuginfo] target(s) in 0.07s
    

    (Second run did not download anything despite the warnings, and was much faster)

  • Changing the checksum fail should make the build fail:

    $ vim ironfish-rust/src/sapling_params/params-sha512.txt # change some hash
    $ cargo build -p ironfish
    error: failed to run custom build command for `ironfish v0.1.0 (/home/andrea/src/ironfish/ironfish-rust)`
    
    Caused by:
      process didn't exit successfully: `/home/andrea/src/ironfish/target/debug/build/ironfish-db13dc5a60ba0e76/build-script-build` (exit status: 101)
      --- stdout
      cargo:rerun-if-changed=src/sapling_params
    
      --- stderr
      thread 'main' panicked at 'integrity verification failed for /home/andrea/src/ironfish/target/debug/build/ironfish-bf04b7c94271cc22/out/sapling_params/sapling-spend.params', ironfish-rust/build.rs:84:13
      note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
    
  • Checking the crate package size:

    $ cargo publish -p ironfish --dry-run
    ...
    $ du -h target/package/ironfish-0.1.0.crate 
    48K	target/package/ironfish-0.1.0.crate
    

Documentation

No

Breaking Change

Not a breaking change

@andreacorbellini andreacorbellini requested a review from a team as a code owner June 6, 2023 23:54
.to_str()
.unwrap();

println!("cargo:warning=fetching {name} from GitHub");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm reconsidering showing this as a warning: this would be displayed for every build on every crate that depends on ironfish. Not a great user experience.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's good enough for now, but I agree, it's not a great permanent solution.

I think a good eventual state would be maybe some feature flags (and no default). I'm assuming feature-flags can be utilized in the build script, but that might not be the case - this is pretty uncharted territory for me.

Maybe something like

  • generate_params which would generate params, good enough for testing or anyone who is using the crate and doesn't plan on connecting to our testnet/mainnet. This is a chicken-and-egg problem though, since you need the built library to create the parameters. Well, I guess technically, you could have ironfish-zkp as a build-dependency, since that's where the code lives for generating basic params (no trusted setup or anything fancy): https://github.com/iron-fish/ironfish/blob/master/ironfish-zkp/src/bin/generate_params.rs
  • download_params which would download the params if missing/invalid hash or whatever.
  • Maybe even a third that is like no_params which requires you to bring your own.

Just kind of brainstorming here, I don't think any of these changes should be added in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fully agree. And yes, $[cfg(feature = ...)] are usable from the build scripts, so this is perfectly doable.

Currently the `ironfish-rust/src/sapling_params` directory contains
almost 90 MB of files (which contain random data and cannot be
compressed). To reduce the size of the `ironfish` crate, this commit
adds build logic that allows that directory to optionally contain no
param files, and fetches them from GitHub.

In particular:
- files are fetched from the GitHub `master` branch if and only if they
  are not present in the `ironfish-rust/src/sapling_params` directory;
- files are checked for integrity using SHA-512, to detect incorrect or
  tampered downloads;
- when performing multiple builds, the files are downloaded and verified
  only during the first build, and results are cached for subsequent
  runs.
@andreacorbellini andreacorbellini merged commit 84030d9 into iron-fish:staging Jun 8, 2023
11 checks passed
@andreacorbellini andreacorbellini deleted the andrea/ifl-1081-reduce-the-size-of-the-ironfish-crate branch July 28, 2023 17:51
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.

None yet

3 participants