[NOD-1035] Use reachability to check finality (#763)

* [NOD-1034] Use reachability to check finality

* [NOD-1034] Add comments and rename variables

* [NOD-1034] Fix comments

* [NOD-1034] Rename checkFinalityRules->checkFinalityViolation

* [NOD-1034] Change isAncestorOf to be exclusive

* [NOD-1034] Make isAncestorOf exclusive and also more explicit, and add TestReachabilityTreeNodeIsAncestorOf

blockdag/dag.go

@@ -566,7 +566,7 @@ func (dag *BlockDAG) connectBlock(node *blockNode,
 		}
 	}
 
-	if err := dag.checkFinalityRules(node); err != nil {
+	if err := dag.checkFinalityViolation(node); err != nil {
 		return nil, err
 	}
 
@@ -816,20 +816,45 @@ func (dag *BlockDAG) LastFinalityPointHash() *daghash.Hash {
 	return dag.lastFinalityPoint.hash
 }
 
-// checkFinalityRules checks the new block does not violate the finality rules
+// isInSelectedParentChain returns whether aNode is in the selected parent chain of bNode.
-// specifically - the new block selectedParent chain should contain the old finality point
+func (dag *BlockDAG) isInSelectedParentChain(aNode, bNode *blockNode) (bool, error) {
-func (dag *BlockDAG) checkFinalityRules(newNode *blockNode) error {
+	aTreeNode, err := dag.reachabilityStore.treeNodeByBlockNode(aNode)
+	if err != nil {
+		return false, err
+	}
+
+	bTreeNode, err := dag.reachabilityStore.treeNodeByBlockNode(bNode)
+	if err != nil {
+		return false, err
+	}
+
+	return aTreeNode.interval.isAncestorOf(bTreeNode.interval), nil
+}
+
+// checkFinalityViolation checks the new block does not violate the finality rules
+// specifically - the new block selectedParent chain should contain the old finality point.
+func (dag *BlockDAG) checkFinalityViolation(newNode *blockNode) error {
 	// the genesis block can not violate finality rules
 	if newNode.isGenesis() {
 		return nil
 	}
 
-	for currentNode := newNode; currentNode != dag.lastFinalityPoint; currentNode = currentNode.selectedParent {
+	// Because newNode doesn't have reachability data we
-		// If we went past dag's last finality point without encountering it -
+	// need to check if the last finality point is in the
-		// the new block has violated finality.
+	// selected parent chain of newNode.selectedParent, so
-		if currentNode.blueScore <= dag.lastFinalityPoint.blueScore {
+	// we explicitly check if newNode.selectedParent is
-			return ruleError(ErrFinality, "The last finality point is not in the selected chain of this block")
+	// the finality point.
-		}
+	if dag.lastFinalityPoint == newNode.selectedParent {
+		return nil
+	}
+
+	isInSelectedChain, err := dag.isInSelectedParentChain(dag.lastFinalityPoint, newNode.selectedParent)
+	if err != nil {
+		return err
+	}
+
+	if !isInSelectedChain {
+		return ruleError(ErrFinality, "the last finality point is not in the selected parent chain of this block")
 	}
 	return nil
 }
@@ -894,7 +919,7 @@ func (dag *BlockDAG) finalizeNodesBelowFinalityPoint(deleteDiffData bool) {
 // IsKnownFinalizedBlock returns whether the block is below the finality point.
 // IsKnownFinalizedBlock might be false-negative because node finality status is
 // updated in a separate goroutine. To get a definite answer if a block
-// is finalized or not, use dag.checkFinalityRules.
+// is finalized or not, use dag.checkFinalityViolation.
 func (dag *BlockDAG) IsKnownFinalizedBlock(blockHash *daghash.Hash) bool {
 	node, ok := dag.index.LookupNode(blockHash)
 	return ok && node.isFinalized

blockdag/reachability.go

@@ -155,7 +155,11 @@ func exponentialFractions(sizes []uint64) []float64 {
 // property of reachability intervals that intervals are either completely disjoint,
 // or one strictly contains the other.
 func (ri *reachabilityInterval) isAncestorOf(other *reachabilityInterval) bool {
-	return ri.start <= other.end && other.end <= ri.end
+	// An interval is not an ancestor of itself.
+	if ri.start == other.start && ri.end == other.end {
+		return false
+	}
+	return ri.start <= other.start && other.end <= ri.end
 }
 
 // String returns a string representation of the interval.

blockdag/reachability_test.go

@@ -57,7 +57,7 @@ func TestAddChild(t *testing.T) {
 
 	// Expect all nodes to be descendant nodes of root
 	currentNode := currentTip
-	for currentNode != nil {
+	for currentNode != root {
 		if !root.isAncestorOf(currentNode) {
 			t.Fatalf("TestAddChild: currentNode is not a descendant of root")
 		}
@@ -118,6 +118,91 @@ func TestAddChild(t *testing.T) {
 	}
 }
 
+func TestReachabilityTreeNodeIsAncestorOf(t *testing.T) {
+	root := newReachabilityTreeNode(&blockNode{})
+	currentTip := root
+	const numberOfDescendants = 6
+	descendants := make([]*reachabilityTreeNode, numberOfDescendants)
+	for i := 0; i < numberOfDescendants; i++ {
+		node := newReachabilityTreeNode(&blockNode{})
+		_, err := currentTip.addChild(node)
+		if err != nil {
+			t.Fatalf("TestReachabilityTreeNodeIsAncestorOf: addChild failed: %s", err)
+		}
+		descendants[i] = node
+		currentTip = node
+	}
+
+	// Expect all descendants to be in the future of root
+	for _, node := range descendants {
+		if !root.isAncestorOf(node) {
+			t.Fatalf("TestReachabilityTreeNodeIsAncestorOf: node is not a descendant of root")
+		}
+	}
+
+	if root.isAncestorOf(root) {
+		t.Fatalf("TestReachabilityTreeNodeIsAncestorOf: root is not expected to be a descendant of root")
+	}
+}
+
+func TestIntervalIsAncestorOf(t *testing.T) {
+	tests := []struct {
+		name                  string
+		this, other           *reachabilityInterval
+		isThisAncestorOfOther bool
+	}{
+		{
+			name:                  "this == other",
+			this:                  newReachabilityInterval(10, 100),
+			other:                 newReachabilityInterval(10, 100),
+			isThisAncestorOfOther: false,
+		},
+		{
+			name:                  "this.start == other.start && this.end < other.end",
+			this:                  newReachabilityInterval(10, 90),
+			other:                 newReachabilityInterval(10, 100),
+			isThisAncestorOfOther: false,
+		},
+		{
+			name:                  "this.start == other.start && this.end > other.end",
+			this:                  newReachabilityInterval(10, 100),
+			other:                 newReachabilityInterval(10, 90),
+			isThisAncestorOfOther: true,
+		},
+		{
+			name:                  "this.start > other.start && this.end == other.end",
+			this:                  newReachabilityInterval(20, 100),
+			other:                 newReachabilityInterval(10, 100),
+			isThisAncestorOfOther: false,
+		},
+		{
+			name:                  "this.start < other.start && this.end == other.end",
+			this:                  newReachabilityInterval(10, 100),
+			other:                 newReachabilityInterval(20, 100),
+			isThisAncestorOfOther: true,
+		},
+		{
+			name:                  "this.start > other.start && this.end < other.end",
+			this:                  newReachabilityInterval(20, 90),
+			other:                 newReachabilityInterval(10, 100),
+			isThisAncestorOfOther: false,
+		},
+		{
+			name:                  "this.start < other.start && this.end > other.end",
+			this:                  newReachabilityInterval(10, 100),
+			other:                 newReachabilityInterval(20, 90),
+			isThisAncestorOfOther: true,
+		},
+	}
+
+	for _, test := range tests {
+		if isAncestorOf := test.this.isAncestorOf(test.other); isAncestorOf != test.isThisAncestorOfOther {
+			t.Errorf("test.this.isAncestorOf(test.other) is expected to be %t but got %t",
+				test.isThisAncestorOfOther, isAncestorOf)
+		}
+	}
+}
+
 func TestSplitFraction(t *testing.T) {
 	tests := []struct {
 		interval      *reachabilityInterval

blockdag/validate.go

@@ -698,7 +698,7 @@ func (dag *BlockDAG) validateParents(blockHeader *wire.BlockHeader, parents bloc
 	for parentA := range parents {
 		// isFinalized might be false-negative because node finality status is
 		// updated in a separate goroutine. This is why later the block is
-		// checked more thoroughly on the finality rules in dag.checkFinalityRules.
+		// checked more thoroughly on the finality rules in dag.checkFinalityViolation.
 		if parentA.isFinalized {
 			return ruleError(ErrFinality, fmt.Sprintf("block %s is a finalized "+
 				"parent of block %s", parentA.hash, blockHeader.BlockHash()))