From 20c810ab7ff616594c654bb7402a2d0a5467ae7f Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Fri, 8 Sep 2023 16:37:33 +0200 Subject: [PATCH 1/7] Fix `NonFinalizedTree::finality_checkpoints` and add a cache --- lib/src/chain/blocks_tree.rs | 13 ++++- lib/src/chain/blocks_tree/finality.rs | 75 ++++++++++++++++----------- lib/src/chain/blocks_tree/verify.rs | 16 ++++++ lib/src/chain/fork_tree.rs | 12 +++++ 4 files changed, 84 insertions(+), 32 deletions(-) diff --git a/lib/src/chain/blocks_tree.rs b/lib/src/chain/blocks_tree.rs index 3f389882f5..bd72f0d7a5 100644 --- a/lib/src/chain/blocks_tree.rs +++ b/lib/src/chain/blocks_tree.rs @@ -65,7 +65,12 @@ use crate::{ header, }; -use alloc::{collections::BTreeMap, format, sync::Arc, vec::Vec}; +use alloc::{ + collections::{BTreeMap, BTreeSet}, + format, + sync::Arc, + vec::Vec, +}; use core::{cmp, fmt, mem, num::NonZeroU64, ops, time::Duration}; use hashbrown::HashMap; @@ -136,6 +141,10 @@ pub struct NonFinalizedTree { /// Blocks indexed by the value in [`Block::best_score`]. The best block is the one with the /// highest score. blocks_by_best_score: BTreeMap, + /// Subset of [`NonFinalizedTree::blocks`] whose [`BlockFinality::Grandpa::triggers_change`] + /// is `true`, indexed by the value in + /// [`BlockFinality::Grandpa::prev_auth_change_trigger_number`]. + blocks_trigger_gp_change: BTreeSet<(Option, fork_tree::NodeIndex)>, /// See [`Config::block_number_bytes`]. block_number_bytes: usize, /// See [`Config::allow_unknown_consensus_engines`]. @@ -210,6 +219,7 @@ impl NonFinalizedTree { Default::default(), ), blocks_by_best_score: BTreeMap::new(), + blocks_trigger_gp_change: BTreeSet::new(), block_number_bytes: config.block_number_bytes, allow_unknown_consensus_engines: config.allow_unknown_consensus_engines, } @@ -220,6 +230,7 @@ impl NonFinalizedTree { self.blocks.clear(); self.blocks_by_hash.clear(); self.blocks_by_best_score.clear(); + self.blocks_trigger_gp_change.clear(); } /// Returns true if there isn't any non-finalized block in the chain. diff --git a/lib/src/chain/blocks_tree/finality.rs b/lib/src/chain/blocks_tree/finality.rs index 0946f3a065..b4c5e0d028 100644 --- a/lib/src/chain/blocks_tree/finality.rs +++ b/lib/src/chain/blocks_tree/finality.rs @@ -20,7 +20,7 @@ use super::*; use crate::finality::{grandpa, justification}; -use core::{cmp, iter}; +use core::cmp; impl NonFinalizedTree { /// Returns a list of blocks (by their height and hash) that need to be finalized before any @@ -31,38 +31,37 @@ impl NonFinalizedTree { /// [`NonFinalizedTree::verify_grandpa_commit_message`], unless they descend from any of the /// blocks returned by this function, in which case that block must be finalized beforehand. pub fn finality_checkpoints(&self) -> impl Iterator { - match &self.finality { - Finality::Outsourced => { - // No checkpoint means all blocks allowed. - either::Left(iter::empty()) - } - Finality::Grandpa { .. } => { - // TODO: O(n), could add a cache to make it O(1) - let iter = self + // Note that the code below assumes that GrandPa is the only finality algorithm currently + // supported. + debug_assert!( + self.blocks_trigger_gp_change.is_empty() + || !matches!(self.finality, Finality::Outsourced) + ); + + // TODO: add finalized block number as a cached field? + self.blocks_trigger_gp_change + .range(( + ops::Bound::Excluded(( + Some(self.finalized_block_header().number), + fork_tree::NodeIndex::max_value(), + )), + ops::Bound::Unbounded, + )) + .map(|(_prev_auth_change_trigger_number, block_index)| { + debug_assert!(_prev_auth_change_trigger_number.is_some()); + + let block = &self .blocks - .iter_unordered() - .filter(move |(_, block)| { - if let BlockFinality::Grandpa { - triggers_change, .. - } = &block.finality - { - *triggers_change - } else { - unreachable!() - } - }) - .map(|(_, block)| { - ( - header::decode(&block.header, self.block_number_bytes) - .unwrap() - .number, - &block.hash, - ) - }); + .get(*block_index) + .unwrap_or_else(|| unreachable!()); - either::Right(iter) - } - } + ( + header::decode(&block.header, self.block_number_bytes) + .unwrap() + .number, + &block.hash, + ) + }) } /// Verifies the given justification. @@ -434,10 +433,12 @@ impl NonFinalizedTree { debug_assert_eq!(self.blocks.len(), self.blocks_by_hash.len()); debug_assert_eq!(self.blocks.len(), self.blocks_by_best_score.len()); + debug_assert!(self.blocks.len() >= self.blocks_trigger_gp_change.len()); SetFinalizedBlockIter { iter: self.blocks.prune_ancestors(block_index_to_finalize), blocks_by_hash: &mut self.blocks_by_hash, blocks_by_best_score: &mut self.blocks_by_best_score, + blocks_trigger_gp_change: &mut self.blocks_trigger_gp_change, updates_best_block, } } @@ -575,6 +576,7 @@ pub struct SetFinalizedBlockIter<'a, T> { iter: fork_tree::PruneAncestorsIter<'a, Block>, blocks_by_hash: &'a mut HashMap<[u8; 32], fork_tree::NodeIndex, fnv::FnvBuildHasher>, blocks_by_best_score: &'a mut BTreeMap, + blocks_trigger_gp_change: &'a mut BTreeSet<(Option, fork_tree::NodeIndex)>, updates_best_block: bool, } @@ -597,6 +599,17 @@ impl<'a, T> Iterator for SetFinalizedBlockIter<'a, T> { .blocks_by_best_score .remove(&pruned.user_data.best_score); debug_assert_eq!(_removed, Some(pruned.index)); + if let BlockFinality::Grandpa { + prev_auth_change_trigger_number, + triggers_change: true, + .. + } = pruned.user_data.finality + { + let _removed = self + .blocks_trigger_gp_change + .remove(&(prev_auth_change_trigger_number, pruned.index)); + debug_assert!(_removed); + } break Some(RemovedBlock { block_hash: pruned.user_data.hash, scale_encoded_header: pruned.user_data.header, diff --git a/lib/src/chain/blocks_tree/verify.rs b/lib/src/chain/blocks_tree/verify.rs index 2231583315..787401adfe 100644 --- a/lib/src/chain/blocks_tree/verify.rs +++ b/lib/src/chain/blocks_tree/verify.rs @@ -498,6 +498,17 @@ impl NonFinalizedTree { insertion_counter: self.blocks_insertion_counter, }; + let prev_auth_change_trigger_number_if_trigger = if let BlockFinality::Grandpa { + prev_auth_change_trigger_number, + triggers_change: true, + .. + } = verified_header.finality_update + { + Some(prev_auth_change_trigger_number) + } else { + None + }; + let new_node_index = self.blocks.insert( parent_tree_index, Block { @@ -518,6 +529,11 @@ impl NonFinalizedTree { self.blocks_by_best_score.insert(best_score, new_node_index); + if let Some(prev_auth_change_trigger_number) = prev_auth_change_trigger_number_if_trigger { + self.blocks_trigger_gp_change + .insert((prev_auth_change_trigger_number, new_node_index)); + } + // An overflow here would break the logic of the module. It is better to panic than to // continue running. self.blocks_insertion_counter = self.blocks_insertion_counter.checked_add(1).unwrap(); diff --git a/lib/src/chain/fork_tree.rs b/lib/src/chain/fork_tree.rs index e74c15a6e1..9a7f08ce19 100644 --- a/lib/src/chain/fork_tree.rs +++ b/lib/src/chain/fork_tree.rs @@ -676,6 +676,18 @@ pub struct PrunedNode { #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)] pub struct NodeIndex(usize); +impl NodeIndex { + /// Returns the value that compares inferior or equal to any possible [`NodeIndex`̀]. + pub fn min_value() -> Self { + NodeIndex(usize::min_value()) + } + + /// Returns the value that compares superior or equal to any possible [`NodeIndex`̀]. + pub fn max_value() -> Self { + NodeIndex(usize::max_value()) + } +} + #[cfg(test)] mod tests { use super::ForkTree; From b2f197e12a19352e29daf41b85e0c4e42b1ff688 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Fri, 8 Sep 2023 16:39:31 +0200 Subject: [PATCH 2/7] CHANGELOG --- wasm-node/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/wasm-node/CHANGELOG.md b/wasm-node/CHANGELOG.md index 11af1b3af9..ccdbe8b778 100644 --- a/wasm-node/CHANGELOG.md +++ b/wasm-node/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Fixed + +- Justifications are no longer downloaded for blocks that can't be finalized because an earlier block needs to be finalized first. + ## 2.0.1 - 2023-09-08 ### Fixed From cfca614b2442e2faba41f9ea13be408a9c6d55c8 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Fri, 8 Sep 2023 16:45:24 +0200 Subject: [PATCH 3/7] Cache block numbers --- lib/src/chain/blocks_tree.rs | 13 ++++++----- lib/src/chain/blocks_tree/finality.rs | 32 +++++++-------------------- lib/src/chain/blocks_tree/verify.rs | 3 +++ 3 files changed, 19 insertions(+), 29 deletions(-) diff --git a/lib/src/chain/blocks_tree.rs b/lib/src/chain/blocks_tree.rs index bd72f0d7a5..01048d7af9 100644 --- a/lib/src/chain/blocks_tree.rs +++ b/lib/src/chain/blocks_tree.rs @@ -118,6 +118,8 @@ pub struct NonFinalizedTree { finalized_block_header: Vec, /// Hash of [`NonFinalizedTree::finalized_block_header`]. finalized_block_hash: [u8; 32], + /// Number of [`NonFinalizedTree::finalized_block_header`]. + finalized_block_number: u64, /// State of the chain finality engine. finality: Finality, /// State of the consensus of the finalized block. @@ -162,15 +164,14 @@ impl NonFinalizedTree { let chain_information: chain_information::ChainInformation = config.chain_information.into(); - let finalized_block_hash = chain_information - .finalized_block_header - .hash(config.block_number_bytes); - NonFinalizedTree { + finalized_block_number: chain_information.finalized_block_header.number, + finalized_block_hash: chain_information + .finalized_block_header + .hash(config.block_number_bytes), finalized_block_header: chain_information .finalized_block_header .scale_encoding_vec(config.block_number_bytes), - finalized_block_hash, finality: match chain_information.finality { chain_information::ChainInformationFinality::Outsourced => Finality::Outsourced, chain_information::ChainInformationFinality::Grandpa { @@ -571,6 +572,8 @@ struct Block { /// Cache of the hash of the block. Always equal to the hash of the header stored in this /// same struct. hash: [u8; 32], + /// Number contains in [`Block::header`]. + number: u64, /// Changes to the consensus made by the block. consensus: BlockConsensus, /// Information about finality attached to each block. diff --git a/lib/src/chain/blocks_tree/finality.rs b/lib/src/chain/blocks_tree/finality.rs index b4c5e0d028..eb5c47f962 100644 --- a/lib/src/chain/blocks_tree/finality.rs +++ b/lib/src/chain/blocks_tree/finality.rs @@ -38,29 +38,22 @@ impl NonFinalizedTree { || !matches!(self.finality, Finality::Outsourced) ); - // TODO: add finalized block number as a cached field? self.blocks_trigger_gp_change .range(( ops::Bound::Excluded(( - Some(self.finalized_block_header().number), + Some(self.finalized_block_number), fork_tree::NodeIndex::max_value(), )), ops::Bound::Unbounded, )) .map(|(_prev_auth_change_trigger_number, block_index)| { - debug_assert!(_prev_auth_change_trigger_number.is_some()); - - let block = &self + debug_assert!(_prev_auth_change_trigger_number + .map_or(false, |n| n > self.finalized_block_number)); + let block = self .blocks .get(*block_index) .unwrap_or_else(|| unreachable!()); - - ( - header::decode(&block.header, self.block_number_bytes) - .unwrap() - .number, - &block.hash, - ) + (block.number, &block.hash) }) } @@ -251,12 +244,7 @@ impl NonFinalizedTree { finalized_scheduled_change, finalized_triggered_authorities, } => { - let finalized_block_number = - header::decode(&self.finalized_block_header, self.block_number_bytes) - .unwrap() - .number; - - match target_number.cmp(&finalized_block_number) { + match target_number.cmp(&self.finalized_block_number) { cmp::Ordering::Equal if *target_hash == self.finalized_block_hash => { return Err(FinalityVerifyError::EqualToFinalized) } @@ -287,7 +275,7 @@ impl NonFinalizedTree { } = self.blocks.get(block_index).unwrap().finality { if let Some(prev_auth_change_trigger_number) = prev_auth_change_trigger_number { - if *prev_auth_change_trigger_number > finalized_block_number { + if *prev_auth_change_trigger_number > self.finalized_block_number { return Err(FinalityVerifyError::TooFarAhead { justification_block_number: target_number, justification_block_hash: *target_hash, @@ -353,11 +341,7 @@ impl NonFinalizedTree { ); debug_assert!(scheduled_change .as_ref() - .map(|(n, _)| *n - > header::decode(&new_finalized_block.header, self.block_number_bytes) - .unwrap() - .number) - .unwrap_or(true)); + .map_or(true, |(n, _)| *n > new_finalized_block.number)); *after_finalized_block_authorities_set_id = *after_block_authorities_set_id; *finalized_triggered_authorities = triggered_authorities.clone(); diff --git a/lib/src/chain/blocks_tree/verify.rs b/lib/src/chain/blocks_tree/verify.rs index 787401adfe..aaa178fa1c 100644 --- a/lib/src/chain/blocks_tree/verify.rs +++ b/lib/src/chain/blocks_tree/verify.rs @@ -455,6 +455,7 @@ impl NonFinalizedTree { Ok(HeaderVerifySuccess::Verified { verified_header: VerifiedHeader { + number: decoded_header.number, scale_encoded_header, consensus_update, finality_update, @@ -514,6 +515,7 @@ impl NonFinalizedTree { Block { header: verified_header.scale_encoded_header, hash: verified_header.hash, + number: verified_header.number, consensus: verified_header.consensus_update, finality: verified_header.finality_update, best_score, @@ -548,6 +550,7 @@ pub struct VerifiedHeader { best_score_num_primary_slots: u64, best_score_num_secondary_slots: u64, hash: [u8; 32], + number: u64, } impl VerifiedHeader { From b2aba11aa91f2f7c875a7b15e7f270094f121ed5 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Fri, 8 Sep 2023 16:48:29 +0200 Subject: [PATCH 4/7] Typo --- lib/src/chain/blocks_tree.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/src/chain/blocks_tree.rs b/lib/src/chain/blocks_tree.rs index 01048d7af9..1e92d93633 100644 --- a/lib/src/chain/blocks_tree.rs +++ b/lib/src/chain/blocks_tree.rs @@ -572,7 +572,7 @@ struct Block { /// Cache of the hash of the block. Always equal to the hash of the header stored in this /// same struct. hash: [u8; 32], - /// Number contains in [`Block::header`]. + /// Number contained in [`Block::header`]. number: u64, /// Changes to the consensus made by the block. consensus: BlockConsensus, From 4d66d4255de1a973663680e63fed682babaed217 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Fri, 8 Sep 2023 16:49:06 +0200 Subject: [PATCH 5/7] Bugfix and optimization --- lib/src/chain/blocks_tree/finality.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/src/chain/blocks_tree/finality.rs b/lib/src/chain/blocks_tree/finality.rs index eb5c47f962..c9f7e64bc8 100644 --- a/lib/src/chain/blocks_tree/finality.rs +++ b/lib/src/chain/blocks_tree/finality.rs @@ -411,8 +411,8 @@ impl NonFinalizedTree { &mut self.finalized_block_header, &mut new_finalized_block.header, ); - self.finalized_block_hash = - header::hash_from_scale_encoded_header(&self.finalized_block_header); + self.finalized_block_hash = new_finalized_block.hash; + self.finalized_block_number = new_finalized_block.number; self.finalized_best_score = new_finalized_block.best_score; debug_assert_eq!(self.blocks.len(), self.blocks_by_hash.len()); From 0520c59cc7a385ba773a7ad377d42b0cf5461221 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Fri, 8 Sep 2023 16:49:24 +0200 Subject: [PATCH 6/7] PR link --- wasm-node/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wasm-node/CHANGELOG.md b/wasm-node/CHANGELOG.md index ccdbe8b778..622c98f0c4 100644 --- a/wasm-node/CHANGELOG.md +++ b/wasm-node/CHANGELOG.md @@ -4,7 +4,7 @@ ### Fixed -- Justifications are no longer downloaded for blocks that can't be finalized because an earlier block needs to be finalized first. +- Justifications are no longer downloaded for blocks that can't be finalized because an earlier block needs to be finalized first. ([#1127](https://github.com/smol-dot/smoldot/pull/1127)) ## 2.0.1 - 2023-09-08 From 6de58b231046c7d1f82f7c56b7ab63bfc70ef580 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Fri, 8 Sep 2023 16:50:35 +0200 Subject: [PATCH 7/7] Comment update --- lib/src/chain/blocks_tree/finality.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/src/chain/blocks_tree/finality.rs b/lib/src/chain/blocks_tree/finality.rs index c9f7e64bc8..f06aecaa47 100644 --- a/lib/src/chain/blocks_tree/finality.rs +++ b/lib/src/chain/blocks_tree/finality.rs @@ -405,8 +405,8 @@ impl NonFinalizedTree { _ => unreachable!(), } - // Update `self.finalized_block_header`, `self.finalized_block_hash`, and - // `self.finalized_best_score`. + // Update `self.finalized_block_header`, `self.finalized_block_hash`, + // `self.finalized_block_number`, and `self.finalized_best_score`. mem::swap( &mut self.finalized_block_header, &mut new_finalized_block.header,