Issue 4804: Notify chain selection of concluded disputes directly

node/core/chain-selection/src/lib.rs

@@ -687,7 +687,7 @@ fn handle_approved_block(backend: &mut impl Backend, approved_block: Hash) -> Re
 // the dispute coordinator has notified chain selection 
 fn handle_revert_blocks(
 	backend: &impl Backend,
-	blocks_to_revert: HashSet<(BlockNumber, Hash)>,
+	blocks_to_revert: Vec<(BlockNumber, Hash)>,
 ) -> Result<Vec<BackendWriteOp>, Error> {
 	let mut overlay = OverlayedBackend::new(backend);
 	for (block_number, block_hash) in blocks_to_revert {

node/core/dispute-coordinator/src/initialized.rs

@@ -1031,7 +1031,7 @@ impl Initialized {
 		// will need to mark the candidate's relay parent as reverted.
 		if import_result.is_freshly_concluded_against() {
 			if let Some(blocks_to_revert) =
-				self.scraper.get_blocks_including_candidate(candidate_hash)
+				self.scraper.get_blocks_including_candidate(&candidate_hash)
 			{
 				ctx.send_message(ChainSelectionMessage::RevertBlocks(blocks_to_revert.clone()))
 					.await;

node/core/dispute-coordinator/src/scraping/candidates.rs

@@ -102,19 +102,15 @@ impl ScrapedCandidates {
 	}
 
 	// Removes all candidates up to a given height. The candidates at the block height are NOT removed.
-	pub fn remove_up_to_height(&mut self, height: &BlockNumber) -> HashSet<CandidateHash> {
-		let mut unique_candidates: HashSet<CandidateHash> = HashSet::new();
+	pub fn remove_up_to_height(&mut self, height: &BlockNumber) {
 		let not_stale = self.candidates_by_block_number.split_off(&height);
 		let stale = std::mem::take(&mut self.candidates_by_block_number);
 		self.candidates_by_block_number = not_stale;
 		for candidates in stale.values() {
 			for c in candidates {
 				self.candidates.remove(c);
-				unique_candidates.insert(c.clone());
 			}
 		}
-
-		unique_candidates
 	}
 
 	pub fn insert(&mut self, block_number: BlockNumber, candidate_hash: CandidateHash) {

node/core/dispute-coordinator/src/scraping/mod.rs

@@ -14,10 +14,7 @@
 // You should have received a copy of the GNU General Public License
 // along with Polkadot.  If not, see <http://www.gnu.org/licenses/>.
 
-use std::{
-	collections::{HashMap, HashSet},
-	num::NonZeroUsize,
-};
+use std::{collections::HashMap, num::NonZeroUsize};
 
 use futures::channel::oneshot;
 use lru::LruCache;
@@ -72,6 +69,60 @@ impl ScrapedUpdates {
 	}
 }
 
+/// A structure meant to facilitate chain reversions in the event of a dispute
+/// concluding against a candidate. Each candidate hash maps to a vector of block
+/// numbers and hashes for all blocks which included the candidate. The entries
+/// in each vector are ordered by decreasing parent block number to facilitate
+/// minimal cost pruning.
+pub struct InclusionsPerCandidate {
+	inclusions_inner: HashMap<CandidateHash, Vec<(BlockNumber, Hash)>>,
+}
+
+impl InclusionsPerCandidate {
+	pub fn new() -> Self {
+		Self { inclusions_inner: HashMap::new() }
+	}
+
+	// Insert parent block into vector for the candidate hash it is including,
+	// maintaining ordering by decreasing parent block number.
+	pub fn insert(
+		&mut self,
+		candidate_hash: CandidateHash,
+		block_number: BlockNumber,
+		block_hash: Hash,
+	) {
+		if let Some(blocks_including) = self.inclusions_inner.get_mut(&candidate_hash) {
+			for idx in 0..blocks_including.len() {
+				if blocks_including[idx].0 < block_number {
+					// Idx is between 0 and blocks_including.len(), therefore in bounds. QED
+					blocks_including.insert(idx, (block_number, block_hash));
+				} else if idx == blocks_including.len() - 1 {
+					blocks_including.push((block_number, block_hash));
+				}
+			}
+		} else {
+			self.inclusions_inner
+				.insert(candidate_hash, Vec::from([(block_number, block_hash)]));
+		}
+	}
+
+	pub fn remove_up_to_height(&mut self, height: &BlockNumber) {
+		for including_blocks in self.inclusions_inner.values_mut() {
+			while including_blocks.len() > 0 &&
+				including_blocks[including_blocks.len() - 1].0 < *height
+			{
+				// Since parent_blocks length is positive, parent_blocks.len() - 1 is in bounds. QED
+				including_blocks.pop();
+			}
+		}
+		self.inclusions_inner.retain(|_, including_blocks| including_blocks.len() > 0);
+	}
+
+	pub fn get(&mut self, candidate: &CandidateHash) -> Option<&Vec<(BlockNumber, Hash)>> {
+		self.inclusions_inner.get(candidate)
+	}
+}
+
 /// Chain scraper
 ///
 /// Scrapes unfinalized chain in order to collect information from blocks. Chain scraping
@@ -105,7 +156,7 @@ pub struct ChainScraper {
 	/// These correspond to all the relay blocks which marked a candidate as included,
 	/// and are needed to apply reversions in case a dispute is concluded against the
 	/// candidate.
-	inclusions_per_candidate: HashMap<CandidateHash, HashSet<(BlockNumber, Hash)>>,
+	inclusions_per_candidate: InclusionsPerCandidate,
 }
 
 impl ChainScraper {
@@ -130,7 +181,7 @@ impl ChainScraper {
 			included_candidates: candidates::ScrapedCandidates::new(),
 			backed_candidates: candidates::ScrapedCandidates::new(),
 			last_observed_blocks: LruCache::new(LRU_OBSERVED_BLOCKS_CAPACITY),
-			inclusions_per_candidate: HashMap::new(),
+			inclusions_per_candidate: InclusionsPerCandidate::new(),
 		};
 		let update =
 			ActiveLeavesUpdate { activated: Some(initial_head), deactivated: Default::default() };
@@ -207,19 +258,8 @@ impl ChainScraper {
 		{
 			Some(key_to_prune) => {
 				self.backed_candidates.remove_up_to_height(&key_to_prune);
-				let candidates_pruned = self.included_candidates.remove_up_to_height(&key_to_prune);
-				// Removing entries from inclusions per candidate referring to blocks before
-				// key_to_prune
-				for candidate_hash in candidates_pruned {
-					self.inclusions_per_candidate.entry(candidate_hash).and_modify(|inclusions| {
-						inclusions.retain(|inclusion| inclusion.0 >= key_to_prune);
-					});
-					if let Some(inclusions) = self.inclusions_per_candidate.get(&candidate_hash) {
-						if inclusions.is_empty() {
-							self.inclusions_per_candidate.remove(&candidate_hash);
-						}
-					}
-				}
+				self.included_candidates.remove_up_to_height(&key_to_prune);
+				self.inclusions_per_candidate.remove_up_to_height(&key_to_prune)
 			},
 			None => {
 				// Nothing to prune. We are still in the beginning of the chain and there are not
@@ -257,12 +297,7 @@ impl ChainScraper {
 						"Processing included event"
 					);
 					self.included_candidates.insert(block_number, candidate_hash);
-					self.inclusions_per_candidate
-						.entry(candidate_hash)
-						.and_modify(|blocks_including| {
-							blocks_including.insert((block_number, block_hash));
-						})
-						.or_insert(HashSet::from([(block_number, block_hash)]));
+					self.inclusions_per_candidate.insert(candidate_hash, block_number, block_hash);
 					included_receipts.push(receipt);
 				},
 				CandidateEvent::CandidateBacked(receipt, _, _, _) => {
@@ -351,9 +386,9 @@ impl ChainScraper {
 
 	pub fn get_blocks_including_candidate(
 		&mut self,
-		candidate: CandidateHash,
-	) -> Option<&HashSet<(BlockNumber, Hash)>> {
-		self.inclusions_per_candidate.get(&candidate)
+		candidate: &CandidateHash,
+	) -> Option<&Vec<(BlockNumber, Hash)>> {
+		self.inclusions_per_candidate.get(candidate)
 	}
 }
 

node/core/dispute-coordinator/src/scraping/tests.rs

@@ -578,3 +578,73 @@ fn scraper_handles_the_same_candidate_incuded_in_two_different_block_heights() {
 		assert!(!scraper.is_candidate_included(&magic_candidate.hash()));
 	});
 }
+
+#[test]
+fn inclusions_per_candidate_properly_adds_and_prunes() {
+	const TEST_TARGET_BLOCK_NUMBER: BlockNumber = 2;
+	const TEST_TARGET_BLOCK_NUMBER_2: BlockNumber = 3;
+
+	// How many blocks should we skip before sending a leaf update.
+	const BLOCKS_TO_SKIP: usize = 4;
+
+	futures::executor::block_on(async {
+		let (state, mut virtual_overseer) = TestState::new().await;
+
+		let TestState { mut chain, mut scraper, mut ctx } = state;
+
+		// 1 because `TestState` starts at leaf 1.
+		let next_update = (1..BLOCKS_TO_SKIP).map(|_| next_leaf(&mut chain)).last().unwrap();
+
+		let mut finalized_block_number = 1;
+		let expected_ancestry_len = BLOCKS_TO_SKIP - finalized_block_number as usize;
+		let overseer_fut = overseer_process_active_leaves_update(
+			&mut virtual_overseer,
+			&chain,
+			finalized_block_number,
+			expected_ancestry_len,
+			|block_num| {
+				if block_num == TEST_TARGET_BLOCK_NUMBER || block_num == TEST_TARGET_BLOCK_NUMBER_2
+				{
+					get_backed_and_included_candidate_events(TEST_TARGET_BLOCK_NUMBER)
+				} else {
+					vec![]
+				}
+			},
+		);
+		join(process_active_leaves_update(ctx.sender(), &mut scraper, next_update), overseer_fut)
+			.await;
+
+		let candidate = make_candidate_receipt(get_block_number_hash(TEST_TARGET_BLOCK_NUMBER));
+
+		// We included the same candidate at two different block heights. So both blocks in which
+		// the candidate is included are recorded
+		assert_eq!(
+			scraper.get_blocks_including_candidate(&candidate.hash()),
+			Some(&Vec::from([
+				(TEST_TARGET_BLOCK_NUMBER_2, get_block_number_hash(TEST_TARGET_BLOCK_NUMBER_2)),
+				(TEST_TARGET_BLOCK_NUMBER, get_block_number_hash(TEST_TARGET_BLOCK_NUMBER))
+			]))
+		);
+
+		// After `DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION` blocks the earlier inclusion should be removed
+		finalized_block_number =
+			TEST_TARGET_BLOCK_NUMBER + DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION;
+		process_finalized_block(&mut scraper, &finalized_block_number);
+
+		// The later inclusion should still be present, as we haven't exceeded its lifetime
+		assert_eq!(
+			scraper.get_blocks_including_candidate(&candidate.hash()),
+			Some(&Vec::from([(
+				TEST_TARGET_BLOCK_NUMBER_2,
+				get_block_number_hash(TEST_TARGET_BLOCK_NUMBER_2)
+			)]))
+		);
+
+		finalized_block_number =
+			TEST_TARGET_BLOCK_NUMBER_2 + DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION;
+		process_finalized_block(&mut scraper, &finalized_block_number);
+
+		// Now both inclusions have exceeded their lifetimes after finalization and should be purged
+		assert_eq!(scraper.get_blocks_including_candidate(&candidate.hash()), None);
+	});
+}

node/subsystem-types/src/messages.rs

@@ -520,7 +520,7 @@ pub enum ChainSelectionMessage {
 	BestLeafContaining(Hash, oneshot::Sender<Option<Hash>>),
 	/// The passed blocks must be marked as reverted, and their children must be marked
 	/// as non-viable.
-	RevertBlocks(HashSet<(BlockNumber, Hash)>),
+	RevertBlocks(Vec<(BlockNumber, Hash)>),
 }
 
 /// A sender for the result of a runtime API request.

roadmap/implementers-guide/src/node/utility/chain-selection.md

@@ -34,7 +34,7 @@ Gets all leaves of the chain, i.e. block hashes that are suitable to build upon
 If the required block is unknown or not viable, then return `None`. Iterate over all leaves in order of descending weight, returning the first leaf containing the required block in its chain, and `None` otherwise.
 
 ### `ChainSelectionMessage::RevertBlocks`
-This message indicates that a dispute has concluded against a parachain block candidate. The message passes along the block number and hash of the disputed candidate's relay parent. The relay parent will be marked as reverted, and its descendants will be marked as non-viable.
+This message indicates that a dispute has concluded against a parachain block candidate. The message passes along a vector containing the block number and block hash of each block where the disputed candidate was included. The passed blocks will be marked as reverted, and their descendants will be marked as non-viable.
 
 
 ### Periodically