From df407795010a9f82dfa2812cc0a586802d7aacef Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Thu, 19 May 2022 20:23:59 +0200 Subject: [PATCH 1/4] Fix node lookup on fork-tree after a warp-sync After a warp-sync, the error condition was triggered by the absence of the parent node of the first imported block. The previous lookup implementation was traversing the tree using a recursive **post-order** DFS, this technique doesn't trigger the issue. In the last iterative implementation we were using a BFS instead. --- utils/fork-tree/src/lib.rs | 60 ++++++++++++++++++++++++++------------ 1 file changed, 41 insertions(+), 19 deletions(-) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index 45957f4390532..9844b35162cc4 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -301,30 +301,49 @@ where F: Fn(&H, &H) -> Result, P: Fn(&V) -> bool, { - let mut path = vec![]; - let mut children = &self.roots; - let mut i = 0; - let mut best_depth = 0; - - while i < children.len() { - let node = &children[i]; - if node.number < *number && is_descendent_of(&node.hash, hash)? { - path.push(i); - if predicate(&node.data) { - best_depth = path.len(); + let mut stack = vec![]; + let mut root_idx = 0; + let mut found = false; + let mut is_descendent = false; + + while root_idx < self.roots.len() { + // The second element in the tuple tracks what is the next children index + // to search into. Once all the children are processed we check the node. + // We stop searching into alternative children as soon as we have found + // ancestor of the node we're looking for + stack.push((&self.roots[root_idx], 0)); + while let Some((node, i)) = stack.pop() { + if i < node.children.len() && !is_descendent { + stack.push((node, i + 1)); + stack.push((&node.children[i], 0)); + } else if node.number < *number && + (is_descendent || is_descendent_of(&node.hash, hash)?) + { + is_descendent = true; + if predicate(&node.data) { + found = true; + break + } } - i = 0; - children = &node.children; - } else { - i += 1; } + + // If the node we are looking for is found to be a descendent of the current + // root then we can stop searching under the other roots. + if is_descendent { + break + } + root_idx += 1; } - Ok(if best_depth == 0 { - None - } else { - path.truncate(best_depth); + Ok(if found { + // The path is the root index followed by the indices of all the children + // we were processing when we found the element (remember the stack + // contains the index of the **next** children to process). + let path: Vec<_> = + std::iter::once(root_idx).chain(stack.iter().map(|(_, i)| *i - 1)).collect(); Some(path) + } else { + None }) } @@ -1468,6 +1487,9 @@ mod test { fn find_node_works() { let (tree, is_descendent_of) = test_fork_tree(); + let node = tree.find_node_where(&"B", &2, &is_descendent_of, &|_| true).unwrap().unwrap(); + assert_eq!((node.hash, node.number), ("A", 1)); + let node = tree.find_node_where(&"D", &4, &is_descendent_of, &|_| true).unwrap().unwrap(); assert_eq!((node.hash, node.number), ("C", 3)); From eb983975b108561b3a42ce6cb9d2eb46e519be0a Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Thu, 19 May 2022 20:41:07 +0200 Subject: [PATCH 2/4] Added internal doc warning --- utils/fork-tree/src/lib.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index 9844b35162cc4..c4081224fe092 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -289,6 +289,10 @@ where /// index in the traverse path goes last. If a node is found that matches the predicate /// the returned path should always contain at least one index, otherwise `None` is /// returned. + // WARNING: some users of this method (i.e. Babe epoch changes tree) currently silently + // rely on a **post-order DFS** traversal of the tree. In particular via the + // `is_descendent_of` that when used after a warp-sync may query the backend for a + // root that is not present. pub fn find_node_index_where( &self, hash: &H, From 6306c1ceefa242cdbac5720714ec74beb61f1ab8 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Fri, 20 May 2022 11:54:37 +0200 Subject: [PATCH 3/4] Small optimization --- utils/fork-tree/src/lib.rs | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index c4081224fe092..af00fb32e68ba 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -289,10 +289,11 @@ where /// index in the traverse path goes last. If a node is found that matches the predicate /// the returned path should always contain at least one index, otherwise `None` is /// returned. - // WARNING: some users of this method (i.e. Babe epoch changes tree) currently silently - // rely on a **post-order DFS** traversal of the tree. In particular via the - // `is_descendent_of` that when used after a warp-sync may query the backend for a - // root that is not present. + // WARNING: some users of this method (i.e. consensus epoch changes tree) currently silently + // rely on a **DFS** traversal. If we are using instead a top-down traversal method then + // the `is_descendent_of` closure, when used after a warp-sync, will end up querying the + // backend for a block (the one corresponding to the root) that is not present and thus + // will return a wrong result. Here we are implementing a post-order DFS. pub fn find_node_index_where( &self, hash: &H, @@ -311,18 +312,21 @@ where let mut is_descendent = false; while root_idx < self.roots.len() { - // The second element in the tuple tracks what is the next children index - // to search into. Once all the children are processed we check the node. - // We stop searching into alternative children as soon as we have found - // ancestor of the node we're looking for + if *number <= self.roots[root_idx].number { + root_idx += 1; + continue + } + // The second element in the stack tuple tracks what is the **next** children + // index to search into. If we find an ancestor then we stop searching into + // alternative branches and we focus on the current path up to the root. stack.push((&self.roots[root_idx], 0)); while let Some((node, i)) = stack.pop() { if i < node.children.len() && !is_descendent { stack.push((node, i + 1)); - stack.push((&node.children[i], 0)); - } else if node.number < *number && - (is_descendent || is_descendent_of(&node.hash, hash)?) - { + if node.children[i].number < *number { + stack.push((&node.children[i], 0)); + } + } else if is_descendent || is_descendent_of(&node.hash, hash)? { is_descendent = true; if predicate(&node.data) { found = true; @@ -331,8 +335,8 @@ where } } - // If the node we are looking for is found to be a descendent of the current - // root then we can stop searching under the other roots. + // If the element we are looking for is a descendent of the current root + // then we can stop the search. if is_descendent { break } From 0290f0c5122c142e713f2cad3be11922b5a7c412 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Fri, 20 May 2022 12:21:06 +0200 Subject: [PATCH 4/4] Specify post-order DFS in the comment --- utils/fork-tree/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index af00fb32e68ba..6f987fa798461 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -290,10 +290,10 @@ where /// the returned path should always contain at least one index, otherwise `None` is /// returned. // WARNING: some users of this method (i.e. consensus epoch changes tree) currently silently - // rely on a **DFS** traversal. If we are using instead a top-down traversal method then - // the `is_descendent_of` closure, when used after a warp-sync, will end up querying the - // backend for a block (the one corresponding to the root) that is not present and thus - // will return a wrong result. Here we are implementing a post-order DFS. + // rely on a **post-order DFS** traversal. If we are using instead a top-down traversal method + // then the `is_descendent_of` closure, when used after a warp-sync, will end up querying the + // backend for a block (the one corresponding to the root) that is not present and thus will + // return a wrong result. Here we are implementing a post-order DFS. pub fn find_node_index_where( &self, hash: &H,