Better spam slots handling (#4845)

* Only bypass spam slots on actualy approval/backing vote import.

* Fix spam slot treatment.

* More tests.

* Make sure backing statements import works.

* Aaaaaand the actual proper fix.

* Better docs + spelling fixes.

* Fix.

* Typo

Co-authored-by: Robert Habermeier <rphmeier@gmail.com>

* Fix doc.

* Update node/core/dispute-coordinator/src/real/initialized.rs

* minor rewording, line wrap

* fix test case

* Fix obsolete comment.

Co-authored-by: Robert Habermeier <rphmeier@gmail.com>
Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>

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

@@ -244,8 +244,10 @@ impl Initialized {
 			if !overlay_db.is_empty() {
 				let ops = overlay_db.into_write_ops();
 				backend.write(ops)?;
-				confirm_write()?;
 			}
+			// even if the changeset was empty,
+			// otherwise the caller will error.
+			confirm_write()?;
 		}
 	}
 
@@ -882,21 +884,28 @@ impl Initialized {
 
 		// Whether or not we know already that this is a good dispute:
 		//
-		// Note we can only know for sure whether we reached the `byzantine_threshold`  after updating candidate votes above, therefore the spam checking is afterwards:
+		// Note we can only know for sure whether we reached the `byzantine_threshold`  after
+		// updating candidate votes above, therefore the spam checking is afterwards:
 		let is_confirmed = is_included ||
 			was_confirmed ||
 			is_local || votes.voted_indices().len() >
 			byzantine_threshold(n_validators);
 
 		// Potential spam:
 		if !is_confirmed {
-			let mut free_spam_slots = statements.is_empty();
+			let mut free_spam_slots_available = true;
+			// Only allow import if all validators voting invalid, have not exceeded
+			// their spam slots:
 			for (statement, index) in statements.iter() {
-				free_spam_slots |= statement.statement().is_backing() ||
+				// Disputes can only be triggered via an invalidity stating vote, thus we only
+				// need to increase spam slots on invalid votes. (If we did not, we would also
+				// increase spam slots for backing validators for example - as validators have to
+				// provide some opposing vote for dispute-distribution).
+				free_spam_slots_available &= statement.statement().indicates_validity() ||
 					self.spam_slots.add_unconfirmed(session, candidate_hash, *index);
 			}
-			// No reporting validator had a free spam slot:
-			if !free_spam_slots {
+			// Only validity stating votes or validator had free spam slot?
+			if !free_spam_slots_available {
 				tracing::debug!(
 					target: LOG_TARGET,
 					?candidate_hash,

node/core/dispute-coordinator/src/real/spam_slots.rs

@@ -23,19 +23,24 @@ use crate::real::LOG_TARGET;
 /// Type used for counting potential spam votes.
 type SpamCount = u32;
 
-/// How many unconfirmed disputes a validator is allowed to be a participant in (per session).
+/// How many unconfirmed disputes a validator is allowed to import (per session).
 ///
 /// Unconfirmed means: Node has not seen the candidate be included on any chain, it has not cast a
 /// vote itself on that dispute, the dispute has not yet reached more than a third of
 /// validator's votes and the including relay chain block has not yet been finalized.
 ///
 /// Exact number of `MAX_SPAM_VOTES` is not that important here. It is important that the number is
-/// low enough to not cause resource exhaustion, if multiple validators spend their limits. Also
-/// if things are working properly, this number cannot really be too low either, as all relevant
-/// disputes _should_ have been seen as included my enough validators. (Otherwise the candidate
-/// would not have been available in the first place and could not have been included.) So this is
-/// really just a fallback mechanism if things go terribly wrong.
+/// low enough to not cause resource exhaustion (disk & memory) on the importing validator, even if
+/// multiple validators fully make use of their assigned spam slots.
+///
+/// Also if things are working properly, this number cannot really be too low either, as all
+/// relevant disputes _should_ have been seen as included by enough validators. (Otherwise the
+/// candidate would not have been available in the first place and could not have been included.)
+/// So this is really just a fallback mechanism if things go terribly wrong.
+#[cfg(not(test))]
 const MAX_SPAM_VOTES: SpamCount = 50;
+#[cfg(test)]
+const MAX_SPAM_VOTES: SpamCount = 1;
 
 /// Spam slots for raised disputes concerning unknown candidates.
 pub struct SpamSlots {
@@ -76,7 +81,12 @@ impl SpamSlots {
 		Self { slots, unconfirmed: unconfirmed_disputes }
 	}
 
-	/// Add an unconfirmed dispute if free slots are available.
+	/// Increase a "voting invalid" validator's spam slot.
+	///
+	/// This function should get called for any validator's invalidity vote for any not yet
+	/// confirmed dispute.
+	///
+	/// Returns: `true` if validator still had vacant spam slots, `false` otherwise.
 	pub fn add_unconfirmed(
 		&mut self,
 		session: SessionIndex,
@@ -90,14 +100,16 @@ impl SpamSlots {
 		let validators = self.unconfirmed.entry((session, candidate)).or_default();
 
 		if validators.insert(validator) {
+			// We only increment spam slots once per candidate, as each validator has to provide an
+			// opposing vote for sending out its own vote. Therefore, receiving multiple votes for
+			// a single candidate is expected and should not get punished here.
 			*c += 1;
-			true
-		} else {
-			false
 		}
+
+		true
 	}
 
-	/// Clear out spam slots for a given candiate in a session.
+	/// Clear out spam slots for a given candidate in a session.
 	///
 	/// This effectively reduces the spam slot count for all validators participating in a dispute
 	/// for that candidate. You should call this function once a dispute became obsolete or got