Skip to content

Commit

Permalink
[Pools] Fix issues with member migration to DelegateStake (#4822)
Browse files Browse the repository at this point in the history
## Context
Pool members using the old `TransferStake` strategy were able to
transfer all their funds to the pool. With `DelegateStake` changes, we
want to ensure similar behaviour by allowing members to delegate all
their stake to the pool.

## Changes
- Ensure all the balance including ED of an account can be delegated
(and used in the pool) by adding a provider for delegators.
- Gates calls that mutates the pool or pool member if they are in
unmigrated state. Closes
paritytech-secops/srlabs_findings#409.
- Adds remote test to migrate all pools and members to `DelegateStake`
which can be used with `Kusama` and `Polkadot` runtime state. closes
#4629.
- Add new runtime apis to read pool and member balance.

## Addressing possible migration errors 
Pool members migrating can run into two types of errors:
- Already Staking: If the pool member is already staking, we cannot
migrate them to `DelegateStake` since this may mean they are able to use
the same staked funds in the pool. Users would need to withdraw all
their funds from staking, in order to migrate their pool funds.
- Pool contribution below ED: For these cases transfer from pool account
to member account would fail. The affected users can top up their
accounts and redo migration.

Another error that was earlier possible was when member's free balance
is below ED. This PR adds a provider to delegator allowing all user
balance including ED can be contributed towards the pool. This helps
`1095` accounts in Polkadot and `41` accounts in Kusama to migrate now
which would have earlier failed.

## Results from RemoteExternalities Tests.

### Kusama
`Migration stats: success: 3017, direct_stakers: 361, unexpected_errors:
0`

### Polkadot
`Migration stats: success: 42859, direct_stakers: 643,
unexpected_errors: 0`

## TODO
- [x] Add runtime api for member total balance.
- [x] New
[issue](#5009) to reap
pool members with contribution below ED.
- [x] Add provider for delegators so whole balance including ED can be
held while contributing to pools.
- [x] Gate all pool extrinsics if pool/member is in non-migrated state.

---------

Co-authored-by: Gonçalo Pestana <g6pestana@gmail.com>
  • Loading branch information
Ank4n and gpestana authored Aug 14, 2024
1 parent 5a9396f commit feacf2f
Show file tree
Hide file tree
Showing 11 changed files with 733 additions and 163 deletions.
49 changes: 9 additions & 40 deletions polkadot/runtime/westend/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ use sp_runtime::{
IdentityLookup, Keccak256, OpaqueKeys, SaturatedConversion, Verify,
},
transaction_validity::{TransactionPriority, TransactionSource, TransactionValidity},
ApplyExtrinsicResult, FixedU128, KeyTypeId, Perbill, Percent, Permill,
ApplyExtrinsicResult, FixedU128, KeyTypeId, Percent, Permill,
};
use sp_staking::SessionIndex;
#[cfg(any(feature = "std", test))]
Expand Down Expand Up @@ -2476,6 +2476,14 @@ sp_api::impl_runtime_apis! {
fn member_needs_delegate_migration(member: AccountId) -> bool {
NominationPools::api_member_needs_delegate_migration(member)
}

fn member_total_balance(member: AccountId) -> Balance {
NominationPools::api_member_total_balance(member)
}

fn pool_balance(pool_id: pallet_nomination_pools::PoolId) -> Balance {
NominationPools::api_pool_balance(pool_id)
}
}

impl pallet_staking_runtime_api::StakingApi<Block, Balance, AccountId> for Runtime {
Expand Down Expand Up @@ -2776,45 +2784,6 @@ sp_api::impl_runtime_apis! {
}
}

#[cfg(all(test, feature = "try-runtime"))]
mod remote_tests {
use super::*;
use frame_try_runtime::{runtime_decl_for_try_runtime::TryRuntime, UpgradeCheckSelect};
use remote_externalities::{
Builder, Mode, OfflineConfig, OnlineConfig, SnapshotConfig, Transport,
};
use std::env::var;

#[tokio::test]
async fn run_migrations() {
if var("RUN_MIGRATION_TESTS").is_err() {
return;
}

sp_tracing::try_init_simple();
let transport: Transport =
var("WS").unwrap_or("wss://westend-rpc.polkadot.io:443".to_string()).into();
let maybe_state_snapshot: Option<SnapshotConfig> = var("SNAP").map(|s| s.into()).ok();
let mut ext = Builder::<Block>::default()
.mode(if let Some(state_snapshot) = maybe_state_snapshot {
Mode::OfflineOrElseOnline(
OfflineConfig { state_snapshot: state_snapshot.clone() },
OnlineConfig {
transport,
state_snapshot: Some(state_snapshot),
..Default::default()
},
)
} else {
Mode::Online(OnlineConfig { transport, ..Default::default() })
})
.build()
.await
.unwrap();
ext.execute_with(|| Runtime::on_runtime_upgrade(UpgradeCheckSelect::PreAndPost));
}
}

mod clean_state_migration {
use super::Runtime;
#[cfg(feature = "try-runtime")]
Expand Down
137 changes: 137 additions & 0 deletions polkadot/runtime/westend/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,3 +99,140 @@ fn check_treasury_pallet_id() {
westend_runtime_constants::TREASURY_PALLET_ID
);
}

#[cfg(all(test, feature = "try-runtime"))]
mod remote_tests {
use super::*;
use frame_try_runtime::{runtime_decl_for_try_runtime::TryRuntime, UpgradeCheckSelect};
use remote_externalities::{
Builder, Mode, OfflineConfig, OnlineConfig, SnapshotConfig, Transport,
};
use std::env::var;

#[tokio::test]
async fn run_migrations() {
if var("RUN_MIGRATION_TESTS").is_err() {
return;
}

sp_tracing::try_init_simple();
let transport: Transport =
var("WS").unwrap_or("wss://westend-rpc.polkadot.io:443".to_string()).into();
let maybe_state_snapshot: Option<SnapshotConfig> = var("SNAP").map(|s| s.into()).ok();
let mut ext = Builder::<Block>::default()
.mode(if let Some(state_snapshot) = maybe_state_snapshot {
Mode::OfflineOrElseOnline(
OfflineConfig { state_snapshot: state_snapshot.clone() },
OnlineConfig {
transport,
state_snapshot: Some(state_snapshot),
..Default::default()
},
)
} else {
Mode::Online(OnlineConfig { transport, ..Default::default() })
})
.build()
.await
.unwrap();
ext.execute_with(|| Runtime::on_runtime_upgrade(UpgradeCheckSelect::PreAndPost));
}

#[tokio::test]
async fn delegate_stake_migration() {
// Intended to be run only manually.
if var("RUN_MIGRATION_TESTS").is_err() {
return;
}
use frame_support::assert_ok;
sp_tracing::try_init_simple();

let transport: Transport = var("WS").unwrap_or("ws://127.0.0.1:9900".to_string()).into();
let maybe_state_snapshot: Option<SnapshotConfig> = var("SNAP").map(|s| s.into()).ok();
let mut ext = Builder::<Block>::default()
.mode(if let Some(state_snapshot) = maybe_state_snapshot {
Mode::OfflineOrElseOnline(
OfflineConfig { state_snapshot: state_snapshot.clone() },
OnlineConfig {
transport,
state_snapshot: Some(state_snapshot),
pallets: vec![
"staking".into(),
"system".into(),
"balances".into(),
"nomination-pools".into(),
"delegated-staking".into(),
],
..Default::default()
},
)
} else {
Mode::Online(OnlineConfig { transport, ..Default::default() })
})
.build()
.await
.unwrap();
ext.execute_with(|| {
// create an account with some balance
let alice = AccountId::from([1u8; 32]);
use frame_support::traits::Currency;
let _ = Balances::deposit_creating(&alice, 100_000 * UNITS);

// iterate over all pools
pallet_nomination_pools::BondedPools::<Runtime>::iter_keys().for_each(|k| {
if pallet_nomination_pools::Pallet::<Runtime>::api_pool_needs_delegate_migration(k)
{
assert_ok!(
pallet_nomination_pools::Pallet::<Runtime>::migrate_pool_to_delegate_stake(
RuntimeOrigin::signed(alice.clone()).into(),
k,
)
);
}
});

// member migration stats
let mut success = 0;
let mut direct_stakers = 0;
let mut unexpected_errors = 0;

// iterate over all pool members
pallet_nomination_pools::PoolMembers::<Runtime>::iter_keys().for_each(|k| {
if pallet_nomination_pools::Pallet::<Runtime>::api_member_needs_delegate_migration(
k.clone(),
) {
// reasons migrations can fail:
let is_direct_staker = pallet_staking::Bonded::<Runtime>::contains_key(&k);

let migration = pallet_nomination_pools::Pallet::<Runtime>::migrate_delegation(
RuntimeOrigin::signed(alice.clone()).into(),
sp_runtime::MultiAddress::Id(k.clone()),
);

if is_direct_staker {
// if the member is a direct staker, the migration should fail until pool
// member unstakes all funds from pallet-staking.
direct_stakers += 1;
assert_eq!(
migration.unwrap_err(),
pallet_delegated_staking::Error::<Runtime>::AlreadyStaking.into()
);
} else if migration.is_err() {
unexpected_errors += 1;
log::error!(target: "remote_test", "Unexpected error {:?} while migrating {:?}", migration.unwrap_err(), k);
} else {
success += 1;
}
}
});

log::info!(
target: "remote_test",
"Migration stats: success: {}, direct_stakers: {}, unexpected_errors: {}",
success,
direct_stakers,
unexpected_errors
);
});
}
}
25 changes: 25 additions & 0 deletions prdoc/pr_4822.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# 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: Ensure as many as possible pool members can migrate to `DelegateStake`

doc:
- audience: Runtime Dev
description: |
1. Allows pool members to use their total balance while joining pool with `DelegateStake`.
2. Gates call mutating pool or member in unmigrated state.
3. Runtime apis for reading pool and member balance.

crates:
- name: westend-runtime
bump: minor
- name: kitchensink-runtime
bump: patch
- name: pallet-delegated-staking
bump: patch
- name: pallet-nomination-pools
bump: minor
- name: sp-staking
bump: patch
- name: pallet-nomination-pools-runtime-api
bump: minor
8 changes: 8 additions & 0 deletions substrate/bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2784,6 +2784,14 @@ impl_runtime_apis! {
fn member_needs_delegate_migration(member: AccountId) -> bool {
NominationPools::api_member_needs_delegate_migration(member)
}

fn member_total_balance(member: AccountId) -> Balance {
NominationPools::api_member_total_balance(member)
}

fn pool_balance(pool_id: pallet_nomination_pools::PoolId) -> Balance {
NominationPools::api_pool_balance(pool_id)
}
}

impl pallet_staking_runtime_api::StakingApi<Block, Balance, AccountId> for Runtime {
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/delegated-staking/src/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
//! Implementations of public traits, namely [`DelegationInterface`] and [`OnStakingUpdate`].

use super::*;
use sp_staking::{Agent, DelegationInterface, DelegationMigrator, Delegator, OnStakingUpdate};
use sp_staking::{DelegationInterface, DelegationMigrator, OnStakingUpdate};

impl<T: Config> DelegationInterface for Pallet<T> {
type Balance = BalanceOf<T>;
Expand Down
Loading

0 comments on commit feacf2f

Please sign in to comment.