disputes pallet: Remove spam slots (#6345)

* disputes pallet: Filter disputes with votes less than supermajority threshold * Remove `max_spam_slots` usages * Remove `SpamSlots` * Remove `SpamSlotChange` * Remove `Error<T>::PotentialSpam` and stale comments * `create_disputes_with_no_spam` -> `create_disputes` * Make tests compile - wip commit * Rework `test_dispute_timeout`. Rename `update_spam_slots` to `filter_dispute_set` * Remove `dispute_statement_becoming_onesided_due_to_spamslots_is_accepted` and `filter_correctly_accounts_spam_slots` -> they bring no value with removed spam slots * Fix `test_provide_multi_dispute_success_and_other` * Remove an old comment * Remove spam slots from tests - clean todo comments * Remove test - `test_decrement_spam` * todo comments * Update TODO comments * Extract `test_unconfirmed_are_ignored` as separate test case * Remove dead code * Fix `test_unconfirmed_are_ignored` * Remove dead code in `filter_dispute_data` * Fix weights (related to commit "Remove `SpamSlots`") * Disputes migration - first try * Remove `dispute_max_spam_slots` + storage migration * Fix `HostConfig` migration tests * Deprecate `SpamSlots` * Code review feedback * add weight for storage version update * fix bound for clear() * Fix weights in disputes migration * Revert "Deprecate `SpamSlots`" This reverts commit 8c4d967. * Make mod migration public * Remove `SpamSlots` from disputes pallet and use `storage_alias` in the migration * Fix call to `clear()` for `SpamSlots` in migration * Update migration and add a `try-runtime` test * Add `pre_upgrade` `try-runtime` test * Fix some test names in `HostConfiguration` migration * Link spamslots migration in all runtimes * Add `test_unconfirmed_disputes_cause_block_import_error` * Update guide - Remove `SpamSlots` related information from roadmap/implementers-guide/src/runtime/disputes.md - Add 'Disputes filtering' to Runtime section of the Implementor's guide * Update runtime/parachains/src/configuration/migration.rs Co-authored-by: Marcin S. <[email protected]> * Code review feedback - update logs * Code review feedback: fix weights * Update runtime/parachains/src/disputes.rs Co-authored-by: s0me0ne-unkn0wn <[email protected]> * Additional logs in disputes migration * Fix merge conflicts * Add version checks in try-runtime tests * Fix a compilation warning` Co-authored-by: Marcin S. <[email protected]> Co-authored-by: s0me0ne-unkn0wn <[email protected]>
paritytech · Jan 7, 2023 · 3c15496 · 3c15496
1 parent d8ef5a5
commit 3c15496
Show file tree

Hide file tree

Showing 13 changed files with 483 additions and 918 deletions.
diff --git a/roadmap/implementers-guide/src/runtime/disputes.md b/roadmap/implementers-guide/src/runtime/disputes.md
@@ -37,12 +37,6 @@ Disputes: double_map (SessionIndex, CandidateHash) -> Option<DisputeState>,
 // All included blocks on the chain, as well as the block number in this chain that
 // should be reverted back to if the candidate is disputed and determined to be invalid.
 Included: double_map (SessionIndex, CandidateHash) -> Option<BlockNumber>,
-// Maps session indices to a vector indicating the number of potentially-spam disputes 
-// each validator is participating in. Potentially-spam disputes are remote disputes which have
-// fewer than `byzantine_threshold + 1` validators.
-//
-// The i'th entry of the vector corresponds to the i'th validator in the session.
-SpamSlots: map SessionIndex -> Option<Vec<u32>>,
 // Whether the chain is frozen or not. Starts as `None`. When this is `Some`,
 // the chain will not accept any new parachain blocks for backing or inclusion,
 // and its value indicates the last valid block number in the chain.
@@ -55,50 +49,45 @@ Frozen: Option<BlockNumber>,
 ## Session Change
 
 1. If the current session is not greater than `config.dispute_period + 1`, nothing to do here.
-1. Set `pruning_target = current_session - config.dispute_period - 1`. We add the extra `1` because we want to keep things for `config.dispute_period` _full_ sessions. 
+1. Set `pruning_target = current_session - config.dispute_period - 1`. We add the extra `1` because we want to keep things for `config.dispute_period` _full_ sessions.
    The stuff at the end of the most recent session has been around for a little over 0 sessions, not a little over 1.
 1. If `LastPrunedSession` is `None`, then set `LastPrunedSession` to `Some(pruning_target)` and return.
-1. Otherwise, clear out all disputes, included candidates, and `SpamSlots` entries in the range `last_pruned..=pruning_target` and set `LastPrunedSession` to `Some(pruning_target)`.
+2. Otherwise, clear out all disputes and included candidates entries in the range `last_pruned..=pruning_target` and set `LastPrunedSession` to `Some(pruning_target)`.
 
 ## Block Initialization
 
-1. Iterate through all disputes. If any have not concluded and started more than `config.dispute_conclusion_by_timeout_period` blocks ago, set them to `Concluded` and mildly punish all validators associated, as they have failed to distribute available data. If the `Included` map does not contain the candidate and there are fewer than `byzantine_threshold + 1` participating validators, reduce `SpamSlots` for all participating validators.
+1. Iterate through all disputes. If any have not concluded and started more than `config.dispute_conclusion_by_timeout_period` blocks ago, set them to `Concluded` and mildly punish all validators associated, as they have failed to distribute available data.
 
 ## Routines
 
 * `filter_multi_dispute_data(MultiDisputeStatementSet) -> MultiDisputeStatementSet`:
   1. Takes a `MultiDisputeStatementSet` and filters it down to a `MultiDisputeStatementSet`
     that satisfies all the criteria of `provide_multi_dispute_data`. That is, eliminating
-    ancient votes, votes which overwhelm the maximum amount of spam slots, and duplicates. 
-    This can be used by block authors to create the final submission in a block which is 
+    ancient votes, duplicates and unconfirmed disputes.
+    This can be used by block authors to create the final submission in a block which is
     guaranteed to pass the `provide_multi_dispute_data` checks.
 
 * `provide_multi_dispute_data(MultiDisputeStatementSet) -> Vec<(SessionIndex, Hash)>`:
   1. Pass on each dispute statement set to `provide_dispute_data`, propagating failure.
-  1. Return a list of all candidates who just had disputes initiated.
+  2. Return a list of all candidates who just had disputes initiated.
 
 * `provide_dispute_data(DisputeStatementSet) -> bool`: Provide data to an ongoing dispute or initiate a dispute.
-  1. All statements must be issued under the correct session for the correct candidate. 
+  1. All statements must be issued under the correct session for the correct candidate.
   1. `SessionInfo` is used to check statement signatures and this function should fail if any signatures are invalid.
   1. If there is no dispute under `Disputes`, create a new `DisputeState` with blank bitfields.
   1. If `concluded_at` is `Some`, and is `concluded_at + config.post_conclusion_acceptance_period < now`, return false.
-  1. If the overlap of the validators in the `DisputeStatementSet` and those already present in the `DisputeState` is fewer in number than `byzantine_threshold + 1` and the candidate is not present in the `Included` map
-      1. increment `SpamSlots` for each validator in the `DisputeStatementSet` which is not already in the `DisputeState`. Initialize the `SpamSlots` to a zeroed vector first, if necessary. do not increment `SpamSlots` if the candidate is local.
-      1. If the value for any spam slot exceeds `config.dispute_max_spam_slots`, return false.
-  1. If the overlap of the validators in the `DisputeStatementSet` and those already present in the `DisputeState` is at least `byzantine_threshold + 1`, the `DisputeState` has fewer than `byzantine_threshold + 1` validators, and the candidate is not present in the `Included` map, then decrease `SpamSlots` by 1 for each validator in the `DisputeState`.
-  1. Import all statements into the dispute. This should fail if any statements are duplicate or if the corresponding bit for the corresponding validator is set in the dispute already.
-  1. If `concluded_at` is `None`, reward all statements.
-  1. If `concluded_at` is `Some`, reward all statements slightly less.
-  1. If either side now has supermajority and did not previously, slash the other side. This may be both sides, and we support this possibility in code, but note that this requires validators to participate on both sides which has negative expected value. Set `concluded_at` to `Some(now)` if it was `None`.
-  1. If just concluded against the candidate and the `Included` map contains `(session, candidate)`: invoke `revert_and_freeze` with the stored block number.
-  1. Return true if just initiated, false otherwise.
+  2. Import all statements into the dispute. This should fail if any statements are duplicate or if the corresponding bit for the corresponding validator is set in the dispute already.
+  3. If `concluded_at` is `None`, reward all statements.
+  4. If `concluded_at` is `Some`, reward all statements slightly less.
+  5. If either side now has supermajority and did not previously, slash the other side. This may be both sides, and we support this possibility in code, but note that this requires validators to participate on both sides which has negative expected value. Set `concluded_at` to `Some(now)` if it was `None`.
+  6. If just concluded against the candidate and the `Included` map contains `(session, candidate)`: invoke `revert_and_freeze` with the stored block number.
+  7. Return true if just initiated, false otherwise.
 
 * `disputes() -> Vec<(SessionIndex, CandidateHash, DisputeState)>`: Get a list of all disputes and info about dispute state.
   1. Iterate over all disputes in `Disputes` and collect into a vector.
 
 * `note_included(SessionIndex, CandidateHash, included_in: BlockNumber)`:
   1. Add `(SessionIndex, CandidateHash)` to the `Included` map with `included_in - 1` as the value.
-  1. If there is a dispute under `(Sessionindex, CandidateHash)` with fewer than `byzantine_threshold + 1` participating validators, decrease `SpamSlots` by 1 for each validator in the `DisputeState`.
   1. If there is a dispute under `(SessionIndex, CandidateHash)` that has concluded against the candidate, invoke `revert_and_freeze` with the stored block number.
 
 * `concluded_invalid(SessionIndex, CandidateHash) -> bool`: Returns whether a candidate has already concluded a dispute in the negative.
@@ -111,3 +100,27 @@ Frozen: Option<BlockNumber>,
   1. If `is_frozen()` return.
   1. Set `Frozen` to `Some(BlockNumber)` to indicate a rollback to the block number.
   1. Issue a `Revert(BlockNumber + 1)` log to indicate a rollback of the block's child in the header chain, which is the same as a rollback to the block number.
+
+# Disputes filtering
+
+All disputes delivered to the runtime by the client are filtered before the actual import. In this context actual import
+means persisted in the runtime storage. The filtering has got two purposes:
+- Limit the amount of data saved onchain.
+- Prevent persisting malicious dispute data onchain.
+
+*Implementation note*: Filtering is performed in function `filter_dispute_data` from `Disputes` pallet.
+
+The filtering is performed on the whole statement set which is about to be imported onchain. The following filters are
+applied:
+1. Remove ancient disputes - if a dispute is concluded before the block number indicated in `OLDEST_ACCEPTED` parameter
+   it is removed from the set. `OLDEST_ACCEPTED` is a runtime configuration option.
+   *Implementation note*: `dispute_post_conclusion_acceptance_period` from
+   `HostConfiguration` is used in the current Polkadot/Kusama implementation.
+2. Remove votes from unknown validators. If there is a vote from a validator which wasn't an authority in the session
+   where the dispute was raised - they are removed. Please note that this step removes only single votes instead of
+   removing the whole dispute.
+3. Remove one sided disputes - if a dispute doesn't contain two opposing votes it is not imported onchain. This serves
+   as a measure not to import one sided disputes. A dispute is raised only if there are two opposing votes so if the
+   client is not sending them the dispute is a potential spam.
+4. Remove unconfirmed disputes - if a dispute contains less votes than the byzantine threshold it is removed. This is
+   also a spam precaution. A legitimate client will send only confirmed disputes to the runtime.
diff --git a/runtime/kusama/src/lib.rs b/runtime/kusama/src/lib.rs
@@ -1493,6 +1493,8 @@ pub type Migrations = (
 	>,
 	pallet_scheduler::migration::v4::CleanupAgendas<Runtime>,
 	pallet_staking::migrations::v13::MigrateToV13<Runtime>,
+	parachains_disputes::migration::v1::MigrateToV1<Runtime>,
+	parachains_configuration::migration::v4::MigrateToV4<Runtime>,
 );
 
 /// Unchecked extrinsic type as expected by this runtime.

diff --git a/runtime/parachains/src/builder.rs b/runtime/parachains/src/builder.rs
@@ -575,7 +575,7 @@ impl<T: paras_inherent::Config> BenchBuilder<T> {
 
 	/// Fill cores `start..last` with dispute statement sets. The statement sets will have 3/4th of
 	/// votes be valid, and 1/4th of votes be invalid.
-	fn create_disputes_with_no_spam(
+	fn create_disputes(
 		&self,
 		start: u32,
 		last: u32,
@@ -664,7 +664,7 @@ impl<T: paras_inherent::Config> BenchBuilder<T> {
 		let backed_candidates = builder
 			.create_backed_candidates(&builder.backed_and_concluding_cores, builder.code_upgrade);
 
-		let disputes = builder.create_disputes_with_no_spam(
+		let disputes = builder.create_disputes(
 			builder.backed_and_concluding_cores.len() as u32,
 			used_cores,
 			builder.dispute_sessions.as_slice(),

diff --git a/runtime/parachains/src/configuration.rs b/runtime/parachains/src/configuration.rs
@@ -192,8 +192,6 @@ pub struct HostConfiguration<BlockNumber> {
 	pub dispute_period: SessionIndex,
 	/// How long after dispute conclusion to accept statements.
 	pub dispute_post_conclusion_acceptance_period: BlockNumber,
-	/// The maximum number of dispute spam slots
-	pub dispute_max_spam_slots: u32,
 	/// How long it takes for a dispute to conclude by time-out, if no supermajority is reached.
 	pub dispute_conclusion_by_time_out_period: BlockNumber,
 	/// The amount of consensus slots that must pass between submitting an assignment and
@@ -263,7 +261,6 @@ impl<BlockNumber: Default + From<u32>> Default for HostConfiguration<BlockNumber
 			max_validators: None,
 			dispute_period: 6,
 			dispute_post_conclusion_acceptance_period: 100.into(),
-			dispute_max_spam_slots: 2,
 			dispute_conclusion_by_time_out_period: 200.into(),
 			n_delay_tranches: Default::default(),
 			zeroth_delay_tranche_width: Default::default(),
@@ -752,19 +749,6 @@ pub mod pallet {
 			})
 		}
 
-		/// Set the maximum number of dispute spam slots.
-		#[pallet::call_index(16)]
-		#[pallet::weight((
-			T::WeightInfo::set_config_with_u32(),
-			DispatchClass::Operational,
-		))]
-		pub fn set_dispute_max_spam_slots(origin: OriginFor<T>, new: u32) -> DispatchResult {
-			ensure_root(origin)?;
-			Self::schedule_config_update(|config| {
-				config.dispute_max_spam_slots = new;
-			})
-		}
-
 		/// Set the dispute conclusion by time out period.
 		#[pallet::call_index(17)]
 		#[pallet::weight((