From 11de12304eff2883eea18fe2fff26e244b644cd7 Mon Sep 17 00:00:00 2001
From: stasatdaglabs <39559713+stasatdaglabs@users.noreply.github.com>
Date: Mon, 27 Jan 2020 11:48:58 +0200
Subject: [PATCH] [NOD-700] Convert blockSet to map[*blockNode]struct{} (#604)

* [NOD-700] Convert blockSet to map[*blockNode]struct{}.

* [NOD-700] Rename bluestNode to bluestBlock in bluest().

* [NOD-700] Make IsInSelectedParentChain not use the now-slower containsHash.

* [NOD-700] Rename block to node in blockset.go.

* [NOD-700] Remove containsHash and hashesEqual.

* [NOD-700] Add a comment to IsInSelectedParentChain about how it'll fail if the given blockHash is not within the block index.
---
 blockdag/blocknode.go        |  2 +-
 blockdag/blockset.go         | 91 ++++++++++++++----------------------
 blockdag/blockset_test.go    | 51 --------------------
 blockdag/common_test.go      |  2 +-
 blockdag/dag.go              | 44 +++++++++++------
 blockdag/dag_test.go         |  8 +++-
 blockdag/ghostdag.go         |  4 +-
 blockdag/indexers/txindex.go |  6 ++-
 blockdag/validate.go         |  6 +--
 blockdag/validate_test.go    |  3 +-
 blockdag/virtualblock.go     |  2 +-
 server/rpc/common.go         |  6 ++-
 12 files changed, 91 insertions(+), 134 deletions(-)

diff --git a/blockdag/blocknode.go b/blockdag/blocknode.go
index ce529157c..63ac50692 100644
--- a/blockdag/blocknode.go
+++ b/blockdag/blocknode.go
@@ -148,7 +148,7 @@ func (dag *BlockDAG) newBlockNode(blockHeader *wire.BlockHeader, parents blockSe
 
 // updateParentsChildren updates the node's parents to point to new node
 func (node *blockNode) updateParentsChildren() {
-	for _, parent := range node.parents {
+	for parent := range node.parents {
 		parent.children.add(node)
 	}
 }
diff --git a/blockdag/blockset.go b/blockdag/blockset.go
index b6d7342c3..98d2e8420 100644
--- a/blockdag/blockset.go
+++ b/blockdag/blockset.go
@@ -7,38 +7,38 @@ import (
 )
 
 // blockSet implements a basic unsorted set of blocks
-type blockSet map[daghash.Hash]*blockNode
+type blockSet map[*blockNode]struct{}
 
 // newSet creates a new, empty BlockSet
 func newSet() blockSet {
-	return map[daghash.Hash]*blockNode{}
+	return map[*blockNode]struct{}{}
 }
 
-// setFromSlice converts a slice of blocks into an unordered set represented as map
-func setFromSlice(blocks ...*blockNode) blockSet {
+// setFromSlice converts a slice of blockNodes into an unordered set represented as map
+func setFromSlice(nodes ...*blockNode) blockSet {
 	set := newSet()
-	for _, block := range blocks {
-		set.add(block)
+	for _, node := range nodes {
+		set.add(node)
 	}
 	return set
 }
 
-// add adds a block to this BlockSet
-func (bs blockSet) add(block *blockNode) {
-	bs[*block.hash] = block
+// add adds a blockNode to this BlockSet
+func (bs blockSet) add(node *blockNode) {
+	bs[node] = struct{}{}
 }
 
-// remove removes a block from this BlockSet, if exists
-// Does nothing if this set does not contain the block
-func (bs blockSet) remove(block *blockNode) {
-	delete(bs, *block.hash)
+// remove removes a blockNode from this BlockSet, if exists
+// Does nothing if this set does not contain the blockNode
+func (bs blockSet) remove(node *blockNode) {
+	delete(bs, node)
 }
 
 // clone clones thie block set
 func (bs blockSet) clone() blockSet {
 	clone := newSet()
-	for _, block := range bs {
-		clone.add(block)
+	for node := range bs {
+		clone.add(node)
 	}
 	return clone
 }
@@ -46,29 +46,29 @@ func (bs blockSet) clone() blockSet {
 // subtract returns the difference between the BlockSet and another BlockSet
 func (bs blockSet) subtract(other blockSet) blockSet {
 	diff := newSet()
-	for _, block := range bs {
-		if !other.contains(block) {
-			diff.add(block)
+	for node := range bs {
+		if !other.contains(node) {
+			diff.add(node)
 		}
 	}
 	return diff
 }
 
-// addSet adds all blocks in other set to this set
+// addSet adds all blockNodes in other set to this set
 func (bs blockSet) addSet(other blockSet) {
-	for _, block := range other {
-		bs.add(block)
+	for node := range other {
+		bs.add(node)
 	}
 }
 
 // addSlice adds provided slice to this set
 func (bs blockSet) addSlice(slice []*blockNode) {
-	for _, block := range slice {
-		bs.add(block)
+	for _, node := range slice {
+		bs.add(node)
 	}
 }
 
-// union returns a BlockSet that contains all blocks included in this set,
+// union returns a BlockSet that contains all blockNodes included in this set,
 // the other set, or both
 func (bs blockSet) union(other blockSet) blockSet {
 	union := bs.clone()
@@ -78,39 +78,16 @@ func (bs blockSet) union(other blockSet) blockSet {
 	return union
 }
 
-// contains returns true iff this set contains block
-func (bs blockSet) contains(block *blockNode) bool {
-	_, ok := bs[*block.hash]
+// contains returns true iff this set contains node
+func (bs blockSet) contains(node *blockNode) bool {
+	_, ok := bs[node]
 	return ok
 }
 
-// containsHash returns true iff this set contains a block hash
-func (bs blockSet) containsHash(hash *daghash.Hash) bool {
-	_, ok := bs[*hash]
-	return ok
-}
-
-// hashesEqual returns true if the given hashes are equal to the hashes
-// of the blocks in this set.
-// NOTE: The given hash slice must not contain duplicates.
-func (bs blockSet) hashesEqual(hashes []*daghash.Hash) bool {
-	if len(hashes) != len(bs) {
-		return false
-	}
-
-	for _, hash := range hashes {
-		if _, wasFound := bs[*hash]; !wasFound {
-			return false
-		}
-	}
-
-	return true
-}
-
-// hashes returns the hashes of the blocks in this set.
+// hashes returns the hashes of the blockNodes in this set.
 func (bs blockSet) hashes() []*daghash.Hash {
 	hashes := make([]*daghash.Hash, 0, len(bs))
-	for _, node := range bs {
+	for node := range bs {
 		hashes = append(hashes, node.hash)
 	}
 	daghash.Sort(hashes)
@@ -119,15 +96,15 @@ func (bs blockSet) hashes() []*daghash.Hash {
 
 func (bs blockSet) String() string {
 	nodeStrs := make([]string, 0, len(bs))
-	for _, node := range bs {
+	for node := range bs {
 		nodeStrs = append(nodeStrs, node.String())
 	}
 	return strings.Join(nodeStrs, ",")
 }
 
-// anyChildInSet returns true iff any child of block is contained within this set
-func (bs blockSet) anyChildInSet(block *blockNode) bool {
-	for _, child := range block.children {
+// anyChildInSet returns true iff any child of node is contained within this set
+func (bs blockSet) anyChildInSet(node *blockNode) bool {
+	for child := range node.children {
 		if bs.contains(child) {
 			return true
 		}
@@ -139,7 +116,7 @@ func (bs blockSet) anyChildInSet(block *blockNode) bool {
 func (bs blockSet) bluest() *blockNode {
 	var bluestNode *blockNode
 	var maxScore uint64
-	for _, node := range bs {
+	for node := range bs {
 		if bluestNode == nil ||
 			node.blueScore > maxScore ||
 			(node.blueScore == maxScore && daghash.Less(node.hash, bluestNode.hash)) {
diff --git a/blockdag/blockset_test.go b/blockdag/blockset_test.go
index 23aff1127..60188cb4d 100644
--- a/blockdag/blockset_test.go
+++ b/blockdag/blockset_test.go
@@ -243,54 +243,3 @@ func TestBlockSetUnion(t *testing.T) {
 		}
 	}
 }
-
-func TestBlockSetHashesEqual(t *testing.T) {
-	node1 := &blockNode{hash: &daghash.Hash{10}}
-	node2 := &blockNode{hash: &daghash.Hash{20}}
-
-	tests := []struct {
-		name           string
-		set            blockSet
-		hashes         []*daghash.Hash
-		expectedResult bool
-	}{
-		{
-			name:           "empty set, no hashes",
-			set:            setFromSlice(),
-			hashes:         []*daghash.Hash{},
-			expectedResult: true,
-		},
-		{
-			name:           "empty set, one hash",
-			set:            setFromSlice(),
-			hashes:         []*daghash.Hash{node1.hash},
-			expectedResult: false,
-		},
-		{
-			name:           "set and hashes of different length",
-			set:            setFromSlice(node1, node2),
-			hashes:         []*daghash.Hash{node1.hash},
-			expectedResult: false,
-		},
-		{
-			name:           "set equal to hashes",
-			set:            setFromSlice(node1, node2),
-			hashes:         []*daghash.Hash{node1.hash, node2.hash},
-			expectedResult: true,
-		},
-		{
-			name:           "set equal to hashes, different order",
-			set:            setFromSlice(node1, node2),
-			hashes:         []*daghash.Hash{node2.hash, node1.hash},
-			expectedResult: true,
-		},
-	}
-
-	for _, test := range tests {
-		result := test.set.hashesEqual(test.hashes)
-		if result != test.expectedResult {
-			t.Errorf("blockSet.hashesEqual: unexpected result in test '%s'. "+
-				"Expected: %t, got: %t", test.name, test.expectedResult, result)
-		}
-	}
-}
diff --git a/blockdag/common_test.go b/blockdag/common_test.go
index 2207386cb..ef2e78eec 100644
--- a/blockdag/common_test.go
+++ b/blockdag/common_test.go
@@ -154,7 +154,7 @@ func newTestNode(dag *BlockDAG, parents blockSet, blockVersion int32, bits uint3
 }
 
 func addNodeAsChildToParents(node *blockNode) {
-	for _, parent := range node.parents {
+	for parent := range node.parents {
 		parent.children.add(node)
 	}
 }
diff --git a/blockdag/dag.go b/blockdag/dag.go
index ebc0f40e8..91303ac78 100644
--- a/blockdag/dag.go
+++ b/blockdag/dag.go
@@ -773,7 +773,7 @@ func (dag *BlockDAG) updateFinalityPoint() {
 
 func (dag *BlockDAG) finalizeNodesBelowFinalityPoint(deleteDiffData bool) {
 	queue := make([]*blockNode, 0, len(dag.lastFinalityPoint.parents))
-	for _, parent := range dag.lastFinalityPoint.parents {
+	for parent := range dag.lastFinalityPoint.parents {
 		queue = append(queue, parent)
 	}
 	var blockHashesToDelete []*daghash.Hash
@@ -788,7 +788,7 @@ func (dag *BlockDAG) finalizeNodesBelowFinalityPoint(deleteDiffData bool) {
 			if deleteDiffData {
 				blockHashesToDelete = append(blockHashesToDelete, current.hash)
 			}
-			for _, parent := range current.parents {
+			for parent := range current.parents {
 				queue = append(queue, parent)
 			}
 		}
@@ -1125,7 +1125,7 @@ func (node *blockNode) updateParentsDiffs(dag *BlockDAG, newBlockUTXO UTXOSet) e
 		return err
 	}
 
-	for _, parent := range node.parents {
+	for parent := range node.parents {
 		diffChild, err := dag.utxoDiffStore.diffChildByNode(parent)
 		if err != nil {
 			return err
@@ -1236,7 +1236,7 @@ func (dag *BlockDAG) restoreUTXO(node *blockNode) (UTXOSet, error) {
 
 // updateTipsUTXO builds and applies new diff UTXOs for all the DAG's tips
 func updateTipsUTXO(dag *BlockDAG, virtualUTXO UTXOSet) error {
-	for _, tip := range dag.virtual.parents {
+	for tip := range dag.virtual.parents {
 		tipUTXO, err := dag.restoreUTXO(tip)
 		if err != nil {
 			return err
@@ -1419,13 +1419,21 @@ func (dag *BlockDAG) acceptingBlock(node *blockNode) (*blockNode, error) {
 	}
 
 	// If the node is a chain-block itself, the accepting block is its chain-child
-	if dag.IsInSelectedParentChain(node.hash) {
+	isNodeInSelectedParentChain, err := dag.IsInSelectedParentChain(node.hash)
+	if err != nil {
+		return nil, err
+	}
+	if isNodeInSelectedParentChain {
 		if len(node.children) == 0 {
 			// If the node is the selected tip, it doesn't have an accepting block
 			return nil, nil
 		}
-		for _, child := range node.children {
-			if dag.IsInSelectedParentChain(child.hash) {
+		for child := range node.children {
+			isChildInSelectedParentChain, err := dag.IsInSelectedParentChain(child.hash)
+			if err != nil {
+				return nil, err
+			}
+			if isChildInSelectedParentChain {
 				return child, nil
 			}
 		}
@@ -1469,11 +1477,17 @@ func (dag *BlockDAG) oldestChainBlockWithBlueScoreGreaterThan(blueScore uint64)
 }
 
 // IsInSelectedParentChain returns whether or not a block hash is found in the selected
-// parent chain.
+// parent chain. Note that this method returns an error if the given blockHash does not
+// exist within the block index.
 //
 // This method MUST be called with the DAG lock held
-func (dag *BlockDAG) IsInSelectedParentChain(blockHash *daghash.Hash) bool {
-	return dag.virtual.selectedParentChainSet.containsHash(blockHash)
+func (dag *BlockDAG) IsInSelectedParentChain(blockHash *daghash.Hash) (bool, error) {
+	blockNode := dag.index.LookupNode(blockHash)
+	if blockNode == nil {
+		str := fmt.Sprintf("block %s is not in the DAG", blockHash)
+		return false, errNotInDAG(str)
+	}
+	return dag.virtual.selectedParentChainSet.contains(blockNode), nil
 }
 
 // SelectedParentChain returns the selected parent chain starting from blockHash (exclusive)
@@ -1494,7 +1508,11 @@ func (dag *BlockDAG) SelectedParentChain(blockHash *daghash.Hash) ([]*daghash.Ha
 	// If blockHash is not in the selected parent chain, go down its selected parent chain
 	// until we find a block that is in the main selected parent chain.
 	var removedChainHashes []*daghash.Hash
-	for !dag.IsInSelectedParentChain(blockHash) {
+	isBlockInSelectedParentChain, err := dag.IsInSelectedParentChain(blockHash)
+	if err != nil {
+		return nil, nil, err
+	}
+	for !isBlockInSelectedParentChain {
 		removedChainHashes = append(removedChainHashes, blockHash)
 
 		node := dag.index.LookupNode(blockHash)
@@ -1544,7 +1562,7 @@ func (dag *BlockDAG) TipHashes() []*daghash.Hash {
 func (dag *BlockDAG) CurrentBits() uint32 {
 	tips := dag.virtual.tips()
 	minBits := uint32(math.MaxUint32)
-	for _, tip := range tips {
+	for tip := range tips {
 		if minBits > tip.Header().Bits {
 			minBits = tip.Header().Bits
 		}
@@ -1665,7 +1683,7 @@ func (dag *BlockDAG) antiPastBetween(lowHash, highHash *daghash.Hash, maxEntries
 			continue
 		}
 		candidateNodes.Push(current)
-		for _, parent := range current.parents {
+		for parent := range current.parents {
 			queue.Push(parent)
 		}
 	}
diff --git a/blockdag/dag_test.go b/blockdag/dag_test.go
index 27db09c6c..73b4846d4 100644
--- a/blockdag/dag_test.go
+++ b/blockdag/dag_test.go
@@ -723,7 +723,7 @@ func TestConfirmations(t *testing.T) {
 
 	// Check that each of the tips has a 0 confirmations
 	tips := dag.virtual.tips()
-	for _, tip := range tips {
+	for tip := range tips {
 		tipConfirmations, err := dag.blockConfirmations(tip)
 		if err != nil {
 			t.Fatalf("TestConfirmations: confirmations for tip unexpectedly failed: %s", err)
@@ -828,7 +828,11 @@ func TestAcceptingBlock(t *testing.T) {
 	branchingChainTip := prepareAndProcessBlock(t, dag, chainBlocks[len(chainBlocks)-3])
 
 	// Make sure that branchingChainTip is not in the selected parent chain
-	if dag.IsInSelectedParentChain(branchingChainTip.BlockHash()) {
+	isBranchingChainTipInSelectedParentChain, err := dag.IsInSelectedParentChain(branchingChainTip.BlockHash())
+	if err != nil {
+		t.Fatalf("TestAcceptingBlock: IsInSelectedParentChain unexpectedly failed: %s", err)
+	}
+	if isBranchingChainTipInSelectedParentChain {
 		t.Fatalf("TestAcceptingBlock: branchingChainTip wasn't expected to be in the selected parent chain")
 	}
 
diff --git a/blockdag/ghostdag.go b/blockdag/ghostdag.go
index 44ac34b37..0644820ba 100644
--- a/blockdag/ghostdag.go
+++ b/blockdag/ghostdag.go
@@ -131,7 +131,7 @@ func (dag *BlockDAG) selectedParentAnticone(node *blockNode) ([]*blockNode, erro
 	selectedParentPast := newSet()
 	var queue []*blockNode
 	// Queueing all parents (other than the selected parent itself) for processing.
-	for _, parent := range node.parents {
+	for parent := range node.parents {
 		if parent == node.selectedParent {
 			continue
 		}
@@ -144,7 +144,7 @@ func (dag *BlockDAG) selectedParentAnticone(node *blockNode) ([]*blockNode, erro
 		current, queue = queue[0], queue[1:]
 		// For each parent of a the current node we check whether it is in the past of the selected parent. If not,
 		// we add the it to the resulting anticone-set and queue it for further processing.
-		for _, parent := range current.parents {
+		for parent := range current.parents {
 			if anticoneSet.contains(parent) || selectedParentPast.contains(parent) {
 				continue
 			}
diff --git a/blockdag/indexers/txindex.go b/blockdag/indexers/txindex.go
index c0658fac7..10068bd31 100644
--- a/blockdag/indexers/txindex.go
+++ b/blockdag/indexers/txindex.go
@@ -389,7 +389,11 @@ func dbFetchTxAcceptingBlock(dbTx database.Tx, txID *daghash.TxID, dag *blockdag
 		if err != nil {
 			return nil, err
 		}
-		if dag.IsInSelectedParentChain(blockHash) {
+		isBlockInSelectedParentChain, err := dag.IsInSelectedParentChain(blockHash)
+		if err != nil {
+			return nil, err
+		}
+		if isBlockInSelectedParentChain {
 			return blockHash, nil
 		}
 	}
diff --git a/blockdag/validate.go b/blockdag/validate.go
index 5bd6ecbc1..0a76d4b8f 100644
--- a/blockdag/validate.go
+++ b/blockdag/validate.go
@@ -602,7 +602,7 @@ func validateParents(blockHeader *wire.BlockHeader, parents blockSet) error {
 	minBlueScore := uint64(math.MaxUint64)
 	queue := newDownHeap()
 	visited := newSet()
-	for _, parent := range parents {
+	for parent := 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.
@@ -612,7 +612,7 @@ func validateParents(blockHeader *wire.BlockHeader, parents blockSet) error {
 		if parent.blueScore < minBlueScore {
 			minBlueScore = parent.blueScore
 		}
-		for _, grandParent := range parent.parents {
+		for grandParent := range parent.parents {
 			if !visited.contains(grandParent) {
 				queue.Push(grandParent)
 				visited.add(grandParent)
@@ -628,7 +628,7 @@ func validateParents(blockHeader *wire.BlockHeader, parents blockSet) error {
 				blockHeader.BlockHash()))
 		}
 		if current.blueScore > minBlueScore {
-			for _, parent := range current.parents {
+			for parent := range current.parents {
 				if !visited.contains(parent) {
 					queue.Push(parent)
 					visited.add(parent)
diff --git a/blockdag/validate_test.go b/blockdag/validate_test.go
index 7ac60e60b..59660a098 100644
--- a/blockdag/validate_test.go
+++ b/blockdag/validate_test.go
@@ -124,7 +124,8 @@ func TestCheckConnectBlockTemplate(t *testing.T) {
 	}
 
 	blockNode3 := dag.index.LookupNode(blocks[3].Hash())
-	if blockNode3.children.containsHash(blocks[4].Hash()) {
+	blockNode4 := dag.index.LookupNode(blocks[4].Hash())
+	if blockNode3.children.contains(blockNode4) {
 		t.Errorf("Block 4 wasn't successfully detached as a child from block3")
 	}
 
diff --git a/blockdag/virtualblock.go b/blockdag/virtualblock.go
index 40949d13b..0231f4c01 100644
--- a/blockdag/virtualblock.go
+++ b/blockdag/virtualblock.go
@@ -134,7 +134,7 @@ func (v *virtualBlock) SetTips(tips blockSet) {
 // This function MUST be called with the view mutex locked (for writes).
 func (v *virtualBlock) addTip(newTip *blockNode) *chainUpdates {
 	updatedTips := v.tips().clone()
-	for _, parent := range newTip.parents {
+	for parent := range newTip.parents {
 		updatedTips.remove(parent)
 	}
 
diff --git a/server/rpc/common.go b/server/rpc/common.go
index 9e5bfe53c..c93b84949 100644
--- a/server/rpc/common.go
+++ b/server/rpc/common.go
@@ -240,7 +240,11 @@ func buildGetBlockVerboseResult(s *Server, block *util.Block, isVerboseTx bool)
 		selectedParentHashStr = selectedParentHash.String()
 	}
 
-	isChainBlock := s.cfg.DAG.IsInSelectedParentChain(hash)
+	isChainBlock, err := s.cfg.DAG.IsInSelectedParentChain(hash)
+	if err != nil {
+		context := "Could not get whether block is in the selected parent chain"
+		return nil, internalRPCError(err.Error(), context)
+	}
 
 	result := &rpcmodel.GetBlockVerboseResult{
 		Hash:                 hash.String(),