Improved dispute votes import in provisioner (#5567)

* Add `DisputeState` to `DisputeCoordinatorMessage::RecentDisputes`

The new signature of the message is:
```
RecentDisputes(oneshot::Sender<Vec<(SessionIndex, CandidateHash, DisputeStatus)>>),
```

As part of the change also add `DispiteStatus` to
`polkadot_node_primitives`.

* Move dummy_signature() in primitives/test-helpers

* Enable staging runtime api on Rococo

* Implementation

* Move disputes to separate module
* Vote prioritisation
* Duplicates handling
* Double vote handling
* Unit tests
* Logs and metrics
* Code review feedback
* Fix ACTIVE/INACTIVE separation and update partition names
* Add `fn dispute_is_inactive` to node primitives and refactor `fn get_active_with_status()` logic
* Keep the 'old' logic if the staging api is not enabled
* Fix some comments in tests
* Add warning message if there are any inactive_unknown_onchain disputes
* Add file headers and remove `use super::*;` usage outside tests
* Adding doc comments
* Fix test methods names

* Fix staging api usage

* Fix `get_disputes` runtime function implementation

* Fix compilation error

* Fix arithmetic operations in tests

* Use smaller test data

* Rename `RuntimeApiRequest::StagingDisputes` to `RuntimeApiRequest::Disputes`

* Remove `staging-client` feature flag

* fmt

* Remove `vstaging` feature flag

* Some comments regarding the staging api

* Rename dispute selection modules in provisioner
with_staging_api -> prioritized_selection
without_staging_api -> random_selection

* Comments for staging api

* Comments

* Additional logging

* Code review feedback

process_selected_disputes -> into_multi_dispute_statement_set
typo
In trait VoteType: vote_value -> is_valid

* Code review feedback

* Fix metrics

* get_disputes -> disputes

* Get time only once during partitioning

* Fix partitioning

* Comments

* Reduce the number of hardcoded api versions

* Code review feedback

* Unused import

* Comments

* More precise log messages

* Code review feedback

* Code review feedback

* Code review feedback - remove `trait VoteType`

* Code review feedback

* Trace log for DisputeCoordinatorMessage::QueryCandidateVotes counter in vote_selection

Cargo.toml

@@ -199,7 +199,6 @@ try-runtime = [ "polkadot-cli/try-runtime" ]
 fast-runtime = [ "polkadot-cli/fast-runtime" ]
 runtime-metrics = [ "polkadot-cli/runtime-metrics" ]
 pyroscope = ["polkadot-cli/pyroscope"]
-staging-client = ["polkadot-cli/staging-client"]
 
 # Configuration for building a .deb package - for use with `cargo-deb`
 [package.metadata.deb]

cli/Cargo.toml

@@ -74,4 +74,3 @@ rococo-native = ["service/rococo-native"]
 
 malus = ["full-node", "service/malus"]
 runtime-metrics = ["service/runtime-metrics", "polkadot-node-metrics/runtime-metrics"]
-staging-client = ["service/staging-client"]

node/core/dispute-coordinator/src/db/v1.rs

@@ -16,6 +16,7 @@
 
 //! `V1` database for the dispute coordinator.
 
+use polkadot_node_primitives::DisputeStatus;
 use polkadot_node_subsystem::{SubsystemError, SubsystemResult};
 use polkadot_node_subsystem_util::database::{DBTransaction, Database};
 use polkadot_primitives::v2::{
@@ -31,7 +32,6 @@ use crate::{
 	backend::{Backend, BackendWriteOp, OverlayedBackend},
 	error::{FatalError, FatalResult},
 	metrics::Metrics,
-	status::DisputeStatus,
 	DISPUTE_WINDOW, LOG_TARGET,
 };
 

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

@@ -26,8 +26,8 @@ use futures::{
 use sc_keystore::LocalKeystore;
 
 use polkadot_node_primitives::{
-	CandidateVotes, DisputeMessage, DisputeMessageCheckError, SignedDisputeStatement,
-	DISPUTE_WINDOW,
+	CandidateVotes, DisputeMessage, DisputeMessageCheckError, DisputeStatus,
+	SignedDisputeStatement, Timestamp, DISPUTE_WINDOW,
 };
 use polkadot_node_subsystem::{
 	messages::{
@@ -49,7 +49,7 @@ use crate::{
 	error::{log_error, Error, FatalError, FatalResult, JfyiError, JfyiResult, Result},
 	import::{CandidateEnvironment, CandidateVoteState},
 	metrics::Metrics,
-	status::{get_active_with_status, Clock, DisputeStatus, Timestamp},
+	status::{get_active_with_status, Clock},
 	DisputeCoordinatorSubsystem, LOG_TARGET,
 };
 
@@ -599,7 +599,9 @@ impl Initialized {
 				};
 				gum::trace!(target: LOG_TARGET, "Loaded recent disputes from db");
 
-				let _ = tx.send(recent_disputes.keys().cloned().collect());
+				let _ = tx.send(
+					recent_disputes.into_iter().map(|(k, v)| (k.0, k.1, v)).collect::<Vec<_>>(),
+				);
 			},
 			DisputeCoordinatorMessage::ActiveDisputes(tx) => {
 				// Return error if session information is missing.

node/core/dispute-coordinator/src/status.rs

@@ -14,125 +14,18 @@
 // You should have received a copy of the GNU General Public License
 // along with Polkadot.  If not, see <http://www.gnu.org/licenses/>.
 
-use std::time::{SystemTime, UNIX_EPOCH};
-
-use parity_scale_codec::{Decode, Encode};
+use polkadot_node_primitives::{dispute_is_inactive, DisputeStatus, Timestamp};
 use polkadot_primitives::v2::{CandidateHash, SessionIndex};
+use std::time::{SystemTime, UNIX_EPOCH};
 
 use crate::LOG_TARGET;
 
-/// The choice here is fairly arbitrary. But any dispute that concluded more than a few minutes ago
-/// is not worth considering anymore. Changing this value has little to no bearing on consensus,
-/// and really only affects the work that the node might do on startup during periods of many
-/// disputes.
-pub const ACTIVE_DURATION_SECS: Timestamp = 180;
-
-/// Timestamp based on the 1 Jan 1970 UNIX base, which is persistent across node restarts and OS reboots.
-pub type Timestamp = u64;
-
-/// The status of dispute. This is a state machine which can be altered by the
-/// 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,
-}
-
-impl DisputeStatus {
-	/// Initialize the status to the active state.
-	pub fn active() -> DisputeStatus {
-		DisputeStatus::Active
-	}
-
-	/// Move status to confirmed status, if not yet concluded/confirmed already.
-	pub fn confirm(self) -> DisputeStatus {
-		match self {
-			DisputeStatus::Active => DisputeStatus::Confirmed,
-			DisputeStatus::Confirmed => DisputeStatus::Confirmed,
-			DisputeStatus::ConcludedFor(_) | DisputeStatus::ConcludedAgainst(_) => self,
-		}
-	}
-
-	/// Check whether the dispute is not a spam dispute.
-	pub fn is_confirmed_concluded(&self) -> bool {
-		match self {
-			&DisputeStatus::Confirmed |
-			&DisputeStatus::ConcludedFor(_) |
-			DisputeStatus::ConcludedAgainst(_) => true,
-			&DisputeStatus::Active => false,
-		}
-	}
-
-	/// Transition the status to a new status after observing the dispute has concluded for the candidate.
-	/// This may be a no-op if the status was already concluded.
-	pub fn concluded_for(self, now: Timestamp) -> DisputeStatus {
-		match self {
-			DisputeStatus::Active | DisputeStatus::Confirmed => DisputeStatus::ConcludedFor(now),
-			DisputeStatus::ConcludedFor(at) => DisputeStatus::ConcludedFor(std::cmp::min(at, now)),
-			against => against,
-		}
-	}
-
-	/// Transition the status to a new status after observing the dispute has concluded against the candidate.
-	/// This may be a no-op if the status was already concluded.
-	pub fn concluded_against(self, now: Timestamp) -> DisputeStatus {
-		match self {
-			DisputeStatus::Active | DisputeStatus::Confirmed =>
-				DisputeStatus::ConcludedAgainst(now),
-			DisputeStatus::ConcludedFor(at) =>
-				DisputeStatus::ConcludedAgainst(std::cmp::min(at, now)),
-			DisputeStatus::ConcludedAgainst(at) =>
-				DisputeStatus::ConcludedAgainst(std::cmp::min(at, now)),
-		}
-	}
-
-	/// Whether the disputed candidate is possibly invalid.
-	pub fn is_possibly_invalid(&self) -> bool {
-		match self {
-			DisputeStatus::Active |
-			DisputeStatus::Confirmed |
-			DisputeStatus::ConcludedAgainst(_) => true,
-			DisputeStatus::ConcludedFor(_) => false,
-		}
-	}
-
-	/// Yields the timestamp this dispute concluded at, if any.
-	pub fn concluded_at(&self) -> Option<Timestamp> {
-		match self {
-			DisputeStatus::Active | DisputeStatus::Confirmed => None,
-			DisputeStatus::ConcludedFor(at) | DisputeStatus::ConcludedAgainst(at) => Some(*at),
-		}
-	}
-}
-
 /// Get active disputes as iterator, preserving its `DisputeStatus`.
 pub fn get_active_with_status(
 	recent_disputes: impl Iterator<Item = ((SessionIndex, CandidateHash), DisputeStatus)>,
 	now: Timestamp,
 ) -> impl Iterator<Item = ((SessionIndex, CandidateHash), DisputeStatus)> {
-	recent_disputes.filter_map(move |(disputed, status)| {
-		status
-			.concluded_at()
-			.filter(|at| *at + ACTIVE_DURATION_SECS < now)
-			.map_or(Some((disputed, status)), |_| None)
-	})
+	recent_disputes.filter(move |(_, status)| !dispute_is_inactive(status, &now))
 }
 
 pub trait Clock: Send + Sync {

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

@@ -49,6 +49,7 @@ use sp_keyring::Sr25519Keyring;
 use sp_keystore::{SyncCryptoStore, SyncCryptoStorePtr};
 
 use ::test_helpers::{dummy_candidate_receipt_bad_sig, dummy_digest, dummy_hash};
+use polkadot_node_primitives::{Timestamp, ACTIVE_DURATION_SECS};
 use polkadot_node_subsystem::{
 	jaeger,
 	messages::{AllMessages, BlockDescription, RuntimeApiMessage, RuntimeApiRequest},
@@ -66,7 +67,7 @@ use crate::{
 	backend::Backend,
 	metrics::Metrics,
 	participation::{participation_full_happy_path, participation_missing_availability},
-	status::{Clock, Timestamp, ACTIVE_DURATION_SECS},
+	status::Clock,
 	Config, DisputeCoordinatorSubsystem,
 };
 

node/core/provisioner/Cargo.toml

@@ -13,15 +13,12 @@ polkadot-primitives = { path = "../../../primitives" }
 polkadot-node-primitives = { path = "../../primitives" }
 polkadot-node-subsystem = { path = "../../subsystem" }
 polkadot-node-subsystem-util = { path = "../../subsystem-util" }
-futures-timer = "3.0.2"
 rand = "0.8.5"
+futures-timer = "3.0.2"
 fatality = "0.0.6"
 
 [dev-dependencies]
 sp-application-crypto = { git = "https://github.com/paritytech/substrate", branch = "master" }
 sp-keystore = { git = "https://github.com/paritytech/substrate", branch = "master" }
 polkadot-node-subsystem-test-helpers = { path = "../../subsystem-test-helpers" }
 test-helpers = { package = "polkadot-primitives-test-helpers", path = "../../../primitives/test-helpers" }
-
-[features]
-staging-client = []

node/core/provisioner/src/disputes/mod.rs

@@ -0,0 +1,53 @@
+// Copyright 2017-2022 Parity Technologies (UK) Ltd.
+// This file is part of Polkadot.
+
+// Polkadot is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// Polkadot is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with Polkadot.  If not, see <http://www.gnu.org/licenses/>.
+
+//! The disputes module is responsible for selecting dispute votes to be sent with the inherent data. It contains two
+//! different implementations, extracted in two separate modules - `random_selection` and `prioritized_selection`. Which
+//! implementation will be executed depends on the version of the runtime. Runtime v2 supports `random_selection`. Runtime
+//! v3 and above - `prioritized_selection`. The entrypoint to these implementations is the `select_disputes` function.
+//! prioritized_selection` is considered superior and will be the default one in the future. Refer to the documentation of
+//! the modules for more details about each implementation.
+
+use crate::LOG_TARGET;
+use futures::channel::oneshot;
+use polkadot_node_primitives::CandidateVotes;
+use polkadot_node_subsystem::{messages::DisputeCoordinatorMessage, overseer};
+use polkadot_primitives::v2::{CandidateHash, SessionIndex};
+
+/// Request the relevant dispute statements for a set of disputes identified by `CandidateHash` and the `SessionIndex`.
+async fn request_votes(
+	sender: &mut impl overseer::ProvisionerSenderTrait,
+	disputes_to_query: Vec<(SessionIndex, CandidateHash)>,
+) -> Vec<(SessionIndex, CandidateHash, CandidateVotes)> {
+	let (tx, rx) = oneshot::channel();
+	// Bounded by block production - `ProvisionerMessage::RequestInherentData`.
+	sender.send_unbounded_message(DisputeCoordinatorMessage::QueryCandidateVotes(
+		disputes_to_query,
+		tx,
+	));
+
+	match rx.await {
+		Ok(v) => v,
+		Err(oneshot::Canceled) => {
+			gum::warn!(target: LOG_TARGET, "Unable to query candidate votes");
+			Vec::new()
+		},
+	}
+}
+
+pub(crate) mod prioritized_selection;
+
+pub(crate) mod random_selection;