Code review feedback

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

@@ -6,7 +6,8 @@ of sessions. Votes older than this session window are pruned.
 
 This subsystem will be the point which produce dispute votes, either positive or negative, based on
 locally-observed validation results as well as a sink for votes received by other subsystems. When
-importing a dispute vote from another node, this will trigger participation in the dispute.
+a statement import makes it clear that a dispute has been raised (there are opposing votes for a
+candidate), the dispute coordinator will make sure the local node will participate in the dispute.
 
 ## Database Schema
 
@@ -29,14 +30,13 @@ This draws on the [dispute statement types][DisputeTypes]
 
 ```rust
 /// Tracked votes on candidates, for the purposes of dispute resolution.
-#[derive(Debug, Clone, Encode, Decode)]
 pub struct CandidateVotes {
-	/// The receipt of the candidate itself.
-	pub candidate_receipt: CandidateReceipt,
-	/// Votes of validity, sorted by validator index.
-	pub valid: Vec<(ValidDisputeStatementKind, ValidatorIndex, ValidatorSignature)>,
-	/// Votes of invalidity, sorted by validator index.
-	pub invalid: Vec<(InvalidDisputeStatementKind, ValidatorIndex, ValidatorSignature)>,
+  /// The receipt of the candidate itself.
+  pub candidate_receipt: CandidateReceipt,
+  /// Votes of validity, sorted by validator index.
+  pub valid: Vec<(ValidDisputeStatementKind, ValidatorIndex, ValidatorSignature)>,
+  /// Votes of invalidity, sorted by validator index.
+  pub invalid: Vec<(InvalidDisputeStatementKind, ValidatorIndex, ValidatorSignature)>,
 }
 
 /// The mapping for recent disputes; any which have not yet been pruned for being ancient.
@@ -46,31 +46,27 @@ pub type RecentDisputes = std::collections::BTreeMap<(SessionIndex, CandidateHas
 /// helper methods.
 #[derive(Debug, Clone, Copy, Encode, Decode, PartialEq)]
 pub enum DisputeStatus {
-	/// The dispute is active and unconcluded.
-	#[codec(index = 0)]
-	Active,
-	/// The dispute has been concluded in favor of the candidate
-	/// since the given timestamp.
-	#[codec(index = 1)]
-	ConcludedFor(Timestamp),
-	/// The dispute has been concluded against the candidate
-	/// since the given timestamp.
-	///
-	/// This takes precedence over `ConcludedFor` in the case that
-	/// both are true, which is impossible unless a large amount of
-	/// validators are participating on both sides.
-	#[codec(index = 2)]
-	ConcludedAgainst(Timestamp),
-	/// Dispute has been confirmed (more than `byzantine_threshold` have already participated/ or
-	/// we have seen the candidate included already/participated successfully ourselves).
-	#[codec(index = 3)]
-	Confirmed,
+  /// The dispute is active and unconcluded.
+  Active,
+  /// The dispute has been concluded in favor of the candidate
+  /// since the given timestamp.
+  ConcludedFor(Timestamp),
+  /// The dispute has been concluded against the candidate
+  /// since the given timestamp.
+  ///
+  /// This takes precedence over `ConcludedFor` in the case that
+  /// both are true, which is impossible unless a large amount of
+  /// validators are participating on both sides.
+  ConcludedAgainst(Timestamp),
+  /// Dispute has been confirmed (more than `byzantine_threshold` have already participated/ or
+  /// we have seen the candidate included already/participated successfully ourselves).
+  Confirmed,
 }
 ```
 
 ## Internal modules
 Dispute coordinator subsystem includes a few internal modules - `ordering`, `participation` and
-`spam_slots`. 
+`spam_slots`.
 
 ### Ordering
 Ordering module contains two structs - `OrderingProvider` and `CandidateComparator`. The former
