Issue 4804: Notify chain selection of concluded disputes directly

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

@@ -32,6 +32,7 @@ use parity_scale_codec::Error as CodecError;
 use std::{
 	sync::Arc,
 	time::{Duration, SystemTime, UNIX_EPOCH},
+	collections::HashSet,
 };
 
 use crate::backend::{Backend, BackendWriteOp, OverlayedBackend};
@@ -466,6 +467,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 +683,20 @@ 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 
+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,103 @@ fn stagnant_makes_childless_parent_leaf() {
 		virtual_overseer
 	})
 }
+
+#[test]
+fn dispute_concluded_against_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 dispute conculded against 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 explicitely 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 dispute_reversion_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 conculded 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

@@ -176,7 +176,6 @@ fn propagate_viability_update(
 				*viability_pivots.entry(new_entry.parent_hash).or_insert(0) += 1;
 			}
 		}
-
 		backend.write_block_entry(new_entry);
 
 		tree_frontier
@@ -247,7 +246,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 +346,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,
@@ -394,6 +393,47 @@ fn apply_reversions(
 	Ok(())
 }
 
+/// 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 mut entry_to_revert = match backend.load_block_entry(&revert_hash)? {
+		None => {
+			gum::warn!(
+				target: LOG_TARGET,
+				?revert_hash,
+				// We keep parent block numbers in the disputes subsystem just
+				// for this log
+				revert_target = revert_number,
+				"The hammer has dropped. \
+				A dispute has concluded against a finalized block. \
+				Please inform an adult.",
+			);
+
+			return Ok(())
+		},
+		Some(entry_to_revert) => {
+			gum::info!(
+				target: LOG_TARGET,
+				?revert_hash,
+				revert_target = revert_number,
+				"A dispute has concluded against a block, causing it to be reverted.",
+			);
+
+			entry_to_revert
+		},
+	};
+	entry_to_revert.viability.explicitly_reverted = true;
+	// Marks children of reverted block as non-viable
+	propagate_viability_update(backend, entry_to_revert)?;
+
+	Ok(())
+}
+
 /// Finalize a block with the given number and hash.
 ///
 /// This will prune all sub-trees not descending from the given block,

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

@@ -31,7 +31,7 @@ use polkadot_node_primitives::{
 };
 use polkadot_node_subsystem::{
 	messages::{
-		ApprovalVotingMessage, BlockDescription, DisputeCoordinatorMessage,
+		ApprovalVotingMessage, BlockDescription, ChainSelectionMessage, DisputeCoordinatorMessage,
 		DisputeDistributionMessage, ImportStatementsResult,
 	},
 	overseer, ActivatedLeaf, ActiveLeavesUpdate, FromOrchestra, OverseerSignal,
@@ -1027,6 +1027,24 @@ 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() {
+			if let Some(blocks_to_revert) =
+				self.scraper.get_blocks_including_candidate(&candidate_hash)
+			{
+				ctx.send_message(ChainSelectionMessage::RevertBlocks(blocks_to_revert.clone()))
+					.await;
+			} else {
+				gum::error!(
+					target: LOG_TARGET,
+					?candidate_hash,
+					?session,
+					"Could not find parent block info for concluded candidate!"
+				);
+			}
+		}
+
 		// Update metrics:
 		if import_result.is_freshly_disputed() {
 			self.metrics.on_open();

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

@@ -14,7 +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::num::NonZeroUsize;
+use std::{collections::HashMap, num::NonZeroUsize};
 
 use futures::channel::oneshot;
 use lru::LruCache;
@@ -69,9 +69,66 @@ 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.
+/// Scrapes unfinalized chain in order to collect information from blocks. Chain scraping
+/// during disputes enables critical spam prevention. It does so by updating two important
+/// criteria determining whether a vote sent during dispute distribution is potential
+/// spam. Namely, whether the candidate being voted on is backed or included.
 ///
 /// Concretely:
 ///
@@ -95,6 +152,11 @@ pub struct ChainScraper {
 	/// We assume that ancestors of cached blocks are already processed, i.e. we have saved
 	/// corresponding included candidates.
 	last_observed_blocks: LruCache<Hash, ()>,
+	/// Maps included candidate hashes to one or more relay block heights and hashes.
+	/// 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: InclusionsPerCandidate,
 }
 
 impl ChainScraper {
@@ -119,6 +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: InclusionsPerCandidate::new(),
 		};
 		let update =
 			ActiveLeavesUpdate { activated: Some(initial_head), deactivated: Default::default() };
@@ -196,6 +259,7 @@ impl ChainScraper {
 			Some(key_to_prune) => {
 				self.backed_candidates.remove_up_to_height(&key_to_prune);
 				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
@@ -233,6 +297,7 @@ impl ChainScraper {
 						"Processing included event"
 					);
 					self.included_candidates.insert(block_number, candidate_hash);
+					self.inclusions_per_candidate.insert(candidate_hash, block_number, block_hash);
 					included_receipts.push(receipt);
 				},
 				CandidateEvent::CandidateBacked(receipt, _, _, _) => {
@@ -318,6 +383,13 @@ impl ChainScraper {
 		}
 		return Ok(ancestors)
 	}
+
+	pub fn get_blocks_including_candidate(
+		&mut self,
+		candidate: &CandidateHash,
+	) -> Option<&Vec<(BlockNumber, Hash)>> {
+		self.inclusions_per_candidate.get(candidate)
+	}
 }
 
 async fn get_finalized_block_number<Sender>(sender: &mut Sender) -> FatalResult<BlockNumber>