staking: avoid proportional slashing leak dust into chunks that should not be slashed #12058

NingLin-P · 2022-08-17T11:13:17Z

Attempt to fix #11810

This PR replaces the slash ratio with the remaining ratio, since calculations are rounded down, the slash_from_target calculated from the remaining ratio will be a bit more than the expected proportion instead of less, thus fixing the issue. This PR also comes with a little refactoring.

polkadot address: 1CWyso9Zw46QdR54sTvKJXTUdmZznYZa2aJfNHdG6xQ6L71

Signed-off-by: linning <linningde25@gmail.com>

NingLin-P · 2022-08-17T11:13:48Z

@kianenigma PTAL

paritytech-cicd-pr · 2022-08-17T11:18:32Z

The CI pipeline was cancelled due to failure one of the required jobs.
The job name - test-linux-stable
The job logs - https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1752357

Signed-off-by: linning <linningde25@gmail.com>

kianenigma · 2022-08-18T06:43:02Z

frame/staking/src/lib.rs

+		// zero otherwise. In both cases, we slash first `self.active`, and then
+		// `slash_chunks_priority`.
+		let (remaining_ratio, slash_chunks_priority) = {
+			let mut affected_balance = BalanceOf::<T>::zero();


I don't quite agree with the changes here, like let mut first_chunk = None; and assigning to it in the loop. This is actually not Rust-idiomatic at all, and the previous if let Some(_) = iterator {} else {} is more preferred.

I thought using a single loop to get first_index and affected_balance was more efficient (less iteration) and cleaner (less if else branch), but indeed hand code the functionality that the Iterator API already provided is not Rust-idiomatic.

kianenigma · 2022-08-18T06:43:03Z

frame/staking/src/lib.rs

@@ -611,21 +621,19 @@ impl<T: Config> StakingLedger<T> {
 			slash_era,
 			self,
 			slash_chunks_priority,
-			maybe_proportional,
+			Perquintill::one() - remaining_ratio,


This should be safe math. Or, yet again, I want to complain that the refactor is more of step backwards, since Option is the prefect type to show a maybe_it_is_proportional, instead of some value being 0.

The intention was to unify both cases (proportional or not) under a ratio, proportional or not just a matter of different ratio, but since it is important to get clear of these cases this indeed may not be a good idea

kianenigma

Right direction and we can fix this, but please:

The refactors are a bit opinionated, let's keep this PR limited only to fixing the rounding down (unless if you want to try a different approach at refactoring the code)
As a simpler approach, please use the same logic as before, but use Rounding::Up when multiplying the Prequintill.

The combination of these two should make the diff of this PR very small, which makes it way easier to merge, since it is a very sensitive code. Thanks!

NingLin-P · 2022-08-18T08:50:24Z

Thanks for the hint @kianenigma !

As a simpler approach, please use the same logic as before, but use Rounding::Up when multiplying the Prequintill.

I found there may have two places need to change to Rounding::Up:

substrate/frame/staking/src/lib.rs

Line 595 in e7aa858

let ratio = Perquintill::from_rational(slash_amount, affected_balance);

substrate/frame/staking/src/lib.rs

Line 619 in e7aa858

ratio * (*target)

the first place may have more impact to the result, it can changing to Perquintill::from_rational_with_rounding but which return a Result I'm not quite sure the proper way to handle the error here. The second place can easily changing to ratio.mul_ceil(*target) though.

kianenigma · 2022-08-19T09:36:36Z

You should do what normal from_rational is doing:

substrate/primitives/arithmetic/src/per_things.rs

Line 332 in 0ebeac4

    
           Self::from_rational_with_rounding(p, q, Rounding::Down).unwrap_or_else(|_| Self::one())

Signed-off-by: linning <linningde25@gmail.com>

paritytech-cicd-pr · 2022-08-19T10:54:38Z

The CI pipeline was cancelled due to failure one of the required jobs.
The job name - cargo-check-benches
The job logs - https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1757628

paritytech-cicd-pr · 2022-08-19T10:54:38Z

The CI pipeline was cancelled due to failure one of the required jobs.
The job name - cargo-check-benches
The job logs - https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1757627

Signed-off-by: linning <linningde25@gmail.com>

frame/nomination-pools/test-staking/src/lib.rs

kianenigma · 2022-08-21T07:05:49Z

frame/staking/src/tests.rs

@@ -5233,26 +5233,12 @@ fn proportional_ledger_slash_works() {
 	ledger.active = unit;
 	ledger.total = unit * 4 + value;
 	// When
-	assert_eq!(ledger.slash(slash, 0, 0), slash - 43);
+	assert_eq!(ledger.slash(slash, 0, 2), slash);


Please add this test case and make sure it is reasonable as well: bdd1399

Ideally, existing tests should not be changed at all.

Not sure why you changed this test?

Oh sorry, I thought the original case was subject to the round down problem and changed it to meet the case not affected chunk should not be slashed (like bdd1399), should not changing it and add a new case instead.

Signed-off-by: linning <linningde25@gmail.com>

frame/nomination-pools/test-staking/src/lib.rs

frame/staking/src/tests.rs

kianenigma

LGTM, except the small question about tests.

Signed-off-by: linning <linningde25@gmail.com> Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

frame/nomination-pools/test-staking/src/lib.rs

ruseinov

seems good!

Signed-off-by: linning <linningde25@gmail.com> Co-authored-by: Roman Useinov <roman.useinov@gmail.com>

kianenigma · 2022-09-06T07:03:30Z

bot rebase

paritytech-processbot · 2022-09-06T07:03:38Z

Rebased

kianenigma · 2022-09-06T12:24:23Z

bot merge

…d not be slashed (paritytech#12058) * replace slash ratio with remaining ratio Signed-off-by: linning <linningde25@gmail.com> * little refactor Signed-off-by: linning <linningde25@gmail.com> * fix test Signed-off-by: linning <linningde25@gmail.com> * fix typo Signed-off-by: linning <linningde25@gmail.com> * revert refactor Signed-off-by: linning <linningde25@gmail.com> * rounding up instead of remaining ratio Signed-off-by: linning <linningde25@gmail.com> * address comment Signed-off-by: linning <linningde25@gmail.com> * Update frame/nomination-pools/test-staking/src/lib.rs Signed-off-by: linning <linningde25@gmail.com> Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * Update frame/nomination-pools/test-staking/src/lib.rs Signed-off-by: linning <linningde25@gmail.com> Co-authored-by: Roman Useinov <roman.useinov@gmail.com> Signed-off-by: linning <linningde25@gmail.com> Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Co-authored-by: Roman Useinov <roman.useinov@gmail.com> Co-authored-by: parity-processbot <>

NingLin-P added 2 commits August 17, 2022 17:48

replace slash ratio with remaining ratio

ad221be

Signed-off-by: linning <linningde25@gmail.com>

little refactor

10af234

Signed-off-by: linning <linningde25@gmail.com>

NingLin-P requested a review from kianenigma as a code owner August 17, 2022 11:13

NingLin-P mentioned this pull request Aug 17, 2022

Rewrite proportional slashing to to not leak dust into chunks that should not be slashed. #11810

Closed

NingLin-P added 2 commits August 17, 2022 23:45

fix test

18abe59

Signed-off-by: linning <linningde25@gmail.com>

fix typo

bc9c855

Signed-off-by: linning <linningde25@gmail.com>

kianenigma reviewed Aug 18, 2022

View reviewed changes

kianenigma suggested changes Aug 18, 2022

View reviewed changes

NingLin-P added 2 commits August 19, 2022 18:37

revert refactor

b8c06e1

Signed-off-by: linning <linningde25@gmail.com>

rounding up instead of remaining ratio

98800a7

Signed-off-by: linning <linningde25@gmail.com>

resolve conflict

e92a37a

Signed-off-by: linning <linningde25@gmail.com>

kianenigma reviewed Aug 21, 2022

View reviewed changes

frame/nomination-pools/test-staking/src/lib.rs Outdated Show resolved Hide resolved

kianenigma reviewed Aug 21, 2022

View reviewed changes

address comment

79001c1

Signed-off-by: linning <linningde25@gmail.com>

kianenigma added D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited and removed D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Aug 28, 2022

kianenigma reviewed Aug 29, 2022

View reviewed changes

frame/nomination-pools/test-staking/src/lib.rs Outdated Show resolved Hide resolved

kianenigma reviewed Aug 29, 2022

View reviewed changes

frame/staking/src/tests.rs Show resolved Hide resolved

kianenigma approved these changes Aug 29, 2022

View reviewed changes

Update frame/nomination-pools/test-staking/src/lib.rs

98f41ea

Signed-off-by: linning <linningde25@gmail.com> Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

kianenigma requested review from ruseinov, shawntabrizi and ggwpez September 5, 2022 08:16

ruseinov reviewed Sep 5, 2022

View reviewed changes

frame/nomination-pools/test-staking/src/lib.rs Outdated Show resolved Hide resolved

ruseinov approved these changes Sep 5, 2022

View reviewed changes

Update frame/nomination-pools/test-staking/src/lib.rs

9291ef2

Signed-off-by: linning <linningde25@gmail.com> Co-authored-by: Roman Useinov <roman.useinov@gmail.com>

Merge remote-tracking branch 'origin/master' into refactor-slash

c30eecf

paritytech-processbot bot merged commit dcfaa41 into paritytech:master Sep 6, 2022

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 Sep 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

staking: avoid proportional slashing leak dust into chunks that should not be slashed #12058

staking: avoid proportional slashing leak dust into chunks that should not be slashed #12058

NingLin-P commented Aug 17, 2022 •

edited

Loading

NingLin-P commented Aug 17, 2022

paritytech-cicd-pr commented Aug 17, 2022

kianenigma Aug 18, 2022

NingLin-P Aug 18, 2022

kianenigma Aug 18, 2022

NingLin-P Aug 18, 2022

kianenigma left a comment

NingLin-P commented Aug 18, 2022

kianenigma commented Aug 19, 2022

paritytech-cicd-pr commented Aug 19, 2022

paritytech-cicd-pr commented Aug 19, 2022

kianenigma Aug 21, 2022

kianenigma Aug 21, 2022

NingLin-P Aug 21, 2022 •

edited

Loading

kianenigma left a comment

ruseinov left a comment

kianenigma commented Sep 6, 2022

paritytech-processbot bot commented Sep 6, 2022

kianenigma commented Sep 6, 2022

staking: avoid proportional slashing leak dust into chunks that should not be slashed #12058

staking: avoid proportional slashing leak dust into chunks that should not be slashed #12058

Conversation

NingLin-P commented Aug 17, 2022 • edited Loading

NingLin-P commented Aug 17, 2022

paritytech-cicd-pr commented Aug 17, 2022

kianenigma Aug 18, 2022

Choose a reason for hiding this comment

NingLin-P Aug 18, 2022

Choose a reason for hiding this comment

kianenigma Aug 18, 2022

Choose a reason for hiding this comment

NingLin-P Aug 18, 2022

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

NingLin-P commented Aug 18, 2022

kianenigma commented Aug 19, 2022

paritytech-cicd-pr commented Aug 19, 2022

paritytech-cicd-pr commented Aug 19, 2022

kianenigma Aug 21, 2022

Choose a reason for hiding this comment

kianenigma Aug 21, 2022

Choose a reason for hiding this comment

NingLin-P Aug 21, 2022 • edited Loading

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

ruseinov left a comment

Choose a reason for hiding this comment

kianenigma commented Sep 6, 2022

paritytech-processbot bot commented Sep 6, 2022

kianenigma commented Sep 6, 2022

NingLin-P commented Aug 17, 2022 •

edited

Loading

NingLin-P Aug 21, 2022 •

edited

Loading