paritytech · Sep 17, 2021 · Jun 30, 2021 · Jun 30, 2021 · Jun 30, 2021 · Jun 30, 2021
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/bin/node/runtime/Cargo.toml b/bin/node/runtime/Cargo.toml
@@ -50,6 +50,7 @@ pallet-assets = { version = "4.0.0-dev", default-features = false, path = "../..
 pallet-authority-discovery = { version = "4.0.0-dev", default-features = false, path = "../../../frame/authority-discovery" }
 pallet-authorship = { version = "4.0.0-dev", default-features = false, path = "../../../frame/authorship" }
 pallet-babe = { version = "4.0.0-dev", default-features = false, path = "../../../frame/babe" }
+pallet-bags-list = { version = "4.0.0-dev", default-features = false, path = "../../../frame/bags-list" }
 pallet-balances = { version = "4.0.0-dev", default-features = false, path = "../../../frame/balances" }
 pallet-bounties = { version = "4.0.0-dev", default-features = false, path = "../../../frame/bounties" }
 pallet-collective = { version = "4.0.0-dev", default-features = false, path = "../../../frame/collective" }
@@ -106,6 +107,7 @@ std = [
 	"pallet-authorship/std",
 	"sp-consensus-babe/std",
 	"pallet-babe/std",
+	"pallet-bags-list/std",
 	"pallet-balances/std",
 	"pallet-bounties/std",
 	"sp-block-builder/std",
@@ -173,6 +175,7 @@ runtime-benchmarks = [
 	"sp-runtime/runtime-benchmarks",
 	"pallet-assets/runtime-benchmarks",
 	"pallet-babe/runtime-benchmarks",
+	"pallet-bags-list/runtime-benchmarks",
 	"pallet-balances/runtime-benchmarks",
 	"pallet-bounties/runtime-benchmarks",
 	"pallet-collective/runtime-benchmarks",

diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs
@@ -486,7 +486,6 @@ parameter_types! {
 	pub const RewardCurve: &'static PiecewiseLinear<'static> = &REWARD_CURVE;
 	pub const MaxNominatorRewardedPerValidator: u32 = 256;
 	pub OffchainRepeat: BlockNumber = 5;
-	pub const VoterBagThresholds: &'static [u64] = &voter_bags::THRESHOLDS;
 }
 
 use frame_election_provider_support::onchain;
@@ -516,8 +515,10 @@ impl pallet_staking::Config for Runtime {
 	type GenesisElectionProvider = onchain::OnChainSequentialPhragmen<
 		pallet_election_provider_multi_phase::OnChainConfig<Self>,
 	>;
+	// Alternatively, use pallet_staking::UseNominatorsMap<Runtime> to just use the nominators map.
+	// Note that this method does not scale to a very large number of nominators.
+	type SortedListProvider = BagsList;
 	type WeightInfo = pallet_staking::weights::SubstrateWeight<Runtime>;
-	type VoterBagThresholds = VoterBagThresholds;
 }
 
 parameter_types! {
@@ -607,6 +608,17 @@ impl pallet_election_provider_multi_phase::Config for Runtime {
 	type VoterSnapshotPerBlock = VoterSnapshotPerBlock;
 }
 
+parameter_types! {
+	pub const BagThresholds: &'static [u64] = &voter_bags::THRESHOLDS;
+}
+
+impl pallet_bags_list::Config for Runtime {
+	type Event = Event;
+	type VoteWeightProvider = Staking;
+	type WeightInfo = ();
+	type BagThresholds = BagThresholds;
+}
+
 parameter_types! {
 	pub const LaunchPeriod: BlockNumber = 28 * 24 * 60 * MINUTES;
 	pub const VotingPeriod: BlockNumber = 28 * 24 * 60 * MINUTES;
@@ -1231,6 +1243,7 @@ construct_runtime!(
 		Gilt: pallet_gilt::{Pallet, Call, Storage, Event<T>, Config},
 		Uniques: pallet_uniques::{Pallet, Call, Storage, Event<T>},
 		TransactionStorage: pallet_transaction_storage::{Pallet, Call, Storage, Inherent, Config<T>, Event<T>},
+		BagsList: pallet_bags_list::{Pallet, Call, Storage, Event<T>},
 	}
 );
 
@@ -1564,6 +1577,7 @@ impl_runtime_apis! {
 
 			list_benchmark!(list, extra, pallet_assets, Assets);
 			list_benchmark!(list, extra, pallet_babe, Babe);
+			list_benchmark!(list, extra, pallet_bags_list, BagsList);
 			list_benchmark!(list, extra, pallet_balances, Balances);
 			list_benchmark!(list, extra, pallet_bounties, Bounties);
 			list_benchmark!(list, extra, pallet_collective, Council);

diff --git a/frame/bags-list/Cargo.toml b/frame/bags-list/Cargo.toml
@@ -17,8 +17,6 @@ codec = { package = "parity-scale-codec", version = "2.0.0", default-features =
 
 sp-runtime = { version = "4.0.0-dev", default-features = false, path = "../../primitives/runtime" }
 sp-std = { version = "4.0.0-dev", default-features = false, path = "../../primitives/std" }
-sp-io = { version = "4.0.0-dev", default-features = false, path = "../../primitives/io" }
-sp-arithmetic = { version = "4.0.0-dev", default-features = false, path = "../../primitives/arithmetic" }
 
 frame-support = { version = "4.0.0-dev", default-features = false, path = "../support" }
 frame-system = { version = "4.0.0-dev", default-features = false, path = "../system" }
@@ -27,12 +25,18 @@ frame-election-provider-support = { version = "4.0.0-dev", default-features = fa
 log = { version = "0.4.14", default-features = false }
 
 # Optional imports for benchmarking
-frame-benchmarking = { version = "4.0.0-dev", default-features = false, path = "../benchmarking", optional = true }
+frame-benchmarking = { version = "4.0.0-dev", path = "../benchmarking", optional = true, default-features = false }
+sp-core = { version = "4.0.0-dev", path = "../../primitives/core", optional = true, default-features = false }
+sp-io = { version = "4.0.0-dev", path = "../../primitives/io", optional = true, default-features = false }
+sp-tracing = { version = "4.0.0-dev", path = "../../primitives/tracing", optional = true, default-features = false }
+pallet-balances = { version = "4.0.0-dev", path = "../balances", optional = true, default-features = false }
 
 [dev-dependencies]
 sp-core = { version = "4.0.0-dev", path = "../../primitives/core"}
+sp-io = { version = "4.0.0-dev", path = "../../primitives/io"}
 sp-tracing = { version = "4.0.0-dev", path = "../../primitives/tracing" }
 pallet-balances = { version = "4.0.0-dev", path = "../balances" }
+frame-benchmarking = { version = "4.0.0-dev", path = "../benchmarking" }
 
 [features]
 default = ["std"]
@@ -47,4 +51,8 @@ std = [
 ]
 runtime-benchmarks = [
 	"frame-benchmarking",
+	"sp-core",
+	"sp-io",
+	"pallet-balances",
+	"sp-tracing",
 ]
diff --git a/frame/bags-list/src/benchmarks.rs b/frame/bags-list/src/benchmarks.rs
@@ -28,8 +28,6 @@ frame_benchmarking::benchmarks! {
 		// - The node to be rebagged should exist as a non-terminal node in a bag with at least
 		//   2 other nodes so both its prev and next are nodes that will need be updated
 		//   when it is removed.
-		//   TODO once we optimize for not writing the bag being removed we will have the case
-		//    of removing head/tail from a bag with at least 2 nodes
 		// - The destination bag is not empty, because then we need to update the `next` pointer
 		//   of the previous node in addition to the work we do otherwise.
 
@@ -62,7 +60,10 @@ frame_benchmarking::benchmarks! {
 		// and the ags are in the expected state after insertions.
 		assert_eq!(
 			get_bags::<T>(),
-			vec![(origin_bag_thresh, vec![origin_head.clone(), origin_middle.clone(), origin_tail.clone()]), (dest_bag_thresh, vec![dest_head.clone()])]
+			vec![
+				(origin_bag_thresh, vec![origin_head.clone(), origin_middle.clone(), origin_tail.clone()]),
+				(dest_bag_thresh, vec![dest_head.clone()])
+			]
 		);
 	}: {
 		// TODO need to figure out how to mock a return value for VoteWeightProvider::vote_weight
@@ -81,19 +82,6 @@ frame_benchmarking::benchmarks! {
 			vec![(origin_bag_thresh, vec![origin_head, origin_tail]), (dest_bag_thresh, vec![dest_head, origin_middle])]
 		);
 	}
-
-	// rebag_head { TODO once we have optimized so we only write the origin bag if head tail was updated
-		// An expensive case for rebagging
-		//
-		// - the node being rebagged is a head node, so origin bag must be updated with a new head
-		//   and second node in the bag must have its prev pointer updated.
-		// - The destination bag is not empty, because then we need to update the `next` pointer
-		//   of the previous node in addition to the work we do otherwise.
-
-	// } _(Origin::signed(white_listed_caller()), origin_head)
-	// verify {
-
-	// }
 }
 
 use frame_benchmarking::impl_benchmark_test_suite;

diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs
@@ -23,6 +23,9 @@
 //! of accounts to another pallet. It needs some other pallet to give it some information about the
 //! weights of accounts via [`sp_election_provider_support::VoteWeightProvider`].
 //!
+//! This pallet is not configurable at genesis. Whoever uses it should call appropriate functions of
+//! the `SortedListProvider` (i.e. `on_insert`) at their genesis.
+//!
 //! # ⚠️ WARNING ⚠️
 //!
 //! Do not insert an id that already exists in the list; doing so can result in catastrophic failure
@@ -53,13 +56,15 @@
 //! *new terminology*: fully decouple this pallet from voting names, use account id instead of
 //! voter and priority instead of weight.
 
+#![cfg_attr(not(feature = "std"), no_std)]
+
 use frame_election_provider_support::{SortedListProvider, VoteWeight, VoteWeightProvider};
 use frame_system::ensure_signed;
+use sp_std::prelude::*;
 
-#[cfg(feature = "runtime-benchmarks")]
+#[cfg(any(feature = "runtime-benchmarks", test))]
 mod benchmarks;
 mod list;
-// #[cfg(any(feature = "runtime-benchmarks", test))]
 #[cfg(test)]
 mod mock;
 #[cfg(test)]
@@ -184,34 +189,31 @@ pub mod pallet {
 
 	#[pallet::call]
 	impl<T: Config> Pallet<T> {
-		/// Declare that some `stash` has, through rewards or penalties, sufficiently changed its
-		/// stake that it should properly fall into a different bag than its current position.
+		/// Declare that some `reposition` account has, through rewards or penalties, sufficiently
+		/// changed its weight that it should properly fall into a different bag than its current
+		/// one.
 		///
-		/// This will adjust its position into the appropriate bag. This will affect its position
-		/// among the nominator/validator set once the snapshot is prepared for the election.
+		/// Anyone can call this function about any target account.
 		///
-		/// Anyone can call this function about any stash.
-		// #[pallet::weight(T::WeightInfo::rebag())]
+		/// Will never return an error; if `reposition` does not exist or doesn't need a rebag, then
+		/// it is a noop and only fees are collected from `origin`.
 		#[pallet::weight(T::WeightInfo::rebag_middle())]
-		pub fn rebag(origin: OriginFor<T>, account: T::AccountId) -> DispatchResult {
+		pub fn rebag(origin: OriginFor<T>, reposition: T::AccountId) -> DispatchResult {
 			ensure_signed(origin)?;
-			let weight = T::VoteWeightProvider::vote_weight(&account);
-			Pallet::<T>::do_rebag(&account, weight);
+			let current_weight = T::VoteWeightProvider::vote_weight(&reposition);
+			Pallet::<T>::do_rebag(&reposition, current_weight);
 			Ok(())
 		}
 	}
 
 	#[pallet::hooks]
 	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
 		fn integrity_test() {
-			sp_std::if_std! {
-				sp_io::TestExternalities::new_empty().execute_with(|| {
-					assert!(
-						T::BagThresholds::get().windows(2).all(|window| window[1] > window[0]),
-						"Voter bag thresholds must strictly increase",
-					);
-				});
-			}
+			// ensure they are strictly increasing, this also implies that duplicates are detected.
+			assert!(
+				T::BagThresholds::get().windows(2).all(|window| window[1] > window[0]),
+				"Voter bag thresholds must strictly increase, and have no duplicates",
+			);
 		}
 	}
 }
@@ -244,10 +246,6 @@ impl<T: Config> SortedListProvider<T::AccountId> for Pallet<T> {
 		CounterForVoters::<T>::get()
 	}
 
-	/// # ⚠️ WARNING ⚠️
-	///
-	/// Do not insert an id that already exists in the list; doing so can result in catastrophic
-	/// failure of your blockchain, including entering into an infinite loop during block execution.
 	fn on_insert(voter: T::AccountId, weight: VoteWeight) {
 		List::<T>::insert(voter, weight);
 	}
@@ -260,6 +258,13 @@ impl<T: Config> SortedListProvider<T::AccountId> for Pallet<T> {
 		List::<T>::remove(voter)
 	}
 
+	fn regenerate(
+		all: impl IntoIterator<Item = T::AccountId>,
+		weight_of: Box<dyn Fn(&T::AccountId) -> VoteWeight>,
+	) -> u32 {
+		List::<T>::regenerate(all, weight_of)
+	}
+
 	fn sanity_check() -> Result<(), &'static str> {
 		List::<T>::sanity_check()
 	}

diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs
@@ -76,13 +76,13 @@ impl<T: Config> List<T> {
 	/// consensus.
 	///
 	/// Returns the number of voters migrated.
-	#[allow(dead_code)]
 	pub fn regenerate(
 		all: impl IntoIterator<Item = T::AccountId>,
 		weight_of: Box<dyn Fn(&T::AccountId) -> VoteWeight>,
-	) {
+	) -> u32 {
 		Self::clear();
 		Self::insert_many(all, weight_of);
+		0 // TODO
 	}
 
 	/// Migrate the voter list from one set of thresholds to another.
@@ -125,7 +125,7 @@ impl<T: Config> List<T> {
 			if !affected_old_bags.insert(affected_bag) {
 				// If the previous threshold list was [10, 20], and we insert [3, 5], then there's
 				// no point iterating through bag 10 twice.
-				continue;
+				continue
 			}
 
 			if let Some(bag) = Bag::<T>::get(affected_bag) {
@@ -136,7 +136,7 @@ impl<T: Config> List<T> {
 		// a removed bag means that all members of that bag must be rebagged
 		for removed_bag in old_set.difference(&new_set).copied() {
 			if !affected_old_bags.insert(removed_bag) {
-				continue;
+				continue
 			}
 
 			if let Some(bag) = Bag::<T>::get(removed_bag) {
@@ -303,14 +303,15 @@ impl<T: Config> List<T> {
 	/// [`self.insert`]. However, given large quantities of voters to move, it may be more efficient
 	/// to call [`self.remove_many`] followed by [`self.insert_many`].
 	pub(crate) fn update_position_for(
-		mut node: Node<T>,
+		node: Node<T>,
 		new_weight: VoteWeight,
 	) -> Option<(VoteWeight, VoteWeight)> {
 		node.is_misplaced(new_weight).then(move || {
-			let old_idx = node.bag_upper;
+			let old_bag_upper = node.bag_upper;
 
 			if !node.is_terminal() {
-				// this node is not a head or a tail, so we can just cut it out of the bag.
+				// this node is not a head or a tail, so we can just cut it out of the list.
+				// update and put the prev and next of this node, we do `node.put` later.
 				node.excise();
 			} else if let Some(mut bag) = Bag::<T>::get(node.bag_upper) {
 				// this is a head or tail, so the bag must be updated.
@@ -325,14 +326,18 @@ impl<T: Config> List<T> {
 				debug_assert!(false, "every node must have an extant bag associated with it");
 			}
 
-			// put the voter into the appropriate new bag
-			let new_idx = notional_bag_for::<T>(new_weight);
-			node.bag_upper = new_idx;
+			// TODO: go through all APIs, and make a standard out of when things will put and when
+			// they don't.
+
+			// put the voter into the appropriate new bag.
+			let new_bag_upper = notional_bag_for::<T>(new_weight);
 			let mut bag = Bag::<T>::get_or_make(node.bag_upper);
+			// prev, next, and bag_upper of the node are updated inside `insert_node`, also
+			// `node.put` is in there.
 			bag.insert_node(node);
 			bag.put();
 
-			(old_idx, new_idx)
+			(old_bag_upper, new_bag_upper)
 		})
 	}
 
@@ -477,7 +482,7 @@ impl<T: Config> Bag<T> {
 				// this should never happen, but this check prevents a worst case infinite loop
 				debug_assert!(false, "system logic error: inserting a node who has the id of tail");
 				crate::log!(warn, "system logic error: inserting a node who has the id of tail");
-				return;
+				return
 			};
 		}
 

diff --git a/frame/bags-list/src/mock.rs b/frame/bags-list/src/mock.rs
@@ -14,6 +14,11 @@ impl frame_election_provider_support::VoteWeightProvider<AccountId> for StakingM
 	fn vote_weight(_: &AccountId) -> VoteWeight {
 		NextVoteWeight::get()
 	}
+	#[cfg(feature = "runtime-benchmarks")]
+	fn set_vote_weight_of(_: &AccountId, weight: VoteWeight) {
+		// we don't really keep a mapping, just set weight for everyone.
+		NextVoteWeight::set(weight)
+	}
 }
 
 impl frame_system::Config for Runtime {
@@ -135,7 +140,6 @@ pub(crate) mod test_utils {
 		bag.iter().map(|n| *n.id()).collect::<Vec<_>>()
 	}
 
-	// TODO change this to get_list_as_ids
 	pub(crate) fn get_list_as_ids() -> Vec<AccountId> {
 		List::<Runtime>::iter().map(|n| *n.id()).collect::<Vec<_>>()
 	}