Improve paras runtime BenchBuilder api

runtime/parachains/src/builder.rs

@@ -477,8 +477,11 @@ impl<T: paras_inherent::Config> BenchBuilder<T> {
 
 		(start..last)
 			.map(|seed| {
-				let session =
-					dispute_sessions.get(seed as usize).cloned().unwrap_or(self.target_session);
+				let dispute_session_idx = (seed - start) as usize;
+				let session = dispute_sessions
+					.get(dispute_session_idx)
+					.cloned()
+					.unwrap_or(self.target_session);
 
 				let (para_id, core_idx, group_idx) = self.create_indexes(seed);
 				let candidate_hash = CandidateHash(H256::from(byte32_slice_from(seed)));
@@ -520,13 +523,20 @@ impl<T: paras_inherent::Config> BenchBuilder<T> {
 
 	/// Build a scenario for testing or benchmarks.
 	///
+	/// Note that this api only allows building scenarios where the `backed_and_concluding_cores`
+	/// are mutually exclusive with the cores for disputes. So
+	/// `backed_and_concluding_cores.len() + dispute_sessions.len()` must be less than the max
+	/// number of cores.
+	///
 	/// - `backed_and_concluding_cores`: Map from core/para id/group index seed to number of
 	/// validity votes.
-	/// - `dispute_sessions`: Session index of for each dispute. Index of slice corresponds to core.
-	/// The length of this must equal total cores used. Seed index for disputes starts at
-	/// `backed_and_concluding_cores.len()`, so `dispute_sessions` needs to be left padded by
-	/// `backed_and_concluding_cores.len()` values which effectively get ignored.
-	/// TODO we should fix this.
+	/// - `dispute_sessions`: Session index of for each dispute. Index of slice corresponds to a core,
+	/// which is offset by the number of entries for `backed_and_concluding_cores`. I.E. if
+	/// `backed_and_concluding_cores` cores has 3 entries, the first index of `dispute_sessions`
+	/// will correspond to core index 3. There must be one entry for each core with a dispute
+	/// statement set.
+	/// - `includes_code_upgrade`: Set to `Some` to include a code upgrade for all backed candidates.
+	/// The value within `Some` will be the byte length of the code.
 	pub(crate) fn build(
 		self,
 		backed_and_concluding_cores: BTreeMap<u32, u32>,
@@ -540,7 +550,7 @@ impl<T: paras_inherent::Config> BenchBuilder<T> {
 
 		// We don't allow a core to have both disputes and be marked fully available at this block.
 		let cores = self.max_cores();
-		let used_cores = dispute_sessions.len() as u32;
+		let used_cores = (dispute_sessions.len() + backed_and_concluding_cores.len()) as u32;
 		assert!(used_cores <= cores);
 
 		// NOTE: there is an n+2 session delay for these actions to take effect

runtime/parachains/src/paras_inherent.rs

@@ -1063,7 +1063,10 @@ mod tests {
 			}: TestConfig,
 		) -> Bench<Test> {
 			BenchBuilder::<Test>::new()
-				.set_max_validators((dispute_sessions.len() as u32) * num_validators_per_core)
+				.set_max_validators(
+					(dispute_sessions.len() + backed_and_concluding.len()) as u32 *
+						num_validators_per_core,
+				)
 				.set_max_validators_per_core(num_validators_per_core)
 				.set_dispute_statements(dispute_statements)
 				.build(backed_and_concluding, dispute_sessions.as_slice(), includes_code_upgrade)
@@ -1083,7 +1086,7 @@ mod tests {
 
 				let scenario = make_inherent_data(TestConfig {
 					dispute_statements,
-					dispute_sessions: vec![0, 0],
+					dispute_sessions: vec![], // No disputes
 					backed_and_concluding,
 					num_validators_per_core: 1,
 					includes_code_upgrade: None,
@@ -1217,12 +1220,12 @@ mod tests {
 			new_test_ext(MockGenesisConfig::default()).execute_with(|| {
 				// Create the inherent data for this block
 				let dispute_statements = BTreeMap::new();
-				// No backed and concluding cores, so all cores will be fileld with disputesw
+				// No backed and concluding cores, so all cores will be filld with disputes.
 				let backed_and_concluding = BTreeMap::new();
 
 				let scenario = make_inherent_data(TestConfig {
 					dispute_statements,
-					dispute_sessions: vec![2, 2, 1], // 3 cores, all disputes
+					dispute_sessions: vec![2, 2, 1], // 3 cores with disputes
 					backed_and_concluding,
 					num_validators_per_core: 6,
 					includes_code_upgrade: None,
@@ -1284,12 +1287,12 @@ mod tests {
 			new_test_ext(MockGenesisConfig::default()).execute_with(|| {
 				// Create the inherent data for this block
 				let dispute_statements = BTreeMap::new();
-				// No backed and concluding cores, so all cores will be fileld with disputesw
+				// No backed and concluding cores, so all cores will be filled with disputes.
 				let backed_and_concluding = BTreeMap::new();
 
 				let scenario = make_inherent_data(TestConfig {
 					dispute_statements,
-					dispute_sessions: vec![2, 2, 1], // 3 cores, all disputes
+					dispute_sessions: vec![2, 2, 1], // 3 cores with disputes
 					backed_and_concluding,
 					num_validators_per_core: 6,
 					includes_code_upgrade: None,
@@ -1335,8 +1338,7 @@ mod tests {
 
 				let scenario = make_inherent_data(TestConfig {
 					dispute_statements,
-					// 2 backed candidates + 3 disputes (at sessions 2, 1 and 1)
-					dispute_sessions: vec![0, 0, 2, 2, 1],
+					dispute_sessions: vec![2, 2, 1], // 3 cores with disputes
 					backed_and_concluding,
 					num_validators_per_core: 4,
 					includes_code_upgrade: None,
@@ -1414,8 +1416,7 @@ mod tests {
 
 				let scenario = make_inherent_data(TestConfig {
 					dispute_statements,
-					// 2 backed candidates + 3 disputes (at sessions 2, 1 and 1)
-					dispute_sessions: vec![0, 0, 2, 2, 1],
+					dispute_sessions: vec![2, 2, 1], // 3 cores with disputes
 					backed_and_concluding,
 					num_validators_per_core: 4,
 					includes_code_upgrade: None,
@@ -1476,8 +1477,7 @@ mod tests {
 
 				let scenario = make_inherent_data(TestConfig {
 					dispute_statements,
-					// 2 backed candidates + 3 disputes (at sessions 2, 1 and 1)
-					dispute_sessions: vec![0, 0, 2, 2, 1],
+					dispute_sessions: vec![2, 2, 1], // 3 cores with disputes
 					backed_and_concluding,
 					num_validators_per_core: 5,
 					includes_code_upgrade: None,
@@ -1557,8 +1557,7 @@ mod tests {
 
 				let scenario = make_inherent_data(TestConfig {
 					dispute_statements,
-					// 2 backed candidates + 3 disputes (at sessions 2, 1 and 1)
-					dispute_sessions: vec![0, 0, 2, 2, 1],
+					dispute_sessions: vec![2, 2, 1], // 3 cores with disputes
 					backed_and_concluding,
 					num_validators_per_core: 5,
 					includes_code_upgrade: None,
@@ -1617,7 +1616,7 @@ mod tests {
 
 				let scenario = make_inherent_data(TestConfig {
 					dispute_statements,
-					dispute_sessions: vec![0, 0, 2, 2, 1], // 2 backed candidates, 3 disputes at sessions 2, 1 and 1 respectively
+					dispute_sessions: vec![2, 2, 1], // 3 cores with disputes
 					backed_and_concluding,
 					num_validators_per_core: 5,
 					includes_code_upgrade: None,
@@ -1688,7 +1687,7 @@ mod tests {
 
 				let scenario = make_inherent_data(TestConfig {
 					dispute_statements,
-					dispute_sessions: vec![0, 0, 2, 2, 1], // 2 backed candidates, 3 disputes at sessions 2, 1 and 1 respectively
+					dispute_sessions: vec![2, 2, 1], // 3 cores with disputes
 					backed_and_concluding,
 					num_validators_per_core: 5,
 					includes_code_upgrade: None,

runtime/parachains/src/paras_inherent/benchmarking.rs

@@ -60,7 +60,7 @@ benchmarks! {
 				.collect();
 
 		let scenario = BenchBuilder::<T>::new()
-			.build(cores_with_backed, &[1], None);
+			.build(cores_with_backed, &[], None);
 
 		let mut benchmark = scenario.data.clone();
 		let bitfield = benchmark.bitfields.pop().unwrap();
@@ -97,7 +97,7 @@ benchmarks! {
 				.collect();
 
 		let scenario = BenchBuilder::<T>::new()
-			.build(cores_with_backed.clone(), &[1], None);
+			.build(cores_with_backed.clone(), &[], None);
 
 		let mut benchmark = scenario.data.clone();
 
@@ -149,7 +149,7 @@ benchmarks! {
 				.collect();
 
 		let scenario = BenchBuilder::<T>::new()
-			.build(cores_with_backed.clone(), &[1], Some(v));
+			.build(cores_with_backed.clone(), &[], Some(v));
 
 		let mut benchmark = scenario.data.clone();