From c8fe8b537803bb975e36100048c24bcb67a67f7b Mon Sep 17 00:00:00 2001 From: Sergei Pepyakin Date: Thu, 28 May 2020 20:07:25 +0200 Subject: [PATCH] pallet-contracts: State rent fixes (#6147) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * Use more clear wording. Co-authored-by: Alexander Theißen * Change the order of decrement and increment for storage size Co-authored-by: Nikolay Volf Co-authored-by: Alexander Theißen --- .../contracts/fixtures/set_empty_storage.wat | 15 +++ substrate/frame/contracts/src/account_db.rs | 126 +++++++++++++----- substrate/frame/contracts/src/lib.rs | 28 +++- substrate/frame/contracts/src/rent.rs | 9 +- substrate/frame/contracts/src/tests.rs | 78 ++++++++++- 5 files changed, 209 insertions(+), 47 deletions(-) create mode 100644 substrate/frame/contracts/fixtures/set_empty_storage.wat diff --git a/substrate/frame/contracts/fixtures/set_empty_storage.wat b/substrate/frame/contracts/fixtures/set_empty_storage.wat new file mode 100644 index 0000000000..d550eb2314 --- /dev/null +++ b/substrate/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 + ) + ) +) diff --git a/substrate/frame/contracts/src/account_db.rs b/substrate/frame/contracts/src/account_db.rs index aae853d2ff..5e1b0c34b5 100644 --- a/substrate/frame/contracts/src/account_db.rs +++ b/substrate/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 { /// /// Trie id is None iff account doesn't have an associated trie id in >. /// Because DirectAccountDb bypass the lookup for this association. - fn get_storage(&self, account: &T::AccountId, trie_id: Option<&TrieId>, location: &StorageKey) -> Option>; + fn get_storage( + &self, + account: &T::AccountId, + trie_id: Option<&TrieId>, + location: &StorageKey, + ) -> Option>; /// If account has an alive contract then return the code hash associated. fn get_code_hash(&self, account: &T::AccountId) -> Option>; /// If account has an alive contract then return the rent allowance associated. @@ -126,9 +131,10 @@ impl AccountDb for DirectAccountDb { &self, _account: &T::AccountId, trie_id: Option<&TrieId>, - location: &StorageKey + location: &StorageKey, ) -> Option> { - 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> { >::get(account).and_then(|i| i.as_alive().map(|i| i.code_hash)) @@ -176,7 +182,9 @@ impl AccountDb for DirectAccountDb { child::kill_storage(&info.child_trie_info()); AliveContractInfo:: { code_hash, - storage_size: T::StorageSizeOffset::get(), + storage_size: 0, + empty_pair_count: 0, + total_pair_count: 0, trie_id: ::TrieIdGenerator::trie_id(&address), deduct_block: >::block_number(), rent_allowance: >::max_value(), @@ -184,16 +192,16 @@ impl AccountDb for DirectAccountDb { } } // New contract is being instantiated. - (_, None, Some(code_hash)) => { - AliveContractInfo:: { - code_hash, - storage_size: T::StorageSizeOffset::get(), - trie_id: ::TrieIdGenerator::trie_id(&address), - deduct_block: >::block_number(), - rent_allowance: >::max_value(), - last_write: None, - } - } + (_, None, Some(code_hash)) => AliveContractInfo:: { + code_hash, + storage_size: 0, + empty_pair_count: 0, + total_pair_count: 0, + trie_id: ::TrieIdGenerator::trie_id(&address), + deduct_block: >::block_number(), + rent_allowance: >::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 AccountDb for DirectAccountDb { new_info.last_write = Some(>::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 AccountDb 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>, underlying: &'a dyn AccountDb, @@ -267,7 +328,8 @@ impl<'a, T: Trait> OverlayAccountDb<'a, T> { location: StorageKey, value: Option>, ) { - 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(>::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 for OverlayAccountDb<'a, T> { &self, account: &T::AccountId, trie_id: Option<&TrieId>, - location: &StorageKey + location: &StorageKey, ) -> Option> { self.local .borrow() diff --git a/substrate/frame/contracts/src/lib.rs b/substrate/frame/contracts/src/lib.rs index 509229cd96..8f90f557ca 100644 --- a/substrate/frame/contracts/src/lib.rs +++ b/substrate/frame/contracts/src/lib.rs @@ -203,8 +203,15 @@ pub type AliveContractInfo = pub struct RawAliveContractInfo { /// 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>; - /// 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; /// 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::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 Module { dest: T::AccountId, code_hash: CodeHash, rent_allowance: BalanceOf, - delta: Vec + delta: Vec, ) -> DispatchResult { let mut origin_contract = >::get(&origin) .and_then(|c| c.get_alive()) @@ -764,6 +778,8 @@ impl Module { >::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, diff --git a/substrate/frame/contracts/src/rent.rs b/substrate/frame/contracts/src/rent.rs index 1aa52fff31..1d8f474627 100644 --- a/substrate/frame/contracts/src/rent.rs +++ b/substrate/frame/contracts/src/rent.rs @@ -92,8 +92,13 @@ fn compute_fee_per_block( .checked_div(&T::RentDepositOffset::get()) .unwrap_or_else(Zero::zero); - let effective_storage_size = - >::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 = >::from( + contract.storage_size + T::StorageSizeOffset::get() + empty_pairs_equivalent, + ) + .saturating_sub(free_storage); effective_storage_size .checked_mul(&T::RentByteFee::get()) diff --git a/substrate/frame/contracts/src/tests.rs b/substrate/frame/contracts/src/tests.rs index 944bca622b..307b311889 100644 --- a/substrate/frame/contracts/src/tests.rs +++ b/substrate/frame/contracts/src/tests.rs @@ -290,7 +290,9 @@ fn account_removal_does_not_remove_storage() { let _ = Balances::deposit_creating(&1, 110); ContractInfoOf::::insert(1, &ContractInfo::Alive(RawAliveContractInfo { trie_id: trie_id1.clone(), - storage_size: ::StorageSizeOffset::get(), + storage_size: 0, + empty_pair_count: 0, + total_pair_count: 0, deduct_block: System::block_number(), code_hash: H256::repeat_byte(1), rent_allowance: 40, @@ -305,7 +307,9 @@ fn account_removal_does_not_remove_storage() { let _ = Balances::deposit_creating(&2, 110); ContractInfoOf::::insert(2, &ContractInfo::Alive(RawAliveContractInfo { trie_id: trie_id2.clone(), - storage_size: ::StorageSizeOffset::get(), + storage_size: 0, + empty_pair_count: 0, + total_pair_count: 0, deduct_block: System::block_number(), code_hash: H256::repeat_byte(2), rent_allowance: 40, @@ -752,7 +756,15 @@ fn storage_size() { .unwrap(); assert_eq!( bob_contract.storage_size, - ::StorageSizeOffset::get() + 4 + 4 + ); + assert_eq!( + bob_contract.total_pair_count, + 1, + ); + assert_eq!( + bob_contract.empty_pair_count, + 0, ); assert_ok!(Contracts::call( @@ -768,7 +780,15 @@ fn storage_size() { .unwrap(); assert_eq!( bob_contract.storage_size, - ::StorageSizeOffset::get() + 4 + 4 + 4 + 4 + ); + assert_eq!( + bob_contract.total_pair_count, + 2, + ); + assert_eq!( + bob_contract.empty_pair_count, + 0, ); assert_ok!(Contracts::call( @@ -784,7 +804,51 @@ fn storage_size() { .unwrap(); assert_eq!( bob_contract.storage_size, - ::StorageSizeOffset::get() + 4 + 4 + ); + assert_eq!( + bob_contract.total_pair_count, + 1, + ); + assert_eq!( + bob_contract.empty_pair_count, + 0, + ); + }); +} + +#[test] +fn empty_kv_pairs() { + let (wasm, code_hash) = compile_module::("set_empty_storage").unwrap(); + + ExtBuilder::default() + .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(), + vec![], + )); + let bob_contract = ContractInfoOf::::get(BOB) + .unwrap() + .get_alive() + .unwrap(); + + assert_eq!( + bob_contract.storage_size, + 0, + ); + assert_eq!( + bob_contract.total_pair_count, + 1, + ); + assert_eq!( + bob_contract.empty_pair_count, + 1, ); }); } @@ -1316,7 +1380,7 @@ fn restoration(test_different_storage: bool, test_restore_to_with_dirty_storage: assert!(ContractInfoOf::::get(BOB).unwrap().get_tombstone().is_some()); let django_contract = ContractInfoOf::::get(DJANGO).unwrap() .get_alive().unwrap(); - assert_eq!(django_contract.storage_size, 16); + assert_eq!(django_contract.storage_size, 8); assert_eq!(django_contract.trie_id, django_trie_id); assert_eq!(django_contract.deduct_block, System::block_number()); match (test_different_storage, test_restore_to_with_dirty_storage) { @@ -1390,7 +1454,7 @@ fn restoration(test_different_storage: bool, test_restore_to_with_dirty_storage: let bob_contract = ContractInfoOf::::get(BOB).unwrap() .get_alive().unwrap(); assert_eq!(bob_contract.rent_allowance, 50); - assert_eq!(bob_contract.storage_size, 12); + assert_eq!(bob_contract.storage_size, 4); assert_eq!(bob_contract.trie_id, django_trie_id); assert_eq!(bob_contract.deduct_block, System::block_number()); assert!(ContractInfoOf::::get(DJANGO).is_none());