srml-contract: Fail calls to removed contracts instead of succeeding. (#2968)

* srml-contract: Refactor to reduce unnecessary storage lookups.

* srml-contract: Fail calls to removed contracts.

Previously, the calls would transfer funds and succeed without executing
any code on the target account, which is unintuitive behavior.

* Bump node runtime spec/impl versions.

substrate/srml/contracts/src/exec.rs

@@ -15,13 +15,14 @@
 // along with Substrate. If not, see <http://www.gnu.org/licenses/>.
 
 use super::{CodeHash, Config, ContractAddressFor, Event, RawEvent, Trait,
-	TrieId, BalanceOf, ContractInfoOf};
+	TrieId, BalanceOf, ContractInfo};
 use crate::account_db::{AccountDb, DirectAccountDb, OverlayAccountDb};
 use crate::gas::{Gas, GasMeter, Token, approx_gas_for_balance};
+use crate::rent;
 
 use rstd::prelude::*;
 use runtime_primitives::traits::{Bounded, CheckedAdd, CheckedSub, Zero};
-use srml_support::{StorageMap, traits::{WithdrawReason, Currency}};
+use srml_support::traits::{WithdrawReason, Currency};
 use timestamp;
 
 pub type AccountIdOf<T> = <T as system::Trait>::AccountId;
