From f39335d63867798a5611ebee9010913a47286e40 Mon Sep 17 00:00:00 2001 From: Sergei Pepyakin Date: Mon, 27 Jan 2020 17:40:57 +0100 Subject: [PATCH] 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 * Update frame/contracts/src/exec.rs Co-Authored-By: Hero Bird Co-authored-by: thiolliere Co-authored-by: Hero Bird --- substrate/bin/node/runtime/src/lib.rs | 2 +- substrate/frame/contracts/src/exec.rs | 6 +- substrate/frame/contracts/src/lib.rs | 2 +- substrate/frame/contracts/src/rent.rs | 295 +++++++++++++++++--------- 4 files changed, 195 insertions(+), 110 deletions(-) diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index 23cd7405dd..7e1082ef54 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/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, }; diff --git a/substrate/frame/contracts/src/exec.rs b/substrate/frame/contracts/src/exec.rs index ceaccd35cb..cfbefa2a72 100644 --- a/substrate/frame/contracts/src/exec.rs +++ b/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::(&dest); + let contract_info = rent::collect_rent::(&dest); // Calls to dead contracts always fail. if let Some(ContractInfo::Tombstone(_)) = contract_info { diff --git a/substrate/frame/contracts/src/lib.rs b/substrate/frame/contracts/src/lib.rs index d16462d8bf..1460ca3cf1 100644 --- a/substrate/frame/contracts/src/lib.rs +++ b/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::(&dest, handicap) == rent::RentOutcome::Evicted { + if rent::snitch_contract_should_be_evicted::(&dest, handicap) { T::Currency::deposit_into_existing(&rewarded, T::SurchargeReward::get())?; } } diff --git a/substrate/frame/contracts/src/rent.rs b/substrate/frame/contracts/src/rent.rs index 508511da4c..46f915e642 100644 --- a/substrate/frame/contracts/src/rent.rs +++ b/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 . -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 { + amount: BalanceOf, } -/// Evict and optionally pay dues (or check account can pay them otherwise) at the current -/// block number (modulo `handicap`, read on). +impl OutstandingAmount { + /// Create the new outstanding amount. + /// + /// The amount should be always withdrawable and it should not kill the account. + fn new(amount: BalanceOf) -> Self { + Self { amount } + } + + /// Returns the amount this instance wraps. + fn peek(&self) -> BalanceOf { + 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 { + /// 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>, + }, + /// Everything is OK, we just only take some charge. + Charge { + amount: OutstandingAmount, + }, +} + +/// 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( +/// 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( account: &T::AccountId, + current_block_number: T::BlockNumber, handicap: T::BlockNumber, - pay_rent: bool, -) -> (RentOutcome, Option>) { - let contract_info = >::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 = >::block_number(); - + contract: &AliveContractInfo, +) -> Verdict { // 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( 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( if balance < subsistence_threshold { // The contract cannot afford to leave a tombstone, so remove the contract info altogether. - >::remove(account); - child::kill_storage(&contract.trie_id, contract.child_trie_unique_id()); - >::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( ) .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 = >::new( - &child_storage_root[..], - contract.code_hash, - ); - let tombstone_info = ContractInfo::Tombstone(tombstone); - >::insert(account, &tombstone_info); - - child::kill_storage(&contract.trie_id, contract.child_trie_unique_id()); - - >::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:: { - 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), + }; +} - >::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( + account: &T::AccountId, + alive_contract_info: AliveContractInfo, + current_block_number: T::BlockNumber, + verdict: Verdict, +) -> Option> { + match verdict { + Verdict::Exempt => return Some(ContractInfo::Alive(alive_contract_info)), + Verdict::Kill => { + >::remove(account); + child::kill_storage( + &alive_contract_info.trie_id, + alive_contract_info.child_trie_unique_id(), + ); + >::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 = >::new( + &child_storage_root[..], + alive_contract_info.code_hash, + ); + let tombstone_info = ContractInfo::Tombstone(tombstone); + >::insert(account, &tombstone_info); + + child::kill_storage( + &alive_contract_info.trie_id, + alive_contract_info.child_trie_unique_id(), + ); + + >::deposit_event(RawEvent::Evicted(account.clone(), true)); + Some(tombstone_info) + } + Verdict::Charge { amount } => { + let contract_info = ContractInfo::Alive(AliveContractInfo:: { + rent_allowance: alive_contract_info.rent_allowance - amount.peek(), + deduct_block: current_block_number, + ..alive_contract_info + }); + >::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(account: &T::AccountId) -> Option> { - try_evict_or_and_pay_rent::(account, Zero::zero(), true).1 +/// NOTE this function performs eviction eagerly. All changes are read and written directly to +/// storage. +pub fn collect_rent(account: &T::AccountId) -> Option> { + let contract_info = >::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 = >::block_number(); + let verdict = consider_case::( + 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(account: &T::AccountId, handicap: T::BlockNumber) -> RentOutcome { - try_evict_or_and_pay_rent::(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( + account: &T::AccountId, + handicap: T::BlockNumber, +) -> bool { + let contract_info = >::get(account); + let alive_contract_info = match contract_info { + None | Some(ContractInfo::Tombstone(_)) => return false, + Some(ContractInfo::Alive(contract)) => contract, + }; + let current_block_number = >::block_number(); + let verdict = consider_case::( + 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, + } }