Issue 4804: Notify chain selection of concluded disputes directly (#6512

)

* Setting up new ChainSelectionMessage

* Partial first pass

* Got dispute conclusion data to provisioner

* Finished first draft for 4804 code

* A bit of polish and code comments

* cargo fmt

* Implementers guide and code comments

* More formatting, and naming issues

* Wrote test for ChainSelection side of change

* Added dispute coordinator side test

* FMT

* Addressing Marcin's comments

* fmt

* Addressing further Marcin comment

* Removing unnecessary test line

* Rough draft addressing Robert changes

* Clean up and test modification

* Majorly refactored scraper change

* Minor fixes for ChainSelection

* Polish and fmt

* Condensing inclusions per candidate logic

* Addressing Tsveto's comments

* Addressing Robert's Comments

* Altered inclusions struct to use nested BTreeMaps

* Naming fix

* Fixing inclusions struct comments

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

Add comment to split_off() use

Co-authored-by: Marcin S. <[email protected]>

* Optimizing removal at block height for inclusions

* fmt

* Using copy trait

Co-authored-by: Marcin S. <[email protected]>

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

@@ -466,6 +466,10 @@ where
 
 							let _ = tx.send(best_containing);
 						}
+						ChainSelectionMessage::RevertBlocks(blocks_to_revert) => {
+							let write_ops = handle_revert_blocks(backend, blocks_to_revert)?;
+							backend.write(write_ops)?;
+						}
 					}
 				}
 			}
@@ -678,6 +682,21 @@ fn handle_approved_block(backend: &mut impl Backend, approved_block: Hash) -> Re
 	backend.write(ops)
 }
 
+// Here we revert a provided group of blocks. The most common cause for this is that
+// the dispute coordinator has notified chain selection of a dispute which concluded
+// against a candidate.
+fn handle_revert_blocks(
+	backend: &impl Backend,
+	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 {
+		tree::apply_single_reversion(&mut overlay, block_hash, block_number)?;
+	}
+
+	Ok(overlay.into_write_ops().collect())
+}
+
 fn detect_stagnant(
 	backend: &mut impl Backend,
 	now: Timestamp,

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

@@ -2014,3 +2014,106 @@ fn stagnant_makes_childless_parent_leaf() {
 		virtual_overseer
 	})
 }
