From 643aa2be2a2c0611eeb648cfc21eb4cb3c1c9cd8 Mon Sep 17 00:00:00 2001
From: PG Herveou <pgherveou@gmail.com>
Date: Wed, 10 Apr 2024 22:32:53 +0200
Subject: [PATCH] Contracts: Remove ED from base deposit (#3536)
- Update internal logic so that the storage_base_deposit does not
include ED
- add v16 migration to update ContractInfo struct with this change
Before:
<img width="820" alt="Screenshot 2024-03-21 at 11 23 29"
src="https://github.com/paritytech/polkadot-sdk/assets/521091/a0a8df0d-e743-42c5-9e16-cf2ec1aa949c">
After:
---------
Co-authored-by: Cyrill Leutwiler <cyrill@parity.io>
Co-authored-by: command-bot <>
---
.../frame/contracts/src/benchmarking/mod.rs | 22 +++-
substrate/frame/contracts/src/lib.rs | 2 +-
substrate/frame/contracts/src/migration.rs | 1 +
.../frame/contracts/src/migration/v16.rs | 107 ++++++++++++++++++
substrate/frame/contracts/src/storage.rs | 13 +--
.../frame/contracts/src/storage/meter.rs | 27 ++---
substrate/frame/contracts/src/tests.rs | 21 ++--
substrate/frame/contracts/src/weights.rs | 23 ++++
8 files changed, 174 insertions(+), 42 deletions(-)
create mode 100644 substrate/frame/contracts/src/migration/v16.rs
diff --git a/substrate/frame/contracts/src/benchmarking/mod.rs b/substrate/frame/contracts/src/benchmarking/mod.rs
index 9e245c319b1..676fd320a17 100644
--- a/substrate/frame/contracts/src/benchmarking/mod.rs
+++ b/substrate/frame/contracts/src/benchmarking/mod.rs
@@ -32,7 +32,7 @@ use self::{
use crate::{
exec::Key,
migration::{
- codegen::LATEST_MIGRATION_VERSION, v09, v10, v11, v12, v13, v14, v15, MigrationStep,
+ codegen::LATEST_MIGRATION_VERSION, v09, v10, v11, v12, v13, v14, v15, v16, MigrationStep,
},
Pallet as Contracts, *,
};
@@ -331,6 +331,26 @@ mod benchmarks {
Ok(())
}
+ // This benchmarks the v16 migration step (Remove ED from base_deposit).
+ #[benchmark(pov_mode = Measured)]
+ fn v16_migration_step() -> Result<(), BenchmarkError> {
+ let contract =
+ <Contract<T>>::with_caller(whitelisted_caller(), WasmModule::dummy(), vec![])?;
+
+ let info = contract.info()?;
+ let base_deposit = v16::store_old_contract_info::<T>(contract.account_id.clone(), &info);
+ let mut m = v16::Migration::<T>::default();
+
+ #[block]
+ {
+ m.step(&mut WeightMeter::new());
+ }
+ let ed = Pallet::<T>::min_balance();
+ let info = v16::ContractInfoOf::<T>::get(&contract.account_id).unwrap();
+ assert_eq!(info.storage_base_deposit, base_deposit - ed);
+ Ok(())
+ }
+
// This benchmarks the weight of executing Migration::migrate to execute a noop migration.
#[benchmark(pov_mode = Measured)]
fn migration_noop() {
diff --git a/substrate/frame/contracts/src/lib.rs b/substrate/frame/contracts/src/lib.rs
index 9a1a0aada89..73c70a7704e 100644
--- a/substrate/frame/contracts/src/lib.rs
+++ b/substrate/frame/contracts/src/lib.rs
@@ -244,7 +244,7 @@ pub mod pallet {
use sp_runtime::Perbill;
/// The in-code storage version.
- pub(crate) const STORAGE_VERSION: StorageVersion = StorageVersion::new(15);
+ pub(crate) const STORAGE_VERSION: StorageVersion = StorageVersion::new(16);
#[pallet::pallet]
#[pallet::storage_version(STORAGE_VERSION)]
diff --git a/substrate/frame/contracts/src/migration.rs b/substrate/frame/contracts/src/migration.rs
index b0df4508285..c633ba9c2d5 100644
--- a/substrate/frame/contracts/src/migration.rs
+++ b/substrate/frame/contracts/src/migration.rs
@@ -64,6 +64,7 @@ pub mod v12;
pub mod v13;
pub mod v14;
pub mod v15;
+pub mod v16;
include!(concat!(env!("OUT_DIR"), "/migration_codegen.rs"));
use crate::{weights::WeightInfo, Config, Error, MigrationInProgress, Pallet, Weight, LOG_TARGET};
diff --git a/substrate/frame/contracts/src/migration/v16.rs b/substrate/frame/contracts/src/migration/v16.rs
new file mode 100644
index 00000000000..74fbc997718
--- /dev/null
+++ b/substrate/frame/contracts/src/migration/v16.rs
@@ -0,0 +1,107 @@
+// This file is part of Substrate.
+
+// Copyright (C) Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: Apache-2.0
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//! Remove ED from storage base deposit.
+//! See <https://github.com/paritytech/polkadot-sdk/pull/3536>.
+
+use crate::{
+ migration::{IsFinished, MigrationStep},
+ weights::WeightInfo,
+ BalanceOf, CodeHash, Config, Pallet, TrieId, Weight, WeightMeter, LOG_TARGET,
+};
+use codec::{Decode, Encode};
+use frame_support::{pallet_prelude::*, storage_alias, DefaultNoBound};
+use sp_runtime::{BoundedBTreeMap, Saturating};
+use sp_std::prelude::*;
+
+#[cfg(feature = "runtime-benchmarks")]
+pub fn store_old_contract_info<T: Config>(
+ account: T::AccountId,
+ info: &crate::ContractInfo<T>,
+) -> BalanceOf<T> {
+ let storage_base_deposit = Pallet::<T>::min_balance() + 1u32.into();
+ ContractInfoOf::<T>::insert(
+ account,
+ ContractInfo {
+ trie_id: info.trie_id.clone(),
+ code_hash: info.code_hash,
+ storage_bytes: Default::default(),
+ storage_items: Default::default(),
+ storage_byte_deposit: Default::default(),
+ storage_item_deposit: Default::default(),
+ storage_base_deposit,
+ delegate_dependencies: Default::default(),
+ },
+ );
+
+ storage_base_deposit
+}
+
+#[storage_alias]
+pub type ContractInfoOf<T: Config> =
+ StorageMap<Pallet<T>, Twox64Concat, <T as frame_system::Config>::AccountId, ContractInfo<T>>;
+
+#[derive(Encode, Decode, CloneNoBound, PartialEq, Eq, RuntimeDebug, TypeInfo, MaxEncodedLen)]
+#[scale_info(skip_type_params(T))]
+pub struct ContractInfo<T: Config> {
+ trie_id: TrieId,
+ code_hash: CodeHash<T>,
+ storage_bytes: u32,
+ storage_items: u32,
+ storage_byte_deposit: BalanceOf<T>,
+ storage_item_deposit: BalanceOf<T>,
+ pub storage_base_deposit: BalanceOf<T>,
+ delegate_dependencies: BoundedBTreeMap<CodeHash<T>, BalanceOf<T>, T::MaxDelegateDependencies>,
+}
+
+#[derive(Encode, Decode, MaxEncodedLen, DefaultNoBound)]
+pub struct Migration<T: Config> {
+ last_account: Option<T::AccountId>,
+}
+
+impl<T: Config> MigrationStep for Migration<T> {
+ const VERSION: u16 = 16;
+
+ fn max_step_weight() -> Weight {
+ T::WeightInfo::v16_migration_step()
+ }
+
+ fn step(&mut self, meter: &mut WeightMeter) -> IsFinished {
+ let mut iter = if let Some(last_account) = self.last_account.take() {
+ ContractInfoOf::<T>::iter_keys_from(ContractInfoOf::<T>::hashed_key_for(last_account))
+ } else {
+ ContractInfoOf::<T>::iter_keys()
+ };
+
+ if let Some(key) = iter.next() {
+ log::debug!(target: LOG_TARGET, "Migrating contract {:?}", key);
+ ContractInfoOf::<T>::mutate(key.clone(), |info| {
+ let ed = Pallet::<T>::min_balance();
+ let mut updated_info = info.take().expect("Item exists; qed");
+ updated_info.storage_base_deposit.saturating_reduce(ed);
+ *info = Some(updated_info);
+ });
+ self.last_account = Some(key);
+ meter.consume(T::WeightInfo::v16_migration_step());
+ IsFinished::No
+ } else {
+ log::debug!(target: LOG_TARGET, "No more contracts to migrate");
+ meter.consume(T::WeightInfo::v16_migration_step());
+ IsFinished::Yes
+ }
+ }
+}
diff --git a/substrate/frame/contracts/src/storage.rs b/substrate/frame/contracts/src/storage.rs
index fc6d514ebf4..1e9739a1599 100644
--- a/substrate/frame/contracts/src/storage.rs
+++ b/substrate/frame/contracts/src/storage.rs
@@ -23,7 +23,7 @@ use crate::{
exec::{AccountIdOf, Key},
weights::WeightInfo,
BalanceOf, CodeHash, CodeInfo, Config, ContractInfoOf, DeletionQueue, DeletionQueueCounter,
- Error, Pallet, TrieId, SENTINEL,
+ Error, TrieId, SENTINEL,
};
use codec::{Decode, Encode, MaxEncodedLen};
use frame_support::{
@@ -125,9 +125,7 @@ impl<T: Config> ContractInfo<T> {
/// Same as [`Self::extra_deposit`] but including the base deposit.
pub fn total_deposit(&self) -> BalanceOf<T> {
- self.extra_deposit()
- .saturating_add(self.storage_base_deposit)
- .saturating_sub(Pallet::<T>::min_balance())
+ self.extra_deposit().saturating_add(self.storage_base_deposit)
}
/// Returns the storage base deposit of the contract.
@@ -213,7 +211,6 @@ impl<T: Config> ContractInfo<T> {
/// The base deposit is updated when the `code_hash` of the contract changes, as it depends on
/// the deposit paid to upload the contract's code.
pub fn update_base_deposit(&mut self, code_info: &CodeInfo<T>) -> BalanceOf<T> {
- let ed = Pallet::<T>::min_balance();
let info_deposit =
Diff { bytes_added: self.encoded_size() as u32, items_added: 1, ..Default::default() }
.update_contract::<T>(None)
@@ -224,11 +221,7 @@ impl<T: Config> ContractInfo<T> {
// to prevent abuse.
let upload_deposit = T::CodeHashLockupDepositPercent::get().mul_ceil(code_info.deposit());
- // Instantiate needs to transfer at least the minimum balance in order to pull the
- // contract's own account into existence, as the deposit itself does not contribute to the
- // `ed`.
- let deposit = info_deposit.saturating_add(upload_deposit).saturating_add(ed);
-
+ let deposit = info_deposit.saturating_add(upload_deposit);
self.storage_base_deposit = deposit;
deposit
}
diff --git a/substrate/frame/contracts/src/storage/meter.rs b/substrate/frame/contracts/src/storage/meter.rs
index 495cbd90db5..5db9a772ad8 100644
--- a/substrate/frame/contracts/src/storage/meter.rs
+++ b/substrate/frame/contracts/src/storage/meter.rs
@@ -435,22 +435,12 @@ where
contract: &T::AccountId,
contract_info: &mut ContractInfo<T>,
code_info: &CodeInfo<T>,
- ) -> Result<DepositOf<T>, DispatchError> {
+ ) -> Result<(), DispatchError> {
debug_assert!(matches!(self.contract_state(), ContractState::Alive));
- let ed = Pallet::<T>::min_balance();
-
- let deposit = contract_info.update_base_deposit(&code_info);
- if deposit > self.limit {
- return Err(<Error<T>>::StorageDepositLimitExhausted.into())
- }
-
- let deposit = Deposit::Charge(deposit);
-
- // We do not increase `own_contribution` because this will be charged later when the
- // contract execution does conclude and hence would lead to a double charge.
- self.total_deposit = Deposit::Charge(ed);
// We need to make sure that the contract's account exists.
+ let ed = Pallet::<T>::min_balance();
+ self.total_deposit = Deposit::Charge(ed);
T::Currency::transfer(origin, contract, ed, Preservation::Preserve)?;
// A consumer is added at account creation and removed it on termination, otherwise the
@@ -458,9 +448,11 @@ where
// With the consumer, a correct runtime cannot remove the account.
System::<T>::inc_consumers(contract)?;
- self.charge_deposit(contract.clone(), deposit.saturating_sub(&Deposit::Charge(ed)));
+ let deposit = contract_info.update_base_deposit(&code_info);
+ let deposit = Deposit::Charge(deposit);
- Ok(deposit)
+ self.charge_deposit(contract.clone(), deposit);
+ Ok(())
}
/// Call to tell the meter that the currently executing contract was terminated.
@@ -859,14 +851,14 @@ mod tests {
let test_cases = vec![
ChargingTestCase {
origin: Origin::<Test>::from_account_id(ALICE),
- deposit: Deposit::Refund(107),
+ deposit: Deposit::Refund(108),
expected: TestExt {
limit_checks: vec![LimitCheck { origin: ALICE, limit: 1_000, min_leftover: 0 }],
charges: vec![
Charge {
origin: ALICE,
contract: CHARLIE,
- amount: Deposit::Refund(119),
+ amount: Deposit::Refund(120),
state: ContractState::Terminated { beneficiary: CHARLIE },
},
Charge {
@@ -915,7 +907,6 @@ mod tests {
meter.absorb(nested0, &BOB, None);
assert_eq!(meter.try_into_deposit(&test_case.origin).unwrap(), test_case.deposit);
-
assert_eq!(TestExtTestValue::get(), test_case.expected)
}
}
diff --git a/substrate/frame/contracts/src/tests.rs b/substrate/frame/contracts/src/tests.rs
index 7a910a6a8f9..0b83358a7f5 100644
--- a/substrate/frame/contracts/src/tests.rs
+++ b/substrate/frame/contracts/src/tests.rs
@@ -3817,7 +3817,7 @@ fn locking_delegate_dependency_works() {
&HoldReason::StorageDepositReserve.into(),
&addr_caller
),
- dependency_deposit + contract.storage_base_deposit() - ED
+ dependency_deposit + contract.storage_base_deposit()
);
// Removing the code should fail, since we have added a dependency.
@@ -3856,7 +3856,7 @@ fn locking_delegate_dependency_works() {
&HoldReason::StorageDepositReserve.into(),
&addr_caller
),
- contract.storage_base_deposit() - ED
+ contract.storage_base_deposit()
);
// Removing a nonexistent dependency should fail.
@@ -3888,7 +3888,7 @@ fn locking_delegate_dependency_works() {
assert_ok!(call(&addr_caller, &terminate_input).result);
assert_eq!(
test_utils::get_balance(&ALICE),
- balance_before + contract.storage_base_deposit() + dependency_deposit
+ ED + balance_before + contract.storage_base_deposit() + dependency_deposit
);
// Terminate should also remove the dependency, so we can remove the code.
@@ -3904,13 +3904,9 @@ fn native_dependency_deposit_works() {
// Set hash lock up deposit to 30%, to test deposit calculation.
CODE_HASH_LOCKUP_DEPOSIT_PERCENT.with(|c| *c.borrow_mut() = Perbill::from_percent(30));
- // Set a low existential deposit so that the base storage deposit is based on the contract
- // storage deposit rather than the existential deposit.
- const ED: u64 = 10;
-
// Test with both existing and uploaded code
for code in [Code::Upload(wasm.clone()), Code::Existing(code_hash)] {
- ExtBuilder::default().existential_deposit(ED).build().execute_with(|| {
+ ExtBuilder::default().build().execute_with(|| {
let _ = Balances::set_balance(&ALICE, 1_000_000);
let lockup_deposit_percent = CodeHashLockupDepositPercent::get();
@@ -3942,16 +3938,16 @@ fn native_dependency_deposit_works() {
let res = builder::bare_instantiate(code).build();
let addr = res.result.unwrap().account_id;
- let base_deposit = ED + test_utils::contract_info_storage_deposit(&addr);
+ let base_deposit = test_utils::contract_info_storage_deposit(&addr);
let upload_deposit = test_utils::get_code_deposit(&code_hash);
let extra_deposit = add_upload_deposit.then(|| upload_deposit).unwrap_or_default();
// Check initial storage_deposit
- // The base deposit should be: ED + contract_info_storage_deposit + 30% * deposit
+ // The base deposit should be: contract_info_storage_deposit + 30% * deposit
let deposit =
extra_deposit + base_deposit + lockup_deposit_percent.mul_ceil(upload_deposit);
- assert_eq!(res.storage_deposit.charge_or_zero(), deposit);
+ assert_eq!(res.storage_deposit.charge_or_zero(), deposit + Contracts::min_balance());
// call set_code_hash
builder::bare_call(addr.clone())
@@ -3962,9 +3958,10 @@ fn native_dependency_deposit_works() {
let code_deposit = test_utils::get_code_deposit(&dummy_code_hash);
let deposit = base_deposit + lockup_deposit_percent.mul_ceil(code_deposit);
assert_eq!(test_utils::get_contract(&addr).storage_base_deposit(), deposit);
+
assert_eq!(
test_utils::get_balance_on_hold(&HoldReason::StorageDepositReserve.into(), &addr),
- deposit - ED
+ deposit
);
});
}
diff --git a/substrate/frame/contracts/src/weights.rs b/substrate/frame/contracts/src/weights.rs
index 5c69d9218fa..ca7f58cf5b0 100644
--- a/substrate/frame/contracts/src/weights.rs
+++ b/substrate/frame/contracts/src/weights.rs
@@ -58,6 +58,7 @@ pub trait WeightInfo {
fn v13_migration_step() -> Weight;
fn v14_migration_step() -> Weight;
fn v15_migration_step() -> Weight;
+ fn v16_migration_step() -> Weight;
fn migration_noop() -> Weight;
fn migrate() -> Weight;
fn on_runtime_upgrade_noop() -> Weight;
@@ -271,6 +272,17 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
.saturating_add(T::DbWeight::get().reads(4_u64))
.saturating_add(T::DbWeight::get().writes(2_u64))
}
+ /// Storage: `Contracts::ContractInfoOf` (r:2 w:1)
+ /// Proof: `Contracts::ContractInfoOf` (`max_values`: None, `max_size`: Some(1795), added: 4270, mode: `Measured`)
+ fn v16_migration_step() -> Weight {
+ // Proof Size summary in bytes:
+ // Measured: `409`
+ // Estimated: `6349`
+ // Minimum execution time: 12_595_000 picoseconds.
+ Weight::from_parts(13_059_000, 6349)
+ .saturating_add(T::DbWeight::get().reads(2_u64))
+ .saturating_add(T::DbWeight::get().writes(1_u64))
+ }
/// Storage: `Contracts::MigrationInProgress` (r:1 w:1)
/// Proof: `Contracts::MigrationInProgress` (`max_values`: Some(1), `max_size`: Some(1026), added: 1521, mode: `Measured`)
fn migration_noop() -> Weight {
@@ -1540,6 +1552,17 @@ impl WeightInfo for () {
.saturating_add(RocksDbWeight::get().reads(4_u64))
.saturating_add(RocksDbWeight::get().writes(2_u64))
}
+ /// Storage: `Contracts::ContractInfoOf` (r:2 w:1)
+ /// Proof: `Contracts::ContractInfoOf` (`max_values`: None, `max_size`: Some(1795), added: 4270, mode: `Measured`)
+ fn v16_migration_step() -> Weight {
+ // Proof Size summary in bytes:
+ // Measured: `409`
+ // Estimated: `6349`
+ // Minimum execution time: 12_595_000 picoseconds.
+ Weight::from_parts(13_059_000, 6349)
+ .saturating_add(RocksDbWeight::get().reads(2_u64))
+ .saturating_add(RocksDbWeight::get().writes(1_u64))
+ }
/// Storage: `Contracts::MigrationInProgress` (r:1 w:1)
/// Proof: `Contracts::MigrationInProgress` (`max_values`: Some(1), `max_size`: Some(1026), added: 1521, mode: `Measured`)
fn migration_noop() -> Weight {
--
GitLab