From 6f73b746d3caee00030e55369f9e5e0ddac15d83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 15 Apr 2024 15:06:22 +0200 Subject: [PATCH] pallet_broker: Support renewing leases expired in a previous period (#4089) Part of: https://github.com/paritytech/polkadot-sdk/issues/4107 --- Cargo.lock | 1 + prdoc/pr_4089.prdoc | 11 ++ substrate/frame/broker/Cargo.toml | 1 + substrate/frame/broker/src/mock.rs | 11 +- substrate/frame/broker/src/tests.rs | 139 +++++++++++++++++++++++ substrate/frame/broker/src/tick_impls.rs | 13 ++- 6 files changed, 169 insertions(+), 7 deletions(-) create mode 100644 prdoc/pr_4089.prdoc diff --git a/Cargo.lock b/Cargo.lock index 27cb7af04d63..2339aff34bcd 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 000000000000..29ac736ccd08 --- /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 969f13e269de..f36b94c3cec5 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 c7205058c972..6219b4eff1b4 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::::get().unwrap(); + + let target_block_number = + sale.region_begin as u64 * <::TimeslicePeriod as Get>::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 0aacd7ef3696..7fc7e31e9bb6 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::::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::::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::::Unavailable); + assert_noop!(Broker::do_renew(1, 0), Error::::Unavailable); + + // Lease for task 1 should have been dropped. + assert!(Leases::::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::::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::::SoldOut); + assert_eq!(balance(1), 800); + + // All leases should have ended + assert!(Leases::::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 388370bce4d4..04e9a65bf8f6 100644 --- a/substrate/frame/broker/src/tick_impls.rs +++ b/substrate/frame/broker/src/tick_impls.rs @@ -216,11 +216,10 @@ impl Pallet { let assignment = CoreAssignment::Task(task); let schedule = BoundedVec::truncate_from(vec![ScheduleItem { mask, assignment }]); Workplan::::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::::insert(renewal_id, &record); @@ -232,8 +231,10 @@ impl Pallet { }); Self::deposit_event(Event::LeaseEnding { when: region_end, task }); } + first_core.saturating_inc(); - !expired + + !expire }); Leases::::put(&leases);