Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Staking] Currency <> Fungible migration #5501

Open
wants to merge 194 commits into
base: master
Choose a base branch
from

Conversation

Ank4n
Copy link
Contributor

@Ank4n Ank4n commented Aug 27, 2024

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

Resolves part of #226.

Changes

Nomination Pool

TransferStake is now incompatible with fungible migration as old pools were not meant to have additional ED. Since they are anyways deprecated, removed its usage from all test runtimes.

Staking

  • Config: Currency becomes of type Fungible while OldCurrency is the LockableCurrency used before.
  • Lazy migration of accounts. Any ledger update 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 requires ED to be left free. It also adds no consumer to staking accounts.
  • If hold cannot be applied to all stake, the un-holdable part is force withdrawn from the ledger.

Delegated Staking

The pallet does not add provider for agents anymore.

Migration stats

Polkadot

Total accounts that can be migrated: 59564
Accounts failing to migrate: 0
Accounts with stake force withdrawn greater than ED: 59
Total force withdrawal: 29591.26 DOT

Kusama

Total accounts that can be migrated: 26311
Accounts failing to migrate: 0
Accounts with stake force withdrawn greater than ED: 48
Total force withdrawal: 1036.05 KSM

Full logs here.

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.

This PR now introduces requirement for stakers to maintain ED in their free balance. This helps with removing the bug prone incrementing and decrementing of consumers and providers.

TODO

  • Test: Vesting + governance locked funds can be staked.
  • can Call::restore_ledger be removed? @gpestana
  • Ensure unclaimed withdrawals is not affected by no provider for pool accounts.
  • Investigate kusama accounts with balance between 0 and ED.
  • 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

@Ank4n Ank4n changed the base branch from master to ankan/staking-migrate-currency-to-fungible August 27, 2024 12:20
@Ank4n Ank4n requested a review from Zebedeusz December 5, 2024 10:19
// Get rid of the extra consumer we used to have with OldCurrency.
frame_system::Pallet::<T>::dec_consumers(&stash);

Self::deposit_event(Event::<T>::CurrencyMigrated { stash: stash.clone(), force_withdraw });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this event suppossed to notify of how much funds were migrated?
Because by looking at the if-else above it looks like it's more like funds_still_locked_in_old_currency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I understand funds_still_locked_in_old_currency ? Once we get here, there should be nothing locked with old currency.

The event is meant to notify clients that the given account has migrated to new currency along with the information if their ledger has reduced by some balance because of this operation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understood that the "else" condition is when someone has more funds locked for staking than in their free balance thus we cannot "hold" as much as is needed so we go just for as much as possible.
Regarding funds_still_locked_in_old_currency, I called it so because in if zero value is returned and in else it's the difference between staked and free balance and since the free balance will be held then the result is what's left to unlock and move to hold.
Is my reasoning correct?

