From 3bfed97fc4b9139fe593ab4d7272f59ce41abd66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Thu, 30 May 2024 12:21:03 +0200 Subject: [PATCH] migration docs; clippy fixes --- .../src/migrations/v13_stake_tracker/mod.rs | 34 +++++++++++++++---- .../frame/staking/stake-tracker/src/lib.rs | 3 -- .../frame/staking/stake-tracker/src/mock.rs | 18 ---------- .../frame/staking/stake-tracker/src/tests.rs | 15 +++----- 4 files changed, 32 insertions(+), 38 deletions(-) diff --git a/substrate/frame/staking/src/migrations/v13_stake_tracker/mod.rs b/substrate/frame/staking/src/migrations/v13_stake_tracker/mod.rs index 44d62041601c..097e8f4a468f 100644 --- a/substrate/frame/staking/src/migrations/v13_stake_tracker/mod.rs +++ b/substrate/frame/staking/src/migrations/v13_stake_tracker/mod.rs @@ -50,16 +50,38 @@ impl Default for Processing { /// V13 Multi-block migration to introduce the stake-tracker pallet. /// -/// A step of the migration consists of processing one nominator in the [`Nominators`] list. All -/// nominatior's target nominations are processed per step (bound by upper bound of the max -/// nominations). +/// A step of the migration consists of processing one nominator in the [`Nominators`] list or one +/// validators in the [`Validators`] list, depending on the cursor's state. +/// +/// The migration has two phases: +/// * [`Processing::Nominators`]: iterates over the nominators list and updates the approvals stake +/// of the nominated targets associated with each of the nominators. +/// * [`Processing::Validators`]: iterates over the validators list and, if the validator does not +/// exist in the target list, add it with self-stake as score. +/// +/// First, the migration processes all the nominators and then the validators. When a validator is +/// added to the target list during the [`Processing::Validators`], it indicates that the validator +/// has no nominations. /// /// The goals of the migration are: -/// - Insert all the nominated targets into the [`SortedListProvider`] target list. -/// - Ensure the target score (total stake) is the sum of the self stake and all its nominations +/// * Insert all the nominated targets into the [`SortedListProvider`] target list. +/// * Ensure the target score (total stake) is the sum of the self stake and all its nominations /// stake. -/// - Ensure the new targets in the list are sorted per total stake (as per the underlying +/// * Ensure the new targets in the list are sorted per total stake (as per the underlying /// [`SortedListProvider`]). +/// * Ensure that there is no duplicate nominations per nominator. +/// +/// ## Potential changes to nominations state during the migration. +/// +/// When migrating a nominator, we need to ensure that the nominator is not nominating duplicate +/// targets. In addition, this migration also "cleans" the nominations by dropping all nominations +/// of targets that are not active validators. This logic is implemented by +/// [`MigrationV13::clean_nominations`]. In sum, the followinf invariants hold true after +/// processing each nominator: +/// +/// 1. A nominator has no duplicate nominations; +/// 2. A nominators is nominating only active validators; +/// 3. A nominator has at least one nomination, otherwise it is chilled. pub struct MigrationV13(PhantomData<(T, W)>); impl SteppedMigration for MigrationV13 { // nominator cursor and validator cursor. diff --git a/substrate/frame/staking/stake-tracker/src/lib.rs b/substrate/frame/staking/stake-tracker/src/lib.rs index bce77b7a2373..176a0116f52a 100644 --- a/substrate/frame/staking/stake-tracker/src/lib.rs +++ b/substrate/frame/staking/stake-tracker/src/lib.rs @@ -79,9 +79,6 @@ //! be removed onced all the voters stop nominating the unbonded account (i.e. the target's score //! drops to 0). //! -//! For further details on the target list invariante, refer to [`Self`::do_try_state_approvals`] -//! and [`Self::do_try_state_target_sorting`]. -//! //! ## Event emitter ordering and staking ledger state updates //! //! It is important to ensure that the events are emitted from staking (i.e. the calls into diff --git a/substrate/frame/staking/stake-tracker/src/mock.rs b/substrate/frame/staking/stake-tracker/src/mock.rs index abc8b999e453..cdc10896b200 100644 --- a/substrate/frame/staking/stake-tracker/src/mock.rs +++ b/substrate/frame/staking/stake-tracker/src/mock.rs @@ -461,10 +461,6 @@ pub(crate) fn voter_bags_events() -> Vec>() } -parameter_types! { - pub static DisableTryRuntimeChecks: bool = false; -} - #[derive(Default, Copy, Clone)] pub struct ExtBuilder { populate_lists: bool, @@ -481,12 +477,6 @@ impl ExtBuilder { self } - #[allow(dead_code)] - pub fn try_state(self, enable: bool) -> Self { - DisableTryRuntimeChecks::set(!enable); - self - } - pub fn build(self) -> sp_io::TestExternalities { sp_tracing::try_init_simple(); let storage = frame_system::GenesisConfig::::default().build_storage().unwrap(); @@ -506,13 +496,5 @@ impl ExtBuilder { System::set_block_number(1); }); ext.execute_with(test); - - if !DisableTryRuntimeChecks::get() { - ext.execute_with(|| { - StakeTracker::do_try_state() - .map_err(|err| println!(" 🕵️‍♂️ try_state failure: {:?}", err)) - .unwrap(); - }); - } } } diff --git a/substrate/frame/staking/stake-tracker/src/tests.rs b/substrate/frame/staking/stake-tracker/src/tests.rs index 41236feb4968..53e204f49abc 100644 --- a/substrate/frame/staking/stake-tracker/src/tests.rs +++ b/substrate/frame/staking/stake-tracker/src/tests.rs @@ -60,11 +60,10 @@ fn update_target_score_works() { let current_score = TargetBagsList::get_score(&10).unwrap(); crate::Pallet::::update_target_score(&10, StakeImbalance::Negative(current_score)); - assert_eq!(TargetBagsList::get_score(&10), Ok(0)); - // disables the try runtime checks since the score of 10 was updated manually, so the target - // list was not updated accordingly. - DisableTryRuntimeChecks::set(true); + // score dropped to 0, node is removed. + assert!(!TargetBagsList::contains(&10)); + assert!(TargetBagsList::get_score(&10).is_err()); }) } @@ -446,13 +445,7 @@ mod staking_integration { chill_staker(2); assert_eq!(StakingMock::status(&2), Ok(StakerStatus::Idle)); - // a chilled validator is kepts in the target list. - assert!(TargetBagsList::contains(&2)); - assert!(!VoterBagsList::contains(&2)); - - remove_staker(2); - assert!(StakingMock::status(&2).is_err()); - // a chilled validator is kepts in the target list if its score is 0. + // a chilled validator is dropped from the target list if its score is 0. assert!(!TargetBagsList::contains(&2)); assert!(!VoterBagsList::contains(&2)); })