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

Author: rphmeier

roadmap/implementers-guide/src/runtime/parainherent.md

@@ -26,11 +26,11 @@ Included: Option<()>,
     1. Hash the parent header and make sure that it corresponds to the block hash of the parent (tracked by the `frame_system` FRAME module),
     1. Invoke `Disputes::provide_multi_dispute_data`.
     1. If `Disputes::is_frozen`, return and set `Included` to `Some(())`.
-    1. If there are any concluded disputes from the current session, invoke `Inclusion::collect_disputed` with the disputed candidates. Annotate each returned core with `FreedReason::Concluded`.
+    1. If there are any concluded disputes from the current session, invoke `Inclusion::collect_disputed` with the disputed candidates. Annotate each returned core with `FreedReason::Concluded`, sort them, and invoke `Scheduler::free_cores` with them.
     1. The `Bitfields` are first forwarded to the `Inclusion::process_bitfields` routine, returning a set of freed cores. Provide the number of availability cores (`Scheduler::availability_cores().len()`) as the expected number of bits and a `Scheduler::core_para` as a core-lookup to the `process_bitfields` routine. Annotate each of these freed cores with `FreedReason::Concluded`.
     1. For each freed candidate from the `Inclusion::process_bitfields` call, invoke `Disputes::note_included(current_session, candidate)`.
     1. If `Scheduler::availability_timeout_predicate` is `Some`, invoke `Inclusion::collect_pending` using it and annotate each of those freed cores with `FreedReason::TimedOut`.
-    1. Combine and sort the dispute-freed cores, the bitfield-freed cores, and the timed-out cores.
+    1. Combine and sort the the bitfield-freed cores and the timed-out cores.
     1. Invoke `Scheduler::clear`
     1. Invoke `Scheduler::schedule(freed_cores, System::current_block())`
     1. Extract `parent_storage_root` from the parent header,

roadmap/implementers-guide/src/runtime/scheduler.md

@@ -82,7 +82,7 @@ digraph {
 
 ## Validator Groups
 
-Validator group assignments do not need to change very quickly. The security benefits of fast rotation are redundant with the challenge mechanism in the [Approval process](../protocol-approval.md). Because of this, we only divide validators into groups at the beginning of the session and do not shuffle membership during the session. However, we do take steps to ensure that no particular validator group has dominance over a single parachain or parathread-multiplexer for an entire session to provide better guarantees of liveness.
+Validator group assignments do not need to change very quickly. The security benefits of fast rotation are redundant with the challenge mechanism in the [Approval process](../protocol-approval.md). Because of this, we only divide validators into groups at the beginning of the session and do not shuffle membership during the session. However, we do take steps to ensure that no particular validator group has dominance over a single parachain or parathread-multiplexer for an entire session to provide better guarantees of live-ness.
 
 Validator groups rotate across availability cores in a round-robin fashion, with rotation occurring at fixed intervals. The i'th group will be assigned to the `(i+k)%n`'th core at any point in time, where `k` is the number of rotations that have occurred in the session, and `n` is the number of cores. This makes upcoming rotations within the same session predictable.
 
@@ -185,7 +185,7 @@ Actions:
 1. Resize `AvailabilityCores` to have length `n_cores` with all `None` entries.
 1. Compute new validator groups by shuffling using a secure randomness beacon
    - Note that the total number of validators `V` in AV may not be evenly divided by `n_cores`.
-   - The groups are selected by partitioning AV.  The first V % N groups will have (V / n_cores) + 1 members, while the remaining groups will have (V / N) members each.
+   - The groups are selected by partitioning AV.  The first `V % N` groups will have `(V / n_cores) + 1` members, while the remaining groups will have `(V / N)` members each.
    - Instead of using the indices within AV, which point to the broader set, indices _into_ AV should be used. This implies that groups should have simply ascending validator indices.
 1. Prune the parathread queue to remove all retries beyond `configuration.parathread_retries`.
    - Also prune all parathread claims corresponding to de-registered parathreads.
@@ -209,11 +209,13 @@ No finalization routine runs for this module.
   - The core used for the parathread claim is the `next_core` field of the `ParathreadQueue` and adding `Paras::parachains().len()` to it.
   - `next_core` is then updated by adding 1 and taking it modulo `config.parathread_cores`.
   - The claim is then added to the claim index.
-- `schedule(Vec<(CoreIndex, FreedReason)>, now: BlockNumber)`: schedule new core assignments, with a parameter indicating previously-occupied cores which are to be considered returned and why they are being returned.
+- `free_cores(Vec<(CoreIndex, FreedReason)>)`: indicate previosuly-occupied cores which are to be considered returned and why they are being returned.
   - All freed parachain cores should be assigned to their respective parachain
   - All freed parathread cores whose reason for freeing was `FreedReason::Concluded` should have the claim removed from the claim index.
   - All freed parathread cores whose reason for freeing was `FreedReason::TimedOut` should have the claim added to the parathread queue again without retries incremented
   - All freed parathread cores should take the next parathread entry from the queue.
+- `schedule(Vec<(CoreIndex, FreedReason)>, now: BlockNumber)`: schedule new core assignments, with a parameter indicating previously-occupied cores which are to be considered returned and why they are being returned.
+  - Invoke `free_cores(freed_cores)`
   - The i'th validator group will be assigned to the `(i+k)%n`'th core at any point in time, where `k` is the number of rotations that have occurred in the session, and `n` is the total number of cores. This makes upcoming rotations within the same session predictable. Rotations are based off of `now`.
 - `scheduled() -> Vec<CoreAssignment>`: Get currently scheduled core assignments.
 - `occupied(Vec<CoreIndex>)`. Note that the given cores have become occupied.

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();

runtime/parachains/src/scheduler.rs

@@ -369,45 +369,53 @@ 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.
 	pub(crate) fn schedule(
 		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