From 25de2fbbbdad179e472972408843a5cb3c841e22 Mon Sep 17 00:00:00 2001
From: pgherveou <pgherveou@gmail.com>
Date: Fri, 12 Apr 2024 15:11:14 +0200
Subject: [PATCH] Revert "Contracts: Remove ED from base deposit (#3536)"
This reverts commit 643aa2be2a2c0611eeb648cfc21eb4cb3c1c9cd8.
---
.../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, 42 insertions(+), 174 deletions(-)
delete 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 676fd320a17..9e245c319b1 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, v16, MigrationStep,
+ codegen::LATEST_MIGRATION_VERSION, v09, v10, v11, v12, v13, v14, v15, MigrationStep,
},
Pallet as Contracts, *,
};
@@ -331,26 +331,6 @@ 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 73c70a7704e..9a1a0aada89 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(16);
+ pub(crate) const STORAGE_VERSION: StorageVersion = StorageVersion::new(15);
#[pallet::pallet]
#[pallet::storage_version(STORAGE_VERSION)]
diff --git a/substrate/frame/contracts/src/migration.rs b/substrate/frame/contracts/src/migration.rs
index c633ba9c2d5..b0df4508285 100644
--- a/substrate/frame/contracts/src/migration.rs
+++ b/substrate/frame/contracts/src/migration.rs
@@ -64,7 +64,6 @@ 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
deleted file mode 100644
index 74fbc997718..00000000000
--- a/substrate/frame/contracts/src/migration/v16.rs
+++ /dev/null
@@ -1,107 +0,0 @@
-// 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 1e9739a1599..fc6d514ebf4 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, TrieId, SENTINEL,
+ Error, Pallet, TrieId, SENTINEL,
};
use codec::{Decode, Encode, MaxEncodedLen};
use frame_support::{
@@ -125,7 +125,9 @@ 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)
+ self.extra_deposit()
+ .saturating_add(self.storage_base_deposit)
+ .saturating_sub(Pallet::<T>::min_balance())
}
/// Returns the storage base deposit of the contract.
@@ -211,6 +213,7 @@ 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)
@@ -221,7 +224,11 @@ impl<T: Config> ContractInfo<T> {
// to prevent abuse.
let upload_deposit = T::CodeHashLockupDepositPercent::get().mul_ceil(code_info.deposit());
- let deposit = info_deposit.saturating_add(upload_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);
+
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 5db9a772ad8..495cbd90db5 100644
--- a/substrate/frame/contracts/src/storage/meter.rs
+++ b/substrate/frame/contracts/src/storage/meter.rs
@@ -435,12 +435,22 @@ where
contract: &T::AccountId,
contract_info: &mut ContractInfo<T>,
code_info: &CodeInfo<T>,
- ) -> Result<(), DispatchError> {
+ ) -> Result<DepositOf<T>, DispatchError> {
debug_assert!(matches!(self.contract_state(), ContractState::Alive));
-
- // We need to make sure that the contract's account exists.
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.
T::Currency::transfer(origin, contract, ed, Preservation::Preserve)?;
// A consumer is added at account creation and removed it on termination, otherwise the
@@ -448,11 +458,9 @@ where
// With the consumer, a correct runtime cannot remove the account.
System::<T>::inc_consumers(contract)?;
- let deposit = contract_info.update_base_deposit(&code_info);
- let deposit = Deposit::Charge(deposit);
+ self.charge_deposit(contract.clone(), deposit.saturating_sub(&Deposit::Charge(ed)));
- self.charge_deposit(contract.clone(), deposit);
- Ok(())
+ Ok(deposit)
}
/// Call to tell the meter that the currently executing contract was terminated.
@@ -851,14 +859,14 @@ mod tests {
let test_cases = vec![
ChargingTestCase {
origin: Origin::<Test>::from_account_id(ALICE),
- deposit: Deposit::Refund(108),
+ deposit: Deposit::Refund(107),
expected: TestExt {
limit_checks: vec![LimitCheck { origin: ALICE, limit: 1_000, min_leftover: 0 }],
charges: vec![
Charge {
origin: ALICE,
contract: CHARLIE,
- amount: Deposit::Refund(120),
+ amount: Deposit::Refund(119),
state: ContractState::Terminated { beneficiary: CHARLIE },
},
Charge {
@@ -907,6 +915,7 @@ 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 0b83358a7f5..7a910a6a8f9 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()
+ dependency_deposit + contract.storage_base_deposit() - ED
);
// 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()
+ contract.storage_base_deposit() - ED
);
// 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),
- ED + balance_before + contract.storage_base_deposit() + dependency_deposit
+ balance_before + contract.storage_base_deposit() + dependency_deposit
);
// Terminate should also remove the dependency, so we can remove the code.
@@ -3904,9 +3904,13 @@ 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().build().execute_with(|| {
+ ExtBuilder::default().existential_deposit(ED).build().execute_with(|| {
let _ = Balances::set_balance(&ALICE, 1_000_000);
let lockup_deposit_percent = CodeHashLockupDepositPercent::get();
@@ -3938,16 +3942,16 @@ fn native_dependency_deposit_works() {
let res = builder::bare_instantiate(code).build();
let addr = res.result.unwrap().account_id;
- let base_deposit = test_utils::contract_info_storage_deposit(&addr);
+ let base_deposit = ED + 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: contract_info_storage_deposit + 30% * deposit
+ // The base deposit should be: ED + 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 + Contracts::min_balance());
+ assert_eq!(res.storage_deposit.charge_or_zero(), deposit);
// call set_code_hash
builder::bare_call(addr.clone())
@@ -3958,10 +3962,9 @@ 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
+ deposit - ED
);
});
}
diff --git a/substrate/frame/contracts/src/weights.rs b/substrate/frame/contracts/src/weights.rs
index ca7f58cf5b0..5c69d9218fa 100644
--- a/substrate/frame/contracts/src/weights.rs
+++ b/substrate/frame/contracts/src/weights.rs
@@ -58,7 +58,6 @@ 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;
@@ -272,17 +271,6 @@ 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 {
@@ -1552,17 +1540,6 @@ 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