Lazy reaping

bin/node/cli/src/chain_spec.rs

@@ -243,7 +243,7 @@ pub fn testnet_genesis(
 		}),
 		pallet_session: Some(SessionConfig {
 			keys: initial_authorities.iter().map(|x| {
-				(x.0.clone(), session_keys(x.2.clone(), x.3.clone(), x.4.clone(), x.5.clone()))
+				(x.0.clone(), x.0.clone(), session_keys(x.2.clone(), x.3.clone(), x.4.clone(), x.5.clone()))
 			}).collect::<Vec<_>>(),
 		}),
 		pallet_staking: Some(StakingConfig {

bin/node/executor/tests/basic.rs

@@ -164,7 +164,7 @@ fn panic_execution_with_foreign_code_gives_error() {
 	let mut t = TestExternalities::<Blake2Hasher>::new_with_code(BLOATY_CODE, Storage {
 		top: map![
 			<frame_system::Account<Runtime>>::hashed_key_for(alice()) => {
-				(69u128, 0u128, 0u128, 0u128).encode()
+				(69u128, 0u8, 0u128, 0u128, 0u128).encode()
 			},
 			<pallet_balances::TotalIssuance<Runtime>>::hashed_key().to_vec() => {
 				69_u128.encode()
@@ -200,7 +200,7 @@ fn bad_extrinsic_with_native_equivalent_code_gives_error() {
 	let mut t = TestExternalities::<Blake2Hasher>::new_with_code(COMPACT_CODE, Storage {
 		top: map![
 			<frame_system::Account<Runtime>>::hashed_key_for(alice()) => {
-				(0u32, 69u128, 0u128, 0u128, 0u128).encode()
+				(0u32, 0u8, 69u128, 0u128, 0u128, 0u128).encode()
 			},
 			<pallet_balances::TotalIssuance<Runtime>>::hashed_key().to_vec() => {
 				69_u128.encode()
@@ -236,7 +236,7 @@ fn successful_execution_with_native_equivalent_code_gives_ok() {
 	let mut t = TestExternalities::<Blake2Hasher>::new_with_code(COMPACT_CODE, Storage {
 		top: map![
 			<frame_system::Account<Runtime>>::hashed_key_for(alice()) => {
-				(0u32, 111 * DOLLARS, 0u128, 0u128, 0u128).encode()
+				(0u32, 0u8, 111 * DOLLARS, 0u128, 0u128, 0u128).encode()
 			},
 			<pallet_balances::TotalIssuance<Runtime>>::hashed_key().to_vec() => {
 				(111 * DOLLARS).encode()
@@ -278,7 +278,7 @@ fn successful_execution_with_foreign_code_gives_ok() {
 	let mut t = TestExternalities::<Blake2Hasher>::new_with_code(BLOATY_CODE, Storage {
 		top: map![
 			<frame_system::Account<Runtime>>::hashed_key_for(alice()) => {
-				(0u32, 111 * DOLLARS, 0u128, 0u128, 0u128).encode()
+				(0u32, 0u8, 111 * DOLLARS, 0u128, 0u128, 0u128).encode()
 			},
 			<pallet_balances::TotalIssuance<Runtime>>::hashed_key().to_vec() => {
 				(111 * DOLLARS).encode()
@@ -733,7 +733,7 @@ fn successful_execution_gives_ok() {
 	let mut t = TestExternalities::<Blake2Hasher>::new_with_code(COMPACT_CODE, Storage {
 		top: map![
 			<frame_system::Account<Runtime>>::hashed_key_for(alice()) => {
-				(0u32, 111 * DOLLARS, 0u128, 0u128, 0u128).encode()
+				(0u32, 0u8, 111 * DOLLARS, 0u128, 0u128, 0u128).encode()
 			},
 			<pallet_balances::TotalIssuance<Runtime>>::hashed_key().to_vec() => {
 				(111 * DOLLARS).encode()

bin/node/executor/tests/fees.rs

@@ -135,10 +135,10 @@ fn transaction_fee_is_correct_ultimate() {
 	let mut t = TestExternalities::<Blake2Hasher>::new_with_code(COMPACT_CODE, Storage {
 		top: map![
 			<frame_system::Account<Runtime>>::hashed_key_for(alice()) => {
-				(0u32, 100 * DOLLARS, 0 * DOLLARS, 0 * DOLLARS, 0 * DOLLARS).encode()
+				(0u32, 0u8, 100 * DOLLARS, 0 * DOLLARS, 0 * DOLLARS, 0 * DOLLARS).encode()
 			},
 			<frame_system::Account<Runtime>>::hashed_key_for(bob()) => {
-				(0u32, 10 * DOLLARS, 0 * DOLLARS, 0 * DOLLARS, 0 * DOLLARS).encode()
+				(0u32, 0u8, 10 * DOLLARS, 0 * DOLLARS, 0 * DOLLARS, 0 * DOLLARS).encode()
 			},
 			<pallet_balances::TotalIssuance<Runtime>>::hashed_key().to_vec() => {
 				(110 * DOLLARS).encode()

bin/node/executor/tests/submit_transaction.rs

@@ -167,8 +167,9 @@ fn submitted_transaction_should_be_valid() {
 		// add balance to the account
 		let author = extrinsic.signature.clone().unwrap().0;
 		let address = Indices::lookup(author).unwrap();
-		let account = pallet_balances::AccountData { free: 5_000_000_000_000, ..Default::default() };
-		<frame_system::Account<Runtime>>::insert(&address, (0u32, account));
+		let data = pallet_balances::AccountData { free: 5_000_000_000_000, ..Default::default() };
+		let account = frame_system::AccountInfo { nonce: 0u32, refcount: 0u8, data };
+		<frame_system::Account<Runtime>>::insert(&address, account);
 
 		// check validity
 		let res = Executive::validate_transaction(extrinsic);

bin/node/runtime/src/lib.rs

@@ -82,7 +82,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	// and set impl_version to 0. If only runtime
 	// implementation changes and behavior does not, then leave spec_version as
 	// is and increment impl_version.
-	spec_version: 221,
+	spec_version: 222,
 	impl_version: 0,
 	apis: RUNTIME_API_VERSIONS,
 };
@@ -132,7 +132,7 @@ impl frame_system::Trait for Runtime {
 	type ModuleToIndex = ModuleToIndex;
 	type AccountData = pallet_balances::AccountData<Balance>;
 	type OnNewAccount = ();
-	type OnReapAccount = (Balances, Staking, Contracts, Session, Recovery);
+	type OnReapAccount = ();
 }
 
 parameter_types! {

bin/node/testing/src/genesis.rs

@@ -69,15 +69,15 @@ pub fn config_endowed(
 		}),
 		pallet_session: Some(SessionConfig {
 			keys: vec![
-				(alice(), to_session_keys(
+				(dave(), alice(), to_session_keys(
 					&Ed25519Keyring::Alice,
 					&Sr25519Keyring::Alice,
 				)),
-				(bob(), to_session_keys(
+				(eve(), bob(), to_session_keys(
 					&Ed25519Keyring::Bob,
 					&Sr25519Keyring::Bob,
 				)),
-				(charlie(), to_session_keys(
+				(ferdie(), charlie(), to_session_keys(
 					&Ed25519Keyring::Charlie,
 					&Sr25519Keyring::Charlie,
 				)),

frame/balances/Cargo.toml

@@ -15,7 +15,6 @@ frame-support = { version = "2.0.0", default-features = false, path = "../suppor
 frame-system = { version = "2.0.0", default-features = false, path = "../system" }
 
 [dev-dependencies]
-sp-io = { version = "2.0.0", path = "../../primitives/io" }
 sp-core = { version = "2.0.0", path = "../../primitives/core" }
 pallet-transaction-payment = { version = "2.0.0", path = "../transaction-payment" }
 

frame/balances/src/lib.rs

@@ -159,6 +159,7 @@ mod benchmarking;
 
 use sp_std::prelude::*;
 use sp_std::{cmp, result, mem, fmt::Debug, ops::BitOr, convert::Infallible};
+use sp_io::hashing::twox_64;
 use codec::{Codec, Encode, Decode};
 use frame_support::{
 	StorageValue, Parameter, decl_event, decl_storage, decl_module, decl_error, ensure,
@@ -178,7 +179,7 @@ use sp_runtime::{
 };
 use frame_system::{self as system, ensure_signed, ensure_root};
 use frame_support::storage::migration::{
-	get_storage_value, take_storage_value, put_storage_value, StorageIterator
+	get_storage_value, take_storage_value, put_storage_value, StorageIterator, have_storage_value
 };
 
 pub use self::imbalances::{PositiveImbalance, NegativeImbalance};
@@ -609,7 +610,16 @@ impl<T: Trait<I>, I: Instance> Module<T, I> {
 
 		for (hash, balances) in StorageIterator::<AccountData<T::Balance>>::new(b"Balances", b"Account").drain() {
 			let nonce = take_storage_value::<T::Index>(b"System", b"AccountNonce", &hash).unwrap_or_default();
-			put_storage_value(b"System", b"Account", &hash, (nonce, balances));
+			let mut refs: system::RefCount = 0;
+			// The items in Kusama that would result in a ref count being incremented.
+			if have_storage_value(b"Democracy", b"Proxy", &hash) { refs += 1 }
+			// We skip Recovered since it's being replaced anyway.
+			let mut prefixed_hash = twox_64(&b":session:keys"[..]).to_vec();
+			prefixed_hash.extend(&b":session:keys"[..]);
+			prefixed_hash.extend(&hash[..]);
+			if have_storage_value(b"Session", b"NextKeys", &prefixed_hash) { refs += 1 }
+			if have_storage_value(b"Staking", b"Bonded", &hash) { refs += 1 }
+			put_storage_value(b"System", b"Account", &hash, (nonce, refs, &balances));
 		}
 	}
 
@@ -721,14 +731,21 @@ impl<T: Trait<I>, I: Instance> Module<T, I> {
 				}
 			}
 		});
-		Locks::<T, I>::insert(who, locks);
-	}
-}
 
-impl<T: Trait<I>, I: Instance> OnReapAccount<T::AccountId> for Module<T, I> {
-	fn on_reap_account(who: &T::AccountId) {
-		Locks::<T, I>::remove(who);
-		Account::<T, I>::remove(who);
+		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::Module::<T>::dec_ref(who);
+			}
+		} else {
+			Locks::<T, I>::insert(who, locks);
+			if !existed {
+				system::Module::<T>::inc_ref(who);
+			}
+		}
 	}
 }
 
@@ -1040,6 +1057,7 @@ impl<T: Trait<I>, I: Instance> Currency<T::AccountId> for Module<T, I> where
 				)?;
 
 				let allow_death = existence_requirement == ExistenceRequirement::AllowDeath;
+				let allow_death = allow_death && system::Module::<T>::allow_death(transactor);
 				ensure!(allow_death || from_account.free >= ed, Error::<T, I>::KeepAlive);
 
 				Ok(())
@@ -1283,6 +1301,17 @@ impl<T: Trait<I>, I: Instance> ReservableCurrency<T::AccountId> for Module<T, I>
 	}
 }
 
+/// Implement `OnReapAccount` to remove the local account, if using local account storage.
+///
+/// NOTE: You probably won't need to use this! This only needs to be "wired in" to System module
+/// if you're using the local balance storage. **If you're using the composite system account
+/// storage (which is the default in most examples and tests) then there's no need.**
+impl<T: Trait<I>, I: Instance> OnReapAccount<T::AccountId> for Module<T, I> {
+	fn on_reap_account(who: &T::AccountId) {
+		Account::<T, I>::remove(who);
+	}
+}
+
 impl<T: Trait<I>, I: Instance> LockableCurrency<T::AccountId> for Module<T, I>
 where
 	T::Balance: MaybeSerializeDeserialize + Debug

frame/balances/src/tests.rs

@@ -24,8 +24,10 @@ macro_rules! decl_tests {
 		use sp_runtime::{Fixed64, traits::{SignedExtension, BadOrigin}};
 		use frame_support::{
 			assert_noop, assert_ok, assert_err,
-			traits::{LockableCurrency, LockIdentifier, WithdrawReason, WithdrawReasons,
-				Currency, ReservableCurrency, ExistenceRequirement::AllowDeath}
+			traits::{
+				LockableCurrency, LockIdentifier, WithdrawReason, WithdrawReasons,
+				Currency, ReservableCurrency, ExistenceRequirement::AllowDeath, StoredMap
+			}
 		};
 		use pallet_transaction_payment::ChargeTransactionPayment;
 		use frame_system::RawOrigin;
@@ -55,6 +57,15 @@ macro_rules! decl_tests {
 			});
 		}
 
+		#[test]
+		fn account_should_be_reaped() {
+			<$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));
+			});
+		}
+
 		#[test]
 		fn partial_locking_should_work() {
 			<$ext_builder>::default().existential_deposit(1).monied(true).build().execute_with(|| {

frame/balances/src/tests_composite.rs

@@ -67,7 +67,7 @@ impl frame_system::Trait for Test {
 	type ModuleToIndex = ();
 	type AccountData = super::AccountData<u64>;
 	type OnNewAccount = ();
-	type OnReapAccount = Module<Test>;
+	type OnReapAccount = ();
 }
 parameter_types! {
 	pub const TransactionBaseFee: u64 = 0;

frame/contracts/src/lib.rs

@@ -941,6 +941,10 @@ decl_storage! {
 	}
 }
 
+// TODO: this should be removed in favour of a self-destruct contract host function allowing the
+// contract to delete all storage and the `ContractInfoOf` key and transfer remaining balance to
+// some other account. As it stands, it's an economic insecurity on any smart-contract chain.
+// https://github.com/paritytech/substrate/issues/4952
 impl<T: Trait> OnReapAccount<T::AccountId> for Module<T> {
 	fn on_reap_account(who: &T::AccountId) {
 		if let Some(ContractInfo::Alive(info)) = <ContractInfoOf<T>>::take(who) {