diff --git a/cumulus/parachains/runtimes/collectives/collectives-westend/src/ambassador/mod.rs b/cumulus/parachains/runtimes/collectives/collectives-westend/src/ambassador/mod.rs index 8b0842697237f..9711a34589de8 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/ambassador/mod.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/ambassador/mod.rs @@ -223,7 +223,7 @@ impl pallet_core_fellowship::Config 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; diff --git a/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs b/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs index 3fcaeb1dfc4fc..ceadc131c384e 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs @@ -211,7 +211,7 @@ impl pallet_core_fellowship::Config for Runtime { >; type FastPromoteOrigin = Self::PromoteOrigin; type EvidenceSize = ConstU32<65536>; - type MaxRank = ConstU32<9>; + type MaxRank = ConstU16<9>; } pub type FellowshipSalaryInstance = pallet_salary::Instance1; diff --git a/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_core_fellowship_ambassador_core.rs b/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_core_fellowship_ambassador_core.rs index c47746be78e97..4f24def3ec505 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_core_fellowship_ambassador_core.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_core_fellowship_ambassador_core.rs @@ -193,7 +193,7 @@ impl 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)` diff --git a/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_core_fellowship_fellowship_core.rs b/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_core_fellowship_fellowship_core.rs index 0ca5c19b88aeb..db9254d5a6d7e 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_core_fellowship_fellowship_core.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_core_fellowship_fellowship_core.rs @@ -193,7 +193,7 @@ impl 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)` diff --git a/prdoc/pr_7720.prdoc b/prdoc/pr_7720.prdoc new file mode 100644 index 0000000000000..48ad47f21277e --- /dev/null +++ b/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` usages for `MaxRank` to `ConstU16` + + 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 diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index d0a9c667fbb23..723bead6a1c1b 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -2060,7 +2060,7 @@ impl pallet_core_fellowship::Config for Runtime { type PromoteOrigin = EnsureRootWithSuccess>; type FastPromoteOrigin = Self::PromoteOrigin; type EvidenceSize = ConstU32<16_384>; - type MaxRank = ConstU32<9>; + type MaxRank = ConstU16<9>; } parameter_types! { diff --git a/substrate/frame/core-fellowship/src/benchmarking.rs b/substrate/frame/core-fellowship/src/benchmarking.rs index ac0d489953c1f..7fdec9748516f 100644 --- a/substrate/frame/core-fellowship/src/benchmarking.rs +++ b/substrate/frame/core-fellowship/src/benchmarking.rs @@ -56,7 +56,7 @@ mod benchmarks { } fn set_benchmark_params, 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::()?; - let member = make_member::(2)?; + let initial_rank = T::MaxRank::get(); + + let member = make_member::(initial_rank)?; // Set it to the max value to ensure that any possible auto-demotion period has passed. frame_system::Pallet::::set_block_number(BlockNumberFor::::max_value()); ensure_evidence::(&member)?; + assert!(Member::::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::::bump(RawOrigin::Signed(member.clone()), member.clone()); assert!(Member::::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::::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::::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::::put(¶ms); - let member = make_member::(1)?; + // Start at rank 0 to allow at least one promotion. + let current_rank = 0; + let member = make_member::(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::::set_block_number(BlockNumberFor::::max_value()); ensure_evidence::(&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::::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::::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::(0)?; ensure_evidence::(&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::::contains_key(&member)); Ok(()) } diff --git a/substrate/frame/core-fellowship/src/lib.rs b/substrate/frame/core-fellowship/src/lib.rs index 3d2da6e9f717e..1aac8d8ef5fce 100644 --- a/substrate/frame/core-fellowship/src/lib.rs +++ b/substrate/frame/core-fellowship/src/lib.rs @@ -163,6 +163,13 @@ impl< } } +pub struct ConvertU16ToU32(PhantomData); +impl> Get for ConvertU16ToU32 { + 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 { @@ -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; + type MaxRank: Get; } - pub type ParamsOf = - ParamsType<>::Balance, BlockNumberFor, >::MaxRank>; + pub type ParamsOf = ParamsType< + >::Balance, + BlockNumberFor, + ConvertU16ToU32<>::MaxRank>, + >; pub type PartialParamsOf = ParamsType< Option<>::Balance>, Option>, - >::MaxRank, + ConvertU16ToU32<>::MaxRank>, >; pub type MemberStatusOf = MemberStatus>; pub type RankOf = <>::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, @@ -534,7 +544,7 @@ pub mod pallet { Ok(allow_rank) => ensure!(allow_rank >= to_rank, Error::::NoPermission), Err(origin) => ensure_root(origin)?, } - ensure!(to_rank as u32 <= T::MaxRank::get(), Error::::InvalidRank); + ensure!(to_rank <= T::MaxRank::get(), Error::::InvalidRank); let curr_rank = T::Members::rank_of(&who).ok_or(Error::::Unranked)?; ensure!(to_rank > curr_rank, Error::::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( - base_slice: &mut BoundedVec>::MaxRank>, - new_slice: BoundedVec, >::MaxRank>, + base_slice: &mut BoundedVec>, + new_slice: BoundedVec, ConvertU16ToU32>, ) { 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) -> Option { - match TryInto::::try_into(rank) { - Ok(r) if r as u32 <= >::MaxRank::get() && r > 0 => Some(r - 1), - _ => return None, + if rank == 0 || rank > T::MaxRank::get() { + None + } else { + Some((rank - 1) as usize) } } diff --git a/substrate/frame/core-fellowship/src/migration.rs b/substrate/frame/core-fellowship/src/migration.rs index b1e27d1e79363..216523f0c7364 100644 --- a/substrate/frame/core-fellowship/src/migration.rs +++ b/substrate/frame/core-fellowship/src/migration.rs @@ -73,7 +73,7 @@ impl, I: 'static> UncheckedOnRuntimeUpgrade for MigrateToV1 { #[cfg(feature = "try-runtime")] fn pre_upgrade() -> Result, 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()) diff --git a/substrate/frame/core-fellowship/src/tests/integration.rs b/substrate/frame/core-fellowship/src/tests/integration.rs index b2149336547d3..db411f91d269f 100644 --- a/substrate/frame/core-fellowship/src/tests/integration.rs +++ b/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, u64>, TryMorphInto>; 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. diff --git a/substrate/frame/core-fellowship/src/tests/unit.rs b/substrate/frame/core-fellowship/src/tests/unit.rs index f4418ed439d0c..f8ae5afcc1f23 100644 --- a/substrate/frame/core-fellowship/src/tests/unit.rs +++ b/substrate/frame/core-fellowship/src/tests/unit.rs @@ -26,7 +26,7 @@ use frame_support::{ assert_noop, assert_ok, derive_impl, hypothetically, ord_parameter_types, pallet_prelude::Weight, parameter_types, - traits::{tokens::GetSalary, ConstU32, IsInVec, TryMapSuccess}, + traits::{tokens::GetSalary, ConstU16, ConstU32, IsInVec, TryMapSuccess}, }; use frame_system::EnsureSignedBy; use sp_runtime::{bounded_vec, traits::TryMorphInto, BuildStorage, DispatchError, DispatchResult}; @@ -119,7 +119,7 @@ impl Config for Test { type PromoteOrigin = TryMapSuccess, u64>, TryMorphInto>; type FastPromoteOrigin = Self::PromoteOrigin; type EvidenceSize = ConstU32<1024>; - type MaxRank = ConstU32<9>; + type MaxRank = ConstU16<9>; } pub fn new_test_ext() -> sp_io::TestExternalities { diff --git a/substrate/frame/core-fellowship/src/weights.rs b/substrate/frame/core-fellowship/src/weights.rs index 9c0fffd3d05ec..25f6310addc92 100644 --- a/substrate/frame/core-fellowship/src/weights.rs +++ b/substrate/frame/core-fellowship/src/weights.rs @@ -79,7 +79,7 @@ pub trait WeightInfo { fn set_active() -> Weight; fn induct() -> Weight; fn promote() -> Weight; - fn promote_fast(r: u32, ) -> Weight; + fn promote_fast(r: u16, ) -> Weight; fn offboard() -> Weight; fn import() -> Weight; fn import_member() -> Weight; @@ -225,7 +225,7 @@ impl WeightInfo for SubstrateWeight { /// Storage: `RankedCollective::IdToIndex` (r:0 w:9) /// Proof: `RankedCollective::IdToIndex` (`max_values`: None, `max_size`: Some(54), added: 2529, mode: `MaxEncodedLen`) /// 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: `16665` // Estimated: `19894 + r * (2489 ±0)` @@ -447,7 +447,7 @@ impl WeightInfo for () { /// Storage: `RankedCollective::IdToIndex` (r:0 w:9) /// Proof: `RankedCollective::IdToIndex` (`max_values`: None, `max_size`: Some(54), added: 2529, mode: `MaxEncodedLen`) /// 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: `16665` // Estimated: `19894 + r * (2489 ±0)`