diff --git a/prdoc/pr_9415.prdoc b/prdoc/pr_9415.prdoc new file mode 100644 index 0000000000000..f16ff64d8fd49 --- /dev/null +++ b/prdoc/pr_9415.prdoc @@ -0,0 +1,9 @@ +title: Cleanup staking try states + fix min bonds +doc: +- audience: Runtime Dev + description: ensures staking try-state code passes, and fixes issues of having a mistakenly high min-bond for validators +crates: +- name: pallet-nomination-pools + bump: patch +- name: pallet-staking-async + bump: patch diff --git a/substrate/frame/nomination-pools/benchmarking/src/inner.rs b/substrate/frame/nomination-pools/benchmarking/src/inner.rs index 10bc129f9acc5..d71022657576f 100644 --- a/substrate/frame/nomination-pools/benchmarking/src/inner.rs +++ b/substrate/frame/nomination-pools/benchmarking/src/inner.rs @@ -966,14 +966,14 @@ mod benchmarks { // Remove ed freeze to create a scenario where the ed deposit needs to be adjusted. let _ = Pools::::unfreeze_pool_deposit(&Pools::::generate_reward_account(1)); - assert!(&Pools::::check_ed_imbalance().is_err()); + assert_eq!(Pools::::check_ed_imbalance().unwrap(), 1); whitelist_account!(depositor); #[extrinsic_call] _(RuntimeOrigin::Signed(depositor), 1); - assert!(&Pools::::check_ed_imbalance().is_ok()); + assert_eq!(Pools::::check_ed_imbalance().unwrap(), 0); } #[benchmark] diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 2bb2e0a325388..8b6fdee58bffe 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -3930,7 +3930,8 @@ impl Pallet { // If this happens, this is most likely due to an old bug and not a recent code change. // We warn about this in try-runtime checks but do not panic. if !pending_rewards_lt_leftover_bal { - log::warn!( + log!( + warn, "pool {:?}, sum pending rewards = {:?}, remaining balance = {:?}", id, pools_members_pending_rewards.get(&id), @@ -3955,12 +3956,19 @@ impl Pallet { ); let depositor = PoolMembers::::get(&bonded_pool.roles.depositor).unwrap(); - ensure!( - bonded_pool.is_destroying_and_only_depositor(depositor.active_points()) || - depositor.active_points() >= MinCreateBond::::get(), - "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 depositor_has_enough_stake = bonded_pool + .is_destroying_and_only_depositor(depositor.active_points()) || + depositor.active_points() >= MinCreateBond::::get(); + if !depositor_has_enough_stake { + log!( + warn, + "pool {:?} has depositor {:?} with insufficient stake {:?}, minimum required is {:?}", + id, + bonded_pool.roles.depositor, + depositor.active_points(), + MinCreateBond::::get() + ); + } ensure!( bonded_pool.points >= bonded_pool.points_to_balance(bonded_pool.points), @@ -4018,7 +4026,7 @@ impl Pallet { // Warn if any pool has incorrect ED frozen. We don't want to fail hard as this could be a // result of an intentional ED change. - Self::check_ed_imbalance()?; + let _needs_adjust = Self::check_ed_imbalance()?; Ok(()) } @@ -4033,8 +4041,8 @@ impl Pallet { test, debug_assertions ))] - pub fn check_ed_imbalance() -> Result<(), DispatchError> { - let mut failed: u32 = 0; + pub fn check_ed_imbalance() -> Result { + let mut needs_adjust = 0; BondedPools::::iter_keys().for_each(|id| { let reward_acc = Self::generate_reward_account(id); let frozen_balance = @@ -4042,9 +4050,10 @@ impl Pallet { let expected_frozen_balance = T::Currency::minimum_balance(); if frozen_balance != expected_frozen_balance { - failed += 1; - log::warn!( - "pool {:?} has incorrect ED frozen that can result from change in ED. Expected = {:?}, Actual = {:?}", + needs_adjust += 1; + log!( + warn, + "pool {:?} has incorrect ED frozen that can result from change in ED. Expected = {:?}, Actual = {:?}. Use `adjust_pool_deposit` to fix it", id, expected_frozen_balance, frozen_balance, @@ -4052,8 +4061,7 @@ impl Pallet { } }); - ensure!(failed == 0, "Some pools do not have correct ED frozen"); - Ok(()) + Ok(needs_adjust) } /// Fully unbond the shares of `member`, when executed from `origin`. /// diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index d8697364a76c5..a0e5b45b88cf7 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -442,7 +442,7 @@ mod v6 { #[cfg(feature = "try-runtime")] fn post_upgrade(_data: Vec) -> Result<(), TryRuntimeError> { // there should be no ED imbalances anymore.. - Pallet::::check_ed_imbalance() + Pallet::::check_ed_imbalance().map(|_| ()) } } } diff --git a/substrate/frame/staking-async/src/pallet/impls.rs b/substrate/frame/staking-async/src/pallet/impls.rs index e07daa4b80644..13bc247bcd6a0 100644 --- a/substrate/frame/staking-async/src/pallet/impls.rs +++ b/substrate/frame/staking-async/src/pallet/impls.rs @@ -85,14 +85,12 @@ impl Pallet { .max(asset::existential_deposit::()) } - /// Returns the minimum required bond for validators, defaulting to `MinNominatorBond` if not - /// set or less than `MinNominatorBond`. + /// Returns the minimum required bond for participation in staking as a validator account. pub(crate) fn min_validator_bond() -> BalanceOf { - MinValidatorBond::::get().max(Self::min_nominator_bond()) + MinValidatorBond::::get().max(asset::existential_deposit::()) } - /// Returns the minimum required bond for nominators, considering the chain’s existential - /// deposit. + /// Returns the minimum required bond for participation in staking as a nominator account. pub(crate) fn min_nominator_bond() -> BalanceOf { MinNominatorBond::::get().max(asset::existential_deposit::()) } @@ -2086,21 +2084,40 @@ impl Pallet { match (is_nominator, is_validator) { (false, false) => { - if ledger.active < Self::min_chilled_bond() { - log!(warn, "Chilled stash {:?} has less than minimum bond", stash); + if ledger.active < Self::min_chilled_bond() && !ledger.active.is_zero() { + // chilled accounts allow to go to zero and fully unbond ^^^^^^^^^ + log!( + warn, + "Chilled stash {:?} has less stake ({:?}) than minimum role bond ({:?})", + stash, + ledger.active, + Self::min_chilled_bond() + ); } // is chilled }, (true, false) => { // Nominators must have a minimum bond. if ledger.active < Self::min_nominator_bond() { - log!(warn, "Nominator {:?} has less than minimum bond", stash); + log!( + warn, + "Nominator {:?} has less stake ({:?}) than minimum role bond ({:?})", + stash, + ledger.active, + Self::min_nominator_bond() + ); } }, (false, true) => { // Validators must have a minimum bond. if ledger.active < Self::min_validator_bond() { - log!(warn, "Validator {:?} has less than minimum bond", stash); + log!( + warn, + "Validator {:?} has less stake ({:?}) than minimum role bond ({:?})", + stash, + ledger.active, + Self::min_validator_bond() + ); } }, (true, true) => { diff --git a/substrate/frame/staking-async/src/pallet/mod.rs b/substrate/frame/staking-async/src/pallet/mod.rs index 1eaf56e58328f..80efe449ede59 100644 --- a/substrate/frame/staking-async/src/pallet/mod.rs +++ b/substrate/frame/staking-async/src/pallet/mod.rs @@ -2026,9 +2026,8 @@ pub mod pallet { /// Remove all data structures concerning a staker/stash once it is at a state where it can /// be considered `dust` in the staking system. The requirements are: /// - /// 1. the `total_balance` of the stash is below minimum bond. - /// 2. or, the `ledger.total` of the stash is below minimum bond. - /// 3. or, existential deposit is zero and either `total_balance` or `ledger.total` is zero. + /// 1. the `total_balance` of the stash is below `min_chilled_bond` or is zero. + /// 2. or, the `ledger.total` of the stash is below `min_chilled_bond` or is zero. /// /// The former can happen in cases like a slash; the latter when a fully unbonded account /// is still receiving staking rewards in `RewardDestination::Staked`. diff --git a/substrate/frame/staking-async/src/tests/bonding.rs b/substrate/frame/staking-async/src/tests/bonding.rs index 3f8b66ff3f774..5da3e7d45fead 100644 --- a/substrate/frame/staking-async/src/tests/bonding.rs +++ b/substrate/frame/staking-async/src/tests/bonding.rs @@ -1387,8 +1387,6 @@ mod reap { Error::::FundedTarget ); - // Note: Even though the stash is a validator, the threshold to reap is min of - // nominator and validator bond // no easy way to cause an account to go below ED, we tweak their staking ledger // instead. @@ -1401,8 +1399,8 @@ mod reap { Error::::FundedTarget ); - // WHEN: set ledger to below min nominator bond. - Ledger::::insert(11, StakingLedger::::new(11, 999)); + // WHEN: set ledger to below ED + Ledger::::insert(11, StakingLedger::::new(11, 9)); // THEN: reap-able assert_ok!(Staking::reap_stash(RuntimeOrigin::signed(20), 11, 0)); @@ -1551,9 +1549,9 @@ mod staking_bounds_chill_other { .min_nominator_bond(1_000) .min_validator_bond(1_500) .build_and_execute(|| { - // 500 is not enough for any role + // 50 is not enough for any role (less than ED) assert_noop!( - Staking::bond(RuntimeOrigin::signed(3), 500, RewardDestination::Stash), + Staking::bond(RuntimeOrigin::signed(3), 50, RewardDestination::Stash), Error::::InsufficientBond ); // 1000 is enough for nominator but not for validator.