srml-contract: Contract refactors (#2924)

* srml-contract: Refactor away unnecessary Option.

* srml-contract: Add assertion to gas_left test.

* srml-contract: Refactor try_evict_or_and_pay_rent to make tests pass.

* srml-contract: Add tests and comments for bugs in rent payment logic.

* srml-contract: Minor cleanup using GasMeter constructor.

* Bump node runtime impl version.

substrate/node/runtime/src/lib.rs

@@ -59,7 +59,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	impl_name: create_runtime_str!("substrate-node"),
 	authoring_version: 10,
 	spec_version: 99,
-	impl_version: 103,
+	impl_version: 104,
 	apis: RUNTIME_API_VERSIONS,
 };
 

substrate/srml/contracts/src/gas.rs

@@ -93,7 +93,6 @@ pub struct GasMeter<T: Trait> {
 	tokens: Vec<ErasedToken>,
 }
 impl<T: Trait> GasMeter<T> {
-	#[cfg(test)]
 	pub fn with_limit(gas_limit: Gas, gas_price: BalanceOf<T>) -> GasMeter<T> {
 		GasMeter {
 			limit: gas_limit,
@@ -162,13 +161,7 @@ impl<T: Trait> GasMeter<T> {
 			f(None)
 		} else {
 			self.gas_left = self.gas_left - amount;
-			let mut nested = GasMeter {
-				limit: amount,
-				gas_left: amount,
-				gas_price: self.gas_price,
-				#[cfg(test)]
-				tokens: Vec::new(),
-			};
+			let mut nested = GasMeter::with_limit(amount, self.gas_price);
 
 			let r = f(Some(&mut nested));
 
@@ -231,14 +224,7 @@ pub fn buy_gas<T: Trait>(
 		ExistenceRequirement::KeepAlive
 	)?;
 
-	Ok((GasMeter {
-		limit: gas_limit,
-		gas_left: gas_limit,
-		gas_price,
-
-		#[cfg(test)]
-		tokens: Vec::new(),
-	}, imbalance))
+	Ok((GasMeter::with_limit(gas_limit, gas_price), imbalance))
 }
 
 /// Refund the unused gas.

substrate/srml/contracts/src/rent.rs

@@ -17,7 +17,7 @@
 use crate::{BalanceOf, ContractInfo, ContractInfoOf, Module, TombstoneContractInfo, Trait};
 use runtime_primitives::traits::{Bounded, CheckedDiv, CheckedMul, Saturating, Zero,
 	SaturatedConversion};
-use srml_support::traits::{Currency, ExistenceRequirement, Imbalance, WithdrawReason};
+use srml_support::traits::{Currency, ExistenceRequirement, WithdrawReason};
 use srml_support::StorageMap;
 
 #[derive(PartialEq, Eq, Copy, Clone)]
@@ -95,14 +95,26 @@ fn try_evict_or_and_pay_rent<T: Trait>(
 	// The minimal amount of funds required for a contract not to be evicted.
 	let subsistence_threshold = T::Currency::minimum_balance() + <Module<T>>::tombstone_deposit();
 
+	if balance < subsistence_threshold {
+		// 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;
+	}
+
 	let dues = fee_per_block
 		.checked_mul(&blocks_passed.saturated_into::<u32>().into())
 		.unwrap_or(<BalanceOf<T>>::max_value());
+	let rent_budget = contract.rent_allowance.min(balance - subsistence_threshold);
+	let insufficient_rent = rent_budget < dues;
 
-	let dues_limited = dues.min(contract.rent_allowance);
-	let rent_allowance_exceeded = dues > contract.rent_allowance;
-	let is_below_subsistence = balance < subsistence_threshold;
-	let go_below_subsistence = balance.saturating_sub(dues_limited) < subsistence_threshold;
+	// If the rent payment cannot be withdrawn due to locks on the account balance, then evict the
+	// account.
+	//
+	// NOTE: This seems problematic because it provides a way to tombstone an account while
+	// avoiding the last rent payment. In effect, someone could retroactively set rent_allowance
+	// for their contract to 0.
+	let dues_limited = dues.min(rent_budget);
 	let can_withdraw_rent = T::Currency::ensure_can_withdraw(
 		account,
 		dues_limited,
@@ -111,68 +123,52 @@ fn try_evict_or_and_pay_rent<T: Trait>(
 	)
 	.is_ok();
 
-	if !rent_allowance_exceeded && can_withdraw_rent && !go_below_subsistence {
-		// Collect dues
-
-		if pay_rent {
-			let imbalance = T::Currency::withdraw(
-				account,
-				dues,
-				WithdrawReason::Fee,
-				ExistenceRequirement::KeepAlive,
-			)
-			.expect(
-				"Withdraw has been checked above;
-				go_below_subsistence is false and subsistence > existencial_deposit;
-				qed",
-			);
-
-			<ContractInfoOf<T>>::mutate(account, |contract| {
-				let contract = contract
-					.as_mut()
-					.and_then(|c| c.as_alive_mut())
-					.expect("Dead or inexistent account has been exempt above; qed");
-
-				contract.rent_allowance -= imbalance.peek(); // rent_allowance is not exceeded
-				contract.deduct_block = current_block_number;
-			})
-		}
-
-		RentOutcome::Ok
-	} else {
-		// Evict
-
-		if can_withdraw_rent && !go_below_subsistence {
-			T::Currency::withdraw(
-				account,
-				dues,
-				WithdrawReason::Fee,
-				ExistenceRequirement::KeepAlive,
-			)
-			.expect("Can withdraw and don't go below subsistence");
-		} else if !is_below_subsistence {
-			T::Currency::make_free_balance_be(account, subsistence_threshold);
-		} else {
-			T::Currency::make_free_balance_be(account, <BalanceOf<T>>::zero());
-		}
-
-		if !is_below_subsistence {
-			// The contract has funds above subsistence deposit and that means it can afford to
-			// leave tombstone.
-
-			// Note: this operation is heavy.
-			let child_storage_root = runtime_io::child_storage_root(&contract.trie_id);
-
-			let tombstone = <TombstoneContractInfo<T>>::new(
-				&child_storage_root[..],
-				contract.code_hash,
-			);
-			<ContractInfoOf<T>>::insert(account, ContractInfo::Tombstone(tombstone));
-			runtime_io::kill_child_storage(&contract.trie_id);
-		}
-
-		RentOutcome::Evicted
+	if can_withdraw_rent && (insufficient_rent || pay_rent) {
+		// Collect dues.
+		let _ = T::Currency::withdraw(
+			account,
+			dues_limited,
+			WithdrawReason::Fee,
+			ExistenceRequirement::KeepAlive,
+		)
+		.expect(
+			"Withdraw has been checked above;
+			dues_limited < rent_budget < balance - subsistence < balance - existential_deposit;
+			qed",
+		);
 	}
+
+	if insufficient_rent || !can_withdraw_rent {
+		// The contract cannot afford the rent payment and has a balance above the subsistence
+		// threshold, so it leaves a tombstone.
+
+		// Note: this operation is heavy.
+		let child_storage_root = runtime_io::child_storage_root(&contract.trie_id);
+
+		let tombstone = <TombstoneContractInfo<T>>::new(
+			&child_storage_root[..],
+			contract.code_hash,
+		);
+		<ContractInfoOf<T>>::insert(account, ContractInfo::Tombstone(tombstone));
+
+		runtime_io::kill_child_storage(&contract.trie_id);
+
+		return RentOutcome::Evicted;
+	}
+
+	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");
+
+			contract.rent_allowance -= dues; // rent_allowance is not exceeded
+			contract.deduct_block = current_block_number;
+		})
+	}
+
+	RentOutcome::Ok
 }
 
 /// Make account paying the rent for the current block number

substrate/srml/contracts/src/tests.rs

@@ -742,7 +742,7 @@ mod call {
 	pub fn null() -> Vec<u8> { vec![0, 0, 0] }
 }
 
-/// Test correspondance of set_rent code and its hash.
+/// Test correspondence of set_rent code and its hash.
 /// Also test that encoded extrinsic in code correspond to the correct transfer
 #[test]
 fn test_set_rent_code_and_hash() {
@@ -954,9 +954,12 @@ fn removals(trigger_call: impl Fn() -> bool) {
 				<Test as balances::Trait>::Balance::from(1_000u32).encode() // rent allowance
 			));
 
+			let subsistence_threshold = 50 /*existential_deposit*/ + 16 /*tombstone_deposit*/;
+
 			// Trigger rent must have no effect
 			assert!(trigger_call());
 			assert_eq!(ContractInfoOf::<Test>::get(BOB).unwrap().get_alive().unwrap().rent_allowance, 1_000);
+			assert_eq!(Balances::free_balance(&BOB), 100);
 
 			// Advance blocks
 			System::initialize(&10, &[0u8; 32].into(), &[0u8; 32].into(), &Default::default());
@@ -964,6 +967,7 @@ fn removals(trigger_call: impl Fn() -> bool) {
 			// Trigger rent through call
 			assert!(trigger_call());
 			assert!(ContractInfoOf::<Test>::get(BOB).unwrap().get_tombstone().is_some());
+			assert_eq!(Balances::free_balance(&BOB), subsistence_threshold);
 
 			// Advance blocks
 			System::initialize(&20, &[0u8; 32].into(), &[0u8; 32].into(), &Default::default());
@@ -971,6 +975,7 @@ fn removals(trigger_call: impl Fn() -> bool) {
 			// Trigger rent must have no effect
 			assert!(trigger_call());
 			assert!(ContractInfoOf::<Test>::get(BOB).unwrap().get_tombstone().is_some());
+			assert_eq!(Balances::free_balance(&BOB), subsistence_threshold);
 		}
 	);
 
@@ -991,6 +996,7 @@ fn removals(trigger_call: impl Fn() -> bool) {
 			// Trigger rent must have no effect
 			assert!(trigger_call());
 			assert_eq!(ContractInfoOf::<Test>::get(BOB).unwrap().get_alive().unwrap().rent_allowance, 100);
+			assert_eq!(Balances::free_balance(&BOB), 1_000);
 
 			// Advance blocks
 			System::initialize(&10, &[0u8; 32].into(), &[0u8; 32].into(), &Default::default());
@@ -998,6 +1004,8 @@ fn removals(trigger_call: impl Fn() -> bool) {
 			// Trigger rent through call
 			assert!(trigger_call());
 			assert!(ContractInfoOf::<Test>::get(BOB).unwrap().get_tombstone().is_some());
+			// Balance should be initial balance - initial rent_allowance
+			assert_eq!(Balances::free_balance(&BOB), 900);
 
 			// Advance blocks
 			System::initialize(&20, &[0u8; 32].into(), &[0u8; 32].into(), &Default::default());
@@ -1005,6 +1013,7 @@ fn removals(trigger_call: impl Fn() -> bool) {
 			// Trigger rent must have no effect
 			assert!(trigger_call());
 			assert!(ContractInfoOf::<Test>::get(BOB).unwrap().get_tombstone().is_some());
+			assert_eq!(Balances::free_balance(&BOB), 900);
 		}
 	);
 
@@ -1025,10 +1034,12 @@ fn removals(trigger_call: impl Fn() -> bool) {
 			// Trigger rent must have no effect
 			assert!(trigger_call());
 			assert_eq!(ContractInfoOf::<Test>::get(BOB).unwrap().get_alive().unwrap().rent_allowance, 1_000);
+			assert_eq!(Balances::free_balance(&BOB), 50 + Balances::minimum_balance());
 
 			// Transfer funds
 			assert_ok!(Contract::call(Origin::signed(ALICE), BOB, 0, 100_000, call::transfer()));
 			assert_eq!(ContractInfoOf::<Test>::get(BOB).unwrap().get_alive().unwrap().rent_allowance, 1_000);
+			assert_eq!(Balances::free_balance(&BOB), Balances::minimum_balance());
 
 			// Advance blocks
 			System::initialize(&10, &[0u8; 32].into(), &[0u8; 32].into(), &Default::default());
@@ -1036,6 +1047,7 @@ fn removals(trigger_call: impl Fn() -> bool) {
 			// Trigger rent through call
 			assert!(trigger_call());
 			assert!(ContractInfoOf::<Test>::get(BOB).is_none());
+			assert_eq!(Balances::free_balance(&BOB), Balances::minimum_balance());
 
 			// Advance blocks
 			System::initialize(&20, &[0u8; 32].into(), &[0u8; 32].into(), &Default::default());
@@ -1043,6 +1055,7 @@ fn removals(trigger_call: impl Fn() -> bool) {
 			// Trigger rent must have no effect
 			assert!(trigger_call());
 			assert!(ContractInfoOf::<Test>::get(BOB).is_none());
+			assert_eq!(Balances::free_balance(&BOB), Balances::minimum_balance());
 		}
 	);
 }

substrate/srml/contracts/src/wasm/mod.rs

@@ -175,7 +175,7 @@ mod tests {
 	use std::collections::HashMap;
 	use substrate_primitives::H256;
 	use crate::exec::{CallReceipt, Ext, InstantiateReceipt, EmptyOutputBuf, StorageKey};
-	use crate::gas::GasMeter;
+	use crate::gas::{Gas, GasMeter};
 	use crate::tests::{Test, Call};
 	use crate::wasm::prepare::prepare_contract;
 	use crate::CodeHash;
@@ -906,14 +906,20 @@ mod tests {
 	fn gas_left() {
 		let mut mock_ext = MockExt::default();
 		let mut gas_meter = GasMeter::with_limit(50_000, 1312);
+
+		let mut return_buf = Vec::new();
 		execute(
 			CODE_GAS_LEFT,
 			&[],
-			&mut Vec::new(),
+			&mut return_buf,
 			&mut mock_ext,
 			&mut gas_meter,
 		)
 		.unwrap();
+
+		let gas_left = Gas::decode(&mut &return_buf[..]).unwrap();
+		assert!(gas_left < 50_000, "gas_left must be less than initial");
+		assert!(gas_left > gas_meter.gas_left(), "gas_left must be greater than final");
 	}
 
 	const CODE_VALUE_TRANSFERRED: &str = r#"

substrate/srml/contracts/src/wasm/prepare.rs

@@ -30,11 +30,7 @@ use runtime_primitives::traits::{SaturatedConversion};
 
 struct ContractModule<'a> {
 	/// A deserialized module. The module is valid (this is Guaranteed by `new` method).
-	///
-	/// An `Option` is used here for loaning (`take()`-ing) the module.
-	/// Invariant: Can't be `None` (i.e. on enter and on exit from the function
-	/// the value *must* be `Some`).
-	module: Option<elements::Module>,
+	module: elements::Module,
 	schedule: &'a Schedule,
 }
 
@@ -58,7 +54,7 @@ impl<'a> ContractModule<'a> {
 		// Return a `ContractModule` instance with
 		// __valid__ module.
 		Ok(ContractModule {
-			module: Some(module),
+			module,
 			schedule,
 		})
 	}
@@ -69,11 +65,7 @@ impl<'a> ContractModule<'a> {
 	/// Memory section contains declarations of internal linear memories, so if we find one
 	/// we reject such a module.
 	fn ensure_no_internal_memory(&self) -> Result<(), &'static str> {
-		let module = self
-			.module
-			.as_ref()
-			.expect("On entry to the function `module` can't be None; qed");
-		if module
+		if self.module
 			.memory_section()
 			.map_or(false, |ms| ms.entries().len() > 0)
 		{
@@ -82,7 +74,7 @@ impl<'a> ContractModule<'a> {
 		Ok(())
 	}
 
-	fn inject_gas_metering(&mut self) -> Result<(), &'static str> {
+	fn inject_gas_metering(self) -> Result<Self, &'static str> {
 		let gas_rules =
 			rules::Set::new(
 				self.schedule.regular_op_cost.clone().saturated_into(),
@@ -91,30 +83,22 @@ impl<'a> ContractModule<'a> {
 			.with_grow_cost(self.schedule.grow_mem_cost.clone().saturated_into())
 			.with_forbidden_floats();
 
-		let module = self
-			.module
-			.take()
-			.expect("On entry to the function `module` can't be `None`; qed");
-
-		let contract_module = pwasm_utils::inject_gas_counter(module, &gas_rules)
+		let contract_module = pwasm_utils::inject_gas_counter(self.module, &gas_rules)
 			.map_err(|_| "gas instrumentation failed")?;
-
-		self.module = Some(contract_module);
-		Ok(())
+		Ok(ContractModule {
+			module: contract_module,
+			schedule: self.schedule,
+		})
 	}
 
-	fn inject_stack_height_metering(&mut self) -> Result<(), &'static str> {
-		let module = self
-			.module
-			.take()
-			.expect("On entry to the function `module` can't be `None`; qed");
-
+	fn inject_stack_height_metering(self) -> Result<Self, &'static str> {
 		let contract_module =
-			pwasm_utils::stack_height::inject_limiter(module, self.schedule.max_stack_height)
+			pwasm_utils::stack_height::inject_limiter(self.module, self.schedule.max_stack_height)
 				.map_err(|_| "stack height instrumentation failed")?;
-
-		self.module = Some(contract_module);
-		Ok(())
+		Ok(ContractModule {
+			module: contract_module,
+			schedule: self.schedule,
+		})
 	}
 
 	/// Check that the module has required exported functions. For now
@@ -128,10 +112,7 @@ impl<'a> ContractModule<'a> {
 		let mut deploy_found = false;
 		let mut call_found = false;
 
-		let module = self
-			.module
-			.as_ref()
-			.expect("On entry to the function `module` can't be `None`; qed");
+		let module = &self.module;
 
 		let types = module.type_section().map(|ts| ts.types()).unwrap_or(&[]);
 		let export_entries = module
@@ -213,10 +194,7 @@ impl<'a> ContractModule<'a> {
 	///   their signatures.
 	/// - if there is a memory import, returns it's descriptor
 	fn scan_imports<C: ImportSatisfyCheck>(&self) -> Result<Option<&MemoryType>, &'static str> {
-		let module = self
-			.module
-			.as_ref()
-			.expect("On entry to the function `module` can't be `None`; qed");
+		let module = &self.module;
 
 		let types = module.type_section().map(|ts| ts.types()).unwrap_or(&[]);
 		let import_entries = module
@@ -269,13 +247,9 @@ impl<'a> ContractModule<'a> {
 		Ok(imported_mem_type)
 	}
 
-	fn into_wasm_code(mut self) -> Result<Vec<u8>, &'static str> {
-		elements::serialize(
-			self.module
-				.take()
-				.expect("On entry to the function `module` can't be `None`; qed"),
-		)
-		.map_err(|_| "error serializing instrumented module")
+	fn into_wasm_code(self) -> Result<Vec<u8>, &'static str> {
+		elements::serialize(self.module)
+			.map_err(|_| "error serializing instrumented module")
 	}
 }
 
@@ -332,8 +306,9 @@ pub fn prepare_contract<C: ImportSatisfyCheck>(
 		}
 	};
 
-	contract_module.inject_gas_metering()?;
-	contract_module.inject_stack_height_metering()?;
+	contract_module = contract_module
+		.inject_gas_metering()?
+		.inject_stack_height_metering()?;
 
 	Ok(PrefabWasmModule {
 		schedule_version: schedule.version,

substrate/srml/contracts/src/wasm/runtime.rs

@@ -276,7 +276,7 @@ define_env!(Env, <E: Ext>,
 		Ok(())
 	},
 
-	// Retrieve the value at the given location from the strorage and return 0.
+	// Retrieve the value at the given location from the storage and return 0.
 	// If there is no entry at the given location then this function will return 1 and
 	// clear the scratch buffer.
 	//