From 9642ad4ce73189c79c30dfc78be1cc673c772dc3 Mon Sep 17 00:00:00 2001 From: Zeke Mostov <32168567+emostov@users.noreply.github.com> Date: Fri, 4 Mar 2022 13:25:12 -0800 Subject: [PATCH 01/18] staking: Proportional ledger slashing --- frame/staking/src/lib.rs | 171 +++++++++++++------- frame/staking/src/mock.rs | 13 ++ frame/staking/src/pallet/impls.rs | 9 +- frame/staking/src/pallet/mod.rs | 7 +- frame/staking/src/slashing.rs | 17 +- frame/staking/src/tests.rs | 249 ++++++++++++++++++++++++++++++ primitives/staking/src/lib.rs | 25 +++ 7 files changed, 426 insertions(+), 65 deletions(-) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index d833ac86fe0bd..9958461d92145 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -440,31 +440,30 @@ pub struct UnlockChunk { /// The ledger of a (bonded) stash. #[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, TypeInfo)] -pub struct StakingLedger { +#[scale_info(skip_type_params(T))] +pub struct StakingLedger { /// The stash account whose balance is actually locked and at stake. - pub stash: AccountId, + pub stash: T::AccountId, /// The total amount of the stash's balance that we are currently accounting for. /// It's just `active` plus all the `unlocking` balances. #[codec(compact)] - pub total: Balance, + pub total: BalanceOf, /// The total amount of the stash's balance that will be at stake in any forthcoming /// rounds. #[codec(compact)] - pub active: Balance, + pub active: BalanceOf, /// Any balance that is becoming free, which may eventually be transferred out of the stash /// (assuming it doesn't get slashed first). It is assumed that this will be treated as a first /// in, first out queue where the new (higher value) eras get pushed on the back. - pub unlocking: BoundedVec, MaxUnlockingChunks>, + pub unlocking: BoundedVec>, MaxUnlockingChunks>, /// List of eras for which the stakers behind a validator have claimed rewards. Only updated /// for validators. pub claimed_rewards: Vec, } -impl - StakingLedger -{ +impl StakingLedger { /// Initializes the default object using the given `validator`. - pub fn default_from(stash: AccountId) -> Self { + pub fn default_from(stash: T::AccountId) -> Self { Self { stash, total: Zero::zero(), @@ -507,8 +506,8 @@ impl (Self, Balance) { - let mut unlocking_balance: Balance = Zero::zero(); + fn rebond(mut self, value: BalanceOf) -> (Self, BalanceOf) { + let mut unlocking_balance = BalanceOf::::zero(); while let Some(last) = self.unlocking.last_mut() { if unlocking_balance + last.value <= value { @@ -530,57 +529,121 @@ impl StakingLedger -where - Balance: AtLeast32BitUnsigned + Saturating + Copy, -{ - /// Slash the validator for a given amount of balance. This can grow the value - /// of the slash in the case that the validator has less than `minimum_balance` - /// active funds. Returns the amount of funds actually slashed. + /// 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. /// - /// Slashes from `active` funds first, and then `unlocking`, starting with the - /// chunks that are closest to unlocking. - fn slash(&mut self, mut value: Balance, minimum_balance: Balance) -> Balance { - let pre_total = self.total; - let total = &mut self.total; - let active = &mut self.active; - - let slash_out_of = - |total_remaining: &mut Balance, target: &mut Balance, value: &mut Balance| { - let mut slash_from_target = (*value).min(*target); - - if !slash_from_target.is_zero() { - *target -= slash_from_target; - - // Don't leave a dust balance in the staking system. - if *target <= minimum_balance { - slash_from_target += *target; - *value += sp_std::mem::replace(target, Zero::zero()); - } + /// Note that this calls `Config::OnStakerSlash::on_slash` with information as to how the slash + /// was applied. + /// + /// Generally slashes are computed by: + /// + /// 1) Balances of the unlocking chunks in range `slash_era + 1..=apply_era` are summed and + /// stored in `total_balance_affected`. + /// + /// 2) `slash_ratio` is computed as `slash_amount / total_balance_affected`. + /// + /// 3) `Ledger::active` is set to `(1- slash_ratio) * Ledger::active`. + /// + /// 4) For all unlocking chunks created in range `slash_era + 1..=apply_era` set their balance + /// to `(1 - slash_ratio) * unbonding_pool_balance`. We start with slash_era + 1, because that + /// is the earliest the active sake while slash could be unbonded. + /// + /// 5) Slash any remaining slash amount from the remaining chunks, starting with the `slash_era` + /// and going backwards. + /// + /// There are some edge cases due to saturating arithmetic - see logic below and tests for + /// details. + fn slash( + &mut self, + slash_amount: BalanceOf, + minimum_balance: BalanceOf, + slash_era: EraIndex, + ) -> BalanceOf { + use sp_runtime::traits::CheckedMul as _; + use sp_staking::OnStakerSlash as _; + use sp_std::ops::Div as _; + if slash_amount.is_zero() { + return Zero::zero() + } - *total_remaining = total_remaining.saturating_sub(slash_from_target); - *value -= slash_from_target; - } + let mut remaining_slash = slash_amount; + let pre_slash_total = self.total; + + // The index of the first chunk after the slash era + let era_after_slash = slash_era + 1; + // When a user unbonds, the chunk is given the era `current_era + BondingDuration`. See + // logic in [`Self::unbond`]. + let era_of_chunks_created_after_slash = era_after_slash + T::BondingDuration::get(); + let start_index = + self.unlocking.partition_point(|c| c.era < era_of_chunks_created_after_slash); + // The indices of from 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(); + + // Calculate the total balance of active funds and unlocking funds in the affected range. + let affected_balance = { + let unbonding_affected_balance = + affected_indices.clone().fold(BalanceOf::::zero(), |sum, i| { + if let Some(chunk) = self.unlocking.get_mut(i) { + sum.saturating_add(chunk.value) + } else { + sum + } + }); + self.active.saturating_add(unbonding_affected_balance) + }; + + let is_proportional_slash = slash_amount < affected_balance; + // Helper to update `target` and the ledgers total after accounting for slashing `target`. + let mut slash_out_of = |target: &mut BalanceOf, slash_remaining: &mut BalanceOf| { + let maybe_numerator = slash_amount.checked_mul(target); + // // Calculate the amount to slash from the target + let slash_from_target = match (maybe_numerator, is_proportional_slash) { + // Equivalent to `(slash_amount / affected_balance) * target`. + (Some(numerator), true) => numerator.div(affected_balance), + (None, _) | (_, false) => (*target).min(*slash_remaining), }; - slash_out_of(total, active, &mut value); + *target = *target - slash_from_target; // slash_from_target cannot be gt target. - let i = self - .unlocking - .iter_mut() - .map(|chunk| { - slash_out_of(total, &mut chunk.value, &mut value); - chunk.value - }) - .take_while(|value| value.is_zero()) // Take all fully-consumed chunks out. - .count(); + let actual_slashed = if *target <= minimum_balance { + // Slash the rest of the target if its dust + sp_std::mem::replace(target, Zero::zero()).saturating_add(slash_from_target) + } else { + slash_from_target + }; + + self.total = self.total.saturating_sub(actual_slashed); + *slash_remaining = slash_remaining.saturating_sub(actual_slashed); + }; + + // If this is *not* a proportional slash, the active will always wiped to 0. + slash_out_of(&mut self.active, &mut remaining_slash); + + let mut slashed_unlocking = BTreeMap::<_, _>::new(); + let indices_to_slash + // First slash unbonding chunks from after the slash + = affected_indices + // Then start slashing older chunks, start from the era before the slash + .chain((0..start_index).rev()); + for i in indices_to_slash { + if let Some(chunk) = self.unlocking.get_mut(i) { + slash_out_of(&mut chunk.value, &mut remaining_slash); + slashed_unlocking.insert(chunk.era, chunk.value); + + if remaining_slash.is_zero() { + break + } + } else { + break // defensive, indices should always be in bounds. + } + } - // Kill all drained chunks. - let _ = self.unlocking.drain(..i); + T::OnStakerSlash::on_slash(&self.stash, self.active, &slashed_unlocking); - pre_total.saturating_sub(*total) + pre_slash_total.saturating_sub(self.total) } } diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index 12d3804b4e303..fa7524f5b3dcc 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -235,6 +235,7 @@ const THRESHOLDS: [sp_npos_elections::VoteWeight; 9] = parameter_types! { pub static BagThresholds: &'static [sp_npos_elections::VoteWeight] = &THRESHOLDS; pub static MaxNominations: u32 = 16; + pub static LedgerSlashPerEra: (BalanceOf, BTreeMap>) = (Zero::zero(), BTreeMap::new()); } impl pallet_bags_list::Config for Test { @@ -249,6 +250,17 @@ impl onchain::Config for Test { type DataProvider = Staking; } +pub struct OnStakerSlashMock(core::marker::PhantomData); +impl sp_staking::OnStakerSlash for OnStakerSlashMock { + fn on_slash( + _pool_account: &AccountId, + slashed_bonded: Balance, + slashed_chunks: &BTreeMap, + ) { + LedgerSlashPerEra::set((slashed_bonded, slashed_chunks.clone())); + } +} + impl crate::pallet::pallet::Config for Test { type MaxNominations = MaxNominations; type Currency = Balances; @@ -272,6 +284,7 @@ impl crate::pallet::pallet::Config for Test { // NOTE: consider a macro and use `UseNominatorsMap` as well. type SortedListProvider = BagsList; type MaxUnlockingChunks = ConstU32<32>; + type OnStakerSlash = OnStakerSlashMock; type BenchmarkingConfig = TestBenchmarkingConfig; type WeightInfo = (); } diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 5cd0d0107f015..b91eab8f6c3e1 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -201,10 +201,7 @@ impl Pallet { /// Update the ledger for a controller. /// /// This will also update the stash lock. - pub(crate) fn update_ledger( - controller: &T::AccountId, - ledger: &StakingLedger>, - ) { + pub(crate) fn update_ledger(controller: &T::AccountId, ledger: &StakingLedger) { T::Currency::set_lock(STAKING_ID, &ledger.stash, ledger.total, WithdrawReasons::all()); >::insert(controller, ledger); } @@ -594,7 +591,7 @@ impl Pallet { for era in (*earliest)..keep_from { let era_slashes = ::UnappliedSlashes::take(&era); for slash in era_slashes { - slashing::apply_slash::(slash); + slashing::apply_slash::(slash, era); } } @@ -1219,7 +1216,7 @@ where unapplied.reporters = details.reporters.clone(); if slash_defer_duration == 0 { // Apply right away. - slashing::apply_slash::(unapplied); + slashing::apply_slash::(unapplied, slash_era); { let slash_cost = (6, 5); let reward_cost = (2, 2); diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 58f9fd237263b..dcdde1a8f279d 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -173,6 +173,10 @@ pub mod pallet { #[pallet::constant] type MaxUnlockingChunks: Get; + /// A hook called when any staker is slashed. Mostly likely this can be a no-op unless + /// there are delegation pools. + type OnStakerSlash: sp_staking::OnStakerSlash>; + /// Some parameters of the benchmarking. type BenchmarkingConfig: BenchmarkingConfig; @@ -235,8 +239,7 @@ pub mod pallet { /// Map from all (unlocked) "controller" accounts to the info regarding the staking. #[pallet::storage] #[pallet::getter(fn ledger)] - pub type Ledger = - StorageMap<_, Blake2_128Concat, T::AccountId, StakingLedger>>; + pub type Ledger = StorageMap<_, Blake2_128Concat, T::AccountId, StakingLedger>; /// Where the reward payment should be made. Keyed by stash. #[pallet::storage] diff --git a/frame/staking/src/slashing.rs b/frame/staking/src/slashing.rs index 2f381ad631fe5..9fc50eaf538f6 100644 --- a/frame/staking/src/slashing.rs +++ b/frame/staking/src/slashing.rs @@ -598,6 +598,7 @@ pub fn do_slash( value: BalanceOf, reward_payout: &mut BalanceOf, slashed_imbalance: &mut NegativeImbalanceOf, + slash_era: EraIndex, ) { let controller = match >::bonded(stash) { None => return, // defensive: should always exist. @@ -609,7 +610,7 @@ pub fn do_slash( None => return, // nothing to do. }; - let value = ledger.slash(value, T::Currency::minimum_balance()); + let value = ledger.slash(value, T::Currency::minimum_balance(), slash_era); if !value.is_zero() { let (imbalance, missing) = T::Currency::slash(stash, value); @@ -628,7 +629,10 @@ pub fn do_slash( } /// Apply a previously-unapplied slash. -pub(crate) fn apply_slash(unapplied_slash: UnappliedSlash>) { +pub(crate) fn apply_slash( + unapplied_slash: UnappliedSlash>, + slash_era: EraIndex, +) { let mut slashed_imbalance = NegativeImbalanceOf::::zero(); let mut reward_payout = unapplied_slash.payout; @@ -637,10 +641,17 @@ pub(crate) fn apply_slash(unapplied_slash: UnappliedSlash(&nominator, nominator_slash, &mut reward_payout, &mut slashed_imbalance); + do_slash::( + &nominator, + nominator_slash, + &mut reward_payout, + &mut slashed_imbalance, + slash_era, + ); } pay_reporters::(reward_payout, slashed_imbalance, &unapplied_slash.reporters); diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 2b2a32e0edab7..70f754056d9da 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -4801,3 +4801,252 @@ fn force_apply_min_commission_works() { ); }); } + +#[test] +fn ledger_slash_works() { + let c = |era, value| UnlockChunk:: { era, value }; + // Given + let mut ledger = StakingLedger:: { + stash: 123, + total: 10, + active: 10, + unlocking: bounded_vec![], + claimed_rewards: vec![], + }; + + assert_eq!(BondingDuration::get(), 3); + + // When we slash a ledger with no unlocking chunks + assert_eq!(ledger.slash(5, 1, 0), 5); + // Then + assert_eq!(ledger.total, 5); + assert_eq!(ledger.active, 5); + assert_eq!(LedgerSlashPerEra::get().0, 5); + assert_eq!(LedgerSlashPerEra::get().1, Default::default()); + + // When we slash a ledger with no unlocking chunks and the slash amount is greater then the + // total + assert_eq!(ledger.slash(11, 1, 0), 5); + // Then + assert_eq!(ledger.total, 0); + assert_eq!(ledger.active, 0); + assert_eq!(LedgerSlashPerEra::get().0, 0); + assert_eq!(LedgerSlashPerEra::get().1, Default::default()); + + // Given + ledger.unlocking = bounded_vec![c(4, 10), c(5, 10)]; + ledger.total = 2 * 10; + ledger.active = 0; + // When all the chunks overlap with the slash eras + assert_eq!(ledger.slash(20, 0, 0), 20); + // Then + assert_eq!(ledger.unlocking, vec![c(4, 0), c(5, 0)]); + assert_eq!(ledger.total, 0); + assert_eq!(LedgerSlashPerEra::get().0, 0); + assert_eq!(LedgerSlashPerEra::get().1, BTreeMap::from([(4, 0), (5, 0)])); + + // Given + ledger.unlocking = bounded_vec![c(4, 40), c(5, 100), c(6, 10), c(7, 250)]; + ledger.active = 500; + ledger.total = 40 + 10 + 100 + 250 + 500; // 900 + + // When we have a partial slash that touches all chunks + assert_eq!(ledger.slash(900 / 2, 0, 0), 450); + + // Then + assert_eq!(ledger.active, 500 / 2); + assert_eq!(ledger.unlocking, vec![c(4, 40 / 2), c(5, 100 / 2), c(6, 10 / 2), c(7, 250 / 2)]); + assert_eq!(ledger.total, 900 / 2); + assert_eq!(LedgerSlashPerEra::get().0, 500 / 2); + assert_eq!( + LedgerSlashPerEra::get().1, + BTreeMap::from([(4, 40 / 2), (5, 100 / 2), (6, 10 / 2), (7, 250 / 2)]) + ); + + // Given we have the same as above, + ledger.unlocking = bounded_vec![c(4, 40), c(5, 100), c(6, 10), c(7, 250)]; + ledger.active = 500; + ledger.total = 40 + 10 + 100 + 250 + 500; // 900 + assert_eq!(ledger.total, 900); + + // When we have a higher min balance + assert_eq!( + ledger.slash( + 900 / 2, + 25, /* min balance - chunks with era 0 & 2 will be slashed to <=25, causing it to + * get swept */ + 0 + ), + 475 + ); + + // Then + assert_eq!(ledger.active, 500 / 2); + assert_eq!(ledger.unlocking, vec![c(4, 0), c(5, 100 / 2), c(6, 0), c(7, 250 / 2)]); + assert_eq!(ledger.total, 425); + assert_eq!(LedgerSlashPerEra::get().0, 500 / 2); + assert_eq!( + LedgerSlashPerEra::get().1, + BTreeMap::from([(4, 0), (5, 100 / 2), (6, 0), (7, 250 / 2)]) + ); + + // Given we have the same as above, + ledger.unlocking = bounded_vec![c(4, 40), c(5, 100), c(6, 10), c(7, 250)]; + ledger.active = 500; + ledger.total = 40 + 10 + 100 + 250 + 500; // 900 + + // When + assert_eq!( + ledger.slash( + 900 / 2, + 25, /* min balance - chunks with era 0 & 2 will be slashed to <=25, causing it to + * get swept */ + 0 + ), + 475 + ); + + // Then + assert_eq!(ledger.active, 500 / 2); + assert_eq!(ledger.unlocking, vec![c(4, 0), c(5, 100 / 2), c(6, 0), c(7, 250 / 2)]); + assert_eq!(ledger.total, 425); + assert_eq!(LedgerSlashPerEra::get().0, 500 / 2); + assert_eq!( + LedgerSlashPerEra::get().1, + BTreeMap::from([(4, 0), (5, 100 / 2), (6, 0), (7, 250 / 2)]) + ); + + // Given + ledger.unlocking = bounded_vec![c(4, 40), c(5, 100), c(6, 10), c(7, 250)]; + ledger.active = 500; + ledger.total = 40 + 10 + 100 + 250 + 500; // 900 + + // When + assert_eq!( + 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 + * ledge.active. This will cause the affected to go to zero, and then we will + * start slashing older chunks */ + ), + 500 + 250 + 10 + 100 / 2 + ); + + assert_eq!(ledger.active, 0); + // iteration order ------------------NA----------2-------------0--------1---- + assert_eq!(ledger.unlocking, vec![c(4, 40), c(5, 100 / 2), c(6, 0), c(7, 0)]); + assert_eq!(ledger.total, 90); + assert_eq!(LedgerSlashPerEra::get().0, 0); + assert_eq!(LedgerSlashPerEra::get().1, BTreeMap::from([(5, 100 / 2), (6, 0), (7, 0)])); + + // Given + ledger.unlocking = bounded_vec![c(4, 100), c(5, 100), c(6, 100), c(7, 100)]; + ledger.active = 100; + ledger.total = 5 * 100; + + // When + assert_eq!( + ledger.slash( + 350, // active + era 2 + era 3 + era 1 / 2 + 50, + 2 /* slash era 2+4 first, so the affected parts are era 2+4, era 3+4 and + * ledge.active. This will cause the affected to go to zero, and then we will + * start slashing older chunks */ + ), + 400 + ); + + // Then + assert_eq!(ledger.active, 0); + // iteration order ------------------NA---------2--------0--------1---- + assert_eq!(ledger.unlocking, vec![c(4, 100), c(5, 0), c(6, 0), c(7, 0)]); + //------goes to min balance and then gets dusted^^^ + assert_eq!(ledger.total, 100); + assert_eq!(LedgerSlashPerEra::get().0, 0); + assert_eq!(LedgerSlashPerEra::get().1, BTreeMap::from([(5, 0), (6, 0), (7, 0)])); + + // Tests for saturating arithmetic + + // 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; + // slash * value will saturate + assert!(slash.checked_mul(value - 20).is_none()); + + ledger.active = 10; + ledger.unlocking = bounded_vec![c(4, 10), c(5, 10), c(6, 10), c(7, value)]; + + ledger.total = value + 40; + + // When + assert_eq!(ledger.slash(slash, 0, 0), slash); + + // Then + assert_eq!(ledger.active, 1); // slash of 9 + assert_eq!( + ledger.unlocking, + vec![ + c(4, 1), // slash of 9 + c(5, 1), // slash of 9 + c(6, 1), // slash of 9 ------------(slash - 9 * 4).min(value) + c(7, 1), // saturates, so slash of remaining_slash.min(value) + ] + ); + assert_eq!(ledger.total, 5); + assert_eq!(LedgerSlashPerEra::get().0, 1); + assert_eq!(LedgerSlashPerEra::get().1, BTreeMap::from([(4, 1), (5, 1), (6, 1), (7, 1)])); + + // Given + let slash = u64::MAX as Balance * 2; + let value = u64::MAX as Balance * 2; + let unit = 100; + // slash * value that will saturate + assert!(slash.checked_mul(value).is_none()); + // but slash * unit. + assert!(slash.checked_mul(unit).is_some()); + + ledger.unlocking = bounded_vec![c(4, unit), c(5, value), c(6, unit), c(7, unit)]; + //--------------------------------------note value^^^ + ledger.active = unit; + ledger.total = unit * 4 + value; + + // When + assert_eq!(ledger.slash(slash, 0, 0), slash); + + // Then + + // The amount slashed out of `unit` + let unit_slash = { + let affected_balance = value + unit * 4; + slash * unit / affected_balance + }; + // `unit` after the slash is applied + let unit_slashed = unit - unit_slash; + + // `value` after the slash is applied + let value_slashed = { + // We slash active and era 1 before we slash era 2 + let previous_slashed_amount = unit_slash * 2; + let remaining_slash = slash - previous_slashed_amount; + value - remaining_slash + }; + assert_eq!(ledger.active, unit_slashed); + assert_eq!( + ledger.unlocking, + vec![ + c(4, unit_slashed), + // We reached the slash here because we slashed value.min(remaining_slash), which was + // remaining_slash + c(5, value_slashed), + c(6, unit), /* The rest are untouched even though they where in the slashing range. + * This is problematic for pools, but should be rare enough that its ok. */ + c(7, unit) + ] + ); + assert_eq!(ledger.total, unit_slashed + unit_slashed + value_slashed + unit + unit); + assert_eq!(LedgerSlashPerEra::get().0, unit_slashed); + assert_eq!(LedgerSlashPerEra::get().1, BTreeMap::from([(4, unit_slashed), (5, value_slashed)])); +} diff --git a/primitives/staking/src/lib.rs b/primitives/staking/src/lib.rs index 15208df62cc66..09eff0717c8e8 100644 --- a/primitives/staking/src/lib.rs +++ b/primitives/staking/src/lib.rs @@ -19,6 +19,7 @@ //! A crate which contains primitives that are useful for implementation that uses staking //! approaches in general. Definitions related to sessions, slashing, etc go here. +use sp_std::collections::btree_map::BTreeMap; pub mod offence; @@ -27,3 +28,27 @@ pub type SessionIndex = u32; /// Counter for the number of eras that have passed. pub type EraIndex = u32; + +/// Trait describing something that implements a hook for any operations to perform when a staker is +/// slashed. +pub trait OnStakerSlash { + /// A hook for any operations to perform when a staker is slashed. + /// + /// # Arguments + /// + /// * `stash` - The stash of the staker whom the slash was applied to. + /// * `slashed_bonded` - The new bonded balance of the staker after the slash was applied. + /// * `slashed_unlocking` - A map from eras that the staker is unbonding in to the new balance + /// after the slash was applied. + fn on_slash( + stash: &AccountId, + slashed_bonded: Balance, + slashed_unlocking: &BTreeMap, + ); +} + +impl OnStakerSlash for () { + fn on_slash(_: &AccountId, _: Balance, _: &BTreeMap) { + // Nothing to do here + } +} From 2a3609d5851040d2cb3c434af631408489e5b329 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Fri, 4 Mar 2022 14:14:20 -0800 Subject: [PATCH 02/18] Some comment cleanup --- frame/staking/src/lib.rs | 8 +++--- frame/staking/src/pallet/mod.rs | 2 +- frame/staking/src/tests.rs | 50 +++++++++++---------------------- 3 files changed, 21 insertions(+), 39 deletions(-) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 9958461d92145..11d726ba562dc 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -571,14 +571,13 @@ impl StakingLedger { let mut remaining_slash = slash_amount; let pre_slash_total = self.total; - // The index of the first chunk after the slash era let era_after_slash = slash_era + 1; // When a user unbonds, the chunk is given the era `current_era + BondingDuration`. See // logic in [`Self::unbond`]. let era_of_chunks_created_after_slash = era_after_slash + T::BondingDuration::get(); let start_index = self.unlocking.partition_point(|c| c.era < era_of_chunks_created_after_slash); - // The indices of from the first chunk after the slash up through the most recent chunk. + // 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(); @@ -599,10 +598,11 @@ impl StakingLedger { // Helper to update `target` and the ledgers total after accounting for slashing `target`. let mut slash_out_of = |target: &mut BalanceOf, slash_remaining: &mut BalanceOf| { let maybe_numerator = slash_amount.checked_mul(target); - // // Calculate the amount to slash from the target let slash_from_target = match (maybe_numerator, is_proportional_slash) { // Equivalent to `(slash_amount / affected_balance) * target`. (Some(numerator), true) => numerator.div(affected_balance), + // If the slash amount is gt than the affected balance OR the arithmetic to + // calculate the proportion saturated, we just try to slash as much as possible. (None, _) | (_, false) => (*target).min(*slash_remaining), }; @@ -626,7 +626,7 @@ impl StakingLedger { let indices_to_slash // First slash unbonding chunks from after the slash = affected_indices - // Then start slashing older chunks, start from the era before the slash + // Then start slashing older chunks, start from the era of the slash .chain((0..start_index).rev()); for i in indices_to_slash { if let Some(chunk) = self.unlocking.get_mut(i) { diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index dcdde1a8f279d..bc5207d91dda7 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -174,7 +174,7 @@ pub mod pallet { type MaxUnlockingChunks: Get; /// A hook called when any staker is slashed. Mostly likely this can be a no-op unless - /// there are delegation pools. + /// there are nomination pools. type OnStakerSlash: sp_staking::OnStakerSlash>; /// Some parameters of the benchmarking. diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 70f754056d9da..c847157c5c120 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -4849,10 +4849,8 @@ fn ledger_slash_works() { ledger.unlocking = bounded_vec![c(4, 40), c(5, 100), c(6, 10), c(7, 250)]; ledger.active = 500; ledger.total = 40 + 10 + 100 + 250 + 500; // 900 - - // When we have a partial slash that touches all chunks + // When we have a partial slash that touches all chunks assert_eq!(ledger.slash(900 / 2, 0, 0), 450); - // Then assert_eq!(ledger.active, 500 / 2); assert_eq!(ledger.unlocking, vec![c(4, 40 / 2), c(5, 100 / 2), c(6, 10 / 2), c(7, 250 / 2)]); @@ -4868,7 +4866,6 @@ fn 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 assert_eq!( ledger.slash( @@ -4879,7 +4876,6 @@ fn ledger_slash_works() { ), 475 ); - // Then assert_eq!(ledger.active, 500 / 2); assert_eq!(ledger.unlocking, vec![c(4, 0), c(5, 100 / 2), c(6, 0), c(7, 250 / 2)]); @@ -4894,18 +4890,16 @@ fn ledger_slash_works() { ledger.unlocking = bounded_vec![c(4, 40), c(5, 100), c(6, 10), c(7, 250)]; ledger.active = 500; ledger.total = 40 + 10 + 100 + 250 + 500; // 900 - - // When + // When assert_eq!( ledger.slash( 900 / 2, - 25, /* min balance - chunks with era 0 & 2 will be slashed to <=25, causing it to + 25, /* min balance - chunks with era 5 & 5 will be slashed to <=25, causing them to * get swept */ 0 ), 475 ); - // Then assert_eq!(ledger.active, 500 / 2); assert_eq!(ledger.unlocking, vec![c(4, 0), c(5, 100 / 2), c(6, 0), c(7, 250 / 2)]); @@ -4920,8 +4914,7 @@ fn ledger_slash_works() { ledger.unlocking = bounded_vec![c(4, 40), c(5, 100), c(6, 10), c(7, 250)]; ledger.active = 500; ledger.total = 40 + 10 + 100 + 250 + 500; // 900 - - // When + // When assert_eq!( ledger.slash( 500 + 10 + 250 + 100 / 2, // active + era 6 + era 7 + era 5 / 2 @@ -4932,7 +4925,7 @@ fn ledger_slash_works() { ), 500 + 250 + 10 + 100 / 2 ); - + // Then assert_eq!(ledger.active, 0); // iteration order ------------------NA----------2-------------0--------1---- assert_eq!(ledger.unlocking, vec![c(4, 40), c(5, 100 / 2), c(6, 0), c(7, 0)]); @@ -4944,19 +4937,17 @@ fn ledger_slash_works() { ledger.unlocking = bounded_vec![c(4, 100), c(5, 100), c(6, 100), c(7, 100)]; ledger.active = 100; ledger.total = 5 * 100; - // When assert_eq!( ledger.slash( - 350, // active + era 2 + era 3 + era 1 / 2 - 50, - 2 /* slash era 2+4 first, so the affected parts are era 2+4, era 3+4 and - * ledge.active. This will cause the affected to go to zero, and then we will - * start slashing older chunks */ + 350, // active + era 6 + era 7 + era 5 / 2 + 50, // min balance - everything slashed to 50 or below will get dusted + 2 /* slash era 2+4 first, so the affected parts are era 2+4, era 3+4 and + * ledge.active. This will cause the affected to go to zero, and then we will + * start slashing older chunks */ ), 400 ); - // Then assert_eq!(ledger.active, 0); // iteration order ------------------NA---------2--------0--------1---- @@ -4975,15 +4966,11 @@ fn ledger_slash_works() { + 1; // slash * value will saturate assert!(slash.checked_mul(value - 20).is_none()); - ledger.active = 10; ledger.unlocking = bounded_vec![c(4, 10), c(5, 10), c(6, 10), c(7, value)]; - ledger.total = value + 40; - // When assert_eq!(ledger.slash(slash, 0, 0), slash); - // Then assert_eq!(ledger.active, 1); // slash of 9 assert_eq!( @@ -4991,8 +4978,8 @@ fn ledger_slash_works() { vec![ c(4, 1), // slash of 9 c(5, 1), // slash of 9 - c(6, 1), // slash of 9 ------------(slash - 9 * 4).min(value) - c(7, 1), // saturates, so slash of remaining_slash.min(value) + c(6, 1), // slash of 9 + c(7, 1), // saturates, so slash of remaining_slash.min(value) equivalent of (slash - 9 * 4).min(value) ] ); assert_eq!(ledger.total, 5); @@ -5005,19 +4992,15 @@ fn ledger_slash_works() { let unit = 100; // slash * value that will saturate assert!(slash.checked_mul(value).is_none()); - // but slash * unit. + // but slash * unit won't. assert!(slash.checked_mul(unit).is_some()); - ledger.unlocking = bounded_vec![c(4, unit), c(5, value), c(6, unit), c(7, unit)]; //--------------------------------------note value^^^ ledger.active = unit; ledger.total = unit * 4 + value; - // When assert_eq!(ledger.slash(slash, 0, 0), slash); - // Then - // The amount slashed out of `unit` let unit_slash = { let affected_balance = value + unit * 4; @@ -5025,10 +5008,9 @@ fn ledger_slash_works() { }; // `unit` after the slash is applied let unit_slashed = unit - unit_slash; - // `value` after the slash is applied let value_slashed = { - // We slash active and era 1 before we slash era 2 + // We slash active and era 4 before we slash era 5 let previous_slashed_amount = unit_slash * 2; let remaining_slash = slash - previous_slashed_amount; value - remaining_slash @@ -5038,8 +5020,8 @@ fn ledger_slash_works() { ledger.unlocking, vec![ c(4, unit_slashed), - // We reached the slash here because we slashed value.min(remaining_slash), which was - // remaining_slash + // We reached the full slash amount here because we slashed value.min(remaining_slash), + // which was remaining_slash c(5, value_slashed), c(6, unit), /* The rest are untouched even though they where in the slashing range. * This is problematic for pools, but should be rare enough that its ok. */ From f071b2df871f6ee16b6ff11e47f050038b3f3e5a Mon Sep 17 00:00:00 2001 From: Zeke Mostov Date: Sun, 6 Mar 2022 10:55:31 -0800 Subject: [PATCH 03/18] Update frame/staking/src/pallet/mod.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- frame/staking/src/pallet/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index bc5207d91dda7..fb37b4aec1734 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -173,8 +173,8 @@ pub mod pallet { #[pallet::constant] type MaxUnlockingChunks: Get; - /// A hook called when any staker is slashed. Mostly likely this can be a no-op unless - /// there are nomination pools. + /// A hook called when any staker is slashed. Mostly likely this can be a no-op unless + /// other pallets exist that are affected by slashing per-staker. type OnStakerSlash: sp_staking::OnStakerSlash>; /// Some parameters of the benchmarking. From b5598a4789abde59d98a1cb38beddcb4e4738368 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Mon, 7 Mar 2022 12:45:27 +0100 Subject: [PATCH 04/18] Fix benchmarks --- frame/staking/src/benchmarking.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index ec7d36c820f3e..fa4624f2d63e8 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -806,7 +806,8 @@ benchmarks! { &stash, slash_amount, &mut BalanceOf::::zero(), - &mut NegativeImbalanceOf::::zero() + &mut NegativeImbalanceOf::::zero(), + EraIndex::zero() ); } verify { let balance_after = T::Currency::free_balance(&stash); From 738b7bbd43eb7caaacb82eef81fce8981fea2580 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 15 Mar 2022 18:08:35 +0000 Subject: [PATCH 05/18] FMT --- frame/staking/src/pallet/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index a171a70bdb062..7c99537b51be6 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -176,7 +176,7 @@ pub mod pallet { #[pallet::constant] type MaxUnlockingChunks: Get; - /// A hook called when any staker is slashed. Mostly likely this can be a no-op unless + /// A hook called when any staker is slashed. Mostly likely this can be a no-op unless /// other pallets exist that are affected by slashing per-staker. type OnStakerSlash: sp_staking::OnStakerSlash>; From ad551f310cf4ab82978e369295679296ebc93eb0 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 15 Mar 2022 18:14:12 +0000 Subject: [PATCH 06/18] Try fill in all staking configs --- bin/node/runtime/src/lib.rs | 1 + frame/babe/src/mock.rs | 1 + frame/grandpa/src/mock.rs | 1 + frame/offences/benchmarking/src/mock.rs | 1 + frame/session/benchmarking/src/mock.rs | 1 + 5 files changed, 5 insertions(+) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 20b718e2fa8f7..b6abc40e66f98 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -559,6 +559,7 @@ impl pallet_staking::Config for Runtime { // Note that the aforementioned does not scale to a very large number of nominators. type SortedListProvider = BagsList; type MaxUnlockingChunks = ConstU32<32>; + type OnStakerSlash = (); type WeightInfo = pallet_staking::weights::SubstrateWeight; type BenchmarkingConfig = StakingBenchmarkingConfig; } diff --git a/frame/babe/src/mock.rs b/frame/babe/src/mock.rs index 152ec5ab206e7..0518b0f3b6644 100644 --- a/frame/babe/src/mock.rs +++ b/frame/babe/src/mock.rs @@ -199,6 +199,7 @@ impl pallet_staking::Config for Test { type GenesisElectionProvider = Self::ElectionProvider; type SortedListProvider = pallet_staking::UseNominatorsMap; type MaxUnlockingChunks = ConstU32<32>; + type OnStakerSlash = (); type BenchmarkingConfig = pallet_staking::TestBenchmarkingConfig; type WeightInfo = (); } diff --git a/frame/grandpa/src/mock.rs b/frame/grandpa/src/mock.rs index 9dac33f979841..aef87b66920e5 100644 --- a/frame/grandpa/src/mock.rs +++ b/frame/grandpa/src/mock.rs @@ -207,6 +207,7 @@ impl pallet_staking::Config for Test { type GenesisElectionProvider = Self::ElectionProvider; type SortedListProvider = pallet_staking::UseNominatorsMap; type MaxUnlockingChunks = ConstU32<32>; + type OnStakerSlash = (); type BenchmarkingConfig = pallet_staking::TestBenchmarkingConfig; type WeightInfo = (); } diff --git a/frame/offences/benchmarking/src/mock.rs b/frame/offences/benchmarking/src/mock.rs index 22c9af0f4c3cd..5937f1cdc29af 100644 --- a/frame/offences/benchmarking/src/mock.rs +++ b/frame/offences/benchmarking/src/mock.rs @@ -177,6 +177,7 @@ impl pallet_staking::Config for Test { type GenesisElectionProvider = Self::ElectionProvider; type SortedListProvider = pallet_staking::UseNominatorsMap; type MaxUnlockingChunks = ConstU32<32>; + type OnStakerSlash = (); type BenchmarkingConfig = pallet_staking::TestBenchmarkingConfig; type WeightInfo = (); } diff --git a/frame/session/benchmarking/src/mock.rs b/frame/session/benchmarking/src/mock.rs index a9328b6546c91..593b532a303e6 100644 --- a/frame/session/benchmarking/src/mock.rs +++ b/frame/session/benchmarking/src/mock.rs @@ -183,6 +183,7 @@ impl pallet_staking::Config for Test { type GenesisElectionProvider = Self::ElectionProvider; type MaxUnlockingChunks = ConstU32<32>; type SortedListProvider = pallet_staking::UseNominatorsMap; + type OnStakerSlash = (); type BenchmarkingConfig = pallet_staking::TestBenchmarkingConfig; type WeightInfo = (); } From 3b975345d5b1060abe8f2be842ba2e1da0a2723d Mon Sep 17 00:00:00 2001 From: kianenigma Date: Wed, 16 Mar 2022 17:55:41 +0000 Subject: [PATCH 07/18] round of feedback and imp from kian --- frame/staking/src/lib.rs | 137 ++++++++++++++++---------------- frame/staking/src/pallet/mod.rs | 4 +- frame/staking/src/tests.rs | 98 +++++++++++++++-------- primitives/staking/src/lib.rs | 8 +- 4 files changed, 139 insertions(+), 108 deletions(-) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 1d1964a399feb..af7a059f66a4c 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -302,7 +302,7 @@ mod pallet; use codec::{Decode, Encode, HasCompact}; use frame_support::{ parameter_types, - traits::{Currency, Get}, + traits::{Currency, Defensive, Get}, weights::Weight, BoundedVec, EqNoBound, PartialEqNoBound, RuntimeDebugNoBound, }; @@ -435,7 +435,7 @@ pub struct UnlockChunk { value: Balance, /// Era number at which point it'll be unlocked. #[codec(compact)] - era: EraIndex, + unlock_era: EraIndex, } /// The ledger of a (bonded) stash. @@ -481,7 +481,7 @@ impl StakingLedger { .unlocking .into_iter() .filter(|chunk| { - if chunk.era > current_era { + if chunk.unlock_era > current_era { true } else { total = total.saturating_sub(chunk.value); @@ -530,31 +530,30 @@ 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 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. /// /// Note that this calls `Config::OnStakerSlash::on_slash` with information as to how the slash /// was applied. - /// - /// Generally slashes are computed by: - /// - /// 1) Balances of the unlocking chunks in range `slash_era + 1..=apply_era` are summed and - /// stored in `total_balance_affected`. - /// - /// 2) `slash_ratio` is computed as `slash_amount / total_balance_affected`. - /// - /// 3) `Ledger::active` is set to `(1- slash_ratio) * Ledger::active`. - /// - /// 4) For all unlocking chunks created in range `slash_era + 1..=apply_era` set their balance - /// to `(1 - slash_ratio) * unbonding_pool_balance`. We start with slash_era + 1, because that - /// is the earliest the active sake while slash could be unbonded. - /// - /// 5) Slash any remaining slash amount from the remaining chunks, starting with the `slash_era` - /// and going backwards. - /// - /// There are some edge cases due to saturating arithmetic - see logic below and tests for - /// details. + // Generally slashes are computed by: + // + // 1) Balances of the unlocking chunks in range `slash_era + 1..=apply_era` are summed and + // stored in `total_balance_affected`. + // + // 2) `slash_ratio` is computed as `slash_amount / total_balance_affected`. + // + // 3) `Ledger::active` is set to `(1- slash_ratio) * Ledger::active`. + // + // 4) For all unlocking chunks created in range `slash_era + 1..=apply_era` set their balance + // to `(1 - slash_ratio) * unbonding_pool_balance`. We start with slash_era + 1, because that + // is the earliest the active sake while slash could be unbonded. + // + // 5) Slash any remaining slash amount from the remaining chunks, starting with the `slash_era` + // and going backwards. + // + // There are some edge cases due to saturating arithmetic - see logic below and tests for + // details. fn slash( &mut self, slash_amount: BalanceOf, @@ -564,6 +563,7 @@ impl StakingLedger { use sp_runtime::traits::CheckedMul as _; use sp_staking::OnStakerSlash as _; use sp_std::ops::Div as _; + if slash_amount.is_zero() { return Zero::zero() } @@ -572,77 +572,80 @@ impl StakingLedger { let pre_slash_total = self.total; let era_after_slash = slash_era + 1; - // When a user unbonds, the chunk is given the era `current_era + BondingDuration`. See - // logic in [`Self::unbond`]. - let era_of_chunks_created_after_slash = era_after_slash + T::BondingDuration::get(); - let start_index = - self.unlocking.partition_point(|c| c.era < era_of_chunks_created_after_slash); - // 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(); + // TODO: test to make sure chunks before the slash are never slashed. + // at this era onwards, the funds can be slashed. + let chunk_unlock_era_after_slash = era_after_slash + T::BondingDuration::get(); // Calculate the total balance of active funds and unlocking funds in the affected range. - let affected_balance = { - let unbonding_affected_balance = - affected_indices.clone().fold(BalanceOf::::zero(), |sum, i| { - if let Some(chunk) = self.unlocking.get_mut(i) { - sum.saturating_add(chunk.value) - } else { - sum - } - }); - self.active.saturating_add(unbonding_affected_balance) + let (affected_balance, slash_chunks_priority): (_, Box>) = { + if let Some(start_index) = + self.unlocking.iter().position(|c| c.unlock_era >= chunk_unlock_era_after_slash) + { + // 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 unbonding_affected_balance = + affected_indices.clone().fold(BalanceOf::::zero(), |sum, i| { + if let Some(chunk) = self.unlocking.get_mut(i).defensive() { + sum.saturating_add(chunk.value) + } else { + sum + } + }); + ( + self.active.saturating_add(unbonding_affected_balance), + Box::new(affected_indices.chain((0..start_index).rev())), + ) + } else { + (self.active, Box::new((0..self.unlocking.len()).rev())) + } }; let is_proportional_slash = slash_amount < affected_balance; // Helper to update `target` and the ledgers total after accounting for slashing `target`. + // TODO: use PerThing let mut slash_out_of = |target: &mut BalanceOf, slash_remaining: &mut BalanceOf| { let maybe_numerator = slash_amount.checked_mul(target); - let slash_from_target = match (maybe_numerator, is_proportional_slash) { + let mut slash_from_target = match (maybe_numerator, is_proportional_slash) { // Equivalent to `(slash_amount / affected_balance) * target`. (Some(numerator), true) => numerator.div(affected_balance), // If the slash amount is gt than the affected balance OR the arithmetic to // calculate the proportion saturated, we just try to slash as much as possible. - (None, _) | (_, false) => (*target).min(*slash_remaining), - }; - - *target = *target - slash_from_target; // slash_from_target cannot be gt target. + (None, _) | (_, false) => *slash_remaining, + } + .min(*target); - let actual_slashed = if *target <= minimum_balance { + // finally, 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 - sp_std::mem::replace(target, Zero::zero()).saturating_add(slash_from_target) - } else { - slash_from_target - }; + slash_from_target = + sp_std::mem::replace(target, Zero::zero()).saturating_add(slash_from_target) + } - self.total = self.total.saturating_sub(actual_slashed); - *slash_remaining = slash_remaining.saturating_sub(actual_slashed); + // TODO: maybe move this outside of the closure to keep it a bit more pure. + self.total = self.total.saturating_sub(slash_from_target); + *slash_remaining = slash_remaining.saturating_sub(slash_from_target); }; // If this is *not* a proportional slash, the active will always wiped to 0. slash_out_of(&mut self.active, &mut remaining_slash); let mut slashed_unlocking = BTreeMap::<_, _>::new(); - let indices_to_slash - // First slash unbonding chunks from after the slash - = affected_indices - // Then start slashing older chunks, start from the era of the slash - .chain((0..start_index).rev()); - for i in indices_to_slash { - if let Some(chunk) = self.unlocking.get_mut(i) { + for i in slash_chunks_priority { + if let Some(chunk) = self.unlocking.get_mut(i).defensive() { slash_out_of(&mut chunk.value, &mut remaining_slash); - slashed_unlocking.insert(chunk.era, chunk.value); - + // write the new slashed value of this chunk to the map. + slashed_unlocking.insert(chunk.unlock_era, chunk.value); if remaining_slash.is_zero() { break } } else { - break // defensive, indices should always be in bounds. + break } } - + 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/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 7c99537b51be6..775cf95d9ab18 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -907,7 +907,7 @@ pub mod pallet { // Note: in case there is no current era it is fine to bond one era more. let era = Self::current_era().unwrap_or(0) + T::BondingDuration::get(); if let Some(mut chunk) = - ledger.unlocking.last_mut().filter(|chunk| chunk.era == era) + ledger.unlocking.last_mut().filter(|chunk| chunk.unlock_era == era) { // To keep the chunk count down, we only keep one chunk per era. Since // `unlocking` is a FiFo queue, if a chunk exists for `era` we know that it will @@ -916,7 +916,7 @@ pub mod pallet { } else { ledger .unlocking - .try_push(UnlockChunk { value, era }) + .try_push(UnlockChunk { value, unlock_era: era }) .map_err(|_| Error::::NoMoreChunks)?; }; // NOTE: ledger must be updated prior to calling `Self::weight_of`. diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index c847157c5c120..624c6c9003799 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -1257,7 +1257,7 @@ fn bond_extra_and_withdraw_unbonded_works() { stash: 11, total: 1000 + 100, active: 100, - unlocking: bounded_vec![UnlockChunk { value: 1000, era: 2 + 3 }], + unlocking: bounded_vec![UnlockChunk { value: 1000, unlock_era: 2 + 3 }], claimed_rewards: vec![] }), ); @@ -1270,7 +1270,7 @@ fn bond_extra_and_withdraw_unbonded_works() { stash: 11, total: 1000 + 100, active: 100, - unlocking: bounded_vec![UnlockChunk { value: 1000, era: 2 + 3 }], + unlocking: bounded_vec![UnlockChunk { value: 1000, unlock_era: 2 + 3 }], claimed_rewards: vec![] }), ); @@ -1286,7 +1286,7 @@ fn bond_extra_and_withdraw_unbonded_works() { stash: 11, total: 1000 + 100, active: 100, - unlocking: bounded_vec![UnlockChunk { value: 1000, era: 2 + 3 }], + unlocking: bounded_vec![UnlockChunk { value: 1000, unlock_era: 2 + 3 }], claimed_rewards: vec![] }), ); @@ -1391,7 +1391,7 @@ fn rebond_works() { stash: 11, total: 1000, active: 100, - unlocking: bounded_vec![UnlockChunk { value: 900, era: 2 + 3 }], + unlocking: bounded_vec![UnlockChunk { value: 900, unlock_era: 2 + 3 }], claimed_rewards: vec![], }) ); @@ -1417,7 +1417,7 @@ fn rebond_works() { stash: 11, total: 1000, active: 100, - unlocking: bounded_vec![UnlockChunk { value: 900, era: 5 }], + unlocking: bounded_vec![UnlockChunk { value: 900, unlock_era: 5 }], claimed_rewards: vec![], }) ); @@ -1430,7 +1430,7 @@ fn rebond_works() { stash: 11, total: 1000, active: 600, - unlocking: bounded_vec![UnlockChunk { value: 400, era: 5 }], + unlocking: bounded_vec![UnlockChunk { value: 400, unlock_era: 5 }], claimed_rewards: vec![], }) ); @@ -1458,7 +1458,7 @@ fn rebond_works() { stash: 11, total: 1000, active: 100, - unlocking: bounded_vec![UnlockChunk { value: 900, era: 5 }], + unlocking: bounded_vec![UnlockChunk { value: 900, unlock_era: 5 }], claimed_rewards: vec![], }) ); @@ -1471,7 +1471,7 @@ fn rebond_works() { stash: 11, total: 1000, active: 600, - unlocking: bounded_vec![UnlockChunk { value: 400, era: 5 }], + unlocking: bounded_vec![UnlockChunk { value: 400, unlock_era: 5 }], claimed_rewards: vec![], }) ); @@ -1513,7 +1513,7 @@ fn rebond_is_fifo() { stash: 11, total: 1000, active: 600, - unlocking: bounded_vec![UnlockChunk { value: 400, era: 2 + 3 }], + unlocking: bounded_vec![UnlockChunk { value: 400, unlock_era: 2 + 3 }], claimed_rewards: vec![], }) ); @@ -1529,8 +1529,8 @@ fn rebond_is_fifo() { total: 1000, active: 300, unlocking: bounded_vec![ - UnlockChunk { value: 400, era: 2 + 3 }, - UnlockChunk { value: 300, era: 3 + 3 }, + UnlockChunk { value: 400, unlock_era: 2 + 3 }, + UnlockChunk { value: 300, unlock_era: 3 + 3 }, ], claimed_rewards: vec![], }) @@ -1547,9 +1547,9 @@ fn rebond_is_fifo() { total: 1000, active: 100, unlocking: bounded_vec![ - UnlockChunk { value: 400, era: 2 + 3 }, - UnlockChunk { value: 300, era: 3 + 3 }, - UnlockChunk { value: 200, era: 4 + 3 }, + UnlockChunk { value: 400, unlock_era: 2 + 3 }, + UnlockChunk { value: 300, unlock_era: 3 + 3 }, + UnlockChunk { value: 200, unlock_era: 4 + 3 }, ], claimed_rewards: vec![], }) @@ -1564,8 +1564,8 @@ fn rebond_is_fifo() { total: 1000, active: 500, unlocking: bounded_vec![ - UnlockChunk { value: 400, era: 2 + 3 }, - UnlockChunk { value: 100, era: 3 + 3 }, + UnlockChunk { value: 400, unlock_era: 2 + 3 }, + UnlockChunk { value: 100, unlock_era: 3 + 3 }, ], claimed_rewards: vec![], }) @@ -1595,7 +1595,7 @@ fn rebond_emits_right_value_in_event() { stash: 11, total: 1000, active: 100, - unlocking: bounded_vec![UnlockChunk { value: 900, era: 1 + 3 }], + unlocking: bounded_vec![UnlockChunk { value: 900, unlock_era: 1 + 3 }], claimed_rewards: vec![], }) ); @@ -1608,7 +1608,7 @@ fn rebond_emits_right_value_in_event() { stash: 11, total: 1000, active: 200, - unlocking: bounded_vec![UnlockChunk { value: 800, era: 1 + 3 }], + unlocking: bounded_vec![UnlockChunk { value: 800, unlock_era: 1 + 3 }], claimed_rewards: vec![], }) ); @@ -1839,7 +1839,7 @@ fn bond_with_no_staked_value() { stash: 1, active: 0, total: 5, - unlocking: bounded_vec![UnlockChunk { value: 5, era: 3 }], + unlocking: bounded_vec![UnlockChunk { value: 5, unlock_era: 3 }], claimed_rewards: vec![], }) ); @@ -3919,7 +3919,7 @@ fn cannot_rebond_to_lower_than_ed() { stash: 21, total: 10 * 1000, active: 0, - unlocking: bounded_vec![UnlockChunk { value: 10 * 1000, era: 3 }], + unlocking: bounded_vec![UnlockChunk { value: 10 * 1000, unlock_era: 3 }], claimed_rewards: vec![] } ); @@ -3956,7 +3956,7 @@ fn cannot_bond_extra_to_lower_than_ed() { stash: 21, total: 10 * 1000, active: 0, - unlocking: bounded_vec![UnlockChunk { value: 10 * 1000, era: 3 }], + unlocking: bounded_vec![UnlockChunk { value: 10 * 1000, unlock_era: 3 }], claimed_rewards: vec![] } ); @@ -4804,7 +4804,7 @@ fn force_apply_min_commission_works() { #[test] fn ledger_slash_works() { - let c = |era, value| UnlockChunk:: { era, value }; + let c = |unlock_era, value| UnlockChunk:: { unlock_era, value }; // Given let mut ledger = StakingLedger:: { stash: 123, @@ -4840,7 +4840,7 @@ fn ledger_slash_works() { // When all the chunks overlap with the slash eras assert_eq!(ledger.slash(20, 0, 0), 20); // Then - assert_eq!(ledger.unlocking, vec![c(4, 0), c(5, 0)]); + assert_eq!(ledger.unlocking, vec![]); assert_eq!(ledger.total, 0); assert_eq!(LedgerSlashPerEra::get().0, 0); assert_eq!(LedgerSlashPerEra::get().1, BTreeMap::from([(4, 0), (5, 0)])); @@ -4848,8 +4848,9 @@ fn ledger_slash_works() { // Given ledger.unlocking = bounded_vec![c(4, 40), c(5, 100), c(6, 10), c(7, 250)]; ledger.active = 500; - ledger.total = 40 + 10 + 100 + 250 + 500; // 900 - // When we have a partial slash that touches all chunks + // 900 + ledger.total = 40 + 10 + 100 + 250 + 500; + // When we have a partial slash that touches all chunks assert_eq!(ledger.slash(900 / 2, 0, 0), 450); // Then assert_eq!(ledger.active, 500 / 2); @@ -4861,6 +4862,35 @@ fn ledger_slash_works() { BTreeMap::from([(4, 40 / 2), (5, 100 / 2), (6, 10 / 2), (7, 250 / 2)]) ); + // slash 1/4th with not chunk. + ledger.unlocking = bounded_vec![]; + ledger.active = 500; + ledger.total = 500; + // When we have a partial slash that touches all chunks + assert_eq!(ledger.slash(500 / 4, 0, 0), 500 / 4); + // Then + assert_eq!(ledger.active, 3 * 500 / 4); + assert_eq!(ledger.unlocking, vec![]); + assert_eq!(ledger.total, ledger.active); + assert_eq!(LedgerSlashPerEra::get().0, 3 * 500 / 4); + assert_eq!(LedgerSlashPerEra::get().1, Default::default()); + + // // slash 1/4th with chunks + // ledger.unlocking = bounded_vec![c(4, 40), c(5, 120), c(6, 80)]; + // ledger.active = 120; + // ledger.total = 40 + 120 + 80 + 120; // 240 + // // When we have a partial slash that touches all chunks + // assert_eq!(ledger.slash(240 / 4, 0, 0), 2 / 4 - 1); + // // Then + // assert_eq!(ledger.unlocking, vec![c(4, 3 * 40 / 4), c(5, 3 * 120 / 4), c(6, 3 * 80 / 4)]); + // assert_eq!(ledger.active, 3 * 120 / 4); + // assert_eq!(ledger.total, 3 * 240 / 4 + 1); + // assert_eq!(LedgerSlashPerEra::get().0, 3 * 240 / 4); + // assert_eq!( + // LedgerSlashPerEra::get().1, + // BTreeMap::from([(4, 3 * 40 / 4), (5, 3 * 80 / 4), (6, 3 * 120 / 4)]) + // ); + // Given we have the same as above, ledger.unlocking = bounded_vec![c(4, 40), c(5, 100), c(6, 10), c(7, 250)]; ledger.active = 500; @@ -4876,10 +4906,10 @@ fn ledger_slash_works() { ), 475 ); - // Then + let dust = (10 / 2) + (40 / 2); assert_eq!(ledger.active, 500 / 2); - assert_eq!(ledger.unlocking, vec![c(4, 0), c(5, 100 / 2), c(6, 0), c(7, 250 / 2)]); - assert_eq!(ledger.total, 425); + assert_eq!(ledger.unlocking, vec![c(5, 100 / 2), c(7, 250 / 2)]); + assert_eq!(ledger.total, 900 / 2 - dust); assert_eq!(LedgerSlashPerEra::get().0, 500 / 2); assert_eq!( LedgerSlashPerEra::get().1, @@ -4890,7 +4920,6 @@ fn ledger_slash_works() { ledger.unlocking = bounded_vec![c(4, 40), c(5, 100), c(6, 10), c(7, 250)]; ledger.active = 500; ledger.total = 40 + 10 + 100 + 250 + 500; // 900 - // When assert_eq!( ledger.slash( 900 / 2, @@ -4902,7 +4931,7 @@ fn ledger_slash_works() { ); // Then assert_eq!(ledger.active, 500 / 2); - assert_eq!(ledger.unlocking, vec![c(4, 0), c(5, 100 / 2), c(6, 0), c(7, 250 / 2)]); + assert_eq!(ledger.unlocking, vec![c(5, 100 / 2), c(7, 250 / 2)]); assert_eq!(ledger.total, 425); assert_eq!(LedgerSlashPerEra::get().0, 500 / 2); assert_eq!( @@ -4911,10 +4940,10 @@ fn ledger_slash_works() { ); // Given + // slash order --------------------NA--------2----------0----------1---- ledger.unlocking = bounded_vec![c(4, 40), c(5, 100), c(6, 10), c(7, 250)]; ledger.active = 500; ledger.total = 40 + 10 + 100 + 250 + 500; // 900 - // When assert_eq!( ledger.slash( 500 + 10 + 250 + 100 / 2, // active + era 6 + era 7 + era 5 / 2 @@ -4927,13 +4956,13 @@ fn ledger_slash_works() { ); // Then assert_eq!(ledger.active, 0); - // iteration order ------------------NA----------2-------------0--------1---- - assert_eq!(ledger.unlocking, vec![c(4, 40), c(5, 100 / 2), c(6, 0), c(7, 0)]); + assert_eq!(ledger.unlocking, vec![c(4, 40), c(5, 100 / 2)]); assert_eq!(ledger.total, 90); assert_eq!(LedgerSlashPerEra::get().0, 0); assert_eq!(LedgerSlashPerEra::get().1, BTreeMap::from([(5, 100 / 2), (6, 0), (7, 0)])); // Given + // iteration order------------------NA---------2----------0----------1---- ledger.unlocking = bounded_vec![c(4, 100), c(5, 100), c(6, 100), c(7, 100)]; ledger.active = 100; ledger.total = 5 * 100; @@ -4950,8 +4979,7 @@ fn ledger_slash_works() { ); // Then assert_eq!(ledger.active, 0); - // iteration order ------------------NA---------2--------0--------1---- - assert_eq!(ledger.unlocking, vec![c(4, 100), c(5, 0), c(6, 0), c(7, 0)]); + assert_eq!(ledger.unlocking, vec![c(4, 100)]); //------goes to min balance and then gets dusted^^^ assert_eq!(ledger.total, 100); assert_eq!(LedgerSlashPerEra::get().0, 0); diff --git a/primitives/staking/src/lib.rs b/primitives/staking/src/lib.rs index 09eff0717c8e8..57d858be1e0ef 100644 --- a/primitives/staking/src/lib.rs +++ b/primitives/staking/src/lib.rs @@ -37,12 +37,12 @@ pub trait OnStakerSlash { /// # Arguments /// /// * `stash` - The stash of the staker whom the slash was applied to. - /// * `slashed_bonded` - The new bonded balance of the staker after the slash was applied. - /// * `slashed_unlocking` - A map from eras that the staker is unbonding in to the new balance - /// after the slash was applied. + /// * `slashed_active` - The new bonded balance of the staker after the slash was applied. + /// * `slashed_unlocking` - a map of slashed eras, and the balance of that unlocking chunk after + /// the slash is applied. Any era not present in the map is not affected at all. fn on_slash( stash: &AccountId, - slashed_bonded: Balance, + slashed_active: Balance, slashed_unlocking: &BTreeMap, ); } From 9ee82684aa991da8701aaa64ada2f9a5288d8b2b Mon Sep 17 00:00:00 2001 From: kianenigma Date: Wed, 16 Mar 2022 22:10:38 +0000 Subject: [PATCH 08/18] demonstrate per_thing usage --- frame/staking/src/lib.rs | 22 ++++++++-------------- frame/staking/src/mock.rs | 1 + frame/staking/src/pallet/mod.rs | 18 ++++++++++++++++-- frame/staking/src/tests.rs | 26 +++++++++----------------- 4 files changed, 34 insertions(+), 33 deletions(-) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index af7a059f66a4c..9765f9705637a 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -310,7 +310,7 @@ use scale_info::TypeInfo; use sp_runtime::{ curve::PiecewiseLinear, traits::{AtLeast32BitUnsigned, Convert, Saturating, Zero}, - Perbill, RuntimeDebug, + Perbill, Perquintill, RuntimeDebug, }; use sp_staking::{ offence::{Offence, OffenceError, ReportOffence}, @@ -338,8 +338,7 @@ macro_rules! log { pub type RewardPoint = u32; /// The balance type of this pallet. -pub type BalanceOf = - <::Currency as Currency<::AccountId>>::Balance; +pub type BalanceOf = ::CurrencyBalance; type PositiveImbalanceOf = <::Currency as Currency< ::AccountId, @@ -560,9 +559,7 @@ impl StakingLedger { minimum_balance: BalanceOf, slash_era: EraIndex, ) -> BalanceOf { - use sp_runtime::traits::CheckedMul as _; use sp_staking::OnStakerSlash as _; - use sp_std::ops::Div as _; if slash_amount.is_zero() { return Zero::zero() @@ -601,21 +598,18 @@ impl StakingLedger { } }; - let is_proportional_slash = slash_amount < affected_balance; // Helper to update `target` and the ledgers total after accounting for slashing `target`. // TODO: use PerThing let mut slash_out_of = |target: &mut BalanceOf, slash_remaining: &mut BalanceOf| { - let maybe_numerator = slash_amount.checked_mul(target); - let mut slash_from_target = match (maybe_numerator, is_proportional_slash) { - // Equivalent to `(slash_amount / affected_balance) * target`. - (Some(numerator), true) => numerator.div(affected_balance), - // If the slash amount is gt than the affected balance OR the arithmetic to - // calculate the proportion saturated, we just try to slash as much as possible. - (None, _) | (_, false) => *slash_remaining, + let mut slash_from_target = if slash_amount < affected_balance { + let ratio = Perquintill::from_rational(slash_amount, affected_balance); + ratio * (*target) + } else { + *slash_remaining } .min(*target); - // finally, slash out from *target exactly `slash_from_target`. + // 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 diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index d4bc383a29499..0ab40cfe019b3 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -265,6 +265,7 @@ impl sp_staking::OnStakerSlash for OnStakerSlashM impl crate::pallet::pallet::Config for Test { type MaxNominations = MaxNominations; type Currency = Balances; + type CurrencyBalance = ::Balance; type UnixTime = Timestamp; type CurrencyToVote = frame_support::traits::SaturatingCurrencyToVote; type RewardRemainder = RewardRemainderMock; diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 775cf95d9ab18..a467a3e112cdc 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -75,8 +75,22 @@ pub mod pallet { #[pallet::config] pub trait Config: frame_system::Config + SendTransactionTypes> { /// The staking balance. - type Currency: LockableCurrency; - + type Currency: LockableCurrency< + Self::AccountId, + Moment = Self::BlockNumber, + Balance = Self::CurrencyBalance, + >; + /// Just the `Currency::Balance` type; we have this item to allow us to constrain it to + /// `From`. + type CurrencyBalance: sp_runtime::traits::AtLeast32BitUnsigned + + codec::FullCodec + + Copy + + MaybeSerializeDeserialize + + sp_std::fmt::Debug + + Default + + From + + TypeInfo + + MaxEncodedLen; /// Time used for computing era duration. /// /// It is guaranteed to start being called from the first `on_finalize`. Thus value at diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 624c6c9003799..90931e50eda93 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -2104,7 +2104,7 @@ fn reward_validator_slashing_validator_does_not_overflow() { &[Perbill::from_percent(100)], ); - assert_eq!(Balances::total_balance(&11), stake - 1); + assert_eq!(Balances::total_balance(&11), stake); assert_eq!(Balances::total_balance(&2), 1); }) } @@ -4992,27 +4992,19 @@ fn ledger_slash_works() { let value = slash - (9 * 4) // The value of the other parts of ledger that will get slashed + 1; - // slash * value will saturate - assert!(slash.checked_mul(value - 20).is_none()); + ledger.active = 10; ledger.unlocking = bounded_vec![c(4, 10), c(5, 10), c(6, 10), c(7, value)]; ledger.total = value + 40; // When - assert_eq!(ledger.slash(slash, 0, 0), slash); + let slash_amount = ledger.slash(slash, 0, 0); + assert_eq_error_rate!(slash_amount, slash, 5); // Then - assert_eq!(ledger.active, 1); // slash of 9 - assert_eq!( - ledger.unlocking, - vec![ - c(4, 1), // slash of 9 - c(5, 1), // slash of 9 - c(6, 1), // slash of 9 - c(7, 1), // saturates, so slash of remaining_slash.min(value) equivalent of (slash - 9 * 4).min(value) - ] - ); - assert_eq!(ledger.total, 5); - assert_eq!(LedgerSlashPerEra::get().0, 1); - assert_eq!(LedgerSlashPerEra::get().1, BTreeMap::from([(4, 1), (5, 1), (6, 1), (7, 1)])); + assert_eq!(ledger.active, 0); // slash of 9 + assert_eq!(ledger.unlocking, vec![]); + assert_eq!(ledger.total, 0); + assert_eq!(LedgerSlashPerEra::get().0, 0); + assert_eq!(LedgerSlashPerEra::get().1, BTreeMap::from([(4, 0), (5, 0), (6, 0), (7, 0)])); // Given let slash = u64::MAX as Balance * 2; From 6c62233d9f9b17adf33511b8c3cb7a9711a7fef8 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Mon, 21 Mar 2022 13:31:25 +0100 Subject: [PATCH 09/18] Update some tests --- frame/staking/src/lib.rs | 4 +-- frame/staking/src/tests.rs | 54 ++++++++++---------------------------- 2 files changed, 15 insertions(+), 43 deletions(-) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 9765f9705637a..4e3f163c29687 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -599,10 +599,9 @@ impl StakingLedger { }; // Helper to update `target` and the ledgers total after accounting for slashing `target`. - // TODO: use PerThing + let ratio = Perquintill::from_rational(slash_amount, affected_balance); let mut slash_out_of = |target: &mut BalanceOf, slash_remaining: &mut BalanceOf| { let mut slash_from_target = if slash_amount < affected_balance { - let ratio = Perquintill::from_rational(slash_amount, affected_balance); ratio * (*target) } else { *slash_remaining @@ -617,7 +616,6 @@ impl StakingLedger { sp_std::mem::replace(target, Zero::zero()).saturating_add(slash_from_target) } - // TODO: maybe move this outside of the closure to keep it a bit more pure. self.total = self.total.saturating_sub(slash_from_target); *slash_remaining = slash_remaining.saturating_sub(slash_from_target); }; diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 90931e50eda93..6538115091b1c 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -4875,22 +4875,6 @@ fn ledger_slash_works() { assert_eq!(LedgerSlashPerEra::get().0, 3 * 500 / 4); assert_eq!(LedgerSlashPerEra::get().1, Default::default()); - // // slash 1/4th with chunks - // ledger.unlocking = bounded_vec![c(4, 40), c(5, 120), c(6, 80)]; - // ledger.active = 120; - // ledger.total = 40 + 120 + 80 + 120; // 240 - // // When we have a partial slash that touches all chunks - // assert_eq!(ledger.slash(240 / 4, 0, 0), 2 / 4 - 1); - // // Then - // assert_eq!(ledger.unlocking, vec![c(4, 3 * 40 / 4), c(5, 3 * 120 / 4), c(6, 3 * 80 / 4)]); - // assert_eq!(ledger.active, 3 * 120 / 4); - // assert_eq!(ledger.total, 3 * 240 / 4 + 1); - // assert_eq!(LedgerSlashPerEra::get().0, 3 * 240 / 4); - // assert_eq!( - // LedgerSlashPerEra::get().1, - // BTreeMap::from([(4, 3 * 40 / 4), (5, 3 * 80 / 4), (6, 3 * 120 / 4)]) - // ); - // Given we have the same as above, ledger.unlocking = bounded_vec![c(4, 40), c(5, 100), c(6, 10), c(7, 250)]; ledger.active = 500; @@ -5019,36 +5003,26 @@ fn ledger_slash_works() { ledger.active = unit; ledger.total = unit * 4 + value; // When - assert_eq!(ledger.slash(slash, 0, 0), slash); + assert_eq!(ledger.slash(slash, 0, 0), slash - 43); // Then // The amount slashed out of `unit` - let unit_slash = { - let affected_balance = value + unit * 4; - slash * unit / affected_balance - }; + let affected_balance = value + unit * 4; + let ratio = Perquintill::from_rational(slash, affected_balance); // `unit` after the slash is applied - let unit_slashed = unit - unit_slash; - // `value` after the slash is applied + let unit_slashed = { + let unit_slash = ratio * unit; + unit - unit_slash + }; let value_slashed = { - // We slash active and era 4 before we slash era 5 - let previous_slashed_amount = unit_slash * 2; - let remaining_slash = slash - previous_slashed_amount; - value - remaining_slash + let value_slash = ratio * value; + value - value_slash }; assert_eq!(ledger.active, unit_slashed); + assert_eq!(ledger.unlocking, vec![c(5, value_slashed)]); + assert_eq!(ledger.total, value_slashed); + assert_eq!(LedgerSlashPerEra::get().0, 0); assert_eq!( - ledger.unlocking, - vec![ - c(4, unit_slashed), - // We reached the full slash amount here because we slashed value.min(remaining_slash), - // which was remaining_slash - c(5, value_slashed), - c(6, unit), /* The rest are untouched even though they where in the slashing range. - * This is problematic for pools, but should be rare enough that its ok. */ - c(7, unit) - ] + LedgerSlashPerEra::get().1, + BTreeMap::from([(4, 0), (5, value_slashed), (6, 0), (7, 0)]) ); - assert_eq!(ledger.total, unit_slashed + unit_slashed + value_slashed + unit + unit); - assert_eq!(LedgerSlashPerEra::get().0, unit_slashed); - assert_eq!(LedgerSlashPerEra::get().1, BTreeMap::from([(4, unit_slashed), (5, value_slashed)])); } From 35a3077caa03eae3309e4fe1e1985772e3701d64 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Mon, 21 Mar 2022 13:40:12 +0100 Subject: [PATCH 10/18] FMT --- frame/staking/src/lib.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 4e3f163c29687..74d973eac5aaf 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -601,12 +601,9 @@ impl StakingLedger { // Helper to update `target` and the ledgers total after accounting for slashing `target`. let ratio = Perquintill::from_rational(slash_amount, affected_balance); 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 slash_amount < affected_balance { ratio * (*target) } else { *slash_remaining } + .min(*target); // slash out from *target exactly `slash_from_target`. *target = *target - slash_from_target; From 7c3ca1e916c6839fe0f91665d30a6e757f63cfaf Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Mon, 21 Mar 2022 21:49:14 +0100 Subject: [PATCH 11/18] Test that era offset works correctly --- frame/staking/src/lib.rs | 2 -- frame/staking/src/tests.rs | 13 +++++++++++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 74d973eac5aaf..ffb4030792824 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -569,8 +569,6 @@ impl StakingLedger { let pre_slash_total = self.total; let era_after_slash = slash_era + 1; - // TODO: test to make sure chunks before the slash are never slashed. - // at this era onwards, the funds can be slashed. let chunk_unlock_era_after_slash = era_after_slash + T::BondingDuration::get(); // Calculate the total balance of active funds and unlocking funds in the affected range. diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index bf023c3183084..9cdeed4a2fc41 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -4850,6 +4850,19 @@ fn ledger_slash_works() { assert_eq!(LedgerSlashPerEra::get().0, 0); assert_eq!(LedgerSlashPerEra::get().1, BTreeMap::from([(4, 0), (5, 0)])); + // TODO test does not slash earlier eras + // Given + ledger.unlocking = bounded_vec![c(4, 100), c(5, 100), c(6, 100), c(7, 100)]; + 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); // TODO + // 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); + assert_eq!(LedgerSlashPerEra::get().0, 0); + assert_eq!(LedgerSlashPerEra::get().1, BTreeMap::from([(6, 30), (7, 30)])); + // Given ledger.unlocking = bounded_vec![c(4, 40), c(5, 100), c(6, 10), c(7, 250)]; ledger.active = 500; From 4a9e8aa0e22ba641de4d3f70b2a9a15501bdadbd Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Mon, 21 Mar 2022 22:49:18 +0100 Subject: [PATCH 12/18] Update mocks --- bin/node/runtime/src/lib.rs | 1 + frame/babe/src/mock.rs | 1 + frame/grandpa/src/mock.rs | 1 + frame/offences/benchmarking/src/mock.rs | 1 + frame/session/benchmarking/src/mock.rs | 1 + frame/staking/src/benchmarking.rs | 4 ++-- 6 files changed, 7 insertions(+), 2 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 96a4ebed24c23..fbf13a5c967d9 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -537,6 +537,7 @@ impl pallet_staking::BenchmarkingConfig for StakingBenchmarkingConfig { impl pallet_staking::Config for Runtime { type MaxNominations = MaxNominations; type Currency = Balances; + type CurrencyBalance = Balance; type UnixTime = Timestamp; type CurrencyToVote = U128CurrencyToVote; type RewardRemainder = Treasury; diff --git a/frame/babe/src/mock.rs b/frame/babe/src/mock.rs index 0518b0f3b6644..42f5f9dac8de6 100644 --- a/frame/babe/src/mock.rs +++ b/frame/babe/src/mock.rs @@ -183,6 +183,7 @@ impl pallet_staking::Config for Test { type CurrencyToVote = frame_support::traits::SaturatingCurrencyToVote; type Event = Event; type Currency = Balances; + type CurrencyBalance = ::Balance; type Slash = (); type Reward = (); type SessionsPerEra = SessionsPerEra; diff --git a/frame/grandpa/src/mock.rs b/frame/grandpa/src/mock.rs index aef87b66920e5..f2bac7de40b5a 100644 --- a/frame/grandpa/src/mock.rs +++ b/frame/grandpa/src/mock.rs @@ -191,6 +191,7 @@ impl pallet_staking::Config for Test { type CurrencyToVote = frame_support::traits::SaturatingCurrencyToVote; type Event = Event; type Currency = Balances; + type CurrencyBalance = ::Balance; type Slash = (); type Reward = (); type SessionsPerEra = SessionsPerEra; diff --git a/frame/offences/benchmarking/src/mock.rs b/frame/offences/benchmarking/src/mock.rs index 5937f1cdc29af..e588bce608188 100644 --- a/frame/offences/benchmarking/src/mock.rs +++ b/frame/offences/benchmarking/src/mock.rs @@ -158,6 +158,7 @@ impl onchain::Config for Test { impl pallet_staking::Config for Test { type MaxNominations = ConstU32<16>; type Currency = Balances; + type CurrencyBalance = ::Balance; type UnixTime = pallet_timestamp::Pallet; type CurrencyToVote = frame_support::traits::SaturatingCurrencyToVote; type RewardRemainder = (); diff --git a/frame/session/benchmarking/src/mock.rs b/frame/session/benchmarking/src/mock.rs index 593b532a303e6..bb39f49602417 100644 --- a/frame/session/benchmarking/src/mock.rs +++ b/frame/session/benchmarking/src/mock.rs @@ -164,6 +164,7 @@ impl onchain::Config for Test { impl pallet_staking::Config for Test { type MaxNominations = ConstU32<16>; type Currency = Balances; + type CurrencyBalance = ::Balance; type UnixTime = pallet_timestamp::Pallet; type CurrencyToVote = frame_support::traits::SaturatingCurrencyToVote; type RewardRemainder = (); diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 7456c16190113..28dae6e657748 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -644,7 +644,7 @@ benchmarks! { assert!(value * l.into() + origin_weight <= dest_weight); let unlock_chunk = UnlockChunk::> { value, - era: EraIndex::zero(), + unlock_era: EraIndex::zero(), }; let stash = scenario.origin_stash1.clone(); @@ -793,7 +793,7 @@ benchmarks! { let mut staking_ledger = Ledger::::get(controller.clone()).unwrap(); let unlock_chunk = UnlockChunk::> { value: 1u32.into(), - era: EraIndex::zero(), + unlock_era: EraIndex::zero(), }; for _ in 0 .. l { staking_ledger.unlocking.try_push(unlock_chunk.clone()).unwrap(); From 7e953dc9505153f3608586af8f0900994ef3d682 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 22 Mar 2022 13:24:34 +0100 Subject: [PATCH 13/18] Remove unnescary docs --- frame/staking/src/lib.rs | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index ffb4030792824..18a8b72a9c70e 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -533,26 +533,10 @@ impl StakingLedger { /// case that either the active bonded or some unlocking chunks become dust after slashing. /// Returns the amount of funds actually slashed. /// - /// Note that this calls `Config::OnStakerSlash::on_slash` with information as to how the slash + /// # Note + /// + /// This calls `Config::OnStakerSlash::on_slash` with information as to how the slash /// was applied. - // Generally slashes are computed by: - // - // 1) Balances of the unlocking chunks in range `slash_era + 1..=apply_era` are summed and - // stored in `total_balance_affected`. - // - // 2) `slash_ratio` is computed as `slash_amount / total_balance_affected`. - // - // 3) `Ledger::active` is set to `(1- slash_ratio) * Ledger::active`. - // - // 4) For all unlocking chunks created in range `slash_era + 1..=apply_era` set their balance - // to `(1 - slash_ratio) * unbonding_pool_balance`. We start with slash_era + 1, because that - // is the earliest the active sake while slash could be unbonded. - // - // 5) Slash any remaining slash amount from the remaining chunks, starting with the `slash_era` - // and going backwards. - // - // There are some edge cases due to saturating arithmetic - see logic below and tests for - // details. fn slash( &mut self, slash_amount: BalanceOf, From 3ba68c7ecd75c4c98415d182644012afe4fcad9b Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Thu, 7 Apr 2022 18:20:06 -0400 Subject: [PATCH 14/18] Remove unlock_era --- frame/staking/src/benchmarking.rs | 4 +-- frame/staking/src/lib.rs | 8 +++--- frame/staking/src/pallet/mod.rs | 4 +-- frame/staking/src/tests.rs | 44 +++++++++++++++---------------- 4 files changed, 30 insertions(+), 30 deletions(-) diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 28dae6e657748..7456c16190113 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -644,7 +644,7 @@ benchmarks! { assert!(value * l.into() + origin_weight <= dest_weight); let unlock_chunk = UnlockChunk::> { value, - unlock_era: EraIndex::zero(), + era: EraIndex::zero(), }; let stash = scenario.origin_stash1.clone(); @@ -793,7 +793,7 @@ benchmarks! { let mut staking_ledger = Ledger::::get(controller.clone()).unwrap(); let unlock_chunk = UnlockChunk::> { value: 1u32.into(), - unlock_era: EraIndex::zero(), + era: EraIndex::zero(), }; for _ in 0 .. l { staking_ledger.unlocking.try_push(unlock_chunk.clone()).unwrap(); diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 18a8b72a9c70e..c3a8823a60639 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -434,7 +434,7 @@ pub struct UnlockChunk { value: Balance, /// Era number at which point it'll be unlocked. #[codec(compact)] - unlock_era: EraIndex, + era: EraIndex, } /// The ledger of a (bonded) stash. @@ -480,7 +480,7 @@ impl StakingLedger { .unlocking .into_iter() .filter(|chunk| { - if chunk.unlock_era > current_era { + if chunk.era > current_era { true } else { total = total.saturating_sub(chunk.value); @@ -558,7 +558,7 @@ impl StakingLedger { // 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.unlock_era >= chunk_unlock_era_after_slash) + self.unlocking.iter().position(|c| c.era >= chunk_unlock_era_after_slash) { // 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) @@ -607,7 +607,7 @@ impl StakingLedger { if let Some(chunk) = self.unlocking.get_mut(i).defensive() { slash_out_of(&mut chunk.value, &mut remaining_slash); // write the new slashed value of this chunk to the map. - slashed_unlocking.insert(chunk.unlock_era, chunk.value); + slashed_unlocking.insert(chunk.era, chunk.value); if remaining_slash.is_zero() { break } diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 26a51b1c8c807..1984e01338b4a 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -921,7 +921,7 @@ pub mod pallet { // Note: in case there is no current era it is fine to bond one era more. let era = Self::current_era().unwrap_or(0) + T::BondingDuration::get(); if let Some(mut chunk) = - ledger.unlocking.last_mut().filter(|chunk| chunk.unlock_era == era) + ledger.unlocking.last_mut().filter(|chunk| chunk.era == era) { // To keep the chunk count down, we only keep one chunk per era. Since // `unlocking` is a FiFo queue, if a chunk exists for `era` we know that it will @@ -930,7 +930,7 @@ pub mod pallet { } else { ledger .unlocking - .try_push(UnlockChunk { value, unlock_era: era }) + .try_push(UnlockChunk { value, era }) .map_err(|_| Error::::NoMoreChunks)?; }; // NOTE: ledger must be updated prior to calling `Self::weight_of`. diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 9cdeed4a2fc41..2de9949a4ec8c 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -1257,7 +1257,7 @@ fn bond_extra_and_withdraw_unbonded_works() { stash: 11, total: 1000 + 100, active: 100, - unlocking: bounded_vec![UnlockChunk { value: 1000, unlock_era: 2 + 3 }], + unlocking: bounded_vec![UnlockChunk { value: 1000, era: 2 + 3 }], claimed_rewards: vec![] }), ); @@ -1270,7 +1270,7 @@ fn bond_extra_and_withdraw_unbonded_works() { stash: 11, total: 1000 + 100, active: 100, - unlocking: bounded_vec![UnlockChunk { value: 1000, unlock_era: 2 + 3 }], + unlocking: bounded_vec![UnlockChunk { value: 1000, era: 2 + 3 }], claimed_rewards: vec![] }), ); @@ -1286,7 +1286,7 @@ fn bond_extra_and_withdraw_unbonded_works() { stash: 11, total: 1000 + 100, active: 100, - unlocking: bounded_vec![UnlockChunk { value: 1000, unlock_era: 2 + 3 }], + unlocking: bounded_vec![UnlockChunk { value: 1000, era: 2 + 3 }], claimed_rewards: vec![] }), ); @@ -1391,7 +1391,7 @@ fn rebond_works() { stash: 11, total: 1000, active: 100, - unlocking: bounded_vec![UnlockChunk { value: 900, unlock_era: 2 + 3 }], + unlocking: bounded_vec![UnlockChunk { value: 900, era: 2 + 3 }], claimed_rewards: vec![], }) ); @@ -1417,7 +1417,7 @@ fn rebond_works() { stash: 11, total: 1000, active: 100, - unlocking: bounded_vec![UnlockChunk { value: 900, unlock_era: 5 }], + unlocking: bounded_vec![UnlockChunk { value: 900, era: 5 }], claimed_rewards: vec![], }) ); @@ -1430,7 +1430,7 @@ fn rebond_works() { stash: 11, total: 1000, active: 600, - unlocking: bounded_vec![UnlockChunk { value: 400, unlock_era: 5 }], + unlocking: bounded_vec![UnlockChunk { value: 400, era: 5 }], claimed_rewards: vec![], }) ); @@ -1458,7 +1458,7 @@ fn rebond_works() { stash: 11, total: 1000, active: 100, - unlocking: bounded_vec![UnlockChunk { value: 900, unlock_era: 5 }], + unlocking: bounded_vec![UnlockChunk { value: 900, era: 5 }], claimed_rewards: vec![], }) ); @@ -1471,7 +1471,7 @@ fn rebond_works() { stash: 11, total: 1000, active: 600, - unlocking: bounded_vec![UnlockChunk { value: 400, unlock_era: 5 }], + unlocking: bounded_vec![UnlockChunk { value: 400, era: 5 }], claimed_rewards: vec![], }) ); @@ -1513,7 +1513,7 @@ fn rebond_is_fifo() { stash: 11, total: 1000, active: 600, - unlocking: bounded_vec![UnlockChunk { value: 400, unlock_era: 2 + 3 }], + unlocking: bounded_vec![UnlockChunk { value: 400, era: 2 + 3 }], claimed_rewards: vec![], }) ); @@ -1529,8 +1529,8 @@ fn rebond_is_fifo() { total: 1000, active: 300, unlocking: bounded_vec![ - UnlockChunk { value: 400, unlock_era: 2 + 3 }, - UnlockChunk { value: 300, unlock_era: 3 + 3 }, + UnlockChunk { value: 400, era: 2 + 3 }, + UnlockChunk { value: 300, era: 3 + 3 }, ], claimed_rewards: vec![], }) @@ -1547,9 +1547,9 @@ fn rebond_is_fifo() { total: 1000, active: 100, unlocking: bounded_vec![ - UnlockChunk { value: 400, unlock_era: 2 + 3 }, - UnlockChunk { value: 300, unlock_era: 3 + 3 }, - UnlockChunk { value: 200, unlock_era: 4 + 3 }, + UnlockChunk { value: 400, era: 2 + 3 }, + UnlockChunk { value: 300, era: 3 + 3 }, + UnlockChunk { value: 200, era: 4 + 3 }, ], claimed_rewards: vec![], }) @@ -1564,8 +1564,8 @@ fn rebond_is_fifo() { total: 1000, active: 500, unlocking: bounded_vec![ - UnlockChunk { value: 400, unlock_era: 2 + 3 }, - UnlockChunk { value: 100, unlock_era: 3 + 3 }, + UnlockChunk { value: 400, era: 2 + 3 }, + UnlockChunk { value: 100, era: 3 + 3 }, ], claimed_rewards: vec![], }) @@ -1595,7 +1595,7 @@ fn rebond_emits_right_value_in_event() { stash: 11, total: 1000, active: 100, - unlocking: bounded_vec![UnlockChunk { value: 900, unlock_era: 1 + 3 }], + unlocking: bounded_vec![UnlockChunk { value: 900, era: 1 + 3 }], claimed_rewards: vec![], }) ); @@ -1608,7 +1608,7 @@ fn rebond_emits_right_value_in_event() { stash: 11, total: 1000, active: 200, - unlocking: bounded_vec![UnlockChunk { value: 800, unlock_era: 1 + 3 }], + unlocking: bounded_vec![UnlockChunk { value: 800, era: 1 + 3 }], claimed_rewards: vec![], }) ); @@ -1839,7 +1839,7 @@ fn bond_with_no_staked_value() { stash: 1, active: 0, total: 5, - unlocking: bounded_vec![UnlockChunk { value: 5, unlock_era: 3 }], + unlocking: bounded_vec![UnlockChunk { value: 5, era: 3 }], claimed_rewards: vec![], }) ); @@ -3919,7 +3919,7 @@ fn cannot_rebond_to_lower_than_ed() { stash: 21, total: 10 * 1000, active: 0, - unlocking: bounded_vec![UnlockChunk { value: 10 * 1000, unlock_era: 3 }], + unlocking: bounded_vec![UnlockChunk { value: 10 * 1000, era: 3 }], claimed_rewards: vec![] } ); @@ -3956,7 +3956,7 @@ fn cannot_bond_extra_to_lower_than_ed() { stash: 21, total: 10 * 1000, active: 0, - unlocking: bounded_vec![UnlockChunk { value: 10 * 1000, unlock_era: 3 }], + unlocking: bounded_vec![UnlockChunk { value: 10 * 1000, era: 3 }], claimed_rewards: vec![] } ); @@ -4809,7 +4809,7 @@ fn force_apply_min_commission_works() { #[test] fn ledger_slash_works() { - let c = |unlock_era, value| UnlockChunk:: { unlock_era, value }; + let c = |era, value| UnlockChunk:: { era, value }; // Given let mut ledger = StakingLedger:: { stash: 123, From 7b9f9747aa347942c2e326de8a08ba496a895881 Mon Sep 17 00:00:00 2001 From: Zeke Mostov Date: Thu, 14 Apr 2022 17:04:29 -0700 Subject: [PATCH 15/18] Update frame/staking/src/lib.rs --- frame/staking/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 7601fb232cd90..8003b9814c8e6 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -589,7 +589,7 @@ impl StakingLedger { // slash out from *target exactly `slash_from_target`. *target = *target - slash_from_target; - if *target <= minimum_balance { + if *target < minimum_balance { // Slash the rest of the target if its dust slash_from_target = sp_std::mem::replace(target, Zero::zero()).saturating_add(slash_from_target) From 21202e68bf87de498af1fe8b7ef36091ff687edb Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Thu, 14 Apr 2022 17:20:08 -0700 Subject: [PATCH 16/18] Adjust tests to account for only remove when < ED --- frame/staking/src/tests.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 7b75827c97e7a..d58df33f4a557 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -4979,8 +4979,8 @@ fn ledger_slash_works() { // When assert_eq!( ledger.slash( - 350, // active + era 6 + era 7 + era 5 / 2 - 50, // min balance - everything slashed to 50 or below will get dusted + 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 * ledge.active. This will cause the affected to go to zero, and then we will * start slashing older chunks */ @@ -4990,7 +4990,6 @@ fn ledger_slash_works() { // Then assert_eq!(ledger.active, 0); assert_eq!(ledger.unlocking, vec![c(4, 100)]); - //------goes to min balance and then gets dusted^^^ assert_eq!(ledger.total, 100); assert_eq!(LedgerSlashPerEra::get().0, 0); assert_eq!(LedgerSlashPerEra::get().1, BTreeMap::from([(5, 0), (6, 0), (7, 0)])); From 52728c0484bc5db348cd47ecd9b575ee37553047 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Thu, 14 Apr 2022 17:23:17 -0700 Subject: [PATCH 17/18] Remove stale TODOs --- frame/staking/src/tests.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index c9575cf869b2a..952994e371d10 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -4868,14 +4868,13 @@ fn ledger_slash_works() { assert_eq!(LedgerSlashPerEra::get().0, 0); assert_eq!(LedgerSlashPerEra::get().1, BTreeMap::from([(4, 0), (5, 0)])); - // TODO test does not slash earlier eras // Given ledger.unlocking = bounded_vec![c(4, 100), c(5, 100), c(6, 100), c(7, 100)]; 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); // TODO - // Then + assert_eq!(ledger.slash(140, 0, 2), 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); assert_eq!(LedgerSlashPerEra::get().0, 0); From d3ef3b45ab74a8aea969811b324e8274205de4ee Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Thu, 14 Apr 2022 17:35:22 -0700 Subject: [PATCH 18/18] Remove dupe test --- frame/staking/src/tests.rs | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 952994e371d10..45aec81c46520 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -4935,29 +4935,6 @@ fn ledger_slash_works() { BTreeMap::from([(4, 0), (5, 100 / 2), (6, 0), (7, 250 / 2)]) ); - // Given we have the same as above, - ledger.unlocking = bounded_vec![c(4, 40), c(5, 100), c(6, 10), c(7, 250)]; - ledger.active = 500; - ledger.total = 40 + 10 + 100 + 250 + 500; // 900 - assert_eq!( - ledger.slash( - 900 / 2, - 25, /* min balance - chunks with era 5 & 5 will be slashed to <=25, causing them to - * get swept */ - 0 - ), - 475 - ); - // Then - assert_eq!(ledger.active, 500 / 2); - assert_eq!(ledger.unlocking, vec![c(5, 100 / 2), c(7, 250 / 2)]); - assert_eq!(ledger.total, 425); - assert_eq!(LedgerSlashPerEra::get().0, 500 / 2); - assert_eq!( - LedgerSlashPerEra::get().1, - BTreeMap::from([(4, 0), (5, 100 / 2), (6, 0), (7, 250 / 2)]) - ); - // Given // slash order --------------------NA--------2----------0----------1---- ledger.unlocking = bounded_vec![c(4, 40), c(5, 100), c(6, 10), c(7, 250)];