[Staking] Currency <> Fungible migration #5501

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

[Update]: This PR needs to be updated to remove adding explicit providers for staking account. We will instead require all stakers to maintain ED in their accounts, including pool accounts.

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

Resolves part of #226.

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

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

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 ?

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

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.

what if account has some consumer or provider reference? not staking related

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.

gui1117 · 2024-11-04T06:40:14Z

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 is the doc for Currency::balance is:

/// Get the balance of `who` which does not include funds which are exclusively allocated to /// subsystems of the chain ("on hold" or "reserved"). /// /// In general this isn't especially useful outside of tests, and for practical purposes, you'll /// want to use [`Inspect::reducible_balance`].

So balance include the frozen amount. Later we do some transfer with Currency::transfer which implies Polite fortitude and thus can't transfer the frozen amount.

Maybe we want to use reducible_balance(account, Expandable, Polie) here? or do I miss something about slashing frozen amount?

With current locks, its possible to stake all your funds including ED. I tried to retain that behaviour by adding provider to staking account. Therefore, you could stake all your free balance including ED, and so I used Currency::balance.

Update: after some discussion with the staking team, we want stakers to maintain ED and I will make necessary updates to the PR.

I meant about the frozen amount. calling balance here doesn't return the liquid balance. it returns the balance except holds, including frozen and ed.

I guess it works because the pool_account will never have frozen amounts.

substrate/frame/staking/src/asset.rs

gui1117 · 2024-11-04T07:20:54Z

substrate/frame/staking/src/asset.rs

+	}
+
+	// dec provider that we incremented for a new stake.
+	let _ = frame_system::Pallet::<T>::dec_providers(who)?;


Am I wrong?

Suggested change

let _ = frame_system::Pallet::<T>::dec_providers(who)?;

frame_system::Pallet::<T>::dec_providers(who)?;

Rust would warn about the unused return type

DecRefStatus is not must use.

it doesn't to me:

info: using existing install for 'stable-x86_64-unknown-linux-gnu' info: default toolchain set to 'stable-x86_64-unknown-linux-gnu' stable-x86_64-unknown-linux-gnu unchanged - rustc 1.81.0 (eeb90cda1 2024-09-04) Checking thiserror v1.0.64 Checking sp-panic-handler v13.0.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/primitives/panic-handler) Compiling frame-support-procedural-tools v10.0.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/frame/support/procedural/tools) Compiling frame-election-provider-solution-type v13.0.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/frame/election-provider-support/solution-type) Compiling sp-version-proc-macro v13.0.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/primitives/version/proc-macro) Checking sp-metadata-ir v0.6.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/primitives/metadata-ir) Checking sp-core v28.0.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/primitives/core) Compiling frame-support-procedural v23.0.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/frame/support/procedural) Checking sp-trie v29.0.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/primitives/trie) Checking sp-keystore v0.34.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/primitives/keystore) Checking sp-state-machine v0.35.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/primitives/state-machine) Checking sp-io v30.0.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/primitives/io) Checking sp-application-crypto v30.0.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/primitives/application-crypto) Checking sp-runtime v31.0.1 (/home/guillaume/remote-builds/12223365420444655616/substrate/primitives/runtime) Checking sp-version v29.0.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/primitives/version) Checking sp-inherents v26.0.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/primitives/inherents) Checking sp-staking v26.0.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/primitives/staking) Checking sp-npos-elections v26.0.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/primitives/npos-elections) Checking sp-api v26.0.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/primitives/api) Checking sp-genesis-builder v0.8.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/primitives/genesis-builder) Checking sp-session v27.0.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/primitives/session) Checking sp-timestamp v26.0.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/primitives/timestamp) Checking frame-support v28.0.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/frame/support) Checking frame-system v28.0.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/frame/system) Checking pallet-timestamp v27.0.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/frame/timestamp) Checking frame-election-provider-support v28.0.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/frame/election-provider-support) Checking pallet-authorship v28.0.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/frame/authorship) Checking pallet-session v28.0.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/frame/session) Checking pallet-staking v28.0.0 (/home/guillaume/remote-builds/12223365420444655616/substrate/frame/staking) Finished `dev` profile [unoptimized + debuginfo] target(s) in 26.34s Connection to 78.137.198.97 closed. [gui@fedora polkadot-sdk]$ git diff diff --git a/substrate/frame/staking/src/asset.rs b/substrate/frame/staking/src/asset.rs index 216ec22a498..3b19cf95ece 100644 --- a/substrate/frame/staking/src/asset.rs +++ b/substrate/frame/staking/src/asset.rs @@ -119,7 +119,7 @@ pub fn kill_stake<T: Config>(who: &T::AccountId) -> DispatchResult { } // dec provider that we incremented for a new stake. - let _ = frame_system::Pallet::<T>::dec_providers(who)?; + frame_system::Pallet::<T>::dec_providers(who)?; Ok(()) }

