Vote Graph and GHOST-hunting by rphmeier · Pull Request #1 · paritytech/finality-grandpa

rphmeier · 2018-08-08T12:15:36Z

This PR contains the logic for imposing a further DAG on top of the blockchain, where nodes are blocks which have been voted on directly.

Cumulative votes (flowing back to the "base" -- in practice, the last finalized block) are kept updated, and a "GHOST" calculation can be done on this DAG to find the highest block referenced in a node or edge of the DAG that fulfills some vote-predicate. This block may fall between some vote-nodes.

The GHOST logic, as it stands right now, operates under the assumption that the predicate will evaluate to true for at most one branch of any fork. Full GHOST is more expensive to implement as we have to compare the cumulative scores of all children and can't early-exit upon finding the first child which satisfies the predicate.

gnunicorn

Just a few minor changes, then feel free to merge it.

gnunicorn · 2018-08-08T15:11:42Z

src/lib.rs

+// Copyright 2018 Parity Technologies (UK) Ltd.
+// This file is part of finality-afg.
+
+// Polkadot is free software: you can redistribute it and/or modify


Shouldn't this be finality-afg rather than Polkadot?

gnunicorn · 2018-08-08T15:11:47Z

src/lib.rs

+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// Polkadot is distributed in the hope that it will be useful,


gnunicorn · 2018-08-08T15:21:49Z

src/vote_graph.rs

