(DNM) Prevent users from both directly staking and joining pool

polkadot/runtime/test-runtime/src/lib.rs

@@ -49,7 +49,7 @@ use frame_support::{
 	construct_runtime, derive_impl,
 	genesis_builder_helper::{build_state, get_preset},
 	parameter_types,
-	traits::{KeyOwnerProofSystem, WithdrawReasons},
+	traits::{KeyOwnerProofSystem, Nothing, WithdrawReasons},
 };
 use pallet_grandpa::{fg_primitives, AuthorityId as GrandpaId};
 use pallet_session::historical as session_historical;
@@ -367,6 +367,7 @@ impl pallet_staking::Config for Runtime {
 	type EventListeners = ();
 	type WeightInfo = ();
 	type DisablingStrategy = pallet_staking::UpToLimitDisablingStrategy;
+	type Blacklist = Nothing;
 }
 
 parameter_types! {

polkadot/runtime/westend/src/lib.rs

@@ -652,6 +652,8 @@ impl pallet_staking::Config for Runtime {
 	type EventListeners = (NominationPools, DelegatedStaking);
 	type WeightInfo = weights::pallet_staking::WeightInfo<Runtime>;
 	type DisablingStrategy = pallet_staking::UpToLimitDisablingStrategy;
+	// We restrict pool members to directly stake.
+	type Blacklist = pallet_nomination_pools::AllPoolMembers<Self>;
 }
 
 impl pallet_fast_unstake::Config for Runtime {
@@ -1383,6 +1385,11 @@ impl pallet_nomination_pools::Config for Runtime {
 	type PalletId = PoolsPalletId;
 	type MaxPointsToBalance = MaxPointsToBalance;
 	type AdminOrigin = EitherOf<EnsureRoot<AccountId>, StakingAdmin>;
+	// Due to difference in how pools and staking pallets handle currency locks, we want to keep
+	// pool members and stakers mutually exclusive. Hence, we want to prevent stakers from using
+	// the pool pallet. Any existing account that is already in the pool but blacklisted can still
+	// withdraw funds but cannot add new funds.
+	type Blacklist = pallet_staking::AllStakers<Self>;
 }
 
 parameter_types! {

prdoc/pr_4804.prdoc

@@ -0,0 +1,27 @@
+# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
+# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
+
+title: Make pool and staking mutually exclusive
+
+doc:
+  - audience: Runtime User
+    description: |
+        This PR aims to make pool accounts and staking accounts mutually exclusive. Accounts doing both are still
+        able to withdraw funds completely from one of them after which they can do all normal operations with other
+        pallet. This mitigates the risk of using same fund to double stake.
+
+crates:
+  - name: westend-runtime
+    bump: minor
+  - name: pallet-nomination-pools-benchmarking
+    bump: minor
+  - name: pallet-nomination-pools
+    bump: minor
+  - name: pallet-staking
+    bump: minor
+  - name: pallet-delegated-staking
+    bump: minor
+  - name: sp-staking
+    bump: minor
+
+

substrate/bin/node/runtime/src/lib.rs

@@ -704,6 +704,7 @@ impl pallet_staking::Config for Runtime {
 	type WeightInfo = pallet_staking::weights::SubstrateWeight<Runtime>;
 	type BenchmarkingConfig = StakingBenchmarkingConfig;
 	type DisablingStrategy = pallet_staking::UpToLimitDisablingStrategy;
+	type Blacklist = pallet_nomination_pools::AllPoolMembers<Self>;
 }
 
 impl pallet_fast_unstake::Config for Runtime {
@@ -922,6 +923,7 @@ impl pallet_nomination_pools::Config for Runtime {
 		EnsureRoot<AccountId>,
 		pallet_collective::EnsureProportionAtLeast<AccountId, CouncilCollective, 3, 4>,
 	>;
+	type Blacklist = pallet_staking::AllStakers<Self>;
 }
 
 parameter_types! {

substrate/frame/delegated-staking/src/lib.rs

@@ -409,6 +409,10 @@ pub mod pallet {
 				Delegation::<T>::can_delegate(&delegator, &agent),
 				Error::<T>::InvalidDelegation
 			);
+
+			// Note: Staking uses locks (freeze) which are not mutually exclusive of holds. We
+			// cannot allow funds to be delegated if they are locked in staking. An account can
+			// only either delegate or stake.
 			ensure!(!Self::is_direct_staker(&delegator), Error::<T>::AlreadyStaking);
 
 			// ensure agent is sane.
@@ -503,7 +507,7 @@ impl<T: Config> Pallet<T> {
 			Preservation::Expendable,
 		)?;
 
-		T::CoreStaking::update_payee(who, reward_account)?;
+		T::CoreStaking::set_payee(who, reward_account)?;
 		// delegate all transferred funds back to agent.
 		Self::do_delegate(proxy_delegator, Agent::from(who.clone()), amount_to_transfer)?;
 

substrate/frame/delegated-staking/src/mock.rs

@@ -111,6 +111,7 @@ impl pallet_staking::Config for Runtime {
 	type VoterList = pallet_staking::UseNominatorsAndValidatorsMap<Self>;
 	type TargetList = pallet_staking::UseValidatorsMap<Self>;
 	type EventListeners = (Pools, DelegatedStaking);
+	type Blacklist = pallet_nomination_pools::AllPoolMembers<Self>;
 }
 
 parameter_types! {
@@ -160,6 +161,7 @@ impl pallet_nomination_pools::Config for Runtime {
 	type StakeAdapter =
 		pallet_nomination_pools::adapter::DelegateStake<Self, Staking, DelegatedStaking>;
 	type AdminOrigin = frame_system::EnsureRoot<Self::AccountId>;
+	type Blacklist = pallet_staking::AllStakers<Self>;
 }
 
 frame_support::construct_runtime!(

substrate/frame/delegated-staking/src/tests.rs

@@ -576,12 +576,12 @@ mod staking_integration {
 
 			// update_payee to self fails.
 			assert_noop!(
-				<Staking as StakingInterface>::update_payee(&200, &200),
+				<Staking as StakingInterface>::set_payee(&200, &200),
 				StakingError::<T>::RewardDestinationRestricted
 			);
 
 			// passing correct reward destination works
-			assert_ok!(<Staking as StakingInterface>::update_payee(&200, &201));
+			assert_ok!(<Staking as StakingInterface>::set_payee(&200, &201));
 
 			// amount is staked correctly
 			assert!(eq_stake(200, 100, 100));
@@ -716,6 +716,7 @@ mod staking_integration {
 mod pool_integration {
 	use super::*;
 	use pallet_nomination_pools::{BondExtra, BondedPools, PoolState};
+	use pallet_staking::RewardDestination;
 
 	#[test]
 	fn create_pool_test() {
@@ -1217,6 +1218,63 @@ mod pool_integration {
 		});
 	}
 
+	#[test]
+	fn existing_stakers_cannot_participate_in_pools() {
+		// Staking uses freezes while pools/delegated-staking uses holds. It is possible that funds
+		// frozen for staking, could be re-used to participate in pools. To avoid this, we can
+		// blacklist the stakers from participating in pools. See
+		// `pallet_nomination_pools::Config::Blacklist`.
+		ExtBuilder::default().build_and_execute(|| {
+			// GIVEN
+			// alice and a pool
+			let alice = 300;
+			fund(&alice, 1000);
+			fund(&200, 5000);
+			let pool_id = create_pool(200, 5000);
+
+			// WHEN
+			// alice directly stakes.
+			assert_ok!(Staking::bond(
+				RuntimeOrigin::signed(alice),
+				300,
+				RewardDestination::Account(alice)
+			));
+
+			// THEN
+			// alice cannot join the pool.
+			assert_noop!(
+				Pools::join(RawOrigin::Signed(alice).into(), 300, pool_id),
+				PoolsError::<T>::Blacklisted
+			);
+		});
+	}
+
+	#[test]
+	fn existing_pool_members_cannot_directly_stake() {
+		// Staking uses freezes while pools/delegated-staking uses holds. It is possible that funds
+		// frozen for staking, could be re-used to participate in pools. To avoid this, we can
+		// blacklist pool members from participating in staking. See
+		// `pallet_staking::Config::Blacklist`.
+		ExtBuilder::default().build_and_execute(|| {
+			// GIVEN
+			// alice and a pool
+			let alice = 300;
+			fund(&alice, 1000);
+			fund(&200, 5000);
+			let pool_id = create_pool(200, 5000);
+
+			// WHEN
+			// alice participates in a pool.
+			assert_ok!(Pools::join(RawOrigin::Signed(alice).into(), 300, pool_id));
+
+			// THEN
+			// alice cannot directly stake.
+			assert_noop!(
+				Staking::bond(RuntimeOrigin::signed(alice), 300, RewardDestination::Account(alice)),
+				StakingError::<T>::Blacklisted
+			);
+		});
+	}
 	fn create_pool(creator: AccountId, amount: Balance) -> u32 {
 		fund(&creator, amount * 2);
 		assert_ok!(Pools::create(

substrate/frame/election-provider-multi-phase/test-staking-e2e/src/mock.rs

@@ -273,6 +273,7 @@ impl pallet_nomination_pools::Config for Runtime {
 	type MaxUnbonding = MaxUnbonding;
 	type MaxPointsToBalance = frame_support::traits::ConstU8<10>;
 	type AdminOrigin = frame_system::EnsureRoot<Self::AccountId>;
+	type Blacklist = pallet_staking::AllStakers<Self>;
 }
 
 parameter_types! {
@@ -307,6 +308,7 @@ impl pallet_staking::Config for Runtime {
 	type WeightInfo = pallet_staking::weights::SubstrateWeight<Runtime>;
 	type DisablingStrategy = pallet_staking::UpToLimitDisablingStrategy<SLASHING_DISABLING_FACTOR>;
 	type BenchmarkingConfig = pallet_staking::TestBenchmarkingConfig;
+	type Blacklist = pallet_nomination_pools::AllPoolMembers<Self>;
 }
 
 impl<LocalCall> frame_system::offchain::SendTransactionTypes<LocalCall> for Runtime

substrate/frame/nomination-pools/benchmarking/src/mock.rs

@@ -21,7 +21,7 @@ use frame_support::{
 	derive_impl,
 	pallet_prelude::*,
 	parameter_types,
-	traits::{ConstU64, VariantCountOf},
+	traits::{ConstU64, Nothing, VariantCountOf},
 	PalletId,
 };
 use sp_runtime::{
@@ -139,6 +139,7 @@ impl pallet_nomination_pools::Config for Runtime {
 	type PalletId = PoolsPalletId;
 	type MaxPointsToBalance = MaxPointsToBalance;
 	type AdminOrigin = frame_system::EnsureRoot<Self::AccountId>;
+	type Blacklist = Nothing;
 }
 
 parameter_types! {

substrate/frame/nomination-pools/src/lib.rs

@@ -360,7 +360,7 @@ use frame_support::{
 	traits::{
 		fungible::{Inspect, InspectFreeze, Mutate, MutateFreeze},
 		tokens::{Fortitude, Preservation},
-		Defensive, DefensiveOption, DefensiveResult, DefensiveSaturating, Get,
+		Contains, Defensive, DefensiveOption, DefensiveResult, DefensiveSaturating, Get,
 	},
 	DefaultNoBound, PalletError,
 };
@@ -1648,6 +1648,9 @@ pub mod pallet {
 
 		/// The origin that can manage pool configurations.
 		type AdminOrigin: EnsureOrigin<Self::RuntimeOrigin>;
+
+		/// Blacklist some accounts from participating in pools.
+		type Blacklist: Contains<Self::AccountId>;
 	}
 
 	/// The sum of funds across all pools.
@@ -1946,6 +1949,9 @@ pub mod pallet {
 		NotMigrated,
 		/// This call is not allowed in the current state of the pallet.
 		NotSupported,
+		/// Account is blacklisted from participation in pools. This may happen if the account is
+		/// staking in another way already.
+		Blacklisted,
 	}
 
 	#[derive(Encode, Decode, PartialEq, TypeInfo, PalletError, RuntimeDebug)]
@@ -1983,8 +1989,9 @@ pub mod pallet {
 
 	#[pallet::call]
 	impl<T: Config> Pallet<T> {
-		/// Stake funds with a pool. The amount to bond is transferred from the member to the
-		/// pools account and immediately increases the pools bond.
+		/// Stake funds with a pool. The amount to bond is delegated (or transferred based on
+		/// [`adapter::StakeStrategyType`]) from the member to the pool account and immediately
+		/// increases the pools bond.
 		///
 		/// # Note
 		///
@@ -2001,6 +2008,7 @@ pub mod pallet {
 			pool_id: PoolId,
 		) -> DispatchResult {
 			let who = ensure_signed(origin)?;
+			ensure!(!T::Blacklist::contains(&who), Error::<T>::Blacklisted);
 
 			ensure!(amount >= MinJoinBond::<T>::get(), Error::<T>::MinimumBondNotMet);
 			// If a member already exists that means they already belong to a pool
@@ -2063,6 +2071,7 @@ pub mod pallet {
 		)]
 		pub fn bond_extra(origin: OriginFor<T>, extra: BondExtra<BalanceOf<T>>) -> DispatchResult {
 			let who = ensure_signed(origin)?;
+			ensure!(!T::Blacklist::contains(&who), Error::<T>::Blacklisted);
 			Self::do_bond_extra(who.clone(), who, extra)
 		}
 
@@ -2411,6 +2420,7 @@ pub mod pallet {
 			bouncer: AccountIdLookupOf<T>,
 		) -> DispatchResult {
 			let depositor = ensure_signed(origin)?;
+			ensure!(!T::Blacklist::contains(&depositor), Error::<T>::Blacklisted);
 
 			let pool_id = LastPoolId::<T>::try_mutate::<_, Error<T>, _>(|id| {
 				*id = id.checked_add(1).ok_or(Error::<T>::OverflowRisk)?;
@@ -2437,6 +2447,7 @@ pub mod pallet {
 			pool_id: PoolId,
 		) -> DispatchResult {
 			let depositor = ensure_signed(origin)?;
+			ensure!(!T::Blacklist::contains(&depositor), Error::<T>::Blacklisted);
 
 			ensure!(!BondedPools::<T>::contains_key(pool_id), Error::<T>::PoolIdInUse);
 			ensure!(pool_id < LastPoolId::<T>::get(), Error::<T>::InvalidPoolId);
@@ -2692,7 +2703,9 @@ pub mod pallet {
 			extra: BondExtra<BalanceOf<T>>,
 		) -> DispatchResult {
 			let who = ensure_signed(origin)?;
-			Self::do_bond_extra(who, T::Lookup::lookup(member)?, extra)
+			let member = T::Lookup::lookup(member)?;
+			ensure!(!T::Blacklist::contains(&member), Error::<T>::Blacklisted);
+			Self::do_bond_extra(who, member, extra)
 		}
 
 		/// Allows a pool member to set a claim permission to allow or disallow permissionless
@@ -2916,6 +2929,8 @@ pub mod pallet {
 			);
 
 			let member_account = T::Lookup::lookup(member_account)?;
+			ensure!(!T::Blacklist::contains(&member_account), Error::<T>::Blacklisted);
+
 			let member =
 				PoolMembers::<T>::get(&member_account).ok_or(Error::<T>::PoolMemberNotFound)?;
 
@@ -3948,3 +3963,10 @@ impl<T: Config> sp_staking::OnStakingUpdate<T::AccountId, BalanceOf<T>> for Pall
 		}
 	}
 }
+
+pub struct AllPoolMembers<T: Config>(PhantomData<T>);
+impl<T: Config> Contains<T::AccountId> for AllPoolMembers<T> {
+	fn contains(t: &T::AccountId) -> bool {
+		PoolMembers::<T>::contains_key(t)
+	}
+}

substrate/frame/nomination-pools/src/mock.rs

@@ -56,6 +56,7 @@ parameter_types! {
 	pub static MaxUnbonding: u32 = 8;
 	pub static StakingMinBond: Balance = 10;
 	pub storage Nominations: Option<Vec<AccountId>> = None;
+	pub static BlacklistedAccounts: Vec<AccountId> = Vec::new();
 }
 pub struct StakingMock;
 
@@ -136,7 +137,7 @@ impl sp_staking::StakingInterface for StakingMock {
 		Ok(())
 	}
 
-	fn update_payee(_stash: &Self::AccountId, _reward_acc: &Self::AccountId) -> DispatchResult {
+	fn set_payee(_stash: &Self::AccountId, _reward_acc: &Self::AccountId) -> DispatchResult {
 		unimplemented!("method currently not used in testing")
 	}
 
@@ -276,6 +277,13 @@ impl Convert<U256, Balance> for U256ToBalance {
 	}
 }
 
+pub struct BlacklistMock;
+impl Contains<AccountId> for BlacklistMock {
+	fn contains(who: &AccountId) -> bool {
+		BlacklistedAccounts::get().contains(who)
+	}
+}
+
 parameter_types! {
 	pub static PostUnbondingPoolsWindow: u32 = 2;
 	pub static MaxMetadataLen: u32 = 2;
@@ -302,6 +310,7 @@ impl pools::Config for Runtime {
 	type MaxUnbonding = MaxUnbonding;
 	type MaxPointsToBalance = frame_support::traits::ConstU8<10>;
 	type AdminOrigin = EnsureSignedBy<Admin, AccountId>;
+	type Blacklist = BlacklistMock;
 }
 
 type Block = frame_system::mocking::MockBlock<Runtime>;
@@ -533,6 +542,16 @@ pub fn reward_imbalance(pool: PoolId) -> RewardImbalance {
 	}
 }
 
+pub fn blacklist(who: &AccountId) {
+	if !BlacklistedAccounts::get().contains(who) {
+		BlacklistedAccounts::mutate(|l| l.push(*who));
+	}
+}
+
+pub fn remove_from_blacklist(who: &AccountId) {
+	BlacklistedAccounts::mutate(|l| l.retain(|x| x != who));
+}
+
 #[cfg(test)]
 mod test {
 	use super::*;