Redjubjub signature verifier service #460

dconnolly · 2020-06-10T05:06:37Z

And stub of redjubjub::BatchVerifier.

codecov · 2020-06-10T05:19:23Z

Codecov Report

Merging #460 into main will increase coverage by 0.30%.
The diff coverage is 86.27%.

@@            Coverage Diff             @@
##             main     #460      +/-   ##
==========================================
+ Coverage   57.95%   58.25%   +0.30%     
==========================================
  Files          91       92       +1     
  Lines        4419     4470      +51     
==========================================
+ Hits         2561     2604      +43     
- Misses       1858     1866       +8

yaahc

🔥

zebra-consensus/src/verify/redjubjub.rs

dconnolly · 2020-06-24T20:13:14Z

I will be closing this PR and moving the updated batch verifier (to work with the new tower-batch) into ZcashFoundation/redjubjub

hdevalence · 2020-06-24T20:21:32Z

@dconnolly The batch verification implementation makes sense to have in the upstream redjubjub crate, but I think that the async tower-based interface to it is probably better here — my experience with ed25519-zebra was that it was much easier to maintain just the synchronous, IUF-style API in the library crate. Also, I don’t think we would be able to publish a version of redjubjub that depended on tower-batch, since got dependencies are not allowed in published crates.

dconnolly · 2020-06-24T20:54:38Z

@dconnolly The batch verification implementation makes sense to have in the upstream redjubjub crate, but I think that the async tower-based interface to it is probably better here — my experience with ed25519-zebra was that it was much easier to maintain just the synchronous, IUF-style API in the library crate. Also, I don’t think we would be able to publish a version of redjubjub that depended on tower-batch, since got dependencies are not allowed in published crates.

Good point, I'll keep this open and update it with the tower interface layer and update redjubjub with its own batch::Verifier

dconnolly · 2020-06-27T21:09:15Z

If I only run the redjubjub tests, they pass. When running with all tests, I see inconsistent failures:

dconnolly · 2020-06-27T21:43:00Z

why was this removed?

dconnolly · 2020-06-27T21:44:28Z

why was this removed?

I don't know why this worked fine when used similarly in here but removing it allowed all tests to pass.

yaahc · 2020-06-27T22:39:15Z

okay, that's a bug. Removing the init will disable our logging and error handling stuff, so we want that to work. I'll open a separate issue to add them back and fix the bug though

teor2345 · 2020-06-29T08:20:03Z

okay, that's a bug. Removing the init will disable our logging and error handling stuff, so we want that to work. I'll open a separate issue to add them back and fix the bug though

Looks like there's a panic in one of the functions that's called during the Once.

Do we have any other copies of the old tracing code in the codebase?
The panic could be happening due to a race between the old code and the Once code.

hdevalence · 2020-06-29T21:33:51Z

zebra-consensus/Cargo.toml

 futures-util = "0.3.5"
-tower = "0.3.1"
+rand = "0.7"
+redjubjub = { git = "https://github.com/ZcashFoundation/redjubjub.git", branch = "batch" }


I'd prefer to wait to merge this until we can depend on a published version of redjubjub, but that can happen essentially as soon as it's ready.

hdevalence · 2020-06-29T21:36:55Z

zebra-consensus/src/verify.rs

 mod script;
 mod transaction;

+pub use self::redjubjub::{RedJubjubItem, RedJubjubVerifier};


naming: if the types are called just Item and Verifier, this reëxport can be dropped and users can choose how much they want to qualify the name, depending on their context, either redjubjub::Item and redjubjub::Verifier or Item and Verifier. I think this is more flexible than including RedJubjub in the struct names.

(There's then a potential naming overlap between the redjubjub crate's Verifier type and this one, but this is only ambiguous when code needs to refer to both synchronous and asynchronous batch verification APIs, which only happens in this specific module, otherwise people just use one or the other).

I think this would still be good to do. More rationale: rust-lang/rfcs#356

hdevalence · 2020-06-29T21:41:20Z

👍 Nice work! The only comments I have are about naming. I guess the API might change slightly when the (sync) batch code is updated to cover both signature types.

yaahc · 2020-06-29T15:48:09Z

zebra-consensus/Cargo.toml

 chrono = "0.4.11"