+
+#[test]
+fn revert_blocks_message_triggers_proper_reversion() {
+	test_harness(|backend, _, mut virtual_overseer| async move {
+		// Building mini chain with 1 finalized block and 3 unfinalized blocks
+		let finalized_number = 0;
+		let finalized_hash = Hash::repeat_byte(0);
+
+		let (head_hash, built_chain) =
+			construct_chain_on_base(vec![1, 2, 3], finalized_number, finalized_hash, |_| {});
+
+		import_blocks_into(
+			&mut virtual_overseer,
+			&backend,
+			Some((finalized_number, finalized_hash)),
+			built_chain.clone(),
+		)
+		.await;
+
+		// Checking mini chain
+		assert_backend_contains(&backend, built_chain.iter().map(|&(ref h, _)| h));
+		assert_leaves(&backend, vec![head_hash]);
+		assert_leaves_query(&mut virtual_overseer, vec![head_hash]).await;
+
+		let block_1_hash = backend.load_blocks_by_number(1).unwrap().get(0).unwrap().clone();
+		let block_2_hash = backend.load_blocks_by_number(2).unwrap().get(0).unwrap().clone();
+
+		// Sending revert blocks message
+		let (_, write_rx) = backend.await_next_write();
+		virtual_overseer
+			.send(FromOrchestra::Communication {
+				msg: ChainSelectionMessage::RevertBlocks(Vec::from([(2, block_2_hash)])),
+			})
+			.await;
+
+		write_rx.await.unwrap();
+
+		// Checking results:
+		// Block 2 should be explicitly reverted
+		assert_eq!(
+			backend
+				.load_block_entry(&block_2_hash)
+				.unwrap()
+				.unwrap()
+				.viability
+				.explicitly_reverted,
+			true
+		);
+		// Block 3 should be non-viable, with 2 as its earliest unviable ancestor
+		assert_eq!(
+			backend
+				.load_block_entry(&head_hash)
+				.unwrap()
+				.unwrap()
+				.viability
+				.earliest_unviable_ancestor,
+			Some(block_2_hash)
+		);
+		// Block 1 should be left as the only leaf
+		assert_leaves(&backend, vec![block_1_hash]);
+
+		virtual_overseer
+	})
+}
+
+#[test]
+fn revert_blocks_against_finalized_is_ignored() {
+	test_harness(|backend, _, mut virtual_overseer| async move {
+		// Building mini chain with 1 finalized block and 3 unfinalized blocks
+		let finalized_number = 0;
+		let finalized_hash = Hash::repeat_byte(0);
+
+		let (head_hash, built_chain) =
+			construct_chain_on_base(vec![1], finalized_number, finalized_hash, |_| {});
+
+		import_blocks_into(
+			&mut virtual_overseer,
+			&backend,
+			Some((finalized_number, finalized_hash)),
+			built_chain.clone(),
+		)
+		.await;
+
+		// Checking mini chain
+		assert_backend_contains(&backend, built_chain.iter().map(|&(ref h, _)| h));
+
+		// Sending dispute concluded against message
+		virtual_overseer
+			.send(FromOrchestra::Communication {
+				msg: ChainSelectionMessage::RevertBlocks(Vec::from([(
+					finalized_number,
+					finalized_hash,
+				)])),
+			})
+			.await;
+
+		// Leaf should be head if reversion of finalized was properly ignored
+		assert_leaves(&backend, vec![head_hash]);
+		assert_leaves_query(&mut virtual_overseer, vec![head_hash]).await;
+
+		virtual_overseer
+	})
+}

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

@@ -247,7 +247,7 @@ pub(crate) fn import_block(
 	stagnant_at: Timestamp,
 ) -> Result<(), Error> {
 	add_block(backend, block_hash, block_number, parent_hash, weight, stagnant_at)?;
-	apply_reversions(backend, block_hash, block_number, reversion_logs)?;
+	apply_ancestor_reversions(backend, block_hash, block_number, reversion_logs)?;
 
 	Ok(())
 }
@@ -347,9 +347,9 @@ fn add_block(
 	Ok(())
 }
 
