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

Update Scheduler to have a configurable block provider #7434 #7441

Merged
merged 34 commits into from
Feb 13, 2025
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
212712d
initial commit
seemantaggarwal Feb 3, 2025
9314582
effort to add block number provider
seemantaggarwal Feb 3, 2025
fbb1eb7
fixing imports in the wrong pallet
seemantaggarwal Feb 4, 2025
8ea5046
fixing imports
seemantaggarwal Feb 4, 2025
a2c3c97
fixing import of blocknumberprovider under the correct pallet
seemantaggarwal Feb 4, 2025
88cf158
importing blocknumberprovider in the pallet
seemantaggarwal Feb 4, 2025
1ea6c26
adding blocknumber to mock
seemantaggarwal Feb 4, 2025
b29cf54
Use pallet's BlockNumberFor
ggwpez Feb 5, 2025
0670883
Merge branch 'master' into seemant/scheduler-bnp
seemantaggarwal Feb 5, 2025
ce52df4
cargo fmt + prdoc added
seemantaggarwal Feb 5, 2025
16585b9
cargo fmt
seemantaggarwal Feb 5, 2025
df2b959
Merge branch 'master' into seemant/scheduler-bnp
seemantaggarwal Feb 5, 2025
279ed58
adding testing
seemantaggarwal Feb 5, 2025
55b0acd
trial and error
seemantaggarwal Feb 6, 2025
d20b051
reverting 55b0acd158d1db765dd76598da716cdd57e62ec0
seemantaggarwal Feb 6, 2025
39a6e6c
trial and error more
seemantaggarwal Feb 8, 2025
0feaccc
reverting 39a6e6c24a1a2a799769cbf850385e774c02e044
seemantaggarwal Feb 8, 2025
62a4592
using block number in benchmarking
seemantaggarwal Feb 9, 2025
2fdf53d
cargo fmt
seemantaggarwal Feb 9, 2025
ae9e068
updating prdoc
seemantaggarwal Feb 10, 2025
c96dd35
adding a suggested block number value as a document
seemantaggarwal Feb 10, 2025
03fc0da
cargo fmt
seemantaggarwal Feb 10, 2025
9041d6c
Merge branch 'master' into seemant/scheduler-bnp
seemantaggarwal Feb 10, 2025
d7a8db1
addressing comment
seemantaggarwal Feb 10, 2025
b745976
improving the rust doc, addressing comment
seemantaggarwal Feb 11, 2025
b887564
improving the pr doc
seemantaggarwal Feb 11, 2025
2ba273a
fixing the soon to be deprecated max_value()
seemantaggarwal Feb 11, 2025
4c54432
updating tests
seemantaggarwal Feb 11, 2025
39ded63
fixup
ggwpez Feb 12, 2025
e09547a
docs
ggwpez Feb 12, 2025
17b7aec
Merge branch 'master' into seemant/scheduler-bnp
ggwpez Feb 12, 2025
441f5d3
prdoc
ggwpez Feb 12, 2025
21683a4
patch
ggwpez Feb 12, 2025
4069c9e
more docs
ggwpez Feb 12, 2025
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 @@ -628,6 +628,7 @@ impl pallet_scheduler::Config for Runtime {
type WeightInfo = weights::pallet_scheduler::WeightInfo<Runtime>;
type OriginPrivilegeCmp = EqualOrGreatestRootCmp;
type Preimages = Preimage;
type BlockNumberProvider = frame_system::Pallet<Runtime>;
}

