From bc702622b2f08509b6d0a10de0821226c5960e11 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Fri, 25 Nov 2022 09:45:32 +0100 Subject: [PATCH 1/7] Removed assumptions about ancestry from fork tree prune method --- client/consensus/epochs/src/lib.rs | 4 +- utils/fork-tree/src/lib.rs | 177 ++++++++++++++++++++++------- 2 files changed, 135 insertions(+), 46 deletions(-) diff --git a/client/consensus/epochs/src/lib.rs b/client/consensus/epochs/src/lib.rs index c213a49b8e4e4..ca2f880f1f047 100644 --- a/client/consensus/epochs/src/lib.rs +++ b/client/consensus/epochs/src/lib.rs @@ -1183,7 +1183,7 @@ mod tests { let mut nodes: Vec<_> = epoch_changes.tree().iter().map(|(h, _, _)| h).collect(); nodes.sort(); - assert_eq!(nodes, vec![b"A", b"B", b"C", b"E", b"F", b"G"]); + assert_eq!(nodes, vec![b"A", b"B", b"C", b"F", b"G"]); // Finalize block y @ number 35, slot 330 // This should prune all nodes imported by blocks with a number < 35 that are not @@ -1194,7 +1194,7 @@ mod tests { let mut nodes: Vec<_> = epoch_changes.tree().iter().map(|(h, _, _)| h).collect(); nodes.sort(); - assert_eq!(nodes, vec![b"B", b"C", b"F", b"G"]); + assert_eq!(nodes, vec![b"B", b"C", b"G"]); } #[test] diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index 76c28d910f943..72e350d5be8b6 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -32,7 +32,7 @@ pub enum Error { UnfinalizedAncestor, /// Imported or finalized node that is an ancestor of previously finalized node. Revert, - /// Error throw by client when checking for node ancestry. + /// Error throw by user when checking for node ancestry. Client(E), } @@ -48,11 +48,7 @@ impl fmt::Display for Error { } } -impl std::error::Error for Error { - fn cause(&self) -> Option<&dyn std::error::Error> { - None - } -} +impl std::error::Error for Error {} impl From for Error { fn from(err: E) -> Error { @@ -83,7 +79,7 @@ pub enum FilterAction { /// A tree data structure that stores several nodes across multiple branches. /// /// Top-level branches are called roots. The tree has functionality for -/// finalizing nodes, which means that that node is traversed, and all competing +/// finalizing nodes, which means that node is traversed, and all competing /// branches are pruned. It also guarantees that nodes in the tree are finalized /// in order. Each node is uniquely identified by its hash but can be ordered by /// its number. In order to build the tree an external function must be provided @@ -99,12 +95,14 @@ where H: PartialEq, N: Ord, { - /// Create a new empty tree. + /// Create a new empty tree instance. pub fn new() -> ForkTree { ForkTree { roots: Vec::new(), best_finalized_number: None } } - /// Rebalance the tree, i.e. sort child nodes by max branch depth (decreasing). + /// Rebalance the tree + /// + /// For each tree level sort child nodes by max branch depth (decreasing). /// /// Most operations in the tree are performed with depth-first search /// starting from the leftmost node at every level, since this tree is meant @@ -120,10 +118,12 @@ where } } - /// Import a new node into the tree. The given function `is_descendent_of` - /// should return `true` if the second hash (target) is a descendent of the - /// first hash (base). This method assumes that nodes in the same branch are - /// imported in order. + /// Import a new node into the tree. + /// + /// The given function `is_descendent_of` should return `true` if the second + /// hash (target) is a descendent of the first hash (base). + /// + /// This method assumes that nodes in the same branch are imported in order. /// /// Returns `true` if the imported node is a root. // WARNING: some users of this method (i.e. consensus epoch changes tree) currently silently @@ -232,9 +232,10 @@ where } /// Find a node in the tree that is the deepest ancestor of the given - /// block hash and which passes the given predicate. The given function - /// `is_descendent_of` should return `true` if the second hash (target) - /// is a descendent of the first hash (base). + /// block hash and which passes the given predicate. + /// + /// The given function `is_descendent_of` should return `true` if the + /// second hash (target) is a descendent of the first hash (base). pub fn find_node_where( &self, hash: &H, @@ -281,10 +282,12 @@ where /// Same as [`find_node_where`](ForkTree::find_node_where), but returns indices. /// /// The returned indices represent the full path to reach the matching node starting - /// from first to last, i.e. the earliest index in the traverse path goes first, and the final - /// 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. + /// from one of the roots, i.e. the earliest index in the traverse path goes first, + /// and the final 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. consensus epoch changes tree) currently silently // 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 @@ -351,14 +354,16 @@ where }) } - /// Prune the tree, removing all non-canonical nodes. We find the node in the - /// tree that is the deepest ancestor of the given hash and that passes the - /// given predicate. If such a node exists, we re-root the tree to this - /// node. Otherwise the tree remains unchanged. The given function - /// `is_descendent_of` should return `true` if the second hash (target) is a - /// descendent of the first hash (base). + /// Prune the tree, removing all non-canonical nodes. /// - /// Returns all pruned node data. + /// We find the node in the tree that is the deepest ancestor of the given hash + /// and that passes the given predicate. If such a node exists, we re-root the + /// tree to this node. Otherwise the tree remains unchanged. + /// + /// The given function `is_descendent_of` should return `true` if the second + /// hash (target) is a descendent of the first hash (base). + /// + /// Returns all pruned nodes data. pub fn prune( &mut self, hash: &H, @@ -379,6 +384,7 @@ where let mut old_roots = std::mem::take(&mut self.roots); + // Find and detach the new root from the removed nodes let curr_children = root_index .iter() .take(root_index.len() - 1) @@ -387,20 +393,40 @@ where let mut removed = old_roots; - // we found the deepest ancestor of the finalized block, so we prune - // out any children that don't include the finalized block. - let root_children = std::mem::take(&mut root.children); - let mut is_first = true; + // If (because of the `predicate`) the new root is not the deepest + // ancestor of `hash` then we can remove all the nodes that are + // descendents of the new `root` but not ancestors of `hash`. + let mut curr = &mut root; + loop { + let mut next_ancestor_idx = usize::MAX; + for (idx, child) in curr.children.iter().enumerate() { + if child.number == *number && child.hash == *hash || + child.number < *number && is_descendent_of(&child.hash, hash)? + { + next_ancestor_idx = idx; + break + } + } + if next_ancestor_idx == usize::MAX { + // We are above all current node children, stop searching + break + } + // Preserve only the valid ancestor node + let mut curr_children = std::mem::take(&mut curr.children); + let keep_node = curr_children.remove(next_ancestor_idx); + removed.append(&mut curr_children); + curr.children = vec![keep_node]; + curr = &mut curr.children[0]; + } - for child in root_children { - if is_first && - (child.number == *number && child.hash == *hash || - child.number < *number && is_descendent_of(&child.hash, hash)?) + // Curr now points to our direct ancestor, + // remove all additional stale nodes + let children = std::mem::take(&mut curr.children); + for child in children { + if child.number == *number && child.hash == *hash || + *number < child.number && is_descendent_of(hash, &child.hash)? { - root.children.push(child); - // assuming that the tree is well formed only one child should pass this - // requirement due to ancestry restrictions (i.e. they must be different forks). - is_first = false; + curr.children.push(child); } else { removed.push(child); } @@ -859,7 +885,8 @@ mod test { // will be on the leftmost side of the tree. let is_descendent_of = |base: &&str, block: &&str| -> Result { let letters = vec!["B", "C", "D", "E", "F", "G", "H", "I", "J", "K", "L", "M", "N", "O"]; - match (*base, *block) { + let block = block.to_uppercase(); + match (*base, block) { ("A", b) => Ok(letters.into_iter().any(|n| n == b)), ("B", b) => Ok(b == "C" || b == "D" || b == "E"), ("C", b) => Ok(b == "D" || b == "E"), @@ -881,21 +908,17 @@ mod test { }; tree.import("A", 1, (), &is_descendent_of).unwrap(); - tree.import("B", 2, (), &is_descendent_of).unwrap(); tree.import("C", 3, (), &is_descendent_of).unwrap(); tree.import("D", 4, (), &is_descendent_of).unwrap(); tree.import("E", 5, (), &is_descendent_of).unwrap(); - tree.import("F", 2, (), &is_descendent_of).unwrap(); tree.import("G", 3, (), &is_descendent_of).unwrap(); - tree.import("H", 3, (), &is_descendent_of).unwrap(); tree.import("I", 4, (), &is_descendent_of).unwrap(); tree.import("L", 4, (), &is_descendent_of).unwrap(); tree.import("M", 5, (), &is_descendent_of).unwrap(); tree.import("O", 5, (), &is_descendent_of).unwrap(); - tree.import("J", 2, (), &is_descendent_of).unwrap(); tree.import("K", 3, (), &is_descendent_of).unwrap(); @@ -1332,6 +1355,72 @@ mod test { assert_eq!(removed.map(|(hash, _, _)| hash).collect::>(), vec!["B", "C"]); } + #[test] + fn prune_works2() { + let (mut tree, is_descendent_of) = test_fork_tree(); + + let removed = tree.prune(&"c", &3, &is_descendent_of, &|_| true).unwrap(); + + assert_eq!(tree.roots.iter().map(|node| node.hash).collect::>(), vec!["B"]); + + assert_eq!( + tree.iter().map(|(hash, _, _)| *hash).collect::>(), + vec!["B", "C", "D", "E"], + ); + + assert_eq!( + removed.map(|(hash, _, _)| hash).collect::>(), + vec!["A", "F", "H", "L", "M", "O", "I", "G", "J", "K"] + ); + } + + #[test] + fn prune_works3() { + let (mut tree, is_descendent_of) = test_fork_tree(); + + let flag = std::cell::RefCell::new(false); + let removed = tree.prune(&"m", &5, &is_descendent_of, &|_| flag.replace(true)).unwrap(); + + assert_eq!(tree.roots.iter().map(|node| node.hash).collect::>(), vec!["H"]); + + assert_eq!( + tree.iter().map(|(hash, _, _)| *hash).collect::>(), + vec!["H", "L", "M", "O"], + ); + + assert_eq!( + removed.map(|(hash, _, _)| hash).collect::>(), + vec!["I", "A", "B", "C", "D", "E", "F", "G", "J", "K"] + ); + } + + #[test] + fn prune_works4() { + let (mut tree, is_descendent_of) = test_fork_tree(); + + let counter = std::cell::RefCell::new(0); + let removed = tree + .prune(&"m", &5, &is_descendent_of, &|_| { + let mut counter = counter.borrow_mut(); + let prev = *counter; + *counter += 1; + prev == 2 + }) + .unwrap(); + + assert_eq!(tree.roots.iter().map(|node| node.hash).collect::>(), vec!["F"]); + + assert_eq!( + tree.iter().map(|(hash, _, _)| *hash).collect::>(), + vec!["F", "H", "L", "M", "O"], + ); + + assert_eq!( + removed.map(|(hash, _, _)| hash).collect::>(), + vec!["I", "G", "A", "B", "C", "D", "E", "J", "K"] + ); + } + #[test] fn find_node_backtracks_after_finding_highest_descending_node() { let mut tree = ForkTree::new(); From f3c12b30c9bec0f010e92a53c82e3020f121a98d Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Sat, 17 Dec 2022 13:26:36 +0100 Subject: [PATCH 2/7] Tests improvement --- utils/fork-tree/src/lib.rs | 228 +++++++++++++++++++------------------ 1 file changed, 118 insertions(+), 110 deletions(-) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index 72e350d5be8b6..254f06cc32503 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -862,21 +862,21 @@ mod test { impl std::error::Error for TestError {} fn test_fork_tree<'a>( - ) -> (ForkTree<&'a str, u64, ()>, impl Fn(&&str, &&str) -> Result) { + ) -> (ForkTree<&'a str, u64, u32>, impl Fn(&&str, &&str) -> Result) { let mut tree = ForkTree::new(); #[rustfmt::skip] // - // - B - C - D - E - // / - // / - G - // / / - // A - F - H - I - // \ \ - // \ - L - M - N - // \ \ - // \ - O - // - J - K + // +---B-c-C---D---E + // | + // | +---G + // | | + // 0---A---F---H---I + // | | + // | +---L-m-M---N + // | | + // | +---O + // +---J---K // // (where N is not a part of fork tree) // @@ -885,10 +885,12 @@ mod test { // will be on the leftmost side of the tree. let is_descendent_of = |base: &&str, block: &&str| -> Result { let letters = vec!["B", "C", "D", "E", "F", "G", "H", "I", "J", "K", "L", "M", "N", "O"]; + // This is a trick to have lowercase blocks be direct parents of their + // uppercase correspondent (A excluded) let block = block.to_uppercase(); match (*base, block) { ("A", b) => Ok(letters.into_iter().any(|n| n == b)), - ("B", b) => Ok(b == "C" || b == "D" || b == "E"), + ("B" | "c", b) => Ok(b == "C" || b == "D" || b == "E"), ("C", b) => Ok(b == "D" || b == "E"), ("D", b) => Ok(b == "E"), ("E", _) => Ok(false), @@ -899,7 +901,8 @@ mod test { ("I", _) => Ok(false), ("J", b) => Ok(b == "K"), ("K", _) => Ok(false), - ("L", b) => Ok(b == "M" || b == "O" || b == "N"), + ("L", b) => Ok(b == "M" || b == "N" || b == "O"), + ("m", b) => Ok(b == "M" || b == "N"), ("M", b) => Ok(b == "N"), ("O", _) => Ok(false), ("0", _) => Ok(true), @@ -907,20 +910,20 @@ mod test { } }; - tree.import("A", 1, (), &is_descendent_of).unwrap(); - tree.import("B", 2, (), &is_descendent_of).unwrap(); - tree.import("C", 3, (), &is_descendent_of).unwrap(); - tree.import("D", 4, (), &is_descendent_of).unwrap(); - tree.import("E", 5, (), &is_descendent_of).unwrap(); - tree.import("F", 2, (), &is_descendent_of).unwrap(); - tree.import("G", 3, (), &is_descendent_of).unwrap(); - tree.import("H", 3, (), &is_descendent_of).unwrap(); - tree.import("I", 4, (), &is_descendent_of).unwrap(); - tree.import("L", 4, (), &is_descendent_of).unwrap(); - tree.import("M", 5, (), &is_descendent_of).unwrap(); - tree.import("O", 5, (), &is_descendent_of).unwrap(); - tree.import("J", 2, (), &is_descendent_of).unwrap(); - tree.import("K", 3, (), &is_descendent_of).unwrap(); + tree.import("A", 10, 1, &is_descendent_of).unwrap(); + tree.import("B", 20, 2, &is_descendent_of).unwrap(); + tree.import("C", 30, 3, &is_descendent_of).unwrap(); + tree.import("D", 40, 4, &is_descendent_of).unwrap(); + tree.import("E", 50, 5, &is_descendent_of).unwrap(); + tree.import("F", 20, 2, &is_descendent_of).unwrap(); + tree.import("G", 30, 3, &is_descendent_of).unwrap(); + tree.import("H", 30, 3, &is_descendent_of).unwrap(); + tree.import("I", 40, 4, &is_descendent_of).unwrap(); + tree.import("L", 40, 4, &is_descendent_of).unwrap(); + tree.import("M", 50, 5, &is_descendent_of).unwrap(); + tree.import("O", 50, 5, &is_descendent_of).unwrap(); + tree.import("J", 20, 2, &is_descendent_of).unwrap(); + tree.import("K", 30, 3, &is_descendent_of).unwrap(); (tree, is_descendent_of) } @@ -931,22 +934,22 @@ mod test { tree.finalize_root(&"A"); - assert_eq!(tree.best_finalized_number, Some(1)); + assert_eq!(tree.best_finalized_number, Some(10)); - assert_eq!(tree.import("A", 1, (), &is_descendent_of), Err(Error::Revert)); + assert_eq!(tree.import("A", 10, 1, &is_descendent_of), Err(Error::Revert)); } #[test] fn import_doesnt_add_duplicates() { let (mut tree, is_descendent_of) = test_fork_tree(); - assert_eq!(tree.import("A", 1, (), &is_descendent_of), Err(Error::Duplicate)); + assert_eq!(tree.import("A", 10, 1, &is_descendent_of), Err(Error::Duplicate)); - assert_eq!(tree.import("I", 4, (), &is_descendent_of), Err(Error::Duplicate)); + assert_eq!(tree.import("I", 40, 4, &is_descendent_of), Err(Error::Duplicate)); - assert_eq!(tree.import("G", 3, (), &is_descendent_of), Err(Error::Duplicate)); + assert_eq!(tree.import("G", 30, 3, &is_descendent_of), Err(Error::Duplicate)); - assert_eq!(tree.import("K", 3, (), &is_descendent_of), Err(Error::Duplicate)); + assert_eq!(tree.import("K", 30, 3, &is_descendent_of), Err(Error::Duplicate)); } #[test] @@ -954,14 +957,14 @@ mod test { let finalize_a = || { let (mut tree, ..) = test_fork_tree(); - assert_eq!(tree.roots().map(|(h, n, _)| (*h, *n)).collect::>(), vec![("A", 1)]); + assert_eq!(tree.roots().map(|(h, n, _)| (*h, *n)).collect::>(), vec![("A", 10)]); // finalizing "A" opens up three possible forks tree.finalize_root(&"A"); assert_eq!( tree.roots().map(|(h, n, _)| (*h, *n)).collect::>(), - vec![("B", 2), ("F", 2), ("J", 2)], + vec![("B", 20), ("F", 20), ("J", 20)], ); tree @@ -973,7 +976,7 @@ mod test { // finalizing "B" will progress on its fork and remove any other competing forks tree.finalize_root(&"B"); - assert_eq!(tree.roots().map(|(h, n, _)| (*h, *n)).collect::>(), vec![("C", 3)],); + assert_eq!(tree.roots().map(|(h, n, _)| (*h, *n)).collect::>(), vec![("C", 30)],); // all the other forks have been pruned assert!(tree.roots.len() == 1); @@ -985,7 +988,7 @@ mod test { // finalizing "J" will progress on its fork and remove any other competing forks tree.finalize_root(&"J"); - assert_eq!(tree.roots().map(|(h, n, _)| (*h, *n)).collect::>(), vec![("K", 3)],); + assert_eq!(tree.roots().map(|(h, n, _)| (*h, *n)).collect::>(), vec![("K", 30)],); // all the other forks have been pruned assert!(tree.roots.len() == 1); @@ -1005,42 +1008,42 @@ mod test { // finalizing "A" opens up three possible forks assert_eq!( - tree.finalize(&"A", 1, &is_descendent_of), - Ok(FinalizationResult::Changed(Some(()))), + tree.finalize(&"A", 10, &is_descendent_of), + Ok(FinalizationResult::Changed(Some(1))), ); assert_eq!( tree.roots().map(|(h, n, _)| (*h, *n)).collect::>(), - vec![("B", 2), ("F", 2), ("J", 2)], + vec![("B", 20), ("F", 20), ("J", 20)], ); // finalizing anything lower than what we observed will fail - assert_eq!(tree.best_finalized_number, Some(1)); + assert_eq!(tree.best_finalized_number, Some(10)); - assert_eq!(tree.finalize(&"Z", 1, &is_descendent_of), Err(Error::Revert)); + assert_eq!(tree.finalize(&"Z", 10, &is_descendent_of), Err(Error::Revert)); // trying to finalize a node without finalizing its ancestors first will fail - assert_eq!(tree.finalize(&"H", 3, &is_descendent_of), Err(Error::UnfinalizedAncestor)); + assert_eq!(tree.finalize(&"H", 30, &is_descendent_of), Err(Error::UnfinalizedAncestor)); // after finalizing "F" we can finalize "H" assert_eq!( - tree.finalize(&"F", 2, &is_descendent_of), - Ok(FinalizationResult::Changed(Some(()))), + tree.finalize(&"F", 20, &is_descendent_of), + Ok(FinalizationResult::Changed(Some(2))), ); assert_eq!( - tree.finalize(&"H", 3, &is_descendent_of), - Ok(FinalizationResult::Changed(Some(()))), + tree.finalize(&"H", 30, &is_descendent_of), + Ok(FinalizationResult::Changed(Some(3))), ); assert_eq!( tree.roots().map(|(h, n, _)| (*h, *n)).collect::>(), - vec![("L", 4), ("I", 4)], + vec![("L", 40), ("I", 40)], ); // finalizing a node from another fork that isn't part of the tree clears the tree assert_eq!( - tree.finalize(&"Z", 5, &is_descendent_of), + tree.finalize(&"Z", 50, &is_descendent_of), Ok(FinalizationResult::Changed(None)), ); @@ -1063,13 +1066,13 @@ mod test { // finalizing "A" opens up three possible forks assert_eq!( - tree.finalize_with_ancestors(&"A", 1, &is_descendent_of), - Ok(FinalizationResult::Changed(Some(()))), + tree.finalize_with_ancestors(&"A", 10, &is_descendent_of), + Ok(FinalizationResult::Changed(Some(1))), ); assert_eq!( tree.roots().map(|(h, n, _)| (*h, *n)).collect::>(), - vec![("B", 2), ("F", 2), ("J", 2)], + vec![("B", 20), ("F", 20), ("J", 20)], ); // finalizing H: @@ -1077,16 +1080,16 @@ mod test { // 2) opens root that is ancestor of H (F -> G+H) // 3) finalizes the just opened root H (H -> I + L) assert_eq!( - tree.finalize_with_ancestors(&"H", 3, &is_descendent_of), - Ok(FinalizationResult::Changed(Some(()))), + tree.finalize_with_ancestors(&"H", 30, &is_descendent_of), + Ok(FinalizationResult::Changed(Some(3))), ); assert_eq!( tree.roots().map(|(h, n, _)| (*h, *n)).collect::>(), - vec![("L", 4), ("I", 4)], + vec![("L", 40), ("I", 40)], ); - assert_eq!(tree.best_finalized_number, Some(3)); + assert_eq!(tree.best_finalized_number, Some(30)); // finalizing N (which is not a part of the tree): // 1) removes roots that are not ancestors/descendants of N (I) @@ -1094,13 +1097,13 @@ mod test { // 3) removes roots that are not ancestors/descendants of N (O) // 4) opens root that is ancestor of N (M -> {}) assert_eq!( - tree.finalize_with_ancestors(&"N", 6, &is_descendent_of), + tree.finalize_with_ancestors(&"N", 60, &is_descendent_of), Ok(FinalizationResult::Changed(None)), ); assert_eq!(tree.roots().map(|(h, n, _)| (*h, *n)).collect::>(), vec![],); - assert_eq!(tree.best_finalized_number, Some(6)); + assert_eq!(tree.best_finalized_number, Some(60)); } #[test] @@ -1232,20 +1235,20 @@ mod test { assert_eq!( tree.iter().map(|(h, n, _)| (*h, *n)).collect::>(), vec![ - ("A", 1), - ("B", 2), - ("C", 3), - ("D", 4), - ("E", 5), - ("F", 2), - ("H", 3), - ("L", 4), - ("M", 5), - ("O", 5), - ("I", 4), - ("G", 3), - ("J", 2), - ("K", 3), + ("A", 10), + ("B", 20), + ("C", 30), + ("D", 40), + ("E", 50), + ("F", 20), + ("H", 30), + ("L", 40), + ("M", 50), + ("O", 50), + ("I", 40), + ("G", 30), + ("J", 20), + ("K", 30), ], ); } @@ -1312,9 +1315,9 @@ mod test { // Extend the single root fork-tree to also excercise the roots order during map. let is_descendent_of = |_: &&str, _: &&str| -> Result { Ok(false) }; - let is_root = tree.import("A1", 1, (), &is_descendent_of).unwrap(); + let is_root = tree.import("A1", 10, 1, &is_descendent_of).unwrap(); assert!(is_root); - let is_root = tree.import("A2", 1, (), &is_descendent_of).unwrap(); + let is_root = tree.import("A2", 10, 1, &is_descendent_of).unwrap(); assert!(is_root); let old_tree = tree.clone(); @@ -1332,7 +1335,7 @@ mod test { fn prune_works() { let (mut tree, is_descendent_of) = test_fork_tree(); - let removed = tree.prune(&"C", &3, &is_descendent_of, &|_| true).unwrap(); + let removed = tree.prune(&"C", &30, &is_descendent_of, &|_| true).unwrap(); assert_eq!(tree.roots.iter().map(|node| node.hash).collect::>(), vec!["B"]); @@ -1346,7 +1349,7 @@ mod test { vec!["A", "F", "H", "L", "M", "O", "I", "G", "J", "K"] ); - let removed = tree.prune(&"E", &5, &is_descendent_of, &|_| true).unwrap(); + let removed = tree.prune(&"E", &50, &is_descendent_of, &|_| true).unwrap(); assert_eq!(tree.roots.iter().map(|node| node.hash).collect::>(), vec!["D"]); @@ -1359,7 +1362,7 @@ mod test { fn prune_works2() { let (mut tree, is_descendent_of) = test_fork_tree(); - let removed = tree.prune(&"c", &3, &is_descendent_of, &|_| true).unwrap(); + let removed = tree.prune(&"c", &25, &is_descendent_of, &|_| true).unwrap(); assert_eq!(tree.roots.iter().map(|node| node.hash).collect::>(), vec!["B"]); @@ -1378,19 +1381,16 @@ mod test { fn prune_works3() { let (mut tree, is_descendent_of) = test_fork_tree(); - let flag = std::cell::RefCell::new(false); - let removed = tree.prune(&"m", &5, &is_descendent_of, &|_| flag.replace(true)).unwrap(); + // This is to re-root not at the immediate ancestor, but the one before. + let removed = tree.prune(&"m", &45, &is_descendent_of, &|height| *height == 3).unwrap(); assert_eq!(tree.roots.iter().map(|node| node.hash).collect::>(), vec!["H"]); - assert_eq!( - tree.iter().map(|(hash, _, _)| *hash).collect::>(), - vec!["H", "L", "M", "O"], - ); + assert_eq!(tree.iter().map(|(hash, _, _)| *hash).collect::>(), vec!["H", "L", "M"],); assert_eq!( removed.map(|(hash, _, _)| hash).collect::>(), - vec!["I", "A", "B", "C", "D", "E", "F", "G", "J", "K"] + vec!["O", "I", "A", "B", "C", "D", "E", "F", "G", "J", "K"] ); } @@ -1398,26 +1398,34 @@ mod test { fn prune_works4() { let (mut tree, is_descendent_of) = test_fork_tree(); - let counter = std::cell::RefCell::new(0); - let removed = tree - .prune(&"m", &5, &is_descendent_of, &|_| { - let mut counter = counter.borrow_mut(); - let prev = *counter; - *counter += 1; - prev == 2 - }) - .unwrap(); + let removed = tree.prune(&"m", &45, &is_descendent_of, &|height| *height == 2).unwrap(); assert_eq!(tree.roots.iter().map(|node| node.hash).collect::>(), vec!["F"]); assert_eq!( tree.iter().map(|(hash, _, _)| *hash).collect::>(), - vec!["F", "H", "L", "M", "O"], + vec!["F", "H", "L", "M"], + ); + + assert_eq!( + removed.map(|(hash, _, _)| hash).collect::>(), + vec!["O", "I", "G", "A", "B", "C", "D", "E", "J", "K"] ); + } + + #[test] + fn prune_works5() { + let (mut tree, is_descendent_of) = test_fork_tree(); + + let removed = tree.prune(&"M", &50, &is_descendent_of, &|_| true).unwrap(); + + assert_eq!(tree.roots.iter().map(|node| node.hash).collect::>(), vec!["L"]); + + assert_eq!(tree.iter().map(|(hash, _, _)| *hash).collect::>(), vec!["L", "M"],); assert_eq!( removed.map(|(hash, _, _)| hash).collect::>(), - vec!["I", "G", "A", "B", "C", "D", "E", "J", "K"] + vec!["O", "A", "B", "C", "D", "E", "F", "H", "I", "G", "J", "K"] ); } @@ -1470,7 +1478,7 @@ mod test { } }; - tree.import("P", 6, (), &is_descendent_of).unwrap(); + tree.import("P", 60, 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 @@ -1485,7 +1493,7 @@ mod test { fn drain_filter_works() { let (mut tree, _) = test_fork_tree(); - let filter = |h: &&str, _: &u64, _: &()| match *h { + let filter = |h: &&str, _: &u64, _: &u32| match *h { "A" | "B" | "F" | "G" => FilterAction::KeepNode, "C" => FilterAction::KeepTree, "H" | "J" => FilterAction::Remove, @@ -1510,19 +1518,19 @@ mod test { let (tree, is_descendent_of) = test_fork_tree(); let path = tree - .find_node_index_where(&"D", &4, &is_descendent_of, &|_| true) + .find_node_index_where(&"D", &40, &is_descendent_of, &|_| true) .unwrap() .unwrap(); assert_eq!(path, [0, 0, 0]); let path = tree - .find_node_index_where(&"O", &5, &is_descendent_of, &|_| true) + .find_node_index_where(&"O", &50, &is_descendent_of, &|_| true) .unwrap() .unwrap(); assert_eq!(path, [0, 1, 0, 0]); let path = tree - .find_node_index_where(&"N", &6, &is_descendent_of, &|_| true) + .find_node_index_where(&"N", &60, &is_descendent_of, &|_| true) .unwrap() .unwrap(); assert_eq!(path, [0, 1, 0, 0, 0]); @@ -1578,17 +1586,17 @@ 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(&"B", &20, &is_descendent_of, &|_| true).unwrap().unwrap(); + assert_eq!((node.hash, node.number), ("A", 10)); - let node = tree.find_node_where(&"D", &4, &is_descendent_of, &|_| true).unwrap().unwrap(); - assert_eq!((node.hash, node.number), ("C", 3)); + let node = tree.find_node_where(&"D", &40, &is_descendent_of, &|_| true).unwrap().unwrap(); + assert_eq!((node.hash, node.number), ("C", 30)); - let node = tree.find_node_where(&"O", &5, &is_descendent_of, &|_| true).unwrap().unwrap(); - assert_eq!((node.hash, node.number), ("L", 4)); + let node = tree.find_node_where(&"O", &50, &is_descendent_of, &|_| true).unwrap().unwrap(); + assert_eq!((node.hash, node.number), ("L", 40)); - let node = tree.find_node_where(&"N", &6, &is_descendent_of, &|_| true).unwrap().unwrap(); - assert_eq!((node.hash, node.number), ("M", 5)); + let node = tree.find_node_where(&"N", &60, &is_descendent_of, &|_| true).unwrap().unwrap(); + assert_eq!((node.hash, node.number), ("M", 50)); } #[test] @@ -1605,13 +1613,13 @@ mod test { // Post order traversal requirement for `find_node_index_where` let path = tree - .find_node_index_where(&"N", &6, &is_descendent_of_for_post_order, &|_| true) + .find_node_index_where(&"N", &60, &is_descendent_of_for_post_order, &|_| true) .unwrap() .unwrap(); assert_eq!(path, [0, 1, 0, 0, 0]); // Post order traversal requirement for `import` - let res = tree.import(&"Z", 100, (), &is_descendent_of_for_post_order); + let res = tree.import(&"Z", 100, 10, &is_descendent_of_for_post_order); assert_eq!(res, Ok(false)); assert_eq!( tree.iter().map(|node| *node.0).collect::>(), From 96db169a7052fb5a20255ac424e193044c0021f2 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Sat, 17 Dec 2022 15:05:00 +0100 Subject: [PATCH 3/7] Fork tree prune refactory --- utils/fork-tree/src/lib.rs | 50 +++++++++++++++++++------------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index 254f06cc32503..7df4822ddc90a 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -382,21 +382,20 @@ where None => return Ok(RemovedIterator { stack: Vec::new() }), }; - let mut old_roots = std::mem::take(&mut self.roots); + let mut removed = std::mem::take(&mut self.roots); // Find and detach the new root from the removed nodes - let curr_children = root_index + let root_siblings = root_index .iter() .take(root_index.len() - 1) - .fold(&mut old_roots, |curr, idx| &mut curr[*idx].children); - let mut root = curr_children.remove(root_index[root_index.len() - 1]); - - let mut removed = old_roots; + .fold(&mut removed, |curr, idx| &mut curr[*idx].children); + let root = root_siblings.remove(root_index[root_index.len() - 1]); + self.roots = vec![root]; - // If (because of the `predicate`) the new root is not the deepest + // If, because of the `predicate`, the new root is not the deepest // ancestor of `hash` then we can remove all the nodes that are - // descendents of the new `root` but not ancestors of `hash`. - let mut curr = &mut root; + // descendants of the new `root` but not ancestors of `hash`. + let mut curr = &mut self.roots[0]; loop { let mut next_ancestor_idx = usize::MAX; for (idx, child) in curr.children.iter().enumerate() { @@ -411,28 +410,29 @@ where // We are above all current node children, stop searching break } - // Preserve only the valid ancestor node - let mut curr_children = std::mem::take(&mut curr.children); - let keep_node = curr_children.remove(next_ancestor_idx); - removed.append(&mut curr_children); - curr.children = vec![keep_node]; + // Preserve only the ancestor node, the siblings are removed + let mut next_siblings = std::mem::take(&mut curr.children); + let next = next_siblings.remove(next_ancestor_idx); + curr.children = vec![next]; + removed.append(&mut next_siblings); curr = &mut curr.children[0]; } - // Curr now points to our direct ancestor, - // remove all additional stale nodes - let children = std::mem::take(&mut curr.children); - for child in children { - if child.number == *number && child.hash == *hash || - *number < child.number && is_descendent_of(hash, &child.hash)? - { - curr.children.push(child); - } else { - removed.push(child); + // Curr now points to our direct ancestor, if necessary remove any node that is + // not a descendant of `hash`. + if curr.children.len() > 0 { + let children = std::mem::take(&mut curr.children); + for child in children { + if child.number == *number && child.hash == *hash || + *number < child.number && is_descendent_of(hash, &child.hash)? + { + curr.children.push(child); + } else { + removed.push(child); + } } } - self.roots = vec![root]; self.rebalance(); Ok(RemovedIterator { stack: removed }) From f59fcc27a11de5d931d08da6c8f7ba740064b64b Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 7 Feb 2023 16:40:00 +0100 Subject: [PATCH 4/7] Code refactory --- utils/fork-tree/src/lib.rs | 67 +++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 33 deletions(-) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index 7df4822ddc90a..353d3753a100e 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -32,7 +32,7 @@ pub enum Error { UnfinalizedAncestor, /// Imported or finalized node that is an ancestor of previously finalized node. Revert, - /// Error throw by user when checking for node ancestry. + /// Error thrown by user when checking for node ancestry. Client(E), } @@ -376,43 +376,41 @@ where F: Fn(&H, &H) -> Result, P: Fn(&V) -> bool, { - let root_index = + let new_root_path = match self.find_node_index_where(hash, number, is_descendent_of, predicate)? { - Some(idx) => idx, + Some(path) => path, None => return Ok(RemovedIterator { stack: Vec::new() }), }; let mut removed = std::mem::take(&mut self.roots); // Find and detach the new root from the removed nodes - let root_siblings = root_index + let root_siblings = new_root_path .iter() - .take(root_index.len() - 1) + .take(new_root_path.len() - 1) .fold(&mut removed, |curr, idx| &mut curr[*idx].children); - let root = root_siblings.remove(root_index[root_index.len() - 1]); + let root = root_siblings.remove(new_root_path[new_root_path.len() - 1]); self.roots = vec![root]; - // If, because of the `predicate`, the new root is not the deepest - // ancestor of `hash` then we can remove all the nodes that are - // descendants of the new `root` but not ancestors of `hash`. + // If, because of the `predicate`, the new root is not the deepest ancestor + // of `hash` then we can remove all the nodes that are descendants of the new + // `root` but not ancestors of `hash`. let mut curr = &mut self.roots[0]; loop { - let mut next_ancestor_idx = usize::MAX; + let mut maybe_ancestor_idx = None; for (idx, child) in curr.children.iter().enumerate() { - if child.number == *number && child.hash == *hash || - child.number < *number && is_descendent_of(&child.hash, hash)? - { - next_ancestor_idx = idx; + if child.number < *number && is_descendent_of(&child.hash, hash)? { + maybe_ancestor_idx = Some(idx); break } } - if next_ancestor_idx == usize::MAX { - // We are above all current node children, stop searching + let Some(ancestor_idx) = maybe_ancestor_idx else { + // Now we are positioned just above block identified by `hash` break - } + }; // Preserve only the ancestor node, the siblings are removed let mut next_siblings = std::mem::take(&mut curr.children); - let next = next_siblings.remove(next_ancestor_idx); + let next = next_siblings.remove(ancestor_idx); curr.children = vec![next]; removed.append(&mut next_siblings); curr = &mut curr.children[0]; @@ -420,16 +418,19 @@ where // Curr now points to our direct ancestor, if necessary remove any node that is // not a descendant of `hash`. - if curr.children.len() > 0 { - let children = std::mem::take(&mut curr.children); - for child in children { - if child.number == *number && child.hash == *hash || - *number < child.number && is_descendent_of(hash, &child.hash)? - { - curr.children.push(child); - } else { - removed.push(child); - } + let mut is_first = true; + let children = std::mem::take(&mut curr.children); + for child in children { + if is_first && + (child.number == *number && child.hash == *hash || + *number < child.number && is_descendent_of(hash, &child.hash)?) + { + curr.children.push(child); + // Assuming that the tree is well formed only one child should pass this + // requirement due to ancestry restrictions (i.e. they must be different forks). + is_first = false; + } else { + removed.push(child); } } @@ -1332,7 +1333,7 @@ mod test { } #[test] - fn prune_works() { + fn prune_works_for_in_tree_hashes() { let (mut tree, is_descendent_of) = test_fork_tree(); let removed = tree.prune(&"C", &30, &is_descendent_of, &|_| true).unwrap(); @@ -1359,7 +1360,7 @@ mod test { } #[test] - fn prune_works2() { + fn prune_works_for_out_of_tree_hashes() { let (mut tree, is_descendent_of) = test_fork_tree(); let removed = tree.prune(&"c", &25, &is_descendent_of, &|_| true).unwrap(); @@ -1378,10 +1379,10 @@ mod test { } #[test] - fn prune_works3() { + fn prune_works_for_distant_ancestor() { let (mut tree, is_descendent_of) = test_fork_tree(); - // This is to re-root not at the immediate ancestor, but the one before. + // This is to re-root the tree not at the immediate ancestor, but the one just before. let removed = tree.prune(&"m", &45, &is_descendent_of, &|height| *height == 3).unwrap(); assert_eq!(tree.roots.iter().map(|node| node.hash).collect::>(), vec!["H"]); @@ -1395,7 +1396,7 @@ mod test { } #[test] - fn prune_works4() { + fn prune_works_for_far_away_ancestor() { let (mut tree, is_descendent_of) = test_fork_tree(); let removed = tree.prune(&"m", &45, &is_descendent_of, &|height| *height == 2).unwrap(); From 4c00660c6acb542e2af124fd3323f59e9dcceaa5 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 7 Feb 2023 16:59:05 +0100 Subject: [PATCH 5/7] Correctly handle borderline, but legit, case --- utils/fork-tree/src/lib.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index 353d3753a100e..fd44b30553859 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -418,17 +418,12 @@ where // Curr now points to our direct ancestor, if necessary remove any node that is // not a descendant of `hash`. - let mut is_first = true; let children = std::mem::take(&mut curr.children); for child in children { - if is_first && - (child.number == *number && child.hash == *hash || - *number < child.number && is_descendent_of(hash, &child.hash)?) + if child.number == *number && child.hash == *hash || + *number < child.number && is_descendent_of(hash, &child.hash)? { curr.children.push(child); - // Assuming that the tree is well formed only one child should pass this - // requirement due to ancestry restrictions (i.e. they must be different forks). - is_first = false; } else { removed.push(child); } From 96cb1e2f2954028dee0247cff03b819f3e0a258a Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 7 Feb 2023 17:39:48 +0100 Subject: [PATCH 6/7] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> --- utils/fork-tree/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index fd44b30553859..4c7ea01fce8ae 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -100,7 +100,7 @@ where ForkTree { roots: Vec::new(), best_finalized_number: None } } - /// Rebalance the tree + /// Rebalance the tree. /// /// For each tree level sort child nodes by max branch depth (decreasing). /// From a7d6309a819697ad2021acf80008f3c54908faaa Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 7 Feb 2023 17:41:20 +0100 Subject: [PATCH 7/7] Removed duplicated test --- utils/fork-tree/src/lib.rs | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index 4c7ea01fce8ae..bbd0daca4a28d 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -1374,7 +1374,7 @@ mod test { } #[test] - fn prune_works_for_distant_ancestor() { + fn prune_works_for_not_direct_ancestor() { let (mut tree, is_descendent_of) = test_fork_tree(); // This is to re-root the tree not at the immediate ancestor, but the one just before. @@ -1409,22 +1409,6 @@ mod test { ); } - #[test] - fn prune_works5() { - let (mut tree, is_descendent_of) = test_fork_tree(); - - let removed = tree.prune(&"M", &50, &is_descendent_of, &|_| true).unwrap(); - - assert_eq!(tree.roots.iter().map(|node| node.hash).collect::>(), vec!["L"]); - - assert_eq!(tree.iter().map(|(hash, _, _)| *hash).collect::>(), vec!["L", "M"],); - - assert_eq!( - removed.map(|(hash, _, _)| hash).collect::>(), - vec!["O", "A", "B", "C", "D", "E", "F", "H", "I", "G", "J", "K"] - ); - } - #[test] fn find_node_backtracks_after_finding_highest_descending_node() { let mut tree = ForkTree::new();