blockchain: Refactor reorganizeChain to exclude verification

reorganizeChain() used to handle the following:
1: That the blocknodes being disconnected/connected indeed to connect
   properly without errors.
2: Perform the actual disconnect/connect of the blocknodes.

The functionality of 1, the validation that the disconnects/connects can
happen without errors are now refactored out into
verifyReorganizationValidity.

This is an effort made so that ReconsiderBlock() can call
verifyReorganizationValidity and set the block status of the
reconsidered chain and return nil even when an error returns as it's ok
to get an error when reconsidering an invalid branch.

blockchain/chain.go

@@ -870,19 +870,128 @@ func countSpentOutputs(block *btcutil.Block) int {
 //
 // This function MUST be called with the chain state lock held (for writes).
 func (b *BlockChain) reorganizeChain(detachNodes, attachNodes *list.List) error {
+	// Check first that the detach and the attach nodes are valid and they
+	// pass verification.
+	detachBlocks, attachBlocks, detachSpentTxOuts,
+		err := b.verifyReorganizationValidity(detachNodes, attachNodes)
+	if err != nil {
+		return err
+	}
+
+	// Track the old and new best chains heads.
+	tip := b.bestChain.Tip()
+	oldBest := tip
+	newBest := tip
+
+	// Reset the view for the actual connection code below.  This is
+	// required because the view was previously modified when checking if
+	// the reorg would be successful and the connection code requires the
+	// view to be valid from the viewpoint of each block being disconnected.
+	view := NewUtxoViewpoint()
+	view.SetBestHash(&b.bestChain.Tip().hash)
+
+	// Disconnect blocks from the main chain.
+	for i, e := 0, detachNodes.Front(); e != nil; i, e = i+1, e.Next() {
+		n := e.Value.(*blockNode)
+		block := detachBlocks[i]
+
+		// Load all of the utxos referenced by the block that aren't
+		// already in the view.
+		err := view.fetchInputUtxos(b.utxoCache, block)
+		if err != nil {
+			return err
+		}
+
+		// Update the view to unspend all of the spent txos and remove
+		// the utxos created by the block.
+		err = view.disconnectTransactions(b.db, block,
+			detachSpentTxOuts[i])
+		if err != nil {
+			return err
+		}
+
+		// Update the database and chain state. The cache will be flushed
+		// here before the utxoview modifications happen to the database.
+		err = b.disconnectBlock(n, block, view)
+		if err != nil {
+			return err
+		}
+
+		newBest = n.parent
+	}
+
+	// Set the fork point only if there are nodes to attach since otherwise
+	// blocks are only being disconnected and thus there is no fork point.
+	var forkNode *blockNode
+	if attachNodes.Len() > 0 {
+		forkNode = newBest
+	}
+
+	// Connect the new best chain blocks using the utxocache directly.  It's more
+	// efficient and since we already checked that the blocks are correct and that
+	// the transactions connect properly, it's ok to access the cache.  If we suddenly
+	// crash here, we are able to recover as well.
+	for i, e := 0, attachNodes.Front(); e != nil; i, e = i+1, e.Next() {
+		n := e.Value.(*blockNode)
+		block := attachBlocks[i]
+
+		// Update the cache to mark all utxos referenced by the block
+		// as spent and add all transactions being created by this block
+		// to it.  Also, provide an stxo slice so the spent txout
+		// details are generated.
+		stxos := make([]SpentTxOut, 0, countSpentOutputs(block))
+		err = b.utxoCache.connectTransactions(block, &stxos)
+		if err != nil {
+			return err
+		}
+
+		// Update the database and chain state.
+		err = b.connectBlock(n, block, stxos)
+		if err != nil {
+			return err
+		}
+
+		newBest = n
+	}
+
+	// Log the point where the chain forked and old and new best chain
+	// heads.
+	if forkNode != nil {
+		log.Infof("REORGANIZE: Chain forks at %v (height %v)", forkNode.hash,
+			forkNode.height)
+	}
+	log.Infof("REORGANIZE: Old best chain head was %v (height %v)",
+		&oldBest.hash, oldBest.height)
+	log.Infof("REORGANIZE: New best chain head is %v (height %v)",
+		newBest.hash, newBest.height)
+
+	return nil
+}
+
+// verifyReorganizationValidity will verify that the disconnects and the connects
+// that are in the list are able to be processed without mutating the chain.
+//
+// For the attach nodes, it'll check that each of the blocks are valid and will
+// change the status of the block node in the list to invalid if the block fails
+// to pass verification.  For the detach nodes, it'll check that the blocks being
+// detached and their spend journals are present on the database.
+func (b *BlockChain) verifyReorganizationValidity(detachNodes, attachNodes *list.List) (
+	[]*btcutil.Block, []*btcutil.Block, [][]SpentTxOut, error) {
+
 	// Nothing to do if no reorganize nodes were provided.
 	if detachNodes.Len() == 0 && attachNodes.Len() == 0 {
-		return nil
+		return nil, nil, nil, nil
 	}
 
 	// Ensure the provided nodes match the current best chain.
 	tip := b.bestChain.Tip()
 	if detachNodes.Len() != 0 {
 		firstDetachNode := detachNodes.Front().Value.(*blockNode)
 		if firstDetachNode.hash != tip.hash {
-			return AssertError(fmt.Sprintf("reorganize nodes to detach are "+
-				"not for the current best chain -- first detach node %v, "+
-				"current chain %v", &firstDetachNode.hash, &tip.hash))
+			return nil, nil, nil,
+				AssertError(fmt.Sprintf("reorganize nodes to detach are "+
+					"not for the current best chain -- first detach node %v, "+
+					"current chain %v", &firstDetachNode.hash, &tip.hash))
 		}
 	}
 
@@ -891,17 +1000,14 @@ func (b *BlockChain) reorganizeChain(detachNodes, attachNodes *list.List) error
 		firstAttachNode := attachNodes.Front().Value.(*blockNode)
 		lastDetachNode := detachNodes.Back().Value.(*blockNode)
 		if firstAttachNode.parent.hash != lastDetachNode.parent.hash {
-			return AssertError(fmt.Sprintf("reorganize nodes do not have the "+
-				"same fork point -- first attach parent %v, last detach "+
-				"parent %v", &firstAttachNode.parent.hash,
-				&lastDetachNode.parent.hash))
+			return nil, nil, nil,
+				AssertError(fmt.Sprintf("reorganize nodes do not have the "+
+					"same fork point -- first attach parent %v, last detach "+
+					"parent %v", &firstAttachNode.parent.hash,
+					&lastDetachNode.parent.hash))
 		}
 	}
 
-	// Track the old and new best chains heads.
-	oldBest := tip
-	newBest := tip
-
 	// All of the blocks to detach and related spend journal entries needed
 	// to unspend transaction outputs in the blocks being disconnected must
 	// be loaded from the database during the reorg check phase below and
@@ -916,7 +1022,7 @@ func (b *BlockChain) reorganizeChain(detachNodes, attachNodes *list.List) error
 	// database and using that information to unspend all of the spent txos
 	// and remove the utxos created by the blocks.
 	view := NewUtxoViewpoint()
-	view.SetBestHash(&oldBest.hash)
+	view.SetBestHash(&tip.hash)
 	for e := detachNodes.Front(); e != nil; e = e.Next() {
 		n := e.Value.(*blockNode)
 		var block *btcutil.Block
@@ -926,19 +1032,20 @@ func (b *BlockChain) reorganizeChain(detachNodes, attachNodes *list.List) error
 			return err
 		})
 		if err != nil {
-			return err
+			return nil, nil, nil, 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()))
+			return nil, nil, nil, AssertError(
+				fmt.Sprintf("detach block node hash %v (height "+
+					"%v) does not match previous parent block hash %v",
+					&n.hash, n.height, block.Hash()))
 		}
 
 		// Load all of the utxos referenced by the block that aren't
 		// already in the view.
 		err = view.fetchInputUtxos(b.utxoCache, block)
 		if err != nil {
-			return err
+			return nil, nil, nil, err
 		}
 
 		// Load all of the spent txos for the block from the spend
@@ -949,7 +1056,7 @@ func (b *BlockChain) reorganizeChain(detachNodes, attachNodes *list.List) error
 			return err
 		})
 		if err != nil {
-			return err
+			return nil, nil, nil, err
 		}
 
 		// Store the loaded block and spend journal entry for later.
@@ -958,17 +1065,8 @@ func (b *BlockChain) reorganizeChain(detachNodes, attachNodes *list.List) error
 
 		err = view.disconnectTransactions(b.db, block, stxos)
 		if err != nil {
-			return err
+			return nil, nil, nil, err
 		}
-
-		newBest = n.parent
-	}
-
-	// Set the fork point only if there are nodes to attach since otherwise
-	// blocks are only being disconnected and thus there is no fork point.
-	var forkNode *blockNode
-	if attachNodes.Len() > 0 {
-		forkNode = newBest
 	}
 
 	// Perform several checks to verify each block that needs to be attached
@@ -993,7 +1091,7 @@ func (b *BlockChain) reorganizeChain(detachNodes, attachNodes *list.List) error
 			return err
 		})
 		if err != nil {
-			return err
+			return nil, nil, nil, err
 		}
 
 		// Store the loaded block for later.
@@ -1005,14 +1103,13 @@ func (b *BlockChain) reorganizeChain(detachNodes, attachNodes *list.List) error
 		if b.index.NodeStatus(n).KnownValid() {
 			err = view.fetchInputUtxos(b.utxoCache, block)
 			if err != nil {
-				return err
+				return nil, nil, nil, err
 			}
 			err = view.connectTransactions(block, nil)
 			if err != nil {
-				return err
+				return nil, nil, nil, err
 			}
 
-			newBest = n
 			continue
 		}
 
@@ -1033,96 +1130,12 @@ func (b *BlockChain) reorganizeChain(detachNodes, attachNodes *list.List) error
 					b.index.SetStatusFlags(dn, statusInvalidAncestor)
 				}
 			}
-			return err
+			return nil, nil, nil, err
 		}
 		b.index.SetStatusFlags(n, statusValid)
-
-		newBest = n
-	}
-
-	// Flush the utxo cache for the block disconnect below.  The disconnect
-	// code assumes that it's directly modifying the database so the cache
-	// will be left in an inconsistent state.  It needs to be flushed beforehand
-	// in order for that to not happen.
-	err := b.db.Update(func(dbTx database.Tx) error {
-		return b.utxoCache.flush(dbTx, FlushRequired, b.BestSnapshot())
-	})
-	if err != nil {
-		return err
-	}
-
-	// Reset the view for the actual connection code below.  This is
-	// required because the view was previously modified when checking if
-	// the reorg would be successful and the connection code requires the
-	// view to be valid from the viewpoint of each block being disconnected.
-	view = NewUtxoViewpoint()
-	view.SetBestHash(&b.bestChain.Tip().hash)
-
-	// Disconnect blocks from the main chain.
-	for i, e := 0, detachNodes.Front(); e != nil; i, e = i+1, e.Next() {
-		n := e.Value.(*blockNode)
-		block := detachBlocks[i]
-
-		// Load all of the utxos referenced by the block that aren't
-		// already in the view.
-		err := view.fetchInputUtxos(b.utxoCache, block)
-		if err != nil {
-			return err
-		}
-
-		// Update the view to unspend all of the spent txos and remove
-		// the utxos created by the block.
-		err = view.disconnectTransactions(b.db, block,
-			detachSpentTxOuts[i])
-		if err != nil {
-			return err
-		}
-
-		// Update the database and chain state. The cache will be flushed
-		// here before the utxoview modifications happen to the database.
-		err = b.disconnectBlock(n, block, view)
-		if err != nil {
-			return err
-		}
 	}
 
-	// Connect the new best chain blocks using the utxocache directly.  It's more
-	// efficient and since we already checked that the blocks are correct and that
-	// the transactions connect properly, it's ok to access the cache.  If we suddenly
-	// crash here, we are able to recover as well.
-	for i, e := 0, attachNodes.Front(); e != nil; i, e = i+1, e.Next() {
-		n := e.Value.(*blockNode)
-		block := attachBlocks[i]
-
-		// Update the cache to mark all utxos referenced by the block
-		// as spent and add all transactions being created by this block
-		// to it.  Also, provide an stxo slice so the spent txout
-		// details are generated.
-		stxos := make([]SpentTxOut, 0, countSpentOutputs(block))
-		err = b.utxoCache.connectTransactions(block, &stxos)
-		if err != nil {
-			return err
-		}
-
-		// Update the database and chain state.
-		err = b.connectBlock(n, block, stxos)
-		if err != nil {
-			return err
-		}
-	}
-
-	// Log the point where the chain forked and old and new best chain
-	// heads.
-	if forkNode != nil {
-		log.Infof("REORGANIZE: Chain forks at %v (height %v)", forkNode.hash,
-			forkNode.height)
-	}
-	log.Infof("REORGANIZE: Old best chain head was %v (height %v)",
-		&oldBest.hash, oldBest.height)
-	log.Infof("REORGANIZE: New best chain head is %v (height %v)",
-		newBest.hash, newBest.height)
-
-	return nil
+	return detachBlocks, attachBlocks, detachSpentTxOuts, nil
 }
 
 // connectBestChain handles connecting the passed block to the chain while