Stop Balances pallet erroneously double incrementing and decrementi…

…ng consumers (#1976)

Closes #1970

Follow up issue to tackle, once the erroneous double
incrementing/decrementing has stopped:
#2037

substrate/frame/balances/src/lib.rs

@@ -935,8 +935,8 @@ pub mod pallet {
 				if did_provide && !does_provide {
 					// This could reap the account so must go last.
 					frame_system::Pallet::<T>::dec_providers(who).map_err(|r| {
+						// best-effort revert consumer change.
 						if did_consume && !does_consume {
-							// best-effort revert consumer change.
 							let _ = frame_system::Pallet::<T>::inc_consumers(who).defensive();
 						}
 						if !did_consume && does_consume {
@@ -1006,8 +1006,8 @@ pub mod pallet {
 			let freezes = Freezes::<T, I>::get(who);
 			let mut prev_frozen = Zero::zero();
 			let mut after_frozen = Zero::zero();
-			// TODO: Revisit this assumption. We no manipulate consumer/provider refs.
 			// No way this can fail since we do not alter the existential balances.
+			// TODO: Revisit this assumption.
 			let res = Self::mutate_account(who, |b| {
 				prev_frozen = b.frozen;
 				b.frozen = Zero::zero();
@@ -1024,26 +1024,9 @@ pub mod pallet {
 				debug_assert!(maybe_dust.is_none(), "Not altering main balance; qed");
 			}
 
-			let existed = Locks::<T, I>::contains_key(who);
-			if locks.is_empty() {
-				Locks::<T, I>::remove(who);
-				if existed {
-					// TODO: use Locks::<T, I>::hashed_key
-					// https://github.com/paritytech/substrate/issues/4969
-					system::Pallet::<T>::dec_consumers(who);
-				}
-			} else {
-				Locks::<T, I>::insert(who, bounded_locks);
-				if !existed && system::Pallet::<T>::inc_consumers_without_limit(who).is_err() {
-					// No providers for the locks. This is impossible under normal circumstances
-					// since the funds that are under the lock will themselves be stored in the
-					// account and therefore will need a reference.
-					log::warn!(
-						target: LOG_TARGET,
-						"Warning: Attempt to introduce lock consumer reference, yet no providers. \
-						This is unexpected but should be safe."
-					);
-				}
+			match locks.is_empty() {
+				true => Locks::<T, I>::remove(who),
+				false => Locks::<T, I>::insert(who, bounded_locks),
 			}
 
 			if prev_frozen > after_frozen {

substrate/frame/balances/src/tests/currency_tests.rs

@@ -144,7 +144,9 @@ fn lock_removal_should_work() {
 		.monied(true)
 		.build_and_execute_with(|| {
 			Balances::set_lock(ID_1, &1, u64::MAX, WithdrawReasons::all());
+			assert_eq!(System::consumers(&1), 1);
 			Balances::remove_lock(ID_1, &1);
+			assert_eq!(System::consumers(&1), 0);
 			assert_ok!(<Balances as Currency<_>>::transfer(&1, &2, 1, AllowDeath));
 		});
 }
@@ -156,7 +158,9 @@ fn lock_replacement_should_work() {
 		.monied(true)
 		.build_and_execute_with(|| {
 			Balances::set_lock(ID_1, &1, u64::MAX, WithdrawReasons::all());
+			assert_eq!(System::consumers(&1), 1);
 			Balances::set_lock(ID_1, &1, 5, WithdrawReasons::all());
+			assert_eq!(System::consumers(&1), 1);
 			assert_ok!(<Balances as Currency<_>>::transfer(&1, &2, 1, AllowDeath));
 		});
 }
@@ -168,7 +172,9 @@ fn double_locking_should_work() {
 		.monied(true)
 		.build_and_execute_with(|| {
 			Balances::set_lock(ID_1, &1, 5, WithdrawReasons::all());
+			assert_eq!(System::consumers(&1), 1);
 			Balances::set_lock(ID_2, &1, 5, WithdrawReasons::all());
+			assert_eq!(System::consumers(&1), 1);
 			assert_ok!(<Balances as Currency<_>>::transfer(&1, &2, 1, AllowDeath));
 		});
 }
@@ -179,8 +185,11 @@ fn combination_locking_should_work() {
 		.existential_deposit(1)
 		.monied(true)
 		.build_and_execute_with(|| {
+			assert_eq!(System::consumers(&1), 0);
 			Balances::set_lock(ID_1, &1, u64::MAX, WithdrawReasons::empty());
+			assert_eq!(System::consumers(&1), 0);
 			Balances::set_lock(ID_2, &1, 0, WithdrawReasons::all());
+			assert_eq!(System::consumers(&1), 0);
 			assert_ok!(<Balances as Currency<_>>::transfer(&1, &2, 1, AllowDeath));
 		});
 }
@@ -192,16 +201,19 @@ fn lock_value_extension_should_work() {
 		.monied(true)
 		.build_and_execute_with(|| {
 			Balances::set_lock(ID_1, &1, 5, WithdrawReasons::all());
+			assert_eq!(System::consumers(&1), 1);
 			assert_noop!(
 				<Balances as Currency<_>>::transfer(&1, &2, 6, AllowDeath),
 				TokenError::Frozen
 			);
 			Balances::extend_lock(ID_1, &1, 2, WithdrawReasons::all());
+			assert_eq!(System::consumers(&1), 1);
 			assert_noop!(
 				<Balances as Currency<_>>::transfer(&1, &2, 6, AllowDeath),
 				TokenError::Frozen
 			);
 			Balances::extend_lock(ID_1, &1, 8, WithdrawReasons::all());
+			assert_eq!(System::consumers(&1), 1);
 			assert_noop!(
 				<Balances as Currency<_>>::transfer(&1, &2, 3, AllowDeath),
 				TokenError::Frozen
@@ -1324,9 +1336,14 @@ fn freezing_and_locking_should_work() {
 		.existential_deposit(1)
 		.monied(true)
 		.build_and_execute_with(|| {
+			// Consumer is shared between freezing and locking.
+			assert_eq!(System::consumers(&1), 0);
 			assert_ok!(<Balances as fungible::MutateFreeze<_>>::set_freeze(&TestId::Foo, &1, 4));
+			assert_eq!(System::consumers(&1), 1);
 			Balances::set_lock(ID_1, &1, 5, WithdrawReasons::all());
-			assert_eq!(System::consumers(&1), 2);
+			assert_eq!(System::consumers(&1), 1);
+
+			// Frozen and locked balances update correctly.
 			assert_eq!(Balances::account(&1).frozen, 5);
 			assert_ok!(<Balances as fungible::MutateFreeze<_>>::set_freeze(&TestId::Foo, &1, 6));
 			assert_eq!(Balances::account(&1).frozen, 6);
@@ -1336,8 +1353,13 @@ fn freezing_and_locking_should_work() {
 			assert_eq!(Balances::account(&1).frozen, 4);
 			Balances::set_lock(ID_1, &1, 5, WithdrawReasons::all());
 			assert_eq!(Balances::account(&1).frozen, 5);
+
+			// Locks update correctly.
 			Balances::remove_lock(ID_1, &1);
 			assert_eq!(Balances::account(&1).frozen, 4);
 			assert_eq!(System::consumers(&1), 1);
+			assert_ok!(<Balances as fungible::MutateFreeze<_>>::set_freeze(&TestId::Foo, &1, 0));
+			assert_eq!(Balances::account(&1).frozen, 0);
+			assert_eq!(System::consumers(&1), 0);
 		});
 }