consider removing liveness slashing

[ordian]

That is to remove im-online pallet and give up slashes for offline-ness.

Ideally we'd want to unify our slashing and disabling strategy and being offline has been mostly an operational issue, not a coordinated malicious attack. Missing out rewards should be enough of a punishment for being offline.

cc @burdges

[burdges]

We originally slashed for being off-line just to wake up inactive validators, who predated the chain doing anything. It's good to remove it now. We could wait for approval rewards to remove it I guess, but likely good to remove it sooner.

We should've liveness tests I think, but without slashing. We'd kinda thought grandpa voting sufficed, but maybe approvals should listen to grandpa's liveness. We can discuss this another time, since it'll never involve slashing.

[ordian]

We could make the removal process two-stage:

1. First, we modify the slashing fraction here to return 0.
2. Then we propose to remove the pallet from the runtimes (Kusama and Polkadot) along with any required migration.

The pallet seems to have some on-chain and off-chain storage which needs to be cleaned up in a migration. Also this pallet defines a session key which will no longer be needed, not sure about implications for a migration.

[andresilva]

Also this pallet defines a session key which will no longer be needed, not sure about implications for a migration.

We'll also need a migration to handle the new session keys format (i.e. removal of im-online key from the struct). It's simple though, same thing as is done here: https://github.com/polkadot-fellows/runtimes/blob/7bd91e826e4b2d95f6d2ee7f6bb0f0f47d095859/relay/kusama/src/lib.rs#L1706-L1714

[bkchr]

We could make the removal process two-stage:\n\nFirst, we modify the slashing fraction here to return 0.\nThen we propose to remove the pallet from the runtimes (Kusama and Polkadot) along with any required migration.

Why do we need to do this in a two stage process?

[ordian]

We don't need to, but considering that writing and testing all the migrations is non-trivial and there's a backlog of changes making it's way into fellowship runtimes, modifying the slashing fraction to 0 would ensure we get to the desired effect quickly.

[bkchr]

We don't need to, but considering that writing and testing all the migrations is non-trivial and there's a backlog of changes making it's way into fellowship runtimes, modifying the slashing fraction to 0 would ensure we get to the desired effect quickly.

Do we NEED this effect quickly? Why not just do it properly from the beginning... I mean writing this SessionKey migration is no magic and also doesn't require that much work or whatever.

[ordian]

I'm just worried it's going to take several months to land this change (in fellowship and then to enact on-chain) if we don't merge a simple PR now. Which might delay the rollout of disabling as well (although we don't currently view this is a hard prerequisite, more like a nice-to-have, but this might change). But it’s not a hill I’m willing to die on.

[bkchr]

I'm just worried it's going to take several months to land this change (in fellowship and then to enact on-chain) if we don't merge a simple PR now.

If we write the migration now, it will take the same amount as getting the "patched" pallet on chain. The next runtime upgrade will be prepared hopefully in the next weeks, enough time to directly add the fix to the fellowship. Writing the initial migration for removing the session key is done in < 1 hour. Removing the old state/offchain state can come later.

[s0me0ne-unkn0wn]

I mean writing this SessionKey migration is no magic and also doesn't require that much work or whatever.

It's not only about the session key... Please have a look at this draft in your spare time: #2265

[bkchr]

Removing the old state/offchain state can come later.

I already said this.

[s0me0ne-unkn0wn]

I already said this.

Oh sorry, missed that. Yes, that does make sense. Removing on-chain storage is not a big deal as well IIUC (it looks like a single frame_support::migrations::RemovePallet migration to me), so we can proceed with session keys migration AND on-chain storage removal, but leave off-chain storage cleanup for a follow-up, am I right?

[bkchr]

so we can proceed with session keys migration AND on-chain storage removal, but leave off-chain storage cleanup for a follow-up, am I right?

Yeah that sounds good to me!

[Overkillus]

With runtime release 1.2 liveness offences should no longer occur.

As seen in this PR removing ImOnline added to the 1.2 runtime release as seen here.

Some residual aspects of ImOnline will be further removed over time but they do not affect disabling. Issue closing.

[ordian]

I'm just worried it's going to take several months to land this change (in fellowship and then to enact on-chain) if we don't merge a simple PR now.

Who would have thought :P