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

Commit

Permalink
pallet-scheduler: Check that when is not in the past (#6480)
Browse files Browse the repository at this point in the history 
* `pallet-scheduler`: Check that `when` is not in the past

* Break some lines
  • Loading branch information
@bkchr
bkchr committed Jun 23, 2020
1 parent 8baaa18 commit 034055a
Show file tree
Hide file tree
Showing 2 changed files with 89 additions and 22 deletions.
109 changes: 88 additions & 21 deletions frame/scheduler/src/lib.rs
View file
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
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

0 comments on commit 034055a

Please sign in to comment.