From 8d55af9b06cd89ee2187db524ee5b79343a24496 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Sun, 19 Feb 2023 14:50:03 +0700 Subject: [PATCH] Revert "Fix longest chain finalization target lookup (#13289)" This reverts commit f26bf7c31ec18a946ec9df95538e18fc874aaea3. --- 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, 149 insertions(+), 242 deletions(-) diff --git a/client/consensus/common/src/longest_chain.rs b/client/consensus/common/src/longest_chain.rs index ece2486209ff3..fab2c3a4c06fd 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, Header, NumberFor}; +use sp_runtime::traits::{Block as BlockT, NumberFor}; use std::{marker::PhantomData, sync::Arc}; /// Implement Longest Chain Select implementation @@ -48,19 +48,15 @@ where LongestChain { backend, _phantom: Default::default() } } - fn best_hash(&self) -> sp_blockchain::Result<::Hash> { + fn best_block_header(&self) -> sp_blockchain::Result<::Header> { let info = self.backend.blockchain().info(); let import_lock = self.backend.get_import_lock(); let best_hash = self .backend .blockchain() - .longest_containing(info.best_hash, import_lock)? + .best_containing(info.best_hash, None, 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() @@ -68,65 +64,6 @@ 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() } @@ -143,15 +80,20 @@ where } async fn best_chain(&self) -> Result<::Header, ConsensusError> { - LongestChain::best_header(self).map_err(|e| ConsensusError::ChainLookup(e.to_string())) + LongestChain::best_block_header(self) + .map_err(|e| ConsensusError::ChainLookup(e.to_string())) } async fn finality_target( &self, - base_hash: Block::Hash, + target_hash: Block::Hash, maybe_max_number: Option>, ) -> Result { - LongestChain::finality_target(self, base_hash, maybe_max_number) + 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)) .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 62ef4709e1909..07ea4649148fb 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, - _base_hash: Hash, + _target_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 12b92afc458b4..97c22a1cb509e 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 finality_target_on_longest_chain_with_single_chain_3_blocks() { +fn best_containing_on_longest_chain_with_single_chain_3_blocks() { // block tree: // G -> A1 -> A2 @@ -606,7 +606,7 @@ fn finality_target_on_longest_chain_with_single_chain_3_blocks() { } #[test] -fn finality_target_on_longest_chain_with_multiple_forks() { +fn best_containing_on_longest_chain_with_multiple_forks() { // block tree: // G -> A1 -> A2 -> A3 -> A4 -> A5 // A1 -> B2 -> B3 -> B4 @@ -731,13 +731,8 @@ fn finality_target_on_longest_chain_with_multiple_forks() { assert!(leaves.contains(&d2.hash())); assert_eq!(leaves.len(), 4); - // 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, + let finality_target = |target_hash, number| { + block_on(longest_chain_select.finality_target(target_hash, number)).unwrap() }; // search without restriction @@ -747,11 +742,11 @@ fn finality_target_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!(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)); + 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)); // search only blocks with number <= 5. equivalent to without restriction for this scenario assert_eq!(a5.hash(), finality_target(genesis_hash, Some(5))); @@ -760,11 +755,11 @@ fn finality_target_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!(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))); + 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))); // search only blocks with number <= 4 assert_eq!(a4.hash(), finality_target(genesis_hash, Some(4))); @@ -772,72 +767,73 @@ fn finality_target_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!(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))); + 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))); // 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!(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))); + 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))); // 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!(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))); + 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))); // 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!(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))); + 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))); // search only blocks with number <= 0 assert_eq!(genesis_hash, finality_target(genesis_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))); + 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))); } #[test] -fn finality_target_on_longest_chain_with_max_depth_higher_than_best() { +fn best_containing_on_longest_chain_with_max_depth_higher_than_best() { // block tree: // G -> A1 -> A2 - let (mut client, chain_select) = TestClientBuilder::new().build_with_longest_chain(); + let (mut client, longest_chain_select) = TestClientBuilder::new().build_with_longest_chain(); // G -> A1 let a1 = client.new_block(Default::default()).unwrap().build().unwrap().block; @@ -849,93 +845,10 @@ fn finality_target_on_longest_chain_with_max_depth_higher_than_best() { let genesis_hash = client.chain_info().genesis_hash; - 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()); + assert_eq!( + a2.hash(), + block_on(longest_chain_select.finality_target(genesis_hash, Some(10))).unwrap(), + ); } #[test] @@ -1082,7 +995,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 (first) longest chain + // A2 is the current best since it's the longest chain assert_eq!(client.chain_info().best_hash, a2.hash()); // we finalize block B1 which is on a different branch from current best @@ -1099,7 +1012,8 @@ 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() @@ -1108,9 +1022,6 @@ 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 7339d4c1a6804..ec9c8ac0d5780 100644 --- a/primitives/blockchain/src/backend.rs +++ b/primitives/blockchain/src/backend.rs @@ -183,43 +183,96 @@ 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 longest chain that contains - /// a block with the given `base_hash`. + /// Get the most recent block hash of the best (longest) chains + /// that contain block with the given `target_hash`. /// /// The search space is always limited to blocks which are in the finalized /// chain or descendents of it. /// - /// 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( + /// 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( &self, - base_hash: Block::Hash, + target_hash: Block::Hash, + maybe_max_number: Option>, import_lock: &RwLock<()>, ) -> Result> { - let Some(base_header) = self.header(base_hash)? else { - return Ok(None) + let target_header = { + match self.header(target_hash)? { + Some(x) => x, + // target not in blockchain + None => 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(); - if info.finalized_number > *base_header.number() { - // `base_header` is on a dead fork. + + // 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. 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 { - if current_hash == base_hash { - return Ok(Some(leaf_hash)) + // until we find target + if current_hash == target_hash { + return Ok(Some(best_hash)) } let current_header = self @@ -227,7 +280,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() < base_header.number() { + if current_header.number() < target_header.number() { break } @@ -240,8 +293,9 @@ 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", - base_hash, + "Block {:?} exists in chain but not found when following all \ + leaves backwards. Number limit = {:?}", + target_hash, maybe_max_number, ); Ok(None) diff --git a/primitives/consensus/common/src/select_chain.rs b/primitives/consensus/common/src/select_chain.rs index 5beab6705046d..f366cd34c51ea 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 `base_hash` that we should attempt to - /// finalize next, if any. It is valid to return the given `base_hash` + /// 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` /// itself if no better descendent exists. async fn finality_target( &self, - base_hash: ::Hash, + target_hash: ::Hash, _maybe_max_number: Option>, ) -> Result<::Hash, Error> { - Ok(base_hash) + Ok(target_hash) } }