From 658f9dead240123705e794d689f6cdb3d556409d Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Wed, 28 Dec 2022 12:17:25 -0800 Subject: [PATCH 01/30] Setting up new ChainSelectionMessage --- node/core/chain-selection/src/lib.rs | 10 ++++++++++ node/subsystem-types/src/messages.rs | 3 +++ 2 files changed, 13 insertions(+) diff --git a/node/core/chain-selection/src/lib.rs b/node/core/chain-selection/src/lib.rs index 786454fb9891..06b634bc7825 100644 --- a/node/core/chain-selection/src/lib.rs +++ b/node/core/chain-selection/src/lib.rs @@ -16,6 +16,7 @@ //! Implements the Chain Selection Subsystem. +use gum::CandidateHash; use polkadot_node_primitives::BlockWeight; use polkadot_node_subsystem::{ errors::ChainApiError, @@ -466,6 +467,9 @@ where let _ = tx.send(best_containing); } + ChainSelectionMessage::DisputeConcludedAgainst(hash) => { + handle_concluded_dispute_reversions(hash)? + } } } } @@ -678,6 +682,12 @@ fn handle_approved_block(backend: &mut impl Backend, approved_block: Hash) -> Re backend.write(ops) } +// A dispute has concluded against a candidate. Here we apply the reverted status to +// all blocks on chains containing that candidate. +fn handle_concluded_dispute_reversions(hash: CandidateHash) { + +} + fn detect_stagnant( backend: &mut impl Backend, now: Timestamp, diff --git a/node/subsystem-types/src/messages.rs b/node/subsystem-types/src/messages.rs index cb0834c5ba34..dee75d667ae4 100644 --- a/node/subsystem-types/src/messages.rs +++ b/node/subsystem-types/src/messages.rs @@ -592,6 +592,8 @@ pub enum ChainSelectionMessage { /// Request the best leaf containing the given block in its ancestry. Return `None` if /// there is no such leaf. BestLeafContaining(Hash, oneshot::Sender>), + /// Apply reverted status to blocks in chains containing the disputed candidate. + DisputeConcludedAgainst(Hash), } impl ChainSelectionMessage { @@ -604,6 +606,7 @@ impl ChainSelectionMessage { ChainSelectionMessage::Approved(_) => None, ChainSelectionMessage::Leaves(_) => None, ChainSelectionMessage::BestLeafContaining(..) => None, + ChainSelectionMessage::DisputeConcludedAgainst(_) => None, } } } From c66ea70707162a62059b6350f2c9ba7dd1b68bc9 Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Mon, 2 Jan 2023 12:56:27 -0800 Subject: [PATCH 02/30] Partial first pass --- node/core/chain-selection/src/lib.rs | 10 ++++++++-- node/core/dispute-coordinator/src/initialized.rs | 8 +++++++- node/overseer/src/lib.rs | 1 + node/subsystem-types/src/messages.rs | 2 +- 4 files changed, 17 insertions(+), 4 deletions(-) diff --git a/node/core/chain-selection/src/lib.rs b/node/core/chain-selection/src/lib.rs index 06b634bc7825..6ef8a9720ec2 100644 --- a/node/core/chain-selection/src/lib.rs +++ b/node/core/chain-selection/src/lib.rs @@ -34,6 +34,7 @@ use std::{ sync::Arc, time::{Duration, SystemTime, UNIX_EPOCH}, }; +use std::collections::BTreeMap; use crate::backend::{Backend, BackendWriteOp, OverlayedBackend}; @@ -328,13 +329,14 @@ pub struct Config { pub struct ChainSelectionSubsystem { config: Config, db: Arc, + blocks_including_candidates: BTreeMap, } impl ChainSelectionSubsystem { /// Create a new instance of the subsystem with the given config /// and key-value store. pub fn new(config: Config, db: Arc) -> Self { - ChainSelectionSubsystem { config, db } + ChainSelectionSubsystem { config, db, blocks_including_candidates: BTreeMap::new() } } /// Revert to the block corresponding to the specified `hash`. @@ -442,6 +444,9 @@ where backend.write(write_ops)?; } + + // Update blocks including candidates based on candidate included events + process_included_events(sender, block_number, block_hash) } FromOrchestra::Signal(OverseerSignal::BlockFinalized(h, n)) => { handle_finalized_block(backend, h, n)? @@ -684,8 +689,9 @@ fn handle_approved_block(backend: &mut impl Backend, approved_block: Hash) -> Re // A dispute has concluded against a candidate. Here we apply the reverted status to // all blocks on chains containing that candidate. -fn handle_concluded_dispute_reversions(hash: CandidateHash) { +fn handle_concluded_dispute_reversions(hash: CandidateHash) -> Result<(), Error> { + Ok(()) } fn detect_stagnant( diff --git a/node/core/dispute-coordinator/src/initialized.rs b/node/core/dispute-coordinator/src/initialized.rs index 245af523685f..eebc2311c8a9 100644 --- a/node/core/dispute-coordinator/src/initialized.rs +++ b/node/core/dispute-coordinator/src/initialized.rs @@ -32,7 +32,7 @@ use polkadot_node_primitives::{ use polkadot_node_subsystem::{ messages::{ ApprovalVotingMessage, BlockDescription, DisputeCoordinatorMessage, - DisputeDistributionMessage, ImportStatementsResult, + DisputeDistributionMessage, ImportStatementsResult, ChainSelectionMessage, }, overseer, ActivatedLeaf, ActiveLeavesUpdate, FromOrchestra, OverseerSignal, }; @@ -1018,6 +1018,12 @@ impl Initialized { } } + // Notify ChainSelection if a dispute has concluded against a candidate + if import_result.is_freshly_concluded_against() { + ctx.send_message(ChainSelectionMessage::DisputeConcludedAgainst(candidate_hash)) + .await; + } + // Update metrics: if import_result.is_freshly_disputed() { self.metrics.on_open(); diff --git a/node/overseer/src/lib.rs b/node/overseer/src/lib.rs index 6253f8b3e264..209a1479f3d6 100644 --- a/node/overseer/src/lib.rs +++ b/node/overseer/src/lib.rs @@ -593,6 +593,7 @@ pub struct Overseer { ApprovalVotingMessage, AvailabilityStoreMessage, AvailabilityRecoveryMessage, + ChainSelectionMessage, ])] dispute_coordinator: DisputeCoordinator, diff --git a/node/subsystem-types/src/messages.rs b/node/subsystem-types/src/messages.rs index dee75d667ae4..29d39f79113a 100644 --- a/node/subsystem-types/src/messages.rs +++ b/node/subsystem-types/src/messages.rs @@ -593,7 +593,7 @@ pub enum ChainSelectionMessage { /// there is no such leaf. BestLeafContaining(Hash, oneshot::Sender>), /// Apply reverted status to blocks in chains containing the disputed candidate. - DisputeConcludedAgainst(Hash), + DisputeConcludedAgainst(CandidateHash), } impl ChainSelectionMessage { From 8ef1202177461e1923310b83a8f42942d67541b8 Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Mon, 2 Jan 2023 16:26:14 -0800 Subject: [PATCH 03/30] Got dispute conclusion data to provisioner --- node/core/chain-selection/src/lib.rs | 13 ++--- .../dispute-coordinator/src/initialized.rs | 19 ++++++- .../src/scraping/candidates.rs | 49 ++++++++++++++----- .../dispute-coordinator/src/scraping/mod.rs | 13 ++++- node/subsystem-types/src/messages.rs | 4 +- 5 files changed, 71 insertions(+), 27 deletions(-) diff --git a/node/core/chain-selection/src/lib.rs b/node/core/chain-selection/src/lib.rs index 6ef8a9720ec2..b736a1027d6a 100644 --- a/node/core/chain-selection/src/lib.rs +++ b/node/core/chain-selection/src/lib.rs @@ -34,7 +34,6 @@ use std::{ sync::Arc, time::{Duration, SystemTime, UNIX_EPOCH}, }; -use std::collections::BTreeMap; use crate::backend::{Backend, BackendWriteOp, OverlayedBackend}; @@ -329,14 +328,13 @@ pub struct Config { pub struct ChainSelectionSubsystem { config: Config, db: Arc, - blocks_including_candidates: BTreeMap, } impl ChainSelectionSubsystem { /// Create a new instance of the subsystem with the given config /// and key-value store. pub fn new(config: Config, db: Arc) -> Self { - ChainSelectionSubsystem { config, db, blocks_including_candidates: BTreeMap::new() } + ChainSelectionSubsystem { config, db, } } /// Revert to the block corresponding to the specified `hash`. @@ -444,9 +442,6 @@ where backend.write(write_ops)?; } - - // Update blocks including candidates based on candidate included events - process_included_events(sender, block_number, block_hash) } FromOrchestra::Signal(OverseerSignal::BlockFinalized(h, n)) => { handle_finalized_block(backend, h, n)? @@ -472,8 +467,8 @@ where let _ = tx.send(best_containing); } - ChainSelectionMessage::DisputeConcludedAgainst(hash) => { - handle_concluded_dispute_reversions(hash)? + ChainSelectionMessage::DisputeConcludedAgainst(block_number, block_hash) => { + handle_concluded_dispute_reversions(block_number, block_hash)? } } } @@ -689,7 +684,7 @@ fn handle_approved_block(backend: &mut impl Backend, approved_block: Hash) -> Re // A dispute has concluded against a candidate. Here we apply the reverted status to // all blocks on chains containing that candidate. -fn handle_concluded_dispute_reversions(hash: CandidateHash) -> Result<(), Error> { +fn handle_concluded_dispute_reversions(block_number: u32, block_hash: Hash) -> Result<(), Error> { Ok(()) } diff --git a/node/core/dispute-coordinator/src/initialized.rs b/node/core/dispute-coordinator/src/initialized.rs index eebc2311c8a9..cd18c5a2b484 100644 --- a/node/core/dispute-coordinator/src/initialized.rs +++ b/node/core/dispute-coordinator/src/initialized.rs @@ -1020,8 +1020,23 @@ impl Initialized { // Notify ChainSelection if a dispute has concluded against a candidate if import_result.is_freshly_concluded_against() { - ctx.send_message(ChainSelectionMessage::DisputeConcludedAgainst(candidate_hash)) - .await; + let possible_info = self.scraper.get_included_candidate_info(candidate_hash); + if let Some(info) = possible_info { + ctx.send_message( + ChainSelectionMessage::DisputeConcludedAgainst( + info.block_number, + info.block_hash, + )) + .await; + } + else { + gum::error!( + target: LOG_TARGET, + ?candidate_hash, + ?session, + "Could not find parent block info for concluded candidate!" + ); + } } // Update metrics: diff --git a/node/core/dispute-coordinator/src/scraping/candidates.rs b/node/core/dispute-coordinator/src/scraping/candidates.rs index 2fe797768cc2..74686ae80856 100644 --- a/node/core/dispute-coordinator/src/scraping/candidates.rs +++ b/node/core/dispute-coordinator/src/scraping/candidates.rs @@ -1,4 +1,4 @@ -use polkadot_primitives::v2::{BlockNumber, CandidateHash}; +use polkadot_primitives::v2::{BlockNumber, CandidateHash, Hash}; use std::collections::{BTreeMap, HashMap, HashSet}; /// Keeps `CandidateHash` in reference counted way. @@ -7,7 +7,13 @@ use std::collections::{BTreeMap, HashMap, HashSet}; /// Each `remove` decreases the reference count for the corresponding `CandidateHash`. /// If the reference count reaches 0 - the value is removed. struct RefCountedCandidates { - candidates: HashMap, + candidates: HashMap, +} + +pub struct CandidateInfo { + count: usize, + pub block_number: BlockNumber, + pub block_hash: Hash, } impl RefCountedCandidates { @@ -17,8 +23,16 @@ impl RefCountedCandidates { // If `CandidateHash` doesn't exist in the `HashMap` it is created and its reference // count is set to 1. // If `CandidateHash` already exists in the `HashMap` its reference count is increased. - pub fn insert(&mut self, candidate: CandidateHash) { - *self.candidates.entry(candidate).or_default() += 1; + pub fn insert( + &mut self, + block_number: BlockNumber, + block_hash: Hash, + candidate: CandidateHash, + ) { + self.candidates + .entry(candidate) + .or_insert(CandidateInfo { count: 0, block_number, block_hash }) + .count += 1; } // If a `CandidateHash` with reference count equals to 1 is about to be removed - the @@ -27,9 +41,9 @@ impl RefCountedCandidates { // reference count is decreased and the candidate remains in the container. pub fn remove(&mut self, candidate: &CandidateHash) { match self.candidates.get_mut(candidate) { - Some(v) if *v > 1 => *v -= 1, + Some(v) if v.count > 1 => v.count -= 1, Some(v) => { - assert!(*v == 1); + assert!(v.count == 1); self.candidates.remove(candidate); }, None => {}, @@ -45,6 +59,7 @@ impl RefCountedCandidates { mod ref_counted_candidates_tests { use super::*; use polkadot_primitives::v2::{BlakeTwo256, HashT}; + use test_helpers::dummy_hash; #[test] fn element_is_removed_when_refcount_reaches_zero() { @@ -53,11 +68,11 @@ mod ref_counted_candidates_tests { let zero = CandidateHash(BlakeTwo256::hash(&vec![0])); let one = CandidateHash(BlakeTwo256::hash(&vec![1])); // add two separate candidates - container.insert(zero); // refcount == 1 - container.insert(one); + container.insert(0, dummy_hash(), zero); // refcount == 1 + container.insert(0, dummy_hash(), one); // and increase the reference count for the first - container.insert(zero); // refcount == 2 + container.insert(0, dummy_hash(), zero); // refcount == 2 assert!(container.contains(&zero)); assert!(container.contains(&one)); @@ -113,8 +128,17 @@ impl ScrapedCandidates { } } - pub fn insert(&mut self, block_number: BlockNumber, candidate_hash: CandidateHash) { - self.candidates.insert(candidate_hash); + pub fn get_candidate_info(&mut self, candidate: CandidateHash) -> Option<&CandidateInfo> { + self.candidates.candidates.get(&candidate) + } + + pub fn insert( + &mut self, + block_number: BlockNumber, + block_hash: Hash, + candidate_hash: CandidateHash, + ) { + self.candidates.insert( block_number, block_hash, candidate_hash); self.candidates_by_block_number .entry(block_number) .or_default() @@ -132,12 +156,13 @@ impl ScrapedCandidates { mod scraped_candidates_tests { use super::*; use polkadot_primitives::v2::{BlakeTwo256, HashT}; + use test_helpers::dummy_hash; #[test] fn stale_candidates_are_removed() { let mut candidates = ScrapedCandidates::new(); let target = CandidateHash(BlakeTwo256::hash(&vec![1, 2, 3])); - candidates.insert(1, target); + candidates.insert(1, dummy_hash(), target); assert!(candidates.contains(&target)); diff --git a/node/core/dispute-coordinator/src/scraping/mod.rs b/node/core/dispute-coordinator/src/scraping/mod.rs index 7f4f819756e3..3e11f0addd20 100644 --- a/node/core/dispute-coordinator/src/scraping/mod.rs +++ b/node/core/dispute-coordinator/src/scraping/mod.rs @@ -34,6 +34,8 @@ use crate::{ LOG_TARGET, }; +use self::candidates::CandidateInfo; + #[cfg(test)] mod tests; @@ -207,7 +209,7 @@ impl ChainScraper { ?block_number, "Processing included event" ); - self.included_candidates.insert(block_number, candidate_hash); + self.included_candidates.insert(block_number, block_hash, candidate_hash); }, CandidateEvent::CandidateBacked(receipt, _, _, _) => { let candidate_hash = receipt.hash(); @@ -217,7 +219,7 @@ impl ChainScraper { ?block_number, "Processing backed event" ); - self.backed_candidates.insert(block_number, candidate_hash); + self.backed_candidates.insert(block_number, block_hash, candidate_hash); }, _ => { // skip the rest @@ -292,6 +294,13 @@ impl ChainScraper { } return Ok(ancestors) } + + pub fn get_included_candidate_info( + &mut self, + candidate: CandidateHash, + ) -> Option<&CandidateInfo> { + self.included_candidates.get_candidate_info(candidate) + } } async fn get_finalized_block_number(sender: &mut Sender) -> FatalResult diff --git a/node/subsystem-types/src/messages.rs b/node/subsystem-types/src/messages.rs index 29d39f79113a..76f7b9be8e3a 100644 --- a/node/subsystem-types/src/messages.rs +++ b/node/subsystem-types/src/messages.rs @@ -593,7 +593,7 @@ pub enum ChainSelectionMessage { /// there is no such leaf. BestLeafContaining(Hash, oneshot::Sender>), /// Apply reverted status to blocks in chains containing the disputed candidate. - DisputeConcludedAgainst(CandidateHash), + DisputeConcludedAgainst(BlockNumber, Hash), } impl ChainSelectionMessage { @@ -606,7 +606,7 @@ impl ChainSelectionMessage { ChainSelectionMessage::Approved(_) => None, ChainSelectionMessage::Leaves(_) => None, ChainSelectionMessage::BestLeafContaining(..) => None, - ChainSelectionMessage::DisputeConcludedAgainst(_) => None, + ChainSelectionMessage::DisputeConcludedAgainst(..) => None, } } } From e69a5b4fb8722d31c8e8d8a4922f6448bc8b55fe Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Thu, 5 Jan 2023 11:30:28 -0800 Subject: [PATCH 04/30] Finished first draft for 4804 code --- node/core/chain-selection/src/lib.rs | 12 +++++--- node/core/chain-selection/src/tree.rs | 41 +++++++++++++++++++++++++-- 2 files changed, 47 insertions(+), 6 deletions(-) diff --git a/node/core/chain-selection/src/lib.rs b/node/core/chain-selection/src/lib.rs index b736a1027d6a..d4de542c7b44 100644 --- a/node/core/chain-selection/src/lib.rs +++ b/node/core/chain-selection/src/lib.rs @@ -16,7 +16,6 @@ //! Implements the Chain Selection Subsystem. -use gum::CandidateHash; use polkadot_node_primitives::BlockWeight; use polkadot_node_subsystem::{ errors::ChainApiError, @@ -468,7 +467,7 @@ where let _ = tx.send(best_containing); } ChainSelectionMessage::DisputeConcludedAgainst(block_number, block_hash) => { - handle_concluded_dispute_reversions(block_number, block_hash)? + handle_concluded_dispute_reversions(backend, block_number, block_hash)? } } } @@ -684,8 +683,13 @@ fn handle_approved_block(backend: &mut impl Backend, approved_block: Hash) -> Re // A dispute has concluded against a candidate. Here we apply the reverted status to // all blocks on chains containing that candidate. -fn handle_concluded_dispute_reversions(block_number: u32, block_hash: Hash) -> Result<(), Error> { - +fn handle_concluded_dispute_reversions( + backend: &mut impl Backend, + block_number: u32, + block_hash: Hash +) -> Result<(), Error> { + let mut overlay = OverlayedBackend::new(backend); + tree::apply_single_reversion(&mut overlay, block_hash, block_number)?; Ok(()) } diff --git a/node/core/chain-selection/src/tree.rs b/node/core/chain-selection/src/tree.rs index aafd75de5f97..00beac68a563 100644 --- a/node/core/chain-selection/src/tree.rs +++ b/node/core/chain-selection/src/tree.rs @@ -247,7 +247,7 @@ pub(crate) fn import_block( stagnant_at: Timestamp, ) -> Result<(), Error> { add_block(backend, block_hash, block_number, parent_hash, weight, stagnant_at)?; - apply_reversions(backend, block_hash, block_number, reversion_logs)?; + apply_ancestor_reversions(backend, block_hash, block_number, reversion_logs)?; Ok(()) } @@ -349,7 +349,7 @@ fn add_block( // Assuming that a block is already imported, accepts the number of the block // as well as a list of reversions triggered by the block in ascending order. -fn apply_reversions( +fn apply_ancestor_reversions( backend: &mut OverlayedBackend, block_hash: Hash, block_number: BlockNumber, @@ -394,6 +394,43 @@ fn apply_reversions( Ok(()) } +pub(crate) fn apply_single_reversion( + backend: &mut OverlayedBackend, + revert_hash: Hash, + revert_number: BlockNumber +) -> Result<(), Error> { + let mut entry_to_revert = + match backend.load_block_entry(&revert_hash)? { + None => { + gum::warn!( + target: LOG_TARGET, + ?revert_hash, + revert_target = revert_number, + "The hammer has dropped. \ + A dispute has concluded against a finalized block. \ + Please inform an adult.", + ); + + return Ok(()); + }, + Some(entry_to_revert) => { + gum::info!( + target: LOG_TARGET, + ?revert_hash, + revert_target = revert_number, + revert_hash = ?entry_to_revert.block_hash, + "A dispute has concluded against a block, causing it to be reverted.", + ); + + entry_to_revert + }, + }; + entry_to_revert.viability.explicitly_reverted = true; + propagate_viability_update(backend, entry_to_revert)?; + + Ok(()) +} + /// Finalize a block with the given number and hash. /// /// This will prune all sub-trees not descending from the given block, From b3709858c0a92c730284888ea6070f97755a891e Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Thu, 5 Jan 2023 12:24:02 -0800 Subject: [PATCH 05/30] A bit of polish and code comments --- node/core/chain-selection/src/lib.rs | 6 +++--- node/core/chain-selection/src/tree.rs | 7 ++++++- node/core/dispute-coordinator/src/initialized.rs | 6 +++--- node/core/dispute-coordinator/src/scraping/candidates.rs | 5 +++++ node/core/dispute-coordinator/src/scraping/mod.rs | 2 +- node/subsystem-types/src/messages.rs | 4 +++- 6 files changed, 21 insertions(+), 9 deletions(-) diff --git a/node/core/chain-selection/src/lib.rs b/node/core/chain-selection/src/lib.rs index d4de542c7b44..1e6f8e4c1f7b 100644 --- a/node/core/chain-selection/src/lib.rs +++ b/node/core/chain-selection/src/lib.rs @@ -333,7 +333,7 @@ impl ChainSelectionSubsystem { /// Create a new instance of the subsystem with the given config /// and key-value store. pub fn new(config: Config, db: Arc) -> Self { - ChainSelectionSubsystem { config, db, } + ChainSelectionSubsystem { config, db } } /// Revert to the block corresponding to the specified `hash`. @@ -681,8 +681,8 @@ fn handle_approved_block(backend: &mut impl Backend, approved_block: Hash) -> Re backend.write(ops) } -// A dispute has concluded against a candidate. Here we apply the reverted status to -// all blocks on chains containing that candidate. +// A dispute has concluded against a candidate. Here we revert the block containing +// that candidate and mark its descendants as non-viable fn handle_concluded_dispute_reversions( backend: &mut impl Backend, block_number: u32, diff --git a/node/core/chain-selection/src/tree.rs b/node/core/chain-selection/src/tree.rs index 00beac68a563..bfd2c6312588 100644 --- a/node/core/chain-selection/src/tree.rs +++ b/node/core/chain-selection/src/tree.rs @@ -394,6 +394,9 @@ fn apply_ancestor_reversions( Ok(()) } +/// Marks a single block as explicitly reverted. Then propegates viability updates +/// to all its children. This is triggered when the disputes subsystem signals that +/// a dispute has concluded against a candidate. pub(crate) fn apply_single_reversion( backend: &mut OverlayedBackend, revert_hash: Hash, @@ -405,6 +408,8 @@ pub(crate) fn apply_single_reversion( gum::warn!( target: LOG_TARGET, ?revert_hash, + // We keep parent block numbers in the disputes subsystem just + // for this log revert_target = revert_number, "The hammer has dropped. \ A dispute has concluded against a finalized block. \ @@ -418,7 +423,6 @@ pub(crate) fn apply_single_reversion( target: LOG_TARGET, ?revert_hash, revert_target = revert_number, - revert_hash = ?entry_to_revert.block_hash, "A dispute has concluded against a block, causing it to be reverted.", ); @@ -426,6 +430,7 @@ pub(crate) fn apply_single_reversion( }, }; entry_to_revert.viability.explicitly_reverted = true; + // Marks children of reverted block as non-viable propagate_viability_update(backend, entry_to_revert)?; Ok(()) diff --git a/node/core/dispute-coordinator/src/initialized.rs b/node/core/dispute-coordinator/src/initialized.rs index 7d2474512c61..274509487503 100644 --- a/node/core/dispute-coordinator/src/initialized.rs +++ b/node/core/dispute-coordinator/src/initialized.rs @@ -1027,10 +1027,10 @@ impl Initialized { } } - // Notify ChainSelection if a dispute has concluded against a candidate + // 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() { - let possible_info = self.scraper.get_included_candidate_info(candidate_hash); - if let Some(info) = possible_info { + if let Some(info) = self.scraper.get_included_candidate_info(candidate_hash) { ctx.send_message( ChainSelectionMessage::DisputeConcludedAgainst( info.block_number, diff --git a/node/core/dispute-coordinator/src/scraping/candidates.rs b/node/core/dispute-coordinator/src/scraping/candidates.rs index 74686ae80856..3cab557f5fad 100644 --- a/node/core/dispute-coordinator/src/scraping/candidates.rs +++ b/node/core/dispute-coordinator/src/scraping/candidates.rs @@ -10,6 +10,8 @@ struct RefCountedCandidates { candidates: HashMap, } +// If a dispute is concluded against this candidate, then the ChainSelection +// subsystem needs block number and block hash to mark the relay parent as reverted. pub struct CandidateInfo { count: usize, pub block_number: BlockNumber, @@ -128,6 +130,9 @@ impl ScrapedCandidates { } } + // Gets candidate info, importantly containing relay parent block number and + // block hash. These are needed for relay block reversions based on concluded + // disputes. pub fn get_candidate_info(&mut self, candidate: CandidateHash) -> Option<&CandidateInfo> { self.candidates.candidates.get(&candidate) } diff --git a/node/core/dispute-coordinator/src/scraping/mod.rs b/node/core/dispute-coordinator/src/scraping/mod.rs index 3bb19fcf32f4..2b7fc6165da7 100644 --- a/node/core/dispute-coordinator/src/scraping/mod.rs +++ b/node/core/dispute-coordinator/src/scraping/mod.rs @@ -234,7 +234,7 @@ impl ChainScraper { ?block_number, "Processing included event" ); - self.included_candidates.insert(block_number, candidate_hash); + self.included_candidates.insert(block_number, block_hash, candidate_hash); included_receipts.push(receipt); }, CandidateEvent::CandidateBacked(receipt, _, _, _) => { diff --git a/node/subsystem-types/src/messages.rs b/node/subsystem-types/src/messages.rs index 76f7b9be8e3a..3377159e1ee2 100644 --- a/node/subsystem-types/src/messages.rs +++ b/node/subsystem-types/src/messages.rs @@ -592,7 +592,9 @@ pub enum ChainSelectionMessage { /// Request the best leaf containing the given block in its ancestry. Return `None` if /// there is no such leaf. BestLeafContaining(Hash, oneshot::Sender>), - /// Apply reverted status to blocks in chains containing the disputed candidate. + /// Passed block number and hash are for the relay parent of the disputed candidate. + /// The parent must be marked as reverted, and its children must be marked as + /// non-viable. DisputeConcludedAgainst(BlockNumber, Hash), } From 043f505e79f414b88ff465d898fea747fe818663 Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Thu, 5 Jan 2023 12:28:34 -0800 Subject: [PATCH 06/30] cargo fmt --- node/core/chain-selection/src/lib.rs | 6 +-- node/core/chain-selection/src/tree.rs | 47 +++++++++---------- .../dispute-coordinator/src/initialized.rs | 18 ++++--- .../src/scraping/candidates.rs | 6 +-- node/subsystem-types/src/messages.rs | 2 +- 5 files changed, 38 insertions(+), 41 deletions(-) diff --git a/node/core/chain-selection/src/lib.rs b/node/core/chain-selection/src/lib.rs index 1e6f8e4c1f7b..b48ee257d7bb 100644 --- a/node/core/chain-selection/src/lib.rs +++ b/node/core/chain-selection/src/lib.rs @@ -684,9 +684,9 @@ fn handle_approved_block(backend: &mut impl Backend, approved_block: Hash) -> Re // A dispute has concluded against a candidate. Here we revert the block containing // that candidate and mark its descendants as non-viable fn handle_concluded_dispute_reversions( - backend: &mut impl Backend, - block_number: u32, - block_hash: Hash + backend: &mut impl Backend, + block_number: u32, + block_hash: Hash, ) -> Result<(), Error> { let mut overlay = OverlayedBackend::new(backend); tree::apply_single_reversion(&mut overlay, block_hash, block_number)?; diff --git a/node/core/chain-selection/src/tree.rs b/node/core/chain-selection/src/tree.rs index bfd2c6312588..3cd3ac78e259 100644 --- a/node/core/chain-selection/src/tree.rs +++ b/node/core/chain-selection/src/tree.rs @@ -400,35 +400,34 @@ fn apply_ancestor_reversions( pub(crate) fn apply_single_reversion( backend: &mut OverlayedBackend, revert_hash: Hash, - revert_number: BlockNumber + revert_number: BlockNumber, ) -> Result<(), Error> { - let mut entry_to_revert = - match backend.load_block_entry(&revert_hash)? { - None => { - gum::warn!( - target: LOG_TARGET, - ?revert_hash, - // We keep parent block numbers in the disputes subsystem just - // for this log - revert_target = revert_number, - "The hammer has dropped. \ + let mut entry_to_revert = match backend.load_block_entry(&revert_hash)? { + None => { + gum::warn!( + target: LOG_TARGET, + ?revert_hash, + // We keep parent block numbers in the disputes subsystem just + // for this log + revert_target = revert_number, + "The hammer has dropped. \ A dispute has concluded against a finalized block. \ Please inform an adult.", - ); + ); - return Ok(()); - }, - Some(entry_to_revert) => { - gum::info!( - target: LOG_TARGET, - ?revert_hash, - revert_target = revert_number, - "A dispute has concluded against a block, causing it to be reverted.", - ); + return Ok(()) + }, + Some(entry_to_revert) => { + gum::info!( + target: LOG_TARGET, + ?revert_hash, + revert_target = revert_number, + "A dispute has concluded against a block, causing it to be reverted.", + ); - entry_to_revert - }, - }; + entry_to_revert + }, + }; entry_to_revert.viability.explicitly_reverted = true; // Marks children of reverted block as non-viable propagate_viability_update(backend, entry_to_revert)?; diff --git a/node/core/dispute-coordinator/src/initialized.rs b/node/core/dispute-coordinator/src/initialized.rs index 274509487503..3251bacb3cb2 100644 --- a/node/core/dispute-coordinator/src/initialized.rs +++ b/node/core/dispute-coordinator/src/initialized.rs @@ -31,8 +31,8 @@ use polkadot_node_primitives::{ }; use polkadot_node_subsystem::{ messages::{ - ApprovalVotingMessage, BlockDescription, DisputeCoordinatorMessage, - DisputeDistributionMessage, ImportStatementsResult, ChainSelectionMessage, + ApprovalVotingMessage, BlockDescription, ChainSelectionMessage, DisputeCoordinatorMessage, + DisputeDistributionMessage, ImportStatementsResult, }, overseer, ActivatedLeaf, ActiveLeavesUpdate, FromOrchestra, OverseerSignal, }; @@ -1031,14 +1031,12 @@ impl Initialized { // will need to mark the candidate's relay parent as reverted. if import_result.is_freshly_concluded_against() { if let Some(info) = self.scraper.get_included_candidate_info(candidate_hash) { - ctx.send_message( - ChainSelectionMessage::DisputeConcludedAgainst( - info.block_number, - info.block_hash, - )) - .await; - } - else { + ctx.send_message(ChainSelectionMessage::DisputeConcludedAgainst( + info.block_number, + info.block_hash, + )) + .await; + } else { gum::error!( target: LOG_TARGET, ?candidate_hash, diff --git a/node/core/dispute-coordinator/src/scraping/candidates.rs b/node/core/dispute-coordinator/src/scraping/candidates.rs index 3cab557f5fad..d71f870cb57c 100644 --- a/node/core/dispute-coordinator/src/scraping/candidates.rs +++ b/node/core/dispute-coordinator/src/scraping/candidates.rs @@ -10,7 +10,7 @@ struct RefCountedCandidates { candidates: HashMap, } -// If a dispute is concluded against this candidate, then the ChainSelection +// If a dispute is concluded against this candidate, then the ChainSelection // subsystem needs block number and block hash to mark the relay parent as reverted. pub struct CandidateInfo { count: usize, @@ -131,7 +131,7 @@ impl ScrapedCandidates { } // Gets candidate info, importantly containing relay parent block number and - // block hash. These are needed for relay block reversions based on concluded + // block hash. These are needed for relay block reversions based on concluded // disputes. pub fn get_candidate_info(&mut self, candidate: CandidateHash) -> Option<&CandidateInfo> { self.candidates.candidates.get(&candidate) @@ -143,7 +143,7 @@ impl ScrapedCandidates { block_hash: Hash, candidate_hash: CandidateHash, ) { - self.candidates.insert( block_number, block_hash, candidate_hash); + self.candidates.insert(block_number, block_hash, candidate_hash); self.candidates_by_block_number .entry(block_number) .or_default() diff --git a/node/subsystem-types/src/messages.rs b/node/subsystem-types/src/messages.rs index 3377159e1ee2..32f95c71a68d 100644 --- a/node/subsystem-types/src/messages.rs +++ b/node/subsystem-types/src/messages.rs @@ -593,7 +593,7 @@ pub enum ChainSelectionMessage { /// there is no such leaf. BestLeafContaining(Hash, oneshot::Sender>), /// Passed block number and hash are for the relay parent of the disputed candidate. - /// The parent must be marked as reverted, and its children must be marked as + /// The parent must be marked as reverted, and its children must be marked as /// non-viable. DisputeConcludedAgainst(BlockNumber, Hash), } From 85db7c925fe530c109099f9911363653b75b3e61 Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Thu, 5 Jan 2023 13:37:34 -0800 Subject: [PATCH 07/30] Implementers guide and code comments --- node/core/dispute-coordinator/src/scraping/mod.rs | 5 ++++- .../src/node/disputes/dispute-coordinator.md | 12 +++++++----- .../src/node/utility/chain-selection.md | 5 +++++ 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/node/core/dispute-coordinator/src/scraping/mod.rs b/node/core/dispute-coordinator/src/scraping/mod.rs index 2b7fc6165da7..4c309afcbe78 100644 --- a/node/core/dispute-coordinator/src/scraping/mod.rs +++ b/node/core/dispute-coordinator/src/scraping/mod.rs @@ -73,7 +73,10 @@ impl ScrapedUpdates { /// Chain scraper /// -/// Scrapes unfinalized chain in order to collect information from blocks. +/// Scrapes unfinalized chain in order to collect information from blocks. Chain scraping +/// during disputes enables critical spam prevention. It does so by updating two important +/// criteria determining whether a vote sent during dispute distribution is potential +/// spam. Namely, whether the candidate being voted on is backed or included. /// /// Concretely: /// diff --git a/roadmap/implementers-guide/src/node/disputes/dispute-coordinator.md b/roadmap/implementers-guide/src/node/disputes/dispute-coordinator.md index dc01664c295a..33ac78ee7077 100644 --- a/roadmap/implementers-guide/src/node/disputes/dispute-coordinator.md +++ b/roadmap/implementers-guide/src/node/disputes/dispute-coordinator.md @@ -9,7 +9,11 @@ In particular the dispute-coordinator is responsible for: - Ensuring that the node is able to raise a dispute in case an invalid candidate is found during approval checking. -- Ensuring approval votes will be recorded. +- Ensuring that backing and approval votes will be recorded on chain. With these + votes on chain we can be certain that appropriate targets for slashing will be + available for concluded disputes. Also, scraping these votes during a dispute + is necessary for critical spam prevention measures. +- Ensuring backing votes will never get overridden by explicit votes. - Coordinating actual participation in a dispute, ensuring that the node participates in any justified dispute in a way that ensures resolution of disputes on the network even in the case of many disputes raised (flood/DoS @@ -17,15 +21,13 @@ In particular the dispute-coordinator is responsible for: - Ensuring disputes resolve, even for candidates on abandoned forks as much as reasonably possible, to rule out "free tries" and thus guarantee our gambler's ruin property. -- Provide an API for chain selection, so we can prevent finalization of any +- Providing an API for chain selection, so we can prevent finalization of any chain which has included candidates for which a dispute is either ongoing or concluded invalid and avoid building on chains with an included invalid candidate. -- Provide an API for retrieving (resolved) disputes, including all votes, both +- Providing an API for retrieving (resolved) disputes, including all votes, both implicit (approval, backing) and explicit dispute votes. So validators can get rewarded/slashed accordingly. -- Ensure backing votes are recorded and will never get overridden by explicit - votes. ## Ensuring That Disputes Can Be Raised diff --git a/roadmap/implementers-guide/src/node/utility/chain-selection.md b/roadmap/implementers-guide/src/node/utility/chain-selection.md index 423b5ed406c0..5e00f8657668 100644 --- a/roadmap/implementers-guide/src/node/utility/chain-selection.md +++ b/roadmap/implementers-guide/src/node/utility/chain-selection.md @@ -8,6 +8,7 @@ This subsystem needs to update its information on the unfinalized chain: * On every leaf-activated signal * On every block-finalized signal * On every `ChainSelectionMessage::Approve` + * On every `ChainSelectionMessage::DisputeConcludedAgainst` * Periodically, to detect stagnation. Simple implementations of these updates do `O(n_unfinalized_blocks)` disk operations. If the amount of unfinalized blocks is relatively small, the updates should not take very much time. However, in cases where there are hundreds or thousands of unfinalized blocks the naive implementations of these update algorithms would have to be replaced with more sophisticated versions. @@ -29,6 +30,10 @@ Update the approval status of the referenced block. If the block was stagnant an If the required block is unknown or not viable, then return `None`. Iterate over all leaves, returning the first leaf containing the required block in its chain, and `None` otherwise. +### `ChainSelectionMessage::DisputeConcludedAgainst` +This message indicates that a dispute has concluded against a parachain block candidate. The message passes along the block number and hash of the disputed candidate's relay parent. The relay parent will be marked as reverted, and its descendants will be marked as non-viable. + + ### Periodically Detect stagnant blocks and apply the stagnant definition to all descendants. Update the set of viable leaves accordingly. From 161122d5ad5137a1c9438e84b171b0693db4cd43 Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Fri, 6 Jan 2023 11:08:45 -0800 Subject: [PATCH 08/30] More formatting, and naming issues --- node/core/chain-selection/src/lib.rs | 4 ++-- .../dispute-coordinator/src/scraping/candidates.rs | 10 +++++----- node/core/dispute-coordinator/src/scraping/mod.rs | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/node/core/chain-selection/src/lib.rs b/node/core/chain-selection/src/lib.rs index b48ee257d7bb..ca46ec64277e 100644 --- a/node/core/chain-selection/src/lib.rs +++ b/node/core/chain-selection/src/lib.rs @@ -467,7 +467,7 @@ where let _ = tx.send(best_containing); } ChainSelectionMessage::DisputeConcludedAgainst(block_number, block_hash) => { - handle_concluded_dispute_reversions(backend, block_number, block_hash)? + handle_concluded_dispute_reversion(backend, block_number, block_hash)? } } } @@ -683,7 +683,7 @@ fn handle_approved_block(backend: &mut impl Backend, approved_block: Hash) -> Re // A dispute has concluded against a candidate. Here we revert the block containing // that candidate and mark its descendants as non-viable -fn handle_concluded_dispute_reversions( +fn handle_concluded_dispute_reversion( backend: &mut impl Backend, block_number: u32, block_hash: Hash, diff --git a/node/core/dispute-coordinator/src/scraping/candidates.rs b/node/core/dispute-coordinator/src/scraping/candidates.rs index d71f870cb57c..b91d8328e6f0 100644 --- a/node/core/dispute-coordinator/src/scraping/candidates.rs +++ b/node/core/dispute-coordinator/src/scraping/candidates.rs @@ -13,7 +13,7 @@ struct RefCountedCandidates { // If a dispute is concluded against this candidate, then the ChainSelection // subsystem needs block number and block hash to mark the relay parent as reverted. pub struct CandidateInfo { - count: usize, + ref_count: usize, pub block_number: BlockNumber, pub block_hash: Hash, } @@ -33,8 +33,8 @@ impl RefCountedCandidates { ) { self.candidates .entry(candidate) - .or_insert(CandidateInfo { count: 0, block_number, block_hash }) - .count += 1; + .or_insert(CandidateInfo { ref_count: 0, block_number, block_hash }) + .ref_count += 1; } // If a `CandidateHash` with reference count equals to 1 is about to be removed - the @@ -43,9 +43,9 @@ impl RefCountedCandidates { // reference count is decreased and the candidate remains in the container. pub fn remove(&mut self, candidate: &CandidateHash) { match self.candidates.get_mut(candidate) { - Some(v) if v.count > 1 => v.count -= 1, + Some(v) if v.ref_count > 1 => v.ref_count -= 1, Some(v) => { - assert!(v.count == 1); + assert!(v.ref_count == 1); self.candidates.remove(candidate); }, None => {}, diff --git a/node/core/dispute-coordinator/src/scraping/mod.rs b/node/core/dispute-coordinator/src/scraping/mod.rs index 4c309afcbe78..fa62ccd1b954 100644 --- a/node/core/dispute-coordinator/src/scraping/mod.rs +++ b/node/core/dispute-coordinator/src/scraping/mod.rs @@ -75,7 +75,7 @@ impl ScrapedUpdates { /// /// Scrapes unfinalized chain in order to collect information from blocks. Chain scraping /// during disputes enables critical spam prevention. It does so by updating two important -/// criteria determining whether a vote sent during dispute distribution is potential +/// criteria determining whether a vote sent during dispute distribution is potential /// spam. Namely, whether the candidate being voted on is backed or included. /// /// Concretely: From 6a7203300ed991a286a8ec03e2e890b79e4af489 Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Fri, 6 Jan 2023 13:19:38 -0800 Subject: [PATCH 09/30] Wrote test for ChainSelection side of change --- node/core/chain-selection/src/lib.rs | 10 ++-- node/core/chain-selection/src/tests.rs | 67 ++++++++++++++++++++++++++ node/core/chain-selection/src/tree.rs | 1 - 3 files changed, 73 insertions(+), 5 deletions(-) diff --git a/node/core/chain-selection/src/lib.rs b/node/core/chain-selection/src/lib.rs index ca46ec64277e..3b36ed4db3b8 100644 --- a/node/core/chain-selection/src/lib.rs +++ b/node/core/chain-selection/src/lib.rs @@ -467,7 +467,8 @@ where let _ = tx.send(best_containing); } ChainSelectionMessage::DisputeConcludedAgainst(block_number, block_hash) => { - handle_concluded_dispute_reversion(backend, block_number, block_hash)? + let write_ops = handle_concluded_dispute_reversion(backend, block_number, block_hash)?; + backend.write(write_ops)?; } } } @@ -684,13 +685,14 @@ fn handle_approved_block(backend: &mut impl Backend, approved_block: Hash) -> Re // A dispute has concluded against a candidate. Here we revert the block containing // that candidate and mark its descendants as non-viable fn handle_concluded_dispute_reversion( - backend: &mut impl Backend, + backend: &impl Backend, block_number: u32, block_hash: Hash, -) -> Result<(), Error> { +) -> Result, Error> { let mut overlay = OverlayedBackend::new(backend); tree::apply_single_reversion(&mut overlay, block_hash, block_number)?; - Ok(()) + + Ok(overlay.into_write_ops().collect()) } fn detect_stagnant( diff --git a/node/core/chain-selection/src/tests.rs b/node/core/chain-selection/src/tests.rs index 404b854d894b..610d56e68e3d 100644 --- a/node/core/chain-selection/src/tests.rs +++ b/node/core/chain-selection/src/tests.rs @@ -2014,3 +2014,70 @@ fn stagnant_makes_childless_parent_leaf() { virtual_overseer }) } + +#[test] +fn dispute_concluded_against_message_triggers_proper_reversion() { + test_harness(|backend, _, mut virtual_overseer| async move { + // Building mini chain with 1 finalized block and 3 unfinalized blocks + let finalized_number = 0; + let finalized_hash = Hash::repeat_byte(0); + + let (head_hash, built_chain) = + construct_chain_on_base(vec![1, 2, 3], finalized_number, finalized_hash, |_| {}); + + import_blocks_into( + &mut virtual_overseer, + &backend, + Some((finalized_number, finalized_hash)), + built_chain.clone(), + ) + .await; + + // Checking mini chain + assert_backend_contains(&backend, built_chain.iter().map(|&(ref h, _)| h)); + assert_leaves(&backend, vec![head_hash]); + assert_leaves_query(&mut virtual_overseer, vec![head_hash]).await; + + let block_1_hash = backend.load_blocks_by_number(1).unwrap().get(0).unwrap().clone(); + let block_2_hash = backend.load_blocks_by_number(2).unwrap().get(0).unwrap().clone(); + + // Sending dispute conculded against message + let (_, write_rx) = backend.await_next_write(); + virtual_overseer + .send(FromOrchestra::Communication { + msg: ChainSelectionMessage::DisputeConcludedAgainst(2, block_2_hash), + }) + .await; + + write_rx.await.unwrap(); + + // Checking results: + // Block 2 should be explicitely reverted + assert_eq!( + backend + .load_block_entry(&block_2_hash) + .unwrap() + .unwrap() + .viability + .explicitly_reverted, + true + ); + // Block 3 should be non-viable, with 2 as its earliest unviable ancestor + assert_eq!( + backend + .load_block_entry(&head_hash) + .unwrap() + .unwrap() + .viability + .earliest_unviable_ancestor, + Some(block_2_hash) + ); + // Block 1 should be left as the only leaf + assert_leaves(&backend, vec![block_1_hash]); + + virtual_overseer + }) +} + +#[test] +fn dispute_concluded_against_finalized_sends_warning() {} diff --git a/node/core/chain-selection/src/tree.rs b/node/core/chain-selection/src/tree.rs index 3cd3ac78e259..708001d6122c 100644 --- a/node/core/chain-selection/src/tree.rs +++ b/node/core/chain-selection/src/tree.rs @@ -176,7 +176,6 @@ fn propagate_viability_update( *viability_pivots.entry(new_entry.parent_hash).or_insert(0) += 1; } } - backend.write_block_entry(new_entry); tree_frontier From 59e04535f97007afcb98ced615c30962b8b40db5 Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Fri, 6 Jan 2023 14:25:02 -0800 Subject: [PATCH 10/30] Added dispute coordinator side test --- node/core/dispute-coordinator/src/tests.rs | 133 ++++++++++++++++++++- 1 file changed, 132 insertions(+), 1 deletion(-) diff --git a/node/core/dispute-coordinator/src/tests.rs b/node/core/dispute-coordinator/src/tests.rs index 023c95d5e23c..b58e56f6d8d5 100644 --- a/node/core/dispute-coordinator/src/tests.rs +++ b/node/core/dispute-coordinator/src/tests.rs @@ -36,7 +36,7 @@ use polkadot_node_primitives::{ }; use polkadot_node_subsystem::{ messages::{ - ApprovalVotingMessage, ChainApiMessage, DisputeCoordinatorMessage, + ApprovalVotingMessage, ChainApiMessage, ChainSelectionMessage, DisputeCoordinatorMessage, DisputeDistributionMessage, ImportStatementsResult, }, overseer::FromOrchestra, @@ -3349,3 +3349,134 @@ fn participation_requests_reprioritized_for_newly_included() { }) }); } + +// When a dispute has concluded against a parachain block candidate we want to notify +// the chain selection subsystem. Then chain selection can revert the relay parent of +// the disputed candidate and mark all descendants as non-viable. This direct +// notification saves time compared to letting chain selection learn about a dispute +// conclusion from on chain sources. +#[test] +fn informs_chain_selection_when_dispute_concluded_against() { + 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_invalid_candidate_receipt(); + let parent_number = 1; + + let candidate_hash = candidate_receipt.hash(); + + // Copied from activate_leaf_at_session to give access to parent_hash + let block_header = Header { + parent_hash: test_state.last_block, + number: parent_number, + digest: dummy_digest(), + state_root: dummy_hash(), + extrinsics_root: dummy_hash(), + }; + let parent_hash = block_header.hash(); + + test_state + .activate_leaf_at_session( + &mut virtual_overseer, + session, + parent_number, + vec![make_candidate_included_event(candidate_receipt.clone())], + ) + .await; + + let supermajority_threshold = + polkadot_primitives::v2::supermajority_threshold(test_state.validators.len()); + + let (valid_vote, invalid_vote) = generate_opposing_votes_pair( + &test_state, + ValidatorIndex(2), + ValidatorIndex(1), + candidate_hash, + session, + VoteType::Explicit, + ) + .await; + + let (pending_confirmation, confirmation_rx) = oneshot::channel(); + virtual_overseer + .send(FromOrchestra::Communication { + msg: DisputeCoordinatorMessage::ImportStatements { + candidate_receipt: candidate_receipt.clone(), + session, + statements: vec![ + (valid_vote, ValidatorIndex(2)), + (invalid_vote, ValidatorIndex(1)), + ], + pending_confirmation: Some(pending_confirmation), + }, + }) + .await; + handle_approval_vote_request(&mut virtual_overseer, &candidate_hash, HashMap::new()) + .await; + assert_matches!(confirmation_rx.await.unwrap(), + ImportStatementsResult::ValidImport => {} + ); + + // Use a different expected commitments hash to ensure the candidate validation returns invalid. + participation_with_distribution( + &mut virtual_overseer, + &candidate_hash, + CandidateCommitments::default().hash(), + ) + .await; + + let mut statements = Vec::new(); + // minus 2, because of local vote and one previously imported invalid vote. + for i in (0_u32..supermajority_threshold as u32 - 2).map(|i| i + 3) { + let vote = test_state + .issue_explicit_statement_with_index( + ValidatorIndex(i), + candidate_hash, + session, + false, + ) + .await; + + statements.push((vote, ValidatorIndex(i as _))); + } + + virtual_overseer + .send(FromOrchestra::Communication { + msg: DisputeCoordinatorMessage::ImportStatements { + candidate_receipt: candidate_receipt.clone(), + session, + statements, + pending_confirmation: None, + }, + }) + .await; + handle_approval_vote_request(&mut virtual_overseer, &candidate_hash, HashMap::new()) + .await; + + test_state.clock.set(ACTIVE_DURATION_SECS + 1); + + assert_matches!( + virtual_overseer.recv().await, + AllMessages::ChainSelection( + ChainSelectionMessage::DisputeConcludedAgainst(block_number, block_hash) + ) => { + assert_eq!(block_number, parent_number); + assert_eq!(block_hash, parent_hash); + }, + "Overseer did not receive `ChainSelectionMessage::DisputeConcludedAgainst` message" + ); + + // Wrap up + virtual_overseer.send(FromOrchestra::Signal(OverseerSignal::Conclude)).await; + assert_matches!( + virtual_overseer.try_recv().await, + None => {} + ); + + test_state + }) + }); +} From 682545de102946f0964263b1d286b545a294326c Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Fri, 6 Jan 2023 14:28:00 -0800 Subject: [PATCH 11/30] FMT --- node/core/chain-selection/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/core/chain-selection/src/tests.rs b/node/core/chain-selection/src/tests.rs index 610d56e68e3d..40b349d3fe73 100644 --- a/node/core/chain-selection/src/tests.rs +++ b/node/core/chain-selection/src/tests.rs @@ -2048,7 +2048,7 @@ fn dispute_concluded_against_message_triggers_proper_reversion() { msg: ChainSelectionMessage::DisputeConcludedAgainst(2, block_2_hash), }) .await; - + write_rx.await.unwrap(); // Checking results: From 87fc1f575d7a82c2a17a86eb1fbe4802a870f3d1 Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Mon, 9 Jan 2023 11:50:06 -0800 Subject: [PATCH 12/30] Addressing Marcin's comments --- node/core/chain-selection/src/tests.rs | 35 ++++++++++++++++++- node/core/chain-selection/src/tree.rs | 4 +-- .../src/scraping/candidates.rs | 10 +++--- 3 files changed, 41 insertions(+), 8 deletions(-) diff --git a/node/core/chain-selection/src/tests.rs b/node/core/chain-selection/src/tests.rs index 40b349d3fe73..05bb2314b62d 100644 --- a/node/core/chain-selection/src/tests.rs +++ b/node/core/chain-selection/src/tests.rs @@ -2080,4 +2080,37 @@ fn dispute_concluded_against_message_triggers_proper_reversion() { } #[test] -fn dispute_concluded_against_finalized_sends_warning() {} +fn dispute_reversion_against_finalized_is_ignored() { + test_harness(|backend, _, mut virtual_overseer| async move { + // Building mini chain with 1 finalized block and 3 unfinalized blocks + let finalized_number = 0; + let finalized_hash = Hash::repeat_byte(0); + + let (head_hash, built_chain) = + construct_chain_on_base(vec![1], finalized_number, finalized_hash, |_| {}); + + import_blocks_into( + &mut virtual_overseer, + &backend, + Some((finalized_number, finalized_hash)), + built_chain.clone(), + ) + .await; + + // Checking mini chain + assert_backend_contains(&backend, built_chain.iter().map(|&(ref h, _)| h)); + + // Sending dispute conculded against message + virtual_overseer + .send(FromOrchestra::Communication { + msg: ChainSelectionMessage::DisputeConcludedAgainst(finalized_number, finalized_hash), + }) + .await; + + // Leaf should be head if reversion of finalized was properly ignored + assert_leaves(&backend, vec![head_hash]); + assert_leaves_query(&mut virtual_overseer, vec![head_hash]).await; + + virtual_overseer + }) +} diff --git a/node/core/chain-selection/src/tree.rs b/node/core/chain-selection/src/tree.rs index 708001d6122c..c16ed37fabeb 100644 --- a/node/core/chain-selection/src/tree.rs +++ b/node/core/chain-selection/src/tree.rs @@ -346,8 +346,8 @@ fn add_block( Ok(()) } -// Assuming that a block is already imported, accepts the number of the block -// as well as a list of reversions triggered by the block in ascending order. +/// Assuming that a block is already imported, accepts the number of the block +/// as well as a list of reversions triggered by the block in ascending order. fn apply_ancestor_reversions( backend: &mut OverlayedBackend, block_hash: Hash, diff --git a/node/core/dispute-coordinator/src/scraping/candidates.rs b/node/core/dispute-coordinator/src/scraping/candidates.rs index b91d8328e6f0..bbfa2e025547 100644 --- a/node/core/dispute-coordinator/src/scraping/candidates.rs +++ b/node/core/dispute-coordinator/src/scraping/candidates.rs @@ -27,9 +27,9 @@ impl RefCountedCandidates { // If `CandidateHash` already exists in the `HashMap` its reference count is increased. pub fn insert( &mut self, + candidate: CandidateHash, block_number: BlockNumber, block_hash: Hash, - candidate: CandidateHash, ) { self.candidates .entry(candidate) @@ -70,11 +70,11 @@ mod ref_counted_candidates_tests { let zero = CandidateHash(BlakeTwo256::hash(&vec![0])); let one = CandidateHash(BlakeTwo256::hash(&vec![1])); // add two separate candidates - container.insert(0, dummy_hash(), zero); // refcount == 1 - container.insert(0, dummy_hash(), one); + container.insert(zero, 0, dummy_hash()); // refcount == 1 + container.insert(one, 0, dummy_hash()); // and increase the reference count for the first - container.insert(0, dummy_hash(), zero); // refcount == 2 + container.insert(zero, 0, dummy_hash()); // refcount == 2 assert!(container.contains(&zero)); assert!(container.contains(&one)); @@ -143,7 +143,7 @@ impl ScrapedCandidates { block_hash: Hash, candidate_hash: CandidateHash, ) { - self.candidates.insert(block_number, block_hash, candidate_hash); + self.candidates.insert(candidate_hash, block_number, block_hash); self.candidates_by_block_number .entry(block_number) .or_default() From 013c4ebade93a1330c4a49864a52730e88254549 Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Mon, 9 Jan 2023 11:52:27 -0800 Subject: [PATCH 13/30] fmt --- node/core/chain-selection/src/tests.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/node/core/chain-selection/src/tests.rs b/node/core/chain-selection/src/tests.rs index 05bb2314b62d..537909b437f1 100644 --- a/node/core/chain-selection/src/tests.rs +++ b/node/core/chain-selection/src/tests.rs @@ -2103,7 +2103,10 @@ fn dispute_reversion_against_finalized_is_ignored() { // Sending dispute conculded against message virtual_overseer .send(FromOrchestra::Communication { - msg: ChainSelectionMessage::DisputeConcludedAgainst(finalized_number, finalized_hash), + msg: ChainSelectionMessage::DisputeConcludedAgainst( + finalized_number, + finalized_hash, + ), }) .await; From 6442838b6d218fda29656b2535abfb37b4ba32f2 Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Mon, 9 Jan 2023 12:26:50 -0800 Subject: [PATCH 14/30] Addressing further Marcin comment --- node/core/dispute-coordinator/src/scraping/candidates.rs | 4 ++-- node/core/dispute-coordinator/src/scraping/mod.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/node/core/dispute-coordinator/src/scraping/candidates.rs b/node/core/dispute-coordinator/src/scraping/candidates.rs index bbfa2e025547..6d0b5700efb3 100644 --- a/node/core/dispute-coordinator/src/scraping/candidates.rs +++ b/node/core/dispute-coordinator/src/scraping/candidates.rs @@ -139,9 +139,9 @@ impl ScrapedCandidates { pub fn insert( &mut self, + candidate_hash: CandidateHash, block_number: BlockNumber, block_hash: Hash, - candidate_hash: CandidateHash, ) { self.candidates.insert(candidate_hash, block_number, block_hash); self.candidates_by_block_number @@ -167,7 +167,7 @@ mod scraped_candidates_tests { fn stale_candidates_are_removed() { let mut candidates = ScrapedCandidates::new(); let target = CandidateHash(BlakeTwo256::hash(&vec![1, 2, 3])); - candidates.insert(1, dummy_hash(), target); + candidates.insert(target, 1, dummy_hash()); assert!(candidates.contains(&target)); diff --git a/node/core/dispute-coordinator/src/scraping/mod.rs b/node/core/dispute-coordinator/src/scraping/mod.rs index fa62ccd1b954..412d8e0ff5ce 100644 --- a/node/core/dispute-coordinator/src/scraping/mod.rs +++ b/node/core/dispute-coordinator/src/scraping/mod.rs @@ -237,7 +237,7 @@ impl ChainScraper { ?block_number, "Processing included event" ); - self.included_candidates.insert(block_number, block_hash, candidate_hash); + self.included_candidates.insert(candidate_hash, block_number, block_hash); included_receipts.push(receipt); }, CandidateEvent::CandidateBacked(receipt, _, _, _) => { @@ -248,7 +248,7 @@ impl ChainScraper { ?block_number, "Processing backed event" ); - self.backed_candidates.insert(block_number, block_hash, candidate_hash); + self.backed_candidates.insert(candidate_hash, block_number, block_hash); }, _ => { // skip the rest From bd6fe635d5db46675f2990d17b8231a01e2ced28 Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Mon, 9 Jan 2023 15:24:07 -0800 Subject: [PATCH 15/30] Removing unnecessary test line --- node/core/dispute-coordinator/src/tests.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/node/core/dispute-coordinator/src/tests.rs b/node/core/dispute-coordinator/src/tests.rs index cdbc1a68224e..eb285276d36c 100644 --- a/node/core/dispute-coordinator/src/tests.rs +++ b/node/core/dispute-coordinator/src/tests.rs @@ -3444,8 +3444,6 @@ fn informs_chain_selection_when_dispute_concluded_against() { handle_approval_vote_request(&mut virtual_overseer, &candidate_hash, HashMap::new()) .await; - test_state.clock.set(ACTIVE_DURATION_SECS + 1); - assert_matches!( virtual_overseer.recv().await, AllMessages::ChainSelection( From eff981b544bf8be1282b6b56a4bb7f30beb34f95 Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Wed, 11 Jan 2023 15:56:32 -0800 Subject: [PATCH 16/30] Rough draft addressing Robert changes --- node/core/chain-selection/src/lib.rs | 18 ++++--- node/core/chain-selection/src/tests.rs | 7 +-- node/core/chain-selection/src/tree.rs | 2 +- .../dispute-coordinator/src/initialized.rs | 11 ++-- .../src/scraping/candidates.rs | 53 ++++++------------- .../dispute-coordinator/src/scraping/mod.rs | 47 ++++++++++++---- node/core/dispute-coordinator/src/tests.rs | 6 +-- node/subsystem-types/src/messages.rs | 7 ++- .../src/node/utility/chain-selection.md | 4 +- 9 files changed, 80 insertions(+), 75 deletions(-) diff --git a/node/core/chain-selection/src/lib.rs b/node/core/chain-selection/src/lib.rs index 3b36ed4db3b8..04158861b3c1 100644 --- a/node/core/chain-selection/src/lib.rs +++ b/node/core/chain-selection/src/lib.rs @@ -32,6 +32,7 @@ use parity_scale_codec::Error as CodecError; use std::{ sync::Arc, time::{Duration, SystemTime, UNIX_EPOCH}, + collections::HashSet, }; use crate::backend::{Backend, BackendWriteOp, OverlayedBackend}; @@ -466,8 +467,8 @@ where let _ = tx.send(best_containing); } - ChainSelectionMessage::DisputeConcludedAgainst(block_number, block_hash) => { - let write_ops = handle_concluded_dispute_reversion(backend, block_number, block_hash)?; + ChainSelectionMessage::RevertBlocks(blocks_to_revert) => { + let write_ops = handle_revert_blocks(backend, blocks_to_revert)?; backend.write(write_ops)?; } } @@ -682,15 +683,16 @@ fn handle_approved_block(backend: &mut impl Backend, approved_block: Hash) -> Re backend.write(ops) } -// A dispute has concluded against a candidate. Here we revert the block containing -// that candidate and mark its descendants as non-viable -fn handle_concluded_dispute_reversion( +// Here we revert a provided group of blocks. The most common cause for this is that +// the dispute coordinator has notified chain selection +fn handle_revert_blocks( backend: &impl Backend, - block_number: u32, - block_hash: Hash, + blocks_to_revert: HashSet<(BlockNumber, Hash)>, ) -> Result, Error> { let mut overlay = OverlayedBackend::new(backend); - tree::apply_single_reversion(&mut overlay, block_hash, block_number)?; + for (block_number, block_hash) in blocks_to_revert { + tree::apply_single_reversion(&mut overlay, block_hash, block_number)?; + } Ok(overlay.into_write_ops().collect()) } diff --git a/node/core/chain-selection/src/tests.rs b/node/core/chain-selection/src/tests.rs index 537909b437f1..75dbf96bba30 100644 --- a/node/core/chain-selection/src/tests.rs +++ b/node/core/chain-selection/src/tests.rs @@ -2045,7 +2045,7 @@ fn dispute_concluded_against_message_triggers_proper_reversion() { let (_, write_rx) = backend.await_next_write(); virtual_overseer .send(FromOrchestra::Communication { - msg: ChainSelectionMessage::DisputeConcludedAgainst(2, block_2_hash), + msg: ChainSelectionMessage::RevertBlocks(Vec::from({(2, block_2_hash)})), }) .await; @@ -2103,10 +2103,7 @@ fn dispute_reversion_against_finalized_is_ignored() { // Sending dispute conculded against message virtual_overseer .send(FromOrchestra::Communication { - msg: ChainSelectionMessage::DisputeConcludedAgainst( - finalized_number, - finalized_hash, - ), + msg: ChainSelectionMessage::RevertBlocks(Vec::from({(finalized_number, finalized_hash)})), }) .await; diff --git a/node/core/chain-selection/src/tree.rs b/node/core/chain-selection/src/tree.rs index c16ed37fabeb..5d374ac7b57a 100644 --- a/node/core/chain-selection/src/tree.rs +++ b/node/core/chain-selection/src/tree.rs @@ -393,7 +393,7 @@ fn apply_ancestor_reversions( Ok(()) } -/// Marks a single block as explicitly reverted. Then propegates viability updates +/// Marks a single block as explicitly reverted, then propagates viability updates /// to all its children. This is triggered when the disputes subsystem signals that /// a dispute has concluded against a candidate. pub(crate) fn apply_single_reversion( diff --git a/node/core/dispute-coordinator/src/initialized.rs b/node/core/dispute-coordinator/src/initialized.rs index 3251bacb3cb2..23d435f6e44a 100644 --- a/node/core/dispute-coordinator/src/initialized.rs +++ b/node/core/dispute-coordinator/src/initialized.rs @@ -1030,12 +1030,11 @@ 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 let Some(info) = self.scraper.get_included_candidate_info(candidate_hash) { - ctx.send_message(ChainSelectionMessage::DisputeConcludedAgainst( - info.block_number, - info.block_hash, - )) - .await; + if let Some(blocks_to_revert) = + self.scraper.get_blocks_including_candidate(candidate_hash) + { + ctx.send_message(ChainSelectionMessage::RevertBlocks(blocks_to_revert.clone())) + .await; } else { gum::error!( target: LOG_TARGET, diff --git a/node/core/dispute-coordinator/src/scraping/candidates.rs b/node/core/dispute-coordinator/src/scraping/candidates.rs index 6d0b5700efb3..f77f306f3ed4 100644 --- a/node/core/dispute-coordinator/src/scraping/candidates.rs +++ b/node/core/dispute-coordinator/src/scraping/candidates.rs @@ -1,4 +1,4 @@ -use polkadot_primitives::v2::{BlockNumber, CandidateHash, Hash}; +use polkadot_primitives::v2::{BlockNumber, CandidateHash}; use std::collections::{BTreeMap, HashMap, HashSet}; /// Keeps `CandidateHash` in reference counted way. @@ -7,15 +7,7 @@ use std::collections::{BTreeMap, HashMap, HashSet}; /// Each `remove` decreases the reference count for the corresponding `CandidateHash`. /// If the reference count reaches 0 - the value is removed. struct RefCountedCandidates { - candidates: HashMap, -} - -// If a dispute is concluded against this candidate, then the ChainSelection -// subsystem needs block number and block hash to mark the relay parent as reverted. -pub struct CandidateInfo { - ref_count: usize, - pub block_number: BlockNumber, - pub block_hash: Hash, + candidates: HashMap, } impl RefCountedCandidates { @@ -25,16 +17,8 @@ impl RefCountedCandidates { // If `CandidateHash` doesn't exist in the `HashMap` it is created and its reference // count is set to 1. // If `CandidateHash` already exists in the `HashMap` its reference count is increased. - pub fn insert( - &mut self, - candidate: CandidateHash, - block_number: BlockNumber, - block_hash: Hash, - ) { - self.candidates - .entry(candidate) - .or_insert(CandidateInfo { ref_count: 0, block_number, block_hash }) - .ref_count += 1; + pub fn insert(&mut self, candidate: CandidateHash) { + *self.candidates.entry(candidate).or_default() += 1; } // If a `CandidateHash` with reference count equals to 1 is about to be removed - the @@ -43,9 +27,9 @@ impl RefCountedCandidates { // reference count is decreased and the candidate remains in the container. pub fn remove(&mut self, candidate: &CandidateHash) { match self.candidates.get_mut(candidate) { - Some(v) if v.ref_count > 1 => v.ref_count -= 1, + Some(v) if *v > 1 => *v -= 1, Some(v) => { - assert!(v.ref_count == 1); + assert!(*v == 1); self.candidates.remove(candidate); }, None => {}, @@ -61,7 +45,6 @@ impl RefCountedCandidates { mod ref_counted_candidates_tests { use super::*; use polkadot_primitives::v2::{BlakeTwo256, HashT}; - use test_helpers::dummy_hash; #[test] fn element_is_removed_when_refcount_reaches_zero() { @@ -70,11 +53,11 @@ mod ref_counted_candidates_tests { let zero = CandidateHash(BlakeTwo256::hash(&vec![0])); let one = CandidateHash(BlakeTwo256::hash(&vec![1])); // add two separate candidates - container.insert(zero, 0, dummy_hash()); // refcount == 1 - container.insert(one, 0, dummy_hash()); + container.insert(zero); // refcount == 1 + container.insert(one); // and increase the reference count for the first - container.insert(zero, 0, dummy_hash()); // refcount == 2 + container.insert(zero); // refcount == 2 assert!(container.contains(&zero)); assert!(container.contains(&one)); @@ -119,31 +102,27 @@ impl ScrapedCandidates { } // Removes all candidates up to a given height. The candidates at the block height are NOT removed. - pub fn remove_up_to_height(&mut self, height: &BlockNumber) { + pub fn remove_up_to_height(&mut self, height: &BlockNumber) -> HashSet { + let mut unique_candidates: HashSet = HashSet::new(); let not_stale = self.candidates_by_block_number.split_off(&height); let stale = std::mem::take(&mut self.candidates_by_block_number); self.candidates_by_block_number = not_stale; for candidates in stale.values() { for c in candidates { self.candidates.remove(c); + unique_candidates.insert(c.clone()); } } - } - // Gets candidate info, importantly containing relay parent block number and - // block hash. These are needed for relay block reversions based on concluded - // disputes. - pub fn get_candidate_info(&mut self, candidate: CandidateHash) -> Option<&CandidateInfo> { - self.candidates.candidates.get(&candidate) + unique_candidates } pub fn insert( &mut self, - candidate_hash: CandidateHash, block_number: BlockNumber, - block_hash: Hash, + candidate_hash: CandidateHash ) { - self.candidates.insert(candidate_hash, block_number, block_hash); + self.candidates.insert(candidate_hash); self.candidates_by_block_number .entry(block_number) .or_default() @@ -167,7 +146,7 @@ mod scraped_candidates_tests { fn stale_candidates_are_removed() { let mut candidates = ScrapedCandidates::new(); let target = CandidateHash(BlakeTwo256::hash(&vec![1, 2, 3])); - candidates.insert(target, 1, dummy_hash()); + candidates.insert(1, target); assert!(candidates.contains(&target)); diff --git a/node/core/dispute-coordinator/src/scraping/mod.rs b/node/core/dispute-coordinator/src/scraping/mod.rs index 412d8e0ff5ce..d40ae7b22de3 100644 --- a/node/core/dispute-coordinator/src/scraping/mod.rs +++ b/node/core/dispute-coordinator/src/scraping/mod.rs @@ -14,7 +14,10 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -use std::num::NonZeroUsize; +use std::{ + collections::{HashMap, HashSet}, + num::NonZeroUsize, +}; use futures::channel::oneshot; use lru::LruCache; @@ -34,8 +37,6 @@ use crate::{ LOG_TARGET, }; -use self::candidates::CandidateInfo; - #[cfg(test)] mod tests; @@ -100,6 +101,11 @@ pub struct ChainScraper { /// We assume that ancestors of cached blocks are already processed, i.e. we have saved /// corresponding included candidates. last_observed_blocks: LruCache, + /// Maps included candidate hashes to one or more relay block heights and hashes. + /// These correspond to all the relay blocks which marked a candidate as included, + /// and are needed to apply reversions in case a dispute is concluded against the + /// candidate. + inclusions_per_candidate: HashMap>, } impl ChainScraper { @@ -124,6 +130,7 @@ impl ChainScraper { included_candidates: candidates::ScrapedCandidates::new(), backed_candidates: candidates::ScrapedCandidates::new(), last_observed_blocks: LruCache::new(LRU_OBSERVED_BLOCKS_CAPACITY), + inclusions_per_candidate: HashMap::new(), }; let update = ActiveLeavesUpdate { activated: Some(initial_head), deactivated: Default::default() }; @@ -200,7 +207,23 @@ impl ChainScraper { { Some(key_to_prune) => { self.backed_candidates.remove_up_to_height(&key_to_prune); - self.included_candidates.remove_up_to_height(&key_to_prune); + let candidates_pruned = self.included_candidates.remove_up_to_height(&key_to_prune); + // Removing entries from inclusions per candidate referring to blocks before + // key_to_prune + for candidate_hash in candidates_pruned { + self.inclusions_per_candidate.entry(candidate_hash).and_modify(|inclusions| { + *inclusions = inclusions + .clone() + .into_iter() + .filter(|inclusion| inclusion.0 >= key_to_prune) + .collect::>(); + }); + if let Some(inclusions) = self.inclusions_per_candidate.get(&candidate_hash) { + if inclusions.is_empty() { + self.inclusions_per_candidate.remove(&candidate_hash); + } + } + } }, None => { // Nothing to prune. We are still in the beginning of the chain and there are not @@ -237,7 +260,13 @@ impl ChainScraper { ?block_number, "Processing included event" ); - self.included_candidates.insert(candidate_hash, block_number, block_hash); + self.included_candidates.insert(block_number, candidate_hash); + self.inclusions_per_candidate + .entry(candidate_hash) + .and_modify(|blocks_including| { + blocks_including.insert((block_number, block_hash)); + }) + .or_insert(HashSet::from([(block_number, block_hash)])); included_receipts.push(receipt); }, CandidateEvent::CandidateBacked(receipt, _, _, _) => { @@ -248,7 +277,7 @@ impl ChainScraper { ?block_number, "Processing backed event" ); - self.backed_candidates.insert(candidate_hash, block_number, block_hash); + self.backed_candidates.insert(block_number, candidate_hash); }, _ => { // skip the rest @@ -324,11 +353,11 @@ impl ChainScraper { return Ok(ancestors) } - pub fn get_included_candidate_info( + pub fn get_blocks_including_candidate( &mut self, candidate: CandidateHash, - ) -> Option<&CandidateInfo> { - self.included_candidates.get_candidate_info(candidate) + ) -> Option<&HashSet<(BlockNumber, Hash)>> { + self.inclusions_per_candidate.get(&candidate) } } diff --git a/node/core/dispute-coordinator/src/tests.rs b/node/core/dispute-coordinator/src/tests.rs index eb285276d36c..391066177eb4 100644 --- a/node/core/dispute-coordinator/src/tests.rs +++ b/node/core/dispute-coordinator/src/tests.rs @@ -3342,7 +3342,7 @@ fn participation_requests_reprioritized_for_newly_included() { // the chain selection subsystem. Then chain selection can revert the relay parent of // the disputed candidate and mark all descendants as non-viable. This direct // notification saves time compared to letting chain selection learn about a dispute -// conclusion from on chain sources. +// conclusion from an on chain revert log. #[test] fn informs_chain_selection_when_dispute_concluded_against() { test_harness(|mut test_state, mut virtual_overseer| { @@ -3447,12 +3447,12 @@ fn informs_chain_selection_when_dispute_concluded_against() { assert_matches!( virtual_overseer.recv().await, AllMessages::ChainSelection( - ChainSelectionMessage::DisputeConcludedAgainst(block_number, block_hash) + ChainSelectionMessage::RevertBlocks(block_number, block_hash) ) => { assert_eq!(block_number, parent_number); assert_eq!(block_hash, parent_hash); }, - "Overseer did not receive `ChainSelectionMessage::DisputeConcludedAgainst` message" + "Overseer did not receive `ChainSelectionMessage::RevertBlocks` message" ); // Wrap up diff --git a/node/subsystem-types/src/messages.rs b/node/subsystem-types/src/messages.rs index ca1d5d52a820..13a82f25c6c8 100644 --- a/node/subsystem-types/src/messages.rs +++ b/node/subsystem-types/src/messages.rs @@ -518,10 +518,9 @@ pub enum ChainSelectionMessage { /// Request the best leaf containing the given block in its ancestry. Return `None` if /// there is no such leaf. BestLeafContaining(Hash, oneshot::Sender>), - /// Passed block number and hash are for the relay parent of the disputed candidate. - /// The parent must be marked as reverted, and its children must be marked as - /// non-viable. - DisputeConcludedAgainst(BlockNumber, Hash), + /// The passed blocks must be marked as reverted, and their children must be marked + /// as non-viable. + RevertBlocks(HashSet<(BlockNumber, Hash)>), } /// A sender for the result of a runtime API request. diff --git a/roadmap/implementers-guide/src/node/utility/chain-selection.md b/roadmap/implementers-guide/src/node/utility/chain-selection.md index f649d05217aa..48366d8d3a7b 100644 --- a/roadmap/implementers-guide/src/node/utility/chain-selection.md +++ b/roadmap/implementers-guide/src/node/utility/chain-selection.md @@ -8,7 +8,7 @@ This subsystem needs to update its information on the unfinalized chain: * On every leaf-activated signal * On every block-finalized signal * On every `ChainSelectionMessage::Approve` - * On every `ChainSelectionMessage::DisputeConcludedAgainst` + * On every `ChainSelectionMessage::RevertBlocks` * Periodically, to detect stagnation. Simple implementations of these updates do `O(n_unfinalized_blocks)` disk operations. If the amount of unfinalized blocks is relatively small, the updates should not take very much time. However, in cases where there are hundreds or thousands of unfinalized blocks the naive implementations of these update algorithms would have to be replaced with more sophisticated versions. @@ -33,7 +33,7 @@ Gets all leaves of the chain, i.e. block hashes that are suitable to build upon If the required block is unknown or not viable, then return `None`. Iterate over all leaves in order of descending weight, returning the first leaf containing the required block in its chain, and `None` otherwise. -### `ChainSelectionMessage::DisputeConcludedAgainst` +### `ChainSelectionMessage::RevertBlocks` This message indicates that a dispute has concluded against a parachain block candidate. The message passes along the block number and hash of the disputed candidate's relay parent. The relay parent will be marked as reverted, and its descendants will be marked as non-viable. From 428e016e5a5048199534fc37717113968855d5ef Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Thu, 12 Jan 2023 11:26:37 -0800 Subject: [PATCH 17/30] Clean up and test modification --- .../src/scraping/candidates.rs | 7 +--- .../dispute-coordinator/src/scraping/mod.rs | 10 ++--- node/core/dispute-coordinator/src/tests.rs | 41 ++++++++++++++----- 3 files changed, 35 insertions(+), 23 deletions(-) diff --git a/node/core/dispute-coordinator/src/scraping/candidates.rs b/node/core/dispute-coordinator/src/scraping/candidates.rs index f77f306f3ed4..f502534f6150 100644 --- a/node/core/dispute-coordinator/src/scraping/candidates.rs +++ b/node/core/dispute-coordinator/src/scraping/candidates.rs @@ -117,11 +117,7 @@ impl ScrapedCandidates { unique_candidates } - pub fn insert( - &mut self, - block_number: BlockNumber, - candidate_hash: CandidateHash - ) { + pub fn insert(&mut self, block_number: BlockNumber, candidate_hash: CandidateHash) { self.candidates.insert(candidate_hash); self.candidates_by_block_number .entry(block_number) @@ -140,7 +136,6 @@ impl ScrapedCandidates { mod scraped_candidates_tests { use super::*; use polkadot_primitives::v2::{BlakeTwo256, HashT}; - use test_helpers::dummy_hash; #[test] fn stale_candidates_are_removed() { diff --git a/node/core/dispute-coordinator/src/scraping/mod.rs b/node/core/dispute-coordinator/src/scraping/mod.rs index d40ae7b22de3..cc8ee6467170 100644 --- a/node/core/dispute-coordinator/src/scraping/mod.rs +++ b/node/core/dispute-coordinator/src/scraping/mod.rs @@ -208,16 +208,12 @@ impl ChainScraper { Some(key_to_prune) => { self.backed_candidates.remove_up_to_height(&key_to_prune); let candidates_pruned = self.included_candidates.remove_up_to_height(&key_to_prune); - // Removing entries from inclusions per candidate referring to blocks before + // Removing entries from inclusions per candidate referring to blocks before // key_to_prune for candidate_hash in candidates_pruned { self.inclusions_per_candidate.entry(candidate_hash).and_modify(|inclusions| { - *inclusions = inclusions - .clone() - .into_iter() - .filter(|inclusion| inclusion.0 >= key_to_prune) - .collect::>(); - }); + inclusions.retain(|inclusion| inclusion.0 >= key_to_prune); + }); if let Some(inclusions) = self.inclusions_per_candidate.get(&candidate_hash) { if inclusions.is_empty() { self.inclusions_per_candidate.remove(&candidate_hash); diff --git a/node/core/dispute-coordinator/src/tests.rs b/node/core/dispute-coordinator/src/tests.rs index 391066177eb4..47191093a654 100644 --- a/node/core/dispute-coordinator/src/tests.rs +++ b/node/core/dispute-coordinator/src/tests.rs @@ -3339,7 +3339,7 @@ fn participation_requests_reprioritized_for_newly_included() { } // When a dispute has concluded against a parachain block candidate we want to notify -// the chain selection subsystem. Then chain selection can revert the relay parent of +// the chain selection subsystem. Then chain selection can revert the relay parents of // the disputed candidate and mark all descendants as non-viable. This direct // notification saves time compared to letting chain selection learn about a dispute // conclusion from an on chain revert log. @@ -3352,29 +3352,49 @@ fn informs_chain_selection_when_dispute_concluded_against() { test_state.handle_resume_sync(&mut virtual_overseer, session).await; let candidate_receipt = make_invalid_candidate_receipt(); - let parent_number = 1; + let parent_1_number = 1; + let parent_2_number = 2; let candidate_hash = candidate_receipt.hash(); - // Copied from activate_leaf_at_session to give access to parent_hash - let block_header = Header { + // Including test candidate in 2 different parent blocks + let block_1_header = Header { parent_hash: test_state.last_block, - number: parent_number, + number: parent_1_number, digest: dummy_digest(), state_root: dummy_hash(), extrinsics_root: dummy_hash(), }; - let parent_hash = block_header.hash(); + let parent_1_hash = block_1_header.hash(); test_state .activate_leaf_at_session( &mut virtual_overseer, session, - parent_number, + parent_1_number, vec![make_candidate_included_event(candidate_receipt.clone())], ) .await; + let block_2_header = Header { + parent_hash: test_state.last_block, + number: parent_2_number, + digest: dummy_digest(), + state_root: dummy_hash(), + extrinsics_root: dummy_hash(), + }; + let parent_2_hash = block_2_header.hash(); + + test_state + .activate_leaf_at_session( + &mut virtual_overseer, + session, + parent_2_number, + vec![make_candidate_included_event(candidate_receipt.clone())], + ) + .await; + + // Concluding dispute let supermajority_threshold = polkadot_primitives::v2::supermajority_threshold(test_state.validators.len()); @@ -3444,13 +3464,14 @@ fn informs_chain_selection_when_dispute_concluded_against() { handle_approval_vote_request(&mut virtual_overseer, &candidate_hash, HashMap::new()) .await; + // Checking that concluded dispute has signaled the reversion of all parent blocks. assert_matches!( virtual_overseer.recv().await, AllMessages::ChainSelection( - ChainSelectionMessage::RevertBlocks(block_number, block_hash) + ChainSelectionMessage::RevertBlocks(revert_set) ) => { - assert_eq!(block_number, parent_number); - assert_eq!(block_hash, parent_hash); + assert!(revert_set.contains(&(parent_1_number, parent_1_hash))); + assert!(revert_set.contains(&(parent_2_number, parent_2_hash))); }, "Overseer did not receive `ChainSelectionMessage::RevertBlocks` message" ); From 9f6f2ab7df271823dc1f09b187908da1e07beb86 Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Thu, 12 Jan 2023 17:25:36 -0800 Subject: [PATCH 18/30] Majorly refactored scraper change --- node/core/chain-selection/src/lib.rs | 2 +- .../dispute-coordinator/src/initialized.rs | 2 +- .../src/scraping/candidates.rs | 6 +- .../dispute-coordinator/src/scraping/mod.rs | 91 +++++++++++++------ .../dispute-coordinator/src/scraping/tests.rs | 70 ++++++++++++++ node/subsystem-types/src/messages.rs | 2 +- .../src/node/utility/chain-selection.md | 2 +- 7 files changed, 138 insertions(+), 37 deletions(-) diff --git a/node/core/chain-selection/src/lib.rs b/node/core/chain-selection/src/lib.rs index 04158861b3c1..15ee45c410a4 100644 --- a/node/core/chain-selection/src/lib.rs +++ b/node/core/chain-selection/src/lib.rs @@ -687,7 +687,7 @@ fn handle_approved_block(backend: &mut impl Backend, approved_block: Hash) -> Re // the dispute coordinator has notified chain selection fn handle_revert_blocks( backend: &impl Backend, - blocks_to_revert: HashSet<(BlockNumber, Hash)>, + blocks_to_revert: Vec<(BlockNumber, Hash)>, ) -> Result, Error> { let mut overlay = OverlayedBackend::new(backend); for (block_number, block_hash) in blocks_to_revert { diff --git a/node/core/dispute-coordinator/src/initialized.rs b/node/core/dispute-coordinator/src/initialized.rs index 23d435f6e44a..a4955d87add8 100644 --- a/node/core/dispute-coordinator/src/initialized.rs +++ b/node/core/dispute-coordinator/src/initialized.rs @@ -1031,7 +1031,7 @@ impl Initialized { // will need to mark the candidate's relay parent as reverted. if import_result.is_freshly_concluded_against() { if let Some(blocks_to_revert) = - self.scraper.get_blocks_including_candidate(candidate_hash) + self.scraper.get_blocks_including_candidate(&candidate_hash) { ctx.send_message(ChainSelectionMessage::RevertBlocks(blocks_to_revert.clone())) .await; diff --git a/node/core/dispute-coordinator/src/scraping/candidates.rs b/node/core/dispute-coordinator/src/scraping/candidates.rs index f502534f6150..2fe797768cc2 100644 --- a/node/core/dispute-coordinator/src/scraping/candidates.rs +++ b/node/core/dispute-coordinator/src/scraping/candidates.rs @@ -102,19 +102,15 @@ impl ScrapedCandidates { } // Removes all candidates up to a given height. The candidates at the block height are NOT removed. - pub fn remove_up_to_height(&mut self, height: &BlockNumber) -> HashSet { - let mut unique_candidates: HashSet = HashSet::new(); + pub fn remove_up_to_height(&mut self, height: &BlockNumber) { let not_stale = self.candidates_by_block_number.split_off(&height); let stale = std::mem::take(&mut self.candidates_by_block_number); self.candidates_by_block_number = not_stale; for candidates in stale.values() { for c in candidates { self.candidates.remove(c); - unique_candidates.insert(c.clone()); } } - - unique_candidates } pub fn insert(&mut self, block_number: BlockNumber, candidate_hash: CandidateHash) { diff --git a/node/core/dispute-coordinator/src/scraping/mod.rs b/node/core/dispute-coordinator/src/scraping/mod.rs index cc8ee6467170..0e12f38aa2d5 100644 --- a/node/core/dispute-coordinator/src/scraping/mod.rs +++ b/node/core/dispute-coordinator/src/scraping/mod.rs @@ -14,10 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -use std::{ - collections::{HashMap, HashSet}, - num::NonZeroUsize, -}; +use std::{collections::HashMap, num::NonZeroUsize}; use futures::channel::oneshot; use lru::LruCache; @@ -72,6 +69,60 @@ impl ScrapedUpdates { } } +/// A structure meant to facilitate chain reversions in the event of a dispute +/// concluding against a candidate. Each candidate hash maps to a vector of block +/// numbers and hashes for all blocks which included the candidate. The entries +/// in each vector are ordered by decreasing parent block number to facilitate +/// minimal cost pruning. +pub struct InclusionsPerCandidate { + inclusions_inner: HashMap>, +} + +impl InclusionsPerCandidate { + pub fn new() -> Self { + Self { inclusions_inner: HashMap::new() } + } + + // Insert parent block into vector for the candidate hash it is including, + // maintaining ordering by decreasing parent block number. + pub fn insert( + &mut self, + candidate_hash: CandidateHash, + block_number: BlockNumber, + block_hash: Hash, + ) { + if let Some(blocks_including) = self.inclusions_inner.get_mut(&candidate_hash) { + for idx in 0..blocks_including.len() { + if blocks_including[idx].0 < block_number { + // Idx is between 0 and blocks_including.len(), therefore in bounds. QED + blocks_including.insert(idx, (block_number, block_hash)); + } else if idx == blocks_including.len() - 1 { + blocks_including.push((block_number, block_hash)); + } + } + } else { + self.inclusions_inner + .insert(candidate_hash, Vec::from([(block_number, block_hash)])); + } + } + + pub fn remove_up_to_height(&mut self, height: &BlockNumber) { + for including_blocks in self.inclusions_inner.values_mut() { + while including_blocks.len() > 0 && + including_blocks[including_blocks.len() - 1].0 < *height + { + // Since parent_blocks length is positive, parent_blocks.len() - 1 is in bounds. QED + including_blocks.pop(); + } + } + self.inclusions_inner.retain(|_, including_blocks| including_blocks.len() > 0); + } + + pub fn get(&mut self, candidate: &CandidateHash) -> Option<&Vec<(BlockNumber, Hash)>> { + self.inclusions_inner.get(candidate) + } +} + /// Chain scraper /// /// Scrapes unfinalized chain in order to collect information from blocks. Chain scraping @@ -105,7 +156,7 @@ pub struct ChainScraper { /// These correspond to all the relay blocks which marked a candidate as included, /// and are needed to apply reversions in case a dispute is concluded against the /// candidate. - inclusions_per_candidate: HashMap>, + inclusions_per_candidate: InclusionsPerCandidate, } impl ChainScraper { @@ -130,7 +181,7 @@ impl ChainScraper { included_candidates: candidates::ScrapedCandidates::new(), backed_candidates: candidates::ScrapedCandidates::new(), last_observed_blocks: LruCache::new(LRU_OBSERVED_BLOCKS_CAPACITY), - inclusions_per_candidate: HashMap::new(), + inclusions_per_candidate: InclusionsPerCandidate::new(), }; let update = ActiveLeavesUpdate { activated: Some(initial_head), deactivated: Default::default() }; @@ -207,19 +258,8 @@ impl ChainScraper { { Some(key_to_prune) => { self.backed_candidates.remove_up_to_height(&key_to_prune); - let candidates_pruned = self.included_candidates.remove_up_to_height(&key_to_prune); - // Removing entries from inclusions per candidate referring to blocks before - // key_to_prune - for candidate_hash in candidates_pruned { - self.inclusions_per_candidate.entry(candidate_hash).and_modify(|inclusions| { - inclusions.retain(|inclusion| inclusion.0 >= key_to_prune); - }); - if let Some(inclusions) = self.inclusions_per_candidate.get(&candidate_hash) { - if inclusions.is_empty() { - self.inclusions_per_candidate.remove(&candidate_hash); - } - } - } + self.included_candidates.remove_up_to_height(&key_to_prune); + self.inclusions_per_candidate.remove_up_to_height(&key_to_prune) }, None => { // Nothing to prune. We are still in the beginning of the chain and there are not @@ -257,12 +297,7 @@ impl ChainScraper { "Processing included event" ); self.included_candidates.insert(block_number, candidate_hash); - self.inclusions_per_candidate - .entry(candidate_hash) - .and_modify(|blocks_including| { - blocks_including.insert((block_number, block_hash)); - }) - .or_insert(HashSet::from([(block_number, block_hash)])); + self.inclusions_per_candidate.insert(candidate_hash, block_number, block_hash); included_receipts.push(receipt); }, CandidateEvent::CandidateBacked(receipt, _, _, _) => { @@ -351,9 +386,9 @@ impl ChainScraper { pub fn get_blocks_including_candidate( &mut self, - candidate: CandidateHash, - ) -> Option<&HashSet<(BlockNumber, Hash)>> { - self.inclusions_per_candidate.get(&candidate) + candidate: &CandidateHash, + ) -> Option<&Vec<(BlockNumber, Hash)>> { + self.inclusions_per_candidate.get(candidate) } } diff --git a/node/core/dispute-coordinator/src/scraping/tests.rs b/node/core/dispute-coordinator/src/scraping/tests.rs index 3a6befa2002d..d4a6adb22f3f 100644 --- a/node/core/dispute-coordinator/src/scraping/tests.rs +++ b/node/core/dispute-coordinator/src/scraping/tests.rs @@ -578,3 +578,73 @@ fn scraper_handles_the_same_candidate_incuded_in_two_different_block_heights() { assert!(!scraper.is_candidate_included(&magic_candidate.hash())); }); } + +#[test] +fn inclusions_per_candidate_properly_adds_and_prunes() { + const TEST_TARGET_BLOCK_NUMBER: BlockNumber = 2; + const TEST_TARGET_BLOCK_NUMBER_2: BlockNumber = 3; + + // How many blocks should we skip before sending a leaf update. + const BLOCKS_TO_SKIP: usize = 4; + + futures::executor::block_on(async { + let (state, mut virtual_overseer) = TestState::new().await; + + let TestState { mut chain, mut scraper, mut ctx } = state; + + // 1 because `TestState` starts at leaf 1. + let next_update = (1..BLOCKS_TO_SKIP).map(|_| next_leaf(&mut chain)).last().unwrap(); + + let mut finalized_block_number = 1; + let expected_ancestry_len = BLOCKS_TO_SKIP - finalized_block_number as usize; + let overseer_fut = overseer_process_active_leaves_update( + &mut virtual_overseer, + &chain, + finalized_block_number, + expected_ancestry_len, + |block_num| { + if block_num == TEST_TARGET_BLOCK_NUMBER || block_num == TEST_TARGET_BLOCK_NUMBER_2 + { + get_backed_and_included_candidate_events(TEST_TARGET_BLOCK_NUMBER) + } else { + vec![] + } + }, + ); + join(process_active_leaves_update(ctx.sender(), &mut scraper, next_update), overseer_fut) + .await; + + let candidate = make_candidate_receipt(get_block_number_hash(TEST_TARGET_BLOCK_NUMBER)); + + // We included the same candidate at two different block heights. So both blocks in which + // the candidate is included are recorded + assert_eq!( + scraper.get_blocks_including_candidate(&candidate.hash()), + Some(&Vec::from([ + (TEST_TARGET_BLOCK_NUMBER_2, get_block_number_hash(TEST_TARGET_BLOCK_NUMBER_2)), + (TEST_TARGET_BLOCK_NUMBER, get_block_number_hash(TEST_TARGET_BLOCK_NUMBER)) + ])) + ); + + // After `DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION` blocks the earlier inclusion should be removed + finalized_block_number = + TEST_TARGET_BLOCK_NUMBER + DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION; + process_finalized_block(&mut scraper, &finalized_block_number); + + // The later inclusion should still be present, as we haven't exceeded its lifetime + assert_eq!( + scraper.get_blocks_including_candidate(&candidate.hash()), + Some(&Vec::from([( + TEST_TARGET_BLOCK_NUMBER_2, + get_block_number_hash(TEST_TARGET_BLOCK_NUMBER_2) + )])) + ); + + finalized_block_number = + TEST_TARGET_BLOCK_NUMBER_2 + DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION; + process_finalized_block(&mut scraper, &finalized_block_number); + + // Now both inclusions have exceeded their lifetimes after finalization and should be purged + assert_eq!(scraper.get_blocks_including_candidate(&candidate.hash()), None); + }); +} diff --git a/node/subsystem-types/src/messages.rs b/node/subsystem-types/src/messages.rs index 13a82f25c6c8..1397795436c0 100644 --- a/node/subsystem-types/src/messages.rs +++ b/node/subsystem-types/src/messages.rs @@ -520,7 +520,7 @@ pub enum ChainSelectionMessage { BestLeafContaining(Hash, oneshot::Sender>), /// The passed blocks must be marked as reverted, and their children must be marked /// as non-viable. - RevertBlocks(HashSet<(BlockNumber, Hash)>), + RevertBlocks(Vec<(BlockNumber, Hash)>), } /// A sender for the result of a runtime API request. diff --git a/roadmap/implementers-guide/src/node/utility/chain-selection.md b/roadmap/implementers-guide/src/node/utility/chain-selection.md index 48366d8d3a7b..640691e55961 100644 --- a/roadmap/implementers-guide/src/node/utility/chain-selection.md +++ b/roadmap/implementers-guide/src/node/utility/chain-selection.md @@ -34,7 +34,7 @@ Gets all leaves of the chain, i.e. block hashes that are suitable to build upon If the required block is unknown or not viable, then return `None`. Iterate over all leaves in order of descending weight, returning the first leaf containing the required block in its chain, and `None` otherwise. ### `ChainSelectionMessage::RevertBlocks` -This message indicates that a dispute has concluded against a parachain block candidate. The message passes along the block number and hash of the disputed candidate's relay parent. The relay parent will be marked as reverted, and its descendants will be marked as non-viable. +This message indicates that a dispute has concluded against a parachain block candidate. The message passes along a vector containing the block number and block hash of each block where the disputed candidate was included. The passed blocks will be marked as reverted, and their descendants will be marked as non-viable. ### Periodically From 5b89cf8b3308ed34faa02cce3c78ba046ec2c91e Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Thu, 12 Jan 2023 17:31:21 -0800 Subject: [PATCH 19/30] Minor fixes for ChainSelection --- node/core/chain-selection/src/lib.rs | 1 - node/core/chain-selection/src/tests.rs | 8 ++++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/node/core/chain-selection/src/lib.rs b/node/core/chain-selection/src/lib.rs index 15ee45c410a4..895c61f70d3b 100644 --- a/node/core/chain-selection/src/lib.rs +++ b/node/core/chain-selection/src/lib.rs @@ -32,7 +32,6 @@ use parity_scale_codec::Error as CodecError; use std::{ sync::Arc, time::{Duration, SystemTime, UNIX_EPOCH}, - collections::HashSet, }; use crate::backend::{Backend, BackendWriteOp, OverlayedBackend}; diff --git a/node/core/chain-selection/src/tests.rs b/node/core/chain-selection/src/tests.rs index 75dbf96bba30..c9f25d9347ad 100644 --- a/node/core/chain-selection/src/tests.rs +++ b/node/core/chain-selection/src/tests.rs @@ -2016,7 +2016,7 @@ fn stagnant_makes_childless_parent_leaf() { } #[test] -fn dispute_concluded_against_message_triggers_proper_reversion() { +fn revert_blocks_message_triggers_proper_reversion() { test_harness(|backend, _, mut virtual_overseer| async move { // Building mini chain with 1 finalized block and 3 unfinalized blocks let finalized_number = 0; @@ -2045,7 +2045,7 @@ fn dispute_concluded_against_message_triggers_proper_reversion() { let (_, write_rx) = backend.await_next_write(); virtual_overseer .send(FromOrchestra::Communication { - msg: ChainSelectionMessage::RevertBlocks(Vec::from({(2, block_2_hash)})), + msg: ChainSelectionMessage::RevertBlocks(Vec::from([(2, block_2_hash)])), }) .await; @@ -2080,7 +2080,7 @@ fn dispute_concluded_against_message_triggers_proper_reversion() { } #[test] -fn dispute_reversion_against_finalized_is_ignored() { +fn revert_blocks_against_finalized_is_ignored() { test_harness(|backend, _, mut virtual_overseer| async move { // Building mini chain with 1 finalized block and 3 unfinalized blocks let finalized_number = 0; @@ -2103,7 +2103,7 @@ fn dispute_reversion_against_finalized_is_ignored() { // Sending dispute conculded against message virtual_overseer .send(FromOrchestra::Communication { - msg: ChainSelectionMessage::RevertBlocks(Vec::from({(finalized_number, finalized_hash)})), + msg: ChainSelectionMessage::RevertBlocks(Vec::from([(finalized_number, finalized_hash)])), }) .await; From d2454d863ca5c5f2373abfa8c936d4c9ab01f947 Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Thu, 12 Jan 2023 17:48:56 -0800 Subject: [PATCH 20/30] Polish and fmt --- node/core/chain-selection/src/lib.rs | 3 ++- node/core/chain-selection/src/tests.rs | 5 ++++- node/core/chain-selection/src/tree.rs | 1 + node/core/dispute-coordinator/src/initialized.rs | 4 ++-- node/core/dispute-coordinator/src/scraping/mod.rs | 2 +- 5 files changed, 10 insertions(+), 5 deletions(-) diff --git a/node/core/chain-selection/src/lib.rs b/node/core/chain-selection/src/lib.rs index 895c61f70d3b..af988978ca98 100644 --- a/node/core/chain-selection/src/lib.rs +++ b/node/core/chain-selection/src/lib.rs @@ -683,7 +683,8 @@ fn handle_approved_block(backend: &mut impl Backend, approved_block: Hash) -> Re } // Here we revert a provided group of blocks. The most common cause for this is that -// the dispute coordinator has notified chain selection +// the dispute coordinator has notified chain selection of a dispute which concluded +// against a candidate. fn handle_revert_blocks( backend: &impl Backend, blocks_to_revert: Vec<(BlockNumber, Hash)>, diff --git a/node/core/chain-selection/src/tests.rs b/node/core/chain-selection/src/tests.rs index c9f25d9347ad..5be64409391c 100644 --- a/node/core/chain-selection/src/tests.rs +++ b/node/core/chain-selection/src/tests.rs @@ -2103,7 +2103,10 @@ fn revert_blocks_against_finalized_is_ignored() { // Sending dispute conculded against message virtual_overseer .send(FromOrchestra::Communication { - msg: ChainSelectionMessage::RevertBlocks(Vec::from([(finalized_number, finalized_hash)])), + msg: ChainSelectionMessage::RevertBlocks(Vec::from([( + finalized_number, + finalized_hash, + )])), }) .await; diff --git a/node/core/chain-selection/src/tree.rs b/node/core/chain-selection/src/tree.rs index 5d374ac7b57a..05eeb4741ec5 100644 --- a/node/core/chain-selection/src/tree.rs +++ b/node/core/chain-selection/src/tree.rs @@ -176,6 +176,7 @@ fn propagate_viability_update( *viability_pivots.entry(new_entry.parent_hash).or_insert(0) += 1; } } + backend.write_block_entry(new_entry); tree_frontier diff --git a/node/core/dispute-coordinator/src/initialized.rs b/node/core/dispute-coordinator/src/initialized.rs index a4955d87add8..1375615c8d6a 100644 --- a/node/core/dispute-coordinator/src/initialized.rs +++ b/node/core/dispute-coordinator/src/initialized.rs @@ -1036,11 +1036,11 @@ impl Initialized { ctx.send_message(ChainSelectionMessage::RevertBlocks(blocks_to_revert.clone())) .await; } else { - gum::error!( + gum::trace!( target: LOG_TARGET, ?candidate_hash, ?session, - "Could not find parent block info for concluded candidate!" + "Could not find an including block for candidate against which a dispute has concluded." ); } } diff --git a/node/core/dispute-coordinator/src/scraping/mod.rs b/node/core/dispute-coordinator/src/scraping/mod.rs index 0e12f38aa2d5..1126c26bdc53 100644 --- a/node/core/dispute-coordinator/src/scraping/mod.rs +++ b/node/core/dispute-coordinator/src/scraping/mod.rs @@ -71,7 +71,7 @@ impl ScrapedUpdates { /// A structure meant to facilitate chain reversions in the event of a dispute /// concluding against a candidate. Each candidate hash maps to a vector of block -/// numbers and hashes for all blocks which included the candidate. The entries +/// number + hash pairs for all blocks which included the candidate. The entries /// in each vector are ordered by decreasing parent block number to facilitate /// minimal cost pruning. pub struct InclusionsPerCandidate { From 32b6f79cc2e6986ad9fda7a2108b9bae455207e6 Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Fri, 13 Jan 2023 09:01:44 -0800 Subject: [PATCH 21/30] Condensing inclusions per candidate logic --- .../dispute-coordinator/src/scraping/mod.rs | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/node/core/dispute-coordinator/src/scraping/mod.rs b/node/core/dispute-coordinator/src/scraping/mod.rs index 1126c26bdc53..e85b47c977e6 100644 --- a/node/core/dispute-coordinator/src/scraping/mod.rs +++ b/node/core/dispute-coordinator/src/scraping/mod.rs @@ -92,14 +92,8 @@ impl InclusionsPerCandidate { block_hash: Hash, ) { if let Some(blocks_including) = self.inclusions_inner.get_mut(&candidate_hash) { - for idx in 0..blocks_including.len() { - if blocks_including[idx].0 < block_number { - // Idx is between 0 and blocks_including.len(), therefore in bounds. QED - blocks_including.insert(idx, (block_number, block_hash)); - } else if idx == blocks_including.len() - 1 { - blocks_including.push((block_number, block_hash)); - } - } + let first_lower_entry = blocks_including.partition_point(|(number_at_idx, _)| *number_at_idx >= block_number); + blocks_including.insert(first_lower_entry, (block_number, block_hash)); } else { self.inclusions_inner .insert(candidate_hash, Vec::from([(block_number, block_hash)])); @@ -108,12 +102,8 @@ impl InclusionsPerCandidate { pub fn remove_up_to_height(&mut self, height: &BlockNumber) { for including_blocks in self.inclusions_inner.values_mut() { - while including_blocks.len() > 0 && - including_blocks[including_blocks.len() - 1].0 < *height - { - // Since parent_blocks length is positive, parent_blocks.len() - 1 is in bounds. QED - including_blocks.pop(); - } + let first_lower_entry = including_blocks.partition_point(|(number_at_idx, _)| *number_at_idx >= *height); + including_blocks.drain(first_lower_entry..); } self.inclusions_inner.retain(|_, including_blocks| including_blocks.len() > 0); } From 548e11bd1d5e4fe04daf3d8b38440ad38b8c1d7a Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Mon, 16 Jan 2023 11:14:10 -0800 Subject: [PATCH 22/30] Addressing Tsveto's comments --- node/core/chain-selection/src/tests.rs | 6 +- node/core/chain-selection/src/tree.rs | 92 +++++++++++----------- node/core/dispute-coordinator/src/tests.rs | 1 - 3 files changed, 47 insertions(+), 52 deletions(-) diff --git a/node/core/chain-selection/src/tests.rs b/node/core/chain-selection/src/tests.rs index 5be64409391c..25957d276a96 100644 --- a/node/core/chain-selection/src/tests.rs +++ b/node/core/chain-selection/src/tests.rs @@ -2041,7 +2041,7 @@ fn revert_blocks_message_triggers_proper_reversion() { let block_1_hash = backend.load_blocks_by_number(1).unwrap().get(0).unwrap().clone(); let block_2_hash = backend.load_blocks_by_number(2).unwrap().get(0).unwrap().clone(); - // Sending dispute conculded against message + // Sending dispute concluded against message let (_, write_rx) = backend.await_next_write(); virtual_overseer .send(FromOrchestra::Communication { @@ -2052,7 +2052,7 @@ fn revert_blocks_message_triggers_proper_reversion() { write_rx.await.unwrap(); // Checking results: - // Block 2 should be explicitely reverted + // Block 2 should be explicitly reverted assert_eq!( backend .load_block_entry(&block_2_hash) @@ -2100,7 +2100,7 @@ fn revert_blocks_against_finalized_is_ignored() { // Checking mini chain assert_backend_contains(&backend, built_chain.iter().map(|&(ref h, _)| h)); - // Sending dispute conculded against message + // Sending dispute concluded against message virtual_overseer .send(FromOrchestra::Communication { msg: ChainSelectionMessage::RevertBlocks(Vec::from([( diff --git a/node/core/chain-selection/src/tree.rs b/node/core/chain-selection/src/tree.rs index 05eeb4741ec5..09cc0563b582 100644 --- a/node/core/chain-selection/src/tree.rs +++ b/node/core/chain-selection/src/tree.rs @@ -176,7 +176,7 @@ fn propagate_viability_update( *viability_pivots.entry(new_entry.parent_hash).or_insert(0) += 1; } } - + backend.write_block_entry(new_entry); tree_frontier @@ -358,37 +358,15 @@ fn apply_ancestor_reversions( // Note: since revert numbers are in ascending order, the expensive propagation // of unviability is only heavy on the first log. for revert_number in reversions { - let mut ancestor_entry = - match load_ancestor(backend, block_hash, block_number, revert_number)? { - None => { - gum::warn!( - target: LOG_TARGET, - ?block_hash, - block_number, - revert_target = revert_number, - "The hammer has dropped. \ - A block has indicated that its finalized ancestor be reverted. \ - Please inform an adult.", - ); - - continue - }, - Some(ancestor_entry) => { - gum::info!( - target: LOG_TARGET, - ?block_hash, - block_number, - revert_target = revert_number, - revert_hash = ?ancestor_entry.block_hash, - "A block has signaled that its ancestor be reverted due to a bad parachain block.", - ); - - ancestor_entry - }, - }; - - ancestor_entry.viability.explicitly_reverted = true; - propagate_viability_update(backend, ancestor_entry)?; + let maybe_block_entry = load_ancestor(backend, block_hash, block_number, revert_number)?; + revert_single_block_entry_if_present( + backend, + maybe_block_entry, + None, + revert_number, + Some(block_hash), + Some(block_number), + )?; } Ok(()) @@ -402,36 +380,54 @@ pub(crate) fn apply_single_reversion( revert_hash: Hash, revert_number: BlockNumber, ) -> Result<(), Error> { - let mut entry_to_revert = match backend.load_block_entry(&revert_hash)? { + let maybe_block_entry = backend.load_block_entry(&revert_hash)?; + revert_single_block_entry_if_present( + backend, + maybe_block_entry, + Some(revert_hash), + revert_number, + None, + None, + )?; + Ok(()) +} + +fn revert_single_block_entry_if_present( + backend: &mut OverlayedBackend, + maybe_block_entry: Option, + maybe_revert_hash: Option, + revert_number: BlockNumber, + maybe_reporting_hash: Option, + maybe_reporting_number: Option, +) -> Result<(), Error> { + match maybe_block_entry { None => { gum::warn!( target: LOG_TARGET, - ?revert_hash, - // We keep parent block numbers in the disputes subsystem just - // for this log + ?maybe_revert_hash, revert_target = revert_number, + ?maybe_reporting_hash, + ?maybe_reporting_number, "The hammer has dropped. \ - A dispute has concluded against a finalized block. \ + The protocol has indicated that a finalized block be reverted. \ Please inform an adult.", ); - - return Ok(()) }, - Some(entry_to_revert) => { + Some(mut block_entry) => { gum::info!( target: LOG_TARGET, - ?revert_hash, + ?maybe_revert_hash, revert_target = revert_number, - "A dispute has concluded against a block, causing it to be reverted.", + ?maybe_reporting_hash, + ?maybe_reporting_number, + "The reversion of a block including an invalid parachain block candidate has been called for.", ); - entry_to_revert + block_entry.viability.explicitly_reverted = true; + // Marks children of reverted block as non-viable + propagate_viability_update(backend, block_entry)?; }, - }; - entry_to_revert.viability.explicitly_reverted = true; - // Marks children of reverted block as non-viable - propagate_viability_update(backend, entry_to_revert)?; - + } Ok(()) } diff --git a/node/core/dispute-coordinator/src/tests.rs b/node/core/dispute-coordinator/src/tests.rs index 47191093a654..aa423c5e4eaa 100644 --- a/node/core/dispute-coordinator/src/tests.rs +++ b/node/core/dispute-coordinator/src/tests.rs @@ -3394,7 +3394,6 @@ fn informs_chain_selection_when_dispute_concluded_against() { ) .await; - // Concluding dispute let supermajority_threshold = polkadot_primitives::v2::supermajority_threshold(test_state.validators.len()); From bd283f0d4db277f428670998370ae6a7726b7863 Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Tue, 17 Jan 2023 11:40:07 -0800 Subject: [PATCH 23/30] Addressing Robert's Comments --- node/core/chain-selection/src/tests.rs | 2 +- node/core/chain-selection/src/tree.rs | 2 +- node/core/dispute-coordinator/src/initialized.rs | 2 +- node/core/dispute-coordinator/src/scraping/mod.rs | 14 +++++++------- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/node/core/chain-selection/src/tests.rs b/node/core/chain-selection/src/tests.rs index 25957d276a96..d2b77a2f5d0b 100644 --- a/node/core/chain-selection/src/tests.rs +++ b/node/core/chain-selection/src/tests.rs @@ -2041,7 +2041,7 @@ fn revert_blocks_message_triggers_proper_reversion() { let block_1_hash = backend.load_blocks_by_number(1).unwrap().get(0).unwrap().clone(); let block_2_hash = backend.load_blocks_by_number(2).unwrap().get(0).unwrap().clone(); - // Sending dispute concluded against message + // Sending revert blocks message let (_, write_rx) = backend.await_next_write(); virtual_overseer .send(FromOrchestra::Communication { diff --git a/node/core/chain-selection/src/tree.rs b/node/core/chain-selection/src/tree.rs index 09cc0563b582..d71649e8b7ee 100644 --- a/node/core/chain-selection/src/tree.rs +++ b/node/core/chain-selection/src/tree.rs @@ -420,7 +420,7 @@ fn revert_single_block_entry_if_present( revert_target = revert_number, ?maybe_reporting_hash, ?maybe_reporting_number, - "The reversion of a block including an invalid parachain block candidate has been called for.", + "The reversion of an unfinalized block has been called for.", ); block_entry.viability.explicitly_reverted = true; diff --git a/node/core/dispute-coordinator/src/initialized.rs b/node/core/dispute-coordinator/src/initialized.rs index 1375615c8d6a..1e9ca6b66a0b 100644 --- a/node/core/dispute-coordinator/src/initialized.rs +++ b/node/core/dispute-coordinator/src/initialized.rs @@ -1036,7 +1036,7 @@ impl Initialized { ctx.send_message(ChainSelectionMessage::RevertBlocks(blocks_to_revert.clone())) .await; } else { - gum::trace!( + gum::debug!( target: LOG_TARGET, ?candidate_hash, ?session, diff --git a/node/core/dispute-coordinator/src/scraping/mod.rs b/node/core/dispute-coordinator/src/scraping/mod.rs index e85b47c977e6..db34c18a30f9 100644 --- a/node/core/dispute-coordinator/src/scraping/mod.rs +++ b/node/core/dispute-coordinator/src/scraping/mod.rs @@ -74,11 +74,11 @@ impl ScrapedUpdates { /// number + hash pairs for all blocks which included the candidate. The entries /// in each vector are ordered by decreasing parent block number to facilitate /// minimal cost pruning. -pub struct InclusionsPerCandidate { +pub struct Inclusions { inclusions_inner: HashMap>, } -impl InclusionsPerCandidate { +impl Inclusions { pub fn new() -> Self { Self { inclusions_inner: HashMap::new() } } @@ -146,7 +146,7 @@ pub struct ChainScraper { /// These correspond to all the relay blocks which marked a candidate as included, /// and are needed to apply reversions in case a dispute is concluded against the /// candidate. - inclusions_per_candidate: InclusionsPerCandidate, + inclusions: Inclusions, } impl ChainScraper { @@ -171,7 +171,7 @@ impl ChainScraper { included_candidates: candidates::ScrapedCandidates::new(), backed_candidates: candidates::ScrapedCandidates::new(), last_observed_blocks: LruCache::new(LRU_OBSERVED_BLOCKS_CAPACITY), - inclusions_per_candidate: InclusionsPerCandidate::new(), + inclusions: Inclusions::new(), }; let update = ActiveLeavesUpdate { activated: Some(initial_head), deactivated: Default::default() }; @@ -249,7 +249,7 @@ impl ChainScraper { Some(key_to_prune) => { self.backed_candidates.remove_up_to_height(&key_to_prune); self.included_candidates.remove_up_to_height(&key_to_prune); - self.inclusions_per_candidate.remove_up_to_height(&key_to_prune) + self.inclusions.remove_up_to_height(&key_to_prune) }, None => { // Nothing to prune. We are still in the beginning of the chain and there are not @@ -287,7 +287,7 @@ impl ChainScraper { "Processing included event" ); self.included_candidates.insert(block_number, candidate_hash); - self.inclusions_per_candidate.insert(candidate_hash, block_number, block_hash); + self.inclusions.insert(candidate_hash, block_number, block_hash); included_receipts.push(receipt); }, CandidateEvent::CandidateBacked(receipt, _, _, _) => { @@ -378,7 +378,7 @@ impl ChainScraper { &mut self, candidate: &CandidateHash, ) -> Option<&Vec<(BlockNumber, Hash)>> { - self.inclusions_per_candidate.get(candidate) + self.inclusions.get(candidate) } } From a257a1c522962256ff61e0bd7ba06ae62ac9253e Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Tue, 17 Jan 2023 12:52:10 -0800 Subject: [PATCH 24/30] Altered inclusions struct to use nested BTreeMaps --- .../dispute-coordinator/src/initialized.rs | 7 ++-- .../dispute-coordinator/src/scraping/mod.rs | 38 ++++++++++++------- .../dispute-coordinator/src/scraping/tests.rs | 14 +++---- 3 files changed, 35 insertions(+), 24 deletions(-) diff --git a/node/core/dispute-coordinator/src/initialized.rs b/node/core/dispute-coordinator/src/initialized.rs index 1e9ca6b66a0b..6f95a1e8defb 100644 --- a/node/core/dispute-coordinator/src/initialized.rs +++ b/node/core/dispute-coordinator/src/initialized.rs @@ -1030,10 +1030,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 let Some(blocks_to_revert) = - self.scraper.get_blocks_including_candidate(&candidate_hash) - { - ctx.send_message(ChainSelectionMessage::RevertBlocks(blocks_to_revert.clone())) + let blocks_including = self.scraper.get_blocks_including_candidate(&candidate_hash); + if blocks_including.len() > 0 { + ctx.send_message(ChainSelectionMessage::RevertBlocks(blocks_including)) .await; } else { gum::debug!( diff --git a/node/core/dispute-coordinator/src/scraping/mod.rs b/node/core/dispute-coordinator/src/scraping/mod.rs index db34c18a30f9..9650112788a9 100644 --- a/node/core/dispute-coordinator/src/scraping/mod.rs +++ b/node/core/dispute-coordinator/src/scraping/mod.rs @@ -14,7 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -use std::{collections::HashMap, num::NonZeroUsize}; +use std::{collections::BTreeMap, num::NonZeroUsize}; use futures::channel::oneshot; use lru::LruCache; @@ -75,12 +75,12 @@ impl ScrapedUpdates { /// in each vector are ordered by decreasing parent block number to facilitate /// minimal cost pruning. pub struct Inclusions { - inclusions_inner: HashMap>, + inclusions_inner: BTreeMap>>, } impl Inclusions { pub fn new() -> Self { - Self { inclusions_inner: HashMap::new() } + Self { inclusions_inner: BTreeMap::new() } } // Insert parent block into vector for the candidate hash it is including, @@ -92,24 +92,36 @@ impl Inclusions { block_hash: Hash, ) { if let Some(blocks_including) = self.inclusions_inner.get_mut(&candidate_hash) { - let first_lower_entry = blocks_including.partition_point(|(number_at_idx, _)| *number_at_idx >= block_number); - blocks_including.insert(first_lower_entry, (block_number, block_hash)); + if let Some(blocks_at_height) = blocks_including.get_mut(&block_number) { + blocks_at_height.push(block_hash); + } else { + blocks_including.insert(block_number, Vec::from([block_hash])); + } } else { + let mut blocks_including: BTreeMap> = BTreeMap::new(); + blocks_including.insert(block_number, Vec::from([block_hash])); self.inclusions_inner - .insert(candidate_hash, Vec::from([(block_number, block_hash)])); + .insert(candidate_hash, blocks_including); } } pub fn remove_up_to_height(&mut self, height: &BlockNumber) { - for including_blocks in self.inclusions_inner.values_mut() { - let first_lower_entry = including_blocks.partition_point(|(number_at_idx, _)| *number_at_idx >= *height); - including_blocks.drain(first_lower_entry..); + for blocks_including in self.inclusions_inner.values_mut() { + *blocks_including = blocks_including.split_off(height); } - self.inclusions_inner.retain(|_, including_blocks| including_blocks.len() > 0); + self.inclusions_inner.retain(|_, including_blocks| including_blocks.keys().len() > 0); } - pub fn get(&mut self, candidate: &CandidateHash) -> Option<&Vec<(BlockNumber, Hash)>> { - self.inclusions_inner.get(candidate) + pub fn get(&mut self, candidate: &CandidateHash) -> Vec<(BlockNumber, Hash)> { + let mut inclusions_as_vec: Vec<(BlockNumber, Hash)> = Vec::new(); + if let Some(blocks_including) = self.inclusions_inner.get(candidate) { + for (height, blocks_at_height) in blocks_including.iter() { + for block in blocks_at_height { + inclusions_as_vec.push((*height, *block)); + } + } + } + inclusions_as_vec } } @@ -377,7 +389,7 @@ impl ChainScraper { pub fn get_blocks_including_candidate( &mut self, candidate: &CandidateHash, - ) -> Option<&Vec<(BlockNumber, Hash)>> { + ) -> Vec<(BlockNumber, Hash)> { self.inclusions.get(candidate) } } diff --git a/node/core/dispute-coordinator/src/scraping/tests.rs b/node/core/dispute-coordinator/src/scraping/tests.rs index d4a6adb22f3f..86ade56203c4 100644 --- a/node/core/dispute-coordinator/src/scraping/tests.rs +++ b/node/core/dispute-coordinator/src/scraping/tests.rs @@ -620,10 +620,10 @@ fn inclusions_per_candidate_properly_adds_and_prunes() { // the candidate is included are recorded assert_eq!( scraper.get_blocks_including_candidate(&candidate.hash()), - Some(&Vec::from([ - (TEST_TARGET_BLOCK_NUMBER_2, get_block_number_hash(TEST_TARGET_BLOCK_NUMBER_2)), - (TEST_TARGET_BLOCK_NUMBER, get_block_number_hash(TEST_TARGET_BLOCK_NUMBER)) - ])) + Vec::from([ + (TEST_TARGET_BLOCK_NUMBER, get_block_number_hash(TEST_TARGET_BLOCK_NUMBER)), + (TEST_TARGET_BLOCK_NUMBER_2, get_block_number_hash(TEST_TARGET_BLOCK_NUMBER_2)) + ]) ); // After `DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION` blocks the earlier inclusion should be removed @@ -634,10 +634,10 @@ fn inclusions_per_candidate_properly_adds_and_prunes() { // The later inclusion should still be present, as we haven't exceeded its lifetime assert_eq!( scraper.get_blocks_including_candidate(&candidate.hash()), - Some(&Vec::from([( + Vec::from([( TEST_TARGET_BLOCK_NUMBER_2, get_block_number_hash(TEST_TARGET_BLOCK_NUMBER_2) - )])) + )]) ); finalized_block_number = @@ -645,6 +645,6 @@ fn inclusions_per_candidate_properly_adds_and_prunes() { process_finalized_block(&mut scraper, &finalized_block_number); // Now both inclusions have exceeded their lifetimes after finalization and should be purged - assert_eq!(scraper.get_blocks_including_candidate(&candidate.hash()), None); + assert!(scraper.get_blocks_including_candidate(&candidate.hash()).len() == 0); }); } From 7c1149b30858c8250bbb630307b5ac960533daa3 Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Tue, 17 Jan 2023 13:13:53 -0800 Subject: [PATCH 25/30] Naming fix --- node/core/dispute-coordinator/src/scraping/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/core/dispute-coordinator/src/scraping/mod.rs b/node/core/dispute-coordinator/src/scraping/mod.rs index 9650112788a9..2d1b89970ed4 100644 --- a/node/core/dispute-coordinator/src/scraping/mod.rs +++ b/node/core/dispute-coordinator/src/scraping/mod.rs @@ -109,7 +109,7 @@ impl Inclusions { for blocks_including in self.inclusions_inner.values_mut() { *blocks_including = blocks_including.split_off(height); } - self.inclusions_inner.retain(|_, including_blocks| including_blocks.keys().len() > 0); + self.inclusions_inner.retain(|_, blocks_including| blocks_including.keys().len() > 0); } pub fn get(&mut self, candidate: &CandidateHash) -> Vec<(BlockNumber, Hash)> { From a75cda3aebebf6b1c863c7fa31be4d5eda1f50ba Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Tue, 17 Jan 2023 13:19:33 -0800 Subject: [PATCH 26/30] Fixing inclusions struct comments --- node/core/dispute-coordinator/src/scraping/mod.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/node/core/dispute-coordinator/src/scraping/mod.rs b/node/core/dispute-coordinator/src/scraping/mod.rs index 2d1b89970ed4..f4e0fe33cade 100644 --- a/node/core/dispute-coordinator/src/scraping/mod.rs +++ b/node/core/dispute-coordinator/src/scraping/mod.rs @@ -70,10 +70,8 @@ impl ScrapedUpdates { } /// A structure meant to facilitate chain reversions in the event of a dispute -/// concluding against a candidate. Each candidate hash maps to a vector of block -/// number + hash pairs for all blocks which included the candidate. The entries -/// in each vector are ordered by decreasing parent block number to facilitate -/// minimal cost pruning. +/// concluding against a candidate. Each candidate hash maps to a number of +/// block heights, which in turn map to vectors of blocks at those heights. pub struct Inclusions { inclusions_inner: BTreeMap>>, } @@ -83,8 +81,8 @@ impl Inclusions { Self { inclusions_inner: BTreeMap::new() } } - // Insert parent block into vector for the candidate hash it is including, - // maintaining ordering by decreasing parent block number. + // Add parent block to the vector which has CandidateHash as an outer key and + // BlockNumber as an inner key pub fn insert( &mut self, candidate_hash: CandidateHash, From 992f2405a1a08053cca0f506bcbe39db56adcbe7 Mon Sep 17 00:00:00 2001 From: Bradley Olson <34992650+BradleyOlson64@users.noreply.github.com> Date: Tue, 17 Jan 2023 13:25:28 -0800 Subject: [PATCH 27/30] Update node/core/dispute-coordinator/src/scraping/mod.rs Add comment to split_off() use Co-authored-by: Marcin S. --- node/core/dispute-coordinator/src/scraping/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/node/core/dispute-coordinator/src/scraping/mod.rs b/node/core/dispute-coordinator/src/scraping/mod.rs index f4e0fe33cade..c9e75ce3420c 100644 --- a/node/core/dispute-coordinator/src/scraping/mod.rs +++ b/node/core/dispute-coordinator/src/scraping/mod.rs @@ -105,6 +105,7 @@ impl Inclusions { pub fn remove_up_to_height(&mut self, height: &BlockNumber) { for blocks_including in self.inclusions_inner.values_mut() { + // Returns everything after the given key, including the key. This works because the blocks are sorted in ascending order. *blocks_including = blocks_including.split_off(height); } self.inclusions_inner.retain(|_, blocks_including| blocks_including.keys().len() > 0); From 492c7ed84e85e96039cc37388d42ecfac747ab54 Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Wed, 18 Jan 2023 13:58:23 -0800 Subject: [PATCH 28/30] Optimizing removal at block height for inclusions --- node/core/chain-selection/src/tree.rs | 2 +- .../src/scraping/candidates.rs | 5 ++++- .../core/dispute-coordinator/src/scraping/mod.rs | 16 +++++++++------- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/node/core/chain-selection/src/tree.rs b/node/core/chain-selection/src/tree.rs index d71649e8b7ee..3db06eb4916b 100644 --- a/node/core/chain-selection/src/tree.rs +++ b/node/core/chain-selection/src/tree.rs @@ -420,7 +420,7 @@ fn revert_single_block_entry_if_present( revert_target = revert_number, ?maybe_reporting_hash, ?maybe_reporting_number, - "The reversion of an unfinalized block has been called for.", + "Unfinalized block reverted due to a bad parachain block.", ); block_entry.viability.explicitly_reverted = true; diff --git a/node/core/dispute-coordinator/src/scraping/candidates.rs b/node/core/dispute-coordinator/src/scraping/candidates.rs index 2fe797768cc2..3b7e20511501 100644 --- a/node/core/dispute-coordinator/src/scraping/candidates.rs +++ b/node/core/dispute-coordinator/src/scraping/candidates.rs @@ -102,15 +102,18 @@ impl ScrapedCandidates { } // Removes all candidates up to a given height. The candidates at the block height are NOT removed. - pub fn remove_up_to_height(&mut self, height: &BlockNumber) { + pub fn remove_up_to_height(&mut self, height: &BlockNumber) -> HashSet { + let mut candidates_modified: HashSet = HashSet::new(); let not_stale = self.candidates_by_block_number.split_off(&height); let stale = std::mem::take(&mut self.candidates_by_block_number); self.candidates_by_block_number = not_stale; for candidates in stale.values() { for c in candidates { self.candidates.remove(c); + candidates_modified.insert(c.clone()); } } + candidates_modified } pub fn insert(&mut self, block_number: BlockNumber, candidate_hash: CandidateHash) { diff --git a/node/core/dispute-coordinator/src/scraping/mod.rs b/node/core/dispute-coordinator/src/scraping/mod.rs index c9e75ce3420c..f4756d3f7fa0 100644 --- a/node/core/dispute-coordinator/src/scraping/mod.rs +++ b/node/core/dispute-coordinator/src/scraping/mod.rs @@ -14,7 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -use std::{collections::BTreeMap, num::NonZeroUsize}; +use std::{collections::BTreeMap, collections::HashSet, num::NonZeroUsize}; use futures::channel::oneshot; use lru::LruCache; @@ -103,10 +103,12 @@ impl Inclusions { } } - pub fn remove_up_to_height(&mut self, height: &BlockNumber) { - for blocks_including in self.inclusions_inner.values_mut() { - // Returns everything after the given key, including the key. This works because the blocks are sorted in ascending order. - *blocks_including = blocks_including.split_off(height); + pub fn remove_up_to_height(&mut self, height: &BlockNumber, candidates_modified: HashSet) { + for candidate in candidates_modified { + if let Some(blocks_including) = self.inclusions_inner.get_mut(&candidate){ + // Returns everything after the given key, including the key. This works because the blocks are sorted in ascending order. + *blocks_including = blocks_including.split_off(height); + } } self.inclusions_inner.retain(|_, blocks_including| blocks_including.keys().len() > 0); } @@ -259,8 +261,8 @@ impl ChainScraper { { Some(key_to_prune) => { self.backed_candidates.remove_up_to_height(&key_to_prune); - self.included_candidates.remove_up_to_height(&key_to_prune); - self.inclusions.remove_up_to_height(&key_to_prune) + let candidates_modified = self.included_candidates.remove_up_to_height(&key_to_prune); + self.inclusions.remove_up_to_height(&key_to_prune, candidates_modified); }, None => { // Nothing to prune. We are still in the beginning of the chain and there are not From f6779cdb757247c9556a7886ace722ce7a68efc4 Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Wed, 18 Jan 2023 14:00:46 -0800 Subject: [PATCH 29/30] fmt --- .../dispute-coordinator/src/initialized.rs | 3 +- .../dispute-coordinator/src/scraping/mod.rs | 28 ++++++++++++------- 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/node/core/dispute-coordinator/src/initialized.rs b/node/core/dispute-coordinator/src/initialized.rs index 6f95a1e8defb..703ff1030319 100644 --- a/node/core/dispute-coordinator/src/initialized.rs +++ b/node/core/dispute-coordinator/src/initialized.rs @@ -1032,8 +1032,7 @@ impl Initialized { if import_result.is_freshly_concluded_against() { let blocks_including = self.scraper.get_blocks_including_candidate(&candidate_hash); if blocks_including.len() > 0 { - ctx.send_message(ChainSelectionMessage::RevertBlocks(blocks_including)) - .await; + ctx.send_message(ChainSelectionMessage::RevertBlocks(blocks_including)).await; } else { gum::debug!( target: LOG_TARGET, diff --git a/node/core/dispute-coordinator/src/scraping/mod.rs b/node/core/dispute-coordinator/src/scraping/mod.rs index f4756d3f7fa0..3504fba842f5 100644 --- a/node/core/dispute-coordinator/src/scraping/mod.rs +++ b/node/core/dispute-coordinator/src/scraping/mod.rs @@ -14,7 +14,10 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -use std::{collections::BTreeMap, collections::HashSet, num::NonZeroUsize}; +use std::{ + collections::{BTreeMap, HashSet}, + num::NonZeroUsize, +}; use futures::channel::oneshot; use lru::LruCache; @@ -70,8 +73,8 @@ impl ScrapedUpdates { } /// A structure meant to facilitate chain reversions in the event of a dispute -/// concluding against a candidate. Each candidate hash maps to a number of -/// block heights, which in turn map to vectors of blocks at those heights. +/// concluding against a candidate. Each candidate hash maps to a number of +/// block heights, which in turn map to vectors of blocks at those heights. pub struct Inclusions { inclusions_inner: BTreeMap>>, } @@ -81,7 +84,7 @@ impl Inclusions { Self { inclusions_inner: BTreeMap::new() } } - // Add parent block to the vector which has CandidateHash as an outer key and + // Add parent block to the vector which has CandidateHash as an outer key and // BlockNumber as an inner key pub fn insert( &mut self, @@ -98,19 +101,23 @@ impl Inclusions { } else { let mut blocks_including: BTreeMap> = BTreeMap::new(); blocks_including.insert(block_number, Vec::from([block_hash])); - self.inclusions_inner - .insert(candidate_hash, blocks_including); + self.inclusions_inner.insert(candidate_hash, blocks_including); } } - pub fn remove_up_to_height(&mut self, height: &BlockNumber, candidates_modified: HashSet) { + pub fn remove_up_to_height( + &mut self, + height: &BlockNumber, + candidates_modified: HashSet, + ) { for candidate in candidates_modified { - if let Some(blocks_including) = self.inclusions_inner.get_mut(&candidate){ + if let Some(blocks_including) = self.inclusions_inner.get_mut(&candidate) { // Returns everything after the given key, including the key. This works because the blocks are sorted in ascending order. *blocks_including = blocks_including.split_off(height); } } - self.inclusions_inner.retain(|_, blocks_including| blocks_including.keys().len() > 0); + self.inclusions_inner + .retain(|_, blocks_including| blocks_including.keys().len() > 0); } pub fn get(&mut self, candidate: &CandidateHash) -> Vec<(BlockNumber, Hash)> { @@ -261,7 +268,8 @@ impl ChainScraper { { Some(key_to_prune) => { self.backed_candidates.remove_up_to_height(&key_to_prune); - let candidates_modified = self.included_candidates.remove_up_to_height(&key_to_prune); + let candidates_modified = + self.included_candidates.remove_up_to_height(&key_to_prune); self.inclusions.remove_up_to_height(&key_to_prune, candidates_modified); }, None => { From f9b92fa7a26133f20d9e22f7f8d039408d5e3aba Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Wed, 18 Jan 2023 16:55:12 -0800 Subject: [PATCH 30/30] Using copy trait --- node/core/dispute-coordinator/src/scraping/candidates.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/core/dispute-coordinator/src/scraping/candidates.rs b/node/core/dispute-coordinator/src/scraping/candidates.rs index 3b7e20511501..de54a4da905c 100644 --- a/node/core/dispute-coordinator/src/scraping/candidates.rs +++ b/node/core/dispute-coordinator/src/scraping/candidates.rs @@ -110,7 +110,7 @@ impl ScrapedCandidates { for candidates in stale.values() { for c in candidates { self.candidates.remove(c); - candidates_modified.insert(c.clone()); + candidates_modified.insert(*c); } } candidates_modified