contracts: Lazy storage removal (#7740)

* Do not evict a contract from within a call stack

We don't want to trigger contract eviction automatically when
a contract is called. This is because those changes can be
reverted due to how storage transactions are used at the moment.
More Information:
paritytech/substrate#6439 (comment)

It can be re-introduced once the linked issue is resolved. In the meantime
`claim_surcharge` must be called to evict a contract.

* Lazily delete storage in on_initialize instead of when removing the contract

* Add missing documentation of new error

* Make Module::claim_surcharge public

It being the only dispatchable that is private is an oversight.

* review: Add final newline

* review: Simplify assert statement

* Add test that checks that partial remove of a contract works

* Premote warning to error

* Added missing docs for seal_terminate

* Lazy deletion should only take AVERAGE_ON_INITIALIZE_RATIO of the block

* Added informational about the lazy deletion throughput

* Avoid lazy deletion in case the block is already full

* Prevent queue decoding in case of an already full block

* Add test that checks that on_initialize honors block limits

Author: athei

src/benchmarking/mod.rs

@@ -39,7 +39,7 @@ use self::{
 use frame_benchmarking::{benchmarks, account, whitelisted_caller};
 use frame_system::{Module as System, RawOrigin};
 use parity_wasm::elements::{Instruction, ValueType, BlockType};
-use sp_runtime::traits::{Hash, Bounded};
+use sp_runtime::traits::{Hash, Bounded, Zero};
 use sp_std::{default::Default, convert::{TryInto}, vec::Vec, vec};
 use pallet_contracts_primitives::RentProjection;
 
@@ -209,37 +209,52 @@ where
 	}
 }
 
-/// A `Contract` that was evicted after accumulating some storage.
+/// A `Contract` that contains some storage items.
 ///
-/// This is used to benchmark contract resurrection.
-struct Tombstone<T: Config> {
+/// This is used to benchmark contract destruction and resurection. Those operations'
+/// weight depend on the amount of storage accumulated.
+struct ContractWithStorage<T: Config> {
 	/// The contract that was evicted.
 	contract: Contract<T>,
 	/// The storage the contract held when it was avicted.
 	storage: Vec<(StorageKey, Vec<u8>)>,
 }
 
-impl<T: Config> Tombstone<T>
+impl<T: Config> ContractWithStorage<T>
 where
 	T: Config,
 	T::AccountId: UncheckedFrom<T::Hash> + AsRef<[u8]>,
 {
-	/// Create and evict a new contract with the supplied storage item count and size each.
+	/// Same as [`Self::with_code`] but with dummy contract code.
 	fn new(stor_num: u32, stor_size: u32) -> Result<Self, &'static str> {
-		let contract = Contract::<T>::new(WasmModule::dummy(), vec![], Endow::CollectRent)?;
+		Self::with_code(WasmModule::dummy(), stor_num, stor_size)
+	}
+
+	/// Create and evict a new contract with the supplied storage item count and size each.
+	fn with_code(code: WasmModule<T>, stor_num: u32, stor_size: u32) -> Result<Self, &'static str> {
+		let contract = Contract::<T>::new(code, vec![], Endow::CollectRent)?;
 		let storage_items = create_storage::<T>(stor_num, stor_size)?;
 		contract.store(&storage_items)?;
-		System::<T>::set_block_number(
-			contract.eviction_at()? + T::SignedClaimHandicap::get() + 5u32.into()
-		);
-		Rent::<T>::collect(&contract.account_id);
-		contract.ensure_tombstone()?;
-
-		Ok(Tombstone {
+		Ok(Self {
 			contract,
 			storage: storage_items,
 		})
 	}
+
+	/// Increase the system block number so that this contract is eligible for eviction.
+	fn set_block_num_for_eviction(&self) -> Result<(), &'static str>  {
+		System::<T>::set_block_number(
+			self.contract.eviction_at()? + T::SignedClaimHandicap::get() + 5u32.into()
+		);
+		Ok(())
+	}
+
+	/// 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())?;
+		self.contract.ensure_tombstone()
+	}
 }
 
 /// Generate `stor_num` storage items. Each has the size `stor_size`.
