contracts: Lazy storage removal (#7740)

* Do not evict a contract from within a call stack

We don't want to trigger contract eviction automatically when
a contract is called. This is because those changes can be
reverted due to how storage transactions are used at the moment.
More Information:
https://github.com/paritytech/substrate/issues/6439#issuecomment-648754324

It can be re-introduced once the linked issue is resolved. In the meantime
`claim_surcharge` must be called to evict a contract.

* Lazily delete storage in on_initialize instead of when removing the contract

* Add missing documentation of new error

* Make Module::claim_surcharge public

It being the only dispatchable that is private is an oversight.

* review: Add final newline

* review: Simplify assert statement

* Add test that checks that partial remove of a contract works

* Premote warning to error

* Added missing docs for seal_terminate

* Lazy deletion should only take AVERAGE_ON_INITIALIZE_RATIO of the block

* Added informational about the lazy deletion throughput

* Avoid lazy deletion in case the block is already full

* Prevent queue decoding in case of an already full block

* Add test that checks that on_initialize honors block limits

substrate/frame/contracts/src/rent.rs

@@ -20,13 +20,16 @@
 use crate::{
 	AliveContractInfo, BalanceOf, ContractInfo, ContractInfoOf, Module, RawEvent,
 	TombstoneContractInfo, Config, CodeHash, ConfigCache, Error,
+	storage::Storage,
 };
 use sp_std::prelude::*;
 use sp_io::hashing::blake2_256;
 use sp_core::crypto::UncheckedFrom;
-use frame_support::storage::child;
-use frame_support::traits::{Currency, ExistenceRequirement, Get, OnUnbalanced, WithdrawReasons};
-use frame_support::StorageMap;
+use frame_support::{
+	debug, StorageMap,
+	storage::child,
+	traits::{Currency, ExistenceRequirement, Get, OnUnbalanced, WithdrawReasons},
+};
 use pallet_contracts_primitives::{ContractAccessError, RentProjection, RentProjectionResult};
 use sp_runtime::{
 	DispatchError,
@@ -74,10 +77,6 @@ enum Verdict<T: Config> {
 	/// For example, it already paid its rent in the current block, or it has enough deposit for not
 	/// paying rent at all.
 	Exempt,
-	/// Funds dropped below the subsistence deposit.
-	///
-	/// Remove the contract along with it's storage.
-	Kill,
 	/// The contract cannot afford payment within its rent budget so it gets evicted. However,
 	/// because its balance is greater than the subsistence threshold it leaves a tombstone.
 	Evict {
@@ -181,11 +180,17 @@ where
 		let rent_budget = match Self::rent_budget(&total_balance, &free_balance, contract) {
 			Some(rent_budget) => rent_budget,
 			None => {
-				// The contract's total balance is already below subsistence threshold. That
-				// indicates that the contract cannot afford to leave a tombstone.
-				//
-				// So cleanly wipe the contract.
-				return Verdict::Kill;
+				// All functions that allow a contract to transfer balance enforce
+				// that the contract always stays above the subsistence threshold.
+				// We want the rent system to always leave a tombstone to prevent the
+				// accidental loss of a contract. Ony `seal_terminate` can remove a
+				// contract without a tombstone. Therefore this case should be never
+				// hit.
+				debug::error!(
+					"Tombstoned a contract that is below the subsistence threshold: {:?}",
+					account
+				);
+				0u32.into()
 			}
 		};
 
@@ -234,19 +239,19 @@ where
 		alive_contract_info: AliveContractInfo<T>,
 		current_block_number: T::BlockNumber,
 		verdict: Verdict<T>,
-	) -> Option<ContractInfo<T>> {
+		allow_eviction: bool,
+	) -> Result<Option<ContractInfo<T>>, DispatchError> {
 		match verdict {
-			Verdict::Exempt => return Some(ContractInfo::Alive(alive_contract_info)),
-			Verdict::Kill => {
-				<ContractInfoOf<T>>::remove(account);
-				child::kill_storage(
-					&alive_contract_info.child_trie_info(),
-					None,
-				);
-				<Module<T>>::deposit_event(RawEvent::Evicted(account.clone(), false));
-				None
+			Verdict::Exempt => return Ok(Some(ContractInfo::Alive(alive_contract_info))),
+			Verdict::Evict { amount: _ } if !allow_eviction => {
+				Ok(None)
 			}
 			Verdict::Evict { amount } => {
+				// We need to remove the trie first because it is the only operation
+				// that can fail and this function is called without a storage
+				// transaction when called through `claim_surcharge`.
+				Storage::<T>::queue_trie_for_deletion(&alive_contract_info)?;
+
 				if let Some(amount) = amount {
 					amount.withdraw(account);
 				}
@@ -262,14 +267,8 @@ where
 				);
 				let tombstone_info = ContractInfo::Tombstone(tombstone);
 				<ContractInfoOf<T>>::insert(account, &tombstone_info);
-
-				child::kill_storage(
-					&alive_contract_info.child_trie_info(),
-					None,
-				);
-
 				<Module<T>>::deposit_event(RawEvent::Evicted(account.clone(), true));
-				Some(tombstone_info)
+				Ok(Some(tombstone_info))
 			}
 			Verdict::Charge { amount } => {
 				let contract_info = ContractInfo::Alive(AliveContractInfo::<T> {
@@ -278,21 +277,21 @@ where
 					..alive_contract_info
 				});
 				<ContractInfoOf<T>>::insert(account, &contract_info);
-
 				amount.withdraw(account);
-				Some(contract_info)
+				Ok(Some(contract_info))
 			}
 		}
 	}
 
 	/// Make account paying the rent for the current block number
 	///
-	/// NOTE this function performs eviction eagerly. All changes are read and written directly to
-	/// storage.
-	pub fn collect(account: &T::AccountId) -> Option<ContractInfo<T>> {
+	/// This functions does **not** evict the contract. It returns `None` in case the
+	/// contract is in need of eviction. [`snitch_contract_should_be_evicted`] must
+	/// be called to perform the eviction.
+	pub fn charge(account: &T::AccountId) -> Result<Option<ContractInfo<T>>, DispatchError> {
 		let contract_info = <ContractInfoOf<T>>::get(account);
 		let alive_contract_info = match contract_info {
-			None | Some(ContractInfo::Tombstone(_)) => return contract_info,
+			None | Some(ContractInfo::Tombstone(_)) => return Ok(contract_info),
 			Some(ContractInfo::Alive(contract)) => contract,
 		};
 
@@ -303,7 +302,7 @@ where
 			Zero::zero(),
 			&alive_contract_info,
 		);
-		Self::enact_verdict(account, alive_contract_info, current_block_number, verdict)
+		Self::enact_verdict(account, alive_contract_info, current_block_number, verdict, false)
 	}
 
 	/// Process a report that a contract under the given address should be evicted.
@@ -321,10 +320,10 @@ where
 	pub fn snitch_contract_should_be_evicted(
 		account: &T::AccountId,
 		handicap: T::BlockNumber,
-	) -> bool {
-		let contract_info = <ContractInfoOf<T>>::get(account);
-		let alive_contract_info = match contract_info {
-			None | Some(ContractInfo::Tombstone(_)) => return false,
+	) -> Result<bool, DispatchError> {
+		let contract = <ContractInfoOf<T>>::get(account);
+		let contract = match contract {
+			None | Some(ContractInfo::Tombstone(_)) => return Ok(false),
 			Some(ContractInfo::Alive(contract)) => contract,
 		};
 		let current_block_number = <frame_system::Module<T>>::block_number();
@@ -332,16 +331,16 @@ where
 			account,
 			current_block_number,
 			handicap,
-			&alive_contract_info,
+			&contract,
 		);
 
 		// Enact the verdict only if the contract gets removed.
 		match verdict {
-			Verdict::Kill | Verdict::Evict { .. } => {
-				Self::enact_verdict(account, alive_contract_info, current_block_number, verdict);
-				true
+			Verdict::Evict { .. } => {
+				Self::enact_verdict(account, contract, current_block_number, verdict, true)?;
+				Ok(true)
 			}
-			_ => false,
+			_ => Ok(false),
 		}
 	}
 
@@ -359,9 +358,11 @@ where
 	pub fn compute_projection(
 		account: &T::AccountId,
 	) -> RentProjectionResult<T::BlockNumber> {
+		use ContractAccessError::IsTombstone;
+
 		let contract_info = <ContractInfoOf<T>>::get(account);
 		let alive_contract_info = match contract_info {
-			None | Some(ContractInfo::Tombstone(_)) => return Err(ContractAccessError::IsTombstone),
+			None | Some(ContractInfo::Tombstone(_)) => return Err(IsTombstone),
 			Some(ContractInfo::Alive(contract)) => contract,
 		};
 		let current_block_number = <frame_system::Module<T>>::block_number();
@@ -372,11 +373,11 @@ where
 			&alive_contract_info,
 		);
 		let new_contract_info =
-			Self::enact_verdict(account, alive_contract_info, current_block_number, verdict);
+			Self::enact_verdict(account, alive_contract_info, current_block_number, verdict, false);
 
 		// Check what happened after enaction of the verdict.
-		let alive_contract_info = match new_contract_info {
-			None | Some(ContractInfo::Tombstone(_)) => return Err(ContractAccessError::IsTombstone),
+		let alive_contract_info = match new_contract_info.map_err(|_| IsTombstone)? {
+			None | Some(ContractInfo::Tombstone(_)) => return Err(IsTombstone),
 			Some(ContractInfo::Alive(contract)) => contract,
 		};