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:
#6439 (comment)

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

frame/contracts/src/benchmarking/mod.rs

@@ -39,7 +39,7 @@ use self::{
 use frame_benchmarking::{benchmarks, account, whitelisted_caller};
 use frame_system::{Module as System, RawOrigin};
 use parity_wasm::elements::{Instruction, ValueType, BlockType};
-use sp_runtime::traits::{Hash, Bounded};
+use sp_runtime::traits::{Hash, Bounded, Zero};
 use sp_std::{default::Default, convert::{TryInto}, vec::Vec, vec};
 use pallet_contracts_primitives::RentProjection;
 
@@ -252,7 +252,7 @@ where
 	/// Evict this contract.
 	fn evict(&mut self) -> Result<(), &'static str> {
 		self.set_block_num_for_eviction()?;
-		Rent::<T>::collect(&self.contract.account_id);
+		Rent::<T>::snitch_contract_should_be_evicted(&self.contract.account_id, Zero::zero());
 		self.contract.ensure_tombstone()
 	}
 }

frame/contracts/src/exec.rs

@@ -267,12 +267,12 @@ where
 			Err(Error::<T>::MaxCallDepthReached)?
 		}
 
-		// Assumption: `collect` doesn't collide with overlay because
-		// `collect` will be done on first call and destination contract and balance
-		// cannot be changed before the first call
-		// We do not allow 'calling' plain accounts. For transfering value
-		// `seal_transfer` must be used.
-		let contract = if let Some(ContractInfo::Alive(info)) = Rent::<T>::collect(&dest) {
+		// This charges the rent and denies access to a contract that is in need of
+		// eviction by returning `None`. We cannot evict eagerly here because those
+		// changes would be rolled back in case this contract is called by another
+		// contract.
+		// See: https://github.com/paritytech/substrate/issues/6439#issuecomment-648754324
+		let contract = if let Some(ContractInfo::Alive(info)) = Rent::<T>::charge(&dest) {
 			info
 		} else {
 			Err(Error::<T>::NotCallable)?

frame/contracts/src/rent.rs

@@ -23,9 +23,11 @@ use crate::{
 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,
@@ -73,10 +75,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 {
@@ -180,11 +178,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()
 			}
 		};
 
@@ -233,16 +237,11 @@ where
 		alive_contract_info: AliveContractInfo<T>,
 		current_block_number: T::BlockNumber,
 		verdict: Verdict<T>,
+		allow_eviction: bool,
 	) -> Option<ContractInfo<T>> {
 		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));
+			Verdict::Evict { amount: _ } if !allow_eviction => {
 				None
 			}
 			Verdict::Evict { amount } => {
@@ -286,9 +285,10 @@ where
 
 	/// 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) -> Option<ContractInfo<T>> {
 		let contract_info = <ContractInfoOf<T>>::get(account);
 		let alive_contract_info = match contract_info {
 			None | Some(ContractInfo::Tombstone(_)) => return contract_info,
@@ -302,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,8 +321,8 @@ where
 		account: &T::AccountId,
 		handicap: T::BlockNumber,
 	) -> bool {
-		let contract_info = <ContractInfoOf<T>>::get(account);
-		let alive_contract_info = match contract_info {
+		let contract = <ContractInfoOf<T>>::get(account);
+		let contract = match contract {
 			None | Some(ContractInfo::Tombstone(_)) => return false,
 			Some(ContractInfo::Alive(contract)) => contract,
 		};
@@ -331,13 +331,13 @@ 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);
+			Verdict::Evict { .. } => {
+				Self::enact_verdict(account, contract, current_block_number, verdict, true);
 				true
 			}
 			_ => false,
@@ -371,7 +371,7 @@ 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 {

frame/contracts/src/tests.rs

@@ -774,15 +774,6 @@ fn deduct_blocks() {
 		});
 }
 
-#[test]
-fn call_contract_removals() {
-	removals(|addr| {
-		// Call on already-removed account might fail, and this is fine.
-		let _ = Contracts::call(Origin::signed(ALICE), addr, 0, GAS_LIMIT, call::null());
-		true
-	});
-}
-
 #[test]
 fn inherent_claim_surcharge_contract_removals() {
 	removals(|addr| Contracts::claim_surcharge(Origin::none(), addr, Some(ALICE)).is_ok());
@@ -1046,25 +1037,23 @@ fn call_removed_contract() {
 			// Advance blocks
 			initialize_block(10);
 
-			// Calling contract should remove contract and fail.
+			// Calling contract should deny access because rent cannot be paid.
 			assert_err_ignore_postinfo!(
 				Contracts::call(Origin::signed(ALICE), addr.clone(), 0, GAS_LIMIT, call::null()),
 				Error::<Test>::NotCallable
 			);
-			// Calling a contract that is about to evict shall emit an event.
-			assert_eq!(System::events(), vec![
-				EventRecord {
-					phase: Phase::Initialization,
-					event: MetaEvent::contracts(RawEvent::Evicted(addr.clone(), true)),
-					topics: vec![],
-				},
-			]);
+			// No event is generated because the contract is not actually removed.
+			assert_eq!(System::events(), vec![]);
 
 			// Subsequent contract calls should also fail.
 			assert_err_ignore_postinfo!(
-				Contracts::call(Origin::signed(ALICE), addr, 0, GAS_LIMIT, call::null()),
+				Contracts::call(Origin::signed(ALICE), addr.clone(), 0, GAS_LIMIT, call::null()),
 				Error::<Test>::NotCallable
 			);
+
+			// A snitch can now remove the contract
+			assert_ok!(Contracts::claim_surcharge(Origin::none(), addr.clone(), Some(ALICE)));
+			assert!(ContractInfoOf::<Test>::get(&addr).unwrap().get_tombstone().is_some());
 		})
 }
 
@@ -1193,13 +1182,17 @@ fn restoration(test_different_storage: bool, test_restore_to_with_dirty_storage:
 			initialize_block(5);
 
 			// Call `BOB`, which makes it pay rent. Since the rent allowance is set to 0
-			// we expect that it will get removed leaving tombstone.
+			// we expect that it is no longer callable but keeps existing until someone
+			// calls `claim_surcharge`.
 			assert_err_ignore_postinfo!(
 				Contracts::call(
 					Origin::signed(ALICE), addr_bob.clone(), 0, GAS_LIMIT, call::null()
 				),
 				Error::<Test>::NotCallable
 			);
+			assert_eq!(System::events(), vec![]);
+			assert!(ContractInfoOf::<Test>::get(&addr_bob).unwrap().get_alive().is_some());
+			assert_ok!(Contracts::claim_surcharge(Origin::none(), addr_bob.clone(), Some(ALICE)));
 			assert!(ContractInfoOf::<Test>::get(&addr_bob).unwrap().get_tombstone().is_some());
 			assert_eq!(System::events(), vec![
 				EventRecord {