substrate/frame/staking/src/asset.rs

gui1117 · 2024-11-04T07:27:04Z

substrate/frame/staking/src/asset.rs

+		// pallet from clearing its provider reference. If the account cannot provide for itself
+		// (via enough free balance) to keep the session key, we try to clean up the session keys.
+		// If the extra consumer comes from somewhere else, reap stash would fail.
+		T::SessionInterface::purge_keys(who.clone())?;


isn't storage dirty if an error is returned now? maybe we can just wrap into transactional to make it safe?

Good point. I think kill_stake is only triggered with the extrinsic withdraw_unbonded, so whole thing is transactional. But not sure if this can happen during slash or other system level operation. I will have a look.

Checked this and slash does not kill ledger. So this should only happen in a transactional context.

TODO: add to the function rustdoc that it expects to be called in a transactional context only.

We can go this way but I am pretty sure few people are not looking at the rustdoc when reviewing a PR.
And people can easily miss it when changing code.

I am afraid it will backfire some day.

gui1117 · 2024-11-04T07:40:20Z

substrate/frame/staking/src/ledger.rs

 				// if not virtual staker, clear locks.
-				asset::kill_stake::<T>(&ledger.stash);
+				asset::kill_stake::<T>(&ledger.stash)?;


storage is tainted no?

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

kianenigma reviewed Oct 23, 2024

View reviewed changes

substrate/frame/staking/src/pallet/impls.rs Outdated Show resolved Hide resolved

kianenigma reviewed Oct 23, 2024

View reviewed changes

substrate/frame/staking/src/pallet/mod.rs Show resolved Hide resolved

kianenigma reviewed Oct 23, 2024

View reviewed changes

muharem reviewed Oct 24, 2024

View reviewed changes

Ank4n added 5 commits October 25, 2024 12:46

update comment

ff1878d

test validator purge keys

e6de610

test session purge for reaping validators

8e957ff

refactor migration code

d2b1084

add rustdoc for migrate

3e48265

Ank4n mentioned this pull request Oct 28, 2024

[WIP] Ensure ED is respected over total balance when depositing new funds to an account. #6256

Closed

gui1117 reviewed Nov 4, 2024

View reviewed changes

Ank4n marked this pull request as draft November 4, 2024 18:21

Ank4n and others added 9 commits November 6, 2024 22:11

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

8463df2

staking should not add provider for accounts

7adfb85

49 fails

60972ca

20 fails

7e7980d

12 fails

69e7bcb

fix migration

bec40b1

staking all test pass

2451f93

fmt

5f8ec96

fix fast unstake tests

bb532f2

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

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

muharem Oct 24, 2024

Ank4n Oct 24, 2024 •

edited

Loading

muharem Oct 25, 2024

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

gui1117 Nov 4, 2024

Ank4n Nov 4, 2024 •

edited

Loading

gui1117 Nov 5, 2024

gui1117 Nov 4, 2024

Ank4n Nov 4, 2024

gui1117 Nov 5, 2024 •

edited

Loading

gui1117 Nov 4, 2024

Ank4n Nov 4, 2024

Ank4n Nov 4, 2024 •

edited

Loading

gui1117 Nov 5, 2024 •

edited

Loading

gui1117 Nov 4, 2024


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

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

	let _ = frame_system::Pallet::<T>::dec_providers(who)?;
	frame_system::Pallet::<T>::dec_providers(who)?;

[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

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

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

Choose a reason for hiding this comment

Ank4n Nov 4, 2024 • edited Loading

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

gui1117 Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ank4n Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

gui1117 Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ank4n commented Aug 27, 2024 •

edited

Loading

Ank4n Oct 24, 2024 •

edited

Loading

muharem Oct 24, 2024 •

edited

Loading

Ank4n Nov 4, 2024 •

edited

Loading

gui1117 Nov 5, 2024 •

edited

Loading

Ank4n Nov 4, 2024 •

edited

Loading

gui1117 Nov 5, 2024 •

edited

Loading