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

seemantaggarwal · 2025-02-03T18:44:32Z

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

gui1117

Direction is good

gui1117 · 2025-02-03T23:14:06Z

substrate/frame/scheduler/src/lib.rs

@@ -297,6 +300,8 @@ pub mod pallet {
 	#[pallet::storage]
 	pub type IncompleteSince<T: Config> = StorageValue<_, BlockNumberFor<T>>;

+	type BlockNumberProvider: BlockNumberProvider;


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.

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 :)

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).

seemantaggarwal · 2025-02-04T07:11:28Z

note: this is a part of #6297

seemantaggarwal · 2025-02-04T11:33:32Z

This is where I am stuck atm:

error[E0277]: the trait bound <<<T as frame_system::Config>::Block as sp_runtime::traits::Block>::Header as sp_runtime::traits::Header>::Number: EncodeLike<<<T as pallet::Config>::BlockNumberProvider as sp_runtime::traits::BlockNumberProvider>::BlockNumber> is not satisfied
     --> /Users/seemantaggarwal/RustroverProjects/polkadot-sdk/substrate/frame/scheduler/src/lib.rs:651:36
      |
  651 | ...                   Lookup::<T>::insert(name, item);
      |                       -------------------       ^^^^ unsatisfied trait bound
      |                       |
      |                       required by a bound introduced by this call

What I have tried:

