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

Ank4n · 2024-06-15T19:44:36Z

Context

Currently, an account can do both, directly stake, and join a pool.

With the migration of pools to DelegateStake (See #3905), the funds of pool members are locked in a different way than for direct stakers.

Pool member funds will use holds after migration.
pallet-staking uses deprecated locks (analogous to freeze) but they can overlap with holds.

Note that once we migrate pallet-staking to use fungibles, it will also start using non-overlapping holds.

Risk

This makes it possible for a direct staker to reuse the staked (frozen) funds to be used for participating in a pool (hold) as well. Essentially double staking with the same funds.

This is prevented currently by pallet-delegated-staking not allowing direct stakers to delegate, as well as by pallet-staking only taking free balance into account. But this is not documented well and a bit fragile.

Changes

A better way would be to make pool and staking mutually exclusive. This PR introduces a new config type Blacklist to pallet-nomination-pools and pallet-staking that can be configured to blacklist some accounts from participating in respective pallets. We then blacklist all pallet-staking stakers from participating in pools, and all pool members from participating in pallet-staking.

Note that there are 648 accounts in Polkadot and 321 accounts in Kusama who are both pool member and a staker. After this change, these accounts would not be able to bond extra funds to their stake but can still withdraw. Once they have withdrawn all their funds from pool, they would be able to bond additional funds to staking and vice versa.

… staking

paritytech-cicd-pr · 2024-06-16T00:52:45Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6471123

kianenigma

sgtm

How much is the stake behind the 27 who are now double staked?

probably they will figure it out, but would be good to think as much as we can about how we can help them find their way out of this maze (without eg. talking to the extrinsics tab of PJS 🤭). Maybe a polkadot wiki page about pools can be updated about this.

Ank4n · 2024-06-19T13:36:42Z

My dirty script to count number of accounts doing both staking and pool was totally off. The number is much larger and yeah we will need to do a better communication about it (such as putting this info on polkadot wiki).

There are 648 accounts on Polkadot and 321 accounts on Kusama that are taking part in both.

Related: #4804. Fixes the try state error in Westend: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6564522. Passes here: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6580393 ## Context Currently in Kusama and Polkadot, an account can do both, directly stake, and join a pool. With the migration of pools to `DelegateStake` (See #3905), the funds of pool members are locked in a different way than for direct stakers. - Pool member funds uses `holds`. - `pallet-staking` uses deprecated locks (analogous to freeze) which can overlap with holds. An existing delegator can stake directly since pallet-staking only uses free balance. But once an account becomes staker, we cannot allow them to be delegator as this risks an account to use already staked (frozen) funds in pools. When an account gets into a situation where it is participating in both pools and staking, it would no longer would be able to add any extra bond to the pool but they can still withdraw funds. ## Changes - Add test for the above scenario. - Removes the assumption that a delegator cannot be a staker.

…#4904) Related: paritytech#4804. Fixes the try state error in Westend: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6564522. Passes here: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6580393 ## Context Currently in Kusama and Polkadot, an account can do both, directly stake, and join a pool. With the migration of pools to `DelegateStake` (See paritytech#3905), the funds of pool members are locked in a different way than for direct stakers. - Pool member funds uses `holds`. - `pallet-staking` uses deprecated locks (analogous to freeze) which can overlap with holds. An existing delegator can stake directly since pallet-staking only uses free balance. But once an account becomes staker, we cannot allow them to be delegator as this risks an account to use already staked (frozen) funds in pools. When an account gets into a situation where it is participating in both pools and staking, it would no longer would be able to add any extra bond to the pool but they can still withdraw funds. ## Changes - Add test for the above scenario. - Removes the assumption that a delegator cannot be a staker.

gpestana · 2024-07-10T15:59:08Z

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

+			// alice cannot directly stake.
+			assert_noop!(
+				Staking::bond(RuntimeOrigin::signed(alice), 300, RewardDestination::Account(alice)),
+				StakingError::<T>::Blacklisted


I find this error too dramatic. If I was a user and didn't know what Blacklisted meant in this context, I'd probably panic. Maybe we could rename it to Error::UnableToBond instead or something else?

gpestana · 2024-07-10T16:00:51Z

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

@@ -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,


As mentioned above, I think "BlackListed" may be too overwhelming for a end-user as it seems it's excluding the user from bonding/staking for malicious reasons. Maybe we could rename it to UnableToStake or something similar?

OTOH naming the config types as Blacklisted seems ok since that's internal to devs.

gpestana · 2024-07-10T16:01:49Z

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

-		/// 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.


Suggested change

/// increases the pools bond.

/// increases the pool's bond.

gpestana · 2024-07-10T16:03:24Z

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

@@ -3948,3 +3963,10 @@ impl<T: Config> sp_staking::OnStakingUpdate<T::AccountId, BalanceOf<T>> for Pall
 		}
 	}
 }
+


Maybe add some rustdoc to this struct?

gpestana · 2024-07-10T16:04:04Z

substrate/frame/staking/src/lib.rs

@@ -1341,3 +1342,10 @@ impl<T: Config, const DISABLING_LIMIT_FACTOR: usize> DisablingStrategy<T>
 		Some(offender_idx)
 	}
 }
+


Maybe add rustdoc to this struct?

gpestana · 2024-07-10T16:04:46Z

substrate/frame/staking/src/pallet/mod.rs

@@ -934,6 +946,9 @@ pub mod pallet {
 		NotEnoughFunds,
 		/// Operation not allowed for virtual stakers.
 		VirtualStakerNotAllowed,
+		/// Account is blacklisted and is only allowed to withdraw existing funds. This may happen
+		/// if the account is participating in staking in another way already.
+		Blacklisted,


Same as in comment above.

Ank4n · 2024-07-18T21:31:40Z

Closing this as will go ahead with this. Instead we will look to migrate staking pallet to use Fungible::holds.

…#4904) Related: paritytech#4804. Fixes the try state error in Westend: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6564522. Passes here: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6580393 ## Context Currently in Kusama and Polkadot, an account can do both, directly stake, and join a pool. With the migration of pools to `DelegateStake` (See paritytech#3905), the funds of pool members are locked in a different way than for direct stakers. - Pool member funds uses `holds`. - `pallet-staking` uses deprecated locks (analogous to freeze) which can overlap with holds. An existing delegator can stake directly since pallet-staking only uses free balance. But once an account becomes staker, we cannot allow them to be delegator as this risks an account to use already staked (frozen) funds in pools. When an account gets into a situation where it is participating in both pools and staking, it would no longer would be able to add any extra bond to the pool but they can still withdraw funds. ## Changes - Add test for the above scenario. - Removes the assumption that a delegator cannot be a staker.

Ank4n added 16 commits June 14, 2024 14:12

default config for staking

05f0d28

beefy

d479b4f

use staking default config in tests

e13e836

remove unnecessary config items for staking in mocks

22b03ea

Merge branch 'master' into ankan/staking-default-config

b65de87

fix compile

d3cb87d

default for bench config only in std

1285dcd

explicitly add benchmark config to independent crates

cf5cb46

remove direct staker check while delegating

1ccf2e6

test

4eb3700

revert removing direct staker check while delegating

f86fc86

add a blacklist check trait for blocking delegators to participate in…

6872c2e

… staking

add a way to blacklist some accounts from staking

b3e964c

add way to blacklist in both pools and staking

9cc9ef8

tests for pool blacklist

2b1739d

make pool members and stakers exclusive

f77ddfc

Ank4n force-pushed the ankan/mutually-exclusive-pool-stakers branch from 54db650 to f77ddfc Compare June 15, 2024 21:34

Ank4n added 3 commits June 16, 2024 02:15

add blacklist to all non mock runtimes

ecec1f7

trying to make clippy happy

f2fae10

fix unused import

357788f

Ank4n and others added 5 commits June 16, 2024 02:53

westend runtime

11a9066

fmt

941f288

Merge branch 'master' into ankan/mutually-exclusive-pool-stakers

179863a

set payee should not redispatch

5ec27dc

Merge branch 'master' into ankan/mutually-exclusive-pool-stakers

8f48031

Ank4n marked this pull request as ready for review June 17, 2024 15:17

Ank4n requested a review from a team as a code owner June 17, 2024 15:17

Ank4n added the T2-pallets This PR/Issue is related to a particular pallet. label Jun 17, 2024

add prdoc

8b945d1

Ank4n and others added 2 commits June 17, 2024 17:59

update prdoc

e7d55e3

Merge branch 'master' into ankan/mutually-exclusive-pool-stakers

d6fcb0d

Ank4n requested review from gpestana, kianenigma and jakoblell June 17, 2024 16:02

kianenigma approved these changes Jun 18, 2024

View reviewed changes

Merge branch 'master' into ankan/mutually-exclusive-pool-stakers

8fdbdf7

Ank4n mentioned this pull request Jun 19, 2024

[Pools] Fix issues with member migration to DelegateStake #4822

Merged

4 tasks

Merge branch 'master' into ankan/mutually-exclusive-pool-stakers

9072e0a

Ank4n mentioned this pull request Jun 27, 2024

[Staking] Delegators can stake but stakers can't delegate #4904

Merged

Ank4n changed the title ~~Prevent users from both directly staking and joining pool~~ (DNM) Prevent users from both directly staking and joining pool Jul 4, 2024

gpestana reviewed Jul 10, 2024

View reviewed changes

Ank4n marked this pull request as draft July 10, 2024 21:44

Ank4n closed this Jul 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Ank4n commented Jun 15, 2024 •

edited

Loading

paritytech-cicd-pr commented Jun 16, 2024

kianenigma left a comment

Ank4n commented Jun 19, 2024

gpestana Jul 10, 2024

gpestana Jul 10, 2024

gpestana Jul 10, 2024

gpestana Jul 10, 2024

gpestana Jul 10, 2024

gpestana Jul 10, 2024

Ank4n commented Jul 18, 2024

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

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

Conversation

Ank4n commented Jun 15, 2024 • edited Loading

Context

Risk

Changes

paritytech-cicd-pr commented Jun 16, 2024

kianenigma left a comment

Choose a reason for hiding this comment

Ank4n commented Jun 19, 2024

gpestana Jul 10, 2024

Choose a reason for hiding this comment

gpestana Jul 10, 2024

Choose a reason for hiding this comment

gpestana Jul 10, 2024

Choose a reason for hiding this comment

gpestana Jul 10, 2024

Choose a reason for hiding this comment

gpestana Jul 10, 2024

Choose a reason for hiding this comment

gpestana Jul 10, 2024

Choose a reason for hiding this comment

Ank4n commented Jul 18, 2024

Ank4n commented Jun 15, 2024 •

edited

Loading