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

pallet Collective: fix genesis member sort order #13988

Merged
merged 3 commits into from
Apr 27, 2023

Conversation

liamaharon
Copy link
Contributor

@liamaharon liamaharon commented Apr 24, 2023

The Collective pallet currently does not check that genesis members are sorted or sort them before setting them as expected by the pallet:

/// The current members of the collective. This is stored sorted (just by value).
#[pallet::storage]
#[pallet::getter(fn members)]
pub type Members<T: Config<I>, I: 'static = ()> =
StorageValue<_, Vec<T::AccountId>, ValueQuery>;

kitchen-sink-runtime invariants are currently broken due to this bug.

This PR adds sorting logic into the initialize_members, ensuring genesis members are inserted in sorted order.

Props to @Szegoo for adding the Collective try-state hooks (#13645) that caught this!


Open question: what's the likelihood there're other pallets that suffer from a similar bug? Should an issue need to be opened to investigate that?

@liamaharon liamaharon added A0-please_review Pull request needs code review. I3-bug The node fails to follow expected behavior. C3-medium PR touches the given topic and has a medium impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit B1-note_worthy Changes should be noted in the release notes labels Apr 24, 2023
@liamaharon liamaharon requested review from kianenigma and ggwpez April 24, 2023 12:43
@liamaharon liamaharon added T1-runtime This PR/Issue is related to the topic “runtime”. and removed I3-bug The node fails to follow expected behavior. labels Apr 24, 2023
@liamaharon liamaharon changed the title Fix Collective genesis member sort order pallet Collective: fix genesis member sort order Apr 25, 2023
frame/collective/src/lib.rs Show resolved Hide resolved
@ggwpez
Copy link
Member

ggwpez commented Apr 25, 2023

Open question: what's the likelihood there're other pallets that suffer from a similar bug? Should an issue need to be opened to investigate that?

Adding more and more try-state hooks should be enough IMHO.

@liamaharon liamaharon merged commit 587959e into master Apr 27, 2023
@liamaharon liamaharon deleted the liam-collective-sort-initial-members branch April 27, 2023 08:55
gpestana pushed a commit that referenced this pull request May 4, 2023
#13988)

* insert members in sorted order

* improve variable name

* enforce genesis members length constraint
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
paritytech#13988)

* insert members in sorted order

* improve variable name

* enforce genesis members length constraint
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 C3-medium PR touches the given topic and has a medium impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants