Some late short-term fixes for dispute slashing

runtime/parachains/src/disputes.rs

@@ -81,6 +81,7 @@ pub trait SlashingHandler<BlockNumber> {
 		session: SessionIndex,
 		candidate_hash: CandidateHash,
 		losers: impl IntoIterator<Item = ValidatorIndex>,
+		backers: impl IntoIterator<Item = ValidatorIndex>,
 	);
 
 	/// Punish a series of validators who were against a valid parablock. This
@@ -89,6 +90,7 @@ pub trait SlashingHandler<BlockNumber> {
 		session: SessionIndex,
 		candidate_hash: CandidateHash,
 		losers: impl IntoIterator<Item = ValidatorIndex>,
+		backers: impl IntoIterator<Item = ValidatorIndex>,
 	);
 
 	/// Called by the initializer to initialize the slashing pallet.
@@ -106,13 +108,15 @@ impl<BlockNumber> SlashingHandler<BlockNumber> for () {
 		_: SessionIndex,
 		_: CandidateHash,
 		_: impl IntoIterator<Item = ValidatorIndex>,
+		_: impl IntoIterator<Item = ValidatorIndex>,
 	) {
 	}
 
 	fn punish_against_valid(
 		_: SessionIndex,
 		_: CandidateHash,
 		_: impl IntoIterator<Item = ValidatorIndex>,
+		_: impl IntoIterator<Item = ValidatorIndex>,
 	) {
 	}
 
