From aac07af03c3453e2f36990f0cccc3d68841482d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Thu, 8 Feb 2024 19:03:22 +0100 Subject: [PATCH] Fixes `TotalValueLocked` out of sync in nomination pools (#3052) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `TotalLockedValue` storage value in nomination pools pallet may get out of sync if the staking pallet does implicit withdrawal of unlocking chunks belonging to a bonded pool stash. This fix is based on a new method in the `OnStakingUpdate` traits, `on_withdraw`, which allows the nomination pools pallet to adjust the `TotalLockedValue` every time there is an implicit or explicit withdrawal from a bonded pool's stash. This PR also adds a migration that checks and updates the on-chain TVL if it got out of sync due to the bug this PR fixes. **Changes to `trait OnStakingUpdate`** In order for staking to notify the nomination pools pallet that chunks where withdrew, we add a new method, `on_withdraw` to the `OnStakingUpdate` trait. The nomination pools pallet filters the withdraws that are related to bonded pool accounts and updates the `TotalValueLocked` accordingly. **Others** - Adds try-state checks to the EPM/staking e2e tests - Adds tests for auto withdrawing in the context of nomination pools **To-do** - [x] check if we need a migration to fix the current `TotalValueLocked` (run try-runtime) - [x] migrations to fix the current on-chain TVL value ✅ **Kusama**: ``` TotalValueLocked: 99.4559 kKSM TotalValueLocked (calculated) 99.4559 kKSM ``` ⚠️ **Westend**: ``` TotalValueLocked: 18.4060 kWND TotalValueLocked (calculated) 18.4050 kWND ``` **Polkadot**: TVL not released yet. Closes https://github.com/paritytech/polkadot-sdk/issues/3055 --------- Co-authored-by: command-bot <> Co-authored-by: Ross Bulat Co-authored-by: Dónal Murray --- Cargo.lock | 1 + polkadot/runtime/westend/src/lib.rs | 2 +- prdoc/pr_3052.prdoc | 15 ++ .../test-staking-e2e/Cargo.toml | 16 ++ .../test-staking-e2e/src/lib.rs | 166 ++++++++++++++++-- .../test-staking-e2e/src/mock.rs | 106 +++++++++-- substrate/frame/nomination-pools/src/lib.rs | 42 ++--- .../frame/nomination-pools/src/migration.rs | 86 +++++++-- substrate/frame/nomination-pools/src/mock.rs | 13 ++ substrate/frame/staking/src/pallet/impls.rs | 5 +- substrate/frame/staking/src/pallet/mod.rs | 2 +- substrate/primitives/staking/src/lib.rs | 3 + 12 files changed, 383 insertions(+), 74 deletions(-) create mode 100644 prdoc/pr_3052.prdoc diff --git a/Cargo.lock b/Cargo.lock index b6fa96cd0a37..845f22178557 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9735,6 +9735,7 @@ dependencies = [ "pallet-bags-list", "pallet-balances", "pallet-election-provider-multi-phase", + "pallet-nomination-pools", "pallet-session", "pallet-staking", "pallet-timestamp", diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 7305684a0d98..8d903ebf62c7 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1647,7 +1647,7 @@ pub mod migrations { pallet_referenda::migration::v1::MigrateV0ToV1, pallet_grandpa::migrations::MigrateV4ToV5, parachains_configuration::migration::v10::MigrateToV10, - pallet_nomination_pools::migration::versioned::V7ToV8, + pallet_nomination_pools::migration::unversioned::TotalValueLockedSync, UpgradeSessionKeys, frame_support::migrations::RemovePallet< ImOnlinePalletName, diff --git a/prdoc/pr_3052.prdoc b/prdoc/pr_3052.prdoc new file mode 100644 index 000000000000..09f3cf59e4da --- /dev/null +++ b/prdoc/pr_3052.prdoc @@ -0,0 +1,15 @@ +title: "Fixes a scenario where a nomination pool's `TotalValueLocked` is out of sync due to staking's implicit withdraw" + +doc: + - audience: Runtime Dev + description: | + The nomination pools pallet `TotalValueLocked` may get out of sync if the staking pallet + does implicit withdrawal of unlocking chunks belonging to a bonded pool stash. This fix + is based on a new method on the `OnStakingUpdate` traits, `on_withdraw`, which allows the + nomination pools pallet to adjust the `TotalValueLocked` every time there is an implicit or + explicit withdrawal from a bonded pool's stash. + +crates: + - name: "pallet-nomination-pools" + - name: "pallet-staking" + - name: "sp-staking" diff --git a/substrate/frame/election-provider-multi-phase/test-staking-e2e/Cargo.toml b/substrate/frame/election-provider-multi-phase/test-staking-e2e/Cargo.toml index 05c6a6d40462..e9bcc96455b1 100644 --- a/substrate/frame/election-provider-multi-phase/test-staking-e2e/Cargo.toml +++ b/substrate/frame/election-provider-multi-phase/test-staking-e2e/Cargo.toml @@ -35,7 +35,23 @@ frame-election-provider-support = { path = "../../election-provider-support" } pallet-election-provider-multi-phase = { path = ".." } pallet-staking = { path = "../../staking" } +pallet-nomination-pools = { path = "../../nomination-pools" } pallet-bags-list = { path = "../../bags-list" } pallet-balances = { path = "../../balances" } pallet-timestamp = { path = "../../timestamp" } pallet-session = { path = "../../session" } + +[features] +try-runtime = [ + "frame-election-provider-support/try-runtime", + "frame-support/try-runtime", + "frame-system/try-runtime", + "pallet-bags-list/try-runtime", + "pallet-balances/try-runtime", + "pallet-election-provider-multi-phase/try-runtime", + "pallet-nomination-pools/try-runtime", + "pallet-session/try-runtime", + "pallet-staking/try-runtime", + "pallet-timestamp/try-runtime", + "sp-runtime/try-runtime", +] diff --git a/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/lib.rs b/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/lib.rs index 1d3f4712b1d6..53bff50f7482 100644 --- a/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/lib.rs +++ b/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/lib.rs @@ -53,9 +53,9 @@ fn log_current_time() { #[test] fn block_progression_works() { - let (mut ext, pool_state, _) = ExtBuilder::default().build_offchainify(); + let (ext, pool_state, _) = ExtBuilder::default().build_offchainify(); - ext.execute_with(|| { + execute_with(ext, || { assert_eq!(active_era(), 0); assert_eq!(Session::current_index(), 0); assert!(ElectionProviderMultiPhase::current_phase().is_off()); @@ -70,9 +70,9 @@ fn block_progression_works() { assert!(ElectionProviderMultiPhase::current_phase().is_signed()); }); - let (mut ext, pool_state, _) = ExtBuilder::default().build_offchainify(); + let (ext, pool_state, _) = ExtBuilder::default().build_offchainify(); - ext.execute_with(|| { + execute_with(ext, || { assert_eq!(active_era(), 0); assert_eq!(Session::current_index(), 0); assert!(ElectionProviderMultiPhase::current_phase().is_off()); @@ -93,12 +93,12 @@ fn offchainify_works() { let staking_builder = StakingExtBuilder::default(); let epm_builder = EpmExtBuilder::default(); - let (mut ext, pool_state, _) = ExtBuilder::default() + let (ext, pool_state, _) = ExtBuilder::default() .epm(epm_builder) .staking(staking_builder) .build_offchainify(); - ext.execute_with(|| { + execute_with(ext, || { // test ocw progression and solution queue if submission when unsigned phase submission is // not delayed. for _ in 0..100 { @@ -142,9 +142,9 @@ fn offchainify_works() { /// restarts. Note that in this test case, the emergency throttling is disabled. fn enters_emergency_phase_after_forcing_before_elect() { let epm_builder = EpmExtBuilder::default().disable_emergency_throttling(); - let (mut ext, pool_state, _) = ExtBuilder::default().epm(epm_builder).build_offchainify(); + let (ext, pool_state, _) = ExtBuilder::default().epm(epm_builder).build_offchainify(); - ext.execute_with(|| { + execute_with(ext, || { log!( trace, "current validators (staking): {:?}", @@ -213,12 +213,12 @@ fn continous_slashes_below_offending_threshold() { let staking_builder = StakingExtBuilder::default().validator_count(10); let epm_builder = EpmExtBuilder::default().disable_emergency_throttling(); - let (mut ext, pool_state, _) = ExtBuilder::default() + let (ext, pool_state, _) = ExtBuilder::default() .epm(epm_builder) .staking(staking_builder) .build_offchainify(); - ext.execute_with(|| { + execute_with(ext, || { assert_eq!(Session::validators().len(), 10); let mut active_validator_set = Session::validators(); @@ -271,12 +271,12 @@ fn set_validation_intention_after_chilled() { use frame_election_provider_support::SortedListProvider; use pallet_staking::{Event, Forcing, Nominators}; - let (mut ext, pool_state, _) = ExtBuilder::default() + let (ext, pool_state, _) = ExtBuilder::default() .epm(EpmExtBuilder::default()) .staking(StakingExtBuilder::default()) .build_offchainify(); - ext.execute_with(|| { + execute_with(ext, || { assert_eq!(active_era(), 0); // validator is part of the validator set. assert!(Session::validators().contains(&41)); @@ -334,10 +334,10 @@ fn set_validation_intention_after_chilled() { fn ledger_consistency_active_balance_below_ed() { use pallet_staking::{Error, Event}; - let (mut ext, pool_state, _) = + let (ext, pool_state, _) = ExtBuilder::default().staking(StakingExtBuilder::default()).build_offchainify(); - ext.execute_with(|| { + execute_with(ext, || { assert_eq!(Staking::ledger(11.into()).unwrap().active, 1000); // unbonding total of active stake fails because the active ledger balance would fall @@ -387,3 +387,141 @@ fn ledger_consistency_active_balance_below_ed() { assert!(Staking::ledger(11.into()).is_err()); }); } + +#[test] +/// Automatic withdrawal of unlocking funds in staking propagates to the nomination pools and its +/// state correctly. +/// +/// The staking pallet may withdraw unlocking funds from a pool's bonded account without a pool +/// member or operator calling explicitly `Call::withdraw*`. This test verifies that the member's +/// are eventually paid and the `TotalValueLocked` is kept in sync in those cases. +fn automatic_unbonding_pools() { + use pallet_nomination_pools::TotalValueLocked; + + // closure to fetch the staking unlocking chunks of an account. + let unlocking_chunks_of = |account: AccountId| -> usize { + Staking::ledger(sp_staking::StakingAccount::Controller(account)) + .unwrap() + .unlocking + .len() + }; + + let (ext, pool_state, _) = ExtBuilder::default() + .pools(PoolsExtBuilder::default().max_unbonding(1)) + .staking(StakingExtBuilder::default().max_unlocking(1).bonding_duration(2)) + .build_offchainify(); + + execute_with(ext, || { + assert_eq!(::MaxUnlockingChunks::get(), 1); + assert_eq!(::BondingDuration::get(), 2); + assert_eq!(::MaxUnbonding::get(), 1); + + // init state of pool members. + let init_free_balance_2 = Balances::free_balance(2); + let init_free_balance_3 = Balances::free_balance(3); + + let pool_bonded_account = Pools::create_bonded_account(1); + + // creates a pool with 5 bonded, owned by 1. + assert_ok!(Pools::create(RuntimeOrigin::signed(1), 5, 1, 1, 1)); + assert_eq!(locked_amount_for(pool_bonded_account), 5); + + let init_tvl = TotalValueLocked::::get(); + + // 2 joins the pool. + assert_ok!(Pools::join(RuntimeOrigin::signed(2), 10, 1)); + assert_eq!(locked_amount_for(pool_bonded_account), 15); + + // 3 joins the pool. + assert_ok!(Pools::join(RuntimeOrigin::signed(3), 10, 1)); + assert_eq!(locked_amount_for(pool_bonded_account), 25); + + assert_eq!(TotalValueLocked::::get(), 25); + + // currently unlocking 0 chunks in the bonded pools ledger. + assert_eq!(unlocking_chunks_of(pool_bonded_account), 0); + + // unbond 2 from pool. + assert_ok!(Pools::unbond(RuntimeOrigin::signed(2), 2, 10)); + + // amount is still locked in the pool, needs to wait for unbonding period. + assert_eq!(locked_amount_for(pool_bonded_account), 25); + + // max chunks in the ledger are now filled up (`MaxUnlockingChunks == 1`). + assert_eq!(unlocking_chunks_of(pool_bonded_account), 1); + + // tries to unbond 3 from pool. it will fail since there are no unlocking chunks left + // available and the current in the queue haven't been there for more than bonding + // duration. + assert_err!( + Pools::unbond(RuntimeOrigin::signed(3), 3, 10), + pallet_staking::Error::::NoMoreChunks + ); + + assert_eq!(current_era(), 0); + + // progress over bonding duration. + for _ in 0..=::BondingDuration::get() { + start_next_active_era(pool_state.clone()).unwrap(); + } + assert_eq!(current_era(), 3); + System::reset_events(); + + let locked_before_withdraw_pool = locked_amount_for(pool_bonded_account); + assert_eq!(Balances::free_balance(pool_bonded_account), 26); + + // now unbonding 3 will work, although the pool's ledger still has the unlocking chunks + // filled up. + assert_ok!(Pools::unbond(RuntimeOrigin::signed(3), 3, 10)); + assert_eq!(unlocking_chunks_of(pool_bonded_account), 1); + + assert_eq!( + staking_events(), + [ + // auto-withdraw happened as expected to release 2's unbonding funds, but the funds + // were not transfered to 2 and stay in the pool's tranferrable balance instead. + pallet_staking::Event::Withdrawn { stash: 7939698191839293293, amount: 10 }, + pallet_staking::Event::Unbonded { stash: 7939698191839293293, amount: 10 } + ] + ); + + // balance of the pool remains the same, it hasn't withdraw explicitly from the pool yet. + assert_eq!(Balances::free_balance(pool_bonded_account), 26); + // but the locked amount in the pool's account decreases due to the auto-withdraw: + assert_eq!(locked_before_withdraw_pool - 10, locked_amount_for(pool_bonded_account)); + + // TVL correctly updated. + assert_eq!(TotalValueLocked::::get(), 25 - 10); + + // however, note that the withdrawing from the pool still works for 2, the funds are taken + // from the pool's free balance. + assert_eq!(Balances::free_balance(pool_bonded_account), 26); + assert_ok!(Pools::withdraw_unbonded(RuntimeOrigin::signed(2), 2, 10)); + assert_eq!(Balances::free_balance(pool_bonded_account), 16); + + assert_eq!(Balances::free_balance(2), 20); + assert_eq!(TotalValueLocked::::get(), 15); + + // 3 cannot withdraw yet. + assert_err!( + Pools::withdraw_unbonded(RuntimeOrigin::signed(3), 3, 10), + pallet_nomination_pools::Error::::CannotWithdrawAny + ); + + // progress over bonding duration. + for _ in 0..=::BondingDuration::get() { + start_next_active_era(pool_state.clone()).unwrap(); + } + assert_eq!(current_era(), 6); + System::reset_events(); + + assert_ok!(Pools::withdraw_unbonded(RuntimeOrigin::signed(3), 3, 10)); + + // final conditions are the expected. + assert_eq!(Balances::free_balance(pool_bonded_account), 6); // 5 init bonded + ED + assert_eq!(Balances::free_balance(2), init_free_balance_2); + assert_eq!(Balances::free_balance(3), init_free_balance_3); + + assert_eq!(TotalValueLocked::::get(), init_tvl); + }); +} diff --git a/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/mock.rs b/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/mock.rs index 32633a0ed7d3..882b894bb22f 100644 --- a/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/mock.rs +++ b/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/mock.rs @@ -69,6 +69,7 @@ frame_support::construct_runtime!( System: frame_system, ElectionProviderMultiPhase: pallet_election_provider_multi_phase, Staking: pallet_staking, + Pools: pallet_nomination_pools, Balances: pallet_balances, BagsList: pallet_bags_list, Session: pallet_session, @@ -114,7 +115,7 @@ impl pallet_balances::Config for Runtime { type MaxFreezes = traits::ConstU32<1>; type RuntimeHoldReason = RuntimeHoldReason; type RuntimeFreezeReason = RuntimeFreezeReason; - type FreezeIdentifier = (); + type FreezeIdentifier = RuntimeFreezeReason; type WeightInfo = (); } @@ -233,7 +234,7 @@ const THRESHOLDS: [VoteWeight; 9] = [10, 20, 30, 40, 50, 60, 1_000, 2_000, 10_00 parameter_types! { pub static BagThresholds: &'static [sp_npos_elections::VoteWeight] = &THRESHOLDS; pub const SessionsPerEra: sp_staking::SessionIndex = 2; - pub const BondingDuration: sp_staking::EraIndex = 28; + pub static BondingDuration: sp_staking::EraIndex = 28; pub const SlashDeferDuration: sp_staking::EraIndex = 7; // 1/4 the bonding duration. pub const OffendingValidatorsThreshold: Perbill = Perbill::from_percent(40); pub HistoryDepth: u32 = 84; @@ -247,6 +248,45 @@ impl pallet_bags_list::Config for Runtime { type Score = VoteWeight; } +pub struct BalanceToU256; +impl sp_runtime::traits::Convert for BalanceToU256 { + fn convert(n: Balance) -> sp_core::U256 { + n.into() + } +} + +pub struct U256ToBalance; +impl sp_runtime::traits::Convert for U256ToBalance { + fn convert(n: sp_core::U256) -> Balance { + n.try_into().unwrap() + } +} + +parameter_types! { + pub const PoolsPalletId: frame_support::PalletId = frame_support::PalletId(*b"py/nopls"); + pub static MaxUnbonding: u32 = 8; +} + +impl pallet_nomination_pools::Config for Runtime { + type RuntimeEvent = RuntimeEvent; + type WeightInfo = (); + type Currency = Balances; + type RuntimeFreezeReason = RuntimeFreezeReason; + type RewardCounter = sp_runtime::FixedU128; + type BalanceToU256 = BalanceToU256; + type U256ToBalance = U256ToBalance; + type Staking = Staking; + type PostUnbondingPoolsWindow = ConstU32<2>; + type PalletId = PoolsPalletId; + type MaxMetadataLen = ConstU32<256>; + type MaxUnbonding = MaxUnbonding; + type MaxPointsToBalance = frame_support::traits::ConstU8<10>; +} + +parameter_types! { + pub static MaxUnlockingChunks: u32 = 32; +} + /// Upper limit on the number of NPOS nominations. const MAX_QUOTA_NOMINATIONS: u32 = 16; @@ -273,10 +313,10 @@ impl pallet_staking::Config for Runtime { type VoterList = BagsList; type NominationsQuota = pallet_staking::FixedNominationsQuota; type TargetList = pallet_staking::UseValidatorsMap; - type MaxUnlockingChunks = ConstU32<32>; + type MaxUnlockingChunks = MaxUnlockingChunks; type MaxControllersInDeprecationBatch = ConstU32<100>; type HistoryDepth = HistoryDepth; - type EventListeners = (); + type EventListeners = Pools; type WeightInfo = pallet_staking::weights::SubstrateWeight; type BenchmarkingConfig = pallet_staking::TestBenchmarkingConfig; } @@ -394,6 +434,14 @@ impl StakingExtBuilder { self.validator_count = n; self } + pub fn max_unlocking(self, max: u32) -> Self { + ::set(max); + self + } + pub fn bonding_duration(self, eras: EraIndex) -> Self { + ::set(eras); + self + } } pub struct EpmExtBuilder {} @@ -417,6 +465,21 @@ impl EpmExtBuilder { } } +pub struct PoolsExtBuilder {} + +impl Default for PoolsExtBuilder { + fn default() -> Self { + PoolsExtBuilder {} + } +} + +impl PoolsExtBuilder { + pub fn max_unbonding(self, max: u32) -> Self { + ::set(max); + self + } +} + pub struct BalancesExtBuilder { balances: Vec<(AccountId, Balance)>, } @@ -464,6 +527,7 @@ pub struct ExtBuilder { staking_builder: StakingExtBuilder, epm_builder: EpmExtBuilder, balances_builder: BalancesExtBuilder, + pools_builder: PoolsExtBuilder, } impl Default for ExtBuilder { @@ -472,6 +536,7 @@ impl Default for ExtBuilder { staking_builder: StakingExtBuilder::default(), epm_builder: EpmExtBuilder::default(), balances_builder: BalancesExtBuilder::default(), + pools_builder: PoolsExtBuilder::default(), } } } @@ -548,6 +613,11 @@ impl ExtBuilder { self } + pub fn pools(mut self, builder: PoolsExtBuilder) -> Self { + self.pools_builder = builder; + self + } + pub fn balances(mut self, builder: BalancesExtBuilder) -> Self { self.balances_builder = builder; self @@ -567,20 +637,20 @@ impl ExtBuilder { (ext, pool_state, offchain_state) } +} - pub fn build_and_execute(self, test: impl FnOnce() -> ()) { - let mut ext = self.build(); - ext.execute_with(test); +pub(crate) fn execute_with(mut ext: sp_io::TestExternalities, test: impl FnOnce() -> ()) { + ext.execute_with(test); - #[cfg(feature = "try-runtime")] - ext.execute_with(|| { - let bn = System::block_number(); + #[cfg(feature = "try-runtime")] + ext.execute_with(|| { + let bn = System::block_number(); - assert_ok!(>::try_state(bn)); - assert_ok!(>::try_state(bn)); - assert_ok!(>::try_state(bn)); - }); - } + assert_ok!(>::try_state(bn)); + assert_ok!(>::try_state(bn)); + assert_ok!(>::try_state(bn)); + assert_ok!(>::try_state(bn)); + }); } // Progress to given block, triggering session and era changes as we progress and ensuring that @@ -606,6 +676,7 @@ pub fn roll_to(n: BlockNumber, delay_solution: bool) { if b != n { Staking::on_finalize(System::block_number()); } + Pools::on_initialize(b); log_current_time(); } @@ -860,6 +931,11 @@ pub(crate) fn set_minimum_election_score( .map_err(|_| ()) } +pub(crate) fn locked_amount_for(account_id: AccountId) -> Balance { + let lock = pallet_balances::Locks::::get(account_id); + lock[0].amount +} + pub(crate) fn staking_events() -> Vec> { System::events() .into_iter() diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index c2653d8bdabc..074d59931ade 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -1293,27 +1293,6 @@ 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(); - - 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 - .defensive_saturating_sub(T::Staking::total_stake(&bonded_account).unwrap_or_default()); - TotalValueLocked::::mutate(|tvl| { - tvl.saturating_reduce(diff); - }); - outcome - } } /// A reward pool. @@ -1733,7 +1712,7 @@ pub mod pallet { CountedStorageMap<_, Twox64Concat, PoolId, BondedPoolInner>; /// Reward pools. This is where there rewards for each pool accumulate. When a members payout is - /// claimed, the balance comes out fo the reward pool. Keyed by the bonded pools account. + /// claimed, the balance comes out of the reward pool. Keyed by the bonded pools account. #[pallet::storage] pub type RewardPools = CountedStorageMap<_, Twox64Concat, PoolId, RewardPool>; @@ -1753,8 +1732,8 @@ pub mod pallet { /// A reverse lookup from the pool's account id to its id. /// - /// This is only used for slashing. In all other instances, the pool id is used, and the - /// accounts are deterministically derived from it. + /// This is only used for slashing and on automatic withdraw update. In all other instances, the + /// pool id is used, and the accounts are deterministically derived from it. #[pallet::storage] pub type ReversePoolIdLookup = CountedStorageMap<_, Twox64Concat, T::AccountId, PoolId, OptionQuery>; @@ -2223,7 +2202,7 @@ pub mod pallet { // 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); - pool.withdraw_from_staking(num_slashing_spans)?; + T::Staking::withdraw_unbonded(pool.bonded_account(), num_slashing_spans)?; Ok(()) } @@ -2275,7 +2254,8 @@ pub mod pallet { // Before calculating the `balance_to_unbond`, we call withdraw unbonded to ensure the // `transferrable_balance` is correct. - let stash_killed = bonded_pool.withdraw_from_staking(num_slashing_spans)?; + let stash_killed = + T::Staking::withdraw_unbonded(bonded_pool.bonded_account(), num_slashing_spans)?; // defensive-only: the depositor puts enough funds into the stash so that it will only // be destroyed when they are leaving. @@ -3628,4 +3608,14 @@ impl sp_staking::OnStakingUpdate> for Pall } Self::deposit_event(Event::::PoolSlashed { pool_id, balance: slashed_bonded }); } + + /// Reduces the overall `TotalValueLocked` if a withdrawal happened for a pool involved in the + /// staking withdraw. + fn on_withdraw(pool_account: &T::AccountId, amount: BalanceOf) { + if ReversePoolIdLookup::::get(pool_account).is_some() { + TotalValueLocked::::mutate(|tvl| { + tvl.saturating_reduce(amount); + }); + } + } } diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index 559baf76e4c6..6887fcfa7eca 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -56,6 +56,59 @@ pub mod versioned { >; } +pub mod unversioned { + use super::*; + + /// Checks and updates `TotalValueLocked` if out of sync. + pub struct TotalValueLockedSync(sp_std::marker::PhantomData); + impl OnRuntimeUpgrade for TotalValueLockedSync { + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, TryRuntimeError> { + Ok(Vec::new()) + } + + fn on_runtime_upgrade() -> Weight { + let migrated = BondedPools::::count(); + + // recalcuate the `TotalValueLocked` to compare with the current on-chain TVL which may + // be out of sync. + let tvl: BalanceOf = helpers::calculate_tvl_by_total_stake::(); + let onchain_tvl = TotalValueLocked::::get(); + + let writes = if tvl != onchain_tvl { + TotalValueLocked::::set(tvl); + + log!( + info, + "on-chain TVL was out of sync, update. Old: {:?}, new: {:?}", + onchain_tvl, + tvl + ); + + // writes: onchain version + set total value locked. + 2 + } else { + log!(info, "on-chain TVL was OK: {:?}", tvl); + + // writes: onchain version write. + 1 + }; + + // reads: migrated * (BondedPools + Staking::total_stake) + count + onchain + // version + // + // writes: current version + (maybe) TVL + T::DbWeight::get() + .reads_writes(migrated.saturating_mul(2).saturating_add(2).into(), writes) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(_: Vec) -> Result<(), TryRuntimeError> { + Ok(()) + } + } +} + pub mod v8 { use super::{v7::V7BondedPoolInner, *}; @@ -146,6 +199,7 @@ pub(crate) mod v7 { } impl V7BondedPool { + #[allow(dead_code)] fn bonded_account(&self) -> T::AccountId { Pallet::::create_bonded_account(self.id) } @@ -157,26 +211,12 @@ pub(crate) mod v7 { CountedStorageMap, Twox64Concat, PoolId, V7BondedPoolInner>; 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( - &V7BondedPool { 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(); + let tvl: BalanceOf = helpers::calculate_tvl_by_total_stake::(); TotalValueLocked::::set(tvl); @@ -198,7 +238,7 @@ pub(crate) mod v7 { 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(); + let tvl: BalanceOf = helpers::calculate_tvl_by_total_stake::(); ensure!( TotalValueLocked::::get() == tvl, "TVL written is not equal to `Staking::total_stake` of all `BondedPools`." @@ -977,3 +1017,17 @@ pub mod v1 { } } } + +mod helpers { + use super::*; + + pub(crate) 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() + } +} diff --git a/substrate/frame/nomination-pools/src/mock.rs b/substrate/frame/nomination-pools/src/mock.rs index 84c41a15b201..f982b72c6356 100644 --- a/substrate/frame/nomination-pools/src/mock.rs +++ b/substrate/frame/nomination-pools/src/mock.rs @@ -134,11 +134,24 @@ impl sp_staking::StakingInterface for StakingMock { fn withdraw_unbonded(who: Self::AccountId, _: u32) -> Result { let mut unbonding_map = UnbondingBalanceMap::get(); + + // closure to calculate the current unlocking funds across all eras/accounts. + let unlocking = |pair: &Vec<(EraIndex, Balance)>| -> Balance { + pair.iter() + .try_fold(Zero::zero(), |acc: Balance, (_at, amount)| acc.checked_add(*amount)) + .unwrap() + }; + let staker_map = unbonding_map.get_mut(&who).ok_or("Nothing to unbond")?; + let unlocking_before = unlocking(&staker_map); let current_era = Self::current_era(); + staker_map.retain(|(unlocking_at, _amount)| *unlocking_at > current_era); + // if there was a withdrawal, notify the pallet. + Pools::on_withdraw(&who, unlocking_before.saturating_sub(unlocking(&staker_map))); + UnbondingBalanceMap::set(&unbonding_map); Ok(UnbondingBalanceMap::get().is_empty() && BondedBalanceMap::get().is_empty()) } diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 8b45430c688e..165972a0560b 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -41,7 +41,7 @@ use sp_runtime::{ use sp_staking::{ currency_to_vote::CurrencyToVote, offence::{DisableStrategy, OffenceDetails, OnOffenceHandler}, - EraIndex, Page, SessionIndex, Stake, + EraIndex, OnStakingUpdate, Page, SessionIndex, Stake, StakingAccount::{self, Controller, Stash}, StakingInterface, }; @@ -150,6 +150,9 @@ impl Pallet { // Already checked that this won't overflow by entry condition. let value = old_total.defensive_saturating_sub(new_total); Self::deposit_event(Event::::Withdrawn { stash, amount: value }); + + // notify listeners. + T::EventListeners::on_withdraw(controller, value); } Ok(used_weight) diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index 0689418d02bd..65e981058e6f 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -275,7 +275,7 @@ pub mod pallet { /// Something that listens to staking updates and performs actions based on the data it /// receives. /// - /// WARNING: this only reports slashing events for the time being. + /// WARNING: this only reports slashing and withdraw events for the time being. type EventListeners: sp_staking::OnStakingUpdate>; /// Some parameters of the benchmarking. diff --git a/substrate/primitives/staking/src/lib.rs b/substrate/primitives/staking/src/lib.rs index c2ac5ae004b1..f5b4a1ed63fb 100644 --- a/substrate/primitives/staking/src/lib.rs +++ b/substrate/primitives/staking/src/lib.rs @@ -150,6 +150,9 @@ pub trait OnStakingUpdate { _slashed_total: Balance, ) { } + + /// Fired when a portion of a staker's balance has been withdrawn. + fn on_withdraw(_stash: &AccountId, _amount: Balance) {} } /// A generic representation of a staking implementation.