pallet-contracts: Refactor and comment rent module. (#4733)

* Refactor and comment `rent` module.

* impl_version bump

* Add doc for Exempt

* Simplify code.

* Update bin/node/runtime/src/lib.rs

Co-Authored-By: thiolliere <gui.thiolliere@gmail.com>

* Update frame/contracts/src/exec.rs

Co-Authored-By: Hero Bird <robin.freyler@gmail.com>

Co-authored-by: thiolliere <gui.thiolliere@gmail.com>
Co-authored-by: Hero Bird <robbepop@web.de>

substrate/bin/node/runtime/src/lib.rs

@@ -81,7 +81,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	// implementation changes and behavior does not, then leave spec_version as
 	// is and increment impl_version.
 	spec_version: 205,
-	impl_version: 205,
+	impl_version: 206,
 	apis: RUNTIME_API_VERSIONS,
 };
 

substrate/frame/contracts/src/exec.rs

@@ -356,10 +356,10 @@ where
 			});
 		}
 
-		// Assumption: pay_rent doesn't collide with overlay because
-		// pay_rent will be done on first call and dest contract and balance
+		// Assumption: `collect_rent` doesn't collide with overlay because
+		// `collect_rent` will be done on first call and destination contract and balance
 		// cannot be changed before the first call
-		let contract_info = rent::pay_rent::<T>(&dest);
+		let contract_info = rent::collect_rent::<T>(&dest);
 
 		// Calls to dead contracts always fail.
 		if let Some(ContractInfo::Tombstone(_)) = contract_info {

substrate/frame/contracts/src/lib.rs

@@ -669,7 +669,7 @@ decl_module! {
 			};
 
 			// If poking the contract has lead to eviction of the contract, give out the rewards.
-			if rent::try_evict::<T>(&dest, handicap) == rent::RentOutcome::Evicted {
+			if rent::snitch_contract_should_be_evicted::<T>(&dest, handicap) {
 				T::Currency::deposit_into_existing(&rewarded, T::SurchargeReward::get())?;
 			}
 		}

substrate/frame/contracts/src/rent.rs

@@ -14,64 +14,91 @@
 // 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::{Module, RawEvent, BalanceOf, ContractInfo, ContractInfoOf, TombstoneContractInfo,
-	Trait, AliveContractInfo};
-use sp_runtime::traits::{Bounded, CheckedDiv, CheckedMul, Saturating, Zero,
-	SaturatedConversion};
-use frame_support::traits::{Currency, ExistenceRequirement, Get, WithdrawReason, OnUnbalanced};
-use frame_support::StorageMap;
+use crate::{
+	AliveContractInfo, BalanceOf, ContractInfo, ContractInfoOf, Module, RawEvent,
+	TombstoneContractInfo, Trait,
+};
 use frame_support::storage::child;
+use frame_support::traits::{Currency, ExistenceRequirement, Get, OnUnbalanced, WithdrawReason};
+use frame_support::StorageMap;
+use sp_runtime::traits::{Bounded, CheckedDiv, CheckedMul, SaturatedConversion, Saturating, Zero};
 
-#[derive(PartialEq, Eq, Copy, Clone)]
-#[must_use]
-pub enum RentOutcome {
-	/// Exempted from rent iff:
-	/// * rent is offset completely by the `rent_deposit_offset`,
-	/// * or rent has already been paid for this block number,
-	/// * or account doesn't have a contract,
-	/// * or account has a tombstone.
-	Exempted,
-	/// Evicted iff:
-	/// * rent exceed rent allowance,
-	/// * or can't withdraw the rent,
-	/// * or go below subsistence threshold.
-	Evicted,
-	/// The outstanding dues were paid or were able to be paid.
-	Ok,
+/// The amount to charge.
+///
+/// This amount respects the contract's rent allowance and the subsistence deposit.
+/// Because of that, charging the amount cannot remove the contract.
+struct OutstandingAmount<T: Trait> {
+	amount: BalanceOf<T>,
 }
 
-/// Evict and optionally pay dues (or check account can pay them otherwise) at the current
-/// block number (modulo `handicap`, read on).
+impl<T: Trait> OutstandingAmount<T> {
+	/// Create the new outstanding amount.
+	///
+	/// The amount should be always withdrawable and it should not kill the account.
+	fn new(amount: BalanceOf<T>) -> Self {
+		Self { amount }
+	}
+
+	/// Returns the amount this instance wraps.
+	fn peek(&self) -> BalanceOf<T> {
+		self.amount
+	}
+
+	/// Withdraws the outstanding amount from the given account.
+	fn withdraw(self, account: &T::AccountId) {
+		if let Ok(imbalance) = T::Currency::withdraw(
+			account,
+			self.amount,
+			WithdrawReason::Fee.into(),
+			ExistenceRequirement::KeepAlive,
+		) {
+			// This should never fail. However, let's err on the safe side.
+			T::RentPayment::on_unbalanced(imbalance);
+		}
+	}
+}
+
+enum Verdict<T: Trait> {
+	/// The contract is exempted from paying rent.
+	///
+	/// 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 {
+		amount: Option<OutstandingAmount<T>>,
+	},
+	/// Everything is OK, we just only take some charge.
+	Charge {
+		amount: OutstandingAmount<T>,
+	},
+}
+
+/// Consider the case for rent payment of the given account and returns a `Verdict`.
 ///
