From c87a5e1b4590bd92f3b466d8cd24b46a1a4d1768 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Sun, 18 Dec 2022 14:56:10 +0000 Subject: [PATCH 1/9] Fix fast-unstake for accounts with slashing --- frame/staking/src/pallet/impls.rs | 2 +- frame/staking/src/tests.rs | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index a7190d70c7061..db9aeba6fb58e 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -1569,7 +1569,7 @@ impl StakingInterface for Pallet { } fn force_unstake(who: Self::AccountId) -> sp_runtime::DispatchResult { - let num_slashing_spans = Self::slashing_spans(&who).iter().count() as u32; + let num_slashing_spans = Self::slashing_spans(&who).map_or(0, |s| s.iter().count() as u32); Self::force_unstake(RawOrigin::Root.into(), who.clone(), num_slashing_spans) } diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index fc6fc68e66d5d..21a0bb98de859 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -5725,3 +5725,28 @@ fn scale_validator_count_errors() { ); }) } + +mod staking_interface { + use frame_support::storage::with_storage_layer; +use sp_staking::StakingInterface; + +use super::*; + + #[test] + fn force_unstake_with_slash_works() { + ExtBuilder::default().build_and_execute(|| { + // without slash + let _ = with_storage_layer::<(), _, _>(|| { + // bond an account, can unstake + assert_eq!(Staking::bonded(&11), Some(10)); + assert_ok!(::force_unstake(11)); + Err(DispatchError::from("revert")) + }); + + // bond again and add a slash, still can unstake. + assert_eq!(Staking::bonded(&11), Some(10)); + add_slash(&11); + assert_ok!(::force_unstake(11)); + }); + } +} From 2ee07f77adc3aef0939234b973429e6e6abaf6af Mon Sep 17 00:00:00 2001 From: command-bot <> Date: Sun, 18 Dec 2022 14:59:58 +0000 Subject: [PATCH 2/9] ".git/.scripts/fmt.sh" 1 --- frame/staking/src/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 21a0bb98de859..793659f2a8bc6 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -5728,9 +5728,9 @@ fn scale_validator_count_errors() { mod staking_interface { use frame_support::storage::with_storage_layer; -use sp_staking::StakingInterface; + use sp_staking::StakingInterface; -use super::*; + use super::*; #[test] fn force_unstake_with_slash_works() { From daa756d1745d39278cb972911eefa306ea94eeaa Mon Sep 17 00:00:00 2001 From: kianenigma Date: Sun, 18 Dec 2022 15:00:02 +0000 Subject: [PATCH 3/9] fmt --- frame/staking/src/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 21a0bb98de859..793659f2a8bc6 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -5728,9 +5728,9 @@ fn scale_validator_count_errors() { mod staking_interface { use frame_support::storage::with_storage_layer; -use sp_staking::StakingInterface; + use sp_staking::StakingInterface; -use super::*; + use super::*; #[test] fn force_unstake_with_slash_works() { From 3a0f49a4e58ef3939444f8ebfba2bb5ae5160c64 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Mon, 19 Dec 2022 09:19:55 +0000 Subject: [PATCH 4/9] fix --- frame/staking/src/pallet/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 1d5babe7ffa8f..7c40f4b8f436b 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -982,7 +982,7 @@ pub mod pallet { // `BondingDuration` to proceed with the unbonding. let maybe_withdraw_weight = { if unlocking == T::MaxUnlockingChunks::get() as usize { - let real_num_slashing_spans = Self::slashing_spans(&controller).iter().count(); + let real_num_slashing_spans = Self::slashing_spans(&controller).map_or(0, |s| s.iter().count()); Some(Self::do_withdraw_unbonded(&controller, real_num_slashing_spans as u32)?) } else { None From ab3609a2332eee853d3411dbaa41b6e5ae6a4490 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Mon, 19 Dec 2022 18:27:09 +0000 Subject: [PATCH 5/9] fix weight tracking --- frame/fast-unstake/src/lib.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/frame/fast-unstake/src/lib.rs b/frame/fast-unstake/src/lib.rs index 7f226826cbc53..89e627b2ab23d 100644 --- a/frame/fast-unstake/src/lib.rs +++ b/frame/fast-unstake/src/lib.rs @@ -81,6 +81,7 @@ macro_rules! log { pub mod pallet { use super::*; use crate::types::*; + use sp_std::collections::btree_set::BTreeSet; use frame_support::{ pallet_prelude::*, traits::{Defensive, ReservableCurrency, StorageVersion}, @@ -429,9 +430,9 @@ pub mod pallet { } }; - let check_stash = |stash, deposit, eras_checked: &mut u32| { + let check_stash = |stash, deposit, eras_checked: &mut BTreeSet| { let is_exposed = unchecked_eras_to_check.iter().any(|e| { - eras_checked.saturating_inc(); + eras_checked.insert(*e); T::Staking::is_exposed_in_era(&stash, e) }); @@ -452,7 +453,7 @@ pub mod pallet { ::WeightInfo::on_idle_unstake() } else { // eras checked so far. - let mut eras_checked = 0u32; + let mut eras_checked = BTreeSet::::new(); let pre_length = stashes.len(); let stashes: BoundedVec<(T::AccountId, BalanceOf), T::BatchSize> = stashes @@ -468,7 +469,7 @@ pub mod pallet { log!( debug, "checked {:?} eras, pre stashes: {:?}, post: {:?}", - eras_checked, + eras_checked.len(), pre_length, post_length, ); @@ -489,7 +490,7 @@ pub mod pallet { }, } - ::WeightInfo::on_idle_check(validator_count * eras_checked) + ::WeightInfo::on_idle_check(validator_count * eras_checked.len() as u32) } } } From 303e930e4795359dd36b0e6b4a3a789f678e3431 Mon Sep 17 00:00:00 2001 From: gpestana Date: Wed, 21 Dec 2022 16:42:12 +0100 Subject: [PATCH 6/9] Adds tests for withdraw_unbonded with slashing --- frame/staking/src/tests.rs | 48 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 793659f2a8bc6..e379e127c545d 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -5749,4 +5749,52 @@ mod staking_interface { assert_ok!(::force_unstake(11)); }); } + + #[test] + fn withdraw_unbonded_with_slash_works() { + ExtBuilder::default().build_and_execute(|| { + let _ = with_storage_layer::<(), _, _>(|| { + assert_ok!(Staking::unbond(RuntimeOrigin::signed(10), 1)); + Err(DispatchError::from("revert")) + }); + + add_slash(&10); + let mut current_era = 0; + assert_ok!(Staking::unbond(RuntimeOrigin::signed(10), 1)); + let max_unlocking_chunks = <::MaxUnlockingChunks as Get>::get(); + + // Fill up all unlocking chunks. + for i in 0..max_unlocking_chunks - 1 { + current_era = i as u32; + mock::start_active_era(current_era); + assert_ok!(Staking::unbond(RuntimeOrigin::signed(10), 1)); + } + + current_era += 1; + mock::start_active_era(current_era); + + // This chunk is locked at `current_era` through `current_era + 2` (because + // `BondingDuration` == 3). + assert_ok!(Staking::unbond(RuntimeOrigin::signed(10), 1)); + assert_eq!( + Staking::ledger(&10).map(|l| l.unlocking.len()).unwrap(), + <::MaxUnlockingChunks as Get>::get() as usize + ); + + // even though the number of unlocked chunks is the same as `MaxUnlockingChunks`, + // unbonding works as expected. + for i in current_era..(current_era + max_unlocking_chunks) - 1 { + // There is only 1 chunk per era, so we need to be in a new era to create a chunk. + current_era = i as u32; + mock::start_active_era(current_era); + assert_ok!(Staking::unbond(RuntimeOrigin::signed(10), 1)); + } + + // only slots within last `BondingDuration` are filled. + assert_eq!( + Staking::ledger(&10).map(|l| l.unlocking.len()).unwrap(), + <::BondingDuration>::get() as usize + ); + }) + } } From b529e422154c427acc535135c04fc3e422072d23 Mon Sep 17 00:00:00 2001 From: gpestana Date: Thu, 22 Dec 2022 15:20:48 +0100 Subject: [PATCH 7/9] Removes tests for withdraw_unbonded with slashing --- frame/staking/src/tests.rs | 48 -------------------------------------- 1 file changed, 48 deletions(-) diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index e379e127c545d..793659f2a8bc6 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -5749,52 +5749,4 @@ mod staking_interface { assert_ok!(::force_unstake(11)); }); } - - #[test] - fn withdraw_unbonded_with_slash_works() { - ExtBuilder::default().build_and_execute(|| { - let _ = with_storage_layer::<(), _, _>(|| { - assert_ok!(Staking::unbond(RuntimeOrigin::signed(10), 1)); - Err(DispatchError::from("revert")) - }); - - add_slash(&10); - let mut current_era = 0; - assert_ok!(Staking::unbond(RuntimeOrigin::signed(10), 1)); - let max_unlocking_chunks = <::MaxUnlockingChunks as Get>::get(); - - // Fill up all unlocking chunks. - for i in 0..max_unlocking_chunks - 1 { - current_era = i as u32; - mock::start_active_era(current_era); - assert_ok!(Staking::unbond(RuntimeOrigin::signed(10), 1)); - } - - current_era += 1; - mock::start_active_era(current_era); - - // This chunk is locked at `current_era` through `current_era + 2` (because - // `BondingDuration` == 3). - assert_ok!(Staking::unbond(RuntimeOrigin::signed(10), 1)); - assert_eq!( - Staking::ledger(&10).map(|l| l.unlocking.len()).unwrap(), - <::MaxUnlockingChunks as Get>::get() as usize - ); - - // even though the number of unlocked chunks is the same as `MaxUnlockingChunks`, - // unbonding works as expected. - for i in current_era..(current_era + max_unlocking_chunks) - 1 { - // There is only 1 chunk per era, so we need to be in a new era to create a chunk. - current_era = i as u32; - mock::start_active_era(current_era); - assert_ok!(Staking::unbond(RuntimeOrigin::signed(10), 1)); - } - - // only slots within last `BondingDuration` are filled. - assert_eq!( - Staking::ledger(&10).map(|l| l.unlocking.len()).unwrap(), - <::BondingDuration>::get() as usize - ); - }) - } } From eb94a00156b5cbd4ae0b74bf86cd847bb4ce5d67 Mon Sep 17 00:00:00 2001 From: command-bot <> Date: Thu, 22 Dec 2022 14:29:00 +0000 Subject: [PATCH 8/9] ".git/.scripts/fmt.sh" --- frame/fast-unstake/src/lib.rs | 7 ++++--- frame/staking/src/pallet/mod.rs | 3 ++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/frame/fast-unstake/src/lib.rs b/frame/fast-unstake/src/lib.rs index 89e627b2ab23d..f2faeebc13478 100644 --- a/frame/fast-unstake/src/lib.rs +++ b/frame/fast-unstake/src/lib.rs @@ -81,7 +81,6 @@ macro_rules! log { pub mod pallet { use super::*; use crate::types::*; - use sp_std::collections::btree_set::BTreeSet; use frame_support::{ pallet_prelude::*, traits::{Defensive, ReservableCurrency, StorageVersion}, @@ -92,7 +91,7 @@ pub mod pallet { DispatchResult, }; use sp_staking::{EraIndex, StakingInterface}; - use sp_std::{prelude::*, vec::Vec}; + use sp_std::{collections::btree_set::BTreeSet, prelude::*, vec::Vec}; pub use weights::WeightInfo; #[derive(scale_info::TypeInfo, codec::Encode, codec::Decode, codec::MaxEncodedLen)] @@ -490,7 +489,9 @@ pub mod pallet { }, } - ::WeightInfo::on_idle_check(validator_count * eras_checked.len() as u32) + ::WeightInfo::on_idle_check( + validator_count * eras_checked.len() as u32, + ) } } } diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 7c40f4b8f436b..f04d13d05f468 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -982,7 +982,8 @@ pub mod pallet { // `BondingDuration` to proceed with the unbonding. let maybe_withdraw_weight = { if unlocking == T::MaxUnlockingChunks::get() as usize { - let real_num_slashing_spans = Self::slashing_spans(&controller).map_or(0, |s| s.iter().count()); + let real_num_slashing_spans = + Self::slashing_spans(&controller).map_or(0, |s| s.iter().count()); Some(Self::do_withdraw_unbonded(&controller, real_num_slashing_spans as u32)?) } else { None From 803c25c2b36a652bfc088978277fdace289d2fc6 Mon Sep 17 00:00:00 2001 From: gpestana Date: Thu, 22 Dec 2022 15:42:14 +0100 Subject: [PATCH 9/9] Adds slash spans calculation test for withdraw_unbonded --- frame/staking/src/tests.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 793659f2a8bc6..5c2766deee307 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -5749,4 +5749,30 @@ mod staking_interface { assert_ok!(::force_unstake(11)); }); } + + #[test] + fn do_withdraw_unbonded_with_wrong_slash_spans_works_as_expected() { + ExtBuilder::default().build_and_execute(|| { + on_offence_now( + &[OffenceDetails { + offender: (11, Staking::eras_stakers(active_era(), 11)), + reporters: vec![], + }], + &[Perbill::from_percent(100)], + ); + + assert_eq!(Staking::bonded(&11), Some(10)); + + assert_noop!( + Staking::withdraw_unbonded(RuntimeOrigin::signed(10), 0), + Error::::IncorrectSlashingSpans + ); + + let num_slashing_spans = Staking::slashing_spans(&11).map_or(0, |s| s.iter().count()); + assert_ok!(Staking::withdraw_unbonded( + RuntimeOrigin::signed(10), + num_slashing_spans as u32 + )); + }); + } }