Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Commit a96ec02

Browse files
pgherveoujuangiriniatheibkchrkoute
committed
[Contracts] Overflowing bounded DeletionQueue allows DoS against contract termination (#13702)
* [Contracts review] Overflowing bounded `DeletionQueue` allows DoS against contract termination * wip * wip * wip * wip * wip * fix doc * wip * PR review * unbreak tests * fixes * update budget computation * PR comment: use BlockWeights::get().max_block * PR comment: Update queue_trie_for_deletion signature * PR comment: update deletion budget docstring * PR comment: impl Default with derive(DefaultNoBound) * PR comment: Remove DeletedContract * PR comment Add ring_buffer test * remove missed comment * misc comments * contracts: add sr25519_recover * Revert "contracts: add sr25519_recover" This reverts commit d4600e0. * ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts * PR comments update print_schedule * Update frame/contracts/src/benchmarking/mod.rs * Update frame/contracts/src/storage.rs * Update frame/contracts/src/storage.rs * rm temporary fixes * fix extra ; * Update frame/contracts/src/storage.rs Co-authored-by: juangirini <[email protected]> * Update frame/contracts/src/storage.rs Co-authored-by: Alexander Theißen <[email protected]> * Update frame/contracts/src/lib.rs Co-authored-by: Alexander Theißen <[email protected]> * Update frame/contracts/src/lib.rs Co-authored-by: Alexander Theißen <[email protected]> * Support stable rust for compiling the runtime (#13580) * Support stable rust for compiling the runtime This pull request brings support for compiling the runtime with stable Rust. This requires at least rust 1.68.0 to work on stable. The code is written in a way that it is backwards compatible and should automatically work when someone compiles with 1.68.0+ stable. * We always support nightlies! * 🤦 * Sort by version * Review feedback * Review feedback * Fix version parsing * Apply suggestions from code review Co-authored-by: Koute <[email protected]> --------- Co-authored-by: Koute <[email protected]> * github PR commit fixes * Revert "Support stable rust for compiling the runtime (#13580)" This reverts commit 0b985aa. * Restore DeletionQueueMap * fix namings * PR comment * move comments * Update frame/contracts/src/storage.rs * Update frame/contracts/src/storage.rs * fixes --------- Co-authored-by: command-bot <> Co-authored-by: juangirini <[email protected]> Co-authored-by: Alexander Theißen <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Koute <[email protected]>
1 parent 23507f2 commit a96ec02

File tree

7 files changed

+1132
-1186
lines changed

7 files changed

+1132
-1186
lines changed

bin/node/runtime/src/lib.rs

-9
Original file line numberDiff line numberDiff line change
@@ -1198,13 +1198,6 @@ impl pallet_tips::Config for Runtime {
11981198
parameter_types! {
11991199
pub const DepositPerItem: Balance = deposit(1, 0);
12001200
pub const DepositPerByte: Balance = deposit(0, 1);
1201-
pub const DeletionQueueDepth: u32 = 128;
1202-
// The lazy deletion runs inside on_initialize.
1203-
pub DeletionWeightLimit: Weight = RuntimeBlockWeights::get()
1204-
.per_class
1205-
.get(DispatchClass::Normal)
1206-
.max_total
1207-
.unwrap_or(RuntimeBlockWeights::get().max_block);
12081201
pub Schedule: pallet_contracts::Schedule<Runtime> = Default::default();
12091202
}
12101203

@@ -1227,8 +1220,6 @@ impl pallet_contracts::Config for Runtime {
12271220
type WeightPrice = pallet_transaction_payment::Pallet<Self>;
12281221
type WeightInfo = pallet_contracts::weights::SubstrateWeight<Self>;
12291222
type ChainExtension = ();
1230-
type DeletionQueueDepth = DeletionQueueDepth;
1231-
type DeletionWeightLimit = DeletionWeightLimit;
12321223
type Schedule = Schedule;
12331224
type AddressGenerator = pallet_contracts::DefaultAddressGenerator;
12341225
type MaxCodeLen = ConstU32<{ 123 * 1024 }>;

frame/contracts/src/benchmarking/mod.rs

+5-21
Original file line numberDiff line numberDiff line change
@@ -214,19 +214,7 @@ benchmarks! {
214214
on_initialize_per_trie_key {
215215
let k in 0..1024;
216216
let instance = Contract::<T>::with_storage(WasmModule::dummy(), k, T::Schedule::get().limits.payload_len)?;
217-
instance.info()?.queue_trie_for_deletion()?;
218-
}: {
219-
ContractInfo::<T>::process_deletion_queue_batch(Weight::MAX)
220-
}
221-
222-
#[pov_mode = Measured]
223-
on_initialize_per_queue_item {
224-
let q in 0..1024.min(T::DeletionQueueDepth::get());
225-
for i in 0 .. q {
226-
let instance = Contract::<T>::with_index(i, WasmModule::dummy(), vec![])?;
227-
instance.info()?.queue_trie_for_deletion()?;
228-
ContractInfoOf::<T>::remove(instance.account_id);
229-
}
217+
instance.info()?.queue_trie_for_deletion();
230218
}: {
231219
ContractInfo::<T>::process_deletion_queue_batch(Weight::MAX)
232220
}
@@ -3020,16 +3008,12 @@ benchmarks! {
30203008
print_schedule {
30213009
#[cfg(feature = "std")]
30223010
{
3023-
let weight_limit = T::DeletionWeightLimit::get();
3024-
let max_queue_depth = T::DeletionQueueDepth::get() as usize;
3025-
let empty_queue_throughput = ContractInfo::<T>::deletion_budget(0, weight_limit);
3026-
let full_queue_throughput = ContractInfo::<T>::deletion_budget(max_queue_depth, weight_limit);
3011+
let max_weight = <T as frame_system::Config>::BlockWeights::get().max_block;
3012+
let (weight_per_key, key_budget) = ContractInfo::<T>::deletion_budget(max_weight);
30273013
println!("{:#?}", Schedule::<T>::default());
30283014
println!("###############################################");
3029-
println!("Lazy deletion weight per key: {}", empty_queue_throughput.0);
3030-
println!("Lazy deletion throughput per block (empty queue, full queue): {}, {}",
3031-
empty_queue_throughput.1, full_queue_throughput.1,
3032-
);
3015+
println!("Lazy deletion weight per key: {weight_per_key}");
3016+
println!("Lazy deletion throughput per block: {key_budget}");
30333017
}
30343018
#[cfg(not(feature = "std"))]
30353019
Err("Run this bench with a native runtime in order to see the schedule.")?;

frame/contracts/src/exec.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -1213,7 +1213,7 @@ where
12131213
T::Currency::reducible_balance(&frame.account_id, Expendable, Polite),
12141214
ExistenceRequirement::AllowDeath,
12151215
)?;
1216-
info.queue_trie_for_deletion()?;
1216+
info.queue_trie_for_deletion();
12171217
ContractInfoOf::<T>::remove(&frame.account_id);
12181218
E::remove_user(info.code_hash);
12191219
Contracts::<T>::deposit_event(

frame/contracts/src/lib.rs

+9-56
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ mod tests;
102102
use crate::{
103103
exec::{AccountIdOf, ErrorOrigin, ExecError, Executable, Key, Stack as ExecStack},
104104
gas::GasMeter,
105-
storage::{meter::Meter as StorageMeter, ContractInfo, DeletedContract},
105+
storage::{meter::Meter as StorageMeter, ContractInfo, DeletionQueueManager},
106106
wasm::{OwnerInfo, PrefabWasmModule, TryInstantiate},
107107
weights::WeightInfo,
108108
};
@@ -245,33 +245,6 @@ pub mod pallet {
245245
/// memory usage of your runtime.
246246
type CallStack: Array<Item = Frame<Self>>;
247247

248-
/// The maximum number of contracts that can be pending for deletion.
249-
///
250-
/// When a contract is deleted by calling `seal_terminate` it becomes inaccessible
251-
/// immediately, but the deletion of the storage items it has accumulated is performed
252-
/// later. The contract is put into the deletion queue. This defines how many
253-
/// contracts can be queued up at the same time. If that limit is reached `seal_terminate`
254-
/// will fail. The action must be retried in a later block in that case.
255-
///
256-
/// The reasons for limiting the queue depth are:
257-
///
258-
/// 1. The queue is in storage in order to be persistent between blocks. We want to limit
259-
/// the amount of storage that can be consumed.
260-
/// 2. The queue is stored in a vector and needs to be decoded as a whole when reading
261-
/// it at the end of each block. Longer queues take more weight to decode and hence
262-
/// limit the amount of items that can be deleted per block.
263-
#[pallet::constant]
264-
type DeletionQueueDepth: Get<u32>;
265-
266-
/// The maximum amount of weight that can be consumed per block for lazy trie removal.
267-
///
268-
/// The amount of weight that is dedicated per block to work on the deletion queue. Larger
269-
/// values allow more trie keys to be deleted in each block but reduce the amount of
270-
/// weight that is left for transactions. See [`Self::DeletionQueueDepth`] for more
271-
/// information about the deletion queue.
272-
#[pallet::constant]
273-
type DeletionWeightLimit: Get<Weight>;
274-
275248
/// The amount of balance a caller has to pay for each byte of storage.
276249
///
277250
/// # Note
@@ -329,25 +302,6 @@ pub mod pallet {
329302
.saturating_add(T::WeightInfo::on_process_deletion_queue_batch())
330303
}
331304

332-
fn on_initialize(_block: T::BlockNumber) -> Weight {
333-
// We want to process the deletion_queue in the on_idle hook. Only in the case
334-
// that the queue length has reached its maximal depth, we process it here.
335-
let max_len = T::DeletionQueueDepth::get() as usize;
336-
let queue_len = <DeletionQueue<T>>::decode_len().unwrap_or(0);
337-
if queue_len >= max_len {
338-
// We do not want to go above the block limit and rather avoid lazy deletion
339-
// in that case. This should only happen on runtime upgrades.
340-
let weight_limit = T::BlockWeights::get()
341-
.max_block
342-
.saturating_sub(System::<T>::block_weight().total())
343-
.min(T::DeletionWeightLimit::get());
344-
ContractInfo::<T>::process_deletion_queue_batch(weight_limit)
345-
.saturating_add(T::WeightInfo::on_process_deletion_queue_batch())
346-
} else {
347-
T::WeightInfo::on_process_deletion_queue_batch()
348-
}
349-
}
350-
351305
fn integrity_test() {
352306
// Total runtime memory is expected to have 128Mb upper limit
353307
const MAX_RUNTIME_MEM: u32 = 1024 * 1024 * 128;
@@ -860,12 +814,6 @@ pub mod pallet {
860814
/// in this error. Note that this usually shouldn't happen as deploying such contracts
861815
/// is rejected.
862816
NoChainExtension,
863-
/// Removal of a contract failed because the deletion queue is full.
864-
///
865-
/// This can happen when calling `seal_terminate`.
866-
/// The queue is filled by deleting contracts and emptied by a fixed amount each block.
867-
/// Trying again during another block is the only way to resolve this issue.
868-
DeletionQueueFull,
869817
/// A contract with the same AccountId already exists.
870818
DuplicateContract,
871819
/// A contract self destructed in its constructor.
@@ -949,10 +897,15 @@ pub mod pallet {
949897
/// Evicted contracts that await child trie deletion.
950898
///
951899
/// Child trie deletion is a heavy operation depending on the amount of storage items
952-
/// stored in said trie. Therefore this operation is performed lazily in `on_initialize`.
900+
/// stored in said trie. Therefore this operation is performed lazily in `on_idle`.
901+
#[pallet::storage]
902+
pub(crate) type DeletionQueue<T: Config> = StorageMap<_, Twox64Concat, u32, TrieId>;
903+
904+
/// A pair of monotonic counters used to track the latest contract marked for deletion
905+
/// and the latest deleted contract in queue.
953906
#[pallet::storage]
954-
pub(crate) type DeletionQueue<T: Config> =
955-
StorageValue<_, BoundedVec<DeletedContract, T::DeletionQueueDepth>, ValueQuery>;
907+
pub(crate) type DeletionQueueCounter<T: Config> =
908+
StorageValue<_, DeletionQueueManager<T>, ValueQuery>;
956909
}
957910

958911
/// Context of a contract invocation.

0 commit comments

Comments
 (0)