From bf20a9ee18f7215210bbbabf79e955c8c35b3360 Mon Sep 17 00:00:00 2001
From: Ankan <10196091+Ank4n@users.noreply.github.com>
Date: Thu, 21 Nov 2024 21:04:47 +0700
Subject: [PATCH] [Fix|NominationPools] Only allow apply slash to be executed
if the slash amount is atleast ED (#6540)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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](https://github.com/polkadot-fellows/runtimes/actions/runs/11879471717/job/33101445269?pr=490#step:11:765)
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.
---------
Co-authored-by: Dónal Murray <donal.murray@parity.io>
---
prdoc/pr_6540.prdoc | 16 ++++++++
.../nomination-pools/runtime-api/src/lib.rs | 3 ++
substrate/frame/nomination-pools/src/lib.rs | 23 +++++++++---
.../test-delegate-stake/Cargo.toml | 2 +-
.../test-delegate-stake/src/lib.rs | 37 +++++++++++++++++--
5 files changed, 71 insertions(+), 10 deletions(-)
create mode 100644 prdoc/pr_6540.prdoc
diff --git a/prdoc/pr_6540.prdoc b/prdoc/pr_6540.prdoc
new file mode 100644
index 00000000000..5e030520552
--- /dev/null
+++ b/prdoc/pr_6540.prdoc
@@ -0,0 +1,16 @@
+# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
+# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
+
+title: Only allow apply slash to be executed if the slash amount is atleast ED
+
+doc:
+ - audience: Runtime User
+ description: |
+ This change prevents `pools::apply_slash` from being executed when the pending slash amount of the member is lower
+ than the ED. With this change, such small slashes will still be applied but only when member funds are withdrawn.
+
+crates:
+- name: pallet-nomination-pools-runtime-api
+ bump: patch
+- name: pallet-nomination-pools
+ bump: major
diff --git a/substrate/frame/nomination-pools/runtime-api/src/lib.rs b/substrate/frame/nomination-pools/runtime-api/src/lib.rs
index 4138dd22d89..644ee07fd63 100644
--- a/substrate/frame/nomination-pools/runtime-api/src/lib.rs
+++ b/substrate/frame/nomination-pools/runtime-api/src/lib.rs
@@ -43,6 +43,9 @@ sp_api::decl_runtime_apis! {
fn pool_pending_slash(pool_id: PoolId) -> Balance;
/// Returns the pending slash for a given pool member.
+ ///
+ /// If pending slash of the member exceeds `ExistentialDeposit`, it can be reported on
+ /// chain.
fn member_pending_slash(member: AccountId) -> Balance;
/// Returns true if the pool with `pool_id` needs migration.
diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs
index 201b0af1d60..dc82bf3a37c 100644
--- a/substrate/frame/nomination-pools/src/lib.rs
+++ b/substrate/frame/nomination-pools/src/lib.rs
@@ -1944,6 +1944,8 @@ pub mod pallet {
NothingToAdjust,
/// No slash pending that can be applied to the member.
NothingToSlash,
+ /// The slash amount is too low to be applied.
+ SlashTooLow,
/// The pool or member delegation has already migrated to delegate stake.
AlreadyMigrated,
/// The pool or member delegation has not migrated yet to delegate stake.
@@ -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) {
Ok(_) => T::WeightInfo::apply_slash(),
Err(e) => {
let no_pending_slash: DispatchResult = Err(Error::<T>::NothingToSlash.into());
@@ -2974,8 +2976,10 @@ pub mod pallet {
/// Fails unless [`crate::pallet::Config::StakeAdapter`] is of strategy type:
/// [`adapter::StakeStrategyType::Delegate`].
///
- /// This call can be dispatched permissionlessly (i.e. by any account). If the member has
- /// slash to be applied, caller may be rewarded with the part of the slash.
+ /// The pending slash amount of the member must be equal or more than `ExistentialDeposit`.
+ /// This call can be dispatched permissionlessly (i.e. by any account). If the execution
+ /// is successful, fee is refunded and caller may be rewarded with a part of the slash
+ /// based on the [`crate::pallet::Config::StakeAdapter`] configuration.
#[pallet::call_index(23)]
#[pallet::weight(T::WeightInfo::apply_slash())]
pub fn apply_slash(
@@ -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)?;
// If successful, refund the fees.
Ok(Pays::No.into())
@@ -3574,15 +3578,21 @@ impl<T: Config> Pallet<T> {
fn do_apply_slash(
member_account: &T::AccountId,
reporter: Option<T::AccountId>,
+ enforce_min_slash: bool,
) -> DispatchResult {
let member = PoolMembers::<T>::get(member_account).ok_or(Error::<T>::PoolMemberNotFound)?;
let pending_slash =
Self::member_pending_slash(Member::from(member_account.clone()), member.clone())?;
- // if nothing to slash, return error.
+ // ensure there is something to slash.
ensure!(!pending_slash.is_zero(), Error::<T>::NothingToSlash);
+ if enforce_min_slash {
+ // ensure slashed amount is at least the minimum balance.
+ ensure!(pending_slash >= T::Currency::minimum_balance(), Error::<T>::SlashTooLow);
+ }
+
T::StakeAdapter::member_slash(
Member::from(member_account.clone()),
Pool::from(Pallet::<T>::generate_bonded_account(member.pool_id)),
@@ -3946,6 +3956,9 @@ impl<T: Config> Pallet<T> {
/// Returns the unapplied slash of a member.
///
/// Pending slash is only applicable with [`adapter::DelegateStake`] strategy.
+ ///
+ /// If pending slash of the member exceeds `ExistentialDeposit`, it can be reported on
+ /// chain via [`Call::apply_slash`].
pub fn api_member_pending_slash(who: T::AccountId) -> BalanceOf<T> {
PoolMembers::<T>::get(who.clone())
.map(|pool_member| {
diff --git a/substrate/frame/nomination-pools/test-delegate-stake/Cargo.toml b/substrate/frame/nomination-pools/test-delegate-stake/Cargo.toml
index 7940caaff77..70e1591409b 100644
--- a/substrate/frame/nomination-pools/test-delegate-stake/Cargo.toml
+++ b/substrate/frame/nomination-pools/test-delegate-stake/Cargo.toml
@@ -26,7 +26,7 @@ sp-staking = { workspace = true, default-features = true }
sp-core = { workspace = true, default-features = true }
frame-system = { workspace = true, default-features = true }
-frame-support = { workspace = true, default-features = true }
+frame-support = { features = ["experimental"], workspace = true, default-features = true }
frame-election-provider-support = { workspace = true, default-features = true }
pallet-timestamp = { workspace = true, default-features = true }
diff --git a/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs b/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs
index 40025cdbb3c..cc6335959ab 100644
--- a/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs
+++ b/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs
@@ -20,7 +20,7 @@
mod mock;
use frame_support::{
- assert_noop, assert_ok,
+ assert_noop, assert_ok, hypothetically,
traits::{fungible::InspectHold, Currency},
};
use mock::*;
@@ -537,10 +537,10 @@ fn pool_slash_proportional() {
// a typical example where 3 pool members unbond in era 99, 100, and 101, and a slash that
// happened in era 100 should only affect the latter two.
new_test_ext().execute_with(|| {
- ExistentialDeposit::set(1);
+ ExistentialDeposit::set(2);
BondingDuration::set(28);
- assert_eq!(Balances::minimum_balance(), 1);
- assert_eq!(CurrentEra::<T>::get(), None);
+ assert_eq!(Balances::minimum_balance(), 2);
+ assert_eq!(Staking::current_era(), None);
// create the pool, we know this has id 1.
assert_ok!(Pools::create(RuntimeOrigin::signed(10), 40, 10, 10, 10));
@@ -670,6 +670,34 @@ fn pool_slash_proportional() {
// no pending slash yet.
assert_eq!(Pools::api_pool_pending_slash(1), 0);
+ // and therefore applying slash fails
+ assert_noop!(
+ Pools::apply_slash(RuntimeOrigin::signed(10), 21),
+ PoolsError::<Runtime>::NothingToSlash
+ );
+
+ hypothetically!({
+ // a very small amount is slashed
+ pallet_staking::slashing::do_slash::<Runtime>(
+ &POOL1_BONDED,
+ 3,
+ &mut Default::default(),
+ &mut Default::default(),
+ 100,
+ );
+
+ // ensure correct amount is pending to be slashed
+ assert_eq!(Pools::api_pool_pending_slash(1), 3);
+
+ // 21 has pending slash lower than ED (2)
+ assert_eq!(Pools::api_member_pending_slash(21), 1);
+
+ // slash fails as minimum pending slash amount not met.
+ assert_noop!(
+ Pools::apply_slash(RuntimeOrigin::signed(10), 21),
+ PoolsError::<Runtime>::SlashTooLow
+ );
+ });
pallet_staking::slashing::do_slash::<Runtime>(
&POOL1_BONDED,
@@ -909,6 +937,7 @@ fn pool_slash_non_proportional_bonded_pool_and_chunks() {
);
});
}
+
#[test]
fn pool_migration_e2e() {
new_test_ext().execute_with(|| {
--
GitLab