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

Use oneshot() in zebra-consensus #1710

Closed
wants to merge 4 commits into from

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Feb 8, 2021

Motivation

Remove possible hangs caused by a bad use of ready_and().

Solution

Change this calls to oneshot() as suggested in #1675

There is a compiler error with the state calls in BlockVerifier that needs to be addressed. The rest of the changes are ready for review.

TODO

  • Make sure zebra-consensus doesn't contain any other uses of poll_ready or ready_and

Review

@teor2345

@teor2345
Copy link
Contributor

teor2345 commented Feb 8, 2021

#1675 is a medium priority ticket, because as far as I can tell, the code doesn't contain any actual hangs.

Let's try to find a high-priority ticket for your next PR?

@teor2345 teor2345 added A-rust Area: Updates to Rust code C-bug Category: This is a bug C-cleanup Category: This is a cleanup P-Medium labels Feb 8, 2021

// We use a `ServiceExt::oneshot`, so that transaction_verifier
// service `poll_ready` has a corresponding `call`. See #1593.
let rsp = transaction_verifier.clone().oneshot(request);
Copy link
Contributor

Choose a reason for hiding this comment

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

oneshot takes ownership of the service, but rustc can't be sure that the transaction_verifier is Send:
https://doc.rust-lang.org/std/marker/trait.Send.html

Send means that the type can be safely sent between threads. Previously, the code was just sending a mutable reference to the verifier between threads, which uses the Sync bound:
https://doc.rust-lang.org/std/marker/trait.Sync.html

Since futures can be sent between threads, the transaction verifier needs to be wrapped in a Buffer, which is a Send type:
https://github.com/ZcashFoundation/zebra/pull/1700/files/140b83b4bab1a161a2bc2a3a3c39e77136687dd6#diff-49f13a370043562a8c8c03657175ad9a6a49f098776eb88e8bfc08d02a1be148

We are going to need a Buffered transaction verifier when we add a mempool anyway. With a shared transaction verifier, we can verify block transactions and loose transactions using the same signature and proof verifiers, to take advantage of batched cryptography.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably also drop the Derive Clone on the transaction and script verifiers. We want to use the buffer instead of cloning the underlying verifier.

I think it's ok to just add the buffer in BlockVerifier::new, but if there are any other transaction verifier users, we should add the buffer in an init method in transaction.rs. (Similarly for the script verifier, but I'm not sure if it will ever do batching.)

Copy link
Contributor

Choose a reason for hiding this comment

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

@yaahc @dconnolly will the script verifier ever do batch verification?

I'm trying to work out if it's worth wrapping it in a Buffer.
(This isn't an urgent question, I'd just like to know.)

@teor2345 teor2345 added the S-needs-triage Status: A bug report needs triage label Feb 10, 2021
@teor2345
Copy link
Contributor

I've marked this PR as needing triage pending the tower::Buffer fix.

@teor2345
Copy link
Contributor

Obsoleted by the fix to the underlying issue in tower::Buffer, see #1593 for details.

@teor2345 teor2345 closed this Feb 15, 2021
@teor2345 teor2345 added this to the 2021 Sprint 3 milestone Feb 15, 2021
@teor2345
Copy link
Contributor

This PR belongs in sprint 3 because we actually did the work.

@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code C-bug Category: This is a bug C-cleanup Category: This is a cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants