diff --git a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/staking.rs b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/staking.rs index cbe3a438225d8..a5b5093ed3eb9 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/staking.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/staking.rs @@ -264,7 +264,6 @@ impl pallet_staking_async::Config for Runtime { type EventListeners = (NominationPools, DelegatedStaking); type WeightInfo = (); type MaxInvulnerables = frame_support::traits::ConstU32<20>; - type MaxDisabledValidators = ConstU32<100>; type PlanningEraOffset = PlanningEraOffset; type RcClientInterface = StakingRcClient; type MaxEraDuration = MaxEraDuration; diff --git a/prdoc/pr_8937.prdoc b/prdoc/pr_8937.prdoc new file mode 100644 index 0000000000000..46cd189de4aa6 --- /dev/null +++ b/prdoc/pr_8937.prdoc @@ -0,0 +1,17 @@ +title: '[Staking] [AHM] Fixes insufficient slashing of nominators and some other small + issues.' +doc: +- audience: Runtime Dev + description: |- + ## Removed + - Config constant `MaxDisabledValidators`: This constant was removed since validator disabling logic has been moved to pallet-session, making it redundant in staking-async. + - Storage DoubleMap `NominatorSlashInEra`: This was used to track per-era maximum slashes for nominators. It’s no longer required — we now only track the highest slash per validator per era. + - Call `withdraw_overstake`: This was a temporary extrinsic meant to fix overstake issues, but with fungible migration of staking funds, this is no longer possible and the extrinsic is obsolete. + + ## Changed + - Nominator slashing logic: The logic now aggregates slashes from distinct offending validators nominated by a nominator within the same era. For repeated offences by the same validator, only the highest slash fraction is applied. Previously, the pallet applied only the highest slash across all validators, regardless of how many were slashed. +crates: +- name: pallet-staking-async + bump: major +- name: pallet-staking-async-parachain-runtime + bump: major diff --git a/substrate/frame/staking-async/ahm-test/src/ah/mock.rs b/substrate/frame/staking-async/ahm-test/src/ah/mock.rs index 5d285725dcc7f..aee4e304ef747 100644 --- a/substrate/frame/staking-async/ahm-test/src/ah/mock.rs +++ b/substrate/frame/staking-async/ahm-test/src/ah/mock.rs @@ -345,7 +345,6 @@ impl pallet_staking_async::Config for Runtime { type HistoryDepth = ConstU32<7>; type MaxControllersInDeprecationBatch = (); - type MaxDisabledValidators = MaxValidators; type MaxValidatorSet = MaxValidators; type MaxExposurePageSize = MaxExposurePageSize; type MaxInvulnerables = MaxValidators; diff --git a/substrate/frame/staking-async/runtimes/parachain/src/staking.rs b/substrate/frame/staking-async/runtimes/parachain/src/staking.rs index 84bddf928f724..fcb724bc5da72 100644 --- a/substrate/frame/staking-async/runtimes/parachain/src/staking.rs +++ b/substrate/frame/staking-async/runtimes/parachain/src/staking.rs @@ -280,7 +280,6 @@ impl pallet_staking_async::Config for Runtime { type WeightInfo = weights::pallet_staking_async::WeightInfo; type MaxInvulnerables = frame_support::traits::ConstU32<20>; type MaxEraDuration = MaxEraDuration; - type MaxDisabledValidators = ConstU32<100>; type PlanningEraOffset = pallet_staking_async::PlanningEraOffsetOf>; type RcClientInterface = StakingRcClient; diff --git a/substrate/frame/staking-async/src/mock.rs b/substrate/frame/staking-async/src/mock.rs index 674212649206e..122c568c4d90e 100644 --- a/substrate/frame/staking-async/src/mock.rs +++ b/substrate/frame/staking-async/src/mock.rs @@ -436,7 +436,6 @@ impl crate::pallet::pallet::Config for Test { type MaxControllersInDeprecationBatch = MaxControllersInDeprecationBatch; type EventListeners = EventListenerMock; type MaxInvulnerables = ConstU32<20>; - type MaxDisabledValidators = ConstU32<100>; type MaxEraDuration = MaxEraDuration; type PlanningEraOffset = PlanningEraOffset; type Filter = MockedRestrictList; diff --git a/substrate/frame/staking-async/src/pallet/mod.rs b/substrate/frame/staking-async/src/pallet/mod.rs index 30c2799ec14c6..5a30ae00362dd 100644 --- a/substrate/frame/staking-async/src/pallet/mod.rs +++ b/substrate/frame/staking-async/src/pallet/mod.rs @@ -308,10 +308,6 @@ pub mod pallet { #[pallet::constant] type MaxInvulnerables: Get; - /// Maximum number of disabled validators. - #[pallet::constant] - type MaxDisabledValidators: Get; - /// Maximum allowed era duration in milliseconds. /// /// This provides a defensive upper bound to cap the effective era duration, preventing @@ -383,7 +379,6 @@ pub mod pallet { type MaxValidatorSet = ConstU32<100>; type MaxControllersInDeprecationBatch = ConstU32<100>; type MaxInvulnerables = ConstU32<20>; - type MaxDisabledValidators = ConstU32<100>; type MaxEraDuration = (); type EventListeners = (); type Filter = Nothing; @@ -782,11 +777,6 @@ pub mod pallet { (Perbill, BalanceOf), >; - /// All slashing events on nominators, mapped by era to the highest slash value of the era. - #[pallet::storage] - pub type NominatorSlashInEra = - StorageDoubleMap<_, Twox64Concat, EraIndex, Twox64Concat, T::AccountId, BalanceOf>; - /// The threshold for when users can start calling `chill_other` for other validators / /// nominators. The threshold is compared to the actual number of validators / nominators /// (`CountFor*`) in the system compared to the configured max (`Max*Count`). @@ -2488,38 +2478,5 @@ pub mod pallet { Ok(Pays::No.into()) } - - /// Adjusts the staking ledger by withdrawing any excess staked amount. - /// - /// This function corrects cases where a user's recorded stake in the ledger - /// exceeds their actual staked funds. This situation can arise due to cases such as - /// external slashing by another pallet, leading to an inconsistency between the ledger - /// and the actual stake. - #[pallet::call_index(32)] - #[pallet::weight(T::DbWeight::get().reads_writes(2, 1))] - pub fn withdraw_overstake(origin: OriginFor, stash: T::AccountId) -> DispatchResult { - use sp_runtime::Saturating; - let _ = ensure_signed(origin)?; - - let ledger = Self::ledger(Stash(stash.clone()))?; - let actual_stake = asset::staked::(&stash); - let force_withdraw_amount = ledger.total.defensive_saturating_sub(actual_stake); - - // ensure there is something to force unstake. - ensure!(!force_withdraw_amount.is_zero(), Error::::BoundNotMet); - - // we ignore if active is 0. It implies the locked amount is not actively staked. The - // account can still get away from potential slash, but we can't do much better here. - StakingLedger { - total: actual_stake, - active: ledger.active.saturating_sub(force_withdraw_amount), - ..ledger - } - .update()?; - - Self::deposit_event(Event::::Withdrawn { stash, amount: force_withdraw_amount }); - - Ok(()) - } } } diff --git a/substrate/frame/staking-async/src/slashing.rs b/substrate/frame/staking-async/src/slashing.rs index 38212a6e7b610..ad8c62910cf2d 100644 --- a/substrate/frame/staking-async/src/slashing.rs +++ b/substrate/frame/staking-async/src/slashing.rs @@ -41,10 +41,9 @@ //! Based on research at use crate::{ - asset, log, session_rotation::Eras, BalanceOf, Config, NegativeImbalanceOf, - NominatorSlashInEra, OffenceQueue, OffenceQueueEras, PagedExposure, Pallet, Perbill, - ProcessingOffence, SlashRewardFraction, UnappliedSlash, UnappliedSlashes, ValidatorSlashInEra, - WeightInfo, + asset, log, session_rotation::Eras, BalanceOf, Config, NegativeImbalanceOf, OffenceQueue, + OffenceQueueEras, PagedExposure, Pallet, Perbill, ProcessingOffence, SlashRewardFraction, + UnappliedSlash, UnappliedSlashes, ValidatorSlashInEra, WeightInfo, }; use alloc::vec::Vec; use codec::{Decode, Encode, MaxEncodedLen}; @@ -178,7 +177,8 @@ fn next_offence() -> Option<(EraIndex, T::AccountId, OffenceRecord() -> Weight { - // We do manual weight racking for early-returns, and use benchmarks for the final two branches. + // We do manual weight tracking for early-returns, and use benchmarks for the final two + // branches. let mut incomplete_consumed_weight = Weight::from_parts(0, 0); let mut add_db_reads_writes = |reads, writes| { incomplete_consumed_weight += T::DbWeight::get().reads_writes(reads, writes); @@ -351,36 +351,31 @@ fn slash_nominators( nominators_slashed.reserve(params.exposure.exposure_page.others.len()); for nominator in ¶ms.exposure.exposure_page.others { let stash = &nominator.who; - let prior_slashed = - NominatorSlashInEra::::get(¶ms.slash_era, stash).unwrap_or_else(Zero::zero); + let prior_slashed = params.prior_slash * nominator.value; let new_slash = params.slash * nominator.value; - let slash_due = new_slash.saturating_sub(prior_slashed); + // this should always be positive since prior slash is always less than the new slash or + // filtered out when offence is reported (`Pallet::on_new_offences`). + let slash_diff = new_slash.defensive_saturating_sub(prior_slashed); - if new_slash == Zero::zero() { + if slash_diff == Zero::zero() { // nothing to do continue } log!( debug, - "🦹 slashing nominator {:?} of stake: {:?} for {:?} in era {:?}", + "🦹 slashing nominator {:?} of stake: {:?} for {:?} in era {:?}. Prior Slash: {:?}, New Slash: {:?}", stash, nominator.value, - slash_due, + slash_diff, params.slash_era, + params.prior_slash, + params.slash, ); - // the era slash of a nominator always grows, if the validator had a new max slash for the - // era. - NominatorSlashInEra::::insert( - ¶ms.slash_era, - stash, - prior_slashed.saturating_add(slash_due), - ); - - nominators_slashed.push((stash.clone(), slash_due)); - total_slashed.saturating_accrue(slash_due); - reward_payout.saturating_accrue(params.reward_proportion * slash_due); + nominators_slashed.push((stash.clone(), slash_diff)); + total_slashed.saturating_accrue(slash_diff); + reward_payout.saturating_accrue(params.reward_proportion * slash_diff); } (total_slashed, reward_payout) @@ -390,8 +385,6 @@ fn slash_nominators( pub(crate) fn clear_era_metadata(obsolete_era: EraIndex) { #[allow(deprecated)] ValidatorSlashInEra::::remove_prefix(&obsolete_era, None); - #[allow(deprecated)] - NominatorSlashInEra::::remove_prefix(&obsolete_era, None); } // apply the slash to a stash account, deducting any missing funds from the reward diff --git a/substrate/frame/staking-async/src/tests/bonding.rs b/substrate/frame/staking-async/src/tests/bonding.rs index d3885b9002656..28fc6c7229624 100644 --- a/substrate/frame/staking-async/src/tests/bonding.rs +++ b/substrate/frame/staking-async/src/tests/bonding.rs @@ -17,7 +17,6 @@ use super::*; use frame_support::{hypothetically_ok, traits::Currency}; -use sp_staking::{Stake, StakingInterface}; #[test] fn existing_stash_cannot_bond() { @@ -1073,73 +1072,7 @@ fn restricted_accounts_can_only_withdraw() { }) } -#[test] -fn permissionless_withdraw_overstake() { - ExtBuilder::default().build_and_execute(|| { - // Given Alice, Bob and Charlie with some stake. - let alice = 301; - let bob = 302; - let charlie = 303; - let _ = Balances::make_free_balance_be(&alice, 500); - let _ = Balances::make_free_balance_be(&bob, 500); - let _ = Balances::make_free_balance_be(&charlie, 500); - assert_ok!(Staking::bond(RuntimeOrigin::signed(alice), 100, RewardDestination::Staked)); - assert_ok!(Staking::bond(RuntimeOrigin::signed(bob), 100, RewardDestination::Staked)); - assert_ok!(Staking::bond(RuntimeOrigin::signed(charlie), 100, RewardDestination::Staked)); - - // WHEN: charlie is partially unbonding. - assert_ok!(Staking::unbond(RuntimeOrigin::signed(charlie), 90)); - let charlie_ledger = StakingLedger::::get(StakingAccount::Stash(charlie)).unwrap(); - - // AND: alice and charlie ledger having higher value than actual stake. - Ledger::::insert(alice, StakingLedger::::new(alice, 200)); - Ledger::::insert( - charlie, - StakingLedger { stash: charlie, total: 200, active: 200 - 90, ..charlie_ledger }, - ); - - // THEN overstake can be permissionlessly withdrawn. - let _ = staking_events_since_last_call(); - - // Alice stake is corrected. - assert_eq!( - ::stake(&alice).unwrap(), - Stake { total: 200, active: 200 } - ); - assert_ok!(Staking::withdraw_overstake(RuntimeOrigin::signed(1), alice)); - assert_eq!( - ::stake(&alice).unwrap(), - Stake { total: 100, active: 100 } - ); - - // Charlie who is partially withdrawing also gets their stake corrected. - assert_eq!( - ::stake(&charlie).unwrap(), - Stake { total: 200, active: 110 } - ); - assert_ok!(Staking::withdraw_overstake(RuntimeOrigin::signed(1), charlie)); - assert_eq!( - ::stake(&charlie).unwrap(), - Stake { total: 200 - 100, active: 110 - 100 } - ); - - assert_eq!( - staking_events_since_last_call(), - vec![ - Event::Withdrawn { stash: alice, amount: 200 - 100 }, - Event::Withdrawn { stash: charlie, amount: 200 - 100 } - ] - ); - - // but Bob ledger is fine and that cannot be withdrawn. - assert_noop!( - Staking::withdraw_overstake(RuntimeOrigin::signed(1), bob), - Error::::BoundNotMet - ); - }); -} - -mod rebobd { +mod rebond { use super::*; #[test] diff --git a/substrate/frame/staking-async/src/tests/slashing.rs b/substrate/frame/staking-async/src/tests/slashing.rs index 9322b3ec76388..c55004cc9c409 100644 --- a/substrate/frame/staking-async/src/tests/slashing.rs +++ b/substrate/frame/staking-async/src/tests/slashing.rs @@ -628,115 +628,145 @@ fn only_slash_validator_for_max_in_era() { } #[test] -fn only_slash_nominator_for_max_in_era() { +fn nominator_is_slashed_by_max_for_validator_in_era() { ExtBuilder::default().build_and_execute(|| { Session::roll_until_active_era(3); - assert_eq!(asset::stakeable_balance::(&11), 1000); - assert_eq!(asset::stakeable_balance::(&21), 1000); - assert_eq!(asset::stakeable_balance::(&101), 500); - assert_eq!(Staking::slashable_balance_of(&21), 1000); + // Validators 11 and 21, Nominator 101 exposed to both. + let validator_one = 11; + let validator_two = 21; + let nominator = 101; - let exposure_11 = Staking::eras_stakers(active_era(), &11); - let exposure_21 = Staking::eras_stakers(active_era(), &21); - let nominated_value_11 = exposure_11.others.iter().find(|o| o.who == 101).unwrap().value; - let nominated_value_21 = exposure_21.others.iter().find(|o| o.who == 101).unwrap().value; + assert_eq!(asset::stakeable_balance::(&validator_one), 1000); + assert_eq!(asset::stakeable_balance::(&validator_two), 1000); + assert_eq!(asset::stakeable_balance::(&nominator), 500); + assert_eq!(Staking::slashable_balance_of(&validator_two), 1000); + + let exposure_v1 = Staking::eras_stakers(active_era(), &11); + let exposure_v2 = Staking::eras_stakers(active_era(), &21); + let nominated_value_v1 = exposure_v1.others.iter().find(|o| o.who == 101).unwrap().value; + let nominated_value_v2 = exposure_v2.others.iter().find(|o| o.who == 101).unwrap().value; // clear staking events until now staking_events_since_last_call(); // First slash - add_slash_in_era(11, 2, Perbill::from_percent(10)); + let slash_era = 2; + add_slash_in_era(validator_one, slash_era, Perbill::from_percent(10)); Session::roll_next(); - let slash_11_amount = Perbill::from_percent(10) * 1000u128; - assert_eq!(slash_11_amount, 100); - let first_slash_101_amount = Perbill::from_percent(10) * nominated_value_11; - assert_eq!(first_slash_101_amount, 25); + let slash_v1_amount = Perbill::from_percent(10) * 1000u128; + assert_eq!(slash_v1_amount, 100); + let first_slash_nominator_amount = Perbill::from_percent(10) * nominated_value_v1; + assert_eq!(first_slash_nominator_amount, 25); assert_eq!( staking_events_since_last_call(), vec![ Event::OffenceReported { - offence_era: 2, - validator: 11, + offence_era: slash_era, + validator: validator_one, fraction: Perbill::from_percent(10) }, - Event::SlashComputed { offence_era: 2, slash_era: 2, offender: 11, page: 0 }, - Event::Slashed { staker: 11, amount: slash_11_amount }, - Event::Slashed { staker: 101, amount: first_slash_101_amount } + Event::SlashComputed { + offence_era: slash_era, + slash_era, + offender: validator_one, + page: 0 + }, + Event::Slashed { staker: validator_one, amount: slash_v1_amount }, + Event::Slashed { staker: nominator, amount: first_slash_nominator_amount } ] ); - assert_eq!(asset::stakeable_balance::(&11), 1000 - slash_11_amount); - - let slash_1_amount = Perbill::from_percent(10) * nominated_value_11; - assert_eq!(asset::stakeable_balance::(&101), 500 - slash_1_amount); + assert_eq!(asset::stakeable_balance::(&validator_one), 1000 - slash_v1_amount); + assert_eq!(asset::stakeable_balance::(&101), 500 - first_slash_nominator_amount); // Second slash: higher value, same era. - add_slash_in_era(21, 2, Perbill::from_percent(30)); + add_slash_in_era(validator_two, slash_era, Perbill::from_percent(30)); Session::roll_next(); - let slash_21_amount = Perbill::from_percent(30) * 1000u128; - assert_eq!(slash_21_amount, 300); - let second_slash_101_amount = - Perbill::from_percent(30) * nominated_value_21 - first_slash_101_amount; - assert_eq!(second_slash_101_amount, 50); + let slash_v2_amount = Perbill::from_percent(30) * 1000u128; + assert_eq!(slash_v2_amount, 300); + // full nominator value is slashed, even though nominator was already slashed in this era. + let second_slash_nominator_amount = Perbill::from_percent(30) * nominated_value_v2; + assert_eq!(second_slash_nominator_amount, 75); assert_eq!( staking_events_since_last_call(), vec![ Event::OffenceReported { - offence_era: 2, - validator: 21, + offence_era: slash_era, + validator: validator_two, fraction: Perbill::from_percent(30) }, - Event::SlashComputed { offence_era: 2, slash_era: 2, offender: 21, page: 0 }, - Event::Slashed { staker: 21, amount: slash_21_amount }, - Event::Slashed { staker: 101, amount: second_slash_101_amount } + Event::SlashComputed { + offence_era: slash_era, + slash_era, + offender: validator_two, + page: 0 + }, + Event::Slashed { staker: validator_two, amount: slash_v2_amount }, + Event::Slashed { staker: nominator, amount: second_slash_nominator_amount } ] ); // 11 was not further slashed, but 21 and 101 were. - assert_eq!(asset::stakeable_balance::(&11), 900); - assert_eq!(asset::stakeable_balance::(&21), 1000 - slash_21_amount); - - let slash_2_amount = Perbill::from_percent(30) * nominated_value_21; - assert!(slash_2_amount > slash_1_amount); - - // only the maximum slash in a single span is taken. - assert_eq!(asset::stakeable_balance::(&101), 500 - slash_2_amount); + assert_eq!(asset::stakeable_balance::(&validator_one), 900); + let v2_stakeable = asset::stakeable_balance::(&validator_two); + assert_eq!(v2_stakeable, 1000 - slash_v2_amount); + // 101 is slashed twice. + let nominator_slashable_balance = Staking::slashable_balance_of(&101); + assert_eq!( + nominator_slashable_balance, + 500 - first_slash_nominator_amount - second_slash_nominator_amount + ); // Third slash: in same era and on same validator as first, higher in-era value, but lower // slash value than slash 2. - add_slash_in_era(11, 2, Perbill::from_percent(20)); + add_slash_in_era(validator_one, slash_era, Perbill::from_percent(20)); Session::roll_next(); + + // the slash perbill delta is (first: 20 - second: 10) = 10% for v1 + let third_slash_nominator_amount = Perbill::from_percent(10) * nominated_value_v1; assert_eq!( staking_events_since_last_call(), vec![ Event::OffenceReported { - offence_era: 2, - validator: 11, + offence_era: slash_era, + validator: validator_one, fraction: Perbill::from_percent(20), }, - Event::SlashComputed { offence_era: 2, slash_era: 2, offender: 11, page: 0 }, + Event::SlashComputed { + offence_era: slash_era, + slash_era, + offender: validator_one, + page: 0 + }, Event::Slashed { - staker: 11, - amount: Perbill::from_percent(10) * 1000u128, // the delta is 10% + staker: validator_one, + amount: Perbill::from_percent(10) * 1000u128, // the slash perbill delta is 10% + }, + Event::Slashed { + staker: nominator, + // the slash perbill delta is 10% for v1 + amount: third_slash_nominator_amount, }, ] ); - // 11 was further slashed, but 21 and 101 were not. - assert_eq!(asset::stakeable_balance::(&11), 800); - assert_eq!(asset::stakeable_balance::(&21), 700); - - let slash_3_amount = Perbill::from_percent(20) * nominated_value_21; - assert!(slash_3_amount < slash_2_amount); - assert!(slash_3_amount > slash_1_amount); - - // only the maximum slash in a single span is taken. - assert_eq!(asset::stakeable_balance::(&101), 500 - slash_2_amount); + // 11 and 101 was further slashed, but 21 was not. + assert_eq!( + asset::stakeable_balance::(&validator_one), + 1000 - slash_v1_amount - (Perbill::from_percent(10) * 1000u128) + ); + assert_eq!( + asset::stakeable_balance::(&nominator), + 500 - first_slash_nominator_amount - + second_slash_nominator_amount - + third_slash_nominator_amount + ); + assert_eq!(asset::stakeable_balance::(&21), v2_stakeable); }); } @@ -773,7 +803,7 @@ fn fully_slashed_account_can_be_reaped() { #[test] fn garbage_collection_on_window_pruning() { - // ensures that `ValidatorSlashInEra` and `NominatorSlashInEra` are cleared after + // ensures that `ValidatorSlashInEra` are cleared after // `BondingDuration`. ExtBuilder::default().build_and_execute(|| { assert_eq!(asset::stakeable_balance::(&11), 1000); @@ -790,18 +820,14 @@ fn garbage_collection_on_window_pruning() { assert_eq!(asset::stakeable_balance::(&101), 500 - (nominated_value / 10)); assert!(ValidatorSlashInEra::::get(&now, &11).is_some()); - assert!(NominatorSlashInEra::::get(&now, &101).is_some()); // + 1 because we have to exit the bonding window. for era in (0..(BondingDuration::get() + 1)).map(|offset| offset + now + 1) { assert!(ValidatorSlashInEra::::get(&now, &11).is_some()); - assert!(NominatorSlashInEra::::get(&now, &101).is_some()); - Session::roll_until_active_era(era); } assert!(ValidatorSlashInEra::::get(&now, &11).is_none()); - assert!(NominatorSlashInEra::::get(&now, &101).is_none()); }) }