Free disputed cores before processing bitfields (#4008)

* guide: extract free_cores in scheduler

* scheduler: extract free cores to a separate function

* guide: remove disputed cores from scheduler first

* free disputed cores in scheduler before processing bitfields

* spellcheck is mostly right but sometimes stupid

* add comment and fmt

polkadot/runtime/parachains/src/paras_inherent.rs

@@ -173,7 +173,7 @@ pub mod pallet {
 
 			// Handle disputes logic.
 			let current_session = <shared::Pallet<T>>::session_index();
-			let freed_disputed: Vec<(_, FreedReason)> = {
+			{
 				let new_current_dispute_sets: Vec<_> = disputes
 					.iter()
 					.filter(|s| s.session == current_session)
@@ -187,7 +187,7 @@ pub mod pallet {
 					return Ok(Some(MINIMAL_INCLUSION_INHERENT_WEIGHT).into())
 				}
 
-				if !new_current_dispute_sets.is_empty() {
+				let mut freed_disputed = if !new_current_dispute_sets.is_empty() {
 					let concluded_invalid_disputes: Vec<_> = new_current_dispute_sets
 						.iter()
 						.filter(|(s, c)| T::DisputesHandler::concluded_invalid(*s, *c))
@@ -200,6 +200,13 @@ pub mod pallet {
 						.collect()
 				} else {
 					Vec::new()
+				};
+
+				if !freed_disputed.is_empty() {
+					// unstable sort is fine, because core indices are unique
+					// i.e. the same candidate can't occupy 2 cores at once.
+					freed_disputed.sort_unstable_by_key(|pair| pair.0); // sort by core index
+					<scheduler::Pallet<T>>::free_cores(freed_disputed);
 				}
 			};
 
@@ -227,12 +234,13 @@ pub mod pallet {
 			};
 
 			// Schedule paras again, given freed cores, and reasons for freeing.
-			let mut freed = freed_disputed
+			let mut freed = freed_concluded
 				.into_iter()
-				.chain(freed_concluded.into_iter().map(|(c, _hash)| (c, FreedReason::Concluded)))
+				.map(|(c, _hash)| (c, FreedReason::Concluded))
 				.chain(freed_timeout.into_iter().map(|c| (c, FreedReason::TimedOut)))
 				.collect::<Vec<_>>();
 
+			// unstable sort is fine, because core indices are unique.
 			freed.sort_unstable_by_key(|pair| pair.0); // sort by core index
 
 			<scheduler::Pallet<T>>::clear();

polkadot/runtime/parachains/src/scheduler.rs

@@ -369,6 +369,43 @@ impl<T: Config> Pallet<T> {
 		})
 	}
 
+	/// Free unassigned cores. Provide a list of cores that should be considered newly-freed along with the reason
+	/// for them being freed. The list is assumed to be sorted in ascending order by core index.
+	pub(crate) fn free_cores(just_freed_cores: impl IntoIterator<Item = (CoreIndex, FreedReason)>) {
+		let config = <configuration::Pallet<T>>::config();
+
+		AvailabilityCores::<T>::mutate(|cores| {
+			for (freed_index, freed_reason) in just_freed_cores {
+				if (freed_index.0 as usize) < cores.len() {
+					match cores[freed_index.0 as usize].take() {
+						None => continue,
+						Some(CoreOccupied::Parachain) => {},
+						Some(CoreOccupied::Parathread(entry)) => {
+							match freed_reason {
+								FreedReason::Concluded => {
+									// After a parathread candidate has successfully been included,
+									// open it up for further claims!
+									ParathreadClaimIndex::<T>::mutate(|index| {
+										if let Ok(i) = index.binary_search(&entry.claim.0) {
+											index.remove(i);
+										}
+									})
+								},
+								FreedReason::TimedOut => {
+									// If a parathread candidate times out, it's not the collator's fault,
+									// so we don't increment retries.
+									ParathreadQueue::<T>::mutate(|queue| {
+										queue.enqueue_entry(entry, config.parathread_cores);
+									})
+								},
+							}
+						},
+					}
+				}
+			}
+		})
+	}
+
 	/// Schedule all unassigned cores, where possible. Provide a list of cores that should be considered
 	/// newly-freed along with the reason for them being freed. The list is assumed to be sorted in
 	/// ascending order by core index.
@@ -376,38 +413,9 @@ impl<T: Config> Pallet<T> {
 		just_freed_cores: impl IntoIterator<Item = (CoreIndex, FreedReason)>,
 		now: T::BlockNumber,
 	) {
-		let mut cores = AvailabilityCores::<T>::get();
-		let config = <configuration::Pallet<T>>::config();
-
-		for (freed_index, freed_reason) in just_freed_cores {
-			if (freed_index.0 as usize) < cores.len() {
-				match cores[freed_index.0 as usize].take() {
-					None => continue,
-					Some(CoreOccupied::Parachain) => {},
-					Some(CoreOccupied::Parathread(entry)) => {
-						match freed_reason {
-							FreedReason::Concluded => {
-								// After a parathread candidate has successfully been included,
-								// open it up for further claims!
-								ParathreadClaimIndex::<T>::mutate(|index| {
-									if let Ok(i) = index.binary_search(&entry.claim.0) {
-										index.remove(i);
-									}
-								})
-							},
-							FreedReason::TimedOut => {
-								// If a parathread candidate times out, it's not the collator's fault,
-								// so we don't increment retries.
-								ParathreadQueue::<T>::mutate(|queue| {
-									queue.enqueue_entry(entry, config.parathread_cores);
-								})
-							},
-						}
-					},
-				}
-			}
-		}
+		Self::free_cores(just_freed_cores);
 
+		let cores = AvailabilityCores::<T>::get();
 		let parachains = <paras::Pallet<T>>::parachains();
 		let mut scheduled = Scheduled::<T>::get();
 		let mut parathread_queue = ParathreadQueue::<T>::get();
@@ -510,7 +518,6 @@ impl<T: Config> Pallet<T> {
 
 		Scheduled::<T>::set(scheduled);
 		ParathreadQueue::<T>::set(parathread_queue);
-		AvailabilityCores::<T>::set(cores);
 	}
 
 	/// Note that the given cores have become occupied. Behavior undefined if any of the given cores were not scheduled