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

Only report concluded if there is an actual dispute. #6270

Merged
merged 4 commits into from
Nov 14, 2022
Merged

Only report concluded if there is an actual dispute. #6270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2d86209
Only report concluded if there is an actual dispute.
Nov 11, 2022
62d919d
Fix redundant check.
Nov 14, 2022
de07e15
Test for no onesided disputes.
Nov 14, 2022
b503024
Merge branch 'master' into rk-fix-onesided-disputes
Nov 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 50 additions & 66 deletions node/core/dispute-coordinator/src/import.rs
View file
Open in desktop
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
Open in desktop
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