-
Notifications
You must be signed in to change notification settings - Fork 835
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
Conversation
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.
Direction is good
substrate/frame/scheduler/src/lib.rs
Outdated
@@ -297,6 +300,8 @@ pub mod pallet { | |||
#[pallet::storage] | |||
pub type IncompleteSince<T: Config> = StorageValue<_, BlockNumberFor<T>>; | |||
|
|||
type BlockNumberProvider: BlockNumberProvider; |
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.
We will need a doc comment that explains this type, and warns that it must not be too much out of sync with the local block number. More precisely between every block the increment of this number should be reasonably small otherwise performance degrades will degrade a lot as it reads one storage per block number.
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.
Thanks, I will add it towards the end of the pr for sure, I will look into it. At this moment, I am not entirely sure of the value that needs to be advised, i will get more insights into it, thanks for the reminder :)
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.
We can suggest people to use the system block number or the relay chain block number (this second option, only if they expect their parachain to execute very regularly).
note: this is a part of #6297 |
This is where I am stuck atm:
What I have tried:
Now this, makes me think I have 2 following options going ahead:
Would like to get some opinion/help here on the options, or if there is any another thing to try Note: things I have tried are just a summary of the rabbit hole,if you do leave a comment here, I can try it out and answer if it works for me or not |
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
The type: <<T as frame_system::Config>::Block as sp_runtime::traits::Block>::Header as sp_runtime::traits::Header>::Number Doesn't implement: EncodeLike<<<T as pallet::Config>::BlockNumberProvider as sp_runtime::traits::BlockNumberProvider>::BlockNumber> Because: <<T as pallet::Config>::BlockNumberProvider as sp_runtime::traits::BlockNumberProvider>::BlockNumber Is a completely different type. I think you are trying somewhere to put the system block number into storage, but the type expect the configured provided block number. |
Looking at the current failure it fails when running:
and other similar. I tried:
|
Reverted the last commit, as it did not really solve much, bringing it to attention of @gui1117 @ggwpez #7441 (comment) -> read the comment here |
@seemantaggarwal should be your new block number alias instead
|
@@ -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; |
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.
Docs? Devs using this pallet wont know what it is or how it should be used.
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.
addressed in commit 03fc0da
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.
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.
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.
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?
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.
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.
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.
added some docs now
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.
@seemantaggarwal adding documentation to public items in Rust is a must, please follow the advise given by other peers in this regard.
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.
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 :)
@@ -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; |
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.
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.
substrate/frame/scheduler/src/lib.rs
Outdated
/// Execute the scheduled calls | ||
fn on_initialize(now: BlockNumberFor<T>) -> Weight { | ||
fn on_initialize(_do_not_use_local_block_number: SystemBlockNumberFor<T>) -> Weight { |
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.
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.
@@ -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), |
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.
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
substrate/frame/scheduler/src/lib.rs
Outdated
/// 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 |
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.
this doc needs to be updated. not clear, not right, not rust style.
what is "very regularly"? what is "much out of sync"? "he increment of this number should be reasonably small", the increment is +1
. but overall you need to rewrite it, wrong direction.
All GitHub workflows were cancelled due to failure one of the required jobs. |
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
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.
LGTM, had to push some fixes since we want to get this into the next release.
@@ -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; |
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.
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
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.
Yeah, keeping the old migrations around is an aspiration that we gave up on a while ago
paritytech#7441) Follow up from paritytech#6362 (comment) 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. Requirement: instead of using the hard coded system block number. We add an associated type BlockNumberProvider --------- Signed-off-by: Oliver Tale-Yazdi <[email protected]> Co-authored-by: Oliver Tale-Yazdi <[email protected]>
Follow up from #6362 (comment)
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.
Requirement:
instead of using the hard coded system block number. We add an associated type BlockNumberProvider