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.

Sign up for GitHub

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

[stable2409] Backport #6540 #6592

Merged
merged 3 commits into from
Dec 9, 2024
Merged

[stable2409] Backport #6540 #6592

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4d15745
[Fix|NominationPools] Only allow apply slash to be executed if the sl…
Ank4n Nov 21, 2024
df6317c
rename prdoc
Ank4n Dec 4, 2024
99f6401
Merge branch 'stable2409' into backport-6540-to-stable2409
Ank4n Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions prdoc/pr_6540.prdoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# 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: Only allow apply slash to be executed if the slash amount is atleast ED

doc:
- audience: Runtime User
description: |
This change prevents `pools::apply_slash` from being executed when the pending slash amount of the member is lower
than the ED. With this change, such small slashes will still be applied but only when member funds are withdrawn.

crates:
- name: pallet-nomination-pools-runtime-api
bump: patch
- name: pallet-nomination-pools
bump: major
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we currently deal with backporting major changes? Something smells about that, depending how far back we go

Copy link
Contributor

Choose a reason for hiding this comment

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

@seadanda I assume it will only patch the existing release.

Also, practically this may not be a breaking change (I think). The client gets the new type information from the metadata, and the module error is only really for error description. Changes in Storage or RuntimeCall are more critical as some dapp's business logic may depend on it.

I could make one of the following change if that helps:

  • Add the new error variant to the end so existing indexes are not affected.
  • The backport version does not add the new error and reuses one of the existing errors.

