[NOD-488] Make getBlueBlocksBetween return error if startHash is not …

…in selected parent chain of stopHash (#514)

* [NOD-488] Make getBlueBlocksBetween return error if start hash is not in the selected parent chain of stop hash

* [NOD-488] Convert for to while style

blockdag/dag.go

@@ -1666,23 +1666,26 @@ func (dag *BlockDAG) IntervalBlockHashes(endHash *daghash.Hash, interval uint64,
 // provided max number of block hashes.
 //
 // This function MUST be called with the DAG state lock held (for reads).
-func (dag *BlockDAG) getBlueBlocksHashesBetween(startHash, stopHash *daghash.Hash, maxHashes uint64) []*daghash.Hash {
-	nodes := dag.getBlueBlocksBetween(startHash, stopHash, maxHashes)
+func (dag *BlockDAG) getBlueBlocksHashesBetween(startHash, stopHash *daghash.Hash, maxHashes uint64) ([]*daghash.Hash, error) {
+	nodes, err := dag.getBlueBlocksBetween(startHash, stopHash, maxHashes)
+	if err != nil {
+		return nil, err
+	}
 	hashes := make([]*daghash.Hash, len(nodes))
 	for i, node := range nodes {
 		hashes[i] = node.hash
 	}
-	return hashes
+	return hashes, nil
 }
 
-func (dag *BlockDAG) getBlueBlocksBetween(startHash, stopHash *daghash.Hash, maxEntries uint64) []*blockNode {
+func (dag *BlockDAG) getBlueBlocksBetween(startHash, stopHash *daghash.Hash, maxEntries uint64) ([]*blockNode, error) {
 	startNode := dag.index.LookupNode(startHash)
 	if startNode == nil {
-		return nil
+		return nil, errors.Errorf("Couldn't find start hash %s", startHash)
 	}
 	stopNode := dag.index.LookupNode(stopHash)
 	if stopNode == nil {
-		stopNode = dag.selectedTip()
+		return nil, errors.Errorf("Couldn't find stop hash %s", stopHash)
 	}
 
 	// In order to get no more then maxEntries of blue blocks from
@@ -1703,42 +1706,54 @@ func (dag *BlockDAG) getBlueBlocksBetween(startHash, stopHash *daghash.Hash, max
 	// Populate and return the found nodes.
 	nodes := make([]*blockNode, 0, stopNode.blueScore-startNode.blueScore+1)
 	nodes = append(nodes, stopNode)
-	for current := stopNode; current != startNode; current = current.selectedParent {
+	current := stopNode
+	for current.blueScore > startNode.blueScore {
 		for _, blue := range current.blues {
 			nodes = append(nodes, blue)
 		}
+		current = current.selectedParent
+	}
+	if current != startNode {
+		return nil, errors.Errorf("the start hash is not found in the " +
+			"selected parent chain of the stop hash")
 	}
 	reversedNodes := make([]*blockNode, len(nodes))
 	for i, node := range nodes {
 		reversedNodes[len(reversedNodes)-i-1] = node
 	}
-	return reversedNodes
+	return reversedNodes, nil
 }
 
 // GetBlueBlocksHashesBetween returns the hashes of the blue blocks after the
 // provided start hash until the provided stop hash is reached, or up to the
 // provided max number of block hashes.
 //
 // This function is safe for concurrent access.
-func (dag *BlockDAG) GetBlueBlocksHashesBetween(startHash, stopHash *daghash.Hash, maxHashes uint64) []*daghash.Hash {
+func (dag *BlockDAG) GetBlueBlocksHashesBetween(startHash, stopHash *daghash.Hash, maxHashes uint64) ([]*daghash.Hash, error) {
 	dag.dagLock.RLock()
-	hashes := dag.getBlueBlocksHashesBetween(startHash, stopHash, maxHashes)
+	hashes, err := dag.getBlueBlocksHashesBetween(startHash, stopHash, maxHashes)
+	if err != nil {
+		return nil, err
+	}
 	dag.dagLock.RUnlock()
-	return hashes
+	return hashes, nil
 }
 
 // getBlueBlocksHeadersBetween returns the headers of the blue blocks after the
 // provided start hash until the provided stop hash is reached, or up to the
 // provided max number of block headers.
 //
 // This function MUST be called with the DAG state lock held (for reads).
-func (dag *BlockDAG) getBlueBlocksHeadersBetween(startHash, stopHash *daghash.Hash, maxHeaders uint64) []*wire.BlockHeader {
-	nodes := dag.getBlueBlocksBetween(startHash, stopHash, maxHeaders)
+func (dag *BlockDAG) getBlueBlocksHeadersBetween(startHash, stopHash *daghash.Hash, maxHeaders uint64) ([]*wire.BlockHeader, error) {
+	nodes, err := dag.getBlueBlocksBetween(startHash, stopHash, maxHeaders)
+	if err != nil {
+		return nil, err
+	}
 	headers := make([]*wire.BlockHeader, len(nodes))
 	for i, node := range nodes {
 		headers[i] = node.Header()
 	}
-	return headers
+	return headers, nil
 }
 
 // GetTopHeaders returns the top wire.MaxBlockHeadersPerMsg block headers ordered by height.
@@ -1792,11 +1807,14 @@ func (dag *BlockDAG) RUnlock() {
 // provided max number of block headers.
 //
 // This function is safe for concurrent access.
-func (dag *BlockDAG) GetBlueBlocksHeadersBetween(startHash, stopHash *daghash.Hash) []*wire.BlockHeader {
+func (dag *BlockDAG) GetBlueBlocksHeadersBetween(startHash, stopHash *daghash.Hash) ([]*wire.BlockHeader, error) {
 	dag.dagLock.RLock()
-	headers := dag.getBlueBlocksHeadersBetween(startHash, stopHash, wire.MaxBlockHeadersPerMsg)
+	headers, err := dag.getBlueBlocksHeadersBetween(startHash, stopHash, wire.MaxBlockHeadersPerMsg)
+	if err != nil {
+		return nil, err
+	}
 	dag.dagLock.RUnlock()
-	return headers
+	return headers, nil
 }
 
 // SubnetworkID returns the node's subnetwork ID

go.mod

@@ -22,6 +22,7 @@ require (
 	github.com/kkdai/bstream v0.0.0-20181106074824-b3251f7901ec
 	github.com/miekg/dns v1.1.6
 	github.com/pkg/errors v0.8.1
+	github.com/prometheus/common v0.2.0
 	github.com/stretchr/testify v1.4.0 // indirect
 	golang.org/x/crypto v0.0.0-20190426145343-a29dc8fdc734
 )

go.sum

@@ -14,7 +14,9 @@ github.com/Shopify/sarama v1.19.0/go.mod h1:FVkBWblsNy7DGZRfXLU0O9RCGt5g3g3yEuWX
 github.com/Shopify/toxiproxy v2.1.4+incompatible/go.mod h1:OXgGpZ6Cli1/URJOF1DMxUHB2q5Ap20/P/eIdh4G0pI=
 github.com/aead/siphash v1.0.1 h1:FwHfE/T45KPKYuuSAKyyvE+oPWcaQ+CUmFW0bPlM+kg=
 github.com/aead/siphash v1.0.1/go.mod h1:Nywa3cDsYNNK3gaciGTWPwHt0wlpNV15vwmswBAUSII=
+github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc h1:cAKDfWh5VpdgMhJosfJnn5/FoN2SRZ4p7fJNX58YPaU=
 github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc=
+github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf h1:qet1QNfXsQxTZqLG4oE62mJzwPIB8+Tee4RNCL9ulrY=
 github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0=
 github.com/apache/thrift v0.12.0/go.mod h1:cp2SuWMxlEZw2r+iP2GNCdIi4C1qmUzdZFSVb+bacwQ=
 github.com/aws/aws-sdk-go v1.17.7/go.mod h1:KmX6BPdI08NWTb3/sm4ZGu5ShLoqVDhKgpiN924inxo=
@@ -188,6 +190,7 @@ github.com/prometheus/client_golang v0.9.1/go.mod h1:7SWBe2y4D6OKWSNQJUaRYU/AaXP
 github.com/prometheus/client_golang v0.9.3-0.20190127221311-3c4408c8b829/go.mod h1:p2iRAGwDERtqlqzRXnrOVns+ignqQo//hLXqYxZYVNs=
 github.com/prometheus/client_model v0.0.0-20180712105110-5c3871d89910/go.mod h1:MbSGuTsp3dbXC40dX6PRTWyKYBIrTGTE9sqQNg2J8bo=
 github.com/prometheus/client_model v0.0.0-20190115171406-56726106282f/go.mod h1:MbSGuTsp3dbXC40dX6PRTWyKYBIrTGTE9sqQNg2J8bo=
+github.com/prometheus/common v0.2.0 h1:kUZDBDTdBVBYBj5Tmh2NZLlF60mfjA27rM34b+cVwNU=
 github.com/prometheus/common v0.2.0/go.mod h1:TNfzLD0ON7rHzMJeJkieUDPYmFC7Snx/y86RQel1bk4=
 github.com/prometheus/procfs v0.0.0-20181005140218-185b4288413d/go.mod h1:c3At6R/oaqEKCNdg8wHV1ftS6bRYblBhIjjI8uT2IGk=
 github.com/prometheus/procfs v0.0.0-20190117184657-bf6a532e95b1/go.mod h1:c3At6R/oaqEKCNdg8wHV1ftS6bRYblBhIjjI8uT2IGk=
@@ -295,6 +298,7 @@ google.golang.org/grpc v1.17.0/go.mod h1:6QZJwpn2B+Zp71q/5VxRsJ6NXXVCE5NRUHRo+f3
 google.golang.org/grpc v1.19.0/go.mod h1:mqu4LbDTu4XGKhr4mRzUsmM4RtVoemTSY81AxZiDr8c=
 google.golang.org/grpc v1.20.1 h1:Hz2g2wirWK7H0qIIhGIqRGTuMwTE8HEKFnDZZ7lm9NU=
 google.golang.org/grpc v1.20.1/go.mod h1:10oTOabMzJvdu6/UiuZezV6QK5dSlG84ov/aaiqXj38=
+gopkg.in/alecthomas/kingpin.v2 v2.2.6 h1:jMFz6MfLP0/4fUyZle81rXUoxOBFi19VUFKVDOQfozc=
 gopkg.in/alecthomas/kingpin.v2 v2.2.6/go.mod h1:FMv+mEhP44yOT+4EoQTLFTRgOQ1FBLkstjWtayDeSgw=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=

netsync/manager.go

@@ -217,7 +217,7 @@ func (sm *SyncManager) PushGetBlockInvsOrHeaders(peer *peerpkg.Peer, startHash *
 			"%d from peer %s", sm.dag.ChainHeight()+1,
 			sm.nextCheckpoint.ChainHeight, peer.Addr()) //TODO: (Ori) This is probably wrong. Done only for compilation
 	}
-	return peer.PushGetBlockInvsMsg(startHash, &daghash.ZeroHash)
+	return peer.PushGetBlockInvsMsg(startHash, peer.SelectedTip())
 }
 
 // resetHeaderState sets the headers-first mode state to values appropriate for
@@ -700,7 +700,7 @@ func (sm *SyncManager) handleBlockMsg(bmsg *blockMsg) {
 	sm.headersFirstMode = false
 	sm.headerList.Init()
 	log.Infof("Reached the final checkpoint -- switching to normal mode")
-	err = peer.PushGetBlockInvsMsg(blockHash, &daghash.ZeroHash)
+	err = peer.PushGetBlockInvsMsg(blockHash, peer.SelectedTip())
 	if err != nil {
 		log.Warnf("Failed to send getblockinvs message to peer %s: %s",
 			peer.Addr(), err)

server/p2p/on_get_block_invs.go

@@ -3,6 +3,7 @@ package p2p
 import (
 	"github.com/daglabs/btcd/peer"
 	"github.com/daglabs/btcd/wire"
+	"github.com/prometheus/common/log"
 )
 
 // OnGetBlockInvs is invoked when a peer receives a getblockinvs bitcoin
@@ -20,8 +21,13 @@ func (sp *Peer) OnGetBlockInvs(_ *peer.Peer, msg *wire.MsgGetBlockInvs) {
 	// This way, if one getblocks is not enough to get the peer
 	// synced, we can know for sure that its selected chain will
 	// change, so we'll have higher shared chain block.
-	hashList := dag.GetBlueBlocksHashesBetween(msg.StartHash, msg.StopHash,
+	hashList, err := dag.GetBlueBlocksHashesBetween(msg.StartHash, msg.StopHash,
 		wire.MaxInvPerMsg)
+	if err != nil {
+		log.Warnf("Error getting blue blocks between %s and %s: %s", msg.StartHash, msg.StopHash, err)
+		sp.Disconnect()
+		return
+	}
 
 	// Generate inventory message.
 	invMsg := wire.NewMsgInv()

server/p2p/on_get_headers.go

@@ -3,6 +3,7 @@ package p2p
 import (
 	"github.com/daglabs/btcd/peer"
 	"github.com/daglabs/btcd/wire"
+	"github.com/prometheus/common/log"
 )
 
 // OnGetHeaders is invoked when a peer receives a getheaders bitcoin
@@ -22,7 +23,12 @@ func (sp *Peer) OnGetHeaders(_ *peer.Peer, msg *wire.MsgGetHeaders) {
 	// provided locator are known.  This does mean the client will start
 	// over with the genesis block if unknown block locators are provided.
 	dag := sp.server.DAG
-	headers := dag.GetBlueBlocksHeadersBetween(msg.StartHash, msg.StopHash)
+	headers, err := dag.GetBlueBlocksHeadersBetween(msg.StartHash, msg.StopHash)
+	if err != nil {
+		log.Warnf("Error getting blue blocks headers between %s and %s: %s", msg.StartHash, msg.StopHash, err)
+		sp.Disconnect()
+		return
+	}
 
 	// Send found headers to the requesting peer.
 	blockHeaders := make([]*wire.BlockHeader, len(headers))

server/rpc/handle_get_headers.go

@@ -28,7 +28,13 @@ func handleGetHeaders(s *Server, cmd interface{}, closeChan <-chan struct{}) (in
 			return nil, rpcDecodeHexError(c.StopHash)
 		}
 	}
-	headers := s.cfg.SyncMgr.GetBlueBlocksHeadersBetween(startHash, stopHash)
+	headers, err := s.cfg.SyncMgr.GetBlueBlocksHeadersBetween(startHash, stopHash)
+	if err != nil {
+		return nil, &btcjson.RPCError{
+			Code:    btcjson.ErrRPCMisc,
+			Message: err.Error(),
+		}
+	}
 
 	// Return the serialized block headers as hex-encoded strings.
 	hexBlockHeaders := make([]string, len(headers))

server/rpc/rpcadapters.go

@@ -275,6 +275,6 @@ func (b *rpcSyncMgr) SyncPeerID() int32 {
 //
 // This function is safe for concurrent access and is part of the
 // rpcserverSyncManager interface implementation.
-func (b *rpcSyncMgr) GetBlueBlocksHeadersBetween(startHash, stopHash *daghash.Hash) []*wire.BlockHeader {
+func (b *rpcSyncMgr) GetBlueBlocksHeadersBetween(startHash, stopHash *daghash.Hash) ([]*wire.BlockHeader, error) {
 	return b.server.DAG.GetBlueBlocksHeadersBetween(startHash, stopHash)
 }

server/rpc/rpcserver.go

@@ -763,7 +763,7 @@ type rpcserverSyncManager interface {
 	// block in the provided locators until the provided stop hash or the
 	// current tip is reached, up to a max of wire.MaxBlockHeadersPerMsg
 	// hashes.
-	GetBlueBlocksHeadersBetween(startHash, stopHash *daghash.Hash) []*wire.BlockHeader
+	GetBlueBlocksHeadersBetween(startHash, stopHash *daghash.Hash) ([]*wire.BlockHeader, error)
 }
 
 // rpcserverConfig is a descriptor containing the RPC server configuration.