diff --git a/Cargo.lock b/Cargo.lock
index 27cb7af04d63390df820dc87ca7922157bfb32ff..2339aff34bcd49dffa02d3b23594e8d84a5e82a0 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -9937,6 +9937,7 @@ dependencies = [
"frame-support",
"frame-system",
"parity-scale-codec",
+ "pretty_assertions",
"scale-info",
"sp-api",
"sp-arithmetic",
diff --git a/prdoc/pr_4089.prdoc b/prdoc/pr_4089.prdoc
new file mode 100644
index 0000000000000000000000000000000000000000..29ac736ccd082ec493b89824fab0f9519bd66c6d
--- /dev/null
+++ b/prdoc/pr_4089.prdoc
@@ -0,0 +1,11 @@
+title: "pallet_broker: Support renewing leases expired in a previous period"
+
+doc:
+ - audience: Runtime User
+ description: |
+ Allow renewals of leases that ended before `start_sales` or in the first period after calling `start_sales`.
+ This ensures that everyone has a smooth experience when migrating to coretime and the timing of
+ `start_sales` isn't that important.
+
+crates:
+ - name: pallet-broker
diff --git a/substrate/frame/broker/Cargo.toml b/substrate/frame/broker/Cargo.toml
index 969f13e269de4208a220150d688a21ffcc0191f1..f36b94c3cec567ebe25a2194876598e5a3b92a56 100644
--- a/substrate/frame/broker/Cargo.toml
+++ b/substrate/frame/broker/Cargo.toml
@@ -29,6 +29,7 @@ frame-system = { path = "../system", default-features = false }
[dev-dependencies]
sp-io = { path = "../../primitives/io" }
+pretty_assertions = "1.3.0"
[features]
default = ["std"]
diff --git a/substrate/frame/broker/src/mock.rs b/substrate/frame/broker/src/mock.rs
index c7205058c9720b15838de3684a33303a45cb8c68..6219b4eff1b457294c9f3656e4be145c3639e2e0 100644
--- a/substrate/frame/broker/src/mock.rs
+++ b/substrate/frame/broker/src/mock.rs
@@ -29,7 +29,7 @@ use frame_support::{
};
use frame_system::{EnsureRoot, EnsureSignedBy};
use sp_arithmetic::Perbill;
-use sp_core::{ConstU32, ConstU64};
+use sp_core::{ConstU32, ConstU64, Get};
use sp_runtime::{
traits::{BlockNumberProvider, Identity},
BuildStorage, Saturating,
@@ -210,6 +210,15 @@ pub fn advance_to(b: u64) {
}
}
+pub fn advance_sale_period() {
+ let sale = SaleInfo::<Test>::get().unwrap();
+
+ let target_block_number =
+ sale.region_begin as u64 * <<Test as crate::Config>::TimeslicePeriod as Get<u64>>::get();
+
+ advance_to(target_block_number)
+}
+
pub fn pot() -> u64 {
balance(Broker::account_id())
}
diff --git a/substrate/frame/broker/src/tests.rs b/substrate/frame/broker/src/tests.rs
index 0aacd7ef3696c600ed9546d66fc84fb851553ee6..7fc7e31e9bb6e3e4141422393944bc2b6fd5b0ee 100644
--- a/substrate/frame/broker/src/tests.rs
+++ b/substrate/frame/broker/src/tests.rs
@@ -24,6 +24,7 @@ use frame_support::{
BoundedVec,
};
use frame_system::RawOrigin::Root;
+use pretty_assertions::assert_eq;
use sp_runtime::{traits::Get, TokenError};
use CoreAssignment::*;
use CoretimeTraceItem::*;
@@ -1092,3 +1093,141 @@ fn config_works() {
assert_noop!(Broker::configure(Root.into(), cfg), Error::<Test>::InvalidConfig);
});
}
+
+/// Ensure that a lease that ended before `start_sales` was called can be renewed.
+#[test]
+fn renewal_works_leases_ended_before_start_sales() {
+ TestExt::new().endow(1, 1000).execute_with(|| {
+ let config = Configuration::<Test>::get().unwrap();
+
+ // This lease is ended before `start_stales` was called.
+ assert_ok!(Broker::do_set_lease(1, 1));
+
+ // Go to some block to ensure that the lease of task 1 already ended.
+ advance_to(5);
+
+ // This lease will end three sale periods in.
+ assert_ok!(Broker::do_set_lease(
+ 2,
+ Broker::latest_timeslice_ready_to_commit(&config) + config.region_length * 3
+ ));
+
+ // This intializes the first sale and the period 0.
+ assert_ok!(Broker::do_start_sales(100, 2));
+ assert_noop!(Broker::do_renew(1, 1), Error::<Test>::Unavailable);
+ assert_noop!(Broker::do_renew(1, 0), Error::<Test>::Unavailable);
+
+ // Lease for task 1 should have been dropped.
+ assert!(Leases::<Test>::get().iter().any(|l| l.task == 2));
+
+ // This intializes the second and the period 1.
+ advance_sale_period();
+
+ // Now we can finally renew the core 0 of task 1.
+ let new_core = Broker::do_renew(1, 0).unwrap();
+ // Renewing the active lease doesn't work.
+ assert_noop!(Broker::do_renew(1, 1), Error::<Test>::SoldOut);
+ assert_eq!(balance(1), 900);
+
+ // This intializes the third sale and the period 2.
+ advance_sale_period();
+ let new_core = Broker::do_renew(1, new_core).unwrap();
+
+ // Renewing the active lease doesn't work.
+ assert_noop!(Broker::do_renew(1, 0), Error::<Test>::SoldOut);
+ assert_eq!(balance(1), 800);
+
+ // All leases should have ended
+ assert!(Leases::<Test>::get().is_empty());
+
+ // This intializes the fourth sale and the period 3.
+ advance_sale_period();
+
+ // Renew again
+ assert_eq!(0, Broker::do_renew(1, new_core).unwrap());
+ // Renew the task 2.
+ assert_eq!(1, Broker::do_renew(1, 0).unwrap());
+ assert_eq!(balance(1), 600);
+
+ // This intializes the fifth sale and the period 4.
+ advance_sale_period();
+
+ assert_eq!(
+ CoretimeTrace::get(),
+ vec![
+ (
+ 10,
+ AssignCore {
+ core: 0,
+ begin: 12,
+ assignment: vec![(Task(1), 57600)],
+ end_hint: None
+ }
+ ),
+ (
+ 10,
+ AssignCore {
+ core: 1,
+ begin: 12,
+ assignment: vec![(Task(2), 57600)],
+ end_hint: None
+ }
+ ),
+ (
+ 16,
+ AssignCore {
+ core: 0,
+ begin: 18,
+ assignment: vec![(Task(2), 57600)],
+ end_hint: None
+ }
+ ),
+ (
+ 16,
+ AssignCore {
+ core: 1,
+ begin: 18,
+ assignment: vec![(Task(1), 57600)],
+ end_hint: None
+ }
+ ),
+ (
+ 22,
+ AssignCore {
+ core: 0,
+ begin: 24,
+ assignment: vec![(Task(2), 57600)],
+ end_hint: None,
+ },
+ ),
+ (
+ 22,
+ AssignCore {
+ core: 1,
+ begin: 24,
+ assignment: vec![(Task(1), 57600)],
+ end_hint: None,
+ },
+ ),
+ (
+ 28,
+ AssignCore {
+ core: 0,
+ begin: 30,
+ assignment: vec![(Task(1), 57600)],
+ end_hint: None,
+ },
+ ),
+ (
+ 28,
+ AssignCore {
+ core: 1,
+ begin: 30,
+ assignment: vec![(Task(2), 57600)],
+ end_hint: None,
+ },
+ ),
+ ]
+ );
+ });
+}
diff --git a/substrate/frame/broker/src/tick_impls.rs b/substrate/frame/broker/src/tick_impls.rs
index 388370bce4d4b8045122c02ad758b62b4476fd15..04e9a65bf8f67b11ef66edf7268eda74e336e65d 100644
--- a/substrate/frame/broker/src/tick_impls.rs
+++ b/substrate/frame/broker/src/tick_impls.rs
@@ -216,11 +216,10 @@ 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);
- // 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.
+ // Will the lease expire at the end of the period?
+ let expire = until < region_end;
+ if expire {
+ // last time for this one - make it renewable in the next sale.
let renewal_id = AllowedRenewalId { core: first_core, when: region_end };
let record = AllowedRenewalRecord { price, completion: Complete(schedule) };
AllowedRenewals::<T>::insert(renewal_id, &record);
@@ -232,8 +231,10 @@ impl<T: Config> Pallet<T> {
});
Self::deposit_event(Event::LeaseEnding { when: region_end, task });
}
+
first_core.saturating_inc();
- !expired
+
+ !expire
});
Leases::<T>::put(&leases);