-// Assuming that a block is already imported, accepts the number of the block
-// as well as a list of reversions triggered by the block in ascending order.
-fn apply_reversions(
+/// Assuming that a block is already imported, accepts the number of the block
+/// as well as a list of reversions triggered by the block in ascending order.
+fn apply_ancestor_reversions(
 	backend: &mut OverlayedBackend<impl Backend>,
 	block_hash: Hash,
 	block_number: BlockNumber,
@@ -358,39 +358,76 @@ fn apply_reversions(
 	// Note: since revert numbers are  in ascending order, the expensive propagation
 	// of unviability is only heavy on the first log.
 	for revert_number in reversions {
-		let mut ancestor_entry =
-			match load_ancestor(backend, block_hash, block_number, revert_number)? {
-				None => {
-					gum::warn!(
-						target: LOG_TARGET,
-						?block_hash,
-						block_number,
-						revert_target = revert_number,
-						"The hammer has dropped. \
-					A block has indicated that its finalized ancestor be reverted. \
-					Please inform an adult.",
-					);
+		let maybe_block_entry = load_ancestor(backend, block_hash, block_number, revert_number)?;
+		revert_single_block_entry_if_present(
+			backend,
+			maybe_block_entry,
+			None,
+			revert_number,
+			Some(block_hash),
+			Some(block_number),
+		)?;
+	}
 
-					continue
-				},
-				Some(ancestor_entry) => {
-					gum::info!(
-						target: LOG_TARGET,
-						?block_hash,
-						block_number,
-						revert_target = revert_number,
-						revert_hash = ?ancestor_entry.block_hash,
-						"A block has signaled that its ancestor be reverted due to a bad parachain block.",
-					);
+	Ok(())
+}
 
-					ancestor_entry
-				},
-			};
+/// Marks a single block as explicitly reverted, then propagates viability updates
+/// to all its children. This is triggered when the disputes subsystem signals that
+/// a dispute has concluded against a candidate.
+pub(crate) fn apply_single_reversion(
+	backend: &mut OverlayedBackend<impl Backend>,
+	revert_hash: Hash,
+	revert_number: BlockNumber,
+) -> Result<(), Error> {
+	let maybe_block_entry = backend.load_block_entry(&revert_hash)?;
+	revert_single_block_entry_if_present(
+		backend,
+		maybe_block_entry,
+		Some(revert_hash),
+		revert_number,
+		None,
+		None,
+	)?;
+	Ok(())
+}
 
-		ancestor_entry.viability.explicitly_reverted = true;
-		propagate_viability_update(backend, ancestor_entry)?;
-	}
+fn revert_single_block_entry_if_present(
+	backend: &mut OverlayedBackend<impl Backend>,
+	maybe_block_entry: Option<BlockEntry>,
+	maybe_revert_hash: Option<Hash>,
+	revert_number: BlockNumber,
+	maybe_reporting_hash: Option<Hash>,
+	maybe_reporting_number: Option<BlockNumber>,
+) -> Result<(), Error> {
+	match maybe_block_entry {
+		None => {
+			gum::warn!(
+				target: LOG_TARGET,
+				?maybe_revert_hash,
+				revert_target = revert_number,
+				?maybe_reporting_hash,
+				?maybe_reporting_number,
+				"The hammer has dropped. \
+				The protocol has indicated that a finalized block be reverted. \
+				Please inform an adult.",
+			);
+		},
+		Some(mut block_entry) => {
+			gum::info!(
+				target: LOG_TARGET,
+				?maybe_revert_hash,
+				revert_target = revert_number,
+				?maybe_reporting_hash,
+				?maybe_reporting_number,
+				"Unfinalized block reverted due to a bad parachain block.",
+			);
 
+			block_entry.viability.explicitly_reverted = true;
+			// Marks children of reverted block as non-viable
+			propagate_viability_update(backend, block_entry)?;
+		},
+	}
 	Ok(())
 }
 

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

@@ -30,7 +30,7 @@ use polkadot_node_primitives::{
 };
 use polkadot_node_subsystem::{
 	messages::{
-		ApprovalVotingMessage, BlockDescription, DisputeCoordinatorMessage,
+		ApprovalVotingMessage, BlockDescription, ChainSelectionMessage, DisputeCoordinatorMessage,
 		DisputeDistributionMessage, ImportStatementsResult,
 	},
 	overseer, ActivatedLeaf, ActiveLeavesUpdate, FromOrchestra, OverseerSignal,
@@ -1023,6 +1023,22 @@ impl Initialized {
 			}
 		}
 
+		// Notify ChainSelection if a dispute has concluded against a candidate. ChainSelection
+		// will need to mark the candidate's relay parent as reverted.
+		if import_result.is_freshly_concluded_against() {
+			let blocks_including = self.scraper.get_blocks_including_candidate(&candidate_hash);
+			if blocks_including.len() > 0 {
+				ctx.send_message(ChainSelectionMessage::RevertBlocks(blocks_including)).await;
+			} else {
+				gum::debug!(
+					target: LOG_TARGET,
+					?candidate_hash,
+					?session,
+					"Could not find an including block for candidate against which a dispute has concluded."
+				);
+			}
+		}
+
 		// Update metrics:
 		if import_result.is_freshly_disputed() {
 			self.metrics.on_open();

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

@@ -102,15 +102,18 @@ 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) {
+	pub fn remove_up_to_height(&mut self, height: &BlockNumber) -> HashSet<CandidateHash> {
+		let mut candidates_modified: HashSet<CandidateHash> = HashSet::new();
 		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);
+				candidates_modified.insert(*c);
 			}
 		}
+		candidates_modified
 	}
 
 	pub fn insert(&mut self, block_number: BlockNumber, candidate_hash: CandidateHash) {