From 895f67a8d4a31e5782ee1b5784bc7ca8295f7a0d Mon Sep 17 00:00:00 2001 From: Ori Newman Date: Mon, 22 Jun 2020 17:14:59 +0300 Subject: [PATCH] [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 | 47 ++++++++++++++----- blockdag/reachability.go | 6 ++- blockdag/reachability_test.go | 87 ++++++++++++++++++++++++++++++++++- blockdag/validate.go | 2 +- 4 files changed, 128 insertions(+), 14 deletions(-) diff --git a/blockdag/dag.go b/blockdag/dag.go index 4493c5ae4..ab3022b9d 100644 --- a/blockdag/dag.go +++ b/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 -// specifically - the new block selectedParent chain should contain the old finality point -func (dag *BlockDAG) checkFinalityRules(newNode *blockNode) error { +// isInSelectedParentChain returns whether aNode is in the selected parent chain of bNode. +func (dag *BlockDAG) isInSelectedParentChain(aNode, bNode *blockNode) (bool, 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 { - // If we went past dag's last finality point without encountering it - - // the new block has violated finality. - if currentNode.blueScore <= dag.lastFinalityPoint.blueScore { - return ruleError(ErrFinality, "The last finality point is not in the selected chain of this block") - } + // Because newNode doesn't have reachability data we + // need to check if the last finality point is in the + // selected parent chain of newNode.selectedParent, so + // we explicitly check if newNode.selectedParent is + // 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 diff --git a/blockdag/reachability.go b/blockdag/reachability.go index 8f785c0d4..e16544826 100644 --- a/blockdag/reachability.go +++ b/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. diff --git a/blockdag/reachability_test.go b/blockdag/reachability_test.go index c2d8c1e84..e7ab60219 100644 --- a/blockdag/reachability_test.go +++ b/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 diff --git a/blockdag/validate.go b/blockdag/validate.go index ff6326924..7103986b1 100644 --- a/blockdag/validate.go +++ b/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()))