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

Commit adcd81e

Browse files
tdimitrovsandreim
authored andcommitted
Update disputes prioritisation in dispute-coordinator (#6130)
* Scraper processes CandidateBacked events * Change definition of best-effort * Fix `dispute-coordinator` tests * Unit test for dispute filtering * Clarification comment * Add tests * Fix logic If a dispute is not backed, not included and not confirmed we don't participate but we do import votes. * Add metrics for refrained participations * Revert "Add tests" This reverts commit 7b8391a. * Revert "Unit test for dispute filtering" This reverts commit 92ba5fe. * fix dispute-coordinator tests * Fix scraping * new tests * Small fixes in guide * Apply suggestions from code review Co-authored-by: Andrei Sandu <54316454+sandreim@users.noreply.github.com> * Fix some comments and remove a pointless test * Code review feedback * Clarification comment in tests * Some tests * Reference counted `CandidateHash` in scraper * Proper handling for Backed and Included candidates in scraper Backed candidates which are not included should be kept for a predetermined window of finalized blocks. E.g. if a candidate is backed but not included in block 2, and the window size is 2, the same candidate should be cleaned after block 4 is finalized. Add reference counting for candidates in scraper. A candidate can be added on multiple block heights so we have to make sure we don't clean it prematurely from the scraper. Add tests. * Update comments in tests * Guide update * Fix cleanup logic for `backed_candidates_by_block_number` * Simplify cleanup * Make spellcheck happy * Update tests * Extract candidate backing logic in separate struct * Code review feedback * Treat backed and included candidates in the same fashion * Update some comments * Small improvements in test * spell check * Fix some more comments * clean -> prune * Code review feedback * Reword comment * spelling Co-authored-by: Andrei Sandu <54316454+sandreim@users.noreply.github.com>
1 parent d7bd141 commit adcd81e

File tree

8 files changed

+873
-99
lines changed

8 files changed

+873
-99
lines changed

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

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -869,12 +869,21 @@ impl Initialized {
869869
}
870870
}
871871

