From 89fc9f1c32cbde4149a38247546844dd540f1fde Mon Sep 17 00:00:00 2001 From: Ankan Date: Tue, 19 Nov 2024 15:34:01 +0100 Subject: [PATCH 1/8] only allow apply slash if amount is atleast ED --- substrate/frame/delegated-staking/src/lib.rs | 4 +- .../frame/delegated-staking/src/tests.rs | 2 +- .../nomination-pools/runtime-api/src/lib.rs | 3 + substrate/frame/nomination-pools/src/lib.rs | 29 +- .../test-delegate-stake/Cargo.toml | 2 +- .../test-delegate-stake/src/lib.rs | 251 +++++++++++++++++- 6 files changed, 274 insertions(+), 17 deletions(-) diff --git a/substrate/frame/delegated-staking/src/lib.rs b/substrate/frame/delegated-staking/src/lib.rs index 1d181eb29cab..33bca1c10825 100644 --- a/substrate/frame/delegated-staking/src/lib.rs +++ b/substrate/frame/delegated-staking/src/lib.rs @@ -240,7 +240,7 @@ pub mod pallet { /// Unapplied pending slash restricts operation on `Agent`. UnappliedSlash, /// `Agent` has no pending slash to be applied. - NothingToSlash, + MinSlashNotMet, /// Failed to withdraw amount from Core Staking. WithdrawFailed, /// Operation not supported by this pallet. @@ -723,7 +723,7 @@ impl Pallet { let agent_ledger = AgentLedgerOuter::::get(&agent)?; // ensure there is something to slash - ensure!(agent_ledger.ledger.pending_slash > Zero::zero(), Error::::NothingToSlash); + ensure!(agent_ledger.ledger.pending_slash > Zero::zero(), Error::::MinSlashNotMet); let mut delegation = >::get(&delegator).ok_or(Error::::NotDelegator)?; ensure!(delegation.agent == agent.clone(), Error::::NotAgent); diff --git a/substrate/frame/delegated-staking/src/tests.rs b/substrate/frame/delegated-staking/src/tests.rs index b7b82a43771e..0e4361ae98ef 100644 --- a/substrate/frame/delegated-staking/src/tests.rs +++ b/substrate/frame/delegated-staking/src/tests.rs @@ -329,7 +329,7 @@ fn apply_pending_slash() { 1, None ), - Error::::NothingToSlash + Error::::MinSlashNotMet ); }); } diff --git a/substrate/frame/nomination-pools/runtime-api/src/lib.rs b/substrate/frame/nomination-pools/runtime-api/src/lib.rs index 4138dd22d898..88691924ff86 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 201b0af1d608..75287a8cd6ec 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -1942,8 +1942,8 @@ pub mod pallet { BondExtraRestricted, /// No imbalance in the ED deposit for the pool. NothingToAdjust, - /// No slash pending that can be applied to the member. - NothingToSlash, + /// The slash amount is zero or too low to be applied. + MinSlashNotMet, /// 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,10 +2300,10 @@ 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::::NothingToSlash.into()); + let no_pending_slash: DispatchResult = Err(Error::::MinSlashNotMet.into()); // This is an expected error. We add appropriate fees and continue withdrawal. if Err(e) == no_pending_slash { T::WeightInfo::apply_slash_fail() @@ -2974,8 +2974,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 +2991,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,14 +3576,20 @@ impl Pallet { fn do_apply_slash( member_account: &T::AccountId, reporter: Option, + enforce_min_slash: bool, ) -> DispatchResult { let member = PoolMembers::::get(member_account).ok_or(Error::::PoolMemberNotFound)?; let pending_slash = Self::member_pending_slash(Member::from(member_account.clone()), member.clone())?; - // if nothing to slash, return error. - ensure!(!pending_slash.is_zero(), Error::::NothingToSlash); + if enforce_min_slash { + // ensure slashed amount is at least the minimum balance. + ensure!(pending_slash >= T::Currency::minimum_balance(), Error::::MinSlashNotMet); + } else { + // in any case ensure there is something to slash. + ensure!(!pending_slash.is_zero(), Error::::MinSlashNotMet); + }; T::StakeAdapter::member_slash( Member::from(member_account.clone()), @@ -3946,6 +3954,9 @@ impl Pallet { /// 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 { PoolMembers::::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 7940caaff775..70e1591409b8 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 40025cdbb3cd..165d75543e9c 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::::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::::MinSlashNotMet + ); + + hypothetically!({ + // a very small amount is slashed + pallet_staking::slashing::do_slash::( + &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::::MinSlashNotMet + ); + }); pallet_staking::slashing::do_slash::( &POOL1_BONDED, @@ -909,6 +937,221 @@ fn pool_slash_non_proportional_bonded_pool_and_chunks() { ); }); } + +#[test] +fn pool_slash_fail() { + // cannot apply slash if slash amount is too small + new_test_ext().execute_with(|| { + ExistentialDeposit::set(1); + BondingDuration::set(28); + assert_eq!(Balances::minimum_balance(), 1); + 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)); + assert_eq!(LastPoolId::::get(), 1); + + assert_eq!( + staking_events_since_last_call(), + vec![StakingEvent::Bonded { stash: POOL1_BONDED, amount: 40 }] + ); + assert_eq!( + delegated_staking_events_since_last_call(), + vec![DelegatedStakingEvent::Delegated { + agent: POOL1_BONDED, + delegator: 10, + amount: 40 + }] + ); + assert_eq!( + pool_events_since_last_call(), + vec![ + PoolsEvent::Created { depositor: 10, pool_id: 1 }, + PoolsEvent::Bonded { member: 10, pool_id: 1, bonded: 40, joined: true }, + ] + ); + + // have two members join + let bond = 20; + assert_ok!(Pools::join(RuntimeOrigin::signed(20), bond, 1)); + assert_ok!(Pools::join(RuntimeOrigin::signed(21), bond, 1)); + assert_ok!(Pools::join(RuntimeOrigin::signed(22), bond, 1)); + + assert_eq!( + staking_events_since_last_call(), + vec![ + StakingEvent::Bonded { stash: POOL1_BONDED, amount: bond }, + StakingEvent::Bonded { stash: POOL1_BONDED, amount: bond }, + StakingEvent::Bonded { stash: POOL1_BONDED, amount: bond }, + ] + ); + assert_eq!( + delegated_staking_events_since_last_call(), + vec![ + DelegatedStakingEvent::Delegated { + agent: POOL1_BONDED, + delegator: 20, + amount: bond + }, + DelegatedStakingEvent::Delegated { + agent: POOL1_BONDED, + delegator: 21, + amount: bond + }, + DelegatedStakingEvent::Delegated { + agent: POOL1_BONDED, + delegator: 22, + amount: bond + } + ] + ); + assert_eq!( + pool_events_since_last_call(), + vec![ + PoolsEvent::Bonded { member: 20, pool_id: 1, bonded: bond, joined: true }, + PoolsEvent::Bonded { member: 21, pool_id: 1, bonded: bond, joined: true }, + PoolsEvent::Bonded { member: 22, pool_id: 1, bonded: bond, joined: true }, + ] + ); + + // now let's progress a lot. + CurrentEra::::set(Some(99)); + + // and unbond + assert_ok!(Pools::unbond(RuntimeOrigin::signed(20), 20, bond)); + + assert_eq!( + staking_events_since_last_call(), + vec![StakingEvent::Unbonded { stash: POOL1_BONDED, amount: bond },] + ); + assert_eq!( + pool_events_since_last_call(), + vec![PoolsEvent::Unbonded { + member: 20, + pool_id: 1, + balance: bond, + points: bond, + era: 127 + }] + ); + + CurrentEra::::set(Some(100)); + assert_ok!(Pools::unbond(RuntimeOrigin::signed(21), 21, bond)); + assert_eq!( + staking_events_since_last_call(), + vec![StakingEvent::Unbonded { stash: POOL1_BONDED, amount: bond },] + ); + assert_eq!( + pool_events_since_last_call(), + vec![PoolsEvent::Unbonded { + member: 21, + pool_id: 1, + balance: bond, + points: bond, + era: 128 + }] + ); + + CurrentEra::::set(Some(101)); + assert_ok!(Pools::unbond(RuntimeOrigin::signed(22), 22, bond)); + assert_eq!( + staking_events_since_last_call(), + vec![StakingEvent::Unbonded { stash: POOL1_BONDED, amount: bond },] + ); + assert_eq!( + pool_events_since_last_call(), + vec![PoolsEvent::Unbonded { + member: 22, + pool_id: 1, + balance: bond, + points: bond, + era: 129 + }] + ); + + // Apply a slash that happened in era 100. This is typically applied with a delay. + // Of the total 100, 50 is slashed. + assert_eq!(BondedPools::::get(1).unwrap().points, 40); + + // no pending slash yet. + assert_eq!(Pools::api_pool_pending_slash(1), 0); + + pallet_staking::slashing::do_slash::( + &POOL1_BONDED, + 50, + &mut Default::default(), + &mut Default::default(), + 100, + ); + + // Pools api returns correct slash amount. + assert_eq!(Pools::api_pool_pending_slash(1), 50); + + assert_eq!( + staking_events_since_last_call(), + vec![StakingEvent::Slashed { staker: POOL1_BONDED, amount: 50 }] + ); + assert_eq!( + pool_events_since_last_call(), + vec![ + // This era got slashed 12.5, which rounded up to 13. + PoolsEvent::UnbondingPoolSlashed { pool_id: 1, era: 128, balance: 7 }, + // This era got slashed 12 instead of 12.5 because an earlier chunk got 0.5 more + // slashed, and 12 is all the remaining slash + PoolsEvent::UnbondingPoolSlashed { pool_id: 1, era: 129, balance: 8 }, + // Bonded pool got slashed for 25, remaining 15 in it. + PoolsEvent::PoolSlashed { pool_id: 1, balance: 15 } + ] + ); + + // 21's balance in the pool is slashed. + assert_eq!(PoolMembers::::get(21).unwrap().total_balance(), 7); + // But their actual balance is still unslashed. + assert_eq!(Balances::total_balance_on_hold(&21), bond); + // 21 has pending slash + assert_eq!(Pools::api_member_pending_slash(21), bond - 7); + // apply slash permissionlessly. + assert_ok!(Pools::apply_slash(RuntimeOrigin::signed(10), 21)); + // member balance is slashed. + assert_eq!(Balances::total_balance_on_hold(&21), 7); + // 21 has no pending slash anymore + assert_eq!(Pools::api_member_pending_slash(21), 0); + + assert_eq!( + delegated_staking_events_since_last_call(), + vec![DelegatedStakingEvent::Slashed { + agent: POOL1_BONDED, + delegator: 21, + amount: bond - 7 + }] + ); + + // 22 balance isn't slashed yet as well. + assert_eq!(PoolMembers::::get(22).unwrap().total_balance(), 8); + assert_eq!(Balances::total_balance_on_hold(&22), bond); + + // they try to withdraw. This should slash them. + CurrentEra::::set(Some(129)); + let pre_balance = Balances::free_balance(&22); + assert_ok!(Pools::withdraw_unbonded(RuntimeOrigin::signed(22), 22, 0)); + // all balance should be released. + assert_eq!(Balances::total_balance_on_hold(&22), 0); + assert_eq!(Balances::free_balance(&22), pre_balance + 8); + + assert_eq!( + delegated_staking_events_since_last_call(), + vec![ + DelegatedStakingEvent::Slashed { + agent: POOL1_BONDED, + delegator: 22, + amount: bond - 8 + }, + DelegatedStakingEvent::Released { agent: POOL1_BONDED, delegator: 22, amount: 8 }, + ] + ); + }); +} + #[test] fn pool_migration_e2e() { new_test_ext().execute_with(|| { From 195204d0fbaf227051279d7ade118913aec04c5f Mon Sep 17 00:00:00 2001 From: Ankan Date: Tue, 19 Nov 2024 16:13:10 +0100 Subject: [PATCH 2/8] revert delegated staking changes --- substrate/frame/delegated-staking/src/lib.rs | 4 ++-- substrate/frame/delegated-staking/src/tests.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/substrate/frame/delegated-staking/src/lib.rs b/substrate/frame/delegated-staking/src/lib.rs index 33bca1c10825..1d181eb29cab 100644 --- a/substrate/frame/delegated-staking/src/lib.rs +++ b/substrate/frame/delegated-staking/src/lib.rs @@ -240,7 +240,7 @@ pub mod pallet { /// Unapplied pending slash restricts operation on `Agent`. UnappliedSlash, /// `Agent` has no pending slash to be applied. - MinSlashNotMet, + NothingToSlash, /// Failed to withdraw amount from Core Staking. WithdrawFailed, /// Operation not supported by this pallet. @@ -723,7 +723,7 @@ impl Pallet { let agent_ledger = AgentLedgerOuter::::get(&agent)?; // ensure there is something to slash - ensure!(agent_ledger.ledger.pending_slash > Zero::zero(), Error::::MinSlashNotMet); + ensure!(agent_ledger.ledger.pending_slash > Zero::zero(), Error::::NothingToSlash); let mut delegation = >::get(&delegator).ok_or(Error::::NotDelegator)?; ensure!(delegation.agent == agent.clone(), Error::::NotAgent); diff --git a/substrate/frame/delegated-staking/src/tests.rs b/substrate/frame/delegated-staking/src/tests.rs index 0e4361ae98ef..b7b82a43771e 100644 --- a/substrate/frame/delegated-staking/src/tests.rs +++ b/substrate/frame/delegated-staking/src/tests.rs @@ -329,7 +329,7 @@ fn apply_pending_slash() { 1, None ), - Error::::MinSlashNotMet + Error::::NothingToSlash ); }); } From eff1f319da9870360f7657c5e5425a6400834b47 Mon Sep 17 00:00:00 2001 From: Ankan Date: Tue, 19 Nov 2024 16:18:23 +0100 Subject: [PATCH 3/8] add prdoc --- prdoc/pr_6540.prdoc | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 prdoc/pr_6540.prdoc diff --git a/prdoc/pr_6540.prdoc b/prdoc/pr_6540.prdoc new file mode 100644 index 000000000000..70d4fb4eeea4 --- /dev/null +++ b/prdoc/pr_6540.prdoc @@ -0,0 +1,17 @@ +# 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: minor + From 43dd30af6b6dd9d73d3e11199b06597df6718212 Mon Sep 17 00:00:00 2001 From: Ankan Date: Tue, 19 Nov 2024 16:41:30 +0100 Subject: [PATCH 4/8] keep it backward compatible --- prdoc/pr_6540.prdoc | 1 - substrate/frame/nomination-pools/src/lib.rs | 12 +++++++----- .../nomination-pools/test-delegate-stake/src/lib.rs | 4 ++-- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/prdoc/pr_6540.prdoc b/prdoc/pr_6540.prdoc index 70d4fb4eeea4..060b46e278b1 100644 --- a/prdoc/pr_6540.prdoc +++ b/prdoc/pr_6540.prdoc @@ -14,4 +14,3 @@ crates: bump: patch - name: pallet-nomination-pools bump: minor - diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 75287a8cd6ec..f762e565721f 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -1942,8 +1942,10 @@ pub mod pallet { BondExtraRestricted, /// No imbalance in the ED deposit for the pool. NothingToAdjust, - /// The slash amount is zero or too low to be applied. - MinSlashNotMet, + /// 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. @@ -2303,7 +2305,7 @@ pub mod pallet { match Self::do_apply_slash(&member_account, None, false) { Ok(_) => T::WeightInfo::apply_slash(), Err(e) => { - let no_pending_slash: DispatchResult = Err(Error::::MinSlashNotMet.into()); + let no_pending_slash: DispatchResult = Err(Error::::NothingToSlash.into()); // This is an expected error. We add appropriate fees and continue withdrawal. if Err(e) == no_pending_slash { T::WeightInfo::apply_slash_fail() @@ -3585,10 +3587,10 @@ impl Pallet { if enforce_min_slash { // ensure slashed amount is at least the minimum balance. - ensure!(pending_slash >= T::Currency::minimum_balance(), Error::::MinSlashNotMet); + ensure!(pending_slash >= T::Currency::minimum_balance(), Error::::SlashTooLow); } else { // in any case ensure there is something to slash. - ensure!(!pending_slash.is_zero(), Error::::MinSlashNotMet); + ensure!(!pending_slash.is_zero(), Error::::NothingToSlash); }; T::StakeAdapter::member_slash( 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 165d75543e9c..2c2b388f8a31 100644 --- a/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs +++ b/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs @@ -673,7 +673,7 @@ fn pool_slash_proportional() { // and therefore applying slash fails assert_noop!( Pools::apply_slash(RuntimeOrigin::signed(10), 21), - PoolsError::::MinSlashNotMet + PoolsError::::SlashTooLow ); hypothetically!({ @@ -695,7 +695,7 @@ fn pool_slash_proportional() { // slash fails as minimum pending slash amount not met. assert_noop!( Pools::apply_slash(RuntimeOrigin::signed(10), 21), - PoolsError::::MinSlashNotMet + PoolsError::::SlashTooLow ); }); From 086d7cd362e6da8a4d55c078c584d670d030ee2e Mon Sep 17 00:00:00 2001 From: Ankan Date: Tue, 19 Nov 2024 16:42:41 +0100 Subject: [PATCH 5/8] remove duplicate test --- .../test-delegate-stake/src/lib.rs | 214 ------------------ 1 file changed, 214 deletions(-) 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 2c2b388f8a31..7cc3229a5962 100644 --- a/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs +++ b/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs @@ -938,220 +938,6 @@ fn pool_slash_non_proportional_bonded_pool_and_chunks() { }); } -#[test] -fn pool_slash_fail() { - // cannot apply slash if slash amount is too small - new_test_ext().execute_with(|| { - ExistentialDeposit::set(1); - BondingDuration::set(28); - assert_eq!(Balances::minimum_balance(), 1); - 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)); - assert_eq!(LastPoolId::::get(), 1); - - assert_eq!( - staking_events_since_last_call(), - vec![StakingEvent::Bonded { stash: POOL1_BONDED, amount: 40 }] - ); - assert_eq!( - delegated_staking_events_since_last_call(), - vec![DelegatedStakingEvent::Delegated { - agent: POOL1_BONDED, - delegator: 10, - amount: 40 - }] - ); - assert_eq!( - pool_events_since_last_call(), - vec![ - PoolsEvent::Created { depositor: 10, pool_id: 1 }, - PoolsEvent::Bonded { member: 10, pool_id: 1, bonded: 40, joined: true }, - ] - ); - - // have two members join - let bond = 20; - assert_ok!(Pools::join(RuntimeOrigin::signed(20), bond, 1)); - assert_ok!(Pools::join(RuntimeOrigin::signed(21), bond, 1)); - assert_ok!(Pools::join(RuntimeOrigin::signed(22), bond, 1)); - - assert_eq!( - staking_events_since_last_call(), - vec![ - StakingEvent::Bonded { stash: POOL1_BONDED, amount: bond }, - StakingEvent::Bonded { stash: POOL1_BONDED, amount: bond }, - StakingEvent::Bonded { stash: POOL1_BONDED, amount: bond }, - ] - ); - assert_eq!( - delegated_staking_events_since_last_call(), - vec![ - DelegatedStakingEvent::Delegated { - agent: POOL1_BONDED, - delegator: 20, - amount: bond - }, - DelegatedStakingEvent::Delegated { - agent: POOL1_BONDED, - delegator: 21, - amount: bond - }, - DelegatedStakingEvent::Delegated { - agent: POOL1_BONDED, - delegator: 22, - amount: bond - } - ] - ); - assert_eq!( - pool_events_since_last_call(), - vec![ - PoolsEvent::Bonded { member: 20, pool_id: 1, bonded: bond, joined: true }, - PoolsEvent::Bonded { member: 21, pool_id: 1, bonded: bond, joined: true }, - PoolsEvent::Bonded { member: 22, pool_id: 1, bonded: bond, joined: true }, - ] - ); - - // now let's progress a lot. - CurrentEra::::set(Some(99)); - - // and unbond - assert_ok!(Pools::unbond(RuntimeOrigin::signed(20), 20, bond)); - - assert_eq!( - staking_events_since_last_call(), - vec![StakingEvent::Unbonded { stash: POOL1_BONDED, amount: bond },] - ); - assert_eq!( - pool_events_since_last_call(), - vec![PoolsEvent::Unbonded { - member: 20, - pool_id: 1, - balance: bond, - points: bond, - era: 127 - }] - ); - - CurrentEra::::set(Some(100)); - assert_ok!(Pools::unbond(RuntimeOrigin::signed(21), 21, bond)); - assert_eq!( - staking_events_since_last_call(), - vec![StakingEvent::Unbonded { stash: POOL1_BONDED, amount: bond },] - ); - assert_eq!( - pool_events_since_last_call(), - vec![PoolsEvent::Unbonded { - member: 21, - pool_id: 1, - balance: bond, - points: bond, - era: 128 - }] - ); - - CurrentEra::::set(Some(101)); - assert_ok!(Pools::unbond(RuntimeOrigin::signed(22), 22, bond)); - assert_eq!( - staking_events_since_last_call(), - vec![StakingEvent::Unbonded { stash: POOL1_BONDED, amount: bond },] - ); - assert_eq!( - pool_events_since_last_call(), - vec![PoolsEvent::Unbonded { - member: 22, - pool_id: 1, - balance: bond, - points: bond, - era: 129 - }] - ); - - // Apply a slash that happened in era 100. This is typically applied with a delay. - // Of the total 100, 50 is slashed. - assert_eq!(BondedPools::::get(1).unwrap().points, 40); - - // no pending slash yet. - assert_eq!(Pools::api_pool_pending_slash(1), 0); - - pallet_staking::slashing::do_slash::( - &POOL1_BONDED, - 50, - &mut Default::default(), - &mut Default::default(), - 100, - ); - - // Pools api returns correct slash amount. - assert_eq!(Pools::api_pool_pending_slash(1), 50); - - assert_eq!( - staking_events_since_last_call(), - vec![StakingEvent::Slashed { staker: POOL1_BONDED, amount: 50 }] - ); - assert_eq!( - pool_events_since_last_call(), - vec![ - // This era got slashed 12.5, which rounded up to 13. - PoolsEvent::UnbondingPoolSlashed { pool_id: 1, era: 128, balance: 7 }, - // This era got slashed 12 instead of 12.5 because an earlier chunk got 0.5 more - // slashed, and 12 is all the remaining slash - PoolsEvent::UnbondingPoolSlashed { pool_id: 1, era: 129, balance: 8 }, - // Bonded pool got slashed for 25, remaining 15 in it. - PoolsEvent::PoolSlashed { pool_id: 1, balance: 15 } - ] - ); - - // 21's balance in the pool is slashed. - assert_eq!(PoolMembers::::get(21).unwrap().total_balance(), 7); - // But their actual balance is still unslashed. - assert_eq!(Balances::total_balance_on_hold(&21), bond); - // 21 has pending slash - assert_eq!(Pools::api_member_pending_slash(21), bond - 7); - // apply slash permissionlessly. - assert_ok!(Pools::apply_slash(RuntimeOrigin::signed(10), 21)); - // member balance is slashed. - assert_eq!(Balances::total_balance_on_hold(&21), 7); - // 21 has no pending slash anymore - assert_eq!(Pools::api_member_pending_slash(21), 0); - - assert_eq!( - delegated_staking_events_since_last_call(), - vec![DelegatedStakingEvent::Slashed { - agent: POOL1_BONDED, - delegator: 21, - amount: bond - 7 - }] - ); - - // 22 balance isn't slashed yet as well. - assert_eq!(PoolMembers::::get(22).unwrap().total_balance(), 8); - assert_eq!(Balances::total_balance_on_hold(&22), bond); - - // they try to withdraw. This should slash them. - CurrentEra::::set(Some(129)); - let pre_balance = Balances::free_balance(&22); - assert_ok!(Pools::withdraw_unbonded(RuntimeOrigin::signed(22), 22, 0)); - // all balance should be released. - assert_eq!(Balances::total_balance_on_hold(&22), 0); - assert_eq!(Balances::free_balance(&22), pre_balance + 8); - - assert_eq!( - delegated_staking_events_since_last_call(), - vec![ - DelegatedStakingEvent::Slashed { - agent: POOL1_BONDED, - delegator: 22, - amount: bond - 8 - }, - DelegatedStakingEvent::Released { agent: POOL1_BONDED, delegator: 22, amount: 8 }, - ] - ); - }); -} - #[test] fn pool_migration_e2e() { new_test_ext().execute_with(|| { From ce55a7fb4f2e9c3f47192be7540155bf65c9c760 Mon Sep 17 00:00:00 2001 From: Ankan Date: Tue, 19 Nov 2024 16:45:27 +0100 Subject: [PATCH 6/8] minor --- substrate/frame/nomination-pools/runtime-api/src/lib.rs | 2 +- substrate/frame/nomination-pools/src/lib.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/nomination-pools/runtime-api/src/lib.rs b/substrate/frame/nomination-pools/runtime-api/src/lib.rs index 88691924ff86..644ee07fd634 100644 --- a/substrate/frame/nomination-pools/runtime-api/src/lib.rs +++ b/substrate/frame/nomination-pools/runtime-api/src/lib.rs @@ -44,7 +44,7 @@ sp_api::decl_runtime_apis! { /// Returns the pending slash for a given pool member. /// - /// If pending slash of the member exceeds ExistentialDeposit, it can be reported on + /// If pending slash of the member exceeds `ExistentialDeposit`, it can be reported on /// chain. fn member_pending_slash(member: AccountId) -> Balance; diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index f762e565721f..543d8d6d44e7 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -3957,7 +3957,7 @@ impl Pallet { /// /// Pending slash is only applicable with [`adapter::DelegateStake`] strategy. /// - /// If pending slash of the member exceeds ExistentialDeposit, it can be reported on + /// 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 { PoolMembers::::get(who.clone()) From 5407bf45d2a687d73441d01a7a7249e378755290 Mon Sep 17 00:00:00 2001 From: Ankan Date: Tue, 19 Nov 2024 16:51:47 +0100 Subject: [PATCH 7/8] small refactor --- substrate/frame/nomination-pools/src/lib.rs | 8 ++++---- .../frame/nomination-pools/test-delegate-stake/src/lib.rs | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 543d8d6d44e7..dc82bf3a37c6 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -3585,13 +3585,13 @@ impl Pallet { let pending_slash = Self::member_pending_slash(Member::from(member_account.clone()), member.clone())?; + // ensure there is something to slash. + ensure!(!pending_slash.is_zero(), Error::::NothingToSlash); + if enforce_min_slash { // ensure slashed amount is at least the minimum balance. ensure!(pending_slash >= T::Currency::minimum_balance(), Error::::SlashTooLow); - } else { - // in any case ensure there is something to slash. - ensure!(!pending_slash.is_zero(), Error::::NothingToSlash); - }; + } T::StakeAdapter::member_slash( Member::from(member_account.clone()), 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 7cc3229a5962..cc6335959ab7 100644 --- a/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs +++ b/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs @@ -673,7 +673,7 @@ fn pool_slash_proportional() { // and therefore applying slash fails assert_noop!( Pools::apply_slash(RuntimeOrigin::signed(10), 21), - PoolsError::::SlashTooLow + PoolsError::::NothingToSlash ); hypothetically!({ From c3d887afc5ec7b4411c267beab53d76b2de4079c Mon Sep 17 00:00:00 2001 From: Ankan <10196091+Ank4n@users.noreply.github.com> Date: Thu, 21 Nov 2024 14:15:24 +0100 Subject: [PATCH 8/8] Update prdoc/pr_6540.prdoc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Dónal Murray --- prdoc/pr_6540.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_6540.prdoc b/prdoc/pr_6540.prdoc index 060b46e278b1..5e0305205521 100644 --- a/prdoc/pr_6540.prdoc +++ b/prdoc/pr_6540.prdoc @@ -13,4 +13,4 @@ crates: - name: pallet-nomination-pools-runtime-api bump: patch - name: pallet-nomination-pools - bump: minor + bump: major