Introduces account existence providers reference counting

frame/balances/src/lib.rs

@@ -1359,12 +1359,3 @@ where
 		Self::update_locks(who, &locks[..]);
 	}
 }
-
-impl<T: Trait<I>, I: Instance> IsDeadAccount<T::AccountId> for Module<T, I> where
-	T::Balance: MaybeSerializeDeserialize + Debug
-{
-	fn is_dead_account(who: &T::AccountId) -> bool {
-		// this should always be exactly equivalent to `Self::account(who).total().is_zero()` if ExistentialDeposit > 0
-		!T::AccountStore::is_explicit(who)
-	}
-}

frame/balances/src/tests.rs

@@ -91,7 +91,8 @@ macro_rules! decl_tests {
 			<$ext_builder>::default().existential_deposit(1).monied(true).build().execute_with(|| {
 				assert_eq!(Balances::free_balance(1), 10);
 				assert_ok!(<Balances as Currency<_>>::transfer(&1, &2, 10, AllowDeath));
-				assert!(!<<Test as Trait>::AccountStore as StoredMap<u64, AccountData<u64>>>::is_explicit(&1));
+				// Check that the account is dead.
+				assert!(!frame_system::Account::<T>::contains_key(&1));
 			});
 		}
 
@@ -262,14 +263,12 @@ macro_rules! decl_tests {
 				.monied(true)
 				.build()
 				.execute_with(|| {
-					assert_eq!(Balances::is_dead_account(&5), true);
 					// account 5 should not exist
 					// ext_deposit is 10, value is 9, not satisfies for ext_deposit
 					assert_noop!(
 						Balances::transfer(Some(1).into(), 5, 9),
 						Error::<$test, _>::ExistentialDeposit,
 					);
-					assert_eq!(Balances::is_dead_account(&5), true); // account 5 should not exist
 					assert_eq!(Balances::free_balance(1), 100);
 				});
 		}
@@ -282,31 +281,25 @@ macro_rules! decl_tests {
 				.build()
 				.execute_with(|| {
 					System::inc_account_nonce(&2);
-					assert_eq!(Balances::is_dead_account(&2), false);
-					assert_eq!(Balances::is_dead_account(&5), true);
 					assert_eq!(Balances::total_balance(&2), 256 * 20);
 
 					assert_ok!(Balances::reserve(&2, 256 * 19 + 1)); // account 2 becomes mostly reserved
 					assert_eq!(Balances::free_balance(2), 255); // "free" account deleted."
 					assert_eq!(Balances::total_balance(&2), 256 * 20); // reserve still exists.
-					assert_eq!(Balances::is_dead_account(&2), false);
 					assert_eq!(System::account_nonce(&2), 1);
 
 					// account 4 tries to take index 1 for account 5.
 					assert_ok!(Balances::transfer(Some(4).into(), 5, 256 * 1 + 0x69));
 					assert_eq!(Balances::total_balance(&5), 256 * 1 + 0x69);
-					assert_eq!(Balances::is_dead_account(&5), false);
 
 					assert!(Balances::slash(&2, 256 * 19 + 2).1.is_zero()); // account 2 gets slashed
 					// "reserve" account reduced to 255 (below ED) so account deleted
 					assert_eq!(Balances::total_balance(&2), 0);
 					assert_eq!(System::account_nonce(&2), 0);    // nonce zero
-					assert_eq!(Balances::is_dead_account(&2), true);
 
 					// account 4 tries to take index 1 again for account 6.
 					assert_ok!(Balances::transfer(Some(4).into(), 6, 256 * 1 + 0x69));
 					assert_eq!(Balances::total_balance(&6), 256 * 1 + 0x69);
-					assert_eq!(Balances::is_dead_account(&6), false);
 				});
 		}
 
@@ -623,7 +616,6 @@ macro_rules! decl_tests {
 					Balances::transfer_keep_alive(Some(1).into(), 2, 100),
 					Error::<$test, _>::KeepAlive
 				);
-				assert_eq!(Balances::is_dead_account(&1), false);
 				assert_eq!(Balances::total_balance(&1), 100);
 				assert_eq!(Balances::total_balance(&2), 0);
 			});
