[Staking] Currency <> Fungible migration #5501

Ank4n · 2024-08-27T12:19:46Z

Migrate staking currency from traits::LockableCurrency to traits::fungible::holds.

Summary of changes

Config: Currency becomes of type Fungible while OldCurrency is the LockableCurrency used before.
Lazy migration of accounts. Any ledger updates will create a new hold with no extra reads/writes. A permissionless extrinsic migrate_currency() releases the old lock along with some housekeeping.
Staking now adds a provider instead of a consumer. More details later.
If hold cannot be applied to all stake, the un-holdable part is force withdrawn from the ledger.
Reap stash might fail now if provider cannot be decremented.

Migration stats

Polkadot

Total accounts that can be migrated: 61752
Accounts failing to migrate: 1
Accounts with some stake force withdrawn: 35
Total force withdrawal: 16287.7 DOT

Kusama

Total accounts that can be migrated: 26835
Accounts failing to migrate: 0
Accounts with some stake force withdrawn: 59
Total force withdrawal: 14.5 KSM

Full logs here. Error in polkadot is caused due to the ledger corruption, will be fixed by this runtime update. It is possible some of the force withdraws are happening because of ledger corruption and the migration stats might change after those ledgers are fixed.

Note about locks (freeze) vs holds

With locks or freezes, staking could use total balance of an account. But with holds, the account needs to be left with at least Existential Deposit in free balance. This would also affect nomination pools which till now has been able to stake all funds contributed to it. An alternate version of this PR is #5658 where staking pallet does not add any provider, but means pools and delegated-staking pallet has to provide for these accounts and makes the end to end logic (of provider and consumer ref) lot less intuitive and prone to bug.

One way to handle this issue is add a provider to staking stash. This then replicates old behaviour of staking where accounts could stake their whole balance. The side effect of this is that, for stakers that have been slashed to zero, their stash may not be reaped until all consumers to this account is removed. This is because, dec_provider would fail until consumer == 0.

Note about providers and consumers

Before

pallet-staking added consumers for stash account. This meant the agent/pool account must need a provider (by holding ED or system inc provider). We did an explicit inc provider for agents in pallet-delegated staking to allow consumer inc.

Now

This is more simplified. pallet-staking adds provider for stash accounts, but no consumer. pallet-delegated-staking or pools does not need to add provider anymore (or consumer), and as stated before, helps retain the old behaviour of being able to stake all balance via both direct staking or pool.

TODO

Permissionless call to release lock.
Migration of consumer (dec) and provider (inc) for direct stakers.
force unstake if hold cannot be applied to all stake.
Fix try state checks (it thinks nothing is staked for unmigrated ledgers).
Bench migrate_currency.
Virtual Staker migration test.
Ensure total issuance is upto date when minting rewards.

Followup

Ensure nomination pool members can directly stake and vice versa. #5742

balance hold checks both frozen and reserved wip: around 25 tests failing check Holds instead of locks 20 tests failing fmt 11 fails 4 fails 2 failing 1 fail all tests pass but pending a hygiene check of code fix compile minor refactor remove T::Currency calls from asset mod

…staking-migrate-currency-to-fungible-2

Ank4n · 2024-10-08T22:18:27Z

bot fmt

command-bot · 2024-10-08T22:18:32Z

@Ank4n https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7538682 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 28-d6cf8ab9-4fc7-44c1-9ffb-9a6716c33a8c to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot · 2024-10-08T22:19:15Z

@Ank4n Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7538682 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7538682/artifacts/download.

kianenigma · 2024-10-23T16:18:05Z

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

