Prepare for asynchronous transaction validation in tx pool

[svyatonik]

This is required to restore txpool && txpool-related RPCs on light client after #3538 (light-related changes will be added in follow-up PRs). The main change in this PR is that ChainApi::validate_transaction now returns Future<> instead of Result<>. This affects Pool::{submit_at, submit_one, submit_and_watch, prune, prune_tags} methods. All other changes are simply fixes to support that change.

Planned light-related changes: (1) separate ChainApi implementation that will use Fetcher to validate transaction on remote node (that has been removed in #3538) and (2) separate maintain_transaction_pool() implementation that will (presumably) fetch proof-of-tx-inclusion from remote node and remove transactions that are included in the block.

[svyatonik]

Ahhh, forgot about this change - could create a separate PR for it, if required. Extrinsics from other nodes are ignored by light tx pool anyway => this change only decreases traffic on light client.

[tomusdrw]

I'm not super happy about how sync and async code operates here. I really think we should avoid locking inside the event pool.

Can we maybe keep the pool synchronous and just decouple validation? Alternatively we could have the (sync) pool running in it's own thread and the asynchronous code would just send messages (like AddTx(raw, TransactionValidity) via mpsc channel.

[tomusdrw]

This is locking inside the event loop :/ I think it might have very bad performance consequences.

[svyatonik]

The separate-thread approach won't work for light-in-browser, I believe :/ By decoupling you mean passing pre-validated transactions to submit_at() / submit_one()? I've tried this initially, but when noticed that revalidation is required in prune(), switched to async-validate-approach. Do you think it still worth move out validation interface from pool.rs?

Also - could you please elaborate on "avoid locking inside the event pool" - not sure I got it :/ Did you mean locks like Mutex::lock, or blocking on futures in event loop threads? Afaik there's no explicit thread spawns (other than in offchain) now in substrate - we are always executing code from within tokio_executor::DefaultExecutor threads. So whenever we call something like Mutex::lock, it is locking within tokio threads.

[tomusdrw]

So whenever we call something like Mutex::lock, it is locking within tokio threads.

Yeah, that's exactly what I mean. I don't think it's healthy to do that.

I've tried this initially, but when noticed that revalidation is required in prune(), switched to async-validate-approach. Do you think it still worth move out validation interface from pool.rs?

I still think it might be a bit cleaner to do so. prune could get passed a function Fn(&[Hash]) -> impl Future<Output=Vec<Validity>> to verify all needed transactions. so that we make sure we never lock in async context.

[svyatonik]

OK, thanks :) I still not sure - what's wrong with locking in this PR - there are exactly same locks as before, locking exactly same (tokio) threads (at different moments, though) :) And after fix, they still be there. But decoupling, probably, makes sense.

[svyatonik]

So the Pool is now ValidatedPool and Pool. Crate-internal ValidatedPool only deals with pre-verified transactions and has synchronous API. Pool has future-based API and internally uses ValidatedPool to store transactions after they're validated.

I've also found that in my first commit I've missed one block_on() call in production code - there's a separate commit that fixes it.

[tomusdrw]

Fair enough. Somehow I though that sync RPC are executed differently than async (in a separate thread pool), but that's obviously not the case.
The decoupling looks good, I think that in the future it may still make sense to dedicate a separate thread for transaction pool alterations / locking and avoid locks in async code completely.
Alternatively we could try to rewrite the pool to avoid locks - that would mean that the ready iterator API would need to be async most likely.

[tomusdrw]

Great!