Validator Re-Enabling

[Overkillus]

Aims to implement Stage 3 of Validator Disbling as outlined here: #4359

Features:

• New Disabling Strategy (Staking level)
• Re-enabling logic (Session level)
• More generic disabling decision output
• New Disabling Events

Testing & Security:

• Unit tests
• Mock tests
• Try-runtime checks
• Try-runtime tested on westend snap
• Try-runtime CI tests
• Re-enabling Zombienet Test (?)
• SRLabs Audit

Closes #4745
Closes #2418

[tdimitrov]

I had a quick pass by focusing mainly on the approach. It looks good, nice work @Overkillus!

I've left some thoughts about a corner case with the re-enabling.

[Overkillus]

bot fmt

[command-bot]

@Overkillus https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7692315 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 15-7908d2d9-da54-4b6e-af31-1b138de1c56d to cancel this command or bot cancel to cancel all commands in this pull request.

[command-bot]

@Overkillus Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7692315 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7692315/artifacts/download.

[Ank4n]

Is it possible to move most of the disabling logic to pallet_session?

It seems staking does not really need to know anything about disabling. Currently, we are maintaing two copies of disabled validator indices in both pallet_session and pallet_staking, apparently because staking only knows about era, but I am not sure what does that mean. Staking does not have any concept of disable in it, and it should only care about slashing. If the concern is that we want to keep a validator disabled for the whole era, staking returns a set of validators only for sessions that trigger an era. For other sessions, it simply returns None. So session should know when to clear disabled validators.

There is a larger reason as to why I am flagging this. With staking moving to AH, the offence lifecycle would be something like this:

• Offense is reported and verified on RC.
• Any disable logic applied and propagated by pallet_session. It should also keep track of SlashPerbill in its DisabledValidators store.
• An offence report (Offender, Session, SlashPerbill) is sent to AH for slashing.

In practice, pallet_staking will be replaced by pallet_staking_client (or a more suitable name) that will fill in the gaps. That is, it will act as OnOffenceHandler and SessionManager on RC, and async communicate with Staking/AH.

tl;dr: Given that Session and Staking communication would become async, this disabling logic doesn’t seem compatible or, at the very least, "good design" with that in mind.

[Overkillus]

@Ank4n

Is it possible to move most of the disabling logic to pallet_session?

(assuming we want to do this)

I will discuss if this is a good idea later below but even if we assume we want to do this I don't believe it is appropriate to do this in this PR. This PR only aims to, with minimal changes and refactors, simply allow for validator re-enabling using all the previous already in-place logic. I want to make sure that this PR is minimal and adheres to what was already pre-agreed in the design document for disabling, to limit audit time and just be more sure that everything does exactly what we think it does.

If we have plans to refactor the staking pallet (which as you mentioned will be done anyway during the port to AH) then it should be done in a separate primarily refactor/port PR.

---

Is it possible to move most of the disabling logic to pallet_session?

(answering if we want to do this)

Staking pallet as it stands is responsible for more than just slashing. It holds important params with regards to the active validator set. For instance min, ideal, and max validator counts. What it means to be disabled is not within the purview of the staking pallet, what it does is simply keeps track of highest offenders up to some limit and makes this info available for others. This is highly customisable and different users of this pallet can add new strategies with new limits.

Since we keep those validators for an era it makes sense to keep track of their offences (potential disabling status) also for a full era. Storing this information in session when we aim to keep it for era seems like an unintuitive approach.

I might agree that we could make it less opinionated by moving disabling strategies to session and instead keeping a history of all offences (unfiltered by disabling strategies) in staking. Nevertheless in this approach we still should keep track of all offences in staking. An offence is more than a slash, there might by other byproducts or consequences and we need to allow for them.

In the new pipeline for offences that you suggested, where do you think a history of offences within an era should be stored?

Session - Staking coupling

It is okay to keep staking bounded to session, but session should not be dependant on staking. Adding logic to session that is actually era-wide in scope through hacky reads of signals from staking seems well, hacky. This information should be kept in a context that is explicitly era-wide and shared with session.

TLDR: This might be a needed change but is outside of the scope of this PR and some of that information should still live in era-wide and not session-wide scopes.

[Ank4n]

@Overkillus

I wouldn't try to block you if you want to go ahead with this PR, especially since most of the things I flagged should ideally had been flagged with the earlier PRs, and this PR does not introduce a new design decision. That said:

I will discuss if this is a good idea later below but even if we assume we want to do this I don't believe it is appropriate to do this in this PR.

