Remove invulnerables form staking-async

[andreitrand]

The "staking-async" pallet has inherited the list of invulnerable validators from the "staking" pallet, but these are no longer used. We can therefore remove them, together with additional clean-up.

Closes #10135 (comment)

[sigurpol]

beside the usual pr_doc, grepping for MaxInvulnerables and set_invulnerables and invulnerables in general under staking-async still show quite few occurrences. Do we want to remove the invulnerable feature completely from the staking-async pallet? If yes, there is more cleanup needed in staking-async and in the runtimes.

It would be great to explain in the PR also if the feature was used in other runtimes (including polkadot, kusama under fellowship) and describe breaking changes there that will need to be addressed once integrated.

Do we also need to cleanup storage items once we remove Invulnerables?

(For some of these questions, surely @kianenigma and @Ank4n can help 😄 )

[kianenigma]

My previous understanding was that while we still mistakenly read Invulnerables (such as in slashing), I am pretty sure we never set it anywhere (other than in genesis), and even if we set it, it has no meaning other than "don't slash these guys". In other words, even if Invulnerables are set, they will not become active validators. So there might be many occurrences left, but I suspect most are a noop and won't break any tests etc.

If there are entanglements that we are not aware of, for example if a million test cases break, @andreitrand please share your findings first and then we can revise if it is sensible to continue with this.

[andreitrand]

/cmd prdoc

[andreitrand]

Based on the original issue I aimed to remove references to (type) Invulnerables, MaxInvulnerables and any related getters, setters (ex: fn set_invulnerables(...)) and tests (ex: fn invulnerables_are_not_slashed()) that appeared in the staking-async pallet only. To this end I made sure the following checks succeed:

cargo build --release
cargo test -p pallet-staking-async
cargo test -p pallet-staking-async --features runtime-benchmarks benchmark
cargo test -p pallet-staking
cargo test -p pallet-staking --features runtime-benchmarks benchmark
cargo check -p asset-hub-westend-runtime
cargo check -p pallet-staking-async
cargo check -p pallet-staking

Having said that, multiple references to type MaxInvulnerables and type Invulnerables remain as part of the collator pallet's Config which is referenced across multiple parachains in Cumulus. Multiple references to variables named invulnerables also remain, in Cumulus and other pallets as well.

Are there any other suitable test commands that can help me which of these are no longer relevant, given what I've removed so far?

[sigurpol]

Based on the original issue I aimed to remove references to (type) Invulnerables, MaxInvulnerables and any related getters, setters (ex: fn set_invulnerables(...)) and tests (ex: fn invulnerables_are_not_slashed()) that appeared in the staking-async pallet only. To this end I made sure the following checks succeed:

cargo build --release
cargo test -p pallet-staking-async
cargo test -p pallet-staking-async --features runtime-benchmarks benchmark
cargo test -p pallet-staking
cargo test -p pallet-staking --features runtime-benchmarks benchmark
cargo check -p asset-hub-westend-runtime
cargo check -p pallet-staking-async
cargo check -p pallet-staking

Having said that, multiple references to type MaxInvulnerables and type Invulnerables remain as part of the collator pallet's Config which is referenced across multiple parachains in Cumulus. Multiple references to variables named invulnerables also remain, in Cumulus and other pallets as well.

Are there any other suitable test commands that can help me which of these are no longer relevant, given what I've removed so far?

I have missed the initial discussion around this task and so probably @kianenigma and @Ank4n are better equipped to answer your question. Without knowing that context, I would naively say that we should aim to remove any reference to Invulnerables from staking-async and staking pallets and don't care about anything else (and just making sure to remove the related configuration in the different runtimes in this PR and then in the following runtime PR where we intregrate these changes). But again, let's hear first from Kian and Ankan

[Ank4n]

Based on the original issue I aimed to remove ....

@andreitrand Can you link to original issue/discussion? Afaik invulnerables are useful for chain at genesis, so I don't see much harm in keeping it.

[kianenigma]

type Invulnerables remain as part of the collator pallet's Config which is referenced across multiple parachains in Cumulus

Yes, this is just about pallet-staking-async, other pallets can/should have invulnerables.

@andreitrand Can you link to original issue/discussion? Afaik invulnerables are useful for chain at genesis, so I don't see much harm in keeping it.

There is no issue for it, it was a note in the project board only. The current code doesn't actually treat invulnerables as anything, other than excluding them in slashing. If they are set in genesis, they are not declared as active validators. So they are actually not useful for genesis setup either.

We could change invulnerables to be non-slashable, and be omni-present validators, but staking-async is IMO too complex to have this backdoor, even if it is for test. Instead of keeping it and fixing it, since there is no use for it and all tests/adjacent pallets work with properly setting up the initial era, I suggested that we remove it.

Moreover, I think the only reason to have this in the original staking pallet was to allow W3F to run the original validators of Polkadot in the PoA phase before public launch + cover their *** of they get slashed 😄

Given that our test setups are capable of working as-is, I don't see a good reason to keep it in the code. For example, there is no reason to have a test case that ensures someone cannot be slashed, because it is a non-action in production.

[kianenigma]

LGTM modulo one wrong update to collator selection + confirming that @Ank4n agrees this can be removed.

[Ank4n]

Revert substrate/frame/staking-async/runtimes/parachain/src/weights/pallet_collator_selection.rs as @kianenigma mentioned. Everything else looks good.

[paritytech-workflow-stopper]

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/19632558738
Failed job name: cargo-clippy

[sigurpol]

Small comment plus what @kianenigma / @Ank4n already mentioned.
Other than that, looks great!

[andreitrand]

Based on the original issue I aimed to remove ....

@andreitrand Can you link to original issue/discussion? Afaik invulnerables are useful for chain at genesis, so I don't see much harm in keeping it.

I've added a link to it in the PR description.

[andreitrand]

/cmd bench --pallet pallet_staking_async --runtime asset-hub-westend

[github-actions]

Command "bench --pallet pallet_staking_async --runtime asset-hub-westend" has started 🚀 See logs here

[github-actions]

Command "bench --pallet pallet_staking_async --runtime asset-hub-westend" has finished ✅ See logs here

Subweight results:

No changes found.

Command output:

✅ Successful benchmarks of runtimes/pallets:
-- asset-hub-westend: ['pallet_staking_async']

[Ank4n]

btw, the contribution guidelines asks us to not use force push. The commits are squashed and merged to master anyways so no need to rewrite branch history.