Coretime Chain: mitigate behaviour with many assignments on one core

CHANGELOG.md

@@ -76,6 +76,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 - Fix claim queue size ([runtimes#381](https://github.com/polkadot-fellows/runtimes/pull/381), [SDK v1.14 #4691](https://github.com/paritytech/polkadot-sdk/pull/4691)).
 - `pallet-referenda`: Ensure to schedule referenda earliest at the next block ([runtimes#381](https://github.com/polkadot-fellows/runtimes/pull/381), [SDK v1.14 #4823](https://github.com/paritytech/polkadot-sdk/pull/4823)).
 - Don't partially modify HRMP pages ([runtimes#381](https://github.com/polkadot-fellows/runtimes/pull/381), [SDK v1.14 #4710](https://github.com/paritytech/polkadot-sdk/pull/4710)).
+- Coretime Chain: mitigate behaviour with many assignments on one core ([runtimes#434][https://github.com/polkadot-fellows/runtimes/pull/434]).
 
 #### From [#322](https://github.com/polkadot-fellows/runtimes/pull/322):
 

integration-tests/emulated/chains/parachains/coretime/coretime-kusama/src/lib.rs

@@ -41,6 +41,7 @@ decl_test_parachains! {
 		pallets = {
 			PolkadotXcm: coretime_kusama_runtime::PolkadotXcm,
 			Balances: coretime_kusama_runtime::Balances,
+			Broker: coretime_kusama_runtime::Broker,
 		}
 	},
 }

integration-tests/emulated/tests/coretime/coretime-kusama/Cargo.toml

@@ -14,12 +14,14 @@ codec = { workspace = true, default-features = true }
 sp-runtime = { workspace = true, default-features = true }
 frame-support = { workspace = true, default-features = true }
 pallet-balances = { workspace = true, default-features = true }
+pallet-broker = { workspace = true, default-features = true }
 pallet-message-queue = { workspace = true, default-features = true }
 pallet-identity = { workspace = true, default-features = true }
 
 # Polkadot
 polkadot-runtime-common = { workspace = true, default-features = true }
 pallet-xcm = { workspace = true, default-features = true }
+runtime-parachains = { workspace = true, default-features = true }
 xcm = { workspace = true, default-features = true }
 xcm-executor = { workspace = true }
 xcm-runtime-apis = { workspace = true, default-features = true }

integration-tests/emulated/tests/coretime/coretime-kusama/src/tests/coretime_interface.rs

@@ -0,0 +1,204 @@
+// Copyright (C) Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: Apache-2.0
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use crate::*;
+use frame_support::traits::OnInitialize;
+use kusama_runtime_constants::system_parachain::coretime::TIMESLICE_PERIOD;
+use pallet_broker::{ConfigRecord, Configuration, CoreAssignment, CoreMask, ScheduleItem};
+use sp_runtime::Perbill;
+
+#[test]
+fn transact_hardcoded_weights_are_sane() {
+	// There are three transacts with hardcoded weights sent from the Coretime Chain to the Relay
+	// Chain across the CoretimeInterface which are triggered at various points in the sales cycle.
+	// - Request core count - triggered directly by `start_sales` or `request_core_count`
+	//   extrinsics.
+	// - Request revenue info - triggered when each timeslice is committed.
+	// - Assign core - triggered when an entry is encountered in the workplan for the next
+	//   timeslice.
+
+	// RuntimeEvent aliases to avoid warning from usage of qualified paths in assertions due to
+	// <https://github.com/rust-lang/rust/issues/86935>
+	type CoretimeEvent = <CoretimeKusama as Chain>::RuntimeEvent;
+	type RelayEvent = <Kusama as Chain>::RuntimeEvent;
+
+	// Reserve a workload, configure broker and start sales.
+	CoretimeKusama::execute_with(|| {
+		// Hooks don't run in emulated tests - workaround as we need `on_initialize` to tick things
+		// along and have no concept of time passing otherwise.
+		<CoretimeKusama as CoretimeKusamaPallet>::Broker::on_initialize(
+			<CoretimeKusama as Chain>::System::block_number(),
+		);
+
+		let coretime_root_origin = <CoretimeKusama as Chain>::RuntimeOrigin::root();
+
+		// Create and populate schedule with the worst case assignment on this core.
+		let mut schedule = Vec::new();
+		for i in 0..80 {
+			schedule.push(ScheduleItem {
+				mask: CoreMask::void().set(i),
+				assignment: CoreAssignment::Task(2000 + i),
+			})
+		}
+
+		assert_ok!(<CoretimeKusama as CoretimeKusamaPallet>::Broker::reserve(
+			coretime_root_origin.clone(),
+			schedule.try_into().expect("Vector is within bounds."),
+		));
+
+		// Configure broker and start sales.
+		let config = ConfigRecord {
+			advance_notice: 1,
+			interlude_length: 1,
+			leadin_length: 2,
+			region_length: 1,
+			ideal_bulk_proportion: Perbill::from_percent(40),
+			limit_cores_offered: None,
+			renewal_bump: Perbill::from_percent(2),
+			contribution_timeout: 1,
+		};
+		assert_ok!(<CoretimeKusama as CoretimeKusamaPallet>::Broker::configure(
+			coretime_root_origin.clone(),
+			config
+		));
+		assert_ok!(<CoretimeKusama as CoretimeKusamaPallet>::Broker::start_sales(
+			coretime_root_origin,
+			100,
+			0
+		));
+		assert_eq!(
+			pallet_broker::Status::<<CoretimeKusama as Chain>::Runtime>::get()
+				.unwrap()
+				.core_count,
+			1
+		);
+
+		assert_expected_events!(
+			CoretimeKusama,
+			vec![
+				CoretimeEvent::Broker(
+					pallet_broker::Event::ReservationMade { .. }
+				) => {},
+				CoretimeEvent::Broker(
+					pallet_broker::Event::CoreCountRequested { core_count: 1 }
+				) => {},
+				CoretimeEvent::ParachainSystem(
+					cumulus_pallet_parachain_system::Event::UpwardMessageSent { .. }
+				) => {},
+			]
+		);
+	});
+
+	// Check that the request_core_count message was processed successfully. This will fail if the
+	// weights are misconfigured.
+	Kusama::execute_with(|| {
+		Kusama::assert_ump_queue_processed(true, Some(CoretimeKusama::para_id()), None);
+
+		assert_expected_events!(
+			Kusama,
+			vec![
+				RelayEvent::MessageQueue(
+					pallet_message_queue::Event::Processed { success: true, .. }
+				) => {},
+			]
+		);
+	});
+
+	// Keep track of the relay chain block number so we can fast forward while still checking the
+	// right block.
+	let mut block_number_cursor = Kusama::ext_wrapper(<Kusama as Chain>::System::block_number);
+
+	let config = CoretimeKusama::ext_wrapper(|| {
+		Configuration::<<CoretimeKusama as Chain>::Runtime>::get()
+			.expect("Pallet was configured earlier.")
+	});
+
+	// Now run up to the block before the sale is rotated.
+	while block_number_cursor < TIMESLICE_PERIOD - config.advance_notice - 1 {
+		CoretimeKusama::execute_with(|| {
+			// Hooks don't run in emulated tests - workaround.
+			<CoretimeKusama as CoretimeKusamaPallet>::Broker::on_initialize(
+				<CoretimeKusama as Chain>::System::block_number(),
+			);
+		});
+
+		Kusama::ext_wrapper(|| {
+			block_number_cursor = <Kusama as Chain>::System::block_number();
+		});
+
+		dbg!(&block_number_cursor);
+	}
+
+	// In this block we trigger assign core.
+	CoretimeKusama::execute_with(|| {
+		// Hooks don't run in emulated tests - workaround.
+		<CoretimeKusama as CoretimeKusamaPallet>::Broker::on_initialize(
+			<CoretimeKusama as Chain>::System::block_number(),
+		);
+
+		assert_expected_events!(
+			CoretimeKusama,
+			vec![
+				CoretimeEvent::Broker(
+					pallet_broker::Event::SaleInitialized { .. }
+				) => {},
+				CoretimeEvent::Broker(
+					pallet_broker::Event::CoreAssigned { .. }
+				) => {},
+				CoretimeEvent::ParachainSystem(
+					cumulus_pallet_parachain_system::Event::UpwardMessageSent { .. }
+				) => {},
+			]
+		);
+	});
+
+	// In this block we trigger request revenue.
+	CoretimeKusama::execute_with(|| {
+		// Hooks don't run in emulated tests - workaround.
+		<CoretimeKusama as CoretimeKusamaPallet>::Broker::on_initialize(
+			<CoretimeKusama as Chain>::System::block_number(),
+		);
+
+		assert_expected_events!(
+			CoretimeKusama,
+			vec![
+				CoretimeEvent::ParachainSystem(
+					cumulus_pallet_parachain_system::Event::UpwardMessageSent { .. }
+				) => {},
+			]
+		);
+	});
+
+	// Check that the assign_core and request_revenue_info_at messages were processed successfully.
+	// This will fail if the weights are misconfigured.
+	Kusama::execute_with(|| {
+		Kusama::assert_ump_queue_processed(true, Some(CoretimeKusama::para_id()), None);
+
+		assert_expected_events!(
+			Kusama,
+			vec![
+				RelayEvent::MessageQueue(
+					pallet_message_queue::Event::Processed { success: true, .. }
+				) => {},
+				RelayEvent::MessageQueue(
+					pallet_message_queue::Event::Processed { success: true, .. }
+				) => {},
+				RelayEvent::Coretime(
+					runtime_parachains::coretime::Event::CoreAssigned { .. }
+				) => {},
+			]
+		);
+	});
+}

integration-tests/emulated/tests/coretime/coretime-kusama/src/tests/mod.rs

@@ -13,4 +13,5 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+mod coretime_interface;
 mod teleport;

integration-tests/emulated/tests/coretime/coretime-polkadot/src/tests/coretime_interface.rs

@@ -44,9 +44,9 @@ fn transact_hardcoded_weights_are_sane() {
 
 		let coretime_root_origin = <CoretimePolkadot as Chain>::RuntimeOrigin::root();
 
-		// Create and populate schedule with some assignments on this core.
+		// Create and populate schedule with the worst case assignment on this core.
 		let mut schedule = Vec::new();
-		for i in 0..10 {
+		for i in 0..80 {
 			schedule.push(ScheduleItem {
 				mask: CoreMask::void().set(i),
 				assignment: CoreAssignment::Task(2000 + i),

system-parachains/coretime/coretime-kusama/src/coretime.rs

@@ -223,37 +223,75 @@ impl CoretimeInterface for CoretimeAllocator {
 		end_hint: Option<RCBlockNumberOf<Self>>,
 	) {
 		use crate::coretime::CoretimeProviderCalls::AssignCore;
-		let assign_core_call =
-			RelayRuntimePallets::Coretime(AssignCore(core, begin, assignment, end_hint));
 
 		// Weight for `assign_core` from Kusama runtime benchmarks:
 		// `ref_time` = 10177115 + (1 * 25000000) + (2 * 100000000) + (80 * 13932) = 236291675
 		// `proof_size` = 3612
 		// Add 5% to each component and round to 2 significant figures.
 		let call_weight = Weight::from_parts(248_000_000, 3800);
 
-		let message = Xcm(vec![
-			Instruction::UnpaidExecution {
-				weight_limit: WeightLimit::Unlimited,
-				check_origin: None,
-			},
-			Instruction::Transact {
-				origin_kind: OriginKind::Native,
-				require_weight_at_most: call_weight,
-				call: assign_core_call.encode().into(),
-			},
-		]);
-
-		match PolkadotXcm::send_xcm(Here, Location::parent(), message) {
-			Ok(_) => log::debug!(
-				target: "runtime::coretime",
-				"Core assignment sent successfully."
-			),
-			Err(e) => log::error!(
-				target: "runtime::coretime",
-				"Core assignment failed to send: {:?}",
-				e
-			),
+		// The relay chain currently only allows `assign_core` to be called with a complete mask
+		// and only ever with increasing `begin`. The assignments must be truncated to avoid
+		// dropping that core's assignment completely.
+
+		// This shadowing of `assignment` is temporary and can be removed when the relay can accept
+		// multiple messages to assign a single core.
+		let assignment = if assignment.len() > 28 {
+			let mut total_parts = 0u16;
+			// Account for missing parts with a new `Idle` assignment at the start as
+			// `assign_core` on the relay assumes this is sorted. We'll add the rest of the
+			// assignments and sum the parts in one pass, so this is just initialized to 0.
+			let mut assignment_truncated = vec![(CoreAssignment::Idle, 0)];
+			// Truncate to first 27 non-idle assignments.
+			assignment_truncated.extend(
+				assignment
+					.into_iter()
+					.filter(|(a, _)| *a != CoreAssignment::Idle)
+					.take(27)
+					.inspect(|(_, parts)| total_parts += *parts)
+					.collect::<Vec<_>>(),
+			);
+
+			// Set the parts of the `Idle` assignment we injected at the start of the vec above.
+			assignment_truncated[0].1 = 57_600u16.saturating_sub(total_parts);
+			assignment_truncated
+		} else {
+			assignment
+		};
+
+		// The maximum number of assignments for a core in a given timeslice is 80, but only 28
+		// assignments fit inside the max size of an XCM, so we need to send in chunks.
+		for assignment_chunk in assignment.chunks(28) {
+			let assign_core_call = RelayRuntimePallets::Coretime(AssignCore(
+				core,
+				begin,
+				assignment_chunk.to_vec(),
+				end_hint,
+			));
+
+			let message = Xcm(vec![
+				Instruction::UnpaidExecution {
+					weight_limit: WeightLimit::Unlimited,
+					check_origin: None,
+				},
+				Instruction::Transact {
+					origin_kind: OriginKind::Native,
+					require_weight_at_most: call_weight,
+					call: assign_core_call.encode().into(),
+				},
+			]);
+
+			match PolkadotXcm::send_xcm(Here, Location::parent(), message) {
+				Ok(_) => log::debug!(
+					target: "runtime::coretime",
+					"Core assignment sent successfully."
+				),
+				Err(e) => log::error!(
+					target: "runtime::coretime",
+					"Core assignment failed to send: {:?}",
+					e
+				),
+			}
 		}
 	}