From 9dea2cf6ded3ffdd78bfbafb2ca86383d340cb0f Mon Sep 17 00:00:00 2001 From: "Roman S. Borschel" Date: Mon, 2 Dec 2019 23:22:32 +0100 Subject: [PATCH] Fix find_ancestor. * Some cleanup for the fuzzing code with a few more assertions. * In order for the round fuzzing to check the eventual completability of the round, all remaining prevotes must be imported. This is a good illustration of the fact that in the general case a single receive omission (of a prevote or precommit) may lead to a voter being "stuck" in a round due not seeing the estimate at the right spot in the block tree (i.e. missed prevotes can cause the estimate to not move "down" far enough and missed precommits can cause it to not move "up" far enough, in a block tree that grows downwards). * The graph fuzzing was always returning early when starting to record precommit weights, thus not checking much. * The (generalised) graph fuzzing showed a problem with find_ancestor whereby it may not return the highest block in the chain satisfying the condition due to a (hidden) assumption that the condition must only be true of at most one child of each vote-node on the chain whose head is the given starting block (this assumption comes from ghost_find_merge_point). In the context of a Round, where this function is used to determine the estimate, this assumption seems satisfied only because the estimate is not computed before a supermajority of precommits have been observed. Since all (honest) precommit weight must be somewhere downwards from the prevote-ghost any voter sees, it is impossible for a chain not descending from the prevote-ghost to have supermajority, thus satisfying the assumption that find_ancestor needs to work correctly. However, since this is very subtle and not documented, the implementation of find_ancestor is instead changed here to a simpler version in which this assumption is not necessary and is arguably easier to see correct. * Regression tests for the previous point have been added. --- src/round.rs | 2 +- src/vote_graph.rs | 145 +++++++++++++++++++++++++--------------------- 2 files changed, 79 insertions(+), 68 deletions(-) diff --git a/src/round.rs b/src/round.rs index 186eee50..bcb85cb7 100644 --- a/src/round.rs +++ b/src/round.rs @@ -382,7 +382,6 @@ impl Round where let threshold = self.threshold(); if self.prevote.current_weight >= threshold { let equivocators = self.bitfield_context.equivocators(); - self.prevote_ghost = self.graph.find_ghost( self.prevote_ghost.take(), |v| v.total_weight(&equivocators, &self.voters).prevote >= threshold, @@ -422,6 +421,7 @@ impl Round where return Ok(import_result) }, }; + let round_number = self.round_number; match multiplicity { diff --git a/src/vote_graph.rs b/src/vote_graph.rs index 8a3ba49d..12c74948 100644 --- a/src/vote_graph.rs +++ b/src/vote_graph.rs @@ -22,8 +22,7 @@ use crate::std::{ use super::{Chain, Error, BlockNumberOps}; -#[cfg_attr(any(feature = "std", test), derive( -Debug))] +#[cfg_attr(any(feature = "std", test), derive(Debug))] struct Entry { number: N, // ancestor hashes in reverse order, e.g. ancestors[0] is the parent @@ -62,16 +61,6 @@ struct Subchain { } impl Subchain { - fn blocks_reverse<'a>(&'a self) -> impl Iterator + 'a { - let best = self.best_number; - let mut count = N::zero(); - self.hashes.iter().rev().cloned().map(move |x| { - let res = (x, best - count); - count = count + N::one(); - res - }) - } - fn best(&self) -> Option<(H, N)> { self.hashes.last().map(|x| (x.clone(), self.best_number)) } @@ -192,65 +181,65 @@ impl VoteGraph where Ok(()) } - /// Find the highest block which is either an ancestor of or equal to the given, which fulfills a - /// condition. - pub fn find_ancestor<'a, F>(&'a self, hash: H, number: N, condition: F) -> Option<(H, N)> + /// Find the block with the highest block number in the chain with the given head + /// which fulfills the given condition. + /// + /// Returns `None` if the given head is not in the graph or no node fulfills the + /// given condition. + pub fn find_ancestor<'a, F>(&'a self, mut hash: H, mut number: N, condition: F) -> Option<(H, N)> where F: Fn(&V) -> bool { - let entries = &self.entries; - let get_node = |hash: &_| -> &'a _ { - entries.get(hash) - .expect("node either base or referenced by other in graph; qed") - }; - - // we store two nodes with an edge between them that is the canonical - // chain. - // the `node_key` always points to the ancestor node, and the `canonical_node` - // points to the higher node. - let (mut node_key, mut canonical_node) = match self.find_containing_nodes(hash.clone(), number) { - None => { - let node = get_node(&hash); - if condition(&node.cumulative_vote) { - return Some((hash, number)) + loop { + match self.find_containing_nodes(hash.clone(), number) { + None => { + // The block has a vote-node in the graph. + let node = self.entries.get(&hash) + .expect("by defn of find_containing_nodes; qed"); + // If the weight is sufficient, we are done. + if condition(&node.cumulative_vote) { + return Some((hash, number)) + } + // Not enough weight, check the parent block. + match node.ancestors.iter().next() { + None => return None, + Some(a) => { + hash = a.clone(); + number = node.number - N::one(); + } + } } + Some(children) => { + // If there are no vote-nodes below the block in the graph, + // the block is not in the graph at all. + if children.is_empty() { + return None + } + // The block is "contained" in the graph (i.e. in the ancestry-chain + // of at least one vote-node) but does not itself have a vote-node. + // Check if the accumulated weight on all child vote-nodes is sufficient. + let mut v = V::default(); + for c in &children { + let e = self.entries.get(&c).expect("all children in graph; qed"); + v += e.cumulative_vote.clone(); + } + if condition(&v) { + return Some((hash, number)) + } - (node.ancestor_node()?, node) - } - Some(ref x) if !x.is_empty() => { - let node = get_node(&x[0]); - let key = node.ancestor_node() - .expect("node containing block in ancestry has ancestor node; qed"); - - (key, node) + // Not enough weight, check the parent block. + let child = children.last().expect("children not empty; qed"); + let entry = self.entries.get(child).expect("all children in graph; qed"); + let offset = (entry.number - number).as_(); + match entry.ancestors.get(offset) { + None => return None, // Reached base without sufficient weight. + Some(parent) => { + hash = parent.clone(); + number = number - N::one(); + } + } + } } - Some(_) => return None, - }; - - // search backwards until we find the first vote-node that - // meets the condition. - let mut active_node = get_node(&node_key); - while !condition(&active_node.cumulative_vote) { - node_key = match active_node.ancestor_node() { - Some(n) => n, - None => return None, - }; - - canonical_node = active_node; - active_node = get_node(&node_key); } - - // find the GHOST merge-point after the active_node. - // constrain it to be within the canonical chain. - let good_subchain = self.ghost_find_merge_point(node_key, active_node, None, condition); - - // FIXME: binding is required for some reason. - let mut blocks_reverse = good_subchain.blocks_reverse(); - - blocks_reverse.find(|&(ref good_hash, good_number)| { - canonical_node - .in_direct_ancestry(good_hash, good_number) - .unwrap_or(false) - }) } /// Find the best GHOST descendent of the given block. @@ -570,7 +559,6 @@ mod tests { assert_eq!(a_entry.descendents, vec!["E1", "F2"]); assert_eq!(a_entry.cumulative_vote, 300); - let e_entry = tracker.entries.get("E1").unwrap(); assert_eq!(e_entry.ancestor_node().unwrap(), "A"); assert_eq!(e_entry.cumulative_vote, 100); @@ -598,7 +586,7 @@ mod tests { tracker2.insert("F2", 7, 100, &chain).unwrap(); tracker2.insert("C", 4, 100, &chain).unwrap(); - for tracker in &[&tracker2] { + for tracker in &[&tracker1, &tracker2] { assert!(tracker.heads.contains("E1")); assert!(tracker.heads.contains("F2")); assert!(!tracker.heads.contains("C")); @@ -804,4 +792,27 @@ mod tests { assert_eq!(tracker.entries.get(GENESIS_HASH).unwrap().cumulative_vote, 15); } + + #[test] + fn find_ancestor_is_largest() { + let mut chain = DummyChain::new(); + let mut tracker = VoteGraph::<_,_,u32>::new(GENESIS_HASH, 0); + + chain.push_blocks(GENESIS_HASH, &["A"]); + chain.push_blocks(GENESIS_HASH, &["B"]); + chain.push_blocks("A", &["A1"]); + chain.push_blocks("A", &["A2"]); + chain.push_blocks("B", &["B1"]); + chain.push_blocks("B", &["B2"]); + + // Inserting the Bs first used to exhibit incorrect behaviour. + tracker.insert("B1", 2, 1, &chain).unwrap(); + tracker.insert("B2", 2, 1, &chain).unwrap(); + tracker.insert("A1", 2, 1, &chain).unwrap(); + tracker.insert("A2", 2, 1, &chain).unwrap(); + + let actual = tracker.find_ancestor("A", 1, |x| x >= &2).unwrap(); + // `actual` used to (incorrectly) be (genesis, 0) + assert_eq!(actual, ("A", 1)); + } }