srml-staking: Force minimum bond amount + proper cleanup. (#3048)

* enforce minimum bond amount.

* Bump.

* Improve doc.

* Update srml/staking/src/lib.rs

Co-Authored-By: Shawn Tabrizi <shawntabrizi@gmail.com>

* Update srml/staking/src/lib.rs

Co-Authored-By: Gavin Wood <gavin@parity.io>

* Doc update.

substrate/node/runtime/src/lib.rs

@@ -69,8 +69,8 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	// and set impl_version to equal spec_version. If only runtime
 	// implementation changes and behavior does not, then leave spec_version as
 	// is and increment impl_version.
-	spec_version: 108,
+	spec_version: 109,
-	impl_version: 108,
+	impl_version: 109,
 	apis: RUNTIME_API_VERSIONS,
 };
 

substrate/srml/staking/src/lib.rs

@@ -653,6 +653,8 @@ decl_module! {
 		/// Take the origin account as a stash and lock up `value` of its balance. `controller` will
 		/// be the account that controls it.
 		///
+		/// `value` must be more than the `existential_deposit` defined in the Balances module.
+		///
 		/// The dispatch origin for this call must be _Signed_ by the stash account.
 		///
 		/// # <weight>
@@ -662,10 +664,6 @@ decl_module! {
 		///
 		/// NOTE: Two of the storage writes (`Self::bonded`, `Self::payee`) are _never_ cleaned unless
 		/// the `origin` falls below _existential deposit_ and gets removed as dust.
-		///
-		/// NOTE: At the moment, there are no financial restrictions to bond
-		/// (which creates a bunch of storage items for an account). In essence, nothing prevents many accounts from
-		/// spamming `Staking` storage by bonding 1 UNIT. See test case: `bond_with_no_staked_value`.
 		/// # </weight>
 		fn bond(origin,
 			controller: <T::Lookup as StaticLookup>::Source,
@@ -684,6 +682,11 @@ decl_module! {
 				return Err("controller already paired")
 			}
 
+			// reject a bond which is considered to be _dust_.
+			if value < T::Currency::minimum_balance() {
+				return Err("can not bond with value less than minimum balance")
+			}
+
 			// You're auto-bonded forever, here. We might improve this by only bonding when
 			// you actually validate/nominate and remove once you unbond __everything__.
 			<Bonded<T>>::insert(&stash, controller.clone());
@@ -699,6 +702,8 @@ decl_module! {
 		/// for staking.
 		///
 		/// Use this if there are additional funds in your stash account that you wish to bond.
+		/// Unlike [`bond`] or [`unbond`] this function does not impose any limitation on the amount
+		/// that can be added.
 		///
 		/// The dispatch origin for this call must be _Signed_ by the stash, not the controller.
 		///
@@ -781,9 +786,9 @@ decl_module! {
 		/// See also [`Call::unbond`].
 		///
 		/// # <weight>
-		/// - Could be dependent on the `origin` argument and how much `unlocking` chunks exist. It implies
+		/// - Could be dependent on the `origin` argument and how much `unlocking` chunks exist.
-		///   `consolidate_unlocked` which loops over `Ledger.unlocking`, which is indirectly
+		///  It implies `consolidate_unlocked` which loops over `Ledger.unlocking`, which is
-		///   user-controlled. See [`unbond`] for more detail.
+		///  indirectly user-controlled. See [`unbond`] for more detail.
 		/// - Contains a limited number of reads, yet the size of which could be large based on `ledger`.
 		/// - Writes are limited to the `origin` account key.
 		/// # </weight>
@@ -791,7 +796,20 @@ decl_module! {
 			let controller = ensure_signed(origin)?;
 			let ledger = Self::ledger(&controller).ok_or("not a controller")?;
 			let ledger = ledger.consolidate_unlocked(Self::current_era());
-			Self::update_ledger(&controller, &ledger);
+
+			if ledger.unlocking.is_empty() && ledger.active.is_zero() {
+				// This account must have called `unbond()` with some value that caused the active
+				// portion to fall below existential deposit + will have no more unlocking chunks
+				// left. We can now safely remove this.
+				let stash = ledger.stash;
+				// remove the lock.
+				T::Currency::remove_lock(STAKING_ID, &stash);
+				// remove all staking-related information.
+				Self::kill_stash(&stash);
+			} else {
+				// This was the consequence of a partial unbond. just update the ledger and move on.
+				Self::update_ledger(&controller, &ledger);
+			}
 		}
 
 		/// Declare the desire to validate for the origin controller.
@@ -1241,6 +1259,21 @@ impl<T: Trait> Module<T> {
 		ForceNewEra::put(true);
 	}
 
+	/// Remove all associated data of a stash account from the staking system.
+	///
+	/// This is called :
+	/// - Immediately when an account's balance falls below existential deposit.
+	/// - after a `withdraw_unbond()` call that frees all of a stash's bonded balance.
+	fn kill_stash(stash: &T::AccountId) {
+		if let Some(controller) = <Bonded<T>>::take(stash) {
+			<Ledger<T>>::remove(&controller);
+		}
+		<Payee<T>>::remove(stash);
+		<SlashCount<T>>::remove(stash);
+		<Validators<T>>::remove(stash);
+		<Nominators<T>>::remove(stash);
+	}
+
 	/// Call when a validator is determined to be offline. `count` is the
 	/// number of offenses the validator has committed.
 	///
@@ -1315,13 +1348,7 @@ impl<T: Trait> OnSessionEnding<T::AccountId, Exposure<T::AccountId, BalanceOf<T>
 
 impl<T: Trait> OnFreeBalanceZero<T::AccountId> for Module<T> {
 	fn on_free_balance_zero(stash: &T::AccountId) {
-		if let Some(controller) = <Bonded<T>>::take(stash) {
+		Self::kill_stash(stash);
-			<Ledger<T>>::remove(&controller);
-		}
-		<Payee<T>>::remove(stash);
-		<SlashCount<T>>::remove(stash);
-		<Validators<T>>::remove(stash);
-		<Nominators<T>>::remove(stash);
 	}
 }
 

substrate/srml/staking/src/tests.rs

@@ -1714,59 +1714,46 @@ fn bond_with_no_staked_value() {
 	// Particularly when she votes and the candidate is elected.
 	with_externalities(&mut ExtBuilder::default()
 	.validator_count(3)
+	.existential_deposit(5)
 	.nominate(false)
 	.minimum_validator_count(1)
 	.build(), || {
-		// setup
+		// Can't bond with 1
-		assert_ok!(Staking::set_payee(Origin::signed(10), RewardDestination::Controller));
+		assert_noop!(
-		let _ = Balances::deposit_creating(&3, 1000);
+			Staking::bond(Origin::signed(1), 2, 1, RewardDestination::Controller),
-		let initial_balance_2 = Balances::free_balance(&2);
+			"can not bond with value less than minimum balance"
-		let initial_balance_4 = Balances::free_balance(&4);
+		);
+		// bonded with absolute minimum value possible.
+		assert_ok!(Staking::bond(Origin::signed(1), 2, 5, RewardDestination::Controller));
+		assert_eq!(Balances::locks(&1)[0].amount, 5);
 
-		// Stingy validator.
+		// unbonding even 1 will cause all to be unbonded.
-		assert_ok!(Staking::bond(Origin::signed(1), 2, 1, RewardDestination::Controller));
+		assert_ok!(Staking::unbond(Origin::signed(2), 1));
-		assert_ok!(Staking::validate(Origin::signed(2), ValidatorPrefs::default()));
+		assert_eq!(
+			Staking::ledger(2),
+			Some(StakingLedger {
+				stash: 1,
+				active: 0,
+				total: 5,
+				unlocking: vec![UnlockChunk {value: 5, era: 3}]
+			})
+		);
 
 		start_era(1);
-
-		assert_eq_uvec!(validator_controllers(), vec![30, 20, 10]);
-
-		// min of 10, 20 and 30 (30 got a payout into staking so it raised it from 1 to 31).
-		assert_eq!(Staking::slot_stake(), 31);
-
-		// make the stingy one elected.
-		assert_ok!(Staking::bond(Origin::signed(3), 4, 500, RewardDestination::Controller));
-		assert_ok!(Staking::nominate(Origin::signed(4), vec![1]));
-
-		// no rewards paid to 2 and 4 yet
-		assert_eq!(Balances::free_balance(&2), initial_balance_2);
-		assert_eq!(Balances::free_balance(&4), initial_balance_4);
-
 		start_era(2);
 
-		// Stingy one is selected
+		// not yet removed.
-		assert_eq_uvec!(validator_controllers(), vec![20, 10, 2]);
+		assert_ok!(Staking::withdraw_unbonded(Origin::signed(2)));
-		assert_eq!(Staking::stakers(1), Exposure {
+		assert!(Staking::ledger(2).is_some());
-			own: 1,
+		assert_eq!(Balances::locks(&1)[0].amount, 5);
-			total: 501,
-			others: vec![IndividualExposure { who: 3, value: 500}],
-		});
-		// New slot stake.
-		assert_eq!(Staking::slot_stake(), 501);
-
-		// no rewards paid to 2 and 4 yet
-		assert_eq!(Balances::free_balance(&2), initial_balance_2);
-		assert_eq!(Balances::free_balance(&4), initial_balance_4);
 
 		start_era(3);
 
-		// Approximation resulting from Perbill conversion
+		// poof. Account 1 is removed from the staking system.
-		let approximation = 1;
+		assert_ok!(Staking::withdraw_unbonded(Origin::signed(2)));
-		let reward = Staking::current_session_reward() * 3;
+		assert!(Staking::ledger(2).is_none());
-		// 2 will not get a reward of only 1
+		assert_eq!(Balances::locks(&1).len(), 0);
-		// 4 will get the rest
+
-		assert_eq!(Balances::free_balance(&2), initial_balance_2 + 3 - approximation);
-		assert_eq!(Balances::free_balance(&4), initial_balance_4 + reward - 3 - approximation);
 	});
 }