@@ -267,11 +268,11 @@ where
 {
 	/// Create the top level execution context.
 	///
-	/// The specified `origin` address will be used as `sender` for
+	/// The specified `origin` address will be used as `sender` for. The `origin` must be a regular
+	/// account (not a contract).
 	pub fn top_level(origin: T::AccountId, cfg: &'a Config<T>, vm: &'a V, loader: &'a L) -> Self {
 		ExecutionContext {
-			self_trie_id: <ContractInfoOf<T>>::get(&origin)
-				.and_then(|i| i.as_alive().map(|i| i.trie_id.clone())),
+			self_trie_id: None,
 			self_account: origin,
 			overlay: OverlayAccountDb::<T>::new(&DirectAccountDb),
 			depth: 0,
@@ -283,10 +284,11 @@ where
 		}
 	}
 
-	fn nested(&self, overlay: OverlayAccountDb<'a, T>, dest: T::AccountId) -> Self {
+	fn nested(&self, overlay: OverlayAccountDb<'a, T>, dest: T::AccountId, trie_id: Option<TrieId>)
+		-> Self
+	{
 		ExecutionContext {
-			self_trie_id: <ContractInfoOf<T>>::get(&dest)
-				.and_then(|i| i.as_alive().map(|i| i.trie_id.clone())),
+			self_trie_id: trie_id,
 			self_account: dest,
 			overlay,
 			depth: self.depth + 1,
@@ -321,14 +323,20 @@ where
 		// Assumption: pay_rent doesn't collide with overlay because
 		// pay_rent will be done on first call and dest contract and balance
 		// cannot be changed before the first call
-		crate::rent::pay_rent::<T>(&dest);
+		let contract_info = rent::pay_rent::<T>(&dest);
+
+		// Calls to dead contracts always fail.
+		if let Some(ContractInfo::Tombstone(_)) = contract_info {
+			return Err("contract has been evicted");
+		};
 
 		let mut output_data = Vec::new();
 
 		let (change_set, events, calls) = {
 			let mut nested = self.nested(
 				OverlayAccountDb::new(&self.overlay),
-				dest.clone()
+				dest.clone(),
+				contract_info.and_then(|i| i.as_alive().map(|i| i.trie_id.clone()))
 			);
 
 			if value > BalanceOf::<T>::zero() {
@@ -342,6 +350,8 @@ where
 				)?;
 			}
 
+			// If code_hash is not none, then the destination account is a live contract, otherwise
+			// it is a regular account since tombstone accounts have already been rejected.
 			if let Some(dest_code_hash) = self.overlay.get_code_hash(&dest) {
 				let executable = self.loader.load_main(&dest_code_hash)?;
 				output_data = self
@@ -400,7 +410,8 @@ where
 
 			overlay.create_contract(&dest, code_hash.clone())?;
 
-			let mut nested = self.nested(overlay, dest.clone());
+			// TrieId has not been generated yet and storage is empty since contract is new.
+			let mut nested = self.nested(overlay, dest.clone(), None);
 
 			// Send funds unconditionally here. If the `endowment` is below existential_deposit
 			// then error will be returned here.

substrate/srml/contracts/src/rent.rs

@@ -14,7 +14,7 @@
 // You should have received a copy of the GNU General Public License
 // along with Substrate. If not, see <http://www.gnu.org/licenses/>.
 
-use crate::{BalanceOf, ContractInfo, ContractInfoOf, TombstoneContractInfo, Trait};
+use crate::{BalanceOf, ContractInfo, ContractInfoOf, TombstoneContractInfo, Trait, AliveContractInfo};
 use runtime_primitives::traits::{Bounded, CheckedDiv, CheckedMul, Saturating, Zero,
 	SaturatedConversion};
 use srml_support::traits::{Currency, ExistenceRequirement, Get, WithdrawReason};
@@ -50,9 +50,10 @@ fn try_evict_or_and_pay_rent<T: Trait>(
 	account: &T::AccountId,
 	handicap: T::BlockNumber,
 	pay_rent: bool,
-) -> RentOutcome {
-	let contract = match <ContractInfoOf<T>>::get(account) {
-		None | Some(ContractInfo::Tombstone(_)) => return RentOutcome::Exempted,
+) -> (RentOutcome, Option<ContractInfo<T>>) {
+	let contract_info = <ContractInfoOf<T>>::get(account);
+	let contract = match contract_info {
+		None | Some(ContractInfo::Tombstone(_)) => return (RentOutcome::Exempted, contract_info),
 		Some(ContractInfo::Alive(contract)) => contract,
 	};
 
@@ -65,7 +66,7 @@ fn try_evict_or_and_pay_rent<T: Trait>(
 		let n = effective_block_number.saturating_sub(contract.deduct_block);
 		if n.is_zero() {
 			// Rent has already been paid
-			return RentOutcome::Exempted;
+			return (RentOutcome::Exempted, Some(ContractInfo::Alive(contract)));
 		}
 		n
 	};
@@ -89,7 +90,7 @@ fn try_evict_or_and_pay_rent<T: Trait>(
 	if fee_per_block.is_zero() {
 		// The rent deposit offset reduced the fee to 0. This means that the contract
 		// gets the rent for free.
-		return RentOutcome::Exempted;
+		return (RentOutcome::Exempted, Some(ContractInfo::Alive(contract)));
 	}
 
 	// The minimal amount of funds required for a contract not to be evicted.
@@ -99,7 +100,7 @@ fn try_evict_or_and_pay_rent<T: Trait>(
 		// The contract cannot afford to leave a tombstone, so remove the contract info altogether.
 		<ContractInfoOf<T>>::remove(account);
 		runtime_io::kill_child_storage(&contract.trie_id);
-		return RentOutcome::Evicted;
+		return (RentOutcome::Evicted, None);
 	}
 
 	let dues = fee_per_block
@@ -149,33 +150,34 @@ fn try_evict_or_and_pay_rent<T: Trait>(
 			&child_storage_root[..],
 			contract.code_hash,
 		);
-		<ContractInfoOf<T>>::insert(account, ContractInfo::Tombstone(tombstone));
+		let tombstone_info = ContractInfo::Tombstone(tombstone);
+		<ContractInfoOf<T>>::insert(account, &tombstone_info);
 
 		runtime_io::kill_child_storage(&contract.trie_id);
 
-		return RentOutcome::Evicted;
+		return (RentOutcome::Evicted, Some(tombstone_info));
 	}
 
 	if pay_rent {
-		<ContractInfoOf<T>>::mutate(account, |contract| {
-			let contract = contract
-				.as_mut()
-				.and_then(|c| c.as_alive_mut())
-				.expect("Dead or nonexistent account has been exempt above; qed");
+		let contract_info = ContractInfo::Alive(AliveContractInfo::<T> {
+			rent_allowance: contract.rent_allowance - dues, // rent_allowance is not exceeded
+			deduct_block: current_block_number,
+			..contract
+		});
 
-			contract.rent_allowance -= dues; // rent_allowance is not exceeded
-			contract.deduct_block = current_block_number;
-		})
+		<ContractInfoOf<T>>::insert(account, &contract_info);
+
+		return (RentOutcome::Ok, Some(contract_info));
 	}
 
-	RentOutcome::Ok
+	(RentOutcome::Ok, Some(ContractInfo::Alive(contract)))
 }
 
 /// Make account paying the rent for the current block number
 ///
 /// NOTE: This function acts eagerly.
-pub fn pay_rent<T: Trait>(account: &T::AccountId) {
-	let _ = try_evict_or_and_pay_rent::<T>(account, Zero::zero(), true);
+pub fn pay_rent<T: Trait>(account: &T::AccountId) -> Option<ContractInfo<T>> {
+	try_evict_or_and_pay_rent::<T>(account, Zero::zero(), true).1
 }
 
 /// Evict the account if it should be evicted at the given block number.
@@ -186,5 +188,5 @@ pub fn pay_rent<T: Trait>(account: &T::AccountId) {
 ///
 /// NOTE: This function acts eagerly.
 pub fn try_evict<T: Trait>(account: &T::AccountId, handicap: T::BlockNumber) -> RentOutcome {
-	try_evict_or_and_pay_rent::<T>(account, handicap, false)
+	try_evict_or_and_pay_rent::<T>(account, handicap, false).0
 }

substrate/srml/contracts/src/tests.rs

@@ -927,7 +927,11 @@ fn deduct_blocks() {
 
 #[test]
 fn call_contract_removals() {
-	removals(|| Contract::call(Origin::signed(ALICE), BOB, 0, 100_000, call::null()).is_ok());
+	removals(|| {
+		// Call on already-removed account might fail, and this is fine.
+		Contract::call(Origin::signed(ALICE), BOB, 0, 100_000, call::null());
+		true
+	});
 }
 
 #[test]
@@ -1115,6 +1119,45 @@ fn removals(trigger_call: impl Fn() -> bool) {
 	);
 }
 
+#[test]
+fn call_removed_contract() {
+	let wasm = wabt::wat2wasm(CODE_SET_RENT).unwrap();
+
+	// Balance reached and superior to subsistence threshold
+	with_externalities(
+		&mut ExtBuilder::default().existential_deposit(50).build(),
+		|| {
+			// Create
+			Balances::deposit_creating(&ALICE, 1_000_000);
+			assert_ok!(Contract::put_code(Origin::signed(ALICE), 100_000, wasm.clone()));
+			assert_ok!(Contract::create(
+				Origin::signed(ALICE),
+				100,
+				100_000, HASH_SET_RENT.into(),
+				<Test as balances::Trait>::Balance::from(1_000u32).encode() // rent allowance
+			));
+
+			// Calling contract should succeed.
+			assert_ok!(Contract::call(Origin::signed(ALICE), BOB, 0, 100_000, call::null()));
+
+			// Advance blocks
+			System::initialize(&10, &[0u8; 32].into(), &[0u8; 32].into(), &Default::default());
+
+			// Calling contract should remove contract and fail.
+			assert_err!(
+				Contract::call(Origin::signed(ALICE), BOB, 0, 100_000, call::null()),
+				"contract has been evicted"
+			);
+
+ 			// Subsequent contract calls should also fail.
+			assert_err!(
+				Contract::call(Origin::signed(ALICE), BOB, 0, 100_000, call::null()),
+				"contract has been evicted"
+			);
+		}
+	)
+}
+
 const CODE_CHECK_DEFAULT_RENT_ALLOWANCE: &str = r#"
 (module
 	(import "env" "ext_rent_allowance" (func $ext_rent_allowance))
@@ -1294,7 +1337,7 @@ fn restoration(test_different_storage: bool, test_restore_to_with_dirty_storage:
 	// same copy of this (modulo hex notation differences) in `CODE_RESTORATION`.
 	//
 	// When this assert is triggered make sure that you update the literals here and in
-	// `CODE_RESTORATION`. Hopefully, we switch to automatical injection of the code.
+	// `CODE_RESTORATION`. Hopefully, we switch to automatic injection of the code.
 	const ENCODED_CALL_LITERAL: &str =
 		"0105020000000000000069aedfb4f6c1c398e97f8a5204de0f95ad5e7dc3540960beab11a86c569fbfcf320000\
 		0000000000080100000000000000000000000000000000000000000000000000000000000000010000000000000\
@@ -1368,7 +1411,10 @@ fn restoration(test_different_storage: bool, test_restore_to_with_dirty_storage:
 
 			// Call `BOB`, which makes it pay rent. Since the rent allowance is set to 0
 			// we expect that it will get removed leaving tombstone.
-			assert_ok!(Contract::call(Origin::signed(ALICE), BOB, 0, 100_000, call::null()));
+			assert_err!(
+				Contract::call(Origin::signed(ALICE), BOB, 0, 100_000, call::null()),
+				"contract has been evicted"
+			);
 			assert!(ContractInfoOf::<Test>::get(BOB).unwrap().get_tombstone().is_some());
 
 			/// Create another account with the address `DJANGO` with `CODE_RESTORATION`.