bags-list: implement pending rebag (#9725)

Fix the issue where registering a nominator or validator while the bags
list is locked would completely ignore the account, making it
unreachable for on_idle rebagging.

Changes:
- Add PendingRebag storage to queue accounts that fail insertion during
lock.
- Modify on_idle to process pending accounts with priority before
regular accounts.
- Add cleanup logic for non-staking pending accounts.

Related issue:
paritytech-secops/srlabs_findings#567

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Kian Paimani <[email protected]>
(cherry picked from commit 5c63e5e)

Author: 3 people

Cargo.lock

@@ -16,9 +16,9 @@
 //! Autogenerated weights for `pallet_bags_list`
 //!
 //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 32.0.0
-//! DATE: 2025-07-01, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]`
+//! DATE: 2025-09-12, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]`
 //! WORST CASE MAP SIZE: `1000000`
-//! HOSTNAME: `66f1737e2c94`, CPU: `Intel(R) Xeon(R) CPU @ 2.60GHz`
+//! HOSTNAME: `157b7e44410a`, CPU: `Intel(R) Xeon(R) CPU @ 2.60GHz`
 //! WASM-EXECUTION: `Compiled`, CHAIN: `None`, DB CACHE: 1024
 
 // Executed Command:
@@ -52,6 +52,8 @@ pub struct WeightInfo<T>(PhantomData<T>);
 impl<T: frame_system::Config> pallet_bags_list::WeightInfo for WeightInfo<T> {
 	/// Storage: `VoterList::Lock` (r:1 w:0)
 	/// Proof: `VoterList::Lock` (`max_values`: Some(1), `max_size`: Some(0), added: 495, mode: `MaxEncodedLen`)
+	/// Storage: `VoterList::PendingRebag` (r:1 w:0)
+	/// Proof: `VoterList::PendingRebag` (`max_values`: None, `max_size`: Some(48), added: 2523, mode: `MaxEncodedLen`)
 	/// Storage: `VoterList::ListNodes` (r:4 w:4)
 	/// Proof: `VoterList::ListNodes` (`max_values`: None, `max_size`: Some(154), added: 2629, mode: `MaxEncodedLen`)
 	/// Storage: `Staking::Bonded` (r:1 w:0)
@@ -64,14 +66,16 @@ impl<T: frame_system::Config> pallet_bags_list::WeightInfo for WeightInfo<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `5012`
 		//  Estimated: `11506`
-		// Minimum execution time: 149_768_000 picoseconds.
-		Weight::from_parts(162_271_000, 0)
+		// Minimum execution time: 154_457_000 picoseconds.
+		Weight::from_parts(161_546_000, 0)
 			.saturating_add(Weight::from_parts(0, 11506))
-			.saturating_add(T::DbWeight::get().reads(8))
+			.saturating_add(T::DbWeight::get().reads(9))
 			.saturating_add(T::DbWeight::get().writes(5))
 	}
 	/// Storage: `VoterList::Lock` (r:1 w:0)
 	/// Proof: `VoterList::Lock` (`max_values`: Some(1), `max_size`: Some(0), added: 495, mode: `MaxEncodedLen`)
+	/// Storage: `VoterList::PendingRebag` (r:1 w:0)
+	/// Proof: `VoterList::PendingRebag` (`max_values`: None, `max_size`: Some(48), added: 2523, mode: `MaxEncodedLen`)
 	/// Storage: `VoterList::ListNodes` (r:3 w:3)
 	/// Proof: `VoterList::ListNodes` (`max_values`: None, `max_size`: Some(154), added: 2629, mode: `MaxEncodedLen`)
 	/// Storage: `Staking::Bonded` (r:1 w:0)
@@ -84,10 +88,10 @@ impl<T: frame_system::Config> pallet_bags_list::WeightInfo for WeightInfo<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `4915`
 		//  Estimated: `8877`
-		// Minimum execution time: 145_767_000 picoseconds.
-		Weight::from_parts(157_284_000, 0)
+		// Minimum execution time: 155_177_000 picoseconds.
+		Weight::from_parts(159_976_000, 0)
 			.saturating_add(Weight::from_parts(0, 8877))
-			.saturating_add(T::DbWeight::get().reads(8))
+			.saturating_add(T::DbWeight::get().reads(9))
 			.saturating_add(T::DbWeight::get().writes(5))
 	}
 	/// Storage: `VoterList::Lock` (r:1 w:0)
@@ -106,34 +110,38 @@ impl<T: frame_system::Config> pallet_bags_list::WeightInfo for WeightInfo<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `7365`
 		//  Estimated: `11506`
