Replace free for all collation in cumulus runtimes

[georgepisaltu]

Partially fixes #103

This PR removes instances of "free for all" collation in the glutton, shell, and seedling runtimes and replaces them with Aura instances. Aura is configured without a session manager, so the initial authority set cannot be changed later on.

[rphmeier]

This doesn't seem to achieve the goals of #103.

1. Free-for-all collation is handled only on the client side and should not require anything in the runtime. It's not clear to me what the purpose of the new pallet is.
2. The glutton runtime is being updated to add both Aura and the new fixed pallet

[georgepisaltu]

This doesn't seem to achieve the goals of #103.

My understanding of #103 is that all runtimes running "free for all" (glutton, seedling and shell) should instead run with Aura.

Free-for-all collation is handled only on the client side and should not require anything in the runtime. It's not clear to me what the purpose of the new pallet is.

Aura needs a session manager, and from what I understand the collator-selection pallet is the only one providing this so far. The purpose of the fixed pallet (to be renamed) is to provide the session manager capability required by Aura but without all the dependencies of collator-selection (e.g. collator-selection requires balances and has rewards). It's just a dumbed down version of the collator-selection pallet: no rewards, no elections, no invulnerables or candidates. Also, there needs to be a session manager because if we have a defined set of collators and we want the ability to add/remove collators, we need discrete sessions.

However, I don't think I have the whole picture because I don't understand what the client side has to do with this specifically. As far as I understand, in runtimes such as glutton which run "free for all" collator selection, the block executor is the executive pallet, which means that users will call the related extrinsics in the executive pallet to actually produce blocks freely, from the client side. In other words, you don't have to be a collator to produce blocks because there is no such thing, since nobody is keeping track of a collator set. The introduction of Aura into the runtime as the block executor is supposed to change this by introducing a collator set. Producing a block will still be done with the same extrinsics, but now you have to be a collator in order to do so. Did I get anything wrong here?

The glutton runtime is being updated to add both Aura and the new fixed pallet

Yes, as Aura needs a session manager and the new fixed pallet provides this. The same needs to be done for seedling and shell.

[rphmeier]

My understanding of #103 is that all runtimes running "free for all" (glutton, seedling and shell) should instead run with Aura.

Got it. We discussed two things in #103 and one of them was to build a new free-for-all implementation, so I thought this might've been about that.

I think the fixed pallet might be overkill, at least for glutton and tick - pallet-aura is already meant to be self-contained, at least for an unchanging authority set. The testing parachains definitely don't need that, but shell chains might really need authority set changes, will defer to @joepetrowski.

[BradleyOlson64]

This PR doesn't include changes I expected in
cumulus/polkadot-parachain/src/command.rs -> run().

Particularly, I think we'd want to change the glutton runtime case to start_generic_aura_node rather than start_shell_node

[georgepisaltu]

I think the fixed pallet might be overkill, at least for glutton and tick - pallet-aura is already meant to be self-contained, at least for an unchanging authority set. The testing parachains definitely don't need that, but shell chains might really need authority set changes, will defer to @joepetrowski.

I spoke with @joepetrowski and it turns out we don't need to change the authority set in shell either, so I will drop the fixed pallet altogether and I'll move towards and impl where it's just Aura with no sessions.

[georgepisaltu]

This PR doesn't include changes I expected in cumulus/polkadot-parachain/src/command.rs -> run().

Particularly, I think we'd want to change the glutton runtime case to start_generic_aura_node rather than start_shell_node

I don't understand exactly what you mean, is it related to the other thing in #103? For reference:

Got it. We discussed two things in #103 and one of them was to build a new free-for-all implementation, so I thought this might've been about that.

I'm not familiar with the code you're pointing at, but judging by the name it seems a shell node would need to run Aura first in order to use start_generic_aura_node. To me it seems like the next step after the Aura switch.

[BradleyOlson64]

I'm not familiar with the code you're pointing at, but judging by the name it seems a shell node would need to run Aura first in order to use start_generic_aura_node.

The impression I get is that by changing to start_generic_aura_node you are no longer running a shell node at all. It seems that since free for all collation isn't supported with Async Backing, we may want to retire shell node entirely. Though I don't know enough about the features of shell vs generic aura to make that determination.

[BradleyOlson64]

Nice work

[joepetrowski]

Looks good, will approve once a few questions re async backing are answered. Comments on Glutton apply to all runtimes.

In cumulus_pallet_parachain_system::Config, I think you also need to change:

type CheckAssociatedRelayNumber = RelayNumberMonotonicallyIncreases;
type ConsensusHook = cumulus_pallet_aura_ext::FixedVelocityConsensusHook<
    Runtime,
    RELAY_CHAIN_SLOT_DURATION_MILLIS,
    BLOCK_PROCESSING_VELOCITY,
    UNINCLUDED_SEGMENT_CAPACITY,
>;

[joepetrowski]

I think this should be OK but maybe @Sophia-Gold can confirm. If OK then remove the TODO.

[Sophia-Gold]

What's wrong with SLOT_DURATION / 2? I've used this in the upgraded tick and it works fine. Otoh, one would think 0 would be okay as well. I don't totally understand this, but anything less than SLOT_DURATION would seem to work.

[georgepisaltu]

There's nothing wrong with SLOT_DURATION / 2, let's leave it as is. I will remove the TODO.

[joepetrowski]

Changing indices is generally a no-no. Probably fine for Glutton since we don't have any in the wild. But what is the reason for this change?

[georgepisaltu]

I was shuffling them around while building the runtime and I didn't check the original indices when I posted the PR. Good catch, restored it now.

[georgepisaltu]

Looks good, will approve once a few questions re async backing are answered. Comments on Glutton apply to all runtimes.

In cumulus_pallet_parachain_system::Config, I think you also need to change:

type CheckAssociatedRelayNumber = RelayNumberMonotonicallyIncreases;
type ConsensusHook = cumulus_pallet_aura_ext::FixedVelocityConsensusHook<
    Runtime,
    RELAY_CHAIN_SLOT_DURATION_MILLIS,
    BLOCK_PROCESSING_VELOCITY,
    UNINCLUDED_SEGMENT_CAPACITY,
>;

From the documentation of those items, I think you're right and it should be the way you describe above. I updated the config, PTAL.