Fix poll_ready usage in ChainVerifier #1700

oxarbitrage · 2021-02-05T18:35:42Z

Motivation

Work in progress for #1679

Solution

Review

Related Issues

Closes #1679.

Fixes part of #1675.

Follow Up Work

Clippy will fail on this PR until #1706 merges.

zebra-consensus/src/chain.rs

And rename the chain verifier fields so `block` means `Arc<Block>`, and `block_verifier` means `Buffer<BlockVerifier, ...>`.

teor2345 · 2021-02-08T00:24:50Z

zebra-consensus/src/chain.rs

    /// block could not be checkpointed
-    Checkpoint(#[source] VerifyCheckpointError),
+    Checkpoint(#[source] BoxError),
    /// block could not be verified
-    Block(#[source] VerifyBlockError),
+    Block(#[source] BoxError),


Since we're using a Buffer, we get BoxErrors back from the checkpoint and block verifiers.

zebra-consensus/src/chain.rs

teor2345 · 2021-02-08T01:01:33Z

zebra-consensus/src/chain.rs

+        // We can't call `poll_ready` on the block and checkpoint verifiers here,
+        // because each `poll_ready` must be followed by a `call`, and we don't
+        // know which verifier we're going to choose yet.


We can't call poll_ready here - it doesn't matter what we do with the results, it's the call that takes up a buffer slot.

@yaahc - this constraint is something to keep in mind for future service code reviews.

If we call poll_ready, but there isn't a matching call, we fill up our Buffers with unused reservations, and Zebra hangs.

teor2345 · 2021-02-08T01:02:30Z

zebra-consensus/src/chain.rs

-                .checkpoint
-                .call(block)
+                .checkpoint_verifier
+                .clone()
+                .oneshot(block)
                .map_err(VerifyChainError::Checkpoint)
                .boxed(),
            // This also covers blocks with no height, which the block verifier
            // will reject immediately.
            _ => self
-                .block
-                .call(block)
+                .block_verifier
+                .clone()
+                .oneshot(block)


These services are now Buffered, so we don't need an async block. We can just call ServiceExt::oneshot on a cloned service, and return those futures.

@yaahc @oxarbitrage the best way to avoid poll_ready / call mismatches is to just use a oneshot.

It handles the poll_ready and call internally, so we can't possibly get it wrong.

teor2345 · 2021-02-08T01:03:10Z

zebra-consensus/src/chain.rs

+    let block_verifier = BlockVerifier::new(network, state_service.clone());
+    let checkpoint_verifier = CheckpointVerifier::from_checkpoint_list(list, tip, state_service);
+
+    let block_verifier = Buffer::new(block_verifier, VERIFIER_BUFFER_BOUND);
+    let checkpoint_verifier = Buffer::new(checkpoint_verifier, VERIFIER_BUFFER_BOUND);


It's a bit easier to read the code if we create the services, then replace them with buffered versions.

And: * make the code look like the `main` branch as much as possible * document the `poll_ready`/`call` invariant

teor2345 · 2021-02-08T01:19:18Z

zebra-consensus/src/chain.rs

+    // Correctness:
+    //
+    // We use `ServiceExt::oneshot` to make sure every `poll_ready` has a
+    // matching `call`. See #1593 for details.
    let tip = match state_service
-        .ready_and()
-        .await
-        .unwrap()
-        .call(zs::Request::Tip)
+        .clone()
+        .oneshot(zs::Request::Tip)
        .await
        .unwrap()


This fix is part of #1675 - and it's actually something I missed during my review last week.

Since we're fixing this file, let's also use ServiceExt::oneshot here.

teor2345

I think this PR is fine now, but I wrote some of it, so @oxarbitrage or @yaahc should review it before it merges.

oxarbitrage · 2021-02-08T13:05:57Z

@teor2345 this looks great and thank you for all the comments, the logic looks good and the code very clean and concise.

I had a few problems with the references we had before to do this change.

1- #1593 was too long and general. This is ok and all useful information but i got lost.
2- #1620 makes more changes than are actually needed to implement oneshot. Got lost trying to understand them.
3- The issue #1679 was suggesting initially a solution without oneshot. I was not smart enough to try to apply it anyways, it is a lot easier than making the matches "manually".

However, with the content of this PR everything is a lot clear to me.

This PR teaches:

how to buffer and use buffered services and errors.
how to use oneshot.
comments explain how to do poll_ready and match calls method.

teor2345 · 2021-02-08T21:22:38Z

I'm re-running the jobs to make sure #1706 fixes clippy.

teor2345 · 2021-02-08T22:12:19Z

I think clippy failed because it's working on the head commit, or an old merge.

Let's just merge, the type is used, so clippy should pass on main.

teor2345

I wrote some of this, and others have checked it, so it's good.

* change `poll_ready()` and `call()` of `ChainVerifier` * add bound, move max_checkpoint_height * add buffers to the checkpoint and block verifiers And rename the chain verifier fields so `block` means `Arc<Block>`, and `block_verifier` means `Buffer<BlockVerifier, ...>`. * Fix the error types * Use `ServiceExt::oneshot` in `ChainVerifier::call` And: * make the code look like the `main` branch as much as possible * document the `poll_ready`/`call` invariant * Use `ServiceExt::oneshot` in `chain::init` Co-authored-by: teor <[email protected]>

This reverts commit 0723ac5.

* Revert "Fix poll_ready usage in ChainVerifier (#1700)" This reverts commit 0723ac5. * Keep the VERIFIER_BUFFER_BOUND change * Correctly implement multiple readiness

change poll_ready() and call() of ChainVerifier

b11407f

zfnd-bot bot assigned oxarbitrage Feb 5, 2021

oxarbitrage added 2 commits February 5, 2021 16:57

add bound, move max_checkpoint_height

3242860

use buffer for checkpointverifier

d61e57d

teor2345 reviewed Feb 7, 2021

View reviewed changes

zebra-consensus/src/chain.rs Outdated Show resolved Hide resolved

teor2345 reviewed Feb 7, 2021

View reviewed changes

zebra-consensus/src/chain.rs Outdated Show resolved Hide resolved

teor2345 added 4 commits February 8, 2021 10:15

Remove unnecessary changes

770bb9c

Fix poll_ready

b15071a

Add buffers to the checkpoint and block verifiers

e6670cd

And rename the chain verifier fields so `block` means `Arc<Block>`, and `block_verifier` means `Buffer<BlockVerifier, ...>`.

Fix the error types

1daff0d

teor2345 reviewed Feb 8, 2021

View reviewed changes

zebra-consensus/src/chain.rs Outdated Show resolved Hide resolved

teor2345 force-pushed the issue1679 branch 2 times, most recently from 89dd361 to 1c8c435 Compare February 8, 2021 00:41

teor2345 added 2 commits February 8, 2021 10:49

Use ServiceExt::oneshot

eb885c4

Remove type bound order swap

0834f50

teor2345 force-pushed the issue1679 branch from ba75c14 to 140b83b Compare February 8, 2021 00:49

teor2345 mentioned this pull request Feb 8, 2021

Actually use VerifyCheckpointError::CommitFinalized #1706

Merged

1 task

teor2345 reviewed Feb 8, 2021

View reviewed changes

teor2345 changed the title ~~[WIP] Attempt to fix poll_ready usage in ChainVerifier~~ Fix poll_ready usage in ChainVerifier Feb 8, 2021

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

teor2345 added this to the 2021 Sprint 2 milestone Feb 8, 2021

teor2345 added 2 commits February 8, 2021 11:12

Remove the async block

17ee36c

And: * make the code look like the `main` branch as much as possible * document the `poll_ready`/`call` invariant

Use ServiceExt::oneshot in chain::init

1434016

teor2345 force-pushed the issue1679 branch from 140b83b to 1434016 Compare February 8, 2021 01:17

teor2345 reviewed Feb 8, 2021

View reviewed changes

teor2345 mentioned this pull request Feb 8, 2021

Make sure Zebra uses poll_ready and Buffer reservations correctly #1593

Closed

9 tasks

teor2345 approved these changes Feb 8, 2021

View reviewed changes

teor2345 merged commit 0723ac5 into ZcashFoundation:main Feb 8, 2021

teor2345 added a commit that referenced this pull request Feb 15, 2021

Revert "Fix poll_ready usage in ChainVerifier (#1700)"

7168068

This reverts commit 0723ac5.

teor2345 mentioned this pull request Feb 15, 2021

Partly revert "Fix poll_ready usage in ChainVerifier" #1735

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix poll_ready usage in ChainVerifier #1700

Fix poll_ready usage in ChainVerifier #1700

oxarbitrage commented Feb 5, 2021 •

edited by teor2345

Loading

teor2345 Feb 8, 2021

teor2345 Feb 8, 2021

teor2345 Feb 8, 2021 •

edited

Loading

teor2345 Feb 8, 2021 •

edited

Loading

teor2345 Feb 8, 2021

teor2345 Feb 8, 2021

teor2345 Feb 8, 2021

teor2345 left a comment

oxarbitrage commented Feb 8, 2021

teor2345 commented Feb 8, 2021

teor2345 commented Feb 8, 2021

teor2345 left a comment

Fix poll_ready usage in ChainVerifier #1700

Fix poll_ready usage in ChainVerifier #1700

Conversation

oxarbitrage commented Feb 5, 2021 • edited by teor2345 Loading

Motivation

Solution

Review

Related Issues

Follow Up Work

teor2345 Feb 8, 2021

Choose a reason for hiding this comment

teor2345 Feb 8, 2021

Choose a reason for hiding this comment

teor2345 Feb 8, 2021 • edited Loading

Choose a reason for hiding this comment

teor2345 Feb 8, 2021 • edited Loading

Choose a reason for hiding this comment

teor2345 Feb 8, 2021

Choose a reason for hiding this comment

teor2345 Feb 8, 2021

Choose a reason for hiding this comment

teor2345 Feb 8, 2021

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

oxarbitrage commented Feb 8, 2021

teor2345 commented Feb 8, 2021

teor2345 commented Feb 8, 2021

teor2345 left a comment

Choose a reason for hiding this comment

oxarbitrage commented Feb 5, 2021 •

edited by teor2345

Loading

teor2345 Feb 8, 2021 •

edited

Loading

teor2345 Feb 8, 2021 •

edited

Loading