From 3089dfe9a61bb23dfc80b16e479b5e3bfb608f4a Mon Sep 17 00:00:00 2001 From: muharem <ismailov.m.h@gmail.com> Date: Wed, 21 Dec 2022 09:35:21 +0100 Subject: [PATCH 1/7] Scheduler: remove empty agenda on cancel --- frame/scheduler/src/lib.rs | 12 ++ frame/scheduler/src/tests.rs | 219 +++++++++++++++++++++++------------ 2 files changed, 158 insertions(+), 73 deletions(-) diff --git a/frame/scheduler/src/lib.rs b/frame/scheduler/src/lib.rs index d6a66c5e2cb2c..b8755297ac0d7 100644 --- a/frame/scheduler/src/lib.rs +++ b/frame/scheduler/src/lib.rs @@ -802,6 +802,9 @@ impl<T: Config> Pallet<T> { if let Some(id) = s.maybe_id { Lookup::<T>::remove(id); } + if Agenda::<T>::get(when).iter().position(|i| i.is_some()).is_none() { + Agenda::<T>::remove(when); + } Self::deposit_event(Event::Canceled { when, index }); Ok(()) } else { @@ -824,6 +827,9 @@ impl<T: Config> Pallet<T> { ensure!(!matches!(task, Some(Scheduled { maybe_id: Some(_), .. })), Error::<T>::Named); task.take().ok_or(Error::<T>::NotFound) })?; + if Agenda::<T>::get(when).iter().position(|i| i.is_some()).is_none() { + Agenda::<T>::remove(when); + } Self::deposit_event(Event::Canceled { when, index }); Self::place_task(new_time, task).map_err(|x| x.0) @@ -880,6 +886,9 @@ impl<T: Config> Pallet<T> { } Ok(()) })?; + if Agenda::<T>::get(when).iter().position(|i| i.is_some()).is_none() { + Agenda::<T>::remove(when); + } Self::deposit_event(Event::Canceled { when, index }); Ok(()) } else { @@ -905,6 +914,9 @@ impl<T: Config> Pallet<T> { let task = agenda.get_mut(index as usize).ok_or(Error::<T>::NotFound)?; task.take().ok_or(Error::<T>::NotFound) })?; + if Agenda::<T>::get(when).iter().position(|i| i.is_some()).is_none() { + Agenda::<T>::remove(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/tests.rs b/frame/scheduler/src/tests.rs index 033d787946709..394af7a7a0d75 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 = <Scheduler as Anon<_, _, _>>::schedule( - DispatchTime::At(4), - None, - 127, - root(), - Preimage::bound(call.clone()).unwrap(), - ) - .unwrap(); - // Cancel the call. - assert_ok!(<Scheduler as Anon<_, _, _>>::cancel(addr_1)); - let addr_2 = <Scheduler as Anon<_, _, _>>::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 = <Test as Config>::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 = <Scheduler as Anon<_, _, _>>::schedule( - DispatchTime::At(4), - None, - 127, - root(), - Preimage::bound(call.clone()).unwrap(), - ) - .unwrap(); - // Cancel the call. - assert_ok!(<Scheduler as Anon<_, _, _>>::cancel(addr_1)); - let addr_2 = <Scheduler as Anon<_, _, _>>::schedule( - DispatchTime::At(5), - None, - 127, - root(), - Preimage::bound(call).unwrap(), - ) - .unwrap(); - // Re-schedule `call` to block 4. - let addr_3 = <Scheduler as Anon<_, _, _>>::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::<Test>::get(when).len() == 2); + assert_ok!(Scheduler::do_cancel(None, address)); + // still two tasks at agenda, `None` and `Some`. + assert!(Agenda::<Test>::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::<Test>::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::<Test>::get(when).len() == 2); + assert_ok!(Scheduler::do_cancel_named(None, [1u8; 32])); + // still two tasks at agenda, `None` and `Some`. + assert!(Agenda::<Test>::get(when).len() == 2); + // cancel last task from `when` agenda. + assert_ok!(Scheduler::do_cancel_named(None, [2u8; 32])); + // if all tasks `None`, agenda fully removed. + assert!(Agenda::<Test>::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::<Test>::get(when).len() == 2); + assert_ok!(Scheduler::do_cancel(None, address)); + // still two tasks at agenda, `None` and `Some`. + assert!(Agenda::<Test>::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::<Test>::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::<Test>::get(when).len() == 2); + assert_ok!(Scheduler::do_cancel_named(None, [1u8; 32])); + // still two tasks at agenda, `None` and `Some`. + assert!(Agenda::<Test>::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::<Test>::get(when).len() == 0); + }); +} From 4d6029b805442559ef911a44c6808d25ec740fee Mon Sep 17 00:00:00 2001 From: muharem <ismailov.m.h@gmail.com> Date: Wed, 21 Dec 2022 10:03:55 +0100 Subject: [PATCH 2/7] use iter any --- frame/scheduler/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frame/scheduler/src/lib.rs b/frame/scheduler/src/lib.rs index b8755297ac0d7..711370f95a9bf 100644 --- a/frame/scheduler/src/lib.rs +++ b/frame/scheduler/src/lib.rs @@ -802,7 +802,7 @@ impl<T: Config> Pallet<T> { if let Some(id) = s.maybe_id { Lookup::<T>::remove(id); } - if Agenda::<T>::get(when).iter().position(|i| i.is_some()).is_none() { + if !Agenda::<T>::get(when).iter().any(|i| i.is_some()) { Agenda::<T>::remove(when); } Self::deposit_event(Event::Canceled { when, index }); @@ -827,7 +827,7 @@ impl<T: Config> Pallet<T> { ensure!(!matches!(task, Some(Scheduled { maybe_id: Some(_), .. })), Error::<T>::Named); task.take().ok_or(Error::<T>::NotFound) })?; - if Agenda::<T>::get(when).iter().position(|i| i.is_some()).is_none() { + if !Agenda::<T>::get(when).iter().any(|i| i.is_some()) { Agenda::<T>::remove(when); } Self::deposit_event(Event::Canceled { when, index }); @@ -886,7 +886,7 @@ impl<T: Config> Pallet<T> { } Ok(()) })?; - if Agenda::<T>::get(when).iter().position(|i| i.is_some()).is_none() { + if !Agenda::<T>::get(when).iter().any(|i| i.is_some()) { Agenda::<T>::remove(when); } Self::deposit_event(Event::Canceled { when, index }); @@ -914,7 +914,7 @@ impl<T: Config> Pallet<T> { let task = agenda.get_mut(index as usize).ok_or(Error::<T>::NotFound)?; task.take().ok_or(Error::<T>::NotFound) })?; - if Agenda::<T>::get(when).iter().position(|i| i.is_some()).is_none() { + if !Agenda::<T>::get(when).iter().any(|i| i.is_some()) { Agenda::<T>::remove(when); } Self::deposit_event(Event::Canceled { when, index }); From b034bc7e8b731cfd10ab6b86e2e35ab042666aa5 Mon Sep 17 00:00:00 2001 From: muharem <ismailov.m.h@gmail.com> Date: Wed, 21 Dec 2022 10:31:25 +0100 Subject: [PATCH 3/7] fix benches --- frame/scheduler/src/benchmarking.rs | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) 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! { }: _<SystemOrigin<T>>(schedule_origin, when, 0) verify { ensure!( - Lookup::<T>::get(u32_to_name(0)).is_none(), - "didn't remove from lookup" + s == 1 || Lookup::<T>::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::<T>::get(when)[0].is_none(), - "didn't remove from schedule" + s == 1 || Agenda::<T>::get(when)[0].is_none(), + "didn't remove from schedule if more than 1 task scheduled for `when`" + ); + ensure!( + s > 1 || Agenda::<T>::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::<T>::get(u32_to_name(0)).is_none(), - "didn't remove from lookup" + s == 1 || Lookup::<T>::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::<T>::get(when)[0].is_none(), - "didn't remove from schedule" + s == 1 || Agenda::<T>::get(when)[0].is_none(), + "didn't remove from schedule if more than 1 task scheduled for `when`" + ); + ensure!( + s > 1 || Agenda::<T>::get(when).len() == 0, + "remove from schedule if only 1 task scheduled for `when`" ); } From 31cc83b54ac06ee46283015276d1ac42d7f823b6 Mon Sep 17 00:00:00 2001 From: muharem <ismailov.m.h@gmail.com> Date: Wed, 21 Dec 2022 13:46:41 +0100 Subject: [PATCH 4/7] remove trailing None --- frame/scheduler/src/lib.rs | 32 ++++++++++++++++++++------------ frame/scheduler/src/tests.rs | 8 ++++---- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/frame/scheduler/src/lib.rs b/frame/scheduler/src/lib.rs index 711370f95a9bf..20e14206767aa 100644 --- a/frame/scheduler/src/lib.rs +++ b/frame/scheduler/src/lib.rs @@ -752,6 +752,22 @@ impl<T: Config> Pallet<T> { 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::<T>::get(when); + match agenda.iter().rposition(|i| i.is_some()) { + Some(i) if agenda.len() > i + 1 => { + agenda.truncate(i + 1); + Agenda::<T>::insert(when, agenda); + }, + Some(_) => {}, + None => { + Agenda::<T>::remove(when); + }, + } + } + fn do_schedule( when: DispatchTime<T::BlockNumber>, maybe_periodic: Option<schedule::Period<T::BlockNumber>>, @@ -802,9 +818,7 @@ impl<T: Config> Pallet<T> { if let Some(id) = s.maybe_id { Lookup::<T>::remove(id); } - if !Agenda::<T>::get(when).iter().any(|i| i.is_some()) { - Agenda::<T>::remove(when); - } + Self::cleanup_agenda(when); Self::deposit_event(Event::Canceled { when, index }); Ok(()) } else { @@ -827,9 +841,7 @@ impl<T: Config> Pallet<T> { ensure!(!matches!(task, Some(Scheduled { maybe_id: Some(_), .. })), Error::<T>::Named); task.take().ok_or(Error::<T>::NotFound) })?; - if !Agenda::<T>::get(when).iter().any(|i| i.is_some()) { - Agenda::<T>::remove(when); - } + Self::cleanup_agenda(when); Self::deposit_event(Event::Canceled { when, index }); Self::place_task(new_time, task).map_err(|x| x.0) @@ -886,9 +898,7 @@ impl<T: Config> Pallet<T> { } Ok(()) })?; - if !Agenda::<T>::get(when).iter().any(|i| i.is_some()) { - Agenda::<T>::remove(when); - } + Self::cleanup_agenda(when); Self::deposit_event(Event::Canceled { when, index }); Ok(()) } else { @@ -914,9 +924,7 @@ impl<T: Config> Pallet<T> { let task = agenda.get_mut(index as usize).ok_or(Error::<T>::NotFound)?; task.take().ok_or(Error::<T>::NotFound) })?; - if !Agenda::<T>::get(when).iter().any(|i| i.is_some()) { - Agenda::<T>::remove(when); - } + 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/tests.rs b/frame/scheduler/src/tests.rs index 394af7a7a0d75..7c261fdd74bfd 100644 --- a/frame/scheduler/src/tests.rs +++ b/frame/scheduler/src/tests.rs @@ -1755,11 +1755,11 @@ fn cancel_named_last_task_removes_agenda() { .unwrap(); // two tasks at agenda. assert!(Agenda::<Test>::get(when).len() == 2); - assert_ok!(Scheduler::do_cancel_named(None, [1u8; 32])); - // still two tasks at agenda, `None` and `Some`. - assert!(Agenda::<Test>::get(when).len() == 2); - // cancel last task from `when` agenda. assert_ok!(Scheduler::do_cancel_named(None, [2u8; 32])); + // removes trailing `None` and leaves one task. + assert!(Agenda::<Test>::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::<Test>::get(when).len() == 0); }); From d87136b9b00c489d00f8ea040421a4220ea2839b Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> Date: Wed, 21 Dec 2022 13:56:18 +0100 Subject: [PATCH 5/7] Add CleanupAgendas migration Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> --- frame/scheduler/src/migration.rs | 163 +++++++++++++++++++++++++++++++ 1 file changed, 163 insertions(+) diff --git a/frame/scheduler/src/migration.rs b/frame/scheduler/src/migration.rs index 6769d20023196..931982ffcfe5c 100644 --- a/frame/scheduler/src/migration.rs +++ b/frame/scheduler/src/migration.rs @@ -198,6 +198,111 @@ 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 + /// <https://github.com/paritytech/substrate/pull/12989> since it piles up `None`-only agendas. This does not modify the pallet version. + pub struct CleanupAgendas<T>(sp_std::marker::PhantomData<T>); + + impl<T: Config> OnRuntimeUpgrade for CleanupAgendas<T> { + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result<Vec<u8>, &'static str> { + assert_eq!( + StorageVersion::get::<Pallet<T>>(), + 4, + "Can only cleanup agendas of the V4 scheduler" + ); + + let agendas = Agenda::<T>::iter_keys().count(); + log::info!(target: TARGET, "Checking {} agendas...", agendas); + + Ok((agendas as u32).encode()) + } + + fn on_runtime_upgrade() -> Weight { + let version = StorageVersion::get::<Pallet<T>>(); + 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::<T>::iter_keys().collect::<Vec<_>>(); + let mut writes = 0; + for k in &keys { + let mut schedules = Agenda::<T>::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::<T>::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::<T>::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<u8>) -> Result<(), &'static str> { + assert_eq!(StorageVersion::get::<Pallet<T>>(), 4, "Version must not change"); + + let old_agendas: u32 = + Decode::decode(&mut state.as_ref()).expect("Must decode pre_upgrade state"); + let new_agendas = Agenda::<T>::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 + ), + } + + Ok(()) + } + } +} + #[cfg(test)] #[cfg(feature = "try-runtime")] mod test { @@ -396,6 +501,64 @@ mod test { }); } + #[test] + fn cleanup_agendas_works() { + use sp_core::bounded_vec; + new_test_ext().execute_with(|| { + StorageVersion::new(4).put::<Scheduler>(); + + let call = RuntimeCall::System(frame_system::Call::remark { remark: vec![] }); + let bounded_call = Preimage::bound(call).unwrap(); + let some = Some(ScheduledOf::<Test> { + 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<Option<ScheduledOf<Test>>, <Test as Config>::MaxScheduledPerBlock>, + Option< + BoundedVec<Option<ScheduledOf<Test>>, <Test as Config>::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::<Test>::insert(i as u64, test.0.clone()); + } + + // Run the migration. + let data = v4::CleanupAgendas::<Test>::pre_upgrade().unwrap(); + let _w = v4::CleanupAgendas::<Test>::on_runtime_upgrade(); + v4::CleanupAgendas::<Test>::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::<Test>::contains_key(i as u64), + "Agenda {} should be removed", + i + ), + Some(new) => + assert_eq!(Agenda::<Test>::get(i as u64), new, "Agenda wrong {}", i), + } + } + }); + } + fn signed(i: u64) -> OriginCaller { system::RawOrigin::Signed(i).into() } From de33d9035e38116050125d34b5ae6b222880498c Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> Date: Wed, 21 Dec 2022 15:09:08 +0100 Subject: [PATCH 6/7] fix ci Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> --- frame/scheduler/src/migration.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/scheduler/src/migration.rs b/frame/scheduler/src/migration.rs index 931982ffcfe5c..e09c95774dd6c 100644 --- a/frame/scheduler/src/migration.rs +++ b/frame/scheduler/src/migration.rs @@ -241,7 +241,7 @@ mod v4 { match all_schedules.checked_sub(suffix_none_schedules) { Some(0) => { log::info!( - "Deleting None-only agenda {} with {} entries", + "Deleting None-only agenda {:?} with {} entries", k, all_schedules ); @@ -250,7 +250,7 @@ mod v4 { }, Some(ne) if ne > 0 => { log::info!( - "Removing {} schedules of {} from agenda {:?}, now {}", + "Removing {} schedules of {} from agenda {:?}, now {:?}", suffix_none_schedules, all_schedules, ne, @@ -267,7 +267,7 @@ mod v4 { ); }, None => { - log::info!("Agenda {} does not have any None suffix schedules", k); + log::info!("Agenda {:?} does not have any None suffix schedules", k); }, } } From aee7ed438ce5d31c341f366e5073d47327348142 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> Date: Wed, 21 Dec 2022 15:32:17 +0100 Subject: [PATCH 7/7] Count non-empty agendas in migration Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> --- frame/scheduler/src/migration.rs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/frame/scheduler/src/migration.rs b/frame/scheduler/src/migration.rs index e09c95774dd6c..97a5ef4c3e883 100644 --- a/frame/scheduler/src/migration.rs +++ b/frame/scheduler/src/migration.rs @@ -218,9 +218,16 @@ mod v4 { ); let agendas = Agenda::<T>::iter_keys().count(); - log::info!(target: TARGET, "Checking {} agendas...", agendas); + let non_empty_agendas = + Agenda::<T>::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).encode()) + Ok((agendas as u32, non_empty_agendas as u32).encode()) } fn on_runtime_upgrade() -> Weight { @@ -281,7 +288,7 @@ mod v4 { fn post_upgrade(state: Vec<u8>) -> Result<(), &'static str> { assert_eq!(StorageVersion::get::<Pallet<T>>(), 4, "Version must not change"); - let old_agendas: u32 = + let (old_agendas, non_empty_agendas): (u32, u32) = Decode::decode(&mut state.as_ref()).expect("Must decode pre_upgrade state"); let new_agendas = Agenda::<T>::iter_keys().count() as u32; @@ -297,6 +304,7 @@ mod v4 { old_agendas, new_agendas ), } + assert_eq!(new_agendas, non_empty_agendas, "Expected to keep all non-empty agendas"); Ok(()) }