diff --git a/substrate/client/finality-grandpa/src/authorities.rs b/substrate/client/finality-grandpa/src/authorities.rs index 2de169fc8285a14a95e9142b3f0df02e29918970..de14c7b3ba390ab64f687e7cf897130fd86d7c4b 100644 --- a/substrate/client/finality-grandpa/src/authorities.rs +++ b/substrate/client/finality-grandpa/src/authorities.rs @@ -758,9 +758,10 @@ mod tests { authorities.add_pending_change(change_d.clone(), &static_is_descendent_of(false)).unwrap(); authorities.add_pending_change(change_e.clone(), &static_is_descendent_of(false)).unwrap(); + // ordered by subtree depth assert_eq!( authorities.pending_changes().collect::<Vec<_>>(), - vec![&change_b, &change_a, &change_c, &change_e, &change_d], + vec![&change_a, &change_c, &change_b, &change_e, &change_d], ); } @@ -798,7 +799,7 @@ mod tests { assert_eq!( authorities.pending_changes().collect::<Vec<_>>(), - vec![&change_b, &change_a], + vec![&change_a, &change_b], ); // finalizing "hash_c" won't enact the change signaled at "hash_a" but it will prune out "hash_b" diff --git a/substrate/utils/fork-tree/src/lib.rs b/substrate/utils/fork-tree/src/lib.rs index 1d01c53417649ac1d3df2802dbe43063ebf8c9e7..f266b6422302ee0a2e87af16a4756c2cc022c3d2 100644 --- a/substrate/utils/fork-tree/src/lib.rs +++ b/substrate/utils/fork-tree/src/lib.rs @@ -229,7 +229,10 @@ impl<H, N, V> ForkTree<H, N, V> where number = n; data = d; }, - None => return Ok(false), + None => { + self.rebalance(); + return Ok(false); + }, } } @@ -251,7 +254,9 @@ impl<H, N, V> ForkTree<H, N, V> where } fn node_iter(&self) -> impl Iterator<Item=&Node<H, N, V>> { - ForkTreeIterator { stack: self.roots.iter().collect() } + // we need to reverse the order of roots to maintain the expected + // ordering since the iterator uses a stack to track state. + ForkTreeIterator { stack: self.roots.iter().rev().collect() } } /// Iterates the nodes in the tree in pre-order. @@ -939,6 +944,10 @@ mod test { // — J - K // // (where N is not a part of fork tree) + // + // NOTE: the tree will get automatically rebalance on import and won't be laid out like the + // diagram above. the children will be ordered by subtree depth and the longest branches + // will be on the leftmost side of the tree. let is_descendent_of = |base: &&str, block: &&str| -> Result<bool, TestError> { let letters = vec!["B", "C", "D", "E", "F", "G", "H", "I", "J", "K", "L", "M", "O"]; match (*base, *block) { @@ -1132,7 +1141,7 @@ mod test { assert_eq!( tree.roots().map(|(h, n, _)| (h.clone(), n.clone())).collect::<Vec<_>>(), - vec![("I", 4), ("L", 4)], + vec![("L", 4), ("I", 4)], ); // finalizing a node from another fork that isn't part of the tree clears the tree @@ -1180,7 +1189,7 @@ mod test { assert_eq!( tree.roots().map(|(h, n, _)| (h.clone(), n.clone())).collect::<Vec<_>>(), - vec![("I", 4), ("L", 4)], + vec![("L", 4), ("I", 4)], ); assert_eq!( @@ -1354,11 +1363,11 @@ mod test { vec![ ("A", 1), ("B", 2), ("C", 3), ("D", 4), ("E", 5), - ("F", 2), + ("F", 2), ("H", 3), ("L", 4), ("M", 5), + ("O", 5), + ("I", 4), ("G", 3), - ("H", 3), ("I", 4), - ("L", 4), ("M", 5), ("O", 5), - ("J", 2), ("K", 3) + ("J", 2), ("K", 3), ], ); } @@ -1480,7 +1489,7 @@ mod test { assert_eq!( removed.map(|(hash, _, _)| hash).collect::<Vec<_>>(), - vec!["A", "F", "G", "H", "I", "L", "M", "O", "J", "K"] + vec!["A", "F", "H", "L", "M", "O", "I", "G", "J", "K"] ); let removed = tree.prune( @@ -1545,19 +1554,30 @@ mod test { fn tree_rebalance() { let (mut tree, _) = test_fork_tree(); + // the tree is automatically rebalanced on import, therefore we should iterate in preorder + // exploring the longest forks first. check the ascii art above to understand the expected + // output below. assert_eq!( tree.iter().map(|(h, _, _)| *h).collect::<Vec<_>>(), - vec!["A", "B", "C", "D", "E", "F", "G", "H", "I", "L", "M", "O", "J", "K"], + vec!["A", "B", "C", "D", "E", "F", "H", "L", "M", "O", "I", "G", "J", "K"], ); - // after rebalancing the tree we should iterate in preorder exploring - // the longest forks first. check the ascii art above to understand the - // expected output below. - tree.rebalance(); + // let's add a block "P" which is a descendent of block "O" + let is_descendent_of = |base: &&str, block: &&str| -> Result<bool, TestError> { + match (*base, *block) { + (b, "P") => Ok(vec!["A", "F", "L", "O"].into_iter().any(|n| n == b)), + _ => Ok(false), + } + }; + + tree.import("P", 6, (), &is_descendent_of).unwrap(); + // this should re-order the tree, since the branch "A -> B -> C -> D -> E" is no longer tied + // with 5 blocks depth. additionally "O" should be visited before "M" now, since it has one + // descendent "P" which makes that branch 6 blocks long. assert_eq!( tree.iter().map(|(h, _, _)| *h).collect::<Vec<_>>(), - ["A", "B", "C", "D", "E", "F", "H", "L", "M", "O", "I", "G", "J", "K"] + ["A", "F", "H", "L", "O", "P", "M", "I", "G", "B", "C", "D", "E", "J", "K"] ); } }