Convert the block number to match expected type:
something like: let converted_number: T::BlockNumber = <T::BlockNumberProvider as BlockNumberProvider>::current_block_number().saturated_into(); Lookup::<T>::insert(name, (converted_number, some_value));
and then something like:
if let Some(item) = old::Lookup::<T>::take(id) { let converted_item: (u32, u32) = (item.1, item.0.into()); // Convert using .into() Lookup::<T>::insert(name, converted_item); }
I have also tried changing the migration functions by adding
pub fn migrate_v2_to_v4() -> Weight where <<<T as frame_system::Config>::Block as sp_runtime::traits::Block>::Header as sp_runtime::traits::Header>::Number: parity_scale_codec::EncodeLike<<<T as pallet::Config>::BlockNumberProvider as sp_runtime::traits::BlockNumberProvider>::BlockNumber>{

Now this, makes me think I have 2 following options going ahead:

Implement a new migrate v4_to_v5 for lookup (I am not sure how to do that yet, but can be figured out)
Implement EncodeLike for BlockNumberProvider (as far as I understand this cannot be done since it is coming from codec and not a custom impl)

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 <oliver.tale-yazdi@parity.io>

gui1117 · 2025-02-05T11:28:07Z

This is where I am stuck atm:
error[E0277]: the trait bound <<<T as frame_system::Config>::Block as sp_runtime::traits::Block>::Header as sp_runtime::traits::Header>::Number: EncodeLike<<<T as pallet::Config>::BlockNumberProvider as sp_runtime::traits::BlockNumberProvider>::BlockNumber> is not satisfied
     --> /Users/seemantaggarwal/RustroverProjects/polkadot-sdk/substrate/frame/scheduler/src/lib.rs:651:36
      |
  651 | ...                   Lookup::<T>::insert(name, item);
      |                       -------------------       ^^^^ unsatisfied trait bound
      |                       |
      |                       required by a bound introduced by this call
What I have tried:

Convert the block number to match expected type:
something like: let converted_number: T::BlockNumber = <T::BlockNumberProvider as BlockNumberProvider>::current_block_number().saturated_into(); Lookup::<T>::insert(name, (converted_number, some_value));
and then something like:
if let Some(item) = old::Lookup::<T>::take(id) { let converted_item: (u32, u32) = (item.1, item.0.into()); // Convert using .into() Lookup::<T>::insert(name, converted_item); }

I have also tried changing the migration functions by adding
pub fn migrate_v2_to_v4() -> Weight where <<<T as frame_system::Config>::Block as sp_runtime::traits::Block>::Header as sp_runtime::traits::Header>::Number: parity_scale_codec::EncodeLike<<<T as pallet::Config>::BlockNumberProvider as sp_runtime::traits::BlockNumberProvider>::BlockNumber>{

Now this, makes me think I have 2 following options going ahead:

Implement a new migrate v4_to_v5 for lookup (I am not sure how to do that yet, but can be figured out)

Implement EncodeLike for BlockNumberProvider (as far as I understand this cannot be done since it is coming from codec and not a custom impl)

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

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.

seemantaggarwal · 2025-02-06T07:24:58Z

Looking at the current failure it fails when running:
cargo nextest run --workspace --features runtime-benchmarks benchmark --locked --cargo-profile testnet --cargo-quiet
with the error:

Error[E0308]: mismatched types --> substrate/frame/scheduler/src/benchmarking.rs:62:40 | 62 | Pallet::<T>::do_schedule_named(name, t, period, 0, origin.clone(), call)?; | ------------------------------ ^ expected sp_runtime::traits::BlockNumberProvider::BlockNumber, found sp_runtime::traits::Header::Number| |

and other similar.
check the error here: https://github.com/paritytech/polkadot-sdk/actions/runs/13163415501/job/36737726646

I tried:
replacing the hardcoded
-const BLOCK_NUMBER: u32 = 2;
and now = BLOCK_NUMBER.into()
with

let now = max_scheduled_blocks::<T>().into();
where
fn max_scheduled_blocks<T: Config>() -> u32 { T::MaxScheduledPerBlock::get() }

seemantaggarwal · 2025-02-06T19:03:56Z

Reverted the last commit, as it did not really solve much, bringing it to attention of @gui1117 @ggwpez
i fixed the test too though.

#7441 (comment) -> read the comment here

muharem · 2025-02-07T06:59:39Z

Error[E0308]: mismatched types --> substrate/frame/scheduler/src/benchmarking.rs:62:40 | 62 | Pallet::::do_schedule_named(name, t, period, 0, origin.clone(), call)?; | ------------------------------ ^ expected sp_runtime::traits::BlockNumberProvider::BlockNumber, found sp_runtime::traits::Header::Number| |

@seemantaggarwal should be your new block number alias instead

polkadot-sdk/substrate/frame/scheduler/src/benchmarking.rs

Line 53 in f373af0

when: frame_system::pallet_prelude::BlockNumberFor<T>,

seemantaggarwal · 2025-02-10T06:13:52Z

Requesting review from @gui1117 @muharem @ggwpez @kianenigma

ggwpez · 2025-02-10T10:31:44Z

substrate/frame/scheduler/src/lib.rs

@@ -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;


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

addressed in commit 03fc0da

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.

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?

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.

prdoc/pr_7441.prdoc

ggwpez · 2025-02-10T11:49:41Z

prdoc/pr_7441.prdoc

+  description: |-
+    Follow up from https://github.com/paritytech/polkadot-sdk/pull/6362#issuecomment-2629744365
+
+        The goal of this PR is to have the treasury 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


and thus have new logic to handle multiple spend periods passing between blocks

I think this is also dated?

ggwpez · 2025-02-10T11:50:18Z

prdoc/pr_7441.prdoc

+
+    Requirement:
+
+     instead of using the hard coded system block number. We add an associated type BlockNumberProvider


This prdoc is meant for the external world. It must explain how to integrate the new change. Putting requirements here will not be so helpful for someone checking this 😄

ggwpez · 2025-02-10T11:53:07Z

substrate/frame/scheduler/src/lib.rs

@@ -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;


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.

muharem · 2025-02-11T07:30:47Z

prdoc/pr_7441.prdoc

+    Follow up from https://github.com/paritytech/polkadot-sdk/pull/6362#issuecomment-2629744365
+


Suggested change

Follow up from https://github.com/paritytech/polkadot-sdk/pull/6362#issuecomment-2629744365

we do not really need it in prdoc

muharem · 2025-02-11T07:32:42Z

prdoc/pr_7441.prdoc

+  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.


This is misleading. It make block number provider configurable. It can be any source, for any type of chain (not only parachain). There is no problem keeping the schduler on parachains system block provider as well.

You should also mention that to keep the behaviour as it used to be before this config introduced, they need to set it frame_system::Pallet::<Runtime>.

Please read the issue and the comments carefully I already mentioned this

muharem · 2025-02-11T07:34:24Z

substrate/frame/scheduler/src/lib.rs

 		/// Execute the scheduled calls
-		fn on_initialize(now: BlockNumberFor<T>) -> Weight {
+		fn on_initialize(_do_not_use_local_block_number: SystemBlockNumberFor<T>) -> Weight {


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

muharem · 2025-02-11T07:35:57Z

substrate/frame/scheduler/src/tests.rs

@@ -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),


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

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

muharem · 2025-02-11T07:37:58Z

substrate/frame/scheduler/src/lib.rs

@@ -926,11 +934,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()));


why clone? please review your code before requesting others to review

muharem · 2025-02-11T07:43:15Z

substrate/frame/scheduler/src/lib.rs

+		/// 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


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.

paritytech-workflow-stopper · 2025-02-11T09:43:09Z

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/13259626868
Failed job name: build-rustdoc

initial commit

212712d

seemantaggarwal requested a review from a team as a code owner February 3, 2025 18:44

seemantaggarwal mentioned this pull request Feb 3, 2025

Update Scheduler to have a configurable block provider #7434

Closed

effort to add block number provider

9314582

gui1117 reviewed Feb 3, 2025

View reviewed changes

gui1117 added the T2-pallets This PR/Issue is related to a particular pallet. label Feb 3, 2025

fixing imports in the wrong pallet

fbb1eb7

paritytech-review-bot bot requested a review from a team February 4, 2025 07:28

seemantaggarwal added 4 commits February 4, 2025 13:06

fixing imports

8ea5046

fixing import of blocknumberprovider under the correct pallet

a2c3c97

importing blocknumberprovider in the pallet

88cf158

adding blocknumber to mock

1ea6c26

Use pallet's BlockNumberFor

b29cf54

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Merge branch 'master' into seemant/scheduler-bnp

0670883

seemantaggarwal self-assigned this Feb 5, 2025

seemantaggarwal and others added 4 commits February 5, 2025 20:40

cargo fmt + prdoc added

ce52df4

cargo fmt

16585b9

Merge branch 'master' into seemant/scheduler-bnp

df2b959

adding testing

279ed58

seemantaggarwal added 2 commits February 6, 2025 13:16

trial and error

55b0acd

reverting 55b0acd

d20b051

muharem self-requested a review February 7, 2025 07:00

muharem mentioned this pull request Feb 7, 2025

[tracking] adopt BlockNumberProvider for the pallets migrating to AH #6297

Open

trial and error more

39a6e6c

seemantaggarwal added 3 commits February 9, 2025 02:33

reverting 39a6e6c

0feaccc

using block number in benchmarking

62a4592

cargo fmt

2fdf53d

seemantaggarwal requested review from ggwpez, gui1117 and a team February 10, 2025 06:14

ggwpez requested changes Feb 10, 2025

View reviewed changes

updating prdoc

ae9e068

paritytech-review-bot bot requested a review from a team February 10, 2025 10:54

seemantaggarwal added 2 commits February 10, 2025 16:26

adding a suggested block number value as a document

c96dd35

cargo fmt

03fc0da

seemantaggarwal enabled auto-merge February 10, 2025 11:41

ggwpez reviewed Feb 10, 2025

View reviewed changes

Merge branch 'master' into seemant/scheduler-bnp

9041d6c

paritytech-review-bot bot requested a review from a team February 10, 2025 12:21

addressing comment

d7a8db1

muharem reviewed Feb 11, 2025

View reviewed changes

improving the rust doc, addressing comment

b745976

paritytech-review-bot bot requested a review from a team February 11, 2025 09:18

improving the pr doc

b887564

seemantaggarwal added 2 commits February 11, 2025 15:23

fixing the soon to be deprecated max_value()

2ba273a

updating tests

4c54432

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

seemantaggarwal commented Feb 3, 2025

gui1117 left a comment

gui1117 Feb 3, 2025

seemantaggarwal Feb 4, 2025

gui1117 Feb 4, 2025

seemantaggarwal commented Feb 4, 2025

seemantaggarwal commented Feb 4, 2025 •

edited by gui1117

Loading

gui1117 commented Feb 5, 2025

seemantaggarwal commented Feb 6, 2025 •

edited

Loading

seemantaggarwal commented Feb 6, 2025

muharem commented Feb 7, 2025

seemantaggarwal commented Feb 10, 2025

ggwpez Feb 10, 2025

seemantaggarwal Feb 10, 2025

ggwpez Feb 10, 2025

seemantaggarwal Feb 10, 2025

gui1117 Feb 11, 2025

ggwpez Feb 10, 2025

ggwpez Feb 10, 2025

ggwpez Feb 10, 2025

muharem Feb 11, 2025

muharem Feb 11, 2025

muharem Feb 11, 2025

muharem Feb 11, 2025 •

edited

Loading

muharem Feb 11, 2025

muharem Feb 11, 2025

seemantaggarwal Feb 11, 2025

muharem Feb 11, 2025 •

edited

Loading

muharem Feb 11, 2025

paritytech-workflow-stopper bot commented Feb 11, 2025

		Follow up from https://github.com/paritytech/polkadot-sdk/pull/6362#issuecomment-2629744365

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

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

Are you sure you want to change the base?

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

Conversation

seemantaggarwal commented Feb 3, 2025

gui1117 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seemantaggarwal commented Feb 4, 2025

seemantaggarwal commented Feb 4, 2025 • edited by gui1117 Loading

gui1117 commented Feb 5, 2025

seemantaggarwal commented Feb 6, 2025 • edited Loading

seemantaggarwal commented Feb 6, 2025

muharem commented Feb 7, 2025

seemantaggarwal commented Feb 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muharem Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muharem Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paritytech-workflow-stopper bot commented Feb 11, 2025

seemantaggarwal commented Feb 4, 2025 •

edited by gui1117

Loading

seemantaggarwal commented Feb 6, 2025 •

edited

Loading

muharem Feb 11, 2025 •

edited

Loading

muharem Feb 11, 2025 •

edited

Loading