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

Revert chain if at least f+1 validators voted against a candidate #7151

Merged
merged 7 commits into from
May 17, 2023
Merged

Revert chain if at least f+1 validators voted against a candidate #7151

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2a8da05
Make `issue_explicit_statement_with_index` regular function
tdimitrov Apr 29, 2023
9d0a8da
Make `issue_backing_statement_with_index` regular function
tdimitrov Apr 29, 2023
344bbcb
Issue `RevertBlocks` as soon as a dispute has `byzantine threshold + …
tdimitrov Apr 29, 2023
6818357
Remove a comment
tdimitrov May 3, 2023
b055ccf
Fix `has_fresh_byzantine_threshold_against()`
tdimitrov May 3, 2023
369edf6
Extend `informs_chain_selection_when_dispute_concluded_against` test
tdimitrov May 4, 2023
abb3804
Merge branch 'master' into tsv-disp-coord-byzantine-revert
tdimitrov May 6, 2023
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
29 changes: 29 additions & 0 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 @@ -477,6 +477,35 @@ impl ImportResult {
self.is_freshly_concluded_against() || self.is_freshly_concluded_for()
}

// Returns the number of invalid own votes from the state provided. This function is a helper for
// `has_fresh_byzantine_threshold_against`
fn get_own_invalid_votes<T>(state: &CandidateVoteState<T>) -> usize {
state
.own_votes()
.map(|votes| {
votes.iter().fold(0, |acc, (_, (vote, _))| match vote {
DisputeStatement::Invalid(_) => acc + 1,
_ => acc,
})
})
.unwrap_or(0)
}

/// Whether or not the invalid vote count for the dispute went beyond the byzantine threshold
/// after the last import
pub fn has_fresh_byzantine_threshold_against(&self, byzantine_threshold: usize) -> bool {
// Total number of imported invalid votes + our own invalid votes
let imported_invalid_votes =
self.imported_invalid_votes() as usize + Self::get_own_invalid_votes(self.new_state());
// The number of invalid votes + our own invalid votes before the import
let old_invalid_votes = (self.imported_invalid_votes() as usize)
.saturating_sub(self.new_invalid_voters().len()) +
Self::get_own_invalid_votes(self.old_state());
tdimitrov marked this conversation as resolved.
Show resolved Hide resolved

old_invalid_votes as usize <= byzantine_threshold &&
imported_invalid_votes as usize > byzantine_threshold
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we only check for newly imported invalid votes?

Copy link
Contributor Author

@tdimitrov tdimitrov May 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want has_fresh_byzantine_threshold_against() to return true only once - when byzantine_threshold is reached. So I check if the invalid votes before import were below (or equal) the byzantine_threshold and at the same time they are above it after the import.
But you probably are asking something else?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I understand that you want to trigger the message once the threshold is passed and not on every import, but you are comparing newly imported votes to the byzantine threshold - so any previously known votes are not taken into account. This would only trigger if we imported f+1 invalid votes all at once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, self.imported_invalid_votes() returns the votes from the last import, not all votes. This is totally messed up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fixed now, but I have to add more tests. The current ones didn't catch that error.

}

/// Modify this `ImportResult`s, by importing additional approval votes.
///
/// Both results and `new_state` will be changed as if those approval votes had been in the
Expand Down
10 changes: 6 additions & 4 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 @@ -41,9 +41,9 @@ use polkadot_node_subsystem::{
};
use polkadot_node_subsystem_util::runtime::RuntimeInfo;
use polkadot_primitives::{
BlockNumber, CandidateHash, CandidateReceipt, CompactStatement, DisputeStatement,
DisputeStatementSet, Hash, ScrapedOnChainVotes, SessionIndex, ValidDisputeStatementKind,
ValidatorId, ValidatorIndex,
byzantine_threshold, BlockNumber, CandidateHash, CandidateReceipt, CompactStatement,
DisputeStatement, DisputeStatementSet, Hash, ScrapedOnChainVotes, SessionIndex,
ValidDisputeStatementKind, ValidatorId, ValidatorIndex,
};

use crate::{
Expand Down Expand Up @@ -1094,7 +1094,9 @@ impl Initialized {

// Notify ChainSelection if a dispute has concluded against a candidate. ChainSelection
// will need to mark the candidate's relay parent as reverted.
if import_result.is_freshly_concluded_against() {
if import_result
.has_fresh_byzantine_threshold_against(byzantine_threshold(env.validators().len()))
{
let blocks_including = self.scraper.get_blocks_including_candidate(&candidate_hash);
for (parent_block_number, parent_block_hash) in &blocks_including {
gum::trace!(
Expand Down
Loading