From 272ac35ed20bc7682866fd801cbac01f2e7f6810 Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Wed, 30 Aug 2023 21:49:11 +0200 Subject: [PATCH 01/43] migrate from substrate PR --- substrate/frame/nomination-pools/Cargo.toml | 8 +- substrate/frame/nomination-pools/src/lib.rs | 151 +++++++++++++---- .../frame/nomination-pools/src/migration.rs | 99 +++++++++++ substrate/frame/nomination-pools/src/mock.rs | 43 +++-- substrate/frame/nomination-pools/src/tests.rs | 158 ++++++++++++++---- substrate/frame/staking/src/lib.rs | 10 +- substrate/frame/staking/src/mock.rs | 1 + substrate/primitives/staking/src/lib.rs | 2 + 8 files changed, 394 insertions(+), 78 deletions(-) diff --git a/substrate/frame/nomination-pools/Cargo.toml b/substrate/frame/nomination-pools/Cargo.toml index 4923af0ab0c5..d673edd8536b 100644 --- a/substrate/frame/nomination-pools/Cargo.toml +++ b/substrate/frame/nomination-pools/Cargo.toml @@ -35,8 +35,12 @@ pallet-balances = { path = "../balances" } sp-tracing = { path = "../../primitives/tracing" } [features] -default = [ "std" ] -fuzzing = [ "pallet-balances", "sp-tracing" ] +default = ["std"] +# Enable `VersionedRuntimeUpgrade` for the migrations that is currently still experimental. +experimental = [ + "frame-support/experimental" +] +fuzzing = ["pallet-balances", "sp-tracing"] std = [ "codec/std", "frame-support/std", diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index c4bebc5a1d03..59a6df905270 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -535,6 +535,35 @@ impl PoolMember { } } + /// Total balance of the member, both active and unbonding. + /// Doesn't mutate state. + #[cfg(any(feature = "try-runtime", feature = "fuzzing", test, debug_assertions))] + fn total_balance(&mut self) -> BalanceOf { + let pool = match BondedPool::::get(self.pool_id).defensive() { + Some(pool) => pool, + None => return Zero::zero(), + }; + + let active_balance = pool.points_to_balance(self.active_points()); + + let sub_pools = match SubPoolsStorage::::get(self.pool_id) { + Some(sub_pools) => sub_pools, + None => return active_balance, + }; + + let unbonding_balance = self.unbonding_eras.iter().fold( + BalanceOf::::zero(), + |accumulator, (era, unlocked_points)| { + // if the [`SubPools::with_era`] has already been merged into the + // [`SubPools::no_era`] use this pool instead. + let era_pool = sub_pools.with_era.get(era).unwrap_or(&sub_pools.no_era); + accumulator.saturating_add(era_pool.point_to_balance(*unlocked_points)) + }, + ); + + active_balance.saturating_add(unbonding_balance) + } + /// Total points of this member, both active and unbonding. fn total_points(&self) -> BalanceOf { self.active_points().saturating_add(self.unbonding_points()) @@ -963,6 +992,7 @@ impl BondedPool { } /// Issue points to [`Self`] for `new_funds`. + /// Increase the [`TotalValueLocked`] by `new_funds`. fn issue(&mut self, new_funds: BalanceOf) -> BalanceOf { let points_to_issue = self.balance_to_point(new_funds); self.points = self.points.saturating_add(points_to_issue); @@ -1183,9 +1213,8 @@ impl BondedPool { /// Bond exactly `amount` from `who`'s funds into this pool. /// - /// If the bond type is `Create`, `Staking::bond` is called, and `who` - /// is allowed to be killed. Otherwise, `Staking::bond_extra` is called and `who` - /// cannot be killed. + /// If the bond is [`BondType::Create`], [`Staking::bond`] is called, and `who` is allowed to be + /// killed. Otherwise, [`Staking::bond_extra`] is called and `who` cannot be killed. /// /// Returns `Ok(points_issues)`, `Err` otherwise. fn try_bond_funds( @@ -1216,6 +1245,9 @@ impl BondedPool { // found, we exit early. BondType::Later => T::Staking::bond_extra(&bonded_account, amount)?, } + TotalValueLocked::::mutate(|tvl| { + tvl.saturating_accrue(amount); + }); Ok(points_issued) } @@ -1231,6 +1263,19 @@ impl BondedPool { }); }; } + + fn withdraw_from_staking(&self, num_slashing_spans: u32) -> Result { + let bonded_account = self.bonded_account(); + + let prev_total = T::Staking::total_stake(&bonded_account.clone()).unwrap_or_default(); + let outcome = T::Staking::withdraw_unbonded(bonded_account.clone(), num_slashing_spans); + let diff = + prev_total.saturating_sub(T::Staking::total_stake(&bonded_account).unwrap_or_default()); + TotalValueLocked::::mutate(|tvl| { + tvl.saturating_reduce(diff); + }); + outcome + } } /// A reward pool. @@ -1416,8 +1461,8 @@ impl UnbondPool { new_points } - /// Dissolve some points from the unbonding pool, reducing the balance of the pool - /// proportionally. + /// Dissolve some points from the unbonding pool, reducing the balance of the pool and the + /// [`TotalValueLocked`] proportionally. /// /// This is the opposite of `issue`. /// @@ -1504,7 +1549,7 @@ pub mod pallet { use sp_runtime::Perbill; /// The current storage version. - const STORAGE_VERSION: StorageVersion = StorageVersion::new(5); + const STORAGE_VERSION: StorageVersion = StorageVersion::new(6); #[pallet::pallet] #[pallet::storage_version(STORAGE_VERSION)] @@ -1577,6 +1622,14 @@ pub mod pallet { type MaxUnbonding: Get; } + /// The sum of funds across all pools. + /// + /// This might be higher but never lower than the actual sum of the currently unlocking and + /// bonded funds as this is only decreased if a user withdraws unlocked funds or a slash + /// happened. + #[pallet::storage] + pub type TotalValueLocked = StorageValue<_, BalanceOf, ValueQuery>; + /// Minimum amount to bond to join a pool. #[pallet::storage] pub type MinJoinBond = StorageValue<_, BalanceOf, ValueQuery>; @@ -1795,9 +1848,9 @@ pub mod pallet { CannotWithdrawAny, /// The amount does not meet the minimum bond to either join or create a pool. /// - /// The depositor can never unbond to a value less than - /// `Pallet::depositor_min_bond`. The caller does not have nominating - /// permissions for the pool. Members can never unbond to a value below `MinJoinBond`. + /// The depositor can never unbond to a value less than `Pallet::depositor_min_bond`. The + /// caller does not have nominating permissions for the pool. Members can never unbond to a + /// value below `MinJoinBond`. MinimumBondNotMet, /// The transaction could not be executed due to overflow risk for the pool. OverflowRisk, @@ -2074,7 +2127,7 @@ pub mod pallet { /// Call `withdraw_unbonded` for the pools account. This call can be made by any account. /// - /// This is useful if their are too many unlocking chunks to call `unbond`, and some + /// This is useful if there are too many unlocking chunks to call `unbond`, and some /// can be cleared by withdrawing. In the case there are too many unlocking chunks, the user /// would probably see an error like `NoMoreChunks` emitted from the staking system when /// they attempt to unbond. @@ -2087,10 +2140,12 @@ pub mod pallet { ) -> DispatchResult { let _ = ensure_signed(origin)?; let pool = BondedPool::::get(pool_id).ok_or(Error::::PoolNotFound)?; + // For now we only allow a pool to withdraw unbonded if its not destroying. If the pool // is destroying then `withdraw_unbonded` can be used. ensure!(pool.state != PoolState::Destroying, Error::::NotDestroying); - T::Staking::withdraw_unbonded(pool.bonded_account(), num_slashing_spans)?; + pool.withdraw_from_staking(num_slashing_spans)?; + Ok(()) } @@ -2141,8 +2196,7 @@ pub mod pallet { // Before calculating the `balance_to_unbond`, we call withdraw unbonded to ensure the // `transferrable_balance` is correct. - let stash_killed = - T::Staking::withdraw_unbonded(bonded_pool.bonded_account(), num_slashing_spans)?; + let stash_killed = bonded_pool.withdraw_from_staking(num_slashing_spans)?; // defensive-only: the depositor puts enough funds into the stash so that it will only // be destroyed when they are leaving. @@ -2796,9 +2850,12 @@ impl Pallet { } // Equivalent of (current_balance / current_points) * points - balance(u256(current_balance).saturating_mul(u256(points))) - // We check for zero above - .div(current_points) + balance( + u256(current_balance) + .saturating_mul(u256(points)) + // We check for zero above + .div(u256(current_points)), + ) } /// If the member has some rewards, transfer a payout from the reward pool to the member. @@ -3169,8 +3226,35 @@ impl Pallet { "depositor must always have MinCreateBond stake in the pool, except for when the \ pool is being destroyed and the depositor is the last member", ); + + let expected_tvl: BalanceOf = BondedPools::::iter() + .map(|(id, inner)| { + T::Staking::total_stake( + &BondedPool { id, inner: inner.clone() }.bonded_account(), + ) + .unwrap_or_default() + }) + .reduce(|acc, total_balance| acc + total_balance) + .unwrap_or_default(); + + assert_eq!( + TotalValueLocked::::get(), + expected_tvl, + "TVL deviates from the actual sum of funds of all Pools." + ); + + let total_balance_members: BalanceOf = PoolMembers::::iter() + .map(|(_, mut member)| member.total_balance()) + .reduce(|acc, total_balance| acc + total_balance) + .unwrap_or_default(); + + assert!( + TotalValueLocked::::get() <= total_balance_members, + "TVL must be equal to or less than the total balance of all PoolMembers." + ); Ok(()) })?; + ensure!( MaxPoolMembers::::get().map_or(true, |max| all_members <= max), Error::::MaxPoolMembers @@ -3269,25 +3353,30 @@ impl sp_staking::OnStakingUpdate> for Pall // anything here. slashed_bonded: BalanceOf, slashed_unlocking: &BTreeMap>, + total_slashed: BalanceOf, ) { - if let Some(pool_id) = ReversePoolIdLookup::::get(pool_account) { - let mut sub_pools = match SubPoolsStorage::::get(pool_id).defensive() { - Some(sub_pools) => sub_pools, - None => return, + if let Some(pool_id) = ReversePoolIdLookup::::get(pool_account).defensive() { + match SubPoolsStorage::::get(pool_id) { + Some(mut sub_pools) => { + for (era, slashed_balance) in slashed_unlocking.iter() { + if let Some(pool) = sub_pools.with_era.get_mut(era) { + pool.balance = *slashed_balance; + Self::deposit_event(Event::::UnbondingPoolSlashed { + era: *era, + pool_id, + balance: *slashed_balance, + }); + } + } + SubPoolsStorage::::insert(pool_id, sub_pools); + }, + None => {}, }; - for (era, slashed_balance) in slashed_unlocking.iter() { - if let Some(pool) = sub_pools.with_era.get_mut(era) { - pool.balance = *slashed_balance; - Self::deposit_event(Event::::UnbondingPoolSlashed { - era: *era, - pool_id, - balance: *slashed_balance, - }); - } - } + TotalValueLocked::::mutate(|tvl| { + tvl.saturating_reduce(total_slashed); + }); Self::deposit_event(Event::::PoolSlashed { pool_id, balance: slashed_bonded }); - SubPoolsStorage::::insert(pool_id, sub_pools); } } } diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index 2ae4cd1b8685..dea0ee8bcf72 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -724,4 +724,103 @@ pub mod v5 { Ok(()) } } + + /// This migration accumulates and initializes the [`TotalValueLocked`] for all pools. + pub struct VersionUncheckedMigrateV5ToV6(sp_std::marker::PhantomData); + impl OnRuntimeUpgrade for VersionUncheckedMigrateV5ToV6 { + fn on_runtime_upgrade() -> Weight { + let migrated = BondedPools::::count(); + // The TVL should be the sum of all the funds that are actively staked and in the + // unbonding process of the account of each pool. + let tvl: BalanceOf = BondedPools::::iter() + .map(|(id, inner)| { + T::Staking::total_stake( + &BondedPool { id, inner: inner.clone() }.bonded_account(), + ) + .unwrap_or_default() + }) + .reduce(|acc, total_balance| acc + total_balance) + .unwrap_or_default(); + + TotalValueLocked::::set(tvl); + + log!(info, "Upgraded {} pools with a TVL of {:?}", migrated, tvl); + + // reads: migrated * (BondedPools + Staking::total_stake) + count + onchain + // version + // + // writes: current version + TVL + T::DbWeight::get().reads_writes(migrated.saturating_mul(2).saturating_add(2).into(), 2) + } + + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, TryRuntimeError> { + ensure!( + PoolMembers::::iter_keys().count() == PoolMembers::::iter_values().count(), + "There are undecodable PoolMembers in storage. This migration will not fix that." + ); + ensure!( + BondedPools::::iter_keys().count() == BondedPools::::iter_values().count(), + "There are undecodable BondedPools in storage. This migration will not fix that." + ); + ensure!( + SubPoolsStorage::::iter_keys().count() == + SubPoolsStorage::::iter_values().count(), + "There are undecodable SubPools in storage. This migration will not fix that." + ); + ensure!( + Metadata::::iter_keys().count() == Metadata::::iter_values().count(), + "There are undecodable Metadata in storage. This migration will not fix that." + ); + + Ok(Vec::new()) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(_data: Vec) -> Result<(), TryRuntimeError> { + // ensure [`TotalValueLocked`] contains a value greater zero if any instances of + // BondedPools exist. + if !BondedPools::::count().is_zero() { + ensure!(!TotalValueLocked::::get().is_zero(), "tvl written incorrectly"); + } + + ensure!( + Pallet::::on_chain_storage_version() >= 6, + "nomination-pools::migration::v6: wrong storage version" + ); + + // These should not have been touched - just in case. + ensure!( + PoolMembers::::iter_keys().count() == PoolMembers::::iter_values().count(), + "There are undecodable PoolMembers in storage." + ); + ensure!( + BondedPools::::iter_keys().count() == BondedPools::::iter_values().count(), + "There are undecodable BondedPools in storage." + ); + ensure!( + SubPoolsStorage::::iter_keys().count() == + SubPoolsStorage::::iter_values().count(), + "There are undecodable SubPools in storage." + ); + ensure!( + Metadata::::iter_keys().count() == Metadata::::iter_values().count(), + "There are undecodable Metadata in storage." + ); + + Ok(()) + } + } + + /// [`VersionUncheckedMigrateV5ToV6`] wrapped in a + /// [`frame_support::migrations::VersionedRuntimeUpgrade`], ensuring the migration is only + /// performed when on-chain version is 5. + #[cfg(feature = "experimental")] + pub type VersionCheckedMigrateV5ToV6 = frame_support::migrations::VersionedRuntimeUpgrade< + 5, + 6, + VersionUncheckedMigrateV5ToV6, + crate::pallet::Pallet, + ::DbWeight, + >; } diff --git a/substrate/frame/nomination-pools/src/mock.rs b/substrate/frame/nomination-pools/src/mock.rs index 7d0d729a40d4..77f63c8c1e47 100644 --- a/substrate/frame/nomination-pools/src/mock.rs +++ b/substrate/frame/nomination-pools/src/mock.rs @@ -3,7 +3,7 @@ use crate::{self as pools}; use frame_support::{assert_ok, parameter_types, PalletId}; use frame_system::RawOrigin; use sp_runtime::{BuildStorage, FixedU128}; -use sp_staking::Stake; +use sp_staking::{OnStakingUpdate, Stake}; pub type BlockNumber = u64; pub type AccountId = u128; @@ -28,7 +28,8 @@ parameter_types! { pub static CurrentEra: EraIndex = 0; pub static BondingDuration: EraIndex = 3; pub storage BondedBalanceMap: BTreeMap = Default::default(); - pub storage UnbondingBalanceMap: BTreeMap = Default::default(); + // map from user, to a vec of era to amount being unlocked in that era. + pub storage UnbondingBalanceMap: BTreeMap> = Default::default(); #[derive(Clone, PartialEq)] pub static MaxUnbonding: u32 = 8; pub static StakingMinBond: Balance = 10; @@ -42,6 +43,14 @@ impl StakingMock { x.insert(who, bonded); BondedBalanceMap::set(&x) } + + pub fn slash_to(pool_id: PoolId, amount: Balance) { + let acc = Pools::create_bonded_account(pool_id); + let bonded = BondedBalanceMap::get(); + let pre_total = bonded.get(&acc).unwrap(); + Self::set_bonded_balance(acc, pre_total - amount); + Pools::on_slash(&acc, pre_total - amount, &Default::default(), amount); + } } impl sp_staking::StakingInterface for StakingMock { @@ -87,8 +96,11 @@ impl sp_staking::StakingInterface for StakingMock { let mut x = BondedBalanceMap::get(); *x.get_mut(who).unwrap() = x.get_mut(who).unwrap().saturating_sub(amount); BondedBalanceMap::set(&x); + + let era = Self::current_era(); + let unlocking_at = era + Self::bonding_duration(); let mut y = UnbondingBalanceMap::get(); - *y.entry(*who).or_insert(Self::Balance::zero()) += amount; + y.entry(*who).or_insert(Default::default()).push((unlocking_at, amount)); UnbondingBalanceMap::set(&y); Ok(()) } @@ -98,11 +110,13 @@ impl sp_staking::StakingInterface for StakingMock { } fn withdraw_unbonded(who: Self::AccountId, _: u32) -> Result { - // Simulates removing unlocking chunks and only having the bonded balance locked - let mut x = UnbondingBalanceMap::get(); - x.remove(&who); - UnbondingBalanceMap::set(&x); + let mut unbonding_map = UnbondingBalanceMap::get(); + let staker_map = unbonding_map.get_mut(&who).ok_or("not a staker")?; + + let current_era = Self::current_era(); + staker_map.retain(|(unlocking_at, _amount)| *unlocking_at > current_era); + UnbondingBalanceMap::set(&unbonding_map); Ok(UnbondingBalanceMap::get().is_empty() && BondedBalanceMap::get().is_empty()) } @@ -126,14 +140,17 @@ impl sp_staking::StakingInterface for StakingMock { } fn stake(who: &Self::AccountId) -> Result, DispatchError> { - match ( - UnbondingBalanceMap::get().get(who).copied(), - BondedBalanceMap::get().get(who).copied(), - ) { + match (UnbondingBalanceMap::get().get(who), BondedBalanceMap::get().get(who).copied()) { (None, None) => Err(DispatchError::Other("balance not found")), - (Some(v), None) => Ok(Stake { total: v, active: 0 }), + (Some(v), None) => Ok(Stake { + total: v.into_iter().fold(0u128, |acc, &x| acc.saturating_add(x.1)), + active: 0, + }), (None, Some(v)) => Ok(Stake { total: v, active: v }), - (Some(a), Some(b)) => Ok(Stake { total: a + b, active: b }), + (Some(a), Some(b)) => Ok(Stake { + total: a.into_iter().fold(0u128, |acc, &x| acc.saturating_add(x.1)) + b, + active: b, + }), } } diff --git a/substrate/frame/nomination-pools/src/tests.rs b/substrate/frame/nomination-pools/src/tests.rs index d0fe4e40a18b..9f64c6e5e655 100644 --- a/substrate/frame/nomination-pools/src/tests.rs +++ b/substrate/frame/nomination-pools/src/tests.rs @@ -59,6 +59,9 @@ fn test_setup_works() { assert_eq!(StakingMock::bonding_duration(), 3); assert!(Metadata::::contains_key(1)); + // initial member. + assert_eq!(TotalValueLocked::::get(), 10); + let last_pool = LastPoolId::::get(); assert_eq!( BondedPool::::get(last_pool).unwrap(), @@ -218,10 +221,7 @@ mod bonded_pool { // slash half of the pool's balance. expected result of `fn api_points_to_balance` // to be 1/2 of the pool's balance. - StakingMock::set_bonded_balance( - default_bonded_account(), - Pools::depositor_min_bond() / 2, - ); + StakingMock::slash_to(1, Pools::depositor_min_bond() / 2); assert_eq!(Pallet::::api_points_to_balance(1, 10), 5); // if pool does not exist, points to balance ratio is 0. @@ -238,10 +238,7 @@ mod bonded_pool { // slash half of the pool's balance. expect result of `fn api_balance_to_points` // to be 2 * of the balance to add to the pool. - StakingMock::set_bonded_balance( - default_bonded_account(), - Pools::depositor_min_bond() / 2, - ); + StakingMock::slash_to(1, Pools::depositor_min_bond() / 2); assert_eq!(Pallet::::api_balance_to_points(1, 10), 20); // if pool does not exist, balance to points ratio is 0. @@ -499,12 +496,12 @@ mod join { // Given Balances::make_free_balance_be(&11, ExistentialDeposit::get() + 2); assert!(!PoolMembers::::contains_key(11)); + assert_eq!(TotalValueLocked::::get(), 10); // When assert_ok!(Pools::join(RuntimeOrigin::signed(11), 2, 1)); // Then - assert_eq!( pool_events_since_last_call(), vec![ @@ -513,6 +510,7 @@ mod join { Event::Bonded { member: 11, pool_id: 1, bonded: 2, joined: true }, ] ); + assert_eq!(TotalValueLocked::::get(), 12); assert_eq!( PoolMembers::::get(11).unwrap(), @@ -536,6 +534,7 @@ mod join { pool_events_since_last_call(), vec![Event::Bonded { member: 12, pool_id: 1, bonded: 12, joined: true }] ); + assert_eq!(TotalValueLocked::::get(), 24); assert_eq!( PoolMembers::::get(12).unwrap(), @@ -2221,11 +2220,15 @@ mod unbond { .min_join_bond(10) .add_members(vec![(20, 20)]) .build_and_execute(|| { + assert_eq!(TotalValueLocked::::get(), 30); // can unbond to above limit assert_ok!(Pools::unbond(RuntimeOrigin::signed(20), 20, 5)); assert_eq!(PoolMembers::::get(20).unwrap().active_points(), 15); assert_eq!(PoolMembers::::get(20).unwrap().unbonding_points(), 5); + // tvl remains unchanged. + assert_eq!(TotalValueLocked::::get(), 30); + // cannot go to below 10: assert_noop!( Pools::unbond(RuntimeOrigin::signed(20), 20, 10), @@ -2531,8 +2534,8 @@ mod unbond { .add_members(vec![(40, 40), (550, 550)]) .build_and_execute(|| { let ed = Balances::minimum_balance(); - // Given a slash from 600 -> 100 - StakingMock::set_bonded_balance(default_bonded_account(), 100); + // Given a slash from 600 -> 500 + StakingMock::slash_to(1, 500); // and unclaimed rewards of 600. Balances::make_free_balance_be(&default_reward_account(), ed + 600); @@ -2564,8 +2567,9 @@ mod unbond { Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true }, Event::Bonded { member: 40, pool_id: 1, bonded: 40, joined: true }, Event::Bonded { member: 550, pool_id: 1, bonded: 550, joined: true }, + Event::PoolSlashed { pool_id: 1, balance: 100 }, Event::PaidOut { member: 40, pool_id: 1, payout: 40 }, - Event::Unbonded { member: 40, pool_id: 1, points: 6, balance: 6, era: 3 } + Event::Unbonded { member: 40, pool_id: 1, balance: 6, points: 6, era: 3 } ] ); @@ -2725,6 +2729,7 @@ mod unbond { ); // When the root kicks then its ok + // Account with ID 100 is kicked. assert_ok!(Pools::fully_unbond(RuntimeOrigin::signed(900), 100)); assert_eq!( @@ -2745,6 +2750,7 @@ mod unbond { ); // When the bouncer kicks then its ok + // Account with ID 200 is kicked. assert_ok!(Pools::fully_unbond(RuntimeOrigin::signed(902), 200)); assert_eq!( @@ -2783,7 +2789,7 @@ mod unbond { ); assert_eq!( *UnbondingBalanceMap::get().get(&default_bonded_account()).unwrap(), - 100 + 200 + vec![(3, 100), (3, 200)], ); }); } @@ -2882,7 +2888,10 @@ mod unbond { } ); assert_eq!(StakingMock::active_stake(&default_bonded_account()).unwrap(), 0); - assert_eq!(*UnbondingBalanceMap::get().get(&default_bonded_account()).unwrap(), 10); + assert_eq!( + *UnbondingBalanceMap::get().get(&default_bonded_account()).unwrap(), + vec![(6, 10)] + ); }); } @@ -3160,7 +3169,7 @@ mod unbond { assert_eq!(PoolMembers::::get(10).unwrap().unbonding_points(), 0); // slash the default pool - StakingMock::set_bonded_balance(Pools::create_bonded_account(1), 5); + StakingMock::slash_to(1, 5); // cannot unbond even 7, because the value of shares is now less. assert_noop!( @@ -3239,21 +3248,23 @@ mod pool_withdraw_unbonded { #[test] fn pool_withdraw_unbonded_works() { - ExtBuilder::default().build_and_execute(|| { + ExtBuilder::default().add_members(vec![(20, 10)]).build_and_execute(|| { // Given 10 unbond'ed directly against the pool account - assert_ok!(StakingMock::unbond(&default_bonded_account(), 5)); - // and the pool account only has 10 balance - assert_eq!(StakingMock::active_stake(&default_bonded_account()), Ok(5)); - assert_eq!(StakingMock::total_stake(&default_bonded_account()), Ok(10)); - assert_eq!(Balances::free_balance(&default_bonded_account()), 10); + + assert_ok!(Pools::unbond(RuntimeOrigin::signed(20), 20, 5)); + + assert_eq!(StakingMock::active_stake(&default_bonded_account()), Ok(15)); + assert_eq!(StakingMock::total_stake(&default_bonded_account()), Ok(20)); + assert_eq!(Balances::free_balance(&default_bonded_account()), 20); // When + CurrentEra::set(StakingMock::current_era() + StakingMock::bonding_duration() + 1); assert_ok!(Pools::pool_withdraw_unbonded(RuntimeOrigin::signed(10), 1, 0)); // Then there unbonding balance is no longer locked - assert_eq!(StakingMock::active_stake(&default_bonded_account()), Ok(5)); - assert_eq!(StakingMock::total_stake(&default_bonded_account()), Ok(5)); - assert_eq!(Balances::free_balance(&default_bonded_account()), 10); + assert_eq!(StakingMock::active_stake(&default_bonded_account()), Ok(15)); + assert_eq!(StakingMock::total_stake(&default_bonded_account()), Ok(15)); + assert_eq!(Balances::free_balance(&default_bonded_account()), 20); }); } } @@ -3290,8 +3301,9 @@ mod withdraw_unbonded { unbond_pool.balance /= 2; // 295 SubPoolsStorage::::insert(1, sub_pools); // Update the equivalent of the unbonding chunks for the `StakingMock` - let mut x = UnbondingBalanceMap::get(); - *x.get_mut(&default_bonded_account()).unwrap() /= 5; + let x = UnbondingBalanceMap::get(); + // TODO: + // *x.get_mut(&default_bonded_account()).unwrap() /= 5; UnbondingBalanceMap::set(&x); Balances::make_free_balance_be( &default_bonded_account(), @@ -3920,6 +3932,7 @@ mod withdraw_unbonded { #[test] fn full_multi_step_withdrawing_non_depositor() { ExtBuilder::default().add_members(vec![(100, 100)]).build_and_execute(|| { + assert_eq!(TotalValueLocked::::get(), 110); // given assert_ok!(Pools::unbond(RuntimeOrigin::signed(100), 100, 75)); assert_eq!( @@ -3927,6 +3940,9 @@ mod withdraw_unbonded { member_unbonding_eras!(3 => 75) ); + // tvl unchanged. + assert_eq!(TotalValueLocked::::get(), 110); + // progress one era and unbond the leftover. CurrentEra::set(1); assert_ok!(Pools::unbond(RuntimeOrigin::signed(100), 100, 25)); @@ -3939,6 +3955,8 @@ mod withdraw_unbonded { Pools::withdraw_unbonded(RuntimeOrigin::signed(100), 100, 0), Error::::CannotWithdrawAny ); + // tvl unchanged. + assert_eq!(TotalValueLocked::::get(), 110); // now the 75 should be free. CurrentEra::set(3); @@ -3958,6 +3976,8 @@ mod withdraw_unbonded { PoolMembers::::get(100).unwrap().unbonding_eras, member_unbonding_eras!(4 => 25) ); + // tvl updated + assert_eq!(TotalValueLocked::::get(), 35); // the 25 should be free now, and the member removed. CurrentEra::set(4); @@ -4266,6 +4286,7 @@ mod create { let next_pool_stash = Pools::create_bonded_account(2); let ed = Balances::minimum_balance(); + assert_eq!(TotalValueLocked::::get(), 10); assert!(!BondedPools::::contains_key(2)); assert!(!RewardPools::::contains_key(2)); assert!(!PoolMembers::::contains_key(11)); @@ -4279,6 +4300,7 @@ mod create { 456, 789 )); + assert_eq!(TotalValueLocked::::get(), 10 + StakingMock::minimum_nominator_bond()); assert_eq!(Balances::free_balance(&11), 0); assert_eq!( @@ -4560,9 +4582,10 @@ mod set_state { // Given unsafe_set_state(1, PoolState::Open); - let mut bonded_pool = BondedPool::::get(1).unwrap(); - bonded_pool.points = 100; - bonded_pool.put(); + // slash the pool to the point that `max_points_to_balance` ratio is + // surpassed. Making this pool destroyable by anyone. + StakingMock::slash_to(1, 10); + // When assert_ok!(Pools::set_state(RuntimeOrigin::signed(11), 1, PoolState::Destroying)); // Then @@ -4588,6 +4611,7 @@ mod set_state { pool_events_since_last_call(), vec![ Event::StateChanged { pool_id: 1, new_state: PoolState::Destroying }, + Event::PoolSlashed { pool_id: 1, balance: 0 }, Event::StateChanged { pool_id: 1, new_state: PoolState::Destroying }, Event::StateChanged { pool_id: 1, new_state: PoolState::Destroying } ] @@ -4788,6 +4812,7 @@ mod bond_extra { assert_eq!(BondedPools::::get(1).unwrap().points, 30); assert_eq!(Balances::free_balance(10), 35); assert_eq!(Balances::free_balance(20), 20); + assert_eq!(TotalValueLocked::::get(), 30); // when assert_ok!(Pools::bond_extra(RuntimeOrigin::signed(10), BondExtra::Rewards)); @@ -4795,6 +4820,7 @@ mod bond_extra { // then assert_eq!(Balances::free_balance(10), 35); + assert_eq!(TotalValueLocked::::get(), 31); // 10's share of the reward is 1/3, since they gave 10/30 of the total shares. assert_eq!(PoolMembers::::get(10).unwrap().points, 10 + 1); assert_eq!(BondedPools::::get(1).unwrap().points, 30 + 1); @@ -4804,6 +4830,7 @@ mod bond_extra { // then assert_eq!(Balances::free_balance(20), 20); + assert_eq!(TotalValueLocked::::get(), 33); // 20's share of the rewards is the other 2/3 of the rewards, since they have 20/30 of // the shares assert_eq!(PoolMembers::::get(20).unwrap().points, 20 + 2); @@ -5213,7 +5240,7 @@ mod reward_counter_precision { ); // slash this pool by 99% of that. - StakingMock::set_bonded_balance(default_bonded_account(), DOT + pool_bond / 100); + StakingMock::slash_to(1, pool_bond * 99 / 100); // some whale now joins with the other half ot the total issuance. This will trigger an // overflow. This test is actually a bit too lenient because all the reward counters are @@ -6727,3 +6754,74 @@ mod commission { }) } } +mod slash { + + use super::*; + + #[test] + fn slash_no_subpool_is_tracked() { + let bonded = |points, member_counter| BondedPool:: { + id: 1, + inner: BondedPoolInner { + commission: Commission::default(), + member_counter, + points, + roles: DEFAULT_ROLES, + state: PoolState::Open, + }, + }; + ExtBuilder::default().with_check(0).build_and_execute(|| { + // Given + Balances::make_free_balance_be(&11, ExistentialDeposit::get() + 2); + assert!(!PoolMembers::::contains_key(11)); + assert_eq!(TotalValueLocked::::get(), 10); + + // When + assert_ok!(Pools::join(RuntimeOrigin::signed(11), 2, 1)); + + // Then + assert_eq!( + pool_events_since_last_call(), + vec![ + Event::Created { depositor: 10, pool_id: 1 }, + Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true }, + Event::Bonded { member: 11, pool_id: 1, bonded: 2, joined: true }, + ] + ); + assert_eq!(TotalValueLocked::::get(), 12); + + assert_eq!( + PoolMembers::::get(11).unwrap(), + PoolMember:: { pool_id: 1, points: 2, ..Default::default() } + ); + assert_eq!(BondedPool::::get(1).unwrap(), bonded(12, 2)); + + // Given + // The bonded balance is slashed in half + StakingMock::slash_to(1, 6); + + // And + Balances::make_free_balance_be(&12, ExistentialDeposit::get() + 12); + assert!(!PoolMembers::::contains_key(12)); + + // When + assert_ok!(Pools::join(RuntimeOrigin::signed(12), 12, 1)); + + // Then + assert_eq!( + pool_events_since_last_call(), + vec![ + Event::PoolSlashed { pool_id: 1, balance: 6 }, + Event::Bonded { member: 12, pool_id: 1, bonded: 12, joined: true } + ] + ); + assert_eq!(TotalValueLocked::::get(), 18); + + assert_eq!( + PoolMembers::::get(12).unwrap(), + PoolMember:: { pool_id: 1, points: 24, ..Default::default() } + ); + assert_eq!(BondedPool::::get(1).unwrap(), bonded(12 + 24, 3)); + }); + } +} diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs index e59b2a3324a6..dcf57a464323 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/substrate/frame/staking/src/lib.rs @@ -671,8 +671,14 @@ impl StakingLedger { // clean unlocking chunks that are set to zero. self.unlocking.retain(|c| !c.value.is_zero()); - T::EventListeners::on_slash(&self.stash, self.active, &slashed_unlocking); - pre_slash_total.saturating_sub(self.total) + let final_slashed_amount = pre_slash_total.saturating_sub(self.total); + T::EventListeners::on_slash( + &self.stash, + self.active, + &slashed_unlocking, + final_slashed_amount, + ); + final_slashed_amount } } diff --git a/substrate/frame/staking/src/mock.rs b/substrate/frame/staking/src/mock.rs index cf08f8be1f27..c41144278f2c 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/substrate/frame/staking/src/mock.rs @@ -278,6 +278,7 @@ impl OnStakingUpdate for EventListenerMock { _pool_account: &AccountId, slashed_bonded: Balance, slashed_chunks: &BTreeMap, + _total_slashed: Balance, ) { LedgerSlashPerEra::set((slashed_bonded, slashed_chunks.clone())); } diff --git a/substrate/primitives/staking/src/lib.rs b/substrate/primitives/staking/src/lib.rs index 1621af164b37..8b5797d7918f 100644 --- a/substrate/primitives/staking/src/lib.rs +++ b/substrate/primitives/staking/src/lib.rs @@ -121,10 +121,12 @@ pub trait OnStakingUpdate { /// * `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. + /// * `slashed_total` - The aggregated balance that was lost due to the slash. fn on_slash( _stash: &AccountId, _slashed_active: Balance, _slashed_unlocking: &BTreeMap, + _slashed_total: Balance, ) { } } From b1f992771c6fada54728f4e4612e55d9470e72c1 Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Thu, 31 Aug 2023 09:26:28 +0200 Subject: [PATCH 02/43] use ensure in try_state --- substrate/frame/nomination-pools/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 59a6df905270..da73025897c7 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -3237,8 +3237,8 @@ impl Pallet { .reduce(|acc, total_balance| acc + total_balance) .unwrap_or_default(); - assert_eq!( - TotalValueLocked::::get(), + ensure!( + TotalValueLocked::::get() == expected_tvl, "TVL deviates from the actual sum of funds of all Pools." ); @@ -3248,7 +3248,7 @@ impl Pallet { .reduce(|acc, total_balance| acc + total_balance) .unwrap_or_default(); - assert!( + ensure!( TotalValueLocked::::get() <= total_balance_members, "TVL must be equal to or less than the total balance of all PoolMembers." ); From 7b75db0177e228f9080a624ac57e8c8ac497b13c Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Thu, 31 Aug 2023 14:02:19 +0200 Subject: [PATCH 03/43] add doc comment --- substrate/frame/nomination-pools/src/lib.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index da73025897c7..f1d3872784a5 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -1264,6 +1264,14 @@ impl BondedPool { }; } + /// Withdraw all the funds that are already unlocked from staking for the + /// [`BondedPool::bonded_account`]. + /// + /// Also reduces the [`TotalValueLocked`] by the difference of the + /// [`T::Staking::total_stake`] of the [`BondedPool::bonded_account`] that might occur by + /// [`T::Staking::withdraw_unbonded`]. + /// + /// Returns the result of [`T::Staking::withdraw_unbonded`] fn withdraw_from_staking(&self, num_slashing_spans: u32) -> Result { let bonded_account = self.bonded_account(); @@ -1461,9 +1469,7 @@ impl UnbondPool { new_points } - /// Dissolve some points from the unbonding pool, reducing the balance of the pool and the - /// [`TotalValueLocked`] proportionally. - /// + /// Dissolve some points from the unbonding pool, reducing the balance of the pool. /// This is the opposite of `issue`. /// /// Returns the actual amount of `Balance` that was removed from the pool. @@ -3238,8 +3244,7 @@ impl Pallet { .unwrap_or_default(); ensure!( - TotalValueLocked::::get() == - expected_tvl, + TotalValueLocked::::get() == expected_tvl, "TVL deviates from the actual sum of funds of all Pools." ); From 994315f99a84738d3fa5ba75bbcabe8bf1253888 Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Thu, 31 Aug 2023 15:31:38 +0200 Subject: [PATCH 04/43] slashing doc comments --- substrate/frame/nomination-pools/src/lib.rs | 5 +++++ substrate/frame/nomination-pools/src/mock.rs | 4 +++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index f1d3872784a5..73ee7a038ae3 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -3352,6 +3352,11 @@ impl Pallet { } impl sp_staking::OnStakingUpdate> for Pallet { + /// Reduces the balances of the [`SubPools::with_era`], that belong to the pool involved in the + /// slash, to the amount that is defined in the `slashed_unlocking` field of + /// [`sp_staking::OnStakingUpdate::on_slash`] + /// + /// Emits [`Event::::PoolsSlashed`]. fn on_slash( pool_account: &T::AccountId, // Bonded balance is always read directly from staking, therefore we don't need to update diff --git a/substrate/frame/nomination-pools/src/mock.rs b/substrate/frame/nomination-pools/src/mock.rs index c2101e41712c..aa6b18d7879a 100644 --- a/substrate/frame/nomination-pools/src/mock.rs +++ b/substrate/frame/nomination-pools/src/mock.rs @@ -60,7 +60,9 @@ impl StakingMock { x.insert(who, bonded); BondedBalanceMap::set(&x) } - + /// Mimics a slash towards a pool specified by `pool_id`. + /// This sets the bonded balance of a pool to the `amount` and calls [`Pools::on_slash`] to + /// enact changes in the nomination-pool pallet. pub fn slash_to(pool_id: PoolId, amount: Balance) { let acc = Pools::create_bonded_account(pool_id); let bonded = BondedBalanceMap::get(); From a38564f8d366598604ad96cfe02b78e5ce3e0a91 Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Sat, 2 Sep 2023 10:58:33 +0200 Subject: [PATCH 05/43] simplify on_slash --- substrate/frame/nomination-pools/src/lib.rs | 44 ++++++++++----------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 73ee7a038ae3..8452a353cff7 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -3365,28 +3365,26 @@ impl sp_staking::OnStakingUpdate> for Pall slashed_unlocking: &BTreeMap>, total_slashed: BalanceOf, ) { - if let Some(pool_id) = ReversePoolIdLookup::::get(pool_account).defensive() { - match SubPoolsStorage::::get(pool_id) { - Some(mut sub_pools) => { - for (era, slashed_balance) in slashed_unlocking.iter() { - if let Some(pool) = sub_pools.with_era.get_mut(era) { - pool.balance = *slashed_balance; - Self::deposit_event(Event::::UnbondingPoolSlashed { - era: *era, - pool_id, - balance: *slashed_balance, - }); - } - } - SubPoolsStorage::::insert(pool_id, sub_pools); - }, - None => {}, - }; - - TotalValueLocked::::mutate(|tvl| { - tvl.saturating_reduce(total_slashed); - }); - Self::deposit_event(Event::::PoolSlashed { pool_id, balance: slashed_bonded }); - } + let Some(pool_id) = ReversePoolIdLookup::::get(pool_account).defensive() else { return }; + // As the slashed account belongs to a [`BondedPool`] the [`TotalValueLocked`] decreases and + // an event is emitted. + TotalValueLocked::::mutate(|tvl| { + tvl.saturating_reduce(total_slashed); + }); + Self::deposit_event(Event::::PoolSlashed { pool_id, balance: slashed_bonded }); + + let Some(mut sub_pools) = SubPoolsStorage::::get(pool_id) else { return }; + // set the reduced balance for each of the [`SubPools`] + slashed_unlocking.iter().for_each(|(era, slashed_balance)| { + if let Some(pool) = sub_pools.with_era.get_mut(era) { + pool.balance = *slashed_balance; + Self::deposit_event(Event::::UnbondingPoolSlashed { + era: *era, + pool_id, + balance: *slashed_balance, + }); + } + }); + SubPoolsStorage::::insert(pool_id, sub_pools); } } From 8480667a8969d6009634caebe805082f8fa6a9c8 Mon Sep 17 00:00:00 2001 From: Piet <75956460+PieWol@users.noreply.github.com> Date: Sat, 2 Sep 2023 11:11:35 +0200 Subject: [PATCH 06/43] Update substrate/frame/nomination-pools/src/mock.rs comment Co-authored-by: Liam Aharon --- substrate/frame/nomination-pools/src/mock.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/nomination-pools/src/mock.rs b/substrate/frame/nomination-pools/src/mock.rs index aa6b18d7879a..cd5fe35fb835 100644 --- a/substrate/frame/nomination-pools/src/mock.rs +++ b/substrate/frame/nomination-pools/src/mock.rs @@ -45,7 +45,7 @@ parameter_types! { pub static CurrentEra: EraIndex = 0; pub static BondingDuration: EraIndex = 3; pub storage BondedBalanceMap: BTreeMap = Default::default(); - // map from user, to a vec of era to amount being unlocked in that era. + // map from a user to a vec of eras and amounts being unlocked in each era. pub storage UnbondingBalanceMap: BTreeMap> = Default::default(); #[derive(Clone, PartialEq)] pub static MaxUnbonding: u32 = 8; From a4f63f83bd00472f6de9a2df5859607b3f9b384b Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Sat, 2 Sep 2023 14:40:51 +0200 Subject: [PATCH 07/43] remove a test + adjust comments --- substrate/frame/nomination-pools/src/mock.rs | 5 +- substrate/frame/nomination-pools/src/tests.rs | 153 ------------------ 2 files changed, 4 insertions(+), 154 deletions(-) diff --git a/substrate/frame/nomination-pools/src/mock.rs b/substrate/frame/nomination-pools/src/mock.rs index cd5fe35fb835..927208bf90de 100644 --- a/substrate/frame/nomination-pools/src/mock.rs +++ b/substrate/frame/nomination-pools/src/mock.rs @@ -61,8 +61,11 @@ impl StakingMock { BondedBalanceMap::set(&x) } /// Mimics a slash towards a pool specified by `pool_id`. - /// This sets the bonded balance of a pool to the `amount` and calls [`Pools::on_slash`] to + /// This reduces the bonded balance of a pool by `amount` and calls [`Pools::on_slash`] to /// enact changes in the nomination-pool pallet. + /// + /// Does not modify any [`SubPools::with_era`] of the pool as [`Default::default`] is passed for + /// `slashed_unlocking`. pub fn slash_to(pool_id: PoolId, amount: Balance) { let acc = Pools::create_bonded_account(pool_id); let bonded = BondedBalanceMap::get(); diff --git a/substrate/frame/nomination-pools/src/tests.rs b/substrate/frame/nomination-pools/src/tests.rs index 9f64c6e5e655..0b14a55bf97a 100644 --- a/substrate/frame/nomination-pools/src/tests.rs +++ b/substrate/frame/nomination-pools/src/tests.rs @@ -3271,159 +3271,6 @@ mod pool_withdraw_unbonded { mod withdraw_unbonded { use super::*; - use sp_runtime::bounded_btree_map; - - #[test] - fn withdraw_unbonded_works_against_slashed_no_era_sub_pool() { - ExtBuilder::default() - .add_members(vec![(40, 40), (550, 550)]) - .build_and_execute(|| { - // reduce the noise a bit. - let _ = balances_events_since_last_call(); - - // Given - assert_eq!(StakingMock::bonding_duration(), 3); - assert_ok!(Pools::fully_unbond(RuntimeOrigin::signed(550), 550)); - assert_ok!(Pools::fully_unbond(RuntimeOrigin::signed(40), 40)); - assert_eq!(Balances::free_balance(&default_bonded_account()), 600); - - let mut current_era = 1; - CurrentEra::set(current_era); - - let mut sub_pools = SubPoolsStorage::::get(1).unwrap(); - let unbond_pool = sub_pools.with_era.get_mut(&3).unwrap(); - // Sanity check - assert_eq!(*unbond_pool, UnbondPool { points: 550 + 40, balance: 550 + 40 }); - - // Simulate a slash to the pool with_era(current_era), decreasing the balance by - // half - { - unbond_pool.balance /= 2; // 295 - SubPoolsStorage::::insert(1, sub_pools); - // Update the equivalent of the unbonding chunks for the `StakingMock` - let x = UnbondingBalanceMap::get(); - // TODO: - // *x.get_mut(&default_bonded_account()).unwrap() /= 5; - UnbondingBalanceMap::set(&x); - Balances::make_free_balance_be( - &default_bonded_account(), - Balances::free_balance(&default_bonded_account()) / 2, // 300 - ); - StakingMock::set_bonded_balance( - default_bonded_account(), - StakingMock::active_stake(&default_bonded_account()).unwrap() / 2, - ); - }; - - // Advance the current_era to ensure all `with_era` pools will be merged into - // `no_era` pool - current_era += TotalUnbondingPools::::get(); - CurrentEra::set(current_era); - - // Simulate some other call to unbond that would merge `with_era` pools into - // `no_era` - let sub_pools = - SubPoolsStorage::::get(1).unwrap().maybe_merge_pools(current_era); - SubPoolsStorage::::insert(1, sub_pools); - - assert_eq!( - SubPoolsStorage::::get(1).unwrap(), - SubPools { - no_era: UnbondPool { points: 550 + 40, balance: 275 + 20 }, - with_era: Default::default() - } - ); - assert_eq!( - pool_events_since_last_call(), - vec![ - Event::Created { depositor: 10, pool_id: 1 }, - Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true }, - Event::Bonded { member: 40, pool_id: 1, bonded: 40, joined: true }, - Event::Bonded { member: 550, pool_id: 1, bonded: 550, joined: true }, - Event::Unbonded { - member: 550, - pool_id: 1, - points: 550, - balance: 550, - era: 3 - }, - Event::Unbonded { member: 40, pool_id: 1, points: 40, balance: 40, era: 3 }, - ] - ); - assert_eq!( - balances_events_since_last_call(), - vec![BEvent::BalanceSet { who: default_bonded_account(), free: 300 }] - ); - - // When - assert_ok!(Pools::withdraw_unbonded(RuntimeOrigin::signed(550), 550, 0)); - - // Then - assert_eq!( - SubPoolsStorage::::get(1).unwrap().no_era, - UnbondPool { points: 40, balance: 20 } - ); - assert_eq!( - pool_events_since_last_call(), - vec![ - Event::Withdrawn { member: 550, pool_id: 1, balance: 275, points: 550 }, - Event::MemberRemoved { pool_id: 1, member: 550 } - ] - ); - assert_eq!( - balances_events_since_last_call(), - vec![BEvent::Transfer { from: default_bonded_account(), to: 550, amount: 275 }] - ); - - // When - assert_ok!(Pools::withdraw_unbonded(RuntimeOrigin::signed(40), 40, 0)); - - // Then - assert_eq!( - SubPoolsStorage::::get(1).unwrap().no_era, - UnbondPool { points: 0, balance: 0 } - ); - assert!(!PoolMembers::::contains_key(40)); - assert_eq!( - pool_events_since_last_call(), - vec![ - Event::Withdrawn { member: 40, pool_id: 1, balance: 20, points: 40 }, - Event::MemberRemoved { pool_id: 1, member: 40 } - ] - ); - assert_eq!( - balances_events_since_last_call(), - vec![BEvent::Transfer { from: default_bonded_account(), to: 40, amount: 20 }] - ); - - // now, finally, the depositor can take out its share. - unsafe_set_state(1, PoolState::Destroying); - assert_ok!(fully_unbond_permissioned(10)); - - current_era += 3; - CurrentEra::set(current_era); - - // when - assert_ok!(Pools::withdraw_unbonded(RuntimeOrigin::signed(10), 10, 0)); - assert_eq!( - pool_events_since_last_call(), - vec![ - Event::Unbonded { member: 10, pool_id: 1, balance: 5, points: 5, era: 9 }, - Event::Withdrawn { member: 10, pool_id: 1, balance: 5, points: 5 }, - Event::MemberRemoved { pool_id: 1, member: 10 }, - Event::Destroyed { pool_id: 1 } - ] - ); - assert!(!Metadata::::contains_key(1)); - assert_eq!( - balances_events_since_last_call(), - vec![ - BEvent::Transfer { from: default_bonded_account(), to: 10, amount: 5 }, - BEvent::Transfer { from: default_reward_account(), to: 10, amount: 5 } - ] - ); - }); - } #[test] fn withdraw_unbonded_works_against_slashed_with_era_sub_pools() { From ff5c2f52d61d124a44139705d8ca9f588f469d9d Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Sat, 2 Sep 2023 16:27:16 +0200 Subject: [PATCH 08/43] enhance `post_upgrade` --- .../frame/nomination-pools/src/migration.rs | 43 +++++++++++++------ 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index dea0ee8bcf72..4e21013b20f3 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -727,12 +727,9 @@ pub mod v5 { /// This migration accumulates and initializes the [`TotalValueLocked`] for all pools. pub struct VersionUncheckedMigrateV5ToV6(sp_std::marker::PhantomData); - impl OnRuntimeUpgrade for VersionUncheckedMigrateV5ToV6 { - fn on_runtime_upgrade() -> Weight { - let migrated = BondedPools::::count(); - // The TVL should be the sum of all the funds that are actively staked and in the - // unbonding process of the account of each pool. - let tvl: BalanceOf = BondedPools::::iter() + impl VersionUncheckedMigrateV5ToV6 { + fn calculate_tvl_by_total_stake() -> BalanceOf { + BondedPools::::iter() .map(|(id, inner)| { T::Staking::total_stake( &BondedPool { id, inner: inner.clone() }.bonded_account(), @@ -740,7 +737,15 @@ pub mod v5 { .unwrap_or_default() }) .reduce(|acc, total_balance| acc + total_balance) - .unwrap_or_default(); + .unwrap_or_default() + } + } + impl OnRuntimeUpgrade for VersionUncheckedMigrateV5ToV6 { + fn on_runtime_upgrade() -> Weight { + let migrated = BondedPools::::count(); + // The TVL should be the sum of all the funds that are actively staked and in the + // unbonding process of the account of each pool. + let tvl: BalanceOf = Self::calculate_tvl_by_total_stake(); TotalValueLocked::::set(tvl); @@ -778,11 +783,25 @@ pub mod v5 { #[cfg(feature = "try-runtime")] fn post_upgrade(_data: Vec) -> Result<(), TryRuntimeError> { - // ensure [`TotalValueLocked`] contains a value greater zero if any instances of - // BondedPools exist. - if !BondedPools::::count().is_zero() { - ensure!(!TotalValueLocked::::get().is_zero(), "tvl written incorrectly"); - } + // check that the `TotalValueLocked` written is actually the sum of `total_stake` of the + // `BondedPools`` + let tvl: BalanceOf = Self::calculate_tvl_by_total_stake(); + ensure!( + TotalValueLocked::::get() == tvl, + "TVL written is not equal to total_balance `of all BondedPools." + ); + + // calculate the sum of `total_balance` of all `PoolMember` as the upper bound for the + // `TotalValueLocked`. + let total_balance_members: BalanceOf = PoolMembers::::iter() + .map(|(_, mut member)| member.total_balance()) + .reduce(|acc, total_balance| acc + total_balance) + .unwrap_or_default(); + + ensure!( + TotalValueLocked::::get() <= total_balance_members, + "TVL is greater than the balance of all PoolMembers." + ); ensure!( Pallet::::on_chain_storage_version() >= 6, From 5c1bb43f39ba0cbace2aa56ff88ce67680d47874 Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Sun, 3 Sep 2023 19:41:00 +0200 Subject: [PATCH 09/43] fixed migration --- .../frame/nomination-pools/src/migration.rs | 30 +++++++++++-------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index 4e21013b20f3..99e57f7d187f 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -724,8 +724,23 @@ pub mod v5 { Ok(()) } } +} + +/// This migration accumulates and initializes the [`TotalValueLocked`] for all pools. +pub mod v6 { + use super::*; - /// This migration accumulates and initializes the [`TotalValueLocked`] for all pools. + /// [`VersionUncheckedMigrateV5ToV6`] wrapped in a + /// [`frame_support::migrations::VersionedMigration`], ensuring the migration is only + /// performed when on-chain version is 5. + #[cfg(feature = "experimental")] + pub type VersionCheckedMigrateV5ToV6 = frame_support::migrations::VersionedMigration< + 5, + 6, + VersionUncheckedMigrateV5ToV6, + crate::pallet::Pallet, + ::DbWeight, + >; pub struct VersionUncheckedMigrateV5ToV6(sp_std::marker::PhantomData); impl VersionUncheckedMigrateV5ToV6 { fn calculate_tvl_by_total_stake() -> BalanceOf { @@ -740,6 +755,7 @@ pub mod v5 { .unwrap_or_default() } } + impl OnRuntimeUpgrade for VersionUncheckedMigrateV5ToV6 { fn on_runtime_upgrade() -> Weight { let migrated = BondedPools::::count(); @@ -830,16 +846,4 @@ pub mod v5 { Ok(()) } } - - /// [`VersionUncheckedMigrateV5ToV6`] wrapped in a - /// [`frame_support::migrations::VersionedRuntimeUpgrade`], ensuring the migration is only - /// performed when on-chain version is 5. - #[cfg(feature = "experimental")] - pub type VersionCheckedMigrateV5ToV6 = frame_support::migrations::VersionedRuntimeUpgrade< - 5, - 6, - VersionUncheckedMigrateV5ToV6, - crate::pallet::Pallet, - ::DbWeight, - >; } From 68527e91368426f43cde7be8dff9cde4c01d5148 Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Sun, 3 Sep 2023 19:48:28 +0200 Subject: [PATCH 10/43] improved readability --- substrate/frame/nomination-pools/src/migration.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index 99e57f7d187f..d982ed85cabd 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -741,6 +741,7 @@ pub mod v6 { crate::pallet::Pallet, ::DbWeight, >; + pub struct VersionUncheckedMigrateV5ToV6(sp_std::marker::PhantomData); impl VersionUncheckedMigrateV5ToV6 { fn calculate_tvl_by_total_stake() -> BalanceOf { @@ -804,7 +805,7 @@ pub mod v6 { let tvl: BalanceOf = Self::calculate_tvl_by_total_stake(); ensure!( TotalValueLocked::::get() == tvl, - "TVL written is not equal to total_balance `of all BondedPools." + "TVL written is not equal to `Staking::total_stake` of all `BondedPools`." ); // calculate the sum of `total_balance` of all `PoolMember` as the upper bound for the @@ -813,7 +814,7 @@ pub mod v6 { .map(|(_, mut member)| member.total_balance()) .reduce(|acc, total_balance| acc + total_balance) .unwrap_or_default(); - +` ensure!( TotalValueLocked::::get() <= total_balance_members, "TVL is greater than the balance of all PoolMembers." From 2e55e65bfdba06a2f6bdb7f1f5475809570dfe14 Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Sun, 3 Sep 2023 21:20:46 +0200 Subject: [PATCH 11/43] comments improved --- substrate/frame/nomination-pools/Cargo.toml | 2 +- substrate/frame/nomination-pools/src/lib.rs | 8 ++++---- substrate/frame/nomination-pools/src/migration.rs | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/substrate/frame/nomination-pools/Cargo.toml b/substrate/frame/nomination-pools/Cargo.toml index d673edd8536b..a2f675880144 100644 --- a/substrate/frame/nomination-pools/Cargo.toml +++ b/substrate/frame/nomination-pools/Cargo.toml @@ -36,7 +36,7 @@ sp-tracing = { path = "../../primitives/tracing" } [features] default = ["std"] -# Enable `VersionedRuntimeUpgrade` for the migrations that is currently still experimental. +# Enable `VersionedMigration` for the migrations that is currently still experimental. experimental = [ "frame-support/experimental" ] diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 8452a353cff7..d6ddeceeea74 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -992,7 +992,6 @@ impl BondedPool { } /// Issue points to [`Self`] for `new_funds`. - /// Increase the [`TotalValueLocked`] by `new_funds`. fn issue(&mut self, new_funds: BalanceOf) -> BalanceOf { let points_to_issue = self.balance_to_point(new_funds); self.points = self.points.saturating_add(points_to_issue); @@ -1211,7 +1210,8 @@ impl BondedPool { Ok(()) } - /// Bond exactly `amount` from `who`'s funds into this pool. + /// Bond exactly `amount` from `who`'s funds into this pool. Increases the [`TotalValueLocked`] + /// by `amount`. /// /// If the bond is [`BondType::Create`], [`Staking::bond`] is called, and `who` is allowed to be /// killed. Otherwise, [`Staking::bond_extra`] is called and `who` cannot be killed. @@ -1469,8 +1469,8 @@ impl UnbondPool { new_points } - /// Dissolve some points from the unbonding pool, reducing the balance of the pool. - /// This is the opposite of `issue`. + /// Dissolve some points from the unbonding pool, reducing the balance of the pool + /// proportionally. This is the opposite of `issue`. /// /// Returns the actual amount of `Balance` that was removed from the pool. fn dissolve(&mut self, points: BalanceOf) -> BalanceOf { diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index d982ed85cabd..7da6b9961a4f 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -814,7 +814,7 @@ pub mod v6 { .map(|(_, mut member)| member.total_balance()) .reduce(|acc, total_balance| acc + total_balance) .unwrap_or_default(); -` + ensure!( TotalValueLocked::::get() <= total_balance_members, "TVL is greater than the balance of all PoolMembers." From babe2ed9c9e9e2ea665240404a27ea489cafc2b4 Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Mon, 4 Sep 2023 00:30:03 +0200 Subject: [PATCH 12/43] keep events in the old order. --- substrate/frame/nomination-pools/src/lib.rs | 29 +++++++++++---------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index d6ddeceeea74..b151df097124 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -3371,20 +3371,21 @@ impl sp_staking::OnStakingUpdate> for Pall TotalValueLocked::::mutate(|tvl| { tvl.saturating_reduce(total_slashed); }); - Self::deposit_event(Event::::PoolSlashed { pool_id, balance: slashed_bonded }); - let Some(mut sub_pools) = SubPoolsStorage::::get(pool_id) else { return }; - // set the reduced balance for each of the [`SubPools`] - slashed_unlocking.iter().for_each(|(era, slashed_balance)| { - if let Some(pool) = sub_pools.with_era.get_mut(era) { - pool.balance = *slashed_balance; - Self::deposit_event(Event::::UnbondingPoolSlashed { - era: *era, - pool_id, - balance: *slashed_balance, - }); - } - }); - SubPoolsStorage::::insert(pool_id, sub_pools); + if let Some(mut sub_pools) = SubPoolsStorage::::get(pool_id) { + // set the reduced balance for each of the [`SubPools`] + slashed_unlocking.iter().for_each(|(era, slashed_balance)| { + if let Some(pool) = sub_pools.with_era.get_mut(era) { + pool.balance = *slashed_balance; + Self::deposit_event(Event::::UnbondingPoolSlashed { + era: *era, + pool_id, + balance: *slashed_balance, + }); + } + }); + SubPoolsStorage::::insert(pool_id, sub_pools); + } + Self::deposit_event(Event::::PoolSlashed { pool_id, balance: slashed_bonded }); } } From 2e2dee84b4542d6d84facbee05614b65aef744ea Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Fri, 8 Sep 2023 16:14:10 +0200 Subject: [PATCH 13/43] revert useless conversion --- substrate/frame/nomination-pools/src/lib.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index b151df097124..9363b22fda15 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -2831,12 +2831,9 @@ impl Pallet { }, (false, false) => { // Equivalent to (current_points / current_balance) * new_funds - balance( - u256(current_points) - .saturating_mul(u256(new_funds)) - // We check for zero above - .div(u256(current_balance)), - ) + balance(u256(current_points).saturating_mul(u256(new_funds))) + // We check for zero above + .div(current_balance) }, } } From 1daa2e1712c123dc17e0294ea820de3dcc66937f Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Fri, 8 Sep 2023 16:29:02 +0200 Subject: [PATCH 14/43] remove useless linking brackets --- substrate/frame/nomination-pools/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 9363b22fda15..cdef124a7a24 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -554,8 +554,8 @@ impl PoolMember { let unbonding_balance = self.unbonding_eras.iter().fold( BalanceOf::::zero(), |accumulator, (era, unlocked_points)| { - // if the [`SubPools::with_era`] has already been merged into the - // [`SubPools::no_era`] use this pool instead. + // if the `SubPools::with_era` has already been merged into the + // `SubPools::no_era` use this pool instead. let era_pool = sub_pools.with_era.get(era).unwrap_or(&sub_pools.no_era); accumulator.saturating_add(era_pool.point_to_balance(*unlocked_points)) }, @@ -3363,14 +3363,14 @@ impl sp_staking::OnStakingUpdate> for Pall total_slashed: BalanceOf, ) { let Some(pool_id) = ReversePoolIdLookup::::get(pool_account).defensive() else { return }; - // As the slashed account belongs to a [`BondedPool`] the [`TotalValueLocked`] decreases and + // As the slashed account belongs to a `BondedPool` the `TotalValueLocked` decreases and // an event is emitted. TotalValueLocked::::mutate(|tvl| { tvl.saturating_reduce(total_slashed); }); if let Some(mut sub_pools) = SubPoolsStorage::::get(pool_id) { - // set the reduced balance for each of the [`SubPools`] + // set the reduced balance for each of the `SubPools` slashed_unlocking.iter().for_each(|(era, slashed_balance)| { if let Some(pool) = sub_pools.with_era.get_mut(era) { pool.balance = *slashed_balance; From cbccde82cc0cf5232a247691c4984d2d9eabe0c7 Mon Sep 17 00:00:00 2001 From: Piet <75956460+PieWol@users.noreply.github.com> Date: Fri, 8 Sep 2023 17:08:56 +0200 Subject: [PATCH 15/43] formatting Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- substrate/frame/nomination-pools/src/tests.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/substrate/frame/nomination-pools/src/tests.rs b/substrate/frame/nomination-pools/src/tests.rs index 0b14a55bf97a..a4d4244360be 100644 --- a/substrate/frame/nomination-pools/src/tests.rs +++ b/substrate/frame/nomination-pools/src/tests.rs @@ -6602,7 +6602,6 @@ mod commission { } } mod slash { - use super::*; #[test] From 2b9066532eb053a4bbc6f426c628d224a84a92d2 Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Fri, 8 Sep 2023 17:17:17 +0200 Subject: [PATCH 16/43] fix unwanted defensive error spam --- substrate/frame/nomination-pools/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index cdef124a7a24..200e8eff8f37 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -3362,7 +3362,7 @@ impl sp_staking::OnStakingUpdate> for Pall slashed_unlocking: &BTreeMap>, total_slashed: BalanceOf, ) { - let Some(pool_id) = ReversePoolIdLookup::::get(pool_account).defensive() else { return }; + let Some(pool_id) = ReversePoolIdLookup::::get(pool_account) else { return }; // As the slashed account belongs to a `BondedPool` the `TotalValueLocked` decreases and // an event is emitted. TotalValueLocked::::mutate(|tvl| { From 28aa4e94ea38a0647a97e4a77a61b08f02cf433e Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Fri, 8 Sep 2023 19:16:19 +0200 Subject: [PATCH 17/43] log defensive error --- substrate/frame/nomination-pools/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 200e8eff8f37..a8ca46e7b9c4 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -3372,7 +3372,7 @@ impl sp_staking::OnStakingUpdate> for Pall if let Some(mut sub_pools) = SubPoolsStorage::::get(pool_id) { // set the reduced balance for each of the `SubPools` slashed_unlocking.iter().for_each(|(era, slashed_balance)| { - if let Some(pool) = sub_pools.with_era.get_mut(era) { + if let Some(pool) = sub_pools.with_era.get_mut(era).defensive() { pool.balance = *slashed_balance; Self::deposit_event(Event::::UnbondingPoolSlashed { era: *era, From 34a032ae6f26ca8cf0634f1b403dde31adca7445 Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Mon, 11 Sep 2023 10:22:50 +0200 Subject: [PATCH 18/43] remove unwanted upgrade checks --- .../frame/nomination-pools/src/migration.rs | 37 ------------------- 1 file changed, 37 deletions(-) diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index 7da6b9961a4f..19ede2d8fe41 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -777,24 +777,6 @@ pub mod v6 { #[cfg(feature = "try-runtime")] fn pre_upgrade() -> Result, TryRuntimeError> { - ensure!( - PoolMembers::::iter_keys().count() == PoolMembers::::iter_values().count(), - "There are undecodable PoolMembers in storage. This migration will not fix that." - ); - ensure!( - BondedPools::::iter_keys().count() == BondedPools::::iter_values().count(), - "There are undecodable BondedPools in storage. This migration will not fix that." - ); - ensure!( - SubPoolsStorage::::iter_keys().count() == - SubPoolsStorage::::iter_values().count(), - "There are undecodable SubPools in storage. This migration will not fix that." - ); - ensure!( - Metadata::::iter_keys().count() == Metadata::::iter_values().count(), - "There are undecodable Metadata in storage. This migration will not fix that." - ); - Ok(Vec::new()) } @@ -825,25 +807,6 @@ pub mod v6 { "nomination-pools::migration::v6: wrong storage version" ); - // These should not have been touched - just in case. - ensure!( - PoolMembers::::iter_keys().count() == PoolMembers::::iter_values().count(), - "There are undecodable PoolMembers in storage." - ); - ensure!( - BondedPools::::iter_keys().count() == BondedPools::::iter_values().count(), - "There are undecodable BondedPools in storage." - ); - ensure!( - SubPoolsStorage::::iter_keys().count() == - SubPoolsStorage::::iter_values().count(), - "There are undecodable SubPools in storage." - ); - ensure!( - Metadata::::iter_keys().count() == Metadata::::iter_values().count(), - "There are undecodable Metadata in storage." - ); - Ok(()) } } From 3672950124566c2e3d9ddc4cce2a81365288cffd Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Mon, 11 Sep 2023 11:42:35 +0200 Subject: [PATCH 19/43] defensively lookup SubPools --- substrate/frame/nomination-pools/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index a8ca46e7b9c4..a4f349b272a0 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -3382,6 +3382,8 @@ impl sp_staking::OnStakingUpdate> for Pall } }); SubPoolsStorage::::insert(pool_id, sub_pools); + } else if !slashed_unlocking.is_empty() { + defensive!("Expected SubPools were not found"); } Self::deposit_event(Event::::PoolSlashed { pool_id, balance: slashed_bonded }); } From deb53ff7abcfc616e8133c698dfcb63c4304321b Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Mon, 11 Sep 2023 12:32:01 +0200 Subject: [PATCH 20/43] defensive TVL reduce --- substrate/frame/nomination-pools/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index a4f349b272a0..4a34c0f56c81 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -3366,7 +3366,7 @@ impl sp_staking::OnStakingUpdate> for Pall // As the slashed account belongs to a `BondedPool` the `TotalValueLocked` decreases and // an event is emitted. TotalValueLocked::::mutate(|tvl| { - tvl.saturating_reduce(total_slashed); + tvl.defensive_saturating_reduce(total_slashed); }); if let Some(mut sub_pools) = SubPoolsStorage::::get(pool_id) { From a05dc7b1e0d67b70a77d1d03989dfd2c277823fa Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Mon, 11 Sep 2023 13:19:02 +0200 Subject: [PATCH 21/43] integrate try_state iterations --- substrate/frame/nomination-pools/src/lib.rs | 40 ++++++++----------- .../frame/nomination-pools/src/migration.rs | 2 +- 2 files changed, 17 insertions(+), 25 deletions(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 4a34c0f56c81..9e3d94bf5cc6 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -538,7 +538,7 @@ impl PoolMember { /// Total balance of the member, both active and unbonding. /// Doesn't mutate state. #[cfg(any(feature = "try-runtime", feature = "fuzzing", test, debug_assertions))] - fn total_balance(&mut self) -> BalanceOf { + fn total_balance(&self) -> BalanceOf { let pool = match BondedPool::::get(self.pool_id).defensive() { Some(pool) => pool, None => return Zero::zero(), @@ -3170,6 +3170,7 @@ impl Pallet { let mut pools_members = BTreeMap::::new(); let mut pools_members_pending_rewards = BTreeMap::>::new(); let mut all_members = 0u32; + let mut total_balance_members = Default::default(); PoolMembers::::iter().try_for_each(|(_, d)| -> Result<(), TryRuntimeError> { let bonded_pool = BondedPools::::get(d.pool_id).unwrap(); ensure!(!d.total_points().is_zero(), "No member should have zero points"); @@ -3185,6 +3186,7 @@ impl Pallet { let pending_rewards = d.pending_rewards(current_rc).unwrap(); *pools_members_pending_rewards.entry(d.pool_id).or_default() += pending_rewards; } // else this pool has been heavily slashed and cannot have any rewards anymore. + total_balance_members += d.total_balance(); Ok(()) })?; @@ -3209,6 +3211,7 @@ impl Pallet { Ok(()) })?; + let mut expected_tvl: BalanceOf = Default::default(); BondedPools::::iter().try_for_each(|(id, inner)| -> Result<(), TryRuntimeError> { let bonded_pool = BondedPool { id, inner }; ensure!( @@ -3230,30 +3233,9 @@ impl Pallet { pool is being destroyed and the depositor is the last member", ); - let expected_tvl: BalanceOf = BondedPools::::iter() - .map(|(id, inner)| { - T::Staking::total_stake( - &BondedPool { id, inner: inner.clone() }.bonded_account(), - ) - .unwrap_or_default() - }) - .reduce(|acc, total_balance| acc + total_balance) - .unwrap_or_default(); - - ensure!( - TotalValueLocked::::get() == expected_tvl, - "TVL deviates from the actual sum of funds of all Pools." - ); + expected_tvl += + T::Staking::total_stake(&bonded_pool.bonded_account()).unwrap_or_default(); - let total_balance_members: BalanceOf = PoolMembers::::iter() - .map(|(_, mut member)| member.total_balance()) - .reduce(|acc, total_balance| acc + total_balance) - .unwrap_or_default(); - - ensure!( - TotalValueLocked::::get() <= total_balance_members, - "TVL must be equal to or less than the total balance of all PoolMembers." - ); Ok(()) })?; @@ -3262,6 +3244,16 @@ impl Pallet { Error::::MaxPoolMembers ); + ensure!( + TotalValueLocked::::get() == expected_tvl, + "TVL deviates from the actual sum of funds of all Pools." + ); + + ensure!( + TotalValueLocked::::get() <= total_balance_members, + "TVL must be equal to or less than the total balance of all PoolMembers." + ); + if level <= 1 { return Ok(()) } diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index 19ede2d8fe41..d6e86689e609 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -793,7 +793,7 @@ pub mod v6 { // calculate the sum of `total_balance` of all `PoolMember` as the upper bound for the // `TotalValueLocked`. let total_balance_members: BalanceOf = PoolMembers::::iter() - .map(|(_, mut member)| member.total_balance()) + .map(|(_, member)| member.total_balance()) .reduce(|acc, total_balance| acc + total_balance) .unwrap_or_default(); From 5d079214b1990fac3d61ebeedc3c71817e016a07 Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Mon, 11 Sep 2023 13:19:08 +0200 Subject: [PATCH 22/43] fix test --- substrate/frame/nomination-pools/src/tests.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/substrate/frame/nomination-pools/src/tests.rs b/substrate/frame/nomination-pools/src/tests.rs index a4d4244360be..61491d4d3dfa 100644 --- a/substrate/frame/nomination-pools/src/tests.rs +++ b/substrate/frame/nomination-pools/src/tests.rs @@ -3281,7 +3281,7 @@ mod withdraw_unbonded { // Given // current bond is 600, we slash it all to 300. - StakingMock::set_bonded_balance(default_bonded_account(), 300); + StakingMock::slash_to(1, 300); Balances::make_free_balance_be(&default_bonded_account(), 300); assert_eq!(StakingMock::total_stake(&default_bonded_account()), Ok(300)); @@ -3301,6 +3301,7 @@ mod withdraw_unbonded { Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true }, Event::Bonded { member: 40, pool_id: 1, bonded: 40, joined: true }, Event::Bonded { member: 550, pool_id: 1, bonded: 550, joined: true }, + Event::PoolSlashed { pool_id: 1, balance: 300 }, Event::Unbonded { member: 40, pool_id: 1, balance: 20, points: 20, era: 3 }, Event::Unbonded { member: 550, From aadbddcd997f99678a2fab162646a0ae23be4f23 Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Mon, 11 Sep 2023 13:28:54 +0200 Subject: [PATCH 23/43] no defensive in off-chain code --- substrate/frame/nomination-pools/src/lib.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 9e3d94bf5cc6..dce3cd4d5cb0 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -539,11 +539,7 @@ impl PoolMember { /// Doesn't mutate state. #[cfg(any(feature = "try-runtime", feature = "fuzzing", test, debug_assertions))] fn total_balance(&self) -> BalanceOf { - let pool = match BondedPool::::get(self.pool_id).defensive() { - Some(pool) => pool, - None => return Zero::zero(), - }; - + let pool = BondedPool::::get(self.pool_id).unwrap(); let active_balance = pool.points_to_balance(self.active_points()); let sub_pools = match SubPoolsStorage::::get(self.pool_id) { @@ -557,11 +553,11 @@ impl PoolMember { // if the `SubPools::with_era` has already been merged into the // `SubPools::no_era` use this pool instead. let era_pool = sub_pools.with_era.get(era).unwrap_or(&sub_pools.no_era); - accumulator.saturating_add(era_pool.point_to_balance(*unlocked_points)) + accumulator + (era_pool.point_to_balance(*unlocked_points)) }, ); - active_balance.saturating_add(unbonding_balance) + active_balance + unbonding_balance } /// Total points of this member, both active and unbonding. From fbf65bd935377cbdafc71b533fabc707591311a0 Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Mon, 11 Sep 2023 13:44:58 +0200 Subject: [PATCH 24/43] fix TVL doc --- substrate/frame/nomination-pools/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index dce3cd4d5cb0..9ec97b08e6cd 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -1626,9 +1626,9 @@ pub mod pallet { /// The sum of funds across all pools. /// - /// This might be higher but never lower than the actual sum of the currently unlocking and - /// bonded funds as this is only decreased if a user withdraws unlocked funds or a slash - /// happened. + /// This might be lower but never higher than the sum of `total_balance` of all [`PoolMembers`] + /// because calling `pool_withdraw_unbonded` might decrease the total stake of the pool's + /// `bonded_account` without adjusting the pallet-internal `UnbondingPool`'s. #[pallet::storage] pub type TotalValueLocked = StorageValue<_, BalanceOf, ValueQuery>; From 4b076a262fef3f07894c3eea5bacaf07160094c4 Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Mon, 11 Sep 2023 13:50:41 +0200 Subject: [PATCH 25/43] add defensive to TVL adjustment --- substrate/frame/nomination-pools/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 9ec97b08e6cd..0977531c0a84 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -1273,8 +1273,8 @@ impl BondedPool { let prev_total = T::Staking::total_stake(&bonded_account.clone()).unwrap_or_default(); let outcome = T::Staking::withdraw_unbonded(bonded_account.clone(), num_slashing_spans); - let diff = - prev_total.saturating_sub(T::Staking::total_stake(&bonded_account).unwrap_or_default()); + let diff = prev_total + .defensive_saturating_sub(T::Staking::total_stake(&bonded_account).unwrap_or_default()); TotalValueLocked::::mutate(|tvl| { tvl.saturating_reduce(diff); }); From f53529873b3bb15b468cc000a2a12302a603e68f Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Thu, 14 Sep 2023 01:01:50 +0200 Subject: [PATCH 26/43] fix test slash --- substrate/frame/nomination-pools/src/tests.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/substrate/frame/nomination-pools/src/tests.rs b/substrate/frame/nomination-pools/src/tests.rs index 61491d4d3dfa..dae6e2e384f0 100644 --- a/substrate/frame/nomination-pools/src/tests.rs +++ b/substrate/frame/nomination-pools/src/tests.rs @@ -520,7 +520,7 @@ mod join { // Given // The bonded balance is slashed in half - StakingMock::set_bonded_balance(Pools::create_bonded_account(1), 6); + StakingMock::slash_to(1, 6); // And Balances::make_free_balance_be(&12, ExistentialDeposit::get() + 12); @@ -532,9 +532,12 @@ mod join { // Then assert_eq!( pool_events_since_last_call(), - vec![Event::Bonded { member: 12, pool_id: 1, bonded: 12, joined: true }] + vec![ + Event::PoolSlashed { pool_id: 1, balance: 6 }, + Event::Bonded { member: 12, pool_id: 1, bonded: 12, joined: true } + ] ); - assert_eq!(TotalValueLocked::::get(), 24); + assert_eq!(TotalValueLocked::::get(), 18); assert_eq!( PoolMembers::::get(12).unwrap(), From 26f2baaf3e731a967fead9f38308d03500df5757 Mon Sep 17 00:00:00 2001 From: Piet <75956460+PieWol@users.noreply.github.com> Date: Thu, 14 Sep 2023 01:05:21 +0200 Subject: [PATCH 27/43] Update mock.rs error Co-authored-by: Ankan <10196091+Ank4n@users.noreply.github.com> --- substrate/frame/nomination-pools/src/mock.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/nomination-pools/src/mock.rs b/substrate/frame/nomination-pools/src/mock.rs index 927208bf90de..2e4c444003a3 100644 --- a/substrate/frame/nomination-pools/src/mock.rs +++ b/substrate/frame/nomination-pools/src/mock.rs @@ -133,7 +133,7 @@ impl sp_staking::StakingInterface for StakingMock { fn withdraw_unbonded(who: Self::AccountId, _: u32) -> Result { let mut unbonding_map = UnbondingBalanceMap::get(); - let staker_map = unbonding_map.get_mut(&who).ok_or("not a staker")?; + let staker_map = unbonding_map.get_mut(&who).ok_or("Nothing to unbond")?; let current_era = Self::current_era(); staker_map.retain(|(unlocking_at, _amount)| *unlocking_at > current_era); From b4e9966db257a19ccb8c641facb247e55d0a79b5 Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Thu, 14 Sep 2023 12:25:45 +0200 Subject: [PATCH 28/43] showcase test for tvl diff --- substrate/frame/nomination-pools/src/tests.rs | 37 ++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/substrate/frame/nomination-pools/src/tests.rs b/substrate/frame/nomination-pools/src/tests.rs index dae6e2e384f0..0baf39ebe90c 100644 --- a/substrate/frame/nomination-pools/src/tests.rs +++ b/substrate/frame/nomination-pools/src/tests.rs @@ -3264,12 +3264,47 @@ mod pool_withdraw_unbonded { CurrentEra::set(StakingMock::current_era() + StakingMock::bonding_duration() + 1); assert_ok!(Pools::pool_withdraw_unbonded(RuntimeOrigin::signed(10), 1, 0)); - // Then there unbonding balance is no longer locked + // Then their unbonding balance is no longer locked assert_eq!(StakingMock::active_stake(&default_bonded_account()), Ok(15)); assert_eq!(StakingMock::total_stake(&default_bonded_account()), Ok(15)); assert_eq!(Balances::free_balance(&default_bonded_account()), 20); }); } + #[test] + fn pool_withdraw_unbonded_creates_tvl_diff() { + ExtBuilder::default().add_members(vec![(20, 10)]).build_and_execute(|| { + // Given 10 unbond'ed directly against the pool account + assert_ok!(Pools::unbond(RuntimeOrigin::signed(20), 20, 5)); + + assert_eq!(StakingMock::active_stake(&default_bonded_account()), Ok(15)); + assert_eq!(StakingMock::total_stake(&default_bonded_account()), Ok(20)); + assert_eq!(Balances::free_balance(&default_bonded_account()), 20); + assert_eq!(TotalValueLocked::::get(), 20); + + // When + CurrentEra::set(StakingMock::current_era() + StakingMock::bonding_duration() + 1); + assert_ok!(Pools::pool_withdraw_unbonded(RuntimeOrigin::signed(10), 1, 0)); + assert_eq!(TotalValueLocked::::get(), 15); + + let member_balance = PoolMembers::::iter() + .map(|(_, member)| member.total_balance()) + .reduce(|acc, total_balance| acc + total_balance) + .unwrap_or_default(); + + // Then their unbonding balance is no longer locked + assert_eq!(StakingMock::active_stake(&default_bonded_account()), Ok(15)); + assert_eq!(StakingMock::total_stake(&default_bonded_account()), Ok(15)); + assert_eq!(Balances::free_balance(&default_bonded_account()), 20); + + // The difference between TVL and member_balance is exactly the difference between + // `total_stake` and the `free_balance`. + // This relation is not guaranteed in the wild as arbitrary transfers towards + // `free_balance` can be made to the pool that are not accounted for. + let non_locked_balance = Balances::free_balance(&default_bonded_account()) - + StakingMock::total_stake(&default_bonded_account()).unwrap(); + assert_eq!(member_balance, TotalValueLocked::::get() + non_locked_balance); + }); + } } mod withdraw_unbonded { From 389fdf5d0d1baec30fe0d22afee9781263089712 Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Sat, 16 Sep 2023 11:50:17 +0200 Subject: [PATCH 29/43] add migration warning --- substrate/frame/nomination-pools/src/migration.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index d6e86689e609..276eec9e5a4f 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -727,6 +727,9 @@ pub mod v5 { } /// This migration accumulates and initializes the [`TotalValueLocked`] for all pools. +/// +/// WARNING: This migration works under the assumption that the [`BondedPools`] cannot be inflated +/// arbitrarily. Otherwise this migration could fail due to too high weight. pub mod v6 { use super::*; From 9f9f821535e73a2c0ab042dff224373e66191a0b Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Fri, 29 Sep 2023 15:11:59 +0200 Subject: [PATCH 30/43] reintroduce test --- substrate/frame/nomination-pools/src/mock.rs | 2 +- substrate/frame/nomination-pools/src/tests.rs | 170 +++++++++++++++++- 2 files changed, 162 insertions(+), 10 deletions(-) diff --git a/substrate/frame/nomination-pools/src/mock.rs b/substrate/frame/nomination-pools/src/mock.rs index 2e4c444003a3..3931301708e8 100644 --- a/substrate/frame/nomination-pools/src/mock.rs +++ b/substrate/frame/nomination-pools/src/mock.rs @@ -66,7 +66,7 @@ impl StakingMock { /// /// Does not modify any [`SubPools::with_era`] of the pool as [`Default::default`] is passed for /// `slashed_unlocking`. - pub fn slash_to(pool_id: PoolId, amount: Balance) { + pub fn slash_by(pool_id: PoolId, amount: Balance) { let acc = Pools::create_bonded_account(pool_id); let bonded = BondedBalanceMap::get(); let pre_total = bonded.get(&acc).unwrap(); diff --git a/substrate/frame/nomination-pools/src/tests.rs b/substrate/frame/nomination-pools/src/tests.rs index 0baf39ebe90c..5a6482b7bbe9 100644 --- a/substrate/frame/nomination-pools/src/tests.rs +++ b/substrate/frame/nomination-pools/src/tests.rs @@ -221,7 +221,7 @@ mod bonded_pool { // slash half of the pool's balance. expected result of `fn api_points_to_balance` // to be 1/2 of the pool's balance. - StakingMock::slash_to(1, Pools::depositor_min_bond() / 2); + StakingMock::slash_by(1, Pools::depositor_min_bond() / 2); assert_eq!(Pallet::::api_points_to_balance(1, 10), 5); // if pool does not exist, points to balance ratio is 0. @@ -238,7 +238,7 @@ mod bonded_pool { // slash half of the pool's balance. expect result of `fn api_balance_to_points` // to be 2 * of the balance to add to the pool. - StakingMock::slash_to(1, Pools::depositor_min_bond() / 2); + StakingMock::slash_by(1, Pools::depositor_min_bond() / 2); assert_eq!(Pallet::::api_balance_to_points(1, 10), 20); // if pool does not exist, balance to points ratio is 0. @@ -520,7 +520,7 @@ mod join { // Given // The bonded balance is slashed in half - StakingMock::slash_to(1, 6); + StakingMock::slash_by(1, 6); // And Balances::make_free_balance_be(&12, ExistentialDeposit::get() + 12); @@ -2538,7 +2538,7 @@ mod unbond { .build_and_execute(|| { let ed = Balances::minimum_balance(); // Given a slash from 600 -> 500 - StakingMock::slash_to(1, 500); + StakingMock::slash_by(1, 500); // and unclaimed rewards of 600. Balances::make_free_balance_be(&default_reward_account(), ed + 600); @@ -3172,7 +3172,7 @@ mod unbond { assert_eq!(PoolMembers::::get(10).unwrap().unbonding_points(), 0); // slash the default pool - StakingMock::slash_to(1, 5); + StakingMock::slash_by(1, 5); // cannot unbond even 7, because the value of shares is now less. assert_noop!( @@ -3309,6 +3309,158 @@ mod pool_withdraw_unbonded { mod withdraw_unbonded { use super::*; + use sp_runtime::bounded_btree_map; + + #[test] + fn withdraw_unbonded_works_against_slashed_no_era_sub_pool() { + ExtBuilder::default() + .add_members(vec![(40, 40), (550, 550)]) + .build_and_execute(|| { + // reduce the noise a bit. + let _ = balances_events_since_last_call(); + + // Given + assert_eq!(StakingMock::bonding_duration(), 3); + assert_ok!(Pools::fully_unbond(RuntimeOrigin::signed(550), 550)); + assert_ok!(Pools::fully_unbond(RuntimeOrigin::signed(40), 40)); + assert_eq!(Balances::free_balance(&default_bonded_account()), 600); + + let mut current_era = 1; + CurrentEra::set(current_era); + + let mut sub_pools = SubPoolsStorage::::get(1).unwrap(); + let unbond_pool = sub_pools.with_era.get_mut(&3).unwrap(); + // Sanity check + assert_eq!(*unbond_pool, UnbondPool { points: 550 + 40, balance: 550 + 40 }); + + // Simulate a slash to the pool with_era(current_era), decreasing the balance by + // half + { + unbond_pool.balance /= 2; // 295 + SubPoolsStorage::::insert(1, sub_pools); + // Update the equivalent of the unbonding chunks for the `StakingMock` + let mut x = UnbondingBalanceMap::get(); + *x.get_mut(&default_bonded_account()).unwrap() /= 5; + UnbondingBalanceMap::set(&x); + Balances::make_free_balance_be( + &default_bonded_account(), + Balances::free_balance(&default_bonded_account()) / 2, // 300 + ); + StakingMock::set_bonded_balance( + default_bonded_account(), + StakingMock::active_stake(&default_bonded_account()).unwrap() / 2, + ); + }; + + // Advance the current_era to ensure all `with_era` pools will be merged into + // `no_era` pool + current_era += TotalUnbondingPools::::get(); + CurrentEra::set(current_era); + + // Simulate some other call to unbond that would merge `with_era` pools into + // `no_era` + let sub_pools = + SubPoolsStorage::::get(1).unwrap().maybe_merge_pools(current_era); + SubPoolsStorage::::insert(1, sub_pools); + + assert_eq!( + SubPoolsStorage::::get(1).unwrap(), + SubPools { + no_era: UnbondPool { points: 550 + 40, balance: 275 + 20 }, + with_era: Default::default() + } + ); + assert_eq!( + pool_events_since_last_call(), + vec![ + Event::Created { depositor: 10, pool_id: 1 }, + Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true }, + Event::Bonded { member: 40, pool_id: 1, bonded: 40, joined: true }, + Event::Bonded { member: 550, pool_id: 1, bonded: 550, joined: true }, + Event::Unbonded { + member: 550, + pool_id: 1, + points: 550, + balance: 550, + era: 3 + }, + Event::Unbonded { member: 40, pool_id: 1, points: 40, balance: 40, era: 3 }, + ] + ); + assert_eq!( + balances_events_since_last_call(), + vec![BEvent::BalanceSet { who: default_bonded_account(), free: 300 }] + ); + + // When + assert_ok!(Pools::withdraw_unbonded(RuntimeOrigin::signed(550), 550, 0)); + + // Then + assert_eq!( + SubPoolsStorage::::get(1).unwrap().no_era, + UnbondPool { points: 40, balance: 20 } + ); + assert_eq!( + pool_events_since_last_call(), + vec![ + Event::Withdrawn { member: 550, pool_id: 1, balance: 275, points: 550 }, + Event::MemberRemoved { pool_id: 1, member: 550 } + ] + ); + assert_eq!( + balances_events_since_last_call(), + vec![BEvent::Transfer { from: default_bonded_account(), to: 550, amount: 275 }] + ); + + // When + assert_ok!(Pools::withdraw_unbonded(RuntimeOrigin::signed(40), 40, 0)); + + // Then + assert_eq!( + SubPoolsStorage::::get(1).unwrap().no_era, + UnbondPool { points: 0, balance: 0 } + ); + assert!(!PoolMembers::::contains_key(40)); + assert_eq!( + pool_events_since_last_call(), + vec![ + Event::Withdrawn { member: 40, pool_id: 1, balance: 20, points: 40 }, + Event::MemberRemoved { pool_id: 1, member: 40 } + ] + ); + assert_eq!( + balances_events_since_last_call(), + vec![BEvent::Transfer { from: default_bonded_account(), to: 40, amount: 20 }] + ); + + // now, finally, the depositor can take out its share. + unsafe_set_state(1, PoolState::Destroying); + assert_ok!(fully_unbond_permissioned(10)); + + current_era += 3; + CurrentEra::set(current_era); + + // when + assert_ok!(Pools::withdraw_unbonded(RuntimeOrigin::signed(10), 10, 0)); + assert_eq!( + pool_events_since_last_call(), + vec![ + Event::Unbonded { member: 10, pool_id: 1, balance: 5, points: 5, era: 9 }, + Event::Withdrawn { member: 10, pool_id: 1, balance: 5, points: 5 }, + Event::MemberRemoved { pool_id: 1, member: 10 }, + Event::Destroyed { pool_id: 1 } + ] + ); + assert!(!Metadata::::contains_key(1)); + assert_eq!( + balances_events_since_last_call(), + vec![ + BEvent::Transfer { from: default_bonded_account(), to: 10, amount: 5 }, + BEvent::Transfer { from: default_reward_account(), to: 10, amount: 5 } + ] + ); + }); + } #[test] fn withdraw_unbonded_works_against_slashed_with_era_sub_pools() { @@ -3319,7 +3471,7 @@ mod withdraw_unbonded { // Given // current bond is 600, we slash it all to 300. - StakingMock::slash_to(1, 300); + StakingMock::slash_by(1, 300); Balances::make_free_balance_be(&default_bonded_account(), 300); assert_eq!(StakingMock::total_stake(&default_bonded_account()), Ok(300)); @@ -4470,7 +4622,7 @@ mod set_state { unsafe_set_state(1, PoolState::Open); // slash the pool to the point that `max_points_to_balance` ratio is // surpassed. Making this pool destroyable by anyone. - StakingMock::slash_to(1, 10); + StakingMock::slash_by(1, 10); // When assert_ok!(Pools::set_state(RuntimeOrigin::signed(11), 1, PoolState::Destroying)); @@ -5126,7 +5278,7 @@ mod reward_counter_precision { ); // slash this pool by 99% of that. - StakingMock::slash_to(1, pool_bond * 99 / 100); + StakingMock::slash_by(1, pool_bond * 99 / 100); // some whale now joins with the other half ot the total issuance. This will trigger an // overflow. This test is actually a bit too lenient because all the reward counters are @@ -6683,7 +6835,7 @@ mod slash { // Given // The bonded balance is slashed in half - StakingMock::slash_to(1, 6); + StakingMock::slash_by(1, 6); // And Balances::make_free_balance_be(&12, ExistentialDeposit::get() + 12); From 5764dfce15ed8e0741584425267c89821cc3fb29 Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Fri, 29 Sep 2023 15:31:59 +0200 Subject: [PATCH 31/43] fix test --- substrate/frame/nomination-pools/src/tests.rs | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/substrate/frame/nomination-pools/src/tests.rs b/substrate/frame/nomination-pools/src/tests.rs index 5a6482b7bbe9..e08d56d71fa0 100644 --- a/substrate/frame/nomination-pools/src/tests.rs +++ b/substrate/frame/nomination-pools/src/tests.rs @@ -3332,24 +3332,33 @@ mod withdraw_unbonded { let unbond_pool = sub_pools.with_era.get_mut(&3).unwrap(); // Sanity check assert_eq!(*unbond_pool, UnbondPool { points: 550 + 40, balance: 550 + 40 }); + assert_eq!(TotalValueLocked::::get(), 600); // Simulate a slash to the pool with_era(current_era), decreasing the balance by // half { unbond_pool.balance /= 2; // 295 SubPoolsStorage::::insert(1, sub_pools); + + // Adjust the TVL for this non-api usage (direct sub-pool modification) + TotalValueLocked::::mutate(|x| *x -= 295); + // Update the equivalent of the unbonding chunks for the `StakingMock` let mut x = UnbondingBalanceMap::get(); - *x.get_mut(&default_bonded_account()).unwrap() /= 5; + x.get_mut(&default_bonded_account()) + .unwrap() + .get_mut(current_era as usize) + .unwrap() + .1 /= 2; UnbondingBalanceMap::set(&x); + Balances::make_free_balance_be( &default_bonded_account(), Balances::free_balance(&default_bonded_account()) / 2, // 300 ); - StakingMock::set_bonded_balance( - default_bonded_account(), - StakingMock::active_stake(&default_bonded_account()).unwrap() / 2, - ); + assert_eq!(StakingMock::active_stake(&default_bonded_account()).unwrap(), 10); + StakingMock::slash_by(1, 5); + assert_eq!(StakingMock::active_stake(&default_bonded_account()).unwrap(), 5); }; // Advance the current_era to ensure all `with_era` pools will be merged into @@ -3385,6 +3394,7 @@ mod withdraw_unbonded { era: 3 }, Event::Unbonded { member: 40, pool_id: 1, points: 40, balance: 40, era: 3 }, + Event::PoolSlashed { pool_id: 1, balance: 5 } ] ); assert_eq!( From b3fab33570bff35541b60b4917dc0bc872b3e858 Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Fri, 29 Sep 2023 15:50:46 +0200 Subject: [PATCH 32/43] remove experimental flag --- substrate/frame/nomination-pools/Cargo.toml | 4 ---- substrate/frame/nomination-pools/src/migration.rs | 1 - 2 files changed, 5 deletions(-) diff --git a/substrate/frame/nomination-pools/Cargo.toml b/substrate/frame/nomination-pools/Cargo.toml index a2f675880144..cc87976c76f1 100644 --- a/substrate/frame/nomination-pools/Cargo.toml +++ b/substrate/frame/nomination-pools/Cargo.toml @@ -36,10 +36,6 @@ sp-tracing = { path = "../../primitives/tracing" } [features] default = ["std"] -# Enable `VersionedMigration` for the migrations that is currently still experimental. -experimental = [ - "frame-support/experimental" -] fuzzing = ["pallet-balances", "sp-tracing"] std = [ "codec/std", diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index 276eec9e5a4f..dac69b315374 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -736,7 +736,6 @@ pub mod v6 { /// [`VersionUncheckedMigrateV5ToV6`] wrapped in a /// [`frame_support::migrations::VersionedMigration`], ensuring the migration is only /// performed when on-chain version is 5. - #[cfg(feature = "experimental")] pub type VersionCheckedMigrateV5ToV6 = frame_support::migrations::VersionedMigration< 5, 6, From 01bcdc5a8ef19a2404c40406929877e0f0f86db3 Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Fri, 29 Sep 2023 16:41:16 +0200 Subject: [PATCH 33/43] formatting and rust docs --- substrate/frame/nomination-pools/Cargo.toml | 4 ++-- substrate/frame/nomination-pools/src/lib.rs | 4 ++-- substrate/frame/nomination-pools/src/mock.rs | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/substrate/frame/nomination-pools/Cargo.toml b/substrate/frame/nomination-pools/Cargo.toml index cc87976c76f1..4923af0ab0c5 100644 --- a/substrate/frame/nomination-pools/Cargo.toml +++ b/substrate/frame/nomination-pools/Cargo.toml @@ -35,8 +35,8 @@ pallet-balances = { path = "../balances" } sp-tracing = { path = "../../primitives/tracing" } [features] -default = ["std"] -fuzzing = ["pallet-balances", "sp-tracing"] +default = [ "std" ] +fuzzing = [ "pallet-balances", "sp-tracing" ] std = [ "codec/std", "frame-support/std", diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 577aec8b1f5b..6cb4c6f0bf65 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -3337,11 +3337,11 @@ impl Pallet { } impl sp_staking::OnStakingUpdate> for Pallet { - /// Reduces the balances of the [`SubPools::with_era`], that belong to the pool involved in the + /// Reduces the balances of the [`SubPools`], that belong to the pool involved in the /// slash, to the amount that is defined in the `slashed_unlocking` field of /// [`sp_staking::OnStakingUpdate::on_slash`] /// - /// Emits [`Event::::PoolsSlashed`]. + /// Emits the `PoolsSlashed` event. fn on_slash( pool_account: &T::AccountId, // Bonded balance is always read directly from staking, therefore we don't need to update diff --git a/substrate/frame/nomination-pools/src/mock.rs b/substrate/frame/nomination-pools/src/mock.rs index 3931301708e8..954055e1158c 100644 --- a/substrate/frame/nomination-pools/src/mock.rs +++ b/substrate/frame/nomination-pools/src/mock.rs @@ -64,7 +64,7 @@ impl StakingMock { /// This reduces the bonded balance of a pool by `amount` and calls [`Pools::on_slash`] to /// enact changes in the nomination-pool pallet. /// - /// Does not modify any [`SubPools::with_era`] of the pool as [`Default::default`] is passed for + /// Does not modify any [`SubPools`] of the pool as [`Default::default`] is passed for /// `slashed_unlocking`. pub fn slash_by(pool_id: PoolId, amount: Balance) { let acc = Pools::create_bonded_account(pool_id); From a361d2a0df902f435a15d1890c7ea42889c62d82 Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Fri, 29 Sep 2023 17:51:38 +0200 Subject: [PATCH 34/43] westend migration added --- polkadot/runtime/westend/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index b1231a5d95fd..826ed356a0d1 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1506,6 +1506,7 @@ pub mod migrations { parachains_configuration::migration::v9::MigrateToV9, paras_registrar::migration::VersionCheckedMigrateToV1, pallet_referenda::migration::v1::MigrateV0ToV1, + pallet_nomination_pools::migration::v6::VersionCheckedMigrateV5ToV6, ); } From d68d93ea5f44d92a9232b443b62d657eff47dfeb Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Fri, 29 Sep 2023 21:09:29 +0200 Subject: [PATCH 35/43] formatting --- substrate/frame/nomination-pools/src/lib.rs | 1 - substrate/frame/nomination-pools/src/tests.rs | 8 ++++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 2cd1f0ec78ae..a032c2f8b162 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -2240,7 +2240,6 @@ pub mod pallet { // `transferrable_balance` is correct. let stash_killed = bonded_pool.withdraw_from_staking(num_slashing_spans)?; - // defensive-only: the depositor puts enough funds into the stash so that it will only // be destroyed when they are leaving. ensure!( diff --git a/substrate/frame/nomination-pools/src/tests.rs b/substrate/frame/nomination-pools/src/tests.rs index 408f33881aaf..7d79e1fc447b 100644 --- a/substrate/frame/nomination-pools/src/tests.rs +++ b/substrate/frame/nomination-pools/src/tests.rs @@ -2677,7 +2677,7 @@ mod unbond { let ed = Currency::minimum_balance(); // Given a slash from 600 -> 500 StakingMock::slash_by(1, 500); - + // and unclaimed rewards of 600. Currency::set_balance(&default_reward_account(), ed + 600); @@ -5003,7 +5003,7 @@ mod bond_extra { assert_eq!(Currency::free_balance(&10), 35); assert_eq!(Currency::free_balance(&20), 20); - assert_eq!(TotalValueLocked::::get(), 30); + assert_eq!(TotalValueLocked::::get(), 30); // when assert_ok!(Pools::bond_extra(RuntimeOrigin::signed(10), BondExtra::Rewards)); @@ -5012,7 +5012,7 @@ mod bond_extra { // then assert_eq!(Currency::free_balance(&10), 35); assert_eq!(TotalValueLocked::::get(), 31); - + // 10's share of the reward is 1/3, since they gave 10/30 of the total shares. assert_eq!(PoolMembers::::get(10).unwrap().points, 10 + 1); assert_eq!(BondedPools::::get(1).unwrap().points, 30 + 1); @@ -5023,7 +5023,7 @@ mod bond_extra { // then assert_eq!(Currency::free_balance(&20), 20); assert_eq!(TotalValueLocked::::get(), 33); - + // 20's share of the rewards is the other 2/3 of the rewards, since they have 20/30 of // the shares assert_eq!(PoolMembers::::get(20).unwrap().points, 20 + 2); From 3af3f5d03e4020f0d967460541a586641575026d Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Fri, 29 Sep 2023 21:14:58 +0200 Subject: [PATCH 36/43] fix migration --- substrate/frame/nomination-pools/src/migration.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index d2f48d3d40f5..de4f0c96ddec 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -793,21 +793,21 @@ pub mod v1 { /// /// WARNING: This migration works under the assumption that the [`BondedPools`] cannot be inflated /// arbitrarily. Otherwise this migration could fail due to too high weight. -pub mod v6 { +mod v7 { use super::*; - /// [`VersionUncheckedMigrateV5ToV6`] wrapped in a + /// [`VersionUncheckedMigrateV6ToV7`] wrapped in a /// [`frame_support::migrations::VersionedMigration`], ensuring the migration is only - /// performed when on-chain version is 5. + /// performed when on-chain version is 6. pub type VersionCheckedMigrateV5ToV6 = frame_support::migrations::VersionedMigration< - 5, 6, + 7, VersionUncheckedMigrateV5ToV6, crate::pallet::Pallet, ::DbWeight, >; - pub struct VersionUncheckedMigrateV5ToV6(sp_std::marker::PhantomData); + pub struct VersionUncheckedMigrateV6ToV7(sp_std::marker::PhantomData); impl VersionUncheckedMigrateV5ToV6 { fn calculate_tvl_by_total_stake() -> BalanceOf { BondedPools::::iter() @@ -868,8 +868,8 @@ pub mod v6 { ); ensure!( - Pallet::::on_chain_storage_version() >= 6, - "nomination-pools::migration::v6: wrong storage version" + Pallet::::on_chain_storage_version() >= 7, + "nomination-pools::migration::v7: wrong storage version" ); Ok(()) From 0a85748a6b9e8a4eb6bec4d7c878a22e81bb2090 Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Fri, 29 Sep 2023 21:15:23 +0200 Subject: [PATCH 37/43] merging leftovers cleared --- substrate/frame/nomination-pools/src/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/nomination-pools/src/tests.rs b/substrate/frame/nomination-pools/src/tests.rs index 7d79e1fc447b..2749e89ecff3 100644 --- a/substrate/frame/nomination-pools/src/tests.rs +++ b/substrate/frame/nomination-pools/src/tests.rs @@ -6964,7 +6964,7 @@ mod slash { }; ExtBuilder::default().with_check(0).build_and_execute(|| { // Given - Balances::make_free_balance_be(&11, ExistentialDeposit::get() + 2); + Currency::set_balance(&11, ExistentialDeposit::get() + 2); assert!(!PoolMembers::::contains_key(11)); assert_eq!(TotalValueLocked::::get(), 10); @@ -6993,7 +6993,7 @@ mod slash { StakingMock::slash_by(1, 6); // And - Balances::make_free_balance_be(&12, ExistentialDeposit::get() + 12); + Currency::set_balance(&12, ExistentialDeposit::get() + 12); assert!(!PoolMembers::::contains_key(12)); // When From 8489cda2275385b58e902857a38a39ab2bc84205 Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Fri, 29 Sep 2023 21:23:52 +0200 Subject: [PATCH 38/43] migration fix --- polkadot/runtime/westend/src/lib.rs | 2 +- .../frame/nomination-pools/src/migration.rs | 176 +++++++++--------- 2 files changed, 89 insertions(+), 89 deletions(-) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 5676bbd81529..bab1b346ccdf 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1508,7 +1508,7 @@ pub mod migrations { paras_registrar::migration::VersionCheckedMigrateToV1, pallet_nomination_pools::migration::versioned_migrations::V5toV6, pallet_referenda::migration::v1::MigrateV0ToV1, - pallet_nomination_pools::migration::v6::VersionCheckedMigrateV5ToV6, + pallet_nomination_pools::migration::v7::VersionCheckedMigrateV6ToV7, ); } diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index de4f0c96ddec..1e94cebb74ff 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -37,6 +37,94 @@ pub mod versioned_migrations { >; } +/// This migration accumulates and initializes the [`TotalValueLocked`] for all pools. +/// +/// WARNING: This migration works under the assumption that the [`BondedPools`] cannot be inflated +/// arbitrarily. Otherwise this migration could fail due to too high weight. +mod v7 { + use super::*; + + /// [`VersionUncheckedMigrateV6ToV7`] wrapped in a + /// [`frame_support::migrations::VersionedMigration`], ensuring the migration is only + /// performed when on-chain version is 6. + pub type VersionCheckedMigrateV6ToV7 = frame_support::migrations::VersionedMigration< + 6, + 7, + VersionUncheckedMigrateV6ToV7, + crate::pallet::Pallet, + ::DbWeight, + >; + + pub struct VersionUncheckedMigrateV6ToV7(sp_std::marker::PhantomData); + impl VersionUncheckedMigrateV6ToV7 { + fn calculate_tvl_by_total_stake() -> BalanceOf { + BondedPools::::iter() + .map(|(id, inner)| { + T::Staking::total_stake( + &BondedPool { id, inner: inner.clone() }.bonded_account(), + ) + .unwrap_or_default() + }) + .reduce(|acc, total_balance| acc + total_balance) + .unwrap_or_default() + } + } + + impl OnRuntimeUpgrade for VersionUncheckedMigrateV6ToV7 { + fn on_runtime_upgrade() -> Weight { + let migrated = BondedPools::::count(); + // The TVL should be the sum of all the funds that are actively staked and in the + // unbonding process of the account of each pool. + let tvl: BalanceOf = Self::calculate_tvl_by_total_stake(); + + TotalValueLocked::::set(tvl); + + log!(info, "Upgraded {} pools with a TVL of {:?}", migrated, tvl); + + // reads: migrated * (BondedPools + Staking::total_stake) + count + onchain + // version + // + // writes: current version + TVL + T::DbWeight::get().reads_writes(migrated.saturating_mul(2).saturating_add(2).into(), 2) + } + + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, TryRuntimeError> { + Ok(Vec::new()) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(_data: Vec) -> Result<(), TryRuntimeError> { + // check that the `TotalValueLocked` written is actually the sum of `total_stake` of the + // `BondedPools`` + let tvl: BalanceOf = Self::calculate_tvl_by_total_stake(); + ensure!( + TotalValueLocked::::get() == tvl, + "TVL written is not equal to `Staking::total_stake` of all `BondedPools`." + ); + + // calculate the sum of `total_balance` of all `PoolMember` as the upper bound for the + // `TotalValueLocked`. + let total_balance_members: BalanceOf = PoolMembers::::iter() + .map(|(_, member)| member.total_balance()) + .reduce(|acc, total_balance| acc + total_balance) + .unwrap_or_default(); + + ensure!( + TotalValueLocked::::get() <= total_balance_members, + "TVL is greater than the balance of all PoolMembers." + ); + + ensure!( + Pallet::::on_chain_storage_version() >= 7, + "nomination-pools::migration::v7: wrong storage version" + ); + + Ok(()) + } + } +} + mod v6 { use super::*; @@ -788,91 +876,3 @@ pub mod v1 { } } } - -/// This migration accumulates and initializes the [`TotalValueLocked`] for all pools. -/// -/// WARNING: This migration works under the assumption that the [`BondedPools`] cannot be inflated -/// arbitrarily. Otherwise this migration could fail due to too high weight. -mod v7 { - use super::*; - - /// [`VersionUncheckedMigrateV6ToV7`] wrapped in a - /// [`frame_support::migrations::VersionedMigration`], ensuring the migration is only - /// performed when on-chain version is 6. - pub type VersionCheckedMigrateV5ToV6 = frame_support::migrations::VersionedMigration< - 6, - 7, - VersionUncheckedMigrateV5ToV6, - crate::pallet::Pallet, - ::DbWeight, - >; - - pub struct VersionUncheckedMigrateV6ToV7(sp_std::marker::PhantomData); - impl VersionUncheckedMigrateV5ToV6 { - fn calculate_tvl_by_total_stake() -> BalanceOf { - BondedPools::::iter() - .map(|(id, inner)| { - T::Staking::total_stake( - &BondedPool { id, inner: inner.clone() }.bonded_account(), - ) - .unwrap_or_default() - }) - .reduce(|acc, total_balance| acc + total_balance) - .unwrap_or_default() - } - } - - impl OnRuntimeUpgrade for VersionUncheckedMigrateV5ToV6 { - fn on_runtime_upgrade() -> Weight { - let migrated = BondedPools::::count(); - // The TVL should be the sum of all the funds that are actively staked and in the - // unbonding process of the account of each pool. - let tvl: BalanceOf = Self::calculate_tvl_by_total_stake(); - - TotalValueLocked::::set(tvl); - - log!(info, "Upgraded {} pools with a TVL of {:?}", migrated, tvl); - - // reads: migrated * (BondedPools + Staking::total_stake) + count + onchain - // version - // - // writes: current version + TVL - T::DbWeight::get().reads_writes(migrated.saturating_mul(2).saturating_add(2).into(), 2) - } - - #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, TryRuntimeError> { - Ok(Vec::new()) - } - - #[cfg(feature = "try-runtime")] - fn post_upgrade(_data: Vec) -> Result<(), TryRuntimeError> { - // check that the `TotalValueLocked` written is actually the sum of `total_stake` of the - // `BondedPools`` - let tvl: BalanceOf = Self::calculate_tvl_by_total_stake(); - ensure!( - TotalValueLocked::::get() == tvl, - "TVL written is not equal to `Staking::total_stake` of all `BondedPools`." - ); - - // calculate the sum of `total_balance` of all `PoolMember` as the upper bound for the - // `TotalValueLocked`. - let total_balance_members: BalanceOf = PoolMembers::::iter() - .map(|(_, member)| member.total_balance()) - .reduce(|acc, total_balance| acc + total_balance) - .unwrap_or_default(); - - ensure!( - TotalValueLocked::::get() <= total_balance_members, - "TVL is greater than the balance of all PoolMembers." - ); - - ensure!( - Pallet::::on_chain_storage_version() >= 7, - "nomination-pools::migration::v7: wrong storage version" - ); - - Ok(()) - } - } -} From 8d4ccb7faf1430cca3ce764f64846366f6f4fe7c Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Fri, 29 Sep 2023 21:29:18 +0200 Subject: [PATCH 39/43] visibilty fix --- substrate/frame/nomination-pools/src/migration.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index 1e94cebb74ff..e2a05239a7aa 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -41,7 +41,7 @@ pub mod versioned_migrations { /// /// WARNING: This migration works under the assumption that the [`BondedPools`] cannot be inflated /// arbitrarily. Otherwise this migration could fail due to too high weight. -mod v7 { +pub mod v7 { use super::*; /// [`VersionUncheckedMigrateV6ToV7`] wrapped in a From 1a22b0ad9aada9d714fc3f8c32420cb76a86ea9e Mon Sep 17 00:00:00 2001 From: Piet Wolff Date: Fri, 29 Sep 2023 21:44:11 +0200 Subject: [PATCH 40/43] pallet version 7 --- substrate/frame/nomination-pools/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index a032c2f8b162..909a930e3821 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -1572,7 +1572,7 @@ pub mod pallet { use sp_runtime::Perbill; /// The current storage version. - const STORAGE_VERSION: StorageVersion = StorageVersion::new(6); + const STORAGE_VERSION: StorageVersion = StorageVersion::new(7); #[pallet::pallet] #[pallet::storage_version(STORAGE_VERSION)] From d6f5646a68331aa626bb3410fcdf4c75d9af99f0 Mon Sep 17 00:00:00 2001 From: Ankan Date: Sun, 1 Oct 2023 01:49:12 +0200 Subject: [PATCH 41/43] make the unchecked migration module private --- polkadot/runtime/westend/src/lib.rs | 2 +- .../frame/nomination-pools/src/migration.rs | 25 ++++++++++--------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index bab1b346ccdf..6085b6e37455 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1508,7 +1508,7 @@ pub mod migrations { paras_registrar::migration::VersionCheckedMigrateToV1, pallet_nomination_pools::migration::versioned_migrations::V5toV6, pallet_referenda::migration::v1::MigrateV0ToV1, - pallet_nomination_pools::migration::v7::VersionCheckedMigrateV6ToV7, + pallet_nomination_pools::migration::versioned_migrations::V6ToV7, ); } diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index e2a05239a7aa..7d7bdee7c7f4 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -27,6 +27,17 @@ use sp_runtime::TryRuntimeError; pub mod versioned_migrations { use super::*; + /// [`VersionUncheckedMigrateV6ToV7`] wrapped in a + /// [`frame_support::migrations::VersionedMigration`], ensuring the migration is only + /// performed when on-chain version is 6. + pub type V6ToV7 = frame_support::migrations::VersionedMigration< + 6, + 7, + v7::VersionUncheckedMigrateV6ToV7, + crate::pallet::Pallet, + ::DbWeight, + >; + /// Wrapper over `MigrateToV6` with convenience version checks. pub type V5toV6 = frame_support::migrations::VersionedMigration< 5, @@ -35,26 +46,16 @@ pub mod versioned_migrations { crate::pallet::Pallet, ::DbWeight, >; + } /// This migration accumulates and initializes the [`TotalValueLocked`] for all pools. /// /// WARNING: This migration works under the assumption that the [`BondedPools`] cannot be inflated /// arbitrarily. Otherwise this migration could fail due to too high weight. -pub mod v7 { +mod v7 { use super::*; - /// [`VersionUncheckedMigrateV6ToV7`] wrapped in a - /// [`frame_support::migrations::VersionedMigration`], ensuring the migration is only - /// performed when on-chain version is 6. - pub type VersionCheckedMigrateV6ToV7 = frame_support::migrations::VersionedMigration< - 6, - 7, - VersionUncheckedMigrateV6ToV7, - crate::pallet::Pallet, - ::DbWeight, - >; - pub struct VersionUncheckedMigrateV6ToV7(sp_std::marker::PhantomData); impl VersionUncheckedMigrateV6ToV7 { fn calculate_tvl_by_total_stake() -> BalanceOf { From c9287cdd23c3a0dcf89d9cf47b3a3c75d3563095 Mon Sep 17 00:00:00 2001 From: Ankan Date: Sun, 1 Oct 2023 02:11:39 +0200 Subject: [PATCH 42/43] rust doc update --- substrate/frame/nomination-pools/src/migration.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index 7d7bdee7c7f4..7845376972df 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -27,9 +27,8 @@ use sp_runtime::TryRuntimeError; pub mod versioned_migrations { use super::*; - /// [`VersionUncheckedMigrateV6ToV7`] wrapped in a - /// [`frame_support::migrations::VersionedMigration`], ensuring the migration is only - /// performed when on-chain version is 6. + /// Migration V6 to V7 wrapped in a [`frame_support::migrations::VersionedMigration`], ensuring + /// the migration is only performed when on-chain version is 6. pub type V6ToV7 = frame_support::migrations::VersionedMigration< 6, 7, From 71175c9e5f9f7afaaf8f1689e2934f9105488be5 Mon Sep 17 00:00:00 2001 From: command-bot <> Date: Sun, 1 Oct 2023 00:15:31 +0000 Subject: [PATCH 43/43] ".git/.scripts/commands/fmt/fmt.sh" --- substrate/frame/nomination-pools/src/migration.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index 7845376972df..eef2a976f1a2 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -45,7 +45,6 @@ pub mod versioned_migrations { crate::pallet::Pallet, ::DbWeight, >; - } /// This migration accumulates and initializes the [`TotalValueLocked`] for all pools.