Clamp benchmark ranks to respect MaxRank (#7720)

resolves #7517 

### Issues

#### 1. Compile-Time vs Runtime Rank Mismatch
- **`promote_fast` Benchmark** uses mock runtime's `MaxRank` during
benchmark generation while allowing runtime overrides. Creating
potential parameter mismatch between benchmark metadata(i.e. generated
`r` values) and runtime configuration(i.e. `T::MaxRank`).


#### 2. Static Rank Assumptions
- **`bump_demote` Benchmark** initialized members at rank 2, making it
incompatible with `MaxRank=1` configurations
- **`promote` Benchmark** contained hardcoded rank values (1 → 2) which
fails for `MaxRank=1`


## Changes

#### Dynamic Rank Clamping
```rust
// promote_fast
// Get target rank for promotion.
let max_rank = T::MaxRank::get();
let target_rank = (r as u16).min(max_rank);

// promote
// Set `to_rank` dynamically based on `max_rank`.
let to_rank = (current_rank + 1).min(max_rank);

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Author: 3 people

cumulus/parachains/runtimes/collectives/collectives-westend/src/ambassador/mod.rs

@@ -223,7 +223,7 @@ impl pallet_core_fellowship::Config<AmbassadorCoreInstance> for Runtime {
 	type PromoteOrigin = PromoteOrigin;
 	type FastPromoteOrigin = Self::PromoteOrigin;
 	type EvidenceSize = ConstU32<65536>;
-	type MaxRank = ConstU32<9>;
+	type MaxRank = ConstU16<9>;
 }
 
 pub type AmbassadorSalaryInstance = pallet_salary::Instance2;

cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs

@@ -211,7 +211,7 @@ impl pallet_core_fellowship::Config<FellowshipCoreInstance> for Runtime {
 	>;
 	type FastPromoteOrigin = Self::PromoteOrigin;
 	type EvidenceSize = ConstU32<65536>;
-	type MaxRank = ConstU32<9>;
+	type MaxRank = ConstU16<9>;
 }
 
 pub type FellowshipSalaryInstance = pallet_salary::Instance1;

cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_core_fellowship_ambassador_core.rs

@@ -193,7 +193,7 @@ impl<T: frame_system::Config> pallet_core_fellowship::WeightInfo for WeightInfo<
 	/// Proof: `AmbassadorCollective::IdToIndex` (`max_values`: None, `max_size`: Some(54), added: 2529, mode: `MaxEncodedLen`)
 	/// The range of component `r` is `[1, 9]`.
 	/// The range of component `r` is `[1, 9]`.
-	fn promote_fast(r: u32, ) -> Weight {
+	fn promote_fast(r: u16, ) -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `65968`
 		//  Estimated: `69046 + r * (2489 ±0)`

cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_core_fellowship_fellowship_core.rs

@@ -193,7 +193,7 @@ impl<T: frame_system::Config> pallet_core_fellowship::WeightInfo for WeightInfo<
 	/// Proof: `FellowshipCollective::IdToIndex` (`max_values`: None, `max_size`: Some(54), added: 2529, mode: `MaxEncodedLen`)
 	/// The range of component `r` is `[1, 9]`.
 	/// The range of component `r` is `[1, 9]`.
-	fn promote_fast(r: u32, ) -> Weight {
+	fn promote_fast(r: u16, ) -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `65996`
 		//  Estimated: `69046 + r * (2489 ±0)`

prdoc/pr_7720.prdoc

@@ -0,0 +1,25 @@
+# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
+# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
+
+title: Clamp Core Fellowship Benchmarks to Runtime MaxRank Configuration
+
+doc:
+  - audience: Runtime Dev
+    description: |      
+
+      This change modifies the `MaxRank` type in the Core Fellowship pallet configuration from
+      `u32` to `u16`. Runtime developers updating to this version must:
+        - Change any `ConstU32<N>` usages for `MaxRank` to `ConstU16<N>`
+
+      Enabling reliable benchmarking for constrained configurations like `MaxRank=1` while
+      maintaining compatibility with larger rank ranges.
+
+
+
+crates:
+  - name: pallet-core-fellowship
+    bump: major
+  - name: collectives-westend-runtime
+    bump: minor
+  - name: kitchensink-runtime
+    bump: minor

substrate/bin/node/runtime/src/lib.rs

@@ -2060,7 +2060,7 @@ impl pallet_core_fellowship::Config for Runtime {
 	type PromoteOrigin = EnsureRootWithSuccess<AccountId, ConstU16<9>>;
 	type FastPromoteOrigin = Self::PromoteOrigin;
 	type EvidenceSize = ConstU32<16_384>;
-	type MaxRank = ConstU32<9>;
+	type MaxRank = ConstU16<9>;
 }
 
 parameter_types! {

substrate/frame/core-fellowship/src/benchmarking.rs

@@ -56,7 +56,7 @@ mod benchmarks {
 	}
 
 	fn set_benchmark_params<T: Config<I>, I: 'static>() -> Result<(), BenchmarkError> {
-		let max_rank = T::MaxRank::get().try_into().unwrap();
+		let max_rank = T::MaxRank::get() as usize;
 		let params = ParamsType {
 			active_salary: BoundedVec::try_from(vec![100u32.into(); max_rank]).unwrap(),
 			passive_salary: BoundedVec::try_from(vec![10u32.into(); max_rank]).unwrap(),
@@ -71,7 +71,7 @@ mod benchmarks {
 
 	#[benchmark]
 	fn set_params() -> Result<(), BenchmarkError> {
-		let max_rank = T::MaxRank::get().try_into().unwrap();
+		let max_rank = T::MaxRank::get() as usize;
 		let params = ParamsType {
 			active_salary: BoundedVec::try_from(vec![100u32.into(); max_rank]).unwrap(),
 			passive_salary: BoundedVec::try_from(vec![10u32.into(); max_rank]).unwrap(),
@@ -89,7 +89,7 @@ mod benchmarks {
 
 	#[benchmark]
 	fn set_partial_params() -> Result<(), BenchmarkError> {
-		let max_rank = T::MaxRank::get().try_into().unwrap();
+		let max_rank = T::MaxRank::get() as usize;
 
 		// Set up the initial default state for the Params storage
 		let params = ParamsType {
@@ -149,19 +149,22 @@ mod benchmarks {
 	fn bump_demote() -> Result<(), BenchmarkError> {
 		set_benchmark_params::<T, I>()?;
 
-		let member = make_member::<T, I>(2)?;
+		let initial_rank = T::MaxRank::get();
+
+		let member = make_member::<T, I>(initial_rank)?;
 
 		// Set it to the max value to ensure that any possible auto-demotion period has passed.
 		frame_system::Pallet::<T>::set_block_number(BlockNumberFor::<T>::max_value());
 		ensure_evidence::<T, I>(&member)?;
+
 		assert!(Member::<T, I>::contains_key(&member));
-		assert_eq!(T::Members::rank_of(&member), Some(2));
+		assert_eq!(T::Members::rank_of(&member), Some(initial_rank));
 
 		#[extrinsic_call]
 		CoreFellowship::<T, I>::bump(RawOrigin::Signed(member.clone()), member.clone());
 
 		assert!(Member::<T, I>::contains_key(&member));
-		assert_eq!(T::Members::rank_of(&member), Some(1));
+		assert_eq!(T::Members::rank_of(&member), Some(initial_rank.saturating_sub(1)));
 		assert!(!MemberEvidence::<T, I>::contains_key(&member));
 		Ok(())
 	}
@@ -194,36 +197,51 @@ mod benchmarks {
 	fn promote() -> Result<(), BenchmarkError> {
 		// Ensure that the `min_promotion_period` wont get in our way.
 		let mut params = Params::<T, I>::get();
-		let max_rank = T::MaxRank::get().try_into().unwrap();
-		params.min_promotion_period = BoundedVec::try_from(vec![Zero::zero(); max_rank]).unwrap();
+		let max_rank = T::MaxRank::get();
+
+		// Get minimum promotion period.
+		params.min_promotion_period =
+			BoundedVec::try_from(vec![Zero::zero(); max_rank as usize]).unwrap();
 		Params::<T, I>::put(&params);
 
-		let member = make_member::<T, I>(1)?;
+		// Start at rank 0 to allow at least one promotion.
+		let current_rank = 0;
+		let member = make_member::<T, I>(current_rank)?;
 
-		// Set it to the max value to ensure that any possible auto-demotion period has passed.
+		// Set `to_rank` dynamically based on `max_rank`.
+		let to_rank = (current_rank + 1).min(max_rank); // Ensure `to_rank` <= `max_rank`.
+
+		// Set block number to avoid auto-demotion.
 		frame_system::Pallet::<T>::set_block_number(BlockNumberFor::<T>::max_value());
 		ensure_evidence::<T, I>(&member)?;
 
 		#[extrinsic_call]
-		_(RawOrigin::Root, member.clone(), 2u8.into());
+		_(RawOrigin::Root, member.clone(), to_rank);
 
-		assert_eq!(T::Members::rank_of(&member), Some(2));
+		// Assert the new rank matches `to_rank` (not a hardcoded value).
+		assert_eq!(T::Members::rank_of(&member), Some(to_rank));
 		assert!(!MemberEvidence::<T, I>::contains_key(&member));
 		Ok(())
 	}
 
 	/// Benchmark the `promote_fast` extrinsic to promote someone up to `r`.
 	#[benchmark]
-	fn promote_fast(r: Linear<1, { T::MaxRank::get() as u32 }>) -> Result<(), BenchmarkError> {
-		let r = r.try_into().expect("r is too large");
+	fn promote_fast(
+		r: Linear<1, { ConvertU16ToU32::<T::MaxRank>::get() }>,
+	) -> Result<(), BenchmarkError> {
+		// Get target rank for promotion.
+		let max_rank = T::MaxRank::get();
+		let target_rank = (r as u16).min(max_rank);
+
+		// Begin with Candidate.
 		let member = make_member::<T, I>(0)?;
 
 		ensure_evidence::<T, I>(&member)?;
 
 		#[extrinsic_call]
-		_(RawOrigin::Root, member.clone(), r);
+		_(RawOrigin::Root, member.clone(), target_rank);
 
-		assert_eq!(T::Members::rank_of(&member), Some(r));
+		assert_eq!(T::Members::rank_of(&member), Some(target_rank));
 		assert!(!MemberEvidence::<T, I>::contains_key(&member));
 		Ok(())
 	}

substrate/frame/core-fellowship/src/lib.rs

@@ -163,6 +163,13 @@ impl<
 	}
 }
 
+pub struct ConvertU16ToU32<Inner>(PhantomData<Inner>);
+impl<Inner: Get<u16>> Get<u32> for ConvertU16ToU32<Inner> {
+	fn get() -> u32 {
+		Inner::get() as u32
+	}
+}
+
 /// The status of a single member.
 #[derive(Encode, Decode, Eq, PartialEq, Clone, TypeInfo, MaxEncodedLen, RuntimeDebug)]
 pub struct MemberStatus<BlockNumber> {
@@ -238,15 +245,18 @@ pub mod pallet {
 		///
 		/// Increasing this value is supported, but decreasing it may lead to a broken state.
 		#[pallet::constant]
-		type MaxRank: Get<u32>;
+		type MaxRank: Get<u16>;
 	}
 
-	pub type ParamsOf<T, I> =
-		ParamsType<<T as Config<I>>::Balance, BlockNumberFor<T>, <T as Config<I>>::MaxRank>;
+	pub type ParamsOf<T, I> = ParamsType<
+		<T as Config<I>>::Balance,
+		BlockNumberFor<T>,
+		ConvertU16ToU32<<T as Config<I>>::MaxRank>,
+	>;
 	pub type PartialParamsOf<T, I> = ParamsType<
 		Option<<T as Config<I>>::Balance>,
 		Option<BlockNumberFor<T>>,
-		<T as Config<I>>::MaxRank,
+		ConvertU16ToU32<<T as Config<I>>::MaxRank>,
 	>;
 	pub type MemberStatusOf<T> = MemberStatus<BlockNumberFor<T>>;
 	pub type RankOf<T, I> = <<T as Config<I>>::Members as RankedMembers>::Rank;
@@ -523,7 +533,7 @@ pub mod pallet {
 		/// This is useful for out-of-band promotions, hence it has its own `FastPromoteOrigin` to
 		/// be (possibly) more restrictive than `PromoteOrigin`. Note that the member must already
 		/// be inducted.
-		#[pallet::weight(T::WeightInfo::promote_fast(*to_rank as u32))]
+		#[pallet::weight(T::WeightInfo::promote_fast(*to_rank))]
 		#[pallet::call_index(10)]
 		pub fn promote_fast(
 			origin: OriginFor<T>,
@@ -534,7 +544,7 @@ pub mod pallet {
 				Ok(allow_rank) => ensure!(allow_rank >= to_rank, Error::<T, I>::NoPermission),
 				Err(origin) => ensure_root(origin)?,
 			}
-			ensure!(to_rank as u32 <= T::MaxRank::get(), Error::<T, I>::InvalidRank);
+			ensure!(to_rank <= T::MaxRank::get(), Error::<T, I>::InvalidRank);
 			let curr_rank = T::Members::rank_of(&who).ok_or(Error::<T, I>::Unranked)?;
 			ensure!(to_rank > curr_rank, Error::<T, I>::UnexpectedRank);
 
@@ -681,8 +691,8 @@ pub mod pallet {
 		///
 		/// Only elements in the base slice which has a new value in the new slice will be updated.
 		pub(crate) fn set_partial_params_slice<S>(
-			base_slice: &mut BoundedVec<S, <T as Config<I>>::MaxRank>,
-			new_slice: BoundedVec<Option<S>, <T as Config<I>>::MaxRank>,
+			base_slice: &mut BoundedVec<S, ConvertU16ToU32<T::MaxRank>>,
+			new_slice: BoundedVec<Option<S>, ConvertU16ToU32<T::MaxRank>>,
 		) {
 			for (base_element, new_element) in base_slice.iter_mut().zip(new_slice) {
 				if let Some(element) = new_element {
@@ -713,9 +723,10 @@ pub mod pallet {
 		/// Rank 1 becomes index 0, rank `RANK_COUNT` becomes index `RANK_COUNT - 1`. Any rank not
 		/// in the range `1..=RANK_COUNT` is `None`.
 		pub(crate) fn rank_to_index(rank: RankOf<T, I>) -> Option<usize> {
-			match TryInto::<usize>::try_into(rank) {
-				Ok(r) if r as u32 <= <T as Config<I>>::MaxRank::get() && r > 0 => Some(r - 1),
-				_ => return None,
+			if rank == 0 || rank > T::MaxRank::get() {
+				None
+			} else {
+				Some((rank - 1) as usize)
 			}
 		}
 

substrate/frame/core-fellowship/src/migration.rs

@@ -73,7 +73,7 @@ impl<T: Config<I>, I: 'static> UncheckedOnRuntimeUpgrade for MigrateToV1<T, I> {
 	#[cfg(feature = "try-runtime")]
 	fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
 		ensure!(
-			T::MaxRank::get() >= v0::RANK_COUNT as u32,
+			T::MaxRank::get() as usize >= v0::RANK_COUNT,
 			"pallet-core-fellowship: new bound should not truncate"
 		);
 		Ok(Default::default())

substrate/frame/core-fellowship/src/tests/integration.rs

@@ -27,7 +27,7 @@ use frame_support::{
 };
 use frame_system::EnsureSignedBy;
 use pallet_ranked_collective::{EnsureRanked, Geometric, Rank};
-use sp_core::{ConstU32, Get};
+use sp_core::Get;
 use sp_runtime::{
 	bounded_vec,
 	traits::{Convert, ReduceBy, ReplaceWithDefault, TryMorphInto},
@@ -82,7 +82,7 @@ impl Config for Test {
 	type PromoteOrigin = TryMapSuccess<EnsureSignedBy<IsInVec<ZeroToNine>, u64>, TryMorphInto<u16>>;
 	type FastPromoteOrigin = Self::PromoteOrigin;
 	type EvidenceSize = EvidenceSize;
-	type MaxRank = ConstU32<9>;
+	type MaxRank = ConstU16<9>;
 }
 
 /// Convert the tally class into the minimum rank required to vote on the poll.