-		// Minimum execution time: 192_992_000 picoseconds.
-		Weight::from_parts(200_226_000, 0)
+		// Minimum execution time: 190_977_000 picoseconds.
+		Weight::from_parts(206_642_000, 0)
 			.saturating_add(Weight::from_parts(0, 11506))
 			.saturating_add(T::DbWeight::get().reads(11))
 			.saturating_add(T::DbWeight::get().writes(6))
 	}
-	/// Storage: `VoterList::CounterForListNodes` (r:1 w:0)
+	/// Storage: `VoterList::CounterForListNodes` (r:1 w:1)
 	/// Proof: `VoterList::CounterForListNodes` (`max_values`: Some(1), `max_size`: Some(4), added: 499, mode: `MaxEncodedLen`)
+	/// Storage: `VoterList::CounterForPendingRebag` (r:1 w:1)
+	/// Proof: `VoterList::CounterForPendingRebag` (`max_values`: Some(1), `max_size`: Some(4), added: 499, mode: `MaxEncodedLen`)
 	/// Storage: `VoterList::Lock` (r:1 w:0)
 	/// Proof: `VoterList::Lock` (`max_values`: Some(1), `max_size`: Some(0), added: 495, mode: `MaxEncodedLen`)
 	/// Storage: `VoterList::NextNodeAutoRebagged` (r:1 w:1)
 	/// Proof: `VoterList::NextNodeAutoRebagged` (`max_values`: Some(1), `max_size`: Some(32), added: 527, mode: `MaxEncodedLen`)
-	/// Storage: `VoterList::ListBags` (r:200 w:4)
+	/// Storage: `VoterList::PendingRebag` (r:12 w:3)
+	/// Proof: `VoterList::PendingRebag` (`max_values`: None, `max_size`: Some(48), added: 2523, mode: `MaxEncodedLen`)
+	/// Storage: `VoterList::ListBags` (r:199 w:3)
 	/// Proof: `VoterList::ListBags` (`max_values`: None, `max_size`: Some(82), added: 2557, mode: `MaxEncodedLen`)
-	/// Storage: `VoterList::ListNodes` (r:11 w:11)
+	/// Storage: `VoterList::ListNodes` (r:11 w:10)
 	/// Proof: `VoterList::ListNodes` (`max_values`: None, `max_size`: Some(154), added: 2629, mode: `MaxEncodedLen`)
 	/// Storage: `Staking::Bonded` (r:10 w:0)
 	/// Proof: `Staking::Bonded` (`max_values`: None, `max_size`: Some(72), added: 2547, mode: `MaxEncodedLen`)
-	/// Storage: `Staking::Ledger` (r:10 w:0)
+	/// Storage: `Staking::Ledger` (r:9 w:0)
 	/// Proof: `Staking::Ledger` (`max_values`: None, `max_size`: Some(753), added: 3228, mode: `MaxEncodedLen`)
 	fn on_idle() -> Weight {
 		// Proof Size summary in bytes:
-		//  Measured:  `24300`
-		//  Estimated: `512390`
-		// Minimum execution time: 1_064_149_000 picoseconds.
-		Weight::from_parts(1_090_878_000, 0)
-			.saturating_add(Weight::from_parts(0, 512390))
-			.saturating_add(T::DbWeight::get().reads(234))
-			.saturating_add(T::DbWeight::get().writes(16))
+		//  Measured:  `23853`
+		//  Estimated: `509833`
+		// Minimum execution time: 1_173_907_000 picoseconds.
+		Weight::from_parts(1_196_347_000, 0)
+			.saturating_add(Weight::from_parts(0, 509833))
+			.saturating_add(T::DbWeight::get().reads(245))
+			.saturating_add(T::DbWeight::get().writes(19))
 	}
 }

cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_bags_list.rs

@@ -0,0 +1,15 @@
+title: 'bags-list: implement pending rebag'
+doc:
+- audience: Runtime Dev
+  description: |-
+    Fix the issue where registering a nominator or validator while the bags list is locked would completely ignore the account, making it unreachable for on_idle rebagging.
+
+    Changes:
+    - Add PendingRebag storage to queue accounts that fail insertion during lock.
+    - Modify on_idle to process pending accounts with priority before regular accounts.
+    - Add cleanup logic for non-staking pending accounts.
+crates:
+- name: pallet-bags-list
+  bump: minor
+- name: asset-hub-westend-runtime
+  bump: patch

prdoc/pr_9725.prdoc

@@ -40,6 +40,7 @@ pallet-balances = { default-features = true, workspace = true }
 sp-core = { default-features = true, workspace = true }
 sp-io = { default-features = true, workspace = true }
 sp-tracing = { default-features = true, workspace = true }
