Allow nomination pools to chill + fix dismantle scenario (paritytech#11426)

* make pool roles optional

* undo lock file changes?

* add migration

* add the ability for pools to chill themselves

* boilerplate of tests

* somewhat stable, but I think I found another bug as well

* Fix it all

* Add more more sophisticated test + capture one more bug.

* Update frame/staking/src/lib.rs

* reduce the diff a little bit

* add some test for the slashing bug

* cleanup

* fix lock file?

* Fix

* fmt

* Update frame/nomination-pools/src/lib.rs

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Update frame/nomination-pools/src/lib.rs

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Update frame/nomination-pools/src/lib.rs

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Update frame/nomination-pools/src/mock.rs

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Fix build

* fix some fishy tests..

* add one last integrity check for MinCreateBond

* remove bad assertion -- needs to be dealt with later

* nits

* fix tests and add benchmarks for chill

* remove stuff

* fix benchmarks

* cargo run --quiet --profile=production  --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark pallet --chain=dev --steps=50 --repeat=20 --pallet=pallet_nomination_pools --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/nomination-pools/src/weights.rs --template=./.maintain/frame-weight-template.hbs

* remove defensive

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-authored-by: Parity Bot <admin@parity.io>

Author: 4 people

Cargo.lock

@@ -115,6 +115,7 @@ members = [
 	"frame/proxy",
 	"frame/nomination-pools",
 	"frame/nomination-pools/benchmarking",
+	"frame/nomination-pools/test-staking",
 	"frame/randomness-collective-flip",
 	"frame/ranked-collective",
 	"frame/recovery",

Cargo.toml

@@ -23,8 +23,9 @@ use hex_literal::hex;
 use node_runtime::{
 	constants::currency::*, wasm_binary_unwrap, AuthorityDiscoveryConfig, BabeConfig,
 	BalancesConfig, Block, CouncilConfig, DemocracyConfig, ElectionsConfig, GrandpaConfig,
-	ImOnlineConfig, IndicesConfig, MaxNominations, SessionConfig, SessionKeys, SocietyConfig,
-	StakerStatus, StakingConfig, SudoConfig, SystemConfig, TechnicalCommitteeConfig,
+	ImOnlineConfig, IndicesConfig, MaxNominations, NominationPoolsConfig, SessionConfig,
+	SessionKeys, SocietyConfig, StakerStatus, StakingConfig, SudoConfig, SystemConfig,
+	TechnicalCommitteeConfig,
 };
 use pallet_im_online::sr25519::AuthorityId as ImOnlineId;
 use sc_chain_spec::ChainSpecExtension;
@@ -365,7 +366,11 @@ pub fn testnet_genesis(
 		transaction_payment: Default::default(),
 		alliance: Default::default(),
 		alliance_motion: Default::default(),
-		nomination_pools: Default::default(),
+		nomination_pools: NominationPoolsConfig {
+			min_create_bond: 10 * DOLLARS,
+			min_join_bond: 1 * DOLLARS,
+			..Default::default()
+		},
 	}
 }
 

bin/node/cli/src/chain_spec.rs

@@ -23,11 +23,11 @@ sp-runtime = { version = "6.0.0", default-features = false, path = "../../primit
 sp-std = { version = "4.0.0", default-features = false, path = "../../primitives/std" }
 sp-staking = { version = "4.0.0-dev", default-features = false, path = "../../primitives/staking" }
 sp-core = { version = "6.0.0", default-features = false, path = "../../primitives/core" }
+sp-io = { version = "6.0.0", default-features = false, path = "../../primitives/io" }
 log = { version = "0.4.0", default-features = false }
 
 [dev-dependencies]
 pallet-balances = { version = "4.0.0-dev", path = "../balances" }
-sp-io = { version = "6.0.0", path = "../../primitives/io" }
 sp-tracing = { version = "5.0.0", path = "../../primitives/tracing" }
 
 [features]
@@ -41,6 +41,7 @@ std = [
 	"frame-system/std",
 	"sp-runtime/std",
 	"sp-std/std",
+	"sp-io/std",
 	"sp-staking/std",
 	"sp-core/std",
 	"log/std",

frame/nomination-pools/Cargo.toml

@@ -24,7 +24,7 @@ mod mock;
 
 use frame_benchmarking::{account, frame_support::traits::Currency, vec, whitelist_account, Vec};
 use frame_election_provider_support::SortedListProvider;
-use frame_support::{ensure, traits::Get};
+use frame_support::{assert_ok, ensure, traits::Get};
 use frame_system::RawOrigin as Origin;
 use pallet_nomination_pools::{
 	BalanceOf, BondExtra, BondedPoolInner, BondedPools, ConfigOp, MaxPoolMembers,
@@ -48,6 +48,12 @@ pub trait Config:
 
 pub struct Pallet<T: Config>(Pools<T>);
 
+fn min_create_bond<T: Config>() -> BalanceOf<T> {
+	MinCreateBond::<T>::get()
+		.max(T::StakingInterface::minimum_bond())
+		.max(CurrencyOf::<T>::minimum_balance())
+}
+
 fn create_funded_user_with_balance<T: pallet_nomination_pools::Config>(
 	string: &'static str,
 	n: u32,
@@ -209,9 +215,7 @@ impl<T: Config> ListScenario<T> {
 
 frame_benchmarking::benchmarks! {
 	join {
-		let origin_weight = pallet_nomination_pools::MinCreateBond::<T>::get()
-			.max(CurrencyOf::<T>::minimum_balance())
-			* 2u32.into();
+		let origin_weight = min_create_bond::<T>() * 2u32.into();
 
 		// setup the worst case list scenario.
 		let scenario = ListScenario::<T>::new(origin_weight, true)?;
@@ -237,9 +241,7 @@ frame_benchmarking::benchmarks! {
 	}
 
 	bond_extra_transfer {
-		let origin_weight = pallet_nomination_pools::MinCreateBond::<T>::get()
-			.max(CurrencyOf::<T>::minimum_balance())
-			* 2u32.into();
+		let origin_weight = min_create_bond::<T>() * 2u32.into();
 		let scenario = ListScenario::<T>::new(origin_weight, true)?;
 		let extra = scenario.dest_weight.clone() - origin_weight;
 
@@ -254,9 +256,7 @@ frame_benchmarking::benchmarks! {
 	}
 
 	bond_extra_reward {
-		let origin_weight = pallet_nomination_pools::MinCreateBond::<T>::get()
-			.max(CurrencyOf::<T>::minimum_balance())
-			* 2u32.into();
+		let origin_weight = min_create_bond::<T>() * 2u32.into();
 		let scenario = ListScenario::<T>::new(origin_weight, true)?;
 		let extra = (scenario.dest_weight.clone() - origin_weight).max(CurrencyOf::<T>::minimum_balance());
 
@@ -274,7 +274,7 @@ frame_benchmarking::benchmarks! {
 	}
 
 	claim_payout {
-		let origin_weight = pallet_nomination_pools::MinCreateBond::<T>::get().max(CurrencyOf::<T>::minimum_balance()) * 2u32.into();
+		let origin_weight = min_create_bond::<T>() * 2u32.into();
 		let ed = CurrencyOf::<T>::minimum_balance();
 		let (depositor, pool_account) = create_pool_account::<T>(0, origin_weight);
 		let reward_account = Pools::<T>::create_reward_account(1);
@@ -304,9 +304,7 @@ frame_benchmarking::benchmarks! {
 	unbond {
 		// The weight the nominator will start at. The value used here is expected to be
 		// significantly higher than the first position in a list (e.g. the first bag threshold).
-		let origin_weight = BalanceOf::<T>::try_from(952_994_955_240_703u128)
-			.map_err(|_| "balance expected to be a u128")
-			.unwrap();
+		let origin_weight = min_create_bond::<T>() * 200u32.into();
 		let scenario = ListScenario::<T>::new(origin_weight, false)?;
 		let amount = origin_weight - scenario.dest_weight.clone();
 
@@ -336,9 +334,7 @@ frame_benchmarking::benchmarks! {
 	pool_withdraw_unbonded {
 		let s in 0 .. MAX_SPANS;
 
-		let min_create_bond = MinCreateBond::<T>::get()
-			.max(T::StakingInterface::minimum_bond())
-			.max(CurrencyOf::<T>::minimum_balance());
+		let min_create_bond = min_create_bond::<T>();
 		let (depositor, pool_account) = create_pool_account::<T>(0, min_create_bond);
 
 		// Add a new member
@@ -380,9 +376,7 @@ frame_benchmarking::benchmarks! {
 	withdraw_unbonded_update {
 		let s in 0 .. MAX_SPANS;
 
-		let min_create_bond = MinCreateBond::<T>::get()
-			.max(T::StakingInterface::minimum_bond())
-			.max(CurrencyOf::<T>::minimum_balance());
+		let min_create_bond = min_create_bond::<T>();
 		let (depositor, pool_account) = create_pool_account::<T>(0, min_create_bond);
 
 		// Add a new member
@@ -427,10 +421,7 @@ frame_benchmarking::benchmarks! {
 	withdraw_unbonded_kill {
 		let s in 0 .. MAX_SPANS;
 
-		let min_create_bond = MinCreateBond::<T>::get()
-			.max(T::StakingInterface::minimum_bond())
-			.max(CurrencyOf::<T>::minimum_balance());
-
+		let min_create_bond = min_create_bond::<T>();
 		let (depositor, pool_account) = create_pool_account::<T>(0, min_create_bond);
 
 		// We set the pool to the destroying state so the depositor can leave
@@ -494,9 +485,7 @@ frame_benchmarking::benchmarks! {
 	}
 
 	create {
-		let min_create_bond = MinCreateBond::<T>::get()
-			.max(T::StakingInterface::minimum_bond())
-			.max(CurrencyOf::<T>::minimum_balance());
+		let min_create_bond = min_create_bond::<T>();
 		let depositor: T::AccountId = account("depositor", USER_SEED, 0);
 
 		// Give the depositor some balance to bond
@@ -542,9 +531,7 @@ frame_benchmarking::benchmarks! {
 		let n in 1 .. T::MaxNominations::get();
 
 		// Create a pool
-		let min_create_bond = MinCreateBond::<T>::get()
-			.max(T::StakingInterface::minimum_bond())
-			.max(CurrencyOf::<T>::minimum_balance());
+		let min_create_bond = min_create_bond::<T>() * 2u32.into();
 		let (depositor, pool_account) = create_pool_account::<T>(0, min_create_bond);
 
 		// Create some accounts to nominate. For the sake of benchmarking they don't need to be
@@ -581,9 +568,7 @@ frame_benchmarking::benchmarks! {
 
 	set_state {
 		// Create a pool
-		let min_create_bond = MinCreateBond::<T>::get()
-			.max(T::StakingInterface::minimum_bond())
-			.max(CurrencyOf::<T>::minimum_balance());
+		let min_create_bond = min_create_bond::<T>();
 		let (depositor, pool_account) = create_pool_account::<T>(0, min_create_bond);
 		BondedPools::<T>::mutate(&1, |maybe_pool| {
 			// Force the pool into an invalid state
@@ -601,10 +586,7 @@ frame_benchmarking::benchmarks! {
 		let n in 1 .. <T as pallet_nomination_pools::Config>::MaxMetadataLen::get();
 
 		// Create a pool
-		let min_create_bond = MinCreateBond::<T>::get()
-			.max(T::StakingInterface::minimum_bond())
-			.max(CurrencyOf::<T>::minimum_balance());
-		let (depositor, pool_account) = create_pool_account::<T>(0, min_create_bond);
+		let (depositor, pool_account) = create_pool_account::<T>(0, min_create_bond::<T>() * 2u32.into());
 
 		// Create metadata of the max possible size
 		let metadata: Vec<u8> = (0..n).map(|_| 42).collect();
@@ -633,7 +615,7 @@ frame_benchmarking::benchmarks! {
 
 	update_roles {
 		let first_id = pallet_nomination_pools::LastPoolId::<T>::get() + 1;
-		let (root, _) = create_pool_account::<T>(0, CurrencyOf::<T>::minimum_balance() * 2u32.into());
+		let (root, _) = create_pool_account::<T>(0, min_create_bond::<T>() * 2u32.into());
 		let random: T::AccountId = account("but is anything really random in computers..?", 0, USER_SEED);
 	}:_(
 		Origin::Signed(root.clone()),
@@ -653,6 +635,24 @@ frame_benchmarking::benchmarks! {
 		)
 	}
 
+	chill {
+		// Create a pool
+		let (depositor, pool_account) = create_pool_account::<T>(0, min_create_bond::<T>() * 2u32.into());
+
+		// Nominate with the pool.
+		 let validators: Vec<_> = (0..T::MaxNominations::get())
+			.map(|i| account("stash", USER_SEED, i))
+			.collect();
+
+		assert_ok!(Pools::<T>::nominate(Origin::Signed(depositor.clone()).into(), 1, validators));
+		assert!(T::StakingInterface::nominations(Pools::<T>::create_bonded_account(1)).is_some());
+
+		whitelist_account!(depositor);
+	}:_(Origin::Signed(depositor.clone()), 1)
+	verify {
+		assert!(T::StakingInterface::nominations(Pools::<T>::create_bonded_account(1)).is_none());
+	}
+
 	impl_benchmark_test_suite!(
 		Pallet,
 		crate::mock::new_test_ext(),