parameter_types! {
Expand Down
1 change: 1 addition & 0 deletions polkadot/runtime/rococo/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,7 @@ impl pallet_scheduler::Config for Runtime {
type WeightInfo = weights::pallet_scheduler::WeightInfo<Runtime>;
type OriginPrivilegeCmp = OriginPrivilegeCmp;
type Preimages = Preimage;
type BlockNumberProvider = frame_system::Pallet<Runtime>;
}

parameter_types! {
Expand Down
1 change: 1 addition & 0 deletions polkadot/runtime/westend/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,7 @@ impl pallet_scheduler::Config for Runtime {
type WeightInfo = weights::pallet_scheduler::WeightInfo<Runtime>;
type OriginPrivilegeCmp = frame_support::traits::EqualPrivilegeOnly;
type Preimages = Preimage;
type BlockNumberProvider = frame_system::Pallet<Runtime>;
}

parameter_types! {
Expand Down
26 changes: 26 additions & 0 deletions prdoc/pr_7441.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
title: 'Update Scheduler to have a configurable block provider #7434'
doc:
- audience: Runtime Dev
description: |-
Follow up from https://github.com/paritytech/polkadot-sdk/pull/6362#issuecomment-2629744365

ggwpez marked this conversation as resolved.
Show resolved Hide resolved
The goal of this PR is to have the scheduler pallet work on a parachain which does not produce blocks on a regular schedule, thus can use the relay chain as a block provider.

Because blocks are not produced regularly, we cannot make the assumption that block number increases monotonically, and thus have new logic to handle multiple spend periods passing between blocks.
ggwpez marked this conversation as resolved.
Show resolved Hide resolved

Requirement:

instead of using the hard coded system block number. We add an associated type BlockNumberProvider
ggwpez marked this conversation as resolved.
Show resolved Hide resolved
crates:
- name: collectives-westend-runtime
bump: minor
- name: rococo-runtime
bump: minor
- name: westend-runtime
bump: minor
- name: pallet-democracy
bump: minor
- name: pallet-referenda
bump: minor
- name: pallet-scheduler
bump: minor
ggwpez marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions substrate/bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,7 @@ impl pallet_scheduler::Config for Runtime {
type WeightInfo = pallet_scheduler::weights::SubstrateWeight<Runtime>;
type OriginPrivilegeCmp = EqualPrivilegeOnly;
type Preimages = Preimage;
type BlockNumberProvider = frame_system::Pallet<Runtime>;
}

impl pallet_glutton::Config for Runtime {
Expand Down
1 change: 1 addition & 0 deletions substrate/frame/democracy/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ impl pallet_scheduler::Config for Test {
type WeightInfo = ();
type OriginPrivilegeCmp = EqualPrivilegeOnly;
type Preimages = ();
type BlockNumberProvider = frame_system::Pallet<Test>;
}

#[derive_impl(pallet_balances::config_preludes::TestDefaultConfig)]
Expand Down
1 change: 1 addition & 0 deletions substrate/frame/referenda/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ impl pallet_scheduler::Config for Test {
type WeightInfo = ();
type OriginPrivilegeCmp = EqualPrivilegeOnly;
type Preimages = Preimage;
type BlockNumberProvider = frame_system::Pallet<Test>;
}
#[derive_impl(pallet_balances::config_preludes::TestDefaultConfig)]
impl pallet_balances::Config for Test {
Expand Down
5 changes: 1 addition & 4 deletions substrate/frame/scheduler/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,7 @@ fn assert_last_event<T: Config>(generic_event: <T as Config>::RuntimeEvent) {
/// - `None`: aborted (hash without preimage)
/// - `Some(true)`: hash resolves into call if possible, plain call otherwise
/// - `Some(false)`: plain call
fn fill_schedule<T: Config>(
when: frame_system::pallet_prelude::BlockNumberFor<T>,
n: u32,
) -> Result<(), &'static str> {
fn fill_schedule<T: Config>(when: BlockNumberFor<T>, n: u32) -> Result<(), &'static str> {
let t = DispatchTime::At(when);
let origin: <T as Config>::PalletsOrigin = frame_system::RawOrigin::Root.into();
for i in 0..n {
Expand Down
26 changes: 15 additions & 11 deletions substrate/frame/scheduler/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,17 +100,16 @@ use frame_support::{
},
weights::{Weight, WeightMeter},
};
use frame_system::{
pallet_prelude::BlockNumberFor,
{self as system},
};
use frame_system::{self as system};
use scale_info::TypeInfo;
use sp_io::hashing::blake2_256;
use sp_runtime::{
traits::{BadOrigin, Dispatchable, One, Saturating, Zero},
BoundedVec, DispatchError, RuntimeDebug,
};

use sp_runtime::traits::BlockNumberProvider;

pub use pallet::*;
pub use weights::WeightInfo;

Expand All @@ -125,6 +124,9 @@ pub type CallOrHashOf<T> =
pub type BoundedCallOf<T> =
Bounded<<T as Config>::RuntimeCall, <T as frame_system::Config>::Hashing>;

pub type BlockNumberFor<T> =
<<T as Config>::BlockNumberProvider as BlockNumberProvider>::BlockNumber;

/// The configuration of the retry mechanism for a given task along with its current state.
#[derive(Clone, Copy, RuntimeDebug, PartialEq, Eq, Encode, Decode, MaxEncodedLen, TypeInfo)]
pub struct RetryConfig<Period> {
Expand Down Expand Up @@ -230,7 +232,7 @@ impl<T: WeightInfo> MarginalWeightInfo for T {}
pub mod pallet {
use super::*;
use frame_support::{dispatch::PostDispatchInfo, pallet_prelude::*};
use frame_system::pallet_prelude::*;
use frame_system::pallet_prelude::{BlockNumberFor as SystemBlockNumberFor, OriginFor};

/// The in-code storage version.
const STORAGE_VERSION: StorageVersion = StorageVersion::new(4);
Expand Down Expand Up @@ -292,6 +294,8 @@ pub mod pallet {

/// The preimage provider with which we look up call hashes to get the call.
type Preimages: QueryPreimage<H = Self::Hashing> + StorePreimage;

type BlockNumberProvider: BlockNumberProvider;
Copy link
Member

Choose a reason for hiding this comment

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

Docs? Devs using this pallet wont know what it is or how it should be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in commit 03fc0da

Copy link
Member

Choose a reason for hiding this comment

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

Please check how we normally write these docs. This is a comment // instead of a rust doc.
Also there should be one introduction sentence on what it does and then more elaborate paragraph following.

Just put yourself into the position of someone having never seen this before. What is it? How am i supposed to use it? And possible implications.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in commit -> d7a8db1
open question, I did try and put myself into the shoes for somebody new (was relatively easy, did not require any roleplaying haha) and I think this should be maybe a part of a larger readme? Not just for this pallet, but over multiple pallets explaining this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Runtime will implement the config trait they usually want documentation as precise as possible.
I agree crate documentation is also good, and in this PR case we can warn in both the crate doc and the config doc.

Copy link
Member

Choose a reason for hiding this comment

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

added some docs now

Copy link
Contributor

Choose a reason for hiding this comment

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

@seemantaggarwal adding documentation to public items in Rust is a must, please follow the advise given by other peers in this regard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @ggwpez and @kianenigma
I will keep this in mind going forward.

For the particular scenario, I should've reached out to the stakeholders, i.e. oliver, and Guillam, and acted more rapidly, to resolve the leftover and fill the gaps in my documentation.

I will keep this in ming going forward, and take more responsibility.

Thanks again :)

}

#[pallet::storage]
Expand Down Expand Up @@ -374,9 +378,10 @@ pub mod pallet {
}

#[pallet::hooks]
impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
impl<T: Config> Hooks<SystemBlockNumberFor<T>> for Pallet<T> {
/// Execute the scheduled calls
fn on_initialize(now: BlockNumberFor<T>) -> Weight {
fn on_initialize(_do_not_use_local_block_number: SystemBlockNumberFor<T>) -> Weight {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fn on_initialize(_do_not_use_local_block_number: SystemBlockNumberFor<T>) -> Weight {
fn on_initialize(_n: SystemBlockNumberFor<T>) -> Weight {

why? using local block provider can be just ok

Copy link
Member

Choose a reason for hiding this comment

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

In this case we want to always use the injected block number provider, so i think it makes sense.

let now = T::BlockNumberProvider::current_block_number();
let mut weight_counter = WeightMeter::with_limit(T::MaximumWeight::get());
Self::service_agendas(&mut weight_counter, now, u32::max_value());
weight_counter.consumed()
Expand Down Expand Up @@ -889,8 +894,7 @@ impl<T: Config> Pallet<T> {
fn resolve_time(
when: DispatchTime<BlockNumberFor<T>>,
) -> Result<BlockNumberFor<T>, DispatchError> {
let now = frame_system::Pallet::<T>::block_number();

let now = T::BlockNumberProvider::current_block_number();
let when = match when {
DispatchTime::At(x) => x,
// The current block has already completed it's scheduled tasks, so
Expand Down Expand Up @@ -926,11 +930,11 @@ impl<T: Config> Pallet<T> {
let mut agenda = Agenda::<T>::get(when);
let index = if (agenda.len() as u32) < T::MaxScheduledPerBlock::get() {
// will always succeed due to the above check.
let _ = agenda.try_push(Some(what));
let _ = agenda.try_push(Some(what.clone()));
ggwpez marked this conversation as resolved.
Show resolved Hide resolved
agenda.len() as u32 - 1
} else {
if let Some(hole_index) = agenda.iter().position(|i| i.is_none()) {
agenda[hole_index] = Some(what);
agenda[hole_index] = Some(what.clone());
hole_index as u32
} else {
return Err((DispatchError::Exhausted, what))
Expand Down
1 change: 0 additions & 1 deletion substrate/frame/scheduler/src/migration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
use super::*;
use frame_support::traits::OnRuntimeUpgrade;
use frame_system::pallet_prelude::BlockNumberFor;

#[cfg(feature = "try-runtime")]
use sp_runtime::TryRuntimeError;
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 probably deprecate the migrations. They would not interact so well with changing the BN provider at the same time and it is a lot of legacy code

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, keeping the old migrations around is an aspiration that we gave up on a while ago

Expand Down
3 changes: 2 additions & 1 deletion substrate/frame/scheduler/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,10 +223,11 @@ impl Config for Test {
type RuntimeCall = RuntimeCall;
type MaximumWeight = MaximumSchedulerWeight;
type ScheduleOrigin = EitherOfDiverse<EnsureRoot<u64>, EnsureSignedBy<One, u64>>;
type OriginPrivilegeCmp = EqualPrivilegeOnly;
type MaxScheduledPerBlock = ConstU32<10>;
type WeightInfo = TestWeightInfo;
type OriginPrivilegeCmp = EqualPrivilegeOnly;
type Preimages = Preimage;
type BlockNumberProvider = frame_system::Pallet<Self>;
}

pub type LoggerCall = logger::Call<Test>;
Expand Down
4 changes: 4 additions & 0 deletions substrate/frame/scheduler/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1636,6 +1636,7 @@ fn on_initialize_weight_is_correct() {
));

// Will include the named periodic only
System::set_block_number(1);
assert_eq!(
Scheduler::on_initialize(1),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Scheduler::on_initialize(1),
Scheduler::on_initialize(0),

I would make it everywhere like this to highlight it has not effect, plus a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, thanks, I need to start it by 0 instead of 1, and just add a comment, that this should not make a difference to the working? I did not understand what is the comment I need to put

TestWeightInfo::service_agendas_base() +
Expand All @@ -1648,6 +1649,7 @@ fn on_initialize_weight_is_correct() {
assert_eq!(logger::log(), vec![(root(), 2600u32)]);

// Will include anon and anon periodic
System::set_block_number(2);
assert_eq!(
Scheduler::on_initialize(2),
TestWeightInfo::service_agendas_base() +
Expand All @@ -1663,6 +1665,7 @@ fn on_initialize_weight_is_correct() {
assert_eq!(logger::log(), vec![(root(), 2600u32), (root(), 69u32), (root(), 42u32)]);

// Will include named only
System::set_block_number(3);
assert_eq!(
Scheduler::on_initialize(3),
TestWeightInfo::service_agendas_base() +
Expand All @@ -1678,6 +1681,7 @@ fn on_initialize_weight_is_correct() {
);

// Will contain none
System::set_block_number(4);
let actual_weight = Scheduler::on_initialize(4);
assert_eq!(
actual_weight,
Expand Down
Loading