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

Fix pallet bags list and doc #10231

Merged
merged 14 commits into from
Dec 3, 2021
4 changes: 2 additions & 2 deletions frame/bags-list/src/benchmarks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ frame_benchmarking::benchmarks! {
// node in the destination in addition to the work we do otherwise. (2 W/R)

// clear any pre-existing storage.
List::<T>::clear(None);
List::<T>::clear();

// define our origin and destination thresholds.
let origin_bag_thresh = T::BagThresholds::get()[0];
Expand Down Expand Up @@ -94,7 +94,7 @@ frame_benchmarking::benchmarks! {
// node in the destination in addition to the work we do otherwise. (2 W/R)

// clear any pre-existing storage.
List::<T>::clear(None);
List::<T>::clear();

// define our origin and destination thresholds.
let origin_bag_thresh = T::BagThresholds::get()[0];
Expand Down
4 changes: 2 additions & 2 deletions frame/bags-list/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -272,8 +272,8 @@ impl<T: Config> SortedListProvider<T::AccountId> for Pallet<T> {
Ok(())
}

fn clear(maybe_count: Option<u32>) -> u32 {
List::<T>::clear(maybe_count)
fn clear() {
List::<T>::clear()
}

#[cfg(feature = "runtime-benchmarks")]
Expand Down
18 changes: 7 additions & 11 deletions frame/bags-list/src/list/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,19 +76,15 @@ pub fn notional_bag_for<T: Config>(weight: VoteWeight) -> VoteWeight {
pub struct List<T: Config>(PhantomData<T>);

impl<T: Config> List<T> {
/// Remove all data associated with the list from storage. Parameter `items` is the number of
/// items to clear from the list.
/// Remove all data associated with the list from storage.
///
/// ## WARNING
///
/// `None` will clear all items and should generally not be used in production as it could lead
/// to a very large number of storage accesses.
pub(crate) fn clear(maybe_count: Option<u32>) -> u32 {
crate::ListBags::<T>::remove_all(maybe_count);
let pre = crate::ListNodes::<T>::count();
crate::ListNodes::<T>::remove_all(maybe_count);
let post = crate::ListNodes::<T>::count();
pre.saturating_sub(post)
/// this function should generally not be used in production as it could lead to a very large
/// number of storage accesses.
pub(crate) fn clear() {
crate::ListBags::<T>::remove_all(None);
crate::ListNodes::<T>::remove_all();
}

/// Regenerate all of the data from the given ids.
Expand All @@ -104,7 +100,7 @@ impl<T: Config> List<T> {
all: impl IntoIterator<Item = T::AccountId>,
weight_of: Box<dyn Fn(&T::AccountId) -> VoteWeight>,
) -> u32 {
Self::clear(None);
Self::clear();
Self::insert_many(all, weight_of)
}

Expand Down
9 changes: 5 additions & 4 deletions frame/election-provider-support/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -338,10 +338,11 @@ pub trait SortedListProvider<AccountId> {
weight_of: Box<dyn Fn(&AccountId) -> VoteWeight>,
) -> u32;

/// Remove `maybe_count` number of items from the list. Returns the number of items actually
/// removed. WARNING: removes all items if `maybe_count` is `None`, which should never be done
/// in production settings because it can lead to an unbounded amount of storage accesses.
fn clear(maybe_count: Option<u32>) -> u32;
/// Remove all items from the list.
///
/// WARNING: should never be called in production settings because it can lead to an unbounded
/// amount of storage accesses.
KiChjang marked this conversation as resolved.
Show resolved Hide resolved
fn clear();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe call this unsafe_clear then? To make the user aware of the problems?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also in pallet-bags-list when implementing the function SortedListProvider::regenerate it calls clear inside. Should we also prepend unsafe to regenerate ? WDYT @kianenigma

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we should

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in fede886


/// Sanity check internal state of list. Only meant for debug compilation.
fn sanity_check() -> Result<(), &'static str>;
Expand Down
6 changes: 5 additions & 1 deletion frame/support/src/storage/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1133,7 +1133,11 @@ pub trait StoragePrefixedMap<Value: FullCodec> {
crate::storage::storage_prefix(Self::module_prefix(), Self::storage_prefix())
}

/// Remove all value of the storage.
/// Remove all values of the storage in the overlay and up to `limit` in the backend.
///
/// All values in the client overlay will be deleted, if there is some `limit` then up to
emostov marked this conversation as resolved.
Show resolved Hide resolved
/// `limit` values are deleted from the client backend, if `limit` is none then all values in
/// the client backend are deleted.
fn remove_all(limit: Option<u32>) -> sp_io::KillStorageResult {
sp_io::storage::clear_prefix(&Self::final_prefix(), limit)
}
Expand Down
11 changes: 6 additions & 5 deletions frame/support/src/storage/types/counted_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ use crate::{
Never,
};
use codec::{Decode, Encode, EncodeLike, FullCodec, MaxEncodedLen, Ref};
use sp_arithmetic::traits::Bounded;
use sp_runtime::traits::Saturating;
use sp_std::prelude::*;

Expand Down Expand Up @@ -263,10 +262,12 @@ where
}

/// Remove all value of the storage.
pub fn remove_all(maybe_limit: Option<u32>) {
let leftover = Self::count().saturating_sub(maybe_limit.unwrap_or_else(Bounded::max_value));
CounterFor::<Prefix>::set(leftover);
<Self as MapWrapper>::Map::remove_all(maybe_limit);
pub fn remove_all() {
// NOTE: it is not possible to remove up to some limit because
// `sp_io::storage::clear_prefix` and `StorageMap::remove_all` don't give the number of
// value removed from the overlay.
CounterFor::<Prefix>::set(0u32);
<Self as MapWrapper>::Map::remove_all(None);
}

/// Iter over all value of the storage.
Expand Down
6 changes: 5 additions & 1 deletion frame/support/src/storage/types/double_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,11 @@ where
>(key1, key2)
}

/// Remove all value of the storage.
/// Remove all values of the storage in the overlay and up to `limit` in the backend.
///
/// All values in the client overlay will be deleted, if there is some `limit` then up to
/// `limit` values are deleted from the client backend, if `limit` is none then all values in
/// the client backend are deleted.
pub fn remove_all(limit: Option<u32>) -> sp_io::KillStorageResult {
<Self as crate::storage::StoragePrefixedMap<Value>>::remove_all(limit)
}
Expand Down
6 changes: 5 additions & 1 deletion frame/support/src/storage/types/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,11 @@ where
<Self as crate::storage::StorageMap<Key, Value>>::migrate_key::<OldHasher, _>(key)
}

/// Remove all value of the storage.
/// Remove all values of the storage in the overlay and up to `limit` in the backend.
///
Comment on lines +237 to +238
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its just a crazy api. This is absolutely not an "expected" behavior, and we shouldn't really expose such weird things in this way...

We need apis like this which are both bounded and behave in expected ways.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I agree, IMO we should at least have sp_io::storage::clear_prefix return the exact number of value removed (in both overlay and backend).

/// All values in the client overlay will be deleted, if there is some `limit` then up to
/// `limit` values are deleted from the client backend, if `limit` is none then all values in
/// the client backend are deleted.
pub fn remove_all(limit: Option<u32>) -> sp_io::KillStorageResult {
<Self as crate::storage::StoragePrefixedMap<Value>>::remove_all(limit)
}
Expand Down
6 changes: 3 additions & 3 deletions primitives/io/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,12 +88,12 @@ pub enum EcdsaVerifyError {
}

/// The outcome of calling `storage_kill`. Returned value is the number of storage items
/// removed from the trie from making the `storage_kill` call.
/// removed from the backend from making the `storage_kill` call.
#[derive(PassByCodec, Encode, Decode)]
pub enum KillStorageResult {
/// No key remains in the child trie.
/// All key to remove were removed, return number of key removed from backend.
AllRemoved(u32),
/// At least one key still resides in the child trie due to the supplied limit.
/// Not all key to remove were removed, return number of key removed from backend.
SomeRemaining(u32),
}

Expand Down