Issue 4804: Notify chain selection of concluded disputes directly

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

@@ -466,6 +466,9 @@ where
 
 							let _ = tx.send(best_containing);
 						}
+						ChainSelectionMessage::DisputeConcludedAgainst(block_number, block_hash) => {
+							handle_concluded_dispute_reversions(backend, block_number, block_hash)?
+						}
 					}
 				}
 			}
@@ -678,6 +681,18 @@ fn handle_approved_block(backend: &mut impl Backend, approved_block: Hash) -> Re
 	backend.write(ops)
 }
 
+// A dispute has concluded against a candidate. Here we revert the block containing
+// that candidate and mark its descendants as non-viable
+fn handle_concluded_dispute_reversions(
+	backend: &mut impl Backend,
+	block_number: u32,
+	block_hash: Hash,
+) -> Result<(), Error> {
+	let mut overlay = OverlayedBackend::new(backend);
+	tree::apply_single_reversion(&mut overlay, block_hash, block_number)?;
+	Ok(())
+}
+
 fn detect_stagnant(
 	backend: &mut impl Backend,
 	now: Timestamp,

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(())
 }
@@ -349,7 +349,7 @@ fn add_block(
 
 // 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(
+fn apply_ancestor_reversions(
 	backend: &mut OverlayedBackend<impl Backend>,
 	block_hash: Hash,
 	block_number: BlockNumber,
@@ -394,6 +394,47 @@ fn apply_reversions(
 	Ok(())
 }
 
+/// Marks a single block as explicitly reverted. Then propegates 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,25 @@ 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(info) = self.scraper.get_included_candidate_info(candidate_hash) {
+				ctx.send_message(ChainSelectionMessage::DisputeConcludedAgainst(
+					info.block_number,
+					info.block_hash,
+				))
+				.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/candidates.rs

@@ -1,4 +1,4 @@
-use polkadot_primitives::v2::{BlockNumber, CandidateHash};
+use polkadot_primitives::v2::{BlockNumber, CandidateHash, Hash};
 use std::collections::{BTreeMap, HashMap, HashSet};
 
 /// Keeps `CandidateHash` in reference counted way.
@@ -7,7 +7,15 @@ use std::collections::{BTreeMap, HashMap, HashSet};
 /// Each `remove` decreases the reference count for the corresponding `CandidateHash`.
 /// If the reference count reaches 0 - the value is removed.
 struct RefCountedCandidates {
-	candidates: HashMap<CandidateHash, usize>,
+	candidates: HashMap<CandidateHash, CandidateInfo>,
+}
+
+// If a dispute is concluded against this candidate, then the ChainSelection
+// subsystem needs block number and block hash to mark the relay parent as reverted.
+pub struct CandidateInfo {
+	count: usize,
+	pub block_number: BlockNumber,
+	pub block_hash: Hash,
 }
 
 impl RefCountedCandidates {
@@ -17,8 +25,16 @@ impl RefCountedCandidates {
 	// If `CandidateHash` doesn't exist in the `HashMap` it is created and its reference
 	// count is set to 1.
 	// If `CandidateHash` already exists in the `HashMap` its reference count is increased.
-	pub fn insert(&mut self, candidate: CandidateHash) {
-		*self.candidates.entry(candidate).or_default() += 1;
+	pub fn insert(
+		&mut self,
+		block_number: BlockNumber,
+		block_hash: Hash,
+		candidate: CandidateHash,
+	) {
+		self.candidates
+			.entry(candidate)
+			.or_insert(CandidateInfo { count: 0, block_number, block_hash })
+			.count += 1;
 	}
 
 	// If a `CandidateHash` with reference count equals to 1 is about to be removed - the
@@ -27,9 +43,9 @@ impl RefCountedCandidates {
 	// reference count is decreased and the candidate remains in the container.
 	pub fn remove(&mut self, candidate: &CandidateHash) {
 		match self.candidates.get_mut(candidate) {
-			Some(v) if *v > 1 => *v -= 1,
+			Some(v) if v.count > 1 => v.count -= 1,
 			Some(v) => {
-				assert!(*v == 1);
+				assert!(v.count == 1);
 				self.candidates.remove(candidate);
 			},
 			None => {},
@@ -45,6 +61,7 @@ impl RefCountedCandidates {
 mod ref_counted_candidates_tests {
 	use super::*;
 	use polkadot_primitives::v2::{BlakeTwo256, HashT};
+	use test_helpers::dummy_hash;
 
 	#[test]
 	fn element_is_removed_when_refcount_reaches_zero() {
@@ -53,11 +70,11 @@ mod ref_counted_candidates_tests {
 		let zero = CandidateHash(BlakeTwo256::hash(&vec![0]));
 		let one = CandidateHash(BlakeTwo256::hash(&vec![1]));
 		// add two separate candidates
-		container.insert(zero); // refcount == 1
-		container.insert(one);
+		container.insert(0, dummy_hash(), zero); // refcount == 1
+		container.insert(0, dummy_hash(), one);
 
 		// and increase the reference count for the first
-		container.insert(zero); // refcount == 2
+		container.insert(0, dummy_hash(), zero); // refcount == 2
 
 		assert!(container.contains(&zero));
 		assert!(container.contains(&one));
@@ -113,8 +130,20 @@ impl ScrapedCandidates {
 		}
 	}
 
-	pub fn insert(&mut self, block_number: BlockNumber, candidate_hash: CandidateHash) {
-		self.candidates.insert(candidate_hash);
+	// Gets candidate info, importantly containing relay parent block number and
+	// block hash. These are needed for relay block reversions based on concluded
+	// disputes.
+	pub fn get_candidate_info(&mut self, candidate: CandidateHash) -> Option<&CandidateInfo> {
+		self.candidates.candidates.get(&candidate)
+	}
+
+	pub fn insert(
+		&mut self,
+		block_number: BlockNumber,
+		block_hash: Hash,
+		candidate_hash: CandidateHash,
+	) {
+		self.candidates.insert(block_number, block_hash, candidate_hash);
 		self.candidates_by_block_number
 			.entry(block_number)
 			.or_default()
@@ -132,12 +161,13 @@ impl ScrapedCandidates {
 mod scraped_candidates_tests {
 	use super::*;
 	use polkadot_primitives::v2::{BlakeTwo256, HashT};
+	use test_helpers::dummy_hash;
 
 	#[test]
 	fn stale_candidates_are_removed() {
 		let mut candidates = ScrapedCandidates::new();
 		let target = CandidateHash(BlakeTwo256::hash(&vec![1, 2, 3]));
-		candidates.insert(1, target);
+		candidates.insert(1, dummy_hash(), target);
 
 		assert!(candidates.contains(&target));
 

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

@@ -34,6 +34,8 @@ use crate::{
 	LOG_TARGET,
 };
 
+use self::candidates::CandidateInfo;
+
 #[cfg(test)]
 mod tests;
 
@@ -71,7 +73,10 @@ impl ScrapedUpdates {
 
 /// 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:
 ///
@@ -232,7 +237,7 @@ impl ChainScraper {
 						?block_number,
 						"Processing included event"
 					);
-					self.included_candidates.insert(block_number, candidate_hash);
+					self.included_candidates.insert(block_number, block_hash, candidate_hash);
 					included_receipts.push(receipt);
 				},
 				CandidateEvent::CandidateBacked(receipt, _, _, _) => {
@@ -243,7 +248,7 @@ impl ChainScraper {
 						?block_number,
 						"Processing backed event"
 					);
-					self.backed_candidates.insert(block_number, candidate_hash);
+					self.backed_candidates.insert(block_number, block_hash, candidate_hash);
 				},
 				_ => {
 					// skip the rest
@@ -318,6 +323,13 @@ impl ChainScraper {
 		}
 		return Ok(ancestors)
 	}
+
+	pub fn get_included_candidate_info(
+		&mut self,
+		candidate: CandidateHash,
+	) -> Option<&CandidateInfo> {
+		self.included_candidates.get_candidate_info(candidate)
+	}
 }
 
 async fn get_finalized_block_number<Sender>(sender: &mut Sender) -> FatalResult<BlockNumber>

node/overseer/src/lib.rs

@@ -593,6 +593,7 @@ pub struct Overseer<SupportsParachains> {
 		ApprovalVotingMessage,
 		AvailabilityStoreMessage,
 		AvailabilityRecoveryMessage,
+		ChainSelectionMessage,
 	])]
 	dispute_coordinator: DisputeCoordinator,
 

node/subsystem-types/src/messages.rs

@@ -592,6 +592,10 @@ pub enum ChainSelectionMessage {
 	/// Request the best leaf containing the given block in its ancestry. Return `None` if
 	/// there is no such leaf.
 	BestLeafContaining(Hash, oneshot::Sender<Option<Hash>>),
+	/// Passed block number and hash are for the relay parent of the disputed candidate.
+	/// The parent must be marked as reverted, and its children must be marked as
+	/// non-viable.
+	DisputeConcludedAgainst(BlockNumber, Hash),
 }
 
 impl ChainSelectionMessage {
@@ -604,6 +608,7 @@ impl ChainSelectionMessage {
 			ChainSelectionMessage::Approved(_) => None,
 			ChainSelectionMessage::Leaves(_) => None,
 			ChainSelectionMessage::BestLeafContaining(..) => None,
+			ChainSelectionMessage::DisputeConcludedAgainst(..) => None,
 		}
 	}
 }

roadmap/implementers-guide/src/node/disputes/dispute-coordinator.md

@@ -9,23 +9,25 @@ In particular the dispute-coordinator is responsible for:
 
 - Ensuring that the node is able to raise a dispute in case an invalid candidate
   is found during approval checking.
-- Ensuring approval votes will be recorded.
+- Ensuring that backing and approval votes will be recorded on chain. With these 
+  votes on chain we can be certain that appropriate targets for slashing will be
+  available for concluded disputes. Also, scraping these votes during a dispute
+  is necessary for critical spam prevention measures.
+- Ensuring backing votes will never get overridden by explicit votes.
 - Coordinating actual participation in a dispute, ensuring that the node
   participates in any justified dispute in a way that ensures resolution of
   disputes on the network even in the case of many disputes raised (flood/DoS
   scenario).
 - Ensuring disputes resolve, even for candidates on abandoned forks as much as
   reasonably possible, to rule out "free tries" and thus guarantee our gambler's
   ruin property.
-- Provide an API for chain selection, so we can prevent finalization of any
+- Providing an API for chain selection, so we can prevent finalization of any
   chain which has included candidates for which a dispute is either ongoing or
   concluded invalid and avoid building on chains with an included invalid
   candidate.
-- Provide an API for retrieving (resolved) disputes, including all votes, both
+- Providing an API for retrieving (resolved) disputes, including all votes, both
   implicit (approval, backing) and explicit dispute votes. So validators can get
   rewarded/slashed accordingly.
-- Ensure backing votes are recorded and will never get overridden by explicit
-  votes.
 
 ## Ensuring That Disputes Can Be Raised
 

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

@@ -8,6 +8,7 @@ This subsystem needs to update its information on the unfinalized chain:
   * On every leaf-activated signal
   * On every block-finalized signal
   * On every `ChainSelectionMessage::Approve`
+  * On every `ChainSelectionMessage::DisputeConcludedAgainst`
   * Periodically, to detect stagnation.
 
 Simple implementations of these updates do `O(n_unfinalized_blocks)` disk operations. If the amount of unfinalized blocks is relatively small, the updates should not take very much time. However, in cases where there are hundreds or thousands of unfinalized blocks the naive implementations of these update algorithms would have to be replaced with more sophisticated versions.
@@ -29,6 +30,10 @@ Update the approval status of the referenced block. If the block was stagnant an
 If the required block is unknown or not viable, then return `None`.
 Iterate over all leaves, returning the first leaf containing the required block in its chain, and `None` otherwise.
 
+### `ChainSelectionMessage::DisputeConcludedAgainst`
+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.
+
+
 ### Periodically
 
 Detect stagnant blocks and apply the stagnant definition to all descendants. Update the set of viable leaves accordingly.