From 4920ce5a61860cfbb5cebac5d4497ad316b02ec1 Mon Sep 17 00:00:00 2001
From: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Date: Sun, 14 Nov 2021 15:04:20 +0000
Subject: [PATCH] rework `staking::reap_stash` (#10178)
* rework reap_stash
* Update frame/staking/src/pallet/mod.rs
Co-authored-by: Zeke Mostov <z.mostov@gmail.com>
* Update frame/staking/src/pallet/mod.rs
Co-authored-by: Zeke Mostov <z.mostov@gmail.com>
* Update frame/staking/src/pallet/mod.rs
Co-authored-by: Zeke Mostov <z.mostov@gmail.com>
* Fix
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-authored-by: Zeke Mostov <z.mostov@gmail.com>
---
substrate/frame/staking/src/benchmarking.rs | 5 +-
substrate/frame/staking/src/pallet/mod.rs | 40 ++++---
substrate/frame/staking/src/tests.rs | 118 +++++---------------
3 files changed, 50 insertions(+), 113 deletions(-)
diff --git a/substrate/frame/staking/src/benchmarking.rs b/substrate/frame/staking/src/benchmarking.rs
index 80630818de7..e312aedbec1 100644
--- a/substrate/frame/staking/src/benchmarking.rs
+++ b/substrate/frame/staking/src/benchmarking.rs
@@ -28,7 +28,7 @@ use frame_support::{
traits::{Currency, CurrencyToVote, Get, Imbalance},
};
use sp_runtime::{
- traits::{StaticLookup, Zero},
+ traits::{Bounded, One, StaticLookup, Zero},
Perbill, Percent,
};
use sp_staking::SessionIndex;
@@ -38,7 +38,6 @@ pub use frame_benchmarking::{
account, benchmarks, impl_benchmark_test_suite, whitelist_account, whitelisted_caller,
};
use frame_system::RawOrigin;
-use sp_runtime::traits::{Bounded, One};
const SEED: u32 = 0;
const MAX_SPANS: u32 = 100;
@@ -695,7 +694,7 @@ benchmarks! {
let stash = scenario.origin_stash1.clone();
add_slashing_spans::<T>(&stash, s);
- T::Currency::make_free_balance_be(&stash, T::Currency::minimum_balance());
+ Ledger::<T>::insert(&controller, StakingLedger { active: T::Currency::minimum_balance() - One::one(), total: T::Currency::minimum_balance() - One::one(), ..Default::default() });
assert!(Bonded::<T>::contains_key(&stash));
assert!(T::SortedListProvider::contains(&stash));
diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs
index 8e97a90e075..197c2eed325 100644
--- a/substrate/frame/staking/src/pallet/mod.rs
+++ b/substrate/frame/staking/src/pallet/mod.rs
@@ -1425,33 +1425,37 @@ pub mod pallet {
Ok(())
}
- /// Remove all data structure concerning a staker/stash once its balance is at the minimum.
- /// This is essentially equivalent to `withdraw_unbonded` except it can be called by anyone
- /// and the target `stash` must have no funds left beyond the ED.
+ /// 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:
///
- /// This can be called from any origin.
+ /// 1. the `total_balance` of the stash is below existential deposit.
+ /// 2. or, the `ledger.total` of the stash is below existential deposit.
///
- /// - `stash`: The stash account to reap. Its balance must be 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`.
///
- /// # <weight>
- /// Complexity: O(S) where S is the number of slashing spans on the account.
- /// DB Weight:
- /// - Reads: Stash Account, Bonded, Slashing Spans, Locks
- /// - Writes: Bonded, Slashing Spans (if S > 0), Ledger, Payee, Validators, Nominators,
- /// Stash Account, Locks
- /// - Writes Each: SpanSlash * S
- /// # </weight>
+ /// It can be called by anyone, as long as `stash` meets the above requirements.
+ ///
+ /// Refunds the transaction fees upon successful execution.
#[pallet::weight(T::WeightInfo::reap_stash(*num_slashing_spans))]
pub fn reap_stash(
- _origin: OriginFor<T>,
+ origin: OriginFor<T>,
stash: T::AccountId,
num_slashing_spans: u32,
- ) -> DispatchResult {
- let at_minimum = T::Currency::total_balance(&stash) == T::Currency::minimum_balance();
- ensure!(at_minimum, Error::<T>::FundedTarget);
+ ) -> DispatchResultWithPostInfo {
+ let _ = ensure_signed(origin)?;
+
+ let ed = T::Currency::minimum_balance();
+ let reapable = T::Currency::total_balance(&stash) < ed ||
+ Self::ledger(Self::bonded(stash.clone()).ok_or(Error::<T>::NotStash)?)
+ .map(|l| l.total)
+ .unwrap_or_default() < ed;
+ ensure!(reapable, Error::<T>::FundedTarget);
+
Self::kill_stash(&stash, num_slashing_spans)?;
T::Currency::remove_lock(STAKING_ID, &stash);
- Ok(())
+
+ Ok(Pays::No.into())
}
/// Remove the given nominations from the calling validator.
diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs
index d6d92d5bd57..8e8a7ee636d 100644
--- a/substrate/frame/staking/src/tests.rs
+++ b/substrate/frame/staking/src/tests.rs
@@ -1633,115 +1633,49 @@ fn reward_to_stake_works() {
}
#[test]
-fn on_free_balance_zero_stash_removes_validator() {
- // Tests that validator storage items are cleaned up when stash is empty
- // Tests that storage items are untouched when controller is empty
+fn reap_stash_works() {
ExtBuilder::default()
.existential_deposit(10)
.balance_factor(10)
.build_and_execute(|| {
- // Check the balance of the validator account
+ // given
assert_eq!(Balances::free_balance(10), 10);
- // Check the balance of the stash account
- assert_eq!(Balances::free_balance(11), 10 * 1000);
- // Check these two accounts are bonded
- assert_eq!(Staking::bonded(&11), Some(10));
-
- // Set payee information
- assert_ok!(Staking::set_payee(Origin::signed(10), RewardDestination::Stash));
-
- // Check storage items that should be cleaned up
- assert!(<Ledger<Test>>::contains_key(&10));
- assert!(<Bonded<Test>>::contains_key(&11));
- assert!(<Validators<Test>>::contains_key(&11));
- assert!(<Payee<Test>>::contains_key(&11));
-
- // Reduce free_balance of controller to 0
- let _ = Balances::slash(&10, Balance::max_value());
-
- // Check the balance of the stash account has not been touched
assert_eq!(Balances::free_balance(11), 10 * 1000);
- // Check these two accounts are still bonded
assert_eq!(Staking::bonded(&11), Some(10));
- // Check storage items have not changed
assert!(<Ledger<Test>>::contains_key(&10));
assert!(<Bonded<Test>>::contains_key(&11));
assert!(<Validators<Test>>::contains_key(&11));
assert!(<Payee<Test>>::contains_key(&11));
- // Reduce free_balance of stash to 0
- let _ = Balances::slash(&11, Balance::max_value());
- // Check total balance of stash
- assert_eq!(Balances::total_balance(&11), 10);
-
- // Reap the stash
- assert_ok!(Staking::reap_stash(Origin::none(), 11, 0));
-
- // Check storage items do not exist
- assert!(!<Ledger<Test>>::contains_key(&10));
- assert!(!<Bonded<Test>>::contains_key(&11));
- assert!(!<Validators<Test>>::contains_key(&11));
- assert!(!<Nominators<Test>>::contains_key(&11));
- assert!(!<Payee<Test>>::contains_key(&11));
- });
-}
-
-#[test]
-fn on_free_balance_zero_stash_removes_nominator() {
- // Tests that nominator storage items are cleaned up when stash is empty
- // Tests that storage items are untouched when controller is empty
- ExtBuilder::default()
- .existential_deposit(10)
- .balance_factor(10)
- .build_and_execute(|| {
- // Make 10 a nominator
- assert_ok!(Staking::nominate(Origin::signed(10), vec![20]));
- // Check that account 10 is a nominator
- assert!(<Nominators<Test>>::contains_key(11));
- // Check the balance of the nominator account
- assert_eq!(Balances::free_balance(10), 10);
- // Check the balance of the stash account
- assert_eq!(Balances::free_balance(11), 10_000);
-
- // Set payee information
- assert_ok!(Staking::set_payee(Origin::signed(10), RewardDestination::Stash));
-
- // Check storage items that should be cleaned up
- assert!(<Ledger<Test>>::contains_key(&10));
- assert!(<Bonded<Test>>::contains_key(&11));
- assert!(<Nominators<Test>>::contains_key(&11));
- assert!(<Payee<Test>>::contains_key(&11));
-
- // Reduce free_balance of controller to 0
- let _ = Balances::slash(&10, Balance::max_value());
- // Check total balance of account 10
- assert_eq!(Balances::total_balance(&10), 0);
-
- // Check the balance of the stash account has not been touched
- assert_eq!(Balances::free_balance(11), 10_000);
- // Check these two accounts are still bonded
- assert_eq!(Staking::bonded(&11), Some(10));
-
- // Check storage items have not changed
- assert!(<Ledger<Test>>::contains_key(&10));
- assert!(<Bonded<Test>>::contains_key(&11));
- assert!(<Nominators<Test>>::contains_key(&11));
- assert!(<Payee<Test>>::contains_key(&11));
+ // stash is not reapable
+ assert_noop!(
+ Staking::reap_stash(Origin::signed(20), 11, 0),
+ Error::<Test>::FundedTarget
+ );
+ // controller or any other account is not reapable
+ assert_noop!(Staking::reap_stash(Origin::signed(20), 10, 0), Error::<Test>::NotStash);
- // Reduce free_balance of stash to 0
- let _ = Balances::slash(&11, Balance::max_value());
- // Check total balance of stash
- assert_eq!(Balances::total_balance(&11), 10);
+ // no easy way to cause an account to go below ED, we tweak their staking ledger
+ // instead.
+ Ledger::<Test>::insert(
+ 10,
+ StakingLedger {
+ stash: 11,
+ total: 5,
+ active: 5,
+ unlocking: vec![],
+ claimed_rewards: vec![],
+ },
+ );
- // Reap the stash
- assert_ok!(Staking::reap_stash(Origin::none(), 11, 0));
+ // reap-able
+ assert_ok!(Staking::reap_stash(Origin::signed(20), 11, 0));
- // Check storage items do not exist
+ // then
assert!(!<Ledger<Test>>::contains_key(&10));
assert!(!<Bonded<Test>>::contains_key(&11));
assert!(!<Validators<Test>>::contains_key(&11));
- assert!(!<Nominators<Test>>::contains_key(&11));
assert!(!<Payee<Test>>::contains_key(&11));
});
}
@@ -2556,10 +2490,10 @@ fn garbage_collection_after_slashing() {
// reap_stash respects num_slashing_spans so that weight is accurate
assert_noop!(
- Staking::reap_stash(Origin::none(), 11, 0),
+ Staking::reap_stash(Origin::signed(20), 11, 0),
Error::<Test>::IncorrectSlashingSpans
);
- assert_ok!(Staking::reap_stash(Origin::none(), 11, 2));
+ assert_ok!(Staking::reap_stash(Origin::signed(20), 11, 2));
assert!(<Staking as crate::Store>::SlashingSpans::get(&11).is_none());
assert_eq!(<Staking as crate::Store>::SpanSlash::get(&(11, 0)).amount_slashed(), &0);
--
GitLab