diff --git a/substrate/frame/nomination-pools/test-staking/src/lib.rs b/substrate/frame/nomination-pools/test-staking/src/lib.rs index 2e40e8c6d9..be7752ce11 100644 --- a/substrate/frame/nomination-pools/test-staking/src/lib.rs +++ b/substrate/frame/nomination-pools/test-staking/src/lib.rs @@ -22,7 +22,8 @@ mod mock; use frame_support::{assert_noop, assert_ok, bounded_btree_map, traits::Currency}; use mock::*; use pallet_nomination_pools::{ - Error as PoolsError, Event as PoolsEvent, LastPoolId, PoolMember, PoolMembers, PoolState, + BondedPools, Error as PoolsError, Event as PoolsEvent, LastPoolId, PoolMember, PoolMembers, + PoolState, }; use pallet_staking::{CurrentEra, Event as StakingEvent, Payee, RewardDestination}; @@ -273,7 +274,7 @@ fn pool_slash_e2e() { 30, &mut Default::default(), &mut Default::default(), - 1, // slash era 1, affects chunks at era 5 onwards. + 2, // slash era 2, affects chunks at era 5 onwards. ); assert_eq!(staking_events_since_last_call(), vec![StakingEvent::Slashed(POOL1_BONDED, 30)]); @@ -371,3 +372,254 @@ fn pool_slash_e2e() { ); }); } + +#[test] +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); + 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(Origin::signed(10), 40, 10, 10, 10)); + assert_eq!(LastPoolId::::get(), 1); + + assert_eq!(staking_events_since_last_call(), vec![StakingEvent::Bonded(POOL1_BONDED, 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(Origin::signed(20), bond, 1)); + assert_ok!(Pools::join(Origin::signed(21), bond, 1)); + assert_ok!(Pools::join(Origin::signed(22), bond, 1)); + + assert_eq!( + staking_events_since_last_call(), + vec![ + StakingEvent::Bonded(POOL1_BONDED, bond), + StakingEvent::Bonded(POOL1_BONDED, bond), + StakingEvent::Bonded(POOL1_BONDED, 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(Origin::signed(20), 20, bond)); + + assert_eq!( + staking_events_since_last_call(), + vec![StakingEvent::Unbonded(POOL1_BONDED, bond),] + ); + assert_eq!( + pool_events_since_last_call(), + vec![PoolsEvent::Unbonded { member: 20, pool_id: 1, balance: bond, points: bond }] + ); + + CurrentEra::::set(Some(100)); + assert_ok!(Pools::unbond(Origin::signed(21), 21, bond)); + assert_eq!( + staking_events_since_last_call(), + vec![StakingEvent::Unbonded(POOL1_BONDED, bond),] + ); + assert_eq!( + pool_events_since_last_call(), + vec![PoolsEvent::Unbonded { member: 21, pool_id: 1, balance: bond, points: bond }] + ); + + CurrentEra::::set(Some(101)); + assert_ok!(Pools::unbond(Origin::signed(22), 22, bond)); + assert_eq!( + staking_events_since_last_call(), + vec![StakingEvent::Unbonded(POOL1_BONDED, bond),] + ); + assert_eq!( + pool_events_since_last_call(), + vec![PoolsEvent::Unbonded { member: 22, pool_id: 1, balance: bond, points: bond }] + ); + + // 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); + pallet_staking::slashing::do_slash::( + &POOL1_BONDED, + 50, + &mut Default::default(), + &mut Default::default(), + 100, + ); + + assert_eq!(staking_events_since_last_call(), vec![StakingEvent::Slashed(POOL1_BONDED, 50)]); + assert_eq!( + pool_events_since_last_call(), + vec![ + // This last pool got slashed only the leftover dust. Otherwise in principle, this + // chunk/pool should have not been affected. + PoolsEvent::UnbondingPoolSlashed { pool_id: 1, era: 127, balance: 19 }, + // This pool got slashed 12.5, which rounded down to 12. + PoolsEvent::UnbondingPoolSlashed { pool_id: 1, era: 128, balance: 8 }, + // This pool got slashed 12.5, which rounded down to 12. + 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 } + ] + ); + }); +} + +#[test] +fn pool_slash_non_proportional_only_bonded_pool() { + // A typical example where a pool member unbonds in era 99, and he can get away with a slash + // that happened in era 100, as long as the pool has enough active bond to cover the slash. If + // everything else in the slashing/staking system works, this should always be the case. + // Nonetheless, `ledger.slash` has been written such that it will slash greedily from any chunk + // if it runs out of chunks that it thinks should be affected by the slash. + 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(Origin::signed(10), 40, 10, 10, 10)); + assert_eq!(staking_events_since_last_call(), vec![StakingEvent::Bonded(POOL1_BONDED, 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(Origin::signed(20), bond, 1)); + assert_eq!( + staking_events_since_last_call(), + vec![StakingEvent::Bonded(POOL1_BONDED, bond)] + ); + assert_eq!( + pool_events_since_last_call(), + vec![PoolsEvent::Bonded { member: 20, pool_id: 1, bonded: bond, joined: true }] + ); + + // progress and unbond. + CurrentEra::::set(Some(99)); + assert_ok!(Pools::unbond(Origin::signed(20), 20, bond)); + assert_eq!( + staking_events_since_last_call(), + vec![StakingEvent::Unbonded(POOL1_BONDED, bond)] + ); + assert_eq!( + pool_events_since_last_call(), + vec![PoolsEvent::Unbonded { member: 20, pool_id: 1, balance: bond, points: bond }] + ); + + // slash for 30. This will be deducted only from the bonded pool. + CurrentEra::::set(Some(100)); + assert_eq!(BondedPools::::get(1).unwrap().points, 40); + pallet_staking::slashing::do_slash::( + &POOL1_BONDED, + 30, + &mut Default::default(), + &mut Default::default(), + 100, + ); + + assert_eq!(staking_events_since_last_call(), vec![StakingEvent::Slashed(POOL1_BONDED, 30)]); + assert_eq!( + pool_events_since_last_call(), + vec![PoolsEvent::PoolSlashed { pool_id: 1, balance: 10 }] + ); + }); +} + +#[test] +fn pool_slash_non_proportional_bonded_pool_and_chunks() { + // An uncommon example where even though some funds are unlocked such that they should not be + // affected by a slash, we still slash out of them. This should not happen at all. If a + // nomination has unbonded, from the next era onwards, their exposure will drop, so if an era + // happens in that era, then their share of that slash should naturally be less, such that only + // their active ledger stake is enough to compensate it. + 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(Origin::signed(10), 40, 10, 10, 10)); + assert_eq!(staking_events_since_last_call(), vec![StakingEvent::Bonded(POOL1_BONDED, 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(Origin::signed(20), bond, 1)); + assert_eq!( + staking_events_since_last_call(), + vec![StakingEvent::Bonded(POOL1_BONDED, bond)] + ); + assert_eq!( + pool_events_since_last_call(), + vec![PoolsEvent::Bonded { member: 20, pool_id: 1, bonded: bond, joined: true }] + ); + + // progress and unbond. + CurrentEra::::set(Some(99)); + assert_ok!(Pools::unbond(Origin::signed(20), 20, bond)); + assert_eq!( + staking_events_since_last_call(), + vec![StakingEvent::Unbonded(POOL1_BONDED, bond)] + ); + assert_eq!( + pool_events_since_last_call(), + vec![PoolsEvent::Unbonded { member: 20, pool_id: 1, balance: bond, points: bond }] + ); + + // slash 50. This will be deducted only from the bonded pool and one of the unbonding pools. + CurrentEra::::set(Some(100)); + assert_eq!(BondedPools::::get(1).unwrap().points, 40); + pallet_staking::slashing::do_slash::( + &POOL1_BONDED, + 50, + &mut Default::default(), + &mut Default::default(), + 100, + ); + + assert_eq!(staking_events_since_last_call(), vec![StakingEvent::Slashed(POOL1_BONDED, 50)]); + assert_eq!( + pool_events_since_last_call(), + vec![ + // out of 20, 10 was taken. + PoolsEvent::UnbondingPoolSlashed { pool_id: 1, era: 127, balance: 10 }, + // out of 40, all was taken. + PoolsEvent::PoolSlashed { pool_id: 1, balance: 0 } + ] + ); + }); +} diff --git a/substrate/frame/nomination-pools/test-staking/src/mock.rs b/substrate/frame/nomination-pools/test-staking/src/mock.rs index 7b720c009b..d13d8aa341 100644 --- a/substrate/frame/nomination-pools/test-staking/src/mock.rs +++ b/substrate/frame/nomination-pools/test-staking/src/mock.rs @@ -24,6 +24,8 @@ type AccountIndex = u32; type BlockNumber = u64; type Balance = u128; +pub(crate) type T = Runtime; + pub(crate) const POOL1_BONDED: AccountId = 20318131474730217858575332831085u128; pub(crate) const POOL1_REWARD: AccountId = 20397359637244482196168876781421u128; @@ -194,13 +196,14 @@ frame_support::construct_runtime!( ); pub fn new_test_ext() -> sp_io::TestExternalities { + sp_tracing::try_init_simple(); let mut storage = frame_system::GenesisConfig::default().build_storage::().unwrap(); let _ = pallet_nomination_pools::GenesisConfig:: { min_join_bond: 2, min_create_bond: 2, max_pools: Some(3), - max_members_per_pool: Some(3), - max_members: Some(3 * 3), + max_members_per_pool: Some(5), + max_members: Some(3 * 5), } .assimilate_storage(&mut storage) .unwrap(); diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs index 360d5b5efb..50c033bdc7 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/substrate/frame/staking/src/lib.rs @@ -529,14 +529,27 @@ impl StakingLedger { (self, unlocking_balance) } - /// Slash the staker for a given amount of balance. This can grow the value of the slash in the - /// case that either the active bonded or some unlocking chunks become dust after slashing. - /// Returns the amount of funds actually slashed. + /// Slash the staker for a given amount of balance. + /// + /// This implements a proportional slashing system, whereby we set our preference to slash as + /// such: + /// + /// - If any unlocking chunks exist that are scheduled to be unlocked at `slash_era + + /// bonding_duration` and onwards, the slash is divided equally between the active ledger and + /// the unlocking chunks. + /// - If no such chunks exist, then only the active balance is slashed. + /// + /// Note that the above is only a *preference*. If for any reason the active ledger, with or + /// without some portion of the unlocking chunks that are more justified to be slashed are not + /// enough, then the slashing will continue and will consume as much of the active and unlocking + /// chunks as needed. + /// + /// This will never slash more than the given amount. If any of the chunks become dusted, the + /// last chunk is slashed slightly less to compensate. Returns the amount of funds actually + /// slashed. /// /// `slash_era` is the era in which the slash (which is being enacted now) actually happened. /// - /// # Note - /// /// This calls `Config::OnStakerSlash::on_slash` with information as to how the slash was /// applied. fn slash( @@ -545,54 +558,81 @@ impl StakingLedger { minimum_balance: BalanceOf, slash_era: EraIndex, ) -> BalanceOf { - use sp_staking::OnStakerSlash as _; - if slash_amount.is_zero() { return Zero::zero() } + use sp_staking::OnStakerSlash as _; let mut remaining_slash = slash_amount; let pre_slash_total = self.total; - let era_after_slash = slash_era + 1; - let chunk_unlock_era_after_slash = era_after_slash + T::BondingDuration::get(); + // for a `slash_era = x`, any chunk that is scheduled to be unlocked at era `x + 28` + // (assuming 28 is the bonding duration) onwards should be slashed. + let slashable_chunks_start = slash_era + T::BondingDuration::get(); - // Calculate the total balance of active funds and unlocking funds in the affected range. - let (affected_balance, slash_chunks_priority): (_, Box>) = { - if let Some(start_index) = - self.unlocking.iter().position(|c| c.era >= chunk_unlock_era_after_slash) + // `Some(ratio)` if this is proportional, with `ratio`, `None` otherwise. In both cases, we + // slash first the active chunk, and then `slash_chunks_priority`. + let (maybe_proportional, slash_chunks_priority) = { + if let Some(first_slashable_index) = + self.unlocking.iter().position(|c| c.era >= slashable_chunks_start) { + // If there exists a chunk who's after the first_slashable_start, then this is a + // proportional slash, because we want to slash active and these chunks + // proportionally. + // The indices of the first chunk after the slash up through the most recent chunk. // (The most recent chunk is at greatest from this era) - let affected_indices = start_index..self.unlocking.len(); + let affected_indices = first_slashable_index..self.unlocking.len(); let unbonding_affected_balance = affected_indices.clone().fold(BalanceOf::::zero(), |sum, i| { - if let Some(chunk) = self.unlocking.get_mut(i).defensive() { + if let Some(chunk) = self.unlocking.get(i).defensive() { sum.saturating_add(chunk.value) } else { sum } }); + let affected_balance = self.active.saturating_add(unbonding_affected_balance); + let ratio = Perquintill::from_rational(slash_amount, affected_balance); ( - self.active.saturating_add(unbonding_affected_balance), - Box::new(affected_indices.chain((0..start_index).rev())), + Some(ratio), + affected_indices.chain((0..first_slashable_index).rev()).collect::>(), ) } else { - (self.active, Box::new((0..self.unlocking.len()).rev())) + // We just slash from the last chunk to the most recent one, if need be. + (None, (0..self.unlocking.len()).rev().collect::>()) } }; // Helper to update `target` and the ledgers total after accounting for slashing `target`. - let ratio = Perquintill::from_rational(slash_amount, affected_balance); + log!( + debug, + "slashing {:?} for era {:?} out of {:?}, priority: {:?}, proportional = {:?}", + slash_amount, + slash_era, + self, + slash_chunks_priority, + maybe_proportional, + ); + let mut slash_out_of = |target: &mut BalanceOf, slash_remaining: &mut BalanceOf| { - let mut slash_from_target = - if slash_amount < affected_balance { ratio * (*target) } else { *slash_remaining } - .min(*target); + let mut slash_from_target = if let Some(ratio) = maybe_proportional { + ratio * (*target) + } else { + *slash_remaining + } + // this is the total that that the slash target has. We can't slash more than + // this anyhow! + .min(*target) + // this is the total amount that we would have wanted to slash + // non-proportionally, a proportional slash should never exceed this either! + .min(*slash_remaining); // slash out from *target exactly `slash_from_target`. *target = *target - slash_from_target; if *target < minimum_balance { - // Slash the rest of the target if its dust + // Slash the rest of the target if it's dust. This might cause the last chunk to be + // slightly under-slashed, by at most `MaxUnlockingChunks * ED`, which is not a big + // deal. slash_from_target = sp_std::mem::replace(target, Zero::zero()).saturating_add(slash_from_target) } @@ -606,10 +646,11 @@ impl StakingLedger { let mut slashed_unlocking = BTreeMap::<_, _>::new(); for i in slash_chunks_priority { + if remaining_slash.is_zero() { + break + } + if let Some(chunk) = self.unlocking.get_mut(i).defensive() { - if remaining_slash.is_zero() { - break - } slash_out_of(&mut chunk.value, &mut remaining_slash); // write the new slashed value of this chunk to the map. slashed_unlocking.insert(chunk.era, chunk.value); @@ -618,7 +659,9 @@ impl StakingLedger { } } + // clean unlocking chunks that are set to zero. self.unlocking.retain(|c| !c.value.is_zero()); + T::OnStakerSlash::on_slash(&self.stash, self.active, &slashed_unlocking); pre_slash_total.saturating_sub(self.total) } diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 9a13a818f4..b76126f0c5 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -2081,8 +2081,7 @@ fn reward_validator_slashing_validator_does_not_overflow() { let _ = Balances::make_free_balance_be(&11, stake); let _ = Balances::make_free_balance_be(&2, stake); - // only slashes out of bonded stake are applied. without this line, - // it is 0. + // only slashes out of bonded stake are applied. without this line, it is 0. Staking::bond(Origin::signed(2), 20000, stake - 1, RewardDestination::default()).unwrap(); // Override exposure of 11 ErasStakers::::insert( @@ -2104,7 +2103,7 @@ fn reward_validator_slashing_validator_does_not_overflow() { &[Perbill::from_percent(100)], ); - assert_eq!(Balances::total_balance(&11), stake); + assert_eq!(Balances::total_balance(&11), stake - 1); assert_eq!(Balances::total_balance(&2), 1); }) } @@ -4960,7 +4959,6 @@ fn proportional_ledger_slash_works() { unlocking: bounded_vec![], claimed_rewards: vec![], }; - assert_eq!(BondingDuration::get(), 3); // When we slash a ledger with no unlocking chunks @@ -4997,7 +4995,7 @@ fn proportional_ledger_slash_works() { ledger.total = 4 * 100; ledger.active = 0; // When the first 2 chunks don't overlap with the affected range of unlock eras. - assert_eq!(ledger.slash(140, 0, 2), 140); + assert_eq!(ledger.slash(140, 0, 3), 140); // Then assert_eq!(ledger.unlocking, vec![c(4, 100), c(5, 100), c(6, 30), c(7, 30)]); assert_eq!(ledger.total, 4 * 100 - 140); @@ -5039,7 +5037,7 @@ fn proportional_ledger_slash_works() { ledger.active = 500; ledger.total = 40 + 10 + 100 + 250 + 500; // 900 assert_eq!(ledger.total, 900); - // When we have a higher min balance + // When we have a higher min balance assert_eq!( ledger.slash( 900 / 2, @@ -5047,16 +5045,17 @@ fn proportional_ledger_slash_works() { * get swept */ 0 ), - 475 + 450 ); - let dust = (10 / 2) + (40 / 2); assert_eq!(ledger.active, 500 / 2); - assert_eq!(ledger.unlocking, vec![c(5, 100 / 2), c(7, 250 / 2)]); - assert_eq!(ledger.total, 900 / 2 - dust); + // the last chunk was not slashed 50% like all the rest, because some other earlier chunks got + // dusted. + assert_eq!(ledger.unlocking, vec![c(5, 100 / 2), c(7, 150)]); + assert_eq!(ledger.total, 900 / 2); assert_eq!(LedgerSlashPerEra::get().0, 500 / 2); assert_eq!( LedgerSlashPerEra::get().1, - BTreeMap::from([(4, 0), (5, 100 / 2), (6, 0), (7, 250 / 2)]) + BTreeMap::from([(4, 0), (5, 100 / 2), (6, 0), (7, 150)]) ); // Given @@ -5068,7 +5067,7 @@ fn proportional_ledger_slash_works() { ledger.slash( 500 + 10 + 250 + 100 / 2, // active + era 6 + era 7 + era 5 / 2 0, - 2 /* slash era 2+4 first, so the affected parts are era 2+4, era 3+4 and + 3 /* slash era 6 first, so the affected parts are era 6, era 7 and * ledge.active. This will cause the affected to go to zero, and then we will * start slashing older chunks */ ), @@ -5091,7 +5090,7 @@ fn proportional_ledger_slash_works() { ledger.slash( 351, // active + era 6 + era 7 + era 5 / 2 + 1 50, // min balance - everything slashed below 50 will get dusted - 2 /* slash era 2+4 first, so the affected parts are era 2+4, era 3+4 and + 3 /* slash era 3+3 first, so the affected parts are era 6, era 7 and * ledge.active. This will cause the affected to go to zero, and then we will * start slashing older chunks */ ), @@ -5108,9 +5107,8 @@ fn proportional_ledger_slash_works() { // Given let slash = u64::MAX as Balance * 2; - let value = slash - - (9 * 4) // The value of the other parts of ledger that will get slashed - + 1; + // The value of the other parts of ledger that will get slashed + let value = slash - (10 * 4); ledger.active = 10; ledger.unlocking = bounded_vec![c(4, 10), c(5, 10), c(6, 10), c(7, value)];