Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clamp benchmark ranks to respect MaxRank #7720

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)`
Expand Down
14 changes: 14 additions & 0 deletions prdoc/pr_7720.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# 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: |
Enables reliable benchmarking for constrained configurations like `MaxRank=1` while
maintaining compatibility with larger rank ranges.

crates:
- name: pallet-core-fellowship
bump: minor
2 changes: 1 addition & 1 deletion substrate/bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2300,7 +2300,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! {
Expand Down
50 changes: 34 additions & 16 deletions substrate/frame/core-fellowship/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand All @@ -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(),
Expand All @@ -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 {
Expand Down Expand Up @@ -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(())
}
Expand Down Expand Up @@ -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(())
}
Expand Down
33 changes: 22 additions & 11 deletions substrate/frame/core-fellowship/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,13 @@ impl<
}
}

pub struct ConvertU16ToU32<Inner>(PhantomData<Inner>);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine for now. After the pr got merged, could you do a commit to the upstream crate that has the Get trait to introduce some sort of GetInto converter type:

impl<T: Get<I>, I: Into<R>, R> Get<R> GetInto<I> {
       fn get() -> R {
              T::get().into()
       }
}

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> {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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))]
Copy link
Contributor Author

@Doordashcon Doordashcon Mar 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would need to run benchmark command for this 🙏

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Generally we have now these weight updates once a week, that should be fine to be done as part of them.

#[pallet::call_index(10)]
pub fn promote_fast(
origin: OriginFor<T>,
Expand All @@ -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);

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
}
}

Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/core-fellowship/src/migration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down
4 changes: 2 additions & 2 deletions substrate/frame/core-fellowship/src/tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions substrate/frame/core-fellowship/src/tests/unit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -119,7 +119,7 @@ impl Config for Test {
type PromoteOrigin = TryMapSuccess<EnsureSignedBy<IsInVec<ZeroToNine>, u64>, TryMorphInto<u16>>;
type FastPromoteOrigin = Self::PromoteOrigin;
type EvidenceSize = ConstU32<1024>;
type MaxRank = ConstU32<9>;
type MaxRank = ConstU16<9>;
}

pub fn new_test_ext() -> sp_io::TestExternalities {
Expand Down
6 changes: 3 additions & 3 deletions substrate/frame/core-fellowship/src/weights.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading