Contracts pallet: removal on idle (#11202)

* on_initialize -> on_idle

* use remaining_weight info

* no weight_limit for on_idle

* call on_idle in tests

* attempt to fix tests

* run on_initiaize when queue full

* add on_idle to weight info

* add on_idle weight info to on_idle hook

* add basic test for on_initialize with full queue

* disbale check for all keys gone in full queue, full block test

* queue_deth as usize, add comment

* comment was removed by accident

* Update frame/contracts/src/lib.rs

Co-authored-by: Alexander Theißen <alex.theissen@me.com>

* cargo +nightly fmt

* update lazy_removal_does_no_run_on_full_queue_and_full_block

* remove changes in weights.rs

* weights on_idle -> on_process_deletion_queue_batch

* use block number for on_idle

* use BlockNumber for on_initialize

* cargo run --quiet --profile=production  --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark pallet --chain=dev --steps=50 --repeat=20 --pallet=pallet_contracts --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/contracts/src/weights.rs --template=./.maintain/frame-weight-template.hbs

* Update frame/contracts/src/lib.rs

Co-authored-by: Alexander Theißen <alex.theissen@me.com>

* remove outcommented code

* add check that queue still full for test

* cargo fmt

* cargo +nightly fmt

* Update frame/contracts/src/benchmarking/mod.rs

Co-authored-by: Alexander Gryaznov <hi@agryaznov.com>

* fix weights.rs

* add lazy_removal_does_no_run_on_low_remaining_weight test

* Apply suggestions from code review

Co-authored-by: Alexander Gryaznov <hi@agryaznov.com>

Co-authored-by: Alexander Theißen <alex.theissen@me.com>
Co-authored-by: Parity Bot <admin@parity.io>
Co-authored-by: Alexander Gryaznov <hi@agryaznov.com>

Author: 4 people

frame/contracts/src/benchmarking/mod.rs

@@ -202,8 +202,8 @@ benchmarks! {
 		<BalanceOf<T> as codec::HasCompact>::Type: Clone + Eq + PartialEq + sp_std::fmt::Debug + scale_info::TypeInfo + codec::Encode,
 	}
 
-	// The base weight without any actual work performed apart from the setup costs.
-	on_initialize {}: {
+	// The base weight consumed on processing contracts deletion queue.
+	on_process_deletion_queue_batch {}: {
 		Storage::<T>::process_deletion_queue_batch(Weight::MAX)
 	}
 

frame/contracts/src/lib.rs

@@ -380,15 +380,28 @@ pub mod pallet {
 		T::AccountId: UncheckedFrom<T::Hash>,
 		T::AccountId: AsRef<[u8]>,
 	{
+		fn on_idle(_block: T::BlockNumber, remaining_weight: Weight) -> Weight {
+			Storage::<T>::process_deletion_queue_batch(remaining_weight)
+				.saturating_add(T::WeightInfo::on_process_deletion_queue_batch())
+		}
+
 		fn on_initialize(_block: T::BlockNumber) -> 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())
+			// We want to process the deletion_queue in the on_idle hook. Only in the case
+			// that the queue length has reached its maximal depth, we process it here.
+			let max_len = T::DeletionQueueDepth::get() as usize;
+			let queue_len = <DeletionQueue<T>>::decode_len().unwrap_or(0);
+			if queue_len >= max_len {
+				// 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_process_deletion_queue_batch())
+			} else {
+				T::WeightInfo::on_process_deletion_queue_batch()
+			}
 		}
 	}
 

frame/contracts/src/storage.rs

@@ -224,7 +224,7 @@ where
 	/// of those keys can be deleted from the deletion queue given the supplied queue length
 	/// and weight limit.
 	pub fn deletion_budget(queue_len: usize, weight_limit: Weight) -> (u64, u32) {
-		let base_weight = T::WeightInfo::on_initialize();
+		let base_weight = T::WeightInfo::on_process_deletion_queue_batch();
 		let weight_per_queue_item = T::WeightInfo::on_initialize_per_queue_item(1) -
 			T::WeightInfo::on_initialize_per_queue_item(0);
 		let weight_per_key = T::WeightInfo::on_initialize_per_trie_key(1) -

frame/contracts/src/tests.rs

@@ -25,7 +25,7 @@ use crate::{
 	wasm::{PrefabWasmModule, ReturnCode as RuntimeReturnCode},
 	weights::WeightInfo,
 	BalanceOf, Code, CodeStorage, Config, ContractInfoOf, DefaultAddressGenerator,
-	DefaultContractAccessWeight, Error, Pallet, Schedule,
+	DefaultContractAccessWeight, DeletionQueue, Error, Pallet, Schedule,
 };
 use assert_matches::assert_matches;
 use codec::Encode;
@@ -35,7 +35,8 @@ use frame_support::{
 	parameter_types,
 	storage::child,
 	traits::{
-		BalanceStatus, ConstU32, ConstU64, Contains, Currency, OnInitialize, ReservableCurrency,
+		BalanceStatus, ConstU32, ConstU64, Contains, Currency, Get, OnIdle, OnInitialize,
+		ReservableCurrency,
 	},
 	weights::{constants::WEIGHT_PER_SECOND, DispatchClass, PostDispatchInfo, Weight},
 };
@@ -1610,13 +1611,32 @@ fn lazy_removal_works() {
 		assert_matches!(child::get(trie, &[99]), Some(42));
 
 		// Run the lazy removal
-		Contracts::on_initialize(Weight::max_value());
+		Contracts::on_idle(System::block_number(), Weight::max_value());
 
 		// Value should be gone now
 		assert_matches!(child::get::<i32>(trie, &[99]), None);
 	});
 }
 
