From 6b955ce74d47c99c33c0093b1b1432c5a3642b22 Mon Sep 17 00:00:00 2001 From: Kris Bitney Date: Fri, 3 May 2024 07:31:45 -0500 Subject: [PATCH] Fix: dust unbonded for zero existential deposit (#4364) When a staker unbonds and withdraws, it is possible that their stash will contain less currency than the existential deposit. If that happens, their stash is reaped. But if the existential deposit is zero, the reap is not triggered. This PR adjusts `pallet_staking` to reap a stash in the special case that the stash value is zero and the existential deposit is zero. This change is important for blockchains built on substrate that require an existential deposit of zero, becuase it conserves valued storage space. There are two places in which ledgers are checked to determine if their value is less than the existential deposit and they should be reaped: in the methods `do_withdraw_unbonded` and `reap_stash`. When the check is made, the condition `ledger_total == 0` is also checked. If `ledger_total` is zero, then it must be below any existential deposit greater than zero and equal to an existential deposit of 0. I added a new test for each method to confirm the change behaves as expected. Closes https://github.com/paritytech/polkadot-sdk/issues/4340 --------- Co-authored-by: command-bot <> --- prdoc/pr_4364.prdoc | 10 +++ substrate/frame/staking/src/pallet/impls.rs | 3 +- substrate/frame/staking/src/pallet/mod.rs | 13 ++- substrate/frame/staking/src/tests.rs | 91 +++++++++++++++++++++ 4 files changed, 113 insertions(+), 4 deletions(-) create mode 100644 prdoc/pr_4364.prdoc diff --git a/prdoc/pr_4364.prdoc b/prdoc/pr_4364.prdoc new file mode 100644 index 0000000000000..88ee8d12286db --- /dev/null +++ b/prdoc/pr_4364.prdoc @@ -0,0 +1,10 @@ +title: Fix dust unbonded for zero existential deposit + +doc: + - audience: Runtime Dev + description: | + When a staker unbonds and withdraws, it is possible that their stash will contain less currency than the existential deposit. If that happens, their stash is reaped. But if the existential deposit is zero, the reap is not triggered. This PR adjusts pallet_staking to reap a stash in the special case that the stash value is zero and the existential deposit is zero. + +crates: + - name: pallet-staking + bump: patch diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 5b2a55303e2c8..52361c6ccdc13 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -198,8 +198,9 @@ impl Pallet { } let new_total = ledger.total; + let ed = T::Currency::minimum_balance(); let used_weight = - if ledger.unlocking.is_empty() && ledger.active < T::Currency::minimum_balance() { + if ledger.unlocking.is_empty() && (ledger.active < ed || ledger.active.is_zero()) { // This account must have called `unbond()` with some value that caused the active // portion to fall below existential deposit + will have no more unlocking chunks // left. We can now safely remove all staking-related information. diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index 16ad510c562bf..f82266c03903c 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -938,7 +938,8 @@ pub mod pallet { /// - Three extra DB entries. /// /// NOTE: Two of the storage writes (`Self::bonded`, `Self::payee`) are _never_ cleaned - /// unless the `origin` falls below _existential deposit_ and gets removed as dust. + /// unless the `origin` falls below _existential deposit_ (or equal to 0) and gets removed + /// as dust. #[pallet::call_index(0)] #[pallet::weight(T::WeightInfo::bond())] pub fn bond( @@ -1615,6 +1616,7 @@ pub mod pallet { /// /// 1. the `total_balance` of the stash is below existential deposit. /// 2. or, the `ledger.total` of the stash is below existential deposit. + /// 3. or, existential deposit is zero and either `total_balance` or `ledger.total` 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`. @@ -1640,8 +1642,13 @@ pub mod pallet { ensure!(!Self::is_virtual_staker(&stash), Error::::VirtualStakerNotAllowed); let ed = T::Currency::minimum_balance(); - let reapable = T::Currency::total_balance(&stash) < ed || - Self::ledger(Stash(stash.clone())).map(|l| l.total).unwrap_or_default() < ed; + let origin_balance = T::Currency::total_balance(&stash); + let ledger_total = + Self::ledger(Stash(stash.clone())).map(|l| l.total).unwrap_or_default(); + let reapable = origin_balance < ed || + origin_balance.is_zero() || + ledger_total < ed || + ledger_total.is_zero(); ensure!(reapable, Error::::FundedTarget); // Remove all staking-related information and lock. diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 3cb51604aa6bb..76afa3333cb46 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -1931,6 +1931,44 @@ fn reap_stash_works() { }); } +#[test] +fn reap_stash_works_with_existential_deposit_zero() { + ExtBuilder::default() + .existential_deposit(0) + .balance_factor(10) + .build_and_execute(|| { + // given + assert_eq!(Balances::balance_locked(STAKING_ID, &11), 10 * 1000); + assert_eq!(Staking::bonded(&11), Some(11)); + + assert!(>::contains_key(&11)); + assert!(>::contains_key(&11)); + assert!(>::contains_key(&11)); + assert!(>::contains_key(&11)); + + // stash is not reapable + assert_noop!( + Staking::reap_stash(RuntimeOrigin::signed(20), 11, 0), + Error::::FundedTarget + ); + + // no easy way to cause an account to go below ED, we tweak their staking ledger + // instead. + Ledger::::insert(11, StakingLedger::::new(11, 0)); + + // reap-able + assert_ok!(Staking::reap_stash(RuntimeOrigin::signed(20), 11, 0)); + + // then + assert!(!>::contains_key(&11)); + assert!(!>::contains_key(&11)); + assert!(!>::contains_key(&11)); + assert!(!>::contains_key(&11)); + // lock is removed. + assert_eq!(Balances::balance_locked(STAKING_ID, &11), 0); + }); +} + #[test] fn switching_roles() { // Test that it should be possible to switch between roles (nominator, validator, idle) with @@ -6953,6 +6991,59 @@ mod staking_interface { }); } + #[test] + fn do_withdraw_unbonded_can_kill_stash_with_existential_deposit_zero() { + ExtBuilder::default() + .existential_deposit(0) + .nominate(false) + .build_and_execute(|| { + // Initial state of 11 + assert_eq!(Staking::bonded(&11), Some(11)); + assert_eq!( + Staking::ledger(11.into()).unwrap(), + StakingLedgerInspect { + stash: 11, + total: 1000, + active: 1000, + unlocking: Default::default(), + legacy_claimed_rewards: bounded_vec![], + } + ); + assert_eq!( + Staking::eras_stakers(active_era(), &11), + Exposure { total: 1000, own: 1000, others: vec![] } + ); + + // Unbond all of the funds in stash. + Staking::chill(RuntimeOrigin::signed(11)).unwrap(); + Staking::unbond(RuntimeOrigin::signed(11), 1000).unwrap(); + assert_eq!( + Staking::ledger(11.into()).unwrap(), + StakingLedgerInspect { + stash: 11, + total: 1000, + active: 0, + unlocking: bounded_vec![UnlockChunk { value: 1000, era: 3 }], + legacy_claimed_rewards: bounded_vec![], + }, + ); + + // trigger future era. + mock::start_active_era(3); + + // withdraw unbonded + assert_ok!(Staking::withdraw_unbonded(RuntimeOrigin::signed(11), 0)); + + // empty stash has been reaped + assert!(!>::contains_key(&11)); + assert!(!>::contains_key(&11)); + assert!(!>::contains_key(&11)); + assert!(!>::contains_key(&11)); + // lock is removed. + assert_eq!(Balances::balance_locked(STAKING_ID, &11), 0); + }); + } + #[test] fn status() { ExtBuilder::default().build_and_execute(|| {