Consider more dead forks.

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

@@ -19,7 +19,7 @@ use std::num::NonZeroUsize;
 use futures::channel::oneshot;
 use lru::LruCache;
 
-use polkadot_node_primitives::MAX_FINALITY_LAG;
+use polkadot_node_primitives::{DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION, MAX_FINALITY_LAG};
 use polkadot_node_subsystem::{
 	messages::ChainApiMessage, overseer, ActivatedLeaf, ActiveLeavesUpdate, ChainApiError,
 	SubsystemSender,
@@ -87,16 +87,6 @@ impl ChainScraper {
 	/// As long as we have `MAX_FINALITY_LAG` this makes sense as a value.
 	pub(crate) const ANCESTRY_SIZE_LIMIT: u32 = MAX_FINALITY_LAG;
 
-	/// How many blocks after finalization a backed/included candidate should be kept.
-	/// We don't want to remove scraped candidates on finalization because we want to
-	/// be sure that disputes will conclude on abandoned forks.
-	/// Removing the candidate on finalization creates a possibility for an attacker to
-	/// avoid slashing. If a bad fork is abandoned too quickly because in the same another
-	/// better one gets finalized the entries for the bad fork will be pruned and we
-	/// will never participate in a dispute for it. We want such disputes to conclude
-	/// in a timely manner so that the offenders are slashed.
-	pub(crate) const CANDIDATE_LIFETIME_AFTER_FINALIZATION: BlockNumber = 2;
-
 	/// Create a properly initialized `OrderingProvider`.
 	///
 	/// Returns: `Self` and any scraped votes.
@@ -180,7 +170,8 @@ impl ChainScraper {
 	pub fn process_finalized_block(&mut self, finalized_block_number: &BlockNumber) {
 		// `CANDIDATE_LIFETIME_AFTER_FINALIZATION - 1` because `finalized_block_number`counts to the
 		// candidate lifetime.
-		match finalized_block_number.checked_sub(Self::CANDIDATE_LIFETIME_AFTER_FINALIZATION - 1) {
+		match finalized_block_number.checked_sub(DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION - 1)
+		{
 			Some(key_to_prune) => {
 				self.backed_candidates.remove_up_to_height(&key_to_prune);
 				self.included_candidates.remove_up_to_height(&key_to_prune);

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

@@ -23,6 +23,7 @@ use parity_scale_codec::Encode;
 use sp_core::testing::TaskExecutor;
 
 use ::test_helpers::{dummy_collator, dummy_collator_signature, dummy_hash};
+use polkadot_node_primitives::DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION;
 use polkadot_node_subsystem::{
 	jaeger,
 	messages::{
@@ -454,7 +455,7 @@ fn scraper_prunes_finalized_candidates() {
 
 		// After `CANDIDATE_LIFETIME_AFTER_FINALIZATION` blocks the candidate should be removed
 		finalized_block_number =
-			TEST_TARGET_BLOCK_NUMBER + ChainScraper::CANDIDATE_LIFETIME_AFTER_FINALIZATION;
+			TEST_TARGET_BLOCK_NUMBER + DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION;
 		process_finalized_block(&mut scraper, &finalized_block_number);
 
 		assert!(!scraper.is_candidate_backed(&candidate.hash()));
@@ -512,10 +513,10 @@ fn scraper_handles_backed_but_not_included_candidate() {
 		// The candidate should be removed.
 		assert!(
 			finalized_block_number <
-				TEST_TARGET_BLOCK_NUMBER + ChainScraper::CANDIDATE_LIFETIME_AFTER_FINALIZATION
+				TEST_TARGET_BLOCK_NUMBER + DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION
 		);
 		finalized_block_number +=
-			TEST_TARGET_BLOCK_NUMBER + ChainScraper::CANDIDATE_LIFETIME_AFTER_FINALIZATION;
+			TEST_TARGET_BLOCK_NUMBER + DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION;
 		process_finalized_block(&mut scraper, &finalized_block_number);
 
 		assert!(!scraper.is_candidate_included(&candidate.hash()));
@@ -562,7 +563,7 @@ fn scraper_handles_the_same_candidate_incuded_in_two_different_block_heights() {
 		// Finalize blocks to enforce pruning of scraped events.
 		// The magic candidate was added twice, so it shouldn't be removed if we finalize two more blocks.
 		finalized_block_number = test_targets.first().expect("there are two block nums") +
-			ChainScraper::CANDIDATE_LIFETIME_AFTER_FINALIZATION;
+			DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION;
 		process_finalized_block(&mut scraper, &finalized_block_number);
 
 		let magic_candidate = make_candidate_receipt(get_magic_candidate_hash());

node/primitives/src/lib.rs

@@ -30,10 +30,10 @@ use parity_scale_codec::{Decode, Encode, Error as CodecError, Input};
 use serde::{de, Deserialize, Deserializer, Serialize, Serializer};
 
 use polkadot_primitives::v2::{
-	BlakeTwo256, CandidateCommitments, CandidateHash, CollatorPair, CommittedCandidateReceipt,
-	CompactStatement, EncodeAs, Hash, HashT, HeadData, Id as ParaId, OutboundHrmpMessage,
-	PersistedValidationData, SessionIndex, Signed, UncheckedSigned, UpwardMessage, ValidationCode,
-	ValidatorIndex, MAX_CODE_SIZE, MAX_POV_SIZE,
+	BlakeTwo256, BlockNumber, CandidateCommitments, CandidateHash, CollatorPair,
+	CommittedCandidateReceipt, CompactStatement, EncodeAs, Hash, HashT, HeadData, Id as ParaId,
+	OutboundHrmpMessage, PersistedValidationData, SessionIndex, Signed, UncheckedSigned,
+	UpwardMessage, ValidationCode, ValidatorIndex, MAX_CODE_SIZE, MAX_POV_SIZE,
 };
 pub use sp_consensus_babe::{
 	AllowedSlots as BabeAllowedSlots, BabeEpochConfiguration, Epoch as BabeEpoch,
@@ -73,8 +73,28 @@ pub const BACKING_EXECUTION_TIMEOUT: Duration = Duration::from_secs(2);
 /// ensure that in the absence of extremely large disparities between hardware,
 /// blocks that pass backing are considered executable by approval checkers or
 /// dispute participants.
+///
+/// NOTE: If this value is increased significantly, also check the dispute coordinator to consider
+/// candidates longer into finalization: `DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION`.
 pub const APPROVAL_EXECUTION_TIMEOUT: Duration = Duration::from_secs(12);
 
+/// How many blocks after finalization a information about backed/included candidate should be
+/// kept.
+///
+/// We don't want to remove scraped candidates on finalization because we want to
+/// be sure that disputes will conclude on abandoned forks.
+/// Removing the candidate on finalization creates a possibility for an attacker to
+/// avoid slashing. If a bad fork is abandoned too quickly because another
+/// better one gets finalized the entries for the bad fork will be pruned and we
+/// might never participate in a dispute for it.
+///
+/// This value should consider the timeout we allow for participation in approval-voting. In
+/// particular, the follwoing condition should hold:
+///
+/// slot time * CANDIDATE_LIFETIME_AFTER_FINALIZATION > APPROVAL_EXECUTION_TIMEOUT
+/// + slot time
+pub const DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION: BlockNumber = 10;
+
 /// Linked to `MAX_FINALITY_LAG` in relay chain selection,
 /// `MAX_HEADS_LOOK_BACK` in `approval-voting` and
 /// `MAX_BATCH_SCRAPE_ANCESTORS` in `dispute-coordinator`