@@ -81,20 +77,20 @@ instances for candidates.
 
 ```rust
 pub struct CandidateComparator {
-	/// Block number of the relay parent.
-	///
-	/// Important, so we will be participating in oldest disputes first.
-	///
-	/// Note: In theory it would make more sense to use the `BlockNumber` of the including
-	/// block, as inclusion time is the actual relevant event when it comes to ordering. The
-	/// problem is, that a candidate can get included multiple times on forks, so the `BlockNumber`
-	/// of the including block is not unique. We could theoretically work around that problem, by
-	/// just using the lowest `BlockNumber` of all available including blocks - the problem is,
-	/// that is not stable. If a new fork appears after the fact, we would start ordering the same
-	/// candidate differently, which would result in the same candidate getting queued twice.
-	relay_parent_block_number: BlockNumber,
-	/// By adding the `CandidateHash`, we can guarantee a unique ordering across candidates.
-	candidate_hash: CandidateHash,
+  /// Block number of the relay parent.
+  ///
+  /// Important, so we will be participating in oldest disputes first.
+  ///
+  /// Note: In theory it would make more sense to use the `BlockNumber` of the including
+  /// block, as inclusion time is the actual relevant event when it comes to ordering. The
+  /// problem is, that a candidate can get included multiple times on forks, so the `BlockNumber`
+  /// of the including block is not unique. We could theoretically work around that problem, by
+  /// just using the lowest `BlockNumber` of all available including blocks - the problem is,
+  /// that is not stable. If a new fork appears after the fact, we would start ordering the same
+  /// candidate differently, which would result in the same candidate getting queued twice.
+  relay_parent_block_number: BlockNumber,
+  /// By adding the `CandidateHash`, we can guarantee a unique ordering across candidates.
+  candidate_hash: CandidateHash,
 }
 ```
 
@@ -112,14 +108,14 @@ module is:
 
 ```rust
 pub struct Participation {
-	/// Participations currently being processed.
-	running_participations: HashSet<CandidateHash>,
-	/// Priority and best effort queues.
-	queue: Queues,
-	/// Sender to be passed to worker tasks.
-	worker_sender: WorkerMessageSender,
-	/// Some recent block for retrieving validation code from chain.
-	recent_block: Option<(BlockNumber, Hash)>,
+  /// Participations currently being processed.
+  running_participations: HashSet<CandidateHash>,
+  /// Priority and best effort queues.
+  queue: Queues,
+  /// Sender to be passed to worker tasks.
+  worker_sender: WorkerMessageSender,
+  /// Some recent block for retrieving validation code from chain.
+  recent_block: Option<(BlockNumber, Hash)>,
 }
 ```
 New candidate is added for processing by calling `fn queue_participation()`. The function either
@@ -155,13 +151,13 @@ What unconfirmed dispute means? Quote from the source code provides an excellent
 
 ```rust
 pub struct SpamSlots {
-	/// Counts per validator and session.
-	///
-	/// Must not exceed `MAX_SPAM_VOTES`.
-	slots: HashMap<(SessionIndex, ValidatorIndex), SpamCount>,
-
-	/// All unconfirmed candidates we are aware of right now.
-	unconfirmed: UnconfirmedDisputes,
+  /// Counts per validator and session.
+  ///
+  /// Must not exceed `MAX_SPAM_VOTES`.
+  slots: HashMap<(SessionIndex, ValidatorIndex), SpamCount>,
+  
+  /// All unconfirmed candidates we are aware of right now.
+  unconfirmed: UnconfirmedDisputes,
 }
 ```
 
@@ -172,9 +168,9 @@ message handling (there is a dedicated section for it below). Spam slots are ind
 and validator index. For each such pair there is a limit of active disputes. If this limit is
 reached - the import is ignored.
 
-Spam protection is performed only on invalid vote statements which are not included in any chain,
-not confirmed, not local and the votes hasn't reached the byzantine threshold. This check is
-performed by `Ordering` module.
+Spam protection is performed only on invalid vote statements where the concerned candidate is not
+included on any chain, not confirmed, not local and the votes hasn't reached the byzantine
+threshold. This check is performed by `Ordering` module.
 
 Spam slots are cleared when the session window advances so that the `SpamSlots` state doesn't grow
 indefinitely.
@@ -195,18 +191,18 @@ Ephemeral in-memory state:
 ```rust
 struct State {
   keystore: Arc<LocalKeystore>,
-	rolling_session_window: RollingSessionWindow,
-	highest_session: SessionIndex,
-	spam_slots: SpamSlots,
-	participation: Participation,
-	ordering_provider: OrderingProvider,
-	participation_receiver: WorkerMessageReceiver,
-	metrics: Metrics,
-	// This tracks only rolling session window failures.
-	// It can be a `Vec` if the need to track more arises.
-	error: Option<SessionsUnavailable>,
-	/// Latest relay blocks that have been successfully scraped.
-	last_scraped_blocks: LruCache<Hash, ()>,
+  rolling_session_window: RollingSessionWindow,
+  highest_session: SessionIndex,
+  spam_slots: SpamSlots,
+  participation: Participation,
+  ordering_provider: OrderingProvider,
+  participation_receiver: WorkerMessageReceiver,
+  metrics: Metrics,
+  // This tracks only rolling session window failures.
+  // It can be a `Vec` if the need to track more arises.
+  error: Option<SessionsUnavailable>,
+  /// Latest relay blocks that have been successfully scraped.
+  last_scraped_blocks: LruCache<Hash, ()>,
 }
 ```