@@ -686,7 +678,6 @@ macro_rules! decl_tests {
 					// Reserve some free balance
 					let _ = Balances::slash(&1, 1);
 					// The account should be dead.
-					assert!(Balances::is_dead_account(&1));
 					assert_eq!(Balances::free_balance(1), 0);
 					assert_eq!(Balances::reserved_balance(1), 0);
 				});

frame/session/src/lib.rs

@@ -444,7 +444,7 @@ decl_storage! {
 			for (account, val, keys) in config.keys.iter().cloned() {
 				<Module<T>>::inner_set_keys(&val, keys)
 					.expect("genesis config must not contain duplicates; qed");
-				frame_system::Module::<T>::inc_ref(&account);
+				assert!(frame_system::Module::<T>::inc_consumers(&account).is_ok());
 			}
 
 			let initial_validators_0 = T::SessionManager::new_session(0)
@@ -498,6 +498,8 @@ decl_error! {
 		DuplicatedKey,
 		/// No keys are associated with this account.
 		NoKeys,
+		/// Key setting account is not live, so it's impossible to associate keys.
+		NoAccount,
 	}
 }
 
@@ -697,9 +699,11 @@ impl<T: Trait> Module<T> {
 		let who = T::ValidatorIdOf::convert(account.clone())
 			.ok_or(Error::<T>::NoAssociatedValidatorId)?;
 
+		frame_system::Module::<T>::inc_consumers(&account).map_err(|_| Error::<T>::NoAccount)?;
 		let old_keys = Self::inner_set_keys(&who, keys)?;
-		if old_keys.is_none() {
-			frame_system::Module::<T>::inc_ref(&account);
+		if old_keys.is_some() {
+			let _ = frame_system::Module::<T>::dec_consumers(&account);
+			// ^^^ Defensive only; Consumers were incremented just before, so should never fail.
 		}
 
 		Ok(())
@@ -747,7 +751,7 @@ impl<T: Trait> Module<T> {
 			let key_data = old_keys.get_raw(*id);
 			Self::clear_key_owner(*id, key_data);
 		}
-		frame_system::Module::<T>::dec_ref(&account);
+		frame_system::Module::<T>::dec_consumers(&account);
 
 		Ok(())
 	}

frame/support/src/traits.rs

@@ -297,44 +297,72 @@ mod test_impl_filter_stack {
 	}
 }
 
+/// Error that can be returned by our impl of `StoredMap`.
+pub enum StoredMapError {
+	/// Attempt to create map value when it is a consumer and there are no providers in place.
+	NoProviders,
+	/// Attempt to anull/remove value when it is the last provider and there is still at
+	/// least one consumer left.
+	ConsumerRemaining,
+}
+
 /// An abstraction of a value stored within storage, but possibly as part of a larger composite
 /// item.