+color-eyre = "0.5"


This should probably be a dev-dependency

zebra-consensus/src/verify/redjubjub.rs

yaahc · 2020-06-29T22:36:50Z

zebra-consensus/src/verify/redjubjub.rs

+            let sig = sk.sign(rng, &msg[..]);
+
+            verifier.ready_and().await?;
+            results.push(span.in_scope(|| verifier.call((vk.into(), sig, msg).into())))


I don't think this in_scope does what is intended, this would enter span for the duration of call, which then returns a future, but then it would push an uninstrumented future into the results. If you want the span to be active while the future is being polled you should use instrument

Suggested change

results.push(span.in_scope(|| verifier.call((vk.into(), sig, msg).into())))

results.push(verifier.call((vk.into(), sig, msg).into()).instrument(span))

zebra-consensus/src/verify/redjubjub.rs

dconnolly · 2020-07-03T23:08:20Z

Ugh I messed up with my rebases.

.github/workflows/coverage.yml

zebra-consensus/Cargo.toml

hdevalence

I think we should change the names to remove the module prefix but otherwise this looks great!

zebra-consensus/src/verify.rs

hdevalence

Awesome!!

Using updated redjubjub::batch::Verifier in the redjubjub#batch branch.

Now the Verifier is agnostic of redjubjub SigTypes. Updated tests to generate sigs of both types and batch verifies the whole batch. Resolves #407