+#[test]
+fn lazy_removal_on_full_queue_works_on_initialize() {
+	ExtBuilder::default().existential_deposit(50).build().execute_with(|| {
+		// Fill the deletion queue with dummy values, so that on_initialize attempts
+		// to clear the queue
+		Storage::<Test>::fill_queue_with_dummies();
+
+		let queue_len_initial = <DeletionQueue<Test>>::decode_len().unwrap_or(0);
+
+		// Run the lazy removal
+		Contracts::on_initialize(System::block_number());
+
+		let queue_len_after_on_initialize = <DeletionQueue<Test>>::decode_len().unwrap_or(0);
+
+		// Queue length should be decreased after call of on_initialize()
+		assert!(queue_len_initial - queue_len_after_on_initialize > 0);
+	});
+}
+
 #[test]
 fn lazy_batch_removal_works() {
 	let (code, hash) = compile_module::<Test>("self_destruct").unwrap();
@@ -1661,7 +1681,7 @@ fn lazy_batch_removal_works() {
 		}
 
 		// Run single lazy removal
-		Contracts::on_initialize(Weight::max_value());
+		Contracts::on_idle(System::block_number(), Weight::max_value());
 
 		// The single lazy removal should have removed all queued tries
 		for trie in tries.iter() {
@@ -1761,7 +1781,34 @@ fn lazy_removal_partial_remove_works() {
 }
 
 #[test]
-fn lazy_removal_does_no_run_on_full_block() {
+fn lazy_removal_does_no_run_on_full_queue_and_full_block() {
+	ExtBuilder::default().existential_deposit(50).build().execute_with(|| {
+		// Fill up the block which should prevent the lazy storage removal from running.
+		System::register_extra_weight_unchecked(
+			<Test as system::Config>::BlockWeights::get().max_block,
+			DispatchClass::Mandatory,
+		);
+
+		// Fill the deletion queue with dummy values, so that on_initialize attempts
+		// to clear the queue
+		Storage::<Test>::fill_queue_with_dummies();
+
+		// Check that on_initialize() tries to perform lazy removal but removes nothing
+		//  as no more weight is left for that.
+		let weight_used = Contracts::on_initialize(System::block_number());
+		let base = <<Test as Config>::WeightInfo as WeightInfo>::on_process_deletion_queue_batch();
+		assert_eq!(weight_used, base);
+
+		// Check that the deletion queue is still full after execution of the
+		// on_initialize() hook.
+		let max_len: u32 = <Test as Config>::DeletionQueueDepth::get();
+		let queue_len: u32 = <DeletionQueue<Test>>::decode_len().unwrap_or(0).try_into().unwrap();
+		assert_eq!(max_len, queue_len);
+	});
+}
+
+#[test]
+fn lazy_removal_does_no_run_on_low_remaining_weight() {
 	let (code, hash) = compile_module::<Test>("self_destruct").unwrap();
 	ExtBuilder::default().existential_deposit(50).build().execute_with(|| {
 		let min_balance = <Test as Config>::Currency::minimum_balance();
@@ -1779,19 +1826,10 @@ fn lazy_removal_does_no_run_on_full_block() {
 
 		let addr = Contracts::contract_address(&ALICE, &hash, &[]);
 		let info = <ContractInfoOf<Test>>::get(&addr).unwrap();
-		let max_keys = 30;
-
-		// Create some storage items for the contract.
-		let vals: Vec<_> = (0..max_keys)
-			.map(|i| (blake2_256(&i.encode()), (i as u32), (i as u32).encode()))
-			.collect();
+		let trie = &info.child_trie_info();
 
 		// Put value into the contracts child trie
-		for val in &vals {
-			Storage::<Test>::write(&info.trie_id, &val.0, Some(val.2.clone()), None, false)
-				.unwrap();
-		}
-		<ContractInfoOf<Test>>::insert(&addr, info.clone());
+		child::put(trie, &[99], &42);
 
 		// Terminate the contract
 		assert_ok!(Contracts::call(
@@ -1806,37 +1844,30 @@ fn lazy_removal_does_no_run_on_full_block() {
 		// Contract info should be gone
 		assert!(!<ContractInfoOf::<Test>>::contains_key(&addr));
 
-		let trie = info.child_trie_info();
-
 		// But value should be still there as the lazy removal did not run, yet.
-		for val in &vals {
-			assert_eq!(child::get::<u32>(&trie, &blake2_256(&val.0)), Some(val.1));
-		}
+		assert_matches!(child::get(trie, &[99]), Some(42));
 
-		// Fill up the block which should prevent the lazy storage removal from running.
-		System::register_extra_weight_unchecked(
-			<Test as system::Config>::BlockWeights::get().max_block,
-			DispatchClass::Mandatory,
-		);
+		// Assign a remaining weight which is too low for a successfull deletion of the contract
+		let low_remaining_weight =
+			<<Test as Config>::WeightInfo as WeightInfo>::on_process_deletion_queue_batch();
 
-		// Run the lazy removal without any limit so that all keys would be removed if there
-		// had been some weight left in the block.
-		let weight_used = Contracts::on_initialize(Weight::max_value());
-		let base = <<Test as Config>::WeightInfo as WeightInfo>::on_initialize();
-		assert_eq!(weight_used, base);
+		// Run the lazy removal
+		Contracts::on_idle(System::block_number(), low_remaining_weight);
 
-		// All the keys are still in place
-		for val in &vals {
-			assert_eq!(child::get::<u32>(&trie, &blake2_256(&val.0)), Some(val.1));
-		}
+		// Value should still be there, since remaining weight was too low for removal
+		assert_matches!(child::get::<i32>(trie, &[99]), Some(42));
 
-		// Run the lazy removal directly which disregards the block limits
-		Storage::<Test>::process_deletion_queue_batch(Weight::max_value());
+		// Run the lazy removal while deletion_queue is not full
+		Contracts::on_initialize(System::block_number());
 
-		// Now the keys should be gone
-		for val in &vals {
-			assert_eq!(child::get::<u32>(&trie, &blake2_256(&val.0)), None);
-		}
+		// Value should still be there, since deletion_queue was not full
+		assert_matches!(child::get::<i32>(trie, &[99]), Some(42));
+
+		// Run on_idle with max remaining weight, this should remove the value
+		Contracts::on_idle(System::block_number(), Weight::max_value());
+
+		// Value should be gone
+		assert_matches!(child::get::<i32>(trie, &[99]), None);
 	});
 }