Fix semantics of ExistenceRequirement::KeepAlive. (#3796)

* Fix semantics of ExistenceRequirement::KeepAlive.

* Bump runtime version

substrate/node/runtime/src/lib.rs

@@ -84,7 +84,7 @@ 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: 174,
+	spec_version: 175,
 	impl_version: 175,
 	apis: RUNTIME_API_VERSIONS,
 };

substrate/srml/balances/src/lib.rs

@@ -947,8 +947,15 @@ where
 		reason: WithdrawReason,
 		liveness: ExistenceRequirement,
 	) -> result::Result<Self::NegativeImbalance, &'static str> {
-		if let Some(new_balance) = Self::free_balance(who).checked_sub(&value) {
-			if liveness == ExistenceRequirement::KeepAlive && new_balance < T::ExistentialDeposit::get() {
+		let old_balance = Self::free_balance(who);
+		if let Some(new_balance) = old_balance.checked_sub(&value) {
+			// if we need to keep the account alive...
+			if liveness == ExistenceRequirement::KeepAlive
+				// ...and it would be dead afterwards...
+				&& new_balance < T::ExistentialDeposit::get()
+				// ...yet is was alive before
+				&& old_balance >= T::ExistentialDeposit::get()
+			{
 				return Err("payment would kill account")
 			}
 			Self::ensure_can_withdraw(who, value, reason, new_balance)?;

substrate/srml/balances/src/tests.rs

@@ -25,6 +25,7 @@ use support::{
 	traits::{LockableCurrency, LockIdentifier, WithdrawReason, WithdrawReasons,
 	Currency, ReservableCurrency}
 };
+use sr_primitives::weights::DispatchClass;
 use system::RawOrigin;
 
 const ID_1: LockIdentifier = *b"1       ";
@@ -783,6 +784,41 @@ fn signed_extension_take_fees_is_bounded() {
 		});
 }
 
+#[test]
+fn signed_extension_allows_free_transactions() {
+	ExtBuilder::default()
+		.transaction_fees(100, 1, 1)
+		.monied(false)
+		.build()
+		.execute_with(|| {
+			// 1 ain't have a penny.
+			assert_eq!(Balances::free_balance(&1), 0);
+
+			// like a FreeOperational
+			let operational_transaction = DispatchInfo {
+				weight: 0,
+				class: DispatchClass::Operational
+			};
+			let len = 100;
+			assert!(
+				TakeFees::<Runtime>::from(0)
+					.validate(&1, CALL, operational_transaction , len)
+					.is_ok()
+			);
+
+			// like a FreeNormal
+			let free_transaction = DispatchInfo {
+				weight: 0,
+				class: DispatchClass::Normal
+			};
+			assert!(
+				TakeFees::<Runtime>::from(0)
+					.validate(&1, CALL, free_transaction , len)
+					.is_err()
+			);
+		});
+}
+
 #[test]
 fn burn_must_work() {
 	ExtBuilder::default().monied(true).build().execute_with(|| {

substrate/srml/support/src/traits.rs

@@ -156,6 +156,9 @@ impl<Imbalance: Drop> OnUnbalanced<Imbalance> for () {
 #[derive(Copy, Clone, Eq, PartialEq)]
 pub enum ExistenceRequirement {
 	/// Operation must not result in the account going out of existence.
+	///
+	/// Note this implies that if the account never existed in the first place, then the operation
+	/// may legitimately leave the account unchanged and still non-existent.
 	KeepAlive,
 	/// Operation may result in account going out of existence.
 	AllowDeath,