From 034055a092bd376c3b7252b58f1777de70dfad59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Tue, 23 Jun 2020 17:25:19 +0200 Subject: [PATCH] `pallet-scheduler`: Check that `when` is not in the past (#6480) * `pallet-scheduler`: Check that `when` is not in the past * Break some lines --- frame/scheduler/src/lib.rs | 109 +++++++++++++++++++++++++++++------- frame/support/src/traits.rs | 2 +- 2 files changed, 89 insertions(+), 22 deletions(-) diff --git a/frame/scheduler/src/lib.rs b/frame/scheduler/src/lib.rs index 18b4eef0a87df..6b47e6258708e 100644 --- a/frame/scheduler/src/lib.rs +++ b/frame/scheduler/src/lib.rs @@ -119,6 +119,8 @@ decl_error! { FailedToSchedule, /// Failed to cancel a scheduled call FailedToCancel, + /// Given target block number is in the past. + TargetBlockNumberInPast, } } @@ -145,7 +147,7 @@ decl_module! { call: Box<::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. @@ -294,7 +296,11 @@ impl Module { maybe_periodic: Option>, priority: schedule::Priority, call: ::Call - ) -> TaskAddress { + ) -> Result, DispatchError> { + if when <= frame_system::Module::::block_number() { + return Err(Error::::TargetBlockNumberInPast.into()) + } + // sanitize maybe_periodic let maybe_periodic = maybe_periodic .filter(|p| p.1 > 1 && !p.0.is_zero()) @@ -304,7 +310,8 @@ impl Module { Agenda::::append(when, s); let index = Agenda::::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) -> Result<(), DispatchError> { @@ -331,6 +338,10 @@ impl Module { return Err(Error::::FailedToSchedule)? } + if when <= frame_system::Module::::block_number() { + return Err(Error::::TargetBlockNumberInPast.into()) + } + // sanitize maybe_periodic let maybe_periodic = maybe_periodic .filter(|p| p.1 > 1 && !p.0.is_zero()) @@ -343,6 +354,7 @@ impl Module { let address = (when, index); Lookup::::insert(&id, &address); Self::deposit_event(RawEvent::Scheduled(when, index)); + Ok(address) } @@ -366,7 +378,7 @@ impl schedule::Anon::Call> for Module maybe_periodic: Option>, priority: schedule::Priority, call: ::Call - ) -> Self::Address { + ) -> Result { Self::do_schedule(when, maybe_periodic, priority, call) } @@ -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 @@ -551,7 +562,7 @@ mod tests { new_test_ext().execute_with(|| { let call = Call::Logger(logger::Call::log(42, 1000)); assert!(!::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); @@ -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); @@ -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())); @@ -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]); @@ -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]); @@ -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]); }); @@ -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); @@ -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); @@ -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::::TargetBlockNumberInPast, + ); + + assert_err!( + Scheduler::schedule(Origin::root(), 2, None, 127, call2.clone()), + Error::::TargetBlockNumberInPast, + ); + + assert_err!( + Scheduler::schedule(Origin::root(), 3, None, 127, call2), + Error::::TargetBlockNumberInPast, + ); + }); + } } diff --git a/frame/support/src/traits.rs b/frame/support/src/traits.rs index 9a2dbf2b299e8..625f216b1b817 100644 --- a/frame/support/src/traits.rs +++ b/frame/support/src/traits.rs @@ -1501,7 +1501,7 @@ pub mod schedule { maybe_periodic: Option>, priority: Priority, call: Call - ) -> Self::Address; + ) -> Result; /// Cancel a scheduled task. If periodic, then it will cancel all further instances of that, /// also.