@@ -270,6 +285,30 @@ benchmarks! {
 	_ {
 	}
 
+	// The base weight without any actual work performed apart from the setup costs.
+	on_initialize {}: {
+		Storage::<T>::process_deletion_queue_batch(Weight::max_value())
+	}
+
+	on_initialize_per_trie_key {
+		let k in 0..1024;
+		let instance = ContractWithStorage::<T>::new(k, T::MaxValueSize::get())?;
+		Storage::<T>::queue_trie_for_deletion(&instance.contract.alive_info()?)?;
+	}: {
+		Storage::<T>::process_deletion_queue_batch(Weight::max_value())
+	}
+
+	on_initialize_per_queue_item {
+		let q in 0..1024.min(T::DeletionQueueDepth::get());
+		for i in 0 .. q {
+			let instance = Contract::<T>::with_index(i, WasmModule::dummy(), vec![], Endow::Max)?;
+			Storage::<T>::queue_trie_for_deletion(&instance.alive_info()?)?;
+			ContractInfoOf::<T>::remove(instance.account_id);
+		}
+	}: {
+		Storage::<T>::process_deletion_queue_batch(Weight::max_value())
+	}
+
 	// This extrinsic is pretty much constant as it is only a simple setter.
 	update_schedule {
 		let schedule = Schedule {
@@ -650,7 +689,8 @@ benchmarks! {
 		// Restore just moves the trie id from origin to destination and therefore
 		// does not depend on the size of the destination contract. However, to not
 		// trigger any edge case we won't use an empty contract as destination.
-		let tombstone = Tombstone::<T>::new(10, T::MaxValueSize::get())?;
+		let mut tombstone = ContractWithStorage::<T>::new(10, T::MaxValueSize::get())?;
+		tombstone.evict()?;
 
 		let dest = tombstone.contract.account_id.encode();
 		let dest_len = dest.len();
@@ -723,7 +763,8 @@ benchmarks! {
 
 	seal_restore_to_per_delta {
 		let d in 0 .. API_BENCHMARK_BATCHES;
-		let tombstone = Tombstone::<T>::new(0, 0)?;
+		let mut tombstone = ContractWithStorage::<T>::new(0, 0)?;
+		tombstone.evict()?;
 		let delta = create_storage::<T>(d * API_BENCHMARK_BATCH_SIZE, T::MaxValueSize::get())?;
 
 		let dest = tombstone.contract.account_id.encode();
@@ -2368,7 +2409,20 @@ benchmarks! {
 	#[extra]
 	print_schedule {
 		#[cfg(feature = "std")]
-		println!("{:#?}", Schedule::<T>::default());
+		{
+			let weight_per_key = T::WeightInfo::on_initialize_per_trie_key(1) -
+				T::WeightInfo::on_initialize_per_trie_key(0);
+			let weight_per_queue_item = T::WeightInfo::on_initialize_per_queue_item(1) -
+				T::WeightInfo::on_initialize_per_queue_item(0);
+			let weight_limit = T::DeletionWeightLimit::get();
+			let queue_depth: u64 = T::DeletionQueueDepth::get().into();
+			println!("{:#?}", Schedule::<T>::default());
+			println!("###############################################");
+			println!("Lazy deletion throughput per block (empty queue, full queue): {}, {}",
+				weight_limit / weight_per_key,
+				(weight_limit - weight_per_queue_item * queue_depth) / weight_per_key,
+			);
+		}
 		#[cfg(not(feature = "std"))]
 		return Err("Run this bench with a native runtime in order to see the schedule.");
 	}: {}
@@ -2394,6 +2448,10 @@ mod tests {
 		}
 	}
 
+	create_test!(on_initialize);
+	create_test!(on_initialize_per_trie_key);
+	create_test!(on_initialize_per_queue_item);
+
 	create_test!(update_schedule);
 	create_test!(put_code);
 	create_test!(instantiate);

src/exec.rs

@@ -268,12 +268,12 @@ where
 			Err(Error::<T>::MaxCallDepthReached)?
 		}
 
-		// Assumption: `collect` doesn't collide with overlay because
-		// `collect` will be done on first call and destination contract and balance
-		// cannot be changed before the first call
-		// We do not allow 'calling' plain accounts. For transfering value
-		// `seal_transfer` must be used.
-		let contract = if let Some(ContractInfo::Alive(info)) = Rent::<T>::collect(&dest) {
+		// This charges the rent and denies access to a contract that is in need of
+		// eviction by returning `None`. We cannot evict eagerly here because those
+		// changes would be rolled back in case this contract is called by another
+		// contract.
+		// See: https://github.com/paritytech/substrate/issues/6439#issuecomment-648754324
+		let contract = if let Ok(Some(ContractInfo::Alive(info))) = Rent::<T>::charge(&dest) {
 			info
 		} else {
 			Err(Error::<T>::NotCallable)?
@@ -575,13 +575,16 @@ where
 			value,
 			self.ctx,
 		)?;
-		let self_trie_id = self.ctx.self_trie_id.as_ref().expect(
-			"this function is only invoked by in the context of a contract;\
-				a contract has a trie id;\
-				this can't be None; qed",
-		);
-		Storage::<T>::destroy_contract(&self_id, self_trie_id);
-		Ok(())
+		if let Some(ContractInfo::Alive(info)) = ContractInfoOf::<T>::take(&self_id) {
+			Storage::<T>::queue_trie_for_deletion(&info)?;
+			Ok(())
+		} else {
+			panic!(
+				"this function is only invoked by in the context of a contract;\
+				this contract is therefore alive;\
+				qed"
+			);
+		}
 	}
 
 	fn call(

src/lib.rs

@@ -123,7 +123,7 @@ use frame_support::{
 	dispatch::{DispatchResult, DispatchResultWithPostInfo},
 	traits::{OnUnbalanced, Currency, Get, Time, Randomness},
 };
-use frame_system::{ensure_signed, ensure_root};
+use frame_system::{ensure_signed, ensure_root, Module as System};
 use pallet_contracts_primitives::{
 	RentProjectionResult, GetStorageResult, ContractAccessError, ContractExecResult, ExecResult,
 };
@@ -325,6 +325,12 @@ pub trait Config: frame_system::Config {
 
 	/// Type that allows the runtime authors to add new host functions for a contract to call.
 	type ChainExtension: chain_extension::ChainExtension;
+
+	/// The maximum number of tries that can be queued for deletion.
+	type DeletionQueueDepth: Get<u32>;
+
+	/// The maximum amount of weight that can be consumed per block for lazy trie removal.
+	type DeletionWeightLimit: Get<Weight>;
 }
 
 decl_error! {
@@ -396,6 +402,17 @@ decl_error! {
 		/// in this error. Note that this usually  shouldn't happen as deploying such contracts
 		/// is rejected.
 		NoChainExtension,
+		/// Removal of a contract failed because the deletion queue is full.
+		///
+		/// This can happen when either calling [`Module::claim_surcharge`] or `seal_terminate`.
+		/// The queue is filled by deleting contracts and emptied by a fixed amount each block.
+		/// Trying again during another block is the only way to resolve this issue.
+		DeletionQueueFull,
+		/// A contract could not be evicted because it has enough balance to pay rent.
+		///
+		/// This can be returned from [`Module::claim_surcharge`] because the target
+		/// contract has enough balance to pay for its rent.
+		ContractNotEvictable,
 	}
 }
 
@@ -449,8 +466,24 @@ decl_module! {
 		/// The maximum size of a storage value in bytes. A reasonable default is 16 KiB.
 		const MaxValueSize: u32 = T::MaxValueSize::get();
 
+		/// The maximum number of tries that can be queued for deletion.
+		const DeletionQueueDepth: u32 = T::DeletionQueueDepth::get();
+
+		/// The maximum amount of weight that can be consumed per block for lazy trie removal.
+		const DeletionWeightLimit: Weight = T::DeletionWeightLimit::get();
+
 		fn deposit_event() = default;
 
+		fn on_initialize() -> Weight {
+			// We do not want to go above the block limit and rather avoid lazy deletion
+			// in that case. This should only happen on runtime upgrades.
+			let weight_limit = T::BlockWeights::get().max_block
+				.saturating_sub(System::<T>::block_weight().total())
+				.min(T::DeletionWeightLimit::get());
+			Storage::<T>::process_deletion_queue_batch(weight_limit)
+				.saturating_add(T::WeightInfo::on_initialize())
+		}
+
 		/// Updates the schedule for metering contracts.
 		///
 		/// The schedule must have a greater version than the stored schedule.
@@ -549,10 +582,14 @@ decl_module! {
 		/// 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.
 		///
-		/// If contract is not evicted as a result of this call, no actions are taken and
-		/// the sender is not eligible for the reward.
+		/// If contract is not evicted as a result of this call, [`Error::ContractNotEvictable`]
+		/// is returned and the sender is not eligible for the reward.
 		#[weight = T::WeightInfo::claim_surcharge()]
-		fn claim_surcharge(origin, dest: T::AccountId, aux_sender: Option<T::AccountId>) {
+		pub fn claim_surcharge(
+			origin,
+			dest: T::AccountId,
+			aux_sender: Option<T::AccountId>
+		) -> DispatchResult {
 			let origin = origin.into();
 			let (signed, rewarded) = match (origin, aux_sender) {
 				(Ok(frame_system::RawOrigin::Signed(account)), None) => {
@@ -574,8 +611,10 @@ 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())?;
+			if Rent::<T>::snitch_contract_should_be_evicted(&dest, handicap)? {
+				T::Currency::deposit_into_existing(&rewarded, T::SurchargeReward::get()).map(|_| ())
+			} else {
+				Err(Error::<T>::ContractNotEvictable.into())
 			}
 		}
 	}
@@ -733,6 +772,11 @@ decl_storage! {
 		///
 		/// TWOX-NOTE: SAFE since `AccountId` is a secure hash.
 		pub ContractInfoOf: map hasher(twox_64_concat) T::AccountId => Option<ContractInfo<T>>;
+		/// Evicted contracts that await child trie deletion.
+		///
+		/// Child trie deletion is a heavy operation depending on the amount of storage items
+		/// stored in said trie. Therefore this operation is performed lazily in `on_initialize`.
+		pub DeletionQueue: Vec<storage::DeletedContract>;
 	}
 }