Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

pallet-scheduler: Check that when is not in the past #6480

Merged
merged 2 commits into from
Jun 23, 2020
Merged

pallet-scheduler: Check that when is not in the past #6480

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
db7ac97
`pallet-scheduler`: Check that `when` is not in the past
bkchr Jun 23, 2020
f42c714
Break some lines
bkchr Jun 23, 2020
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
109 changes: 88 additions & 21 deletions frame/scheduler/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,8 @@ decl_error! {
FailedToSchedule,
/// Failed to cancel a scheduled call
FailedToCancel,
/// Given target block number is in the past.
TargetBlockNumberInPast,
}
}

Expand All @@ -145,7 +147,7 @@ decl_module! {
call: Box<<T as Trait>::Call>,
) {
ensure_root(origin)?;
let _ = Self::do_schedule(when, maybe_periodic, priority, *call);
Self::do_schedule(when, maybe_periodic, priority, *call)?;
}

/// Cancel an anonymously scheduled task.
Expand Down Expand Up @@ -294,7 +296,11 @@ impl<T: Trait> Module<T> {
maybe_periodic: Option<schedule::Period<T::BlockNumber>>,
priority: schedule::Priority,
call: <T as Trait>::Call
) -> TaskAddress<T::BlockNumber> {
) -> Result<TaskAddress<T::BlockNumber>, DispatchError> {
if when <= frame_system::Module::<T>::block_number() {
return Err(Error::<T>::TargetBlockNumberInPast.into())
}

// sanitize maybe_periodic
let maybe_periodic = maybe_periodic
.filter(|p| p.1 > 1 && !p.0.is_zero())
Expand All @@ -304,7 +310,8 @@ impl<T: Trait> Module<T> {
Agenda::<T>::append(when, s);
let index = Agenda::<T>::decode_len(when).unwrap_or(1) as u32 - 1;
Self::deposit_event(RawEvent::Scheduled(when, index));
(when, index)

Ok((when, index))
}

fn do_cancel((when, index): TaskAddress<T::BlockNumber>) -> Result<(), DispatchError> {
Expand All @@ -331,6 +338,10 @@ impl<T: Trait> Module<T> {
return Err(Error::<T>::FailedToSchedule)?
}

if when <= frame_system::Module::<T>::block_number() {
return Err(Error::<T>::TargetBlockNumberInPast.into())
}

// sanitize maybe_periodic
let maybe_periodic = maybe_periodic
.filter(|p| p.1 > 1 && !p.0.is_zero())
Expand All @@ -343,6 +354,7 @@ impl<T: Trait> Module<T> {
let address = (when, index);
Lookup::<T>::insert(&id, &address);
Self::deposit_event(RawEvent::Scheduled(when, index));

Ok(address)
}

Expand All @@ -366,7 +378,7 @@ impl<T: Trait> schedule::Anon<T::BlockNumber, <T as Trait>::Call> for Module<T>
maybe_periodic: Option<schedule::Period<T::BlockNumber>>,
priority: schedule::Priority,
call: <T as Trait>::Call
) -> Self::Address {
) -> Result<Self::Address, DispatchError> {
Self::do_schedule(when, maybe_periodic, priority, call)
}

Expand Down Expand Up @@ -399,8 +411,7 @@ mod tests {

use frame_support::{
impl_outer_event, impl_outer_origin, impl_outer_dispatch, parameter_types, assert_ok,
traits::{OnInitialize, OnFinalize, Filter},
weights::constants::RocksDbWeight,
assert_err, traits::{OnInitialize, OnFinalize, Filter}, weights::constants::RocksDbWeight,
};
use sp_core::H256;
// The testing primitives are very useful for avoiding having to work with signatures
Expand Down Expand Up @@ -551,7 +562,7 @@ mod tests {
new_test_ext().execute_with(|| {
let call = Call::Logger(logger::Call::log(42, 1000));
assert!(!<Test as frame_system::Trait>::BaseCallFilter::filter(&call));
Scheduler::do_schedule(4, None, 127, call);
let _ = Scheduler::do_schedule(4, None, 127, call);
run_to_block(3);
assert!(logger::log().is_empty());
run_to_block(4);
Expand All @@ -565,7 +576,7 @@ mod tests {
fn periodic_scheduling_works() {
new_test_ext().execute_with(|| {
// at #4, every 3 blocks, 3 times.
Scheduler::do_schedule(4, Some((3, 3)), 127, Call::Logger(logger::Call::log(42, 1000)));
let _ = Scheduler::do_schedule(4, Some((3, 3)), 127, Call::Logger(logger::Call::log(42, 1000)));
run_to_block(3);
assert!(logger::log().is_empty());
run_to_block(4);
Expand All @@ -588,7 +599,7 @@ mod tests {
new_test_ext().execute_with(|| {
// at #4.
Scheduler::do_schedule_named(1u32.encode(), 4, None, 127, Call::Logger(logger::Call::log(69, 1000))).unwrap();
let i = Scheduler::do_schedule(4, None, 127, Call::Logger(logger::Call::log(42, 1000)));
let i = Scheduler::do_schedule(4, None, 127, Call::Logger(logger::Call::log(42, 1000))).unwrap();
run_to_block(3);
assert!(logger::log().is_empty());
assert_ok!(Scheduler::do_cancel_named(1u32.encode()));
Expand Down Expand Up @@ -621,8 +632,8 @@ mod tests {
#[test]
fn scheduler_respects_weight_limits() {
new_test_ext().execute_with(|| {
Scheduler::do_schedule(4, None, 127, Call::Logger(logger::Call::log(42, MaximumSchedulerWeight::get() / 2)));
Scheduler::do_schedule(4, None, 127, Call::Logger(logger::Call::log(69, MaximumSchedulerWeight::get() / 2)));
let _ = Scheduler::do_schedule(4, None, 127, Call::Logger(logger::Call::log(42, MaximumSchedulerWeight::get() / 2)));
let _ = Scheduler::do_schedule(4, None, 127, Call::Logger(logger::Call::log(69, MaximumSchedulerWeight::get() / 2)));
// 69 and 42 do not fit together
run_to_block(4);
assert_eq!(logger::log(), vec![42u32]);
Expand All @@ -634,8 +645,8 @@ mod tests {
#[test]
fn scheduler_respects_hard_deadlines_more() {
new_test_ext().execute_with(|| {
Scheduler::do_schedule(4, None, 0, Call::Logger(logger::Call::log(42, MaximumSchedulerWeight::get() / 2)));
Scheduler::do_schedule(4, None, 0, Call::Logger(logger::Call::log(69, MaximumSchedulerWeight::get() / 2)));
let _ = Scheduler::do_schedule(4, None, 0, Call::Logger(logger::Call::log(42, MaximumSchedulerWeight::get() / 2)));
let _ = Scheduler::do_schedule(4, None, 0, Call::Logger(logger::Call::log(69, MaximumSchedulerWeight::get() / 2)));
// With base weights, 69 and 42 should not fit together, but do because of hard deadlines
run_to_block(4);
assert_eq!(logger::log(), vec![42u32, 69u32]);
Expand All @@ -645,8 +656,8 @@ mod tests {
#[test]
fn scheduler_respects_priority_ordering() {
new_test_ext().execute_with(|| {
Scheduler::do_schedule(4, None, 1, Call::Logger(logger::Call::log(42, MaximumSchedulerWeight::get() / 2)));
Scheduler::do_schedule(4, None, 0, Call::Logger(logger::Call::log(69, MaximumSchedulerWeight::get() / 2)));
let _ = Scheduler::do_schedule(4, None, 1, Call::Logger(logger::Call::log(42, MaximumSchedulerWeight::get() / 2)));
let _ = Scheduler::do_schedule(4, None, 0, Call::Logger(logger::Call::log(69, MaximumSchedulerWeight::get() / 2)));
run_to_block(4);
assert_eq!(logger::log(), vec![69u32, 42u32]);
});
Expand All @@ -655,9 +666,24 @@ mod tests {
#[test]
fn scheduler_respects_priority_ordering_with_soft_deadlines() {
new_test_ext().execute_with(|| {
Scheduler::do_schedule(4, None, 255, Call::Logger(logger::Call::log(42, MaximumSchedulerWeight::get() / 3)));
Scheduler::do_schedule(4, None, 127, Call::Logger(logger::Call::log(69, MaximumSchedulerWeight::get() / 2)));
Scheduler::do_schedule(4, None, 126, Call::Logger(logger::Call::log(2600, MaximumSchedulerWeight::get() / 2)));
let _ = Scheduler::do_schedule(
4,
None,
255,
Call::Logger(logger::Call::log(42, MaximumSchedulerWeight::get() / 3)),
);
let _ = Scheduler::do_schedule(
4,
None,
127,
Call::Logger(logger::Call::log(69, MaximumSchedulerWeight::get() / 2)),
);
let _ = Scheduler::do_schedule(
4,
None,
126,
Call::Logger(logger::Call::log(2600, MaximumSchedulerWeight::get() / 2)),
);

// 2600 does not fit with 69 or 42, but has higher priority, so will go through
run_to_block(4);
Expand All @@ -679,11 +705,27 @@ mod tests {
// Named
assert_ok!(Scheduler::do_schedule_named(1u32.encode(), 1, None, 255, Call::Logger(logger::Call::log(3, MaximumSchedulerWeight::get() / 3))));
// Anon Periodic
Scheduler::do_schedule(1, Some((1000, 3)), 128, Call::Logger(logger::Call::log(42, MaximumSchedulerWeight::get() / 3)));
let _ = Scheduler::do_schedule(
1,
Some((1000, 3)),
128,
Call::Logger(logger::Call::log(42, MaximumSchedulerWeight::get() / 3)),
);
// Anon
Scheduler::do_schedule(1, None, 127, Call::Logger(logger::Call::log(69, MaximumSchedulerWeight::get() / 2)));
let _ = Scheduler::do_schedule(
1,
None,
127,
Call::Logger(logger::Call::log(69, MaximumSchedulerWeight::get() / 2)),
);
// Named Periodic
assert_ok!(Scheduler::do_schedule_named(2u32.encode(), 1, Some((1000, 3)), 126, Call::Logger(logger::Call::log(2600, MaximumSchedulerWeight::get() / 2))));
assert_ok!(Scheduler::do_schedule_named(
2u32.encode(),
1,
Some((1000, 3)),
126,
Call::Logger(logger::Call::log(2600, MaximumSchedulerWeight::get() / 2)),
));

// Will include the named periodic only
let actual_weight = Scheduler::on_initialize(1);
Expand Down Expand Up @@ -727,4 +769,29 @@ mod tests {
assert!(logger::log().is_empty());
});
}

#[test]
fn fails_to_schedule_task_in_the_past() {
new_test_ext().execute_with(|| {
run_to_block(3);

let call = Box::new(Call::Logger(logger::Call::log(69, 1000)));
let call2 = Box::new(Call::Logger(logger::Call::log(42, 1000)));

assert_err!(
Scheduler::schedule_named(Origin::root(), 1u32.encode(), 2, None, 127, call),
Error::<Test>::TargetBlockNumberInPast,
);

assert_err!(
Scheduler::schedule(Origin::root(), 2, None, 127, call2.clone()),
Error::<Test>::TargetBlockNumberInPast,
);

assert_err!(
Scheduler::schedule(Origin::root(), 3, None, 127, call2),
Error::<Test>::TargetBlockNumberInPast,
);
});
}
}
2 changes: 1 addition & 1 deletion frame/support/src/traits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1501,7 +1501,7 @@ pub mod schedule {
maybe_periodic: Option<Period<BlockNumber>>,
priority: Priority,
call: Call
) -> Self::Address;
) -> Result<Self::Address, DispatchError>;

/// Cancel a scheduled task. If periodic, then it will cancel all further instances of that,
/// also.
Expand Down