From 7258b9307573afe5635ff4b0a874932802a0a013 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Thu, 6 Jun 2024 16:55:18 +0200 Subject: [PATCH 1/4] Skip tree route calculation if leaves.len() == 1. --- substrate/client/db/src/lib.rs | 107 +++++++++++++++++- .../primitives/blockchain/src/backend.rs | 36 +++++- .../blockchain/src/header_metadata.rs | 23 +++- 3 files changed, 159 insertions(+), 7 deletions(-) diff --git a/substrate/client/db/src/lib.rs b/substrate/client/db/src/lib.rs index 36f9aea817c9c..67ce2e77d11d7 100644 --- a/substrate/client/db/src/lib.rs +++ b/substrate/client/db/src/lib.rs @@ -2547,7 +2547,7 @@ pub(crate) mod tests { backend::{Backend as BTrait, BlockImportOperation as Op}, blockchain::Backend as BLBTrait, }; - use sp_blockchain::{lowest_common_ancestor, tree_route}; + use sp_blockchain::{lowest_common_ancestor, lowest_common_ancestor_multiblock, tree_route}; use sp_core::H256; use sp_runtime::{ testing::{Block as RawBlock, ExtrinsicWrapper, Header}, @@ -3108,6 +3108,111 @@ pub(crate) mod tests { } } + #[test] + fn lowest_common_ancestors_multiblock_works() { + let backend = Backend::::new_test(1000, 100); + let blockchain = backend.blockchain(); + let block0 = insert_header(&backend, 0, Default::default(), None, Default::default()); + + // fork from genesis: 3 prong. + let a1 = insert_header(&backend, 1, block0, None, Default::default()); + let a2 = insert_header(&backend, 2, a1, None, Default::default()); + let a3 = insert_header(&backend, 3, a2, None, Default::default()); + + // fork from genesis: 2 prong. + let b1 = insert_header(&backend, 1, block0, None, H256::from([1; 32])); + let b2 = insert_header(&backend, 2, b1, None, Default::default()); + + // fork from b2. + let c1 = insert_header(&backend, 3, b2, None, H256::from([2; 32])); + let c2 = insert_header(&backend, 4, c1, None, Default::default()); + + // fork from b1. + let d1 = insert_header(&backend, 2, b1, None, H256::from([3; 32])); + let d2 = insert_header(&backend, 3, d1, None, Default::default()); + { + let lca = lowest_common_ancestor_multiblock(blockchain, vec![a3, b2]).unwrap().unwrap(); + + assert_eq!(lca.hash, block0); + assert_eq!(lca.number, 0); + } + + { + let lca = lowest_common_ancestor_multiblock(blockchain, vec![a1, a3]).unwrap().unwrap(); + + assert_eq!(lca.hash, a1); + assert_eq!(lca.number, 1); + } + + { + let lca = lowest_common_ancestor_multiblock(blockchain, vec![a3, a1]).unwrap().unwrap(); + + assert_eq!(lca.hash, a1); + assert_eq!(lca.number, 1); + } + + { + let lca = lowest_common_ancestor_multiblock(blockchain, vec![a2, a3]).unwrap().unwrap(); + + assert_eq!(lca.hash, a2); + assert_eq!(lca.number, 2); + } + + { + let lca = lowest_common_ancestor_multiblock(blockchain, vec![a2, a1]).unwrap().unwrap(); + + assert_eq!(lca.hash, a1); + assert_eq!(lca.number, 1); + } + + { + let lca = lowest_common_ancestor_multiblock(blockchain, vec![a2, a2]).unwrap().unwrap(); + + assert_eq!(lca.hash, a2); + assert_eq!(lca.number, 2); + } + + { + let lca = lowest_common_ancestor_multiblock(blockchain, vec![a3, d2, c2]) + .unwrap() + .unwrap(); + + assert_eq!(lca.hash, block0); + assert_eq!(lca.number, 0); + } + + { + let lca = lowest_common_ancestor_multiblock(blockchain, vec![c2, d2, b2]) + .unwrap() + .unwrap(); + + assert_eq!(lca.hash, b1); + assert_eq!(lca.number, 1); + } + + { + let lca = lowest_common_ancestor_multiblock(blockchain, vec![a1, a2, a3]) + .unwrap() + .unwrap(); + + assert_eq!(lca.hash, a1); + assert_eq!(lca.number, 1); + } + + { + let lca = lowest_common_ancestor_multiblock(blockchain, vec![]); + + assert_eq!(true, matches!(lca, Ok(None))); + } + + { + let lca = lowest_common_ancestor_multiblock(blockchain, vec![a1]).unwrap().unwrap(); + + assert_eq!(lca.hash, a1); + assert_eq!(lca.number, 1); + } + } + #[test] fn test_tree_route_regression() { // NOTE: this is a test for a regression introduced in #3665, the result diff --git a/substrate/primitives/blockchain/src/backend.rs b/substrate/primitives/blockchain/src/backend.rs index 06e5b682964a4..93b51c85637db 100644 --- a/substrate/primitives/blockchain/src/backend.rs +++ b/substrate/primitives/blockchain/src/backend.rs @@ -21,16 +21,15 @@ use log::warn; use parking_lot::RwLock; use sp_runtime::{ generic::BlockId, - traits::{Block as BlockT, Header as HeaderT, NumberFor, Zero}, + traits::{Block as BlockT, CheckedSub, Header as HeaderT, NumberFor, Zero}, Justifications, }; use std::collections::{btree_map::BTreeMap, btree_set::BTreeSet}; -use crate::header_metadata::HeaderMetadata; - use crate::{ error::{Error, Result}, - tree_route, TreeRoute, + header_metadata::{self, HeaderMetadata}, + lowest_common_ancestor_multiblock, tree_route, TreeRoute, }; /// Blockchain database header backend. Does not perform any validation. @@ -229,10 +228,37 @@ pub trait Backend: ) -> std::result::Result, Error> { let mut result = DisplacedLeavesAfterFinalization::default(); - if finalized_block_number == Zero::zero() { + let leaves = self.leaves()?; + + // If we have only one leaf there are no forks, and we can return early. + if finalized_block_number == Zero::zero() || leaves.len() == 1 { return Ok(result) } + let first_leaf = leaves[0]; + let leaf_block_header = self.expect_header(first_leaf)?; + + // If the distance between the leafs and the finalized block is large, calculating + // tree routes can be very expensive. In that case, we will try to find the + // lowest common ancestor between all the leaves. The assumption here is that the forks are + // close to the tip and not long. So the LCA can be computed from the header cache. If the + // LCA is above the finalized block, we know that there are no displaced leaves by the + // finalization. + if leaf_block_header + .number() + .checked_sub(&finalized_block_number) + .unwrap_or(0u32.into()) > + header_metadata::LRU_CACHE_SIZE.into() + { + if let Some(lca) = lowest_common_ancestor_multiblock(self, leaves)? { + if lca.number > finalized_block_number { + return Ok(result) + } else { + log::warn!("The distance between leafs and finalized block is large. Finalization can take a long time."); + } + }; + } + // For each leaf determine whether it belongs to a non-canonical branch. for leaf_hash in self.leaves()? { let leaf_block_header = self.expect_header(leaf_hash)?; diff --git a/substrate/primitives/blockchain/src/header_metadata.rs b/substrate/primitives/blockchain/src/header_metadata.rs index 27caaae71add1..3c47703ffb8f2 100644 --- a/substrate/primitives/blockchain/src/header_metadata.rs +++ b/substrate/primitives/blockchain/src/header_metadata.rs @@ -23,7 +23,7 @@ use schnellru::{ByLength, LruMap}; use sp_runtime::traits::{Block as BlockT, Header, NumberFor, One}; /// Set to the expected max difference between `best` and `finalized` blocks at sync. -const LRU_CACHE_SIZE: u32 = 5_000; +pub(crate) const LRU_CACHE_SIZE: u32 = 5_000; /// Get lowest common ancestor between two blocks in the tree. /// @@ -96,6 +96,27 @@ pub fn lowest_common_ancestor + ?Sized>( Ok(HashAndNumber { hash: header_one.hash, number: header_one.number }) } +/// Get lowest common ancestor between multiple blocks. +pub fn lowest_common_ancestor_multiblock + ?Sized>( + backend: &T, + hashes: Vec, +) -> Result>, T::Error> { + // Ensure the list of hashes is not empty + if hashes.is_empty() { + return Ok(None); + } + + // Start with the first hash as the initial LCA + let cached = backend.header_metadata(hashes[0])?; + let mut lca = HashAndNumber { number: cached.number, hash: cached.hash }; + for hash in hashes.iter().skip(1) { + // Calculate the LCA of the current LCA and the next hash + lca = lowest_common_ancestor(backend, lca.hash, *hash)?; + } + + Ok(Some(lca)) +} + /// Compute a tree-route between two blocks. See tree-route docs for more details. pub fn tree_route + ?Sized>( backend: &T, From 6a270a510b8371f521eaa5499c617d6490354ab5 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Mon, 10 Jun 2024 18:57:09 +0200 Subject: [PATCH 2/4] Reviewer comments --- substrate/client/db/src/lib.rs | 14 ++++++++++++++ substrate/primitives/blockchain/src/backend.rs | 10 ++++++---- .../blockchain/src/header_metadata.rs | 17 ++++++++++------- 3 files changed, 30 insertions(+), 11 deletions(-) diff --git a/substrate/client/db/src/lib.rs b/substrate/client/db/src/lib.rs index 67ce2e77d11d7..8d8b7a2aff88f 100644 --- a/substrate/client/db/src/lib.rs +++ b/substrate/client/db/src/lib.rs @@ -3115,6 +3115,11 @@ pub(crate) mod tests { let block0 = insert_header(&backend, 0, Default::default(), None, Default::default()); // fork from genesis: 3 prong. + // block 0 -> a1 -> a2 -> a3 + // | + // -> b1 -> b2 -> c1 -> c2 + // | + // -> d1 -> d2 let a1 = insert_header(&backend, 1, block0, None, Default::default()); let a2 = insert_header(&backend, 2, a1, None, Default::default()); let a3 = insert_header(&backend, 3, a2, None, Default::default()); @@ -3199,6 +3204,15 @@ pub(crate) mod tests { assert_eq!(lca.number, 1); } + { + let lca = lowest_common_ancestor_multiblock(blockchain, vec![b1, b2, d1]) + .unwrap() + .unwrap(); + + assert_eq!(lca.hash, b1); + assert_eq!(lca.number, 1); + } + { let lca = lowest_common_ancestor_multiblock(blockchain, vec![]); diff --git a/substrate/primitives/blockchain/src/backend.rs b/substrate/primitives/blockchain/src/backend.rs index 93b51c85637db..992b196d89bc1 100644 --- a/substrate/primitives/blockchain/src/backend.rs +++ b/substrate/primitives/blockchain/src/backend.rs @@ -235,8 +235,10 @@ pub trait Backend: return Ok(result) } - let first_leaf = leaves[0]; - let leaf_block_header = self.expect_header(first_leaf)?; + let first_leaf = leaves.first().ok_or(Error::Backend( + "Unable to find any leaves. This should not happen.".to_string(), + ))?; + let leaf_block_header = self.expect_header(*first_leaf)?; // If the distance between the leafs and the finalized block is large, calculating // tree routes can be very expensive. In that case, we will try to find the @@ -250,7 +252,7 @@ pub trait Backend: .unwrap_or(0u32.into()) > header_metadata::LRU_CACHE_SIZE.into() { - if let Some(lca) = lowest_common_ancestor_multiblock(self, leaves)? { + if let Some(lca) = lowest_common_ancestor_multiblock(self, leaves.clone())? { if lca.number > finalized_block_number { return Ok(result) } else { @@ -260,7 +262,7 @@ pub trait Backend: } // For each leaf determine whether it belongs to a non-canonical branch. - for leaf_hash in self.leaves()? { + for leaf_hash in leaves { let leaf_block_header = self.expect_header(leaf_hash)?; let leaf_number = *leaf_block_header.number(); diff --git a/substrate/primitives/blockchain/src/header_metadata.rs b/substrate/primitives/blockchain/src/header_metadata.rs index 3c47703ffb8f2..c2054445b0676 100644 --- a/substrate/primitives/blockchain/src/header_metadata.rs +++ b/substrate/primitives/blockchain/src/header_metadata.rs @@ -102,16 +102,19 @@ pub fn lowest_common_ancestor_multiblock hashes: Vec, ) -> Result>, T::Error> { // Ensure the list of hashes is not empty - if hashes.is_empty() { - return Ok(None); - } + let mut hashes_iter = hashes.into_iter(); + + let first_hash = match hashes_iter.next() { + Some(hash) => hash, + None => return Ok(None), + }; // Start with the first hash as the initial LCA - let cached = backend.header_metadata(hashes[0])?; - let mut lca = HashAndNumber { number: cached.number, hash: cached.hash }; - for hash in hashes.iter().skip(1) { + let first_cached = backend.header_metadata(first_hash)?; + let mut lca = HashAndNumber { number: first_cached.number, hash: first_cached.hash }; + for hash in hashes_iter { // Calculate the LCA of the current LCA and the next hash - lca = lowest_common_ancestor(backend, lca.hash, *hash)?; + lca = lowest_common_ancestor(backend, lca.hash, hash)?; } Ok(Some(lca)) From b0584b5548434cb23a8e07c5ae1b0362cfc1f3b3 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Mon, 10 Jun 2024 20:37:40 +0200 Subject: [PATCH 3/4] Add prdoc --- prdoc/pr_4721.prdoc | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 prdoc/pr_4721.prdoc diff --git a/prdoc/pr_4721.prdoc b/prdoc/pr_4721.prdoc new file mode 100644 index 0000000000000..7bcb38c82595d --- /dev/null +++ b/prdoc/pr_4721.prdoc @@ -0,0 +1,17 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Skip tree route calculation if no forks present + +doc: + - audience: Node Operator + description: | + Fixes an issue with synchronisation on parachains. Once they reached the tip of the chain, + nodes would show `Preparing 0.0 bps`. This is shown because the node is blocked on calculating + the tree route from genesis to the tip of the chain many times. This PR solves that by skipping + tree route calculation if there is only one leave. In addition, further optimizations have been + done to alleviate long finalization distances. + +crates: + - name: sp-blockchain + bump: minor From b3cca4a64beb3a2f78e1bdd83bd2597992eab3d4 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Tue, 11 Jun 2024 14:33:36 +0200 Subject: [PATCH 4/4] Update prdoc --- prdoc/pr_4721.prdoc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/prdoc/pr_4721.prdoc b/prdoc/pr_4721.prdoc index 7bcb38c82595d..730ac4d830869 100644 --- a/prdoc/pr_4721.prdoc +++ b/prdoc/pr_4721.prdoc @@ -15,3 +15,5 @@ doc: crates: - name: sp-blockchain bump: minor + - name: sc-client-db + bump: none