@@ -262,7 +262,8 @@ impl<T: Config, Staking: StakingInterface<Balance = BalanceOf<T>, AccountId = T:
 		pool_account: Pool<Self::AccountId>,
 		_: Member<Self::AccountId>,
 	) -> BalanceOf<T> {
-		T::Currency::balance(&pool_account.0).saturating_sub(Self::active_stake(pool_account))
+		// free/liquid balance of the pool account.
+		T::Currency::balance(&pool_account.0)


This number does not change for any pool given the README info. Correct?

Yes the number does not change.

Longer explanation: Previously, staked (locked) funds were part of free balance. So we calculated transferable balance as free balance - staked balance.

Now, holds are taken away from free part of the balance, so transferable balance = free balance.

kianenigma · 2024-10-23T16:19:08Z

substrate/frame/session/benchmarking/src/mock.rs

@@ -131,6 +131,7 @@ impl onchain::Config for OnChainSeqPhragmen {

 #[derive_impl(pallet_staking::config_preludes::TestDefaultConfig)]
 impl pallet_staking::Config for Test {
+	type OldCurrency = Balances;


Why don't you add this to TestDefaultConfig and be done with it? And later when you remove it nothing has to change.

Can it have a default? Since its a runtime pallet (depends on system config)?

substrate/frame/staking/src/asset.rs

kianenigma · 2024-10-23T16:24:55Z

substrate/frame/staking/src/asset.rs

+	T::Currency::release_all(&HoldReason::Staking.into(), who, Precision::BestEffort)
+		.map(|_| ())?;
+
+	if !frame_system::Pallet::<T>::can_dec_provider(who) {


This is a very good one, making sure that validators can also fully unstake, and laeve. Would be good to ensure this flow is tested.

substrate/frame/staking/src/ledger.rs

kianenigma · 2024-10-23T16:32:03Z

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

@@ -797,8 +795,6 @@ impl<T: Config> Pallet<T> {
 		Self::do_remove_validator(&stash);
 		Self::do_remove_nominator(&stash);

-		frame_system::Pallet::<T>::dec_consumers(&stash);


I would expect assets::kill_stake to be called here, shouldn't it?

StakingLedger::<T>::kill(&stash) calls that.

kianenigma · 2024-10-23T16:34:21Z

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

+			return Ok(())
+		}
+
+		ensure!(!Self::is_virtual_staker(stash), Error::<T>::VirtualStakerNotAllowed);


Not a big deal, but this code can be probably better written by having two diffent functions to handle virtual and non virtual, without an ensure like this, and have the parent one only have a match and forward.

kianenigma · 2024-10-23T16:34:53Z

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

+		}
+
+		ensure!(!Self::is_virtual_staker(stash), Error::<T>::VirtualStakerNotAllowed);
+		let ledger = Self::ledger(Stash(stash.clone()))?;


A good future refactor is to remove the need for clone in Stash. It should not be needed. Makes a good junior mentor issue.

kianenigma · 2024-10-23T16:38:39Z

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

+		ensure!(ledger.total == locked, Error::<T>::BadState);
+
+		let max_hold = asset::stakeable_balance::<T>(&stash);
+		let force_withdraw = if max_hold >= locked {


The fact that the StakingLedger is updated via two different methods in if and else is not ideal IMO.

I think a nicely written version would look like:

let mut ledger = todo!(); if { ledger.total = todo!("this") } else { ledger.total = todo!("that") } ledger.update()?;

but yeah, I am not super familiar with these interfaces.

fwiw, this allows us to use something similar to the Rust dropping mechanism for StakingLedger via let mut ledger. Once this is dropped (via fn update() taking in self), storage is updated.

My nites aside, the code looks clean and easy to get :)

Ledger is mutated only in else block? Or do I misunderstand?

kianenigma · 2024-10-23T16:41:00Z

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

@@ -2087,7 +2121,7 @@ pub mod pallet {
 					let new_total = if let Some(total) = maybe_total {
 						let new_total = total.min(stash_balance);
 						// enforce lock == ledger.amount.
-						asset::update_stake::<T>(&stash, new_total);
+						asset::update_stake::<T>(&stash, new_total)?;


I think we can already remove this tx?

Yes probably. @gpestana ?

kianenigma · 2024-10-23T16:41:43Z

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

+			Self::do_migrate_currency(&stash)?;
+
+			// Refund the transaction fee if successful.
+			Ok(Pays::No.into())


I've checked this in your code already, but important to ensure do_migrate_currency errors on duplicates, to not make this abuse-able.

kianenigma · 2024-10-23T16:44:05Z

substrate/frame/support/src/traits/tokens/fungible/regular.rs

-		if new_balance < Self::minimum_balance() {
+
+		// look at total balance to ensure ED is respected.
+		let new_total_balance = amount.saturating_add(Self::total_balance(who));


So this is an important change:

In the past amount + Self::balance() had to meed ED requirements, now amount + Self::total_balance.

What is your explanation of this change? Is it affecting any other systme?

The holds intentionally was not included into ED, I think the reason is that with the new fungibles contract we wanna guaranty that holds are always slashable, for the same reason we introduced the hold reasons. To change this you need really strong reasoning and for sure a separate PR.

a separate PR

A separate PR is fair.

What is your explanation of this change

Without this, any staking reward of less than 1 DOT to an account who staked all their balance would fail.

Also, ED requirement even when an account has an active hold seemed like an oversight to me, since the account can not be killed if it has an active hold.

I think the reason is that with the new fungibles contract we wanna guaranty that holds are always slashable

How is it related? Why would holds not be slashable if free part of the balance does not have ED requirements on its own? All held amount can still be slashed and account would be dusted in that case right? Or am I missing something.

kianenigma · 2024-10-23T16:45:02Z

substrate/frame/treasury/src/lib.rs

@@ -987,6 +989,19 @@ impl<T: Config<I>, I: 'static> OnUnbalanced<NegativeImbalanceOf<T, I>> for Palle
 	}
 }

+/// Implement the `OnUnbalanced` handler for [`frame_support::traits::fungible`] trait currency.


Probably unrelated to the PR?

Staking uses fungible now, and treasury is the slash handler for it. While treasury still uses Currency which is not compatible with fungibles. FungibleCompat adds a way for treasury to manage fungible imbalance.

kianenigma

Overall looks good! I will approve after this round of comments are addressed.

The new code path for purging session keys seems new, but untested.

kianenigma · 2024-10-24T11:31:43Z

One meta comment I have is that this pallet is already passed the point of being useful to anyone else beyond the polkadot relay chain.

If we pretend to be general, we need to having migration guides, and so on.

Instead, I suggest that as a part of AHM, we move this + other polkadot-only pallets that might be lingering here to the fellowship repo.

And importantly, rename this pallet to pallet-polkadot-npos. We can keep an older version of this that is bug-free in polkadot-sdk as pallet-staking.

muharem · 2024-10-24T15:30:53Z

substrate/frame/treasury/src/lib.rs

@@ -987,6 +989,19 @@ impl<T: Config<I>, I: 'static> OnUnbalanced<NegativeImbalanceOf<T, I>> for Palle
 	}
 }

+/// Implement the `OnUnbalanced` handler for [`frame_support::traits::fungible`] trait currency.
+pub struct FungibleCompat<T>(PhantomData<T>);


you can use tokens::imbalance::ResolveTo type with getter type of account id

muharem · 2024-10-24T15:32:06Z

substrate/frame/treasury/src/lib.rs

+
+		let _ = T::Currency::resolve(&Pallet::<T>::account_id(), credit).defensive();
+
+		Pallet::<T>::deposit_event(Event::Deposit { value: numeric_amount });


the even already publishing already there on resolve completion, done by implementation of fungibles::Balanced::done_deposit method.

muharem · 2024-10-24T15:44:07Z

substrate/frame/support/src/traits/tokens/fungible/regular.rs

-		if new_balance < Self::minimum_balance() {
+
+		// look at total balance to ensure ED is respected.
+		let new_total_balance = amount.saturating_add(Self::total_balance(who));


The holds intentionally was not included into ED, I think the reason is that with the new fungibles contract we wanna guaranty that holds are always slashable, for the same reason we introduced the hold reasons. To change this you need really strong reasoning and for sure a separate PR.

muharem · 2024-10-24T15:46:39Z

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

@@ -2146,6 +2180,19 @@ pub mod pallet {
 			);
 			Ok(())
 		}
+
+		#[pallet::call_index(30)]


how about a doc? (=

so basically we can migrate everything ourself? users no need to do anything? and lazy migrating on ledger update just to make sure nothing happens before

Ledger update does not migrate. I did it this way since 1. its safe (just extra locked funds), and 2. no extra read/write while ledger updates since this code can linger for a while. But practically, we will migrate all accounts via a bot, so there will be no extra locked funds.

Ank4n and others added 21 commits August 18, 2024 20:02

prep for abstracting out all currency calls in a separate mod

02d3a70

replace ED calls

fa1064f

replace ti, burn, and set balance calls

babc205

add read balance and update lock fn to asset

2f0cd28

replace all currency call with existing asset fn

4af4aee

add remaining currency fn

1995e06

all currency fn replaced

f3a3081

fix imports

6a03abb

remove unused import

040c630

Merge branch 'master' into ankan/staking-migrate-currency-to-fungible

3d3ceac

fix clippy

61eb7b7

repalce Balances api calls in test to asset as well

b8a0e4b

clippy

2926255

Merge branch 'master' into ankan/staking-migrate-currency-to-fungible

9e38e8e

rustdoc update

34e1d48

refactor

31c6467

reorder

851c5f3

rustdoc fix

ebf60ef

rename Fungible to Currency

07e3ca9

add support for fungible imbalance handler in pallet treasury

58e8e83

Ank4n changed the base branch from master to ankan/staking-migrate-currency-to-fungible August 27, 2024 12:20

Ank4n mentioned this pull request Aug 27, 2024

[Staking] Noop refactor to prep pallet for currency fungible migration #5399

Merged

Ank4n and others added 7 commits August 27, 2024 14:28

add license

fe893a8

Merge branch 'master' into ankan/staking-migrate-currency-to-fungible

087692b

fix unused imports

a9d0345

fix rust doc

67bd3fa

Merge branch 'ankan/staking-migrate-currency-to-fungible' into ankan/…

5157b3b

…staking-migrate-currency-to-fungible-2

missing import

e6c705e

conditional import of fun mutate

c4a9c9d

Ank4n and others added 5 commits October 8, 2024 19:28

stash needs ed to stay alive

e688c93

update prdoc

d9e0f86

fix prdoc

8db37b6

fix prdoc

e87b5e4

Merge branch 'master' into ankan/staking-migrate-currency-to-fungible-2

31d04f8

".git/.scripts/commands/fmt/fmt.sh"

b5e473b

Ank4n added 3 commits October 10, 2024 14:31

Merge branch 'master' into ankan/staking-migrate-currency-to-fungible-2

15961dc

Merge branch 'master' into ankan/staking-migrate-currency-to-fungible-2

02ecebd

Merge branch 'master' into ankan/staking-migrate-currency-to-fungible-2

3d5a7c0

Ank4n requested a review from muharem October 22, 2024 11:10

kianenigma reviewed Oct 23, 2024

View reviewed changes

substrate/frame/staking/src/asset.rs Show resolved Hide resolved

kianenigma reviewed Oct 23, 2024

View reviewed changes

substrate/frame/staking/src/ledger.rs Show resolved Hide resolved

kianenigma reviewed Oct 23, 2024

View reviewed changes

muharem reviewed Oct 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Staking] Currency <> Fungible migration #5501

[Staking] Currency <> Fungible migration #5501

Ank4n commented Aug 27, 2024 •

edited

Loading

Ank4n commented Oct 8, 2024

command-bot bot commented Oct 8, 2024 •

edited

Loading

command-bot bot commented Oct 8, 2024

kianenigma Oct 23, 2024

Ank4n Oct 24, 2024

kianenigma Oct 23, 2024

Ank4n Oct 24, 2024

kianenigma Oct 23, 2024

kianenigma Oct 23, 2024

Ank4n Oct 24, 2024

kianenigma Oct 23, 2024

kianenigma Oct 23, 2024

kianenigma Oct 23, 2024

kianenigma Oct 23, 2024

kianenigma Oct 23, 2024

Ank4n Oct 24, 2024

kianenigma Oct 23, 2024

Ank4n Oct 24, 2024

kianenigma Oct 23, 2024

kianenigma Oct 23, 2024

muharem Oct 24, 2024

Ank4n Oct 24, 2024 •

edited

Loading

kianenigma Oct 23, 2024

Ank4n Oct 24, 2024

kianenigma left a comment

kianenigma commented Oct 24, 2024

muharem Oct 24, 2024

muharem Oct 24, 2024

muharem Oct 24, 2024

muharem Oct 24, 2024 •

edited

Loading

Ank4n Oct 24, 2024


		let _ = T::Currency::resolve(&Pallet::<T>::account_id(), credit).defensive();

		Pallet::<T>::deposit_event(Event::Deposit { value: numeric_amount });

[Staking] Currency <> Fungible migration #5501

Are you sure you want to change the base?

[Staking] Currency <> Fungible migration #5501

Conversation

Ank4n commented Aug 27, 2024 • edited Loading

Summary of changes

Migration stats

Polkadot

Kusama

Note about locks (freeze) vs holds

Note about providers and consumers

Before

Now

TODO

Followup

Ank4n commented Oct 8, 2024

command-bot bot commented Oct 8, 2024 • edited Loading

command-bot bot commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ank4n Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

kianenigma commented Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muharem Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ank4n commented Aug 27, 2024 •

edited

Loading

command-bot bot commented Oct 8, 2024 •

edited

Loading

Ank4n Oct 24, 2024 •

edited

Loading

muharem Oct 24, 2024 •

edited

Loading