+substrate-test-utils = { workspace = true }
 
 [features]
 default = ["std"]

substrate/frame/bags-list/Cargo.toml

@@ -352,7 +352,8 @@ benchmarks_instance_pallet! {
 		// This benchmark generates weights for `on_idle` based on runtime configuration.
 		// The main input is the runtime's `MaxAutoRebagPerBlock` type, which defines how many
 		// nodes can be rebagged per block.
-		// This benchmark simulates a more realistic and fragmented rebag scenario.
+		// This benchmark simulates a scenario with both pending rebag processing
+		// and fragmented rebag scenario.
 
 		List::<T, _>::unsafe_clear();
 
@@ -362,11 +363,14 @@ benchmarks_instance_pallet! {
 		let high = bag_thresh[2];
 
 		let rebag_budget = <T as Config<I>>::MaxAutoRebagPerBlock::get();
-		let n = rebag_budget + 5;
 
-		// Insert nodes with varying scores
-		for i in 0..n {
-			let node: T::AccountId = account("node", i, 0);
+		// Adjust counts to ensure exact budget usage
+		let pending_count = rebag_budget / 3; // Smaller portion for pending
+		let regular_count = rebag_budget + 5;
+
+		// Insert regular nodes with varying scores
+		for i in 0..regular_count {
+			let node: T::AccountId = account("regular_node", i, 0);
 			let score = match i % 3 {
 				0 => low - One::one(),
 				1 => mid - One::one(),
@@ -376,14 +380,38 @@ benchmarks_instance_pallet! {
 		}
 
 		// Corrupt some nodes to simulate edge cases
-		for i in (0..n).step_by(4) {
-			let node: T::AccountId = account("node", i, 0);
+		for i in (0..regular_count).step_by(4) {
+			let node: T::AccountId = account("regular_node", i, 0);
 			let _ = List::<T, _>::remove(&node); // orphan nodes
 		}
 
+		// Lock the list and simulate pending rebag insertions
+		<Pallet<T, I>>::lock();
+
+		// Create pending rebag entries (mix of valid and corrupted)
+		for i in 0..pending_count {
+			let pending_node: T::AccountId = account("pending_node", i, 0);
+			let pending_score = match i % 3 {
+				0 => mid,
+				1 => high,
+				_ => high + high,
+			};
+
+			// Set score first for most nodes, but skip some to simulate cleanup scenarios
+			if i % 7 != 0 {
+				T::ScoreProvider::set_score_of(&pending_node, pending_score);
+			}
+
+			let _ = <Pallet<T, I> as SortedListProvider<T::AccountId>>::on_insert(
+				pending_node, pending_score
+			);
+		}
+
+		<Pallet<T, I>>::unlock();
+
 		// Now set new scores that will move nodes into higher bags
-		for i in 0..n {
-			let node: T::AccountId = account("node", i, 0);
+		for i in 0..regular_count {
+			let node: T::AccountId = account("regular_node", i, 0);
 			let new_score = match i % 3 {
 				0 => mid,
 				1 => high,
@@ -392,6 +420,12 @@ benchmarks_instance_pallet! {
 			T::ScoreProvider::set_score_of(&node, new_score);
 		}
 
+		assert_eq!(
+			PendingRebag::<T, I>::count(),
+			pending_count,
+			"Expected exactly {} pending rebag entries",
+			pending_count
+		);
 		// Ensure we have at least three bags populated before rebag
 		assert!(List::<T, _>::get_bags().len() >= 2);
 	}
@@ -400,6 +434,11 @@ benchmarks_instance_pallet! {
 		<Pallet<T, I> as Hooks<_>>::on_idle(Default::default(), Weight::MAX);
 	}
 	verify {
+		// Verify all pending rebag entries were processed.
+		// This should always be true since pending_count = rebag_budget / 3 < rebag_budget,
+		// and pending accounts are processed first so all pending entries fit within the budget.
+		assert_eq!(PendingRebag::<T, I>::count(), 0, "All pending rebag entries should be processed");
+
 		// Count how many nodes ended up in higher bags
 		let total_rebagged: usize = List::<T, _>::get_bags()
 			.iter()
@@ -408,7 +447,7 @@ benchmarks_instance_pallet! {
 			.sum();
 
 		let expected = <T as Config<I>>::MaxAutoRebagPerBlock::get() as usize;
-		assert_eq!(total_rebagged, expected,"Expected exactly{:?} rebagged nodes, found {:?}", expected, total_rebagged);
+		assert_eq!(total_rebagged, expected, "Expected exactly {:?} rebagged nodes, found {:?}", expected, total_rebagged);
 	}
 
 	impl_benchmark_test_suite!(