pallet-contracts: State rent fixes (#6147)

* Don't store the storage size offset in the contract itself.

* Clean the AccountDb code a bit

* Use `storage_size: 0` when creating AliveContractInfo

* Count empty storage items.

* Update frame/contracts/src/account_db.rs

Co-authored-by: Nikolay Volf <nikvolf@gmail.com>

* Use more clear wording.

Co-authored-by: Alexander Theißen <athei@users.noreply.github.com>

* Change the order of decrement and increment for storage size

Co-authored-by: Nikolay Volf <nikvolf@gmail.com>
Co-authored-by: Alexander Theißen <athei@users.noreply.github.com>

Author: 3 people

frame/contracts/fixtures/set_empty_storage.wat

@@ -0,0 +1,15 @@
+;; This module stores a KV pair into the storage
+(module
+	(import "env" "ext_set_storage" (func $ext_set_storage (param i32 i32 i32)))
+	(import "env" "memory" (memory 16 16))
+
+	(func (export "call")
+	)
+	(func (export "deploy")
+		(call $ext_set_storage
+			(i32.const 0)				;; Pointer to storage key
+			(i32.const 0)				;; Pointer to value
+			(i32.load (i32.const 0))	;; Size of value
+		)
+	)
+)

frame/contracts/src/account_db.rs

@@ -26,7 +26,7 @@ use sp_std::collections::btree_map::{BTreeMap, Entry};
 use sp_std::prelude::*;
 use sp_io::hashing::blake2_256;
 use sp_runtime::traits::{Bounded, Zero};
-use frame_support::traits::{Currency, Get, Imbalance, SignedImbalance};
+use frame_support::traits::{Currency, Imbalance, SignedImbalance};
 use frame_support::{storage::child, StorageMap};
 use frame_system;
 
@@ -108,7 +108,12 @@ pub trait AccountDb<T: Trait> {
 	///
 	/// Trie id is None iff account doesn't have an associated trie id in <ContractInfoOf<T>>.
 	/// Because DirectAccountDb bypass the lookup for this association.
-	fn get_storage(&self, account: &T::AccountId, trie_id: Option<&TrieId>, location: &StorageKey) -> Option<Vec<u8>>;
+	fn get_storage(
+		&self,
+		account: &T::AccountId,
+		trie_id: Option<&TrieId>,
+		location: &StorageKey,
+	) -> Option<Vec<u8>>;
 	/// If account has an alive contract then return the code hash associated.
 	fn get_code_hash(&self, account: &T::AccountId) -> Option<CodeHash<T>>;
 	/// If account has an alive contract then return the rent allowance associated.
@@ -126,9 +131,10 @@ impl<T: Trait> AccountDb<T> for DirectAccountDb {
 		&self,
 		_account: &T::AccountId,
 		trie_id: Option<&TrieId>,
-		location: &StorageKey
+		location: &StorageKey,
 	) -> Option<Vec<u8>> {
-		trie_id.and_then(|id| child::get_raw(&crate::child_trie_info(&id[..]), &blake2_256(location)))
+		trie_id
+			.and_then(|id| child::get_raw(&crate::child_trie_info(&id[..]), &blake2_256(location)))
 	}
 	fn get_code_hash(&self, account: &T::AccountId) -> Option<CodeHash<T>> {
 		<ContractInfoOf<T>>::get(account).and_then(|i| i.as_alive().map(|i| i.code_hash))
@@ -176,24 +182,26 @@ impl<T: Trait> AccountDb<T> for DirectAccountDb {
 						child::kill_storage(&info.child_trie_info());
 						AliveContractInfo::<T> {
 							code_hash,
-							storage_size: T::StorageSizeOffset::get(),
+							storage_size: 0,
+							empty_pair_count: 0,
+							total_pair_count: 0,
 							trie_id: <T as Trait>::TrieIdGenerator::trie_id(&address),
 							deduct_block: <frame_system::Module<T>>::block_number(),
 							rent_allowance: <BalanceOf<T>>::max_value(),
 							last_write: None,
 						}
 					}
 					// New contract is being instantiated.
-					(_, None, Some(code_hash)) => {
-						AliveContractInfo::<T> {
-							code_hash,
-							storage_size: T::StorageSizeOffset::get(),
-							trie_id: <T as Trait>::TrieIdGenerator::trie_id(&address),
-							deduct_block: <frame_system::Module<T>>::block_number(),
-							rent_allowance: <BalanceOf<T>>::max_value(),
-							last_write: None,
-						}
-					}
+					(_, None, Some(code_hash)) => AliveContractInfo::<T> {
+						code_hash,
+						storage_size: 0,
+						empty_pair_count: 0,
+						total_pair_count: 0,
+						trie_id: <T as Trait>::TrieIdGenerator::trie_id(&address),
+						deduct_block: <frame_system::Module<T>>::block_number(),
+						rent_allowance: <BalanceOf<T>>::max_value(),
+						last_write: None,
+					},
 					// There is no existing at the address nor a new one to be instantiated.
 					(_, None, None) => continue,
 				};
@@ -210,18 +218,69 @@ impl<T: Trait> AccountDb<T> for DirectAccountDb {
 					new_info.last_write = Some(<frame_system::Module<T>>::block_number());
 				}
 
-				for (k, v) in changed.storage.into_iter() {
-					if let Some(value) = child::get_raw(
-						&new_info.child_trie_info(),
-						&blake2_256(&k),
-					) {
-						new_info.storage_size -= value.len() as u32;
+				// NB: this call allocates internally. To keep allocations to the minimum we cache
+				// the child trie info here.
+				let child_trie_info = new_info.child_trie_info();
+
+				// Here we iterate over all storage key-value pairs that were changed throughout the
+				// execution of a contract and apply them to the substrate storage.
+				for (key, opt_new_value) in changed.storage.into_iter() {
+					let hashed_key = blake2_256(&key);
+
+					// In order to correctly update the book keeping we need to fetch the previous
+					// value of the key-value pair.
+					//
+					// It might be a bit more clean if we had an API that supported getting the size
+					// of the value without going through the loading of it. But at the moment of
+					// writing, there is no such API.
+					//
+					// That's not a show stopper in any case, since the performance cost is
+					// dominated by the trie traversal anyway.
+					let opt_prev_value = child::get_raw(&child_trie_info, &hashed_key);
+
+					// Update the total number of KV pairs and the number of empty pairs.
+					match (&opt_prev_value, &opt_new_value) {
+						(Some(prev_value), None) => {
+							new_info.total_pair_count -= 1;
+							if prev_value.is_empty() {
+								new_info.empty_pair_count -= 1;
+							}
+						},
+						(None, Some(new_value)) => {
+							new_info.total_pair_count += 1;
+							if new_value.is_empty() {
+								new_info.empty_pair_count += 1;
+							}
+						},
+						(Some(prev_value), Some(new_value)) => {
+							if prev_value.is_empty() {
+								new_info.empty_pair_count -= 1;
+							}
+							if new_value.is_empty() {
+								new_info.empty_pair_count += 1;
+							}
+						}
+						(None, None) => {}
 					}
-					if let Some(value) = v {
-						new_info.storage_size += value.len() as u32;
-						child::put_raw(&new_info.child_trie_info(), &blake2_256(&k), &value[..]);
-					} else {
-						child::kill(&new_info.child_trie_info(), &blake2_256(&k));
+
+					// Update the total storage size.
+					let prev_value_len = opt_prev_value
+						.as_ref()
+						.map(|old_value| old_value.len() as u32)
+						.unwrap_or(0);
+					let new_value_len = opt_new_value
+						.as_ref()
+						.map(|new_value| new_value.len() as u32)
+						.unwrap_or(0);
+					new_info.storage_size = new_info
+						.storage_size
+						.saturating_add(new_value_len)
+						.saturating_sub(prev_value_len);
+
+					// Finally, perform the change on the storage.
+					match opt_new_value {
+						Some(new_value) => child::put_raw(&child_trie_info, &hashed_key, &new_value[..]),
+						None => child::kill(&child_trie_info, &hashed_key),
 					}
 				}
 
@@ -239,12 +298,14 @@ impl<T: Trait> AccountDb<T> for DirectAccountDb {
 			// then it's indicative of a buggy contracts system.
 			// Panicking is far from ideal as it opens up a DoS attack on block validators, however
 			// it's a less bad option than allowing arbitrary value to be created.
-			SignedImbalance::Positive(ref p) if !p.peek().is_zero() =>
-				panic!("contract subsystem resulting in positive imbalance!"),
+			SignedImbalance::Positive(ref p) if !p.peek().is_zero() => {
+				panic!("contract subsystem resulting in positive imbalance!")
+			}
 			_ => {}
 		}
 	}
 }
+
 pub struct OverlayAccountDb<'a, T: Trait + 'a> {
 	local: RefCell<ChangeSet<T>>,
 	underlying: &'a dyn AccountDb<T>,
@@ -267,7 +328,8 @@ impl<'a, T: Trait> OverlayAccountDb<'a, T> {
 		location: StorageKey,
 		value: Option<Vec<u8>>,
 	) {
-		self.local.borrow_mut()
+		self.local
+			.borrow_mut()
 			.entry(account.clone())
 			.or_insert(Default::default())
 			.storage
@@ -285,7 +347,7 @@ impl<'a, T: Trait> OverlayAccountDb<'a, T> {
 		}
 
 		let mut local = self.local.borrow_mut();
-		let contract = local.entry(account.clone()).or_insert_with(|| Default::default());
+		let contract = local.entry(account.clone()).or_default();
 
 		contract.code_hash = Some(code_hash);
 		contract.rent_allowance = Some(<BalanceOf<T>>::max_value());
@@ -301,7 +363,7 @@ impl<'a, T: Trait> OverlayAccountDb<'a, T> {
 			ChangeEntry {
 				reset: true,
 				..Default::default()
-			}
+			},
 		);
 	}
 
@@ -327,7 +389,7 @@ impl<'a, T: Trait> AccountDb<T> for OverlayAccountDb<'a, T> {
 		&self,
 		account: &T::AccountId,
 		trie_id: Option<&TrieId>,
-		location: &StorageKey
+		location: &StorageKey,
 	) -> Option<Vec<u8>> {
 		self.local
 			.borrow()

frame/contracts/src/lib.rs

@@ -203,8 +203,15 @@ pub type AliveContractInfo<T> =
 pub struct RawAliveContractInfo<CodeHash, Balance, BlockNumber> {
 	/// Unique ID for the subtree encoded as a bytes vector.
 	pub trie_id: TrieId,
-	/// The size of stored value in octet.
+	/// The total number of bytes used by this contract.
+	///
+	/// It is a sum of each key-value pair stored by this contract.
 	pub storage_size: u32,
+	/// The number of key-value pairs that have values of zero length.
+	/// The condition `empty_pair_count ≤ total_pair_count` always holds.
+	pub empty_pair_count: u32,
+	/// The total number of key-value pairs in storage of this contract.
+	pub total_pair_count: u32,
 	/// The code associated with a given account.
 	pub code_hash: CodeHash,
 	/// Pay rent at most up to this value.
@@ -338,8 +345,11 @@ pub trait Trait: frame_system::Trait + pallet_transaction_payment::Trait {
 	/// The minimum amount required to generate a tombstone.
 	type TombstoneDeposit: Get<BalanceOf<Self>>;
 
-	/// Size of a contract at the time of instantiation. This is a simple way to ensure
-	/// that empty contracts eventually gets deleted.
+	/// A size offset for an contract. A just created account with untouched storage will have that
+	/// much of storage from the perspective of the state rent.
+	///
+	/// This is a simple way to ensure that contracts with empty storage eventually get deleted by
+	/// making them pay rent. This creates an incentive to remove them early in order to save rent.
 	type StorageSizeOffset: Get<u32>;
 
 	/// Price of a byte of storage per one block interval. Should be greater than 0.
@@ -420,8 +430,12 @@ decl_module! {
 		/// The minimum amount required to generate a tombstone.
 		const TombstoneDeposit: BalanceOf<T> = T::TombstoneDeposit::get();
 
-		/// Size of a contract at the time of instantiation. This is a simple way to ensure that
-		/// empty contracts eventually gets deleted.
+		/// A size offset for an contract. A just created account with untouched storage will have that
+		/// much of storage from the perspective of the state rent.
+		///
+		/// This is a simple way to ensure that contracts with empty storage eventually get deleted
+		/// by making them pay rent. This creates an incentive to remove them early in order to save
+		/// rent.
 		const StorageSizeOffset: u32 = T::StorageSizeOffset::get();
 
 		/// Price of a byte of storage per one block interval. Should be greater than 0.
@@ -697,7 +711,7 @@ impl<T: Trait> Module<T> {
 		dest: T::AccountId,
 		code_hash: CodeHash<T>,
 		rent_allowance: BalanceOf<T>,
-		delta: Vec<exec::StorageKey>
+		delta: Vec<exec::StorageKey>,
 	) -> DispatchResult {
 		let mut origin_contract = <ContractInfoOf<T>>::get(&origin)
 			.and_then(|c| c.get_alive())
@@ -764,6 +778,8 @@ impl<T: Trait> Module<T> {
 		<ContractInfoOf<T>>::insert(&dest, ContractInfo::Alive(RawAliveContractInfo {
 			trie_id: origin_contract.trie_id,
 			storage_size: origin_contract.storage_size,
+			empty_pair_count: origin_contract.empty_pair_count,
+			total_pair_count: origin_contract.total_pair_count,
 			code_hash,
 			rent_allowance,
 			deduct_block: current_block,

frame/contracts/src/rent.rs

@@ -92,8 +92,13 @@ fn compute_fee_per_block<T: Trait>(
 		.checked_div(&T::RentDepositOffset::get())
 		.unwrap_or_else(Zero::zero);
 
-	let effective_storage_size =
-		<BalanceOf<T>>::from(contract.storage_size).saturating_sub(free_storage);
+	// For now, we treat every empty KV pair as if it was one byte long.
+	let empty_pairs_equivalent = contract.empty_pair_count;
+
+	let effective_storage_size = <BalanceOf<T>>::from(
+		contract.storage_size + T::StorageSizeOffset::get() + empty_pairs_equivalent,
+	)
+	.saturating_sub(free_storage);
 
 	effective_storage_size
 		.checked_mul(&T::RentByteFee::get())