-/// `pay_rent` gives an ability to pay or skip paying rent.
-/// `handicap` gives a way to check or pay the rent up to a moment in the past instead
-/// of current block.
-///
-/// NOTE: This function acts eagerly, all modification are committed into the storage.
-fn try_evict_or_and_pay_rent<T: Trait>(
+/// The `current_block_number` must be equal to the current block number. Use `handicap` do
+/// change the reference block number. (See `snitch_contract_should_be_evicted` for more details).
+fn consider_case<T: Trait>(
 	account: &T::AccountId,
+	current_block_number: T::BlockNumber,
 	handicap: T::BlockNumber,
-	pay_rent: bool,
-) -> (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,
-	};
-
-	let current_block_number = <frame_system::Module<T>>::block_number();
-
+	contract: &AliveContractInfo<T>,
+) -> Verdict<T> {
 	// How much block has passed since the last deduction for the contract.
 	let blocks_passed = {
 		// Calculate an effective block number, i.e. after adjusting for handicap.
 		let effective_block_number = current_block_number.saturating_sub(handicap);
-		let n = effective_block_number.saturating_sub(contract.deduct_block);
-		if n.is_zero() {
-			// Rent has already been paid
-			return (RentOutcome::Exempted, Some(ContractInfo::Alive(contract)));
-		}
-		n
+		effective_block_number.saturating_sub(contract.deduct_block)
 	};
+	if blocks_passed.is_zero() {
+		// Rent has already been paid
+		return Verdict::Exempt;
+	}
 
 	let balance = T::Currency::free_balance(account);
 
@@ -92,7 +119,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, Some(ContractInfo::Alive(contract)));
+		return Verdict::Exempt;
 	}
 
 	// The minimal amount of funds required for a contract not to be evicted.
@@ -100,10 +127,7 @@ fn try_evict_or_and_pay_rent<T: Trait>(
 
 	if balance < subsistence_threshold {
 		// The contract cannot afford to leave a tombstone, so remove the contract info altogether.
-		<ContractInfoOf<T>>::remove(account);
-		child::kill_storage(&contract.trie_id, contract.child_trie_unique_id());
-		<Module<T>>::deposit_event(RawEvent::Evicted(account.clone(), false));
-		return (RentOutcome::Evicted, None);
+		return Verdict::Kill;
 	}
 
 	let dues = fee_per_block
@@ -127,75 +151,136 @@ fn try_evict_or_and_pay_rent<T: Trait>(
 	)
 	.is_ok();
 
-	if can_withdraw_rent && (insufficient_rent || pay_rent) {
-		// Collect dues.
-		let imbalance = T::Currency::withdraw(
-			account,
-			dues_limited,
-			WithdrawReason::Fee.into(),
-			ExistenceRequirement::KeepAlive,
-		)
-		.expect(
-			"Withdraw has been checked above;
-			dues_limited < rent_budget < balance - subsistence < balance - existential_deposit;
-			qed",
-		);
-
-		T::RentPayment::on_unbalanced(imbalance);
-	}
-
 	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 = child::child_root(
-			&contract.trie_id,
-		);
-
-		let tombstone = <TombstoneContractInfo<T>>::new(
-			&child_storage_root[..],
-			contract.code_hash,
-		);
-		let tombstone_info = ContractInfo::Tombstone(tombstone);
-		<ContractInfoOf<T>>::insert(account, &tombstone_info);
-
-		child::kill_storage(&contract.trie_id, contract.child_trie_unique_id());
-
-		<Module<T>>::deposit_event(RawEvent::Evicted(account.clone(), true));
-
-		return (RentOutcome::Evicted, Some(tombstone_info));
+		let amount = if can_withdraw_rent {
+			Some(OutstandingAmount::new(dues_limited))
+		} else {
+			None
+		};
+		return Verdict::Evict { amount };
 	}
 
