From 283bc9cbbad4fcfb3cfcb9e0e3bac46ced38e8d1 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Sat, 11 Feb 2023 18:35:04 +0100 Subject: [PATCH] Fix longest chain finalization target lookup (#13289) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Finalization target should be chosed as some ancestor of SelectChain::best_chain * More test assertions * Improve docs * Removed stale docs * Rename 'target' to 'base' in lookup method * Fix typo * Apply suggestions from code review Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> * Rename 'target_hash' to 'base_hash' in 'SelectChain::finality_target()' * Apply suggestions from code review Co-authored-by: Anton * Docs improvement * Doc fix * Apply suggestions from code review Co-authored-by: Bastian Köcher * Apply more code suggestions --------- Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> Co-authored-by: Anton Co-authored-by: Bastian Köcher --- client/consensus/common/src/longest_chain.rs | 80 ++++++- client/finality-grandpa/src/tests.rs | 2 +- client/service/test/src/client/mod.rs | 217 ++++++++++++------ primitives/blockchain/src/backend.rs | 84 ++----- .../consensus/common/src/select_chain.rs | 8 +- 5 files changed, 242 insertions(+), 149 deletions(-) diff --git a/client/consensus/common/src/longest_chain.rs b/client/consensus/common/src/longest_chain.rs index fab2c3a4c06fd..ece2486209ff3 100644 --- a/client/consensus/common/src/longest_chain.rs +++ b/client/consensus/common/src/longest_chain.rs @@ -21,7 +21,7 @@ use sc_client_api::backend; use sp_blockchain::{Backend, HeaderBackend}; use sp_consensus::{Error as ConsensusError, SelectChain}; -use sp_runtime::traits::{Block as BlockT, NumberFor}; +use sp_runtime::traits::{Block as BlockT, Header, NumberFor}; use std::{marker::PhantomData, sync::Arc}; /// Implement Longest Chain Select implementation @@ -48,15 +48,19 @@ where LongestChain { backend, _phantom: Default::default() } } - fn best_block_header(&self) -> sp_blockchain::Result<::Header> { + fn best_hash(&self) -> sp_blockchain::Result<::Hash> { let info = self.backend.blockchain().info(); let import_lock = self.backend.get_import_lock(); let best_hash = self .backend .blockchain() - .best_containing(info.best_hash, None, import_lock)? + .longest_containing(info.best_hash, import_lock)? .unwrap_or(info.best_hash); + Ok(best_hash) + } + fn best_header(&self) -> sp_blockchain::Result<::Header> { + let best_hash = self.best_hash()?; Ok(self .backend .blockchain() @@ -64,6 +68,65 @@ where .expect("given block hash was fetched from block in db; qed")) } + /// Returns the highest descendant of the given block that is a valid + /// candidate to be finalized. + /// + /// In this context, being a valid target means being an ancestor of + /// the best chain according to the `best_header` method. + /// + /// If `maybe_max_number` is `Some(max_block_number)` the search is + /// limited to block `number <= max_block_number`. In other words + /// as if there were no blocks greater than `max_block_number`. + fn finality_target( + &self, + base_hash: Block::Hash, + maybe_max_number: Option>, + ) -> sp_blockchain::Result { + use sp_blockchain::Error::{Application, MissingHeader}; + let blockchain = self.backend.blockchain(); + + let mut current_head = self.best_header()?; + let mut best_hash = current_head.hash(); + + let base_header = blockchain + .header(base_hash)? + .ok_or_else(|| MissingHeader(base_hash.to_string()))?; + let base_number = *base_header.number(); + + if let Some(max_number) = maybe_max_number { + if max_number < base_number { + let msg = format!( + "Requested a finality target using max number {} below the base number {}", + max_number, base_number + ); + return Err(Application(msg.into())) + } + + while current_head.number() > &max_number { + best_hash = *current_head.parent_hash(); + current_head = blockchain + .header(best_hash)? + .ok_or_else(|| MissingHeader(format!("{best_hash:?}")))?; + } + } + + while current_head.hash() != base_hash { + if *current_head.number() < base_number { + let msg = format!( + "Requested a finality target using a base {:?} not in the best chain {:?}", + base_hash, best_hash, + ); + return Err(Application(msg.into())) + } + let current_hash = *current_head.parent_hash(); + current_head = blockchain + .header(current_hash)? + .ok_or_else(|| MissingHeader(format!("{best_hash:?}")))?; + } + + Ok(best_hash) + } + fn leaves(&self) -> Result::Hash>, sp_blockchain::Error> { self.backend.blockchain().leaves() } @@ -80,20 +143,15 @@ where } async fn best_chain(&self) -> Result<::Header, ConsensusError> { - LongestChain::best_block_header(self) - .map_err(|e| ConsensusError::ChainLookup(e.to_string())) + LongestChain::best_header(self).map_err(|e| ConsensusError::ChainLookup(e.to_string())) } async fn finality_target( &self, - target_hash: Block::Hash, + base_hash: Block::Hash, maybe_max_number: Option>, ) -> Result { - let import_lock = self.backend.get_import_lock(); - self.backend - .blockchain() - .best_containing(target_hash, maybe_max_number, import_lock) - .map(|maybe_hash| maybe_hash.unwrap_or(target_hash)) + LongestChain::finality_target(self, base_hash, maybe_max_number) .map_err(|e| ConsensusError::ChainLookup(e.to_string())) } } diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 05561ad856119..d132abd940ba3 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -237,7 +237,7 @@ impl SelectChain for MockSelectChain { async fn finality_target( &self, - _target_hash: Hash, + _base_hash: Hash, _maybe_max_number: Option>, ) -> Result { Ok(self.finality_target.lock().take().unwrap()) diff --git a/client/service/test/src/client/mod.rs b/client/service/test/src/client/mod.rs index 97c22a1cb509e..12b92afc458b4 100644 --- a/client/service/test/src/client/mod.rs +++ b/client/service/test/src/client/mod.rs @@ -581,7 +581,7 @@ fn uncles_with_multiple_forks() { } #[test] -fn best_containing_on_longest_chain_with_single_chain_3_blocks() { +fn finality_target_on_longest_chain_with_single_chain_3_blocks() { // block tree: // G -> A1 -> A2 @@ -606,7 +606,7 @@ fn best_containing_on_longest_chain_with_single_chain_3_blocks() { } #[test] -fn best_containing_on_longest_chain_with_multiple_forks() { +fn finality_target_on_longest_chain_with_multiple_forks() { // block tree: // G -> A1 -> A2 -> A3 -> A4 -> A5 // A1 -> B2 -> B3 -> B4 @@ -731,8 +731,13 @@ fn best_containing_on_longest_chain_with_multiple_forks() { assert!(leaves.contains(&d2.hash())); assert_eq!(leaves.len(), 4); - let finality_target = |target_hash, number| { - block_on(longest_chain_select.finality_target(target_hash, number)).unwrap() + // On error we return a quite improbable hash + let error_hash = Hash::from([0xff; 32]); + let finality_target = |target_hash, number| match block_on( + longest_chain_select.finality_target(target_hash, number), + ) { + Ok(hash) => hash, + Err(_) => error_hash, }; // search without restriction @@ -742,11 +747,11 @@ fn best_containing_on_longest_chain_with_multiple_forks() { assert_eq!(a5.hash(), finality_target(a3.hash(), None)); assert_eq!(a5.hash(), finality_target(a4.hash(), None)); assert_eq!(a5.hash(), finality_target(a5.hash(), None)); - assert_eq!(b4.hash(), finality_target(b2.hash(), None)); - assert_eq!(b4.hash(), finality_target(b3.hash(), None)); - assert_eq!(b4.hash(), finality_target(b4.hash(), None)); - assert_eq!(c3.hash(), finality_target(c3.hash(), None)); - assert_eq!(d2.hash(), finality_target(d2.hash(), None)); + assert_eq!(error_hash, finality_target(b2.hash(), None)); + assert_eq!(error_hash, finality_target(b3.hash(), None)); + assert_eq!(error_hash, finality_target(b4.hash(), None)); + assert_eq!(error_hash, finality_target(c3.hash(), None)); + assert_eq!(error_hash, finality_target(d2.hash(), None)); // search only blocks with number <= 5. equivalent to without restriction for this scenario assert_eq!(a5.hash(), finality_target(genesis_hash, Some(5))); @@ -755,11 +760,11 @@ fn best_containing_on_longest_chain_with_multiple_forks() { assert_eq!(a5.hash(), finality_target(a3.hash(), Some(5))); assert_eq!(a5.hash(), finality_target(a4.hash(), Some(5))); assert_eq!(a5.hash(), finality_target(a5.hash(), Some(5))); - assert_eq!(b4.hash(), finality_target(b2.hash(), Some(5))); - assert_eq!(b4.hash(), finality_target(b3.hash(), Some(5))); - assert_eq!(b4.hash(), finality_target(b4.hash(), Some(5))); - assert_eq!(c3.hash(), finality_target(c3.hash(), Some(5))); - assert_eq!(d2.hash(), finality_target(d2.hash(), Some(5))); + assert_eq!(error_hash, finality_target(b2.hash(), Some(5))); + assert_eq!(error_hash, finality_target(b3.hash(), Some(5))); + assert_eq!(error_hash, finality_target(b4.hash(), Some(5))); + assert_eq!(error_hash, finality_target(c3.hash(), Some(5))); + assert_eq!(error_hash, finality_target(d2.hash(), Some(5))); // search only blocks with number <= 4 assert_eq!(a4.hash(), finality_target(genesis_hash, Some(4))); @@ -767,73 +772,72 @@ fn best_containing_on_longest_chain_with_multiple_forks() { assert_eq!(a4.hash(), finality_target(a2.hash(), Some(4))); assert_eq!(a4.hash(), finality_target(a3.hash(), Some(4))); assert_eq!(a4.hash(), finality_target(a4.hash(), Some(4))); - assert_eq!(a5.hash(), finality_target(a5.hash(), Some(4))); - assert_eq!(b4.hash(), finality_target(b2.hash(), Some(4))); - assert_eq!(b4.hash(), finality_target(b3.hash(), Some(4))); - assert_eq!(b4.hash(), finality_target(b4.hash(), Some(4))); - assert_eq!(c3.hash(), finality_target(c3.hash(), Some(4))); - assert_eq!(d2.hash(), finality_target(d2.hash(), Some(4))); + assert_eq!(error_hash, finality_target(a5.hash(), Some(4))); + assert_eq!(error_hash, finality_target(b2.hash(), Some(4))); + assert_eq!(error_hash, finality_target(b3.hash(), Some(4))); + assert_eq!(error_hash, finality_target(b4.hash(), Some(4))); + assert_eq!(error_hash, finality_target(c3.hash(), Some(4))); + assert_eq!(error_hash, finality_target(d2.hash(), Some(4))); // search only blocks with number <= 3 assert_eq!(a3.hash(), finality_target(genesis_hash, Some(3))); assert_eq!(a3.hash(), finality_target(a1.hash(), Some(3))); assert_eq!(a3.hash(), finality_target(a2.hash(), Some(3))); assert_eq!(a3.hash(), finality_target(a3.hash(), Some(3))); - assert_eq!(a4.hash(), finality_target(a4.hash(), Some(3))); - assert_eq!(a5.hash(), finality_target(a5.hash(), Some(3))); - assert_eq!(b3.hash(), finality_target(b2.hash(), Some(3))); - assert_eq!(b3.hash(), finality_target(b3.hash(), Some(3))); - assert_eq!(b4.hash(), finality_target(b4.hash(), Some(3))); - assert_eq!(c3.hash(), finality_target(c3.hash(), Some(3))); - assert_eq!(d2.hash(), finality_target(d2.hash(), Some(3))); + assert_eq!(error_hash, finality_target(a4.hash(), Some(3))); + assert_eq!(error_hash, finality_target(a5.hash(), Some(3))); + assert_eq!(error_hash, finality_target(b2.hash(), Some(3))); + assert_eq!(error_hash, finality_target(b3.hash(), Some(3))); + assert_eq!(error_hash, finality_target(b4.hash(), Some(3))); + assert_eq!(error_hash, finality_target(c3.hash(), Some(3))); + assert_eq!(error_hash, finality_target(d2.hash(), Some(3))); // search only blocks with number <= 2 assert_eq!(a2.hash(), finality_target(genesis_hash, Some(2))); assert_eq!(a2.hash(), finality_target(a1.hash(), Some(2))); assert_eq!(a2.hash(), finality_target(a2.hash(), Some(2))); - assert_eq!(a3.hash(), finality_target(a3.hash(), Some(2))); - assert_eq!(a4.hash(), finality_target(a4.hash(), Some(2))); - assert_eq!(a5.hash(), finality_target(a5.hash(), Some(2))); - assert_eq!(b2.hash(), finality_target(b2.hash(), Some(2))); - assert_eq!(b3.hash(), finality_target(b3.hash(), Some(2))); - assert_eq!(b4.hash(), finality_target(b4.hash(), Some(2))); - assert_eq!(c3.hash(), finality_target(c3.hash(), Some(2))); - assert_eq!(d2.hash(), finality_target(d2.hash(), Some(2))); + assert_eq!(error_hash, finality_target(a3.hash(), Some(2))); + assert_eq!(error_hash, finality_target(a4.hash(), Some(2))); + assert_eq!(error_hash, finality_target(a5.hash(), Some(2))); + assert_eq!(error_hash, finality_target(b2.hash(), Some(2))); + assert_eq!(error_hash, finality_target(b3.hash(), Some(2))); + assert_eq!(error_hash, finality_target(b4.hash(), Some(2))); + assert_eq!(error_hash, finality_target(c3.hash(), Some(2))); + assert_eq!(error_hash, finality_target(d2.hash(), Some(2))); // search only blocks with number <= 1 assert_eq!(a1.hash(), finality_target(genesis_hash, Some(1))); assert_eq!(a1.hash(), finality_target(a1.hash(), Some(1))); - assert_eq!(a2.hash(), finality_target(a2.hash(), Some(1))); - assert_eq!(a3.hash(), finality_target(a3.hash(), Some(1))); - assert_eq!(a4.hash(), finality_target(a4.hash(), Some(1))); - assert_eq!(a5.hash(), finality_target(a5.hash(), Some(1))); - - assert_eq!(b2.hash(), finality_target(b2.hash(), Some(1))); - assert_eq!(b3.hash(), finality_target(b3.hash(), Some(1))); - assert_eq!(b4.hash(), finality_target(b4.hash(), Some(1))); - assert_eq!(c3.hash(), finality_target(c3.hash(), Some(1))); - assert_eq!(d2.hash(), finality_target(d2.hash(), Some(1))); + assert_eq!(error_hash, finality_target(a2.hash(), Some(1))); + assert_eq!(error_hash, finality_target(a3.hash(), Some(1))); + assert_eq!(error_hash, finality_target(a4.hash(), Some(1))); + assert_eq!(error_hash, finality_target(a5.hash(), Some(1))); + assert_eq!(error_hash, finality_target(b2.hash(), Some(1))); + assert_eq!(error_hash, finality_target(b3.hash(), Some(1))); + assert_eq!(error_hash, finality_target(b4.hash(), Some(1))); + assert_eq!(error_hash, finality_target(c3.hash(), Some(1))); + assert_eq!(error_hash, finality_target(d2.hash(), Some(1))); // search only blocks with number <= 0 assert_eq!(genesis_hash, finality_target(genesis_hash, Some(0))); - assert_eq!(a1.hash(), finality_target(a1.hash(), Some(0))); - assert_eq!(a2.hash(), finality_target(a2.hash(), Some(0))); - assert_eq!(a3.hash(), finality_target(a3.hash(), Some(0))); - assert_eq!(a4.hash(), finality_target(a4.hash(), Some(0))); - assert_eq!(a5.hash(), finality_target(a5.hash(), Some(0))); - assert_eq!(b2.hash(), finality_target(b2.hash(), Some(0))); - assert_eq!(b3.hash(), finality_target(b3.hash(), Some(0))); - assert_eq!(b4.hash(), finality_target(b4.hash(), Some(0))); - assert_eq!(c3.hash(), finality_target(c3.hash(), Some(0))); - assert_eq!(d2.hash(), finality_target(d2.hash(), Some(0))); + assert_eq!(error_hash, finality_target(a1.hash(), Some(0))); + assert_eq!(error_hash, finality_target(a2.hash(), Some(0))); + assert_eq!(error_hash, finality_target(a3.hash(), Some(0))); + assert_eq!(error_hash, finality_target(a4.hash(), Some(0))); + assert_eq!(error_hash, finality_target(a5.hash(), Some(0))); + assert_eq!(error_hash, finality_target(b2.hash(), Some(0))); + assert_eq!(error_hash, finality_target(b3.hash(), Some(0))); + assert_eq!(error_hash, finality_target(b4.hash(), Some(0))); + assert_eq!(error_hash, finality_target(c3.hash(), Some(0))); + assert_eq!(error_hash, finality_target(d2.hash(), Some(0))); } #[test] -fn best_containing_on_longest_chain_with_max_depth_higher_than_best() { +fn finality_target_on_longest_chain_with_max_depth_higher_than_best() { // block tree: // G -> A1 -> A2 - let (mut client, longest_chain_select) = TestClientBuilder::new().build_with_longest_chain(); + let (mut client, chain_select) = TestClientBuilder::new().build_with_longest_chain(); // G -> A1 let a1 = client.new_block(Default::default()).unwrap().build().unwrap().block; @@ -845,10 +849,93 @@ fn best_containing_on_longest_chain_with_max_depth_higher_than_best() { let genesis_hash = client.chain_info().genesis_hash; - assert_eq!( - a2.hash(), - block_on(longest_chain_select.finality_target(genesis_hash, Some(10))).unwrap(), - ); + assert_eq!(a2.hash(), block_on(chain_select.finality_target(genesis_hash, Some(10))).unwrap(),); +} + +#[test] +fn finality_target_with_best_not_on_longest_chain() { + // block tree: + // G -> A1 -> A2 -> A3 -> A4 -> A5 + // -> B2 -> (B3) -> B4 + // ^best + + let (mut client, chain_select) = TestClientBuilder::new().build_with_longest_chain(); + let genesis_hash = client.chain_info().genesis_hash; + + // G -> A1 + let a1 = client.new_block(Default::default()).unwrap().build().unwrap().block; + block_on(client.import(BlockOrigin::Own, a1.clone())).unwrap(); + + // A1 -> A2 + let a2 = client.new_block(Default::default()).unwrap().build().unwrap().block; + block_on(client.import(BlockOrigin::Own, a2.clone())).unwrap(); + + // A2 -> A3 + let a3 = client.new_block(Default::default()).unwrap().build().unwrap().block; + block_on(client.import(BlockOrigin::Own, a3.clone())).unwrap(); + + // A3 -> A4 + let a4 = client.new_block(Default::default()).unwrap().build().unwrap().block; + block_on(client.import(BlockOrigin::Own, a4.clone())).unwrap(); + + // A3 -> A5 + let a5 = client.new_block(Default::default()).unwrap().build().unwrap().block; + block_on(client.import(BlockOrigin::Own, a5.clone())).unwrap(); + + // A1 -> B2 + let mut builder = client + .new_block_at(&BlockId::Hash(a1.hash()), Default::default(), false) + .unwrap(); + // this push is required as otherwise B2 has the same hash as A2 and won't get imported + builder + .push_transfer(Transfer { + from: AccountKeyring::Alice.into(), + to: AccountKeyring::Ferdie.into(), + amount: 41, + nonce: 0, + }) + .unwrap(); + let b2 = builder.build().unwrap().block; + block_on(client.import(BlockOrigin::Own, b2.clone())).unwrap(); + + assert_eq!(a5.hash(), block_on(chain_select.finality_target(genesis_hash, None)).unwrap()); + assert_eq!(a5.hash(), block_on(chain_select.finality_target(a1.hash(), None)).unwrap()); + assert_eq!(a5.hash(), block_on(chain_select.finality_target(a2.hash(), None)).unwrap()); + assert_eq!(a5.hash(), block_on(chain_select.finality_target(a3.hash(), None)).unwrap()); + assert_eq!(a5.hash(), block_on(chain_select.finality_target(a4.hash(), None)).unwrap()); + assert_eq!(a5.hash(), block_on(chain_select.finality_target(a5.hash(), None)).unwrap()); + + // B2 -> B3 + let b3 = client + .new_block_at(&BlockId::Hash(b2.hash()), Default::default(), false) + .unwrap() + .build() + .unwrap() + .block; + block_on(client.import_as_best(BlockOrigin::Own, b3.clone())).unwrap(); + + // B3 -> B4 + let b4 = client + .new_block_at(&BlockId::Hash(b3.hash()), Default::default(), false) + .unwrap() + .build() + .unwrap() + .block; + let (header, extrinsics) = b4.clone().deconstruct(); + let mut import_params = BlockImportParams::new(BlockOrigin::Own, header); + import_params.body = Some(extrinsics); + import_params.fork_choice = Some(ForkChoiceStrategy::Custom(false)); + block_on(client.import_block(import_params, Default::default())).unwrap(); + + // double check that B3 is still the best... + assert_eq!(client.info().best_hash, b3.hash()); + + assert_eq!(b4.hash(), block_on(chain_select.finality_target(genesis_hash, None)).unwrap()); + assert_eq!(b4.hash(), block_on(chain_select.finality_target(a1.hash(), None)).unwrap()); + assert!(block_on(chain_select.finality_target(a2.hash(), None)).is_err()); + assert_eq!(b4.hash(), block_on(chain_select.finality_target(b2.hash(), None)).unwrap()); + assert_eq!(b4.hash(), block_on(chain_select.finality_target(b3.hash(), None)).unwrap()); + assert_eq!(b4.hash(), block_on(chain_select.finality_target(b4.hash(), None)).unwrap()); } #[test] @@ -995,7 +1082,7 @@ fn finalizing_diverged_block_should_trigger_reorg() { .block; block_on(client.import(BlockOrigin::Own, b2.clone())).unwrap(); - // A2 is the current best since it's the longest chain + // A2 is the current best since it's the (first) longest chain assert_eq!(client.chain_info().best_hash, a2.hash()); // we finalize block B1 which is on a different branch from current best @@ -1012,8 +1099,7 @@ fn finalizing_diverged_block_should_trigger_reorg() { // `SelectChain` should report B2 as best block though assert_eq!(block_on(select_chain.best_chain()).unwrap().hash(), b2.hash()); - // after we build B3 on top of B2 and import it - // it should be the new best block, + // after we build B3 on top of B2 and import it, it should be the new best block let b3 = client .new_block_at(&BlockId::Hash(b2.hash()), Default::default(), false) .unwrap() @@ -1022,6 +1108,9 @@ fn finalizing_diverged_block_should_trigger_reorg() { .block; block_on(client.import(BlockOrigin::Own, b3.clone())).unwrap(); + // `SelectChain` should report B3 as best block though + assert_eq!(block_on(select_chain.best_chain()).unwrap().hash(), b3.hash()); + assert_eq!(client.chain_info().best_hash, b3.hash()); ClientExt::finalize_block(&client, b3.hash(), None).unwrap(); diff --git a/primitives/blockchain/src/backend.rs b/primitives/blockchain/src/backend.rs index ec9c8ac0d5780..7339d4c1a6804 100644 --- a/primitives/blockchain/src/backend.rs +++ b/primitives/blockchain/src/backend.rs @@ -183,96 +183,43 @@ pub trait Backend: /// Return hashes of all blocks that are children of the block with `parent_hash`. fn children(&self, parent_hash: Block::Hash) -> Result>; - /// Get the most recent block hash of the best (longest) chains - /// that contain block with the given `target_hash`. + /// Get the most recent block hash of the longest chain that contains + /// a block with the given `base_hash`. /// /// The search space is always limited to blocks which are in the finalized /// chain or descendents of it. /// - /// If `maybe_max_block_number` is `Some(max_block_number)` - /// the search is limited to block `numbers <= max_block_number`. - /// in other words as if there were no blocks greater `max_block_number`. - /// Returns `Ok(None)` if `target_hash` is not found in search space. - /// TODO: document time complexity of this, see [#1444](https://github.com/paritytech/substrate/issues/1444) - fn best_containing( + /// Returns `Ok(None)` if `base_hash` is not found in search space. + // TODO: document time complexity of this, see [#1444](https://github.com/paritytech/substrate/issues/1444) + fn longest_containing( &self, - target_hash: Block::Hash, - maybe_max_number: Option>, + base_hash: Block::Hash, import_lock: &RwLock<()>, ) -> Result> { - let target_header = { - match self.header(target_hash)? { - Some(x) => x, - // target not in blockchain - None => return Ok(None), - } + let Some(base_header) = self.header(base_hash)? else { + return Ok(None) }; - if let Some(max_number) = maybe_max_number { - // target outside search range - if target_header.number() > &max_number { - return Ok(None) - } - } - let leaves = { // ensure no blocks are imported during this code block. // an import could trigger a reorg which could change the canonical chain. // we depend on the canonical chain staying the same during this code block. let _import_guard = import_lock.read(); - let info = self.info(); - - // this can be `None` if the best chain is shorter than the target header. - let maybe_canon_hash = self.hash(*target_header.number())?; - - if maybe_canon_hash.as_ref() == Some(&target_hash) { - // if a `max_number` is given we try to fetch the block at the - // given depth, if it doesn't exist or `max_number` is not - // provided, we continue to search from all leaves below. - if let Some(max_number) = maybe_max_number { - if let Some(header) = self.hash(max_number)? { - return Ok(Some(header)) - } - } - } else if info.finalized_number >= *target_header.number() { - // header is on a dead fork. + if info.finalized_number > *base_header.number() { + // `base_header` is on a dead fork. return Ok(None) } - self.leaves()? }; // for each chain. longest chain first. shortest last for leaf_hash in leaves { - // start at the leaf let mut current_hash = leaf_hash; - - // if search is not restricted then the leaf is the best - let mut best_hash = leaf_hash; - - // go backwards entering the search space - // waiting until we are <= max_number - if let Some(max_number) = maybe_max_number { - loop { - let current_header = self - .header(current_hash)? - .ok_or_else(|| Error::MissingHeader(current_hash.to_string()))?; - - if current_header.number() <= &max_number { - best_hash = current_header.hash(); - break - } - - current_hash = *current_header.parent_hash(); - } - } - // go backwards through the chain (via parent links) loop { - // until we find target - if current_hash == target_hash { - return Ok(Some(best_hash)) + if current_hash == base_hash { + return Ok(Some(leaf_hash)) } let current_header = self @@ -280,7 +227,7 @@ pub trait Backend: .ok_or_else(|| Error::MissingHeader(current_hash.to_string()))?; // stop search in this chain once we go below the target's block number - if current_header.number() < target_header.number() { + if current_header.number() < base_header.number() { break } @@ -293,9 +240,8 @@ pub trait Backend: // // FIXME #1558 only issue this warning when not on a dead fork warn!( - "Block {:?} exists in chain but not found when following all \ - leaves backwards. Number limit = {:?}", - target_hash, maybe_max_number, + "Block {:?} exists in chain but not found when following all leaves backwards", + base_hash, ); Ok(None) diff --git a/primitives/consensus/common/src/select_chain.rs b/primitives/consensus/common/src/select_chain.rs index f366cd34c51ea..5beab6705046d 100644 --- a/primitives/consensus/common/src/select_chain.rs +++ b/primitives/consensus/common/src/select_chain.rs @@ -43,14 +43,14 @@ pub trait SelectChain: Sync + Send + Clone { /// finalize. async fn best_chain(&self) -> Result<::Header, Error>; - /// Get the best descendent of `target_hash` that we should attempt to - /// finalize next, if any. It is valid to return the given `target_hash` + /// Get the best descendent of `base_hash` that we should attempt to + /// finalize next, if any. It is valid to return the given `base_hash` /// itself if no better descendent exists. async fn finality_target( &self, - target_hash: ::Hash, + base_hash: ::Hash, _maybe_max_number: Option>, ) -> Result<::Hash, Error> { - Ok(target_hash) + Ok(base_hash) } }