pallet_broker: Let start_sales calculate and request the correct core count (#4221)

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

substrate/frame/broker/src/benchmarking.rs

@@ -189,11 +189,15 @@ mod benches {
 		let config = new_config_record::<T>();
 		Configuration::<T>::put(config.clone());
 
+		let mut extra_cores = n;
+
 		// Assume Reservations to be filled for worst case
-		setup_reservations::<T>(T::MaxReservedCores::get());
+		setup_reservations::<T>(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>(T::MaxLeasedCores::get(), 1, 10);
+		setup_leases::<T>(extra_cores.min(T::MaxLeasedCores::get()), 1, 10);
+		extra_cores = extra_cores.saturating_sub(T::MaxLeasedCores::get());
 
 		let latest_region_begin = Broker::<T>::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::<T>::get().is_some());
 		assert_last_event::<T>(

substrate/frame/broker/src/dispatchable_impls.rs

@@ -70,8 +70,16 @@ impl<T: Config> Pallet<T> {
 		Ok(())
 	}
 
-	pub(crate) fn do_start_sales(price: BalanceOf<T>, core_count: CoreIndex) -> DispatchResult {
+	pub(crate) fn do_start_sales(price: BalanceOf<T>, extra_cores: CoreIndex) -> DispatchResult {
 		let config = Configuration::<T>::get().ok_or(Error::<T>::Uninitialized)?;
+
+		// Determine the core count
+		let core_count = Leases::<T>::decode_len().unwrap_or(0) as CoreIndex +
+			Reservations::<T>::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,

substrate/frame/broker/src/lib.rs

@@ -559,27 +559,22 @@ 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() + *extra_cores as u32
+		))]
 		pub fn start_sales(
 			origin: OriginFor<T>,
 			initial_price: BalanceOf<T>,
-			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())
 		}
 

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::<Test>::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::<Test>::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::<Test>::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::<Test>::Unavailable);
 		assert_noop!(Broker::do_renew(1, 0), Error::<Test>::Unavailable);
 
@@ -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::<Test>::CoreCountRequested { core_count: 9 }.into());
+	})
+}