From 3b88c1379ec274e27289dd1ddf5b10dc66a2f536 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 30 Sep 2022 14:03:14 +0100 Subject: [PATCH 01/14] implement a brand new batch with all tests passing. --- bin/node/runtime/src/lib.rs | 1 + frame/fast-unstake/src/lib.rs | 167 ++++++++++++++++++------------- frame/fast-unstake/src/mock.rs | 2 + frame/fast-unstake/src/tests.rs | 170 ++++++++++++++------------------ frame/fast-unstake/src/types.rs | 23 ++--- 5 files changed, 187 insertions(+), 176 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 5e4fdb4748d15..30b7d84ce2358 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -582,6 +582,7 @@ impl pallet_staking::Config for Runtime { impl pallet_fast_unstake::Config for Runtime { type RuntimeEvent = RuntimeEvent; type ControlOrigin = frame_system::EnsureRoot; + type BatchSize = ConstU32<{ 128 }>; type Deposit = ConstU128<{ DOLLARS }>; type DepositCurrency = Balances; type WeightInfo = (); diff --git a/frame/fast-unstake/src/lib.rs b/frame/fast-unstake/src/lib.rs index 8fdb7a79dd537..b4c0596cd26f9 100644 --- a/frame/fast-unstake/src/lib.rs +++ b/frame/fast-unstake/src/lib.rs @@ -125,14 +125,18 @@ pub mod pallet { /// The origin that can control this pallet. type ControlOrigin: frame_support::traits::EnsureOrigin; + /// Batch size. + /// + /// This many stashes are processed in each unstake request. + type BatchSize: Get; + /// The weight information of this pallet. type WeightInfo: WeightInfo; } /// The current "head of the queue" being unstaked. #[pallet::storage] - pub type Head = - StorageValue<_, UnstakeRequest, BalanceOf>, OptionQuery>; + pub type Head = StorageValue<_, UnstakeRequest, OptionQuery>; /// The map of all accounts wishing to be unstaked. /// @@ -157,13 +161,18 @@ pub mod pallet { Unstaked { stash: T::AccountId, result: DispatchResult }, /// A staker was slashed for requesting fast-unstake whilst being exposed. Slashed { stash: T::AccountId, amount: BalanceOf }, - /// A staker was partially checked for the given eras, but the process did not finish. - Checking { stash: T::AccountId, eras: Vec }, /// Some internal error happened while migrating stash. They are removed as head as a /// consequence. Errored { stash: T::AccountId }, /// An internal error happened. Operations will be paused now. InternalError, + /// A batch was partially checked for the given eras, but the process did not finish. + Checking { eras: Vec }, + /// A batch was terminated. + /// + /// This is always follows by a number of `Unstaked` or `Slashed` events, marking the end + /// of the batch. A new batch will be created upon next block. + Terminated, } #[pallet::error] @@ -225,10 +234,7 @@ pub mod pallet { let ledger = pallet_staking::Ledger::::get(&ctrl).ok_or(Error::::NotController)?; ensure!(!Queue::::contains_key(&ledger.stash), Error::::AlreadyQueued); - ensure!( - Head::::get().map_or(true, |UnstakeRequest { stash, .. }| stash != ledger.stash), - Error::::AlreadyHead - ); + ensure!(!Self::is_head(&ledger.stash), Error::::AlreadyHead); // second part of the && is defensive. ensure!( ledger.active == ledger.total && ledger.unlocking.is_empty(), @@ -263,10 +269,7 @@ pub mod pallet { .map(|l| l.stash) .ok_or(Error::::NotController)?; ensure!(Queue::::contains_key(&stash), Error::::NotQueued); - ensure!( - Head::::get().map_or(true, |UnstakeRequest { stash, .. }| stash != stash), - Error::::AlreadyHead - ); + ensure!(!Self::is_head(&stash), Error::::AlreadyHead); let deposit = Queue::::take(stash.clone()); if let Some(deposit) = deposit.defensive() { @@ -293,6 +296,20 @@ pub mod pallet { } impl Pallet { + /// Returns `true` if `staker` is anywhere to be found in the `head`. + pub(crate) fn is_head(staker: &T::AccountId) -> bool { + Head::::get().map_or(false, |UnstakeRequest { stashes, .. }| { + stashes.iter().any(|(stash, _)| stash == staker) + }) + } + + /// Halt the operations of this pallet. + pub(crate) fn halt(reason: &'static str) { + frame_support::defensive!(reason); + ErasToCheckPerBlock::::put(0); + Self::deposit_event(Event::::InternalError) + } + /// process up to `remaining_weight`. /// /// Returns the actual weight consumed. @@ -339,28 +356,30 @@ pub mod pallet { return T::DbWeight::get().reads(2) } - let UnstakeRequest { stash, mut checked, deposit } = - match Head::::take().or_else(|| { - // NOTE: there is no order guarantees in `Queue`. - Queue::::drain() - .map(|(stash, deposit)| UnstakeRequest { - stash, - deposit, - checked: Default::default(), - }) - .next() - }) { - None => { - // There's no `Head` and nothing in the `Queue`, nothing to do here. - return T::DbWeight::get().reads(4) - }, - Some(head) => head, - }; + let UnstakeRequest { stashes, mut checked } = match Head::::take().or_else(|| { + // NOTE: there is no order guarantees in `Queue`. + let stashes: BoundedVec<_, T::BatchSize> = Queue::::drain() + .take(T::BatchSize::get() as usize) + .collect::>() + .try_into() + .expect("take ensures bound is met; qed"); + if stashes.is_empty() { + None + } else { + Some(UnstakeRequest { stashes, checked: Default::default() }) + } + }) { + None => { + // There's no `Head` and nothing in the `Queue`, nothing to do here. + return T::DbWeight::get().reads(4) + }, + Some(head) => head, + }; log!( debug, "checking {:?}, eras_to_check_per_block = {:?}, remaining_weight = {:?}", - stash, + stashes, eras_to_check_per_block, remaining_weight ); @@ -368,9 +387,11 @@ pub mod pallet { // the range that we're allowed to check in this round. let current_era = pallet_staking::CurrentEra::::get().unwrap_or_default(); let bonding_duration = ::BondingDuration::get(); + // prune all the old eras that we don't care about. This will help us keep the bound // of `checked`. checked.retain(|e| *e >= current_era.saturating_sub(bonding_duration)); + let unchecked_eras_to_check = { // get the last available `bonding_duration` eras up to current era in reverse // order. @@ -400,10 +421,8 @@ pub mod pallet { unchecked_eras_to_check ); - if unchecked_eras_to_check.is_empty() { - // `stash` is not exposed in any era now -- we can let go of them now. + let unstake_stash = |stash: T::AccountId, deposit| { let num_slashing_spans = Staking::::slashing_spans(&stash).iter().count() as u32; - let result = pallet_staking::Pallet::::force_unstake( RawOrigin::Root.into(), stash.clone(), @@ -412,61 +431,71 @@ pub mod pallet { let remaining = T::DepositCurrency::unreserve(&stash, deposit); if !remaining.is_zero() { - frame_support::defensive!("`not enough balance to unreserve`"); - ErasToCheckPerBlock::::put(0); - Self::deposit_event(Event::::InternalError) + Self::halt("not enough balance to unreserve"); } else { log!(info, "unstaked {:?}, outcome: {:?}", stash, result); Self::deposit_event(Event::::Unstaked { stash, result }); } + }; - ::WeightInfo::on_idle_unstake() - } else { - // eras remaining to be checked. - let mut eras_checked = 0u32; + let check_stash = |stash, deposit, eras_checked: &mut u32| { let is_exposed = unchecked_eras_to_check.iter().any(|e| { eras_checked.saturating_inc(); Self::is_exposed_in_era(&stash, e) }); - log!( - debug, - "checked {:?} eras, exposed? {}, (v: {:?}, u: {:?})", - eras_checked, - is_exposed, - validator_count, - unchecked_eras_to_check.len() - ); - - // NOTE: you can be extremely unlucky and get slashed here: You are not exposed in - // the last 28 eras, have registered yourself to be unstaked, midway being checked, - // you are exposed. if is_exposed { T::DepositCurrency::slash_reserved(&stash, deposit); log!(info, "slashed {:?} by {:?}", stash, deposit); Self::deposit_event(Event::::Slashed { stash, amount: deposit }); + false } else { - // Not exposed in these eras. - match checked.try_extend(unchecked_eras_to_check.clone().into_iter()) { - Ok(_) => { - Head::::put(UnstakeRequest { - stash: stash.clone(), - checked, - deposit, - }); + true + } + }; + + if unchecked_eras_to_check.is_empty() { + // `stash` is not exposed in any era now -- we can let go of them now. + stashes.into_iter().for_each(|(stash, deposit)| unstake_stash(stash, deposit)); + Self::deposit_event(Event::::Terminated); + ::WeightInfo::on_idle_unstake() + } else { + // eras checked so far. + let mut eras_checked = 0u32; + + let pre_length = stashes.len(); + let stashes: BoundedVec<(T::AccountId, BalanceOf), T::BatchSize> = stashes + .into_iter() + .filter(|(stash, deposit)| { + check_stash(stash.clone(), *deposit, &mut eras_checked) + }) + .collect::>() + .try_into() + .expect("filter can only lesson the length; still in bound; qed"); + let post_length = stashes.len(); + + log!( + debug, + "checked {:?} eras, pre stashes: {:?}, post: {:?}", + eras_checked, + pre_length, + post_length, + ); + + match checked.try_extend(unchecked_eras_to_check.clone().into_iter()) { + Ok(_) => + if stashes.is_empty() { + Self::deposit_event(Event::::Terminated); + } else { + Head::::put(UnstakeRequest { stashes, checked }); Self::deposit_event(Event::::Checking { - stash, eras: unchecked_eras_to_check, }); }, - Err(_) => { - // don't put the head back in -- there is an internal error in the - // pallet. - frame_support::defensive!("`checked is pruned via retain above`"); - ErasToCheckPerBlock::::put(0); - Self::deposit_event(Event::::InternalError); - }, - } + Err(_) => { + // don't put the head back in -- there is an internal error in the pallet. + Self::halt("checked is pruned via retain above") + }, } ::WeightInfo::on_idle_check(validator_count * eras_checked) diff --git a/frame/fast-unstake/src/mock.rs b/frame/fast-unstake/src/mock.rs index 71fc2d4ba905a..489d6c7c58332 100644 --- a/frame/fast-unstake/src/mock.rs +++ b/frame/fast-unstake/src/mock.rs @@ -169,6 +169,7 @@ impl Convert for U256ToBalance { parameter_types! { pub static DepositAmount: u128 = 7; + pub static BatchSize: u32 = 1; } impl fast_unstake::Config for Runtime { @@ -176,6 +177,7 @@ impl fast_unstake::Config for Runtime { type Deposit = DepositAmount; type DepositCurrency = Balances; type ControlOrigin = frame_system::EnsureRoot; + type BatchSize = BatchSize; type WeightInfo = (); } diff --git a/frame/fast-unstake/src/tests.rs b/frame/fast-unstake/src/tests.rs index 6e617fd992028..921791172ae28 100644 --- a/frame/fast-unstake/src/tests.rs +++ b/frame/fast-unstake/src/tests.rs @@ -107,9 +107,8 @@ fn cannot_register_if_head() { ErasToCheckPerBlock::::put(1); // Insert some Head item for stash Head::::put(UnstakeRequest { - stash: 1, + stashes: bounded_vec![(1, DepositAmount::get())], checked: bounded_vec![], - deposit: DepositAmount::get(), }); // Controller attempts to regsiter assert_noop!( @@ -191,9 +190,8 @@ fn cannot_deregister_already_head() { assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(2))); // Insert some Head item for stash. Head::::put(UnstakeRequest { - stash: 1, + stashes: bounded_vec![(1, DepositAmount::get())], checked: bounded_vec![], - deposit: DepositAmount::get(), }); // Controller attempts to deregister assert_noop!(FastUnstake::deregister(RuntimeOrigin::signed(2)), Error::::AlreadyHead); @@ -262,13 +260,12 @@ mod on_idle { // then assert_eq!( fast_unstake_events_since_last_call(), - vec![Event::Checking { stash: 1, eras: vec![3] }] + vec![Event::Checking { eras: vec![3] }] ); assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: 1, + stashes: bounded_vec![(1, DepositAmount::get())], checked: bounded_vec![3] }) ); @@ -282,13 +279,12 @@ mod on_idle { // then: assert_eq!( fast_unstake_events_since_last_call(), - vec![Event::Checking { stash: 1, eras: bounded_vec![2] }] + vec![Event::Checking { eras: bounded_vec![2] }] ); assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: 1, + stashes: bounded_vec![(1, DepositAmount::get())], checked: bounded_vec![3, 2] }) ); @@ -308,13 +304,12 @@ mod on_idle { // then: assert_eq!( fast_unstake_events_since_last_call(), - vec![Event::Checking { stash: 1, eras: vec![1, 0] }] + vec![Event::Checking { eras: vec![1, 0] }] ); assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: 1, + stashes: bounded_vec![(1, DepositAmount::get())], checked: bounded_vec![3, 2, 1, 0] }) ); @@ -329,8 +324,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: 1, + stashes: bounded_vec![(1, DepositAmount::get())], checked: bounded_vec![3, 2, 1, 0] }) ); @@ -348,7 +342,7 @@ mod on_idle { // then we finish the unbonding: assert_eq!( fast_unstake_events_since_last_call(), - vec![Event::Unstaked { stash: 1, result: Ok(()) }] + vec![Event::Unstaked { stash: 1, result: Ok(()) }, Event::Terminated], ); assert_eq!(Head::::get(), None,); @@ -383,8 +377,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: 1, + stashes: bounded_vec![(1, DepositAmount::get())], checked: bounded_vec![3, 2, 1, 0] }) ); @@ -404,8 +397,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: 5, + stashes: bounded_vec![(5, DepositAmount::get())], checked: bounded_vec![3, 2, 1, 0] }), ); @@ -416,9 +408,10 @@ mod on_idle { assert_eq!( fast_unstake_events_since_last_call(), vec![ - Event::Checking { stash: 1, eras: vec![3, 2, 1, 0] }, + Event::Checking { eras: vec![3, 2, 1, 0] }, Event::Unstaked { stash: 1, result: Ok(()) }, - Event::Checking { stash: 5, eras: vec![3, 2, 1, 0] } + Event::Terminated, + Event::Checking { eras: vec![3, 2, 1, 0] } ] ); }); @@ -463,10 +456,12 @@ mod on_idle { assert_eq!( fast_unstake_events_since_last_call(), vec![ - Event::Checking { stash: 1, eras: vec![3, 2, 1, 0] }, + Event::Checking { eras: vec![3, 2, 1, 0] }, Event::Unstaked { stash: 1, result: Ok(()) }, - Event::Checking { stash: 3, eras: vec![3, 2, 1, 0] }, + Event::Terminated, + Event::Checking { eras: vec![3, 2, 1, 0] }, Event::Unstaked { stash: 3, result: Ok(()) }, + Event::Terminated, ] ); @@ -495,8 +490,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: 1, + stashes: bounded_vec![(1, DepositAmount::get())], checked: bounded_vec![3, 2, 1, 0] }) ); @@ -507,8 +501,9 @@ mod on_idle { assert_eq!( fast_unstake_events_since_last_call(), vec![ - Event::Checking { stash: 1, eras: vec![3, 2, 1, 0] }, - Event::Unstaked { stash: 1, result: Ok(()) } + Event::Checking { eras: vec![3, 2, 1, 0] }, + Event::Unstaked { stash: 1, result: Ok(()) }, + Event::Terminated ] ); assert_unstaked(&1); @@ -537,8 +532,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: 1, + stashes: bounded_vec![(1, DepositAmount::get())], checked: bounded_vec![3, 2, 1, 0] }) ); @@ -549,8 +543,9 @@ mod on_idle { assert_eq!( fast_unstake_events_since_last_call(), vec![ - Event::Checking { stash: 1, eras: vec![3, 2, 1, 0] }, - Event::Unstaked { stash: 1, result: Ok(()) } + Event::Checking { eras: vec![3, 2, 1, 0] }, + Event::Unstaked { stash: 1, result: Ok(()) }, + Event::Terminated ] ); assert_unstaked(&1); @@ -578,8 +573,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: 1, + stashes: bounded_vec![(1, DepositAmount::get())], checked: bounded_vec![3] }) ); @@ -589,8 +583,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: 1, + stashes: bounded_vec![(1, DepositAmount::get())], checked: bounded_vec![3, 2] }) ); @@ -600,8 +593,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: 1, + stashes: bounded_vec![(1, DepositAmount::get())], checked: bounded_vec![3, 2, 1] }) ); @@ -611,8 +603,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: 1, + stashes: bounded_vec![(1, DepositAmount::get())], checked: bounded_vec![3, 2, 1, 0] }) ); @@ -624,11 +615,12 @@ mod on_idle { assert_eq!( fast_unstake_events_since_last_call(), vec![ - Event::Checking { stash: 1, eras: vec![3] }, - Event::Checking { stash: 1, eras: vec![2] }, - Event::Checking { stash: 1, eras: vec![1] }, - Event::Checking { stash: 1, eras: vec![0] }, - Event::Unstaked { stash: 1, result: Ok(()) } + Event::Checking { eras: vec![3] }, + Event::Checking { eras: vec![2] }, + Event::Checking { eras: vec![1] }, + Event::Checking { eras: vec![0] }, + Event::Unstaked { stash: 1, result: Ok(()) }, + Event::Terminated ] ); assert_unstaked(&1); @@ -653,8 +645,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: 1, + stashes: bounded_vec![(1, DepositAmount::get())], checked: bounded_vec![3] }) ); @@ -663,8 +654,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: 1, + stashes: bounded_vec![(1, DepositAmount::get())], checked: bounded_vec![3, 2] }) ); @@ -673,8 +663,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: 1, + stashes: bounded_vec![(1, DepositAmount::get())], checked: bounded_vec![3, 2, 1] }) ); @@ -683,8 +672,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: 1, + stashes: bounded_vec![(1, DepositAmount::get())], checked: bounded_vec![3, 2, 1, 0] }) ); @@ -698,10 +686,9 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stash: 1, + stashes: bounded_vec![(1, DepositAmount::get())], // note era 0 is pruned to keep the vector length sane. checked: bounded_vec![3, 2, 1, 4], - deposit: DepositAmount::get(), }) ); @@ -711,12 +698,13 @@ mod on_idle { assert_eq!( fast_unstake_events_since_last_call(), vec![ - Event::Checking { stash: 1, eras: vec![3] }, - Event::Checking { stash: 1, eras: vec![2] }, - Event::Checking { stash: 1, eras: vec![1] }, - Event::Checking { stash: 1, eras: vec![0] }, - Event::Checking { stash: 1, eras: vec![4] }, - Event::Unstaked { stash: 1, result: Ok(()) } + Event::Checking { eras: vec![3] }, + Event::Checking { eras: vec![2] }, + Event::Checking { eras: vec![1] }, + Event::Checking { eras: vec![0] }, + Event::Checking { eras: vec![4] }, + Event::Unstaked { stash: 1, result: Ok(()) }, + Event::Terminated ] ); assert_unstaked(&1); @@ -738,8 +726,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: 1, + stashes: bounded_vec![(1, DepositAmount::get())], checked: bounded_vec![3] }) ); @@ -748,8 +735,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: 1, + stashes: bounded_vec![(1, DepositAmount::get())], checked: bounded_vec![3, 2] }) ); @@ -762,8 +748,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: 1, + stashes: bounded_vec![(1, DepositAmount::get())], checked: bounded_vec![3, 2] }) ); @@ -772,8 +757,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: 1, + stashes: bounded_vec![(1, DepositAmount::get())], checked: bounded_vec![3, 2] }) ); @@ -788,8 +772,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: 1, + stashes: bounded_vec![(1, DepositAmount::get())], checked: bounded_vec![3, 2, 4] }) ); @@ -799,8 +782,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: 1, + stashes: bounded_vec![(1, DepositAmount::get())], checked: bounded_vec![3, 2, 4, 1] }) ); @@ -812,11 +794,12 @@ mod on_idle { assert_eq!( fast_unstake_events_since_last_call(), vec![ - Event::Checking { stash: 1, eras: vec![3] }, - Event::Checking { stash: 1, eras: vec![2] }, - Event::Checking { stash: 1, eras: vec![4] }, - Event::Checking { stash: 1, eras: vec![1] }, - Event::Unstaked { stash: 1, result: Ok(()) } + Event::Checking { eras: vec![3] }, + Event::Checking { eras: vec![2] }, + Event::Checking { eras: vec![4] }, + Event::Checking { eras: vec![1] }, + Event::Unstaked { stash: 1, result: Ok(()) }, + Event::Terminated ] ); @@ -853,8 +836,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: exposed, + stashes: bounded_vec![(exposed, DepositAmount::get())], checked: bounded_vec![3] }) ); @@ -862,8 +844,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: exposed, + stashes: bounded_vec![(exposed, DepositAmount::get())], checked: bounded_vec![3, 2] }) ); @@ -873,9 +854,10 @@ mod on_idle { assert_eq!( fast_unstake_events_since_last_call(), vec![ - Event::Checking { stash: exposed, eras: vec![3] }, - Event::Checking { stash: exposed, eras: vec![2] }, - Event::Slashed { stash: exposed, amount: DepositAmount::get() } + Event::Checking { eras: vec![3] }, + Event::Checking { eras: vec![2] }, + Event::Slashed { stash: exposed, amount: DepositAmount::get() }, + Event::Terminated ] ); }); @@ -911,8 +893,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: exposed, + stashes: bounded_vec![(exposed, DepositAmount::get())], checked: bounded_vec![3, 2] }) ); @@ -923,8 +904,9 @@ mod on_idle { fast_unstake_events_since_last_call(), // we slash them vec![ - Event::Checking { stash: exposed, eras: vec![3, 2] }, - Event::Slashed { stash: exposed, amount: DepositAmount::get() } + Event::Checking { eras: vec![3, 2] }, + Event::Slashed { stash: exposed, amount: DepositAmount::get() }, + Event::Terminated ] ); }); @@ -955,7 +937,7 @@ mod on_idle { assert_eq!( fast_unstake_events_since_last_call(), - vec![Event::Slashed { stash: 100, amount: DepositAmount::get() }] + vec![Event::Slashed { stash: 100, amount: DepositAmount::get() }, Event::Terminated] ); }); } @@ -979,8 +961,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - deposit: DepositAmount::get(), - stash: 42, + stashes: bounded_vec![(42, DepositAmount::get())], checked: bounded_vec![3, 2, 1, 0] }) ); @@ -990,8 +971,9 @@ mod on_idle { assert_eq!( fast_unstake_events_since_last_call(), vec![ - Event::Checking { stash: 42, eras: vec![3, 2, 1, 0] }, - Event::Unstaked { stash: 42, result: Ok(()) } + Event::Checking { eras: vec![3, 2, 1, 0] }, + Event::Unstaked { stash: 42, result: Ok(()) }, + Event::Terminated ] ); }); diff --git a/frame/fast-unstake/src/types.rs b/frame/fast-unstake/src/types.rs index 08b9ab4326eb2..4b76138aa8e70 100644 --- a/frame/fast-unstake/src/types.rs +++ b/frame/fast-unstake/src/types.rs @@ -17,14 +17,14 @@ //! Types used in the Fast Unstake pallet. +use crate::{Config, MaxChecking}; use codec::{Decode, Encode, MaxEncodedLen}; use frame_support::{ - traits::{Currency, Get}, - BoundedVec, EqNoBound, PartialEqNoBound, RuntimeDebugNoBound, + traits::Currency, BoundedVec, EqNoBound, PartialEqNoBound, RuntimeDebugNoBound, }; use scale_info::TypeInfo; use sp_staking::EraIndex; -use sp_std::{fmt::Debug, prelude::*}; +use sp_std::prelude::*; pub type BalanceOf = <::Currency as Currency< ::AccountId, @@ -34,15 +34,12 @@ pub type BalanceOf = <::Currency as Currency< #[derive( Encode, Decode, EqNoBound, PartialEqNoBound, Clone, TypeInfo, RuntimeDebugNoBound, MaxEncodedLen, )] -pub struct UnstakeRequest< - AccountId: Eq + PartialEq + Debug, - MaxChecked: Get, - Balance: PartialEq + Debug, -> { - /// Their stash account. - pub(crate) stash: AccountId, +#[scale_info(skip_type_params(T))] +pub struct UnstakeRequest { + /// This list of stashes being processed in this request, and their corresponding deposit. + pub(crate) stashes: BoundedVec<(T::AccountId, BalanceOf), T::BatchSize>, /// The list of eras for which they have been checked. - pub(crate) checked: BoundedVec, - /// Deposit to be slashed if the unstake was unsuccessful. - pub(crate) deposit: Balance, + pub(crate) checked: BoundedVec>, } + +// TODO: If we fail at reading the head, we should just remove it. From 53aa232ad458cdcd22f8eb43443a9788b7ee4238 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 30 Sep 2022 14:22:44 +0100 Subject: [PATCH 02/14] fix benchmarks as well --- bin/node/runtime/src/lib.rs | 2 +- frame/fast-unstake/src/benchmarking.rs | 51 +++++++++------ frame/fast-unstake/src/mock.rs | 4 +- frame/fast-unstake/src/tests.rs | 90 +++++++++++++------------- 4 files changed, 79 insertions(+), 68 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 30b7d84ce2358..1016a9cae4d76 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -582,7 +582,7 @@ impl pallet_staking::Config for Runtime { impl pallet_fast_unstake::Config for Runtime { type RuntimeEvent = RuntimeEvent; type ControlOrigin = frame_system::EnsureRoot; - type BatchSize = ConstU32<{ 128 }>; + type BatchSize = ConstU32<128>; type Deposit = ConstU128<{ DOLLARS }>; type DepositCurrency = Balances; type WeightInfo = (); diff --git a/frame/fast-unstake/src/benchmarking.rs b/frame/fast-unstake/src/benchmarking.rs index 8770cc6b64c0d..91e0ccb5e058f 100644 --- a/frame/fast-unstake/src/benchmarking.rs +++ b/frame/fast-unstake/src/benchmarking.rs @@ -43,10 +43,12 @@ fn l( T::Lookup::unlookup(who) } -fn create_unexposed_nominator() -> T::AccountId { - let account = frame_benchmarking::account::("nominator_42", 0, USER_SEED); - fund_and_bond_account::(&account); - account +fn create_unexposed_nominators() -> Vec { + (0..T::BatchSize::get()).map(|_| { + let account = frame_benchmarking::account::("nominator_42", 0, USER_SEED); + fund_and_bond_account::(&account); + account + }).collect() } fn fund_and_bond_account(account: &T::AccountId) { @@ -108,21 +110,27 @@ fn on_idle_full_block() { } benchmarks! { - // on_idle, we we don't check anyone, but fully unbond and move them to another pool. + // on_idle, we we don't check anyone, but fully unbond them. on_idle_unstake { ErasToCheckPerBlock::::put(1); - let who = create_unexposed_nominator::(); - assert_ok!(FastUnstake::::register_fast_unstake( - RawOrigin::Signed(who.clone()).into(), - )); + for who in create_unexposed_nominators::() { + assert_ok!(FastUnstake::::register_fast_unstake( + RawOrigin::Signed(who.clone()).into(), + )); + } // run on_idle once. This will check era 0. assert_eq!(Head::::get(), None); on_idle_full_block::(); - assert_eq!( + + assert!(matches!( Head::::get(), - Some(UnstakeRequest { stash: who.clone(), checked: vec![0].try_into().unwrap(), deposit: T::Deposit::get() }) - ); + Some(UnstakeRequest { + checked, + stashes, + .. + }) if checked.len() == 1 && stashes.len() as u32 == T::BatchSize::get() + )); } : { on_idle_full_block::(); @@ -130,7 +138,7 @@ benchmarks! { verify { assert!(matches!( fast_unstake_events::().last(), - Some(Event::Unstaked { .. }) + Some(Event::Terminated) )); } @@ -147,10 +155,13 @@ benchmarks! { // setup staking with v validators and u eras of data (0..=u) setup_staking::(v, u); - let who = create_unexposed_nominator::(); - assert_ok!(FastUnstake::::register_fast_unstake( - RawOrigin::Signed(who.clone()).into(), - )); + + let stashes = create_unexposed_nominators::().into_iter().map(|s| { + assert_ok!(FastUnstake::::register_fast_unstake( + RawOrigin::Signed(s.clone()).into(), + )); + (s, T::Deposit::get()) + }).collect::>().try_into().unwrap(); // no one is queued thus far. assert_eq!(Head::::get(), None); @@ -162,7 +173,7 @@ benchmarks! { let checked: frame_support::BoundedVec<_, _> = (1..=u).rev().collect::>().try_into().unwrap(); assert_eq!( Head::::get(), - Some(UnstakeRequest { stash: who.clone(), checked, deposit: T::Deposit::get() }) + Some(UnstakeRequest { stashes, checked }) ); assert!(matches!( fast_unstake_events::().last(), @@ -172,7 +183,7 @@ benchmarks! { register_fast_unstake { ErasToCheckPerBlock::::put(1); - let who = create_unexposed_nominator::(); + let who = create_unexposed_nominators::().get(0).cloned().unwrap(); whitelist_account!(who); assert_eq!(Queue::::count(), 0); @@ -184,7 +195,7 @@ benchmarks! { deregister { ErasToCheckPerBlock::::put(1); - let who = create_unexposed_nominator::(); + let who = create_unexposed_nominators::().get(0).cloned().unwrap(); assert_ok!(FastUnstake::::register_fast_unstake( RawOrigin::Signed(who.clone()).into(), )); diff --git a/frame/fast-unstake/src/mock.rs b/frame/fast-unstake/src/mock.rs index 489d6c7c58332..a123c7fc60a87 100644 --- a/frame/fast-unstake/src/mock.rs +++ b/frame/fast-unstake/src/mock.rs @@ -168,13 +168,13 @@ impl Convert for U256ToBalance { } parameter_types! { - pub static DepositAmount: u128 = 7; + pub static Deposit: u128 = 7; pub static BatchSize: u32 = 1; } impl fast_unstake::Config for Runtime { type RuntimeEvent = RuntimeEvent; - type Deposit = DepositAmount; + type Deposit = Deposit; type DepositCurrency = Balances; type ControlOrigin = frame_system::EnsureRoot; type BatchSize = BatchSize; diff --git a/frame/fast-unstake/src/tests.rs b/frame/fast-unstake/src/tests.rs index 921791172ae28..552c38d11874f 100644 --- a/frame/fast-unstake/src/tests.rs +++ b/frame/fast-unstake/src/tests.rs @@ -107,7 +107,7 @@ fn cannot_register_if_head() { ErasToCheckPerBlock::::put(1); // Insert some Head item for stash Head::::put(UnstakeRequest { - stashes: bounded_vec![(1, DepositAmount::get())], + stashes: bounded_vec![(1, Deposit::get())], checked: bounded_vec![], }); // Controller attempts to regsiter @@ -141,7 +141,7 @@ fn deregister_works() { // Controller account registers for fast unstake. assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(2))); - assert_eq!(::DepositCurrency::reserved_balance(&1), DepositAmount::get()); + assert_eq!(::DepositCurrency::reserved_balance(&1), Deposit::get()); // Controller then changes mind and deregisters. assert_ok!(FastUnstake::deregister(RuntimeOrigin::signed(2))); @@ -190,7 +190,7 @@ fn cannot_deregister_already_head() { assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(2))); // Insert some Head item for stash. Head::::put(UnstakeRequest { - stashes: bounded_vec![(1, DepositAmount::get())], + stashes: bounded_vec![(1, Deposit::get())], checked: bounded_vec![], }); // Controller attempts to deregister @@ -225,14 +225,14 @@ mod on_idle { // set up Queue item assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(2))); - assert_eq!(Queue::::get(1), Some(DepositAmount::get())); + assert_eq!(Queue::::get(1), Some(Deposit::get())); // call on_idle with no remaining weight FastUnstake::on_idle(System::block_number(), Weight::from_ref_time(0)); // assert nothing changed in Queue and Head assert_eq!(Head::::get(), None); - assert_eq!(Queue::::get(1), Some(DepositAmount::get())); + assert_eq!(Queue::::get(1), Some(Deposit::get())); }); } @@ -245,7 +245,7 @@ mod on_idle { // given assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(2))); - assert_eq!(Queue::::get(1), Some(DepositAmount::get())); + assert_eq!(Queue::::get(1), Some(Deposit::get())); assert_eq!(Queue::::count(), 1); assert_eq!(Head::::get(), None); @@ -265,7 +265,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(1, DepositAmount::get())], + stashes: bounded_vec![(1, Deposit::get())], checked: bounded_vec![3] }) ); @@ -284,7 +284,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(1, DepositAmount::get())], + stashes: bounded_vec![(1, Deposit::get())], checked: bounded_vec![3, 2] }) ); @@ -309,7 +309,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(1, DepositAmount::get())], + stashes: bounded_vec![(1, Deposit::get())], checked: bounded_vec![3, 2, 1, 0] }) ); @@ -324,7 +324,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(1, DepositAmount::get())], + stashes: bounded_vec![(1, Deposit::get())], checked: bounded_vec![3, 2, 1, 0] }) ); @@ -365,7 +365,7 @@ mod on_idle { assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(8))); assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(10))); - assert_eq!(::DepositCurrency::reserved_balance(&1), DepositAmount::get()); + assert_eq!(::DepositCurrency::reserved_balance(&1), Deposit::get()); assert_eq!(Queue::::count(), 5); assert_eq!(Head::::get(), None); @@ -377,7 +377,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(1, DepositAmount::get())], + stashes: bounded_vec![(1, Deposit::get())], checked: bounded_vec![3, 2, 1, 0] }) ); @@ -397,7 +397,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(5, DepositAmount::get())], + stashes: bounded_vec![(5, Deposit::get())], checked: bounded_vec![3, 2, 1, 0] }), ); @@ -425,9 +425,9 @@ mod on_idle { // register multi accounts for fast unstake assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(2))); - assert_eq!(Queue::::get(1), Some(DepositAmount::get())); + assert_eq!(Queue::::get(1), Some(Deposit::get())); assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(4))); - assert_eq!(Queue::::get(3), Some(DepositAmount::get())); + assert_eq!(Queue::::get(3), Some(Deposit::get())); // assert 2 queue items are in Queue & None in Head to start with assert_eq!(Queue::::count(), 2); @@ -478,7 +478,7 @@ mod on_idle { // register for fast unstake assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(2))); - assert_eq!(Queue::::get(1), Some(DepositAmount::get())); + assert_eq!(Queue::::get(1), Some(Deposit::get())); // process on idle next_block(true); @@ -490,7 +490,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(1, DepositAmount::get())], + stashes: bounded_vec![(1, Deposit::get())], checked: bounded_vec![3, 2, 1, 0] }) ); @@ -520,7 +520,7 @@ mod on_idle { // register for fast unstake assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(2))); - assert_eq!(Queue::::get(1), Some(DepositAmount::get())); + assert_eq!(Queue::::get(1), Some(Deposit::get())); // process on idle next_block(true); @@ -532,7 +532,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(1, DepositAmount::get())], + stashes: bounded_vec![(1, Deposit::get())], checked: bounded_vec![3, 2, 1, 0] }) ); @@ -561,7 +561,7 @@ mod on_idle { // register for fast unstake assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(2))); - assert_eq!(Queue::::get(1), Some(DepositAmount::get())); + assert_eq!(Queue::::get(1), Some(Deposit::get())); // process on idle next_block(true); @@ -573,7 +573,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(1, DepositAmount::get())], + stashes: bounded_vec![(1, Deposit::get())], checked: bounded_vec![3] }) ); @@ -583,7 +583,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(1, DepositAmount::get())], + stashes: bounded_vec![(1, Deposit::get())], checked: bounded_vec![3, 2] }) ); @@ -593,7 +593,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(1, DepositAmount::get())], + stashes: bounded_vec![(1, Deposit::get())], checked: bounded_vec![3, 2, 1] }) ); @@ -603,7 +603,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(1, DepositAmount::get())], + stashes: bounded_vec![(1, Deposit::get())], checked: bounded_vec![3, 2, 1, 0] }) ); @@ -639,13 +639,13 @@ mod on_idle { // register for fast unstake assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(2))); - assert_eq!(Queue::::get(1), Some(DepositAmount::get())); + assert_eq!(Queue::::get(1), Some(Deposit::get())); next_block(true); assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(1, DepositAmount::get())], + stashes: bounded_vec![(1, Deposit::get())], checked: bounded_vec![3] }) ); @@ -654,7 +654,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(1, DepositAmount::get())], + stashes: bounded_vec![(1, Deposit::get())], checked: bounded_vec![3, 2] }) ); @@ -663,7 +663,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(1, DepositAmount::get())], + stashes: bounded_vec![(1, Deposit::get())], checked: bounded_vec![3, 2, 1] }) ); @@ -672,7 +672,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(1, DepositAmount::get())], + stashes: bounded_vec![(1, Deposit::get())], checked: bounded_vec![3, 2, 1, 0] }) ); @@ -686,7 +686,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(1, DepositAmount::get())], + stashes: bounded_vec![(1, Deposit::get())], // note era 0 is pruned to keep the vector length sane. checked: bounded_vec![3, 2, 1, 4], }) @@ -726,7 +726,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(1, DepositAmount::get())], + stashes: bounded_vec![(1, Deposit::get())], checked: bounded_vec![3] }) ); @@ -735,7 +735,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(1, DepositAmount::get())], + stashes: bounded_vec![(1, Deposit::get())], checked: bounded_vec![3, 2] }) ); @@ -748,7 +748,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(1, DepositAmount::get())], + stashes: bounded_vec![(1, Deposit::get())], checked: bounded_vec![3, 2] }) ); @@ -757,7 +757,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(1, DepositAmount::get())], + stashes: bounded_vec![(1, Deposit::get())], checked: bounded_vec![3, 2] }) ); @@ -772,7 +772,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(1, DepositAmount::get())], + stashes: bounded_vec![(1, Deposit::get())], checked: bounded_vec![3, 2, 4] }) ); @@ -782,7 +782,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(1, DepositAmount::get())], + stashes: bounded_vec![(1, Deposit::get())], checked: bounded_vec![3, 2, 4, 1] }) ); @@ -836,7 +836,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(exposed, DepositAmount::get())], + stashes: bounded_vec![(exposed, Deposit::get())], checked: bounded_vec![3] }) ); @@ -844,7 +844,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(exposed, DepositAmount::get())], + stashes: bounded_vec![(exposed, Deposit::get())], checked: bounded_vec![3, 2] }) ); @@ -856,7 +856,7 @@ mod on_idle { vec![ Event::Checking { eras: vec![3] }, Event::Checking { eras: vec![2] }, - Event::Slashed { stash: exposed, amount: DepositAmount::get() }, + Event::Slashed { stash: exposed, amount: Deposit::get() }, Event::Terminated ] ); @@ -876,7 +876,7 @@ mod on_idle { pallet_staking::ErasStakers::::mutate(0, VALIDATORS_PER_ERA, |expo| { expo.others.push(IndividualExposure { who: exposed, value: 0 as Balance }); }); - Balances::make_free_balance_be(&exposed, DepositAmount::get() + 100); + Balances::make_free_balance_be(&exposed, Deposit::get() + 100); assert_ok!(Staking::bond( RuntimeOrigin::signed(exposed), exposed, @@ -893,7 +893,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(exposed, DepositAmount::get())], + stashes: bounded_vec![(exposed, Deposit::get())], checked: bounded_vec![3, 2] }) ); @@ -905,7 +905,7 @@ mod on_idle { // we slash them vec![ Event::Checking { eras: vec![3, 2] }, - Event::Slashed { stash: exposed, amount: DepositAmount::get() }, + Event::Slashed { stash: exposed, amount: Deposit::get() }, Event::Terminated ] ); @@ -937,7 +937,7 @@ mod on_idle { assert_eq!( fast_unstake_events_since_last_call(), - vec![Event::Slashed { stash: 100, amount: DepositAmount::get() }, Event::Terminated] + vec![Event::Slashed { stash: 100, amount: Deposit::get() }, Event::Terminated] ); }); } @@ -949,7 +949,7 @@ mod on_idle { CurrentEra::::put(BondingDuration::get()); // create a new validator that 100% not exposed. - Balances::make_free_balance_be(&42, 100 + DepositAmount::get()); + Balances::make_free_balance_be(&42, 100 + Deposit::get()); assert_ok!(Staking::bond(RuntimeOrigin::signed(42), 42, 10, RewardDestination::Staked)); assert_ok!(Staking::validate(RuntimeOrigin::signed(42), Default::default())); @@ -961,7 +961,7 @@ mod on_idle { assert_eq!( Head::::get(), Some(UnstakeRequest { - stashes: bounded_vec![(42, DepositAmount::get())], + stashes: bounded_vec![(42, Deposit::get())], checked: bounded_vec![3, 2, 1, 0] }) ); From ac2be3d34a8b5aa0d7d5d587d831acb69df242cf Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 30 Sep 2022 14:50:13 +0100 Subject: [PATCH 03/14] make benchmarks more or less work --- frame/fast-unstake/src/benchmarking.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/frame/fast-unstake/src/benchmarking.rs b/frame/fast-unstake/src/benchmarking.rs index 91e0ccb5e058f..e9ced9a0da733 100644 --- a/frame/fast-unstake/src/benchmarking.rs +++ b/frame/fast-unstake/src/benchmarking.rs @@ -44,8 +44,8 @@ fn l( } fn create_unexposed_nominators() -> Vec { - (0..T::BatchSize::get()).map(|_| { - let account = frame_benchmarking::account::("nominator_42", 0, USER_SEED); + (0..T::BatchSize::get()).map(|i| { + let account = frame_benchmarking::account::("nominator_42", i, USER_SEED); fund_and_bond_account::(&account); account }).collect() @@ -161,7 +161,7 @@ benchmarks! { RawOrigin::Signed(s.clone()).into(), )); (s, T::Deposit::get()) - }).collect::>().try_into().unwrap(); + }).collect::>(); // no one is queued thus far. assert_eq!(Head::::get(), None); @@ -170,11 +170,10 @@ benchmarks! { on_idle_full_block::(); } verify { - let checked: frame_support::BoundedVec<_, _> = (1..=u).rev().collect::>().try_into().unwrap(); - assert_eq!( - Head::::get(), - Some(UnstakeRequest { stashes, checked }) - ); + let checked = (1..=u).rev().collect::>(); + let request = Head::::get().unwrap(); + assert_eq!(checked, request.checked.into_inner()); + assert!(stashes.iter().all(|(s, _)| request.stashes.iter().find(|(ss, _)| ss == s).is_some())); assert!(matches!( fast_unstake_events::().last(), Some(Event::Checking { .. }) From 0ba02aab54ea090e90ca7d88a3cbe2f11b1fc5c0 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 30 Sep 2022 14:56:36 +0100 Subject: [PATCH 04/14] fix migration --- frame/fast-unstake/src/benchmarking.rs | 12 +++++++----- frame/fast-unstake/src/lib.rs | 23 +++++++++++++++++++++++ frame/fast-unstake/src/types.rs | 2 -- 3 files changed, 30 insertions(+), 7 deletions(-) diff --git a/frame/fast-unstake/src/benchmarking.rs b/frame/fast-unstake/src/benchmarking.rs index e9ced9a0da733..64b3806d6a2fa 100644 --- a/frame/fast-unstake/src/benchmarking.rs +++ b/frame/fast-unstake/src/benchmarking.rs @@ -44,11 +44,13 @@ fn l( } fn create_unexposed_nominators() -> Vec { - (0..T::BatchSize::get()).map(|i| { - let account = frame_benchmarking::account::("nominator_42", i, USER_SEED); - fund_and_bond_account::(&account); - account - }).collect() + (0..T::BatchSize::get()) + .map(|i| { + let account = frame_benchmarking::account::("nominator_42", i, USER_SEED); + fund_and_bond_account::(&account); + account + }) + .collect() } fn fund_and_bond_account(account: &T::AccountId) { diff --git a/frame/fast-unstake/src/lib.rs b/frame/fast-unstake/src/lib.rs index b4c0596cd26f9..b365fab9e761c 100644 --- a/frame/fast-unstake/src/lib.rs +++ b/frame/fast-unstake/src/lib.rs @@ -201,6 +201,29 @@ pub mod pallet { return Weight::from_ref_time(0) } + // Temporary for migration. + if Head::::exists() && Head::::get().is_none() { + // Migrate head. + let key = Head::::hashed_key(); + let (stash, deposit) = match frame_support::storage::unhashed::get::<( + T::AccountId, + Vec, + BalanceOf, + )>(&key) + .defensive() + { + Some((stash, _, deposit)) => (stash, deposit), + None => { + Head::::kill(); + return T::DbWeight::get().reads_writes(1, 1) + }, + }; + // insert them back into the queue. + Queue::::insert(stash, deposit); + Head::::kill(); + return T::DbWeight::get().reads_writes(1, 2) + } + Self::do_on_idle(remaining_weight) } } diff --git a/frame/fast-unstake/src/types.rs b/frame/fast-unstake/src/types.rs index 4b76138aa8e70..d8d4593435b0e 100644 --- a/frame/fast-unstake/src/types.rs +++ b/frame/fast-unstake/src/types.rs @@ -41,5 +41,3 @@ pub struct UnstakeRequest { /// The list of eras for which they have been checked. pub(crate) checked: BoundedVec>, } - -// TODO: If we fail at reading the head, we should just remove it. From e34f7a079731b8451ed2ea5084f7828bf76ab7aa Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 30 Sep 2022 15:25:42 +0100 Subject: [PATCH 05/14] add some testing --- frame/fast-unstake/src/mock.rs | 38 ++++- frame/fast-unstake/src/tests.rs | 285 ++++++++++++++++++++++++++++---- 2 files changed, 284 insertions(+), 39 deletions(-) diff --git a/frame/fast-unstake/src/mock.rs b/frame/fast-unstake/src/mock.rs index a123c7fc60a87..094a1365ea06f 100644 --- a/frame/fast-unstake/src/mock.rs +++ b/frame/fast-unstake/src/mock.rs @@ -16,8 +16,12 @@ // limitations under the License. use crate::{self as fast_unstake}; +use frame_benchmarking::frame_support::assert_ok; use frame_support::{ - pallet_prelude::*, parameter_types, traits::ConstU64, weights::constants::WEIGHT_PER_SECOND, + pallet_prelude::*, + parameter_types, + traits::{ConstU64, Currency}, + weights::constants::WEIGHT_PER_SECOND, }; use sp_runtime::traits::{Convert, IdentityLookup}; @@ -213,13 +217,13 @@ pub(crate) fn fast_unstake_events_since_last_call() -> Vec } pub struct ExtBuilder { - exposed_nominators: Vec<(AccountId, AccountId, Balance)>, + unexposed: Vec<(AccountId, AccountId, Balance)>, } impl Default for ExtBuilder { fn default() -> Self { Self { - exposed_nominators: vec![ + unexposed: vec![ (1, 2, 7 + 100), (3, 4, 7 + 100), (5, 6, 7 + 100), @@ -256,6 +260,11 @@ impl ExtBuilder { }); } + pub(crate) fn batch(self, size: u32) -> Self { + BatchSize::set(size); + self + } + pub(crate) fn build(self) -> sp_io::TestExternalities { sp_tracing::try_init_simple(); let mut storage = @@ -267,12 +276,12 @@ impl ExtBuilder { let _ = pallet_balances::GenesisConfig:: { balances: self - .exposed_nominators + .unexposed .clone() .into_iter() .map(|(stash, _, balance)| (stash, balance * 2)) .chain( - self.exposed_nominators + self.unexposed .clone() .into_iter() .map(|(_, ctrl, balance)| (ctrl, balance * 2)), @@ -285,7 +294,7 @@ impl ExtBuilder { let _ = pallet_staking::GenesisConfig:: { stakers: self - .exposed_nominators + .unexposed .into_iter() .map(|(x, y, z)| (x, y, z, pallet_staking::StakerStatus::Nominator(vec![42]))) .chain(validators_range.map(|x| (x, x, 100, StakerStatus::Validator))) @@ -348,3 +357,20 @@ pub fn assert_unstaked(stash: &AccountId) { assert!(!pallet_staking::Validators::::contains_key(stash)); assert!(!pallet_staking::Nominators::::contains_key(stash)); } + +pub fn create_exposed_nominator(exposed: AccountId, era: u32) { + // create an exposed nominator in era 1 + pallet_staking::ErasStakers::::mutate(era, VALIDATORS_PER_ERA, |expo| { + expo.others.push(IndividualExposure { who: exposed, value: 0 as Balance }); + }); + Balances::make_free_balance_be(&exposed, 100); + assert_ok!(Staking::bond( + RuntimeOrigin::signed(exposed), + exposed, + 10, + pallet_staking::RewardDestination::Staked + )); + assert_ok!(Staking::nominate(RuntimeOrigin::signed(exposed), vec![exposed])); + // register the exposed one. + assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(exposed))); +} diff --git a/frame/fast-unstake/src/tests.rs b/frame/fast-unstake/src/tests.rs index 552c38d11874f..9360021ad4ded 100644 --- a/frame/fast-unstake/src/tests.rs +++ b/frame/fast-unstake/src/tests.rs @@ -20,7 +20,7 @@ use super::*; use crate::{mock::*, types::*, weights::WeightInfo, Event}; use frame_support::{assert_noop, assert_ok, bounded_vec, pallet_prelude::*, traits::Currency}; -use pallet_staking::{CurrentEra, IndividualExposure, RewardDestination}; +use pallet_staking::{CurrentEra, RewardDestination}; use sp_runtime::traits::BadOrigin; use sp_staking::StakingInterface; @@ -814,22 +814,8 @@ mod on_idle { CurrentEra::::put(BondingDuration::get()); // create an exposed nominator in era 1 - let exposed = 666 as AccountId; - pallet_staking::ErasStakers::::mutate(1, VALIDATORS_PER_ERA, |expo| { - expo.others.push(IndividualExposure { who: exposed, value: 0 as Balance }); - }); - Balances::make_free_balance_be(&exposed, 100); - assert_ok!(Staking::bond( - RuntimeOrigin::signed(exposed), - exposed, - 10, - RewardDestination::Staked - )); - assert_ok!(Staking::nominate(RuntimeOrigin::signed(exposed), vec![exposed])); - - Balances::make_free_balance_be(&exposed, 100_000); - // register the exposed one. - assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(exposed))); + let exposed = 666; + create_exposed_nominator(exposed, 1); // a few blocks later, we realize they are slashed next_block(true); @@ -871,22 +857,9 @@ mod on_idle { ErasToCheckPerBlock::::put(2); CurrentEra::::put(BondingDuration::get()); - // create an exposed nominator in era 1 - let exposed = 666 as AccountId; - pallet_staking::ErasStakers::::mutate(0, VALIDATORS_PER_ERA, |expo| { - expo.others.push(IndividualExposure { who: exposed, value: 0 as Balance }); - }); - Balances::make_free_balance_be(&exposed, Deposit::get() + 100); - assert_ok!(Staking::bond( - RuntimeOrigin::signed(exposed), - exposed, - 10, - RewardDestination::Staked - )); - assert_ok!(Staking::nominate(RuntimeOrigin::signed(exposed), vec![exposed])); - - // register the exposed one. - assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(exposed))); + // create an exposed nominator in era 0 + let exposed = 666; + create_exposed_nominator(exposed, 0); // a few blocks later, we realize they are slashed next_block(true); @@ -979,3 +952,249 @@ mod on_idle { }); } } + +mod batched { + use super::*; + + #[test] + fn single_block_batched_successful() { + ExtBuilder::default().batch(3).build_and_execute(|| { + ErasToCheckPerBlock::::put(BondingDuration::get() + 1); + CurrentEra::::put(BondingDuration::get()); + + assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(2))); + assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(4))); + assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(6))); + assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(8))); + + assert_eq!(Queue::::count(), 4); + assert_eq!(Head::::get(), None); + + // when + next_block(true); + + // then + assert_eq!( + Head::::get(), + Some(UnstakeRequest { + stashes: bounded_vec![ + (1, Deposit::get()), + (5, Deposit::get()), + (7, Deposit::get()) + ], + checked: bounded_vec![3, 2, 1, 0] + }) + ); + assert_eq!(Queue::::count(), 1); + + // when + next_block(true); + + // then + assert_eq!(Head::::get(), None); + assert_eq!(Queue::::count(), 1); + + assert_eq!( + fast_unstake_events_since_last_call(), + vec![ + Event::Checking { eras: vec![3, 2, 1, 0] }, + Event::Unstaked { stash: 1, result: Ok(()) }, + Event::Unstaked { stash: 5, result: Ok(()) }, + Event::Unstaked { stash: 7, result: Ok(()) }, + Event::Terminated + ] + ); + }); + } + + #[test] + fn multi_block_batched_successful() { + ExtBuilder::default().batch(3).build_and_execute(|| { + ErasToCheckPerBlock::::put(2); + CurrentEra::::put(BondingDuration::get()); + + assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(2))); + assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(4))); + assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(6))); + assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(8))); + + assert_eq!(Queue::::count(), 4); + assert_eq!(Head::::get(), None); + + // when + next_block(true); + + // then + assert_eq!( + Head::::get(), + Some(UnstakeRequest { + stashes: bounded_vec![ + (1, Deposit::get()), + (5, Deposit::get()), + (7, Deposit::get()) + ], + checked: bounded_vec![3, 2] + }) + ); + + // when + next_block(true); + + // then + assert_eq!( + Head::::get(), + Some(UnstakeRequest { + stashes: bounded_vec![ + (1, Deposit::get()), + (5, Deposit::get()), + (7, Deposit::get()) + ], + checked: bounded_vec![3, 2, 1, 0] + }) + ); + + // when + next_block(true); + + // then + assert_eq!(Head::::get(), None); + + assert_eq!( + fast_unstake_events_since_last_call(), + vec![ + Event::Checking { eras: vec![3, 2] }, + Event::Checking { eras: vec![1, 0] }, + Event::Unstaked { stash: 1, result: Ok(()) }, + Event::Unstaked { stash: 5, result: Ok(()) }, + Event::Unstaked { stash: 7, result: Ok(()) }, + Event::Terminated + ] + ); + }); + } + + #[test] + fn multi_block_batched_some_fail() { + ExtBuilder::default().batch(4).build_and_execute(|| { + ErasToCheckPerBlock::::put(2); + CurrentEra::::put(BondingDuration::get()); + + // register two good ones. + assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(2))); + assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(4))); + create_exposed_nominator(666, 1); + create_exposed_nominator(667, 3); + + // then + assert_eq!(Queue::::count(), 4); + assert_eq!(Head::::get(), None); + + // when + next_block(true); + + // then + assert_eq!( + Head::::get(), + Some(UnstakeRequest { + stashes: bounded_vec![ + (1, Deposit::get()), + (3, Deposit::get()), + (666, Deposit::get()) + ], + checked: bounded_vec![3, 2] + }) + ); + + // when + next_block(true); + + // then + assert_eq!( + Head::::get(), + Some(UnstakeRequest { + stashes: bounded_vec![(1, Deposit::get()), (3, Deposit::get()),], + checked: bounded_vec![3, 2, 1, 0] + }) + ); + + // when + next_block(true); + + // then + assert_eq!(Head::::get(), None); + + assert_eq!( + fast_unstake_events_since_last_call(), + vec![ + Event::Slashed { stash: 667, amount: 7 }, + Event::Checking { eras: vec![3, 2] }, + Event::Slashed { stash: 666, amount: 7 }, + Event::Checking { eras: vec![1, 0] }, + Event::Unstaked { stash: 1, result: Ok(()) }, + Event::Unstaked { stash: 3, result: Ok(()) }, + Event::Terminated + ] + ); + }); + } + + #[test] + fn multi_block_batched_all_fail_early_exit() { + ExtBuilder::default().batch(2).build_and_execute(|| { + ErasToCheckPerBlock::::put(1); + CurrentEra::::put(BondingDuration::get()); + + // register two bad ones. + create_exposed_nominator(666, 3); + create_exposed_nominator(667, 2); + + // then + assert_eq!(Queue::::count(), 2); + assert_eq!(Head::::get(), None); + + // when we progress a block.. + next_block(true); + + // ..and register two good ones. + assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(2))); + assert_ok!(FastUnstake::register_fast_unstake(RuntimeOrigin::signed(4))); + + // then one of the bad ones is reaped. + assert_eq!( + Head::::get(), + Some(UnstakeRequest { + stashes: bounded_vec![(667, Deposit::get())], + checked: bounded_vec![3] + }) + ); + + // when we go to next block + next_block(true); + + // then the head is empty, we early terminate the batch. + assert_eq!(Head::::get(), None); + + // upon next block, we will assemble a new head. + next_block(true); + + assert_eq!( + Head::::get(), + Some(UnstakeRequest { + stashes: bounded_vec![(1, Deposit::get()), (3, Deposit::get()),], + checked: bounded_vec![3] + }) + ); + + assert_eq!( + fast_unstake_events_since_last_call(), + vec![ + Event::Slashed { stash: 666, amount: Deposit::get() }, + Event::Checking { eras: vec![3] }, + Event::Slashed { stash: 667, amount: Deposit::get() }, + Event::Terminated, + Event::Checking { eras: vec![3] } + ] + ); + }); + } +} From d875edc428635ac5a1f0065fbce06239234c5cdf Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Sun, 9 Oct 2022 13:00:23 +0200 Subject: [PATCH 06/14] Update frame/fast-unstake/src/benchmarking.rs Co-authored-by: Roman Useinov --- frame/fast-unstake/src/benchmarking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/fast-unstake/src/benchmarking.rs b/frame/fast-unstake/src/benchmarking.rs index 64b3806d6a2fa..f354be4d2dbed 100644 --- a/frame/fast-unstake/src/benchmarking.rs +++ b/frame/fast-unstake/src/benchmarking.rs @@ -112,7 +112,7 @@ fn on_idle_full_block() { } benchmarks! { - // on_idle, we we don't check anyone, but fully unbond them. + // on_idle, we don't check anyone, but fully unbond them. on_idle_unstake { ErasToCheckPerBlock::::put(1); for who in create_unexposed_nominators::() { From 8c404525c5b11ca2338246ec40737a63dce29c83 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Sun, 9 Oct 2022 13:51:44 +0200 Subject: [PATCH 07/14] review comments --- frame/fast-unstake/src/benchmarking.rs | 4 +- frame/fast-unstake/src/lib.rs | 21 ++++-- frame/fast-unstake/src/migrations.rs | 74 ++++++++++++++++++ frame/fast-unstake/src/mock.rs | 1 + frame/fast-unstake/src/tests.rs | 100 ++++++++++++------------- 5 files changed, 140 insertions(+), 60 deletions(-) create mode 100644 frame/fast-unstake/src/migrations.rs diff --git a/frame/fast-unstake/src/benchmarking.rs b/frame/fast-unstake/src/benchmarking.rs index f354be4d2dbed..2192d09b289e8 100644 --- a/frame/fast-unstake/src/benchmarking.rs +++ b/frame/fast-unstake/src/benchmarking.rs @@ -140,7 +140,7 @@ benchmarks! { verify { assert!(matches!( fast_unstake_events::().last(), - Some(Event::Terminated) + Some(Event::BatchFinished) )); } @@ -178,7 +178,7 @@ benchmarks! { assert!(stashes.iter().all(|(s, _)| request.stashes.iter().find(|(ss, _)| ss == s).is_some())); assert!(matches!( fast_unstake_events::().last(), - Some(Event::Checking { .. }) + Some(Event::BatchChecked { .. }) )); } diff --git a/frame/fast-unstake/src/lib.rs b/frame/fast-unstake/src/lib.rs index b365fab9e761c..ba48751eb47ab 100644 --- a/frame/fast-unstake/src/lib.rs +++ b/frame/fast-unstake/src/lib.rs @@ -60,6 +60,7 @@ mod tests; // NOTE: enable benchmarking in tests as well. #[cfg(feature = "runtime-benchmarks")] mod benchmarking; +pub mod migrations; pub mod types; pub mod weights; @@ -83,7 +84,7 @@ pub mod pallet { use frame_election_provider_support::ElectionProviderBase; use frame_support::{ pallet_prelude::*, - traits::{Defensive, ReservableCurrency}, + traits::{Defensive, ReservableCurrency, StorageVersion}, }; use frame_system::{pallet_prelude::*, RawOrigin}; use pallet_staking::Pallet as Staking; @@ -105,7 +106,10 @@ pub mod pallet { } } + const STORAGE_VERSION: StorageVersion = StorageVersion::new(1); + #[pallet::pallet] + #[pallet::storage_version(STORAGE_VERSION)] pub struct Pallet(_); #[pallet::config] @@ -167,12 +171,12 @@ pub mod pallet { /// An internal error happened. Operations will be paused now. InternalError, /// A batch was partially checked for the given eras, but the process did not finish. - Checking { eras: Vec }, + BatchChecked { eras: Vec }, /// A batch was terminated. /// /// This is always follows by a number of `Unstaked` or `Slashed` events, marking the end /// of the batch. A new batch will be created upon next block. - Terminated, + BatchFinished, } #[pallet::error] @@ -218,6 +222,7 @@ pub mod pallet { return T::DbWeight::get().reads_writes(1, 1) }, }; + // insert them back into the queue. Queue::::insert(stash, deposit); Head::::kill(); @@ -401,8 +406,8 @@ pub mod pallet { log!( debug, - "checking {:?}, eras_to_check_per_block = {:?}, remaining_weight = {:?}", - stashes, + "checking {:?} stashes, eras_to_check_per_block = {:?}, remaining_weight = {:?}", + stashes.len(), eras_to_check_per_block, remaining_weight ); @@ -480,7 +485,7 @@ pub mod pallet { if unchecked_eras_to_check.is_empty() { // `stash` is not exposed in any era now -- we can let go of them now. stashes.into_iter().for_each(|(stash, deposit)| unstake_stash(stash, deposit)); - Self::deposit_event(Event::::Terminated); + Self::deposit_event(Event::::BatchFinished); ::WeightInfo::on_idle_unstake() } else { // eras checked so far. @@ -508,10 +513,10 @@ pub mod pallet { match checked.try_extend(unchecked_eras_to_check.clone().into_iter()) { Ok(_) => if stashes.is_empty() { - Self::deposit_event(Event::::Terminated); + Self::deposit_event(Event::::BatchFinished); } else { Head::::put(UnstakeRequest { stashes, checked }); - Self::deposit_event(Event::::Checking { + Self::deposit_event(Event::::BatchChecked { eras: unchecked_eras_to_check, }); }, diff --git a/frame/fast-unstake/src/migrations.rs b/frame/fast-unstake/src/migrations.rs new file mode 100644 index 0000000000000..33f202c9e7aee --- /dev/null +++ b/frame/fast-unstake/src/migrations.rs @@ -0,0 +1,74 @@ +// This file is part of Substrate. + +// Copyright (C) 2022 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +pub mod v1 { + use crate::{types::BalanceOf, *}; + use frame_support::{ + storage::unhashed, + traits::{Defensive, Get, GetStorageVersion, OnRuntimeUpgrade}, + weights::Weight, + }; + use sp_staking::EraIndex; + + pub struct MigrateToV1(sp_std::marker::PhantomData); + impl OnRuntimeUpgrade for MigrateToV1 { + fn on_runtime_upgrade() -> Weight { + let current = Pallet::::current_storage_version(); + let onchain = Pallet::::on_chain_storage_version(); + + log!( + info, + "Running migration with current storage version {:?} / onchain {:?}", + current, + onchain + ); + + if current == 1 && onchain == 0 { + // if a head exists, then we put them back into the queue. + if Head::::exists() { + if let Some((stash, _, deposit)) = + unhashed::take::<(T::AccountId, Vec, BalanceOf)>( + &Head::::hashed_key(), + ) + .defensive() + { + Queue::::insert(stash, deposit); + current.put::>(); + } else { + // not much we can do here -- head is already deleted. + } + T::DbWeight::get().reads_writes(2, 3) + } else { + T::DbWeight::get().reads(2) + } + } else { + log!(info, "Migration did not executed. This probably should be removed"); + T::DbWeight::get().reads(1) + } + } + + #[cfg(feature = "try-runtime")] + fn pre_upgrade(_: Vec) -> Result<(), &'static str> { + assert_eq!(Pallet::::on_chain_storage_version(), 0); + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(_: Vec) -> Result<(), &'static str> { + assert_eq!(Pallet::::on_chain_storage_version(), 1); + } + } +} diff --git a/frame/fast-unstake/src/mock.rs b/frame/fast-unstake/src/mock.rs index 094a1365ea06f..cd1f6adc11d5c 100644 --- a/frame/fast-unstake/src/mock.rs +++ b/frame/fast-unstake/src/mock.rs @@ -317,6 +317,7 @@ impl ExtBuilder { // because we read this value as a measure of how many validators we have. pallet_staking::ValidatorCount::::put(VALIDATORS_PER_ERA as u32); }); + ext } diff --git a/frame/fast-unstake/src/tests.rs b/frame/fast-unstake/src/tests.rs index 9360021ad4ded..dc7308f29dd64 100644 --- a/frame/fast-unstake/src/tests.rs +++ b/frame/fast-unstake/src/tests.rs @@ -260,7 +260,7 @@ mod on_idle { // then assert_eq!( fast_unstake_events_since_last_call(), - vec![Event::Checking { eras: vec![3] }] + vec![Event::BatchChecked { eras: vec![3] }] ); assert_eq!( Head::::get(), @@ -279,7 +279,7 @@ mod on_idle { // then: assert_eq!( fast_unstake_events_since_last_call(), - vec![Event::Checking { eras: bounded_vec![2] }] + vec![Event::BatchChecked { eras: bounded_vec![2] }] ); assert_eq!( Head::::get(), @@ -304,7 +304,7 @@ mod on_idle { // then: assert_eq!( fast_unstake_events_since_last_call(), - vec![Event::Checking { eras: vec![1, 0] }] + vec![Event::BatchChecked { eras: vec![1, 0] }] ); assert_eq!( Head::::get(), @@ -342,7 +342,7 @@ mod on_idle { // then we finish the unbonding: assert_eq!( fast_unstake_events_since_last_call(), - vec![Event::Unstaked { stash: 1, result: Ok(()) }, Event::Terminated], + vec![Event::Unstaked { stash: 1, result: Ok(()) }, Event::BatchFinished], ); assert_eq!(Head::::get(), None,); @@ -408,10 +408,10 @@ mod on_idle { assert_eq!( fast_unstake_events_since_last_call(), vec![ - Event::Checking { eras: vec![3, 2, 1, 0] }, + Event::BatchChecked { eras: vec![3, 2, 1, 0] }, Event::Unstaked { stash: 1, result: Ok(()) }, - Event::Terminated, - Event::Checking { eras: vec![3, 2, 1, 0] } + Event::BatchFinished, + Event::BatchChecked { eras: vec![3, 2, 1, 0] } ] ); }); @@ -456,12 +456,12 @@ mod on_idle { assert_eq!( fast_unstake_events_since_last_call(), vec![ - Event::Checking { eras: vec![3, 2, 1, 0] }, + Event::BatchChecked { eras: vec![3, 2, 1, 0] }, Event::Unstaked { stash: 1, result: Ok(()) }, - Event::Terminated, - Event::Checking { eras: vec![3, 2, 1, 0] }, + Event::BatchFinished, + Event::BatchChecked { eras: vec![3, 2, 1, 0] }, Event::Unstaked { stash: 3, result: Ok(()) }, - Event::Terminated, + Event::BatchFinished, ] ); @@ -501,9 +501,9 @@ mod on_idle { assert_eq!( fast_unstake_events_since_last_call(), vec![ - Event::Checking { eras: vec![3, 2, 1, 0] }, + Event::BatchChecked { eras: vec![3, 2, 1, 0] }, Event::Unstaked { stash: 1, result: Ok(()) }, - Event::Terminated + Event::BatchFinished ] ); assert_unstaked(&1); @@ -543,9 +543,9 @@ mod on_idle { assert_eq!( fast_unstake_events_since_last_call(), vec![ - Event::Checking { eras: vec![3, 2, 1, 0] }, + Event::BatchChecked { eras: vec![3, 2, 1, 0] }, Event::Unstaked { stash: 1, result: Ok(()) }, - Event::Terminated + Event::BatchFinished ] ); assert_unstaked(&1); @@ -615,12 +615,12 @@ mod on_idle { assert_eq!( fast_unstake_events_since_last_call(), vec![ - Event::Checking { eras: vec![3] }, - Event::Checking { eras: vec![2] }, - Event::Checking { eras: vec![1] }, - Event::Checking { eras: vec![0] }, + Event::BatchChecked { eras: vec![3] }, + Event::BatchChecked { eras: vec![2] }, + Event::BatchChecked { eras: vec![1] }, + Event::BatchChecked { eras: vec![0] }, Event::Unstaked { stash: 1, result: Ok(()) }, - Event::Terminated + Event::BatchFinished ] ); assert_unstaked(&1); @@ -698,13 +698,13 @@ mod on_idle { assert_eq!( fast_unstake_events_since_last_call(), vec![ - Event::Checking { eras: vec![3] }, - Event::Checking { eras: vec![2] }, - Event::Checking { eras: vec![1] }, - Event::Checking { eras: vec![0] }, - Event::Checking { eras: vec![4] }, + Event::BatchChecked { eras: vec![3] }, + Event::BatchChecked { eras: vec![2] }, + Event::BatchChecked { eras: vec![1] }, + Event::BatchChecked { eras: vec![0] }, + Event::BatchChecked { eras: vec![4] }, Event::Unstaked { stash: 1, result: Ok(()) }, - Event::Terminated + Event::BatchFinished ] ); assert_unstaked(&1); @@ -794,12 +794,12 @@ mod on_idle { assert_eq!( fast_unstake_events_since_last_call(), vec![ - Event::Checking { eras: vec![3] }, - Event::Checking { eras: vec![2] }, - Event::Checking { eras: vec![4] }, - Event::Checking { eras: vec![1] }, + Event::BatchChecked { eras: vec![3] }, + Event::BatchChecked { eras: vec![2] }, + Event::BatchChecked { eras: vec![4] }, + Event::BatchChecked { eras: vec![1] }, Event::Unstaked { stash: 1, result: Ok(()) }, - Event::Terminated + Event::BatchFinished ] ); @@ -840,10 +840,10 @@ mod on_idle { assert_eq!( fast_unstake_events_since_last_call(), vec![ - Event::Checking { eras: vec![3] }, - Event::Checking { eras: vec![2] }, + Event::BatchChecked { eras: vec![3] }, + Event::BatchChecked { eras: vec![2] }, Event::Slashed { stash: exposed, amount: Deposit::get() }, - Event::Terminated + Event::BatchFinished ] ); }); @@ -877,9 +877,9 @@ mod on_idle { fast_unstake_events_since_last_call(), // we slash them vec![ - Event::Checking { eras: vec![3, 2] }, + Event::BatchChecked { eras: vec![3, 2] }, Event::Slashed { stash: exposed, amount: Deposit::get() }, - Event::Terminated + Event::BatchFinished ] ); }); @@ -910,7 +910,7 @@ mod on_idle { assert_eq!( fast_unstake_events_since_last_call(), - vec![Event::Slashed { stash: 100, amount: Deposit::get() }, Event::Terminated] + vec![Event::Slashed { stash: 100, amount: Deposit::get() }, Event::BatchFinished] ); }); } @@ -944,9 +944,9 @@ mod on_idle { assert_eq!( fast_unstake_events_since_last_call(), vec![ - Event::Checking { eras: vec![3, 2, 1, 0] }, + Event::BatchChecked { eras: vec![3, 2, 1, 0] }, Event::Unstaked { stash: 42, result: Ok(()) }, - Event::Terminated + Event::BatchFinished ] ); }); @@ -997,11 +997,11 @@ mod batched { assert_eq!( fast_unstake_events_since_last_call(), vec![ - Event::Checking { eras: vec![3, 2, 1, 0] }, + Event::BatchChecked { eras: vec![3, 2, 1, 0] }, Event::Unstaked { stash: 1, result: Ok(()) }, Event::Unstaked { stash: 5, result: Ok(()) }, Event::Unstaked { stash: 7, result: Ok(()) }, - Event::Terminated + Event::BatchFinished ] ); }); @@ -1062,12 +1062,12 @@ mod batched { assert_eq!( fast_unstake_events_since_last_call(), vec![ - Event::Checking { eras: vec![3, 2] }, - Event::Checking { eras: vec![1, 0] }, + Event::BatchChecked { eras: vec![3, 2] }, + Event::BatchChecked { eras: vec![1, 0] }, Event::Unstaked { stash: 1, result: Ok(()) }, Event::Unstaked { stash: 5, result: Ok(()) }, Event::Unstaked { stash: 7, result: Ok(()) }, - Event::Terminated + Event::BatchFinished ] ); }); @@ -1127,12 +1127,12 @@ mod batched { fast_unstake_events_since_last_call(), vec![ Event::Slashed { stash: 667, amount: 7 }, - Event::Checking { eras: vec![3, 2] }, + Event::BatchChecked { eras: vec![3, 2] }, Event::Slashed { stash: 666, amount: 7 }, - Event::Checking { eras: vec![1, 0] }, + Event::BatchChecked { eras: vec![1, 0] }, Event::Unstaked { stash: 1, result: Ok(()) }, Event::Unstaked { stash: 3, result: Ok(()) }, - Event::Terminated + Event::BatchFinished ] ); }); @@ -1189,10 +1189,10 @@ mod batched { fast_unstake_events_since_last_call(), vec![ Event::Slashed { stash: 666, amount: Deposit::get() }, - Event::Checking { eras: vec![3] }, + Event::BatchChecked { eras: vec![3] }, Event::Slashed { stash: 667, amount: Deposit::get() }, - Event::Terminated, - Event::Checking { eras: vec![3] } + Event::BatchFinished, + Event::BatchChecked { eras: vec![3] } ] ); }); From 752d22f8451d3f656c6c4d83ecd25b1a46daa262 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 25 Oct 2022 13:00:13 +0100 Subject: [PATCH 08/14] some fixes --- frame/fast-unstake/src/benchmarking.rs | 2 +- frame/fast-unstake/src/lib.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/fast-unstake/src/benchmarking.rs b/frame/fast-unstake/src/benchmarking.rs index 64b3806d6a2fa..ba048d68aa8c9 100644 --- a/frame/fast-unstake/src/benchmarking.rs +++ b/frame/fast-unstake/src/benchmarking.rs @@ -46,7 +46,7 @@ fn l( fn create_unexposed_nominators() -> Vec { (0..T::BatchSize::get()) .map(|i| { - let account = frame_benchmarking::account::("nominator_42", i, USER_SEED); + let account = frame_benchmarking::account::("nominator", i, USER_SEED); fund_and_bond_account::(&account); account }) diff --git a/frame/fast-unstake/src/lib.rs b/frame/fast-unstake/src/lib.rs index b365fab9e761c..d7c01679cd3a2 100644 --- a/frame/fast-unstake/src/lib.rs +++ b/frame/fast-unstake/src/lib.rs @@ -494,7 +494,7 @@ pub mod pallet { }) .collect::>() .try_into() - .expect("filter can only lesson the length; still in bound; qed"); + .expect("filter can only lessen the length; still in bound; qed"); let post_length = stashes.len(); log!( From 70461dd014247d88092a01e0f2d0efa99fddbba2 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 25 Oct 2022 14:29:54 +0100 Subject: [PATCH 09/14] fix review comments --- frame/fast-unstake/src/lib.rs | 24 ------------------------ frame/fast-unstake/src/migrations.rs | 2 +- 2 files changed, 1 insertion(+), 25 deletions(-) diff --git a/frame/fast-unstake/src/lib.rs b/frame/fast-unstake/src/lib.rs index ead1079ad9fa5..d9eaf860ddfbc 100644 --- a/frame/fast-unstake/src/lib.rs +++ b/frame/fast-unstake/src/lib.rs @@ -205,30 +205,6 @@ pub mod pallet { return Weight::from_ref_time(0) } - // Temporary for migration. - if Head::::exists() && Head::::get().is_none() { - // Migrate head. - let key = Head::::hashed_key(); - let (stash, deposit) = match frame_support::storage::unhashed::get::<( - T::AccountId, - Vec, - BalanceOf, - )>(&key) - .defensive() - { - Some((stash, _, deposit)) => (stash, deposit), - None => { - Head::::kill(); - return T::DbWeight::get().reads_writes(1, 1) - }, - }; - - // insert them back into the queue. - Queue::::insert(stash, deposit); - Head::::kill(); - return T::DbWeight::get().reads_writes(1, 2) - } - Self::do_on_idle(remaining_weight) } } diff --git a/frame/fast-unstake/src/migrations.rs b/frame/fast-unstake/src/migrations.rs index 33f202c9e7aee..490db395ff39e 100644 --- a/frame/fast-unstake/src/migrations.rs +++ b/frame/fast-unstake/src/migrations.rs @@ -56,7 +56,7 @@ pub mod v1 { T::DbWeight::get().reads(2) } } else { - log!(info, "Migration did not executed. This probably should be removed"); + log!(info, "Migration did not execute. This probably should be removed"); T::DbWeight::get().reads(1) } } From 268208ec27f6392e1f091a365c149d7783fe2dc6 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 25 Oct 2022 14:44:52 +0100 Subject: [PATCH 10/14] fix build --- frame/fast-unstake/src/migrations.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/frame/fast-unstake/src/migrations.rs b/frame/fast-unstake/src/migrations.rs index 490db395ff39e..51a84b00c2a26 100644 --- a/frame/fast-unstake/src/migrations.rs +++ b/frame/fast-unstake/src/migrations.rs @@ -16,6 +16,7 @@ // limitations under the License. pub mod v1 { + use sp_std::prelude::*; use crate::{types::BalanceOf, *}; use frame_support::{ storage::unhashed, @@ -62,13 +63,15 @@ pub mod v1 { } #[cfg(feature = "try-runtime")] - fn pre_upgrade(_: Vec) -> Result<(), &'static str> { + fn pre_upgrade() -> Result, &'static str> { assert_eq!(Pallet::::on_chain_storage_version(), 0); + Ok(Default::default()) } #[cfg(feature = "try-runtime")] fn post_upgrade(_: Vec) -> Result<(), &'static str> { assert_eq!(Pallet::::on_chain_storage_version(), 1); + Ok(()) } } } From db2174b3d8e4c9c0c4139b8c2da3fd0999e4a470 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Wed, 26 Oct 2022 10:51:14 +0200 Subject: [PATCH 11/14] fmt --- frame/fast-unstake/src/migrations.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/fast-unstake/src/migrations.rs b/frame/fast-unstake/src/migrations.rs index 51a84b00c2a26..10d8e54134785 100644 --- a/frame/fast-unstake/src/migrations.rs +++ b/frame/fast-unstake/src/migrations.rs @@ -16,7 +16,6 @@ // limitations under the License. pub mod v1 { - use sp_std::prelude::*; use crate::{types::BalanceOf, *}; use frame_support::{ storage::unhashed, @@ -24,6 +23,7 @@ pub mod v1 { weights::Weight, }; use sp_staking::EraIndex; + use sp_std::prelude::*; pub struct MigrateToV1(sp_std::marker::PhantomData); impl OnRuntimeUpgrade for MigrateToV1 { From 1a91286da7eec05765d43a1ba01f94f5cf65cbf7 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 27 Oct 2022 11:33:25 +0200 Subject: [PATCH 12/14] fix benchmarks --- frame/fast-unstake/src/benchmarking.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/fast-unstake/src/benchmarking.rs b/frame/fast-unstake/src/benchmarking.rs index bba3bb8b12845..39f530b7be889 100644 --- a/frame/fast-unstake/src/benchmarking.rs +++ b/frame/fast-unstake/src/benchmarking.rs @@ -46,7 +46,7 @@ fn l( fn create_unexposed_nominators() -> Vec { (0..T::BatchSize::get()) .map(|i| { - let account = frame_benchmarking::account::("nominator", i, USER_SEED); + let account = frame_benchmarking::account::("unexposed_nominator", i, USER_SEED); fund_and_bond_account::(&account); account }) @@ -175,11 +175,11 @@ benchmarks! { let checked = (1..=u).rev().collect::>(); let request = Head::::get().unwrap(); assert_eq!(checked, request.checked.into_inner()); - assert!(stashes.iter().all(|(s, _)| request.stashes.iter().find(|(ss, _)| ss == s).is_some())); assert!(matches!( fast_unstake_events::().last(), Some(Event::BatchChecked { .. }) )); + assert!(stashes.iter().all(|(s, _)| request.stashes.iter().find(|(ss, _)| ss == s).is_some())); } register_fast_unstake { From 73342328e52aac8f5cc976581c4f086b90ef4b9e Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 8 Nov 2022 11:33:44 +0000 Subject: [PATCH 13/14] fmt --- frame/fast-unstake/src/benchmarking.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frame/fast-unstake/src/benchmarking.rs b/frame/fast-unstake/src/benchmarking.rs index 5f37a45b85c8c..b4a5e21dcfc13 100644 --- a/frame/fast-unstake/src/benchmarking.rs +++ b/frame/fast-unstake/src/benchmarking.rs @@ -39,7 +39,8 @@ type CurrencyOf = ::Currency; fn create_unexposed_nominators() -> Vec { (0..T::BatchSize::get()) .map(|i| { - let account = frame_benchmarking::account::("unexposed_nominator", i, USER_SEED); + let account = + frame_benchmarking::account::("unexposed_nominator", i, USER_SEED); fund_and_bond_account::(&account); account }) From d2db1b63ead72b0a7913226cb061a14aa57c3a4a Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 8 Nov 2022 14:22:20 +0000 Subject: [PATCH 14/14] update --- frame/fast-unstake/src/lib.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/frame/fast-unstake/src/lib.rs b/frame/fast-unstake/src/lib.rs index ab72b7b0568d3..c83054189feb7 100644 --- a/frame/fast-unstake/src/lib.rs +++ b/frame/fast-unstake/src/lib.rs @@ -274,9 +274,7 @@ pub mod pallet { if let Some(deposit) = deposit.defensive() { let remaining = T::Currency::unreserve(&stash_account, deposit); if !remaining.is_zero() { - frame_support::defensive!("`not enough balance to unreserve`"); - ErasToCheckPerBlock::::put(0); - Self::deposit_event(Event::::InternalError) + Self::halt("not enough balance to unreserve"); } }