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

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::<T>::snitch_contract_should_be_evicted(&self.contract.account_id, Zero::zero())?;
+		Rent::<T>::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::<T>() - instance.endowment
+		);
+		assert!(
+			T::Currency::free_balance(&instance.caller) <
 			caller_funding::<T>() - instance.endowment + <T as Config>::SurchargeReward::get(),
 		);
 	}

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<CodeHash, Balance, BlockNumber> {
 	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<T::AccountId>
-		) -> 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::<T>::snitch_contract_should_be_evicted(&dest, handicap)? {
-				T::Currency::deposit_into_existing(&rewarded, T::SurchargeReward::get()).map(|_| ())
+			if let Some(rent_payed) = Rent::<T>::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::<T>::ContractNotEvictable.into())
 			}

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::<T> {
 					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
 				});
 				<ContractInfoOf<T>>::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<Option<ContractInfo<T>>, DispatchError> {
 		let contract_info = <ContractInfoOf<T>>::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<bool, DispatchError> {
+	) -> Result<Option<BalanceOf<T>>, DispatchError> {
 		let contract = <ContractInfoOf<T>>::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 = <frame_system::Module<T>>::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(|| <BalanceOf<T>>::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: <BalanceOf<T>>::zero(),
 			deduct_block: current_block,
 			last_write,
 		}));

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.
 						<frame_system::Module<T>>::block_number().saturating_sub(1u32.into()),
 					rent_allowance: <BalanceOf<T>>::max_value(),
+					rent_payed: <BalanceOf<T>>::zero(),
 					pair_count: 0,
 					last_write: None,
 				}

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::<Test>::from(first_rent) - BalanceOf::<Test>::from(1u32))
-						.encode(), // rent allowance
-					vec![],
-				),
-				Error::<Test>::NewContractNotFunded,
-			));
-			let addr = Contracts::contract_address(&ALICE, &code_hash, &[]);
-			assert_matches!(ContractInfoOf::<Test>::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::<Test>::from(first_rent) - BalanceOf::<Test>::from(1u32))
+					.encode(), // rent allowance
+				vec![],
+			),
+			Error::<Test>::NewContractNotFunded,
+		));
+		let addr = Contracts::contract_address(&ALICE, &code_hash, &[]);
+		assert_matches!(ContractInfoOf::<Test>::get(&addr), None);
+	});
+}
+
+#[test]
+fn surcharge_reward_is_capped() {
+	let (wasm, code_hash) = compile_module::<Test>("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(),
+			<BalanceOf<Test>>::from(1_000u32).encode(), // rent allowance
+			vec![],
+		));
+		let addr = Contracts::contract_address(&ALICE, &code_hash, &[]);
+		let contract = <ContractInfoOf::<Test>>::get(&addr).unwrap().get_alive().unwrap();
+		let balance = Balances::free_balance(&ALICE);
+		let reward = <Test as Config>::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);
+	});
 }