Clean up Consensus Service traits and infrastructure

[gnunicorn]

Following #911, most consensus infrastructure was extracted into substrate-consensus-common or the respective consensus engine. However for the integration substrate-service and substrate-network still contain a few traits and a generic implementation of the OfflineTracker. The interfaces should be cleaned up and - as much as possible - moved into and provided by substrate-consensus-common and in-turn be exposed by the specific engines.

[bkchr]

While doing this, the consensus::BlockImport should also be moved. Currently consensus::BlockImport is implemented for the client in client.rs. Move the implementation into consensus-common where the trait is defined.

[rphmeier]

While doing this, the consensus::BlockImport should also be moved. Currently consensus::BlockImport is implemented for the client in client.rs. Move the implementation into consensus-common where the trait is defined.

Do you mean that we should move BlockImport and related types into client? If so, I disagree -- BlockImport is an interface that shouldn't bring all client code with it.

What's wrong with the dependency chain client <- consensus-common? Why should consensus-common need to be aware of client?

[bkchr]

For example the transaction-pool is also depending on the client and not in the reverse order. In this case I wanted to move the TransactionPool runtime api into the transaction-pool crate, but that does not work currently as the TransactionPool api is also required in the client.

[rphmeier]

That seems like a different issue -- how does it relate to consensus?

[bkchr]

Consensus use the function in Client that requires the TransactionPool api.

[bkchr]

It is the execute_and_import_block function in the Client.

[gnunicorn]

I think this is a question of general architecture and purpose of consensus-common. At the moment quite a few crates contain consensus.rs-files which provide mostly glue of the consensus common to their respective API, e.g. in service, which links to this issue.

For many crates in substrate we opted for a primitives-style approach, where we have a super thin almost-no-dependencies crate that provides traits and super general impls (if useful) that others than link to and can use their interface for. However, this isn't the case here. I didn't opt for calling consensus a primitives-crate because I was thinking of consensus-common to become the "catchall" for all consensus-code that is common across many consensus-algorithms and that they can reuse - like the offline tracker or the proposer code base. That though requires that common becomes a "fat" crate with dependencies onto many other crates throughout the project rather than the slimmer primitives-approach.

I can understand the urge to keep the dependency tree of the specific consensus crates slimmer and them potentially providing all the necessary parts themselves, but in practice this isn't actually possible because many other crates do actually provide glue that they are then bound to - e.g #1158. By moving most of it into common I was hoping for that to become more explicit and over time make no other crate directly bind to any common implementation anymore.

I am also okay with splitting up consensus-common into this bundle and a second super slim consensus-primitives that only provides the basic traits and aim for no non-consensus-substrate-crate to have consensus-common as a dependency (but allow node to do that). What do you think @rphmeier , @bkchr ?

[rphmeier]

That sounds reasonable. So consensus-common would hold things like basic proposers or perhaps networking/import-queue logic useful for consensus algorithms, while consensus-primitives would contain traits and types that even the client could depend upon.

[rphmeier]

It is the execute_and_import_block function in the Client.

@bkchr

It seems like a bigger error that execute_and_import_block requires TaggedTransactionQueue at all. The implication is that we can't import blocks from any chains which don't implement this API, which is not the goal of substrate at all. Every API that is not Core should not be needed to import blocks at all.

BlockImport should remain defined in consensus-primitives so that consensus algorithm implementations don't need to link the substrate-client crate.

[bkchr]

Yeah, I'm also for keeping BlockImport in consensus-primitives.
If execute_and_import_block is the problem, I'm okay with keeping BlockImport implemented in client.rs. Then we should tackle execute_and_import_block and remove the dependency on TaggedTransactionQueue.