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

Update dispute participation on active leaves update #6303

Merged
merged 37 commits into from
Dec 30, 2022
Merged
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
c039614
Passed candidate events from scraper to participation
BradleyOlson64 Nov 15, 2022
7666c67
First draft PR 5875
BradleyOlson64 Nov 16, 2022
3829bd7
Merge branch 'master' of https://github.com/paritytech/polkadot into …
BradleyOlson64 Nov 16, 2022
1cdc72d
Added support for timestamp in changes
BradleyOlson64 Nov 16, 2022
e9d1273
Some necessary refactoring
BradleyOlson64 Nov 18, 2022
4eebacd
Removed SessionIndex from unconfirmed_disputes key
BradleyOlson64 Nov 19, 2022
45477d2
Removed duplicate logic in import statements
BradleyOlson64 Nov 19, 2022
5b11951
Merge branch 'master' of https://github.com/paritytech/polkadot into …
BradleyOlson64 Nov 19, 2022
04b214c
Replaced queue_participation call with re-prio
BradleyOlson64 Nov 19, 2022
58f6371
Simplifying refactor. Backed were already handled
BradleyOlson64 Nov 21, 2022
efa2870
Removed unneeded spam slots logic
BradleyOlson64 Nov 21, 2022
c0ee1fe
Implementers guide edits
BradleyOlson64 Nov 22, 2022
8b27aef
Undid the spam slots refactor
BradleyOlson64 Nov 22, 2022
319ea05
Added comments and implementers guide edit
BradleyOlson64 Nov 22, 2022
26c9ebe
Added test for participation upon backing
BradleyOlson64 Nov 25, 2022
78cc97b
Merge branch 'master' of https://github.com/paritytech/polkadot into …
BradleyOlson64 Nov 25, 2022
16edc2b
Merge branch 'master' into brad-issue-5875
Dec 1, 2022
e1c356c
Round of fixes + ran fmt
BradleyOlson64 Dec 1, 2022
a7615e5
Round of changes + fmt
BradleyOlson64 Dec 1, 2022
9aea6c6
Merge branch 'brad-issue-5875' of https://github.com/paritytech/polka…
BradleyOlson64 Dec 1, 2022
87cb5a2
Error handling draft
BradleyOlson64 Dec 3, 2022
595fe63
Changed errors to bubble up from reprioritization
BradleyOlson64 Dec 5, 2022
38c6986
Starting to construct new test
BradleyOlson64 Dec 5, 2022
afd63f6
Clarifying participation function rename
BradleyOlson64 Dec 6, 2022
efd7088
Reprio test draft
BradleyOlson64 Dec 9, 2022
8382edb
Merge branch 'master' of https://github.com/paritytech/polkadot into …
BradleyOlson64 Dec 9, 2022
e9eb4e8
Very rough bump to priority queue test draft
BradleyOlson64 Dec 13, 2022
d49bd0b
Improving logging
BradleyOlson64 Dec 15, 2022
7546335
Most concise reproduction of error on third import
BradleyOlson64 Dec 15, 2022
16e35c8
Add `handle_approval_vote_request`
tdimitrov Dec 16, 2022
2954a96
Removing reprioritization on included event test
BradleyOlson64 Dec 16, 2022
b3b3d8a
Removing unneeded test config
BradleyOlson64 Dec 16, 2022
72ae47a
cargo fmt
BradleyOlson64 Dec 16, 2022
e38a933
Test works
tdimitrov Dec 21, 2022
b53035c
Fixing final nits
BradleyOlson64 Dec 22, 2022
dede25c
Merge branch 'brad-issue-5875-temp' of https://github.com/paritytech/…
BradleyOlson64 Dec 22, 2022
d7f0250
Tweaks to test Tsveto figured out
BradleyOlson64 Dec 22, 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
35 changes: 22 additions & 13 deletions node/core/dispute-coordinator/src/initialized.rs
Original file line number Diff line number Diff line change
Expand Up @@ -269,8 +269,13 @@ impl Initialized {
update: ActiveLeavesUpdate,
now: u64,
) -> Result<()> {
let on_chain_votes =
let scraped_updates =
self.scraper.process_active_leaves_update(ctx.sender(), &update).await?;
log_error(
eskimor marked this conversation as resolved.
Show resolved Hide resolved
self.participation
.bump_to_priority_for_candidates(ctx, &scraped_updates.included_receipts)
.await,
)?;
self.participation.process_active_leaves_update(ctx, &update).await?;

if let Some(new_leaf) = update.activated {
Expand Down Expand Up @@ -308,7 +313,7 @@ impl Initialized {

// The `runtime-api` subsystem has an internal queue which serializes the execution,
// so there is no point in running these in parallel.
for votes in on_chain_votes {
for votes in scraped_updates.on_chain_votes {
let _ = self.process_on_chain_votes(ctx, overlay_db, votes, now).await.map_err(
|error| {
gum::warn!(
Expand Down Expand Up @@ -416,6 +421,8 @@ impl Initialized {
})
.collect();

// Importantly, handling import statements for backing votes also
// clears spam slots for any newly backed candidates
let import_result = self
.handle_import_statements(
ctx,
Expand Down Expand Up @@ -830,8 +837,15 @@ impl Initialized {
let new_state = import_result.new_state();

let is_included = self.scraper.is_candidate_included(&candidate_hash);

let potential_spam = !is_included && !new_state.is_confirmed() && !new_state.has_own_vote();
let is_backed = self.scraper.is_candidate_backed(&candidate_hash);
BradleyOlson64 marked this conversation as resolved.
Show resolved Hide resolved
let has_own_vote = new_state.has_own_vote();
let is_disputed = new_state.is_disputed();
let has_controlled_indices = !env.controlled_indices().is_empty();
let is_confirmed = new_state.is_confirmed();
let potential_spam =
!is_included && !is_backed && !new_state.is_confirmed() && !new_state.has_own_vote();
// We participate only in disputes which are included, backed or confirmed
let allow_participation = is_included || is_backed || is_confirmed;

gum::trace!(
target: LOG_TARGET,
Expand All @@ -844,8 +858,11 @@ impl Initialized {
"Is spam?"
);

// This check is responsible for all clearing of spam slots. It runs
// whenever a vote is imported from on or off chain, and decrements
// slots whenever a candidate is newly backed, confirmed, or has our
// own vote.
if !potential_spam {
// Former spammers have not been spammers after all:
self.spam_slots.clear(&(session, candidate_hash));

// Potential spam:
Expand Down Expand Up @@ -873,14 +890,6 @@ impl Initialized {
}
}

let has_own_vote = new_state.has_own_vote();
let is_disputed = new_state.is_disputed();
let has_controlled_indices = !env.controlled_indices().is_empty();
let is_backed = self.scraper.is_candidate_backed(&candidate_hash);
let is_confirmed = new_state.is_confirmed();
// We participate only in disputes which are included, backed or confirmed
let allow_participation = is_included || is_backed || is_confirmed;

// Participate in dispute if we did not cast a vote before and actually have keys to cast a
// local vote. Disputes should fall in one of the categories below, otherwise we will refrain
// from participation:
Expand Down
13 changes: 13 additions & 0 deletions node/core/dispute-coordinator/src/participation/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,19 @@ impl Participation {
Ok(())
}

/// Moving any request concerning the given candidates from best-effort to
/// priority, ignoring any candidates that don't have any queued participation requests.
pub async fn bump_to_priority_for_candidates<Context>(
&mut self,
ctx: &mut Context,
included_receipts: &Vec<CandidateReceipt>,
) -> Result<()> {
for receipt in included_receipts {
self.queue.prioritize_if_present(ctx.sender(), receipt).await?;
}
Ok(())
}

/// Dequeue until `MAX_PARALLEL_PARTICIPATIONS` is reached.
async fn dequeue_until_capacity<Context>(
&mut self,
Expand Down
28 changes: 28 additions & 0 deletions node/core/dispute-coordinator/src/participation/queues/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,34 @@ impl Queues {
self.pop_best_effort().map(|d| d.1)
}

/// Reprioritizes any participation requests pertaining to the
/// passed candidates from best effort to priority.
///
/// Returns: Either a bool telling the caller whether the priority queue is now full
BradleyOlson64 marked this conversation as resolved.
Show resolved Hide resolved
/// or an error resulting from the failed creation of a comparator.
pub async fn prioritize_if_present(
&mut self,
sender: &mut impl overseer::DisputeCoordinatorSenderTrait,
receipt: &CandidateReceipt,
) -> Result<()> {
let comparator = CandidateComparator::new(sender, receipt).await?;
self.prioritize_with_comparator(comparator)?;
Ok(())
}

fn prioritize_with_comparator(
&mut self,
comparator: CandidateComparator,
) -> std::result::Result<(), QueueError> {
if self.priority.len() >= PRIORITY_QUEUE_SIZE {
return Err(QueueError::PriorityFull)
}
if let Some(request) = self.best_effort.remove(&comparator) {
self.priority.insert(comparator, request);
}
Ok(())
}

fn queue_with_comparator(
&mut self,
comparator: CandidateComparator,
Expand Down
52 changes: 39 additions & 13 deletions node/core/dispute-coordinator/src/scraping/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use polkadot_node_subsystem::{
};
use polkadot_node_subsystem_util::runtime::{get_candidate_events, get_on_chain_votes};
use polkadot_primitives::v2::{
BlockNumber, CandidateEvent, CandidateHash, Hash, ScrapedOnChainVotes,
BlockNumber, CandidateEvent, CandidateHash, CandidateReceipt, Hash, ScrapedOnChainVotes,
};

use crate::{
Expand All @@ -51,6 +51,24 @@ const LRU_OBSERVED_BLOCKS_CAPACITY: NonZeroUsize = match NonZeroUsize::new(20) {
None => panic!("Observed blocks cache size must be non-zero"),
};

/// ScrapedUpdates
///
/// Updates to on_chain_votes and included receipts for new active leaf and its unprocessed
/// ancestors.
///
/// on_chain_votes: New votes as seen on chain
/// included_receipts: Newly included parachain block candidate receipts as seen on chain
pub struct ScrapedUpdates {
pub on_chain_votes: Vec<ScrapedOnChainVotes>,
pub included_receipts: Vec<CandidateReceipt>,
}

impl ScrapedUpdates {
pub fn new() -> Self {
Self { on_chain_votes: Vec::new(), included_receipts: Vec::new() }
}
}

/// Chain scraper
///
/// Scrapes unfinalized chain in order to collect information from blocks.
Expand Down Expand Up @@ -104,8 +122,8 @@ impl ChainScraper {
};
let update =
ActiveLeavesUpdate { activated: Some(initial_head), deactivated: Default::default() };
let votes = s.process_active_leaves_update(sender, &update).await?;
Ok((s, votes))
let updates = s.process_active_leaves_update(sender, &update).await?;
Ok((s, updates.on_chain_votes))
}

/// Check whether we have seen a candidate included on any chain.
Expand All @@ -122,18 +140,19 @@ impl ChainScraper {
///
/// and updates current heads, so we can query candidates for all non finalized blocks.
///
/// Returns: On chain vote for the leaf and any ancestors we might not yet have seen.
/// Returns: On chain votes and included candidate receipts for the leaf and any
/// ancestors we might not yet have seen.
pub async fn process_active_leaves_update<Sender>(
&mut self,
sender: &mut Sender,
update: &ActiveLeavesUpdate,
) -> Result<Vec<ScrapedOnChainVotes>>
) -> Result<ScrapedUpdates>
where
Sender: overseer::DisputeCoordinatorSenderTrait,
{
let activated = match update.activated.as_ref() {
Some(activated) => activated,
None => return Ok(Vec::new()),
None => return Ok(ScrapedUpdates::new()),
};

// Fetch ancestry up to last finalized block.
Expand All @@ -147,20 +166,22 @@ impl ChainScraper {

let block_hashes = std::iter::once(activated.hash).chain(ancestors);

let mut on_chain_votes = Vec::new();
let mut scraped_updates = ScrapedUpdates::new();
for (block_number, block_hash) in block_numbers.zip(block_hashes) {
gum::trace!(?block_number, ?block_hash, "In ancestor processing.");

self.process_candidate_events(sender, block_number, block_hash).await?;
let receipts_for_block =
self.process_candidate_events(sender, block_number, block_hash).await?;
scraped_updates.included_receipts.extend(receipts_for_block);

if let Some(votes) = get_on_chain_votes(sender, block_hash).await? {
on_chain_votes.push(votes);
scraped_updates.on_chain_votes.push(votes);
}
}

self.last_observed_blocks.put(activated.hash, ());

Ok(on_chain_votes)
Ok(scraped_updates)
}

/// Prune finalized candidates.
Expand All @@ -187,17 +208,21 @@ impl ChainScraper {
/// Process candidate events of a block.
///
/// Keep track of all included and backed candidates.
///
/// Returns freshly included candidate receipts
async fn process_candidate_events<Sender>(
&mut self,
sender: &mut Sender,
block_number: BlockNumber,
block_hash: Hash,
) -> Result<()>
) -> Result<Vec<CandidateReceipt>>
tdimitrov marked this conversation as resolved.
Show resolved Hide resolved
where
Sender: overseer::DisputeCoordinatorSenderTrait,
{
let events = get_candidate_events(sender, block_hash).await?;
let mut included_receipts: Vec<CandidateReceipt> = Vec::new();
// Get included and backed events:
for ev in get_candidate_events(sender, block_hash).await? {
for ev in events {
match ev {
CandidateEvent::CandidateIncluded(receipt, _, _, _) => {
let candidate_hash = receipt.hash();
Expand All @@ -208,6 +233,7 @@ impl ChainScraper {
"Processing included event"
);
self.included_candidates.insert(block_number, candidate_hash);
included_receipts.push(receipt);
},
CandidateEvent::CandidateBacked(receipt, _, _, _) => {
let candidate_hash = receipt.hash();
Expand All @@ -224,7 +250,7 @@ impl ChainScraper {
},
}
}
Ok(())
Ok(included_receipts)
}

/// Returns ancestors of `head` in the descending order, stopping
Expand Down
119 changes: 118 additions & 1 deletion node/core/dispute-coordinator/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1273,7 +1273,7 @@ fn backing_statements_import_works_and_no_spam() {
})
.await;

// Result should be valid, because our node participated, so spam slots are cleared:
// Import should be valid, as spam slots were not filled
assert_matches!(confirmation_rx.await, Ok(ImportStatementsResult::ValidImport));

virtual_overseer.send(FromOrchestra::Signal(OverseerSignal::Conclude)).await;
Expand Down Expand Up @@ -3035,3 +3035,120 @@ fn participation_for_included_candidates() {
})
});
}

/// Shows that importing backing votes when a backing event is being processed
BradleyOlson64 marked this conversation as resolved.
Show resolved Hide resolved
/// results in participation.
#[test]
fn local_participation_in_dispute_for_backed_candidate() {
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();

// Step 1: Show that we don't participate when not backed, confirmed, or included

// activate leaf - without candidate backed event
test_state
.activate_leaf_at_session(&mut virtual_overseer, session, 1, vec![])
.await;

// generate two votes
let (valid_vote, invalid_vote) = generate_opposing_votes_pair(
&test_state,
ValidatorIndex(1),
ValidatorIndex(2),
candidate_hash,
session,
VoteType::Explicit,
)
.await;

virtual_overseer
.send(FromOrchestra::Communication {
msg: DisputeCoordinatorMessage::ImportStatements {
candidate_receipt: candidate_receipt.clone(),
session,
statements: vec![
(valid_vote, ValidatorIndex(1)),
(invalid_vote, ValidatorIndex(2)),
],
pending_confirmation: None,
},
})
.await;

handle_approval_vote_request(&mut virtual_overseer, &candidate_hash, HashMap::new())
.await;

assert_matches!(virtual_overseer.recv().timeout(TEST_TIMEOUT).await, None);

// Step 2: Show that once backing votes are processed we participate

// Activate leaf: With candidate backed event
test_state
.activate_leaf_at_session(
&mut virtual_overseer,
session,
1,
vec![make_candidate_backed_event(candidate_receipt.clone())],
)
.await;

let backing_valid = test_state
.issue_backing_statement_with_index(ValidatorIndex(3), candidate_hash, session)
.await;

virtual_overseer
.send(FromOrchestra::Communication {
msg: DisputeCoordinatorMessage::ImportStatements {
candidate_receipt: candidate_receipt.clone(),
session,
statements: vec![(backing_valid, ValidatorIndex(3))],
pending_confirmation: None,
},
})
.await;

participation_with_distribution(
&mut virtual_overseer,
&candidate_hash,
candidate_receipt.commitments_hash,
)
.await;

// Check for our 1 active dispute
let (tx, rx) = oneshot::channel();
virtual_overseer
.send(FromOrchestra::Communication {
msg: DisputeCoordinatorMessage::ActiveDisputes(tx),
})
.await;

assert_eq!(rx.await.unwrap().len(), 1);

// check if we have participated (casted a vote)
BradleyOlson64 marked this conversation as resolved.
Show resolved Hide resolved
let (tx, rx) = oneshot::channel();
virtual_overseer
.send(FromOrchestra::Communication {
msg: DisputeCoordinatorMessage::QueryCandidateVotes(
vec![(session, candidate_hash)],
tx,
),
})
.await;

let (_, _, votes) = rx.await.unwrap().get(0).unwrap().clone();
assert_eq!(votes.valid.raw().len(), 3); // 3 => 1 initial vote, 1 backing vote, and our vote
assert_eq!(votes.invalid.len(), 1);

// Wrap up
virtual_overseer.send(FromOrchestra::Signal(OverseerSignal::Conclude)).await;

test_state
})
});
}
BradleyOlson64 marked this conversation as resolved.
Show resolved Hide resolved
Loading