diff --git a/substrate/core/client/db/src/lib.rs b/substrate/core/client/db/src/lib.rs index 7cc6ef54a06ee198b4cb2df065f3b4b2bcaa40d9..7061e9d29af6cd866cbd2c3deccd0da13e5de47a 100644 --- a/substrate/core/client/db/src/lib.rs +++ b/substrate/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)); diff --git a/substrate/core/client/header-metadata/src/lib.rs b/substrate/core/client/header-metadata/src/lib.rs index cce45f264e8ea08bb543323f2bfcafd6b5c3165a..a8c3886020e271daac7fafb9a75cbe4cd4e2548b 100644 --- a/substrate/core/client/header-metadata/src/lib.rs +++ b/substrate/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,