Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Properly prevent slashing during offchain phragmén election window. #5188

Closed
kianenigma opened this issue Mar 9, 2020 · 3 comments
Closed
Assignees
Labels
I4-annoyance The client behaves within expectations, however this “expected behaviour” itself is at issue.

Comments

@kianenigma
Copy link
Contributor

kianenigma commented Mar 9, 2020

the problem is brief: no exposure shall change while staking is accepting and processing solutions for the next era.

with the current proposed solutions at #4517 and #5151, we do this at the offences module, preventing further queuing complexity to be leaked into staking. Ideally though, this logic isn't really pertaining to the offences module and should live in staking (or even elsewhere, open to discussion).

Implementing this in Staking would be rather involved since there is already a parameter there for SlashDeferDuration.

Additionally, an update to this should make sure in all storage items of Offences, we don't store duplicate IdentificationTuples. They resolve to Exposure which can be very big and unnecessary to store per validator in case of multiple offences, since they are the same. See #5151 (comment) for more detail.

@kianenigma kianenigma added the I4-annoyance The client behaves within expectations, however this “expected behaviour” itself is at issue. label Mar 9, 2020
@kianenigma kianenigma linked a pull request Mar 9, 2020 that will close this issue
12 tasks
@kianenigma kianenigma removed a link to a pull request Mar 9, 2020
12 tasks
@kianenigma
Copy link
Contributor Author

Some dumps from a chat offline with @tomusdrw and @rphmeier:

there is another side issue as well: we currently dump all validators in a Vec (called snapshot henceforth) before election and the solution uses indices into this vector. If a misbehaviour is reported, then a validator gets chilled and hence should not be counted for the next election. But with this index system, we don't check this. for now I assume strictly that the snapshot is valid. Otherwise if you want to check that each validator_index is correct, this really kills a big chunk of the benefits of creating this snapshot in the first place. Although we could do it.

Let's say we assume slashing in election window, and a slashing happens. Aside from the situation that I denoted above, we have a few situations:

  • if the solution does not respect the rule that says "nominators of a recently slashed validator shold be filtered", then it is rejected.
  • if it respects this rule, or:
  • even simpler, the slashed validator has no nomination, then we are safe, since (I must have said this before) solutions do not encode the final exposure/stake value or any other value of any sort that resonates with balance (since it is too big to transmit). Instead, the solution encodes the ratios of nominator stakes being distributed to validators.

so: a solution, at the time of submission (not generation -- these can be a few blocks apart) will always create a valid exposure. please verify though.

any slashing after solution has already been submitted and queued will f*** sh** up if the slashed validator is in the winners list.

I am basically focusing on this note from Rob:

As long as Exposure is always calculated correctly I don't worry about accounting being broken.

and what we can do is perhaps:

  • invalidate a solution if a slashing happens and if it overlaps with one of the queued winner solutions.
  • improve our offchain worker code to detect this and try and resubmit a new solution
    indeed there's a way to annoy the system here by causing cheap misbehaviours but as mentioned it is not free, so it might be a good alternative to the current approach.

I think the final solution that I named above is a good way to go about this.

@rphmeier
Copy link
Contributor

rphmeier commented May 6, 2020

a solution, at the time of submission (not generation -- these can be a few blocks apart) will always create a valid exposure. please verify though.

If I understand you correctly, yes. A sequence of events generate -> slash -> submit may violate the nomination-filtering rule, but if it does not, then it will probably just be a worse solution than intended. Since the nomination-filtering rule is checked during submission, nothing to worry about there.

and what we can do is perhaps:

invalidate a solution if a slashing happens and if it overlaps with one of the queued winner solutions.
improve our offchain worker code to detect this and try and resubmit a new solution
indeed there's a way to annoy the system here by causing cheap misbehaviours but as mentioned it is not free, so it might be a good alternative to the current approach.

Yup, I think invalidating a solution if it overlaps with a slash submitted afterwards will work fine. I think you meant this, but I'll clarify that overlap in slashed nominators, not just in validators, should be considered just to have it on the record.

Like you said, the fact that it's not free means that we've got a non-zero griefing factor which means that we have room to tweak fees if this ends up being a probem in practice, but I doubt it will be.

@kianenigma
Copy link
Contributor Author

not relevant anymore.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I4-annoyance The client behaves within expectations, however this “expected behaviour” itself is at issue.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants