blockchain: Explicit block fetch semantics.

This makes the semantics of block fetching much more explicit by
introducing a new function named fetchMainChainBlockByHash which only
attempts to the load a block from the main chain block cache and then
falls back to the database.  While currently only blocks in the main
chain are in the database, the intention is for future commits to
allow the database to house side chain blocks as well, so having clearly
defined semantics will help make that transition easier to verify.

While here, it also renames {f,F}etchBlockFromHash to {f,F}etchBlockByHash
to be consistent with the naming used by the other existing functions
and updates the comments to better describe the function semantics.

blockchain/accept.go

@@ -296,7 +296,7 @@ func (b *BlockChain) maybeAcceptBlock(block *dcrutil.Block, flags BehaviorFlags)
 
 	// Grab the parent block since it is required throughout the block
 	// connection process.
-	parent, err := b.fetchBlockFromHash(&newNode.parentHash)
+	parent, err := b.fetchBlockByHash(&newNode.parentHash)
 	if err != nil {
 		return false, err
 	}

blockchain/chain.go

@@ -564,61 +564,96 @@ func (b *BlockChain) findNode(nodeHash *chainhash.Hash, searchDepth int) (*block
 	return node, err
 }
 
-// fetchBlockFromHash searches the internal chain block stores and the database in
-// an attempt to find the block.  If it finds the block, it returns it.
+// fetchMainChainBlockByHash returns the block from the main chain with the
+// given hash.  It first attempts to use cache and then falls back to loading it
+// from the database.
+// An error is returned if the block is either not found or not in the main
+// chain.
 //
-// This function is NOT safe for concurrent access.
-func (b *BlockChain) fetchBlockFromHash(hash *chainhash.Hash) (*dcrutil.Block, error) {
-	// Check side chain block cache
+// This function is safe for concurrent access.
+func (b *BlockChain) fetchMainChainBlockByHash(hash *chainhash.Hash) (*dcrutil.Block, error) {
+	b.mainchainBlockCacheLock.RLock()
+	block, ok := b.mainchainBlockCache[*hash]
+	b.mainchainBlockCacheLock.RUnlock()
+	if ok {
+		return block, nil
+	}
+
+	err := b.db.View(func(dbTx database.Tx) error {
+		var err error
+		block, err = dbFetchBlockByHash(dbTx, hash)
+		return err
+	})
+	return block, err
+}
+
+// fetchBlockByHash returns the block with the given hash from all known sources
+// such as the internal caches and the database.
+//
+// This function is safe for concurrent access.
+func (b *BlockChain) fetchBlockByHash(hash *chainhash.Hash) (*dcrutil.Block, error) {
+	// Check side chain block cache.
 	b.blockCacheLock.RLock()
-	blockSidechain, existsSidechain := b.blockCache[*hash]
+	block, existsSidechain := b.blockCache[*hash]
 	b.blockCacheLock.RUnlock()
 	if existsSidechain {
-		return blockSidechain, nil
+		return block, nil
 	}
 
-	// Check orphan cache
+	// Check orphan cache.
 	b.orphanLock.RLock()
 	orphan, existsOrphans := b.orphans[*hash]
 	b.orphanLock.RUnlock()
 	if existsOrphans {
 		return orphan.block, nil
 	}
 
-	// Check main chain
+	// Check main chain cache.
 	b.mainchainBlockCacheLock.RLock()
 	block, ok := b.mainchainBlockCache[*hash]
 	b.mainchainBlockCacheLock.RUnlock()
 	if ok {
 		return block, nil
 	}
 
-	var blockMainchain *dcrutil.Block
-	errFetchMainchain := b.db.View(func(dbTx database.Tx) error {
-		var err error
-		blockMainchain, err = dbFetchBlockByHash(dbTx, hash)
-		return err
+	// Attempt to load the block from the database.
+	err := b.db.View(func(dbTx database.Tx) error {
+		// NOTE: This does not use the dbFetchBlockByHash function since that
+		// function only works with main chain blocks.
+		blockBytes, err := dbTx.FetchBlock(hash)
+		if err != nil {
+			return err
+		}
+
+		block, err = dcrutil.NewBlockFromBytes(blockBytes)
+		if err != nil {
+			return err
+		}
+
+		return nil
 	})
-	if errFetchMainchain == nil && blockMainchain != nil {
-		return blockMainchain, nil
+	if err == nil && block != nil {
+		return block, nil
 	}
 
-	// Implicit !existsMainchain && !existsSidechain && !existsOrphans
-	return nil, fmt.Errorf("unable to find block %v in "+
-		"side chain cache, orphan cache, and main chain db", hash)
+	return nil, fmt.Errorf("unable to find block %v in cache or db", hash)
 }
 
-// FetchBlockFromHash is the generalized and exported version of
-// fetchBlockFromHash.  It is safe for concurrent access.
-func (b *BlockChain) FetchBlockFromHash(hash *chainhash.Hash) (*dcrutil.Block, error) {
-	return b.fetchBlockFromHash(hash)
+// FetchBlockByHash searches the internal chain block stores and the database
+// in an attempt to find the requested block.
+//
+// This function differs from BlockByHash in that this one also returns blocks
+// that are not part of the main chain (if they are known).
+//
+// This function is safe for concurrent access.
+func (b *BlockChain) FetchBlockByHash(hash *chainhash.Hash) (*dcrutil.Block, error) {
+	return b.fetchBlockByHash(hash)
 }
 
 // GetTopBlock returns the current block at HEAD on the blockchain.  Needed
 // for mining in the daemon.
 func (b *BlockChain) GetTopBlock() (*dcrutil.Block, error) {
-	block, err := b.fetchBlockFromHash(&b.bestNode.hash)
-	return block, err
+	return b.fetchMainChainBlockByHash(&b.bestNode.hash)
 }
 
 // pruneStakeNodes removes references to old stake nodes which should no
@@ -1256,7 +1291,7 @@ func (b *BlockChain) reorganizeChain(detachNodes, attachNodes *list.List, flags
 		block := nextBlockToDetach
 		if block == nil {
 			var err error
-			block, err = b.fetchBlockFromHash(&n.hash)
+			block, err = b.fetchMainChainBlockByHash(&n.hash)
 			if err != nil {
 				return err
 			}
@@ -1270,7 +1305,7 @@ func (b *BlockChain) reorganizeChain(detachNodes, attachNodes *list.List, flags
 		// Grab the parent of the current block and also save a reference to it
 		// as the next block to detach so it doesn't need to be loaded again on
 		// the next iteration.
-		parent, err := b.fetchBlockFromHash(&n.parentHash)
+		parent, err := b.fetchMainChainBlockByHash(&n.parentHash)
 		if err != nil {
 			return err
 		}
@@ -1319,7 +1354,7 @@ func (b *BlockChain) reorganizeChain(detachNodes, attachNodes *list.List, flags
 			return err
 		}
 
-		forkBlock, err = b.fetchBlockFromHash(&forkNode.hash)
+		forkBlock, err = b.fetchMainChainBlockByHash(&forkNode.hash)
 		if err != nil {
 			return err
 		}
@@ -1517,7 +1552,7 @@ func (b *BlockChain) forceHeadReorganization(formerBest chainhash.Hash, newBest
 			"common parent for forced reorg")
 	}
 
-	newBestBlock, err := b.fetchBlockFromHash(&newBest)
+	newBestBlock, err := b.fetchBlockByHash(&newBest)
 	if err != nil {
 		return err
 	}
@@ -1527,11 +1562,12 @@ func (b *BlockChain) forceHeadReorganization(formerBest chainhash.Hash, newBest
 	view.SetBestHash(&b.bestNode.parentHash)
 	view.SetStakeViewpoint(ViewpointPrevValidInitial)
 
-	formerBestBlock, err := b.fetchBlockFromHash(&formerBest)
+	formerBestBlock, err := b.fetchBlockByHash(&formerBest)
 	if err != nil {
 		return err
 	}
-	commonParentBlock, err := b.fetchBlockFromHash(&formerBestNode.parent.hash)
+	commonParentBlock, err := b.fetchMainChainBlockByHash(
+		&formerBestNode.parent.hash)
 	if err != nil {
 		return err
 	}

blockchain/chainio.go

@@ -1656,15 +1656,14 @@ func (b *BlockChain) BlockByHeight(blockHeight int64) (*dcrutil.Block, error) {
 	return block, err
 }
 
-// BlockByHash returns the block from the main chain with the given hash with
-// the appropriate chain height set.
+// BlockByHash returns the block from the main chain with the given hash.
 //
 // This function is safe for concurrent access.
 func (b *BlockChain) BlockByHash(hash *chainhash.Hash) (*dcrutil.Block, error) {
 	b.chainLock.RLock()
 	defer b.chainLock.RUnlock()
 
-	return b.fetchBlockFromHash(hash)
+	return b.fetchMainChainBlockByHash(hash)
 }
 
 // HeightRange returns a range of block hashes for the given start and end

blockchain/stakenode.go

@@ -65,7 +65,7 @@ func (b *BlockChain) fetchNewTicketsForNode(node *blockNode) ([]chainhash.Hash,
 		return nil, err
 	}
 
-	matureBlock, errBlock := b.fetchBlockFromHash(&matureNode.hash)
+	matureBlock, errBlock := b.fetchBlockByHash(&matureNode.hash)
 	if errBlock != nil {
 		return nil, errBlock
 	}

blockchain/utxoviewpoint.go

@@ -1089,11 +1089,11 @@ func (b *BlockChain) FetchUtxoView(tx *dcrutil.Tx, treeValid bool) (*UtxoViewpoi
 	view := NewUtxoViewpoint()
 	if treeValid {
 		view.SetStakeViewpoint(ViewpointPrevValidRegular)
-		block, err := b.fetchBlockFromHash(&b.bestNode.hash)
+		block, err := b.fetchMainChainBlockByHash(&b.bestNode.hash)
 		if err != nil {
 			return nil, err
 		}
-		parent, err := b.fetchBlockFromHash(&b.bestNode.parentHash)
+		parent, err := b.fetchMainChainBlockByHash(&b.bestNode.parentHash)
 		if err != nil {
 			return nil, err
 		}

blockchain/validate.go

@@ -2612,13 +2612,6 @@ func (b *BlockChain) CheckConnectBlock(block *dcrutil.Block) error {
 		return ruleError(ErrMissingParent, err.Error())
 	}
 
-	// Grab the parent block since it is required throughout the block
-	// connection process.
-	parent, err := b.fetchBlockFromHash(&parentHash)
-	if err != nil {
-		return ruleError(ErrMissingParent, err.Error())
-	}
-
 	newNode := newBlockNode(&block.MsgBlock().Header,
 		stake.FindSpentTicketsInBlock(block.MsgBlock()))
 	newNode.parent = prevNode
@@ -2628,6 +2621,14 @@ func (b *BlockChain) CheckConnectBlock(block *dcrutil.Block) error {
 	// the ticket database we already have.
 	if b.bestNode == nil || (prevNode != nil &&
 		prevNode.hash == b.bestNode.hash) {
+
+		// Grab the parent block since it is required throughout the block
+		// connection process.
+		parent, err := b.fetchMainChainBlockByHash(&parentHash)
+		if err != nil {
+			return ruleError(ErrMissingParent, err.Error())
+		}
+
 		view := NewUtxoViewpoint()
 		view.SetBestHash(&prevNode.hash)
 		return b.checkConnectBlock(newNode, block, parent, view, nil)
@@ -2647,23 +2648,35 @@ func (b *BlockChain) CheckConnectBlock(block *dcrutil.Block) error {
 	view.SetBestHash(&b.bestNode.hash)
 	view.SetStakeViewpoint(ViewpointPrevValidInitial)
 	var stxos []spentTxOut
+	var nextBlockToDetach *dcrutil.Block
 	for e := detachNodes.Front(); e != nil; e = e.Next() {
+		// Grab the block to detach based on the node.  Use the fact that the
+		// parent of the block is already required, and the next block to detach
+		// will also be the parent to optimize.
 		n := e.Value.(*blockNode)
-		block, err := b.fetchBlockFromHash(&n.hash)
-		if err != nil {
-			return err
+		block := nextBlockToDetach
+		if block == nil {
+			var err error
+			block, err = b.fetchMainChainBlockByHash(&n.hash)
+			if err != nil {
+				return err
+			}
+		}
+		if n.hash != *block.Hash() {
+			return AssertError(fmt.Sprintf("detach block node hash %v (height "+
+				"%v) does not match previous parent block hash %v", &n.hash,
+				n.height, block.Hash()))
 		}
 
-		parent, err := b.fetchBlockFromHash(&n.parentHash)
+		parent, err := b.fetchMainChainBlockByHash(&n.parentHash)
 		if err != nil {
 			return err
 		}
+		nextBlockToDetach = parent
 
-		// Load all of the spent txos for the block from the spend
-		// journal.
+		// Load all of the spent txos for the block from the spend journal.
 		err = b.db.View(func(dbTx database.Tx) error {
-			stxos, err = dbFetchSpendJournalEntry(dbTx, block,
-				parent)
+			stxos, err = dbFetchSpendJournalEntry(dbTx, block, parent)
 			return err
 		})
 		if err != nil {
@@ -2683,32 +2696,62 @@ func (b *BlockChain) CheckConnectBlock(block *dcrutil.Block) error {
 	// attachNodes list indicate the requested node is on a side chain, so
 	// if there are no nodes to attach, we're done.
 	if attachNodes.Len() == 0 {
+		// Grab the parent block since it is required throughout the block
+		// connection process.
+		parent, err := b.fetchMainChainBlockByHash(&parentHash)
+		if err != nil {
+			return ruleError(ErrMissingParent, err.Error())
+		}
+
 		view.SetBestHash(&parentHash)
 		return b.checkConnectBlock(newNode, block, parent, view, nil)
 	}
 
 	// The requested node is on a side chain, so we need to apply the
 	// transactions and spend information from each of the nodes to attach.
+	var prevAttachBlock *dcrutil.Block
 	for e := attachNodes.Front(); e != nil; e = e.Next() {
+		// Grab the block to attach based on the node.  Use the fact that the
+		// parent of the block is either the fork point for the first node being
+		// attached or the previous one that was attached for subsequent blocks
+		// to optimize.
 		n := e.Value.(*blockNode)
 		block, exists := b.blockCache[n.hash]
 		if !exists {
 			return fmt.Errorf("unable to find block %v in "+
 				"side chain cache for utxo view construction",
 				n.hash)
 		}
-
-		parent, err := b.fetchBlockFromHash(&n.parentHash)
-		if err != nil {
-			return err
+		parent := prevAttachBlock
+		if parent == nil {
+			var err error
+			parent, err = b.fetchMainChainBlockByHash(&n.parentHash)
+			if err != nil {
+				return err
+			}
 		}
+		if n.parentHash != *parent.Hash() {
+			return AssertError(fmt.Sprintf("attach block node hash %v (height "+
+				"%v) parent hash %v does not match previous parent block "+
+				"hash %v", &n.hash, n.height, &n.parentHash, parent.Hash()))
+		}
+
+		// Store the loaded block for the next iteration.
+		prevAttachBlock = block
 
 		err = b.connectTransactions(view, block, parent, &stxos)
 		if err != nil {
 			return err
 		}
 	}
 
+	// Grab the parent block since it is required throughout the block
+	// connection process.
+	parent, err := b.fetchBlockByHash(&parentHash)
+	if err != nil {
+		return ruleError(ErrMissingParent, err.Error())
+	}
+
 	view.SetBestHash(&parentHash)
 	return b.checkConnectBlock(newNode, block, parent, view, &stxos)
 }

rpcserver.go

@@ -1901,7 +1901,7 @@ func handleGetBlock(s *rpcServer, cmd interface{}, closeChan <-chan struct{}) (i
 	if err != nil {
 		return nil, rpcDecodeHexError(c.Hash)
 	}
-	blk, err := s.server.blockManager.chain.FetchBlockFromHash(hash)
+	blk, err := s.server.blockManager.chain.FetchBlockByHash(hash)
 	if err != nil {
 		return nil, &dcrjson.RPCError{
 			Code:    dcrjson.ErrRPCBlockNotFound,

server.go

@@ -1123,7 +1123,7 @@ func (s *server) pushTxMsg(sp *serverPeer, hash *chainhash.Hash, doneChan chan<-
 // pushBlockMsg sends a block message for the provided block hash to the
 // connected peer.  An error is returned if the block hash is not known.
 func (s *server) pushBlockMsg(sp *serverPeer, hash *chainhash.Hash, doneChan chan<- struct{}, waitChan <-chan struct{}) error {
-	block, err := sp.server.blockManager.chain.FetchBlockFromHash(hash)
+	block, err := sp.server.blockManager.chain.FetchBlockByHash(hash)
 	if err != nil {
 		peerLog.Tracef("Unable to fetch requested block hash %v: %v",
 			hash, err)