Lazy reaping (paritytech#4895)

* Squash and rebase from gav-lazy-reaping

* Bump version

* Bump runtime again

* Docs.

* Remove old functions

* Update frame/balances/src/lib.rs

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

* Update frame/contracts/src/lib.rs

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

* Warnings

* Bump runtime version

* Update frame/democracy/src/lib.rs

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

* Update frame/system/src/lib.rs

* Clean up OnReapAccount

* Use frame_support debug

* Bump spec

* Renames and fix

* Fix

* Fix rename

* Fix

* Increase time for test

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-authored-by: Benjamin Kampmann <ben.kampmann@googlemail.com>

bin/node-template/pallets/template/src/mock.rs

@@ -41,7 +41,7 @@ impl system::Trait for Test {
 	type ModuleToIndex = ();
 	type AccountData = ();
 	type OnNewAccount = ();
-	type OnReapAccount = ();
+	type OnKilledAccount = ();
 }
 impl Trait for Test {
 	type Event = ();

bin/node-template/runtime/src/lib.rs

@@ -164,7 +164,7 @@ impl system::Trait for Runtime {
 	/// What to do if a new account is created.
 	type OnNewAccount = ();
 	/// What to do if an account is fully reaped from the system.
-	type OnReapAccount = Balances;
+	type OnKilledAccount = ();
 	/// The data to be stored in an account.
 	type AccountData = balances::AccountData<Balance>;
 }

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/cli/tests/common.rs

@@ -60,5 +60,5 @@ pub fn run_command_for_a_while(base_path: &Path, dev: bool) {
 
 	// Stop the process
 	kill(Pid::from_raw(cmd.id().try_into().unwrap()), SIGINT).unwrap();
-	assert!(wait_for(&mut cmd, 20).map(|x| x.success()).unwrap_or_default());
+	assert!(wait_for(&mut cmd, 40).map(|x| x.success()).unwrap_or_default());
 }

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()
@@ -734,7 +734,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

@@ -139,10 +139,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

@@ -81,8 +81,8 @@ 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: 224,
-	impl_version: 2,
+	spec_version: 225,
+	impl_version: 0,
 	apis: RUNTIME_API_VERSIONS,
 };
 
@@ -131,7 +131,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 OnKilledAccount = ();
 }
 
 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/assets/src/lib.rs

