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.

Sign up for GitHub

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

Jump to bottom

Use batch validation for Sapling proofs and signatures #6048

Merged
merged 3 commits into from Jul 5, 2022
Merged

Use batch validation for Sapling proofs and signatures #6048

merged 3 commits into from Jul 5, 2022

Conversation

nuttycom
Copy link
Contributor

@nuttycom nuttycom commented Jul 4, 2022 •
edited

This is based on #6000

The original branch is str4d/zcash@sapling-batch-validation; opening this PR from my fork as I don't have permissions to push this back to @str4d's remote.

@nuttycom nuttycom added the safe-to-build Used to send PR to prod CI environment label Jul 4, 2022
daira
daira reviewed Jul 4, 2022
View reviewed changes
src/rust/src/sapling.rs Outdated Show resolved Hide resolved
daira
daira reviewed Jul 4, 2022
View reviewed changes
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.

Provisional ACK for f80a61df4fd2c6380a0f574672fdf5d53b89c7da (obviously after splitting into a separate PR) with a question.

@str4d str4d force-pushed the sapling-batch-validation branch from f80a61d to fab9e9b Compare July 4, 2022 15:21
@str4d
Copy link
Contributor

str4d commented Jul 4, 2022

Rebased to make some improvements now that #6000 and #6046 are merged. This PR will need to be force-pushed at least once more in order to migrate to zcash_proofs 0.7.1 once that is published (which will be after we have confirmed this PR does what we need).

@str4d str4d removed the safe-to-build Used to send PR to prod CI environment label Jul 4, 2022
@str4d str4d added this to the Release 5.1.0 milestone Jul 4, 2022
@str4d str4d added I-performance Problems and improvements with respect to performance A-consensus Area: Consensus rules L-rust Involves Rust code. A-chain-sync Area: Chain synchronization / Initial Block Download A-rust-ffi Area: The Rust FFI in the librustzcash library. safe-to-build Used to send PR to prod CI environment labels Jul 4, 2022
daira
daira reviewed Jul 4, 2022
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
nuttycom
nuttycom commented Jul 4, 2022
View reviewed changes
Copy link
Contributor Author

@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.

testedACK; I can't make this an official github "approval" because I opened the PR, despite the fact that I didn't write it.

daira
daira reviewed Jul 4, 2022
View reviewed changes
src/rust/src/sapling.rs Outdated Show resolved Hide resolved
daira
daira reviewed Jul 4, 2022
View reviewed changes
src/uint256.h Show resolved Hide resolved
daira
daira previously approved these changes Jul 4, 2022
View reviewed changes
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 with suggestions.

@str4d
bench: Add ConnectBlock benchmark using block 1723244
5d9ae0b 
This block has 470 transactions, containing a total of 452 Sapling
spends and 1862 outputs, making for a nice benchmark of verification
performance.
@str4d str4d force-pushed the sapling-batch-validation branch from fab9e9b to 828f566 Compare July 4, 2022 18:05
@str4d str4d added safe-to-build Used to send PR to prod CI environment and removed safe-to-build Used to send PR to prod CI environment labels Jul 4, 2022
@str4d
Copy link
Contributor

str4d commented Jul 4, 2022 •
edited

Force-pushed to fix the gtest failure, and add a Sapling-full block benchmark.

Average validation time for block 1723244 on my Ryzen 9 5950X:

  • First commit (effectively 5.0.0): 8.94s
  • Second commit (with batching): 3.87s

The current PR therefore gives a reduction of 56.7%. Note that both of these are running on a single thread, so there are futher reductions we could get, either by splitting the batch items across multiple batches, or using threadpools inside the batch logic (which @ebfull is currently trying out).

@str4d str4d mentioned this pull request Jul 4, 2022
Merged
therealyingtong
therealyingtong previously approved these changes Jul 5, 2022
View reviewed changes
Copy link
Contributor

@therealyingtong therealyingtong left a comment

Choose a reason for hiding this comment

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

utACK modulo updating librustzcash version.

@softminus
Copy link
Contributor

Yeah, running gitian on https://github.com/nuttycom/zcash/tree/sapling-batch-validation (hash828f566f1c2bbbeed06197c6cabf2752b32c170c) failed with

then echo Extracting pre-vendored crates from /home/debian/cache/common/vendored-crates.tar.gz...; \
	tar xf /home/debian/cache/common/vendored-crates.tar.gz -C /home/debian/build/zcash/depends/x86_64-linux-gnu; \
