Move spawning tasks from thread pools to Service's TaskManager for block importing

[pscott]

This PR moves the spawning of tasks using a threadpool (from the tx pool and from the import queue) to the service, unifying code, adding on_exit behavior, and allowing stats monitoring via Prometheus.

• Remove threadpool from import queue
  Remove threadpool from tx pool -- EDIT, cancelled.

refs #5621
polkadot companion: paritytech/polkadot#1035

[parity-cla-bot]

It looks like @pscott signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

[NikVolf]

You probably meant from import queue? Transaction pool code does not seems to be touched.

[pscott]

You probably meant from import queue? Transaction pool code does not seems to be touched.

It's WIP, I'll be touching

substrate/client/transaction-pool/src/api.rs

Line 91 in 29fa243

self.pool.spawn_ok(futures_diagnose::diagnose("validate-transaction", async move {

next :)

EDIT: I've edited the PR name and PR description to be more explicit :)

[tomaka]

The approach looks good to me so far.

[pscott]

So I have not touched the transaction pool thread pool, essentially because we don't want the transaction validation to potentially take up all cores (potential attack vector). For more details, see the comments of #5621 .

[pscott]

I think we need #5725 to get merged in before we can solve the current issue :) See this comment

[tomaka]

@gnunicorn So are we still allowed to break the public API (which this PR does)? 😬
I guess it would be acceptable, albeit less great, to revert the TaskType thing. EDIT: ah yeah it breaks the ServiceBuilder too, so this PR is all-or-nothing

[gnunicorn]

So are we still allowed to break the public API (which this PR does)

Yes. Not doing a beta with API stability because that is still unfeasible, but stabilization by focusing on fixing rather than adding features.

[tomaka]

Please merge master for the Polkadot CI thing to pass

[tomaka]

Merge master here, not on the Polkadot PR.

[pscott]

Merge master here, not on the Polkadot PR.

But I've already merged master here... ?

[tomaka]

I guess I've misinterpreted the CI errors. Anyway, I've restarted it and there are legitimate errors now.

[pscott]

I guess I've misinterpreted the CI errors. Anyway, I've restarted it and there are legitimate errors now.

Yeah I had built polkadot locally but not build for tests. Building for it locally now, will push once it's done :)

[gavofyork]

@pscott still not building?

[gnunicorn]

tests failing. Lots of

'Receiver::next_message called after `None`',

[tomaka]

tests failing. Lots of

I don't see this?

Let's merge this now?

[gnunicorn]

yeah, not sure what I saw.

[gnunicorn]

lgtm