diff --git a/prdoc/pr_6520.prdoc b/prdoc/pr_6520.prdoc new file mode 100644 index 0000000000000..f1d68201d2fa8 --- /dev/null +++ b/prdoc/pr_6520.prdoc @@ -0,0 +1,10 @@ +title: Implements a mechanism to perserve the bags-list ID order + +doc: + - audience: Runtime Dev + description: | + Refactors the bags list pallet to support a mode which perserves the order of the elements in the list. + +crates: + - name: pallet-bags-list + bump: major diff --git a/substrate/frame/bags-list/src/lib.rs b/substrate/frame/bags-list/src/lib.rs index ee36a3a3ebd82..4470632dcda67 100644 --- a/substrate/frame/bags-list/src/lib.rs +++ b/substrate/frame/bags-list/src/lib.rs @@ -68,6 +68,15 @@ //! - iteration over the top* N items by score, where the precise ordering of items doesn't //! particularly matter. //! +//! ### Preserve order mode +//! +//! The bags list pallet supports a mode which preserves the order of the elements in the list. +//! While the implementor of [`Config::PreserveOrder`] returns `true`, no rebags or moves within a +//! bag are allowed. Which in practice means that: +//! - calling [`Pallet::rebag`], [`Pallet::put_in_front_of`] and [`Pallet::put_in_front_of_other`] +//! will return a [`Error::MustPreserveOrder`]; +//! - calling [`SortedListProvider::on_update`] is a noop. +//! //! ### Further Details //! //! - items are kept in bags, which are delineated by their range of score (See @@ -127,8 +136,12 @@ extern crate alloc; use alloc::boxed::Box; use codec::FullCodec; use frame_election_provider_support::{ScoreProvider, SortedListProvider}; +use frame_support::{ensure, traits::Get}; use frame_system::ensure_signed; -use sp_runtime::traits::{AtLeast32BitUnsigned, Bounded, StaticLookup}; +use sp_runtime::{ + traits::{AtLeast32BitUnsigned, Bounded, StaticLookup}, + DispatchError, +}; #[cfg(any(test, feature = "try-runtime", feature = "fuzz"))] use sp_runtime::TryRuntimeError; @@ -247,6 +260,9 @@ pub mod pallet { + TypeInfo + FullCodec + MaxEncodedLen; + + /// Something that signals whether the order of the element in the bags list may change. + type PreserveOrder: Get; } /// A single node, within some bag. @@ -277,6 +293,8 @@ pub mod pallet { pub enum Error { /// A error in the list interface implementation. List(ListError), + /// A request that does not preserve the list's order was requested out of time. + MustPreserveOrder, } impl From for Error { @@ -301,6 +319,8 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::rebag_non_terminal().max(T::WeightInfo::rebag_terminal()))] pub fn rebag(origin: OriginFor, dislocated: AccountIdLookupOf) -> DispatchResult { ensure_signed(origin)?; + ensure!(!T::PreserveOrder::get(), Error::::MustPreserveOrder); + let dislocated = T::Lookup::lookup(dislocated)?; let current_score = T::ScoreProvider::score(&dislocated); let _ = Pallet::::do_rebag(&dislocated, current_score) @@ -324,6 +344,8 @@ pub mod pallet { origin: OriginFor, lighter: AccountIdLookupOf, ) -> DispatchResult { + ensure!(!T::PreserveOrder::get(), Error::::MustPreserveOrder); + let heavier = ensure_signed(origin)?; let lighter = T::Lookup::lookup(lighter)?; List::::put_in_front_of(&lighter, &heavier) @@ -341,6 +363,8 @@ pub mod pallet { heavier: AccountIdLookupOf, lighter: AccountIdLookupOf, ) -> DispatchResult { + ensure!(!T::PreserveOrder::get(), Error::::MustPreserveOrder); + let _ = ensure_signed(origin)?; let lighter = T::Lookup::lookup(lighter)?; let heavier = T::Lookup::lookup(heavier)?; @@ -431,7 +455,13 @@ impl, I: 'static> SortedListProvider for Pallet } fn on_update(id: &T::AccountId, new_score: T::Score) -> Result<(), ListError> { - Pallet::::do_rebag(id, new_score).map(|_| ()) + if T::PreserveOrder::get() { + // lock is set, on_update is a noop. + ensure!(list::Node::::get(&id).is_some(), ListError::NodeNotFound); + Ok(()) + } else { + Pallet::::do_rebag(id, new_score).map(|_| ()) + } } fn on_remove(id: &T::AccountId) -> Result<(), ListError> { diff --git a/substrate/frame/bags-list/src/list/mod.rs b/substrate/frame/bags-list/src/list/mod.rs index 696b64d40e9b9..f982e4a5214e7 100644 --- a/substrate/frame/bags-list/src/list/mod.rs +++ b/substrate/frame/bags-list/src/list/mod.rs @@ -840,7 +840,7 @@ impl, I: 'static> Node { crate::ListNodes::::insert(self.id.clone(), self); } - /// Update neighboring nodes to point to reach other. + /// Update neighboring nodes to point to each other. /// /// Only updates storage for adjacent nodes, but not `self`; so the user may need to call /// `self.put`. diff --git a/substrate/frame/bags-list/src/mock.rs b/substrate/frame/bags-list/src/mock.rs index 3690a876f62d8..52b282261905c 100644 --- a/substrate/frame/bags-list/src/mock.rs +++ b/substrate/frame/bags-list/src/mock.rs @@ -56,6 +56,7 @@ impl frame_system::Config for Runtime { parameter_types! { pub static BagThresholds: &'static [VoteWeight] = &[10, 20, 30, 40, 50, 60, 1_000, 2_000, 10_000]; + pub static PreserveOrder: bool = false; } impl bags_list::Config for Runtime { @@ -64,6 +65,7 @@ impl bags_list::Config for Runtime { type BagThresholds = BagThresholds; type ScoreProvider = StakingMock; type Score = VoteWeight; + type PreserveOrder = PreserveOrder; } type Block = frame_system::mocking::MockBlock; diff --git a/substrate/frame/bags-list/src/tests.rs b/substrate/frame/bags-list/src/tests.rs index 0b382a4fcefa9..4181842c9c762 100644 --- a/substrate/frame/bags-list/src/tests.rs +++ b/substrate/frame/bags-list/src/tests.rs @@ -735,4 +735,67 @@ mod sorted_list_provider { assert!(non_existent_ids.iter().all(|id| !BagsList::contains(id))); }) } + + #[test] + fn on_update_with_preserve_order_works() { + ExtBuilder::default().build_and_execute(|| { + // initial iter state. + let initial_iter = ListNodes::::iter().map(|(a, _)| a).collect::>(); + assert_eq!(initial_iter, vec![3, 4, 1, 2]); + + assert_eq!(ListBags::::iter().count(), 2); + assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]); + + assert_eq!(BagsList::get_score(&1), Ok(10)); + + // sets preserve order. + PreserveOrder::set(true); + + // update 1's score in score provider and call on_update. + StakingMock::set_score_of(&1, 2_000); + assert_ok!(BagsList::on_update(&1, 2_000)); + + // preserve order is set, so the node's bag score and iter state remain the same, + // despite the updated score in the score provider for 1. + assert_eq!(BagsList::get_score(&1), Ok(10)); + assert_eq!( + initial_iter, + ListNodes::::iter().map(|(a, _)| a).collect::>() + ); + assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]); + + // unsets preserve order. + PreserveOrder::set(false); + assert_ok!(BagsList::rebag(RuntimeOrigin::signed(0), 1)); + + // now the rebag worked as expected. + assert_eq!(BagsList::get_score(&1), Ok(2_000)); + assert_eq!(List::::get_bags(), vec![(1_000, vec![2, 3, 4]), (2_000, vec![1])]); + }) + } + + #[test] + fn calls_with_preserve_order_fails() { + use crate::pallet::Error; + + ExtBuilder::default().build_and_execute(|| { + // sets preserve order. + PreserveOrder::set(true); + + assert_noop!( + BagsList::rebag(RuntimeOrigin::signed(0), 1), + Error::::MustPreserveOrder + ); + + assert_noop!( + BagsList::put_in_front_of(RuntimeOrigin::signed(0), 1), + Error::::MustPreserveOrder + ); + + assert_noop!( + BagsList::put_in_front_of_other(RuntimeOrigin::signed(0), 1, 2), + Error::::MustPreserveOrder + ); + }) + } } diff --git a/substrate/frame/nomination-pools/test-delegate-stake/src/mock.rs b/substrate/frame/nomination-pools/test-delegate-stake/src/mock.rs index d1bc4ef8ff281..10994bc8bc44a 100644 --- a/substrate/frame/nomination-pools/test-delegate-stake/src/mock.rs +++ b/substrate/frame/nomination-pools/test-delegate-stake/src/mock.rs @@ -117,6 +117,7 @@ impl pallet_bags_list::Config for Runtime { type BagThresholds = BagThresholds; type ScoreProvider = Staking; type Score = VoteWeight; + type PreserveOrder = Staking; } pub struct BalanceToU256; diff --git a/substrate/frame/nomination-pools/test-transfer-stake/src/mock.rs b/substrate/frame/nomination-pools/test-transfer-stake/src/mock.rs index d913c5fe6948c..fd1456ad35f62 100644 --- a/substrate/frame/nomination-pools/test-transfer-stake/src/mock.rs +++ b/substrate/frame/nomination-pools/test-transfer-stake/src/mock.rs @@ -109,6 +109,7 @@ impl pallet_bags_list::Config for Runtime { type BagThresholds = BagThresholds; type ScoreProvider = Staking; type Score = VoteWeight; + type PreserveOrder = Staking; } pub struct BalanceToU256; diff --git a/substrate/frame/staking/src/mock.rs b/substrate/frame/staking/src/mock.rs index df8cb38e8b371..3a754b3c88a16 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/substrate/frame/staking/src/mock.rs @@ -217,6 +217,7 @@ impl pallet_bags_list::Config for Test { type ScoreProvider = Staking; type BagThresholds = BagThresholds; type Score = VoteWeight; + type PreserveOrder = Staking; } pub struct OnChainSeqPhragmen; diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 2ae925d036435..82d84c8a6c030 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -1559,6 +1559,18 @@ where } } +/// Implements the lock to be used by the the sorted list providers implemented by the bags list +/// pallet. +/// +/// When it returns `true`, the bags list ID ordering should be disabled. +impl Get for Pallet { + fn get() -> bool { + // sorted list provider order perservation not enabled yet, see + // . + false + } +} + impl ScoreProvider for Pallet { type Score = VoteWeight; @@ -1640,6 +1652,7 @@ impl SortedListProvider for UseValidatorsMap { // nothing to do upon regenerate. 0 } + #[cfg(feature = "try-runtime")] fn try_state() -> Result<(), TryRuntimeError> { Ok(())