Since this PR touches the logic that we know we will have to migrate in next couple of months, it is reasonable enough enough to make those changes in this PR.

Staking pallet as it stands is responsible for more than just slashing. It holds important params with regards to the active validator set. For instance min, ideal, and max validator counts. What it means to be disabled is not within the purview of the staking pallet, what it does is simply keeps track of highest offenders up to some limit and makes this info available for others. This is highly customisable and different users of this pallet can add new strategies with new limits.

I believe you only require current count of active validator set. Could you point me any param that you need specifically from pallet_staking for disabling? The SlashPerbill is decided based on offense (also outside pallet_staking) and can be tracked in storage for disabled validators in pallet_session as well. I don't see it makes any difference.

In the new pipeline for offences that you suggested, where do you think a history of offences within an era should be stored?
It is okay to keep staking bounded to session, but session should not be dependant on staking. Adding logic to session that is actually era-wide in scope through hacky reads of signals from staking seems well, hacky. This information should be kept in a context that is explicitly era-wide and shared with session.

Why does offence or disabling has to relate with an era? pallet_session can use variety of ways to manage this.

• Only clear disabled validator when underlying economic condition changes, i.e. when it receives a new set of validators.
• Any time validator is disabled, keep them disabled for the next x sessions. x can be 6.

I think we have already tried to hack around with the concept of Era and Session, and having to maintain DisabledValidators in two places is a code smell stemming from that. If we look at pallet-staking independently, disabled in its context makes no meaningful sense. Another code smell is that the disable logic right now is spread between staking and session, with session acting like a proxy that receives disabled validators from staking and passes it to the session handlers.

IMO, this is a flawed design, in addition to the fact that it also doesn’t fit well with the post-AH migration setup.

[Overkillus]

@Ank4n

I wouldn't try to block you if you want to go ahead with this PR, especially since most of the things I flagged should ideally had been flagged with the earlier PRs, and this PR does not introduce a new design decision.

That is what I would suggest. This impl is simply a part of an earlier design which was pre-approved by SRLabs in the design's audit. It just adds some new functionality using the same infrastructure without altering it too much.

Since this PR touches the logic that we know we will have to migrate in next couple of months, it is reasonable enough enough to make those changes in this PR.

We have the power to open multiple PRs to separate unrelated changes between the PRs. Enabling new functionality in the current design should not come with a major refactor if it is not neccesary. I am open to the refactor at a later stage but nevertheless it should be a separate PR.

This PR does not make the future refactor harder or easier. I would understand withholding the change if it made the refactor harder. It does not make it harder and is orthogonal.

On top of all of that this PR is a security-related fix. It has a higher priority than a genral refactor and should not be budled with it without a good reason. It might only obfuscate the auditing process.

The rest of the discussion dives into why the refactor might be a good idea later on which we should separate into a separate issue or ticket. I'd be happy to participate in those discussions and help as much as I can.

[tdimitrov]

We have the power to open multiple PRs to separate unrelated changes between the PRs. Enabling new functionality in the current design should not come with a major refactor if it is not neccesary. I am open to the refactor at a later stage but nevertheless it should be a separate PR.

I agree with @Overkillus here. Considering that this PR is part of the disabling strategy roll out I am strongly against doing any out of scope re-factorings.

What @Ank4n suggests definitely makes sense and we should do it but as a separate effort. Let's focus on the disabling strategy in this PR.

[sandreim]

Since this PR touches the logic that we know we will have to migrate in next couple of months, it is reasonable enough enough to make those changes in this PR.

Given that this PR doesn't make it harder to do the migration in the future, your suggestion is best handled as separate task/PR. In general I think it is not a good idea to require unrelated refactorings in the scope of a change that is concerned with security.

@Ank4n please take another look and if there are no other causes of concern I would say we should merge. We want to get this change in production as soon as possible.

[Ank4n]

@tdimitrov @sandreim The refactoring needed is not unrelated. But blocking also serves no purpose, and we can handle this in a followup issue.

@Overkillus Could you check if its possible to get rid of the storage item DisabledValidators in pallet-staking and only maintain it in pallet-session (even if it results in a bit messy code)? This would make the followup refactor noop. Otherwise, I'm ready to approve.

@gpestana You may want to take a quick look at the changes as well.

[Overkillus]

Added all the defensive suggestions from @Ank4n Good eye for spotting them, thanks!

Could you check if its possible to get rid of the storage item DisabledValidators in pallet-staking and only maintain it in pallet-session (even if it results in a bit messy code)?

This is not trivial. 99% of the disabling logic lives in staking so moving it over is not easy and this is a major part of the needed refactor.