Allow at most one candidate with a code upgrade in a block (#2456)

* guide: max one candidate with upgrade per block

* max one candidate with upgrade per block

* ignore on runtime level too

polkadot/node/core/provisioner/src/lib.rs

@@ -448,7 +448,7 @@ async fn select_candidates(
 		selected_candidates.clone(),
 		tx,
 	)).into()).await.map_err(|err| Error::GetBackedCandidatesSend(err))?;
-	let candidates = rx.await.map_err(|err| Error::CanceledBackedCandidates(err))?;
+	let mut candidates = rx.await.map_err(|err| Error::CanceledBackedCandidates(err))?;
 
 	// `selected_candidates` is generated in ascending order by core index, and `GetBackedCandidates`
 	// _should_ preserve that property, but let's just make sure.
@@ -466,6 +466,20 @@ async fn select_candidates(
 		Err(Error::BackedCandidateOrderingProblem)?;
 	}
 
+	// keep only one candidate with validation code.
+	let mut with_validation_code = false;
+	candidates.retain(|c| {
+		if c.candidate.commitments.new_validation_code.is_some() {
+			if with_validation_code {
+				return false
+			}
+
+			with_validation_code = true;
+		}
+
+		true
+	});
+
 	Ok(candidates)
 }
 

polkadot/node/core/provisioner/src/tests.rs

@@ -426,4 +426,63 @@ mod select_candidates {
 				);
 		})
 	}
+
+	#[test]
+	fn selects_max_one_code_upgrade() {
+		let mock_cores = mock_availability_cores();
+		let n_cores = mock_cores.len();
+
+		let empty_hash = PersistedValidationData::<BlockNumber>::default().hash();
+
+		// why those particular indices? see the comments on mock_availability_cores()
+		// the first candidate with code is included out of [1, 4, 7, 8, 10].
+		let cores = [1, 7, 10];
+		let cores_with_code = [1, 4, 8];
+
+		let committed_receipts: Vec<_> = (0..mock_cores.len())
+			.map(|i| CommittedCandidateReceipt {
+				descriptor: CandidateDescriptor {
+					para_id: i.into(),
+					persisted_validation_data_hash: empty_hash,
+					..Default::default()
+				},
+				commitments: CandidateCommitments {
+					new_validation_code: if cores_with_code.contains(&i) { Some(vec![].into()) } else { None },
+					..Default::default()
+				},
+				..Default::default()
+			})
+			.collect();
+
+		let candidates: Vec<_> = committed_receipts.iter().map(|r| r.to_plain()).collect();
+
+		let expected_candidates: Vec<_> = cores
+			.iter()
+			.map(|&idx| candidates[idx].clone())
+			.collect();
+
+		let expected_backed: Vec<_> = cores
+			.iter()
+			.map(|&idx| BackedCandidate {
+				candidate: committed_receipts[idx].clone(),
+				validity_votes: Vec::new(),
+				validator_indices: default_bitvec(n_cores),
+			})
+			.collect();
+
+		test_harness(|r| mock_overseer(r, expected_backed), |mut tx: mpsc::Sender<FromJobCommand>| async move {
+			let result =
+				select_candidates(&mock_cores, &[], &candidates, Default::default(), &mut tx)
+					.await.unwrap();
+
+			result.into_iter()
+				.for_each(|c|
+					assert!(
+						expected_candidates.iter().any(|c2| c.candidate.corresponds_to(c2)),
+						"Failed to find candidate: {:?}",
+						c,
+					)
+				);
+		})
+	}
 }

polkadot/roadmap/implementers-guide/src/node/utility/provisioner.md

@@ -69,7 +69,7 @@ To determine availability:
     - There are two constraints: `backed_candidate.candidate.descriptor.para_id == scheduled_core.para_id && candidate.candidate.descriptor.validation_data_hash == computed_validation_data_hash`.
     - In the event that more than one candidate meets the constraints, selection between the candidates is arbitrary. However, not more than one candidate can be selected per core.
 
-The end result of this process is a vector of `BackedCandidate`s, sorted in order of their core index.
+The end result of this process is a vector of `BackedCandidate`s, sorted in order of their core index. Furthermore, this process should select at maximum one candidate which upgrades the runtime validation code.
 
 ### Determining Bitfield Availability
 

polkadot/runtime/parachains/src/inclusion_inherent.rs

@@ -169,8 +169,29 @@ decl_module! {
 /// This is somewhat less desirable than attempting to fit some of them, but is more fair in the
 /// even that we can't trust the provisioner to provide a fair / random ordering of candidates.
 fn limit_backed_candidates<T: Config>(
-	backed_candidates: Vec<BackedCandidate<T::Hash>>,
+	mut backed_candidates: Vec<BackedCandidate<T::Hash>>,
 ) -> Vec<BackedCandidate<T::Hash>> {
+	const MAX_CODE_UPGRADES: usize = 1;
+
+	// Ignore any candidates beyond one that contain code upgrades.
+	//
+	// This is an artificial limitation that does not appear in the guide as it is a practical
+	// concern around execution.
+	{
+		let mut code_upgrades = 0;
+		backed_candidates.retain(|c| {
+			if c.candidate.commitments.new_validation_code.is_some() {
+				if code_upgrades >= MAX_CODE_UPGRADES {
+					return false
+				}
+
+				code_upgrades +=1;
+			}
+
+			true
+		});
+	}
+
 	// the weight of the inclusion inherent is already included in the current block weight,
 	// so our operation is simple: if the block is currently overloaded, make this intrinsic smaller
 	if frame_system::Module::<T>::block_weight().total() > <T as frame_system::Config>::BlockWeights::get().max_block {
@@ -272,6 +293,16 @@ mod tests {
 				assert_eq!(limit_backed_candidates::<Test>(backed_candidates).len(), 0);
 			});
 		}
+
+		#[test]
+		fn ignores_subsequent_code_upgrades() {
+			new_test_ext(MockGenesisConfig::default()).execute_with(|| {
+				let mut backed = BackedCandidate::default();
+				backed.candidate.commitments.new_validation_code = Some(Vec::new().into());
+				let backed_candidates = (0..3).map(|_| backed.clone()).collect();
+				assert_eq!(limit_backed_candidates::<Test>(backed_candidates).len(), 1);
+			});
+		}
 	}
 
 	mod inclusion_inherent_weight {