fix regression in approval-voting introduced in paritytech#3747 (pari…

…tytech#3831)

Fixes paritytech#3826.

The docs on the `candidates` field of `BlockEntry` were incorrectly
stating that they are sorted by core index. The (incorrect) optimization
was introduced in paritytech#3747 based on this assumption. The actual ordering is
based on `CandidateIncluded` events ordering in the runtime. We revert
this optimization here.

- [x] verify the underlying issue
- [x] add a regression test

---------

Co-authored-by: Bastian Köcher <git@kchr.de>

polkadot/node/core/approval-voting/src/lib.rs

@@ -1285,10 +1285,10 @@ fn cores_to_candidate_indices(
 
 	// Map from core index to candidate index.
 	for claimed_core_index in core_indices.iter_ones() {
-		// Candidates are sorted by core index.
-		if let Ok(candidate_index) = block_entry
+		if let Some(candidate_index) = block_entry
 			.candidates()
-			.binary_search_by_key(&(claimed_core_index as u32), |(core_index, _)| core_index.0)
+			.iter()
+			.position(|(core_index, _)| core_index.0 == claimed_core_index as u32)
 		{
 			candidate_indices.push(candidate_index as _);
 		}

polkadot/node/core/approval-voting/src/persisted_entries.rs

@@ -454,7 +454,7 @@ pub struct BlockEntry {
 	slot: Slot,
 	relay_vrf_story: RelayVRFStory,
 	// The candidates included as-of this block and the index of the core they are
-	// leaving. Sorted ascending by core index.
+	// leaving.
 	candidates: Vec<(CoreIndex, CandidateHash)>,
 	// A bitfield where the i'th bit corresponds to the i'th candidate in `candidates`.
 	// The i'th bit is `true` iff the candidate has been approved in the context of this

polkadot/node/core/approval-voting/src/tests.rs

@@ -2479,6 +2479,173 @@ fn subsystem_import_checked_approval_sets_one_block_bit_at_a_time() {
 	});
 }
 
+// See https://github.com/paritytech/polkadot-sdk/issues/3826
+#[test]
+fn inclusion_events_can_be_unordered_by_core_index() {
+	let assignment_criteria = Box::new(MockAssignmentCriteria(
+		|| {
+			let mut assignments = HashMap::new();
+			for core in 0..3 {
+				let _ = assignments.insert(
+					CoreIndex(core),
+					approval_db::v2::OurAssignment {
+						cert: garbage_assignment_cert_v2(
+							AssignmentCertKindV2::RelayVRFModuloCompact {
+								core_bitfield: vec![CoreIndex(0), CoreIndex(1), CoreIndex(2)]
+									.try_into()
+									.unwrap(),
+							},
+						),
+						tranche: 0,
+						validator_index: ValidatorIndex(0),
+						triggered: false,
+					}
+					.into(),
+				);
+			}
+			assignments
+		},
+		|_| Ok(0),
+	));
+	let config = HarnessConfigBuilder::default().assignment_criteria(assignment_criteria).build();
+	let store = config.backend();
+
+	test_harness(config, |test_harness| async move {
+		let TestHarness {
+			mut virtual_overseer,
+			clock,
+			sync_oracle_handle: _sync_oracle_handle,
+			..
+		} = test_harness;
+
+		assert_matches!(
+			overseer_recv(&mut virtual_overseer).await,
+			AllMessages::ChainApi(ChainApiMessage::FinalizedBlockNumber(rx)) => {
+				rx.send(Ok(0)).unwrap();
+			}
+		);
+
+		let block_hash = Hash::repeat_byte(0x01);
+
+		let candidate_receipt0 = {
+			let mut receipt = dummy_candidate_receipt(block_hash);
+			receipt.descriptor.para_id = ParaId::from(0_u32);
+			receipt
+		};
+		let candidate_receipt1 = {
+			let mut receipt = dummy_candidate_receipt(block_hash);
+			receipt.descriptor.para_id = ParaId::from(1_u32);
+			receipt
+		};
+		let candidate_receipt2 = {
+			let mut receipt = dummy_candidate_receipt(block_hash);
+			receipt.descriptor.para_id = ParaId::from(2_u32);
+			receipt
+		};
+		let candidate_index0 = 0;
+		let candidate_index1 = 1;
+		let candidate_index2 = 2;
+
+		let validator0 = ValidatorIndex(0);
+		let validator1 = ValidatorIndex(1);
+		let validator2 = ValidatorIndex(2);
+		let validator3 = ValidatorIndex(3);
+
+		let validators = vec![
+			Sr25519Keyring::Alice,
+			Sr25519Keyring::Bob,
+			Sr25519Keyring::Charlie,
+			Sr25519Keyring::Dave,
+			Sr25519Keyring::Eve,
+		];
+		let session_info = SessionInfo {
+			validator_groups: IndexedVec::<GroupIndex, Vec<ValidatorIndex>>::from(vec![
+				vec![validator0, validator1],
+				vec![validator2],
+				vec![validator3],
+			]),
+			needed_approvals: 1,
+			zeroth_delay_tranche_width: 1,
+			relay_vrf_modulo_samples: 1,
+			n_delay_tranches: 1,
+			no_show_slots: 1,
+			..session_info(&validators)
+		};
+
+		ChainBuilder::new()
+			.add_block(
+				block_hash,
+				ChainBuilder::GENESIS_HASH,
+				1,
+				BlockConfig {
+					slot: Slot::from(0),
+					candidates: Some(vec![
+						(candidate_receipt0.clone(), CoreIndex(2), GroupIndex(2)),
+						(candidate_receipt1.clone(), CoreIndex(1), GroupIndex(0)),
+						(candidate_receipt2.clone(), CoreIndex(0), GroupIndex(1)),
+					]),
+					session_info: Some(session_info),
+					end_syncing: true,
+				},
+			)
+			.build(&mut virtual_overseer)
+			.await;
+
+		assert_eq!(clock.inner.lock().next_wakeup().unwrap(), 2);
+		clock.inner.lock().wakeup_all(100);
+
+		assert_eq!(clock.inner.lock().wakeups.len(), 0);
+
+		futures_timer::Delay::new(Duration::from_millis(100)).await;
+
+		// Assignment is distributed only once from `approval-voting`
+		assert_matches!(
+			overseer_recv(&mut virtual_overseer).await,
+			AllMessages::ApprovalDistribution(ApprovalDistributionMessage::DistributeAssignment(
+				_,
+				c_indices,
+			)) => {
+				assert_eq!(c_indices, vec![candidate_index0, candidate_index1, candidate_index2].try_into().unwrap());
+			}
+		);
+
+		// Candidate 0
+		recover_available_data(&mut virtual_overseer).await;
+		fetch_validation_code(&mut virtual_overseer).await;
+
+		// Candidate 1
+		recover_available_data(&mut virtual_overseer).await;
+		fetch_validation_code(&mut virtual_overseer).await;
+
+		// Candidate 2
+		recover_available_data(&mut virtual_overseer).await;
+		fetch_validation_code(&mut virtual_overseer).await;
+
+		// Check if assignment was triggered for candidate 0.
+		let candidate_entry =
+			store.load_candidate_entry(&candidate_receipt0.hash()).unwrap().unwrap();
+		let our_assignment =
+			candidate_entry.approval_entry(&block_hash).unwrap().our_assignment().unwrap();
+		assert!(our_assignment.triggered());
+
+		// Check if assignment was triggered for candidate 1.
+		let candidate_entry =
+			store.load_candidate_entry(&candidate_receipt1.hash()).unwrap().unwrap();
+		let our_assignment =
+			candidate_entry.approval_entry(&block_hash).unwrap().our_assignment().unwrap();
+		assert!(our_assignment.triggered());
+
+		// Check if assignment was triggered for candidate 2.
+		let candidate_entry =
+			store.load_candidate_entry(&candidate_receipt2.hash()).unwrap().unwrap();
+		let our_assignment =
+			candidate_entry.approval_entry(&block_hash).unwrap().our_assignment().unwrap();
+		assert!(our_assignment.triggered());
+
+		virtual_overseer
+	});
+}
+
 fn approved_ancestor_test(
 	skip_approval: impl Fn(BlockNumber) -> bool,
 	approved_height: BlockNumber,