-	if pay_rent {
-		let contract_info = ContractInfo::Alive(AliveContractInfo::<T> {
-			rent_allowance: contract.rent_allowance - dues, // rent_allowance is not exceeded
-			deduct_block: current_block_number,
-			..contract
-		});
+	return Verdict::Charge {
+		// We choose to use `dues_limited` here instead of `dues` just to err on the safer side.
+		amount: OutstandingAmount::new(dues_limited),
+	};
+}
 
-		<ContractInfoOf<T>>::insert(account, &contract_info);
+/// Enacts the given verdict and returns the updated `ContractInfo`.
+///
+/// `alive_contract_info` should be from the same address as `account`.
+fn enact_verdict<T: Trait>(
+	account: &T::AccountId,
+	alive_contract_info: AliveContractInfo<T>,
+	current_block_number: T::BlockNumber,
+	verdict: Verdict<T>,
+) -> 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.trie_id,
+				alive_contract_info.child_trie_unique_id(),
+			);
+			<Module<T>>::deposit_event(RawEvent::Evicted(account.clone(), false));
+			None
+		}
+		Verdict::Evict { amount } => {
+			if let Some(amount) = amount {
+				amount.withdraw(account);
+			}
 
-		return (RentOutcome::Ok, Some(contract_info));
+			// Note: this operation is heavy.
+			let child_storage_root = child::child_root(&alive_contract_info.trie_id);
+
+			let tombstone = <TombstoneContractInfo<T>>::new(
+				&child_storage_root[..],
+				alive_contract_info.code_hash,
+			);
+			let tombstone_info = ContractInfo::Tombstone(tombstone);
+			<ContractInfoOf<T>>::insert(account, &tombstone_info);
+
+			child::kill_storage(
+				&alive_contract_info.trie_id,
+				alive_contract_info.child_trie_unique_id(),
+			);
+
+			<Module<T>>::deposit_event(RawEvent::Evicted(account.clone(), true));
+			Some(tombstone_info)
+		}
+		Verdict::Charge { amount } => {
+			let contract_info = ContractInfo::Alive(AliveContractInfo::<T> {
+				rent_allowance: alive_contract_info.rent_allowance - amount.peek(),
+				deduct_block: current_block_number,
+				..alive_contract_info
+			});
+			<ContractInfoOf<T>>::insert(account, &contract_info);
+
+			amount.withdraw(account);
+			Some(contract_info)
+		}
 	}
-
-	(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) -> Option<ContractInfo<T>> {
-	try_evict_or_and_pay_rent::<T>(account, Zero::zero(), true).1
+/// NOTE this function performs eviction eagerly. All changes are read and written directly to
+/// storage.
+pub fn collect_rent<T: Trait>(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,
+		Some(ContractInfo::Alive(contract)) => contract,
+	};
+
+	let current_block_number = <frame_system::Module<T>>::block_number();
+	let verdict = consider_case::<T>(
+		account,
+		current_block_number,
+		Zero::zero(),
+		&alive_contract_info,
+	);
+	enact_verdict(account, alive_contract_info, current_block_number, verdict)
 }
 
-/// Evict the account if it should be evicted at the given block number.
+/// Process a snitch that a contract under the given address should be evicted.
 ///
-/// `handicap` gives a way to check or pay the rent up to a moment in the past instead
+/// Enact the eviction right away if the contract should be evicted and return true.
+/// Otherwise, **do nothing** and return false.
+///
+/// The `handicap` parameter gives a way to check the rent to a moment in the past instead
 /// of current block. E.g. if the contract is going to be evicted at the current block,
-/// `handicap=1` can defer the eviction for 1 block.
+/// `handicap = 1` can defer the eviction for 1 block. This is useful to handicap certain snitchers
+/// relative to others.
 ///
-/// 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).0
+/// NOTE this function performs eviction eagerly. All changes are read and written directly to
+/// storage.
+pub fn snitch_contract_should_be_evicted<T: Trait>(
+	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,
+		Some(ContractInfo::Alive(contract)) => contract,
+	};
+	let current_block_number = <frame_system::Module<T>>::block_number();
+	let verdict = consider_case::<T>(
+		account,
+		current_block_number,
+		handicap,
+		&alive_contract_info,
+	);
+
+	// Enact the verdict only if the contract gets removed.
+	match verdict {
+		Verdict::Kill | Verdict::Evict { .. } => {
+			enact_verdict(account, alive_contract_info, current_block_number, verdict);
+			true
+		}
+		_ => false,
+	}
 }