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

Properly defer slashes #11823

Merged
merged 9 commits into from
Jul 26, 2022
Merged

Properly defer slashes #11823

merged 9 commits into from
Jul 26, 2022

Conversation

kianenigma
Copy link
Contributor

@kianenigma kianenigma commented Jul 12, 2022

(assuming polkadot context, where slash deferring is 27 eras and bonding duration is 28).
A slash can happen at any given era s, and can be reported at any given era r, as long as s + 28 > r >= s. This is because we must assume that even if a slash is reported later, the offender knows about it, and they will attempt to unbond and leave the system. Therefore, we must apply the slash latest at the beginning of era s + 28, otherwise the offender may have fully unbonded, leaving the slash incomplete, and posing a fundamental risk to the economic security of the chain.

The old logic would defer a slash reported at r to r + 27, and this seems clearly wrong to me. This would easily allow an offence that is being reported with a delay to evaded entirely. Instead a slash should be deferred to s + 27, i.e. relative to when it occurred, not when it was reported.

  • Polkadot PR to add migration.

@kianenigma kianenigma added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jul 12, 2022
@kianenigma kianenigma marked this pull request as ready for review July 12, 2022 16:17
@github-actions github-actions bot added A3-in_progress Pull request is in progress. No review needed at this stage. A0-please_review Pull request needs code review. and removed A0-please_review Pull request needs code review. A3-in_progress Pull request is in progress. No review needed at this stage. labels Jul 12, 2022
frame/staking/src/pallet/impls.rs Show resolved Hide resolved
frame/staking/src/pallet/impls.rs Outdated Show resolved Hide resolved
frame/staking/src/pallet/impls.rs Outdated Show resolved Hide resolved
frame/staking/src/pallet/impls.rs Show resolved Hide resolved
frame/staking/src/tests.rs Outdated Show resolved Hide resolved
frame/staking/src/tests.rs Outdated Show resolved Hide resolved
frame/staking/src/tests.rs Show resolved Hide resolved
@kianenigma kianenigma requested a review from ggwpez July 20, 2022 12:42
@kianenigma
Copy link
Contributor Author

@shawntabrizi @ggwpez would be good if you take a look here sometime soon.

@kianenigma
Copy link
Contributor Author

bot rebase

@paritytech-processbot
Copy link

Rebased

@ruseinov ruseinov self-requested a review July 26, 2022 08:46
Copy link
Contributor

@ruseinov ruseinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really into the code but looks good.

slash_era + slash_defer_duration + 1,
);
<Self as Store>::UnappliedSlashes::mutate(
slash_era.saturating_add(slash_defer_duration).saturating_add(One::one()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe pull the slash_era.saturating_add(slash_defer_duration).saturating_add(One::one()) out and use it in the log above as well?
There should also be saturating_inc.

@kianenigma
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit fdfdc73 into master Jul 26, 2022
@paritytech-processbot paritytech-processbot bot deleted the kiz-fix-staking-slash branch July 26, 2022 12:44
@kianenigma
Copy link
Contributor Author

This PR is assuming that there are no existing pending slashes in storage. If there are, they might become prematurely executed (or other behavior). Something to look into.

@kianenigma
Copy link
Contributor Author

aghhh shit, this would probably allow you to evade a slash, since in the old logic we write to a key in UnappliedSlashes that is no longer being checked.

Options:

  1. We add a migration code such that we apply any pending slashes one last time before doing the runtime upgrade.
  2. (that seems to be it, any other option would be risky)

@jakoblell jakoblell added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Aug 23, 2022
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
* initial draft of fixing slashing

* fix test

* Update frame/staking/src/tests.rs

Co-authored-by: Piotr Mikołajczyk <piomiko41@gmail.com>

* last touches

* add more detail about unbonding

* add migration

* fmt

Co-authored-by: Piotr Mikołajczyk <piomiko41@gmail.com>
Co-authored-by: parity-processbot <>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* initial draft of fixing slashing

* fix test

* Update frame/staking/src/tests.rs

Co-authored-by: Piotr Mikołajczyk <piomiko41@gmail.com>

* last touches

* add more detail about unbonding

* add migration

* fmt

Co-authored-by: Piotr Mikołajczyk <piomiko41@gmail.com>
Co-authored-by: parity-processbot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants