From 1b8e11b3e147292e86e29e64074c32126bb8ea5c Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 4 May 2022 18:47:35 +0100 Subject: [PATCH 01/14] Add Score to Bags List --- frame/bags-list/fuzzer/src/main.rs | 11 +- frame/bags-list/src/lib.rs | 40 +++++++- frame/bags-list/src/list/mod.rs | 59 ++++++----- frame/bags-list/src/list/tests.rs | 111 ++++++++++++--------- frame/bags-list/src/migrations.rs | 4 +- frame/bags-list/src/tests.rs | 21 ++-- frame/election-provider-support/src/lib.rs | 34 +++++-- frame/staking/src/pallet/impls.rs | 5 +- 8 files changed, 189 insertions(+), 96 deletions(-) diff --git a/frame/bags-list/fuzzer/src/main.rs b/frame/bags-list/fuzzer/src/main.rs index d0586f0372ac4..c17fbe0a2f77f 100644 --- a/frame/bags-list/fuzzer/src/main.rs +++ b/frame/bags-list/fuzzer/src/main.rs @@ -70,13 +70,20 @@ fn main() { }, Action::Update => { let already_contains = BagsList::contains(&id); - BagsList::on_update(&id, vote_weight).unwrap(); if already_contains { + BagsList::on_update(&id, vote_weight).unwrap(); assert!(BagsList::contains(&id)); + } else { + BagsList::on_update(&id, vote_weight).unwrap_err(); } }, Action::Remove => { - BagsList::on_remove(&id).unwrap(); + let already_contains = BagsList::contains(&id); + if already_contains { + BagsList::on_remove(&id).unwrap(); + } else { + BagsList::on_remove(&id).unwrap_err(); + } assert!(!BagsList::contains(&id)); }, } diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index 77fe609ad8123..7eee8fdfa23d8 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -82,7 +82,10 @@ macro_rules! log { ($level:tt, $patter:expr $(, $values:expr)* $(,)?) => { log::$level!( target: crate::LOG_TARGET, - concat!("[{:?}] 👜", $patter), >::block_number() $(, $values)* + concat!("[{:?}] 👜 [{}]", $patter), + >::block_number(), + as frame_support::traits::PalletInfoAccess>::name() + $(, $values)* ) }; } @@ -189,6 +192,8 @@ pub mod pallet { pub enum Event, I: 'static = ()> { /// Moved an account from one bag to another. Rebagged { who: T::AccountId, from: T::Score, to: T::Score }, + /// Updated the score of some account to the given amount. + ScoreUpdated { who: T::AccountId, new_score: T::Score }, } #[pallet::error] @@ -212,13 +217,16 @@ pub mod pallet { /// /// Anyone can call this function about any potentially dislocated account. /// - /// Will never return an error; if `dislocated` does not exist or doesn't need a rebag, then - /// it is a noop and fees are still collected from `origin`. + /// Will always update the stored score of `dislocated` to the correct score, based on + /// `ScoreProvider`. + /// + /// If `dislocated` does not exists, it returns an error. #[pallet::weight(T::WeightInfo::rebag_non_terminal().max(T::WeightInfo::rebag_terminal()))] pub fn rebag(origin: OriginFor, dislocated: T::AccountId) -> DispatchResult { ensure_signed(origin)?; let current_score = T::ScoreProvider::score(&dislocated); - let _ = Pallet::::do_rebag(&dislocated, current_score); + let _ = Pallet::::do_rebag(&dislocated, current_score) + .map_err::, _>(Into::into)?; Ok(()) } @@ -265,6 +273,7 @@ impl, I: 'static> Pallet { if let Some((from, to)) = maybe_movement { Self::deposit_event(Event::::Rebagged { who: account.clone(), from, to }); }; + Self::deposit_event(Event::::ScoreUpdated { who: account.clone(), new_score }); Ok(maybe_movement) } @@ -302,6 +311,10 @@ impl, I: 'static> SortedListProvider for Pallet List::::insert(id, score) } + fn get_score(id: &T::AccountId) -> Result { + List::::get_score(id) + } + fn on_update(id: &T::AccountId, new_score: T::Score) -> Result<(), ListError> { Pallet::::do_rebag(id, new_score).map(|_| ()) } @@ -359,3 +372,22 @@ impl, I: 'static> SortedListProvider for Pallet } } } + +impl, I: 'static> ScoreProvider for Pallet { + type Score = as SortedListProvider>::Score; + + fn score(id: &T::AccountId) -> T::Score { + Node::::get(id).map(|node| node.score()).unwrap_or_default() + } + + #[cfg(any(feature = "runtime-benchmarks", test))] + fn set_score_of(id: &T::AccountId, new_score: T::Score) { + ListNodes::::mutate(id, |maybe_node| { + if let Some(node) = maybe_node.as_mut() { + node.set_score(new_score) + } else { + panic!("trying to mutate {:?} which does not exists", id); + } + }) + } +} diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs index f93bfca1081e9..0ba2a9c503b9d 100644 --- a/frame/bags-list/src/list/mod.rs +++ b/frame/bags-list/src/list/mod.rs @@ -28,6 +28,7 @@ use crate::Config; use codec::{Decode, Encode, MaxEncodedLen}; use frame_election_provider_support::ScoreProvider; use frame_support::{ + ensure, traits::{Defensive, Get}, DefaultNoBound, PalletError, }; @@ -38,7 +39,7 @@ use sp_std::{ collections::{btree_map::BTreeMap, btree_set::BTreeSet}, iter, marker::PhantomData, - vec::Vec, + prelude::*, }; #[derive(Debug, PartialEq, Eq, Encode, Decode, MaxEncodedLen, TypeInfo, PalletError)] @@ -227,6 +228,11 @@ impl, I: 'static> List { crate::ListNodes::::contains_key(id) } + /// Get the score of the given node, + pub fn get_score(id: &T::AccountId) -> Result { + Node::::get(id).map(|node| node.score()).ok_or(ListError::NodeNotFound) + } + /// Iterate over all nodes in all bags in the list. /// /// Full iteration can be expensive; it's recommended to limit the number of items with @@ -310,7 +316,7 @@ impl, I: 'static> List { let bag_score = notional_bag_for::(score); let mut bag = Bag::::get_or_make(bag_score); // unchecked insertion is okay; we just got the correct `notional_bag_for`. - bag.insert_unchecked(id.clone()); + bag.insert_unchecked(id.clone(), score); // new inserts are always the tail, so we must write the bag. bag.put(); @@ -381,16 +387,18 @@ impl, I: 'static> List { /// If the node was in the correct bag, no effect. If the node was in the incorrect bag, they /// are moved into the correct bag. /// - /// Returns `Some((old_idx, new_idx))` if the node moved, otherwise `None`. + /// Returns `Some((old_idx, new_idx))` if the node moved, otherwise `None`. In both cases, the + /// node's score is written to the `score_score`. Thus, this is not a noop, even if `None`. /// /// This operation is somewhat more efficient than simply calling [`self.remove`] followed by /// [`self.insert`]. However, given large quantities of nodes to move, it may be more efficient /// to call [`self.remove_many`] followed by [`self.insert_many`]. pub(crate) fn update_position_for( - node: Node, + mut node: Node, new_score: T::Score, ) -> Option<(T::Score, T::Score)> { - node.is_misplaced(new_score).then(move || { + node.score = new_score; + if node.is_misplaced(new_score) { let old_bag_upper = node.bag_upper; if !node.is_terminal() { @@ -402,12 +410,9 @@ impl, I: 'static> List { bag.remove_node_unchecked(&node); bag.put(); } else { - crate::log!( - error, - "Node {:?} did not have a bag; ListBags is in an inconsistent state", - node.id, + frame_support::defensive!( + "Node did not have a bag; BagsList is in an inconsistent state" ); - debug_assert!(false, "every node must have an extant bag associated with it"); } // put the node into the appropriate new bag. @@ -418,8 +423,12 @@ impl, I: 'static> List { bag.insert_node_unchecked(node); bag.put(); - (old_bag_upper, new_bag_upper) - }) + Some((old_bag_upper, new_bag_upper)) + } else { + // just write the new score. + node.put(); + None + } } /// Put `heavier_id` to the position directly in front of `lighter_id`. Both ids must be in the @@ -428,8 +437,6 @@ impl, I: 'static> List { lighter_id: &T::AccountId, heavier_id: &T::AccountId, ) -> Result<(), ListError> { - use frame_support::ensure; - let lighter_node = Node::::get(&lighter_id).ok_or(ListError::NodeNotFound)?; let heavier_node = Node::::get(&heavier_id).ok_or(ListError::NodeNotFound)?; @@ -510,7 +517,6 @@ impl, I: 'static> List { /// all bags and nodes are checked per *any* update to `List`. #[cfg(feature = "std")] pub(crate) fn sanity_check() -> Result<(), &'static str> { - use frame_support::ensure; let mut seen_in_list = BTreeSet::new(); ensure!( Self::iter().map(|node| node.id).all(|id| seen_in_list.insert(id)), @@ -523,7 +529,7 @@ impl, I: 'static> List { ensure!(iter_count == stored_count, "iter_count != stored_count"); ensure!(stored_count == nodes_count, "stored_count != nodes_count"); - crate::log!(debug, "count of nodes: {}", stored_count); + crate::log!(trace, "count of nodes: {}", stored_count); let active_bags = { let thresholds = T::BagThresholds::get().iter().copied(); @@ -544,7 +550,7 @@ impl, I: 'static> List { active_bags.clone().fold(0u32, |acc, cur| acc + cur.iter().count() as u32); ensure!(nodes_count == nodes_in_bags_count, "stored_count != nodes_in_bags_count"); - crate::log!(debug, "count of active bags {}", active_bags.count()); + crate::log!(trace, "count of active bags {}", active_bags.count()); // check that all nodes are sane. We check the `ListNodes` storage item directly in case we // have some "stale" nodes that are not in a bag. @@ -667,7 +673,7 @@ impl, I: 'static> Bag { /// /// Storage note: this modifies storage, but only for the nodes. You still need to call /// `self.put()` after use. - fn insert_unchecked(&mut self, id: T::AccountId) { + fn insert_unchecked(&mut self, id: T::AccountId, score: T::Score) { // insert_node will overwrite `prev`, `next` and `bag_upper` to the proper values. As long // as this bag is the correct one, we're good. All calls to this must come after getting the // correct [`notional_bag_for`]. @@ -676,6 +682,7 @@ impl, I: 'static> Bag { prev: None, next: None, bag_upper: Zero::zero(), + score, _phantom: PhantomData, }); } @@ -784,11 +791,6 @@ impl, I: 'static> Bag { Ok(()) } - #[cfg(not(feature = "std"))] - fn sanity_check(&self) -> Result<(), &'static str> { - Ok(()) - } - /// Iterate over the nodes in this bag (public for tests). #[cfg(feature = "std")] #[allow(dead_code)] @@ -813,6 +815,7 @@ pub struct Node, I: 'static = ()> { prev: Option, next: Option, bag_upper: T::Score, + score: T::Score, #[codec(skip)] _phantom: PhantomData, } @@ -877,6 +880,11 @@ impl, I: 'static> Node { &self.id } + /// Get the current vote weight of the node. + pub(crate) fn score(&self) -> T::Score { + self.score + } + /// Get the underlying voter (public fo tests). #[cfg(feature = "std")] #[allow(dead_code)] @@ -884,6 +892,11 @@ impl, I: 'static> Node { &self.id } + #[cfg(any(feature = "runtime-benchmarks", test))] + pub fn set_score(&mut self, s: T::Score) { + self.score = s + } + /// The bag this nodes belongs to (public for benchmarks). #[cfg(feature = "runtime-benchmarks")] #[allow(dead_code)] diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs index ff7dd2871c237..23458aa173d73 100644 --- a/frame/bags-list/src/list/tests.rs +++ b/frame/bags-list/src/list/tests.rs @@ -21,20 +21,20 @@ use crate::{ ListBags, ListNodes, }; use frame_election_provider_support::{SortedListProvider, VoteWeight}; -use frame_support::{assert_noop, assert_ok, assert_storage_noop}; +use frame_support::{assert_ok, assert_storage_noop}; + +fn node( + id: AccountId, + prev: Option, + next: Option, + bag_upper: VoteWeight, +) -> Node { + Node:: { id, prev, next, bag_upper, score: bag_upper, _phantom: PhantomData } +} #[test] fn basic_setup_works() { ExtBuilder::default().build_and_execute(|| { - // syntactic sugar to create a raw node - let node = |id, prev, next, bag_upper| Node:: { - id, - prev, - next, - bag_upper, - _phantom: PhantomData, - }; - assert_eq!(ListNodes::::count(), 4); assert_eq!(ListNodes::::iter().count(), 4); assert_eq!(ListBags::::iter().count(), 2); @@ -83,10 +83,10 @@ fn notional_bag_for_works() { let max_explicit_threshold = *::BagThresholds::get().last().unwrap(); assert_eq!(max_explicit_threshold, 10_000); - // if the max explicit threshold is less than T::Value::max_value(), + // if the max explicit threshold is less than T::Score::max_value(), assert!(VoteWeight::MAX > max_explicit_threshold); - // then anything above it will belong to the T::Value::max_value() bag. + // then anything above it will belong to the T::Score::max_value() bag. assert_eq!(notional_bag_for::(max_explicit_threshold), max_explicit_threshold); assert_eq!(notional_bag_for::(max_explicit_threshold + 1), VoteWeight::MAX); } @@ -154,6 +154,8 @@ fn migrate_works() { } mod list { + use frame_support::assert_noop; + use super::*; #[test] @@ -175,7 +177,7 @@ mod list { ] ); - // when adding an id that has a higher weight than pre-existing ids in the bag + // when adding an id that has a higher score than pre-existing ids in the bag assert_ok!(List::::insert(7, 10)); // then @@ -246,10 +248,7 @@ mod list { assert!(get_list_as_ids().contains(&3)); // then - assert_storage_noop!(assert_eq!( - List::::insert(3, 20).unwrap_err(), - ListError::Duplicate - )); + assert_noop!(List::::insert(3, 20), ListError::Duplicate); }); } @@ -317,37 +316,36 @@ mod list { ExtBuilder::default().build_and_execute(|| { // given a correctly placed account 1 at bag 10. let node = Node::::get(&1).unwrap(); + assert_eq!(node.score, 10); assert!(!node.is_misplaced(10)); - // .. it is invalid with weight 20 + // .. it is invalid with score 20 assert!(node.is_misplaced(20)); // move it to bag 20. - assert_eq!(List::::update_position_for(node, 20), Some((10, 20))); + assert_eq!(List::::update_position_for(node.clone(), 20), Some((10, 20))); + assert_eq!(Node::::get(&1).unwrap().score, 20); assert_eq!(List::::get_bags(), vec![(20, vec![1]), (1_000, vec![2, 3, 4])]); - // get the new updated node; try and update the position with no change in weight. + // get the new updated node; try and update the position with no change in score. let node = Node::::get(&1).unwrap(); assert_storage_noop!(assert_eq!( List::::update_position_for(node.clone(), 20), None )); - // then move it to bag 1_000 by giving it weight 500. + // then move it to bag 1_000 by giving it score 500. assert_eq!(List::::update_position_for(node.clone(), 500), Some((20, 1_000))); + assert_eq!(Node::::get(&1).unwrap().score, 500); assert_eq!(List::::get_bags(), vec![(1_000, vec![2, 3, 4, 1])]); // moving within that bag again is a noop let node = Node::::get(&1).unwrap(); - assert_storage_noop!(assert_eq!( - List::::update_position_for(node.clone(), 750), - None, - )); - assert_storage_noop!(assert_eq!( - List::::update_position_for(node, 1_000), - None, - )); + assert_eq!(List::::update_position_for(node.clone(), 750), None); + assert_eq!(Node::::get(&1).unwrap().score, 750); + assert_eq!(List::::update_position_for(node.clone(), 1_000), None,); + assert_eq!(Node::::get(&1).unwrap().score, 1_000); }); } @@ -359,7 +357,7 @@ mod list { // make sure there are no duplicates. ExtBuilder::default().build_and_execute_no_post_check(|| { - Bag::::get(10).unwrap().insert_unchecked(2); + Bag::::get(10).unwrap().insert_unchecked(2, 10); assert_eq!(List::::sanity_check(), Err("duplicate identified")); }); @@ -399,6 +397,7 @@ mod list { prev: None, next: None, bag_upper: 15, + score: 15, _phantom: PhantomData, }; let node_11_no_bag = Node:: { @@ -406,6 +405,7 @@ mod list { prev: None, next: None, bag_upper: 15, + score: 15, _phantom: PhantomData, }; @@ -437,6 +437,7 @@ mod list { prev: Some(1), next: Some(2), bag_upper: 1_000, + score: 1_000, _phantom: PhantomData, }; assert!(!crate::ListNodes::::contains_key(42)); @@ -466,6 +467,7 @@ mod list { prev: Some(4), next: None, bag_upper: 1_000, + score: 1_000, _phantom: PhantomData, }; assert!(!crate::ListNodes::::contains_key(42)); @@ -495,6 +497,7 @@ mod list { prev: None, next: Some(2), bag_upper: 1_000, + score: 1_000, _phantom: PhantomData, }; assert!(!crate::ListNodes::::contains_key(42)); @@ -524,6 +527,7 @@ mod list { prev: Some(42), next: Some(42), bag_upper: 1_000, + score: 1_000, _phantom: PhantomData, }; assert!(!crate::ListNodes::::contains_key(42)); @@ -588,6 +592,7 @@ mod bags { prev: None, next: None, bag_upper, + score: bag_upper, _phantom: PhantomData, }; @@ -598,7 +603,14 @@ mod bags { assert_eq!( ListNodes::::get(&42).unwrap(), - Node { bag_upper: 10, prev: Some(1), next: None, id: 42, _phantom: PhantomData } + Node { + bag_upper: 10, + score: 5, + prev: Some(1), + next: None, + id: 42, + _phantom: PhantomData + } ); }); } @@ -611,6 +623,7 @@ mod bags { prev: None, next: None, bag_upper, + score: bag_upper, _phantom: PhantomData, }; @@ -638,6 +651,7 @@ mod bags { prev: Some(21), next: Some(101), bag_upper: 20, + score: 20, _phantom: PhantomData, }; bag_20.insert_node_unchecked(node_61); @@ -651,6 +665,7 @@ mod bags { prev: Some(62), next: None, bag_upper: 20, + score: 20, _phantom: PhantomData, } ); @@ -667,13 +682,6 @@ mod bags { // Document improper ways `insert_node` may be getting used. #[test] fn insert_node_bad_paths_documented() { - let node = |id, prev, next, bag_upper| Node:: { - id, - prev, - next, - bag_upper, - _phantom: PhantomData, - }; ExtBuilder::default().build_and_execute_no_post_check(|| { // when inserting a node with both prev & next pointing at an account in an incorrect // bag. @@ -686,7 +694,14 @@ mod bags { // and when the node is re-fetched all the info is correct assert_eq!( Node::::get(&42).unwrap(), - node(42, Some(4), None, bag_1000.bag_upper) + Node:: { + id: 42, + prev: Some(4), + next: None, + bag_upper: bag_1000.bag_upper, + score: 500, + _phantom: PhantomData + } ); }); @@ -722,7 +737,14 @@ mod bags { // and the re-fetched node has bad pointers assert_eq!( Node::::get(&2).unwrap(), - node(2, Some(4), None, bag_1000.bag_upper) + Node:: { + id: 2, + prev: Some(4), + next: None, + bag_upper: bag_1000.bag_upper, + score: 0, + _phantom: PhantomData + }, ); // ^^^ despite being the bags head, it has a prev @@ -741,14 +763,6 @@ mod bags { )] fn insert_node_duplicate_tail_panics_with_debug_assert() { ExtBuilder::default().build_and_execute(|| { - let node = |id, prev, next, bag_upper| Node:: { - id, - prev, - next, - bag_upper, - _phantom: PhantomData, - }; - // given assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])],); let mut bag_1000 = Bag::::get(1_000).unwrap(); @@ -879,6 +893,7 @@ mod bags { prev: None, next: Some(3), bag_upper: 10, // should be 1_000 + score: 10, _phantom: PhantomData, }; let mut bag_1000 = Bag::::get(1_000).unwrap(); diff --git a/frame/bags-list/src/migrations.rs b/frame/bags-list/src/migrations.rs index 696733e8c7ba5..28ccb3b654fc2 100644 --- a/frame/bags-list/src/migrations.rs +++ b/frame/bags-list/src/migrations.rs @@ -20,8 +20,8 @@ use frame_support::traits::OnRuntimeUpgrade; /// A struct that does not migration, but only checks that the counter prefix exists and is correct. -pub struct CheckCounterPrefix(sp_std::marker::PhantomData); -impl OnRuntimeUpgrade for CheckCounterPrefix { +pub struct CheckCounterPrefix>(sp_std::marker::PhantomData<(I, T)>); +impl> OnRuntimeUpgrade for CheckCounterPrefix { fn on_runtime_upgrade() -> frame_support::weights::Weight { 0 } diff --git a/frame/bags-list/src/tests.rs b/frame/bags-list/src/tests.rs index c63ab26d59a67..01c1642f882c1 100644 --- a/frame/bags-list/src/tests.rs +++ b/frame/bags-list/src/tests.rs @@ -35,8 +35,10 @@ mod pallet { ); // when increasing score to the level of non-existent bag + assert_eq!(List::::get_score(&42).unwrap(), 20); StakingMock::set_score_of(&42, 2_000); assert_ok!(BagsList::rebag(Origin::signed(0), 42)); + assert_eq!(List::::get_score(&42).unwrap(), 2_000); // then a new bag is created and the id moves into it assert_eq!( @@ -53,6 +55,8 @@ mod pallet { List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4]), (2_000, vec![42])] ); + // but the score is updated + assert_eq!(List::::get_score(&42).unwrap(), 1_001); // when reducing score to the level of a non-existent bag StakingMock::set_score_of(&42, 30); @@ -63,6 +67,7 @@ mod pallet { List::::get_bags(), vec![(10, vec![1]), (30, vec![42]), (1_000, vec![2, 3, 4])] ); + assert_eq!(List::::get_score(&42).unwrap(), 30); // when increasing score to the level of a pre-existing bag StakingMock::set_score_of(&42, 500); @@ -73,6 +78,7 @@ mod pallet { List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4, 42])] ); + assert_eq!(List::::get_score(&42).unwrap(), 500); }); } @@ -145,21 +151,20 @@ mod pallet { } #[test] - fn wrong_rebag_is_noop() { + fn wrong_rebag_errs() { ExtBuilder::default().build_and_execute(|| { let node_3 = list::Node::::get(&3).unwrap(); - // when account 3 is _not_ misplaced with weight 500 + // when account 3 is _not_ misplaced with score 500 NextVoteWeight::set(500); assert!(!node_3.is_misplaced(500)); - // then calling rebag on account 3 with weight 500 is a noop + // then calling rebag on account 3 with score 500 is a noop assert_storage_noop!(assert_eq!(BagsList::rebag(Origin::signed(0), 3), Ok(()))); // when account 42 is not in the list assert!(!BagsList::contains(&42)); - - // then rebag-ing account 42 is a noop - assert_storage_noop!(assert_eq!(BagsList::rebag(Origin::signed(0), 42), Ok(()))); + // then rebag-ing account 42 is an error + assert_storage_noop!(assert!(matches!(BagsList::rebag(Origin::signed(0), 42), Err(_)))); }); } @@ -182,7 +187,6 @@ mod pallet { #[test] fn empty_threshold_works() { BagThresholds::set(Default::default()); // which is the same as passing `()` to `Get<_>`. - ExtBuilder::default().build_and_execute(|| { // everyone in the same bag. assert_eq!(List::::get_bags(), vec![(VoteWeight::MAX, vec![1, 2, 3, 4])]); @@ -196,8 +200,7 @@ mod pallet { ); // any rebag is noop. - assert_storage_noop!(assert!(BagsList::rebag(Origin::signed(0), 1).is_ok())); - assert_storage_noop!(assert!(BagsList::rebag(Origin::signed(0), 10).is_ok())); + assert_storage_noop!(assert_eq!(BagsList::rebag(Origin::signed(0), 1), Ok(()))); }) } diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index af366d5b8f919..27bf7a37f9622 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -168,15 +168,12 @@ pub mod onchain; pub mod traits; -#[cfg(feature = "std")] -use codec::{Decode, Encode}; -use frame_support::{weights::Weight, BoundedVec, RuntimeDebug}; use sp_runtime::traits::{Bounded, Saturating, Zero}; use sp_std::{fmt::Debug, prelude::*}; /// Re-export the solution generation macro. pub use frame_election_provider_solution_type::generate_solution_type; -pub use frame_support::traits::Get; +pub use frame_support::{traits::Get, weights::Weight, BoundedVec, RuntimeDebug}; /// Re-export some type as they are used in the interface. pub use sp_arithmetic::PerThing; pub use sp_npos_elections::{ @@ -223,7 +220,7 @@ impl __OrInvalidIndex for Option { /// making it fast to repeatedly encode into a `SolutionOf`. This property turns out /// to be important when trimming for solution length. #[derive(RuntimeDebug, Clone, Default)] -#[cfg_attr(feature = "std", derive(PartialEq, Eq, Encode, Decode))] +#[cfg_attr(feature = "std", derive(PartialEq, Eq, codec::Encode, codec::Decode))] pub struct IndexAssignment { /// Index of the voter among the voters list. pub who: VoterIndex, @@ -467,6 +464,29 @@ pub trait SortedListProvider { /// Returns `Ok(())` iff it successfully updates an item, an `Err(_)` otherwise. fn on_update(id: &AccountId, score: Self::Score) -> Result<(), Self::Error>; + /// Get the score of `id`. + fn get_score(id: &AccountId) -> Result; + + /// Same as `on_update`, but incorporate some increased score. + fn on_increase(id: &AccountId, additional: Self::Score) -> Result<(), Self::Error> { + let old_score = Self::get_score(id)?; + let new_score = old_score.saturating_add(additional); + Self::on_update(id, new_score) + } + + /// Same as `on_update`, but incorporate some decreased score. + /// + /// If the new score of the item is `Zero`, it is removed. + fn on_decrease(id: &AccountId, decreased: Self::Score) -> Result<(), Self::Error> { + let old_score = Self::get_score(id)?; + let new_score = old_score.saturating_sub(decreased); + if new_score.is_zero() { + Self::on_remove(id) + } else { + Self::on_update(id, new_score) + } + } + /// Hook for removing am id from the list. /// /// Returns `Ok(())` iff it successfully removes an item, an `Err(_)` otherwise. @@ -504,7 +524,7 @@ pub trait SortedListProvider { } } -/// Something that can provide the `VoteWeight` of an account. Similar to [`ElectionProvider`] and +/// Something that can provide the `Score` of an account. Similar to [`ElectionProvider`] and /// [`ElectionDataProvider`], this should typically be implementing by whoever is supposed to *use* /// `SortedListProvider`. pub trait ScoreProvider { @@ -513,7 +533,7 @@ pub trait ScoreProvider { /// Get the current `Score` of `who`. fn score(who: &AccountId) -> Self::Score; - /// For tests and benchmarks, set the `VoteWeight`. + /// For tests and benchmarks, set the `score`. #[cfg(any(feature = "runtime-benchmarks", test))] fn set_score_of(_: &AccountId, _: Self::Score) {} } diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 34d7a176670f2..4de1ea36cb591 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -1343,7 +1343,10 @@ impl SortedListProvider for UseNominatorsAndValidatorsM // nothing to do on insert. Ok(()) } - fn on_update(_: &T::AccountId, _weight: VoteWeight) -> Result<(), Self::Error> { + fn get_score(id: &T::AccountId) -> Result { + Ok(Pallet::::weight_of(id)) + } + fn on_update(_: &T::AccountId, _weight: Self::Score) -> Result<(), Self::Error> { // nothing to do on update. Ok(()) } From 8a193418d9a4df7233b60234f54da4c80481e5db Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 4 May 2022 18:53:10 +0100 Subject: [PATCH 02/14] fix ordering --- frame/bags-list/src/migrations.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/bags-list/src/migrations.rs b/frame/bags-list/src/migrations.rs index 28ccb3b654fc2..681a79014d0ef 100644 --- a/frame/bags-list/src/migrations.rs +++ b/frame/bags-list/src/migrations.rs @@ -20,8 +20,8 @@ use frame_support::traits::OnRuntimeUpgrade; /// A struct that does not migration, but only checks that the counter prefix exists and is correct. -pub struct CheckCounterPrefix>(sp_std::marker::PhantomData<(I, T)>); -impl> OnRuntimeUpgrade for CheckCounterPrefix { +pub struct CheckCounterPrefix, I: 'static>(sp_std::marker::PhantomData<(T, I)>); +impl, I: 'static> OnRuntimeUpgrade for CheckCounterPrefix { fn on_runtime_upgrade() -> frame_support::weights::Weight { 0 } From 8ac50da7bda1b9ff94e2ac5bc1260b6b6538dffd Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 4 May 2022 19:08:54 +0100 Subject: [PATCH 03/14] make compile --- frame/bags-list/src/migrations.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/bags-list/src/migrations.rs b/frame/bags-list/src/migrations.rs index 681a79014d0ef..37011c1bd77eb 100644 --- a/frame/bags-list/src/migrations.rs +++ b/frame/bags-list/src/migrations.rs @@ -34,14 +34,14 @@ impl, I: 'static> OnRuntimeUpgrade for CheckCounterPrefix::count() == CounterForListNodes::get().unwrap(), + crate::ListNodes::::count() == CounterForListNodes::get().unwrap(), "wrong list node counter" ); crate::log!( info, "checked bags-list prefix to be correct and have {} nodes", - crate::ListNodes::::count() + crate::ListNodes::::count() ); Ok(()) From 8825e6b4166dcbdf128d4ef4cb68fc90b8aaba38 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 4 May 2022 19:39:30 +0100 Subject: [PATCH 04/14] in progress migration --- frame/bags-list/src/migrations.rs | 68 ++++++++++++++++++++++++++++++- 1 file changed, 66 insertions(+), 2 deletions(-) diff --git a/frame/bags-list/src/migrations.rs b/frame/bags-list/src/migrations.rs index 37011c1bd77eb..7cc93945a9e38 100644 --- a/frame/bags-list/src/migrations.rs +++ b/frame/bags-list/src/migrations.rs @@ -17,7 +17,10 @@ //! The migrations of this pallet. -use frame_support::traits::OnRuntimeUpgrade; +use codec::{Decode, Encode}; +use core::marker::PhantomData; +use frame_support::{ensure, storage::migration, traits::OnRuntimeUpgrade}; +use frame_election_provider_support::ScoreProvider; /// A struct that does not migration, but only checks that the counter prefix exists and is correct. pub struct CheckCounterPrefix, I: 'static>(sp_std::marker::PhantomData<(T, I)>); @@ -28,7 +31,6 @@ impl, I: 'static> OnRuntimeUpgrade for CheckCounterPrefix Result<(), &'static str> { - use frame_support::ensure; // The old explicit storage item. frame_support::generate_storage_alias!(BagsList, CounterForListNodes => Value); @@ -47,3 +49,65 @@ impl, I: 'static> OnRuntimeUpgrade for CheckCounterPrefix, I: 'static = ()> { + id: T::AccountId, + prev: Option, + next: Option, + bag_upper: T::Score, + #[codec(skip)] + _phantom: PhantomData, +} + +/// A struct that migrates all bags lists to contain a score value. +pub struct AddScore, I: 'static>(sp_std::marker::PhantomData<(T, I)>); +impl, I: 'static> OnRuntimeUpgrade for AddScore { + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result<(), &'static str> { + ensure!( + crate::ListNodes::::iter().count() == 0, + "Items already exist where none were expected." + ); + ensure!( + crate::ListBags::::iter().count() == 0, + "Items already exist where none were expected." + ); + Ok(()) + } + + fn on_runtime_upgrade() -> frame_support::weights::Weight { + migration::move_pallet(b"BagsList", b"VoterList"); + let old_nodes = migration::storage_iter::>(b"VoterList", b"ListNodes"); + + for node in old_nodes.iter() { + let score = T::ScoreProvider::score(node.id); + + let new_node = crate::Node { + id: node.id, + prev: node.prev, + next: node.next, + bag_upper: node.bag_upper, + score, + _phantom: node._phantom, + }; + + crate::ListNodes::::insert(node.id, new_node); + } + + return u64::MAX + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade() -> Result<(), &'static str> { + ensure!( + crate::ListNodes::::iter().count() > 0, + "Items do not exist where some were expected." + ); + ensure!( + crate::ListBags::::iter().count() > 0, + "Items do not exist where some were expected." + ); + Ok(()) + } +} From 5fef49cce2dc4b3d4e2a45a1ab28dc075d1d9eda Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 4 May 2022 22:46:33 +0100 Subject: [PATCH 05/14] make migration compile --- frame/bags-list/src/list/mod.rs | 12 ++++++------ frame/bags-list/src/migrations.rs | 22 +++++++++++++--------- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs index 0ba2a9c503b9d..237295d53348a 100644 --- a/frame/bags-list/src/list/mod.rs +++ b/frame/bags-list/src/list/mod.rs @@ -811,13 +811,13 @@ impl, I: 'static> Bag { #[scale_info(skip_type_params(T, I))] #[cfg_attr(feature = "std", derive(frame_support::DebugNoBound, Clone, PartialEq))] pub struct Node, I: 'static = ()> { - id: T::AccountId, - prev: Option, - next: Option, - bag_upper: T::Score, - score: T::Score, + pub(crate) id: T::AccountId, + pub(crate) prev: Option, + pub(crate) next: Option, + pub(crate) bag_upper: T::Score, + pub(crate) score: T::Score, #[codec(skip)] - _phantom: PhantomData, + pub(crate) _phantom: PhantomData, } impl, I: 'static> Node { diff --git a/frame/bags-list/src/migrations.rs b/frame/bags-list/src/migrations.rs index 7cc93945a9e38..c324047e5381e 100644 --- a/frame/bags-list/src/migrations.rs +++ b/frame/bags-list/src/migrations.rs @@ -19,8 +19,9 @@ use codec::{Decode, Encode}; use core::marker::PhantomData; -use frame_support::{ensure, storage::migration, traits::OnRuntimeUpgrade}; use frame_election_provider_support::ScoreProvider; +use frame_support::{ensure, storage::migration, traits::OnRuntimeUpgrade}; +use sp_runtime::traits::Zero; /// A struct that does not migration, but only checks that the counter prefix exists and is correct. pub struct CheckCounterPrefix, I: 'static>(sp_std::marker::PhantomData<(T, I)>); @@ -52,12 +53,12 @@ impl, I: 'static> OnRuntimeUpgrade for CheckCounterPrefix, I: 'static = ()> { - id: T::AccountId, - prev: Option, - next: Option, - bag_upper: T::Score, + pub id: T::AccountId, + pub prev: Option, + pub next: Option, + pub bag_upper: T::Score, #[codec(skip)] - _phantom: PhantomData, + pub _phantom: PhantomData, } /// A struct that migrates all bags lists to contain a score value. @@ -80,11 +81,11 @@ impl, I: 'static> OnRuntimeUpgrade for AddScore { migration::move_pallet(b"BagsList", b"VoterList"); let old_nodes = migration::storage_iter::>(b"VoterList", b"ListNodes"); - for node in old_nodes.iter() { - let score = T::ScoreProvider::score(node.id); + for (_key, node) in old_nodes.drain() { + let score = T::ScoreProvider::score(&node.id); let new_node = crate::Node { - id: node.id, + id: node.id.clone(), prev: node.prev, next: node.next, bag_upper: node.bag_upper, @@ -104,6 +105,9 @@ impl, I: 'static> OnRuntimeUpgrade for AddScore { crate::ListNodes::::iter().count() > 0, "Items do not exist where some were expected." ); + for (_id, node) in crate::ListNodes::::iter() { + ensure!(!node.score.is_zero(), "Score should be greater than zero"); + } ensure!( crate::ListBags::::iter().count() > 0, "Items do not exist where some were expected." From 3d16d731ca720d982c821bdb633299ebe8718de1 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 5 May 2022 14:00:57 +0100 Subject: [PATCH 06/14] remove old check --- bin/node/runtime/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 660ba7ab86229..76de74e40df25 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1545,7 +1545,7 @@ pub type Executive = frame_executive::Executive< frame_system::ChainContext, Runtime, AllPalletsWithSystem, - pallet_bags_list::migrations::CheckCounterPrefix, + (), >; /// MMR helper types. From dfb0ca8ed7caab9e8e65e9f62a306e132ac27efa Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 6 May 2022 15:36:55 +0100 Subject: [PATCH 07/14] remove runtime specific migration --- frame/bags-list/src/migrations.rs | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/frame/bags-list/src/migrations.rs b/frame/bags-list/src/migrations.rs index c324047e5381e..4125cf4d369eb 100644 --- a/frame/bags-list/src/migrations.rs +++ b/frame/bags-list/src/migrations.rs @@ -66,19 +66,11 @@ pub struct AddScore, I: 'static>(sp_std::marker::PhantomData impl, I: 'static> OnRuntimeUpgrade for AddScore { #[cfg(feature = "try-runtime")] fn pre_upgrade() -> Result<(), &'static str> { - ensure!( - crate::ListNodes::::iter().count() == 0, - "Items already exist where none were expected." - ); - ensure!( - crate::ListBags::::iter().count() == 0, - "Items already exist where none were expected." - ); + // Nothing that can really be checked here... Ok(()) } fn on_runtime_upgrade() -> frame_support::weights::Weight { - migration::move_pallet(b"BagsList", b"VoterList"); let old_nodes = migration::storage_iter::>(b"VoterList", b"ListNodes"); for (_key, node) in old_nodes.drain() { @@ -96,7 +88,7 @@ impl, I: 'static> OnRuntimeUpgrade for AddScore { crate::ListNodes::::insert(node.id, new_node); } - return u64::MAX + return frame_support::weights::Weight::MAX } #[cfg(feature = "try-runtime")] @@ -108,10 +100,6 @@ impl, I: 'static> OnRuntimeUpgrade for AddScore { for (_id, node) in crate::ListNodes::::iter() { ensure!(!node.score.is_zero(), "Score should be greater than zero"); } - ensure!( - crate::ListBags::::iter().count() > 0, - "Items do not exist where some were expected." - ); Ok(()) } } From ef2408f9a09b6bb1b7eb1731dbd18f88b95e10ae Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 9 May 2022 11:39:42 -0400 Subject: [PATCH 08/14] fix warning --- frame/bags-list/src/migrations.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/frame/bags-list/src/migrations.rs b/frame/bags-list/src/migrations.rs index 4125cf4d369eb..c3dce54f1b832 100644 --- a/frame/bags-list/src/migrations.rs +++ b/frame/bags-list/src/migrations.rs @@ -20,14 +20,17 @@ use codec::{Decode, Encode}; use core::marker::PhantomData; use frame_election_provider_support::ScoreProvider; -use frame_support::{ensure, storage::migration, traits::OnRuntimeUpgrade}; +use frame_support::{storage::migration, traits::OnRuntimeUpgrade}; use sp_runtime::traits::Zero; +#[cfg(feature = "try-runtime")] +use frame_support::ensure; + /// A struct that does not migration, but only checks that the counter prefix exists and is correct. pub struct CheckCounterPrefix, I: 'static>(sp_std::marker::PhantomData<(T, I)>); impl, I: 'static> OnRuntimeUpgrade for CheckCounterPrefix { fn on_runtime_upgrade() -> frame_support::weights::Weight { - 0 + frame_support::weights::Weight::zero() } #[cfg(feature = "try-runtime")] From 238f7b1a55409e2603fdd2c5879512b2ea13de58 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 9 May 2022 14:52:24 -0400 Subject: [PATCH 09/14] Apply suggestions from code review Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- frame/bags-list/src/list/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs index 4a9604f115334..d95c5de8132ea 100644 --- a/frame/bags-list/src/list/mod.rs +++ b/frame/bags-list/src/list/mod.rs @@ -388,7 +388,7 @@ impl, I: 'static> List { /// are moved into the correct bag. /// /// Returns `Some((old_idx, new_idx))` if the node moved, otherwise `None`. In both cases, the - /// node's score is written to the `score_score`. Thus, this is not a noop, even if `None`. + /// node's score is written to the `score` field. Thus, this is not a noop, even if `None`. /// /// This operation is somewhat more efficient than simply calling [`self.remove`] followed by /// [`self.insert`]. However, given large quantities of nodes to move, it may be more efficient From 8943fee7a9c572f6219c9c425097285e457f5055 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 9 May 2022 17:11:58 -0400 Subject: [PATCH 10/14] improve migration --- frame/bags-list/src/migrations.rs | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/frame/bags-list/src/migrations.rs b/frame/bags-list/src/migrations.rs index c3dce54f1b832..d0e79487c5283 100644 --- a/frame/bags-list/src/migrations.rs +++ b/frame/bags-list/src/migrations.rs @@ -20,7 +20,10 @@ use codec::{Decode, Encode}; use core::marker::PhantomData; use frame_election_provider_support::ScoreProvider; -use frame_support::{storage::migration, traits::OnRuntimeUpgrade}; +use frame_support::{ + storage::migration, + traits::{OnRuntimeUpgrade, PalletInfo}, +}; use sp_runtime::traits::Zero; #[cfg(feature = "try-runtime")] @@ -64,17 +67,22 @@ struct PreScoreNode, I: 'static = ()> { pub _phantom: PhantomData, } +const TEMP_STORAGE: &[u8] = b"upgrade_bags_list_score"; + /// A struct that migrates all bags lists to contain a score value. pub struct AddScore, I: 'static>(sp_std::marker::PhantomData<(T, I)>); impl, I: 'static> OnRuntimeUpgrade for AddScore { #[cfg(feature = "try-runtime")] fn pre_upgrade() -> Result<(), &'static str> { - // Nothing that can really be checked here... + let node_count: u32 = crate::ListNodes::::iter().count() as u32; + frame_support::storage::unhashed::put(TEMP_STORAGE, &node_count); + crate::log!(info, "number of nodes before: {:?}", node_count); Ok(()) } fn on_runtime_upgrade() -> frame_support::weights::Weight { - let old_nodes = migration::storage_iter::>(b"VoterList", b"ListNodes"); + let pallet_name = T::PalletInfo::name::().unwrap().as_bytes(); + let old_nodes = migration::storage_iter::>(pallet_name, b"ListNodes"); for (_key, node) in old_nodes.drain() { let score = T::ScoreProvider::score(&node.id); @@ -96,10 +104,11 @@ impl, I: 'static> OnRuntimeUpgrade for AddScore { #[cfg(feature = "try-runtime")] fn post_upgrade() -> Result<(), &'static str> { - ensure!( - crate::ListNodes::::iter().count() > 0, - "Items do not exist where some were expected." - ); + let node_count_before: u32 = + frame_support::storage::unhashed::get(TEMP_STORAGE).unwrap_or_default(); + let node_count_after: u32 = crate::ListNodes::::iter().count() as u32; + crate::log!(info, "number of nodes after: {:?}", node_count_after); + ensure!(node_count_after == node_count_before, "Not all nodes were migrated."); for (_id, node) in crate::ListNodes::::iter() { ensure!(!node.score.is_zero(), "Score should be greater than zero"); } From 7e863638c413062ec749db6e51476c1338825d1d Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Tue, 10 May 2022 13:12:06 -0400 Subject: [PATCH 11/14] fix --- frame/bags-list/src/migrations.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/frame/bags-list/src/migrations.rs b/frame/bags-list/src/migrations.rs index d0e79487c5283..016f5debaa691 100644 --- a/frame/bags-list/src/migrations.rs +++ b/frame/bags-list/src/migrations.rs @@ -67,6 +67,7 @@ struct PreScoreNode, I: 'static = ()> { pub _phantom: PhantomData, } +#[cfg(feature = "try-runtime")] const TEMP_STORAGE: &[u8] = b"upgrade_bags_list_score"; /// A struct that migrates all bags lists to contain a score value. @@ -81,7 +82,9 @@ impl, I: 'static> OnRuntimeUpgrade for AddScore { } fn on_runtime_upgrade() -> frame_support::weights::Weight { - let pallet_name = T::PalletInfo::name::().unwrap().as_bytes(); + let pallet_name = ::PalletInfo::name::>() + .unwrap() + .as_bytes(); let old_nodes = migration::storage_iter::>(pallet_name, b"ListNodes"); for (_key, node) in old_nodes.drain() { @@ -105,7 +108,7 @@ impl, I: 'static> OnRuntimeUpgrade for AddScore { #[cfg(feature = "try-runtime")] fn post_upgrade() -> Result<(), &'static str> { let node_count_before: u32 = - frame_support::storage::unhashed::get(TEMP_STORAGE).unwrap_or_default(); + frame_support::storage::unhashed::take(TEMP_STORAGE).unwrap_or_default(); let node_count_after: u32 = crate::ListNodes::::iter().count() as u32; crate::log!(info, "number of nodes after: {:?}", node_count_after); ensure!(node_count_after == node_count_before, "Not all nodes were migrated."); From 338cb45c8bb65dcc5f045acb47124793e7f4d6bd Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Tue, 17 May 2022 20:04:55 -0400 Subject: [PATCH 12/14] fix merge --- frame/bags-list/src/migrations.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/bags-list/src/migrations.rs b/frame/bags-list/src/migrations.rs index 253f23371c32f..847b4c484c2aa 100644 --- a/frame/bags-list/src/migrations.rs +++ b/frame/bags-list/src/migrations.rs @@ -40,11 +40,11 @@ impl, I: 'static> OnRuntimeUpgrade for CheckCounterPrefix Result<(), &'static str> { // The old explicit storage item. #[frame_support::storage_alias] - type CounterForListNodes = StorageValue, u32>; + type CounterForListNodes, I: 'static> = StorageValue, u32>; // ensure that a value exists in the counter struct. ensure!( - crate::ListNodes::::count() == CounterForListNodes::::get().unwrap(), + crate::ListNodes::::count() == CounterForListNodes::::get().unwrap(), "wrong list node counter" ); From 26651c685e17f74e2630dec21b988532a24299dc Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Tue, 17 May 2022 20:07:19 -0400 Subject: [PATCH 13/14] fmt --- frame/bags-list/src/migrations.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frame/bags-list/src/migrations.rs b/frame/bags-list/src/migrations.rs index 847b4c484c2aa..79b3698805f56 100644 --- a/frame/bags-list/src/migrations.rs +++ b/frame/bags-list/src/migrations.rs @@ -40,7 +40,8 @@ impl, I: 'static> OnRuntimeUpgrade for CheckCounterPrefix Result<(), &'static str> { // The old explicit storage item. #[frame_support::storage_alias] - type CounterForListNodes, I: 'static> = StorageValue, u32>; + type CounterForListNodes, I: 'static> = + StorageValue, u32>; // ensure that a value exists in the counter struct. ensure!( From 5a49d2b7adf939e70a22dc690fae7cacf7da02c0 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 18 May 2022 00:36:33 -0400 Subject: [PATCH 14/14] Update migrations.rs --- frame/bags-list/src/migrations.rs | 85 +++++++++++++++++++------------ 1 file changed, 52 insertions(+), 33 deletions(-) diff --git a/frame/bags-list/src/migrations.rs b/frame/bags-list/src/migrations.rs index 79b3698805f56..a77beb23bd667 100644 --- a/frame/bags-list/src/migrations.rs +++ b/frame/bags-list/src/migrations.rs @@ -20,10 +20,7 @@ use codec::{Decode, Encode}; use core::marker::PhantomData; use frame_election_provider_support::ScoreProvider; -use frame_support::{ - storage::migration, - traits::{OnRuntimeUpgrade, PalletInfo}, -}; +use frame_support::traits::OnRuntimeUpgrade; use sp_runtime::traits::Zero; #[cfg(feature = "try-runtime")] @@ -59,37 +56,55 @@ impl, I: 'static> OnRuntimeUpgrade for CheckCounterPrefix, I: 'static = ()> { - pub id: T::AccountId, - pub prev: Option, - pub next: Option, - pub bag_upper: T::Score, - #[codec(skip)] - pub _phantom: PhantomData, -} +mod old { + use super::*; + use frame_support::pallet_prelude::*; + + #[derive(Encode, Decode)] + pub struct PreScoreNode, I: 'static = ()> { + pub id: T::AccountId, + pub prev: Option, + pub next: Option, + pub bag_upper: T::Score, + #[codec(skip)] + pub _phantom: PhantomData, + } -#[cfg(feature = "try-runtime")] -const TEMP_STORAGE: &[u8] = b"upgrade_bags_list_score"; + #[frame_support::storage_alias] + pub type ListNodes, I: 'static> = StorageMap< + crate::Pallet, + Twox64Concat, + ::AccountId, + PreScoreNode, + >; + + #[frame_support::storage_alias] + pub type CounterForListNodes, I: 'static> = + StorageValue, u32, ValueQuery>; + + #[frame_support::storage_alias] + pub type TempStorage, I: 'static> = + StorageValue, u32, ValueQuery>; +} /// A struct that migrates all bags lists to contain a score value. -pub struct AddScore, I: 'static>(sp_std::marker::PhantomData<(T, I)>); +pub struct AddScore, I: 'static = ()>(sp_std::marker::PhantomData<(T, I)>); impl, I: 'static> OnRuntimeUpgrade for AddScore { #[cfg(feature = "try-runtime")] fn pre_upgrade() -> Result<(), &'static str> { - let node_count: u32 = crate::ListNodes::::iter().count() as u32; - frame_support::storage::unhashed::put(TEMP_STORAGE, &node_count); - crate::log!(info, "number of nodes before: {:?}", node_count); + // The list node data should be corrupt at this point, so this is zero. + ensure!(crate::ListNodes::::iter().count() == 0, "list node data is not corrupt"); + // We can use the helper `old::ListNode` to get the existing data. + let iter_node_count: u32 = old::ListNodes::::iter().count() as u32; + let tracked_node_count: u32 = old::CounterForListNodes::::get(); + crate::log!(info, "number of nodes before: {:?} {:?}", iter_node_count, tracked_node_count); + ensure!(iter_node_count == tracked_node_count, "Node count is wrong."); + old::TempStorage::::put(iter_node_count); Ok(()) } fn on_runtime_upgrade() -> frame_support::weights::Weight { - let pallet_name = ::PalletInfo::name::>() - .unwrap() - .as_bytes(); - let old_nodes = migration::storage_iter::>(pallet_name, b"ListNodes"); - - for (_key, node) in old_nodes.drain() { + for (_key, node) in old::ListNodes::::iter() { let score = T::ScoreProvider::score(&node.id); let new_node = crate::Node { @@ -109,14 +124,18 @@ impl, I: 'static> OnRuntimeUpgrade for AddScore { #[cfg(feature = "try-runtime")] fn post_upgrade() -> Result<(), &'static str> { - let node_count_before: u32 = - frame_support::storage::unhashed::take(TEMP_STORAGE).unwrap_or_default(); - let node_count_after: u32 = crate::ListNodes::::iter().count() as u32; - crate::log!(info, "number of nodes after: {:?}", node_count_after); - ensure!(node_count_after == node_count_before, "Not all nodes were migrated."); - for (_id, node) in crate::ListNodes::::iter() { - ensure!(!node.score.is_zero(), "Score should be greater than zero"); - } + let node_count_before = old::TempStorage::::take(); + // Now, the list node data is not corrupt anymore. + let iter_node_count_after: u32 = crate::ListNodes::::iter().count() as u32; + let tracked_node_count_after: u32 = crate::ListNodes::::count(); + crate::log!( + info, + "number of nodes after: {:?} {:?}", + iter_node_count_after, + tracked_node_count_after, + ); + ensure!(iter_node_count_after == node_count_before, "Not all nodes were migrated."); + ensure!(tracked_node_count_after == iter_node_count_after, "Node count is wrong."); Ok(()) } }