From ad1717293d9ea5f9267e581dd50188201dd23dd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Thu, 14 Jan 2021 13:45:13 +0100 Subject: [PATCH] contracts: Cap the surcharge reward by the amount of rent that way payed by a contract (#7870) * Add rent_payed field to the contract info * Don't pay out more as reward as was spent in rent * Make successful evictions free * Add tests to check that surcharge reward is capped by rent payed * review: Fixed docs --- .../frame/contracts/src/benchmarking/mod.rs | 12 ++- substrate/frame/contracts/src/lib.rs | 24 ++++-- substrate/frame/contracts/src/rent.rs | 29 ++++--- substrate/frame/contracts/src/storage.rs | 3 +- substrate/frame/contracts/src/tests.rs | 84 ++++++++++++++----- 5 files changed, 112 insertions(+), 40 deletions(-) diff --git a/substrate/frame/contracts/src/benchmarking/mod.rs b/substrate/frame/contracts/src/benchmarking/mod.rs index b5c0a0da13..c6fc3bf3ac 100644 --- a/substrate/frame/contracts/src/benchmarking/mod.rs +++ b/substrate/frame/contracts/src/benchmarking/mod.rs @@ -250,7 +250,7 @@ where /// Evict this contract. fn evict(&mut self) -> Result<(), &'static str> { self.set_block_num_for_eviction()?; - Rent::::snitch_contract_should_be_evicted(&self.contract.account_id, Zero::zero())?; + Rent::::try_eviction(&self.contract.account_id, Zero::zero())?; self.contract.ensure_tombstone() } } @@ -406,8 +406,14 @@ benchmarks! { instance.ensure_tombstone()?; // the caller should get the reward for being a good snitch - assert_eq!( - T::Currency::free_balance(&instance.caller), + // this is capped by the maximum amount of rent payed. So we only now that it should + // have increased by at most the surcharge reward. + assert!( + T::Currency::free_balance(&instance.caller) > + caller_funding::() - instance.endowment + ); + assert!( + T::Currency::free_balance(&instance.caller) < caller_funding::() - instance.endowment + ::SurchargeReward::get(), ); } diff --git a/substrate/frame/contracts/src/lib.rs b/substrate/frame/contracts/src/lib.rs index 1c191bfa04..d585ac4f7f 100644 --- a/substrate/frame/contracts/src/lib.rs +++ b/substrate/frame/contracts/src/lib.rs @@ -122,6 +122,7 @@ use frame_support::{ storage::child::ChildInfo, dispatch::{DispatchResult, DispatchResultWithPostInfo}, traits::{OnUnbalanced, Currency, Get, Time, Randomness}, + weights::Pays, }; use frame_system::{ensure_signed, ensure_root, Module as System}; use pallet_contracts_primitives::{ @@ -211,6 +212,10 @@ pub struct RawAliveContractInfo { pub code_hash: CodeHash, /// Pay rent at most up to this value. pub rent_allowance: Balance, + /// The amount of rent that was payed by the contract over its whole lifetime. + /// + /// A restored contract starts with a value of zero just like a new contract. + pub rent_payed: Balance, /// Last block rent has been payed. pub deduct_block: BlockNumber, /// Last block child storage has been written. @@ -602,8 +607,12 @@ decl_module! { gas_meter.into_dispatch_result(result) } - /// Allows block producers to claim a small reward for evicting a contract. If a block producer - /// fails to do so, a regular users will be allowed to claim the reward. + /// Allows block producers to claim a small reward for evicting a contract. If a block + /// producer fails to do so, a regular users will be allowed to claim the reward. + /// + /// In case of a successful eviction no fees are charged from the sender. However, the + /// reward is capped by the total amount of rent that was payed by the contract while + /// it was alive. /// /// If contract is not evicted as a result of this call, [`Error::ContractNotEvictable`] /// is returned and the sender is not eligible for the reward. @@ -612,7 +621,7 @@ decl_module! { origin, dest: T::AccountId, aux_sender: Option - ) -> DispatchResult { + ) -> DispatchResultWithPostInfo { let origin = origin.into(); let (signed, rewarded) = match (origin, aux_sender) { (Ok(frame_system::RawOrigin::Signed(account)), None) => { @@ -634,8 +643,13 @@ decl_module! { }; // If poking the contract has lead to eviction of the contract, give out the rewards. - if Rent::::snitch_contract_should_be_evicted(&dest, handicap)? { - T::Currency::deposit_into_existing(&rewarded, T::SurchargeReward::get()).map(|_| ()) + if let Some(rent_payed) = Rent::::try_eviction(&dest, handicap)? { + T::Currency::deposit_into_existing( + &rewarded, + T::SurchargeReward::get().min(rent_payed), + ) + .map(|_| Pays::No.into()) + .map_err(Into::into) } else { Err(Error::::ContractNotEvictable.into()) } diff --git a/substrate/frame/contracts/src/rent.rs b/substrate/frame/contracts/src/rent.rs index c67776c9e1..0bf229d494 100644 --- a/substrate/frame/contracts/src/rent.rs +++ b/substrate/frame/contracts/src/rent.rs @@ -142,7 +142,7 @@ where /// Consider the case for rent payment of the given account and returns a `Verdict`. /// /// Use `handicap` in case you want to change the reference block number. (To get more details see - /// `snitch_contract_should_be_evicted` ). + /// `try_eviction` ). fn consider_case( account: &T::AccountId, current_block_number: T::BlockNumber, @@ -268,6 +268,7 @@ where let contract_info = ContractInfo::Alive(AliveContractInfo:: { rent_allowance: alive_contract_info.rent_allowance - amount.peek(), deduct_block: current_block_number, + rent_payed: alive_contract_info.rent_payed.saturating_add(amount.peek()), ..alive_contract_info }); >::insert(account, &contract_info); @@ -280,7 +281,7 @@ where /// Make account paying the rent for the current block number /// /// 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 + /// contract is in need of eviction. [`try_eviction`] must /// be called to perform the eviction. pub fn charge(account: &T::AccountId) -> Result>, DispatchError> { let contract_info = >::get(account); @@ -301,8 +302,9 @@ where /// Process a report that a contract under the given address should be evicted. /// - /// Enact the eviction right away if the contract should be evicted and return true. - /// Otherwise, **do nothing** and return false. + /// Enact the eviction right away if the contract should be evicted and return the amount + /// of rent that the contract payed over its lifetime. + /// Otherwise, **do nothing** and return None. /// /// 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, @@ -311,13 +313,13 @@ where /// /// NOTE this function performs eviction eagerly. All changes are read and written directly to /// storage. - pub fn snitch_contract_should_be_evicted( + pub fn try_eviction( account: &T::AccountId, handicap: T::BlockNumber, - ) -> Result { + ) -> Result>, DispatchError> { let contract = >::get(account); let contract = match contract { - None | Some(ContractInfo::Tombstone(_)) => return Ok(false), + None | Some(ContractInfo::Tombstone(_)) => return Ok(None), Some(ContractInfo::Alive(contract)) => contract, }; let current_block_number = >::block_number(); @@ -330,11 +332,17 @@ where // Enact the verdict only if the contract gets removed. match verdict { - Verdict::Evict { .. } => { + Verdict::Evict { ref amount } => { + // The outstanding `amount` is withdrawn inside `enact_verdict`. + let rent_payed = amount + .as_ref() + .map(|a| a.peek()) + .unwrap_or_else(|| >::zero()) + .saturating_add(contract.rent_payed); Self::enact_verdict(account, contract, current_block_number, verdict, true)?; - Ok(true) + Ok(Some(rent_payed)) } - _ => Ok(false), + _ => Ok(None), } } @@ -481,6 +489,7 @@ where pair_count: origin_contract.pair_count, code_hash, rent_allowance, + rent_payed: >::zero(), deduct_block: current_block, last_write, })); diff --git a/substrate/frame/contracts/src/storage.rs b/substrate/frame/contracts/src/storage.rs index 0d4393aa96..030f62fc40 100644 --- a/substrate/frame/contracts/src/storage.rs +++ b/substrate/frame/contracts/src/storage.rs @@ -27,7 +27,7 @@ use codec::{Encode, Decode}; use sp_std::prelude::*; use sp_std::marker::PhantomData; use sp_io::hashing::blake2_256; -use sp_runtime::traits::{Bounded, Saturating}; +use sp_runtime::traits::{Bounded, Saturating, Zero}; use sp_core::crypto::UncheckedFrom; use frame_support::{ dispatch::DispatchResult, @@ -181,6 +181,7 @@ where // charge rent for it during instantation. >::block_number().saturating_sub(1u32.into()), rent_allowance: >::max_value(), + rent_payed: >::zero(), pair_count: 0, last_write: None, } diff --git a/substrate/frame/contracts/src/tests.rs b/substrate/frame/contracts/src/tests.rs index 78b1f7e30f..965cb7e49a 100644 --- a/substrate/frame/contracts/src/tests.rs +++ b/substrate/frame/contracts/src/tests.rs @@ -245,7 +245,7 @@ parameter_types! { pub const DepositPerStorageByte: u64 = 10_000; pub const DepositPerStorageItem: u64 = 10_000; pub RentFraction: Perbill = Perbill::from_rational_approximation(4u32, 10_000u32); - pub const SurchargeReward: u64 = 150; + pub const SurchargeReward: u64 = 500_000; pub const MaxDepth: u32 = 100; pub const MaxValueSize: u32 = 16_384; pub const DeletionQueueDepth: u32 = 1024; @@ -392,6 +392,7 @@ fn account_removal_does_not_remove_storage() { deduct_block: System::block_number(), code_hash: H256::repeat_byte(1), rent_allowance: 40, + rent_payed: 0, last_write: None, }); let _ = Balances::deposit_creating(&ALICE, 110); @@ -406,6 +407,7 @@ fn account_removal_does_not_remove_storage() { deduct_block: System::block_number(), code_hash: H256::repeat_byte(2), rent_allowance: 40, + rent_payed: 0, last_write: None, }); let _ = Balances::deposit_creating(&BOB, 110); @@ -2506,24 +2508,64 @@ fn not_deployed_if_endowment_too_low_for_first_rent() { // blocks to rent * 1; - ExtBuilder::default() - .existential_deposit(50) - .build() - .execute_with(|| { - // Create - let _ = Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contracts::put_code(Origin::signed(ALICE), wasm)); - assert_storage_noop!(assert_err_ignore_postinfo!(Contracts::instantiate( - Origin::signed(ALICE), - 30_000, - GAS_LIMIT, code_hash.into(), - (BalanceOf::::from(first_rent) - BalanceOf::::from(1u32)) - .encode(), // rent allowance - vec![], - ), - Error::::NewContractNotFunded, - )); - let addr = Contracts::contract_address(&ALICE, &code_hash, &[]); - assert_matches!(ContractInfoOf::::get(&addr), None); - }); + ExtBuilder::default().existential_deposit(50).build().execute_with(|| { + // Create + let _ = Balances::deposit_creating(&ALICE, 1_000_000); + assert_ok!(Contracts::put_code(Origin::signed(ALICE), wasm)); + assert_storage_noop!(assert_err_ignore_postinfo!( + Contracts::instantiate( + Origin::signed(ALICE), + 30_000, + GAS_LIMIT, code_hash.into(), + (BalanceOf::::from(first_rent) - BalanceOf::::from(1u32)) + .encode(), // rent allowance + vec![], + ), + Error::::NewContractNotFunded, + )); + let addr = Contracts::contract_address(&ALICE, &code_hash, &[]); + assert_matches!(ContractInfoOf::::get(&addr), None); + }); +} + +#[test] +fn surcharge_reward_is_capped() { + let (wasm, code_hash) = compile_module::("set_rent").unwrap(); + ExtBuilder::default().existential_deposit(50).build().execute_with(|| { + let _ = Balances::deposit_creating(&ALICE, 1_000_000); + assert_ok!(Contracts::put_code(Origin::signed(ALICE), wasm)); + assert_ok!(Contracts::instantiate( + Origin::signed(ALICE), + 30_000, + GAS_LIMIT, code_hash.into(), + >::from(1_000u32).encode(), // rent allowance + vec![], + )); + let addr = Contracts::contract_address(&ALICE, &code_hash, &[]); + let contract = >::get(&addr).unwrap().get_alive().unwrap(); + let balance = Balances::free_balance(&ALICE); + let reward = ::SurchargeReward::get(); + + // some rent should have payed due to instantation + assert_ne!(contract.rent_payed, 0); + + // the reward should be parameterized sufficiently high to make this test useful + assert!(reward > contract.rent_payed); + + // make contract eligible for eviction + initialize_block(40); + + // this should have removed the contract + assert_ok!(Contracts::claim_surcharge(Origin::none(), addr.clone(), Some(ALICE))); + + // this reward does not take into account the last rent payment collected during eviction + let capped_reward = reward.min(contract.rent_payed); + + // this is smaller than the actual reward because it does not take into account the + // rent collected during eviction + assert!(Balances::free_balance(&ALICE) > balance + capped_reward); + + // the full reward is not payed out because of the cap introduced by rent_payed + assert!(Balances::free_balance(&ALICE) < balance + reward); + }); }