Ensure custom select chain is applied consistently

[gnunicorn]

following #2240 , which focused on refactoring client to provide to customize the strategy to pick a fork easily, it has been pointed out that ForkChoiceStrategy and some related code in client.rs that uses it, might still implicitly pick the longest chain.

Let's go through client.rs and all other places ForkChoiceStrategy influences and make sure that:

1. the customised strategy is applied successfully and no other code implies to know which best chain to pick instead
2. ForkChoiceStrategy is in a consistent state with the chosen strategy

[andresilva]

I think we can unify the two if we add a new method to SelectChain:

fn is_new_best(&self, &Header) -> Result<bool, Error>;

Which we can use when importing a new block (https://github.com/paritytech/substrate/blob/master/core/client/src/client.rs#L824).

[gnunicorn]

And dropping ForkChoiceStrategy - sounds good to me!

Though we might want to do this past #2532, to make sure an implementation doesn't accidentially dead lock on that call - as LongestChain currently would.

[ghost]

@andresilva , I assume either a U3-nice-to-have, or closable?

[kostekIV]

Hi, @gavofyork sorry for pinging. We want to use a kinda custom select chain and we are not sure if this issue is still relevant. Do we now have some confidence that the select chain is applied consistently? Thx

[bkchr]

@kostekIV select chain should be used by the block production and block finalization, which is done currently. Do you have issues?

[kostekIV]

No, I don't have issues with it but I saw that this issue was open and I have started worrying that there might be some inconsistencies left.

[bkchr]

The handling is not perfect, but the most important point are the consensus engines and they are using SelectChain.