@@ -295,7 +295,7 @@ mod tests {
 		type ModuleToIndex = ();
 		type AccountData = ();
 		type OnNewAccount = ();
-		type OnReapAccount = ();
+		type OnKilledAccount = ();
 	}
 	impl Trait for Test {
 		type Event = ();

frame/aura/src/mock.rs

@@ -63,7 +63,7 @@ impl frame_system::Trait for Test {
 	type ModuleToIndex = ();
 	type AccountData = ();
 	type OnNewAccount = ();
-	type OnReapAccount = ();
+	type OnKilledAccount = ();
 }
 
 impl pallet_timestamp::Trait for Test {

frame/authority-discovery/src/lib.rs

@@ -159,7 +159,7 @@ mod tests {
 		type ModuleToIndex = ();
 		type AccountData = ();
 		type OnNewAccount = ();
-		type OnReapAccount = ();
+		type OnKilledAccount = ();
 	}
 
 	impl_outer_origin! {

frame/authorship/src/lib.rs

@@ -433,7 +433,7 @@ mod tests {
 		type ModuleToIndex = ();
 		type AccountData = ();
 		type OnNewAccount = ();
-		type OnReapAccount = ();
+		type OnKilledAccount = ();
 	}
 
 	parameter_types! {

frame/babe/src/mock.rs

@@ -66,7 +66,7 @@ impl frame_system::Trait for Test {
 	type ModuleToIndex = ();
 	type AccountData = ();
 	type OnNewAccount = ();
-	type OnReapAccount = ();
+	type OnKilledAccount = ();
 }
 
 impl_opaque_keys! {

frame/balances/Cargo.toml

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

frame/balances/src/lib.rs

@@ -159,11 +159,12 @@ 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,
 	weights::SimpleDispatchInfo, traits::{
-		Currency, OnReapAccount, OnUnbalanced, TryDrop, StoredMap,
+		Currency, OnKilledAccount, OnUnbalanced, TryDrop, StoredMap,
 		WithdrawReason, WithdrawReasons, LockIdentifier, LockableCurrency, ExistenceRequirement,
 		Imbalance, SignedImbalance, ReservableCurrency, Get, ExistenceRequirement::KeepAlive,
 		ExistenceRequirement::AllowDeath, IsDeadAccount, BalanceStatus as Status
@@ -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);
+			}
+		}
 	}
 }
 
@@ -923,7 +940,7 @@ impl<T: Subtrait<I>, I: Instance> frame_system::Trait for ElevatedTrait<T, I> {
 	type Version = T::Version;
 	type ModuleToIndex = T::ModuleToIndex;
 	type OnNewAccount = T::OnNewAccount;
-	type OnReapAccount = T::OnReapAccount;
+	type OnKilledAccount = T::OnKilledAccount;
 	type AccountData = T::AccountData;
 }
 impl<T: Subtrait<I>, I: Instance> Trait<I> for ElevatedTrait<T, I> {
@@ -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 `OnKilledAccount` 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> OnKilledAccount<T::AccountId> for Module<T, I> {
+	fn on_killed_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 OnKilledAccount = ();
 }
 parameter_types! {
 	pub const TransactionBaseFee: u64 = 0;

frame/balances/src/tests_local.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 OnKilledAccount = Module<Test>;
 }
 parameter_types! {
 	pub const TransactionBaseFee: u64 = 0;

frame/collective/src/lib.rs

@@ -435,7 +435,7 @@ mod tests {
 		type ModuleToIndex = ();
 		type AccountData = ();
 		type OnNewAccount = ();
-		type OnReapAccount = ();
+		type OnKilledAccount = ();
 	}
 	impl Trait<Instance1> for Test {
 		type Origin = Origin;

frame/contracts/src/account_db.rs

@@ -151,7 +151,7 @@ impl<T: Trait> AccountDb<T> for DirectAccountDb {
 				let exists  = !T::Currency::total_balance(&address).is_zero();
 				total_imbalance = total_imbalance.merge(imbalance);
 				if existed && !exists {
-					// Account killed. This will ultimately lead to calling `OnReapAccount` callback
+					// Account killed. This will ultimately lead to calling `OnKilledAccount` callback
 					// which will make removal of CodeHashOf and AccountStorage for this account.
 					// In order to avoid writing over the deleted properties we `continue` here.
 					continue;

frame/contracts/src/lib.rs

@@ -125,7 +125,7 @@ use frame_support::{
 	parameter_types, IsSubType,
 	weights::DispatchInfo,
 };
-use frame_support::traits::{OnReapAccount, OnUnbalanced, Currency, Get, Time, Randomness};
+use frame_support::traits::{OnKilledAccount, OnUnbalanced, Currency, Get, Time, Randomness};
 use frame_system::{self as system, ensure_signed, RawOrigin, ensure_root};
 use sp_core::storage::well_known_keys::CHILD_STORAGE_KEY_PREFIX;
 use pallet_contracts_primitives::{RentProjection, ContractAccessError};
@@ -941,8 +941,12 @@ decl_storage! {
 	}
 }
 
-impl<T: Trait> OnReapAccount<T::AccountId> for Module<T> {
-	fn on_reap_account(who: &T::AccountId) {
+// 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> OnKilledAccount<T::AccountId> for Module<T> {
+	fn on_killed_account(who: &T::AccountId) {
 		if let Some(ContractInfo::Alive(info)) = <ContractInfoOf<T>>::take(who) {
 			child::kill_storage(&info.trie_id, info.child_trie_unique_id());
 		}