fix prospective-parachains best backable chain reversion bug (#6417)

Kudos to @EclesioMeloJunior for noticing it

Also added a regression test for it. The existing unit test was
exercising only the case where the full chain is reverted

---------

Co-authored-by: GitHub Action <action@github.com>
Co-authored-by: Bastian Köcher <git@kchr.de>
(cherry picked from commit 05ad547)

polkadot/node/core/prospective-parachains/src/fragment_chain/mod.rs

@@ -630,7 +630,7 @@ impl BackedChain {
 	) -> impl Iterator<Item = FragmentNode> + 'a {
 		let mut found_index = None;
 		for index in 0..self.chain.len() {
-			let node = &self.chain[0];
+			let node = &self.chain[index];
 
 			if found_index.is_some() {
 				self.by_parent_head.remove(&node.parent_head_data_hash);

polkadot/node/core/prospective-parachains/src/fragment_chain/tests.rs

@@ -1163,8 +1163,9 @@ fn test_populate_and_check_potential() {
 		Err(Error::CandidateAlreadyKnown)
 	);
 
-	// Simulate a best chain reorg by backing a2.
+	// Simulate some best chain reorgs.
 	{
+		// Back A2. The reversion should happen right at the root.
 		let mut chain = chain.clone();
 		chain.candidate_backed(&candidate_a2_hash);
 		assert_eq!(chain.best_chain_vec(), vec![candidate_a2_hash, candidate_b2_hash]);
@@ -1183,6 +1184,66 @@ fn test_populate_and_check_potential() {
 			chain.can_add_candidate_as_potential(&candidate_a_entry),
 			Err(Error::ForkChoiceRule(_))
 		);
+
+		// Simulate a more complex chain reorg.
+		// A2 points to B2, which is backed.
+		// A2 has underneath a subtree A2 -> B2 -> C3 and A2 -> B2 -> C4. B2 and C3 are backed. C4
+		// is kept because it has a lower candidate hash than C3. Backing C4 will cause a chain
+		// reorg.
+
+		// Candidate C3.
+		let (pvd_c3, candidate_c3) = make_committed_candidate(
+			para_id,
+			relay_parent_y_info.hash,
+			relay_parent_y_info.number,
+			vec![0xb4].into(),
+			vec![0xc2].into(),
+			relay_parent_y_info.number,
+		);
+		let candidate_c3_hash = candidate_c3.hash();
+		let candidate_c3_entry =
+			CandidateEntry::new(candidate_c3_hash, candidate_c3, pvd_c3, CandidateState::Seconded)
+				.unwrap();
+
+		// Candidate C4.
+		let (pvd_c4, candidate_c4) = make_committed_candidate(
+			para_id,
+			relay_parent_y_info.hash,
+			relay_parent_y_info.number,
+			vec![0xb4].into(),
+			vec![0xc3].into(),
+			relay_parent_y_info.number,
+		);
+		let candidate_c4_hash = candidate_c4.hash();
+		// C4 should have a lower candidate hash than C3.
+		assert_eq!(fork_selection_rule(&candidate_c4_hash, &candidate_c3_hash), Ordering::Less);
+		let candidate_c4_entry =
+			CandidateEntry::new(candidate_c4_hash, candidate_c4, pvd_c4, CandidateState::Seconded)
+				.unwrap();
+
+		let mut storage = storage.clone();
+		storage.add_candidate_entry(candidate_c3_entry).unwrap();
+		storage.add_candidate_entry(candidate_c4_entry).unwrap();
+		let mut chain = populate_chain_from_previous_storage(&scope, &storage);
+		chain.candidate_backed(&candidate_a2_hash);
+		chain.candidate_backed(&candidate_c3_hash);
+
+		assert_eq!(
+			chain.best_chain_vec(),
+			vec![candidate_a2_hash, candidate_b2_hash, candidate_c3_hash]
+		);
+
+		// Backing C4 will cause a reorg.
+		chain.candidate_backed(&candidate_c4_hash);
+		assert_eq!(
+			chain.best_chain_vec(),
+			vec![candidate_a2_hash, candidate_b2_hash, candidate_c4_hash]
+		);
+
+		assert_eq!(
+			chain.unconnected().map(|c| c.candidate_hash).collect::<HashSet<_>>(),
+			[candidate_f_hash].into_iter().collect()
+		);
 	}
 
 	// Candidate F has an invalid hrmp watermark. however, it was not checked beforehand as we don't

prdoc/pr_6417.prdoc

@@ -0,0 +1,9 @@
+title: fix prospective-parachains best backable chain reversion bug
+doc:
+  - audience: Node Dev
+    description: |
+      Fixes a bug in the prospective-parachains subsystem that prevented proper best backable chain reorg.
+
+crates:
+- name: polkadot-node-core-prospective-parachains
+  bump: patch