Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Commit

Permalink
Only report concluded if there is an actual dispute. (#6270)
Browse files Browse the repository at this point in the history 
* Only report concluded if there is an actual dispute.

Hence no "non"-disputes will be added to disputes anymore.

* Fix redundant check.

* Test for no onesided disputes.

Co-authored-by: eskimor <eskimor@no-such-url.com>
  • Loading branch information
@eskimor
eskimor and eskimor authored Nov 14, 2022
1 parent 1d46275 commit 89dc70f
Show file tree
Hide file tree
Showing 5 changed files with 166 additions and 107 deletions.
116 changes: 50 additions & 66 deletions node/core/dispute-coordinator/src/import.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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> {
Expand All @@ -179,35 +165,43 @@ 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();

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.
Expand All @@ -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();

Expand Down Expand Up @@ -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,
Expand All @@ -313,40 +308,23 @@ 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.
///
/// 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.
Expand All @@ -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.
Expand Down Expand Up @@ -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.
Expand All @@ -473,6 +456,7 @@ impl ImportResult {
self,
env: &CandidateEnvironment,
approval_votes: HashMap<ValidatorIndex, ValidatorSignature>,
now: Timestamp,
) -> Self {
let Self {
old_state,
Expand Down Expand Up @@ -508,7 +492,7 @@ impl ImportResult {
}
}

let new_state = CandidateVoteState::new(votes, env);
let new_state = CandidateVoteState::new(votes, env, now);

Self {
old_state,
Expand Down
65 changes: 28 additions & 37 deletions node/core/dispute-coordinator/src/initialized.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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:
//
Expand Down Expand Up @@ -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!(
Expand Down Expand Up @@ -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:
Expand All @@ -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,
Expand All @@ -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,
Expand Down
Loading

0 comments on commit 89dc70f

Please sign in to comment.