@@ -459,6 +463,18 @@ pub mod pallet {
 		DisputeState<T::BlockNumber>,
 	>;
 
+	/// Backing votes stored for each dispute.
+	/// This storage is used for slashing.
+	#[pallet::storage]
+	pub(super) type BackersOnDisputes<T: Config> = StorageDoubleMap<
+		_,
+		Twox64Concat,
+		SessionIndex,
+		Blake2_128Concat,
+		CandidateHash,
+		Vec<ValidatorIndex>,
+	>;
+
 	/// 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.
 	#[pallet::storage]
@@ -521,6 +537,8 @@ pub mod pallet {
 		PotentialSpam,
 		/// A dispute where there are only votes on one side.
 		SingleSidedDispute,
+		/// No backing votes were provides along dispute statements.
+		MissingBackingVotes,
 	}
 
 	#[pallet::call]
@@ -888,6 +906,8 @@ impl<T: Config> Pallet<T> {
 				// This should be small, as disputes are rare, so `None` is fine.
 				#[allow(deprecated)]
 				<Disputes<T>>::remove_prefix(to_prune, None);
+				#[allow(deprecated)]
+				<BackersOnDisputes<T>>::remove_prefix(to_prune, None);
 
 				// This is larger, and will be extracted to the `shared` pallet for more proper pruning.
 				// TODO: https://github.com/paritytech/polkadot/issues/3469
@@ -984,7 +1004,8 @@ impl<T: Config> Pallet<T> {
 		let mut summary = {
 			let mut importer = DisputeStateImporter::new(dispute_state, now);
 			for (i, (statement, validator_index, signature)) in set.statements.iter().enumerate() {
-				// assure the validator index and is present in the session info
+				// ensure the validator index is present in the session info
+				// and the signature is valid
 				let validator_public = match session_info.validators.get(*validator_index) {
 					None => {
 						filter.remove_index(i);
@@ -1181,13 +1202,24 @@ impl<T: Config> Pallet<T> {
 			}
 		};
 
+		let mut backers =
+			<BackersOnDisputes<T>>::get(&set.session, &set.candidate_hash).unwrap_or_default();
+
 		// Import all votes. They were pre-checked.
 		let summary = {
 			let mut importer = DisputeStateImporter::new(dispute_state, now);
 			for (statement, validator_index, _signature) in &set.statements {
 				let valid = statement.indicates_validity();
 
 				importer.import(*validator_index, valid).map_err(Error::<T>::from)?;
+
+				match statement {
+					DisputeStatement::Valid(ValidDisputeStatementKind::BackingSeconded(_)) |
+					DisputeStatement::Valid(ValidDisputeStatementKind::BackingValid(_)) => {
+						backers.push(validator_index.clone());
+					},
+					_ => {},
+				}
 			}
 
 			importer.finish()
@@ -1200,6 +1232,11 @@ impl<T: Config> Pallet<T> {
 			Error::<T>::SingleSidedDispute,
 		);
 
+		// Reject statements with no accompanying backing votes.
+		ensure!(!backers.is_empty(), Error::<T>::MissingBackingVotes,);
+
+		<BackersOnDisputes<T>>::insert(&set.session, &set.candidate_hash, backers.clone());
+
 		let DisputeStatementSet { ref session, ref candidate_hash, .. } = set;
 		let session = *session;
 		let candidate_hash = *candidate_hash;
@@ -1246,10 +1283,16 @@ impl<T: Config> Pallet<T> {
 				session,
 				candidate_hash,
 				summary.slash_against,
+				backers.clone(),
 			);
 
 			// an invalid candidate, according to 2/3. Punish those on the 'for' side.
-			T::SlashingHandler::punish_for_invalid(session, candidate_hash, summary.slash_for);
+			T::SlashingHandler::punish_for_invalid(
+				session,
+				candidate_hash,
+				summary.slash_for,
+				backers,
+			);
 		}
 
 		<Disputes<T>>::insert(&session, &candidate_hash, &summary.state);

runtime/parachains/src/disputes/slashing.rs

@@ -16,32 +16,31 @@
 
 //! Dispute slashing pallet.
 //!
-//! Once a dispute is concluded, we want to slash validators
-//! who were on the wrong side of the dispute. The slashing amount
-//! depends on whether the candidate was valid (small) or invalid (big).
-//! In addition to that, we might want to kick out the validators from the
-//! active set.
+//! Once a dispute is concluded, we want to slash validators who were on the
+//! wrong side of the dispute. The slashing amount depends on whether the
+//! candidate was valid (none at the moment) or invalid (big). In addition to
+//! that, we might want to kick out the validators from the active set.
+//! Currently, we limit slashing to the backing group for invalid disputes.
 //!
 //! The `offences` pallet from Substrate provides us with a way to do both.
-//! Currently, the interface expects us to provide staking information
-//! including nominator exposure in order to submit an offence.
+//! Currently, the interface expects us to provide staking information including
+//! nominator exposure in order to submit an offence.
 //!
 //! Normally, we'd able to fetch this information from the runtime as soon as
 //! the dispute is concluded. This is also what `im-online` pallet does.
 //! However, since a dispute can conclude several sessions after the candidate
 //! was backed (see `dispute_period` in `HostConfiguration`), we can't rely on
 //! this information be available in the context of the current block. The
-//! `babe` and `grandpa` equivocation handlers also have to deal
-//! with this problem.
+//! `babe` and `grandpa` equivocation handlers also have to deal with this
+//! problem.
 //!
 //! Our implementation looks like a hybrid of `im-online` and `grandpa`
 //! equivocation handlers. Meaning, we submit an `offence` for the concluded
-//! disputes about the current session candidate directly from the runtime.
-//! If, however, the dispute is about a past session, we record unapplied
-//! slashes on chain, without `FullIdentification` of the offenders.
-//! Later on, a block producer can submit an unsigned transaction with
-//! `KeyOwnershipProof` of an offender and submit it to the runtime
-//! to produce an offence.
+//! disputes about the current session candidate directly from the runtime. If,
+//! however, the dispute is about a past session, we record unapplied slashes on
+//! chain, without `FullIdentification` of the offenders. Later on, a block
+//! producer can submit an unsigned transaction with `KeyOwnershipProof` of an
+//! offender and submit it to the runtime to produce an offence.
 
 use crate::{disputes, initializer::ValidatorSetCount, session_info::IdentificationTuple};
 use frame_support::{
@@ -64,7 +63,10 @@ use sp_runtime::{
 use sp_session::{GetSessionNumber, GetValidatorCount};
 use sp_staking::offence::{DisableStrategy, Kind, Offence, OffenceError, ReportOffence};
 use sp_std::{
-	collections::btree_map::{BTreeMap, Entry},
+	collections::{
+		btree_map::{BTreeMap, Entry},
+		btree_set::BTreeSet,
+	},
 	prelude::*,
 };
 
@@ -73,7 +75,7 @@ const LOG_TARGET: &str = "runtime::parachains::slashing";
 // These are constants, but we want to make them configurable
 // via `HostConfiguration` in the future.
 const SLASH_FOR_INVALID: Perbill = Perbill::from_percent(100);
-const SLASH_AGAINST_VALID: Perbill = Perbill::from_perthousand(1);
+const SLASH_AGAINST_VALID: Perbill = Perbill::zero();
 const DEFENSIVE_PROOF: &'static str = "disputes module should bail on old session";
 
 #[cfg(feature = "runtime-benchmarks")]
@@ -228,18 +230,40 @@ where
 		candidate_hash: CandidateHash,
 		kind: SlashingOffenceKind,
 		losers: impl IntoIterator<Item = ValidatorIndex>,
+		backers: impl IntoIterator<Item = ValidatorIndex>,
 	) {
-		let losers: Vec<ValidatorIndex> = losers.into_iter().collect();
-		if losers.is_empty() {
-			// Nothing to do
+		// sanity check for the current implementation
+		if kind == SlashingOffenceKind::AgainstValid {
+			debug_assert!(false, "should only slash ForInvalid disputes");
+			return
+		}
+		let backers: Vec<ValidatorIndex> = backers.into_iter().collect();
+		debug_assert!(
+			{
+				let losers_index: BTreeSet<_> = losers.into_iter().collect();
+				backers.iter().all(|i| losers_index.contains(i))
+			},
+			"backers should be a subset of losing ForInvalid validators",
+		);
+		debug_assert_eq!(
+			{
+				let backers_dedup: BTreeSet<_> = backers.iter().collect();
+				backers_dedup.len()
+			},
+			backers.len(),
+			"backers should be deduplicated, this is ensured by disputes pallet",
+		);
+		debug_assert!(!backers.is_empty(), "backers should not be empty",);
+		if backers.is_empty() {
 			return
 		}
 		let session_info = crate::session_info::Pallet::<T>::session_info(session_index);
 		let session_info = match session_info.defensive_proof(DEFENSIVE_PROOF) {
 			Some(info) => info,
 			None => return,
 		};
-		let maybe = Self::maybe_identify_validators(session_index, losers.iter().cloned());
+
+		let maybe = Self::maybe_identify_validators(session_index, backers.iter().cloned());
 		if let Some(offenders) = maybe {
 			let validator_set_count = session_info.discovery_keys.len() as ValidatorSetCount;
 			let offence = SlashingOffence::new(
@@ -255,7 +279,7 @@ where
 			return
 		}
 
-		let keys = losers
+		let keys = backers
 			.into_iter()
 			.filter_map(|i| session_info.validators.get(i).cloned().map(|id| (i, id)))
 			.collect();
@@ -272,18 +296,20 @@ where
 		session_index: SessionIndex,
 		candidate_hash: CandidateHash,
 		losers: impl IntoIterator<Item = ValidatorIndex>,
+		backers: impl IntoIterator<Item = ValidatorIndex>,
 	) {
 		let kind = SlashingOffenceKind::ForInvalid;
-		Self::do_punish(session_index, candidate_hash, kind, losers);
+		Self::do_punish(session_index, candidate_hash, kind, losers, backers);
 	}
 
 	fn punish_against_valid(
-		session_index: SessionIndex,
-		candidate_hash: CandidateHash,
-		losers: impl IntoIterator<Item = ValidatorIndex>,
+		_session_index: SessionIndex,
+		_candidate_hash: CandidateHash,
+		_losers: impl IntoIterator<Item = ValidatorIndex>,
+		_backers: impl IntoIterator<Item = ValidatorIndex>,
 	) {
-		let kind = SlashingOffenceKind::AgainstValid;
-		Self::do_punish(session_index, candidate_hash, kind, losers);
+		// do nothing for now
+		// NOTE: changing that requires modifying `do_punish` implementation
 	}
 
 	fn initializer_initialize(now: T::BlockNumber) -> Weight {

runtime/parachains/src/disputes/slashing/benchmarking.rs

@@ -109,8 +109,9 @@ where
 
 	let validator_index = ValidatorIndex(0);
 	let losers = [validator_index].into_iter();
+	let backers = losers.clone();
 
-	T::SlashingHandler::punish_against_valid(session_index, CANDIDATE_HASH, losers);
+	T::SlashingHandler::punish_for_invalid(session_index, CANDIDATE_HASH, losers, backers);
 
 	let unapplied = <UnappliedSlashes<T>>::get(session_index, CANDIDATE_HASH);
 	assert_eq!(unapplied.unwrap().keys.len(), 1);
@@ -123,7 +124,7 @@ fn dispute_proof(
 	validator_id: ValidatorId,
 	validator_index: ValidatorIndex,
 ) -> DisputeProof {
-	let kind = SlashingOffenceKind::AgainstValid;
+	let kind = SlashingOffenceKind::ForInvalid;
 	let time_slot = DisputesTimeSlot::new(session_index, CANDIDATE_HASH);
 
 	DisputeProof { time_slot, kind, validator_index, validator_id }
@@ -134,7 +135,7 @@ benchmarks! {
 		where T: Config<KeyOwnerProof = MembershipProof>,
 	}
 
-	// in this setup we have a single `AgainstValid` dispute
+	// in this setup we have a single `ForInvalid` dispute
 	// submitted for a past session
 	report_dispute_lost {
 		let n in 4..<<T as super::Config>::BenchmarkingConfig as BenchmarkingConfiguration>::MAX_VALIDATORS;