diff --git a/frame/scheduler/src/benchmarking.rs b/frame/scheduler/src/benchmarking.rs index e621c913b2386..ca98cabc3c08b 100644 --- a/frame/scheduler/src/benchmarking.rs +++ b/frame/scheduler/src/benchmarking.rs @@ -244,13 +244,17 @@ benchmarks! { }: _>(schedule_origin, when, 0) verify { ensure!( - Lookup::::get(u32_to_name(0)).is_none(), - "didn't remove from lookup" + s == 1 || Lookup::::get(u32_to_name(0)).is_none(), + "didn't remove from lookup if more than 1 task scheduled for `when`" ); // Removed schedule is NONE ensure!( - Agenda::::get(when)[0].is_none(), - "didn't remove from schedule" + s == 1 || Agenda::::get(when)[0].is_none(), + "didn't remove from schedule if more than 1 task scheduled for `when`" + ); + ensure!( + s > 1 || Agenda::::get(when).len() == 0, + "remove from schedule if only 1 task scheduled for `when`" ); } @@ -280,13 +284,17 @@ benchmarks! { }: _(RawOrigin::Root, u32_to_name(0)) verify { ensure!( - Lookup::::get(u32_to_name(0)).is_none(), - "didn't remove from lookup" + s == 1 || Lookup::::get(u32_to_name(0)).is_none(), + "didn't remove from lookup if more than 1 task scheduled for `when`" ); // Removed schedule is NONE ensure!( - Agenda::::get(when)[0].is_none(), - "didn't remove from schedule" + s == 1 || Agenda::::get(when)[0].is_none(), + "didn't remove from schedule if more than 1 task scheduled for `when`" + ); + ensure!( + s > 1 || Agenda::::get(when).len() == 0, + "remove from schedule if only 1 task scheduled for `when`" ); } diff --git a/frame/scheduler/src/lib.rs b/frame/scheduler/src/lib.rs index d6a66c5e2cb2c..20e14206767aa 100644 --- a/frame/scheduler/src/lib.rs +++ b/frame/scheduler/src/lib.rs @@ -752,6 +752,22 @@ impl Pallet { Ok(index) } + /// Remove trailing `None` items of an agenda at `when`. If all items are `None` remove the + /// agenda record entirely. + fn cleanup_agenda(when: T::BlockNumber) { + let mut agenda = Agenda::::get(when); + match agenda.iter().rposition(|i| i.is_some()) { + Some(i) if agenda.len() > i + 1 => { + agenda.truncate(i + 1); + Agenda::::insert(when, agenda); + }, + Some(_) => {}, + None => { + Agenda::::remove(when); + }, + } + } + fn do_schedule( when: DispatchTime, maybe_periodic: Option>, @@ -802,6 +818,7 @@ impl Pallet { if let Some(id) = s.maybe_id { Lookup::::remove(id); } + Self::cleanup_agenda(when); Self::deposit_event(Event::Canceled { when, index }); Ok(()) } else { @@ -824,6 +841,7 @@ impl Pallet { ensure!(!matches!(task, Some(Scheduled { maybe_id: Some(_), .. })), Error::::Named); task.take().ok_or(Error::::NotFound) })?; + Self::cleanup_agenda(when); Self::deposit_event(Event::Canceled { when, index }); Self::place_task(new_time, task).map_err(|x| x.0) @@ -880,6 +898,7 @@ impl Pallet { } Ok(()) })?; + Self::cleanup_agenda(when); Self::deposit_event(Event::Canceled { when, index }); Ok(()) } else { @@ -905,6 +924,7 @@ impl Pallet { let task = agenda.get_mut(index as usize).ok_or(Error::::NotFound)?; task.take().ok_or(Error::::NotFound) })?; + Self::cleanup_agenda(when); Self::deposit_event(Event::Canceled { when, index }); Self::place_task(new_time, task).map_err(|x| x.0) } diff --git a/frame/scheduler/src/migration.rs b/frame/scheduler/src/migration.rs index 6769d20023196..97a5ef4c3e883 100644 --- a/frame/scheduler/src/migration.rs +++ b/frame/scheduler/src/migration.rs @@ -198,6 +198,119 @@ pub mod v3 { } } +mod v4 { + use super::*; + use frame_support::pallet_prelude::*; + + /// This migration cleans up empty agendas of the V4 scheduler. + /// + /// This should be run on a scheduler that does not have + /// since it piles up `None`-only agendas. This does not modify the pallet version. + pub struct CleanupAgendas(sp_std::marker::PhantomData); + + impl OnRuntimeUpgrade for CleanupAgendas { + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, &'static str> { + assert_eq!( + StorageVersion::get::>(), + 4, + "Can only cleanup agendas of the V4 scheduler" + ); + + let agendas = Agenda::::iter_keys().count(); + let non_empty_agendas = + Agenda::::iter_values().filter(|a| a.iter().any(|s| s.is_some())).count(); + log::info!( + target: TARGET, + "There are {} total and {} non-empty agendas", + agendas, + non_empty_agendas + ); + + Ok((agendas as u32, non_empty_agendas as u32).encode()) + } + + fn on_runtime_upgrade() -> Weight { + let version = StorageVersion::get::>(); + if version != 4 { + log::warn!(target: TARGET, "Skipping CleanupAgendas migration since it was run on the wrong version: {:?} != 4", version); + return T::DbWeight::get().reads(1) + } + + let keys = Agenda::::iter_keys().collect::>(); + let mut writes = 0; + for k in &keys { + let mut schedules = Agenda::::get(k); + let all_schedules = schedules.len(); + let suffix_none_schedules = + schedules.iter().rev().take_while(|s| s.is_none()).count(); + + match all_schedules.checked_sub(suffix_none_schedules) { + Some(0) => { + log::info!( + "Deleting None-only agenda {:?} with {} entries", + k, + all_schedules + ); + Agenda::::remove(k); + writes.saturating_inc(); + }, + Some(ne) if ne > 0 => { + log::info!( + "Removing {} schedules of {} from agenda {:?}, now {:?}", + suffix_none_schedules, + all_schedules, + ne, + k + ); + schedules.truncate(ne); + Agenda::::insert(k, schedules); + writes.saturating_inc(); + }, + Some(_) => { + frame_support::defensive!( + // Bad but let's not panic. + "Cannot have more None suffix schedules that schedules in total" + ); + }, + None => { + log::info!("Agenda {:?} does not have any None suffix schedules", k); + }, + } + } + + // We don't modify the pallet version. + + T::DbWeight::get().reads_writes(1 + keys.len().saturating_mul(2) as u64, writes) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(state: Vec) -> Result<(), &'static str> { + assert_eq!(StorageVersion::get::>(), 4, "Version must not change"); + + let (old_agendas, non_empty_agendas): (u32, u32) = + Decode::decode(&mut state.as_ref()).expect("Must decode pre_upgrade state"); + let new_agendas = Agenda::::iter_keys().count() as u32; + + match old_agendas.checked_sub(new_agendas) { + Some(0) => log::warn!( + target: TARGET, + "Did not clean up any agendas. v4::CleanupAgendas can be removed." + ), + Some(n) => + log::info!(target: TARGET, "Cleaned up {} agendas, now {}", n, new_agendas), + None => unreachable!( + "Number of agendas cannot increase, old {} new {}", + old_agendas, new_agendas + ), + } + assert_eq!(new_agendas, non_empty_agendas, "Expected to keep all non-empty agendas"); + + Ok(()) + } + } +} + #[cfg(test)] #[cfg(feature = "try-runtime")] mod test { @@ -396,6 +509,64 @@ mod test { }); } + #[test] + fn cleanup_agendas_works() { + use sp_core::bounded_vec; + new_test_ext().execute_with(|| { + StorageVersion::new(4).put::(); + + let call = RuntimeCall::System(frame_system::Call::remark { remark: vec![] }); + let bounded_call = Preimage::bound(call).unwrap(); + let some = Some(ScheduledOf:: { + maybe_id: None, + priority: 1, + call: bounded_call, + maybe_periodic: None, + origin: root(), + _phantom: Default::default(), + }); + + // Put some empty, and some non-empty agendas in there. + let test_data: Vec<( + BoundedVec>, ::MaxScheduledPerBlock>, + Option< + BoundedVec>, ::MaxScheduledPerBlock>, + >, + )> = vec![ + (bounded_vec![some.clone()], Some(bounded_vec![some.clone()])), + (bounded_vec![None, some.clone()], Some(bounded_vec![None, some.clone()])), + (bounded_vec![None, some.clone(), None], Some(bounded_vec![None, some.clone()])), + (bounded_vec![some.clone(), None, None], Some(bounded_vec![some.clone()])), + (bounded_vec![None, None], None), + (bounded_vec![None, None, None], None), + (bounded_vec![], None), + ]; + + // Insert all the agendas. + for (i, test) in test_data.iter().enumerate() { + Agenda::::insert(i as u64, test.0.clone()); + } + + // Run the migration. + let data = v4::CleanupAgendas::::pre_upgrade().unwrap(); + let _w = v4::CleanupAgendas::::on_runtime_upgrade(); + v4::CleanupAgendas::::post_upgrade(data).unwrap(); + + // Check that the post-state is correct. + for (i, test) in test_data.iter().enumerate() { + match test.1.clone() { + None => assert!( + !Agenda::::contains_key(i as u64), + "Agenda {} should be removed", + i + ), + Some(new) => + assert_eq!(Agenda::::get(i as u64), new, "Agenda wrong {}", i), + } + } + }); + } + fn signed(i: u64) -> OriginCaller { system::RawOrigin::Signed(i).into() } diff --git a/frame/scheduler/src/tests.rs b/frame/scheduler/src/tests.rs index 033d787946709..7c261fdd74bfd 100644 --- a/frame/scheduler/src/tests.rs +++ b/frame/scheduler/src/tests.rs @@ -1291,40 +1291,6 @@ fn scheduler_v3_anon_reschedule_works() { }); } -/// Cancelling a call and then scheduling a second call for the same -/// block results in different addresses. -#[test] -fn scheduler_v3_anon_schedule_does_not_resuse_addr() { - use frame_support::traits::schedule::v3::Anon; - new_test_ext().execute_with(|| { - let call = - RuntimeCall::Logger(LoggerCall::log { i: 42, weight: Weight::from_ref_time(10) }); - - // Schedule both calls. - let addr_1 = >::schedule( - DispatchTime::At(4), - None, - 127, - root(), - Preimage::bound(call.clone()).unwrap(), - ) - .unwrap(); - // Cancel the call. - assert_ok!(>::cancel(addr_1)); - let addr_2 = >::schedule( - DispatchTime::At(4), - None, - 127, - root(), - Preimage::bound(call).unwrap(), - ) - .unwrap(); - - // Should not re-use the address. - assert!(addr_1 != addr_2); - }); -} - #[test] fn scheduler_v3_anon_next_schedule_time_works() { use frame_support::traits::schedule::v3::Anon; @@ -1531,45 +1497,6 @@ fn scheduler_v3_anon_reschedule_fills_holes() { }); } -/// Re-scheduling into the same block produces a different address -/// if there is still space in the agenda. -#[test] -fn scheduler_v3_anon_reschedule_does_not_resuse_addr_if_agenda_not_full() { - use frame_support::traits::schedule::v3::Anon; - let max: u32 = ::MaxScheduledPerBlock::get(); - assert!(max > 1, "This test only makes sense for MaxScheduledPerBlock > 1"); - - new_test_ext().execute_with(|| { - let call = - RuntimeCall::Logger(LoggerCall::log { i: 42, weight: Weight::from_ref_time(10) }); - - // Schedule both calls. - let addr_1 = >::schedule( - DispatchTime::At(4), - None, - 127, - root(), - Preimage::bound(call.clone()).unwrap(), - ) - .unwrap(); - // Cancel the call. - assert_ok!(>::cancel(addr_1)); - let addr_2 = >::schedule( - DispatchTime::At(5), - None, - 127, - root(), - Preimage::bound(call).unwrap(), - ) - .unwrap(); - // Re-schedule `call` to block 4. - let addr_3 = >::reschedule(addr_2, DispatchTime::At(4)).unwrap(); - - // Should not re-use the address. - assert!(addr_1 != addr_3); - }); -} - /// The scheduler can be used as `v3::Named` trait. #[test] fn scheduler_v3_named_basic_works() { @@ -1767,3 +1694,149 @@ fn scheduler_v3_named_next_schedule_time_works() { ); }); } + +#[test] +fn cancel_last_task_removes_agenda() { + new_test_ext().execute_with(|| { + let when = 4; + let call = + RuntimeCall::Logger(LoggerCall::log { i: 42, weight: Weight::from_ref_time(10) }); + let address = Scheduler::do_schedule( + DispatchTime::At(when), + None, + 127, + root(), + Preimage::bound(call.clone()).unwrap(), + ) + .unwrap(); + let address2 = Scheduler::do_schedule( + DispatchTime::At(when), + None, + 127, + root(), + Preimage::bound(call).unwrap(), + ) + .unwrap(); + // two tasks at agenda. + assert!(Agenda::::get(when).len() == 2); + assert_ok!(Scheduler::do_cancel(None, address)); + // still two tasks at agenda, `None` and `Some`. + assert!(Agenda::::get(when).len() == 2); + // cancel last task from `when` agenda. + assert_ok!(Scheduler::do_cancel(None, address2)); + // if all tasks `None`, agenda fully removed. + assert!(Agenda::::get(when).len() == 0); + }); +} + +#[test] +fn cancel_named_last_task_removes_agenda() { + new_test_ext().execute_with(|| { + let when = 4; + let call = + RuntimeCall::Logger(LoggerCall::log { i: 42, weight: Weight::from_ref_time(10) }); + Scheduler::do_schedule_named( + [1u8; 32], + DispatchTime::At(when), + None, + 127, + root(), + Preimage::bound(call.clone()).unwrap(), + ) + .unwrap(); + Scheduler::do_schedule_named( + [2u8; 32], + DispatchTime::At(when), + None, + 127, + root(), + Preimage::bound(call).unwrap(), + ) + .unwrap(); + // two tasks at agenda. + assert!(Agenda::::get(when).len() == 2); + assert_ok!(Scheduler::do_cancel_named(None, [2u8; 32])); + // removes trailing `None` and leaves one task. + assert!(Agenda::::get(when).len() == 1); + // cancel last task from `when` agenda. + assert_ok!(Scheduler::do_cancel_named(None, [1u8; 32])); + // if all tasks `None`, agenda fully removed. + assert!(Agenda::::get(when).len() == 0); + }); +} + +#[test] +fn reschedule_last_task_removes_agenda() { + new_test_ext().execute_with(|| { + let when = 4; + let call = + RuntimeCall::Logger(LoggerCall::log { i: 42, weight: Weight::from_ref_time(10) }); + let address = Scheduler::do_schedule( + DispatchTime::At(when), + None, + 127, + root(), + Preimage::bound(call.clone()).unwrap(), + ) + .unwrap(); + let address2 = Scheduler::do_schedule( + DispatchTime::At(when), + None, + 127, + root(), + Preimage::bound(call).unwrap(), + ) + .unwrap(); + // two tasks at agenda. + assert!(Agenda::::get(when).len() == 2); + assert_ok!(Scheduler::do_cancel(None, address)); + // still two tasks at agenda, `None` and `Some`. + assert!(Agenda::::get(when).len() == 2); + // reschedule last task from `when` agenda. + assert_eq!( + Scheduler::do_reschedule(address2, DispatchTime::At(when + 1)).unwrap(), + (when + 1, 0) + ); + // if all tasks `None`, agenda fully removed. + assert!(Agenda::::get(when).len() == 0); + }); +} + +#[test] +fn reschedule_named_last_task_removes_agenda() { + new_test_ext().execute_with(|| { + let when = 4; + let call = + RuntimeCall::Logger(LoggerCall::log { i: 42, weight: Weight::from_ref_time(10) }); + Scheduler::do_schedule_named( + [1u8; 32], + DispatchTime::At(when), + None, + 127, + root(), + Preimage::bound(call.clone()).unwrap(), + ) + .unwrap(); + Scheduler::do_schedule_named( + [2u8; 32], + DispatchTime::At(when), + None, + 127, + root(), + Preimage::bound(call).unwrap(), + ) + .unwrap(); + // two tasks at agenda. + assert!(Agenda::::get(when).len() == 2); + assert_ok!(Scheduler::do_cancel_named(None, [1u8; 32])); + // still two tasks at agenda, `None` and `Some`. + assert!(Agenda::::get(when).len() == 2); + // reschedule last task from `when` agenda. + assert_eq!( + Scheduler::do_reschedule_named([2u8; 32], DispatchTime::At(when + 1)).unwrap(), + (when + 1, 0) + ); + // if all tasks `None`, agenda fully removed. + assert!(Agenda::::get(when).len() == 0); + }); +}