diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 40316cee9b2b7..4795b12879f82 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1570,7 +1570,7 @@ pub type Executive = frame_executive::Executive< frame_system::ChainContext, Runtime, AllPalletsWithSystem, - pallet_bags_list::migrations::CheckCounterPrefix, + (), >; /// MMR helper types. 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 2e65c3be25b24..d95c5de8132ea 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` 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 /// 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)] @@ -809,12 +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, + 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 { @@ -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 40a174b35a5d3..9bdd54289fd88 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")); }); @@ -397,6 +395,7 @@ mod list { prev: None, next: None, bag_upper: 15, + score: 15, _phantom: PhantomData, }; let node_11_no_bag = Node:: { @@ -404,6 +403,7 @@ mod list { prev: None, next: None, bag_upper: 15, + score: 15, _phantom: PhantomData, }; @@ -435,6 +435,7 @@ mod list { prev: Some(1), next: Some(2), bag_upper: 1_000, + score: 1_000, _phantom: PhantomData, }; assert!(!crate::ListNodes::::contains_key(42)); @@ -464,6 +465,7 @@ mod list { prev: Some(4), next: None, bag_upper: 1_000, + score: 1_000, _phantom: PhantomData, }; assert!(!crate::ListNodes::::contains_key(42)); @@ -493,6 +495,7 @@ mod list { prev: None, next: Some(2), bag_upper: 1_000, + score: 1_000, _phantom: PhantomData, }; assert!(!crate::ListNodes::::contains_key(42)); @@ -522,6 +525,7 @@ mod list { prev: Some(42), next: Some(42), bag_upper: 1_000, + score: 1_000, _phantom: PhantomData, }; assert!(!crate::ListNodes::::contains_key(42)); @@ -586,6 +590,7 @@ mod bags { prev: None, next: None, bag_upper, + score: bag_upper, _phantom: PhantomData, }; @@ -596,7 +601,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 + } ); }); } @@ -609,6 +621,7 @@ mod bags { prev: None, next: None, bag_upper, + score: bag_upper, _phantom: PhantomData, }; @@ -636,6 +649,7 @@ mod bags { prev: Some(21), next: Some(101), bag_upper: 20, + score: 20, _phantom: PhantomData, }; bag_20.insert_node_unchecked(node_61); @@ -649,6 +663,7 @@ mod bags { prev: Some(62), next: None, bag_upper: 20, + score: 20, _phantom: PhantomData, } ); @@ -665,13 +680,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. @@ -684,7 +692,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 + } ); }); @@ -720,7 +735,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 @@ -739,14 +761,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(); @@ -877,6 +891,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 f8041327f10be..a77beb23bd667 100644 --- a/frame/bags-list/src/migrations.rs +++ b/frame/bags-list/src/migrations.rs @@ -17,34 +17,125 @@ //! The migrations of this pallet. +use codec::{Decode, Encode}; +use core::marker::PhantomData; +use frame_election_provider_support::ScoreProvider; use frame_support::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(sp_std::marker::PhantomData); -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 + frame_support::weights::Weight::zero() } #[cfg(feature = "try-runtime")] fn pre_upgrade() -> Result<(), &'static str> { - use frame_support::ensure; // 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" ); crate::log!( info, "checked bags-list prefix to be correct and have {} nodes", - crate::ListNodes::::count() + crate::ListNodes::::count() ); Ok(()) } } + +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, + } + + #[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)>); +impl, I: 'static> OnRuntimeUpgrade for AddScore { + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result<(), &'static str> { + // 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 { + for (_key, node) in old::ListNodes::::iter() { + let score = T::ScoreProvider::score(&node.id); + + let new_node = crate::Node { + id: node.id.clone(), + prev: node.prev, + next: node.next, + bag_upper: node.bag_upper, + score, + _phantom: node._phantom, + }; + + crate::ListNodes::::insert(node.id, new_node); + } + + return frame_support::weights::Weight::MAX + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade() -> Result<(), &'static str> { + 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(()) + } +} 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(()) }