Double-check the collation related pallets configuration for litmus

[Kailai-Wang]

As previously agreed, we are running the collators ourselves for litmus.

We shall ensure the following two things:

• no entry for the normal user to become a collator, currently it's guarded by BaseCallFilter anyway, but we have to examine it after the filter is removed. (e.g. via MaxCandidates from pallet_collator_selection?) Ideally with integration-test.
• the order of 5 collation-related pallets (Authorship, CollatorSelection, Session, Aura, AuraExt) is desired or doesn't matter, see the discussion screenshot below and the comment in the v0.9.16 update. I did first step research and it appears to be the case.

It would be ideal if we can have documentation about the collator selection process for the session, not mandatory though.

// About the order of these 5 pallets, the comment in cumulus seems to be outdated.
//
// The main thing is Authorship looks for the block author (T::FindAuthor::find_author)
// in its `on_initialize` hook -> Session::find_author, where Session::validators() is enquired.
// Meanwhile Session could modify the validators storage in its `on_initialize` hook. If Session
// comes after Authorship, the changes on validators() will only take effect in the next block.
//
// I assume it's the desired behavior though or it doesn't really matter.
//
// also see the comment above `AllPalletsWithSystem` and

[Kailai-Wang]

The original substrate commit: https://github.com/paritytech/substrate/pull/10043

See the discussion in polkadot companion too: https://github.com/paritytech/polkadot/pull/4181

It seems that the change of the order (reversed -> normal) actually fixes the problem that the last block author in a session can't get the reward (unless it's elected again after session rotation)

[wangminqi]

The modification of this commit effects the decl_all_pallets function

However, no reorder of pallet should be made, since expand::expand_outer_inherent did not interact with AllPallets Reverse/non Reverse choice. In some extreme case, for instance, moonriver use inherent to recognize right block_author, the reorder of pallet might cause trouble (TBD).

We also have several collator related pallet with inherent setting. So I suggest using AllPalletsSystemReverse instead first. However, so far I see no evidence that this issue will bother us.

[wangminqi]

Back to general suggestion. In actual in_intialize hooks call. For our parachain now, pallet_authorship hook will call find_author which relies on pallet_aura and pallet_session. (pallet_babe has something to do with aura but it is not used for parachain) pallet_authorship hook will call EventHandler which relies on pallet_collator_selection and pallet_balances.

Except pallet_session, which if its' hook is not called before pallet_authorship, pallet_authorship will use the old validators set.

[Kailai-Wang]

Yea but as discussed in the polkadot companion, pallet_authorship just need that author to call note_author, which allocates the rewards to the block author. pallet_session could change the validator set in his on_intiialize in case of a session rotation.

If it comes before authorship, then the last block author of the previous session won't get any block reward, which I do think is a bug.

I don't think it will affect any of the production of blocks (this also matches what I tested).

[wangminqi]

Agree. Make sense.

[Kailai-Wang]

In the original substrate discussion thread it says:

NOTE: frame_executive::Executive will execute the hooks on_initialize, on_idle, on_runtime_upgrade, on_finalize, offchain_worker on the generic AllPallets, so the order should be reviewed for those execution.

[Kailai-Wang]

@wangminqi I just tried substrate, the inherent hooks are actually called in normal order (non-reversed), e.g. create_inherent hook in pallet-titmestamp is called prior to that in pallet-authorship, which matches the order in declaration.