+		match self.find_containing_nodes(hash.clone(), number) {
+			Some(containing) => if containing.is_empty() {
+				self.append(hash.clone(), number, chain)?;
+			} else {


Shouldn't this be indented one level more rather than be on the level as the match-arms?

I don't think so but open for discussion

gnunicorn · 2018-08-08T15:29:39Z

src/vote_graph.rs

+	// given a key, node pair (which must correspond), assuming this node fulfills the condition,
+	// this function will find the highest point at which its descendents merge, which may be the
+	// node itself.
+	fn ghost_find_merge_point<'a, F>(&'a self, mut node_key: H, mut active_node: &'a Entry<H, V>, condition: F) -> (H, usize)


please wrap.

gnunicorn · 2018-08-08T15:33:20Z

src/vote_graph.rs

+						Ok(idx) => {
+							descendent_blocks[idx].1 += d_node.cumulative_vote.clone();
+							if condition(&descendent_blocks[idx].1) {
+								new_best = Some(d_block.clone());


uui.... 7 indentation levels... I can still follow, but it is a bit against the style guide ;)

yep, it's not ideal but I couldn't find a way to rewrite.

andresilva · 2018-08-08T16:34:54Z

I'm still reviewing this, please don't merge it yet.

andresilva

Some questions and comments to help me understand the code.

andresilva · 2018-08-08T17:32:25Z

src/vote_graph.rs

+/// Maintains a DAG of blocks in the chain which have votes attached to them,
+/// and vote data which is accumulated along edges.
+pub struct VoteGraph<H: Hash + Eq, V> {
+	entries: HashMap<H, Entry<H, V>>,


We assume that the same hash won't be proposed with different block numbers. I think it's safe to assume this since the block hash will usually depend on the block number, but just wanted to confirm my assumption.

true. I think it's fair to treat hashes as unique

andresilva · 2018-08-08T17:33:25Z

src/vote_graph.rs

+			maybe_entry
+		});
+
+		if let Some(new_entry) = produced_entry {


If descendents is not empty, then this must be Some.

indeed. but what am I supposed to do, panic?

No, I think it's fine as is, it was just a comment regarding my understanding.

andresilva · 2018-08-08T18:43:56Z

src/vote_graph.rs

+
+		let mut active_node = get_node(&node_key);
+
+		if !condition(&active_node.cumulative_vote) { return None }


We take the parent vote node of the current block descendants. If this block isn't finalized shouldn't we keep searching backwards for another GHOST block, potentially until we reach the base?

this function is defined to always return with a descendent of the supplied node. this question cuts to the crux of the issue mentioned in the PR description and comment on this function: that find_ghost isn't a proper GHOST but a weird "majority-GHOST" thing where you either have one descendent of a node that meets the condition or none. So we find the best block that meets the condition or none and we don't need to backtrack because of the majority condition.

to note why the property of returning a descendent is useful: this function is needed for the vote-casting logic, where we want to cast votes either after some time has passed or as soon as GHOST(S) == GHOST(S + Y) where Y could be any new valid votes to add to the vote set.

And as the document shows, the GHOST block never backtracks as we include more votes. So by bookkeeping the current GHOST(S) node we can easily check GHOST(S + Y) with another call to the same function. Although it can still be optimized a bit it should still be pretty fast.

andresilva · 2018-08-08T18:55:14Z

src/vote_graph.rs

+			}
+		}
+
+		// active_node and node_key now correspond to the head node which has enough cumulative votes.


correspond to the highest vote node with enough cumulative votes?

andresilva · 2018-08-08T19:15:48Z

src/vote_graph.rs

+		for offset in 1usize.. {
+			let mut new_best = None;
+			for d_node in descendent_nodes.iter() {
+				if let Some(d_block) = d_node.ancestor_block(base_number + offset) {


We return None if the block is past the parent vote-node of d_node. It's ok to ignore it since the parent vote-node of d_node should eventually be reached (if needed) as the offset grows.

the parent vote-node of d_node will always be the GHOST vote-node. ancestor_block will return None when we have reached the block corresponding to d_node, at which point we will want to prune anyway.

andresilva · 2018-08-08T19:25:25Z

src/vote_graph.rs

+
+			descendent_blocks.clear();
+			descendent_nodes.retain(
+				|n| n.in_direct_ancestry(&best_hash, best_number).unwrap_or(false)


Shouldn't this be unwrap_or(true)? We're removing nodes that are too high compared to the current best GHOST node, i.e. there are other vote nodes in the middle. It doesn't necessarily mean that they're not part of this branch.

If we get None back, then we've exhausted all the blocks in the "edge" between our GHOST vote-node and the descendent.

Since we're looking for an ancestor-block of the descendent vote-node's block which has enough cumulative votes, once we've reached this point we know that it's safe to remove the descendent node from our frontier, as we've checked all ancestors of the descendent vote-node already.

The other theoretical case (which cannot occur in this part of the code) where in_direct_ancestry returns None is where the block is earlier than the best GHOST vote-node. But we don't care about that case anyway since we're only looking for descendent blocks of that node.

andresilva · 2018-08-08T19:29:02Z

Really clean code 👍 It's possible I misunderstood some parts of how this works or what invariants hold.

andresilva · 2018-08-08T19:34:41Z

src/vote_graph.rs

+	/// The GHOST (hash, number) returned will be the block with highest number for which the
+	/// cumulative votes of descendents and itself causes the closure to evaluate to true.
+	///
+	/// This assumes that the evaluation closure is one which returns true for at most a single


This holds assuming an honest majority will follow the heaviest chain.

I think it holds as long as nobody double-votes (or in the case of supermajority, less than 1/3 double-vote). But I intend to filter out all double-votes in a higher level that instantiates the graph in a follow-up PR, so the property should always hold in our use-case.

I would like more unit tests to ensure that my logic is sound here

andresilva

LGTM. Thanks for the detailed explanations regarding my questions. I'll review the document again to further my understanding. 👍

rphmeier added 6 commits August 8, 2018 14:06

initial stab at GHOST

de8df89

polish up append/branch logic

8a89002

add tests and fix some minor bugs

b80e9a3

factor out to vote_graph implementation and generic vote type

cfc19c4

GHOST finding routine

61c27e8

fix doc and add error type

e21b418

rphmeier added the A0-pleasereview label Aug 8, 2018

gnunicorn approved these changes Aug 8, 2018

View reviewed changes

andresilva reviewed Aug 8, 2018

View reviewed changes

address grumbles

e8454bb

gnunicorn approved these changes Aug 9, 2018

View reviewed changes

andresilva approved these changes Aug 9, 2018

View reviewed changes

rphmeier merged commit 39a73f4 into master Aug 14, 2018

rphmeier deleted the rh-vote-graph branch August 14, 2018 11:13

rphmeier mentioned this pull request Sep 26, 2022

Member Request polkadot-fellows/seeding#4

Merged


		let mut active_node = get_node(&node_key);

		if !condition(&active_node.cumulative_vote) { return None }

Conversation

rphmeier commented Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnunicorn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andresilva commented Aug 8, 2018

Uh oh!

andresilva left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andresilva commented Aug 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andresilva left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rphmeier commented Aug 8, 2018 •

edited

Loading