@@ -89,13 +93,27 @@ pub mod pallet {

#[pallet::config(with_default)]
pub trait Config: frame_system::Config {
/// The old trait for staking balance. Deprecated and only used for migrating old ledgers.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the pallet be annotated with deprecated then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not pallet but yeah may be config item can be annotated as deprecated (if its possible).

///
/// Does not include the current stake.
pub fn free_to_stake<T: Config>(who: &T::AccountId) -> BalanceOf<T> {
// since we want to be able to use frozen funds for staking, we force the reduction.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The note was related to the usage of Fortitude::Force. Any funds that is locked or frozen (such as for governance or vesting) should still be usable for staking.

@gpestana gpestana self-requested a review December 9, 2024 13:19
let _ = T::Currency::set_balance(who, value - staked_balance + ed);
} else {
// else reduce the staked balance.
update_stake::<T>(who, value).expect("can remove from what is staked");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
update_stake::<T>(who, value).expect("can remove from what is staked");
update_stake::<T>(who, value).expect("cannot remove from what is staked");

#[test]
fn ledger_update_creates_hold() {
ExtBuilder::default().has_stakers(true).build_and_execute(|| {
// GIVEN alice who is a nominator with old currency
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// GIVEN alice who is a nominator with old currency
// GIVEN alice who is a nominator with new currency

prdoc/pr_5501.prdoc Show resolved Hide resolved
substrate/bin/node/runtime/src/lib.rs Show resolved Hide resolved
substrate/frame/staking/src/pallet/mod.rs Outdated Show resolved Hide resolved
substrate/frame/staking/src/asset.rs Outdated Show resolved Hide resolved
Comment on lines +1226 to +1230
// We should never have more than expected providers.
ensure!(actual_providers <= expected_providers, Error::<T>::BadState);

// if actual provider is less than expected, it is already migrated.
ensure!(actual_providers == expected_providers, Error::<T>::AlreadyMigrated);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this means it should be just equal. first ensure is redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The errors will be different. If greater -> bad state, if less -> AlreadyMigrated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but you never get AlreadyMigrated error

let expected_providers =
// provider is expected to be 1 but someone can always transfer some free funds to
// these accounts, increasing the provider.
if asset::free_to_stake::<T>(&stash) >= asset::existential_deposit::<T>() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should not be balance >= ed? free_to_stake does not include the ED

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These (keyless) accounts had a explicit provider added to them by delegated staking.

When you have the extra provider, free_to_stake will include ED. Retrying to migrate will always fail as actual provider should be 0, always less than expected providers.

The test hold_migration::virtual_staker_consumer_provider_dec checks these scenarios.

@@ -262,7 +264,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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

balance() contains the frozen part which means can be not liquid

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically this works because pool will never have frozen funds (its a pool generated pot account) but is confusing. I will fix it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems fixed now?

@muharem
Copy link
Contributor

muharem commented Dec 17, 2024

Accounts with some stake force withdrawn: 59
Total force withdrawal: 29591.26 DOT

@Ank4n can you explain why force withdrawal not just 59 DOT? so locks are not there?

@Ank4n
Copy link
Contributor Author

Ank4n commented Dec 18, 2024

Accounts with some stake force withdrawn: 59
Total force withdrawal: 29591.26 DOT

@Ank4n can you explain why force withdrawal not just 59 DOT? so locks are not there?

Yes we had a bug that caused ledgers to be overwritten. It didn't exactly allow anyone to stake more than their balance, but it overwritten their controller account with stash account's stake.

Most of them have been fixed here: polkadot-fellows/runtimes#447.
These 59 accounts still had more at stake than their free funds (@gpestana was this supposed to be handled with your migration?). The full logs are here, and the biggest chunk of total amount is coming from one account that will be force withdrawn 12k funds.

image

@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/12391754099
Failed job name: build-rustdoc

Copy link
Contributor

@muharem muharem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. But consider this approve as from someone who is not an expert in the staking topic

@@ -2020,6 +2020,7 @@ pub mod pallet {
ensure!(!PoolMembers::<T>::contains_key(&who), Error::<T>::AccountBelongsToOtherPool);

let mut bonded_pool = BondedPool::<T>::get(pool_id).ok_or(Error::<T>::PoolNotFound)?;
ensure!(bonded_pool.state == PoolState::Open, Error::<T>::NotOpen);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eh, seems like a bug from the past? let's add a unit test.

impl DelegationInterface for DelegateMock {
type Balance = Balance;
type AccountId = AccountId;
fn agent_balance(agent: Agent<Self::AccountId>) -> Option<Self::Balance> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is basically the "staked, and active in staking (minus pending slash)" balance?

}

fn remove_agent(agent: Agent<Self::AccountId>) -> DispatchResult {
let mut agents = AgentBalanceMap::get();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably not needed for tests but to simulate the real code better, we should error if agent is in fact not an agent?

Or assert!(agents.contains(agent.get()))


let agent = agent.get();
let mut agents = AgentBalanceMap::get();
agents.get_mut(&agent).map(|(d, _, _)| *d += amount);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
agents.get_mut(&agent).map(|(d, _, _)| *d += amount);
agents.get_mut(&agent).map(|(d, _, _)| *d += amount).ok_or(/* some dispatch error */);

@@ -3925,6 +3861,10 @@ mod withdraw_unbonded {
let mut member = PoolMember { pool_id: 1, points: 10, ..Default::default() };
PoolMembers::<Runtime>::insert(11, member.clone());

// set agent and delegator balance
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is it that these two were not needed before this change?

@@ -1331,11 +1344,12 @@ fn disable_pool_operations_on_non_migrated() {
assert_ok!(Pools::migrate_pool_to_delegate_stake(RuntimeOrigin::signed(10), 1));
assert_eq!(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file had really good tests, I learned a lot by reading them!

@kianenigma
Copy link
Contributor

kianenigma commented Jan 10, 2025

PR is in good shape, and thanks to incremental code, the changes are easy to digest, good work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T2-pallets This PR/Issue is related to a particular pallet.
Projects
Status: In review
Status: Scheduled
Development

Successfully merging this pull request may close these issues.

7 participants