From 56748716b511f9bc8abf8db6f61822983d4fcdb3 Mon Sep 17 00:00:00 2001 From: kris Date: Thu, 2 May 2024 14:34:28 -0500 Subject: [PATCH 1/7] Update conditions for reaping stashing account The stashing account reaping conditions have been updated to include cases where the total balance or the ledger total is zero. Previously, checks were only performed for totals falling under the existential deposit. This expanded reaping condition allows accounts with zero balances to be handled more efficiently. --- substrate/frame/staking/src/pallet/impls.rs | 3 ++- substrate/frame/staking/src/pallet/mod.rs | 9 +++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 5b2a55303e2c..d7d559dc93e4 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 == 0u32.into()) { // 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 16ad510c562b..b673c7182d37 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -1615,6 +1615,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 +1641,12 @@ 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 == 0u32.into() || + ledger_total < ed || + ledger_total == 0u32.into(); ensure!(reapable, Error::::FundedTarget); // Remove all staking-related information and lock. From a06bfe5aea318a6c7ad165e8492a40458828500c Mon Sep 17 00:00:00 2001 From: kris Date: Thu, 2 May 2024 18:08:31 -0500 Subject: [PATCH 2/7] Added tests and functionality for zero-existential-deposit staking scenario This commit adds two tests - `reap_stash_works_with_existential_deposit_zero` and `do_withdraw_unbonded_can_kill_stash_with_existential_deposit_zero` - to check staking functionality when existential deposit is set to zero. Also, updated the code to use `is_zero` method for zero balance check, making it more semantically clear and aligning it with other parts of the codebase. This should make the staking module more robust and easier to understand. --- substrate/frame/staking/src/pallet/impls.rs | 2 +- substrate/frame/staking/src/pallet/mod.rs | 4 +- substrate/frame/staking/src/tests.rs | 91 +++++++++++++++++++++ 3 files changed, 94 insertions(+), 3 deletions(-) diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index d7d559dc93e4..52361c6ccdc1 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -200,7 +200,7 @@ impl Pallet { let ed = T::Currency::minimum_balance(); let used_weight = - if ledger.unlocking.is_empty() && (ledger.active < ed || ledger.active == 0u32.into()) { + 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 b673c7182d37..1c8eed8bd009 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -1644,9 +1644,9 @@ pub mod pallet { 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 == 0u32.into() || + origin_balance.is_zero() || ledger_total < ed || - ledger_total == 0u32.into(); + 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 3cb51604aa6b..862ed7b61954 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)); + 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(|| { From f1f3bd7c0d6d905ad98b0f2692a8d0bc522416b0 Mon Sep 17 00:00:00 2001 From: kris Date: Thu, 2 May 2024 20:37:10 -0500 Subject: [PATCH 3/7] added prdoc for pr 4364 --- prdoc/pr_4364.prdoc | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 prdoc/pr_4364.prdoc diff --git a/prdoc/pr_4364.prdoc b/prdoc/pr_4364.prdoc new file mode 100644 index 000000000000..ab4e412df284 --- /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 From 9cc9cc5dbf808a0c6df3fe1f78662b70b80177ba Mon Sep 17 00:00:00 2001 From: kris Date: Thu, 2 May 2024 20:42:32 -0500 Subject: [PATCH 4/7] updated doc comment for `bond` --- substrate/frame/staking/src/pallet/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index 1c8eed8bd009..dc6be52b7d0a 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -938,7 +938,7 @@ 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( From 2a7861b3baf173779b316d61ddeb6ac160246257 Mon Sep 17 00:00:00 2001 From: kris Date: Thu, 2 May 2024 21:03:14 -0500 Subject: [PATCH 5/7] updated prdoc 4364 for schema --- prdoc/pr_4364.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_4364.prdoc b/prdoc/pr_4364.prdoc index ab4e412df284..88ee8d12286d 100644 --- a/prdoc/pr_4364.prdoc +++ b/prdoc/pr_4364.prdoc @@ -1,4 +1,4 @@ -title: Fix: dust unbonded for zero existential deposit +title: Fix dust unbonded for zero existential deposit doc: - audience: Runtime Dev From 771165188c51ee483529c89f0efa7df10ad6e2a3 Mon Sep 17 00:00:00 2001 From: kris Date: Thu, 2 May 2024 21:13:44 -0500 Subject: [PATCH 6/7] unwrap result in new test --- substrate/frame/staking/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 862ed7b61954..ef405a4d8048 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -7015,7 +7015,7 @@ mod staking_interface { ); // Unbond all of the funds in stash. - Staking::chill(RuntimeOrigin::signed(11)); + Staking::chill(RuntimeOrigin::signed(11)).unwrap(); Staking::unbond(RuntimeOrigin::signed(11), 1000).unwrap(); assert_eq!( Staking::ledger(11.into()).unwrap(), From 6061eac4d7735edb1dd58d7599b323ed683fe98c Mon Sep 17 00:00:00 2001 From: command-bot <> Date: Fri, 3 May 2024 11:42:39 +0000 Subject: [PATCH 7/7] ".git/.scripts/commands/fmt/fmt.sh" --- substrate/frame/staking/src/pallet/mod.rs | 6 +- substrate/frame/staking/src/tests.rs | 82 +++++++++++------------ 2 files changed, 45 insertions(+), 43 deletions(-) diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index dc6be52b7d0a..f82266c03903 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_ (or equal to 0) 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( @@ -1642,7 +1643,8 @@ pub mod pallet { let ed = T::Currency::minimum_balance(); let origin_balance = T::Currency::total_balance(&stash); - let ledger_total = Self::ledger(Stash(stash.clone())).map(|l| l.total).unwrap_or_default(); + 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 || diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index ef405a4d8048..76afa3333cb4 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -6997,51 +6997,51 @@ mod staking_interface { .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![] } - ); + // 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![], - }, - ); + // 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); + // trigger future era. + mock::start_active_era(3); - // withdraw unbonded - assert_ok!(Staking::withdraw_unbonded(RuntimeOrigin::signed(11), 0)); + // 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); - }); + // 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]