-
Notifications
You must be signed in to change notification settings - Fork 700
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Fix|NominationPools] Only allow apply slash to be executed if the slash amount is atleast ED #6540
base: master
Are you sure you want to change the base?
Conversation
@@ -2989,7 +2993,7 @@ pub mod pallet { | |||
|
|||
let who = ensure_signed(origin)?; | |||
let member_account = T::Lookup::lookup(member_account)?; | |||
Self::do_apply_slash(&member_account, Some(who))?; | |||
Self::do_apply_slash(&member_account, Some(who), true)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want to to avoid slashing below the ED here (pub fn apply_slash
)?
I don't think this is an issue, slash are applied for rare circumstances no? |
@@ -2300,7 +2302,7 @@ pub mod pallet { | |||
|
|||
let slash_weight = | |||
// apply slash if any before withdraw. | |||
match Self::do_apply_slash(&member_account, None) { | |||
match Self::do_apply_slash(&member_account, None, false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My previous comment only made sense when taken with another made to this line, which asked: why don't we want to enforce the ED checks here?
Sorry, I had the GH open for a while, it might have been lost when I refreshed the browser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point, a gas fee is charged for slashing. Additionally, we need to clean up all bookkeeping before a member withdraws.
Single slash can affect unbounded number of pool members. The slashes are applied per member and if that is too small, is it worth to provide free compute for it? |
This change prevents
pools::apply_slash
from being executed when the pending slash amount of the member is lower than the ED.The issue came to light with the failing benchmark test in Kusama. The problem arises from the inexact conversion between points and balance. Specifically, when points are converted to balance and then back to points, rounding can introduce a small discrepancy between the input and the resulting value. This issue surfaced in Kusama due to its ED being different from Westend and Polkadot (1 UNIT/300), making the rounding issue noticeable.
This fix is also significant because applying a slash is feeless and permissionless. Allowing super small slash amounts to be applied without a fee is undesirable. With this change, such small slashes will still be applied but only when member funds are withdrawn.