client: fix comparison of CachedHeaderMetadata in tree_route

core/client/db/src/lib.rs

@@ -2222,6 +2222,40 @@ mod tests {
 		}
 	}
 
+	#[test]
+	fn test_tree_route_regression() {
+		// NOTE: this is a test for a regression introduced in #3665, the result
+		// of tree_route would be erroneously computed, since it was taking into
+		// account the `ancestor` in `CachedHeaderMetadata` for the comparison.
+		// in this test we simulate the same behavior with the side-effect
+		// triggering the issue being eviction of a previously fetched record
+		// from the cache, therefore this test is dependent on the LRU cache
+		// size for header metadata, which is currently set to 5000 elements.
+		let backend = Backend::<Block>::new_test(10000, 10000);
+		let blockchain = backend.blockchain();
+
+		let genesis = insert_header(&backend, 0, Default::default(), Vec::new(), Default::default());
+
+		let block100 = (1..=100).fold(genesis, |parent, n| {
+			insert_header(&backend, n, parent, Vec::new(), Default::default())
+		});
+
+		let block7000 = (101..=7000).fold(block100, |parent, n| {
+			insert_header(&backend, n, parent, Vec::new(), Default::default())
+		});
+
+		// This will cause the ancestor of `block100` to be set to `genesis` as a side-effect.
+		lowest_common_ancestor(blockchain, genesis, block100).unwrap();
+
+		// While traversing the tree we will have to do 6900 calls to
+		// `header_metadata`, which will make sure we will exhaust our cache
+		// which only takes 5000 elements. In particular, the `CachedHeaderMetadata` struct for
+		// block #100 will be evicted and will get a new value (with ancestor set to its parent).
+		let tree_route = tree_route(blockchain, block100, block7000).unwrap();
+
+		assert!(tree_route.retracted().is_empty());
+	}
+
 	#[test]
 	fn test_leaves_with_complex_block_tree() {
 		let backend: Arc<Backend<test_client::runtime::Block>> = Arc::new(Backend::new_test(20, 20));

core/client/header-metadata/src/lib.rs

@@ -122,7 +122,7 @@ pub fn tree_route<Block: BlockT, T: HeaderMetadata<Block>>(
 
 	// numbers are equal now. walk backwards until the block is the same
 
-	while to != from {
+	while to.hash != from.hash {
 		to_branch.push(HashAndNumber {
 			number: to.number,
 			hash: to.hash,
@@ -257,7 +257,7 @@ impl<Block: BlockT> HeaderMetadata<Block> for HeaderMetadataCache<Block> {
 }
 
 /// Cached header metadata. Used to efficiently traverse the tree.
-#[derive(Debug, Clone, Eq, PartialEq)]
+#[derive(Debug, Clone)]
 pub struct CachedHeaderMetadata<Block: BlockT> {
 	/// Hash of the header.
 	pub hash: Block::Hash,