From 6bb3a651893960679bf1de3190dd2ed484a34710 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 27 Aug 2019 18:09:31 +1000 Subject: [PATCH 1/2] Guard reduced tree from errors --- eth2/lmd_ghost/src/reduced_tree.rs | 107 +++++++++++++++-------------- 1 file changed, 57 insertions(+), 50 deletions(-) diff --git a/eth2/lmd_ghost/src/reduced_tree.rs b/eth2/lmd_ghost/src/reduced_tree.rs index cd3a38c46..a388d2c38 100644 --- a/eth2/lmd_ghost/src/reduced_tree.rs +++ b/eth2/lmd_ghost/src/reduced_tree.rs @@ -311,51 +311,53 @@ where /// become redundant and removed from the reduced tree. fn remove_latest_message(&mut self, validator_index: usize) -> Result<()> { if let Some(vote) = *self.latest_votes.get(validator_index) { - self.get_mut_node(vote.hash)?.remove_voter(validator_index); - let node = self.get_node(vote.hash)?.clone(); + if self.nodes.contains_key(&vote.hash) { + self.get_mut_node(vote.hash)?.remove_voter(validator_index); + let node = self.get_node(vote.hash)?.clone(); - if let Some(parent_hash) = node.parent_hash { - if node.has_votes() || node.children.len() > 1 { - // A node with votes or more than one child is never removed. - } else if node.children.len() == 1 { - // A node which has only one child may be removed. - // - // Load the child of the node and set it's parent to be the parent of this - // node (viz., graft the node's child to the node's parent) - let child = self.get_mut_node(node.children[0])?; - child.parent_hash = node.parent_hash; + if let Some(parent_hash) = node.parent_hash { + if node.has_votes() || node.children.len() > 1 { + // A node with votes or more than one child is never removed. + } else if node.children.len() == 1 { + // A node which has only one child may be removed. + // + // Load the child of the node and set it's parent to be the parent of this + // node (viz., graft the node's child to the node's parent) + let child = self.get_mut_node(node.children[0])?; + child.parent_hash = node.parent_hash; - // Graft the parent of this node to it's child. - if let Some(parent_hash) = node.parent_hash { - let parent = self.get_mut_node(parent_hash)?; - parent.replace_child(node.block_hash, node.children[0])?; + // Graft the parent of this node to it's child. + if let Some(parent_hash) = node.parent_hash { + let parent = self.get_mut_node(parent_hash)?; + parent.replace_child(node.block_hash, node.children[0])?; + } + + self.nodes.remove(&vote.hash); + } else if node.children.is_empty() { + // Remove the to-be-deleted node from it's parent. + if let Some(parent_hash) = node.parent_hash { + self.get_mut_node(parent_hash)? + .remove_child(node.block_hash)?; + } + + self.nodes.remove(&vote.hash); + + // A node which has no children may be deleted and potentially it's parent + // too. + self.maybe_delete_node(parent_hash)?; + } else { + // It is impossible for a node to have a number of children that is not 0, 1 or + // greater than one. + // + // This code is strictly unnecessary, however we keep it for readability. + unreachable!(); } - - self.nodes.remove(&vote.hash); - } else if node.children.is_empty() { - // Remove the to-be-deleted node from it's parent. - if let Some(parent_hash) = node.parent_hash { - self.get_mut_node(parent_hash)? - .remove_child(node.block_hash)?; - } - - self.nodes.remove(&vote.hash); - - // A node which has no children may be deleted and potentially it's parent - // too. - self.maybe_delete_node(parent_hash)?; } else { - // It is impossible for a node to have a number of children that is not 0, 1 or - // greater than one. - // - // This code is strictly unnecessary, however we keep it for readability. - unreachable!(); + // A node without a parent is the genesis/finalized node and should never be removed. } - } else { - // A node without a parent is the genesis/finalized node and should never be removed. - } - self.latest_votes.insert(validator_index, Some(vote)); + self.latest_votes.insert(validator_index, Some(vote)); + } } Ok(()) @@ -370,25 +372,30 @@ where /// - it does not have any votes. fn maybe_delete_node(&mut self, hash: Hash256) -> Result<()> { let should_delete = { - let node = self.get_node(hash)?.clone(); + if let Ok(node) = self.get_node(hash) { + let node = node.clone(); - if let Some(parent_hash) = node.parent_hash { - if (node.children.len() == 1) && !node.has_votes() { - let child_hash = node.children[0]; + if let Some(parent_hash) = node.parent_hash { + if (node.children.len() == 1) && !node.has_votes() { + let child_hash = node.children[0]; - // Graft the single descendant `node` to the `parent` of node. - self.get_mut_node(child_hash)?.parent_hash = Some(parent_hash); + // Graft the single descendant `node` to the `parent` of node. + self.get_mut_node(child_hash)?.parent_hash = Some(parent_hash); - // Detach `node` from `parent`, replacing it with `child`. - self.get_mut_node(parent_hash)? - .replace_child(hash, child_hash)?; + // Detach `node` from `parent`, replacing it with `child`. + self.get_mut_node(parent_hash)? + .replace_child(hash, child_hash)?; - true + true + } else { + false + } } else { + // A node without a parent is the genesis node and should not be deleted. false } } else { - // A node without a parent is the genesis node and should not be deleted. + // No need to delete a node that does not exist. false } }; From e9e912323e7a1327f6f6b26e64d7429fa9311a29 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 29 Aug 2019 13:56:00 +1000 Subject: [PATCH 2/2] Restrict fork choice iterators to the root --- eth2/lmd_ghost/src/reduced_tree.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/eth2/lmd_ghost/src/reduced_tree.rs b/eth2/lmd_ghost/src/reduced_tree.rs index a388d2c38..73fab13bf 100644 --- a/eth2/lmd_ghost/src/reduced_tree.rs +++ b/eth2/lmd_ghost/src/reduced_tree.rs @@ -470,6 +470,7 @@ where // descendant of both `node` and `prev_in_tree`. if self .iter_ancestors(child_hash)? + .take_while(|(_, slot)| *slot >= self.root_slot()) .any(|(ancestor, _slot)| ancestor == node.block_hash) { let child = self.get_mut_node(child_hash)?; @@ -555,6 +556,7 @@ where fn find_prev_in_tree(&mut self, hash: Hash256) -> Option { self.iter_ancestors(hash) .ok()? + .take_while(|(_, slot)| *slot >= self.root_slot()) .find(|(root, _slot)| self.nodes.contains_key(root)) .and_then(|(root, _slot)| Some(root)) } @@ -562,8 +564,12 @@ where /// For the two given block roots (`a_root` and `b_root`), find the first block they share in /// the tree. Viz, find the block that these two distinct blocks forked from. fn find_highest_common_ancestor(&self, a_root: Hash256, b_root: Hash256) -> Result { - let mut a_iter = self.iter_ancestors(a_root)?; - let mut b_iter = self.iter_ancestors(b_root)?; + let mut a_iter = self + .iter_ancestors(a_root)? + .take_while(|(_, slot)| *slot >= self.root_slot()); + let mut b_iter = self + .iter_ancestors(b_root)? + .take_while(|(_, slot)| *slot >= self.root_slot()); // Combines the `next()` fns on the `a_iter` and `b_iter` and returns the roots of two // blocks at the same slot, or `None` if we have gone past genesis or the root of this tree.