Fix longest chain finalization target lookup (paritytech#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>

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,22 +48,85 @@ 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()
 			.header(best_hash)?
 			.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()))
 	}
 }

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())