-pub trait StoredMap<K, T> {
+pub trait StoredMap<K, T: Default> {
 	/// Get the item, or its default if it doesn't yet exist; we make no distinction between the
 	/// two.
 	fn get(k: &K) -> T;
-	/// Get whether the item takes up any storage. If this is `false`, then `get` will certainly
-	/// return the `T::default()`. If `true`, then there is no implication for `get` (i.e. it
-	/// may return any value, including the default).
-	///
-	/// NOTE: This may still be `true`, even after `remove` is called. This is the case where
-	/// a single storage entry is shared between multiple `StoredMap` items single, without
-	/// additional logic to enforce it, deletion of any one them doesn't automatically imply
-	/// deletion of them all.
-	fn is_explicit(k: &K) -> bool;
-	/// Mutate the item.
-	fn mutate<R>(k: &K, f: impl FnOnce(&mut T) -> R) -> R;
-	/// Mutate the item, removing or resetting to default value if it has been mutated to `None`.
-	fn mutate_exists<R>(k: &K, f: impl FnOnce(&mut Option<T>) -> R) -> R;
+
 	/// Maybe mutate the item only if an `Ok` value is returned from `f`. Do nothing if an `Err` is
 	/// returned. It is removed or reset to default value if it has been mutated to `None`
-	fn try_mutate_exists<R, E>(k: &K, f: impl FnOnce(&mut Option<T>) -> Result<R, E>) -> Result<R, E>;
+	fn try_mutate_exists<R, E: From<StoredMapError>>(
+		k: &K,
+		f: impl FnOnce(&mut Option<T>) -> Result<R, E>,
+	) -> Result<R, E>;
+
+	// Everything past here has a default implementation.
+
+	/// Mutate the item.
+	fn mutate<R>(k: &K, f: impl FnOnce(&mut T) -> R) -> Result<R, StoredMapError> {
+		Self::mutate_exists(k, |maybe_account| match maybe_account {
+			Some(ref mut account) => f(account),
+			x @ None => {
+				let mut account = Default::default();
+				let r = f(&mut account);
+				*x = Some(account);
+				r
+			}
+		})
+	}
+
+	/// Mutate the item, removing or resetting to default value if it has been mutated to `None`.
+	///
+	/// This is infallible as long as the value does not get destroyed.
+	fn mutate_exists<R>(
+		k: &K,
+		f: impl FnOnce(&mut Option<T>) -> R,
+	) -> Result<R, StoredMapError> {
+		Self::try_mutate_exists(k, |x| -> Result<R, StoredMapError> { Ok(f(x)) })
+	}
+
 	/// Set the item to something new.
-	fn insert(k: &K, t: T) { Self::mutate(k, |i| *i = t); }
+	fn insert(k: &K, t: T) -> Result<(), StoredMapError> { Self::mutate(k, |i| *i = t) }
+
 	/// Remove the item or otherwise replace it with its default value; we don't care which.
-	fn remove(k: &K);
+	fn remove(k: &K) -> Result<(), StoredMapError> { Self::mutate_exists(k, |x| *x = None) }
 }
 
 /// A simple, generic one-parameter event notifier/handler.
-pub trait Happened<T> {
-	/// The thing happened.
-	fn happened(t: &T);
-}
+pub trait HandleLifetime<T> {
+	/// An account was created.
+	fn created(_t: &T) -> Result<(), StoredMapError> { Ok(()) }
 
-impl<T> Happened<T> for () {
-	fn happened(_: &T) {}
+	/// An account was killed.
+	fn killed(_t: &T) -> Result<(), StoredMapError> { Ok(()) }
 }
 
+impl<T> HandleLifetime<T> for () {}
+
 /// A shim for placing around a storage item in order to use it as a `StoredValue`. Ideally this
 /// wouldn't be needed as `StorageValue`s should blanket implement `StoredValue`s, however this
 /// would break the ability to have custom impls of `StoredValue`. The other workaround is to
@@ -346,68 +374,63 @@ impl<T> Happened<T> for () {
 /// be the default value), or where the account is being removed or reset back to the default value
 /// where previously it did exist (though may have been in a default state). This works well with
 /// system module's `CallOnCreatedAccount` and `CallKillAccount`.
-pub struct StorageMapShim<
-	S,
-	Created,
-	Removed,
-	K,
-	T
->(sp_std::marker::PhantomData<(S, Created, Removed, K, T)>);
+pub struct StorageMapShim<S, L, K, T>(sp_std::marker::PhantomData<(S, L, K, T)>);
 impl<
 	S: StorageMap<K, T, Query=T>,
-	Created: Happened<K>,
-	Removed: Happened<K>,
+	L: HandleLifetime<K>,
 	K: FullCodec,
-	T: FullCodec,
-> StoredMap<K, T> for StorageMapShim<S, Created, Removed, K, T> {
+	T: FullCodec + Default,
+> StoredMap<K, T> for StorageMapShim<S, L, K, T> {
 	fn get(k: &K) -> T { S::get(k) }
-	fn is_explicit(k: &K) -> bool { S::contains_key(k) }
-	fn insert(k: &K, t: T) {
-		let existed = S::contains_key(&k);
-		S::insert(k, t);
-		if !existed {
-			Created::happened(k);
+	fn insert(k: &K, t: T) -> Result<(), StoredMapError> {
+		if !S::contains_key(&k) {
+			L::created(k)?;
 		}
+		S::insert(k, t);
+		Ok(())
 	}
-	fn remove(k: &K) {
-		let existed = S::contains_key(&k);
-		S::remove(k);
-		if existed {
-			Removed::happened(&k);
+	fn remove(k: &K) -> Result<(), StoredMapError> {
+		if S::contains_key(&k) {
+			L::killed(&k)?;
+			S::remove(k);
 		}
+		Ok(())
 	}
-	fn mutate<R>(k: &K, f: impl FnOnce(&mut T) -> R) -> R {
-		let existed = S::contains_key(&k);
-		let r = S::mutate(k, f);
-		if !existed {
-			Created::happened(k);
+	fn mutate<R>(k: &K, f: impl FnOnce(&mut T) -> R) -> Result<R, StoredMapError> {
+		if !S::contains_key(&k) {
+			L::created(k)?;
 		}
-		r
+		Ok(S::mutate(k, f))
 	}
-	fn mutate_exists<R>(k: &K, f: impl FnOnce(&mut Option<T>) -> R) -> R {
-		let (existed, exists, r) = S::mutate_exists(k, |maybe_value| {
+	fn mutate_exists<R>(k: &K, f: impl FnOnce(&mut Option<T>) -> R) -> Result<R, StoredMapError> {
+		S::try_mutate_exists(k, |maybe_value| {
 			let existed = maybe_value.is_some();
 			let r = f(maybe_value);
-			(existed, maybe_value.is_some(), r)
-		});
-		if !existed && exists {
-			Created::happened(k);
-		} else if existed && !exists {
-			Removed::happened(k);
-		}
-		r
+			let exists = maybe_value.is_some();
+
+			if !existed && exists {
+				L::created(k)?;
+			} else if existed && !exists {
+				L::killed(k)?;
+			}
+			Ok(r)
+		})
 	}
-	fn try_mutate_exists<R, E>(k: &K, f: impl FnOnce(&mut Option<T>) -> Result<R, E>) -> Result<R, E> {
+	fn try_mutate_exists<R, E: From<StoredMapError>>(
+		k: &K,
+		f: impl FnOnce(&mut Option<T>) -> Result<R, E>,
+	) -> Result<R, E> {
 		S::try_mutate_exists(k, |maybe_value| {
 			let existed = maybe_value.is_some();
-			f(maybe_value).map(|v| (existed, maybe_value.is_some(), v))
-		}).map(|(existed, exists, v)| {
+			let r = f(maybe_value)?;
+			let exists = maybe_value.is_some();
+
 			if !existed && exists {
-				Created::happened(k);
+				L::created(k).map_err(E::from)?;
 			} else if existed && !exists {
-				Removed::happened(k);
+				L::killed(k).map_err(E::from)?;
 			}
-			v
+			Ok(r)
 		})
 	}
 }
@@ -506,18 +529,6 @@ pub trait ContainsLengthBound {
 	fn max_len() -> usize;
 }
 
-/// Determiner to say whether a given account is unused.
-pub trait IsDeadAccount<AccountId> {
-	/// Is the given account dead?
-	fn is_dead_account(who: &AccountId) -> bool;
-}
-
-impl<AccountId> IsDeadAccount<AccountId> for () {
-	fn is_dead_account(_who: &AccountId) -> bool {
-		true
-	}
-}
-
 /// Handler for when a new account has been created.
 #[impl_for_tuples(30)]
 pub trait OnNewAccount<AccountId> {
@@ -1557,7 +1568,7 @@ pub mod schedule {
 		/// Reschedule a task. For one-off tasks, this dispatch is guaranteed to succeed
 		/// only if it is executed *before* the currently scheduled block. For periodic tasks,
 		/// this dispatch is guaranteed to succeed only before the *initial* execution; for
-		/// others, use `reschedule_named`. 
+		/// others, use `reschedule_named`.
 		///
 		/// Will return an error if the `address` is invalid.
 		fn reschedule(

frame/system/src/extensions/check_nonce.rs

@@ -129,7 +129,8 @@ mod tests {
 		new_test_ext().execute_with(|| {
 			crate::Account::<Test>::insert(1, crate::AccountInfo {
 				nonce: 1,
-				refcount: 0,
+				consumers: 0,
+				providers: 0,
 				data: 0,
 			});
 			let info = DispatchInfo::default();