Bumps [chrono](https://github.com/chronotope/chrono) from 0.4.11 to 0.4.12. - [Release notes](https://github.com/chronotope/chrono/releases) - [Changelog](https://github.com/chronotope/chrono/blob/master/CHANGELOG.md) - [Commits](chronotope/chrono@v0.4.11...v0.4.12) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Using updated redjubjub::batch::Verifier in the redjubjub#batch branch.

To follow this convention RFC: rust-lang/rfcs#356

dconnolly added the A-rust Area: Updates to Rust code label Jun 10, 2020

dconnolly added this to the Validate transactions. milestone Jun 10, 2020

dconnolly requested review from yaahc, hdevalence and teor2345 June 10, 2020 05:06

dconnolly linked an issue Jun 10, 2020 that may be closed by this pull request

Implement a RedJubjubVerifier service #407

Closed

yaahc previously approved these changes Jun 10, 2020

View reviewed changes

zebra-consensus/src/verify/redjubjub.rs Outdated Show resolved Hide resolved

hdevalence mentioned this pull request Jun 24, 2020

Implement batch verification ZcashFoundation/redjubjub#30

Closed

dconnolly dismissed yaahc’s stale review via 6bd45de June 25, 2020 01:01

dconnolly force-pushed the redjubjub-verifier branch 2 times, most recently from 6bd45de to 88121d4 Compare June 26, 2020 23:49

dconnolly changed the title ~~[WIP] Add singleton redjubjub signature verifier service~~ [WIP] Redjubjub signature verifier service Jun 26, 2020

dconnolly changed the title ~~[WIP] Redjubjub signature verifier service~~ Redjubjub signature verifier service Jun 27, 2020

dconnolly marked this pull request as ready for review June 27, 2020 03:51

dconnolly requested a review from yaahc June 27, 2020 03:51

hdevalence reviewed Jun 29, 2020

View reviewed changes

yaahc reviewed Jun 29, 2020

View reviewed changes

dconnolly commented Jun 29, 2020

View reviewed changes

zebra-consensus/src/verify/redjubjub.rs Outdated Show resolved Hide resolved

dconnolly requested review from yaahc and hdevalence July 3, 2020 22:51

dconnolly force-pushed the redjubjub-verifier branch from b641d63 to 4346c2e Compare July 3, 2020 23:10

hdevalence reviewed Jul 7, 2020

View reviewed changes

.github/workflows/coverage.yml Show resolved Hide resolved

hdevalence reviewed Jul 7, 2020

View reviewed changes

zebra-consensus/Cargo.toml Show resolved Hide resolved

hdevalence reviewed Jul 7, 2020

View reviewed changes

zebra-consensus/Cargo.toml Outdated Show resolved Hide resolved

hdevalence suggested changes Jul 7, 2020

View reviewed changes

dconnolly commented Jul 7, 2020

View reviewed changes

zebra-consensus/src/verify.rs Show resolved Hide resolved

dconnolly requested a review from hdevalence July 7, 2020 23:05

yaahc removed their request for review July 7, 2020 23:30

hdevalence previously approved these changes Jul 8, 2020

View reviewed changes

dconnolly dismissed hdevalence’s stale review via 429e89c July 8, 2020 20:05

dconnolly and others added 13 commits July 8, 2020 16:56

Add singleton redjubjub signature verifier service

5b30dbd

Update RedJubjubVerifier to use tower-batch-based async pattern

e285b7b

Using updated redjubjub::batch::Verifier in the redjubjub#batch branch.

Tidy

45937a0

Remove zebra_test::init()

3d399a7

Remove defunct 'msg lifetime

2a9845e

Update to use merged redjubjub::batch::Verifier

fd685c1

Now the Verifier is agnostic of redjubjub SigTypes. Updated tests to generate sigs of both types and batch verifies the whole batch. Resolves #407

Re-remove defunct 'msg lifetime

1273029

Update RedJubjubVerifier to use tower-batch-based async pattern

01f2e3a

Using updated redjubjub::batch::Verifier in the redjubjub#batch branch.

Bump timeout

b865ac2

Depending on the testrunner, it can take longer to verify batch

9ef83f6

Only run coverage on PRs and pushes to main

0d97f81

Rename structs to remove module's name

1c40af5

To follow this convention RFC: rust-lang/rfcs#356

dconnolly force-pushed the redjubjub-verifier branch from 429e89c to 1c40af5 Compare July 8, 2020 21:01

dconnolly merged commit 2cd58c8 into main Jul 8, 2020

dconnolly deleted the redjubjub-verifier branch July 8, 2020 21:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redjubjub signature verifier service #460

Redjubjub signature verifier service #460

dconnolly commented Jun 10, 2020

codecov bot commented Jun 10, 2020 •

edited

Loading

yaahc left a comment

dconnolly commented Jun 24, 2020

hdevalence commented Jun 24, 2020

dconnolly commented Jun 24, 2020

dconnolly commented Jun 27, 2020

dconnolly commented Jun 27, 2020

dconnolly commented Jun 27, 2020

yaahc commented Jun 27, 2020 •

edited

Loading

teor2345 commented Jun 29, 2020

hdevalence Jun 29, 2020

hdevalence Jun 29, 2020

hdevalence Jun 29, 2020

hdevalence Jul 7, 2020

hdevalence commented Jun 29, 2020

yaahc Jun 29, 2020

yaahc Jun 29, 2020

dconnolly commented Jul 3, 2020

hdevalence left a comment

hdevalence left a comment

	results.push(span.in_scope(\|\| verifier.call((vk.into(), sig, msg).into())))
	results.push(verifier.call((vk.into(), sig, msg).into()).instrument(span))

Redjubjub signature verifier service #460

Redjubjub signature verifier service #460

Conversation

dconnolly commented Jun 10, 2020

codecov bot commented Jun 10, 2020 • edited Loading

Codecov Report

yaahc left a comment

Choose a reason for hiding this comment

dconnolly commented Jun 24, 2020

hdevalence commented Jun 24, 2020

dconnolly commented Jun 24, 2020

dconnolly commented Jun 27, 2020

dconnolly commented Jun 27, 2020

dconnolly commented Jun 27, 2020

yaahc commented Jun 27, 2020 • edited Loading

teor2345 commented Jun 29, 2020

hdevalence Jun 29, 2020

Choose a reason for hiding this comment

hdevalence Jun 29, 2020

Choose a reason for hiding this comment

hdevalence Jun 29, 2020

Choose a reason for hiding this comment

hdevalence Jul 7, 2020

Choose a reason for hiding this comment

hdevalence commented Jun 29, 2020

yaahc Jun 29, 2020

Choose a reason for hiding this comment

yaahc Jun 29, 2020

Choose a reason for hiding this comment

dconnolly commented Jul 3, 2020

hdevalence left a comment

Choose a reason for hiding this comment

hdevalence left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 10, 2020 •

edited

Loading

yaahc commented Jun 27, 2020 •

edited

Loading