Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Fix benchmarks failing on scheduler overflow #13143

Closed
wants to merge 6 commits into from
Closed
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
8 changes: 4 additions & 4 deletions bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,7 @@ impl pallet_scheduler::Config for Runtime {
type RuntimeCall = RuntimeCall;
type MaximumWeight = MaximumSchedulerWeight;
type ScheduleOrigin = EnsureRoot<AccountId>;
type MaxScheduledPerBlock = ConstU32<512>;
type MaxScheduledPerBlock = ConstU32<50>;
type WeightInfo = pallet_scheduler::weights::SubstrateWeight<Runtime>;
type OriginPrivilegeCmp = EqualPrivilegeOnly;
type Preimages = Preimage;
Expand Down Expand Up @@ -828,10 +828,10 @@ impl pallet_referenda::TracksInfo<Balance, BlockNumber> for TracksInfo {
0u16,
pallet_referenda::TrackInfo {
name: "root",
max_deciding: 1,
max_deciding: 50,
decision_deposit: 10,
prepare_period: 4,
decision_period: 4,
prepare_period: 10,
decision_period: 20,
confirm_period: 2,
min_enactment_period: 4,
min_approval: pallet_referenda::Curve::LinearDecreasing {
Expand Down
50 changes: 42 additions & 8 deletions frame/conviction-voting/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use frame_support::{
dispatch::RawOrigin,
traits::{fungible, Currency, Get},
};
use sp_runtime::traits::Bounded;
use sp_runtime::traits::{Bounded, One};
use sp_std::collections::btree_map::BTreeMap;

use crate::Pallet as ConvictionVoting;
Expand Down Expand Up @@ -61,6 +61,12 @@ fn account_vote<T: Config<I>, I: 'static>(b: BalanceOf<T, I>) -> AccountVote<Bal
AccountVote::Standard { vote: v, balance: b }
}

fn move_to_next_block<T: frame_system::Config>() {
frame_system::Pallet::<T>::set_block_number(
frame_system::Pallet::<T>::block_number() + One::one(),
);
}

benchmarks_instance_pallet! {
where_clause { where T::MaxVotes: core::fmt::Debug }

Expand All @@ -73,7 +79,11 @@ benchmarks_instance_pallet! {
let polls = &all_polls[&class];
let r = polls.len() - 1;
// We need to create existing votes
for i in polls.iter().skip(1) {
for (p, i) in polls.iter().skip(1).enumerate() {
if p as u32 % T::Polls::max_access_poll_per_block() == 0 {
// move to the next block to not overflow the scheduler agenda.
move_to_next_block::<T>();
}
ConvictionVoting::<T, I>::vote(RawOrigin::Signed(caller.clone()).into(), *i, account_vote)?;
}
let votes = match VotingFor::<T, I>::get(&caller, &class) {
Expand All @@ -100,7 +110,11 @@ benchmarks_instance_pallet! {
let polls = &all_polls[&class];
let r = polls.len();
// We need to create existing votes
for i in polls.iter() {
for (p, i) in polls.iter().enumerate() {
if p as u32 % T::Polls::max_access_poll_per_block() == 0 {
// move to the next block to not overflow the scheduler agenda.
move_to_next_block::<T>();
}
ConvictionVoting::<T, I>::vote(RawOrigin::Signed(caller.clone()).into(), *i, old_account_vote)?;
}
let votes = match VotingFor::<T, I>::get(&caller, &class) {
Expand Down Expand Up @@ -128,7 +142,11 @@ benchmarks_instance_pallet! {
let polls = &all_polls[&class];
let r = polls.len();
// We need to create existing votes
for i in polls.iter() {
for (p, i) in polls.iter().enumerate() {
if p as u32 % T::Polls::max_access_poll_per_block() == 0 {
// move to the next block to not overflow the scheduler agenda.
move_to_next_block::<T>();
};
ConvictionVoting::<T, I>::vote(RawOrigin::Signed(caller.clone()).into(), *i, old_account_vote)?;
}
let votes = match VotingFor::<T, I>::get(&caller, &class) {
Expand Down Expand Up @@ -157,7 +175,11 @@ benchmarks_instance_pallet! {
let polls = &all_polls[&class];
let r = polls.len();
// We need to create existing votes
for i in polls.iter() {
for (p, i) in polls.iter().enumerate() {
if p as u32 % T::Polls::max_access_poll_per_block() == 0 {
// move to the next block to not overflow the scheduler agenda.
move_to_next_block::<T>();
}
ConvictionVoting::<T, I>::vote(RawOrigin::Signed(voter.clone()).into(), *i, old_account_vote)?;
}
let votes = match VotingFor::<T, I>::get(&caller, &class) {
Expand Down Expand Up @@ -191,7 +213,11 @@ benchmarks_instance_pallet! {
let delegate_vote = account_vote::<T, I>(delegated_balance);

// We need to create existing delegations
for i in polls.iter().take(r as usize) {
for (p, i) in polls.iter().enumerate().take(r as usize) {
if p as u32 % T::Polls::max_access_poll_per_block() == 0 {
// move to the next block to not overflow the scheduler agenda.
move_to_next_block::<T>();
}
ConvictionVoting::<T, I>::vote(RawOrigin::Signed(voter.clone()).into(), *i, delegate_vote)?;
}
assert_matches!(
Expand Down Expand Up @@ -227,7 +253,11 @@ benchmarks_instance_pallet! {
)?;

// We need to create delegations
for i in polls.iter().take(r as usize) {
for (p, i) in polls.iter().enumerate().take(r as usize) {
if p as u32 % T::Polls::max_access_poll_per_block() == 0 {
// move to the next block to not overflow the scheduler agenda.
move_to_next_block::<T>();
}
ConvictionVoting::<T, I>::vote(RawOrigin::Signed(voter.clone()).into(), *i, delegate_vote)?;
}
assert_matches!(
Expand All @@ -252,7 +282,11 @@ benchmarks_instance_pallet! {
assert!(all_polls.len() > 0);
for (class, polls) in all_polls.iter() {
assert!(polls.len() > 0);
for i in polls.iter() {
for (p, i) in polls.iter().enumerate() {
if p as u32 % T::Polls::max_access_poll_per_block() == 0 {
// move to the next block to not overflow the scheduler agenda.
move_to_next_block::<T>();
}
ConvictionVoting::<T, I>::vote(RawOrigin::Signed(caller.clone()).into(), *i, normal_account_vote)?;
}
}
Expand Down
46 changes: 41 additions & 5 deletions frame/referenda/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,25 @@ fn fill_queue<T: Config<I>, I: 'static>(
spaces: u32,
pass_after: u32,
) -> Vec<ReferendumIndex> {
let track_info = info::<T, I>(index);
// the block number when the decision period of the 'index' referendum ends.
let first_decision_end = now::<T>() + track_info.prepare_period + track_info.decision_period;

let maybe_move_to_next_block = |others_count| {
// check if scheduler max capacity for the current block is reached.
if (others_count + 1) % (T::Scheduler::max_scheduled_per_block() as usize) == 0 {
let next_block = now::<T>() + One::one();
// decision period of the first referendum must not end
// after the block number will be set to the referendum's prepare_period.
assert!(next_block + track_info.prepare_period < first_decision_end);
frame_system::Pallet::<T>::set_block_number(next_block);
}
};

// First, create enough other referendums to fill the track.
let mut others = vec![];
for _ in 0..info::<T, I>(index).max_deciding {
for _ in 0..track_info.max_deciding {
maybe_move_to_next_block(others.len());
let (_origin, index) = create_referendum::<T, I>();
place_deposit::<T, I>(index);
others.push(index);
Expand All @@ -90,21 +106,41 @@ fn fill_queue<T: Config<I>, I: 'static>(
// We will also need enough referenda which are queued and passing, we want `MaxQueued - 1`
// in order to force the maximum amount of work to insert ours into the queue.
for _ in spaces..T::MaxQueued::get() {
maybe_move_to_next_block(others.len());
let (_origin, index) = create_referendum::<T, I>();
place_deposit::<T, I>(index);
make_passing_after::<T, I>(index, Perbill::from_percent(pass_after));
others.push(index);
}

// Skip to when they can start being decided.
skip_prepare_period::<T, I>(index);
// Skip to when all referendums can start being decided.
frame_system::Pallet::<T>::set_block_number(now::<T>() + track_info.prepare_period);

// make queued referendums passing.
others
.iter()
.skip(track_info.max_deciding as usize)
.enumerate()
.for_each(|(i, &item)| {
maybe_move_to_next_block(i);
make_passing_after::<T, I>(item, Perbill::from_percent(pass_after));
});

// Move to the next block to not overflow the scheduler agenda.
frame_system::Pallet::<T>::set_block_number(now::<T>() + One::one());

// Manually nudge the other referenda first to ensure that they begin.
others.iter().for_each(|&i| nudge::<T, I>(i));
others.iter().enumerate().for_each(|(i, &item)| {
maybe_move_to_next_block(i);
nudge::<T, I>(item);
});

others
}

fn now<T: frame_system::Config>() -> T::BlockNumber {
frame_system::Pallet::<T>::block_number()
}

fn info<T: Config<I>, I: 'static>(index: ReferendumIndex) -> &'static TrackInfoOf<T, I> {
let status = Referenda::<T, I>::ensure_ongoing(index).unwrap();
T::Tracks::info(status.track).expect("Id value returned from T::Tracks")
Expand Down
19 changes: 16 additions & 3 deletions frame/referenda/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ use frame_support::{
traits::{
schedule::{
v3::{Anon as ScheduleAnon, Named as ScheduleNamed},
DispatchTime,
ConfigInfo, DispatchTime,
},
Currency, LockIdentifier, OnUnbalanced, OriginTrait, PollStatus, Polling, QueryPreimage,
ReservableCurrency, StorePreimage, VoteTally,
Expand Down Expand Up @@ -163,7 +163,8 @@ pub mod pallet {
type WeightInfo: WeightInfo;
/// The Scheduler.
type Scheduler: ScheduleAnon<Self::BlockNumber, CallOf<Self, I>, PalletsOriginOf<Self>>
+ ScheduleNamed<Self::BlockNumber, CallOf<Self, I>, PalletsOriginOf<Self>>;
+ ScheduleNamed<Self::BlockNumber, CallOf<Self, I>, PalletsOriginOf<Self>>
+ ConfigInfo;
/// Currency type for this pallet.
type Currency: ReservableCurrency<Self::AccountId>;
// Origins and unbalances.
Expand Down Expand Up @@ -690,6 +691,7 @@ impl<T: Config<I>, I: 'static> Polling<T::Tally> for Pallet<T, I> {

#[cfg(feature = "runtime-benchmarks")]
fn create_ongoing(class: Self::Class) -> Result<Self::Index, ()> {
use sp_runtime::traits::Bounded;
let index = ReferendumCount::<T, I>::mutate(|x| {
let r = *x;
*x += 1;
Expand All @@ -713,7 +715,12 @@ impl<T: Config<I>, I: 'static> Polling<T::Tally> for Pallet<T, I> {
in_queue: false,
alarm: None,
};
Self::ensure_alarm_at(&mut status, index, sp_runtime::traits::Bounded::max_value());
Self::ensure_alarm_at(
&mut status,
index,
// shift the scheduled for block to not overflow the scheduler agenda.
T::BlockNumber::max_value() - T::BlockNumber::from(index),
);
ReferendumInfoFor::<T, I>::insert(index, ReferendumInfo::Ongoing(status));
Ok(index)
}
Expand Down Expand Up @@ -741,6 +748,12 @@ impl<T: Config<I>, I: 'static> Polling<T::Tally> for Pallet<T, I> {
.expect("Always one class");
(r.0, r.1.max_deciding)
}

/// The maximum number of the access poll invocations possible per block.
#[cfg(feature = "runtime-benchmarks")]
fn max_access_poll_per_block() -> u32 {
T::Scheduler::max_scheduled_per_block()
}
}

impl<T: Config<I>, I: 'static> Pallet<T, I> {
Expand Down
9 changes: 9 additions & 0 deletions frame/scheduler/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1299,3 +1299,12 @@ fn map_err_to_v3_err<T: Config>(err: DispatchError) -> DispatchError {
err
}
}

/// Implements [schedule::ConfigInfo] trait.
impl<T: Config> schedule::ConfigInfo for Pallet<T> {
/// Return the maximum number of tasks possible to schedule per block.
#[cfg(feature = "runtime-benchmarks")]
fn max_scheduled_per_block() -> u32 {
T::MaxScheduledPerBlock::get()
}
}
7 changes: 7 additions & 0 deletions frame/support/src/traits/schedule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -473,3 +473,10 @@ pub mod v3 {
}

pub use v1::*;

/// A type that aware about a scheduler configuration.
pub trait ConfigInfo {
/// Return the maximum number of tasks possible to schedule per block.
#[cfg(feature = "runtime-benchmarks")]
fn max_scheduled_per_block() -> u32;
}
6 changes: 6 additions & 0 deletions frame/support/src/traits/voting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,4 +186,10 @@ pub trait Polling<Tally> {
fn max_ongoing() -> (Self::Class, u32) {
(Self::classes().into_iter().next().expect("Always one class"), u32::max_value())
}

/// The maximum number of the access poll invocations possible per block.
#[cfg(feature = "runtime-benchmarks")]
fn max_access_poll_per_block() -> u32 {
u32::max_value()
}
}