-
Notifications
You must be signed in to change notification settings - Fork 767
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
[FRAME Core] General system for recognising and executing service work #206
[FRAME Core] General system for recognising and executing service work #206
Comments
I would be happy to take this on in the near future once benchmarking stuff wraps up |
On old attempt at trying to achieve something like this: paritytech/substrate#8197 Also related: #198 |
I already proposed something similar at https://forum.polkadot.network/t/pallet-idea-safe-scheduler/1009 |
If I want to rephrase what this issue is describing, I would do it as follows: Currently, in order to have "occasional free tasks" in a pallet, one needs to put together a large amount of boilerplate code: An unsigned (or signed, but conditionally free) extrinsic, appropriate Putting this next to #198, we have 3 ways to schedule optional tasks:
If you care about safety of panics, only the first one is safe. If you care about going overweight, all 3 of them are safe. Note that I am a bit torn about which approach is the best to invest further in. An extrinsic sounds the most user friendly, given that it is fool-proof. If you put a panic in your code, fail to benchmark it correctly and so on, it will still work. But it has a few downsides too:
I personally think that all of the above are useful in certain scenarios. We can start small and incrementally add different "modes of scheduling" to it, whilst keeping the API consistent. So as a user pallet/runtime, you would schedule a task into the task scheduler, possibly by specifying: enum Scheduling {
/// Safest option.
Extrinsic,
/// Scheduled at the end of blocks, as long as there is weight left after all mandatory and non-mandatory consumers.
OnIdle(Weight),
/// The most aggressive scheduling type.
///
/// Schedules the task to happen after all mandatory consumers, if there is weight left, but before all non-mandatory consumers.
Poll(Weight),
} A task could be something as simple as: trait Task {
/// Run the tests, returning `Some(_)` if this task is not Over, `None` otherwise.
///
/// From the scheduling mode, an implementation can decide if and how they should run. Note that `OnIdle` and `Poll` specify a `max_weight_to_consume`, while `Extrinsic` does not.
fn run(self, scheduling: Scheduling) -> (Option<Self>, Weight)
}
/// All dispatchables are tasks by default, so you can just make a call be task.
impl<T: Dispatchable> Task for T { .. } I will try and come up with a PoC of a "omni-scheduler" as explained above and see what I can come up with. |
I want to highlight a feature we implemented in the idle-scheduler is that it will automatically pause task dispatching if chain stall is detected. This ensures even if the task is bad (panic or overweight), it is still possible to rescue the chain. In one of my old unfinished draft design of the omni scheduler, the tasks priorities and valid block range and that influence how the task are dispatched. Dispatch mechanism:
High priority / time sensitive task will be dispatched with on_initialize. Time incentive task (such as lazy migration) will be dispatched with on_idle and fallback to unsigned_tx when approaching to deadline. Far future tasks are dispatched with ungined_tx. If those tasks are low priority, the unsigned_tx will schedule them into idle queue instead of dispatching them. Anyone can use signed tx to trigger a task as long as it is valid to dispatch it (i.e. current block within the execution range) |
please share you design docs for discussion before start coding |
yup going to go over everything and propose something before I start coding 👍🏻 |
The idle-scheduler's While I think it's reasonable to comment on the API as presented in the issue description to find means of syntactic improvement, I would not want this relatively well-defined and self-contained issue to suffer either from getting derailed or bikeshedded. |
Hey @sam0x17, are you already working on this? If you already have your proposed solution and/or PR, could you please share it? |
I have been mostly blocked until today getting paritytech/substrate#13454 merged but at this point I'm finally through all the change requests and merge conflicts and just have several places in the docs I need to update this morning and I should be good to finally put this as my first priority. I have notes on a proposed solution I've been mulling over that I will formalize and post shortly. In the mean time if anyone I haven't spoken to has any other thoughts please feel free to share them! |
@gavofyork I have one question about your syntax above.. My understanding is that this closely follows how we implement |
How else could an off-chain worker understand what tasks exist to execute? |
All right I think I have my head around this much better after talking to @gupnik ❤️ I would propose we use something like the following as a starting point for the pub trait Task: Sized {
/// Inspects the pallet's state and enumerates tasks of this kind.
fn enumerate() -> Iter<Self>;
/// Checks if a particular value of the `Task` variant is a valid piece of work.
fn is_valid(&self) -> bool;
/// Returns the `task_index` (analogous to `call_index`, but for tasks) of this
/// `Task` variant.
fn task_index(&self) -> usize;
/// Performs the work for this particular `Task` variant.
fn run(&self) -> Result<(), DispatchError>;
} If this makes sense, I'd like to start by setting up the struct and seeing if I can get a manual end-to-end example working without the macros. Then we can iterate on this and implement the macros. Sound good? |
|
yes you are right editing that, I had meant to write iter |
Looks like the right API, yes. I guess each pallet (which has tasks) would expose an |
That would make sense to me, so we can send these over RPC etc. |
Will this also be useful for storing tasks in storage, mid-flight, just as we do with MBMs? In general, a few examples of what would be first applications of this would be useful to share. My guesses is that anything that can be coded as a MBM, can also be coded as this + a lazy-decode implementation that would do the migration upon first decoded. These tasks can then be used to enumerate and re-encode certain keys/prefixes. Very similar to what Does that sounds right? |
This issue has been mentioned on Polkadot Forum. There might be relevant details there: https://forum.polkadot.network/t/pallet-idea-safe-scheduler/1009/7 |
This issue has been mentioned on Polkadot Forum. There might be relevant details there: https://forum.polkadot.network/t/make-xcm-execution-non-mandatory/3294/2 |
@sam0x17 Would we need something around weights here? |
This is a very good question. @gavofyork will weights need to factor in to this at all? |
Tasks will have weights, just like calls. If in doubt, consider a |
Actually, it will be good if we have less macro magics. Proposed syntax: /// Some running total.
#[pallet::storage]
pub(super) type Total<T: Config<I>, I: 'static = ()> =
StorageValue<_, (u32, u32), ValueQuery>;
/// Numbers to be added into the total.
#[pallet::storage]
pub(super) type Numbers<T: Config<I>, I: 'static = ()> =
StorageMap<_, Twox64Concat, u32, u32, OptionQuery>;
struct AddNumberIntoTotal;
impl Task for AddNumberIntoTotal {
const INDEX: u32 = 0;
type Id = u32;
fn all_tasks() -> Iterator<Self::Id> {
Numbers::<T, I>::iter_keys()
}
// not very sure why this is needed. `all_tasks` should already filter out invalid tasks.
fn condition(id: &Self::Id) -> bool {
Numbers::<T, I>::contains_key(i)
}
fn run(id: &Self::Id) -> DispatchResult {
let v = Numbers::<T, I>::take(i).ok_or(Error::<T, I>::NotFound)?;
Total::<T, I>::mutate(|(total_keys, total_values)| {
*total_keys += i;
*total_values += v;
});
Ok(())
}
}
#[pallet::tasks]
type AllTasks = (AddNumberIntoTotal, ) |
RE: @xlc
FYI the current syntax I have built parsing support for lives here: #1343 (comment) and I have reproduced it below for reference: #[pallet::task]
pub enum Task<T: Config> {
AddNumberIntoTotal,
}
/// Some running total.
#[pallet::storage]
pub(super) type Total<T: Config<I>, I: 'static = ()> =
StorageValue<_, (u32, u32), ValueQuery>;
/// Numbers to be added into the total.
#[pallet::storage]
pub(super) type Numbers<T: Config<I>, I: 'static = ()> =
StorageMap<_, Twox64Concat, u32, u32, OptionQuery>;
#[pallet::tasks]
impl<T: Config<I>, I: 'static> Pallet<T, I> {
/// Add a pair of numbers into the totals and remove them.
#[pallet::task_list(Numbers::<T, I>::iter_keys())]
#[pallet::task_condition(|i| Numbers::<T, I>::contains_key(i))]
#[pallet::task_index(0)]
pub fn add_number_into_total(i: u32) -> DispatchResult {
let v = Numbers::<T, I>::take(i).ok_or(Error::<T, I>::NotFound)?;
Total::<T, I>::mutate(|(total_keys, total_values)| {
*total_keys += i;
*total_values += v;
});
Ok(())
}
} This is almost identical to Gav's proposed syntax introduced above (#206 (comment)) with a few naming changes for clarity and some pieces that weren't included in his original example like the The way I have implemented the syntax, you should be able to manually impl The only place where your syntax seems to differ from the above is tasks behave like events and errors in my syntax so there is a I have no problem with leaving it open such that if One thing I don't like about my syntax is I don't think it is very clear that but yes @xlc thank you for making the point that we want to make the macro syntax as optional as possible and I will stay true to that. |
Seems fine except for the requirement to declare the items within |
Yeah retrofitting so we can omit the enum entirely and auto-generate it from the impl 👍🏻 edit: done |
`polkadot-sdk` version of original tasks PR located here: paritytech/substrate#14329 Fixes #206 ## Status - [x] Generic `Task` trait - [x] `RuntimeTask` aggregated enum, compatible with `construct_runtime!` - [x] Casting between `Task` and `RuntimeTask` without needing `dyn` or `Box` - [x] Tasks Example pallet - [x] Runtime tests for Tasks example pallet - [x] Parsing for task-related macros - [x] Retrofit parsing to make macros optional - [x] Expansion for task-related macros - [x] Adds support for args in tasks - [x] Retrofit tasks example pallet to use macros instead of manual syntax - [x] Weights - [x] Cleanup - [x] UI tests - [x] Docs ## Target Syntax Adapted from #206 (comment) ```rust // NOTE: this enum is optional and is auto-generated by the other macros if not present #[pallet::task] pub enum Task<T: Config> { AddNumberIntoTotal { i: u32, } } /// Some running total. #[pallet::storage] pub(super) type Total<T: Config<I>, I: 'static = ()> = StorageValue<_, (u32, u32), ValueQuery>; /// Numbers to be added into the total. #[pallet::storage] pub(super) type Numbers<T: Config<I>, I: 'static = ()> = StorageMap<_, Twox64Concat, u32, u32, OptionQuery>; #[pallet::tasks_experimental] impl<T: Config<I>, I: 'static> Pallet<T, I> { /// Add a pair of numbers into the totals and remove them. #[pallet::task_list(Numbers::<T, I>::iter_keys())] #[pallet::task_condition(|i| Numbers::<T, I>::contains_key(i))] #[pallet::task_index(0)] pub fn add_number_into_total(i: u32) -> DispatchResult { let v = Numbers::<T, I>::take(i).ok_or(Error::<T, I>::NotFound)?; Total::<T, I>::mutate(|(total_keys, total_values)| { *total_keys += i; *total_values += v; }); Ok(()) } } ``` --------- Co-authored-by: Nikhil Gupta <[email protected]> Co-authored-by: kianenigma <[email protected]> Co-authored-by: Nikhil Gupta <> Co-authored-by: Gavin Wood <[email protected]> Co-authored-by: Oliver Tale-Yazdi <[email protected]> Co-authored-by: gupnik <[email protected]>
…itytech#1343) `polkadot-sdk` version of original tasks PR located here: paritytech/substrate#14329 Fixes paritytech#206 ## Status - [x] Generic `Task` trait - [x] `RuntimeTask` aggregated enum, compatible with `construct_runtime!` - [x] Casting between `Task` and `RuntimeTask` without needing `dyn` or `Box` - [x] Tasks Example pallet - [x] Runtime tests for Tasks example pallet - [x] Parsing for task-related macros - [x] Retrofit parsing to make macros optional - [x] Expansion for task-related macros - [x] Adds support for args in tasks - [x] Retrofit tasks example pallet to use macros instead of manual syntax - [x] Weights - [x] Cleanup - [x] UI tests - [x] Docs ## Target Syntax Adapted from paritytech#206 (comment) ```rust // NOTE: this enum is optional and is auto-generated by the other macros if not present #[pallet::task] pub enum Task<T: Config> { AddNumberIntoTotal { i: u32, } } /// Some running total. #[pallet::storage] pub(super) type Total<T: Config<I>, I: 'static = ()> = StorageValue<_, (u32, u32), ValueQuery>; /// Numbers to be added into the total. #[pallet::storage] pub(super) type Numbers<T: Config<I>, I: 'static = ()> = StorageMap<_, Twox64Concat, u32, u32, OptionQuery>; #[pallet::tasks_experimental] impl<T: Config<I>, I: 'static> Pallet<T, I> { /// Add a pair of numbers into the totals and remove them. #[pallet::task_list(Numbers::<T, I>::iter_keys())] #[pallet::task_condition(|i| Numbers::<T, I>::contains_key(i))] #[pallet::task_index(0)] pub fn add_number_into_total(i: u32) -> DispatchResult { let v = Numbers::<T, I>::take(i).ok_or(Error::<T, I>::NotFound)?; Total::<T, I>::mutate(|(total_keys, total_values)| { *total_keys += i; *total_values += v; }); Ok(()) } } ``` --------- Co-authored-by: Nikhil Gupta <[email protected]> Co-authored-by: kianenigma <[email protected]> Co-authored-by: Nikhil Gupta <> Co-authored-by: Gavin Wood <[email protected]> Co-authored-by: Oliver Tale-Yazdi <[email protected]> Co-authored-by: gupnik <[email protected]>
Reopened as task-dispatch is still very much unfinished. |
There is often work (read: a state-transition which is potentially heavy) which needs to be done in a pallet when certain circumstances are reached. This is generally done either:
on_initialize
to periodically check for such conditions and conduct the resulting work if met.Ok(Pays::No)
returned rather than a fund-consumingErr
).All three of these have drawbacks. (1) introduces serious complexity into the code, possibly making effective benchmarking of an operation impractical. (2) wastes on-chain compute and introduces inescapable codepaths into block execution dramatically increasing the severity of bugs. (3) requires custom scripts to be made and spreads code with the same concern between languages (Rust and scripts) and codebases (the main pallet and a script repo) hurting development and maintenance.
What would be ideal is a standard means of determining what work can be executed via an off-chain worker or script and describing that work. A single script or (better) off-chain worker would recognise and be able to create and submit all possible work items at any given time as "service transactions". Pallets would only need to declare:
Task
data type (probably anenum
likeCall
) which describes items of work.Task
variant is indeed a valid piece of work (i.e. is contained in the prior enumeration);Code-wise, it could look like this:
This task would be called into via a System-pallet extrinsic (either through the block-builder, an OCW, submitted via a script or even manually through a transaction) without any further logic in the pallet whatsoever. The extrinsic could be called something like
fn do_task(origin: OriginFor<T>, task: Task)
.A custom
ValidateUnsigned
within the System pallet which recognises this extrinsic would ensure that thecondition
for thetask
is met prior to accepting it via an inherent/unsigned extrinsic (signed transactions can call it anyway and risk payment not being refunded).tasks
would ensure that the OCW/script could easily determine parameters which passed thiscondition
.If there were ever a bad bug (i.e. if
add_number_into_total
errored or panicked), then it would be easy to just not include the task in the block and the chain could continue operating as usual.This would massively facilitate lazy-migrations. It would allow Scheduler, Referenda and other
on_initialize
code to be adapted to avoid big problems if there are unexpected circumstances/errors. And it would improve overall transaction bandwidth by allowing non-time-critical ramifications to be uncoupled from time-critical state-transitions.Tasks could be labelled feeless in which case an origin check is unnecessary for all tasks which are considered tasks in the pre-dispatch.
TODO
The text was updated successfully, but these errors were encountered: