-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Update Scheduler to have a configurable block provider #7434 #7441
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
Changes from 24 commits
212712d
9314582
fbb1eb7
8ea5046
a2c3c97
88cf158
1ea6c26
b29cf54
0670883
ce52df4
16585b9
df2b959
279ed58
55b0acd
d20b051
39a6e6c
0feaccc
62a4592
2fdf53d
ae9e068
c96dd35
03fc0da
9041d6c
d7a8db1
b745976
b887564
2ba273a
4c54432
39ded63
e09547a
17b7aec
441f5d3
21683a4
4069c9e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| 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 | ||
|
|
||
| This PR modifies the pallet_scheduler to support parachains that do not produce blocks on a regular schedule. Since block numbers may not increase monotonically in such environments, the scheduler cannot rely on frame_system::Pallet::<T>::block_number(). Instead, this update introduces BlockNumberProvider = frame_system::Pallet<Runtime> in pallet_scheduler::Config, allowing the scheduler to use an external provider, such as the relay chain, for block numbers. | ||
ggwpez marked this conversation as resolved.
Outdated
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: major | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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; | ||||||
|
|
||||||
|
|
@@ -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> { | ||||||
|
|
@@ -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); | ||||||
|
|
@@ -292,6 +294,12 @@ pub mod pallet { | |||||
|
|
||||||
| /// The preimage provider with which we look up call hashes to get the call. | ||||||
| type Preimages: QueryPreimage<H = Self::Hashing> + StorePreimage; | ||||||
|
|
||||||
| /// Suggested value: Use the system block number or if you expect the parachain to execute | ||||||
| /// very regularly you can use the relay chain block number. Warning: This value must not | ||||||
| /// be too much out of sync with the local block number. Between every block the increment of | ||||||
| /// this number should be reasonably small to avoid performance degradation and added latency | ||||||
|
||||||
| type BlockNumberProvider: BlockNumberProvider; | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed in commit 03fc0da
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please check how we normally write these docs. This is a comment 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed in commit -> d7a8db1
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added some docs now
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @ggwpez and @kianenigma 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] | ||||||
|
|
@@ -374,9 +382,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 { | ||||||
|
||||||
| 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
There was a problem hiding this comment.
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.
ggwpez marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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), | ||||||
|
||||||
| Scheduler::on_initialize(1), | |
| Scheduler::on_initialize(0), |
I would make it everywhere like this to highlight it has not effect, plus a comment
There was a problem hiding this comment.
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
Uh oh!
There was an error while loading. Please reload this page.