3 changes: 3 additions & 0 deletions substrate/frame/nomination-pools/runtime-api/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ sp_api::decl_runtime_apis! {
fn pool_pending_slash(pool_id: PoolId) -> Balance;

/// Returns the pending slash for a given pool member.
///
/// If pending slash of the member exceeds `ExistentialDeposit`, it can be reported on
/// chain.
fn member_pending_slash(member: AccountId) -> Balance;

/// Returns true if the pool with `pool_id` needs migration.
Expand Down
23 changes: 18 additions & 5 deletions substrate/frame/nomination-pools/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1944,6 +1944,8 @@ pub mod pallet {
NothingToAdjust,
/// No slash pending that can be applied to the member.
NothingToSlash,
/// The slash amount is too low to be applied.
SlashTooLow,
/// The pool or member delegation has already migrated to delegate stake.
AlreadyMigrated,
/// The pool or member delegation has not migrated yet to delegate stake.
Expand Down Expand Up @@ -2300,7 +2302,7 @@ pub mod pallet {

let slash_weight =
// apply slash if any before withdraw.
match Self::do_apply_slash(&member_account, None) {
match Self::do_apply_slash(&member_account, None, false) {
Ok(_) => T::WeightInfo::apply_slash(),
Err(e) => {
let no_pending_slash: DispatchResult = Err(Error::<T>::NothingToSlash.into());
Expand Down Expand Up @@ -2974,8 +2976,10 @@ pub mod pallet {
/// Fails unless [`crate::pallet::Config::StakeAdapter`] is of strategy type:
/// [`adapter::StakeStrategyType::Delegate`].
///
/// This call can be dispatched permissionlessly (i.e. by any account). If the member has
/// slash to be applied, caller may be rewarded with the part of the slash.
/// The pending slash amount of the member must be equal or more than `ExistentialDeposit`.
/// This call can be dispatched permissionlessly (i.e. by any account). If the execution
/// is successful, fee is refunded and caller may be rewarded with a part of the slash
/// based on the [`crate::pallet::Config::StakeAdapter`] configuration.
#[pallet::call_index(23)]
#[pallet::weight(T::WeightInfo::apply_slash())]
pub fn apply_slash(
Expand All @@ -2989,7 +2993,7 @@ pub mod pallet {

let who = ensure_signed(origin)?;
let member_account = T::Lookup::lookup(member_account)?;
Self::do_apply_slash(&member_account, Some(who))?;
Self::do_apply_slash(&member_account, Some(who), true)?;

// If successful, refund the fees.
Ok(Pays::No.into())
Expand Down Expand Up @@ -3574,15 +3578,21 @@ impl<T: Config> Pallet<T> {
fn do_apply_slash(
member_account: &T::AccountId,
reporter: Option<T::AccountId>,
enforce_min_slash: bool,
) -> DispatchResult {
let member = PoolMembers::<T>::get(member_account).ok_or(Error::<T>::PoolMemberNotFound)?;

let pending_slash =
Self::member_pending_slash(Member::from(member_account.clone()), member.clone())?;

// if nothing to slash, return error.
// ensure there is something to slash.
ensure!(!pending_slash.is_zero(), Error::<T>::NothingToSlash);

if enforce_min_slash {
// ensure slashed amount is at least the minimum balance.
ensure!(pending_slash >= T::Currency::minimum_balance(), Error::<T>::SlashTooLow);
}

T::StakeAdapter::member_slash(
Member::from(member_account.clone()),
Pool::from(Pallet::<T>::generate_bonded_account(member.pool_id)),
Expand Down Expand Up @@ -3946,6 +3956,9 @@ impl<T: Config> Pallet<T> {
/// Returns the unapplied slash of a member.
///
/// Pending slash is only applicable with [`adapter::DelegateStake`] strategy.
///
/// If pending slash of the member exceeds `ExistentialDeposit`, it can be reported on
/// chain via [`Call::apply_slash`].
pub fn api_member_pending_slash(who: T::AccountId) -> BalanceOf<T> {
PoolMembers::<T>::get(who.clone())
.map(|pool_member| {
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/nomination-pools/test-delegate-stake/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ sp-staking = { default-features = true, path = "../../../primitives/staking" }
sp-core = { default-features = true, path = "../../../primitives/core" }

frame-system = { default-features = true, path = "../../system" }
frame-support = { default-features = true, path = "../../support" }
frame-support = { features = ["experimental"], default-features = true, path = "../../support" }
frame-election-provider-support = { default-features = true, path = "../../election-provider-support" }

pallet-timestamp = { default-features = true, path = "../../timestamp" }
Expand Down
35 changes: 32 additions & 3 deletions substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
mod mock;

use frame_support::{
assert_noop, assert_ok,
assert_noop, assert_ok, hypothetically,
traits::{fungible::InspectHold, Currency},
};
use mock::*;
Expand Down Expand Up @@ -537,9 +537,9 @@ fn pool_slash_proportional() {
// a typical example where 3 pool members unbond in era 99, 100, and 101, and a slash that
// happened in era 100 should only affect the latter two.
new_test_ext().execute_with(|| {
ExistentialDeposit::set(1);
ExistentialDeposit::set(2);
BondingDuration::set(28);
assert_eq!(Balances::minimum_balance(), 1);
assert_eq!(Balances::minimum_balance(), 2);
assert_eq!(Staking::current_era(), None);

// create the pool, we know this has id 1.
Expand Down Expand Up @@ -670,6 +670,34 @@ fn pool_slash_proportional() {

// no pending slash yet.
assert_eq!(Pools::api_pool_pending_slash(1), 0);
// and therefore applying slash fails
assert_noop!(
Pools::apply_slash(RuntimeOrigin::signed(10), 21),
PoolsError::<Runtime>::NothingToSlash
);

hypothetically!({
// a very small amount is slashed
pallet_staking::slashing::do_slash::<Runtime>(
&POOL1_BONDED,
3,
&mut Default::default(),
&mut Default::default(),
100,
);

// ensure correct amount is pending to be slashed
assert_eq!(Pools::api_pool_pending_slash(1), 3);

// 21 has pending slash lower than ED (2)
assert_eq!(Pools::api_member_pending_slash(21), 1);

// slash fails as minimum pending slash amount not met.
assert_noop!(
Pools::apply_slash(RuntimeOrigin::signed(10), 21),
PoolsError::<Runtime>::SlashTooLow
);
});

pallet_staking::slashing::do_slash::<Runtime>(
&POOL1_BONDED,
Expand Down Expand Up @@ -909,6 +937,7 @@ fn pool_slash_non_proportional_bonded_pool_and_chunks() {
);
});
}

#[test]
fn pool_migration_e2e() {
new_test_ext().execute_with(|| {
Expand Down
Loading