From d75d0d59c493a6039eb6f2eaab5e25e292d7d35d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 19 Apr 2024 22:48:26 +0200 Subject: [PATCH 1/4] pallet_broker: Let `start_sales` calculate and request the correct core count --- .../frame/broker/src/dispatchable_impls.rs | 10 ++++++++- substrate/frame/broker/src/lib.rs | 21 +++++++------------ substrate/frame/broker/src/tests.rs | 20 ++++++++++++++++++ 3 files changed, 36 insertions(+), 15 deletions(-) diff --git a/substrate/frame/broker/src/dispatchable_impls.rs b/substrate/frame/broker/src/dispatchable_impls.rs index cb7393cc9e32..45a0a514c307 100644 --- a/substrate/frame/broker/src/dispatchable_impls.rs +++ b/substrate/frame/broker/src/dispatchable_impls.rs @@ -70,8 +70,16 @@ impl Pallet { Ok(()) } - pub(crate) fn do_start_sales(price: BalanceOf, core_count: CoreIndex) -> DispatchResult { + pub(crate) fn do_start_sales(price: BalanceOf, extra_cores: CoreIndex) -> DispatchResult { let config = Configuration::::get().ok_or(Error::::Uninitialized)?; + + // Determine the core count + let core_count = Leases::::decode_len().unwrap_or(0) as CoreIndex + + Reservations::::decode_len().unwrap_or(0) as CoreIndex + + extra_cores; + + Self::do_request_core_count(core_count)?; + let commit_timeslice = Self::latest_timeslice_ready_to_commit(&config); let status = StatusRecord { core_count, diff --git a/substrate/frame/broker/src/lib.rs b/substrate/frame/broker/src/lib.rs index 1ef9e59f0186..f494fd51f091 100644 --- a/substrate/frame/broker/src/lib.rs +++ b/substrate/frame/broker/src/lib.rs @@ -559,27 +559,20 @@ pub mod pallet { /// /// - `origin`: Must be Root or pass `AdminOrigin`. /// - `initial_price`: The price of Bulk Coretime in the first sale. - /// - `total_core_count`: This is the total number of cores the relay chain should have - /// after the sale concludes. + /// - `extra_cores`: Number of extra cores that should be requested on top of the cores + /// required for `Reservations` and `Leases`. /// - /// NOTE: This function does not actually request that new core count from the relay chain. - /// You need to make sure to call `request_core_count` afterwards to bring the relay chain - /// in sync. - /// - /// When to call the function depends on the new core count. If it is larger than what it - /// was before, you can call it immediately or even before `start_sales` as non allocated - /// cores will just be `Idle`. If you are actually reducing the number of cores, you should - /// call `request_core_count`, right before the next sale, to avoid shutting down tasks too - /// early. + /// This will call [`Self::request_core_count`] internally to set the correct core count on + /// the relay chain. #[pallet::call_index(4)] - #[pallet::weight(T::WeightInfo::start_sales((*total_core_count).into()))] + #[pallet::weight(T::WeightInfo::start_sales(T::MaxLeasedCores::get() + T::MaxReservedCores::get()))] pub fn start_sales( origin: OriginFor, initial_price: BalanceOf, - total_core_count: CoreIndex, + extra_cores: CoreIndex, ) -> DispatchResultWithPostInfo { T::AdminOrigin::ensure_origin_or_root(origin)?; - Self::do_start_sales(initial_price, total_core_count)?; + Self::do_start_sales(initial_price, extra_cores)?; Ok(Pays::No.into()) } diff --git a/substrate/frame/broker/src/tests.rs b/substrate/frame/broker/src/tests.rs index c573b6c55a20..11fea8fde3d6 100644 --- a/substrate/frame/broker/src/tests.rs +++ b/substrate/frame/broker/src/tests.rs @@ -1408,3 +1408,23 @@ fn renewal_works_leases_ended_before_start_sales() { ); }); } + +#[test] +fn start_sales_sets_correct_core_count() { + TestExt::new().endow(1, 1000).execute_with(|| { + advance_to(1); + + Broker::do_set_lease(1, 100).unwrap(); + Broker::do_set_lease(2, 100).unwrap(); + Broker::do_set_lease(3, 100).unwrap(); + Broker::do_reserve(Schedule::truncate_from(vec![ScheduleItem { + assignment: Pool, + mask: CoreMask::complete(), + }])) + .unwrap(); + + Broker::do_start_sales(5, 5).unwrap(); + + System::assert_has_event(Event::::CoreCountRequested { core_count: 9 }.into()); + }) +} From e29be9e5c8b6c44a73422169c9c440574c0b354a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Sat, 20 Apr 2024 11:05:46 +0200 Subject: [PATCH 2/4] Fix tests --- substrate/frame/broker/src/tests.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/substrate/frame/broker/src/tests.rs b/substrate/frame/broker/src/tests.rs index 11fea8fde3d6..f929f0d50dcf 100644 --- a/substrate/frame/broker/src/tests.rs +++ b/substrate/frame/broker/src/tests.rs @@ -329,7 +329,7 @@ fn nft_metadata_works() { fn migration_works() { TestExt::new().endow(1, 1000).execute_with(|| { assert_ok!(Broker::do_set_lease(1000, 8)); - assert_ok!(Broker::do_start_sales(100, 2)); + assert_ok!(Broker::do_start_sales(100, 1)); // Sale is for regions from TS4..7 // Not ending in this sale period. @@ -385,7 +385,7 @@ fn instapool_payouts_work() { TestExt::new().endow(1, 1000).execute_with(|| { let item = ScheduleItem { assignment: Pool, mask: CoreMask::complete() }; assert_ok!(Broker::do_reserve(Schedule::truncate_from(vec![item]))); - assert_ok!(Broker::do_start_sales(100, 3)); + assert_ok!(Broker::do_start_sales(100, 2)); advance_to(2); let region = Broker::do_purchase(1, u64::max_value()).unwrap(); assert_ok!(Broker::do_pool(region, None, 2, Final)); @@ -411,7 +411,7 @@ fn instapool_partial_core_payouts_work() { TestExt::new().endow(1, 1000).execute_with(|| { let item = ScheduleItem { assignment: Pool, mask: CoreMask::complete() }; assert_ok!(Broker::do_reserve(Schedule::truncate_from(vec![item]))); - assert_ok!(Broker::do_start_sales(100, 2)); + assert_ok!(Broker::do_start_sales(100, 1)); advance_to(2); let region = Broker::do_purchase(1, u64::max_value()).unwrap(); let (region1, region2) = @@ -477,7 +477,7 @@ fn initialize_with_system_paras_works() { ScheduleItem { assignment: Task(4u32), mask: 0x00000_00000_00000_fffff.into() }, ]; assert_ok!(Broker::do_reserve(Schedule::truncate_from(items))); - assert_ok!(Broker::do_start_sales(100, 2)); + assert_ok!(Broker::do_start_sales(100, 0)); advance_to(10); assert_eq!( CoretimeTrace::get(), @@ -510,7 +510,7 @@ fn initialize_with_leased_slots_works() { TestExt::new().execute_with(|| { assert_ok!(Broker::do_set_lease(1000, 6)); assert_ok!(Broker::do_set_lease(1001, 7)); - assert_ok!(Broker::do_start_sales(100, 2)); + assert_ok!(Broker::do_start_sales(100, 0)); advance_to(18); let end_hint = None; assert_eq!( @@ -925,7 +925,7 @@ fn leases_can_be_renewed() { assert_ok!(Broker::do_set_lease(2001, 9)); assert_eq!(Leases::::get().len(), 1); // Start the sales with only one core for this lease. - assert_ok!(Broker::do_start_sales(100, 1)); + assert_ok!(Broker::do_start_sales(100, 0)); // Advance to sale period 1, we should get an AllowedRenewal for task 2001 for the next // sale. @@ -1018,7 +1018,7 @@ fn short_leases_cannot_be_renewed() { assert_ok!(Broker::do_set_lease(2001, 3)); assert_eq!(Leases::::get().len(), 1); // Start the sales with one core for this lease. - assert_ok!(Broker::do_start_sales(100, 1)); + assert_ok!(Broker::do_start_sales(100, 0)); // The lease is removed. assert_eq!(Leases::::get().len(), 0); @@ -1290,7 +1290,7 @@ fn renewal_works_leases_ended_before_start_sales() { )); // This intializes the first sale and the period 0. - assert_ok!(Broker::do_start_sales(100, 2)); + assert_ok!(Broker::do_start_sales(100, 0)); assert_noop!(Broker::do_renew(1, 1), Error::::Unavailable); assert_noop!(Broker::do_renew(1, 0), Error::::Unavailable); From a301fcd7d496c45fc8140534f88288f63cc0d8bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Tue, 23 Apr 2024 16:49:45 +0200 Subject: [PATCH 3/4] Fix benchmarks --- substrate/frame/broker/src/benchmarking.rs | 10 +++++++--- substrate/frame/broker/src/lib.rs | 4 +++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/substrate/frame/broker/src/benchmarking.rs b/substrate/frame/broker/src/benchmarking.rs index 1fc1c3a101ab..7533e3dc68c4 100644 --- a/substrate/frame/broker/src/benchmarking.rs +++ b/substrate/frame/broker/src/benchmarking.rs @@ -189,11 +189,15 @@ mod benches { let config = new_config_record::(); Configuration::::put(config.clone()); + let mut extra_cores = n; + // Assume Reservations to be filled for worst case - setup_reservations::(T::MaxReservedCores::get()); + setup_reservations::(extra_cores.min(T::MaxReservedCores::get())); + extra_cores = extra_cores.saturating_sub(T::MaxReservedCores::get()); // Assume Leases to be filled for worst case - setup_leases::(T::MaxLeasedCores::get(), 1, 10); + setup_leases::(extra_cores.min(T::MaxLeasedCores::get()), 1, 10); + extra_cores = extra_cores.saturating_sub(T::MaxLeasedCores::get()); let latest_region_begin = Broker::::latest_timeslice_ready_to_commit(&config); @@ -203,7 +207,7 @@ mod benches { T::AdminOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; #[extrinsic_call] - _(origin as T::RuntimeOrigin, initial_price, n.try_into().unwrap()); + _(origin as T::RuntimeOrigin, initial_price, extra_cores.try_into().unwrap()); assert!(SaleInfo::::get().is_some()); assert_last_event::( diff --git a/substrate/frame/broker/src/lib.rs b/substrate/frame/broker/src/lib.rs index f494fd51f091..d59c4c9c6b24 100644 --- a/substrate/frame/broker/src/lib.rs +++ b/substrate/frame/broker/src/lib.rs @@ -565,7 +565,9 @@ pub mod pallet { /// This will call [`Self::request_core_count`] internally to set the correct core count on /// the relay chain. #[pallet::call_index(4)] - #[pallet::weight(T::WeightInfo::start_sales(T::MaxLeasedCores::get() + T::MaxReservedCores::get()))] + #[pallet::weight(T::WeightInfo::start_sales( + T::MaxLeasedCores::get() + T::MaxReservedCores::get() + *extra_cores as u32 + ))] pub fn start_sales( origin: OriginFor, initial_price: BalanceOf, From 235527e978c0fd7ef99a105c0833e8b07cd1ce04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Tue, 23 Apr 2024 17:11:42 +0200 Subject: [PATCH 4/4] PRDOC --- prdoc/pr_4221.prdoc | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 prdoc/pr_4221.prdoc diff --git a/prdoc/pr_4221.prdoc b/prdoc/pr_4221.prdoc new file mode 100644 index 000000000000..e4941cce892a --- /dev/null +++ b/prdoc/pr_4221.prdoc @@ -0,0 +1,15 @@ +title: "pallet_broker::start_sales: Take `extra_cores` and not total cores" + +doc: + - audience: Runtime User + description: | + Change `pallet_broker::start_sales` to take `extra_cores` and not total cores. + It will calculate the total number of cores to offer based on number of + reservations plus number of leases plus `extra_cores`. Internally it will + also notify the relay chain of the required number of cores. + + Thus, starting the first sales with `pallet-broker` requires less brain power ;) + +crates: +- name: pallet-broker + bump: minor