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

Introduce Pallet paged-list #14120

Merged
merged 44 commits into from
Jul 19, 2023
Merged

Introduce Pallet paged-list #14120

merged 44 commits into from
Jul 19, 2023

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented May 10, 2023

Abstract

Introduces a pallet to provide a linked list data structure: pallet-paged-list. The elements are aggregated into pages to keep a low PoV footprint on reading. The main advantage is a smaller memory footprint within the runtime when a transaction errors and causes a storage rollback.

(Detailed implementation documentation is in the Rust Code)

API

The external API is defined by trait StoragePagedList:

pub trait StoragePagedList<V: FullCodec> {
	/// Iterator type to loop over all elements in FIFO order.
	type Iterator: Iterator<Item = V>;
	/// Fast append iterator to append multiple elements.
	type Appender: StorageAppender<V>;

	/// List all elements in append (FIFO) order.
	fn iter() -> Self::Iterator;

	/// Drain the elements.
	fn drain() -> Self::Iterator;

	/// A fast append iterator.
	fn appender() -> Self::Appender;

	/// Append a single element.
	///
	/// Should not be called repeatedly; use `append_many` instead.
	fn append_one<EncodeLikeValue>(item: EncodeLikeValue)
	where
		EncodeLikeValue: EncodeLike<V>,
	{
		Self::append_many(core::iter::once(item));
	}

	/// Append many elements.
	///
	/// Should not be called repeatedly; use `appender` instead.
	fn append_many<EncodeLikeValue, I>(items: I)
	where
		EncodeLikeValue: EncodeLike<V>,
		I: IntoIterator<Item = EncodeLikeValue>,
	{
		let mut ap = Self::appender();
		ap.append_many(items);
	}
}

And specifically trait StorageAppender to conveniently append values:

pub trait StorageAppender<V: FullCodec> {
	fn append<EncodeLikeValue>(&mut self, item: EncodeLikeValue)
	where
		EncodeLikeValue: EncodeLike<V>;

	// Note: this has a default impl, as an `Appender` is already assumed to be fast for appending.
	fn append_many<EncodeLikeValue, I>(&mut self, items: I)
	where
		EncodeLikeValue: EncodeLike<V>,
		I: IntoIterator<Item = EncodeLikeValue>,
	{
		for item in items.into_iter() {
			self.append(item);
		}
	}
}

Testing

Fuzzer tests are in place to test random sequences of appending and removal of elements. The fuzzer tests are currently not running in the CI.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Koute <koute@users.noreply.github.com>

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez marked this pull request as ready for review May 11, 2023 17:15
@ggwpez ggwpez requested a review from a team May 11, 2023 17:15
@ggwpez ggwpez added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels May 11, 2023
@ggwpez ggwpez changed the title Prototype StoragePagedList Introduce StoragePagedList May 11, 2023
@ggwpez ggwpez added B1-note_worthy Changes should be noted in the release notes T1-runtime This PR/Issue is related to the topic “runtime”. and removed B0-silent Changes should not be mentioned in any release notes labels May 11, 2023
ggwpez added 10 commits May 11, 2023 19:54
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez requested a review from gavofyork May 13, 2023 13:51
ggwpez and others added 6 commits May 31, 2023 19:11
Co-authored-by: Koute <koute@users.noreply.github.com>
Co-authored-by: Koute <koute@users.noreply.github.com>

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

In terms of organization, this is very similar to what pallet-bags-list is providing.

Perhaps we can put their associated traits in frame-support::collectio ns or something similar, with a more holistic documentation, explaining what each does and when they should each be used. If not done here, would be great if you make an issue about it and assign me.

I recall the queue in message-queue was also something that I imagined could be extracted as one of these "data structure as a pallet" thingies.

@louismerlin louismerlin added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jul 19, 2023
@ggwpez
Copy link
Member Author

ggwpez commented Jul 19, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@ggwpez
Copy link
Member Author

ggwpez commented Jul 19, 2023

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@ggwpez ggwpez changed the title Introduce pallet-paged-list Introduce Pallet paged-list Jul 19, 2023
@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Statuses failed for dbceffa

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Copy link
Member Author

ggwpez commented Jul 19, 2023

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Statuses failed for 551ad22

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Copy link
Member Author

ggwpez commented Jul 19, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 2ca6619 into master Jul 19, 2023
4 checks passed
@paritytech-processbot paritytech-processbot bot deleted the oty-paginated-list branch July 19, 2023 20:14

/// 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<T, I> = StoragePagedList<
Copy link
Contributor

@xlc xlc Jul 19, 2023

Choose a reason for hiding this comment

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

This pallet only holds a single storage. It should be possible to make the PagedList struct to be generic to take storage as generic parameter. So that the storage can be defined in other pallets and have no need to have an actual paged list pallet here. This can help reduce the complexity (all related storages are defined in a single pallet) and reduce numbers of pallets, which we have a hard cap of 255.

This is an example https://github.com/open-web3-stack/open-runtime-module-library/blob/58146e4a227c4cc4c49669a749dc04ee54d8f009/utilities/src/linked_item.rs

Copy link
Member

Choose a reason for hiding this comment

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

Yes, why isn't this just another storage primitive?

Copy link
Member Author

@ggwpez ggwpez Jul 21, 2023

Choose a reason for hiding this comment

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

Yes the pallet could provide multiple lists per instance. We can still add that. Currently it was designed in the simplest way possible to allow for a fix to be deployed in a timely manner.

It could also have been done as FRAME primitive, but then it needs support in the metadata.

Copy link
Member

Choose a reason for hiding this comment

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

But now it is also not supported in metadata :P

Ank4n pushed a commit that referenced this pull request Jul 22, 2023
* Prototype StoragePagedList

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add drain

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Remove stale docs

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add fuzzer tests

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Update

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Review

Co-authored-by: Koute <koute@users.noreply.github.com>

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* fmt

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Docs and clippy

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Sum docs

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Cleanup

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Undo WIP

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add pallet-paged-list

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Move code to pallet

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Move fuzzer

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Cleanup

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* fmt

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* docs

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Rename Appendix -> Appender

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Rename clear -> delete

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Feature gate testing stuff

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Docs review

Co-authored-by: Koute <koute@users.noreply.github.com>

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Cleanup

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* doc review

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Review renames

Co-authored-by: Koute <koute@users.noreply.github.com>

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add docs

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Fix fuzzer

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Docs + examples

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Remove hasher

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Remove empty Event and Call

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Remove MaxPages

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Fix docs

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Test eager page removal

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Cleanup

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Update frame/paged-list/src/paged_list.rs

Co-authored-by: Koute <koute@users.noreply.github.com>

* Fix docs

Co-authored-by: Koute <koute@users.noreply.github.com>

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Remove as_*_vec

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Update versions

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Rename ValuesPerPage -> ValuesPerNewPage

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Update lockfile

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Fix mock

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Koute <koute@users.noreply.github.com>
Co-authored-by: parity-processbot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants