diff --git a/prdoc/pr_7738.prdoc b/prdoc/pr_7738.prdoc new file mode 100644 index 0000000000000..3b452d5e7d7fb --- /dev/null +++ b/prdoc/pr_7738.prdoc @@ -0,0 +1,15 @@ +title: 'Introduce filter to restrict accounts from staking' + +doc: + - audience: Runtime Dev + description: | + Introduce an associated fn `filter` in pallet-staking to restrict some accounts from staking. + This is useful for restricting certain accounts from staking, for example, accounts staking via pools. + +crates: + - name: pallet-staking + bump: patch + - name: pallet-nomination-pools + bump: patch + - name: pallet-delegated-staking + bump: patch diff --git a/substrate/frame/delegated-staking/src/mock.rs b/substrate/frame/delegated-staking/src/mock.rs index 811d5739f4e98..f092346bf6d54 100644 --- a/substrate/frame/delegated-staking/src/mock.rs +++ b/substrate/frame/delegated-staking/src/mock.rs @@ -20,7 +20,7 @@ use frame_support::{ assert_ok, derive_impl, pallet_prelude::*, parameter_types, - traits::{ConstU64, Currency, VariantCountOf}, + traits::{ConstU64, Contains, Currency, VariantCountOf}, PalletId, }; @@ -111,6 +111,9 @@ impl pallet_staking::Config for Runtime { type VoterList = pallet_staking::UseNominatorsAndValidatorsMap; type TargetList = pallet_staking::UseValidatorsMap; type EventListeners = (Pools, DelegatedStaking); + fn filter(who: &AccountId) -> bool { + pallet_nomination_pools::AllPoolMembers::::contains(who) + } } parameter_types! { diff --git a/substrate/frame/delegated-staking/src/tests.rs b/substrate/frame/delegated-staking/src/tests.rs index b7b82a43771eb..9521a6808c428 100644 --- a/substrate/frame/delegated-staking/src/tests.rs +++ b/substrate/frame/delegated-staking/src/tests.rs @@ -1361,7 +1361,7 @@ mod pool_integration { } #[test] - fn existing_pool_member_can_stake() { + fn existing_pool_member_cannot_stake() { // A pool member is able to stake directly since staking only uses free funds but once a // staker, they cannot join/add extra bond to the pool. They can still withdraw funds. ExtBuilder::default().build_and_execute(|| { @@ -1375,28 +1375,42 @@ mod pool_integration { fund(&delegator, 1000); assert_ok!(Pools::join(RawOrigin::Signed(delegator).into(), 200, pool_id)); - // THEN: they can still stake directly. + // THEN: they cannot stake anymore + assert_noop!( + Staking::bond( + RuntimeOrigin::signed(delegator), + 500, + RewardDestination::Account(101) + ), + StakingError::::BoundNotMet + ); + }); + } + + #[test] + fn stakers_cannot_join_pool() { + ExtBuilder::default().build_and_execute(|| { + start_era(1); + // GIVEN: a pool. + fund(&200, 1000); + let pool_id = create_pool(200, 800); + + // WHEN: an account is a staker. + let staker = 100; + fund(&staker, 1000); + assert_ok!(Staking::bond( - RuntimeOrigin::signed(delegator), + RuntimeOrigin::signed(staker), 500, RewardDestination::Account(101) )); - assert_ok!(Staking::nominate( - RuntimeOrigin::signed(delegator), - vec![GENESIS_VALIDATOR] - )); + assert_ok!(Staking::nominate(RuntimeOrigin::signed(staker), vec![GENESIS_VALIDATOR])); - // The delegator cannot add any extra bond to the pool anymore. + // THEN: they cannot join pool. assert_noop!( - Pools::bond_extra(RawOrigin::Signed(delegator).into(), BondExtra::FreeBalance(100)), + Pools::join(RawOrigin::Signed(staker).into(), 200, pool_id), Error::::AlreadyStaking ); - - // But they can unbond - assert_ok!(Pools::unbond(RawOrigin::Signed(delegator).into(), delegator, 50)); - // and withdraw - start_era(4); - assert_ok!(Pools::withdraw_unbonded(RawOrigin::Signed(delegator).into(), delegator, 0)); }); } diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 201b0af1d6080..89a1161fa29ae 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -4079,3 +4079,11 @@ impl sp_staking::OnStakingUpdate> for Pall } } } + +/// A utility struct that provides a way to check if a given account is a pool member. +pub struct AllPoolMembers(PhantomData); +impl frame_support::traits::Contains for AllPoolMembers { + fn contains(t: &T::AccountId) -> bool { + PoolMembers::::contains_key(t) + } +} diff --git a/substrate/frame/staking/Cargo.toml b/substrate/frame/staking/Cargo.toml index fa33562f070b6..da0385ebcbf0e 100644 --- a/substrate/frame/staking/Cargo.toml +++ b/substrate/frame/staking/Cargo.toml @@ -46,6 +46,7 @@ substrate-test-utils = { path = "../../test-utils" } frame-benchmarking = { default-features = true, path = "../benchmarking" } frame-election-provider-support = { default-features = true, path = "../election-provider-support" } rand_chacha = { workspace = true, default-features = true } +frame-support = { features = ["experimental"], default-features = true, workspace = true } [features] default = ["std"] diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs index 19d999109d8dd..c61e2125e0460 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/substrate/frame/staking/src/lib.rs @@ -312,7 +312,8 @@ use codec::{Decode, Encode, HasCompact, MaxEncodedLen}; use frame_support::{ defensive, defensive_assert, traits::{ - ConstU32, Currency, Defensive, DefensiveMax, DefensiveSaturating, Get, LockIdentifier, + ConstU32, Contains, Currency, Defensive, DefensiveMax, DefensiveSaturating, Get, + LockIdentifier, }, weights::Weight, BoundedVec, CloneNoBound, EqNoBound, PartialEqNoBound, RuntimeDebugNoBound, @@ -540,6 +541,52 @@ impl StakingLedger { } } + /// Sets ledger total to the `new_total`. + /// + /// Removes entries from `unlocking` upto `amount` starting from the oldest first. + fn update_total_stake(mut self, new_total: BalanceOf) -> Self { + let old_total = self.total; + self.total = new_total; + debug_assert!( + new_total <= old_total, + "new_total {:?} must be <= old_total {:?}", + new_total, + old_total + ); + + let to_withdraw = old_total.defensive_saturating_sub(new_total); + // accumulator to keep track of how much is withdrawn. + // First we take out from active. + let mut withdrawn = BalanceOf::::zero(); + + // first we try to remove stake from active + if self.active >= to_withdraw { + self.active -= to_withdraw; + return self + } else { + withdrawn += self.active; + self.active = BalanceOf::::zero(); + } + + // start removing from the oldest chunk. + while let Some(last) = self.unlocking.last_mut() { + if withdrawn.defensive_saturating_add(last.value) <= to_withdraw { + withdrawn += last.value; + self.unlocking.pop(); + } else { + let diff = to_withdraw.defensive_saturating_sub(withdrawn); + withdrawn += diff; + last.value -= diff; + } + + if withdrawn >= to_withdraw { + break + } + } + + self + } + /// Re-bond funds that were scheduled for unlocking. /// /// Returns the updated ledger, and the amount actually rebonded. @@ -1249,6 +1296,24 @@ impl EraInfo { } } +/// A utility struct that provides a way to check if a given account is a staker. +/// +/// This struct implements the `Contains` trait, allowing it to determine whether +/// a particular account is currently staking by checking if the account exists in +/// the staking ledger. +pub struct AllStakers(core::marker::PhantomData); + +impl Contains for AllStakers { + /// Checks if the given account ID corresponds to a staker. + /// + /// # Returns + /// - `true` if the account has an entry in the staking ledger (indicating it is staking). + /// - `false` otherwise. + fn contains(account: &T::AccountId) -> bool { + Ledger::::contains_key(account) + } +} + /// Configurations of the benchmarking of the pallet. pub trait BenchmarkingConfig { /// The maximum number of validators to use. diff --git a/substrate/frame/staking/src/mock.rs b/substrate/frame/staking/src/mock.rs index 4a0209fc5b083..8bd05c21b88e6 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/substrate/frame/staking/src/mock.rs @@ -241,6 +241,7 @@ parameter_types! { (BalanceOf, BTreeMap>) = (Zero::zero(), BTreeMap::new()); pub static SlashObserver: BTreeMap> = BTreeMap::new(); + pub static RestrictedAccounts: Vec = Vec::new(); } pub struct EventListenerMock; @@ -258,7 +259,8 @@ impl OnStakingUpdate for EventListenerMock { } } -// Disabling threshold for `UpToLimitDisablingStrategy` +// Disabling threshold for `UpToLimitDisablingStrategy` and +// `UpToLimitWithReEnablingDisablingStrategy`` pub(crate) const DISABLING_LIMIT_FACTOR: usize = 3; #[derive_impl(crate::config_preludes::TestDefaultConfig)] @@ -285,6 +287,10 @@ impl crate::pallet::pallet::Config for Test { type MaxControllersInDeprecationBatch = MaxControllersInDeprecationBatch; type EventListeners = EventListenerMock; type DisablingStrategy = pallet_staking::UpToLimitDisablingStrategy; + + fn filter(who: &AccountId) -> bool { + RestrictedAccounts::get().contains(who) + } } pub struct WeightedNominationsQuota; @@ -928,3 +934,13 @@ pub(crate) fn staking_events_since_last_call() -> Vec> { pub(crate) fn balances(who: &AccountId) -> (Balance, Balance) { (Balances::free_balance(who), Balances::reserved_balance(who)) } + +pub(crate) fn restrict(who: &AccountId) { + if !RestrictedAccounts::get().contains(who) { + RestrictedAccounts::mutate(|l| l.push(*who)); + } +} + +pub(crate) fn remove_from_restrict_list(who: &AccountId) { + RestrictedAccounts::mutate(|l| l.retain(|x| x != who)); +} diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index 5210bef853b29..e5fcf017e179f 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -25,7 +25,7 @@ use frame_election_provider_support::{ use frame_support::{ pallet_prelude::*, traits::{ - Defensive, DefensiveSaturating, EnsureOrigin, EstimateNextNewSession, Get, + Currency, Defensive, DefensiveSaturating, EnsureOrigin, EstimateNextNewSession, Get, InspectLockableCurrency, LockableCurrency, OnUnbalanced, UnixTime, }, weights::Weight, @@ -306,6 +306,16 @@ pub mod pallet { /// Weight information for extrinsics in this pallet. type WeightInfo: WeightInfo; + + /// Determines whether a given account should be filtered out from staking operations. + /// + /// This function provides a way to exclude certain accounts from bonding to staking. An + /// already bonded account is allowed to withdraw. + /// + /// The default implementation does not filter out any accounts. + fn filter(_who: &Self::AccountId) -> bool { + false + } } /// Default implementations of [`DefaultConfig`], which can be used to implement [`Config`]. @@ -1018,6 +1028,8 @@ pub mod pallet { ) -> DispatchResult { let stash = ensure_signed(origin)?; + ensure!(!T::filter(&stash), Error::::BoundNotMet); + if StakingLedger::::is_bonded(StakingAccount::Stash(stash.clone())) { return Err(Error::::AlreadyBonded.into()) } @@ -1068,6 +1080,7 @@ pub mod pallet { #[pallet::compact] max_additional: BalanceOf, ) -> DispatchResult { let stash = ensure_signed(origin)?; + ensure!(!T::filter(&stash), Error::::BoundNotMet); Self::do_bond_extra(&stash, max_additional) } @@ -1654,6 +1667,8 @@ pub mod pallet { ) -> DispatchResultWithPostInfo { let controller = ensure_signed(origin)?; let ledger = Self::ledger(Controller(controller))?; + + ensure!(!T::filter(&ledger.stash), Error::::BoundNotMet); ensure!(!ledger.unlocking.is_empty(), Error::::NoUnlockChunk); let initial_unlocking = ledger.unlocking.len() as u32; @@ -2151,6 +2166,39 @@ pub mod pallet { ); Ok(()) } + + /// Adjusts the staking ledger by withdrawing any excess staked amount. + /// + /// This function corrects cases where a user's recorded stake in the ledger + /// exceeds their actual staked funds. This situation can arise due to cases such as + /// external slashing by another pallet, leading to an inconsistency between the ledger + /// and the actual stake. + #[pallet::call_index(32)] + #[pallet::weight(T::DbWeight::get().reads_writes(2, 1))] + pub fn withdraw_overstake(origin: OriginFor, stash: T::AccountId) -> DispatchResult { + let _ = ensure_signed(origin)?; + + // Virtual stakers are controlled by some other pallet. + ensure!(!Self::is_virtual_staker(&stash), Error::::VirtualStakerNotAllowed); + + let ledger = Self::ledger(Stash(stash.clone()))?; + let stash_balance = T::Currency::free_balance(&stash); + + // Ensure there is an overstake. + ensure!(ledger.total > stash_balance, Error::::BoundNotMet); + + let force_withdraw_amount = ledger.total.defensive_saturating_sub(stash_balance); + + // Update the ledger by withdrawing excess stake. + ledger.update_total_stake(stash_balance).update()?; + + // Ensure lock is updated. + debug_assert_eq!(T::Currency::balance_locked(crate::STAKING_ID, &stash), stash_balance); + + Self::deposit_event(Event::::Withdrawn { stash, amount: force_withdraw_amount }); + + Ok(()) + } } } diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index d1dc6c3db659b..3a7aaa3aaf996 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -26,6 +26,7 @@ use frame_election_provider_support::{ use frame_support::{ assert_noop, assert_ok, assert_storage_noop, dispatch::{extract_actual_weight, GetDispatchInfo, WithPostDispatchInfo}, + hypothetically, pallet_prelude::*, traits::{Currency, Get, ReservableCurrency}, }; @@ -39,7 +40,7 @@ use sp_runtime::{ }; use sp_staking::{ offence::{OffenceDetails, OnOffenceHandler}, - SessionIndex, + SessionIndex, Stake, StakingInterface, StakingUnchecked, }; use substrate_test_utils::assert_eq_uvec; @@ -5111,6 +5112,208 @@ fn on_finalize_weight_is_nonzero() { }) } +#[test] +fn restricted_accounts_can_only_withdraw() { + ExtBuilder::default().build_and_execute(|| { + start_active_era(1); + // alice is a non blacklisted account. + let alice = 301; + let _ = Balances::make_free_balance_be(&alice, 500); + // alice can bond + assert_ok!(Staking::bond(RuntimeOrigin::signed(alice), 100, RewardDestination::Staked)); + // and bob is a blacklisted account + let bob = 302; + let _ = Balances::make_free_balance_be(&bob, 500); + restrict(&bob); + + // Bob cannot bond + assert_noop!( + Staking::bond(RuntimeOrigin::signed(bob), 100, RewardDestination::Staked,), + Error::::BoundNotMet + ); + + // alice is blacklisted now and cannot bond anymore + restrict(&alice); + assert_noop!( + Staking::bond_extra(RuntimeOrigin::signed(alice), 100), + Error::::BoundNotMet + ); + // but she can unbond her existing bond + assert_ok!(Staking::unbond(RuntimeOrigin::signed(alice), 100)); + + // she cannot rebond the unbonded amount + start_active_era(2); + assert_noop!(Staking::rebond(RuntimeOrigin::signed(alice), 50), Error::::BoundNotMet); + + // move to era when alice fund can be withdrawn + start_active_era(4); + // alice can withdraw now + assert_ok!(Staking::withdraw_unbonded(RuntimeOrigin::signed(alice), 0)); + // she still cannot bond + assert_noop!( + Staking::bond(RuntimeOrigin::signed(alice), 100, RewardDestination::Staked,), + Error::::BoundNotMet + ); + + // bob is removed from restrict list + remove_from_restrict_list(&bob); + // bob can bond now + assert_ok!(Staking::bond(RuntimeOrigin::signed(bob), 100, RewardDestination::Staked)); + // and bond extra + assert_ok!(Staking::bond_extra(RuntimeOrigin::signed(bob), 100)); + + start_active_era(6); + // unbond also works. + assert_ok!(Staking::unbond(RuntimeOrigin::signed(bob), 100)); + // bob can withdraw as well. + start_active_era(9); + assert_ok!(Staking::withdraw_unbonded(RuntimeOrigin::signed(bob), 0)); + }) +} + +#[test] +fn permissionless_withdraw_overstake() { + ExtBuilder::default().build_and_execute(|| { + // Given Alice, Bob, Charlie and Dave with some funds. + let alice = 301; + let bob = 302; + let charlie = 303; + let dave = 304; + let eve = 305; + let _ = Balances::make_free_balance_be(&alice, 200); + let _ = Balances::make_free_balance_be(&bob, 200); + let _ = Balances::make_free_balance_be(&charlie, 200); + let _ = Balances::make_free_balance_be(&dave, 200); + let _ = Balances::make_free_balance_be(&eve, 200); + + // AND: except Alice, all of them have some funds reserved by another pallet. + assert_ok!(Balances::reserve(&bob, 100)); + assert_ok!(Balances::reserve(&charlie, 100)); + assert_ok!(Balances::reserve(&dave, 100)); + assert_ok!(Balances::reserve(&eve, 199)); + + // WHEN: they are fully staked with rest of their funds. + assert_ok!(Staking::bond(RuntimeOrigin::signed(alice), 200, RewardDestination::Staked)); + assert_ok!(Staking::bond(RuntimeOrigin::signed(bob), 100, RewardDestination::Staked)); + assert_ok!(Staking::bond(RuntimeOrigin::signed(charlie), 100, RewardDestination::Staked)); + assert_ok!(Staking::bond(RuntimeOrigin::signed(dave), 100, RewardDestination::Staked)); + + // AND: charlie & dave are partially unbonding. + assert_ok!(Staking::unbond(RuntimeOrigin::signed(charlie), 90)); + assert_ok!(Staking::unbond(RuntimeOrigin::signed(dave), 10)); + + // AND: eve joins as a virtual staker + assert_ok!(::virtual_bond(&eve, 200, &alice)); + + System::reset_events(); + + // Eve can not withdraw, because it is a virtual staker + assert_noop!( + Staking::withdraw_overstake(RuntimeOrigin::signed(1), eve), + Error::::VirtualStakerNotAllowed + ); + + // THEN Alice cannot transfer any funds out of her account. + assert_noop!( + Balances::transfer_allow_death(RuntimeOrigin::signed(alice), 1, 1), + TokenError::Frozen, + ); + + // Bob can transfer out all their staked funds aside from ED. + assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(bob), 1, 99)); + + // hypothetically Dave & Charlie also can transfer out all their staked funds aside from ED. + hypothetically!({ + assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(charlie), 1, 99)); + assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(dave), 1, 99)); + }); + + // We withdraw only a part of it for the test. + assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(charlie), 1, 50)); + assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(dave), 1, 50)); + + // --- + // GIVEN: Now Alice has no over-stake. + let alice_free_bal = Balances::free_balance(&alice); + assert_eq!(alice_free_bal, 200); + assert_eq!( + ::stake(&alice).unwrap(), + Stake { total: 200, active: 200 } + ); + + // THEN: permissionlessly withdrawing overstake fails. + assert_noop!( + Staking::withdraw_overstake(RuntimeOrigin::signed(1), alice), + Error::::BoundNotMet + ); + + // --- + // GIVEN: Bob is overstaked with no partial unbonding chunks. + let bob_free_bal = Balances::free_balance(&bob); + assert_eq!(bob_free_bal, 1); + assert_eq!( + ::stake(&bob).unwrap(), + Stake { total: 100, active: 100 } + ); + + // WHEN: permissionlessly withdrawing overstake. + assert_ok!(Staking::withdraw_overstake(RuntimeOrigin::signed(1), bob)); + + // THEN: Bob's stake is corrected. + assert_eq!( + ::stake(&bob).unwrap(), + Stake { total: 100 - 99, active: 100 - 99 } + ); + + // --- + // GIVEN: Charlie is overstaked with partial unbonding chunks. + let charlie_free_bal = Balances::free_balance(&charlie); + assert_eq!(charlie_free_bal, 50); + assert_eq!( + ::stake(&charlie).unwrap(), + Stake { total: 100, active: 100 - 90 } + ); + + // WHEN: permissionlessly withdrawing overstake. + assert_ok!(Staking::withdraw_overstake(RuntimeOrigin::signed(1), charlie)); + + // THEN: Charlie's stake is corrected and overstake is taken from both active stake and + // unbonding chunks. + // (Note: Unlocking chunks are not explicitly checked here but the consistency is checked + // as try state check at the end of the test.) + assert_eq!( + ::stake(&charlie).unwrap(), + Stake { total: 100 - 50, active: 0 } + ); + + // --- + // GIVEN: Dave is overstaked with partial unbonding chunks. + let dave_free_bal = Balances::free_balance(&dave); + assert_eq!(dave_free_bal, 50); + assert_eq!( + ::stake(&dave).unwrap(), + Stake { total: 100, active: 100 - 10 } + ); + + // WHEN: permissionlessly withdrawing overstake. + assert_ok!(Staking::withdraw_overstake(RuntimeOrigin::signed(1), dave)); + + // THEN: Dave's stake is corrected and overstake is taken from active stake. + assert_eq!( + ::stake(&dave).unwrap(), + Stake { total: 100 - 50, active: 90 - 50 } + ); + + assert_eq!( + staking_events_since_last_call(), + vec![ + Event::Withdrawn { stash: bob, amount: 99 }, + Event::Withdrawn { stash: charlie, amount: 50 }, + Event::Withdrawn { stash: dave, amount: 50 } + ] + ); + }); +} mod election_data_provider { use super::*; use frame_election_provider_support::ElectionDataProvider;