diff --git a/polkadot/node/core/dispute-coordinator/src/import.rs b/polkadot/node/core/dispute-coordinator/src/import.rs
index 8eb3d173dcf7bb54bdfb4867602f4850a0c5e5ce..c0f0d3d9e00979230e38942712f3e3fb2bf2c586 100644
--- a/polkadot/node/core/dispute-coordinator/src/import.rs
+++ b/polkadot/node/core/dispute-coordinator/src/import.rs
@@ -28,7 +28,7 @@
use std::collections::{BTreeMap, HashMap, HashSet};
-use polkadot_node_primitives::{CandidateVotes, SignedDisputeStatement};
+use polkadot_node_primitives::{CandidateVotes, DisputeStatus, SignedDisputeStatement, Timestamp};
use polkadot_node_subsystem_util::rolling_session_window::RollingSessionWindow;
use polkadot_primitives::v2::{
CandidateReceipt, DisputeStatement, IndexedVec, SessionIndex, SessionInfo,
@@ -154,22 +154,8 @@ pub struct CandidateVoteState<Votes> {
/// Information about own votes:
own_vote: OwnVoteState,
- /// Whether or not the dispute concluded invalid.
- concluded_invalid: bool,
-
- /// Whether or not the dispute concluded valid.
- ///
- /// Note: Due to equivocations it is technically possible for a dispute to conclude both valid
- /// and invalid. In that case the invalid result takes precedence.
- concluded_valid: bool,
-
- /// There is an ongoing dispute and we reached f+1 votes -> the dispute is confirmed
- ///
- /// as at least one honest validator cast a vote for the candidate.
- is_confirmed: bool,
-
- /// Whether or not we have an ongoing dispute.
- is_disputed: bool,
+ /// Current dispute status, if there is any.
+ dispute_status: Option<DisputeStatus>,
}
impl CandidateVoteState<CandidateVotes> {
@@ -179,18 +165,11 @@ impl CandidateVoteState<CandidateVotes> {
pub fn new_from_receipt(candidate_receipt: CandidateReceipt) -> Self {
let votes =
CandidateVotes { candidate_receipt, valid: BTreeMap::new(), invalid: BTreeMap::new() };
- Self {
- votes,
- own_vote: OwnVoteState::NoVote,
- concluded_invalid: false,
- concluded_valid: false,
- is_confirmed: false,
- is_disputed: false,
- }
+ Self { votes, own_vote: OwnVoteState::NoVote, dispute_status: None }
}
/// Create a new `CandidateVoteState` from already existing votes.
- pub fn new<'a>(votes: CandidateVotes, env: &CandidateEnvironment<'a>) -> Self {
+ pub fn new<'a>(votes: CandidateVotes, env: &CandidateEnvironment<'a>, now: Timestamp) -> Self {
let own_vote = OwnVoteState::new(&votes, env);
let n_validators = env.validators().len();
@@ -198,16 +177,31 @@ impl CandidateVoteState<CandidateVotes> {
let supermajority_threshold =
polkadot_primitives::v2::supermajority_threshold(n_validators);
- let concluded_invalid = votes.invalid.len() >= supermajority_threshold;
- let concluded_valid = votes.valid.len() >= supermajority_threshold;
-
// We have a dispute, if we have votes on both sides:
let is_disputed = !votes.invalid.is_empty() && !votes.valid.is_empty();
- let byzantine_threshold = polkadot_primitives::v2::byzantine_threshold(n_validators);
- let is_confirmed = votes.voted_indices().len() > byzantine_threshold && is_disputed;
+ let dispute_status = if is_disputed {
+ let mut status = DisputeStatus::active();
+ let byzantine_threshold = polkadot_primitives::v2::byzantine_threshold(n_validators);
+ let is_confirmed = votes.voted_indices().len() > byzantine_threshold;
+ if is_confirmed {
+ status = status.confirm();
+ };
+ let concluded_for = votes.valid.len() >= supermajority_threshold;
+ if concluded_for {
+ status = status.conclude_for(now);
+ };
+
+ let concluded_against = votes.invalid.len() >= supermajority_threshold;
+ if concluded_against {
+ status = status.conclude_against(now);
+ };
+ Some(status)
+ } else {
+ None
+ };
- Self { votes, own_vote, concluded_invalid, concluded_valid, is_confirmed, is_disputed }
+ Self { votes, own_vote, dispute_status }
}
/// Import fresh statements.
@@ -217,6 +211,7 @@ impl CandidateVoteState<CandidateVotes> {
self,
env: &CandidateEnvironment,
statements: Vec<(SignedDisputeStatement, ValidatorIndex)>,
+ now: Timestamp,
) -> ImportResult {
let (mut votes, old_state) = self.into_old_state();
@@ -294,7 +289,7 @@ impl CandidateVoteState<CandidateVotes> {
}
}
- let new_state = Self::new(votes, env);
+ let new_state = Self::new(votes, env, now);
ImportResult {
old_state,
@@ -313,32 +308,15 @@ impl CandidateVoteState<CandidateVotes> {
/// Extract `CandidateVotes` for handling import of new statements.
fn into_old_state(self) -> (CandidateVotes, CandidateVoteState<()>) {
- let CandidateVoteState {
- votes,
- own_vote,
- concluded_invalid,
- concluded_valid,
- is_confirmed,
- is_disputed,
- } = self;
- (
- votes,
- CandidateVoteState {
- votes: (),
- own_vote,
- concluded_invalid,
- concluded_valid,
- is_confirmed,
- is_disputed,
- },
- )
+ let CandidateVoteState { votes, own_vote, dispute_status } = self;
+ (votes, CandidateVoteState { votes: (), own_vote, dispute_status })
}
}
impl<V> CandidateVoteState<V> {
/// Whether or not we have an ongoing dispute.
pub fn is_disputed(&self) -> bool {
- self.is_disputed
+ self.dispute_status.is_some()
}
/// Whether there is an ongoing confirmed dispute.
@@ -346,7 +324,7 @@ impl<V> CandidateVoteState<V> {
/// This checks whether there is a dispute ongoing and we have more than byzantine threshold
/// votes.
pub fn is_confirmed(&self) -> bool {
- self.is_confirmed
+ self.dispute_status.map_or(false, |s| s.is_confirmed_concluded())
}
/// This machine already cast some vote in that dispute/for that candidate.
@@ -359,14 +337,19 @@ impl<V> CandidateVoteState<V> {
self.own_vote.approval_votes()
}
- /// Whether or not this dispute has already enough valid votes to conclude.
- pub fn is_concluded_valid(&self) -> bool {
- self.concluded_valid
+ /// Whether or not there is a dispute and it has already enough valid votes to conclude.
+ pub fn has_concluded_for(&self) -> bool {
+ self.dispute_status.map_or(false, |s| s.has_concluded_for())
+ }
+
+ /// Whether or not there is a dispute and it has already enough invalid votes to conclude.
+ pub fn has_concluded_against(&self) -> bool {
+ self.dispute_status.map_or(false, |s| s.has_concluded_against())
}
- /// Whether or not this dispute has already enough invalid votes to conclude.
- pub fn is_concluded_invalid(&self) -> bool {
- self.concluded_invalid
+ /// Get access to the dispute status, in case there is one.
+ pub fn dispute_status(&self) -> &Option<DisputeStatus> {
+ &self.dispute_status
}
/// Access to underlying votes.
@@ -451,18 +434,18 @@ impl ImportResult {
}
/// Whether or not any dispute just concluded valid due to the import.
- pub fn is_freshly_concluded_valid(&self) -> bool {
- !self.old_state().is_concluded_valid() && self.new_state().is_concluded_valid()
+ pub fn is_freshly_concluded_for(&self) -> bool {
+ !self.old_state().has_concluded_for() && self.new_state().has_concluded_for()
}
/// Whether or not any dispute just concluded invalid due to the import.
- pub fn is_freshly_concluded_invalid(&self) -> bool {
- !self.old_state().is_concluded_invalid() && self.new_state().is_concluded_invalid()
+ pub fn is_freshly_concluded_against(&self) -> bool {
+ !self.old_state().has_concluded_against() && self.new_state().has_concluded_against()
}
/// Whether or not any dispute just concluded either invalid or valid due to the import.
pub fn is_freshly_concluded(&self) -> bool {
- self.is_freshly_concluded_invalid() || self.is_freshly_concluded_valid()
+ self.is_freshly_concluded_against() || self.is_freshly_concluded_for()
}
/// Modify this `ImportResult`s, by importing additional approval votes.
@@ -473,6 +456,7 @@ impl ImportResult {
self,
env: &CandidateEnvironment,
approval_votes: HashMap<ValidatorIndex, ValidatorSignature>,
+ now: Timestamp,
) -> Self {
let Self {
old_state,
@@ -508,7 +492,7 @@ impl ImportResult {
}
}
- let new_state = CandidateVoteState::new(votes, env);
+ let new_state = CandidateVoteState::new(votes, env, now);
Self {
old_state,
diff --git a/polkadot/node/core/dispute-coordinator/src/initialized.rs b/polkadot/node/core/dispute-coordinator/src/initialized.rs
index 901a6d863ed2a61565a5c3af51f3fde1469f7da1..0df1a620826ca7d3183fa171a8e93150c42ea553 100644
--- a/polkadot/node/core/dispute-coordinator/src/initialized.rs
+++ b/polkadot/node/core/dispute-coordinator/src/initialized.rs
@@ -748,7 +748,7 @@ impl Initialized {
.load_candidate_votes(session, &candidate_hash)?
.map(CandidateVotes::from)
{
- Some(votes) => CandidateVoteState::new(votes, &env),
+ Some(votes) => CandidateVoteState::new(votes, &env, now),
None =>
if let MaybeCandidateReceipt::Provides(candidate_receipt) = candidate_receipt {
CandidateVoteState::new_from_receipt(candidate_receipt)
@@ -766,7 +766,7 @@ impl Initialized {
gum::trace!(target: LOG_TARGET, ?candidate_hash, ?session, "Loaded votes");
let import_result = {
- let intermediate_result = old_state.import_statements(&env, statements);
+ let intermediate_result = old_state.import_statements(&env, statements, now);
// Handle approval vote import:
//
@@ -803,7 +803,7 @@ impl Initialized {
);
intermediate_result
},
- Ok(votes) => intermediate_result.import_approval_votes(&env, votes),
+ Ok(votes) => intermediate_result.import_approval_votes(&env, votes, now),
}
} else {
gum::trace!(
@@ -977,43 +977,34 @@ impl Initialized {
}
// All good, update recent disputes if state has changed:
- if import_result.dispute_state_changed() {
- let mut recent_disputes = overlay_db.load_recent_disputes()?.unwrap_or_default();
+ if let Some(new_status) = new_state.dispute_status() {
+ // Only bother with db access, if there was an actual change.
+ if import_result.dispute_state_changed() {
+ let mut recent_disputes = overlay_db.load_recent_disputes()?.unwrap_or_default();
+
+ let status =
+ recent_disputes.entry((session, candidate_hash)).or_insert_with(|| {
+ gum::info!(
+ target: LOG_TARGET,
+ ?candidate_hash,
+ session,
+ "New dispute initiated for candidate.",
+ );
+ DisputeStatus::active()
+ });
+
+ *status = *new_status;
- let status = recent_disputes.entry((session, candidate_hash)).or_insert_with(|| {
- gum::info!(
+ gum::trace!(
target: LOG_TARGET,
?candidate_hash,
- session,
- "New dispute initiated for candidate.",
+ ?status,
+ has_concluded_for = ?new_state.has_concluded_for(),
+ has_concluded_against = ?new_state.has_concluded_against(),
+ "Writing recent disputes with updates for candidate"
);
- DisputeStatus::active()
- });
-
- if new_state.is_confirmed() {
- *status = status.confirm();
+ overlay_db.write_recent_disputes(recent_disputes);
}
-
- // Note: concluded-invalid overwrites concluded-valid,
- // so we do this check first. Dispute state machine is
- // non-commutative.
- if new_state.is_concluded_valid() {
- *status = status.concluded_for(now);
- }
-
- if new_state.is_concluded_invalid() {
- *status = status.concluded_against(now);
- }
-
- gum::trace!(
- target: LOG_TARGET,
- ?candidate_hash,
- ?status,
- is_concluded_valid = ?new_state.is_concluded_valid(),
- is_concluded_invalid = ?new_state.is_concluded_invalid(),
- "Writing recent disputes with updates for candidate"
- );
- overlay_db.write_recent_disputes(recent_disputes);
}
// Update metrics:
@@ -1036,7 +1027,7 @@ impl Initialized {
);
self.metrics.on_approval_votes(import_result.imported_approval_votes());
- if import_result.is_freshly_concluded_valid() {
+ if import_result.is_freshly_concluded_for() {
gum::info!(
target: LOG_TARGET,
?candidate_hash,
@@ -1045,7 +1036,7 @@ impl Initialized {
);
self.metrics.on_concluded_valid();
}
- if import_result.is_freshly_concluded_invalid() {
+ if import_result.is_freshly_concluded_against() {
gum::info!(
target: LOG_TARGET,
?candidate_hash,
diff --git a/polkadot/node/core/dispute-coordinator/src/tests.rs b/polkadot/node/core/dispute-coordinator/src/tests.rs
index d7208c1a59ebbaa9a11734611a387301f013bb30..b2f779041a4c03b04db7daf1c8edfcd905e955c4 100644
--- a/polkadot/node/core/dispute-coordinator/src/tests.rs
+++ b/polkadot/node/core/dispute-coordinator/src/tests.rs
@@ -2917,6 +2917,68 @@ fn redundant_votes_ignored() {
});
}
+#[test]
+/// Make sure no disputes are recorded when there are no opposing votes, even if we reached supermajority.
+fn no_onesided_disputes() {
+ test_harness(|mut test_state, mut virtual_overseer| {
+ Box::pin(async move {
+ let session = 1;
+
+ test_state.handle_resume_sync(&mut virtual_overseer, session).await;
+
+ let candidate_receipt = make_valid_candidate_receipt();
+ let candidate_hash = candidate_receipt.hash();
+ test_state
+ .activate_leaf_at_session(&mut virtual_overseer, session, 1, Vec::new())
+ .await;
+
+ let mut statements = Vec::new();
+ for index in 1..10 {
+ statements.push((
+ test_state
+ .issue_backing_statement_with_index(
+ ValidatorIndex(index),
+ candidate_hash,
+ session,
+ )
+ .await,
+ ValidatorIndex(index),
+ ));
+ }
+
+ let (pending_confirmation, confirmation_rx) = oneshot::channel();
+ virtual_overseer
+ .send(FromOrchestra::Communication {
+ msg: DisputeCoordinatorMessage::ImportStatements {
+ candidate_receipt: candidate_receipt.clone(),
+ session,
+ statements,
+ pending_confirmation: Some(pending_confirmation),
+ },
+ })
+ .await;
+ assert_matches!(confirmation_rx.await, Ok(ImportStatementsResult::ValidImport));
+
+ // We should not have any active disputes now.
+ let (tx, rx) = oneshot::channel();
+ virtual_overseer
+ .send(FromOrchestra::Communication {
+ msg: DisputeCoordinatorMessage::ActiveDisputes(tx),
+ })
+ .await;
+
+ assert!(rx.await.unwrap().is_empty());
+
+ virtual_overseer.send(FromOrchestra::Signal(OverseerSignal::Conclude)).await;
+
+ // No more messages expected:
+ assert!(virtual_overseer.try_recv().await.is_none());
+
+ test_state
+ })
+ });
+}
+
#[test]
fn refrain_from_participation() {
test_harness(|mut test_state, mut virtual_overseer| {
diff --git a/polkadot/node/primitives/src/disputes/message.rs b/polkadot/node/primitives/src/disputes/message.rs
index 93d4e804f90604b7fcc0e4bbcce659e9629081cb..1a943f8dcee61c30a240b7927145831960630396 100644
--- a/polkadot/node/primitives/src/disputes/message.rs
+++ b/polkadot/node/primitives/src/disputes/message.rs
@@ -33,6 +33,9 @@ use polkadot_primitives::v2::{
///
/// And most likely has been constructed correctly. This is used with
/// `DisputeDistributionMessage::SendDispute` for sending out votes.
+///
+/// NOTE: This is sent over the wire, any changes are a change in protocol and need to be
+/// versioned.
#[derive(Debug, Clone)]
pub struct DisputeMessage(UncheckedDisputeMessage);
diff --git a/polkadot/node/primitives/src/disputes/status.rs b/polkadot/node/primitives/src/disputes/status.rs
index c0ffb907423b6b2d96b50fae329fd98f0d90c1af..7cc26c1a1f7a93de73dd6be286df030eda9aa8a9 100644
--- a/polkadot/node/primitives/src/disputes/status.rs
+++ b/polkadot/node/primitives/src/disputes/status.rs
@@ -19,8 +19,12 @@ use parity_scale_codec::{Decode, Encode};
/// 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.
+/// The status of dispute.
+///
+/// As managed by the dispute coordinator.
+///
+/// NOTE: This status is persisted to the database, any changes have to be versioned and a db
+/// migration will be needed.
#[derive(Debug, Clone, Copy, Encode, Decode, PartialEq)]
pub enum DisputeStatus {
/// The dispute is active and unconcluded.
@@ -69,9 +73,24 @@ impl DisputeStatus {
}
}
+ /// Concluded valid?
+ pub fn has_concluded_for(&self) -> bool {
+ match self {
+ &DisputeStatus::ConcludedFor(_) => true,
+ _ => false,
+ }
+ }
+ /// Concluded invalid?
+ pub fn has_concluded_against(&self) -> bool {
+ match self {
+ &DisputeStatus::ConcludedAgainst(_) => true,
+ _ => 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 {
+ pub fn conclude_for(self, now: Timestamp) -> DisputeStatus {
match self {
DisputeStatus::Active | DisputeStatus::Confirmed => DisputeStatus::ConcludedFor(now),
DisputeStatus::ConcludedFor(at) => DisputeStatus::ConcludedFor(std::cmp::min(at, now)),
@@ -81,7 +100,7 @@ impl DisputeStatus {
/// 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 {
+ pub fn conclude_against(self, now: Timestamp) -> DisputeStatus {
match self {
DisputeStatus::Active | DisputeStatus::Confirmed =>
DisputeStatus::ConcludedAgainst(now),