diff --git a/prdoc/pr_6520.prdoc b/prdoc/pr_6520.prdoc new file mode 100644 index 000000000000..f1d68201d2fa --- /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 ee36a3a3ebd8..4470632dcda6 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 696b64d40e9b..f982e4a5214e 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 3690a876f62d..52b282261905 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 0b382a4fcefa..4181842c9c76 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 d1bc4ef8ff28..10994bc8bc44 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 d913c5fe6948..fd1456ad35f6 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 08b4f66b86be..c65fca783255 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/substrate/frame/staking/src/mock.rs @@ -220,6 +220,7 @@ impl pallet_bags_list::Config for Test { type ScoreProvider = Staking; type BagThresholds = BagThresholds; type Score = VoteWeight; + type PreserveOrder = Staking; } // multi-page types and controller. diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 785c688453dd..d7c9204660c2 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -1033,9 +1033,15 @@ impl Pallet { let mut sorted_voters = match VoterSnapshotStatus::::get() { // start the snapshot procssing from the beginning. SnapshotStatus::Waiting => T::VoterList::iter(), - // snapshot continues, start from the last iterated voter in the list. - SnapshotStatus::Ongoing(account_id) => T::VoterList::iter_from(&account_id) - .defensive_unwrap_or(Box::new(vec![].into_iter())), + // snapshot continues. + SnapshotStatus::Ongoing(account_id) => { + let iter = T::VoterList::iter_from(&account_id) + .defensive_unwrap_or(Box::new(vec![].into_iter())); + + // add cursor account to the beginning of the iter, as it hasn't been processed + // yet. + Box::new(vec![account_id].into_iter().chain(iter)) + }, // all voters have been consumed already, return an empty iterator. SnapshotStatus::Consumed => Box::new(vec![].into_iter()), }; @@ -1112,24 +1118,21 @@ impl Pallet { } } - // update the voter snapshot status. + // update cursor if there are remaining accounts in the voter list. The cursor must be the + // next *non-processed* voter if it exists. VoterSnapshotStatus::::mutate(|status| { match (page, status.clone()) { // last page, reset status for next round. (0, _) => *status = SnapshotStatus::Waiting, + // progress, update cursor. (_, SnapshotStatus::Waiting) | (_, SnapshotStatus::Ongoing(_)) => { - let maybe_last = all_voters.last().map(|(x, _, _)| x).cloned(); - - if let Some(ref last) = maybe_last { - if maybe_last == T::VoterList::iter().last() { - // all voters in the voter list have been consumed. - *status = SnapshotStatus::Consumed; - } else { - *status = SnapshotStatus::Ongoing(last.clone()); - } + if let Some(cursor) = sorted_voters.next() { + // cursor is the next non-processed element in the iter. + *status = SnapshotStatus::Ongoing(cursor); } else { - debug_assert!(*status == SnapshotStatus::Consumed); + // all voters in the list consumed. + *status = SnapshotStatus::Consumed; } }, // do nothing. @@ -1236,6 +1239,8 @@ impl Pallet { /// `Nominators` or `VoterList` outside of this function is almost certainly /// wrong. pub fn do_remove_nominator(who: &T::AccountId) -> bool { + Self::ensure_voter_cursor(who); + let outcome = if Nominators::::contains_key(who) { Nominators::::remove(who); let _ = T::VoterList::on_remove(who).defensive(); @@ -1248,6 +1253,7 @@ impl Pallet { Nominators::::count() + Validators::::count(), T::VoterList::count() ); + debug_assert!(VoterSnapshotStatus::::get() != SnapshotStatus::Ongoing(who.clone())); outcome } @@ -1281,8 +1287,11 @@ impl Pallet { /// `Validators` or `VoterList` outside of this function is almost certainly /// wrong. pub fn do_remove_validator(who: &T::AccountId) -> bool { + Self::ensure_voter_cursor(who); + let outcome = if Validators::::contains_key(who) { Validators::::remove(who); + let _ = T::VoterList::on_remove(who).defensive(); true } else { @@ -1293,6 +1302,7 @@ impl Pallet { Nominators::::count() + Validators::::count(), T::VoterList::count() ); + debug_assert!(VoterSnapshotStatus::::get() != SnapshotStatus::Ongoing(who.clone())); outcome } @@ -1727,6 +1737,20 @@ where } } +/// Implements the lock to be used by the the sorted list providers implemented by the bags list +/// pallet. +/// +/// If the voter snapshot creation is in progress return `true`, in which case the bags list ID +/// re-ordering should be disabled. +impl Get for Pallet { + fn get() -> bool { + match VoterSnapshotStatus::::get() { + SnapshotStatus::Ongoing(_) => true, + _ => false, + } + } +} + impl ScoreProvider for Pallet { type Score = VoteWeight; @@ -1808,6 +1832,7 @@ impl SortedListProvider for UseValidatorsMap { // nothing to do upon regenerate. 0 } + #[cfg(feature = "try-runtime")] fn try_state() -> Result<(), TryRuntimeError> { Ok(()) diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index 31d25be0bd6b..cc798b067f81 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -917,11 +917,17 @@ pub mod pallet { mode: Forcing, }, /// Report of a controller batch deprecation. - ControllerBatchDeprecated { failures: u32 }, + ControllerBatchDeprecated { + failures: u32, + }, /// Validator has been disabled. - ValidatorDisabled { stash: T::AccountId }, + ValidatorDisabled { + stash: T::AccountId, + }, /// Validator has been re-enabled. - ValidatorReenabled { stash: T::AccountId }, + ValidatorReenabled { + stash: T::AccountId, + }, } #[pallet::error] @@ -1233,6 +1239,21 @@ pub mod pallet { pub fn current_planned_session() -> SessionIndex { CurrentPlannedSession::::get() } + + /// Ensures the snapshot voter cursor is *not* the same as `who`. Updates to next in the + /// voter list interator if required. + pub(crate) fn ensure_voter_cursor(who: &T::AccountId) { + if VoterSnapshotStatus::::get() == SnapshotStatus::Ongoing(who.clone()) { + let next = + T::VoterList::iter_from(who).unwrap_or(Box::new(vec![].into_iter())).next(); + + match next { + Some(new_cursor) => + VoterSnapshotStatus::::set(SnapshotStatus::Ongoing(new_cursor)), + None => VoterSnapshotStatus::::set(SnapshotStatus::Consumed), + } + } + } } #[pallet::call] diff --git a/substrate/frame/staking/src/tests_paged_election.rs b/substrate/frame/staking/src/tests_paged_election.rs index 2610b93916cf..9f1e85a71710 100644 --- a/substrate/frame/staking/src/tests_paged_election.rs +++ b/substrate/frame/staking/src/tests_paged_election.rs @@ -673,6 +673,176 @@ mod paged_snapshot { assert_eq!(all_voters, single_page_voters); }) } + + #[test] + fn voter_snapshot_updates_validator_cursor_works() { + ExtBuilder::default() + .nominate(true) + .set_status(51, StakerStatus::Validator) + .set_status(41, StakerStatus::Nominator(vec![51])) + .set_status(101, StakerStatus::Validator) + .multi_page_election_provider(3) + .build_and_execute(|| { + assert_eq!(Pages::get(), 3); + // voter snapshot status is waiting. + assert_eq!(VoterSnapshotStatus::::get(), SnapshotStatus::Waiting); + // and voter list is not "locked". + assert!(!>::get()); + + assert_eq!( + ::VoterList::iter().collect::>(), + vec![11, 21, 31, 41, 51, 101] + ); + + // 2 voters per snapshot page. + let bounds = ElectionBoundsBuilder::default().voters_count(2.into()).build().voters; + + let page_2 = Staking::get_npos_voters(bounds, 2); + assert_eq!(page_2.len(), 2); + let last_in_page = + page_2.iter().map(|(a, _, _)| a).cloned().collect::>().pop().unwrap(); + + // cursor was updated as expected (i.e. next in the iter that was not processed) + let expected_cursor = + ::VoterList::iter_from(&last_in_page).unwrap().next().unwrap(); + assert_eq!( + VoterSnapshotStatus::::get(), + SnapshotStatus::Ongoing(expected_cursor) + ); + // and now the sorted list provider is "locked". + assert!(>::get()); + + // cursor is validator. + assert_eq!(Staking::status(&expected_cursor), Ok(StakerStatus::Validator)); + + // after chilling/removing the cursor from the voter list, the cursor will be + // update with the next non-processed voter in the list. + let expected_cursor_after_chill = + ::VoterList::iter_from(&expected_cursor) + .unwrap() + .next() + .unwrap(); + + let chilled = expected_cursor; + + // now we chill the voter that is currently the cursor. + assert_ok!(Staking::chill(RuntimeOrigin::signed(chilled))); + // cursor was updated as expected. + assert_eq!( + VoterSnapshotStatus::::get(), + SnapshotStatus::Ongoing(expected_cursor_after_chill) + ); + + // fetch and check all remaining pages. + let page_2 = page_2.into_iter().map(|(a, _, _)| a).collect::>(); + assert_eq!(page_2, vec![11, 21]); + let page_1 = Staking::get_npos_voters(bounds, 1) + .into_iter() + .map(|(a, _, _)| a) + .collect::>(); + assert_eq!(page_1, vec![41, 51]); + // page 0. + // sorted list provider remains "locked" until fetching page 0. + assert!(>::get()); + let page_0 = Staking::get_npos_voters(bounds, 0) + .into_iter() + .map(|(a, _, _)| a) + .collect::>(); + assert_eq!(page_0, vec![101]); + // unlocked now. + assert!(!>::get()); + + // cursor was reset for next round after fetching page 0. + assert_eq!(VoterSnapshotStatus::::get(), SnapshotStatus::Waiting); + + // note that the chilled account was not included in any of the pages. + assert!(!page_2.contains(&chilled)); + assert!(!page_1.contains(&chilled)); + assert!(!page_0.contains(&chilled)); + }) + } + + #[test] + fn voter_snapshot_updates_nominator_cursor_works() { + ExtBuilder::default() + .nominate(true) + .set_status(51, StakerStatus::Validator) + .set_status(41, StakerStatus::Nominator(vec![51])) + .set_status(101, StakerStatus::Validator) + .multi_page_election_provider(3) + .build_and_execute(|| { + // voter snapshot status is waiting. + assert_eq!(VoterSnapshotStatus::::get(), SnapshotStatus::Waiting); + assert_eq!(Pages::get(), 3); + + assert_eq!( + ::VoterList::iter().collect::>(), + vec![11, 21, 31, 41, 51, 101] + ); + + // 3 voters per snapshot page. + let bounds = ElectionBoundsBuilder::default().voters_count(3.into()).build().voters; + + let page_2 = Staking::get_npos_voters(bounds, 2); + assert_eq!(page_2.len(), 3); + let last_in_page = + page_2.iter().map(|(a, _, _)| a).cloned().collect::>().pop().unwrap(); + + // cursor was updated as expected (i.e. next in the iter that was not processed) + let expected_cursor = + ::VoterList::iter_from(&last_in_page).unwrap().next().unwrap(); + assert_eq!( + VoterSnapshotStatus::::get(), + SnapshotStatus::Ongoing(expected_cursor) + ); + // cursor is a nominator. + assert_eq!( + Staking::status(&expected_cursor), + Ok(StakerStatus::Nominator(vec![51])) + ); + + // after chilling/removing the cursor from the voter list, the cursor will be + // update with the next non-processed voter in the list. + let expected_cursor_after_chill = + ::VoterList::iter_from(&expected_cursor) + .unwrap() + .next() + .unwrap(); + + let chilled = expected_cursor; + + // now we chill the voter that is currently the cursor. + assert_ok!(Staking::chill(RuntimeOrigin::signed(chilled))); + // cursor was updated as expected. + assert_eq!( + VoterSnapshotStatus::::get(), + SnapshotStatus::Ongoing(expected_cursor_after_chill) + ); + + // fetch and check all remaining pages. + let page_2 = page_2.into_iter().map(|(a, _, _)| a).collect::>(); + assert_eq!(page_2, vec![11, 21, 31]); + let page_1 = Staking::get_npos_voters(bounds, 1) + .into_iter() + .map(|(a, _, _)| a) + .collect::>(); + assert_eq!(page_1, vec![51, 101]); + // page 0. + let page_0 = Staking::get_npos_voters(bounds, 0) + .into_iter() + .map(|(a, _, _)| a) + .collect::>(); + assert!(page_0.is_empty()); + + // cursor was reset for next round after fetching page 0. + assert_eq!(VoterSnapshotStatus::::get(), SnapshotStatus::Waiting); + + // note that the chilled account was not included in any of the pages. + assert!(!page_2.contains(&chilled)); + assert!(!page_1.contains(&chilled)); + assert!(!page_0.contains(&chilled)); + }) + } } mod paged_exposures {