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 5 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
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
38 changes: 28 additions & 10 deletions substrate/frame/core-fellowship/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

use super::*;
use crate::Pallet as CoreFellowship;
use sp_runtime::SaturatedConversion;

use alloc::{boxed::Box, vec};
use frame_benchmarking::v2::*;
Expand Down Expand Up @@ -149,19 +150,23 @@ mod benchmarks {
fn bump_demote() -> Result<(), BenchmarkError> {
set_benchmark_params::<T, I>()?;

let member = make_member::<T, I>(2)?;
let max_rank: RankOf<T, I> = T::MaxRank::get().try_into().unwrap();
let initial_rank = max_rank.saturated_into::<u16>();
Copy link
Member

Choose a reason for hiding this comment

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

Then we can remove all the conversion code.


let member = make_member::<T, I>(max_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 @@ -195,35 +200,48 @@ mod benchmarks {
// 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();

// Get minimum promotion period.
params.min_promotion_period = BoundedVec::try_from(vec![Zero::zero(); max_rank]).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.try_into().unwrap()); // 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");
// Get target rank for promotion.
let max_rank = T::MaxRank::get();
let target_rank = r.min(max_rank).try_into().unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let target_rank = r.min(max_rank).try_into().unwrap();
let target_rank = r.min(max_rank);

r and max_rank are already a u32.

Copy link
Member

Choose a reason for hiding this comment

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

We should change MaxRank to u16. I don't get why ranks are u16 and MaxRank is u32.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pub struct ConvertU16ToU32<Inner>(PhantomData<Inner>);

for BoundedVec


// Begin with Candidate.
let member = make_member::<T, I>(0)?;
log::info!("Benchmark promote_fast: r={} MaxRank={}", r, T::MaxRank::get());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
log::info!("Benchmark promote_fast: r={} MaxRank={}", r, T::MaxRank::get());


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
Loading