From dca037d4c9199388157f9e7c561e0884bb452d00 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 5 Apr 2022 18:46:47 +0200 Subject: [PATCH 01/19] Iterative version of some fork-tree methods --- utils/fork-tree/src/lib.rs | 288 ++++++++++++++++++++++--------------- 1 file changed, 172 insertions(+), 116 deletions(-) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index 1d9b39f7dc04b..ea1d8ce630ff5 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -122,26 +122,14 @@ where let new_root_index = self.find_node_index_where(hash, number, is_descendent_of, predicate)?; - let removed = if let Some(mut root_index) = new_root_index { + let removed = if let Some(root_index) = new_root_index { let mut old_roots = std::mem::take(&mut self.roots); - let mut root = None; - let mut cur_children = Some(&mut old_roots); - - while let Some(cur_index) = root_index.pop() { - if let Some(children) = cur_children.take() { - if root_index.is_empty() { - root = Some(children.remove(cur_index)); - } else { - cur_children = Some(&mut children[cur_index].children); - } - } - } - - let mut root = root.expect( - "find_node_index_where will return array with at least one index; \ - this results in at least one item in removed; qed", - ); + let cur_children = root_index + .iter() + .take(root_index.len() - 1) + .fold(&mut old_roots, |curr, idx| &mut curr[*idx].children); + let mut root = cur_children.remove(root_index[root_index.len() - 1]); let mut removed = old_roots; @@ -198,8 +186,11 @@ where /// longest ones). pub fn rebalance(&mut self) { self.roots.sort_by_key(|n| Reverse(n.max_depth())); - for root in &mut self.roots { - root.rebalance(); + let mut stack = Vec::new(); + self.roots.iter_mut().for_each(|e| stack.push(e)); + while let Some(node) = stack.pop() { + node.children.sort_by_key(|n| Reverse(n.max_depth())); + node.children.iter_mut().for_each(|n| stack.push(n)); } } @@ -267,6 +258,16 @@ where self.node_iter().map(|node| (&node.hash, &node.number, &node.data)) } + /// Map fork tree into values of new types. + pub fn map(self, f: &mut F) -> ForkTree + where + F: FnMut(&H, &N, V) -> VT, + { + let roots = self.roots.into_iter().map(|root| root.map(f)).collect(); + + ForkTree { roots, best_finalized_number: self.best_finalized_number } + } + /// 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) @@ -296,16 +297,6 @@ where Ok(None) } - /// Map fork tree into values of new types. - pub fn map(self, f: &mut F) -> ForkTree - where - F: FnMut(&H, &N, V) -> VT, - { - let roots = self.roots.into_iter().map(|root| root.map(f)).collect(); - - ForkTree { roots, best_finalized_number: self.best_finalized_number } - } - /// Same as [`find_node_where`](ForkTree::find_node_where), but returns mutable reference. pub fn find_node_where_mut( &mut self, @@ -346,16 +337,15 @@ where P: Fn(&V) -> bool, { // search for node starting from all roots - for (index, root) in self.roots.iter().enumerate() { - let node = root.find_node_index_where(hash, number, is_descendent_of, predicate)?; + for (idx, root) in self.roots.iter().enumerate() { + let path = root.find_node_index_where(hash, number, is_descendent_of, predicate)?; // found the node, early exit - if let FindOutcome::Found(mut node) = node { - node.push(index); - return Ok(Some(node)) + if let FindOutcome::Found(mut path) = path { + path.insert(0, idx); + return Ok(Some(path)) } } - Ok(None) } @@ -684,23 +674,17 @@ mod node_implementation { } impl Node { - /// Rebalance the tree, i.e. sort child nodes by max branch depth (decreasing). - pub fn rebalance(&mut self) { - self.children.sort_by_key(|n| Reverse(n.max_depth())); - for child in &mut self.children { - child.rebalance(); - } - } - /// Finds the max depth among all branches descendent from this node. pub fn max_depth(&self) -> usize { - let mut max = 0; - - for node in &self.children { - max = node.max_depth().max(max) + let mut max: usize = 0; + let mut stack = vec![(self, 1)]; + while let Some((n, height)) = stack.pop() { + if height > max { + max = height; + } + n.children.iter().for_each(|n| stack.push((n, height + 1))); } - - max + 1 + max } /// Map node data into values of new types. @@ -753,6 +737,25 @@ mod node_implementation { } } + // pub fn import_iter( + // &mut self, + // mut hash: H, + // mut number: N, + // mut data: V, + // is_descendent_of: &F, + // ) -> Result, Error> + // where + // E: fmt::Debug, + // F: Fn(&H, &H) -> Result, + // { + // let stack = vec![self]; + // while let Some(node) = stack.pop() { + // if node.hash == hash { + // return Err(Error::Duplicate); + // } + // } + // } + /// Find a node in the tree that is the deepest ancestor of the given /// block hash which also passes the given predicate, backtracking /// when the predicate fails. @@ -762,6 +765,62 @@ mod node_implementation { /// The returned indices are from last to first. The earliest index in the traverse path /// goes last, and the final index in the traverse path goes first. An empty list means /// that the current node is the result. + // pub fn find_node_index_where( + // &self, + // hash: &H, + // number: &N, + // is_descendent_of: &F, + // predicate: &P, + // ) -> Result>, Error> + // where + // E: std::error::Error, + // F: Fn(&H, &H) -> Result, + // P: Fn(&V) -> bool, + // { + // // stop searching this branch + // if *number < self.number { + // return Ok(FindOutcome::Failure(false)) + // } + + // let mut known_descendent_of = false; + + // // continue depth-first search through all children + // for (i, node) in self.children.iter().enumerate() { + // // found node, early exit + // match node.find_node_index_where_iter(hash, number, is_descendent_of, predicate)? { + // FindOutcome::Abort => return Ok(FindOutcome::Abort), + // FindOutcome::Found(mut x) => { + // x.push(i); + // return Ok(FindOutcome::Found(x)) + // }, + // FindOutcome::Failure(true) => { + // // if the block was a descendent of this child, + // // then it cannot be a descendent of any others, + // // so we don't search them. + // known_descendent_of = true; + // break + // }, + // FindOutcome::Failure(false) => {}, + // } + // } + + // // node not found in any of the descendents, if the node we're + // // searching for is a descendent of this node then we will stop the + // // search here, since there aren't any more children and we found + // // the correct node so we don't want to backtrack. + // let is_descendent_of = known_descendent_of || is_descendent_of(&self.hash, hash)?; + // if is_descendent_of { + // // if the predicate passes we return the node + // if predicate(&self.data) { + // return Ok(FindOutcome::Found(Vec::new())) + // } + // } + + // // otherwise, tell our ancestor that we failed, and whether + // // the block was a descendent. + // Ok(FindOutcome::Failure(is_descendent_of)) + // } + pub fn find_node_index_where( &self, hash: &H, @@ -774,48 +833,32 @@ mod node_implementation { F: Fn(&H, &H) -> Result, P: Fn(&V) -> bool, { - // stop searching this branch - if *number < self.number { - return Ok(FindOutcome::Failure(false)) - } - - let mut known_descendent_of = false; - - // continue depth-first search through all children - for (i, node) in self.children.iter().enumerate() { - // found node, early exit - match node.find_node_index_where(hash, number, is_descendent_of, predicate)? { - FindOutcome::Abort => return Ok(FindOutcome::Abort), - FindOutcome::Found(mut x) => { - x.push(i); - return Ok(FindOutcome::Found(x)) - }, - FindOutcome::Failure(true) => { - // if the block was a descendent of this child, - // then it cannot be a descendent of any others, - // so we don't search them. - known_descendent_of = true; + let mut curr = self; + let mut path = vec![]; + loop { + let mut pushed = false; + for (idx, child) in curr.children.iter().enumerate() { + if child.number < *number && is_descendent_of(&child.hash, hash)? { + curr = child; + path.push(idx); + pushed = true; break - }, - FindOutcome::Failure(false) => {}, + } + } + if !pushed { + break } } - // node not found in any of the descendents, if the node we're - // searching for is a descendent of this node then we will stop the - // search here, since there aren't any more children and we found - // the correct node so we don't want to backtrack. - let is_descendent_of = known_descendent_of || is_descendent_of(&self.hash, hash)?; - if is_descendent_of { - // if the predicate passes we return the node - if predicate(&self.data) { - return Ok(FindOutcome::Found(Vec::new())) + if curr.hash == *hash || is_descendent_of(&curr.hash, hash)? { + if predicate(&curr.data) { + Ok(FindOutcome::Found(path)) + } else { + Ok(FindOutcome::Failure(true)) } + } else { + Ok(FindOutcome::Failure(false)) } - - // otherwise, tell our ancestor that we failed, and whether - // the block was a descendent. - Ok(FindOutcome::Failure(is_descendent_of)) } /// Find a node in the tree that is the deepest ancestor of the given @@ -840,13 +883,9 @@ mod node_implementation { match outcome { FindOutcome::Abort => Ok(FindOutcome::Abort), FindOutcome::Failure(f) => Ok(FindOutcome::Failure(f)), - FindOutcome::Found(mut indexes) => { - let mut cur = self; - - while let Some(i) = indexes.pop() { - cur = &cur.children[i]; - } - Ok(FindOutcome::Found(cur)) + FindOutcome::Found(path) => { + let node = path.iter().fold(self, |curr, i| &curr.children[*i]); + Ok(FindOutcome::Found(node)) }, } } @@ -873,13 +912,9 @@ mod node_implementation { match outcome { FindOutcome::Abort => Ok(FindOutcome::Abort), FindOutcome::Failure(f) => Ok(FindOutcome::Failure(f)), - FindOutcome::Found(mut indexes) => { - let mut cur = self; - - while let Some(i) = indexes.pop() { - cur = &mut cur.children[i]; - } - Ok(FindOutcome::Found(cur)) + FindOutcome::Found(path) => { + let node = path.iter().fold(self, |curr, i| &mut curr.children[*i]); + Ok(FindOutcome::Found(node)) }, } } @@ -984,7 +1019,7 @@ mod test { // / / // A - F - H - I // \ \ - // \ - L - M + // \ - L - M - (N) // \ \ // \ - O // - J - K @@ -995,7 +1030,7 @@ mod test { // 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 { - let letters = vec!["B", "C", "D", "E", "F", "G", "H", "I", "J", "K", "L", "M", "O"]; + let letters = vec!["B", "C", "D", "E", "F", "G", "H", "I", "J", "K", "L", "M", "N", "O"]; match (*base, *block) { ("A", b) => Ok(letters.into_iter().any(|n| n == b)), ("B", b) => Ok(b == "C" || b == "D" || b == "E"), @@ -1003,14 +1038,14 @@ mod test { ("D", b) => Ok(b == "E"), ("E", _) => Ok(false), ("F", b) => - Ok(b == "G" || b == "H" || b == "I" || b == "L" || b == "M" || b == "O"), + Ok(b == "G" || b == "H" || b == "I" || b == "L" || b == "M" || b == "N" || b == "O"), ("G", _) => Ok(false), - ("H", b) => Ok(b == "I" || b == "L" || b == "M" || b == "O"), + ("H", b) => Ok(b == "I" || b == "L" || b == "M" || b == "N" || b == "O"), ("I", _) => Ok(false), ("J", b) => Ok(b == "K"), ("K", _) => Ok(false), - ("L", b) => Ok(b == "M" || b == "O"), - ("M", _) => Ok(false), + ("L", b) => Ok(b == "M" || b == "O" || b == "N"), + ("M", b) => Ok(b == "N"), ("O", _) => Ok(false), ("0", _) => Ok(true), _ => Ok(false), @@ -1419,16 +1454,6 @@ mod test { } } - #[test] - fn find_node_works() { - let (tree, is_descendent_of) = test_fork_tree(); - - let node = tree.find_node_where(&"D", &4, &is_descendent_of, &|_| true).unwrap().unwrap(); - - assert_eq!(node.hash, "C"); - assert_eq!(node.number, 3); - } - #[test] fn map_works() { let (tree, _is_descendent_of) = test_fork_tree(); @@ -1546,4 +1571,35 @@ mod test { ["J", "K", "H", "L", "M", "O", "I"] ); } + + #[test] + fn find_node_index_works() { + let (tree, is_descendent_of) = test_fork_tree(); + + let path = tree + .find_node_index_where(&"O", &5, &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) + .unwrap() + .unwrap(); + assert_eq!(path, [0, 1, 0, 0, 0]); + } + + #[test] + fn find_node_works() { + let (tree, is_descendent_of) = test_fork_tree(); + + 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(&"O", &5, &is_descendent_of, &|_| true).unwrap().unwrap(); + assert_eq!((node.hash, node.number), ("L", 4)); + + let node = tree.find_node_where(&"N", &6, &is_descendent_of, &|_| true).unwrap().unwrap(); + assert_eq!((node.hash, node.number), ("M", 5)); + } } From 1e746fc3d544714f3128e57e70742acdff1b7c74 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Wed, 6 Apr 2022 19:06:25 +0200 Subject: [PATCH 02/19] Prune doesn't require its generic args to be 'clone' --- utils/fork-tree/src/lib.rs | 137 ++++++++++++++++++------------------- 1 file changed, 65 insertions(+), 72 deletions(-) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index ea1d8ce630ff5..b8a2a81eb6d8b 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -93,78 +93,6 @@ pub struct ForkTree { best_finalized_number: Option, } -impl ForkTree -where - H: PartialEq + Clone, - N: Ord + Clone, - V: Clone, -{ - /// 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). - /// - /// Returns all pruned node data. - pub fn prune( - &mut self, - hash: &H, - number: &N, - is_descendent_of: &F, - predicate: &P, - ) -> Result, Error> - where - E: std::error::Error, - F: Fn(&H, &H) -> Result, - P: Fn(&V) -> bool, - { - let new_root_index = - self.find_node_index_where(hash, number, is_descendent_of, predicate)?; - - let removed = if let Some(root_index) = new_root_index { - let mut old_roots = std::mem::take(&mut self.roots); - - let cur_children = root_index - .iter() - .take(root_index.len() - 1) - .fold(&mut old_roots, |curr, idx| &mut curr[*idx].children); - let mut root = cur_children.remove(root_index[root_index.len() - 1]); - - 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; - - for child in root_children { - if is_first && - (child.number == *number && child.hash == *hash || - child.number < *number && is_descendent_of(&child.hash, 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; - } else { - removed.push(child); - } - } - - self.roots = vec![root]; - - removed - } else { - Vec::new() - }; - - self.rebalance(); - - Ok(RemovedIterator { stack: removed }) - } -} - impl ForkTree where H: PartialEq, @@ -349,6 +277,71 @@ where Ok(None) } + /// 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). + /// + /// Returns all pruned node data. + pub fn prune( + &mut self, + hash: &H, + number: &N, + is_descendent_of: &F, + predicate: &P, + ) -> Result, Error> + where + E: std::error::Error, + F: Fn(&H, &H) -> Result, + P: Fn(&V) -> bool, + { + let new_root_index = + self.find_node_index_where(hash, number, is_descendent_of, predicate)?; + + let removed = if let Some(root_index) = new_root_index { + let mut old_roots = std::mem::take(&mut self.roots); + + let curr_children = 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; + + // 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; + + for child in root_children { + if is_first && + (child.number == *number && child.hash == *hash || + child.number < *number && is_descendent_of(&child.hash, 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; + } else { + removed.push(child); + } + } + + self.roots = vec![root]; + + removed + } else { + Vec::new() + }; + + self.rebalance(); + + Ok(RemovedIterator { stack: removed }) + } + /// Finalize a root in the tree and return it, return `None` in case no root /// with the given hash exists. All other roots are pruned, and the children /// of the finalized node become the new roots. From 23cc533c573170856a8093b5628201cf07fe02c0 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Thu, 14 Apr 2022 17:21:36 +0200 Subject: [PATCH 03/19] Fork-tree import and drain-filter iterative implementations --- utils/fork-tree/src/lib.rs | 210 ++++++++++++++----------------------- 1 file changed, 80 insertions(+), 130 deletions(-) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index b8a2a81eb6d8b..850625f6e73d3 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -103,19 +103,16 @@ where ForkTree { roots: Vec::new(), best_finalized_number: None } } - /// Rebalance the tree, i.e. sort child nodes by max branch depth - /// (decreasing). + /// Rebalance the tree, i.e. 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 /// to be used in a blockchain context, a good heuristic is that the node - /// we'll be looking - /// for at any point will likely be in one of the deepest chains (i.e. the - /// longest ones). + /// we'll be looking for at any point will likely be in one of the deepest chains + /// (i.e. the longest ones). pub fn rebalance(&mut self) { self.roots.sort_by_key(|n| Reverse(n.max_depth())); - let mut stack = Vec::new(); - self.roots.iter_mut().for_each(|e| stack.push(e)); + let mut stack: Vec<_> = self.roots.iter_mut().collect(); while let Some(node) = stack.pop() { node.children.sort_by_key(|n| Reverse(n.max_depth())); node.children.iter_mut().for_each(|n| stack.push(n)); @@ -128,7 +125,7 @@ where /// imported in order. /// /// Returns `true` if the imported node is a root. - pub fn import( + pub fn old_import( &mut self, mut hash: H, mut number: N, @@ -170,6 +167,53 @@ where Ok(true) } + /// 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. + pub fn import( + &mut self, + hash: H, + number: N, + data: V, + is_descendent_of: &F, + ) -> Result> + where + E: std::error::Error, + F: Fn(&H, &H) -> Result, + { + if let Some(ref best_finalized_number) = self.best_finalized_number { + if number <= *best_finalized_number { + return Err(Error::Revert) + } + } + + let mut children = &mut self.roots; + + let mut i = 0; + let mut len = children.len(); + while i < len { + let child = &children[i]; + if child.hash == hash { + return Err(Error::Duplicate) + } + if child.number < number && is_descendent_of(&child.hash, &hash)? { + children = &mut children[i].children; + len = children.len(); + i = 0; + continue + } + i += 1; + } + + children.push(Node { data, hash, number, children: Default::default() }); + self.rebalance(); + + Ok(true) + } + /// Iterates over the existing roots in the tree. pub fn roots(&self) -> impl Iterator { self.roots.iter().map(|node| (&node.hash, &node.number, &node.data)) @@ -629,15 +673,33 @@ where where F: FnMut(&H, &N, &V) -> FilterAction, { - let mut removed = Vec::new(); - let mut i = 0; - while i < self.roots.len() { - if self.roots[i].drain_filter(&mut filter, &mut removed) { - removed.push(self.roots.remove(i)); - } else { - i += 1; + let mut stack: Vec<*mut Node> = vec![]; + let mut children = &mut self.roots; + let mut removed = vec![]; + + loop { + for node in std::mem::take(children) { + match filter(&node.hash, &node.number, &node.data) { + FilterAction::KeepNode => { + let i = children.len(); + children.push(node); + stack.push(&mut children[i] as *mut _); + }, + FilterAction::KeepTree => { + children.push(node); + }, + FilterAction::Remove => { + removed.push(node); + }, + } } + + children = match stack.pop() { + Some(n) => unsafe { &mut (*n).children }, + None => break, + }; } + self.rebalance(); RemovedIterator { stack: removed } } @@ -730,90 +792,6 @@ mod node_implementation { } } - // pub fn import_iter( - // &mut self, - // mut hash: H, - // mut number: N, - // mut data: V, - // is_descendent_of: &F, - // ) -> Result, Error> - // where - // E: fmt::Debug, - // F: Fn(&H, &H) -> Result, - // { - // let stack = vec![self]; - // while let Some(node) = stack.pop() { - // if node.hash == hash { - // return Err(Error::Duplicate); - // } - // } - // } - - /// Find a node in the tree that is the deepest ancestor of the given - /// block hash which also passes the given predicate, backtracking - /// when the predicate fails. - /// The given function `is_descendent_of` should return `true` if the second hash (target) - /// is a descendent of the first hash (base). - /// - /// The returned indices are from last to first. The earliest index in the traverse path - /// goes last, and the final index in the traverse path goes first. An empty list means - /// that the current node is the result. - // pub fn find_node_index_where( - // &self, - // hash: &H, - // number: &N, - // is_descendent_of: &F, - // predicate: &P, - // ) -> Result>, Error> - // where - // E: std::error::Error, - // F: Fn(&H, &H) -> Result, - // P: Fn(&V) -> bool, - // { - // // stop searching this branch - // if *number < self.number { - // return Ok(FindOutcome::Failure(false)) - // } - - // let mut known_descendent_of = false; - - // // continue depth-first search through all children - // for (i, node) in self.children.iter().enumerate() { - // // found node, early exit - // match node.find_node_index_where_iter(hash, number, is_descendent_of, predicate)? { - // FindOutcome::Abort => return Ok(FindOutcome::Abort), - // FindOutcome::Found(mut x) => { - // x.push(i); - // return Ok(FindOutcome::Found(x)) - // }, - // FindOutcome::Failure(true) => { - // // if the block was a descendent of this child, - // // then it cannot be a descendent of any others, - // // so we don't search them. - // known_descendent_of = true; - // break - // }, - // FindOutcome::Failure(false) => {}, - // } - // } - - // // node not found in any of the descendents, if the node we're - // // searching for is a descendent of this node then we will stop the - // // search here, since there aren't any more children and we found - // // the correct node so we don't want to backtrack. - // let is_descendent_of = known_descendent_of || is_descendent_of(&self.hash, hash)?; - // if is_descendent_of { - // // if the predicate passes we return the node - // if predicate(&self.data) { - // return Ok(FindOutcome::Found(Vec::new())) - // } - // } - - // // otherwise, tell our ancestor that we failed, and whether - // // the block was a descendent. - // Ok(FindOutcome::Failure(is_descendent_of)) - // } - pub fn find_node_index_where( &self, hash: &H, @@ -911,34 +889,6 @@ mod node_implementation { }, } } - - /// Calls a `filter` predicate for the given node. - /// The `filter` is called over tree nodes and returns a filter action: - /// - `Remove` if the node and its subtree should be removed; - /// - `KeepNode` if we should maintain the node and keep processing the tree; - /// - `KeepTree` if we should maintain the node and its entire subtree. - /// Pruned subtrees are added to the `removed` list. - /// Returns a booleans indicateing if this node (and its subtree) should be removed. - pub fn drain_filter(&mut self, filter: &mut F, removed: &mut Vec>) -> bool - where - F: FnMut(&H, &N, &V) -> FilterAction, - { - match filter(&self.hash, &self.number, &self.data) { - FilterAction::KeepNode => { - let mut i = 0; - while i < self.children.len() { - if self.children[i].drain_filter(filter, removed) { - removed.push(self.children.remove(i)); - } else { - i += 1; - } - } - false - }, - FilterAction::KeepTree => false, - FilterAction::Remove => true, - } - } } } @@ -1276,7 +1226,7 @@ mod test { // // Nodes B, C, F and G are not part of the tree. match (*base, *block) { - ("A0", b) => Ok(b == "B" || b == "C" || b == "D" || b == "G"), + ("A0", b) => Ok(b == "B" || b == "C" || b == "D" || b == "E" || b == "G"), ("A1", _) => Ok(false), ("C", b) => Ok(b == "D"), ("D", b) => Ok(b == "E" || b == "F" || b == "G"), @@ -1525,7 +1475,7 @@ mod test { // let's add a block "P" which is a descendent of block "O" let is_descendent_of = |base: &&str, block: &&str| -> Result { match (*base, *block) { - (b, "P") => Ok(vec!["A", "F", "L", "O"].into_iter().any(|n| n == b)), + (b, "P") => Ok(vec!["A", "F", "H", "L", "O"].into_iter().any(|n| n == b)), _ => Ok(false), } }; @@ -1561,7 +1511,7 @@ mod test { assert_eq!( removed.map(|(h, _, _)| h).collect::>(), - ["J", "K", "H", "L", "M", "O", "I"] + ["H", "L", "M", "O", "I", "J", "K"] ); } From 47d175984758d48e222d3056570e4aee5d403017 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Fri, 15 Apr 2022 10:49:14 +0200 Subject: [PATCH 04/19] Fork-tree map iterative implementation --- utils/fork-tree/src/lib.rs | 256 ++++++++++++++++++++++--------------- 1 file changed, 155 insertions(+), 101 deletions(-) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index 850625f6e73d3..c1bf49ba4e65f 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -125,47 +125,47 @@ where /// imported in order. /// /// Returns `true` if the imported node is a root. - pub fn old_import( - &mut self, - mut hash: H, - mut number: N, - mut data: V, - is_descendent_of: &F, - ) -> Result> - where - E: std::error::Error, - F: Fn(&H, &H) -> Result, - { - if let Some(ref best_finalized_number) = self.best_finalized_number { - if number <= *best_finalized_number { - return Err(Error::Revert) - } - } - - for root in self.roots.iter_mut() { - if root.hash == hash { - return Err(Error::Duplicate) - } - - match root.import(hash, number, data, is_descendent_of)? { - Some((h, n, d)) => { - hash = h; - number = n; - data = d; - }, - None => { - self.rebalance(); - return Ok(false) - }, - } - } - - self.roots.push(Node { data, hash, number, children: Vec::new() }); - - self.rebalance(); - - Ok(true) - } + // pub fn old_import( + // &mut self, + // mut hash: H, + // mut number: N, + // mut data: V, + // is_descendent_of: &F, + // ) -> Result> + // where + // E: std::error::Error, + // F: Fn(&H, &H) -> Result, + // { + // if let Some(ref best_finalized_number) = self.best_finalized_number { + // if number <= *best_finalized_number { + // return Err(Error::Revert) + // } + // } + + // for root in self.roots.iter_mut() { + // if root.hash == hash { + // return Err(Error::Duplicate) + // } + + // match root.import(hash, number, data, is_descendent_of)? { + // Some((h, n, d)) => { + // hash = h; + // number = n; + // data = d; + // }, + // None => { + // self.rebalance(); + // return Ok(false) + // }, + // } + // } + + // self.roots.push(Node { data, hash, number, children: Vec::new() }); + + // self.rebalance(); + + // Ok(true) + // } /// 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 @@ -235,9 +235,46 @@ where where F: FnMut(&H, &N, V) -> VT, { - let roots = self.roots.into_iter().map(|root| root.map(f)).collect(); + let mut stack = vec![]; + + let mut new_tree = ForkTree { + roots: Default::default(), + best_finalized_number: self.best_finalized_number, + }; - ForkTree { roots, best_finalized_number: self.best_finalized_number } + let mut old_children = self.roots; + let mut new_children = &mut new_tree.roots; + + loop { + let mut tmp = vec![]; + for node in old_children { + let data = f(&node.hash, &node.number, node.data); + new_children.push(Node { + data, + hash: node.hash, + number: node.number, + children: Default::default(), + }); + + tmp.push(node.children); + } + // SAFETY: take new children addresses after they are not subject to relocation. + for (i, old_children) in tmp.into_iter().enumerate() { + stack.push((old_children, &mut new_children[i].children as *mut _)); + } + // SAFETY: borrow checker doesn't allow to have multiple mutable references to the + // same vector. Here we carefully use the raw pointer pushed after that referenced + // elements have a stable memory address. + (old_children, new_children) = match stack.pop() { + Some((old_children, new_children_ptr)) => { + let new_children = unsafe { &mut *new_children_ptr }; + (old_children, new_children) + }, + None => break, + }; + } + + new_tree } /// Find a node in the tree that is the deepest ancestor of the given @@ -673,29 +710,39 @@ where where F: FnMut(&H, &N, &V) -> FilterAction, { - let mut stack: Vec<*mut Node> = vec![]; + let mut stack = vec![]; let mut children = &mut self.roots; let mut removed = vec![]; loop { - for node in std::mem::take(children) { + let mut push_indices = vec![]; + let mut i = 0; + while i < children.len() { + let node = &children[i]; match filter(&node.hash, &node.number, &node.data) { FilterAction::KeepNode => { - let i = children.len(); - children.push(node); - stack.push(&mut children[i] as *mut _); + push_indices.push(i); + i += 1; }, FilterAction::KeepTree => { - children.push(node); + i += 1; }, FilterAction::Remove => { - removed.push(node); + removed.push(children.remove(i)); }, } } - + // This should be performed once we know that the addresses are stable, + // i.e. we've optionally removed some elements. + push_indices.into_iter().for_each(|i| { + stack.push(&mut children[i].children as *mut _); + }); + // Get the next children list. + // SAFETY: mutable borrow of multiple parts of the tree is not allowed + // in safe Rust. This action is safe since we've taken care to push only + // pointers that are not subject to relocation. children = match stack.pop() { - Some(n) => unsafe { &mut (*n).children }, + Some(children) => unsafe { &mut *children }, None => break, }; } @@ -743,54 +790,54 @@ mod node_implementation { } /// Map node data into values of new types. - pub fn map(self, f: &mut F) -> Node - where - F: FnMut(&H, &N, V) -> VT, - { - let children = self.children.into_iter().map(|node| node.map(f)).collect(); - - let vt = f(&self.hash, &self.number, self.data); - Node { hash: self.hash, number: self.number, data: vt, children } - } - - pub fn import( - &mut self, - mut hash: H, - mut number: N, - mut data: V, - is_descendent_of: &F, - ) -> Result, Error> - where - E: fmt::Debug, - F: Fn(&H, &H) -> Result, - { - if self.hash == hash { - return Err(Error::Duplicate) - }; - - if number <= self.number { - return Ok(Some((hash, number, data))) - } - - for node in self.children.iter_mut() { - match node.import(hash, number, data, is_descendent_of)? { - Some((h, n, d)) => { - hash = h; - number = n; - data = d; - }, - None => return Ok(None), - } - } - - if is_descendent_of(&self.hash, &hash)? { - self.children.push(Node { data, hash, number, children: Vec::new() }); - - Ok(None) - } else { - Ok(Some((hash, number, data))) - } - } + // pub fn map(self, f: &mut F) -> Node + // where + // F: FnMut(&H, &N, V) -> VT, + // { + // let children = self.children.into_iter().map(|node| node.map(f)).collect(); + + // let vt = f(&self.hash, &self.number, self.data); + // Node { hash: self.hash, number: self.number, data: vt, children } + // } + + // pub fn import( + // &mut self, + // mut hash: H, + // mut number: N, + // mut data: V, + // is_descendent_of: &F, + // ) -> Result, Error> + // where + // E: fmt::Debug, + // F: Fn(&H, &H) -> Result, + // { + // if self.hash == hash { + // return Err(Error::Duplicate) + // }; + + // if number <= self.number { + // return Ok(Some((hash, number, data))) + // } + + // for node in self.children.iter_mut() { + // match node.import(hash, number, data, is_descendent_of)? { + // Some((h, n, d)) => { + // hash = h; + // number = n; + // data = d; + // }, + // None => return Ok(None), + // } + // } + + // if is_descendent_of(&self.hash, &hash)? { + // self.children.push(Node { data, hash, number, children: Vec::new() }); + + // Ok(None) + // } else { + // Ok(Some((hash, number, data))) + // } + // } pub fn find_node_index_where( &self, @@ -1401,7 +1448,14 @@ mod test { fn map_works() { let (tree, _is_descendent_of) = test_fork_tree(); - let _tree = tree.map(&mut |_, _, _| ()); + let old_tree = tree.clone(); + let new_tree = tree.map(&mut |hash, _, _| hash.to_owned()); + + assert!(new_tree.iter().all(|(hash, _, data)| hash == data)); + assert_eq!( + old_tree.iter().map(|(hash, _, _)| *hash).collect::>(), + new_tree.iter().map(|(hash, _, _)| *hash).collect::>(), + ); } #[test] From acdbbb5a1e6d4bb56a0a800ef39978570cb5978a Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Fri, 15 Apr 2022 17:42:28 +0200 Subject: [PATCH 05/19] Optimization of some operations, e.g. rebalance only when required --- utils/fork-tree/src/lib.rs | 357 ++++++++++--------------------------- 1 file changed, 91 insertions(+), 266 deletions(-) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index 74c85c73b090a..0040969118bf5 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -119,54 +119,6 @@ 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. - /// - /// Returns `true` if the imported node is a root. - // pub fn old_import( - // &mut self, - // mut hash: H, - // mut number: N, - // mut data: V, - // is_descendent_of: &F, - // ) -> Result> - // where - // E: std::error::Error, - // F: Fn(&H, &H) -> Result, - // { - // if let Some(ref best_finalized_number) = self.best_finalized_number { - // if number <= *best_finalized_number { - // return Err(Error::Revert) - // } - // } - - // for root in self.roots.iter_mut() { - // if root.hash == hash { - // return Err(Error::Duplicate) - // } - - // match root.import(hash, number, data, is_descendent_of)? { - // Some((h, n, d)) => { - // hash = h; - // number = n; - // data = d; - // }, - // None => { - // self.rebalance(); - // return Ok(false) - // }, - // } - // } - - // self.roots.push(Node { data, hash, number, children: Vec::new() }); - - // self.rebalance(); - - // Ok(true) - // } - /// 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 @@ -191,7 +143,6 @@ where } let mut children = &mut self.roots; - let mut i = 0; let mut len = children.len(); while i < len { @@ -208,10 +159,17 @@ where i += 1; } + let is_first = children.is_empty(); children.push(Node { data, hash, number, children: Default::default() }); - self.rebalance(); - Ok(true) + // Quick way to check if the pushed node is a root + let is_root = children.as_ptr() == self.roots.as_ptr(); + + if is_first { + self.rebalance(); + } + + Ok(is_root) } /// Iterates over the existing roots in the tree. @@ -235,8 +193,6 @@ where where F: FnMut(&H, &N, V) -> VT, { - let mut stack = vec![]; - let mut new_tree = ForkTree { roots: Default::default(), best_finalized_number: self.best_finalized_number, @@ -245,6 +201,8 @@ where let mut old_children = self.roots; let mut new_children = &mut new_tree.roots; + let mut stack = vec![]; + loop { let mut tmp = vec![]; for node in old_children { @@ -258,13 +216,17 @@ where tmp.push(node.children); } - // SAFETY: take new children addresses after they are not subject to relocation. + + // SAFETY: borrow checker doesn't allow mutable references to multiple parts of + // the tree. Usage or raw pointers is safe as far as we take care to handle + // only pointers to data that is not subject to relocation. + // + // This should be performed after the node children have a stable addresses, + // i.e. vector buffer not subject to relocation due to some element insertion. for (i, old_children) in tmp.into_iter().enumerate() { stack.push((old_children, &mut new_children[i].children as *mut _)); } - // SAFETY: borrow checker doesn't allow to have multiple mutable references to the - // same vector. Here we carefully use the raw pointer pushed after that referenced - // elements have a stable memory address. + // Process the next children lists. (old_children, new_children) = match stack.pop() { Some((old_children, new_children_ptr)) => { let new_children = unsafe { &mut *new_children_ptr }; @@ -293,17 +255,16 @@ where F: Fn(&H, &H) -> Result, P: Fn(&V) -> bool, { - // search for node starting from all roots - for root in self.roots.iter() { - let node = root.find_node_where(hash, number, is_descendent_of, predicate)?; - - // found the node, early exit - if let FindOutcome::Found(node) = node { - return Ok(Some(node)) - } - } - - Ok(None) + self.find_node_index_where(hash, number, is_descendent_of, predicate) + .map(|maybe_path| { + maybe_path.map(|path| { + let children = path + .iter() + .take(path.len() - 1) + .fold(&self.roots, |curr, &i| &curr[i].children); + &children[path[path.len() - 1]] + }) + }) } /// Same as [`find_node_where`](ForkTree::find_node_where), but returns mutable reference. @@ -319,17 +280,16 @@ where F: Fn(&H, &H) -> Result, P: Fn(&V) -> bool, { - // search for node starting from all roots - for root in self.roots.iter_mut() { - let node = root.find_node_where_mut(hash, number, is_descendent_of, predicate)?; - - // found the node, early exit - if let FindOutcome::Found(node) = node { - return Ok(Some(node)) - } - } - - Ok(None) + self.find_node_index_where(hash, number, is_descendent_of, predicate) + .map(|maybe_path| { + maybe_path.map(|path| { + let children = path + .iter() + .take(path.len() - 1) + .fold(&mut self.roots, |curr, &i| &mut curr[i].children); + &mut children[path[path.len() - 1]] + }) + }) } /// Same as [`find_node_where`](ForkTree::find_node_where), but returns indexes. @@ -345,17 +305,27 @@ where F: Fn(&H, &H) -> Result, P: Fn(&V) -> bool, { - // search for node starting from all roots - for (idx, root) in self.roots.iter().enumerate() { - let path = root.find_node_index_where(hash, number, is_descendent_of, predicate)?; - - // found the node, early exit - if let FindOutcome::Found(mut path) = path { - path.insert(0, idx); - return Ok(Some(path)) + let mut path = vec![]; + let mut children = &self.roots; + + loop { + let mut keep_search = false; + for (idx, node) in children.iter().enumerate() { + if node.number < *number && is_descendent_of(&node.hash, hash)? { + if predicate(&node.data) { + path.push(idx); + children = &node.children; + keep_search = true; + } + break + } + } + if !keep_search { + break } } - Ok(None) + + Ok(if path.is_empty() { None } else { Some(path) }) } /// Prune the tree, removing all non-canonical nodes. We find the node in the @@ -412,14 +382,13 @@ where } self.roots = vec![root]; + self.rebalance(); removed } else { Vec::new() }; - self.rebalance(); - Ok(RemovedIterator { stack: removed }) } @@ -735,41 +704,36 @@ where }, } } - // This should be performed once we know that the addresses are stable, - // i.e. we've optionally removed some elements. + + // SAFETY: borrow checker doesn't allow mutable references to multiple parts of + // the tree. Usage or raw pointers is safe as far as we take care to handle + // only pointers to data that is not subject to relocation. + // + // This should be performed after the node children have a stable addresses, + // i.e. not subject to relocation due to some element removal. push_indices.into_iter().for_each(|i| { stack.push(&mut children[i].children as *mut _); }); - // Get the next children list. - // SAFETY: mutable borrow of multiple parts of the tree is not allowed - // in safe Rust. This action is safe since we've taken care to push only - // pointers that are not subject to relocation. + // Process the next children list. children = match stack.pop() { Some(children) => unsafe { &mut *children }, None => break, }; } - self.rebalance(); + if !removed.is_empty() { + self.rebalance(); + } RemovedIterator { stack: removed } } } // Workaround for: https://github.com/rust-lang/rust/issues/34537 +use node_implementation::Node; + mod node_implementation { use super::*; - /// The outcome of a search within a node. - pub enum FindOutcome { - // this is the node we were looking for. - Found(T), - // not the node we're looking for. contains a flag indicating - // whether the node was a descendent. true implies the predicate failed. - Failure(bool), - // Abort search. - Abort, - } - #[derive(Clone, Debug, Decode, Encode, PartialEq)] pub struct Node { pub hash: H, @@ -782,169 +746,18 @@ mod node_implementation { /// Finds the max depth among all branches descendent from this node. pub fn max_depth(&self) -> usize { let mut max: usize = 0; - let mut stack = vec![(self, 1)]; - while let Some((n, height)) = stack.pop() { + let mut stack = vec![(self, 0)]; + while let Some((node, height)) = stack.pop() { if height > max { max = height; } - n.children.iter().for_each(|n| stack.push((n, height + 1))); + node.children.iter().for_each(|n| stack.push((n, height + 1))); } max } - - /// Map node data into values of new types. - // pub fn map(self, f: &mut F) -> Node - // where - // F: FnMut(&H, &N, V) -> VT, - // { - // let children = self.children.into_iter().map(|node| node.map(f)).collect(); - - // let vt = f(&self.hash, &self.number, self.data); - // Node { hash: self.hash, number: self.number, data: vt, children } - // } - - // pub fn import( - // &mut self, - // mut hash: H, - // mut number: N, - // mut data: V, - // is_descendent_of: &F, - // ) -> Result, Error> - // where - // E: fmt::Debug, - // F: Fn(&H, &H) -> Result, - // { - // if self.hash == hash { - // return Err(Error::Duplicate) - // }; - - // if number <= self.number { - // return Ok(Some((hash, number, data))) - // } - - // for node in self.children.iter_mut() { - // match node.import(hash, number, data, is_descendent_of)? { - // Some((h, n, d)) => { - // hash = h; - // number = n; - // data = d; - // }, - // None => return Ok(None), - // } - // } - - // if is_descendent_of(&self.hash, &hash)? { - // self.children.push(Node { data, hash, number, children: Vec::new() }); - - // Ok(None) - // } else { - // Ok(Some((hash, number, data))) - // } - // } - - pub fn find_node_index_where( - &self, - hash: &H, - number: &N, - is_descendent_of: &F, - predicate: &P, - ) -> Result>, Error> - where - E: std::error::Error, - F: Fn(&H, &H) -> Result, - P: Fn(&V) -> bool, - { - let mut curr = self; - let mut path = vec![]; - loop { - let mut pushed = false; - for (idx, child) in curr.children.iter().enumerate() { - if child.number < *number && is_descendent_of(&child.hash, hash)? { - curr = child; - path.push(idx); - pushed = true; - break - } - } - if !pushed { - break - } - } - - if curr.hash == *hash || is_descendent_of(&curr.hash, hash)? { - if predicate(&curr.data) { - Ok(FindOutcome::Found(path)) - } else { - Ok(FindOutcome::Failure(true)) - } - } else { - Ok(FindOutcome::Failure(false)) - } - } - - /// Find a node in the tree that is the deepest ancestor of the given - /// block hash which also passes the given predicate, backtracking - /// when the predicate fails. - /// 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, - number: &N, - is_descendent_of: &F, - predicate: &P, - ) -> Result>, Error> - where - E: std::error::Error, - F: Fn(&H, &H) -> Result, - P: Fn(&V) -> bool, - { - let outcome = self.find_node_index_where(hash, number, is_descendent_of, predicate)?; - - match outcome { - FindOutcome::Abort => Ok(FindOutcome::Abort), - FindOutcome::Failure(f) => Ok(FindOutcome::Failure(f)), - FindOutcome::Found(path) => { - let node = path.iter().fold(self, |curr, i| &curr.children[*i]); - Ok(FindOutcome::Found(node)) - }, - } - } - - /// Find a node in the tree that is the deepest ancestor of the given - /// block hash which also passes the given predicate, backtracking - /// when the predicate fails. - /// 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_mut( - &mut self, - hash: &H, - number: &N, - is_descendent_of: &F, - predicate: &P, - ) -> Result>, Error> - where - E: std::error::Error, - F: Fn(&H, &H) -> Result, - P: Fn(&V) -> bool, - { - let outcome = self.find_node_index_where(hash, number, is_descendent_of, predicate)?; - - match outcome { - FindOutcome::Abort => Ok(FindOutcome::Abort), - FindOutcome::Failure(f) => Ok(FindOutcome::Failure(f)), - FindOutcome::Found(path) => { - let node = path.iter().fold(self, |curr, i| &mut curr.children[*i]); - Ok(FindOutcome::Found(node)) - }, - } - } } } -// Workaround for: https://github.com/rust-lang/rust/issues/34537 -use node_implementation::{FindOutcome, Node}; - struct ForkTreeIterator<'a, H, N, V> { stack: Vec<&'a Node>, } @@ -1285,10 +1098,16 @@ mod test { } }; - tree.import("A0", 1, Change { effective: 5 }, &is_descendent_of).unwrap(); - tree.import("A1", 1, Change { effective: 5 }, &is_descendent_of).unwrap(); - tree.import("D", 10, Change { effective: 10 }, &is_descendent_of).unwrap(); - tree.import("E", 15, Change { effective: 50 }, &is_descendent_of).unwrap(); + let is_root = tree.import("A0", 1, Change { effective: 5 }, &is_descendent_of).unwrap(); + assert!(is_root); + let is_root = tree.import("A1", 1, Change { effective: 5 }, &is_descendent_of).unwrap(); + assert!(is_root); + let is_root = + tree.import("D", 10, Change { effective: 10 }, &is_descendent_of).unwrap(); + assert!(!is_root); + let is_root = + tree.import("E", 15, Change { effective: 50 }, &is_descendent_of).unwrap(); + assert!(!is_root); (tree, is_descendent_of) }; @@ -1589,6 +1408,12 @@ mod test { fn find_node_index_works() { let (tree, is_descendent_of) = test_fork_tree(); + let path = tree + .find_node_index_where(&"D", &4, &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) .unwrap() From a090eb1ffcffe239ea11eeaf8c7a9c5b8cc42a02 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Fri, 15 Apr 2022 19:27:16 +0200 Subject: [PATCH 06/19] Destructuring assignments not supported by stable rustc 1.57 --- 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 0040969118bf5..d66663d347e57 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -227,10 +227,10 @@ where stack.push((old_children, &mut new_children[i].children as *mut _)); } // Process the next children lists. - (old_children, new_children) = match stack.pop() { - Some((old_children, new_children_ptr)) => { - let new_children = unsafe { &mut *new_children_ptr }; - (old_children, new_children) + match stack.pop() { + Some((old, new_ptr)) => { + old_children = old; + new_children = unsafe { &mut *new_ptr }; }, None => break, }; From 051ac86ac165c2c7750ab24c2db27d84a5dcb81d Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Fri, 15 Apr 2022 19:53:04 +0200 Subject: [PATCH 07/19] Nitpicks --- utils/fork-tree/src/lib.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index d66663d347e57..e3d75be6d541f 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -143,8 +143,8 @@ where } let mut children = &mut self.roots; - let mut i = 0; let mut len = children.len(); + let mut i = 0; while i < len { let child = &children[i]; if child.hash == hash { @@ -154,9 +154,9 @@ where children = &mut children[i].children; len = children.len(); i = 0; - continue + } else { + i += 1; } - i += 1; } let is_first = children.is_empty(); @@ -166,6 +166,7 @@ where let is_root = children.as_ptr() == self.roots.as_ptr(); if is_first { + // Rebalance is required only if we've extended the branch depth. self.rebalance(); } From 9163787a9f75d3d7e4f4cd4d2ee3c1a77423504a Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Fri, 22 Apr 2022 14:06:53 +0200 Subject: [PATCH 08/19] Safe implementation of 'map' and 'drain_filter' methods --- utils/fork-tree/src/lib.rs | 122 ++++++++++++++++++------------------- 1 file changed, 59 insertions(+), 63 deletions(-) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index e3d75be6d541f..11f5ff1ed31a1 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -194,50 +194,42 @@ where where F: FnMut(&H, &N, V) -> VT, { - let mut new_tree = ForkTree { - roots: Default::default(), - best_finalized_number: self.best_finalized_number, - }; - - let mut old_children = self.roots; - let mut new_children = &mut new_tree.roots; - - let mut stack = vec![]; - - loop { - let mut tmp = vec![]; - for node in old_children { - let data = f(&node.hash, &node.number, node.data); - new_children.push(Node { - data, + let mut queue: Vec<_> = + self.roots.into_iter().rev().map(|node| (usize::MAX, node)).collect(); + let mut next_queue = Vec::new(); + let mut output = Vec::new(); + + while !queue.is_empty() { + for (parent_index, node) in queue.drain(..) { + let new_data = f(&node.hash, &node.number, node.data); + let new_node = Node { hash: node.hash, number: node.number, - children: Default::default(), - }); + data: new_data, + children: Vec::with_capacity(node.children.len()), + }; - tmp.push(node.children); + let node_id = output.len(); + output.push((parent_index, new_node)); + + for child in node.children.into_iter().rev() { + next_queue.push((node_id, child)); + } } - // SAFETY: borrow checker doesn't allow mutable references to multiple parts of - // the tree. Usage or raw pointers is safe as far as we take care to handle - // only pointers to data that is not subject to relocation. - // - // This should be performed after the node children have a stable addresses, - // i.e. vector buffer not subject to relocation due to some element insertion. - for (i, old_children) in tmp.into_iter().enumerate() { - stack.push((old_children, &mut new_children[i].children as *mut _)); + std::mem::swap(&mut queue, &mut next_queue); + } + + let mut roots = Vec::new(); + while let Some((parent_index, new_node)) = output.pop() { + if parent_index == usize::MAX { + roots.push(new_node); + } else { + output[parent_index].1.children.push(new_node); } - // Process the next children lists. - match stack.pop() { - Some((old, new_ptr)) => { - old_children = old; - new_children = unsafe { &mut *new_ptr }; - }, - None => break, - }; } - new_tree + ForkTree { roots, best_finalized_number: self.best_finalized_number } } /// Find a node in the tree that is the deepest ancestor of the given @@ -683,43 +675,46 @@ where where F: FnMut(&H, &N, &V) -> FilterAction, { - let mut stack = vec![]; - let mut children = &mut self.roots; let mut removed = vec![]; + let mut retained = Vec::new(); - loop { - let mut push_indices = vec![]; - let mut i = 0; - while i < children.len() { - let node = &children[i]; + let mut queue: Vec<_> = std::mem::take(&mut self.roots) + .into_iter() + .rev() + .map(|node| (usize::MAX, node)) + .collect(); + let mut next_queue = Vec::new(); + + while !queue.is_empty() { + for (parent_idx, mut node) in queue.drain(..) { match filter(&node.hash, &node.number, &node.data) { FilterAction::KeepNode => { - push_indices.push(i); - i += 1; + let node_idx = retained.len(); + let children = std::mem::take(&mut node.children); + retained.push((parent_idx, node)); + for child in children.into_iter().rev() { + next_queue.push((node_idx, child)); + } }, FilterAction::KeepTree => { - i += 1; + retained.push((parent_idx, node)); }, FilterAction::Remove => { - removed.push(children.remove(i)); + removed.push(node); }, } } - // SAFETY: borrow checker doesn't allow mutable references to multiple parts of - // the tree. Usage or raw pointers is safe as far as we take care to handle - // only pointers to data that is not subject to relocation. - // - // This should be performed after the node children have a stable addresses, - // i.e. not subject to relocation due to some element removal. - push_indices.into_iter().for_each(|i| { - stack.push(&mut children[i].children as *mut _); - }); - // Process the next children list. - children = match stack.pop() { - Some(children) => unsafe { &mut *children }, - None => break, - }; + std::mem::swap(&mut queue, &mut next_queue); + } + + // Reconstruct + while let Some((parent_idx, node)) = retained.pop() { + if parent_idx == usize::MAX { + self.roots.push(node); + } else { + retained[parent_idx].1.children.push(node); + } } if !removed.is_empty() { @@ -1287,6 +1282,7 @@ mod test { let old_tree = tree.clone(); let new_tree = tree.map(&mut |hash, _, _| hash.to_owned()); + // Check content and order assert!(new_tree.iter().all(|(hash, _, data)| hash == data)); assert_eq!( old_tree.iter().map(|(hash, _, _)| *hash).collect::>(), @@ -1351,7 +1347,7 @@ mod test { } #[test] - fn tree_rebalance() { + fn rebalance_works() { let (mut tree, _) = test_fork_tree(); // the tree is automatically rebalanced on import, therefore we should iterate in preorder @@ -1382,7 +1378,7 @@ mod test { } #[test] - fn tree_drain_filter() { + fn drain_filter_works() { let (mut tree, _) = test_fork_tree(); let filter = |h: &&str, _: &u64, _: &()| match *h { From 406b52695b05143350201a0e263d8d78f7370717 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Sat, 23 Apr 2022 01:34:46 +0200 Subject: [PATCH 09/19] Remove dummy comment --- utils/fork-tree/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index 11f5ff1ed31a1..3736c6cc34b1d 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -708,7 +708,6 @@ where std::mem::swap(&mut queue, &mut next_queue); } - // Reconstruct while let Some((parent_idx, node)) = retained.pop() { if parent_idx == usize::MAX { self.roots.push(node); From 9f8fd20bc433b43392fe03be54f1b7dac326c739 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Sat, 23 Apr 2022 17:37:10 +0200 Subject: [PATCH 10/19] Trigger CI pipeline From 1dde93bba78db928887ff0009f81ea8fa0434d69 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 26 Apr 2022 18:15:33 +0200 Subject: [PATCH 11/19] Test map for multi-root fork-tree and refactory of `find_node_index_where` --- utils/fork-tree/src/lib.rs | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index 3736c6cc34b1d..91091653781fc 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -300,21 +300,19 @@ where { let mut path = vec![]; let mut children = &self.roots; + let mut i = 0; - loop { - let mut keep_search = false; - for (idx, node) in children.iter().enumerate() { - if node.number < *number && is_descendent_of(&node.hash, hash)? { - if predicate(&node.data) { - path.push(idx); - children = &node.children; - keep_search = true; - } + while i < children.len() { + let node = &children[i]; + if node.number < *number && is_descendent_of(&node.hash, hash)? { + if !predicate(&node.data) { break } - } - if !keep_search { - break + path.push(i); + i = 0; + children = &node.children; + } else { + i += 1; } } @@ -1276,7 +1274,14 @@ mod test { #[test] fn map_works() { - let (tree, _is_descendent_of) = test_fork_tree(); + let (mut tree, _) = test_fork_tree(); + + // 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(); + assert!(is_root); + let is_root = tree.import("A2", 1, (), &is_descendent_of).unwrap(); + assert!(is_root); let old_tree = tree.clone(); let new_tree = tree.map(&mut |hash, _, _| hash.to_owned()); From 7d8c7a3e0d995451a6277231231414f0e1c68fed Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Thu, 28 Apr 2022 17:32:08 +0200 Subject: [PATCH 12/19] Fix find node index with predicate We should find the deepest ancestor satisfying the predicate regardless of the predicate evaluation result on the found node ancestors. Added one test to excercise the bug --- utils/fork-tree/src/lib.rs | 137 +++++++++++++++++++++++++------------ 1 file changed, 94 insertions(+), 43 deletions(-) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index 91091653781fc..93b6b4fd6a264 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -115,7 +115,7 @@ where let mut stack: Vec<_> = self.roots.iter_mut().collect(); while let Some(node) = stack.pop() { node.children.sort_by_key(|n| Reverse(n.max_depth())); - node.children.iter_mut().for_each(|n| stack.push(n)); + stack.extend(node.children.iter_mut()); } } @@ -301,14 +301,15 @@ where 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)? { - if !predicate(&node.data) { - break - } path.push(i); + if predicate(&node.data) { + best_depth = path.len(); + } i = 0; children = &node.children; } else { @@ -316,7 +317,12 @@ where } } - Ok(if path.is_empty() { None } else { Some(path) }) + Ok(if best_depth == 0 { + None + } else { + path.truncate(best_depth); + Some(path) + }) } /// Prune the tree, removing all non-canonical nodes. We find the node in the @@ -339,46 +345,43 @@ where F: Fn(&H, &H) -> Result, P: Fn(&V) -> bool, { - let new_root_index = - self.find_node_index_where(hash, number, is_descendent_of, predicate)?; - - let removed = if let Some(root_index) = new_root_index { - let mut old_roots = std::mem::take(&mut self.roots); - - let curr_children = 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; - - // 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; - - for child in root_children { - if is_first && - (child.number == *number && child.hash == *hash || - child.number < *number && is_descendent_of(&child.hash, 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; - } else { - removed.push(child); - } - } + let root_index = + match self.find_node_index_where(hash, number, is_descendent_of, predicate)? { + Some(idx) => idx, + None => return Ok(RemovedIterator { stack: Vec::new() }), + }; - self.roots = vec![root]; - self.rebalance(); + let mut old_roots = std::mem::take(&mut self.roots); - removed - } else { - Vec::new() - }; + let curr_children = 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; + + // 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; + + for child in root_children { + if is_first && + (child.number == *number && child.hash == *hash || + child.number < *number && is_descendent_of(&child.hash, 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; + } else { + removed.push(child); + } + } + + self.roots = vec![root]; + self.rebalance(); Ok(RemovedIterator { stack: removed }) } @@ -1428,6 +1431,54 @@ mod test { assert_eq!(path, [0, 1, 0, 0, 0]); } + #[test] + fn find_node_index_with_predicate_works() { + fn is_descendent_of(parent: &char, child: &char) -> Result { + match *parent { + 'A' => Ok(['B', 'C', 'D', 'E', 'F'].contains(child)), + 'B' => Ok(['C', 'D'].contains(child)), + 'C' => Ok(['D'].contains(child)), + 'E' => Ok(['F'].contains(child)), + 'D' | 'F' => Ok(false), + _ => unreachable!(), + } + } + + // A(t) --- B(f) --- C(t) --- D(f) + // \-- E(t) --- F(f) + let mut tree: ForkTree = ForkTree::new(); + tree.import('A', 1, true, &is_descendent_of).unwrap(); + tree.import('B', 2, false, &is_descendent_of).unwrap(); + tree.import('C', 3, true, &is_descendent_of).unwrap(); + tree.import('D', 4, false, &is_descendent_of).unwrap(); + + tree.import('E', 2, true, &is_descendent_of).unwrap(); + tree.import('F', 3, false, &is_descendent_of).unwrap(); + + let path = tree + .find_node_index_where(&'D', &4, &is_descendent_of, &|&value| !value) + .unwrap() + .unwrap(); + assert_eq!(path, [0, 0]); + + let path = tree + .find_node_index_where(&'D', &4, &is_descendent_of, &|&value| value) + .unwrap() + .unwrap(); + assert_eq!(path, [0, 0, 0]); + + let path = tree + .find_node_index_where(&'F', &3, &is_descendent_of, &|&value| !value) + .unwrap(); + assert_eq!(path, None); + + let path = tree + .find_node_index_where(&'F', &3, &is_descendent_of, &|&value| value) + .unwrap() + .unwrap(); + assert_eq!(path, [0, 1]); + } + #[test] fn find_node_works() { let (tree, is_descendent_of) = test_fork_tree(); From 29215b5813a7caeec604996772e908293da70da4 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Thu, 28 Apr 2022 17:38:17 +0200 Subject: [PATCH 13/19] Nits --- utils/fork-tree/src/lib.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index 93b6b4fd6a264..aff29a6a4a4ca 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -143,16 +143,14 @@ where } let mut children = &mut self.roots; - let mut len = children.len(); let mut i = 0; - while i < len { + while i < children.len() { let child = &children[i]; if child.hash == hash { return Err(Error::Duplicate) } if child.number < number && is_descendent_of(&child.hash, &hash)? { children = &mut children[i].children; - len = children.len(); i = 0; } else { i += 1; From 182d0739bac817b261ff658bdaec39c5ee8d3b1e Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Fri, 29 Apr 2022 15:01:22 +0200 Subject: [PATCH 14/19] Tree traversal algorithm is not specified --- utils/fork-tree/src/lib.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index aff29a6a4a4ca..dc2fce7e19603 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -81,12 +81,19 @@ 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 /// 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 /// when interacting with the tree to establish a node's ancestry. +/// +/// In order to provide some kind of services, some methods interface requires +/// the user to provide one or more predicates. Keep in mind that tree traversal +/// technique (e.g. BFS vs DFS) is left as not specified and may be subject to +/// change in the future. In other words, you predicates should not rely on the +/// observed traversal technique currently in use. #[derive(Clone, Debug, Decode, Encode, PartialEq)] pub struct ForkTree { roots: Vec>, From 9cc8b515d2aa66b199a57f138210410a76cdf3d7 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Fri, 29 Apr 2022 15:11:07 +0200 Subject: [PATCH 15/19] Move unspecified tree traversal warning to 'map' --- utils/fork-tree/src/lib.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index dc2fce7e19603..da7afcdeaf357 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -88,12 +88,6 @@ pub enum FilterAction { /// 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 /// when interacting with the tree to establish a node's ancestry. -/// -/// In order to provide some kind of services, some methods interface requires -/// the user to provide one or more predicates. Keep in mind that tree traversal -/// technique (e.g. BFS vs DFS) is left as not specified and may be subject to -/// change in the future. In other words, you predicates should not rely on the -/// observed traversal technique currently in use. #[derive(Clone, Debug, Decode, Encode, PartialEq)] pub struct ForkTree { roots: Vec>, @@ -195,6 +189,10 @@ where } /// Map fork tree into values of new types. + /// + /// Tree traversal technique (e.g. BFS vs DFS) is left as not specified and + /// may be subject to change in the future. In other words, your predicates + /// should not rely on the observed traversal technique currently in use. pub fn map(self, f: &mut F) -> ForkTree where F: FnMut(&H, &N, V) -> VT, From f6d04e12c12f9db275db25815417d34796fce35f Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Fri, 29 Apr 2022 15:13:12 +0200 Subject: [PATCH 16/19] Immutable 'drain_filter' predicate --- client/finality-grandpa/src/authorities.rs | 4 ++-- utils/fork-tree/src/lib.rs | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index 668fe5f269051..f590096709d57 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -229,7 +229,7 @@ where where F: Fn(&H, &H) -> Result, { - let mut filter = |node_hash: &H, node_num: &N, _: &PendingChange| { + let filter = |node_hash: &H, node_num: &N, _: &PendingChange| { if number >= *node_num && (is_descendent_of(node_hash, &hash).unwrap_or_default() || *node_hash == hash) { @@ -245,7 +245,7 @@ where }; // Remove standard changes. - let _ = self.pending_standard_changes.drain_filter(&mut filter); + let _ = self.pending_standard_changes.drain_filter(&filter); // Remove forced changes. self.pending_forced_changes diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index da7afcdeaf357..cf7deac0f0604 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -670,14 +670,16 @@ where } /// Remove from the tree some nodes (and their subtrees) using a `filter` predicate. + /// /// The `filter` is called over tree nodes and returns a filter action: /// - `Remove` if the node and its subtree should be removed; /// - `KeepNode` if we should maintain the node and keep processing the tree. /// - `KeepTree` if we should maintain the node and its entire subtree. + /// /// An iterator over all the pruned nodes is returned. - pub fn drain_filter(&mut self, mut filter: F) -> impl Iterator + pub fn drain_filter(&mut self, filter: F) -> impl Iterator where - F: FnMut(&H, &N, &V) -> FilterAction, + F: Fn(&H, &N, &V) -> FilterAction, { let mut removed = vec![]; let mut retained = Vec::new(); From 863105f377557ea260b0ddc8c1d169fa8ce61894 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 10 May 2022 14:54:54 +0200 Subject: [PATCH 17/19] 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 cf7deac0f0604..f2e1fc5036878 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -826,7 +826,7 @@ mod test { // / / // A - F - H - I // \ \ - // \ - L - M - (N) + // \ - L - M - N // \ \ // \ - O // - J - K From bc5d9fb852ef123a5136a963bd60c339ca2e6a28 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 10 May 2022 15:03:06 +0200 Subject: [PATCH 18/19] 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 | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index f2e1fc5036878..b97c25ad201ed 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -288,7 +288,13 @@ where }) } - /// Same as [`find_node_where`](ForkTree::find_node_where), but returns indexes. + /// 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 last to first, i.e. the earliest index in the traverse path goes last, and the final + /// index in the traverse path goes first. If a node is found that matches the predicate + /// the returned path should always contain at least one index, otherwise `None` is + /// returned. pub fn find_node_index_where( &self, hash: &H, From f0269d41e1cc2871c0791f67380dc192e92b9e06 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 10 May 2022 15:08:07 +0200 Subject: [PATCH 19/19] Remove double mapping --- utils/fork-tree/src/lib.rs | 40 ++++++++++++++++---------------------- 1 file changed, 17 insertions(+), 23 deletions(-) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index b97c25ad201ed..e78e67da88c6d 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -251,16 +251,12 @@ where F: Fn(&H, &H) -> Result, P: Fn(&V) -> bool, { - self.find_node_index_where(hash, number, is_descendent_of, predicate) - .map(|maybe_path| { - maybe_path.map(|path| { - let children = path - .iter() - .take(path.len() - 1) - .fold(&self.roots, |curr, &i| &curr[i].children); - &children[path[path.len() - 1]] - }) - }) + let maybe_path = self.find_node_index_where(hash, number, is_descendent_of, predicate)?; + Ok(maybe_path.map(|path| { + let children = + path.iter().take(path.len() - 1).fold(&self.roots, |curr, &i| &curr[i].children); + &children[path[path.len() - 1]] + })) } /// Same as [`find_node_where`](ForkTree::find_node_where), but returns mutable reference. @@ -276,24 +272,22 @@ where F: Fn(&H, &H) -> Result, P: Fn(&V) -> bool, { - self.find_node_index_where(hash, number, is_descendent_of, predicate) - .map(|maybe_path| { - maybe_path.map(|path| { - let children = path - .iter() - .take(path.len() - 1) - .fold(&mut self.roots, |curr, &i| &mut curr[i].children); - &mut children[path[path.len() - 1]] - }) - }) + let maybe_path = self.find_node_index_where(hash, number, is_descendent_of, predicate)?; + Ok(maybe_path.map(|path| { + let children = path + .iter() + .take(path.len() - 1) + .fold(&mut self.roots, |curr, &i| &mut curr[i].children); + &mut children[path[path.len() - 1]] + })) } /// 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 last to first, i.e. the earliest index in the traverse path goes last, and the final - /// index in the traverse path goes first. If a node is found that matches the predicate - /// the returned path should always contain at least one index, otherwise `None` is + /// 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. pub fn find_node_index_where( &self,