Fix longest chain finalization target lookup (#13289)

* 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 <anton.kalyaev@gmail.com>

* Docs improvement

* Doc fix

* Apply suggestions from code review

Co-authored-by: Bastian Köcher <git@kchr.de>

* Apply more code suggestions

---------

Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>
Co-authored-by: Anton <anton.kalyaev@gmail.com>
Co-authored-by: Bastian Köcher <git@kchr.de>

substrate/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<<Block as BlockT>::Header> {
+	fn best_hash(&self) -> sp_blockchain::Result<<Block as BlockT>::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<<Block as BlockT>::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<NumberFor<Block>>,
+	) -> sp_blockchain::Result<Block::Hash> {
+		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<Vec<<Block as BlockT>::Hash>, sp_blockchain::Error> {
 		self.backend.blockchain().leaves()
 	}
@@ -80,20 +143,15 @@ where
 	}
 
 	async fn best_chain(&self) -> Result<<Block as BlockT>::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<NumberFor<Block>>,
 	) -> Result<Block::Hash, ConsensusError> {
-		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()))
 	}
 }

substrate/client/finality-grandpa/src/tests.rs

@@ -237,7 +237,7 @@ impl SelectChain<Block> for MockSelectChain {
 
 	async fn finality_target(
 		&self,
-		_target_hash: Hash,
+		_base_hash: Hash,
 		_maybe_max_number: Option<NumberFor<Block>>,
 	) -> Result<Hash, ConsensusError> {
 		Ok(self.finality_target.lock().take().unwrap())

substrate/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();

substrate/primitives/blockchain/src/backend.rs

@@ -183,96 +183,43 @@ pub trait Backend<Block: BlockT>:
 	/// Return hashes of all blocks that are children of the block with `parent_hash`.
 	fn children(&self, parent_hash: Block::Hash) -> Result<Vec<Block::Hash>>;
 
-	/// 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<NumberFor<Block>>,
+		base_hash: Block::Hash,
 		import_lock: &RwLock<()>,
 	) -> Result<Option<Block::Hash>> {
-		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<Block: BlockT>:
 					.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<Block: BlockT>:
 		//
 		// 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)

substrate/primitives/consensus/common/src/select_chain.rs

@@ -43,14 +43,14 @@ pub trait SelectChain<Block: BlockT>: Sync + Send + Clone {
 	/// finalize.
 	async fn best_chain(&self) -> Result<<Block as BlockT>::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: <Block as BlockT>::Hash,
+		base_hash: <Block as BlockT>::Hash,
 		_maybe_max_number: Option<NumberFor<Block>>,
 	) -> Result<<Block as BlockT>::Hash, Error> {
-		Ok(target_hash)
+		Ok(base_hash)
 	}
 }