From 2ea6bcf1958b6427cd67eb63968b61782f2becc4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?D=C3=B3nal=20Murray?= <donal.murray@parity.io>
Date: Thu, 8 Feb 2024 12:23:36 +0000
Subject: [PATCH] [pallet_broker] Remove leases that have already expired in
 rotate_sale (#3213)

Leases can be force set, but since `Leases` is a `StorageValue`, if a
lease misses its sale rotation in which it should expire, it can never
be cleared.

This can happen if a lease is added with an `until` timeslice that lies
in a region whose sale has already started or has passed, even if the
timeslice itself hasn't passed.

This solves that issue in a minimal way, with all expired leases being
cleaned up in each sale rotation, not just the ones that are expiring in
the coming region.

TODO:
- [x] Write test
---
 substrate/frame/broker/src/tests.rs      | 23 +++++++++++++++++++++++
 substrate/frame/broker/src/tick_impls.rs |  6 ++++--
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/substrate/frame/broker/src/tests.rs b/substrate/frame/broker/src/tests.rs
index e5efb70ae8..3e1e36f7d4 100644
--- a/substrate/frame/broker/src/tests.rs
+++ b/substrate/frame/broker/src/tests.rs
@@ -863,6 +863,29 @@ fn cannot_set_expired_lease() {
 	});
 }
 
+#[test]
+fn short_leases_are_cleaned() {
+	TestExt::new().region_length(3).execute_with(|| {
+		assert_ok!(Broker::do_start_sales(200, 1));
+		advance_to(2);
+
+		// New leases are allowed to expire within this region given expiry > `current_timeslice`.
+		assert_noop!(
+			Broker::do_set_lease(1000, Broker::current_timeslice()),
+			Error::<Test>::AlreadyExpired
+		);
+		assert_eq!(Leases::<Test>::get().len(), 0);
+		assert_ok!(Broker::do_set_lease(1000, Broker::current_timeslice().saturating_add(1)));
+		assert_eq!(Leases::<Test>::get().len(), 1);
+
+		// But are cleaned up in the next rotate_sale.
+		let config = Configuration::<Test>::get().unwrap();
+		let timeslice_period: u64 = <Test as Config>::TimeslicePeriod::get();
+		advance_to(timeslice_period.saturating_mul(config.region_length.into()));
+		assert_eq!(Leases::<Test>::get().len(), 0);
+	});
+}
+
 #[test]
 fn leases_are_limited() {
 	TestExt::new().execute_with(|| {
diff --git a/substrate/frame/broker/src/tick_impls.rs b/substrate/frame/broker/src/tick_impls.rs
index 8b7860c8e3..388370bce4 100644
--- a/substrate/frame/broker/src/tick_impls.rs
+++ b/substrate/frame/broker/src/tick_impls.rs
@@ -216,7 +216,9 @@ impl<T: Config> Pallet<T> {
 			let assignment = CoreAssignment::Task(task);
 			let schedule = BoundedVec::truncate_from(vec![ScheduleItem { mask, assignment }]);
 			Workplan::<T>::insert((region_begin, first_core), &schedule);
-			let expiring = until >= region_begin && until < region_end;
+			// Separate these to avoid missed expired leases hanging around forever.
+			let expired = until < region_end;
+			let expiring = until >= region_begin && expired;
 			if expiring {
 				// last time for this one - make it renewable.
 				let renewal_id = AllowedRenewalId { core: first_core, when: region_end };
@@ -231,7 +233,7 @@ impl<T: Config> Pallet<T> {
 				Self::deposit_event(Event::LeaseEnding { when: region_end, task });
 			}
 			first_core.saturating_inc();
-			!expiring
+			!expired
 		});
 		Leases::<T>::put(&leases);