872+
let has_own_vote = new_state.has_own_vote();
873+
let is_disputed = new_state.is_disputed();
874+
let has_controlled_indices = !env.controlled_indices().is_empty();
875+
let is_backed = self.scraper.is_candidate_backed(&candidate_hash);
876+
let is_confirmed = new_state.is_confirmed();
877+
// We participate only in disputes which are included, backed or confirmed
878+
let allow_participation = is_included || is_backed || is_confirmed;
879+
872880
// Participate in dispute if we did not cast a vote before and actually have keys to cast a
873-
// local vote:
874-
if !new_state.has_own_vote() &&
875-
new_state.is_disputed() &&
876-
!env.controlled_indices().is_empty()
877-
{
881+
// local vote. Disputes should fall in one of the categories below, otherwise we will refrain
882+
// from participation:
883+
// - `is_included` lands in prioritised queue
884+
// - `is_confirmed` | `is_backed` lands in best effort queue
885+
// We don't participate in disputes on finalized candidates.
886+
if !has_own_vote && is_disputed && has_controlled_indices && allow_participation {
878887
let priority = ParticipationPriority::with_priority_if(is_included);
879888
gum::trace!(
880889
target: LOG_TARGET,
@@ -896,6 +905,23 @@ impl Initialized {
896905
)
897906
.await;
898907
log_error(r)?;
908+
} else {
909+
gum::trace!(
910+
target: LOG_TARGET,
911+
?candidate_hash,
912+
?is_confirmed,
913+
?has_own_vote,
914+
?is_disputed,
915+
?has_controlled_indices,
916+
?allow_participation,
917+
?is_included,
918+
?is_backed,
919+
"Will not queue participation for candidate"
920+
);
921+
922+
if !allow_participation {
923+
self.metrics.on_refrained_participation();
924+
}
899925
}
900926

901927
// Also send any already existing approval vote on new disputes:

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -286,7 +286,7 @@ impl DisputeCoordinatorSubsystem {
286286

287287
let mut participation_requests = Vec::new();
288288
let mut unconfirmed_disputes: UnconfirmedDisputes = UnconfirmedDisputes::new();
289-
let (mut scraper, votes) = ChainScraper::new(ctx.sender(), initial_head).await?;
289+
let (scraper, votes) = ChainScraper::new(ctx.sender(), initial_head).await?;
290290
for ((session, ref candidate_hash), status) in active_disputes {
291291
let votes: CandidateVotes =
292292
match overlay_db.load_candidate_votes(session, candidate_hash) {

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ struct MetricsInner {
3030
queued_participations: prometheus::CounterVec<prometheus::U64>,
3131
/// How long vote cleanup batches take.
3232
vote_cleanup_time: prometheus::Histogram,
33+
/// Number of refrained participations.
34+
refrained_participations: prometheus::Counter<prometheus::U64>,
3335
}
3436

3537
/// Candidate validation metrics.
@@ -88,6 +90,12 @@ impl Metrics {
8890
pub(crate) fn time_vote_cleanup(&self) -> Option<prometheus::prometheus::HistogramTimer> {
8991
self.0.as_ref().map(|metrics| metrics.vote_cleanup_time.start_timer())
9092
}
93+
94+
pub(crate) fn on_refrained_participation(&self) {
95+
if let Some(metrics) = &self.0 {
96+
metrics.refrained_participations.inc();
97+
}
98+
}
9199
}
92100

93101
impl metrics::Metrics for Metrics {
@@ -147,6 +155,14 @@ impl metrics::Metrics for Metrics {
147155
)?,
148156
registry,
149157
)?,
158+
refrained_participations: prometheus::register(
159+
prometheus::Counter::with_opts(
160+
prometheus::Opts::new(
161+
"polkadot_parachain_dispute_refrained_participations",
162+
"Number of refrained participations. We refrain from participation if all of the following conditions are met: disputed candidate is not included, not backed and not confirmed.",
163+
))?,
164+
registry,
165+
)?,
150166
};
151167
Ok(Metrics(Some(metrics)))
152168
}
Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
use polkadot_primitives::v2::{BlockNumber, CandidateHash};
2+
use std::collections::{BTreeMap, HashMap, HashSet};
3+
4+
/// Keeps `CandidateHash` in reference counted way.
5+
/// Each `insert` saves a value with `reference count == 1` or increases the reference
6+
/// count if the value already exists.
7+
/// Each `remove` decreases the reference count for the corresponding `CandidateHash`.
8+
/// If the reference count reaches 0 - the value is removed.
9+
struct RefCountedCandidates {
10+
candidates: HashMap<CandidateHash, usize>,
11+
}
12+
13+
impl RefCountedCandidates {
14+
pub fn new() -> Self {
15+
Self { candidates: HashMap::new() }
16+
}
17+
// If `CandidateHash` doesn't exist in the `HashMap` it is created and its reference
18+
// count is set to 1.
19+
// If `CandidateHash` already exists in the `HashMap` its reference count is increased.
20+
pub fn insert(&mut self, candidate: CandidateHash) {
21+
*self.candidates.entry(candidate).or_default() += 1;
22+
}
23+
24+
// If a `CandidateHash` with reference count equals to 1 is about to be removed - the
25+
// candidate is dropped from the container too.
26+
// If a `CandidateHash` with reference count biger than 1 is about to be removed - the
27+
// reference count is decreased and the candidate remains in the container.
28+
pub fn remove(&mut self, candidate: &CandidateHash) {
29+
match self.candidates.get_mut(candidate) {
30+
Some(v) if *v > 1 => *v -= 1,
31+
Some(v) => {
32+
assert!(*v == 1);
33+
self.candidates.remove(candidate);
34+
},
35+
None => {},
36+
}
37+
}
38+
39+
pub fn contains(&self, candidate: &CandidateHash) -> bool {
40+
self.candidates.contains_key(&candidate)
41+
}
42+
}
43+
44+
#[cfg(test)]
45+
mod ref_counted_candidates_tests {
46+
use super::*;
47+
use polkadot_primitives::v2::{BlakeTwo256, HashT};
48+
49+
#[test]
50+
fn element_is_removed_when_refcount_reaches_zero() {
51+
let mut container = RefCountedCandidates::new();
52+
53+
let zero = CandidateHash(BlakeTwo256::hash(&vec![0]));
54+
let one = CandidateHash(BlakeTwo256::hash(&vec![1]));
55+
// add two separate candidates
56+
container.insert(zero); // refcount == 1
57+
container.insert(one);
58+
59+
// and increase the reference count for the first
60+
container.insert(zero); // refcount == 2
61+
62+
assert!(container.contains(&zero));
63+
assert!(container.contains(&one));
64+
65+
// remove once -> refcount == 1
66+
container.remove(&zero);
67+
assert!(container.contains(&zero));
68+
assert!(container.contains(&one));
69+
70+
// remove once -> refcount == 0
71+
container.remove(&zero);
72+
assert!(!container.contains(&zero));
73+
assert!(container.contains(&one));
74+
75+
// remove the other element
76+
container.remove(&one);
77+
assert!(!container.contains(&zero));
78+
assert!(!container.contains(&one));
79+
}
80+
}
81+
82+
/// Keeps track of scraped candidates. Supports `insert`, `remove_up_to_height` and `contains`
83+
/// operations.
84+
pub struct ScrapedCandidates {
85+
/// Main data structure which keeps the candidates we know about. `contains` does lookups only here.
86+
candidates: RefCountedCandidates,
87+
/// Keeps track at which block number a candidate was inserted. Used in `remove_up_to_height`.
88+
/// Without this tracking we won't be able to remove all candidates before block X.
89+
candidates_by_block_number: BTreeMap<BlockNumber, HashSet<CandidateHash>>,
90+
}
91+
92+
impl ScrapedCandidates {
93+
pub fn new() -> Self {
94+
Self {
95+
candidates: RefCountedCandidates::new(),
96+
candidates_by_block_number: BTreeMap::new(),
97+
}
98+
}
99+
100+
pub fn contains(&self, candidate_hash: &CandidateHash) -> bool {
101+
self.candidates.contains(candidate_hash)
102+
}
103+
104+
// Removes all candidates up to a given height. The candidates at the block height are NOT removed.
105+
pub fn remove_up_to_height(&mut self, height: &BlockNumber) {
106+
let not_stale = self.candidates_by_block_number.split_off(&height);
107+
let stale = std::mem::take(&mut self.candidates_by_block_number);
108+
self.candidates_by_block_number = not_stale;
109+
for candidates in stale.values() {
110+
for c in candidates {
111+
self.candidates.remove(c);
112+
}
113+
}
114+
}
115+
116+
pub fn insert(&mut self, block_number: BlockNumber, candidate_hash: CandidateHash) {
117+
self.candidates.insert(candidate_hash);
118+
self.candidates_by_block_number
119+
.entry(block_number)
120+
.or_default()
121+
.insert(candidate_hash);
122+
}
123+
124+
// Used only for tests to verify the pruning doesn't leak data.
125+
#[cfg(test)]
126+
pub fn candidates_by_block_number_is_empty(&self) -> bool {
127+
self.candidates_by_block_number.is_empty()
128+
}
129+
}
130+
131+
#[cfg(test)]
132+
mod scraped_candidates_tests {
133+
use super::*;
134+
use polkadot_primitives::v2::{BlakeTwo256, HashT};
135+
136+
#[test]
137+
fn stale_candidates_are_removed() {
138+
let mut candidates = ScrapedCandidates::new();
139+
let target = CandidateHash(BlakeTwo256::hash(&vec![1, 2, 3]));
140+
candidates.insert(1, target);
141+
142+
assert!(candidates.contains(&target));
143+
144+
candidates.remove_up_to_height(&2);
145+
assert!(!candidates.contains(&target));
146+
assert!(candidates.candidates_by_block_number_is_empty());
147+
}
148+
}

0 commit comments

Comments
 (0)