From 1525edea8cd5bb6f47df6940481d54f80cc1871d Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 6 Apr 2023 11:06:07 +0300 Subject: [PATCH 1/3] Onchain scraper in `dispute-coordinator` will scrape `SCRAPED_FINALIZED_BLOCKS_COUNT` blocks before finality The purpose is to make the availability of a `CandidateReceipt` for finalized candidates more likely. For details see: /~https://github.com/paritytech/polkadot/issues/7009 --- .../dispute-coordinator/src/scraping/mod.rs | 26 ++++++++++++++----- .../dispute-coordinator/src/scraping/tests.rs | 4 ++- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/node/core/dispute-coordinator/src/scraping/mod.rs b/node/core/dispute-coordinator/src/scraping/mod.rs index 0e3c4ecc29ba..20849a9d4d78 100644 --- a/node/core/dispute-coordinator/src/scraping/mod.rs +++ b/node/core/dispute-coordinator/src/scraping/mod.rs @@ -16,7 +16,7 @@ use std::{ collections::{BTreeMap, HashSet}, - num::NonZeroUsize, + num::{NonZeroU32, NonZeroUsize}, }; use futures::channel::oneshot; @@ -177,6 +177,17 @@ impl ChainScraper { /// As long as we have `MAX_FINALITY_LAG` this makes sense as a value. pub(crate) const ANCESTRY_SIZE_LIMIT: u32 = MAX_FINALITY_LAG; + /// How many finalized block the scraper should process. + /// + /// Initialize this value with `DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION` + /// If `SCRAPED_FINALIZED_BLOCKS_COUNT` > `DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION` + /// scraper's job will be undone on pruning. + pub(crate) const SCRAPED_FINALIZED_BLOCKS_COUNT: NonZeroU32 = + match NonZeroU32::new(DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION) { + Some(cap) => cap, + None => panic!("Scraped finalized blocks count must be non-zero"), + }; + /// Create a properly initialized `OrderingProvider`. /// /// Returns: `Self` and any scraped votes. @@ -230,7 +241,7 @@ impl ChainScraper { // Fetch ancestry up to last finalized block. let ancestors = self - .get_unfinalized_block_ancestors(sender, activated.hash, activated.number) + .get_relevant_block_ancestors(sender, activated.hash, activated.number) .await?; // Ancestors block numbers are consecutive in the descending order. @@ -330,10 +341,11 @@ impl ChainScraper { } /// Returns ancestors of `head` in the descending order, stopping - /// either at the block present in cache or at the last finalized block. + /// either at the block present in cache or at `SCRAPED_FINALIZED_BLOCKS_COUNT -1` blocks after + /// the last finalized one (called `target_ancestor`). /// - /// Both `head` and the latest finalized block are **not** included in the result. - async fn get_unfinalized_block_ancestors( + /// Both `head` and the `target_ancestor` blocks are **not** included in the result. + async fn get_relevant_block_ancestors( &mut self, sender: &mut Sender, mut head: Hash, @@ -342,7 +354,9 @@ impl ChainScraper { where Sender: overseer::DisputeCoordinatorSenderTrait, { - let target_ancestor = get_finalized_block_number(sender).await?; + let target_ancestor = get_finalized_block_number(sender) + .await? + .saturating_sub(Self::SCRAPED_FINALIZED_BLOCKS_COUNT.get() - 1); let mut ancestors = Vec::new(); diff --git a/node/core/dispute-coordinator/src/scraping/tests.rs b/node/core/dispute-coordinator/src/scraping/tests.rs index b7183739d8f8..8347ce46919b 100644 --- a/node/core/dispute-coordinator/src/scraping/tests.rs +++ b/node/core/dispute-coordinator/src/scraping/tests.rs @@ -410,7 +410,9 @@ fn scraper_requests_candidates_of_non_finalized_ancestors() { &mut virtual_overseer, &chain, finalized_block_number, - BLOCKS_TO_SKIP - finalized_block_number as usize, // Expect the provider not to go past finalized block. + BLOCKS_TO_SKIP - + (finalized_block_number - ChainScraper::SCRAPED_FINALIZED_BLOCKS_COUNT.get() + 1) + as usize, // Expect the provider not to go past finalized block. get_backed_and_included_candidate_events, ); join(process_active_leaves_update(ctx.sender(), &mut ordering, next_update), overseer_fut) From 162ee29e1006f22176394db65d579c1bd6779bc8 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 7 Apr 2023 12:17:23 +0300 Subject: [PATCH 2/3] Fix off by one error --- node/core/dispute-coordinator/src/scraping/mod.rs | 5 +++-- node/core/dispute-coordinator/src/scraping/tests.rs | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/node/core/dispute-coordinator/src/scraping/mod.rs b/node/core/dispute-coordinator/src/scraping/mod.rs index 20849a9d4d78..a510a9d360a8 100644 --- a/node/core/dispute-coordinator/src/scraping/mod.rs +++ b/node/core/dispute-coordinator/src/scraping/mod.rs @@ -239,7 +239,8 @@ impl ChainScraper { None => return Ok(ScrapedUpdates::new()), }; - // Fetch ancestry up to last finalized block. + // Fetch ancestry up to `SCRAPED_FINALIZED_BLOCKS_COUNT` blocks beyond + // the last finalized one let ancestors = self .get_relevant_block_ancestors(sender, activated.hash, activated.number) .await?; @@ -356,7 +357,7 @@ impl ChainScraper { { let target_ancestor = get_finalized_block_number(sender) .await? - .saturating_sub(Self::SCRAPED_FINALIZED_BLOCKS_COUNT.get() - 1); + .saturating_sub(Self::SCRAPED_FINALIZED_BLOCKS_COUNT.get()); let mut ancestors = Vec::new(); diff --git a/node/core/dispute-coordinator/src/scraping/tests.rs b/node/core/dispute-coordinator/src/scraping/tests.rs index 8347ce46919b..dc00b62e512e 100644 --- a/node/core/dispute-coordinator/src/scraping/tests.rs +++ b/node/core/dispute-coordinator/src/scraping/tests.rs @@ -411,7 +411,7 @@ fn scraper_requests_candidates_of_non_finalized_ancestors() { &chain, finalized_block_number, BLOCKS_TO_SKIP - - (finalized_block_number - ChainScraper::SCRAPED_FINALIZED_BLOCKS_COUNT.get() + 1) + (finalized_block_number - ChainScraper::SCRAPED_FINALIZED_BLOCKS_COUNT.get()) as usize, // Expect the provider not to go past finalized block. get_backed_and_included_candidate_events, ); From e997f5e54502f534062448c9488239609cc10d01 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 7 Apr 2023 22:04:33 +0300 Subject: [PATCH 3/3] Replace `SCRAPED_FINALIZED_BLOCKS_COUNT` with `DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION` --- .../dispute-coordinator/src/scraping/mod.rs | 20 ++++++------------- .../dispute-coordinator/src/scraping/tests.rs | 3 +-- node/primitives/src/lib.rs | 8 +++++++- 3 files changed, 14 insertions(+), 17 deletions(-) diff --git a/node/core/dispute-coordinator/src/scraping/mod.rs b/node/core/dispute-coordinator/src/scraping/mod.rs index a510a9d360a8..554f79bd0b84 100644 --- a/node/core/dispute-coordinator/src/scraping/mod.rs +++ b/node/core/dispute-coordinator/src/scraping/mod.rs @@ -16,7 +16,7 @@ use std::{ collections::{BTreeMap, HashSet}, - num::{NonZeroU32, NonZeroUsize}, + num::NonZeroUsize, }; use futures::channel::oneshot; @@ -151,7 +151,10 @@ impl Inclusions { /// `process_active_leaves_update` any scraped votes. /// /// Scraped candidates are available `DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION` more blocks -/// after finalization as a precaution not to prune them prematurely. +/// after finalization as a precaution not to prune them prematurely. Besides the newly scraped +/// candidates `DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION` finalized blocks are parsed as +/// another precaution to have their `CandidateReceipts` available in case a dispute is raised on +/// them, pub struct ChainScraper { /// All candidates we have seen included, which not yet have been finalized. included_candidates: candidates::ScrapedCandidates, @@ -177,17 +180,6 @@ impl ChainScraper { /// As long as we have `MAX_FINALITY_LAG` this makes sense as a value. pub(crate) const ANCESTRY_SIZE_LIMIT: u32 = MAX_FINALITY_LAG; - /// How many finalized block the scraper should process. - /// - /// Initialize this value with `DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION` - /// If `SCRAPED_FINALIZED_BLOCKS_COUNT` > `DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION` - /// scraper's job will be undone on pruning. - pub(crate) const SCRAPED_FINALIZED_BLOCKS_COUNT: NonZeroU32 = - match NonZeroU32::new(DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION) { - Some(cap) => cap, - None => panic!("Scraped finalized blocks count must be non-zero"), - }; - /// Create a properly initialized `OrderingProvider`. /// /// Returns: `Self` and any scraped votes. @@ -357,7 +349,7 @@ impl ChainScraper { { let target_ancestor = get_finalized_block_number(sender) .await? - .saturating_sub(Self::SCRAPED_FINALIZED_BLOCKS_COUNT.get()); + .saturating_sub(DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION); let mut ancestors = Vec::new(); diff --git a/node/core/dispute-coordinator/src/scraping/tests.rs b/node/core/dispute-coordinator/src/scraping/tests.rs index dc00b62e512e..da76a93b7353 100644 --- a/node/core/dispute-coordinator/src/scraping/tests.rs +++ b/node/core/dispute-coordinator/src/scraping/tests.rs @@ -411,8 +411,7 @@ fn scraper_requests_candidates_of_non_finalized_ancestors() { &chain, finalized_block_number, BLOCKS_TO_SKIP - - (finalized_block_number - ChainScraper::SCRAPED_FINALIZED_BLOCKS_COUNT.get()) - as usize, // Expect the provider not to go past finalized block. + (finalized_block_number - DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION) as usize, // Expect the provider not to go past finalized block. get_backed_and_included_candidate_events, ); join(process_active_leaves_update(ctx.sender(), &mut ordering, next_update), overseer_fut) diff --git a/node/primitives/src/lib.rs b/node/primitives/src/lib.rs index 18b7fa18a0c8..c113769563ad 100644 --- a/node/primitives/src/lib.rs +++ b/node/primitives/src/lib.rs @@ -65,7 +65,7 @@ pub const VALIDATION_CODE_BOMB_LIMIT: usize = (MAX_CODE_SIZE * 4u32) as usize; pub const POV_BOMB_LIMIT: usize = (MAX_POV_SIZE * 4u32) as usize; /// How many blocks after finalization an information about backed/included candidate should be -/// kept. +/// pre-loaded (when scraoing onchain votes) and kept locally (when pruning). /// /// We don't want to remove scraped candidates on finalization because we want to /// be sure that disputes will conclude on abandoned forks. @@ -74,6 +74,12 @@ pub const POV_BOMB_LIMIT: usize = (MAX_POV_SIZE * 4u32) as usize; /// better one gets finalized the entries for the bad fork will be pruned and we /// might never participate in a dispute for it. /// +/// Why pre-load finalized blocks? I dispute might be raised against finalized candidate. In most +/// of the cases it will conclude valid (otherwise we are in big trouble) but never the less the +/// node must participate. It's possible to see a vote for such dispute onchain before we have it +/// imported by `dispute-distribution`. In this case we won't have `CandidateReceipt` and the import +/// will fail unless we keep them preloaded. +/// /// This value should consider the timeout we allow for participation in approval-voting. In /// particular, the following condition should hold: ///