else echo Vendoring crates...; \
	/home/debian/build/zcash/depends/x86_64-linux-gnu/native/bin/cargo vendor --locked --manifest-path /home/debian/build/zcash/depends/../Cargo.toml /home/debian/build/zcash/depends/x86_64-linux-gnu/vendored-sources; \
fi
Vendoring crates...
    Updating git repository `https://github.com/zcash/librustzcash.git`
error: failed to sync

Caused by:
  failed to load pkg lockfile

Caused by:
  failed to resolve patches for `https://github.com/rust-lang/crates.io-index`

Caused by:
  failed to load source for dependency `f4jumble`

Caused by:
  Unable to update https://github.com/zcash/librustzcash.git?rev=2519e16ad8ab3a6af59a49028e014e13dddfc36c#2519e16a

Caused by:
  object not found - no match for id (2519e16ad8ab3a6af59a49028e014e13dddfc36c); class=Odb (9); code=NotFound (-3)
make: *** [Makefile:129: /home/debian/build/zcash/depends/x86_64-linux-gnu/.stamp_dce65f034f8] Error 101
make: Leaving directory '/home/debian/build/zcash/depends'

and the relevant cargo lint failed:

Missing replacement in .cargo/config.offline:
  [source."https://github.com/zcash/librustzcash.git"]
  git = "https://github.com/zcash/librustzcash.git"
  rev = "2519e16ad8ab3a6af59a49028e014e13dddfc36c"
  replace-with = "vendored-sources"

@str4d
Copy link
Contributor

str4d commented Jul 5, 2022

Yep, that Gitian failure is expected because I did not fully update this PR to correctly use patching, since that is not what we will be deploying. I'm updating the PR right now to use the new crate releases.

@str4d str4d force-pushed the sapling-batch-validation branch from 828f566 to af59b2a Compare July 5, 2022 18:24
@str4d
Copy link
Contributor

str4d commented Jul 5, 2022

Force-pushed to use the published bellman 0.13.1 and zcash_proofs 0.7.1.

@str4d str4d added safe-to-build Used to send PR to prod CI environment and removed safe-to-build Used to send PR to prod CI environment labels Jul 5, 2022
@str4d
Copy link
Contributor

str4d commented Jul 5, 2022

Re-ran the benchmarks from the first commit in this PR on my Ryzen 9 5950x:

Unbatched (s) Batched (s) Reduction
8.88 1.88 78.8%
Benchmark data

Unbatched

[
  {
    "runningtime": 8.884456
  },
  {
    "runningtime": 8.887702000000001
  },
  {
    "runningtime": 8.880763
  },
  {
    "runningtime": 8.86946
  },
  {
    "runningtime": 8.870950000000001
  },
  {
    "runningtime": 8.872097
  },
  {
    "runningtime": 8.880618999999999
  },
  {
    "runningtime": 8.871511999999999
  },
  {
    "runningtime": 8.887917999999999
  },
  {
    "runningtime": 8.873296
  }
]

Batched

[
  {
    "runningtime": 1.900179
  },
  {
    "runningtime": 1.88112
  },
  {
    "runningtime": 1.882656
  },
  {
    "runningtime": 1.888303
  },
  {
    "runningtime": 1.879558
  },
  {
    "runningtime": 1.892048
  },
  {
    "runningtime": 1.882502
  },
  {
    "runningtime": 1.880664
  },
  {
    "runningtime": 1.881228
  },
  {
    "runningtime": 1.880931
  }
]

therealyingtong
therealyingtong approved these changes Jul 5, 2022
View reviewed changes
Copy link
Contributor

@therealyingtong therealyingtong left a comment

Choose a reason for hiding this comment

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

utACK af59b2a

defuse
defuse approved these changes Jul 5, 2022
View reviewed changes
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

@str4d str4d merged commit 9d93dff into zcash:master Jul 5, 2022
@str4d str4d mentioned this pull request Jul 5, 2022
Closed
@nuttycom nuttycom deleted the sapling-batch-validation branch July 6, 2022 20:22
This was referenced Jun 29, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daira daira daira left review comments

@str4d str4d str4d left review comments

@ebfull ebfull ebfull approved these changes

@defuse defuse defuse approved these changes

@therealyingtong therealyingtong therealyingtong approved these changes

Assignees
No one assigned
Labels
A-chain-sync Area: Chain synchronization / Initial Block Download A-consensus Area: Consensus rules A-rust-ffi Area: The Rust FFI in the librustzcash library. I-performance Problems and improvements with respect to performance L-rust Involves Rust code. safe-to-build Used to send PR to prod CI environment
Projects
None yet
Milestone
Release 5.1.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@nuttycom @str4d @softminus @daira @ebfull @defuse @therealyingtong