Cancel download and verify tasks when the mempool is deactivated

[conradoplg]

Motivation

When the mempool is deactivate we should cancel all current transaction download and verify tasks.

Specifications

Designs

Solution

Add a method to cancel the downloads (copied from the block downloader). Call it when the mempool is deactivated.

Additionally, actually deactivate the mempool: when it's disabled, answer any requests with an error. (I can move this to a separate PR, but it seemed simple enough)

Closes #2629

Review

This is in draft because it's waiting for the enabling/disabling handling be improved in #2723 as discussed in #2754 (review) I realized it made more sense to do this directly here.

Anyone working on the mempool can review it.

One detail I'm not sure of is how the mempool should behave when deactivated. I considered making poll_ready return it's not ready, but I think that's not the correct behaviour (the polling seems to be for capacity) and it maybe would lead to blocking callers?

Reviewer Checklist

• Code implements Specs and Designs
• Tests for Expected Behaviour
• Tests for Errors

Follow Up Work

[teor2345]

I really like the tests here!
And thanks for filling in some missing tests from other PRs.

I think there might be a peer disconnection bug here.
(It might not have shown up during testing, because it would only slow down the sync.)

[conradoplg]

I was doing the refactoring to enable/disable the mempool without relying on boolean and realized that it would automatically solve this issue. For this reason I've committed the refactoring here.

One thing I'm not sure and I'd like feedback on is the enable() and disable() helper functions - if they're a good idea in the first place and also if they're implemented correctly (I'm not sure it's OK to call ready_and() and not call the service aftewards... it works, though)

[teor2345]

One detail I'm not sure of is how the mempool should behave when deactivated. I considered making poll_ready return it's not ready, but I think that's not the correct behaviour (the polling seems to be for capacity) and it maybe would lead to blocking callers?

Yeah there isn't really a state for "still initializing".

And in practice, we don't want to block inbound service requests for 2-6 hours while we sync to the tip.
So we just give the most accurate answer we can: "there's nothing in our mempool yet".

Similarly, while we're initializing the inbound service, we either return an empty response, or we don't respond at all.

One thing I'm not sure and I'd like feedback on is the enable() and disable() helper functions - if they're a good idea in the first place and also if they're implemented correctly (I'm not sure it's OK to call ready_and() and not call the service aftewards... it works, though)

Technically this is ok:

Once poll_ready returns Poll::Ready(Ok(())), a request may be dispatched to the service using call. Until a request is dispatched, repeated calls to poll_ready must return either Poll::Ready(Ok(())) or Poll::Ready(Err(_)).

https://docs.rs/tower-service/0.3.1/tower_service/trait.Service.html#tymethod.poll_ready

But it can cause tower::Buffers to block until some of the spurious readiness slots are dropped:
https://github.com/ZcashFoundation/zebra/blob/main/book/src/dev/rfcs/0011-async-rust-in-zebra.md#buffered-services

So it's better to just submit a cheap read-only request, like oneshot(Request::TransactionIds).

It probably won't matter in this specific situation, because:

• there aren't any buffers (yet!)
• the tests are small (but it might slow or fail larger tests)

But it's worth avoiding anyway, so we don't cause failures during future refactors.

[teor2345]

This looks good - we could merge it as it is.

But there's a future bug risk we might want to fix, and a few possible refactors we could do to simplify things.

[teor2345]

One thing I'm not sure and I'd like feedback on is the enable() and disable() helper functions - if they're a good idea in the first place

I just realised I didn't answer this:

Yes, I think these functions are good, because they make the tests simpler.

[conradoplg]

Addressed comments and synced with main

[conradoplg]

@teor2345 can you please review the last changes?

[teor2345]

Looks great, but we'll need to resolve conflicts with PR #2765.

Then feel free to merge!

[conradoplg]

Solved conflicts and synced with main.

[teor2345]

Let's merge!