From 2604a36f3f42970b6b5b5e436f81c7c7e9f114a7 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 10 May 2023 18:57:53 +0200 Subject: [PATCH 01/40] Prototype StoragePagedList Signed-off-by: Oliver Tale-Yazdi --- frame/support/src/storage/mod.rs | 15 + frame/support/src/storage/types/mod.rs | 2 + frame/support/src/storage/types/paged_list.rs | 266 ++++++++++++++++++ .../pallet_ui/storage_info_unsatisfied.rs | 2 +- 4 files changed, 284 insertions(+), 1 deletion(-) create mode 100644 frame/support/src/storage/types/paged_list.rs diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 4c6ea943c6920..99b82147448e3 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -160,6 +160,21 @@ pub trait StorageValue { } } +pub trait StoragePagedList { + fn append(item: EncodeLikeItem) + where + Item: Encode, + EncodeLikeItem: EncodeLike; +} + +pub trait IterableStorageList { + type Iterator: Iterator; + + /// Enumerate all elements in the map in lexicographical order of the encoded key. If you + /// alter the map while doing this, you'll get undefined results. + fn iter() -> Self::Iterator; +} + /// A strongly-typed map in storage. /// /// Details on implementation can be found at [`generator::StorageMap`]. diff --git a/frame/support/src/storage/types/mod.rs b/frame/support/src/storage/types/mod.rs index 3a5bae2e608b7..bb8347017c7f1 100644 --- a/frame/support/src/storage/types/mod.rs +++ b/frame/support/src/storage/types/mod.rs @@ -27,6 +27,7 @@ mod double_map; mod key; mod map; mod nmap; +mod paged_list; mod value; pub use counted_map::{CountedStorageMap, CountedStorageMapInstance}; @@ -37,6 +38,7 @@ pub use key::{ }; pub use map::StorageMap; pub use nmap::StorageNMap; +pub use paged_list::StoragePagedList; pub use value::StorageValue; /// Trait implementing how the storage optional value is converted into the queried type. diff --git a/frame/support/src/storage/types/paged_list.rs b/frame/support/src/storage/types/paged_list.rs new file mode 100644 index 0000000000000..74259e776089d --- /dev/null +++ b/frame/support/src/storage/types/paged_list.rs @@ -0,0 +1,266 @@ +// 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. + +//! Storage map type. Implements StorageMap, StorageIterableMap, StoragePrefixedMap traits and their +//! methods directly. + +#![allow(unused_imports)] // FAIL-CI + +use crate::{ + defensive, + storage::{KeyLenOf, StorageAppend, StorageDecodeLength, StoragePrefixedMap, StorageTryAppend}, + traits::{Defensive, Get, GetDefault, StorageInfo, StorageInstance}, + CloneNoBound, DebugNoBound, DefaultNoBound, EqNoBound, PartialEqNoBound, StorageHasher, + Twox128, +}; +use codec::{Decode, Encode, EncodeLike, FullCodec, MaxEncodedLen}; +use sp_arithmetic::traits::{SaturatedConversion, Saturating}; +use sp_std::prelude::*; + +pub type PageIndex = u32; +pub type ValueIndex = u32; + +/// A paginated storage list that packs multiple elements per page. +/// +/// The map is lined by a storage lookup of `(prefix, page_index)`. See [`StoragePagedListMeta`]. +/// +/// Supported APIs are iteration and appending. +pub struct StoragePagedList { + _phantom: core::marker::PhantomData<(Prefix, Hasher, Value, ValuesPerPage, MaxPages)>, +} + +/// State info for book-keeping. Can be used as append-only iterator. +#[derive( + Encode, Decode, CloneNoBound, PartialEqNoBound, EqNoBound, DebugNoBound, DefaultNoBound, +)] +// todo ignore scale bounds +pub struct StoragePagedListMeta { + current_page: PageIndex, + current_value: ValueIndex, // inside `current_page` + + _phantom: core::marker::PhantomData<(Prefix, Value, Hasher, ValuesPerPage)>, +} + +impl + StoragePagedListMeta +where + Prefix: StorageInstance, + Value: FullCodec, + Hasher: StorageHasher, + ValuesPerPage: Get, +{ + pub fn from_storage() -> Option { + let key = Self::key(); + let raw = sp_io::storage::get(key.as_ref()).unwrap_or_default(); + Self::decode(&mut &raw[..]).ok() + } + + pub fn key() -> Hasher::Output { + (Prefix::STORAGE_PREFIX, b"paged_list_state").using_encoded(Hasher::hash) + } + + pub fn append(&mut self, item: EncodeLikeItem) + where + EncodeLikeItem: EncodeLike, + { + if self.current_value == ValuesPerPage::get() { + self.current_value = 0; + self.current_page.saturating_inc(); + } + let key = page_key::(self.current_page); + // Pages are `Vec` and therefore appendable. + sp_io::storage::append(key.as_ref(), item.encode()); + } +} + +// invariant: empty pages do not exist +pub struct Page { + current: PageIndex, + values: sp_std::vec::Vec, +} + +impl Page { + fn from_storage( + index: PageIndex, + ) -> Option { + let key = page_key::(index); + let values = sp_io::storage::get(key.as_ref()) + .map(|raw| sp_std::vec::Vec::::decode(&mut &raw[..]).ok()) + .flatten(); + let values = values.unwrap_or_default(); + if values.is_empty() { + None + } else { + Some(Self { current: 0, values }) + } + } +} + +// Note: does not live under `Page` since it does not require `Value`. +pub fn page_key( + index: PageIndex, +) -> Hasher::Output { + (Prefix::STORAGE_PREFIX, index).using_encoded(Hasher::hash) +} + +impl Iterator for Page { + type Item = V; + + fn next(&mut self) -> Option { + let val = self.values.get(self.current as usize).cloned(); + self.current.saturating_inc(); + val + } +} + +pub struct StoragePagedListIterator { + // Design: we put the Page into the iterator to have fewer storage look-ups. Yes, these + // look-ups would be cached anyway, but bugging the overlay on each `.next` call still seems + // like a poor trade-off than caching it in the iterator directly. Note: if Page is empty then + // the iterator did not find any data upon setup or ran out of pages. + page: Option>, + current_page: PageIndex, + _phantom: core::marker::PhantomData<(Prefix, Value, Hasher, ValuesPerPage)>, +} + +impl + StoragePagedListIterator +where + Prefix: StorageInstance, + Value: FullCodec + Clone, + Hasher: StorageHasher, + ValuesPerPage: Get, +{ + pub fn from_storage() -> Self { + let page = Page::::from_storage::(0); + + Self { page, current_page: 0, _phantom: Default::default() } + } +} + +impl Iterator + for StoragePagedListIterator +where + Prefix: StorageInstance, + Value: FullCodec + Clone, + Hasher: StorageHasher, + ValuesPerPage: Get, +{ + type Item = Value; + + fn next(&mut self) -> Option { + let Some(ref mut page) = self.page else { + return None; + }; + if let Some(val) = page.next() { + return Some(val) + } + + self.current_page.saturating_inc(); + let Some(mut page) = Page::from_storage::(self.current_page) else { + // No more pages, this is fine. + return None; + }; + + if let Some(val) = page.next() { + self.page = Some(page); + return Some(val) + } + + defensive!("StoragePagedListIterator: empty pages do not exist: storage corrupted"); + self.page = None; + None + } +} + +impl crate::storage::IterableStorageList + for StoragePagedList +where + Prefix: StorageInstance, + Value: FullCodec + Clone, + Hasher: StorageHasher, + ValuesPerPage: Get, + MaxPages: Get>, +{ + type Iterator = StoragePagedListIterator; + + fn iter() -> Self::Iterator { + StoragePagedListIterator::from_storage() + } +} + +impl + StoragePagedList +where + Prefix: StorageInstance, + Value: FullCodec + Clone, + Hasher: StorageHasher, + ValuesPerPage: Get, + MaxPages: Get>, +{ + fn read_meta() -> StoragePagedListMeta { + // We unwrap here to not require a setup migration. + StoragePagedListMeta::from_storage().unwrap_or_default() + } + + /// Provides a fast append iterator. + /// + /// The list should not be modified while appending. Also don't call it recursively. + pub fn appendix() -> StoragePagedListMeta { + Self::read_meta() + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::{ + hash::*, + metadata_ir::{StorageEntryModifierIR, StorageEntryTypeIR, StorageHasherIR}, + parameter_types, + storage::{types::ValueQuery, IterableStorageList}, + }; + use sp_io::{hashing::twox_128, TestExternalities}; + + parameter_types! { + pub const ValuesPerPage: u32 = 2; + pub const MaxPages: Option = Some(20); + } + + struct Prefix; + impl StorageInstance for Prefix { + fn pallet_prefix() -> &'static str { + "test" + } + const STORAGE_PREFIX: &'static str = "foo"; + } + + type List = StoragePagedList; + + #[test] + fn basic_works() { + TestExternalities::default().execute_with(|| { + let mut append_iter = List::appendix(); + for i in 0..1000 { + append_iter.append(i); + } + + let vals = List::iter().collect::>(); + assert_eq!(vals, (0..1000).collect::>()); + }); + } +} diff --git a/frame/support/test/tests/pallet_ui/storage_info_unsatisfied.rs b/frame/support/test/tests/pallet_ui/storage_info_unsatisfied.rs index 4d43e3a17a9ec..2a676b30aea38 100644 --- a/frame/support/test/tests/pallet_ui/storage_info_unsatisfied.rs +++ b/frame/support/test/tests/pallet_ui/storage_info_unsatisfied.rs @@ -1,4 +1,4 @@ -#[frame_support::pallet] +i#[frame_support::pallet] mod pallet { use frame_support::pallet_prelude::{Hooks, StorageValue}; use frame_system::pallet_prelude::BlockNumberFor; From 5b16ac7c81e0901384bddf28bd14de6e2c67b9f8 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 10 May 2023 22:56:28 +0200 Subject: [PATCH 02/40] Add drain Signed-off-by: Oliver Tale-Yazdi --- frame/support/src/storage/mod.rs | 2 + frame/support/src/storage/types/paged_list.rs | 165 ++++++++++++++++-- .../pallet_ui/storage_info_unsatisfied.rs | 2 +- 3 files changed, 151 insertions(+), 18 deletions(-) diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 99b82147448e3..08a35c66d9c8f 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -173,6 +173,8 @@ pub trait IterableStorageList { /// Enumerate all elements in the map in lexicographical order of the encoded key. If you /// alter the map while doing this, you'll get undefined results. fn iter() -> Self::Iterator; + + fn drain() -> Self::Iterator; } /// A strongly-typed map in storage. diff --git a/frame/support/src/storage/types/paged_list.rs b/frame/support/src/storage/types/paged_list.rs index 74259e776089d..136f63e639ddd 100644 --- a/frame/support/src/storage/types/paged_list.rs +++ b/frame/support/src/storage/types/paged_list.rs @@ -25,7 +25,7 @@ use crate::{ storage::{KeyLenOf, StorageAppend, StorageDecodeLength, StoragePrefixedMap, StorageTryAppend}, traits::{Defensive, Get, GetDefault, StorageInfo, StorageInstance}, CloneNoBound, DebugNoBound, DefaultNoBound, EqNoBound, PartialEqNoBound, StorageHasher, - Twox128, + StorageNoopGuard, Twox128, }; use codec::{Decode, Encode, EncodeLike, FullCodec, MaxEncodedLen}; use sp_arithmetic::traits::{SaturatedConversion, Saturating}; @@ -49,7 +49,10 @@ pub struct StoragePagedList { - current_page: PageIndex, + first_page: PageIndex, // can be >0 when pages were delete. + first_value: ValueIndex, // inside `first_page` + + current_page: PageIndex, // next one is free, always >= first_page current_value: ValueIndex, // inside `current_page` _phantom: core::marker::PhantomData<(Prefix, Value, Hasher, ValuesPerPage)>, @@ -77,19 +80,36 @@ where where EncodeLikeItem: EncodeLike, { - if self.current_value == ValuesPerPage::get() { + // Note: we use >= here in case someone decreased it in a runtime upgrade. + if self.current_value >= ValuesPerPage::get() { self.current_value = 0; self.current_page.saturating_inc(); } let key = page_key::(self.current_page); + self.current_value.saturating_inc(); + log::info!("Appending to page/val {}/{}", self.current_page, self.current_value); // Pages are `Vec` and therefore appendable. sp_io::storage::append(key.as_ref(), item.encode()); + self.store(); + } + + pub fn store(&self) { + let key = Self::key(); + log::info!("Storing: {:?}", &self); + self.using_encoded(|encoded| sp_io::storage::set(key.as_ref(), encoded)); + } + + // only good for testing. + pub fn clear() { + let key = Self::key(); + sp_io::storage::clear(key.as_ref()); } } // invariant: empty pages do not exist pub struct Page { - current: PageIndex, + current: PageIndex, // current value for iteration + index: PageIndex, values: sp_std::vec::Vec, } @@ -105,13 +125,22 @@ impl Page { if values.is_empty() { None } else { - Some(Self { current: 0, values }) + Some(Self { current: 0, index, values }) } } + + pub fn delete(&self) { + delete_page::(self.index); + } +} + +pub(crate) fn delete_page(index: PageIndex) { + let key = page_key::(index); + sp_io::storage::clear(key.as_ref()); } // Note: does not live under `Page` since it does not require `Value`. -pub fn page_key( +pub(crate) fn page_key( index: PageIndex, ) -> Hasher::Output { (Prefix::STORAGE_PREFIX, index).using_encoded(Hasher::hash) @@ -133,7 +162,8 @@ pub struct StoragePagedListIterator { // like a poor trade-off than caching it in the iterator directly. Note: if Page is empty then // the iterator did not find any data upon setup or ran out of pages. page: Option>, - current_page: PageIndex, + drain: bool, + meta: StoragePagedListMeta, _phantom: core::marker::PhantomData<(Prefix, Value, Hasher, ValuesPerPage)>, } @@ -145,10 +175,17 @@ where Hasher: StorageHasher, ValuesPerPage: Get, { - pub fn from_storage() -> Self { - let page = Page::::from_storage::(0); + pub fn from_meta( + meta: StoragePagedListMeta, + drain: bool, + ) -> Self { + let mut page = Page::::from_storage::(meta.first_page); + + if let Some(ref mut page) = page { + page.current = meta.first_value; + } - Self { page, current_page: 0, _phantom: Default::default() } + Self { page, drain, meta, _phantom: Default::default() } } } @@ -167,21 +204,42 @@ where return None; }; if let Some(val) = page.next() { + if self.drain { + self.meta.first_value.saturating_inc(); + self.meta.store() + } return Some(val) } + if self.drain { + // page is empty + page.delete::(); + self.meta.first_value = 0; + self.meta.first_page.saturating_inc(); + self.meta.store(); + } - self.current_page.saturating_inc(); - let Some(mut page) = Page::from_storage::(self.current_page) else { + let Some(mut page) = Page::from_storage::(page.index.saturating_add(1)) else { + if self.drain { + // All is gone - back to zero. + self.meta = StoragePagedListMeta::default(); + self.meta.store(); + } // No more pages, this is fine. return None; }; if let Some(val) = page.next() { + if self.drain { + self.meta.first_value.saturating_inc(); + self.meta.store(); + } self.page = Some(page); return Some(val) } defensive!("StoragePagedListIterator: empty pages do not exist: storage corrupted"); + self.meta = StoragePagedListMeta::default(); + self.meta.store(); self.page = None; None } @@ -199,7 +257,11 @@ where type Iterator = StoragePagedListIterator; fn iter() -> Self::Iterator { - StoragePagedListIterator::from_storage() + StoragePagedListIterator::from_meta(Self::read_meta(), false) + } + + fn drain() -> Self::Iterator { + StoragePagedListIterator::from_meta(Self::read_meta(), true) } } @@ -212,8 +274,8 @@ where ValuesPerPage: Get, MaxPages: Get>, { - fn read_meta() -> StoragePagedListMeta { - // We unwrap here to not require a setup migration. + pub(crate) fn read_meta() -> StoragePagedListMeta { + // Use default here to not require a setup migration. StoragePagedListMeta::from_storage().unwrap_or_default() } @@ -237,7 +299,7 @@ mod tests { use sp_io::{hashing::twox_128, TestExternalities}; parameter_types! { - pub const ValuesPerPage: u32 = 2; + pub const ValuesPerPage: u32 = 5; pub const MaxPages: Option = Some(20); } @@ -252,7 +314,7 @@ mod tests { type List = StoragePagedList; #[test] - fn basic_works() { + fn append_works() { TestExternalities::default().execute_with(|| { let mut append_iter = List::appendix(); for i in 0..1000 { @@ -261,6 +323,75 @@ mod tests { let vals = List::iter().collect::>(); assert_eq!(vals, (0..1000).collect::>()); + + let meta = List::read_meta(); + assert_eq!(meta.first_page, 0); + }); + } + + /// Draining all works. + #[test] + fn simple_drain_works() { + TestExternalities::default().execute_with(|| { + let _g = StorageNoopGuard::default(); // All in all a No-Op + let mut append_iter = List::appendix(); + for i in 0..1000 { + append_iter.append(i); + } + + let vals = List::drain().collect::>(); + assert_eq!(vals, (0..1000).collect::>()); + + assert_eq!(List::read_meta(), Default::default()); + + // all gone + let vals = List::iter().collect::>(); + assert_eq!(vals, Vec::::new()); + // Need to delete the metadata manually. + StoragePagedListMeta::::clear(); + }); + } + + /// Drain half of the elements and iterator the rest. + #[test] + fn partial_drain_works() { + TestExternalities::default().execute_with(|| { + let mut append_iter = List::appendix(); + for i in 0..100 { + append_iter.append(i); + } + + let vals = List::drain().take(50).collect::>(); + assert_eq!(vals, (0..50).collect::>()); + + let meta = List::read_meta(); + // Will switch over to `10/0`, but will in the next call. + assert_eq!((meta.first_page, meta.first_value), (9, 5)); + + // 50 gone, 50 to go + let vals = List::iter().collect::>(); + assert_eq!(vals, (50..100).collect::>()); + }); + } + + /// Draining, appending and iterating work together. + #[test] + fn drain_append_iter_works() { + sp_tracing::try_init_simple(); + + TestExternalities::default().execute_with(|| { + for r in 1..=100 { + log::warn!("round {}", r); + let mut appendix = List::appendix(); + (0..12).for_each(|i| appendix.append(i)); + (0..12).for_each(|i| appendix.append(i)); + + let dropped = List::drain().take(12).collect::>(); + assert_eq!(dropped, (0..12).collect::>()); + + let all = List::iter().collect::>(); + assert_eq!(all, (0..12).cycle().take(r * 12).collect::>()); + } }); } } diff --git a/frame/support/test/tests/pallet_ui/storage_info_unsatisfied.rs b/frame/support/test/tests/pallet_ui/storage_info_unsatisfied.rs index 2a676b30aea38..4d43e3a17a9ec 100644 --- a/frame/support/test/tests/pallet_ui/storage_info_unsatisfied.rs +++ b/frame/support/test/tests/pallet_ui/storage_info_unsatisfied.rs @@ -1,4 +1,4 @@ -i#[frame_support::pallet] +#[frame_support::pallet] mod pallet { use frame_support::pallet_prelude::{Hooks, StorageValue}; use frame_system::pallet_prelude::BlockNumberFor; From 6ece8602b5bc851aefb5c3b5a956d8bca516b232 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 10 May 2023 23:42:25 +0200 Subject: [PATCH 03/40] Remove stale docs Signed-off-by: Oliver Tale-Yazdi --- frame/support/src/storage/mod.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 08a35c66d9c8f..573c22a106d4d 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -170,8 +170,6 @@ pub trait StoragePagedList { pub trait IterableStorageList { type Iterator: Iterator; - /// Enumerate all elements in the map in lexicographical order of the encoded key. If you - /// alter the map while doing this, you'll get undefined results. fn iter() -> Self::Iterator; fn drain() -> Self::Iterator; From c9f8a5cf5cc02e66d60e28e7f3944d4b63c62fef Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Thu, 11 May 2023 19:10:17 +0200 Subject: [PATCH 04/40] Add fuzzer tests Signed-off-by: Oliver Tale-Yazdi --- frame/support/storage-fuzzer/Cargo.toml | 21 +++ .../support/storage-fuzzer/src/paged_list.rs | 126 ++++++++++++++++++ 2 files changed, 147 insertions(+) create mode 100644 frame/support/storage-fuzzer/Cargo.toml create mode 100644 frame/support/storage-fuzzer/src/paged_list.rs diff --git a/frame/support/storage-fuzzer/Cargo.toml b/frame/support/storage-fuzzer/Cargo.toml new file mode 100644 index 0000000000000..8bc38b8f20034 --- /dev/null +++ b/frame/support/storage-fuzzer/Cargo.toml @@ -0,0 +1,21 @@ +[package] +name = "frame-support-storage-fuzzer" +version = "0.1.0" +authors = ["Parity Technologies "] +edition = "2021" +license = "Apache-2.0" +homepage = "https://substrate.io" +repository = "https://github.com/paritytech/substrate/" +description = "Fuzz FRAME storage types" +publish = false + +[[bin]] +name = "paged-list" +path = "src/paged_list.rs" + +[dependencies] +arbitrary = "1.3.0" +honggfuzz = "0.5.49" + +frame-support = { path = "../", default-features = false, features = ["std"] } +sp-io = { path = "../../../primitives/io", default-features = false, features = ["std"] } diff --git a/frame/support/storage-fuzzer/src/paged_list.rs b/frame/support/storage-fuzzer/src/paged_list.rs new file mode 100644 index 0000000000000..41fb8382b52fe --- /dev/null +++ b/frame/support/storage-fuzzer/src/paged_list.rs @@ -0,0 +1,126 @@ +// 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. + +//! # Running +//! Running this fuzzer can be done with `cargo hfuzz run paged-list`. `honggfuzz` CLI options can +//! be used by setting `HFUZZ_RUN_ARGS`, such as `-n 4` to use 4 threads. +//! +//! # Debugging a panic +//! Once a panic is found, it can be debugged with +//! `cargo hfuzz run-debug paged-list hfuzz_workspace/paged-list/*.fuzz`. +//! +//! # More information +//! More information about `honggfuzz` can be found +//! [here](https://docs.rs/honggfuzz/). + +use arbitrary::Arbitrary; +use honggfuzz::fuzz; + +fn main() { + loop { + fuzz!(|data: (Vec, u8)| { + drain_append_work(data.0, data.1); + }); + } +} + +/// Appends and drains random number of elements in random order and checks storage invariants. +fn drain_append_work(ops: Vec, page_size: u8) { + use mock::*; + if page_size == 0 { + return; + } + + TestExternalities::default().execute_with(|| { + ValuesPerPage::set(&page_size.into()); + let _g = StorageNoopGuard::default(); + let mut total: i64 = 0; + + for op in ops.into_iter() { + total += op.exec(); + + assert!(total >= 0); + assert_eq!(List::iter().count(), total as usize); + + // We have the assumption that the queue removes the metadata when being empty. + if total == 0 { + assert_eq!(List::drain().count(), 0); + assert_eq!(List::read_meta().unwrap_or_default(), Default::default()); + } + } + + assert_eq!(List::drain().count(), total as usize); + // No storage leaking (checked by `StorageNoopGuard`). + }); +} + +enum Op { + Append(Vec), + Drain(u8), +} + +impl Arbitrary<'_> for Op { + fn arbitrary(u: &mut arbitrary::Unstructured<'_>) -> arbitrary::Result { + if u.arbitrary::()? { + Ok(Op::Append(Vec::::arbitrary(u)?)) + } else { + Ok(Op::Drain(u.arbitrary::()?)) + } + } +} + +impl Op { + pub fn exec(self) -> i64 { + use mock::*; + + match self { + Op::Append(v) => { + let l = v.len(); + List::append_many(v); + l as i64 + }, + Op::Drain(v) => -(List::drain().take(v as usize).count() as i64), + } + } +} + +#[allow(dead_code)] +mod mock { + pub use frame_support::{ + metadata_ir::{StorageEntryModifierIR, StorageEntryTypeIR, StorageHasherIR}, + parameter_types, + storage::{types::StoragePagedList, StoragePagedList as _, TestingStoragePagedList as _}, + traits::StorageInstance, + Blake2_128Concat, StorageNoopGuard, + }; + pub use sp_io::TestExternalities; + + parameter_types! { + pub storage ValuesPerPage: u32 = 10; + pub const MaxPages: Option = Some(20); + } + + pub struct Prefix; + impl StorageInstance for Prefix { + fn pallet_prefix() -> &'static str { + "test" + } + const STORAGE_PREFIX: &'static str = "foo"; + } + + pub type List = StoragePagedList; +} From da9deb34f7218ef3a9854b6bc3cb8979cbd6f3a3 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Thu, 11 May 2023 19:10:24 +0200 Subject: [PATCH 05/40] Update Signed-off-by: Oliver Tale-Yazdi --- Cargo.lock | 10 ++ Cargo.toml | 1 + frame/support/src/storage/mod.rs | 66 ++++++- frame/support/src/storage/types/paged_list.rs | 166 ++++++++++++------ 4 files changed, 184 insertions(+), 59 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3c6c2496963b5..b45ce3c53fb3d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2751,6 +2751,16 @@ dependencies = [ "syn 2.0.15", ] +[[package]] +name = "frame-support-storage-fuzzer" +version = "0.1.0" +dependencies = [ + "arbitrary", + "frame-support", + "honggfuzz", + "sp-io", +] + [[package]] name = "frame-support-test" version = "3.0.0" diff --git a/Cargo.toml b/Cargo.toml index da5e1d532a970..24806132409cc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -158,6 +158,7 @@ members = [ "frame/support/procedural", "frame/support/procedural/tools", "frame/support/procedural/tools/derive", + "frame/support/storage-fuzzer", "frame/support/test", "frame/support/test/compile_pass", "frame/support/test/pallet", diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 573c22a106d4d..8439c9eff5c29 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -161,18 +161,70 @@ pub trait StorageValue { } pub trait StoragePagedList { - fn append(item: EncodeLikeItem) - where - Item: Encode, - EncodeLikeItem: EncodeLike; -} - -pub trait IterableStorageList { type Iterator: Iterator; + type Appendix: StorageAppendix; + /// List all elements in append order. fn iter() -> Self::Iterator; + /// Drain the elements. + /// + /// Note that this drains all inspected values. For example `take_while(|_| false)` will drain + /// the first element. This also applies to `peek()`. fn drain() -> Self::Iterator; + + /// A fast append iterator. + fn appendix() -> Self::Appendix; + + /// Append a single element. + /// + /// Should not be called repeatedly; use `append_many` instead. + fn append_one(item: EncodeLikeValue) + where + EncodeLikeValue: EncodeLike, + { + Self::append_many(core::iter::once(item)); + } + + /// Append many elements. + /// + /// Should not be called repeatedly; use `appendix` instead. + fn append_many(items: I) + where + EncodeLikeValue: EncodeLike, + I: IntoIterator, + { + let mut ap = Self::appendix(); + ap.append_many(items); + } +} + +#[cfg(feature = "std")] +pub trait TestingStoragePagedList { + type Metadata; + + fn read_meta() -> Option; + fn clear_meta(); +} + +/// A fast append iterator. +/// +/// Makes sense in situations where appending does not have constant time complexity. +pub trait StorageAppendix { + fn append(&mut self, item: EncodeLikeValue) + where + EncodeLikeValue: EncodeLike; + + // Note: this has a default impl, as an `Appendix` is already assumed to be fast for appending. + fn append_many(&mut self, items: I) + where + EncodeLikeValue: EncodeLike, + I: IntoIterator, + { + for item in items.into_iter() { + self.append(item); + } + } } /// A strongly-typed map in storage. diff --git a/frame/support/src/storage/types/paged_list.rs b/frame/support/src/storage/types/paged_list.rs index 136f63e639ddd..709e21d8b2a1d 100644 --- a/frame/support/src/storage/types/paged_list.rs +++ b/frame/support/src/storage/types/paged_list.rs @@ -21,13 +21,14 @@ #![allow(unused_imports)] // FAIL-CI use crate::{ - defensive, + assert_storage_noop, defensive, storage::{KeyLenOf, StorageAppend, StorageDecodeLength, StoragePrefixedMap, StorageTryAppend}, traits::{Defensive, Get, GetDefault, StorageInfo, StorageInstance}, CloneNoBound, DebugNoBound, DefaultNoBound, EqNoBound, PartialEqNoBound, StorageHasher, StorageNoopGuard, Twox128, }; use codec::{Decode, Encode, EncodeLike, FullCodec, MaxEncodedLen}; +use core::iter::Peekable; use sp_arithmetic::traits::{SaturatedConversion, Saturating}; use sp_std::prelude::*; @@ -58,6 +59,22 @@ pub struct StoragePagedListMeta { _phantom: core::marker::PhantomData<(Prefix, Value, Hasher, ValuesPerPage)>, } +impl crate::storage::StorageAppendix + for StoragePagedListMeta +where + Prefix: StorageInstance, + Value: FullCodec, + Hasher: StorageHasher, + ValuesPerPage: Get, +{ + fn append(&mut self, item: EncodeLikeValue) + where + EncodeLikeValue: EncodeLike, + { + self.append_one(item); + } +} + impl StoragePagedListMeta where @@ -68,17 +85,21 @@ where { pub fn from_storage() -> Option { let key = Self::key(); - let raw = sp_io::storage::get(key.as_ref()).unwrap_or_default(); - Self::decode(&mut &raw[..]).ok() + + if let Some(raw) = sp_io::storage::get(key.as_ref()) { + Self::decode(&mut &raw[..]).ok() + } else { + None + } } pub fn key() -> Hasher::Output { (Prefix::STORAGE_PREFIX, b"paged_list_state").using_encoded(Hasher::hash) } - pub fn append(&mut self, item: EncodeLikeItem) + pub fn append_one(&mut self, item: EncodeLikeValue) where - EncodeLikeItem: EncodeLike, + EncodeLikeValue: EncodeLike, { // Note: we use >= here in case someone decreased it in a runtime upgrade. if self.current_value >= ValuesPerPage::get() { @@ -99,7 +120,11 @@ where self.using_encoded(|encoded| sp_io::storage::set(key.as_ref(), encoded)); } - // only good for testing. + pub fn reset(&mut self) { + *self = Default::default(); + Self::clear(); + } + pub fn clear() { let key = Self::key(); sp_io::storage::clear(key.as_ref()); @@ -219,33 +244,31 @@ where } let Some(mut page) = Page::from_storage::(page.index.saturating_add(1)) else { + self.page = None; if self.drain { - // All is gone - back to zero. - self.meta = StoragePagedListMeta::default(); - self.meta.store(); + self.meta.reset(); } - // No more pages, this is fine. return None; }; if let Some(val) = page.next() { + self.page = Some(page); if self.drain { self.meta.first_value.saturating_inc(); self.meta.store(); } - self.page = Some(page); return Some(val) } defensive!("StoragePagedListIterator: empty pages do not exist: storage corrupted"); - self.meta = StoragePagedListMeta::default(); - self.meta.store(); + // Put the storage back into a consistent state. + self.meta.reset(); self.page = None; None } } -impl crate::storage::IterableStorageList +impl crate::storage::StoragePagedList for StoragePagedList where Prefix: StorageInstance, @@ -255,6 +278,7 @@ where MaxPages: Get>, { type Iterator = StoragePagedListIterator; + type Appendix = StoragePagedListMeta; fn iter() -> Self::Iterator { StoragePagedListIterator::from_meta(Self::read_meta(), false) @@ -263,6 +287,31 @@ where fn drain() -> Self::Iterator { StoragePagedListIterator::from_meta(Self::read_meta(), true) } + + fn appendix() -> Self::Appendix { + Self::appendix() + } +} + +#[cfg(feature = "std")] +impl crate::storage::TestingStoragePagedList + for StoragePagedList +where + Prefix: StorageInstance, + Value: FullCodec + Clone, + Hasher: StorageHasher, + ValuesPerPage: Get, + MaxPages: Get>, +{ + type Metadata = StoragePagedListMeta; + + fn read_meta() -> Option { + Self::Metadata::from_storage() + } + + fn clear_meta() { + Self::Metadata::clear() + } } impl @@ -285,25 +334,39 @@ where pub fn appendix() -> StoragePagedListMeta { Self::read_meta() } + + // convenience stuff for tests + #[cfg(feature = "std")] + pub fn as_vec() -> Vec { + >::iter().collect() + } + + // convenience stuff for tests + #[cfg(feature = "std")] + pub fn drain_as_vec() -> Vec { + >::drain().collect() + } } -#[cfg(test)] -mod tests { - use super::*; - use crate::{ +/// Prelude for doc-tests. +#[cfg(feature = "std")] +#[allow(dead_code)] +pub(crate) mod test_predlude { + pub use super::*; + pub use crate::{ hash::*, metadata_ir::{StorageEntryModifierIR, StorageEntryTypeIR, StorageHasherIR}, parameter_types, - storage::{types::ValueQuery, IterableStorageList}, + storage::{types::ValueQuery, StoragePagedList as _}, }; - use sp_io::{hashing::twox_128, TestExternalities}; + pub use sp_io::{hashing::twox_128, TestExternalities}; parameter_types! { pub const ValuesPerPage: u32 = 5; pub const MaxPages: Option = Some(20); } - struct Prefix; + pub struct Prefix; impl StorageInstance for Prefix { fn pallet_prefix() -> &'static str { "test" @@ -311,21 +374,18 @@ mod tests { const STORAGE_PREFIX: &'static str = "foo"; } - type List = StoragePagedList; + pub type List = StoragePagedList; +} + +#[cfg(test)] +mod tests { + use super::test_predlude::*; #[test] fn append_works() { TestExternalities::default().execute_with(|| { - let mut append_iter = List::appendix(); - for i in 0..1000 { - append_iter.append(i); - } - - let vals = List::iter().collect::>(); - assert_eq!(vals, (0..1000).collect::>()); - - let meta = List::read_meta(); - assert_eq!(meta.first_page, 0); + List::append_many(0..1000); + assert_eq!(List::as_vec(), (0..1000).collect::>()); }); } @@ -334,19 +394,14 @@ mod tests { fn simple_drain_works() { TestExternalities::default().execute_with(|| { let _g = StorageNoopGuard::default(); // All in all a No-Op - let mut append_iter = List::appendix(); - for i in 0..1000 { - append_iter.append(i); - } + List::append_many(0..1000); - let vals = List::drain().collect::>(); - assert_eq!(vals, (0..1000).collect::>()); + assert_eq!(List::drain_as_vec(), (0..1000).collect::>()); assert_eq!(List::read_meta(), Default::default()); // all gone - let vals = List::iter().collect::>(); - assert_eq!(vals, Vec::::new()); + assert_eq!(List::as_vec(), Vec::::new()); // Need to delete the metadata manually. StoragePagedListMeta::::clear(); }); @@ -356,10 +411,7 @@ mod tests { #[test] fn partial_drain_works() { TestExternalities::default().execute_with(|| { - let mut append_iter = List::appendix(); - for i in 0..100 { - append_iter.append(i); - } + List::append_many(0..100); let vals = List::drain().take(50).collect::>(); assert_eq!(vals, (0..50).collect::>()); @@ -369,8 +421,7 @@ mod tests { assert_eq!((meta.first_page, meta.first_value), (9, 5)); // 50 gone, 50 to go - let vals = List::iter().collect::>(); - assert_eq!(vals, (50..100).collect::>()); + assert_eq!(List::as_vec(), (50..100).collect::>()); }); } @@ -381,17 +432,28 @@ mod tests { TestExternalities::default().execute_with(|| { for r in 1..=100 { - log::warn!("round {}", r); - let mut appendix = List::appendix(); - (0..12).for_each(|i| appendix.append(i)); - (0..12).for_each(|i| appendix.append(i)); + List::append_many(0..12); + List::append_many(0..12); let dropped = List::drain().take(12).collect::>(); assert_eq!(dropped, (0..12).collect::>()); - let all = List::iter().collect::>(); - assert_eq!(all, (0..12).cycle().take(r * 12).collect::>()); + assert_eq!(List::as_vec(), (0..12).cycle().take(r * 12).collect::>()); } }); } + + #[test] + fn peekable_drain_also_deletes() { + sp_tracing::try_init_simple(); + + TestExternalities::default().execute_with(|| { + List::append_many(0..10); + + let mut iter = List::drain().peekable(); + assert_eq!(iter.peek(), Some(&0)); + // `peek` does remove one element... + assert_eq!(List::iter().count(), 9); + }); + } } From 8a8acaa865510e194ac6dde5cf3e271dc7f1f92f Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Thu, 11 May 2023 19:13:34 +0200 Subject: [PATCH 06/40] Review Co-authored-by: Koute Signed-off-by: Oliver Tale-Yazdi --- frame/support/src/storage/types/paged_list.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/frame/support/src/storage/types/paged_list.rs b/frame/support/src/storage/types/paged_list.rs index 709e21d8b2a1d..74f11b6b3d093 100644 --- a/frame/support/src/storage/types/paged_list.rs +++ b/frame/support/src/storage/types/paged_list.rs @@ -184,12 +184,11 @@ impl Iterator for Page { pub struct StoragePagedListIterator { // Design: we put the Page into the iterator to have fewer storage look-ups. Yes, these // look-ups would be cached anyway, but bugging the overlay on each `.next` call still seems - // like a poor trade-off than caching it in the iterator directly. Note: if Page is empty then + // like a poor trade-off than caching it in the iterator directly. Iterating and modifying is not allowed at the same time anyway, just like with maps. Note: if Page is empty then // the iterator did not find any data upon setup or ran out of pages. page: Option>, drain: bool, meta: StoragePagedListMeta, - _phantom: core::marker::PhantomData<(Prefix, Value, Hasher, ValuesPerPage)>, } impl @@ -225,9 +224,8 @@ where type Item = Value; fn next(&mut self) -> Option { - let Some(ref mut page) = self.page else { - return None; - }; + let page = self.page.as_mut()?; + if let Some(val) = page.next() { if self.drain { self.meta.first_value.saturating_inc(); From caff2a252ea35077f039e8d0aa369d2abad145c8 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Thu, 11 May 2023 19:14:22 +0200 Subject: [PATCH 07/40] fmt Signed-off-by: Oliver Tale-Yazdi --- frame/support/src/storage/types/paged_list.rs | 5 +++-- frame/support/storage-fuzzer/src/paged_list.rs | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/frame/support/src/storage/types/paged_list.rs b/frame/support/src/storage/types/paged_list.rs index 74f11b6b3d093..c6ace6542bc30 100644 --- a/frame/support/src/storage/types/paged_list.rs +++ b/frame/support/src/storage/types/paged_list.rs @@ -184,7 +184,8 @@ impl Iterator for Page { pub struct StoragePagedListIterator { // Design: we put the Page into the iterator to have fewer storage look-ups. Yes, these // look-ups would be cached anyway, but bugging the overlay on each `.next` call still seems - // like a poor trade-off than caching it in the iterator directly. Iterating and modifying is not allowed at the same time anyway, just like with maps. Note: if Page is empty then + // like a poor trade-off than caching it in the iterator directly. Iterating and modifying is + // not allowed at the same time anyway, just like with maps. Note: if Page is empty then // the iterator did not find any data upon setup or ran out of pages. page: Option>, drain: bool, @@ -209,7 +210,7 @@ where page.current = meta.first_value; } - Self { page, drain, meta, _phantom: Default::default() } + Self { page, drain, meta } } } diff --git a/frame/support/storage-fuzzer/src/paged_list.rs b/frame/support/storage-fuzzer/src/paged_list.rs index 41fb8382b52fe..afdcbb54f5c93 100644 --- a/frame/support/storage-fuzzer/src/paged_list.rs +++ b/frame/support/storage-fuzzer/src/paged_list.rs @@ -42,7 +42,7 @@ fn main() { fn drain_append_work(ops: Vec, page_size: u8) { use mock::*; if page_size == 0 { - return; + return } TestExternalities::default().execute_with(|| { From 630784d7dec73185fe9296482d58f0bdaef7b75b Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Thu, 11 May 2023 19:54:52 +0200 Subject: [PATCH 08/40] Docs and clippy Signed-off-by: Oliver Tale-Yazdi --- frame/support/src/storage/mod.rs | 17 ++- frame/support/src/storage/types/paged_list.rs | 119 +++++++++--------- 2 files changed, 79 insertions(+), 57 deletions(-) diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 8439c9eff5c29..89484316f1dd8 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -199,12 +199,27 @@ pub trait StoragePagedList { } } +/// Convenience trait for testing. #[cfg(feature = "std")] -pub trait TestingStoragePagedList { +pub trait TestingStoragePagedList: StoragePagedList { + /// Expose the metadata of the list. Normally this is inaccessible. type Metadata; + /// Read the metadata of the list from storage. fn read_meta() -> Option; + + /// Clear the metadata of the list from storage. fn clear_meta(); + + /// Return the elements of the list. + fn as_vec() -> Vec { + >::iter().collect() + } + + /// Remove and return all elements of the list. + fn as_drained_vec() -> Vec { + >::drain().collect() + } } /// A fast append iterator. diff --git a/frame/support/src/storage/types/paged_list.rs b/frame/support/src/storage/types/paged_list.rs index c6ace6542bc30..05fe7751ee81c 100644 --- a/frame/support/src/storage/types/paged_list.rs +++ b/frame/support/src/storage/types/paged_list.rs @@ -15,48 +15,58 @@ // See the License for the specific language governing permissions and // limitations under the License. -//! Storage map type. Implements StorageMap, StorageIterableMap, StoragePrefixedMap traits and their -//! methods directly. - -#![allow(unused_imports)] // FAIL-CI +// links are better than no links - even when they refer to private stuff. +#![allow(rustdoc::private_intra_doc_links)] use crate::{ - assert_storage_noop, defensive, - storage::{KeyLenOf, StorageAppend, StorageDecodeLength, StoragePrefixedMap, StorageTryAppend}, - traits::{Defensive, Get, GetDefault, StorageInfo, StorageInstance}, + defensive, + traits::{Get, GetDefault, StorageInstance}, CloneNoBound, DebugNoBound, DefaultNoBound, EqNoBound, PartialEqNoBound, StorageHasher, - StorageNoopGuard, Twox128, }; -use codec::{Decode, Encode, EncodeLike, FullCodec, MaxEncodedLen}; -use core::iter::Peekable; -use sp_arithmetic::traits::{SaturatedConversion, Saturating}; +use codec::{Decode, Encode, EncodeLike, FullCodec}; +use core::marker::PhantomData; +use sp_arithmetic::traits::Saturating; use sp_std::prelude::*; pub type PageIndex = u32; pub type ValueIndex = u32; -/// A paginated storage list that packs multiple elements per page. -/// -/// The map is lined by a storage lookup of `(prefix, page_index)`. See [`StoragePagedListMeta`]. +/// A paginated list that encodes multiple elements per page. /// -/// Supported APIs are iteration and appending. +/// The pages are indexed by a storage lookup of `(prefix, page_index)` done by +/// `Page::from_storage`. The state is managed by [`StoragePagedListMeta`]. pub struct StoragePagedList { - _phantom: core::marker::PhantomData<(Prefix, Hasher, Value, ValuesPerPage, MaxPages)>, + _phantom: PhantomData<(Prefix, Hasher, Value, ValuesPerPage, MaxPages)>, } -/// State info for book-keeping. Can be used as append-only iterator. +/// The state of a [`StoragePagedList`]. +/// +/// This struct doubles as [`crate::storage::StoragePagedList::Appendix`]. #[derive( Encode, Decode, CloneNoBound, PartialEqNoBound, EqNoBound, DebugNoBound, DefaultNoBound, )] // todo ignore scale bounds pub struct StoragePagedListMeta { - first_page: PageIndex, // can be >0 when pages were delete. - first_value: ValueIndex, // inside `first_page` + /// The first page that contains a value. + /// + /// Can be >0 when pages were deleted. + first_page: PageIndex, + /// The first value inside `first_page` that contains a value. + /// + /// Can be >0 when values were deleted. + first_value: ValueIndex, - current_page: PageIndex, // next one is free, always >= first_page - current_value: ValueIndex, // inside `current_page` + /// The last page that could contain data. + /// + /// Iteration starts at this page index. + last_page: PageIndex, + /// The last value inside `last_page` that could contain a value. + /// + /// Iteration starts at this index. If the page does not hold a value at this index, then the + /// whole list is empty. The only case where this can happen is when both are `0`. + last_value: ValueIndex, - _phantom: core::marker::PhantomData<(Prefix, Value, Hasher, ValuesPerPage)>, + _phantom: PhantomData<(Prefix, Value, Hasher, ValuesPerPage)>, } impl crate::storage::StorageAppendix @@ -102,13 +112,13 @@ where EncodeLikeValue: EncodeLike, { // Note: we use >= here in case someone decreased it in a runtime upgrade. - if self.current_value >= ValuesPerPage::get() { - self.current_value = 0; - self.current_page.saturating_inc(); + if self.last_value >= ValuesPerPage::get() { + self.last_value = 0; + self.last_page.saturating_inc(); } - let key = page_key::(self.current_page); - self.current_value.saturating_inc(); - log::info!("Appending to page/val {}/{}", self.current_page, self.current_value); + let key = page_key::(self.last_page); + self.last_value.saturating_inc(); + log::info!("Appending to page/val {}/{}", self.last_page, self.last_value); // Pages are `Vec` and therefore appendable. sp_io::storage::append(key.as_ref(), item.encode()); self.store(); @@ -131,40 +141,44 @@ where } } -// invariant: empty pages do not exist pub struct Page { - current: PageIndex, // current value for iteration + next: ValueIndex, index: PageIndex, + // invariant: this is **never** empty values: sp_std::vec::Vec, } impl Page { - fn from_storage( + /// Decode a page with `index` from storage. + pub fn from_storage( index: PageIndex, ) -> Option { let key = page_key::(index); let values = sp_io::storage::get(key.as_ref()) - .map(|raw| sp_std::vec::Vec::::decode(&mut &raw[..]).ok()) - .flatten(); + .and_then(|raw| sp_std::vec::Vec::::decode(&mut &raw[..]).ok()); let values = values.unwrap_or_default(); if values.is_empty() { None } else { - Some(Self { current: 0, index, values }) + Some(Self { next: 0, index, values }) } } + /// Delete this page from storage. pub fn delete(&self) { delete_page::(self.index); } } +/// Delete a page with `index` from storage. +// Does not live under `Page` since it does not require the `Value` generic. pub(crate) fn delete_page(index: PageIndex) { let key = page_key::(index); sp_io::storage::clear(key.as_ref()); } -// Note: does not live under `Page` since it does not require `Value`. +/// Storage key of a page with `index`. +// Does not live under `Page` since it does not require the `Value` generic. pub(crate) fn page_key( index: PageIndex, ) -> Hasher::Output { @@ -175,12 +189,15 @@ impl Iterator for Page { type Item = V; fn next(&mut self) -> Option { - let val = self.values.get(self.current as usize).cloned(); - self.current.saturating_inc(); + let val = self.values.get(self.next as usize).cloned(); + self.next.saturating_inc(); val } } +/// Iterates over values of a [`StoragePagedList`]. +/// +/// Can optionally drain the iterated values. pub struct StoragePagedListIterator { // Design: we put the Page into the iterator to have fewer storage look-ups. Yes, these // look-ups would be cached anyway, but bugging the overlay on each `.next` call still seems @@ -200,6 +217,7 @@ where Hasher: StorageHasher, ValuesPerPage: Get, { + /// Read self from the storage. pub fn from_meta( meta: StoragePagedListMeta, drain: bool, @@ -207,7 +225,7 @@ where let mut page = Page::::from_storage::(meta.first_page); if let Some(ref mut page) = page { - page.current = meta.first_value; + page.next = meta.first_value; } Self { page, drain, meta } @@ -322,7 +340,7 @@ where ValuesPerPage: Get, MaxPages: Get>, { - pub(crate) fn read_meta() -> StoragePagedListMeta { + fn read_meta() -> StoragePagedListMeta { // Use default here to not require a setup migration. StoragePagedListMeta::from_storage().unwrap_or_default() } @@ -330,33 +348,22 @@ where /// Provides a fast append iterator. /// /// The list should not be modified while appending. Also don't call it recursively. - pub fn appendix() -> StoragePagedListMeta { + fn appendix() -> StoragePagedListMeta { Self::read_meta() } - - // convenience stuff for tests - #[cfg(feature = "std")] - pub fn as_vec() -> Vec { - >::iter().collect() - } - - // convenience stuff for tests - #[cfg(feature = "std")] - pub fn drain_as_vec() -> Vec { - >::drain().collect() - } } /// Prelude for doc-tests. #[cfg(feature = "std")] #[allow(dead_code)] -pub(crate) mod test_predlude { +pub(crate) mod mock { pub use super::*; pub use crate::{ hash::*, metadata_ir::{StorageEntryModifierIR, StorageEntryTypeIR, StorageHasherIR}, parameter_types, - storage::{types::ValueQuery, StoragePagedList as _}, + storage::{types::ValueQuery, StoragePagedList as _, TestingStoragePagedList}, + StorageNoopGuard, }; pub use sp_io::{hashing::twox_128, TestExternalities}; @@ -378,7 +385,7 @@ pub(crate) mod test_predlude { #[cfg(test)] mod tests { - use super::test_predlude::*; + use super::mock::*; #[test] fn append_works() { @@ -395,7 +402,7 @@ mod tests { let _g = StorageNoopGuard::default(); // All in all a No-Op List::append_many(0..1000); - assert_eq!(List::drain_as_vec(), (0..1000).collect::>()); + assert_eq!(List::as_drained_vec(), (0..1000).collect::>()); assert_eq!(List::read_meta(), Default::default()); From 7760e970f850732c4a617f69e3144765f514324d Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Thu, 11 May 2023 22:15:09 +0200 Subject: [PATCH 09/40] Sum docs Signed-off-by: Oliver Tale-Yazdi --- frame/support/src/storage/mod.rs | 34 ++++++++----- frame/support/src/storage/types/paged_list.rs | 50 ++++++++++++++++--- 2 files changed, 66 insertions(+), 18 deletions(-) diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 89484316f1dd8..daa1188f2adea 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -160,17 +160,21 @@ pub trait StorageValue { } } -pub trait StoragePagedList { +/// A non-continuous container type. +pub trait StorageList { + /// Iterator for normal and draining iteration. type Iterator: Iterator; + + /// Append iterator for fast append operations. type Appendix: StorageAppendix; - /// List all elements in append order. + /// List the elements in append order. fn iter() -> Self::Iterator; - /// Drain the elements. + /// Drain the elements in append order. /// - /// Note that this drains all inspected values. For example `take_while(|_| false)` will drain - /// the first element. This also applies to `peek()`. + /// Note that this drains a value as soon as it is being inspected. For example `take_while(|_| + /// false)` still drains the first element. This also applies to `peek()`. fn drain() -> Self::Iterator; /// A fast append iterator. @@ -178,7 +182,8 @@ pub trait StoragePagedList { /// Append a single element. /// - /// Should not be called repeatedly; use `append_many` instead. + /// Should not be called repeatedly; use `append_many` instead. + /// Worst case linear `O(len)` with `len` being the number if elements in the list. fn append_one(item: EncodeLikeValue) where EncodeLikeValue: EncodeLike, @@ -188,7 +193,9 @@ pub trait StoragePagedList { /// Append many elements. /// - /// Should not be called repeatedly; use `appendix` instead. + /// Should not be called repeatedly; use `appendix` instead. + /// Worst case linear `O(len + items.count())` with `len` beings the number if elements in the + /// list. fn append_many(items: I) where EncodeLikeValue: EncodeLike, @@ -201,8 +208,8 @@ pub trait StoragePagedList { /// Convenience trait for testing. #[cfg(feature = "std")] -pub trait TestingStoragePagedList: StoragePagedList { - /// Expose the metadata of the list. Normally this is inaccessible. +pub trait TestingStoragePagedList { + /// Expose the metadata of the list. It is otherwise inaccessible. type Metadata; /// Read the metadata of the list from storage. @@ -222,15 +229,18 @@ pub trait TestingStoragePagedList: StoragePagedList { } } -/// A fast append iterator. +/// Append iterator to append values to a storage struct. /// -/// Makes sense in situations where appending does not have constant time complexity. +/// Can be used in situations where appending does not have constant time complexity. pub trait StorageAppendix { + /// Append a single item in constant time `O(1)`. fn append(&mut self, item: EncodeLikeValue) where EncodeLikeValue: EncodeLike; - // Note: this has a default impl, as an `Appendix` is already assumed to be fast for appending. + /// Append many items in linear time `O(items.count())`. + // Note: a default impl is provided since `Self` is already assumed to be optimal for single + // append operations. fn append_many(&mut self, items: I) where EncodeLikeValue: EncodeLike, diff --git a/frame/support/src/storage/types/paged_list.rs b/frame/support/src/storage/types/paged_list.rs index 05fe7751ee81c..5493a6c5461f4 100644 --- a/frame/support/src/storage/types/paged_list.rs +++ b/frame/support/src/storage/types/paged_list.rs @@ -15,6 +15,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +//! Paged storage list. + // links are better than no links - even when they refer to private stuff. #![allow(rustdoc::private_intra_doc_links)] @@ -31,17 +33,53 @@ use sp_std::prelude::*; pub type PageIndex = u32; pub type ValueIndex = u32; -/// A paginated list that encodes multiple elements per page. +/// A paginated storage list. +/// +/// # Motivation +/// +/// This type replaces `StorageValue>` in situations where only iteration and appending is +/// needed. There are a few places where this is the case. A paginated structure reduces the memory +/// usage when a storage transactions needs to be rolled back. The main motivation is therefore a +/// reduction of runtime memory on storage transaction rollback. Should be configured such that the +/// size of a page is about 64KiB. This can only be ensured when `V` implements `MaxEncodedLen`. +/// +/// # Implementation +/// +/// The metadata of this struct is stored in [`StoragePagedListMeta`]. The data is stored in +/// [`Page`]s. +/// +/// Each [`Page`] holds at most `ValuesPerPage` values in its `values` vector. The last page is the +/// only one that could have less than `ValuesPerPage` values. +/// **Iteration** happens by starting +/// at[`StoragePagedListMeta::first_page`]/[`StoragePagedListMeta::first_value`] and incrementing +/// these indices as long as there are elements in the page and there are pages in storage. All +/// elements of a page are loaded once a page is read from storage. Iteration then happens on the +/// cached elements. This reduces the storage `read` calls on the overlay. +/// **Appending** to the list happens by appending to the last page by utilizing +/// [`sp_io::storage::append`]. It allows to directly extend the elements of `values` vector of the +/// page without loading the whole vector from storage. A new page is instanced once [`Page::next`] +/// overflows `ValuesPerPage`. Its vector will also be created through [`sp_io::storage::append`]. +/// **Draining** advances the internal indices identical to Iteration. It additionally persists the +/// increments to storage and thereby 'drains' elements. Completely drained pages are deleted from +/// storage. +/// +/// # Further Observations /// -/// The pages are indexed by a storage lookup of `(prefix, page_index)` done by -/// `Page::from_storage`. The state is managed by [`StoragePagedListMeta`]. +/// - The encoded layout of a page is exactly its [`Page::values`]. The [`Page::next`] offset is +/// stored in the [`StoragePagedListMeta`] instead. There is no particular reason for this, +/// besides having all management state handy in one location. +/// - The PoV complexity of iterating compared to a `StorageValue>` is improved for +/// "shortish" iterations and worse for total iteration. The append complexity is identical in the +/// asymptotic case when using an `Appendix`, and worse in all. For example when appending just +/// one value. +/// - It does incur a read overhead on the host side as compared to a `StorageValue>`. pub struct StoragePagedList { _phantom: PhantomData<(Prefix, Hasher, Value, ValuesPerPage, MaxPages)>, } /// The state of a [`StoragePagedList`]. /// -/// This struct doubles as [`crate::storage::StoragePagedList::Appendix`]. +/// This struct doubles as [`crate::storage::StorageList::Appendix`]. #[derive( Encode, Decode, CloneNoBound, PartialEqNoBound, EqNoBound, DebugNoBound, DefaultNoBound, )] @@ -285,7 +323,7 @@ where } } -impl crate::storage::StoragePagedList +impl crate::storage::StorageList for StoragePagedList where Prefix: StorageInstance, @@ -362,7 +400,7 @@ pub(crate) mod mock { hash::*, metadata_ir::{StorageEntryModifierIR, StorageEntryTypeIR, StorageHasherIR}, parameter_types, - storage::{types::ValueQuery, StoragePagedList as _, TestingStoragePagedList}, + storage::{types::ValueQuery, StorageList as _, TestingStoragePagedList}, StorageNoopGuard, }; pub use sp_io::{hashing::twox_128, TestExternalities}; From 42a0e485893f122bf4d60bfb4f70c5a75c192d8b Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Thu, 11 May 2023 23:04:37 +0200 Subject: [PATCH 10/40] Cleanup Signed-off-by: Oliver Tale-Yazdi --- frame/support/src/storage/mod.rs | 9 +++++---- frame/support/src/storage/stream_iter.rs | 2 +- frame/support/src/storage/types/paged_list.rs | 19 ++++++++++++------- .../support/storage-fuzzer/src/paged_list.rs | 2 +- 4 files changed, 19 insertions(+), 13 deletions(-) diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index daa1188f2adea..72e45e7bd249a 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -206,9 +206,10 @@ pub trait StorageList { } } -/// Convenience trait for testing. +/// Convenience trait for testing internal details of [`types::StoragePagedList`]. +// FAIL-CI I am not sure if this is such a good idea, but dont see another way to allow external white-box testing the metadata. #[cfg(feature = "std")] -pub trait TestingStoragePagedList { +pub trait TestingStoragePagedList: StorageList { /// Expose the metadata of the list. It is otherwise inaccessible. type Metadata; @@ -220,12 +221,12 @@ pub trait TestingStoragePagedList { /// Return the elements of the list. fn as_vec() -> Vec { - >::iter().collect() + >::iter().collect() } /// Remove and return all elements of the list. fn as_drained_vec() -> Vec { - >::drain().collect() + >::drain().collect() } } diff --git a/frame/support/src/storage/stream_iter.rs b/frame/support/src/storage/stream_iter.rs index e784ebd14c52a..2205601938b88 100644 --- a/frame/support/src/storage/stream_iter.rs +++ b/frame/support/src/storage/stream_iter.rs @@ -134,7 +134,7 @@ impl ScaleContainerStreamIter { /// /// - `key`: Storage key of the container in the state. /// - /// Same as [`Self::try_new`], but logs a potential error and sets the length to `0`. + /// Same as [`Self::new_try`], but logs a potential error and sets the length to `0`. pub fn new(key: Vec) -> Self { let mut input = StorageInput::new(key); let length = if input.exists() { diff --git a/frame/support/src/storage/types/paged_list.rs b/frame/support/src/storage/types/paged_list.rs index 5493a6c5461f4..e417b1f18c5b6 100644 --- a/frame/support/src/storage/types/paged_list.rs +++ b/frame/support/src/storage/types/paged_list.rs @@ -19,6 +19,9 @@ // links are better than no links - even when they refer to private stuff. #![allow(rustdoc::private_intra_doc_links)] +#![deny(rustdoc::broken_intra_doc_links)] +#![deny(missing_docs)] +#![deny(unsafe_code)] use crate::{ defensive, @@ -51,7 +54,7 @@ pub type ValueIndex = u32; /// Each [`Page`] holds at most `ValuesPerPage` values in its `values` vector. The last page is the /// only one that could have less than `ValuesPerPage` values. /// **Iteration** happens by starting -/// at[`StoragePagedListMeta::first_page`]/[`StoragePagedListMeta::first_value`] and incrementing +/// at [`first_page`][StoragePagedListMeta::first_page]/[`first_value`][StoragePagedListMeta::first_value] and incrementing /// these indices as long as there are elements in the page and there are pages in storage. All /// elements of a page are loaded once a page is read from storage. Iteration then happens on the /// cached elements. This reduces the storage `read` calls on the overlay. @@ -134,15 +137,13 @@ where pub fn from_storage() -> Option { let key = Self::key(); - if let Some(raw) = sp_io::storage::get(key.as_ref()) { + sp_io::storage::get(key.as_ref()).and_then(|raw| { Self::decode(&mut &raw[..]).ok() - } else { - None - } + }) } pub fn key() -> Hasher::Output { - (Prefix::STORAGE_PREFIX, b"paged_list_state").using_encoded(Hasher::hash) + (Prefix::STORAGE_PREFIX, b"meta").using_encoded(Hasher::hash) } pub fn append_one(&mut self, item: EncodeLikeValue) @@ -220,7 +221,7 @@ pub(crate) fn delete_page(index: pub(crate) fn page_key( index: PageIndex, ) -> Hasher::Output { - (Prefix::STORAGE_PREFIX, index).using_encoded(Hasher::hash) + (Prefix::STORAGE_PREFIX, b"page", index).using_encoded(Hasher::hash) } impl Iterator for Page { @@ -268,6 +269,10 @@ where Self { page, drain, meta } } + + fn next_in_page() -> Option { + todo!() + } } impl Iterator diff --git a/frame/support/storage-fuzzer/src/paged_list.rs b/frame/support/storage-fuzzer/src/paged_list.rs index afdcbb54f5c93..1fb690f8363fe 100644 --- a/frame/support/storage-fuzzer/src/paged_list.rs +++ b/frame/support/storage-fuzzer/src/paged_list.rs @@ -103,7 +103,7 @@ mod mock { pub use frame_support::{ metadata_ir::{StorageEntryModifierIR, StorageEntryTypeIR, StorageHasherIR}, parameter_types, - storage::{types::StoragePagedList, StoragePagedList as _, TestingStoragePagedList as _}, + storage::{types::StoragePagedList, StorageList as _, TestingStoragePagedList as _}, traits::StorageInstance, Blake2_128Concat, StorageNoopGuard, }; From 38c0ca1b8852a042d5e95160fd2f77f5358154a1 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Fri, 12 May 2023 15:57:28 +0200 Subject: [PATCH 11/40] Undo WIP Signed-off-by: Oliver Tale-Yazdi --- frame/support/src/storage/types/paged_list.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/frame/support/src/storage/types/paged_list.rs b/frame/support/src/storage/types/paged_list.rs index e417b1f18c5b6..9e79e66849019 100644 --- a/frame/support/src/storage/types/paged_list.rs +++ b/frame/support/src/storage/types/paged_list.rs @@ -269,10 +269,6 @@ where Self { page, drain, meta } } - - fn next_in_page() -> Option { - todo!() - } } impl Iterator From 1a6d383a2df914f0ae3ebdc6a454c601143c6a51 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Fri, 12 May 2023 23:56:37 +0200 Subject: [PATCH 12/40] Add pallet-paged-list Signed-off-by: Oliver Tale-Yazdi --- Cargo.lock | 14 +++ Cargo.toml | 1 + frame/paged-list/Cargo.toml | 48 ++++++++++ frame/paged-list/README.md | 30 ++++++ frame/paged-list/src/lib.rs | 96 +++++++++++++++++++ frame/paged-list/src/mock.rs | 84 ++++++++++++++++ frame/paged-list/src/tests.rs | 33 +++++++ frame/support/src/lib.rs | 1 + frame/support/src/storage/mod.rs | 16 +++- frame/support/src/storage/types/paged_list.rs | 13 +-- 10 files changed, 327 insertions(+), 9 deletions(-) create mode 100644 frame/paged-list/Cargo.toml create mode 100644 frame/paged-list/README.md create mode 100644 frame/paged-list/src/lib.rs create mode 100644 frame/paged-list/src/mock.rs create mode 100644 frame/paged-list/src/tests.rs diff --git a/Cargo.lock b/Cargo.lock index f2ac20abc36c0..2c06539008977 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6908,6 +6908,20 @@ dependencies = [ "sp-std", ] +[[package]] +name = "pallet-paged-list" +version = "0.1.0" +dependencies = [ + "frame-benchmarking", + "frame-support", + "frame-system", + "parity-scale-codec", + "scale-info", + "sp-core", + "sp-io", + "sp-runtime", +] + [[package]] name = "pallet-preimage" version = "4.0.0-dev" diff --git a/Cargo.toml b/Cargo.toml index ed2911edb83ad..9e89013b054d6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -134,6 +134,7 @@ members = [ "frame/nomination-pools/benchmarking", "frame/nomination-pools/test-staking", "frame/nomination-pools/runtime-api", + "frame/paged-list", "frame/insecure-randomness-collective-flip", "frame/ranked-collective", "frame/recovery", diff --git a/frame/paged-list/Cargo.toml b/frame/paged-list/Cargo.toml new file mode 100644 index 0000000000000..026c6c9639c05 --- /dev/null +++ b/frame/paged-list/Cargo.toml @@ -0,0 +1,48 @@ +[package] +name = "pallet-paged-list" +version = "0.1.0" +description = "FRAME pallet that provides a paged list data structure." +authors = ["Parity Technologies "] +homepage = "https://substrate.io" +edition = "2021" +license = "Apache-2.0" +repository = "https://github.com/paritytech/substrate" + +[package.metadata.docs.rs] +targets = ["x86_64-unknown-linux-gnu"] + +[dependencies] +codec = { package = "parity-scale-codec", version = "3.0.0", default-features = false, features = [ "derive"] } +scale-info = { version = "2.0.0", default-features = false, features = ["derive"] } + +frame-benchmarking = { version = "4.0.0-dev", default-features = false, optional = true, path = "../benchmarking" } +frame-support = { version = "4.0.0-dev", default-features = false, path = "../support" } +frame-system = { version = "4.0.0-dev", default-features = false, path = "../system" } + +[dev-dependencies] +sp-core = { version = "7.0.0", path = "../../primitives/core" } +sp-io = { version = "7.0.0", path = "../../primitives/io" } +sp-runtime = { version = "7.0.0", path = "../../primitives/runtime" } + +[features] +default = ["std"] + +std = [ + "codec/std", + "frame-benchmarking?/std", + "frame-support/std", + "frame-system/std", + "scale-info/std", + "sp-runtime/std", +] + +runtime-benchmarks = [ + "frame-benchmarking/runtime-benchmarks", + "frame-support/runtime-benchmarks", + "frame-system/runtime-benchmarks", + "sp-runtime/runtime-benchmarks", +] + +try-runtime = [ + "frame-support/try-runtime" +] diff --git a/frame/paged-list/README.md b/frame/paged-list/README.md new file mode 100644 index 0000000000000..e4dc07fd4e752 --- /dev/null +++ b/frame/paged-list/README.md @@ -0,0 +1,30 @@ +# Pallet PagedList + +FRAME pallet that provides a paged list data structure. +(TODO add more). + +# Design Goals + +1. TODO + +# Design + +TODO + +# Examples + +```rust +use pallet_paged_list::Pallet as PagedList; + +fn main() { + +} +``` + +# License + +Apache-2.0 + +# References +This crate was auto generated by FRAMY CLI . +Please report bugs and build failures at . diff --git a/frame/paged-list/src/lib.rs b/frame/paged-list/src/lib.rs new file mode 100644 index 0000000000000..6523ff32c48e7 --- /dev/null +++ b/frame/paged-list/src/lib.rs @@ -0,0 +1,96 @@ +// 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. + +//! A minimal wrapper around the [`frame_support::storage::StoragePagedList`]. + +#![cfg_attr(not(feature = "std"), no_std)] +#![doc = include_str!("../README.md")] + +pub use pallet::*; + +mod mock; +mod tests; + +use codec::FullCodec; +use frame_support::traits::StorageInstance; +use frame_support::traits::PalletInfoAccess; +use frame_support::pallet_prelude::StorageList; + +#[frame_support::pallet] +pub mod pallet { + use super::*; + use frame_support::{pallet_prelude::*, storage::types::StoragePagedList}; + + #[pallet::pallet] + pub struct Pallet(_); + + /// This type alias is what FRAME normally get us. + pub type List = StoragePagedList< + ListPrefix, + Blake2_128Concat, + ::Value, + ::ValuesPerPage, + ::MaxPages, + >; + + #[pallet::config] + pub trait Config: frame_system::Config { + type RuntimeEvent: From> + IsType<::RuntimeEvent>; + + type Value: FullCodec + Clone + MaxEncodedLen; + + #[pallet::constant] + type ValuesPerPage: Get; + + #[pallet::constant] + type MaxPages: Get>; + } + + #[pallet::event] + pub enum Event {} + + #[pallet::call] + impl Pallet {} +} + +/// The storage prefix for the list. +pub struct ListPrefix(core::marker::PhantomData); + +impl StorageInstance for ListPrefix { + fn pallet_prefix() -> &'static str { + crate::Pallet::::module_name() // TODO double check + } + const STORAGE_PREFIX: &'static str = "list"; +} + +// Pass everything through to the underlying list. +impl StorageList for Pallet { + type Iterator = as StorageList>::Iterator; + type Appendix = as StorageList>::Appendix; + + fn iter() -> Self::Iterator { + List::::iter() + } + + fn drain() -> Self::Iterator { + List::::drain() + } + + fn appendix() -> Self::Appendix { + List::::appendix() + } +} diff --git a/frame/paged-list/src/mock.rs b/frame/paged-list/src/mock.rs new file mode 100644 index 0000000000000..112f3e72f4ca4 --- /dev/null +++ b/frame/paged-list/src/mock.rs @@ -0,0 +1,84 @@ +// 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. + +#![cfg(test)] + +use frame_support::traits::{ConstU16, ConstU64}; +use sp_core::H256; +use sp_runtime::{ + testing::Header, + traits::{BlakeTwo256, IdentityLookup}, +}; + +type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; +type Block = frame_system::mocking::MockBlock; + +// Configure a mock runtime to test the pallet. +frame_support::construct_runtime!( + pub enum Test where + Block = Block, + NodeBlock = Block, + UncheckedExtrinsic = UncheckedExtrinsic, + { + System: frame_system, + PagedList: crate, + } +); + +impl frame_system::Config for Test { + type BaseCallFilter = frame_support::traits::Everything; + type BlockWeights = (); + type BlockLength = (); + type DbWeight = (); + type RuntimeOrigin = RuntimeOrigin; + type RuntimeCall = RuntimeCall; + type Index = u64; + type BlockNumber = u64; + type Hash = H256; + type Hashing = BlakeTwo256; + type AccountId = u64; + type Lookup = IdentityLookup; + type Header = Header; + type RuntimeEvent = RuntimeEvent; + type BlockHashCount = ConstU64<250>; + type Version = (); + type PalletInfo = PalletInfo; + type AccountData = (); + type OnNewAccount = (); + type OnKilledAccount = (); + type SystemWeightInfo = (); + type SS58Prefix = ConstU16<42>; + type OnSetCode = (); + type MaxConsumers = frame_support::traits::ConstU32<16>; +} + +frame_support::parameter_types! { + pub const ValuesPerPage: u32 = 5; + pub const MaxPages: Option = Some(20); +} + +impl crate::Config for Test { + type RuntimeEvent = RuntimeEvent; + type Value = u32; + type ValuesPerPage = ValuesPerPage; + type MaxPages = MaxPages; +} + +// Build genesis storage according to the mock runtime. +pub fn new_test_ext() -> sp_io::TestExternalities { + frame_system::GenesisConfig::default().build_storage::().unwrap().into() +} diff --git a/frame/paged-list/src/tests.rs b/frame/paged-list/src/tests.rs new file mode 100644 index 0000000000000..6fd61e33d95fc --- /dev/null +++ b/frame/paged-list/src/tests.rs @@ -0,0 +1,33 @@ +// 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. + +#![cfg(test)] + +use crate::{mock::*, Pallet}; +use frame_support::{storage::StorageList}; + +#[test] +fn iter_works() { + new_test_ext().execute_with(|| { + Pallet::::append_many(0..1000); + + assert_eq!(Pallet::::iter().collect::>(), (0..1000).collect::>()); + // drain + assert_eq!(Pallet::::drain().collect::>(), (0..1000).collect::>()); + assert_eq!(Pallet::::iter().count(), 0); + }); +} diff --git a/frame/support/src/lib.rs b/frame/support/src/lib.rs index 3cd8378be45d1..bb56176a2942c 100644 --- a/frame/support/src/lib.rs +++ b/frame/support/src/lib.rs @@ -1538,6 +1538,7 @@ pub mod pallet_prelude { CountedStorageMap, Key as NMapKey, OptionQuery, ResultQuery, StorageDoubleMap, StorageMap, StorageNMap, StorageValue, ValueQuery, }, + StorageList, }, traits::{ ConstU32, EnsureOrigin, Get, GetDefault, GetStorageVersion, Hooks, IsType, diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 72e45e7bd249a..068b1fba098a8 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -1265,6 +1265,20 @@ impl Iterator for ChildTriePrefixIterator { } } +/// Trait for storage types that store all its value after a unique prefix. +pub trait StoragePrefixContainer { + /// Module prefix. Used for generating final key. + fn module_prefix() -> &'static [u8]; + + /// Storage prefix. Used for generating final key. + fn storage_prefix() -> &'static [u8]; + + /// Final full prefix that prefixes all keys. + fn final_prefix() -> [u8; 32] { + crate::storage::storage_prefix(Self::module_prefix(), Self::storage_prefix()) + } +} + /// Trait for maps that store all its value after a unique prefix. /// /// By default the final prefix is: @@ -1273,7 +1287,7 @@ impl Iterator for ChildTriePrefixIterator { /// ``` pub trait StoragePrefixedMap { /// Module prefix. Used for generating final key. - fn module_prefix() -> &'static [u8]; + fn module_prefix() -> &'static [u8]; // TODO move to StoragePrefixedContainer /// Storage prefix. Used for generating final key. fn storage_prefix() -> &'static [u8]; diff --git a/frame/support/src/storage/types/paged_list.rs b/frame/support/src/storage/types/paged_list.rs index 9e79e66849019..a149f58eaf757 100644 --- a/frame/support/src/storage/types/paged_list.rs +++ b/frame/support/src/storage/types/paged_list.rs @@ -91,21 +91,21 @@ pub struct StoragePagedListMeta { /// The first page that contains a value. /// /// Can be >0 when pages were deleted. - first_page: PageIndex, + pub first_page: PageIndex, /// The first value inside `first_page` that contains a value. /// /// Can be >0 when values were deleted. - first_value: ValueIndex, + pub first_value: ValueIndex, /// The last page that could contain data. /// /// Iteration starts at this page index. - last_page: PageIndex, + pub last_page: PageIndex, /// The last value inside `last_page` that could contain a value. /// /// Iteration starts at this index. If the page does not hold a value at this index, then the /// whole list is empty. The only case where this can happen is when both are `0`. - last_value: ValueIndex, + pub last_value: ValueIndex, _phantom: PhantomData<(Prefix, Value, Hasher, ValuesPerPage)>, } @@ -349,6 +349,7 @@ where } } + #[cfg(feature = "std")] impl crate::storage::TestingStoragePagedList for StoragePagedList @@ -473,8 +474,6 @@ mod tests { /// Draining, appending and iterating work together. #[test] fn drain_append_iter_works() { - sp_tracing::try_init_simple(); - TestExternalities::default().execute_with(|| { for r in 1..=100 { List::append_many(0..12); @@ -490,8 +489,6 @@ mod tests { #[test] fn peekable_drain_also_deletes() { - sp_tracing::try_init_simple(); - TestExternalities::default().execute_with(|| { List::append_many(0..10); From 1d82f6f271c9aeaaa64b0408f42a9c82e4556aca Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sat, 13 May 2023 15:24:02 +0200 Subject: [PATCH 13/40] Move code to pallet Signed-off-by: Oliver Tale-Yazdi --- Cargo.lock | 1 + frame/paged-list/Cargo.toml | 11 +- frame/paged-list/src/lib.rs | 71 +++++----- frame/paged-list/src/mock.rs | 10 +- .../types => paged-list/src}/paged_list.rs | 123 +++++++++++------- frame/paged-list/src/tests.rs | 26 +++- frame/support/src/storage/mod.rs | 26 +--- frame/support/src/storage/types/mod.rs | 2 - 8 files changed, 152 insertions(+), 118 deletions(-) rename frame/{support/src/storage/types => paged-list/src}/paged_list.rs (80%) diff --git a/Cargo.lock b/Cargo.lock index 2c06539008977..b445c1e0d01bf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6920,6 +6920,7 @@ dependencies = [ "sp-core", "sp-io", "sp-runtime", + "sp-std", ] [[package]] diff --git a/frame/paged-list/Cargo.toml b/frame/paged-list/Cargo.toml index 026c6c9639c05..3806219fa3840 100644 --- a/frame/paged-list/Cargo.toml +++ b/frame/paged-list/Cargo.toml @@ -18,11 +18,11 @@ scale-info = { version = "2.0.0", default-features = false, features = ["derive" frame-benchmarking = { version = "4.0.0-dev", default-features = false, optional = true, path = "../benchmarking" } frame-support = { version = "4.0.0-dev", default-features = false, path = "../support" } frame-system = { version = "4.0.0-dev", default-features = false, path = "../system" } +sp-runtime = { version = "7.0.0", default-features = false, path = "../../primitives/runtime" } +sp-std = { version = "5.0.0", default-features = false, path = "../../primitives/std" } -[dev-dependencies] -sp-core = { version = "7.0.0", path = "../../primitives/core" } -sp-io = { version = "7.0.0", path = "../../primitives/io" } -sp-runtime = { version = "7.0.0", path = "../../primitives/runtime" } +sp-core = { version = "7.0.0", path = "../../primitives/core", default-features = false, optional = true } +sp-io = { version = "7.0.0", path = "../../primitives/io", default-features = false, optional = true } [features] default = ["std"] @@ -33,7 +33,10 @@ std = [ "frame-support/std", "frame-system/std", "scale-info/std", + "sp-core/std", + "sp-io/std", "sp-runtime/std", + "sp-std/std", ] runtime-benchmarks = [ diff --git a/frame/paged-list/src/lib.rs b/frame/paged-list/src/lib.rs index 6523ff32c48e7..f80db3993dfdf 100644 --- a/frame/paged-list/src/lib.rs +++ b/frame/paged-list/src/lib.rs @@ -15,7 +15,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -//! A minimal wrapper around the [`frame_support::storage::StoragePagedList`]. +//! A minimal wrapper around the [`paged_list::StoragePagedList`]. #![cfg_attr(not(feature = "std"), no_std)] #![doc = include_str!("../README.md")] @@ -23,33 +23,37 @@ pub use pallet::*; mod mock; +mod paged_list; mod tests; use codec::FullCodec; -use frame_support::traits::StorageInstance; -use frame_support::traits::PalletInfoAccess; -use frame_support::pallet_prelude::StorageList; +use frame_support::{ + pallet_prelude::StorageList, + traits::{PalletInfoAccess, StorageInstance}, +}; +pub use paged_list::StoragePagedList; #[frame_support::pallet] pub mod pallet { use super::*; - use frame_support::{pallet_prelude::*, storage::types::StoragePagedList}; + use frame_support::pallet_prelude::*; #[pallet::pallet] - pub struct Pallet(_); + pub struct Pallet(_); /// This type alias is what FRAME normally get us. - pub type List = StoragePagedList< - ListPrefix, + pub type List = StoragePagedList< + ListPrefix, Blake2_128Concat, - ::Value, - ::ValuesPerPage, - ::MaxPages, + >::Value, + >::ValuesPerPage, + >::MaxPages, >; #[pallet::config] - pub trait Config: frame_system::Config { - type RuntimeEvent: From> + IsType<::RuntimeEvent>; + pub trait Config: frame_system::Config { + type RuntimeEvent: From> + + IsType<::RuntimeEvent>; type Value: FullCodec + Clone + MaxEncodedLen; @@ -61,36 +65,39 @@ pub mod pallet { } #[pallet::event] - pub enum Event {} + pub enum Event, I: 'static = ()> {} #[pallet::call] - impl Pallet {} + impl, I: 'static> Pallet {} } -/// The storage prefix for the list. -pub struct ListPrefix(core::marker::PhantomData); - -impl StorageInstance for ListPrefix { - fn pallet_prefix() -> &'static str { - crate::Pallet::::module_name() // TODO double check - } - const STORAGE_PREFIX: &'static str = "list"; -} - -// Pass everything through to the underlying list. -impl StorageList for Pallet { - type Iterator = as StorageList>::Iterator; - type Appendix = as StorageList>::Appendix; +// This exposes the list functionality to other pallets. +impl, I: 'static> StorageList for Pallet { + type Iterator = as StorageList>::Iterator; + type Appendix = as StorageList>::Appendix; fn iter() -> Self::Iterator { - List::::iter() + List::::iter() } fn drain() -> Self::Iterator { - List::::drain() + List::::drain() } fn appendix() -> Self::Appendix { - List::::appendix() + List::::appendix() } } + +/// The storage prefix for the list. +/// +/// Unique for each instance. +pub struct ListPrefix(core::marker::PhantomData<(T, I)>); + +impl, I: 'static> StorageInstance for ListPrefix { + fn pallet_prefix() -> &'static str { + crate::Pallet::::name() + } + + const STORAGE_PREFIX: &'static str = "paged_list"; +} diff --git a/frame/paged-list/src/mock.rs b/frame/paged-list/src/mock.rs index 112f3e72f4ca4..bfdb0d01a1f2a 100644 --- a/frame/paged-list/src/mock.rs +++ b/frame/paged-list/src/mock.rs @@ -35,7 +35,8 @@ frame_support::construct_runtime!( UncheckedExtrinsic = UncheckedExtrinsic, { System: frame_system, - PagedList: crate, + PagedList1: crate, + PagedList2: crate::, } ); @@ -78,6 +79,13 @@ impl crate::Config for Test { type MaxPages = MaxPages; } +impl crate::Config for Test { + type RuntimeEvent = RuntimeEvent; + type Value = u32; + type ValuesPerPage = ValuesPerPage; + type MaxPages = MaxPages; +} + // Build genesis storage according to the mock runtime. pub fn new_test_ext() -> sp_io::TestExternalities { frame_system::GenesisConfig::default().build_storage::().unwrap().into() diff --git a/frame/support/src/storage/types/paged_list.rs b/frame/paged-list/src/paged_list.rs similarity index 80% rename from frame/support/src/storage/types/paged_list.rs rename to frame/paged-list/src/paged_list.rs index a149f58eaf757..53859fe8c06b0 100644 --- a/frame/support/src/storage/types/paged_list.rs +++ b/frame/paged-list/src/paged_list.rs @@ -23,14 +23,16 @@ #![deny(missing_docs)] #![deny(unsafe_code)] -use crate::{ +use codec::{Decode, Encode, EncodeLike, FullCodec}; +use core::marker::PhantomData; +use frame_support::{ defensive, + storage::StoragePrefixedContainer, traits::{Get, GetDefault, StorageInstance}, - CloneNoBound, DebugNoBound, DefaultNoBound, EqNoBound, PartialEqNoBound, StorageHasher, + Blake2_128Concat, CloneNoBound, DebugNoBound, DefaultNoBound, EqNoBound, PartialEqNoBound, + StorageHasher, }; -use codec::{Decode, Encode, EncodeLike, FullCodec}; -use core::marker::PhantomData; -use sp_arithmetic::traits::Saturating; +use sp_runtime::traits::Saturating; use sp_std::prelude::*; pub type PageIndex = u32; @@ -54,17 +56,17 @@ pub type ValueIndex = u32; /// Each [`Page`] holds at most `ValuesPerPage` values in its `values` vector. The last page is the /// only one that could have less than `ValuesPerPage` values. /// **Iteration** happens by starting -/// at [`first_page`][StoragePagedListMeta::first_page]/[`first_value`][StoragePagedListMeta::first_value] and incrementing -/// these indices as long as there are elements in the page and there are pages in storage. All -/// elements of a page are loaded once a page is read from storage. Iteration then happens on the -/// cached elements. This reduces the storage `read` calls on the overlay. -/// **Appending** to the list happens by appending to the last page by utilizing -/// [`sp_io::storage::append`]. It allows to directly extend the elements of `values` vector of the -/// page without loading the whole vector from storage. A new page is instanced once [`Page::next`] -/// overflows `ValuesPerPage`. Its vector will also be created through [`sp_io::storage::append`]. -/// **Draining** advances the internal indices identical to Iteration. It additionally persists the -/// increments to storage and thereby 'drains' elements. Completely drained pages are deleted from -/// storage. +/// at [`first_page`][StoragePagedListMeta::first_page]/ +/// [`first_value`][StoragePagedListMeta::first_value] and incrementing these indices as long as +/// there are elements in the page and there are pages in storage. All elements of a page are loaded +/// once a page is read from storage. Iteration then happens on the cached elements. This reduces +/// the storage `read` calls on the overlay. **Appending** to the list happens by appending to the +/// last page by utilizing [`sp_io::storage::append`]. It allows to directly extend the elements of +/// `values` vector of the page without loading the whole vector from storage. A new page is +/// instanced once [`Page::next`] overflows `ValuesPerPage`. Its vector will also be created through +/// [`sp_io::storage::append`]. **Draining** advances the internal indices identical to Iteration. +/// It additionally persists the increments to storage and thereby 'drains' elements. Completely +/// drained pages are deleted from storage. /// /// # Further Observations /// @@ -82,7 +84,7 @@ pub struct StoragePagedList { _phantom: PhantomData<(Prefix, Value, Hasher, ValuesPerPage)>, } -impl crate::storage::StorageAppendix +impl frame_support::storage::StorageAppendix for StoragePagedListMeta where Prefix: StorageInstance, @@ -137,13 +139,11 @@ where pub fn from_storage() -> Option { let key = Self::key(); - sp_io::storage::get(key.as_ref()).and_then(|raw| { - Self::decode(&mut &raw[..]).ok() - }) + sp_io::storage::get(key.as_ref()).and_then(|raw| Self::decode(&mut &raw[..]).ok()) } pub fn key() -> Hasher::Output { - (Prefix::STORAGE_PREFIX, b"meta").using_encoded(Hasher::hash) + (StoragePagedListPrefix::::final_prefix(), b"meta").using_encoded(Hasher::hash) } pub fn append_one(&mut self, item: EncodeLikeValue) @@ -157,7 +157,6 @@ where } let key = page_key::(self.last_page); self.last_value.saturating_inc(); - log::info!("Appending to page/val {}/{}", self.last_page, self.last_value); // Pages are `Vec` and therefore appendable. sp_io::storage::append(key.as_ref(), item.encode()); self.store(); @@ -165,7 +164,6 @@ where pub fn store(&self) { let key = Self::key(); - log::info!("Storing: {:?}", &self); self.using_encoded(|encoded| sp_io::storage::set(key.as_ref(), encoded)); } @@ -221,7 +219,7 @@ pub(crate) fn delete_page(index: pub(crate) fn page_key( index: PageIndex, ) -> Hasher::Output { - (Prefix::STORAGE_PREFIX, b"page", index).using_encoded(Hasher::hash) + (StoragePagedListPrefix::::final_prefix(), b"page", index).using_encoded(Hasher::hash) } impl Iterator for Page { @@ -324,7 +322,7 @@ where } } -impl crate::storage::StorageList +impl frame_support::storage::StorageList for StoragePagedList where Prefix: StorageInstance, @@ -349,10 +347,8 @@ where } } - -#[cfg(feature = "std")] -impl crate::storage::TestingStoragePagedList - for StoragePagedList +impl + StoragePagedList where Prefix: StorageInstance, Value: FullCodec + Clone, @@ -360,19 +356,57 @@ where ValuesPerPage: Get, MaxPages: Get>, { - type Metadata = StoragePagedListMeta; + fn read_meta() -> StoragePagedListMeta { + // Use default here to not require a setup migration. + StoragePagedListMeta::from_storage().unwrap_or_default() + } + + /// Provides a fast append iterator. + /// + /// The list should not be modified while appending. Also don't call it recursively. + fn appendix() -> StoragePagedListMeta { + Self::read_meta() + } - fn read_meta() -> Option { - Self::Metadata::from_storage() + /// Return the elements of the list. + #[cfg(feature = "std")] + #[allow(dead_code)] + fn as_vec() -> Vec { + >::iter().collect() } - fn clear_meta() { - Self::Metadata::clear() + /// Remove and return all elements of the list. + #[cfg(feature = "std")] + #[allow(dead_code)] + fn as_drained_vec() -> Vec { + >::drain().collect() + } +} + +/// Provides the final prefix for a [`StoragePagedList`]. +/// +/// It solely exists so that when re-using it from the iterator and meta struct, none of the un-used +/// generics bleed through. Otherwise when only having the `StoragePrefixedContainer` implementation +/// on the list directly, the iterator and metadata need to muster *all* generics, even the ones +/// that are completely useless for prefix calculation. +struct StoragePagedListPrefix(PhantomData); + +impl frame_support::storage::StoragePrefixedContainer for StoragePagedListPrefix +where + Prefix: StorageInstance, +{ + fn module_prefix() -> &'static [u8] { + Prefix::pallet_prefix().as_bytes() + } + + fn storage_prefix() -> &'static [u8] { + Prefix::STORAGE_PREFIX.as_bytes() } } impl - StoragePagedList + frame_support::storage::StoragePrefixedContainer + for StoragePagedList where Prefix: StorageInstance, Value: FullCodec + Clone, @@ -380,16 +414,12 @@ where ValuesPerPage: Get, MaxPages: Get>, { - fn read_meta() -> StoragePagedListMeta { - // Use default here to not require a setup migration. - StoragePagedListMeta::from_storage().unwrap_or_default() + fn module_prefix() -> &'static [u8] { + StoragePagedListPrefix::::module_prefix() } - /// Provides a fast append iterator. - /// - /// The list should not be modified while appending. Also don't call it recursively. - fn appendix() -> StoragePagedListMeta { - Self::read_meta() + fn storage_prefix() -> &'static [u8] { + StoragePagedListPrefix::::storage_prefix() } } @@ -398,11 +428,10 @@ where #[allow(dead_code)] pub(crate) mod mock { pub use super::*; - pub use crate::{ - hash::*, + pub use frame_support::{ metadata_ir::{StorageEntryModifierIR, StorageEntryTypeIR, StorageHasherIR}, parameter_types, - storage::{types::ValueQuery, StorageList as _, TestingStoragePagedList}, + storage::{types::ValueQuery, StorageList as _}, StorageNoopGuard, }; pub use sp_io::{hashing::twox_128, TestExternalities}; diff --git a/frame/paged-list/src/tests.rs b/frame/paged-list/src/tests.rs index 6fd61e33d95fc..cdaa7ccac6c03 100644 --- a/frame/paged-list/src/tests.rs +++ b/frame/paged-list/src/tests.rs @@ -17,17 +17,29 @@ #![cfg(test)] -use crate::{mock::*, Pallet}; -use frame_support::{storage::StorageList}; +use crate::{mock::*, *}; +use frame_support::storage::{StorageList, StoragePrefixedContainer}; #[test] -fn iter_works() { +fn iter_independent_works() { new_test_ext().execute_with(|| { - Pallet::::append_many(0..1000); + PagedList1::append_many(0..1000); + PagedList2::append_many(0..1000); + + assert_eq!(PagedList1::iter().collect::>(), (0..1000).collect::>()); + assert_eq!(PagedList1::iter().collect::>(), (0..1000).collect::>()); - assert_eq!(Pallet::::iter().collect::>(), (0..1000).collect::>()); // drain - assert_eq!(Pallet::::drain().collect::>(), (0..1000).collect::>()); - assert_eq!(Pallet::::iter().count(), 0); + assert_eq!(PagedList1::drain().collect::>(), (0..1000).collect::>()); + assert_eq!(PagedList2::iter().collect::>(), (0..1000).collect::>()); + + assert_eq!(PagedList1::iter().count(), 0); }); } + +#[test] +fn prefix_distinct() { + let p1 = List::::final_prefix(); + let p2 = List::::final_prefix(); + assert_ne!(p1, p2); +} diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 068b1fba098a8..cc089cd7ec1c7 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -206,30 +206,6 @@ pub trait StorageList { } } -/// Convenience trait for testing internal details of [`types::StoragePagedList`]. -// FAIL-CI I am not sure if this is such a good idea, but dont see another way to allow external white-box testing the metadata. -#[cfg(feature = "std")] -pub trait TestingStoragePagedList: StorageList { - /// Expose the metadata of the list. It is otherwise inaccessible. - type Metadata; - - /// Read the metadata of the list from storage. - fn read_meta() -> Option; - - /// Clear the metadata of the list from storage. - fn clear_meta(); - - /// Return the elements of the list. - fn as_vec() -> Vec { - >::iter().collect() - } - - /// Remove and return all elements of the list. - fn as_drained_vec() -> Vec { - >::drain().collect() - } -} - /// Append iterator to append values to a storage struct. /// /// Can be used in situations where appending does not have constant time complexity. @@ -1266,7 +1242,7 @@ impl Iterator for ChildTriePrefixIterator { } /// Trait for storage types that store all its value after a unique prefix. -pub trait StoragePrefixContainer { +pub trait StoragePrefixedContainer { /// Module prefix. Used for generating final key. fn module_prefix() -> &'static [u8]; diff --git a/frame/support/src/storage/types/mod.rs b/frame/support/src/storage/types/mod.rs index bb8347017c7f1..3a5bae2e608b7 100644 --- a/frame/support/src/storage/types/mod.rs +++ b/frame/support/src/storage/types/mod.rs @@ -27,7 +27,6 @@ mod double_map; mod key; mod map; mod nmap; -mod paged_list; mod value; pub use counted_map::{CountedStorageMap, CountedStorageMapInstance}; @@ -38,7 +37,6 @@ pub use key::{ }; pub use map::StorageMap; pub use nmap::StorageNMap; -pub use paged_list::StoragePagedList; pub use value::StorageValue; /// Trait implementing how the storage optional value is converted into the queried type. From 2d6d7e343384c4a5b70043c1ce7baaa4aa77b02a Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sat, 13 May 2023 15:48:51 +0200 Subject: [PATCH 14/40] Move fuzzer Signed-off-by: Oliver Tale-Yazdi --- Cargo.lock | 21 ++++--- Cargo.toml | 2 +- .../fuzzer}/Cargo.toml | 11 ++-- .../fuzzer}/src/paged_list.rs | 41 +++---------- frame/paged-list/src/lib.rs | 17 +++++- frame/paged-list/src/mock.rs | 14 ++++- frame/paged-list/src/paged_list.rs | 61 +++++++++++-------- 7 files changed, 87 insertions(+), 80 deletions(-) rename frame/{support/storage-fuzzer => paged-list/fuzzer}/Cargo.toml (52%) rename frame/{support/storage-fuzzer => paged-list/fuzzer}/src/paged_list.rs (70%) diff --git a/Cargo.lock b/Cargo.lock index b445c1e0d01bf..b89c61a1e5b1d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2752,16 +2752,6 @@ dependencies = [ "syn 2.0.15", ] -[[package]] -name = "frame-support-storage-fuzzer" -version = "0.1.0" -dependencies = [ - "arbitrary", - "frame-support", - "honggfuzz", - "sp-io", -] - [[package]] name = "frame-support-test" version = "3.0.0" @@ -6923,6 +6913,17 @@ dependencies = [ "sp-std", ] +[[package]] +name = "pallet-paged-list-fuzzer" +version = "0.1.0" +dependencies = [ + "arbitrary", + "frame-support", + "honggfuzz", + "pallet-paged-list", + "sp-io", +] + [[package]] name = "pallet-preimage" version = "4.0.0-dev" diff --git a/Cargo.toml b/Cargo.toml index 9e89013b054d6..0653079bdea36 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -135,6 +135,7 @@ members = [ "frame/nomination-pools/test-staking", "frame/nomination-pools/runtime-api", "frame/paged-list", + "frame/paged-list/fuzzer", "frame/insecure-randomness-collective-flip", "frame/ranked-collective", "frame/recovery", @@ -159,7 +160,6 @@ members = [ "frame/support/procedural", "frame/support/procedural/tools", "frame/support/procedural/tools/derive", - "frame/support/storage-fuzzer", "frame/support/test", "frame/support/test/compile_pass", "frame/support/test/pallet", diff --git a/frame/support/storage-fuzzer/Cargo.toml b/frame/paged-list/fuzzer/Cargo.toml similarity index 52% rename from frame/support/storage-fuzzer/Cargo.toml rename to frame/paged-list/fuzzer/Cargo.toml index 8bc38b8f20034..9402ca8a48477 100644 --- a/frame/support/storage-fuzzer/Cargo.toml +++ b/frame/paged-list/fuzzer/Cargo.toml @@ -1,21 +1,22 @@ [package] -name = "frame-support-storage-fuzzer" +name = "pallet-paged-list-fuzzer" version = "0.1.0" authors = ["Parity Technologies "] edition = "2021" license = "Apache-2.0" homepage = "https://substrate.io" repository = "https://github.com/paritytech/substrate/" -description = "Fuzz FRAME storage types" +description = "Fuzz storage types of pallet-paged-list" publish = false [[bin]] -name = "paged-list" +name = "pallet-paged-list" path = "src/paged_list.rs" [dependencies] arbitrary = "1.3.0" honggfuzz = "0.5.49" -frame-support = { path = "../", default-features = false, features = ["std"] } -sp-io = { path = "../../../primitives/io", default-features = false, features = ["std"] } +frame-support = { version = "4.0.0-dev", default-features = false, features = [ "std" ], path = "../../support" } +sp-io = { path = "../../../primitives/io", default-features = false, features = [ "std" ] } +pallet-paged-list = { path = "../", default-features = false, features = [ "std" ] } diff --git a/frame/support/storage-fuzzer/src/paged_list.rs b/frame/paged-list/fuzzer/src/paged_list.rs similarity index 70% rename from frame/support/storage-fuzzer/src/paged_list.rs rename to frame/paged-list/fuzzer/src/paged_list.rs index 1fb690f8363fe..a00ce75624b9b 100644 --- a/frame/support/storage-fuzzer/src/paged_list.rs +++ b/frame/paged-list/fuzzer/src/paged_list.rs @@ -16,12 +16,12 @@ // limitations under the License. //! # Running -//! Running this fuzzer can be done with `cargo hfuzz run paged-list`. `honggfuzz` CLI options can +//! Running this fuzzer can be done with `cargo hfuzz run pallet-paged-list`. `honggfuzz` CLI options can //! be used by setting `HFUZZ_RUN_ARGS`, such as `-n 4` to use 4 threads. //! //! # Debugging a panic //! Once a panic is found, it can be debugged with -//! `cargo hfuzz run-debug paged-list hfuzz_workspace/paged-list/*.fuzz`. +//! `cargo hfuzz run-debug pallet-paged-list hfuzz_workspace/pallet-paged-list/*.fuzz`. //! //! # More information //! More information about `honggfuzz` can be found @@ -30,6 +30,11 @@ use arbitrary::Arbitrary; use honggfuzz::fuzz; +use frame_support::{StorageNoopGuard, storage::StorageList}; +use sp_io::TestExternalities; +use pallet_paged_list::mock::{*, PagedList1 as List}; +type Meta = MetaOf; + fn main() { loop { fuzz!(|data: (Vec, u8)| { @@ -40,7 +45,6 @@ fn main() { /// Appends and drains random number of elements in random order and checks storage invariants. fn drain_append_work(ops: Vec, page_size: u8) { - use mock::*; if page_size == 0 { return } @@ -59,7 +63,7 @@ fn drain_append_work(ops: Vec, page_size: u8) { // We have the assumption that the queue removes the metadata when being empty. if total == 0 { assert_eq!(List::drain().count(), 0); - assert_eq!(List::read_meta().unwrap_or_default(), Default::default()); + assert_eq!(Meta::from_storage().unwrap_or_default(), Default::default()); } } @@ -85,8 +89,6 @@ impl Arbitrary<'_> for Op { impl Op { pub fn exec(self) -> i64 { - use mock::*; - match self { Op::Append(v) => { let l = v.len(); @@ -97,30 +99,3 @@ impl Op { } } } - -#[allow(dead_code)] -mod mock { - pub use frame_support::{ - metadata_ir::{StorageEntryModifierIR, StorageEntryTypeIR, StorageHasherIR}, - parameter_types, - storage::{types::StoragePagedList, StorageList as _, TestingStoragePagedList as _}, - traits::StorageInstance, - Blake2_128Concat, StorageNoopGuard, - }; - pub use sp_io::TestExternalities; - - parameter_types! { - pub storage ValuesPerPage: u32 = 10; - pub const MaxPages: Option = Some(20); - } - - pub struct Prefix; - impl StorageInstance for Prefix { - fn pallet_prefix() -> &'static str { - "test" - } - const STORAGE_PREFIX: &'static str = "foo"; - } - - pub type List = StoragePagedList; -} diff --git a/frame/paged-list/src/lib.rs b/frame/paged-list/src/lib.rs index f80db3993dfdf..d7df0a26e801c 100644 --- a/frame/paged-list/src/lib.rs +++ b/frame/paged-list/src/lib.rs @@ -22,7 +22,7 @@ pub use pallet::*; -mod mock; +pub mod mock; mod paged_list; mod tests; @@ -89,6 +89,21 @@ impl, I: 'static> StorageList for Pallet { } } +// Helper stuff for tests. +#[cfg(feature = "std")] +impl, I: 'static> Pallet +{ + /// Return the elements of the list. + pub fn as_vec() -> Vec { + >::iter().collect() + } + + /// Remove and return all elements of the list. + pub fn as_drained_vec() -> Vec { + >::drain().collect() + } +} + /// The storage prefix for the list. /// /// Unique for each instance. diff --git a/frame/paged-list/src/mock.rs b/frame/paged-list/src/mock.rs index bfdb0d01a1f2a..8935fdbdc064e 100644 --- a/frame/paged-list/src/mock.rs +++ b/frame/paged-list/src/mock.rs @@ -15,14 +15,15 @@ // See the License for the specific language governing permissions and // limitations under the License. -#![cfg(test)] +#![cfg(feature = "std")] -use frame_support::traits::{ConstU16, ConstU64}; +use frame_support::{Blake2_128Concat, traits::{ConstU16, ConstU64}}; use sp_core::H256; use sp_runtime::{ testing::Header, traits::{BlakeTwo256, IdentityLookup}, }; +use crate::{Config, ListPrefix, paged_list::StoragePagedListMeta}; type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; type Block = frame_system::mocking::MockBlock; @@ -68,7 +69,7 @@ impl frame_system::Config for Test { } frame_support::parameter_types! { - pub const ValuesPerPage: u32 = 5; + pub storage ValuesPerPage: u32 = 5; pub const MaxPages: Option = Some(20); } @@ -90,3 +91,10 @@ impl crate::Config for Test { pub fn new_test_ext() -> sp_io::TestExternalities { frame_system::GenesisConfig::default().build_storage::().unwrap().into() } + +pub type MetaOf = StoragePagedListMeta< + ListPrefix, + ::Value, + Blake2_128Concat, + ::ValuesPerPage, +>; diff --git a/frame/paged-list/src/paged_list.rs b/frame/paged-list/src/paged_list.rs index 53859fe8c06b0..338ead4fc0a75 100644 --- a/frame/paged-list/src/paged_list.rs +++ b/frame/paged-list/src/paged_list.rs @@ -23,15 +23,15 @@ #![deny(missing_docs)] #![deny(unsafe_code)] -use codec::{Decode, Encode, EncodeLike, FullCodec}; -use core::marker::PhantomData; use frame_support::{ defensive, - storage::StoragePrefixedContainer, + Blake2_128Concat, traits::{Get, GetDefault, StorageInstance}, - Blake2_128Concat, CloneNoBound, DebugNoBound, DefaultNoBound, EqNoBound, PartialEqNoBound, - StorageHasher, + CloneNoBound, DebugNoBound, DefaultNoBound, EqNoBound, PartialEqNoBound, StorageHasher, }; +use frame_support::storage::StoragePrefixedContainer; +use codec::{Decode, Encode, EncodeLike, FullCodec}; +use core::marker::PhantomData; use sp_runtime::traits::Saturating; use sp_std::prelude::*; @@ -56,17 +56,17 @@ pub type ValueIndex = u32; /// Each [`Page`] holds at most `ValuesPerPage` values in its `values` vector. The last page is the /// only one that could have less than `ValuesPerPage` values. /// **Iteration** happens by starting -/// at [`first_page`][StoragePagedListMeta::first_page]/ -/// [`first_value`][StoragePagedListMeta::first_value] and incrementing these indices as long as -/// there are elements in the page and there are pages in storage. All elements of a page are loaded -/// once a page is read from storage. Iteration then happens on the cached elements. This reduces -/// the storage `read` calls on the overlay. **Appending** to the list happens by appending to the -/// last page by utilizing [`sp_io::storage::append`]. It allows to directly extend the elements of -/// `values` vector of the page without loading the whole vector from storage. A new page is -/// instanced once [`Page::next`] overflows `ValuesPerPage`. Its vector will also be created through -/// [`sp_io::storage::append`]. **Draining** advances the internal indices identical to Iteration. -/// It additionally persists the increments to storage and thereby 'drains' elements. Completely -/// drained pages are deleted from storage. +/// at [`first_page`][StoragePagedListMeta::first_page]/[`first_value`][StoragePagedListMeta::first_value] and incrementing +/// these indices as long as there are elements in the page and there are pages in storage. All +/// elements of a page are loaded once a page is read from storage. Iteration then happens on the +/// cached elements. This reduces the storage `read` calls on the overlay. +/// **Appending** to the list happens by appending to the last page by utilizing +/// [`sp_io::storage::append`]. It allows to directly extend the elements of `values` vector of the +/// page without loading the whole vector from storage. A new page is instanced once [`Page::next`] +/// overflows `ValuesPerPage`. Its vector will also be created through [`sp_io::storage::append`]. +/// **Draining** advances the internal indices identical to Iteration. It additionally persists the +/// increments to storage and thereby 'drains' elements. Completely drained pages are deleted from +/// storage. /// /// # Further Observations /// @@ -112,8 +112,10 @@ pub struct StoragePagedListMeta { _phantom: PhantomData<(Prefix, Value, Hasher, ValuesPerPage)>, } -impl frame_support::storage::StorageAppendix - for StoragePagedListMeta +impl + frame_support::storage::StorageAppendix +for + StoragePagedListMeta where Prefix: StorageInstance, Value: FullCodec, @@ -139,7 +141,9 @@ where pub fn from_storage() -> Option { let key = Self::key(); - sp_io::storage::get(key.as_ref()).and_then(|raw| Self::decode(&mut &raw[..]).ok()) + sp_io::storage::get(key.as_ref()).and_then(|raw| { + Self::decode(&mut &raw[..]).ok() + }) } pub fn key() -> Hasher::Output { @@ -322,8 +326,10 @@ where } } -impl frame_support::storage::StorageList - for StoragePagedList +impl + frame_support::storage::StorageList +for + StoragePagedList where Prefix: StorageInstance, Value: FullCodec + Clone, @@ -385,13 +391,13 @@ where /// Provides the final prefix for a [`StoragePagedList`]. /// -/// It solely exists so that when re-using it from the iterator and meta struct, none of the un-used -/// generics bleed through. Otherwise when only having the `StoragePrefixedContainer` implementation -/// on the list directly, the iterator and metadata need to muster *all* generics, even the ones -/// that are completely useless for prefix calculation. +/// It solely exists so that when re-using it from the iterator and meta struct, none of the un-used generics bleed through. Otherwise when only having the `StoragePrefixedContainer` implementation on the list directly, the iterator and metadata need to muster *all* generics, even the ones that are completely useless for prefix calculation. struct StoragePagedListPrefix(PhantomData); -impl frame_support::storage::StoragePrefixedContainer for StoragePagedListPrefix +impl + frame_support::storage::StoragePrefixedContainer +for + StoragePagedListPrefix where Prefix: StorageInstance, { @@ -406,7 +412,8 @@ where impl frame_support::storage::StoragePrefixedContainer - for StoragePagedList +for + StoragePagedList where Prefix: StorageInstance, Value: FullCodec + Clone, From fd7f1a55b54feed7a480463b7e16acd83113b76e Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sat, 13 May 2023 15:51:21 +0200 Subject: [PATCH 15/40] Cleanup Signed-off-by: Oliver Tale-Yazdi --- frame/paged-list/src/mock.rs | 12 +++++----- frame/paged-list/src/paged_list.rs | 36 +++++++++++++++--------------- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/frame/paged-list/src/mock.rs b/frame/paged-list/src/mock.rs index 8935fdbdc064e..7cef73bac8a40 100644 --- a/frame/paged-list/src/mock.rs +++ b/frame/paged-list/src/mock.rs @@ -87,14 +87,14 @@ impl crate::Config for Test { type MaxPages = MaxPages; } -// Build genesis storage according to the mock runtime. -pub fn new_test_ext() -> sp_io::TestExternalities { - frame_system::GenesisConfig::default().build_storage::().unwrap().into() -} - pub type MetaOf = StoragePagedListMeta< ListPrefix, - ::Value, Blake2_128Concat, + ::Value, ::ValuesPerPage, >; + +// Build genesis storage according to the mock runtime. +pub fn new_test_ext() -> sp_io::TestExternalities { + frame_system::GenesisConfig::default().build_storage::().unwrap().into() +} diff --git a/frame/paged-list/src/paged_list.rs b/frame/paged-list/src/paged_list.rs index 338ead4fc0a75..3d3dc00c711cf 100644 --- a/frame/paged-list/src/paged_list.rs +++ b/frame/paged-list/src/paged_list.rs @@ -89,7 +89,7 @@ pub struct StoragePagedList { +pub struct StoragePagedListMeta { /// The first page that contains a value. /// /// Can be >0 when pages were deleted. @@ -109,13 +109,13 @@ pub struct StoragePagedListMeta { /// whole list is empty. The only case where this can happen is when both are `0`. pub last_value: ValueIndex, - _phantom: PhantomData<(Prefix, Value, Hasher, ValuesPerPage)>, + _phantom: PhantomData<(Prefix, Hasher, Value, ValuesPerPage)>, } -impl +impl frame_support::storage::StorageAppendix for - StoragePagedListMeta + StoragePagedListMeta where Prefix: StorageInstance, Value: FullCodec, @@ -130,8 +130,8 @@ where } } -impl - StoragePagedListMeta +impl + StoragePagedListMeta where Prefix: StorageInstance, Value: FullCodec, @@ -239,7 +239,7 @@ impl Iterator for Page { /// Iterates over values of a [`StoragePagedList`]. /// /// Can optionally drain the iterated values. -pub struct StoragePagedListIterator { +pub struct StoragePagedListIterator { // Design: we put the Page into the iterator to have fewer storage look-ups. Yes, these // look-ups would be cached anyway, but bugging the overlay on each `.next` call still seems // like a poor trade-off than caching it in the iterator directly. Iterating and modifying is @@ -247,11 +247,11 @@ pub struct StoragePagedListIterator { // the iterator did not find any data upon setup or ran out of pages. page: Option>, drain: bool, - meta: StoragePagedListMeta, + meta: StoragePagedListMeta, } -impl - StoragePagedListIterator +impl + StoragePagedListIterator where Prefix: StorageInstance, Value: FullCodec + Clone, @@ -260,7 +260,7 @@ where { /// Read self from the storage. pub fn from_meta( - meta: StoragePagedListMeta, + meta: StoragePagedListMeta, drain: bool, ) -> Self { let mut page = Page::::from_storage::(meta.first_page); @@ -273,8 +273,8 @@ where } } -impl Iterator - for StoragePagedListIterator +impl Iterator + for StoragePagedListIterator where Prefix: StorageInstance, Value: FullCodec + Clone, @@ -337,8 +337,8 @@ where ValuesPerPage: Get, MaxPages: Get>, { - type Iterator = StoragePagedListIterator; - type Appendix = StoragePagedListMeta; + type Iterator = StoragePagedListIterator; + type Appendix = StoragePagedListMeta; fn iter() -> Self::Iterator { StoragePagedListIterator::from_meta(Self::read_meta(), false) @@ -362,7 +362,7 @@ where ValuesPerPage: Get, MaxPages: Get>, { - fn read_meta() -> StoragePagedListMeta { + fn read_meta() -> StoragePagedListMeta { // Use default here to not require a setup migration. StoragePagedListMeta::from_storage().unwrap_or_default() } @@ -370,7 +370,7 @@ where /// Provides a fast append iterator. /// /// The list should not be modified while appending. Also don't call it recursively. - fn appendix() -> StoragePagedListMeta { + fn appendix() -> StoragePagedListMeta { Self::read_meta() } @@ -485,7 +485,7 @@ mod tests { // all gone assert_eq!(List::as_vec(), Vec::::new()); // Need to delete the metadata manually. - StoragePagedListMeta::::clear(); + StoragePagedListMeta::::clear(); }); } From 6d3aceca3847075a38b27eda2aeb222ac065325e Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sat, 13 May 2023 15:51:32 +0200 Subject: [PATCH 16/40] fmt Signed-off-by: Oliver Tale-Yazdi --- frame/paged-list/fuzzer/src/paged_list.rs | 8 +-- frame/paged-list/src/lib.rs | 3 +- frame/paged-list/src/mock.rs | 7 ++- frame/paged-list/src/paged_list.rs | 61 ++++++++++------------- 4 files changed, 37 insertions(+), 42 deletions(-) diff --git a/frame/paged-list/fuzzer/src/paged_list.rs b/frame/paged-list/fuzzer/src/paged_list.rs index a00ce75624b9b..a2a4809f0a703 100644 --- a/frame/paged-list/fuzzer/src/paged_list.rs +++ b/frame/paged-list/fuzzer/src/paged_list.rs @@ -16,8 +16,8 @@ // limitations under the License. //! # Running -//! Running this fuzzer can be done with `cargo hfuzz run pallet-paged-list`. `honggfuzz` CLI options can -//! be used by setting `HFUZZ_RUN_ARGS`, such as `-n 4` to use 4 threads. +//! Running this fuzzer can be done with `cargo hfuzz run pallet-paged-list`. `honggfuzz` CLI +//! options can be used by setting `HFUZZ_RUN_ARGS`, such as `-n 4` to use 4 threads. //! //! # Debugging a panic //! Once a panic is found, it can be debugged with @@ -30,9 +30,9 @@ use arbitrary::Arbitrary; use honggfuzz::fuzz; -use frame_support::{StorageNoopGuard, storage::StorageList}; +use frame_support::{storage::StorageList, StorageNoopGuard}; +use pallet_paged_list::mock::{PagedList1 as List, *}; use sp_io::TestExternalities; -use pallet_paged_list::mock::{*, PagedList1 as List}; type Meta = MetaOf; fn main() { diff --git a/frame/paged-list/src/lib.rs b/frame/paged-list/src/lib.rs index d7df0a26e801c..a23c623afb8bb 100644 --- a/frame/paged-list/src/lib.rs +++ b/frame/paged-list/src/lib.rs @@ -91,8 +91,7 @@ impl, I: 'static> StorageList for Pallet { // Helper stuff for tests. #[cfg(feature = "std")] -impl, I: 'static> Pallet -{ +impl, I: 'static> Pallet { /// Return the elements of the list. pub fn as_vec() -> Vec { >::iter().collect() diff --git a/frame/paged-list/src/mock.rs b/frame/paged-list/src/mock.rs index 7cef73bac8a40..1afa997ffb629 100644 --- a/frame/paged-list/src/mock.rs +++ b/frame/paged-list/src/mock.rs @@ -17,13 +17,16 @@ #![cfg(feature = "std")] -use frame_support::{Blake2_128Concat, traits::{ConstU16, ConstU64}}; +use crate::{paged_list::StoragePagedListMeta, Config, ListPrefix}; +use frame_support::{ + traits::{ConstU16, ConstU64}, + Blake2_128Concat, +}; use sp_core::H256; use sp_runtime::{ testing::Header, traits::{BlakeTwo256, IdentityLookup}, }; -use crate::{Config, ListPrefix, paged_list::StoragePagedListMeta}; type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; type Block = frame_system::mocking::MockBlock; diff --git a/frame/paged-list/src/paged_list.rs b/frame/paged-list/src/paged_list.rs index 3d3dc00c711cf..68578e0a4c520 100644 --- a/frame/paged-list/src/paged_list.rs +++ b/frame/paged-list/src/paged_list.rs @@ -23,15 +23,15 @@ #![deny(missing_docs)] #![deny(unsafe_code)] +use codec::{Decode, Encode, EncodeLike, FullCodec}; +use core::marker::PhantomData; use frame_support::{ defensive, - Blake2_128Concat, + storage::StoragePrefixedContainer, traits::{Get, GetDefault, StorageInstance}, - CloneNoBound, DebugNoBound, DefaultNoBound, EqNoBound, PartialEqNoBound, StorageHasher, + Blake2_128Concat, CloneNoBound, DebugNoBound, DefaultNoBound, EqNoBound, PartialEqNoBound, + StorageHasher, }; -use frame_support::storage::StoragePrefixedContainer; -use codec::{Decode, Encode, EncodeLike, FullCodec}; -use core::marker::PhantomData; use sp_runtime::traits::Saturating; use sp_std::prelude::*; @@ -56,17 +56,17 @@ pub type ValueIndex = u32; /// Each [`Page`] holds at most `ValuesPerPage` values in its `values` vector. The last page is the /// only one that could have less than `ValuesPerPage` values. /// **Iteration** happens by starting -/// at [`first_page`][StoragePagedListMeta::first_page]/[`first_value`][StoragePagedListMeta::first_value] and incrementing -/// these indices as long as there are elements in the page and there are pages in storage. All -/// elements of a page are loaded once a page is read from storage. Iteration then happens on the -/// cached elements. This reduces the storage `read` calls on the overlay. -/// **Appending** to the list happens by appending to the last page by utilizing -/// [`sp_io::storage::append`]. It allows to directly extend the elements of `values` vector of the -/// page without loading the whole vector from storage. A new page is instanced once [`Page::next`] -/// overflows `ValuesPerPage`. Its vector will also be created through [`sp_io::storage::append`]. -/// **Draining** advances the internal indices identical to Iteration. It additionally persists the -/// increments to storage and thereby 'drains' elements. Completely drained pages are deleted from -/// storage. +/// at [`first_page`][StoragePagedListMeta::first_page]/ +/// [`first_value`][StoragePagedListMeta::first_value] and incrementing these indices as long as +/// there are elements in the page and there are pages in storage. All elements of a page are loaded +/// once a page is read from storage. Iteration then happens on the cached elements. This reduces +/// the storage `read` calls on the overlay. **Appending** to the list happens by appending to the +/// last page by utilizing [`sp_io::storage::append`]. It allows to directly extend the elements of +/// `values` vector of the page without loading the whole vector from storage. A new page is +/// instanced once [`Page::next`] overflows `ValuesPerPage`. Its vector will also be created through +/// [`sp_io::storage::append`]. **Draining** advances the internal indices identical to Iteration. +/// It additionally persists the increments to storage and thereby 'drains' elements. Completely +/// drained pages are deleted from storage. /// /// # Further Observations /// @@ -112,10 +112,8 @@ pub struct StoragePagedListMeta { _phantom: PhantomData<(Prefix, Hasher, Value, ValuesPerPage)>, } -impl - frame_support::storage::StorageAppendix -for - StoragePagedListMeta +impl frame_support::storage::StorageAppendix + for StoragePagedListMeta where Prefix: StorageInstance, Value: FullCodec, @@ -141,9 +139,7 @@ where pub fn from_storage() -> Option { let key = Self::key(); - sp_io::storage::get(key.as_ref()).and_then(|raw| { - Self::decode(&mut &raw[..]).ok() - }) + sp_io::storage::get(key.as_ref()).and_then(|raw| Self::decode(&mut &raw[..]).ok()) } pub fn key() -> Hasher::Output { @@ -326,10 +322,8 @@ where } } -impl - frame_support::storage::StorageList -for - StoragePagedList +impl frame_support::storage::StorageList + for StoragePagedList where Prefix: StorageInstance, Value: FullCodec + Clone, @@ -391,13 +385,13 @@ where /// Provides the final prefix for a [`StoragePagedList`]. /// -/// It solely exists so that when re-using it from the iterator and meta struct, none of the un-used generics bleed through. Otherwise when only having the `StoragePrefixedContainer` implementation on the list directly, the iterator and metadata need to muster *all* generics, even the ones that are completely useless for prefix calculation. +/// It solely exists so that when re-using it from the iterator and meta struct, none of the un-used +/// generics bleed through. Otherwise when only having the `StoragePrefixedContainer` implementation +/// on the list directly, the iterator and metadata need to muster *all* generics, even the ones +/// that are completely useless for prefix calculation. struct StoragePagedListPrefix(PhantomData); -impl - frame_support::storage::StoragePrefixedContainer -for - StoragePagedListPrefix +impl frame_support::storage::StoragePrefixedContainer for StoragePagedListPrefix where Prefix: StorageInstance, { @@ -412,8 +406,7 @@ where impl frame_support::storage::StoragePrefixedContainer -for - StoragePagedList + for StoragePagedList where Prefix: StorageInstance, Value: FullCodec + Clone, From a3c82f38634fac52f83544ebaed0ed3876077a64 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 30 May 2023 13:27:09 +0200 Subject: [PATCH 17/40] docs Signed-off-by: Oliver Tale-Yazdi --- frame/paged-list/src/lib.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/frame/paged-list/src/lib.rs b/frame/paged-list/src/lib.rs index a23c623afb8bb..c6e05c6c0128b 100644 --- a/frame/paged-list/src/lib.rs +++ b/frame/paged-list/src/lib.rs @@ -41,7 +41,8 @@ pub mod pallet { #[pallet::pallet] pub struct Pallet(_); - /// This type alias is what FRAME normally get us. + /// A storage paged list akin to what the FRAME macros would generate. + // Note that FRAME does natively support paged lists in storage. pub type List = StoragePagedList< ListPrefix, Blake2_128Concat, @@ -55,11 +56,14 @@ pub mod pallet { type RuntimeEvent: From> + IsType<::RuntimeEvent>; + /// The value type that can be stored in the list. type Value: FullCodec + Clone + MaxEncodedLen; + /// The number of values per page. #[pallet::constant] type ValuesPerPage: Get; + /// The maximal number of pages in the list. #[pallet::constant] type MaxPages: Get>; } From e5183311bda16277727bb0d1c2496c9ae647294a Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 30 May 2023 13:27:18 +0200 Subject: [PATCH 18/40] Rename Appendix -> Appender Signed-off-by: Oliver Tale-Yazdi --- frame/paged-list/src/lib.rs | 6 +++--- frame/paged-list/src/paged_list.rs | 14 +++++++------- frame/support/src/storage/mod.rs | 10 +++++----- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/frame/paged-list/src/lib.rs b/frame/paged-list/src/lib.rs index c6e05c6c0128b..81feadd537579 100644 --- a/frame/paged-list/src/lib.rs +++ b/frame/paged-list/src/lib.rs @@ -78,7 +78,7 @@ pub mod pallet { // This exposes the list functionality to other pallets. impl, I: 'static> StorageList for Pallet { type Iterator = as StorageList>::Iterator; - type Appendix = as StorageList>::Appendix; + type Appender = as StorageList>::Appender; fn iter() -> Self::Iterator { List::::iter() @@ -88,8 +88,8 @@ impl, I: 'static> StorageList for Pallet { List::::drain() } - fn appendix() -> Self::Appendix { - List::::appendix() + fn appender() -> Self::Appender { + List::::appender() } } diff --git a/frame/paged-list/src/paged_list.rs b/frame/paged-list/src/paged_list.rs index 68578e0a4c520..3c7db4ce4474e 100644 --- a/frame/paged-list/src/paged_list.rs +++ b/frame/paged-list/src/paged_list.rs @@ -75,7 +75,7 @@ pub type ValueIndex = u32; /// besides having all management state handy in one location. /// - The PoV complexity of iterating compared to a `StorageValue>` is improved for /// "shortish" iterations and worse for total iteration. The append complexity is identical in the -/// asymptotic case when using an `Appendix`, and worse in all. For example when appending just +/// asymptotic case when using an `Appender`, and worse in all. For example when appending just /// one value. /// - It does incur a read overhead on the host side as compared to a `StorageValue>`. pub struct StoragePagedList { @@ -84,7 +84,7 @@ pub struct StoragePagedList { _phantom: PhantomData<(Prefix, Hasher, Value, ValuesPerPage)>, } -impl frame_support::storage::StorageAppendix +impl frame_support::storage::StorageAppender for StoragePagedListMeta where Prefix: StorageInstance, @@ -332,7 +332,7 @@ where MaxPages: Get>, { type Iterator = StoragePagedListIterator; - type Appendix = StoragePagedListMeta; + type Appender = StoragePagedListMeta; fn iter() -> Self::Iterator { StoragePagedListIterator::from_meta(Self::read_meta(), false) @@ -342,8 +342,8 @@ where StoragePagedListIterator::from_meta(Self::read_meta(), true) } - fn appendix() -> Self::Appendix { - Self::appendix() + fn appender() -> Self::Appender { + Self::appender() } } @@ -364,7 +364,7 @@ where /// Provides a fast append iterator. /// /// The list should not be modified while appending. Also don't call it recursively. - fn appendix() -> StoragePagedListMeta { + fn appender() -> StoragePagedListMeta { Self::read_meta() } diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index cc089cd7ec1c7..f208f8d80597a 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -166,7 +166,7 @@ pub trait StorageList { type Iterator: Iterator; /// Append iterator for fast append operations. - type Appendix: StorageAppendix; + type Appender: StorageAppender; /// List the elements in append order. fn iter() -> Self::Iterator; @@ -178,7 +178,7 @@ pub trait StorageList { fn drain() -> Self::Iterator; /// A fast append iterator. - fn appendix() -> Self::Appendix; + fn appender() -> Self::Appender; /// Append a single element. /// @@ -193,7 +193,7 @@ pub trait StorageList { /// Append many elements. /// - /// Should not be called repeatedly; use `appendix` instead. + /// Should not be called repeatedly; use `appender` instead. /// Worst case linear `O(len + items.count())` with `len` beings the number if elements in the /// list. fn append_many(items: I) @@ -201,7 +201,7 @@ pub trait StorageList { EncodeLikeValue: EncodeLike, I: IntoIterator, { - let mut ap = Self::appendix(); + let mut ap = Self::appender(); ap.append_many(items); } } @@ -209,7 +209,7 @@ pub trait StorageList { /// Append iterator to append values to a storage struct. /// /// Can be used in situations where appending does not have constant time complexity. -pub trait StorageAppendix { +pub trait StorageAppender { /// Append a single item in constant time `O(1)`. fn append(&mut self, item: EncodeLikeValue) where From 314e4c08f299382b6863d4ece7b0180f09a7da52 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 30 May 2023 13:33:58 +0200 Subject: [PATCH 19/40] Rename clear -> delete Signed-off-by: Oliver Tale-Yazdi --- frame/paged-list/src/paged_list.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/paged-list/src/paged_list.rs b/frame/paged-list/src/paged_list.rs index 3c7db4ce4474e..74b2cd03dc7ce 100644 --- a/frame/paged-list/src/paged_list.rs +++ b/frame/paged-list/src/paged_list.rs @@ -169,10 +169,10 @@ where pub fn reset(&mut self) { *self = Default::default(); - Self::clear(); + Self::delete(); } - pub fn clear() { + pub fn delete() { let key = Self::key(); sp_io::storage::clear(key.as_ref()); } @@ -478,7 +478,7 @@ mod tests { // all gone assert_eq!(List::as_vec(), Vec::::new()); // Need to delete the metadata manually. - StoragePagedListMeta::::clear(); + StoragePagedListMeta::::delete(); }); } From d2aa1b63bec729e97a4ec3f030aa8aa77738b8a9 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 30 May 2023 13:54:45 +0200 Subject: [PATCH 20/40] Feature gate testing stuff Signed-off-by: Oliver Tale-Yazdi --- frame/paged-list/src/lib.rs | 14 --------- frame/paged-list/src/paged_list.rs | 49 +++++++++++++----------------- 2 files changed, 21 insertions(+), 42 deletions(-) diff --git a/frame/paged-list/src/lib.rs b/frame/paged-list/src/lib.rs index 81feadd537579..1048427e48381 100644 --- a/frame/paged-list/src/lib.rs +++ b/frame/paged-list/src/lib.rs @@ -93,20 +93,6 @@ impl, I: 'static> StorageList for Pallet { } } -// Helper stuff for tests. -#[cfg(feature = "std")] -impl, I: 'static> Pallet { - /// Return the elements of the list. - pub fn as_vec() -> Vec { - >::iter().collect() - } - - /// Remove and return all elements of the list. - pub fn as_drained_vec() -> Vec { - >::drain().collect() - } -} - /// The storage prefix for the list. /// /// Unique for each instance. diff --git a/frame/paged-list/src/paged_list.rs b/frame/paged-list/src/paged_list.rs index 74b2cd03dc7ce..70883b9175093 100644 --- a/frame/paged-list/src/paged_list.rs +++ b/frame/paged-list/src/paged_list.rs @@ -179,26 +179,28 @@ where } pub struct Page { - next: ValueIndex, + /// The index of the page. index: PageIndex, - // invariant: this is **never** empty - values: sp_std::vec::Vec, + /// The remaining values of the page, to be drained by [`Page::next`]. + values: sp_std::iter::Skip>, } impl Page { /// Decode a page with `index` from storage. pub fn from_storage( index: PageIndex, + value_index: ValueIndex, ) -> Option { let key = page_key::(index); let values = sp_io::storage::get(key.as_ref()) - .and_then(|raw| sp_std::vec::Vec::::decode(&mut &raw[..]).ok()); - let values = values.unwrap_or_default(); + .and_then(|raw| sp_std::vec::Vec::::decode(&mut &raw[..]).ok())?; if values.is_empty() { - None - } else { - Some(Self { next: 0, index, values }) + // Dont create empty pages. + return None; } + let values = values.into_iter().skip(value_index as usize); + + Some(Self { index, values }) } /// Delete this page from storage. @@ -222,13 +224,11 @@ pub(crate) fn page_key( (StoragePagedListPrefix::::final_prefix(), b"page", index).using_encoded(Hasher::hash) } -impl Iterator for Page { +impl Iterator for Page { type Item = V; fn next(&mut self) -> Option { - let val = self.values.get(self.next as usize).cloned(); - self.next.saturating_inc(); - val + self.values.next() } } @@ -250,7 +250,7 @@ impl StoragePagedListIterator where Prefix: StorageInstance, - Value: FullCodec + Clone, + Value: FullCodec, Hasher: StorageHasher, ValuesPerPage: Get, { @@ -259,12 +259,7 @@ where meta: StoragePagedListMeta, drain: bool, ) -> Self { - let mut page = Page::::from_storage::(meta.first_page); - - if let Some(ref mut page) = page { - page.next = meta.first_value; - } - + let page = Page::::from_storage::(meta.first_page, meta.first_value); Self { page, drain, meta } } } @@ -273,7 +268,7 @@ impl Iterator for StoragePagedListIterator where Prefix: StorageInstance, - Value: FullCodec + Clone, + Value: FullCodec, Hasher: StorageHasher, ValuesPerPage: Get, { @@ -297,7 +292,7 @@ where self.meta.store(); } - let Some(mut page) = Page::from_storage::(page.index.saturating_add(1)) else { + let Some(mut page) = Page::from_storage::(page.index.saturating_add(1), 0) else { self.page = None; if self.drain { self.meta.reset(); @@ -326,7 +321,7 @@ impl frame_support::storage::Sto for StoragePagedList where Prefix: StorageInstance, - Value: FullCodec + Clone, + Value: FullCodec, Hasher: StorageHasher, ValuesPerPage: Get, MaxPages: Get>, @@ -351,7 +346,7 @@ impl StoragePagedList where Prefix: StorageInstance, - Value: FullCodec + Clone, + Value: FullCodec, Hasher: StorageHasher, ValuesPerPage: Get, MaxPages: Get>, @@ -369,15 +364,13 @@ where } /// Return the elements of the list. - #[cfg(feature = "std")] - #[allow(dead_code)] + #[cfg(test)] fn as_vec() -> Vec { >::iter().collect() } /// Remove and return all elements of the list. - #[cfg(feature = "std")] - #[allow(dead_code)] + #[cfg(test)] fn as_drained_vec() -> Vec { >::drain().collect() } @@ -409,7 +402,7 @@ impl for StoragePagedList where Prefix: StorageInstance, - Value: FullCodec + Clone, + Value: FullCodec, Hasher: StorageHasher, ValuesPerPage: Get, MaxPages: Get>, From f0d413314ac3554a26ba5d3e73ade0611b3d7236 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 30 May 2023 13:56:29 +0200 Subject: [PATCH 21/40] Docs review Co-authored-by: Koute Signed-off-by: Oliver Tale-Yazdi --- frame/paged-list/src/paged_list.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/frame/paged-list/src/paged_list.rs b/frame/paged-list/src/paged_list.rs index 70883b9175093..593ebd549318f 100644 --- a/frame/paged-list/src/paged_list.rs +++ b/frame/paged-list/src/paged_list.rs @@ -63,10 +63,10 @@ pub type ValueIndex = u32; /// the storage `read` calls on the overlay. **Appending** to the list happens by appending to the /// last page by utilizing [`sp_io::storage::append`]. It allows to directly extend the elements of /// `values` vector of the page without loading the whole vector from storage. A new page is -/// instanced once [`Page::next`] overflows `ValuesPerPage`. Its vector will also be created through -/// [`sp_io::storage::append`]. **Draining** advances the internal indices identical to Iteration. -/// It additionally persists the increments to storage and thereby 'drains' elements. Completely -/// drained pages are deleted from storage. +/// instantiated once [`Page::next`] overflows `ValuesPerPage`. Its vector will also be created +/// through [`sp_io::storage::append`]. **Draining** advances the internal indices identical to +/// Iteration. It additionally persists the increments to storage and thereby 'drains' elements. +/// Completely drained pages are deleted from storage. /// /// # Further Observations /// @@ -196,7 +196,7 @@ impl Page { .and_then(|raw| sp_std::vec::Vec::::decode(&mut &raw[..]).ok())?; if values.is_empty() { // Dont create empty pages. - return None; + return None } let values = values.into_iter().skip(value_index as usize); From 9a7afbb56ad25e3ce7592425de740da07697548b Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 30 May 2023 14:32:44 +0200 Subject: [PATCH 22/40] Cleanup Signed-off-by: Oliver Tale-Yazdi --- frame/paged-list/src/paged_list.rs | 38 ++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/frame/paged-list/src/paged_list.rs b/frame/paged-list/src/paged_list.rs index 593ebd549318f..c8f21ea548841 100644 --- a/frame/paged-list/src/paged_list.rs +++ b/frame/paged-list/src/paged_list.rs @@ -157,7 +157,6 @@ where } let key = page_key::(self.last_page); self.last_value.saturating_inc(); - // Pages are `Vec` and therefore appendable. sp_io::storage::append(key.as_ref(), item.encode()); self.store(); } @@ -178,6 +177,7 @@ where } } +/// A page that was decoded from storage and caches its values. pub struct Page { /// The index of the page. index: PageIndex, @@ -186,7 +186,7 @@ pub struct Page { } impl Page { - /// Decode a page with `index` from storage. + /// Read the page with `index` from storage and assume the first value at `value_index`. pub fn from_storage( index: PageIndex, value_index: ValueIndex, @@ -369,7 +369,7 @@ where >::iter().collect() } - /// Remove and return all elements of the list. + /// Return and remove the elements of the list. #[cfg(test)] fn as_drained_vec() -> Vec { >::drain().collect() @@ -416,7 +416,7 @@ where } } -/// Prelude for doc-tests. +/// Prelude for (doc)tests. #[cfg(feature = "std")] #[allow(dead_code)] pub(crate) mod mock { @@ -509,6 +509,36 @@ mod tests { }); } + /// Appending encodes pages as `Vec`. + #[test] + fn append_storage_layout() { + TestExternalities::default().execute_with(|| { + List::append_many(0..9); + + let key = page_key::(0); + let raw = + frame_support::storage::unhashed::get_raw(&key).expect("Page should be present"); + let as_vec = Vec::::decode(&mut &raw[..]).unwrap(); + assert_eq!(as_vec.len(), 5, "First page contains 5"); + + let key = page_key::(1); + let raw = + frame_support::storage::unhashed::get_raw(&key).expect("Page should be present"); + let as_vec = Vec::::decode(&mut &raw[..]).unwrap(); + assert_eq!(as_vec.len(), 4, "Second page contains 4"); + }); + } + + #[test] + fn page_key_correct() { + let got = page_key::(0); + // FAIL-CI this is wrong + let want = (StoragePagedListPrefix::::final_prefix(), b"page", 0) + .using_encoded(Blake2_128Concat::hash); + + assert_eq!(got, want); + } + #[test] fn peekable_drain_also_deletes() { TestExternalities::default().execute_with(|| { From ac3c0db1ffbbe7089fc3764d14c26396527ed265 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 30 May 2023 14:43:39 +0200 Subject: [PATCH 23/40] doc review Signed-off-by: Oliver Tale-Yazdi --- frame/paged-list/src/paged_list.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/paged-list/src/paged_list.rs b/frame/paged-list/src/paged_list.rs index c8f21ea548841..28000521717ae 100644 --- a/frame/paged-list/src/paged_list.rs +++ b/frame/paged-list/src/paged_list.rs @@ -101,11 +101,11 @@ pub struct StoragePagedListMeta { /// The last page that could contain data. /// - /// Iteration starts at this page index. + /// Appending starts at this page index. pub last_page: PageIndex, /// The last value inside `last_page` that could contain a value. /// - /// Iteration starts at this index. If the page does not hold a value at this index, then the + /// Appending starts at this index. If the page does not hold a value at this index, then the /// whole list is empty. The only case where this can happen is when both are `0`. pub last_value: ValueIndex, From df2a610ace3ce45d36f83922abe370f0119b62cd Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 30 May 2023 14:46:14 +0200 Subject: [PATCH 24/40] Review renames Co-authored-by: Koute Signed-off-by: Oliver Tale-Yazdi --- frame/paged-list/src/paged_list.rs | 39 +++++++++++++++--------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/frame/paged-list/src/paged_list.rs b/frame/paged-list/src/paged_list.rs index 28000521717ae..3a2ec32c3923a 100644 --- a/frame/paged-list/src/paged_list.rs +++ b/frame/paged-list/src/paged_list.rs @@ -57,15 +57,15 @@ pub type ValueIndex = u32; /// only one that could have less than `ValuesPerPage` values. /// **Iteration** happens by starting /// at [`first_page`][StoragePagedListMeta::first_page]/ -/// [`first_value`][StoragePagedListMeta::first_value] and incrementing these indices as long as -/// there are elements in the page and there are pages in storage. All elements of a page are loaded -/// once a page is read from storage. Iteration then happens on the cached elements. This reduces -/// the storage `read` calls on the overlay. **Appending** to the list happens by appending to the -/// last page by utilizing [`sp_io::storage::append`]. It allows to directly extend the elements of -/// `values` vector of the page without loading the whole vector from storage. A new page is -/// instantiated once [`Page::next`] overflows `ValuesPerPage`. Its vector will also be created -/// through [`sp_io::storage::append`]. **Draining** advances the internal indices identical to -/// Iteration. It additionally persists the increments to storage and thereby 'drains' elements. +/// [`first_value_offset`][StoragePagedListMeta::first_value_offset] and incrementing these indices +/// as long as there are elements in the page and there are pages in storage. All elements of a page +/// are loaded once a page is read from storage. Iteration then happens on the cached elements. This +/// reduces the storage `read` calls on the overlay. **Appending** to the list happens by appending +/// to the last page by utilizing [`sp_io::storage::append`]. It allows to directly extend the +/// elements of `values` vector of the page without loading the whole vector from storage. A new +/// page is instantiated once [`Page::next`] overflows `ValuesPerPage`. Its vector will also be +/// created through [`sp_io::storage::append`]. **Draining** advances the internal indices identical +/// to Iteration. It additionally persists the increments to storage and thereby 'drains' elements. /// Completely drained pages are deleted from storage. /// /// # Further Observations @@ -97,7 +97,7 @@ pub struct StoragePagedListMeta { /// The first value inside `first_page` that contains a value. /// /// Can be >0 when values were deleted. - pub first_value: ValueIndex, + pub first_value_offset: ValueIndex, /// The last page that could contain data. /// @@ -107,7 +107,7 @@ pub struct StoragePagedListMeta { /// /// Appending starts at this index. If the page does not hold a value at this index, then the /// whole list is empty. The only case where this can happen is when both are `0`. - pub last_value: ValueIndex, + pub last_page_len: ValueIndex, _phantom: PhantomData<(Prefix, Hasher, Value, ValuesPerPage)>, } @@ -151,12 +151,12 @@ where EncodeLikeValue: EncodeLike, { // Note: we use >= here in case someone decreased it in a runtime upgrade. - if self.last_value >= ValuesPerPage::get() { - self.last_value = 0; + if self.last_page_len >= ValuesPerPage::get() { self.last_page.saturating_inc(); + self.last_page_len = 0; } let key = page_key::(self.last_page); - self.last_value.saturating_inc(); + self.last_page_len.saturating_inc(); sp_io::storage::append(key.as_ref(), item.encode()); self.store(); } @@ -259,7 +259,8 @@ where meta: StoragePagedListMeta, drain: bool, ) -> Self { - let page = Page::::from_storage::(meta.first_page, meta.first_value); + let page = + Page::::from_storage::(meta.first_page, meta.first_value_offset); Self { page, drain, meta } } } @@ -279,7 +280,7 @@ where if let Some(val) = page.next() { if self.drain { - self.meta.first_value.saturating_inc(); + self.meta.first_value_offset.saturating_inc(); self.meta.store() } return Some(val) @@ -287,7 +288,7 @@ where if self.drain { // page is empty page.delete::(); - self.meta.first_value = 0; + self.meta.first_value_offset = 0; self.meta.first_page.saturating_inc(); self.meta.store(); } @@ -303,7 +304,7 @@ where if let Some(val) = page.next() { self.page = Some(page); if self.drain { - self.meta.first_value.saturating_inc(); + self.meta.first_value_offset.saturating_inc(); self.meta.store(); } return Some(val) @@ -486,7 +487,7 @@ mod tests { let meta = List::read_meta(); // Will switch over to `10/0`, but will in the next call. - assert_eq!((meta.first_page, meta.first_value), (9, 5)); + assert_eq!((meta.first_page, meta.first_value_offset), (9, 5)); // 50 gone, 50 to go assert_eq!(List::as_vec(), (50..100).collect::>()); From c3d78db540f4e9de7c0c59ee5571841440e9ad20 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 30 May 2023 15:56:24 +0200 Subject: [PATCH 25/40] Add docs Signed-off-by: Oliver Tale-Yazdi --- Cargo.lock | 1 + frame/paged-list/Cargo.toml | 36 +++++------------ frame/paged-list/README.md | 30 -------------- frame/paged-list/src/lib.rs | 50 ++++++++++++++++++++++- frame/paged-list/src/mock.rs | 10 ++++- frame/paged-list/src/tests.rs | 74 ++++++++++++++++++++++++++++++++--- 6 files changed, 135 insertions(+), 66 deletions(-) delete mode 100644 frame/paged-list/README.md diff --git a/Cargo.lock b/Cargo.lock index 3b7cb1c6ff1ea..3281e9c54f35f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6979,6 +6979,7 @@ dependencies = [ name = "pallet-paged-list" version = "0.1.0" dependencies = [ + "docify", "frame-benchmarking", "frame-support", "frame-system", diff --git a/frame/paged-list/Cargo.toml b/frame/paged-list/Cargo.toml index 3806219fa3840..3bb0265b5e91a 100644 --- a/frame/paged-list/Cargo.toml +++ b/frame/paged-list/Cargo.toml @@ -13,39 +13,23 @@ targets = ["x86_64-unknown-linux-gnu"] [dependencies] codec = { package = "parity-scale-codec", version = "3.0.0", default-features = false, features = [ "derive"] } +docify = "0.1.13" scale-info = { version = "2.0.0", default-features = false, features = ["derive"] } frame-benchmarking = { version = "4.0.0-dev", default-features = false, optional = true, path = "../benchmarking" } frame-support = { version = "4.0.0-dev", default-features = false, path = "../support" } frame-system = { version = "4.0.0-dev", default-features = false, path = "../system" } -sp-runtime = { version = "7.0.0", default-features = false, path = "../../primitives/runtime" } -sp-std = { version = "5.0.0", default-features = false, path = "../../primitives/std" } -sp-core = { version = "7.0.0", path = "../../primitives/core", default-features = false, optional = true } -sp-io = { version = "7.0.0", path = "../../primitives/io", default-features = false, optional = true } +sp-runtime = { version = "8.0.0", default-features = false, path = "../../primitives/runtime" } +sp-std = { version = "6.0.0", default-features = false, path = "../../primitives/std" } +sp-core = { version = "8.0.0", path = "../../primitives/core", default-features = false, optional = true } +sp-io = { version = "8.0.0", path = "../../primitives/io", default-features = false, optional = true } [features] default = ["std"] -std = [ - "codec/std", - "frame-benchmarking?/std", - "frame-support/std", - "frame-system/std", - "scale-info/std", - "sp-core/std", - "sp-io/std", - "sp-runtime/std", - "sp-std/std", -] - -runtime-benchmarks = [ - "frame-benchmarking/runtime-benchmarks", - "frame-support/runtime-benchmarks", - "frame-system/runtime-benchmarks", - "sp-runtime/runtime-benchmarks", -] - -try-runtime = [ - "frame-support/try-runtime" -] +std = ["codec/std", "frame-benchmarking?/std", "frame-support/std", "frame-system/std", "scale-info/std", "sp-core/std", "sp-io/std", "sp-runtime/std", "sp-std/std"] + +runtime-benchmarks = ["frame-benchmarking/runtime-benchmarks", "frame-support/runtime-benchmarks", "frame-system/runtime-benchmarks", "sp-runtime/runtime-benchmarks"] + +try-runtime = ["frame-support/try-runtime"] diff --git a/frame/paged-list/README.md b/frame/paged-list/README.md deleted file mode 100644 index e4dc07fd4e752..0000000000000 --- a/frame/paged-list/README.md +++ /dev/null @@ -1,30 +0,0 @@ -# Pallet PagedList - -FRAME pallet that provides a paged list data structure. -(TODO add more). - -# Design Goals - -1. TODO - -# Design - -TODO - -# Examples - -```rust -use pallet_paged_list::Pallet as PagedList; - -fn main() { - -} -``` - -# License - -Apache-2.0 - -# References -This crate was auto generated by FRAMY CLI . -Please report bugs and build failures at . diff --git a/frame/paged-list/src/lib.rs b/frame/paged-list/src/lib.rs index 1048427e48381..2193d48fb43fa 100644 --- a/frame/paged-list/src/lib.rs +++ b/frame/paged-list/src/lib.rs @@ -15,10 +15,49 @@ // See the License for the specific language governing permissions and // limitations under the License. -//! A minimal wrapper around the [`paged_list::StoragePagedList`]. +//! > Made with *Substrate*, for *DotSama*. +//! +//! [![github]](https://github.com/paritytech/substrate/frame/fast-unstake) - +//! [![polkadot]](https://polkadot.network) +//! +//! [polkadot]: https://img.shields.io/badge/polkadot-E6007A?style=for-the-badge&logo=polkadot&logoColor=white +//! [github]: https://img.shields.io/badge/github-8da0cb?style=for-the-badge&labelColor=555555&logo=github +//! +//! # Paged List Pallet +//! +//! A thin wrapper pallet around a [`paged_list::StoragePagedList`]. It provides an API for a single +//! paginated list. It can be instantiated multiple times to provide multiple lists. +//! +//! ## Overview +//! +//! The pallet is quite unique since it does not expose any `Call`s, `Error`s or `Event`s. All +//! interaction goes through the implemented [`StorageList`] trait. +//! +//! ## Examples +//! +//! 1. **Appending** some data to the list can happen either by [`Pallet::append_one`]: +#![doc = docify::embed!("frame/paged-list/src/tests.rs", append_one_works)] +//! 2. or by [`Pallet::append_many`]. This should always be preferred to repeated calls to +//! [`Pallet::append_one`]: +#![doc = docify::embed!("frame/paged-list/src/tests.rs", append_many_works)] +//! 3. If you want to append many values (ie. in a loop), then best use the [`Pallet::appender`]: +#![doc = docify::embed!("frame/paged-list/src/tests.rs", appender_works)] +//! 4. **Iterating** over the list can be done with [`Pallet::iter`]. It uses the standard +//! `Iterator` trait. For testing, there is a `Pallet::as_vec` convenience function: +#![doc = docify::embed!("frame/paged-list/src/tests.rs", iter_works)] +//! 5. **Draining** elements happens through the [`Pallet::drain`] iterator. For testing, there is a +//! `Pallet::as_drained_vec` convenience function. Note that even *peeking* a value will remove it. +#![doc = docify::embed!("frame/paged-list/src/tests.rs", drain_works)] +//! +//! ## Pallet API +//! +//! None. Only things to consider is the [`Config`] traits. +//! +//! ## Low Level / Implementation Details +//! +//! Implementation details are documented in [`paged_list::StoragePagedList`]. #![cfg_attr(not(feature = "std"), no_std)] -#![doc = include_str!("../README.md")] pub use pallet::*; @@ -105,3 +144,10 @@ impl, I: 'static> StorageInstance for ListPrefix { const STORAGE_PREFIX: &'static str = "paged_list"; } + +#[cfg(test)] +impl, I: 'static> Pallet { + pub(crate) fn as_vec() -> Vec { + List::::iter().collect::>() + } +} diff --git a/frame/paged-list/src/mock.rs b/frame/paged-list/src/mock.rs index 1afa997ffb629..f12d807139ed2 100644 --- a/frame/paged-list/src/mock.rs +++ b/frame/paged-list/src/mock.rs @@ -39,7 +39,7 @@ frame_support::construct_runtime!( UncheckedExtrinsic = UncheckedExtrinsic, { System: frame_system, - PagedList1: crate, + PagedList: crate, PagedList2: crate::, } ); @@ -97,7 +97,13 @@ pub type MetaOf = StoragePagedListMeta< ::ValuesPerPage, >; -// Build genesis storage according to the mock runtime. +/// Build genesis storage according to the mock runtime. pub fn new_test_ext() -> sp_io::TestExternalities { frame_system::GenesisConfig::default().build_storage::().unwrap().into() } + +/// Run this closure in test externalities. +pub fn test_closure(f: impl FnOnce() -> R) -> R { + let mut ext = new_test_ext(); + ext.execute_with(f) +} diff --git a/frame/paged-list/src/tests.rs b/frame/paged-list/src/tests.rs index cdaa7ccac6c03..1fcf0b51cdd65 100644 --- a/frame/paged-list/src/tests.rs +++ b/frame/paged-list/src/tests.rs @@ -15,25 +15,87 @@ // See the License for the specific language governing permissions and // limitations under the License. +//! Pallet tests. Other tests are in [`super::paged_list`]. + #![cfg(test)] use crate::{mock::*, *}; use frame_support::storage::{StorageList, StoragePrefixedContainer}; +#[docify::export] +#[test] +fn append_one_works() { + test_closure(|| { + PagedList::append_one(1); + + assert_eq!(PagedList::as_vec(), vec![1]); + }); +} + +#[docify::export] +#[test] +fn append_many_works() { + test_closure(|| { + PagedList::append_many(0..3); + + assert_eq!(PagedList::as_vec(), vec![0, 1, 2]); + }); +} + +#[docify::export] +#[test] +fn appender_works() { + use frame_support::storage::StorageAppender; + test_closure(|| { + let mut appender = PagedList::appender(); + + appender.append(0); + appender.append(1); // Repeated calls are fine here. + appender.append_many(2..4); + + assert_eq!(PagedList::as_vec(), vec![0, 1, 2, 3]); + }); +} + +#[docify::export] +#[test] +fn iter_works() { + test_closure(|| { + PagedList::append_many(0..10); + let mut iter = PagedList::iter(); + + assert_eq!(iter.next(), Some(0)); + assert_eq!(iter.next(), Some(1)); + assert_eq!(iter.collect::>(), (2..10).collect::>()); + }); +} + +#[docify::export] +#[test] +fn drain_works() { + test_closure(|| { + PagedList::append_many(0..3); + PagedList::drain().next(); + assert_eq!(PagedList::as_vec(), vec![1, 2], "0 is drained"); + PagedList::drain().peekable().peek(); + assert_eq!(PagedList::as_vec(), vec![2], "Peeking removed 1"); + }); +} + #[test] fn iter_independent_works() { - new_test_ext().execute_with(|| { - PagedList1::append_many(0..1000); + test_closure(|| { + PagedList::append_many(0..1000); PagedList2::append_many(0..1000); - assert_eq!(PagedList1::iter().collect::>(), (0..1000).collect::>()); - assert_eq!(PagedList1::iter().collect::>(), (0..1000).collect::>()); + assert_eq!(PagedList::iter().collect::>(), (0..1000).collect::>()); + assert_eq!(PagedList::iter().collect::>(), (0..1000).collect::>()); // drain - assert_eq!(PagedList1::drain().collect::>(), (0..1000).collect::>()); + assert_eq!(PagedList::drain().collect::>(), (0..1000).collect::>()); assert_eq!(PagedList2::iter().collect::>(), (0..1000).collect::>()); - assert_eq!(PagedList1::iter().count(), 0); + assert_eq!(PagedList::iter().count(), 0); }); } From fbfe4521aa0eaa4fa224c3e1d85fc9ab8df5568f Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 30 May 2023 16:10:29 +0200 Subject: [PATCH 26/40] Fix fuzzer Signed-off-by: Oliver Tale-Yazdi --- frame/paged-list/fuzzer/src/paged_list.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/frame/paged-list/fuzzer/src/paged_list.rs b/frame/paged-list/fuzzer/src/paged_list.rs index a2a4809f0a703..acf735deb3886 100644 --- a/frame/paged-list/fuzzer/src/paged_list.rs +++ b/frame/paged-list/fuzzer/src/paged_list.rs @@ -31,7 +31,7 @@ use arbitrary::Arbitrary; use honggfuzz::fuzz; use frame_support::{storage::StorageList, StorageNoopGuard}; -use pallet_paged_list::mock::{PagedList1 as List, *}; +use pallet_paged_list::mock::{PagedList as List, *}; use sp_io::TestExternalities; type Meta = MetaOf; @@ -44,6 +44,8 @@ fn main() { } /// Appends and drains random number of elements in random order and checks storage invariants. +/// +/// It also changes the maximal number of elements per page dynamically, hence the `page_size`. fn drain_append_work(ops: Vec, page_size: u8) { if page_size == 0 { return @@ -60,7 +62,7 @@ fn drain_append_work(ops: Vec, page_size: u8) { assert!(total >= 0); assert_eq!(List::iter().count(), total as usize); - // We have the assumption that the queue removes the metadata when being empty. + // We have the assumption that the queue removes the metadata when empty. if total == 0 { assert_eq!(List::drain().count(), 0); assert_eq!(Meta::from_storage().unwrap_or_default(), Default::default()); @@ -68,7 +70,7 @@ fn drain_append_work(ops: Vec, page_size: u8) { } assert_eq!(List::drain().count(), total as usize); - // No storage leaking (checked by `StorageNoopGuard`). + // `StorageNoopGuard` checks that there is no storage leaked. }); } From 5e18eed30095b81080de1b990e14fe1ee3cd03a3 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 30 May 2023 16:10:40 +0200 Subject: [PATCH 27/40] Docs + examples Signed-off-by: Oliver Tale-Yazdi --- frame/paged-list/src/lib.rs | 22 +++++++++++++++------- frame/paged-list/src/mock.rs | 2 ++ frame/paged-list/src/tests.rs | 33 +++++++++++++++++++++++++++------ 3 files changed, 44 insertions(+), 13 deletions(-) diff --git a/frame/paged-list/src/lib.rs b/frame/paged-list/src/lib.rs index 2193d48fb43fa..3664ded7b8508 100644 --- a/frame/paged-list/src/lib.rs +++ b/frame/paged-list/src/lib.rs @@ -31,7 +31,10 @@ //! ## Overview //! //! The pallet is quite unique since it does not expose any `Call`s, `Error`s or `Event`s. All -//! interaction goes through the implemented [`StorageList`] trait. +//! interaction goes through the implemented [`StorageList`][frame_support::storage::StorageList] +//! trait. +//! +//! A fuzzer for testing is provided in crate `pallet-paged-list-fuzzer`. //! //! ## Examples //! @@ -43,11 +46,14 @@ //! 3. If you want to append many values (ie. in a loop), then best use the [`Pallet::appender`]: #![doc = docify::embed!("frame/paged-list/src/tests.rs", appender_works)] //! 4. **Iterating** over the list can be done with [`Pallet::iter`]. It uses the standard -//! `Iterator` trait. For testing, there is a `Pallet::as_vec` convenience function: +//! `Iterator` trait: #![doc = docify::embed!("frame/paged-list/src/tests.rs", iter_works)] -//! 5. **Draining** elements happens through the [`Pallet::drain`] iterator. For testing, there is a -//! `Pallet::as_drained_vec` convenience function. Note that even *peeking* a value will remove it. +//! 5. **Draining** elements happens through the [`Pallet::drain`] iterator. Note that even +//! *peeking* a value will already remove it. #![doc = docify::embed!("frame/paged-list/src/tests.rs", drain_works)] +//! 6. **Testing** convenience functions are provided by `as_vec` and `as_drained_vec`: +#![doc = docify::embed!("frame/paged-list/src/tests.rs", as_vec_works)] +#![doc = docify::embed!("frame/paged-list/src/tests.rs", as_drained_vec_works)] //! //! ## Pallet API //! @@ -132,9 +138,7 @@ impl, I: 'static> StorageList for Pallet { } } -/// The storage prefix for the list. -/// -/// Unique for each instance. +/// Generates a unique storage prefix for each instance of the pallet. pub struct ListPrefix(core::marker::PhantomData<(T, I)>); impl, I: 'static> StorageInstance for ListPrefix { @@ -150,4 +154,8 @@ impl, I: 'static> Pallet { pub(crate) fn as_vec() -> Vec { List::::iter().collect::>() } + + pub(crate) fn as_drained_vec() -> Vec { + List::::drain().collect::>() + } } diff --git a/frame/paged-list/src/mock.rs b/frame/paged-list/src/mock.rs index f12d807139ed2..13aa41b45dc9b 100644 --- a/frame/paged-list/src/mock.rs +++ b/frame/paged-list/src/mock.rs @@ -15,6 +15,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +//! Testing helpers. + #![cfg(feature = "std")] use crate::{paged_list::StoragePagedListMeta, Config, ListPrefix}; diff --git a/frame/paged-list/src/tests.rs b/frame/paged-list/src/tests.rs index 1fcf0b51cdd65..5f7565ade724a 100644 --- a/frame/paged-list/src/tests.rs +++ b/frame/paged-list/src/tests.rs @@ -15,7 +15,8 @@ // See the License for the specific language governing permissions and // limitations under the License. -//! Pallet tests. Other tests are in [`super::paged_list`]. +//! Mostly pallet doc-tests. Real tests are in [`super::paged_list`] and crate +//! `pallet-paged-list-fuzzer`. #![cfg(test)] @@ -28,7 +29,7 @@ fn append_one_works() { test_closure(|| { PagedList::append_one(1); - assert_eq!(PagedList::as_vec(), vec![1]); + assert_eq!(PagedList::iter().collect::>(), vec![1]); }); } @@ -38,7 +39,7 @@ fn append_many_works() { test_closure(|| { PagedList::append_many(0..3); - assert_eq!(PagedList::as_vec(), vec![0, 1, 2]); + assert_eq!(PagedList::iter().collect::>(), vec![0, 1, 2]); }); } @@ -53,7 +54,7 @@ fn appender_works() { appender.append(1); // Repeated calls are fine here. appender.append_many(2..4); - assert_eq!(PagedList::as_vec(), vec![0, 1, 2, 3]); + assert_eq!(PagedList::iter().collect::>(), vec![0, 1, 2, 3]); }); } @@ -76,9 +77,29 @@ fn drain_works() { test_closure(|| { PagedList::append_many(0..3); PagedList::drain().next(); - assert_eq!(PagedList::as_vec(), vec![1, 2], "0 is drained"); + assert_eq!(PagedList::iter().collect::>(), vec![1, 2], "0 is drained"); PagedList::drain().peekable().peek(); - assert_eq!(PagedList::as_vec(), vec![2], "Peeking removed 1"); + assert_eq!(PagedList::iter().collect::>(), vec![2], "Peeking removed 1"); + }); +} + +#[docify::export] +#[test] +fn as_vec_works() { + test_closure(|| { + PagedList::append_many(0..3); + assert_eq!(PagedList::as_vec(), vec![0, 1, 2]); + assert_eq!(PagedList::as_vec(), vec![0, 1, 2]); + }); +} + +#[docify::export] +#[test] +fn as_drained_vec_works() { + test_closure(|| { + PagedList::append_many(0..3); + assert_eq!(PagedList::as_drained_vec(), vec![0, 1, 2]); + assert!(PagedList::as_drained_vec().is_empty()); }); } From 7315a3b3a1280a17eae94449e06391a7b92ff3c2 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 30 May 2023 18:02:04 +0200 Subject: [PATCH 28/40] Remove hasher Signed-off-by: Oliver Tale-Yazdi --- frame/paged-list/src/lib.rs | 4 +- frame/paged-list/src/mock.rs | 15 +-- frame/paged-list/src/paged_list.rs | 157 ++++++++++++++++------------- 3 files changed, 92 insertions(+), 84 deletions(-) diff --git a/frame/paged-list/src/lib.rs b/frame/paged-list/src/lib.rs index 3664ded7b8508..3c2fb6bcfcaf3 100644 --- a/frame/paged-list/src/lib.rs +++ b/frame/paged-list/src/lib.rs @@ -61,7 +61,8 @@ //! //! ## Low Level / Implementation Details //! -//! Implementation details are documented in [`paged_list::StoragePagedList`]. +//! Implementation details are documented in [`paged_list::StoragePagedList`]. +//! All storage entries are prefixed with a unique prefix that is generated by [`ListPrefix`]. #![cfg_attr(not(feature = "std"), no_std)] @@ -90,7 +91,6 @@ pub mod pallet { // Note that FRAME does natively support paged lists in storage. pub type List = StoragePagedList< ListPrefix, - Blake2_128Concat, >::Value, >::ValuesPerPage, >::MaxPages, diff --git a/frame/paged-list/src/mock.rs b/frame/paged-list/src/mock.rs index 13aa41b45dc9b..6f8d9c327852b 100644 --- a/frame/paged-list/src/mock.rs +++ b/frame/paged-list/src/mock.rs @@ -15,15 +15,12 @@ // See the License for the specific language governing permissions and // limitations under the License. -//! Testing helpers. +//! Helpers for tests. #![cfg(feature = "std")] use crate::{paged_list::StoragePagedListMeta, Config, ListPrefix}; -use frame_support::{ - traits::{ConstU16, ConstU64}, - Blake2_128Concat, -}; +use frame_support::traits::{ConstU16, ConstU64}; use sp_core::H256; use sp_runtime::{ testing::Header, @@ -92,12 +89,8 @@ impl crate::Config for Test { type MaxPages = MaxPages; } -pub type MetaOf = StoragePagedListMeta< - ListPrefix, - Blake2_128Concat, - ::Value, - ::ValuesPerPage, ->; +pub type MetaOf = + StoragePagedListMeta, ::Value, ::ValuesPerPage>; /// Build genesis storage according to the mock runtime. pub fn new_test_ext() -> sp_io::TestExternalities { diff --git a/frame/paged-list/src/paged_list.rs b/frame/paged-list/src/paged_list.rs index 3a2ec32c3923a..96e389c475307 100644 --- a/frame/paged-list/src/paged_list.rs +++ b/frame/paged-list/src/paged_list.rs @@ -29,8 +29,7 @@ use frame_support::{ defensive, storage::StoragePrefixedContainer, traits::{Get, GetDefault, StorageInstance}, - Blake2_128Concat, CloneNoBound, DebugNoBound, DefaultNoBound, EqNoBound, PartialEqNoBound, - StorageHasher, + CloneNoBound, DebugNoBound, DefaultNoBound, EqNoBound, PartialEqNoBound, }; use sp_runtime::traits::Saturating; use sp_std::prelude::*; @@ -78,8 +77,8 @@ pub type ValueIndex = u32; /// asymptotic case when using an `Appender`, and worse in all. For example when appending just /// one value. /// - It does incur a read overhead on the host side as compared to a `StorageValue>`. -pub struct StoragePagedList { - _phantom: PhantomData<(Prefix, Hasher, Value, ValuesPerPage, MaxPages)>, +pub struct StoragePagedList { + _phantom: PhantomData<(Prefix, Value, ValuesPerPage, MaxPages)>, } /// The state of a [`StoragePagedList`]. @@ -89,7 +88,7 @@ pub struct StoragePagedList { +pub struct StoragePagedListMeta { /// The first page that contains a value. /// /// Can be >0 when pages were deleted. @@ -109,15 +108,14 @@ pub struct StoragePagedListMeta { /// whole list is empty. The only case where this can happen is when both are `0`. pub last_page_len: ValueIndex, - _phantom: PhantomData<(Prefix, Hasher, Value, ValuesPerPage)>, + _phantom: PhantomData<(Prefix, Value, ValuesPerPage)>, } -impl frame_support::storage::StorageAppender - for StoragePagedListMeta +impl frame_support::storage::StorageAppender + for StoragePagedListMeta where Prefix: StorageInstance, Value: FullCodec, - Hasher: StorageHasher, ValuesPerPage: Get, { fn append(&mut self, item: EncodeLikeValue) @@ -128,22 +126,20 @@ where } } -impl - StoragePagedListMeta +impl StoragePagedListMeta where Prefix: StorageInstance, Value: FullCodec, - Hasher: StorageHasher, ValuesPerPage: Get, { pub fn from_storage() -> Option { let key = Self::key(); - sp_io::storage::get(key.as_ref()).and_then(|raw| Self::decode(&mut &raw[..]).ok()) + sp_io::storage::get(&key).and_then(|raw| Self::decode(&mut &raw[..]).ok()) } - pub fn key() -> Hasher::Output { - (StoragePagedListPrefix::::final_prefix(), b"meta").using_encoded(Hasher::hash) + pub fn key() -> Vec { + meta_key::() } pub fn append_one(&mut self, item: EncodeLikeValue) @@ -155,15 +151,15 @@ where self.last_page.saturating_inc(); self.last_page_len = 0; } - let key = page_key::(self.last_page); + let key = page_key::(self.last_page); self.last_page_len.saturating_inc(); - sp_io::storage::append(key.as_ref(), item.encode()); + sp_io::storage::append(&key, item.encode()); self.store(); } pub fn store(&self) { let key = Self::key(); - self.using_encoded(|encoded| sp_io::storage::set(key.as_ref(), encoded)); + self.using_encoded(|enc| sp_io::storage::set(&key, enc)); } pub fn reset(&mut self) { @@ -172,8 +168,7 @@ where } pub fn delete() { - let key = Self::key(); - sp_io::storage::clear(key.as_ref()); + sp_io::storage::clear(&Self::key()); } } @@ -187,12 +182,12 @@ pub struct Page { impl Page { /// Read the page with `index` from storage and assume the first value at `value_index`. - pub fn from_storage( + pub fn from_storage( index: PageIndex, value_index: ValueIndex, ) -> Option { - let key = page_key::(index); - let values = sp_io::storage::get(key.as_ref()) + let key = page_key::(index); + let values = sp_io::storage::get(&key) .and_then(|raw| sp_std::vec::Vec::::decode(&mut &raw[..]).ok())?; if values.is_empty() { // Dont create empty pages. @@ -204,24 +199,26 @@ impl Page { } /// Delete this page from storage. - pub fn delete(&self) { - delete_page::(self.index); + pub fn delete(&self) { + delete_page::(self.index); } } /// Delete a page with `index` from storage. // Does not live under `Page` since it does not require the `Value` generic. -pub(crate) fn delete_page(index: PageIndex) { - let key = page_key::(index); - sp_io::storage::clear(key.as_ref()); +pub(crate) fn delete_page(index: PageIndex) { + let key = page_key::(index); + sp_io::storage::clear(&key); } /// Storage key of a page with `index`. // Does not live under `Page` since it does not require the `Value` generic. -pub(crate) fn page_key( - index: PageIndex, -) -> Hasher::Output { - (StoragePagedListPrefix::::final_prefix(), b"page", index).using_encoded(Hasher::hash) +pub(crate) fn page_key(index: PageIndex) -> Vec { + (StoragePagedListPrefix::::final_prefix(), b"page", index).encode() +} + +pub(crate) fn meta_key() -> Vec { + (StoragePagedListPrefix::::final_prefix(), b"meta").encode() } impl Iterator for Page { @@ -235,7 +232,7 @@ impl Iterator for Page { /// Iterates over values of a [`StoragePagedList`]. /// /// Can optionally drain the iterated values. -pub struct StoragePagedListIterator { +pub struct StoragePagedListIterator { // Design: we put the Page into the iterator to have fewer storage look-ups. Yes, these // look-ups would be cached anyway, but bugging the overlay on each `.next` call still seems // like a poor trade-off than caching it in the iterator directly. Iterating and modifying is @@ -243,34 +240,30 @@ pub struct StoragePagedListIterator { // the iterator did not find any data upon setup or ran out of pages. page: Option>, drain: bool, - meta: StoragePagedListMeta, + meta: StoragePagedListMeta, } -impl - StoragePagedListIterator +impl StoragePagedListIterator where Prefix: StorageInstance, Value: FullCodec, - Hasher: StorageHasher, ValuesPerPage: Get, { /// Read self from the storage. pub fn from_meta( - meta: StoragePagedListMeta, + meta: StoragePagedListMeta, drain: bool, ) -> Self { - let page = - Page::::from_storage::(meta.first_page, meta.first_value_offset); + let page = Page::::from_storage::(meta.first_page, meta.first_value_offset); Self { page, drain, meta } } } -impl Iterator - for StoragePagedListIterator +impl Iterator + for StoragePagedListIterator where Prefix: StorageInstance, Value: FullCodec, - Hasher: StorageHasher, ValuesPerPage: Get, { type Item = Value; @@ -287,13 +280,13 @@ where } if self.drain { // page is empty - page.delete::(); + page.delete::(); self.meta.first_value_offset = 0; self.meta.first_page.saturating_inc(); self.meta.store(); } - let Some(mut page) = Page::from_storage::(page.index.saturating_add(1), 0) else { + let Some(mut page) = Page::from_storage::(page.index.saturating_add(1), 0) else { self.page = None; if self.drain { self.meta.reset(); @@ -318,17 +311,16 @@ where } } -impl frame_support::storage::StorageList - for StoragePagedList +impl frame_support::storage::StorageList + for StoragePagedList where Prefix: StorageInstance, Value: FullCodec, - Hasher: StorageHasher, ValuesPerPage: Get, MaxPages: Get>, { - type Iterator = StoragePagedListIterator; - type Appender = StoragePagedListMeta; + type Iterator = StoragePagedListIterator; + type Appender = StoragePagedListMeta; fn iter() -> Self::Iterator { StoragePagedListIterator::from_meta(Self::read_meta(), false) @@ -343,16 +335,15 @@ where } } -impl - StoragePagedList +impl + StoragePagedList where Prefix: StorageInstance, Value: FullCodec, - Hasher: StorageHasher, ValuesPerPage: Get, MaxPages: Get>, { - fn read_meta() -> StoragePagedListMeta { + fn read_meta() -> StoragePagedListMeta { // Use default here to not require a setup migration. StoragePagedListMeta::from_storage().unwrap_or_default() } @@ -360,7 +351,7 @@ where /// Provides a fast append iterator. /// /// The list should not be modified while appending. Also don't call it recursively. - fn appender() -> StoragePagedListMeta { + fn appender() -> StoragePagedListMeta { Self::read_meta() } @@ -398,13 +389,11 @@ where } } -impl - frame_support::storage::StoragePrefixedContainer - for StoragePagedList +impl frame_support::storage::StoragePrefixedContainer + for StoragePagedList where Prefix: StorageInstance, Value: FullCodec, - Hasher: StorageHasher, ValuesPerPage: Get, MaxPages: Get>, { @@ -443,7 +432,7 @@ pub(crate) mod mock { const STORAGE_PREFIX: &'static str = "foo"; } - pub type List = StoragePagedList; + pub type List = StoragePagedList; } #[cfg(test)] @@ -472,7 +461,7 @@ mod tests { // all gone assert_eq!(List::as_vec(), Vec::::new()); // Need to delete the metadata manually. - StoragePagedListMeta::::delete(); + StoragePagedListMeta::::delete(); }); } @@ -516,27 +505,53 @@ mod tests { TestExternalities::default().execute_with(|| { List::append_many(0..9); - let key = page_key::(0); - let raw = - frame_support::storage::unhashed::get_raw(&key).expect("Page should be present"); + let key = page_key::(0); + let raw = sp_io::storage::get(&key).expect("Page should be present"); let as_vec = Vec::::decode(&mut &raw[..]).unwrap(); assert_eq!(as_vec.len(), 5, "First page contains 5"); - let key = page_key::(1); - let raw = - frame_support::storage::unhashed::get_raw(&key).expect("Page should be present"); + let key = page_key::(1); + let raw = sp_io::storage::get(&key).expect("Page should be present"); let as_vec = Vec::::decode(&mut &raw[..]).unwrap(); assert_eq!(as_vec.len(), 4, "Second page contains 4"); + + let meta = sp_io::storage::get(&meta_key::()).expect("Meta should be present"); + let meta: StoragePagedListMeta = + Decode::decode(&mut &meta[..]).unwrap(); + assert_eq!(meta.first_page, 0); + assert_eq!(meta.first_value_offset, 0); + assert_eq!(meta.last_page, 1); + assert_eq!(meta.last_page_len, 4); + + let page = Page::::from_storage::(0, 0).unwrap(); + assert_eq!(page.index, 0); + assert_eq!(page.values.count(), 5); + + let page = Page::::from_storage::(1, 0).unwrap(); + assert_eq!(page.index, 1); + assert_eq!(page.values.count(), 4); }); } #[test] fn page_key_correct() { - let got = page_key::(0); - // FAIL-CI this is wrong - let want = (StoragePagedListPrefix::::final_prefix(), b"page", 0) - .using_encoded(Blake2_128Concat::hash); + let got = page_key::(0); + let pallet_prefix = StoragePagedListPrefix::::final_prefix(); + let want = (pallet_prefix, b"page", 0).encode(); + + assert_eq!(want.len(), 32 + 4 + 4); + assert!(want.starts_with(&pallet_prefix[..])); + assert_eq!(got, want); + } + + #[test] + fn meta_key_correct() { + let got = meta_key::(); + let pallet_prefix = StoragePagedListPrefix::::final_prefix(); + let want = (pallet_prefix, b"meta").encode(); + assert_eq!(want.len(), 32 + 4); + assert!(want.starts_with(&pallet_prefix[..])); assert_eq!(got, want); } From 4958b104dc11c62a51ae4867baa6ac35f589842a Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 30 May 2023 18:21:12 +0200 Subject: [PATCH 29/40] Remove empty Event and Call Signed-off-by: Oliver Tale-Yazdi --- frame/paged-list/src/lib.rs | 25 ++++++++----------------- frame/paged-list/src/mock.rs | 2 -- 2 files changed, 8 insertions(+), 19 deletions(-) diff --git a/frame/paged-list/src/lib.rs b/frame/paged-list/src/lib.rs index 3c2fb6bcfcaf3..2141c5eb5bf45 100644 --- a/frame/paged-list/src/lib.rs +++ b/frame/paged-list/src/lib.rs @@ -87,20 +87,8 @@ pub mod pallet { #[pallet::pallet] pub struct Pallet(_); - /// A storage paged list akin to what the FRAME macros would generate. - // Note that FRAME does natively support paged lists in storage. - pub type List = StoragePagedList< - ListPrefix, - >::Value, - >::ValuesPerPage, - >::MaxPages, - >; - #[pallet::config] pub trait Config: frame_system::Config { - type RuntimeEvent: From> - + IsType<::RuntimeEvent>; - /// The value type that can be stored in the list. type Value: FullCodec + Clone + MaxEncodedLen; @@ -113,11 +101,14 @@ pub mod pallet { type MaxPages: Get>; } - #[pallet::event] - pub enum Event, I: 'static = ()> {} - - #[pallet::call] - impl, I: 'static> Pallet {} + /// A storage paged list akin to what the FRAME macros would generate. + // Note that FRAME does natively support paged lists in storage. + pub type List = StoragePagedList< + ListPrefix, + >::Value, + >::ValuesPerPage, + >::MaxPages, + >; } // This exposes the list functionality to other pallets. diff --git a/frame/paged-list/src/mock.rs b/frame/paged-list/src/mock.rs index 6f8d9c327852b..14c2c461c044a 100644 --- a/frame/paged-list/src/mock.rs +++ b/frame/paged-list/src/mock.rs @@ -76,14 +76,12 @@ frame_support::parameter_types! { } impl crate::Config for Test { - type RuntimeEvent = RuntimeEvent; type Value = u32; type ValuesPerPage = ValuesPerPage; type MaxPages = MaxPages; } impl crate::Config for Test { - type RuntimeEvent = RuntimeEvent; type Value = u32; type ValuesPerPage = ValuesPerPage; type MaxPages = MaxPages; From a13223be86bee22de27116e45956116ba6e7cf0b Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 30 May 2023 18:23:58 +0200 Subject: [PATCH 30/40] Remove MaxPages Signed-off-by: Oliver Tale-Yazdi --- frame/paged-list/src/lib.rs | 5 ----- frame/paged-list/src/mock.rs | 2 -- frame/paged-list/src/paged_list.rs | 22 +++++++++------------- 3 files changed, 9 insertions(+), 20 deletions(-) diff --git a/frame/paged-list/src/lib.rs b/frame/paged-list/src/lib.rs index 2141c5eb5bf45..b2b0d2dfb729c 100644 --- a/frame/paged-list/src/lib.rs +++ b/frame/paged-list/src/lib.rs @@ -95,10 +95,6 @@ pub mod pallet { /// The number of values per page. #[pallet::constant] type ValuesPerPage: Get; - - /// The maximal number of pages in the list. - #[pallet::constant] - type MaxPages: Get>; } /// A storage paged list akin to what the FRAME macros would generate. @@ -107,7 +103,6 @@ pub mod pallet { ListPrefix, >::Value, >::ValuesPerPage, - >::MaxPages, >; } diff --git a/frame/paged-list/src/mock.rs b/frame/paged-list/src/mock.rs index 14c2c461c044a..d8b0ee1b608e9 100644 --- a/frame/paged-list/src/mock.rs +++ b/frame/paged-list/src/mock.rs @@ -78,13 +78,11 @@ frame_support::parameter_types! { impl crate::Config for Test { type Value = u32; type ValuesPerPage = ValuesPerPage; - type MaxPages = MaxPages; } impl crate::Config for Test { type Value = u32; type ValuesPerPage = ValuesPerPage; - type MaxPages = MaxPages; } pub type MetaOf = diff --git a/frame/paged-list/src/paged_list.rs b/frame/paged-list/src/paged_list.rs index 96e389c475307..3ed668ac1fb4a 100644 --- a/frame/paged-list/src/paged_list.rs +++ b/frame/paged-list/src/paged_list.rs @@ -28,7 +28,7 @@ use core::marker::PhantomData; use frame_support::{ defensive, storage::StoragePrefixedContainer, - traits::{Get, GetDefault, StorageInstance}, + traits::{Get, StorageInstance}, CloneNoBound, DebugNoBound, DefaultNoBound, EqNoBound, PartialEqNoBound, }; use sp_runtime::traits::Saturating; @@ -77,8 +77,8 @@ pub type ValueIndex = u32; /// asymptotic case when using an `Appender`, and worse in all. For example when appending just /// one value. /// - It does incur a read overhead on the host side as compared to a `StorageValue>`. -pub struct StoragePagedList { - _phantom: PhantomData<(Prefix, Value, ValuesPerPage, MaxPages)>, +pub struct StoragePagedList { + _phantom: PhantomData<(Prefix, Value, ValuesPerPage)>, } /// The state of a [`StoragePagedList`]. @@ -311,13 +311,12 @@ where } } -impl frame_support::storage::StorageList - for StoragePagedList +impl frame_support::storage::StorageList + for StoragePagedList where Prefix: StorageInstance, Value: FullCodec, ValuesPerPage: Get, - MaxPages: Get>, { type Iterator = StoragePagedListIterator; type Appender = StoragePagedListMeta; @@ -335,13 +334,11 @@ where } } -impl - StoragePagedList +impl StoragePagedList where Prefix: StorageInstance, Value: FullCodec, ValuesPerPage: Get, - MaxPages: Get>, { fn read_meta() -> StoragePagedListMeta { // Use default here to not require a setup migration. @@ -389,13 +386,12 @@ where } } -impl frame_support::storage::StoragePrefixedContainer - for StoragePagedList +impl frame_support::storage::StoragePrefixedContainer + for StoragePagedList where Prefix: StorageInstance, Value: FullCodec, ValuesPerPage: Get, - MaxPages: Get>, { fn module_prefix() -> &'static [u8] { StoragePagedListPrefix::::module_prefix() @@ -432,7 +428,7 @@ pub(crate) mod mock { const STORAGE_PREFIX: &'static str = "foo"; } - pub type List = StoragePagedList; + pub type List = StoragePagedList; } #[cfg(test)] From 4d3ad58ceda700f7ed91768341072f2f45962386 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 30 May 2023 18:32:37 +0200 Subject: [PATCH 31/40] Fix docs Signed-off-by: Oliver Tale-Yazdi --- frame/paged-list/src/paged_list.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/paged-list/src/paged_list.rs b/frame/paged-list/src/paged_list.rs index 3ed668ac1fb4a..8124ea9ee7035 100644 --- a/frame/paged-list/src/paged_list.rs +++ b/frame/paged-list/src/paged_list.rs @@ -89,11 +89,11 @@ pub struct StoragePagedList { )] // todo ignore scale bounds pub struct StoragePagedListMeta { - /// The first page that contains a value. + /// The first page that could contain a value. /// /// Can be >0 when pages were deleted. pub first_page: PageIndex, - /// The first value inside `first_page` that contains a value. + /// The first index inside `first_page` that could contain a value. /// /// Can be >0 when values were deleted. pub first_value_offset: ValueIndex, From 79cdffa47f1c5e8d6a68f73ca55d4e2c6e776e94 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 30 May 2023 18:41:15 +0200 Subject: [PATCH 32/40] Test eager page removal Signed-off-by: Oliver Tale-Yazdi --- frame/paged-list/src/paged_list.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/frame/paged-list/src/paged_list.rs b/frame/paged-list/src/paged_list.rs index 8124ea9ee7035..da67b9d97d6f1 100644 --- a/frame/paged-list/src/paged_list.rs +++ b/frame/paged-list/src/paged_list.rs @@ -495,6 +495,22 @@ mod tests { }); } + /// Pages are removed ASAP. + #[test] + fn drain_eager_page_removal() { + TestExternalities::default().execute_with(|| { + List::append_many(0..9); + + assert!(sp_io::storage::exists(&page_key::(0))); + assert!(sp_io::storage::exists(&page_key::(1))); + + assert_eq!(List::drain().take(5).count(), 5); + // Page 0 is eagerly removed. + assert!(!sp_io::storage::exists(&page_key::(0))); + assert!(sp_io::storage::exists(&page_key::(1))); + }); + } + /// Appending encodes pages as `Vec`. #[test] fn append_storage_layout() { From ff3638cde0844f70ce3dffd5d5ea842764240530 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 30 May 2023 19:38:50 +0200 Subject: [PATCH 33/40] Cleanup Signed-off-by: Oliver Tale-Yazdi --- frame/paged-list/Cargo.toml | 4 +- frame/paged-list/src/lib.rs | 2 +- frame/paged-list/src/paged_list.rs | 62 +++++++++++++++--------------- 3 files changed, 33 insertions(+), 35 deletions(-) diff --git a/frame/paged-list/Cargo.toml b/frame/paged-list/Cargo.toml index 3bb0265b5e91a..8a37fb95b9eb9 100644 --- a/frame/paged-list/Cargo.toml +++ b/frame/paged-list/Cargo.toml @@ -22,8 +22,8 @@ frame-system = { version = "4.0.0-dev", default-features = false, path = "../sys sp-runtime = { version = "8.0.0", default-features = false, path = "../../primitives/runtime" } sp-std = { version = "6.0.0", default-features = false, path = "../../primitives/std" } -sp-core = { version = "8.0.0", path = "../../primitives/core", default-features = false, optional = true } -sp-io = { version = "8.0.0", path = "../../primitives/io", default-features = false, optional = true } +sp-core = { version = "8.0.0", path = "../../primitives/core", default-features = false } +sp-io = { version = "8.0.0", path = "../../primitives/io", default-features = false } [features] default = ["std"] diff --git a/frame/paged-list/src/lib.rs b/frame/paged-list/src/lib.rs index b2b0d2dfb729c..283238ee1f81a 100644 --- a/frame/paged-list/src/lib.rs +++ b/frame/paged-list/src/lib.rs @@ -90,7 +90,7 @@ pub mod pallet { #[pallet::config] pub trait Config: frame_system::Config { /// The value type that can be stored in the list. - type Value: FullCodec + Clone + MaxEncodedLen; + type Value: FullCodec; /// The number of values per page. #[pallet::constant] diff --git a/frame/paged-list/src/paged_list.rs b/frame/paged-list/src/paged_list.rs index da67b9d97d6f1..40b8dc3ceff15 100644 --- a/frame/paged-list/src/paged_list.rs +++ b/frame/paged-list/src/paged_list.rs @@ -198,6 +198,11 @@ impl Page { Some(Self { index, values }) } + /// Whether no more values can be read from this page. + pub fn is_eof(&self) -> bool { + self.values.len() == 0 + } + /// Delete this page from storage. pub fn delete(&self) { delete_page::(self.index); @@ -270,44 +275,37 @@ where fn next(&mut self) -> Option { let page = self.page.as_mut()?; - - if let Some(val) = page.next() { - if self.drain { - self.meta.first_value_offset.saturating_inc(); - self.meta.store() - } - return Some(val) - } - if self.drain { - // page is empty - page.delete::(); - self.meta.first_value_offset = 0; - self.meta.first_page.saturating_inc(); - self.meta.store(); - } - - let Some(mut page) = Page::from_storage::(page.index.saturating_add(1), 0) else { - self.page = None; - if self.drain { + let value = match page.next() { + Some(value) => { + if self.drain { + self.meta.first_value_offset.saturating_inc(); + self.meta.store() + } + value + }, + None => { + defensive!("There are no empty pages in storage; nuking the list"); self.meta.reset(); - } - return None; + self.page = None; + return None + }, }; - if let Some(val) = page.next() { - self.page = Some(page); + if page.is_eof() { if self.drain { - self.meta.first_value_offset.saturating_inc(); + page.delete::(); + self.meta.first_value_offset = 0; + self.meta.first_page.saturating_inc(); self.meta.store(); } - return Some(val) - } - defensive!("StoragePagedListIterator: empty pages do not exist: storage corrupted"); - // Put the storage back into a consistent state. - self.meta.reset(); - self.page = None; - None + debug_assert!(!self.drain || self.meta.first_page == page.index + 1); + self.page = Page::from_storage::(page.index.saturating_add(1), 0); + if self.page.is_none() && self.drain { + self.meta.reset(); + } + } + Some(value) } } @@ -472,7 +470,7 @@ mod tests { let meta = List::read_meta(); // Will switch over to `10/0`, but will in the next call. - assert_eq!((meta.first_page, meta.first_value_offset), (9, 5)); + assert_eq!((meta.first_page, meta.first_value_offset), (10, 0)); // 50 gone, 50 to go assert_eq!(List::as_vec(), (50..100).collect::>()); From 351678c4e1c9d55ba69ff41db5e2e188fd822697 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 31 May 2023 19:11:51 +0200 Subject: [PATCH 34/40] Update frame/paged-list/src/paged_list.rs Co-authored-by: Koute --- frame/paged-list/src/paged_list.rs | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/frame/paged-list/src/paged_list.rs b/frame/paged-list/src/paged_list.rs index 40b8dc3ceff15..0cf267ca65ac2 100644 --- a/frame/paged-list/src/paged_list.rs +++ b/frame/paged-list/src/paged_list.rs @@ -276,13 +276,7 @@ where fn next(&mut self) -> Option { let page = self.page.as_mut()?; let value = match page.next() { - Some(value) => { - if self.drain { - self.meta.first_value_offset.saturating_inc(); - self.meta.store() - } - value - }, + Some(value) => value, None => { defensive!("There are no empty pages in storage; nuking the list"); self.meta.reset(); @@ -296,13 +290,21 @@ where page.delete::(); self.meta.first_value_offset = 0; self.meta.first_page.saturating_inc(); - self.meta.store(); } debug_assert!(!self.drain || self.meta.first_page == page.index + 1); self.page = Page::from_storage::(page.index.saturating_add(1), 0); - if self.page.is_none() && self.drain { - self.meta.reset(); + if self.drain { + if self.page.is_none() { + self.meta.reset(); + } else { + self.meta.store(); + } + } + } else { + if self.drain { + self.meta.first_value_offset.saturating_inc(); + self.meta.store(); } } Some(value) From a36a9238659f754ec07db98cd5d62cdd8f7c0fb7 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 31 May 2023 19:12:43 +0200 Subject: [PATCH 35/40] Fix docs Co-authored-by: Koute Signed-off-by: Oliver Tale-Yazdi --- frame/paged-list/src/paged_list.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/paged-list/src/paged_list.rs b/frame/paged-list/src/paged_list.rs index 0cf267ca65ac2..ad1162a5512e3 100644 --- a/frame/paged-list/src/paged_list.rs +++ b/frame/paged-list/src/paged_list.rs @@ -59,9 +59,9 @@ pub type ValueIndex = u32; /// [`first_value_offset`][StoragePagedListMeta::first_value_offset] and incrementing these indices /// as long as there are elements in the page and there are pages in storage. All elements of a page /// are loaded once a page is read from storage. Iteration then happens on the cached elements. This -/// reduces the storage `read` calls on the overlay. **Appending** to the list happens by appending -/// to the last page by utilizing [`sp_io::storage::append`]. It allows to directly extend the -/// elements of `values` vector of the page without loading the whole vector from storage. A new +/// reduces the number of storage `read` calls on the overlay. **Appending** to the list happens by +/// appending to the last page by utilizing [`sp_io::storage::append`]. It allows to directly extend +/// the elements of `values` vector of the page without loading the whole vector from storage. A new /// page is instantiated once [`Page::next`] overflows `ValuesPerPage`. Its vector will also be /// created through [`sp_io::storage::append`]. **Draining** advances the internal indices identical /// to Iteration. It additionally persists the increments to storage and thereby 'drains' elements. From 715fc97256e6cd5d17d5f35420e6d74370209e52 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 31 May 2023 19:14:48 +0200 Subject: [PATCH 36/40] Remove as_*_vec Signed-off-by: Oliver Tale-Yazdi --- frame/paged-list/src/lib.rs | 14 -------------- frame/paged-list/src/tests.rs | 20 -------------------- 2 files changed, 34 deletions(-) diff --git a/frame/paged-list/src/lib.rs b/frame/paged-list/src/lib.rs index 283238ee1f81a..0b781fc883388 100644 --- a/frame/paged-list/src/lib.rs +++ b/frame/paged-list/src/lib.rs @@ -51,9 +51,6 @@ //! 5. **Draining** elements happens through the [`Pallet::drain`] iterator. Note that even //! *peeking* a value will already remove it. #![doc = docify::embed!("frame/paged-list/src/tests.rs", drain_works)] -//! 6. **Testing** convenience functions are provided by `as_vec` and `as_drained_vec`: -#![doc = docify::embed!("frame/paged-list/src/tests.rs", as_vec_works)] -#![doc = docify::embed!("frame/paged-list/src/tests.rs", as_drained_vec_works)] //! //! ## Pallet API //! @@ -134,14 +131,3 @@ impl, I: 'static> StorageInstance for ListPrefix { const STORAGE_PREFIX: &'static str = "paged_list"; } - -#[cfg(test)] -impl, I: 'static> Pallet { - pub(crate) fn as_vec() -> Vec { - List::::iter().collect::>() - } - - pub(crate) fn as_drained_vec() -> Vec { - List::::drain().collect::>() - } -} diff --git a/frame/paged-list/src/tests.rs b/frame/paged-list/src/tests.rs index 5f7565ade724a..becb4b23508ef 100644 --- a/frame/paged-list/src/tests.rs +++ b/frame/paged-list/src/tests.rs @@ -83,26 +83,6 @@ fn drain_works() { }); } -#[docify::export] -#[test] -fn as_vec_works() { - test_closure(|| { - PagedList::append_many(0..3); - assert_eq!(PagedList::as_vec(), vec![0, 1, 2]); - assert_eq!(PagedList::as_vec(), vec![0, 1, 2]); - }); -} - -#[docify::export] -#[test] -fn as_drained_vec_works() { - test_closure(|| { - PagedList::append_many(0..3); - assert_eq!(PagedList::as_drained_vec(), vec![0, 1, 2]); - assert!(PagedList::as_drained_vec().is_empty()); - }); -} - #[test] fn iter_independent_works() { test_closure(|| { From 86836862c947caeb965a4b94538ea7519dfcb51d Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 31 May 2023 19:15:48 +0200 Subject: [PATCH 37/40] Update versions Signed-off-by: Oliver Tale-Yazdi --- frame/paged-list/Cargo.toml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frame/paged-list/Cargo.toml b/frame/paged-list/Cargo.toml index 8a37fb95b9eb9..301eeb3be2ab2 100644 --- a/frame/paged-list/Cargo.toml +++ b/frame/paged-list/Cargo.toml @@ -20,10 +20,10 @@ frame-benchmarking = { version = "4.0.0-dev", default-features = false, optional frame-support = { version = "4.0.0-dev", default-features = false, path = "../support" } frame-system = { version = "4.0.0-dev", default-features = false, path = "../system" } -sp-runtime = { version = "8.0.0", default-features = false, path = "../../primitives/runtime" } -sp-std = { version = "6.0.0", default-features = false, path = "../../primitives/std" } -sp-core = { version = "8.0.0", path = "../../primitives/core", default-features = false } -sp-io = { version = "8.0.0", path = "../../primitives/io", default-features = false } +sp-runtime = { version = "24.0.0", default-features = false, path = "../../primitives/runtime" } +sp-std = { version = "8.0.0", default-features = false, path = "../../primitives/std" } +sp-core = { version = "21.0.0", path = "../../primitives/core", default-features = false } +sp-io = { version = "23.0.0", path = "../../primitives/io", default-features = false } [features] default = ["std"] From 19bdd0e35910d45c8c29145211338da310bf3674 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 31 May 2023 19:17:56 +0200 Subject: [PATCH 38/40] Rename ValuesPerPage -> ValuesPerNewPage Signed-off-by: Oliver Tale-Yazdi --- frame/paged-list/fuzzer/src/paged_list.rs | 2 +- frame/paged-list/src/lib.rs | 9 ++- frame/paged-list/src/mock.rs | 8 +-- frame/paged-list/src/paged_list.rs | 74 +++++++++++------------ 4 files changed, 48 insertions(+), 45 deletions(-) diff --git a/frame/paged-list/fuzzer/src/paged_list.rs b/frame/paged-list/fuzzer/src/paged_list.rs index acf735deb3886..43b797eee6bfb 100644 --- a/frame/paged-list/fuzzer/src/paged_list.rs +++ b/frame/paged-list/fuzzer/src/paged_list.rs @@ -52,7 +52,7 @@ fn drain_append_work(ops: Vec, page_size: u8) { } TestExternalities::default().execute_with(|| { - ValuesPerPage::set(&page_size.into()); + ValuesPerNewPage::set(&page_size.into()); let _g = StorageNoopGuard::default(); let mut total: i64 = 0; diff --git a/frame/paged-list/src/lib.rs b/frame/paged-list/src/lib.rs index 0b781fc883388..9aa089bb09273 100644 --- a/frame/paged-list/src/lib.rs +++ b/frame/paged-list/src/lib.rs @@ -89,9 +89,12 @@ pub mod pallet { /// The value type that can be stored in the list. type Value: FullCodec; - /// The number of values per page. + /// The number of values that can be put into newly created pages. + /// + /// Note that this does not retroactively affect already created pages. This value can be + /// changed at any time without requiring a runtime migration. #[pallet::constant] - type ValuesPerPage: Get; + type ValuesPerNewPage: Get; } /// A storage paged list akin to what the FRAME macros would generate. @@ -99,7 +102,7 @@ pub mod pallet { pub type List = StoragePagedList< ListPrefix, >::Value, - >::ValuesPerPage, + >::ValuesPerNewPage, >; } diff --git a/frame/paged-list/src/mock.rs b/frame/paged-list/src/mock.rs index d8b0ee1b608e9..5ae1d49dc2f41 100644 --- a/frame/paged-list/src/mock.rs +++ b/frame/paged-list/src/mock.rs @@ -71,22 +71,22 @@ impl frame_system::Config for Test { } frame_support::parameter_types! { - pub storage ValuesPerPage: u32 = 5; + pub storage ValuesPerNewPage: u32 = 5; pub const MaxPages: Option = Some(20); } impl crate::Config for Test { type Value = u32; - type ValuesPerPage = ValuesPerPage; + type ValuesPerNewPage = ValuesPerNewPage; } impl crate::Config for Test { type Value = u32; - type ValuesPerPage = ValuesPerPage; + type ValuesPerNewPage = ValuesPerNewPage; } pub type MetaOf = - StoragePagedListMeta, ::Value, ::ValuesPerPage>; + StoragePagedListMeta, ::Value, ::ValuesPerNewPage>; /// Build genesis storage according to the mock runtime. pub fn new_test_ext() -> sp_io::TestExternalities { diff --git a/frame/paged-list/src/paged_list.rs b/frame/paged-list/src/paged_list.rs index ad1162a5512e3..37ebe80d93448 100644 --- a/frame/paged-list/src/paged_list.rs +++ b/frame/paged-list/src/paged_list.rs @@ -52,8 +52,8 @@ pub type ValueIndex = u32; /// The metadata of this struct is stored in [`StoragePagedListMeta`]. The data is stored in /// [`Page`]s. /// -/// Each [`Page`] holds at most `ValuesPerPage` values in its `values` vector. The last page is the -/// only one that could have less than `ValuesPerPage` values. +/// Each [`Page`] holds at most `ValuesPerNewPage` values in its `values` vector. The last page is +/// the only one that could have less than `ValuesPerNewPage` values. /// **Iteration** happens by starting /// at [`first_page`][StoragePagedListMeta::first_page]/ /// [`first_value_offset`][StoragePagedListMeta::first_value_offset] and incrementing these indices @@ -62,7 +62,7 @@ pub type ValueIndex = u32; /// reduces the number of storage `read` calls on the overlay. **Appending** to the list happens by /// appending to the last page by utilizing [`sp_io::storage::append`]. It allows to directly extend /// the elements of `values` vector of the page without loading the whole vector from storage. A new -/// page is instantiated once [`Page::next`] overflows `ValuesPerPage`. Its vector will also be +/// page is instantiated once [`Page::next`] overflows `ValuesPerNewPage`. Its vector will also be /// created through [`sp_io::storage::append`]. **Draining** advances the internal indices identical /// to Iteration. It additionally persists the increments to storage and thereby 'drains' elements. /// Completely drained pages are deleted from storage. @@ -77,8 +77,8 @@ pub type ValueIndex = u32; /// asymptotic case when using an `Appender`, and worse in all. For example when appending just /// one value. /// - It does incur a read overhead on the host side as compared to a `StorageValue>`. -pub struct StoragePagedList { - _phantom: PhantomData<(Prefix, Value, ValuesPerPage)>, +pub struct StoragePagedList { + _phantom: PhantomData<(Prefix, Value, ValuesPerNewPage)>, } /// The state of a [`StoragePagedList`]. @@ -88,7 +88,7 @@ pub struct StoragePagedList { Encode, Decode, CloneNoBound, PartialEqNoBound, EqNoBound, DebugNoBound, DefaultNoBound, )] // todo ignore scale bounds -pub struct StoragePagedListMeta { +pub struct StoragePagedListMeta { /// The first page that could contain a value. /// /// Can be >0 when pages were deleted. @@ -108,15 +108,15 @@ pub struct StoragePagedListMeta { /// whole list is empty. The only case where this can happen is when both are `0`. pub last_page_len: ValueIndex, - _phantom: PhantomData<(Prefix, Value, ValuesPerPage)>, + _phantom: PhantomData<(Prefix, Value, ValuesPerNewPage)>, } -impl frame_support::storage::StorageAppender - for StoragePagedListMeta +impl frame_support::storage::StorageAppender + for StoragePagedListMeta where Prefix: StorageInstance, Value: FullCodec, - ValuesPerPage: Get, + ValuesPerNewPage: Get, { fn append(&mut self, item: EncodeLikeValue) where @@ -126,11 +126,11 @@ where } } -impl StoragePagedListMeta +impl StoragePagedListMeta where Prefix: StorageInstance, Value: FullCodec, - ValuesPerPage: Get, + ValuesPerNewPage: Get, { pub fn from_storage() -> Option { let key = Self::key(); @@ -147,7 +147,7 @@ where EncodeLikeValue: EncodeLike, { // Note: we use >= here in case someone decreased it in a runtime upgrade. - if self.last_page_len >= ValuesPerPage::get() { + if self.last_page_len >= ValuesPerNewPage::get() { self.last_page.saturating_inc(); self.last_page_len = 0; } @@ -237,7 +237,7 @@ impl Iterator for Page { /// Iterates over values of a [`StoragePagedList`]. /// /// Can optionally drain the iterated values. -pub struct StoragePagedListIterator { +pub struct StoragePagedListIterator { // Design: we put the Page into the iterator to have fewer storage look-ups. Yes, these // look-ups would be cached anyway, but bugging the overlay on each `.next` call still seems // like a poor trade-off than caching it in the iterator directly. Iterating and modifying is @@ -245,18 +245,18 @@ pub struct StoragePagedListIterator { // the iterator did not find any data upon setup or ran out of pages. page: Option>, drain: bool, - meta: StoragePagedListMeta, + meta: StoragePagedListMeta, } -impl StoragePagedListIterator +impl StoragePagedListIterator where Prefix: StorageInstance, Value: FullCodec, - ValuesPerPage: Get, + ValuesPerNewPage: Get, { /// Read self from the storage. pub fn from_meta( - meta: StoragePagedListMeta, + meta: StoragePagedListMeta, drain: bool, ) -> Self { let page = Page::::from_storage::(meta.first_page, meta.first_value_offset); @@ -264,12 +264,12 @@ where } } -impl Iterator - for StoragePagedListIterator +impl Iterator + for StoragePagedListIterator where Prefix: StorageInstance, Value: FullCodec, - ValuesPerPage: Get, + ValuesPerNewPage: Get, { type Item = Value; @@ -311,15 +311,15 @@ where } } -impl frame_support::storage::StorageList - for StoragePagedList +impl frame_support::storage::StorageList + for StoragePagedList where Prefix: StorageInstance, Value: FullCodec, - ValuesPerPage: Get, + ValuesPerNewPage: Get, { - type Iterator = StoragePagedListIterator; - type Appender = StoragePagedListMeta; + type Iterator = StoragePagedListIterator; + type Appender = StoragePagedListMeta; fn iter() -> Self::Iterator { StoragePagedListIterator::from_meta(Self::read_meta(), false) @@ -334,13 +334,13 @@ where } } -impl StoragePagedList +impl StoragePagedList where Prefix: StorageInstance, Value: FullCodec, - ValuesPerPage: Get, + ValuesPerNewPage: Get, { - fn read_meta() -> StoragePagedListMeta { + fn read_meta() -> StoragePagedListMeta { // Use default here to not require a setup migration. StoragePagedListMeta::from_storage().unwrap_or_default() } @@ -348,7 +348,7 @@ where /// Provides a fast append iterator. /// /// The list should not be modified while appending. Also don't call it recursively. - fn appender() -> StoragePagedListMeta { + fn appender() -> StoragePagedListMeta { Self::read_meta() } @@ -386,12 +386,12 @@ where } } -impl frame_support::storage::StoragePrefixedContainer - for StoragePagedList +impl frame_support::storage::StoragePrefixedContainer + for StoragePagedList where Prefix: StorageInstance, Value: FullCodec, - ValuesPerPage: Get, + ValuesPerNewPage: Get, { fn module_prefix() -> &'static [u8] { StoragePagedListPrefix::::module_prefix() @@ -416,7 +416,7 @@ pub(crate) mod mock { pub use sp_io::{hashing::twox_128, TestExternalities}; parameter_types! { - pub const ValuesPerPage: u32 = 5; + pub const ValuesPerNewPage: u32 = 5; pub const MaxPages: Option = Some(20); } @@ -428,7 +428,7 @@ pub(crate) mod mock { const STORAGE_PREFIX: &'static str = "foo"; } - pub type List = StoragePagedList; + pub type List = StoragePagedList; } #[cfg(test)] @@ -457,7 +457,7 @@ mod tests { // all gone assert_eq!(List::as_vec(), Vec::::new()); // Need to delete the metadata manually. - StoragePagedListMeta::::delete(); + StoragePagedListMeta::::delete(); }); } @@ -528,7 +528,7 @@ mod tests { assert_eq!(as_vec.len(), 4, "Second page contains 4"); let meta = sp_io::storage::get(&meta_key::()).expect("Meta should be present"); - let meta: StoragePagedListMeta = + let meta: StoragePagedListMeta = Decode::decode(&mut &meta[..]).unwrap(); assert_eq!(meta.first_page, 0); assert_eq!(meta.first_value_offset, 0); From 551ad22362b8154817418331a96b6cb7c16b5e09 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 19 Jul 2023 18:14:19 +0200 Subject: [PATCH 39/40] Update lockfile Signed-off-by: Oliver Tale-Yazdi --- Cargo.lock | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2b7096f2adcc7..0b8753b40eb5c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2145,13 +2145,39 @@ version = "0.3.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "fea41bba32d969b513997752735605054bc0dfa92b4c56bf1189f2e174be7a10" +[[package]] +name = "docify" +version = "0.1.16" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "af1b04e6ef3d21119d3eb7b032bca17f99fe041e9c072f30f32cc0e1a2b1f3c4" +dependencies = [ + "docify_macros 0.1.16", +] + [[package]] name = "docify" version = "0.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f6491709f76fb7ceb951244daf624d480198b427556084391d6e3c33d3ae74b9" dependencies = [ - "docify_macros", + "docify_macros 0.2.0", +] + +[[package]] +name = "docify_macros" +version = "0.1.16" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8b5610df7f2acf89a1bb5d1a66ae56b1c7fcdcfe3948856fb3ace3f644d70eb7" +dependencies = [ + "common-path", + "derive-syn-parse", + "lazy_static", + "proc-macro2", + "quote", + "regex", + "syn 2.0.18", + "termcolor", + "walkdir", ] [[package]] @@ -6654,7 +6680,7 @@ dependencies = [ name = "pallet-fast-unstake" version = "4.0.0-dev" dependencies = [ - "docify", + "docify 0.2.0", "frame-benchmarking", "frame-election-provider-support", "frame-support", @@ -7118,7 +7144,7 @@ dependencies = [ name = "pallet-paged-list" version = "0.1.0" dependencies = [ - "docify", + "docify 0.1.16", "frame-benchmarking", "frame-support", "frame-system", From 2cbaa8a00518784357607c0d67fd53b2f87be794 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 19 Jul 2023 18:33:32 +0200 Subject: [PATCH 40/40] Fix mock Signed-off-by: Oliver Tale-Yazdi --- frame/paged-list/src/mock.rs | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/frame/paged-list/src/mock.rs b/frame/paged-list/src/mock.rs index 5ae1d49dc2f41..390b4a8530dce 100644 --- a/frame/paged-list/src/mock.rs +++ b/frame/paged-list/src/mock.rs @@ -23,20 +23,15 @@ use crate::{paged_list::StoragePagedListMeta, Config, ListPrefix}; use frame_support::traits::{ConstU16, ConstU64}; use sp_core::H256; use sp_runtime::{ - testing::Header, traits::{BlakeTwo256, IdentityLookup}, + BuildStorage, }; -type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; type Block = frame_system::mocking::MockBlock; // Configure a mock runtime to test the pallet. frame_support::construct_runtime!( - pub enum Test where - Block = Block, - NodeBlock = Block, - UncheckedExtrinsic = UncheckedExtrinsic, - { + pub enum Test { System: frame_system, PagedList: crate, PagedList2: crate::, @@ -50,13 +45,12 @@ impl frame_system::Config for Test { type DbWeight = (); type RuntimeOrigin = RuntimeOrigin; type RuntimeCall = RuntimeCall; - type Index = u64; - type BlockNumber = u64; + type Nonce = u64; type Hash = H256; type Hashing = BlakeTwo256; type AccountId = u64; type Lookup = IdentityLookup; - type Header = Header; + type Block = Block; type RuntimeEvent = RuntimeEvent; type BlockHashCount = ConstU64<250>; type Version = (); @@ -90,7 +84,7 @@ pub type MetaOf = /// Build genesis storage according to the mock runtime. pub fn new_test_ext() -> sp_io::TestExternalities { - frame_system::GenesisConfig::default().build_storage::().unwrap().into() + frame_system::GenesisConfig::::default().build_storage().unwrap().into() } /// Run this closure in test externalities.