[DEV-255] create checkConnectToPastUTXO and move the required functionalities to it from checkConnectBlock1)

* [DEV-255] create checkConnectToPastUTXO and move the required functionalities to it from checkConnectBlock

* [DEV-255] get rid of checkConnectBlock

* [DEV-255] rename pNode -> node

* [DEV-255] add comment to describe ErrWithDiff

blockdag/dag.go

@@ -5,6 +5,7 @@
 package blockdag
 
 import (
+	"errors"
 	"fmt"
 	"math"
 	"sync"
@@ -446,37 +447,21 @@ func (dag *BlockDAG) connectToDAG(node *blockNode, parentNodes blockSet, block *
 	// Skip checks if node has already been fully validated.
 	fastAdd := flags&BFFastAdd == BFFastAdd || dag.index.NodeStatus(node).KnownValid()
 
-	// Perform several checks to verify the block can be connected
+	// Connect the block to the DAG.
-	// to the DAG without violating any rules and without actually
+	err := dag.connectBlock(node, block, fastAdd)
-	// connecting the block.
+	if _, ok := err.(RuleError); ok {
-	if !fastAdd {
+		dag.index.SetStatusFlags(node, statusValidateFailed)
-		err := dag.checkConnectBlock(node, block)
+	} else {
-		if err == nil {
+		return err
-			dag.index.SetStatusFlags(node, statusValid)
-		} else if _, ok := err.(RuleError); ok {
-			dag.index.SetStatusFlags(node, statusValidateFailed)
-		} else {
-			return err
-		}
-
-		// Intentionally ignore errors writing updated node status to DB. If
-		// it fails to write, it's not the end of the world. If the block is
-		// valid, we flush in connectBlock and if the block is invalid, the
-		// worst that can happen is we revalidate the block after a restart.
-		if writeErr := dag.index.flushToDB(); writeErr != nil {
-			log.Warnf("Error flushing block index changes to disk: %v",
-				writeErr)
-		}
-
-		if err != nil {
-			return err
-		}
 	}
 
-	// Connect the block to the DAG.
+	// Intentionally ignore errors writing updated node status to DB. If
-	err := dag.connectBlock(node, block)
+	// it fails to write, it's not the end of the world. If the block is
-	if err != nil {
+	// invalid, the worst that can happen is we revalidate the block
-		return err
+	// after a restart.
+	if writeErr := dag.index.flushToDB(); writeErr != nil {
+		log.Warnf("Error flushing block index changes to disk: %v",
+			writeErr)
 	}
 
 	return nil
@@ -485,7 +470,14 @@ func (dag *BlockDAG) connectToDAG(node *blockNode, parentNodes blockSet, block *
 // connectBlock handles connecting the passed node/block to the DAG.
 //
 // This function MUST be called with the chain state lock held (for writes).
-func (dag *BlockDAG) connectBlock(node *blockNode, block *util.Block) error {
+func (dag *BlockDAG) connectBlock(node *blockNode, block *util.Block, fastAdd bool) error {
+	// The coinbase for the Genesis block is not spendable, so just return
+	// an error now.
+	if node.hash.IsEqual(dag.dagParams.GenesisHash) {
+		str := "the coinbase for the genesis block is not spendable"
+		return ruleError(ErrMissingTxOut, str)
+	}
+
 	// No warnings about unknown rules or versions until the DAG is
 	// current.
 	if dag.isCurrent() {
@@ -503,7 +495,7 @@ func (dag *BlockDAG) connectBlock(node *blockNode, block *util.Block) error {
 	}
 
 	// Add the node to the virtual and update the UTXO set of the DAG.
-	utxoDiff, acceptedTxsData, err := dag.applyUTXOChanges(node, block)
+	utxoDiff, acceptedTxsData, err := dag.applyUTXOChanges(node, block, fastAdd)
 	if err != nil {
 		return err
 	}
@@ -570,7 +562,7 @@ func (dag *BlockDAG) connectBlock(node *blockNode, block *util.Block) error {
 // 5. Updates each of the tips' utxoDiff.
 //
 // This function MUST be called with the chain state lock held (for writes).
-func (dag *BlockDAG) applyUTXOChanges(node *blockNode, block *util.Block) (utxoDiff *UTXODiff, acceptedTxData []*TxWithBlockHash, err error) {
+func (dag *BlockDAG) applyUTXOChanges(node *blockNode, block *util.Block, fastAdd bool) (utxoDiff *UTXODiff, acceptedTxData []*TxWithBlockHash, err error) {
 	// Prepare provisionalNodes for all the relevant nodes to avoid modifying the original nodes.
 	// We avoid modifying the original nodes in this function because it could potentially
 	// fail if the block is not valid, thus bringing all the affected nodes (and the virtual)
@@ -581,9 +573,13 @@ func (dag *BlockDAG) applyUTXOChanges(node *blockNode, block *util.Block) (utxoD
 	// Clone the virtual block so that we don't modify the existing one.
 	virtualClone := dag.virtual.clone()
 
-	newBlockUTXO, acceptedTxData, err := newNodeProvisional.verifyAndBuildUTXO(virtualClone, dag.db)
+	newBlockUTXO, acceptedTxData, err := newNodeProvisional.verifyAndBuildUTXO(virtualClone, dag, fastAdd)
 	if err != nil {
-		return nil, nil, fmt.Errorf("error verifying UTXO for %v: %s", node, err)
+		newErrString := fmt.Sprintf("error verifying UTXO for %v: %s", node, err)
+		if err, ok := err.(RuleError); ok {
+			return nil, nil, ruleError(err.ErrorCode, newErrString)
+		}
+		return nil, nil, errors.New(newErrString)
 	}
 
 	err = newNodeProvisional.updateParents(virtualClone, newBlockUTXO)
@@ -598,13 +594,21 @@ func (dag *BlockDAG) applyUTXOChanges(node *blockNode, block *util.Block) (utxoD
 	virtualNodeProvisional := provisionalSet.newProvisionalNode(&virtualClone.blockNode, true, nil)
 	newVirtualUTXO, _, err := virtualNodeProvisional.pastUTXO(virtualClone, dag.db)
 	if err != nil {
-		return nil, nil, fmt.Errorf("could not restore past UTXO for virtual %v: %s", virtualClone, err)
+		newErrString := fmt.Sprintf("could not restore past UTXO for virtual %v: %s", virtualClone, err)
+		if err, ok := err.(RuleError); ok {
+			return nil, nil, ruleError(err.ErrorCode, newErrString)
+		}
+		return nil, nil, errors.New(newErrString)
 	}
 
 	// Apply new utxoDiffs to all the tips
 	err = updateTipsUTXO(virtualNodeProvisional.parents, virtualClone, newVirtualUTXO)
 	if err != nil {
-		return nil, nil, fmt.Errorf("failed updating the tips' UTXO: %s", err)
+		newErrString := fmt.Sprintf("failed updating the tips' UTXO: %s", err)
+		if err, ok := err.(RuleError); ok {
+			return nil, nil, ruleError(err.ErrorCode, newErrString)
+		}
+		return nil, nil, errors.New(newErrString)
 	}
 
 	// It is now safe to meld the UTXO set to base.
@@ -704,12 +708,19 @@ func (pns provisionalNodeSet) newProvisionalNode(node *blockNode, withRelatives
 }
 
 // verifyAndBuildUTXO verifies all transactions in the given block (in provisionalNode format) and builds its UTXO
-func (p *provisionalNode) verifyAndBuildUTXO(virtual *virtualBlock, db database.DB) (utxoSet UTXOSet, acceptedTxData []*TxWithBlockHash, err error) {
+func (p *provisionalNode) verifyAndBuildUTXO(virtual *virtualBlock, dag *BlockDAG, fastAdd bool) (utxoSet UTXOSet, acceptedTxData []*TxWithBlockHash, err error) {
-	pastUTXO, pastUTXOaccpetedTxData, err := p.pastUTXO(virtual, db)
+	pastUTXO, pastUTXOaccpetedTxData, err := p.pastUTXO(virtual, dag.db)
 	if err != nil {
 		return nil, nil, err
 	}
 
+	if !fastAdd {
+		err = dag.checkConnectToPastUTXO(p, pastUTXO)
+		if err != nil {
+			return nil, nil, err
+		}
+	}
+
 	diff := NewUTXODiff()
 	acceptedTxData = make([]*TxWithBlockHash, 0, len(pastUTXOaccpetedTxData)+len(p.transactions))
 	if len(pastUTXOaccpetedTxData) != 0 {

blockdag/error.go

@@ -205,6 +205,9 @@ const (
 	// current chain tip. This is not a block validation rule, but is required
 	// for block proposals submitted via getblocktemplate RPC.
 	ErrPrevBlockNotBest
+
+	// ErrWithDiff indicates that there was an error with UTXOSet.WithDiff
+	ErrWithDiff
 )
 
 // Map of ErrorCode values back to their constant names for pretty printing.
@@ -248,6 +251,7 @@ var errorCodeStrings = map[ErrorCode]string{
 	ErrPreviousBlockUnknown:  "ErrPreviousBlockUnknown",
 	ErrInvalidAncestorBlock:  "ErrInvalidAncestorBlock",
 	ErrPrevBlockNotBest:      "ErrPrevBlockNotBest",
+	ErrWithDiff:              "ErrWithDiff",
 }
 
 // String returns the ErrorCode as a human-readable name.

blockdag/error_test.go

@@ -54,6 +54,7 @@ func TestErrorCodeStringer(t *testing.T) {
 		{ErrPreviousBlockUnknown, "ErrPreviousBlockUnknown"},
 		{ErrInvalidAncestorBlock, "ErrInvalidAncestorBlock"},
 		{ErrPrevBlockNotBest, "ErrPrevBlockNotBest"},
+		{ErrWithDiff, "ErrWithDiff"},
 		{0xffff, "Unknown ErrorCode (65535)"},
 	}
 

blockdag/scriptval.go

@@ -205,15 +205,15 @@ func ValidateTransactionScripts(tx *util.Tx, utxoSet UTXOSet, flags txscript.Scr
 
 // checkBlockScripts executes and validates the scripts for all transactions in
 // the passed block using multiple goroutines.
-func checkBlockScripts(block *util.Block, utxoSet UTXOSet, scriptFlags txscript.ScriptFlags, sigCache *txscript.SigCache) error {
+func checkBlockScripts(block *provisionalNode, utxoSet UTXOSet, scriptFlags txscript.ScriptFlags, sigCache *txscript.SigCache) error {
 	// Collect all of the transaction inputs and required information for
 	// validation for all transactions in the block into a single slice.
 	numInputs := 0
-	for _, tx := range block.Transactions() {
+	for _, tx := range block.transactions {
 		numInputs += len(tx.MsgTx().TxIn)
 	}
 	txValItems := make([]*txValidateItem, 0, numInputs)
-	for _, tx := range block.Transactions() {
+	for _, tx := range block.transactions {
 		for txInIdx, txIn := range tx.MsgTx().TxIn {
 			// Skip coinbases.
 			if txIn.PreviousOutPoint.Index == math.MaxUint32 {
@@ -237,7 +237,7 @@ func checkBlockScripts(block *util.Block, utxoSet UTXOSet, scriptFlags txscript.
 	}
 	elapsed := time.Since(start)
 
-	log.Tracef("block %v took %v to verify", block.Hash(), elapsed)
+	log.Tracef("block %v took %v to verify", block.original.hash, elapsed)
 
 	return nil
 }

blockdag/scriptval_test.go

@@ -40,8 +40,15 @@ func TestCheckBlockScripts(t *testing.T) {
 		return
 	}
 
+	node := &provisionalNode{
+		original: &blockNode{
+			hash: *blocks[0].Hash(),
+		},
+		transactions: blocks[0].Transactions(),
+	}
+
 	scriptFlags := txscript.ScriptNoFlags
-	err = checkBlockScripts(blocks[0], utxoSet, scriptFlags, nil)
+	err = checkBlockScripts(node, utxoSet, scriptFlags, nil)
 	if err != nil {
 		t.Errorf("Transaction script validation failed: %v\n", err)
 		return

blockdag/utxoset.go

@@ -257,7 +257,7 @@ func (d *UTXODiff) WithDiff(diff *UTXODiff) (*UTXODiff, error) {
 			result.toAdd.add(outPoint, utxoEntry)
 		}
 		if diff.toAdd.contains(outPoint) {
-			return nil, errors.New("WithDiff: transaction both in d.toAdd and in other.toAdd")
+			return nil, ruleError(ErrWithDiff, "WithDiff: transaction both in d.toAdd and in other.toAdd")
 		}
 	}
 
@@ -270,7 +270,7 @@ func (d *UTXODiff) WithDiff(diff *UTXODiff) (*UTXODiff, error) {
 			result.toRemove.add(outPoint, utxoEntry)
 		}
 		if diff.toRemove.contains(outPoint) {
-			return nil, errors.New("WithDiff: transaction both in d.toRemove and in other.toRemove")
+			return nil, ruleError(ErrWithDiff, "WithDiff: transaction both in d.toRemove and in other.toRemove")
 		}
 	}
 
@@ -367,9 +367,9 @@ func diffFromTx(u UTXOSet, tx *wire.MsgTx, containingNode *blockNode) (*UTXODiff
 			if entry, ok := u.Get(txIn.PreviousOutPoint); ok {
 				diff.toRemove.add(txIn.PreviousOutPoint, entry)
 			} else {
-				return nil, fmt.Errorf(
+				return nil, ruleError(ErrMissingTxOut, fmt.Sprintf(
 					"Transaction %s is invalid because spends outpoint %s that is not in utxo set",
-					tx.TxHash(), txIn.PreviousOutPoint)
+					tx.TxHash(), txIn.PreviousOutPoint))
 			}
 		}
 	}

blockdag/utxoset_test.go

@@ -848,6 +848,14 @@ func TestApplyUTXOChanges(t *testing.T) {
 	}
 	defer teardownFunc()
 
+	dag.dagParams.CoinbaseMaturity = 1
+	// Create artificial checkpoint in order to prevent script validation for blocks that comes before it
+	dag.checkpoints = []dagconfig.Checkpoint{
+		{
+			Height: 1000,
+		},
+	}
+
 	cbTx, err := createCoinbaseTx(1, 1)
 	if err != nil {
 		t.Errorf("createCoinbaseTx: %v", err)
@@ -878,7 +886,7 @@ func TestApplyUTXOChanges(t *testing.T) {
 	initBlockNode(&node1, blockHeader, setFromSlice(dag.genesis), dagconfig.MainNetParams.K)
 
 	//Checks that dag.applyUTXOChanges fails because we don't allow a transaction to spend another transaction from the same block
-	_, _, err = dag.applyUTXOChanges(&node1, block1)
+	_, _, err = dag.applyUTXOChanges(&node1, block1, false)
 	if err == nil {
 		t.Errorf("applyUTXOChanges expected an error\n")
 	}
@@ -905,7 +913,7 @@ func TestApplyUTXOChanges(t *testing.T) {
 	initBlockNode(&node2, blockHeader, setFromSlice(dag.genesis), dagconfig.MainNetParams.K)
 
 	//Checks that dag.applyUTXOChanges doesn't fail because all of its transaction are dependant on transactions from previous blocks
-	_, _, err = dag.applyUTXOChanges(&node2, block2)
+	_, _, err = dag.applyUTXOChanges(&node2, block2, false)
 	if err != nil {
 		t.Errorf("applyUTXOChanges: %v", err)
 	}

blockdag/validate.go

@@ -778,11 +778,11 @@ func (dag *BlockDAG) checkBlockContext(block *util.Block, parents blockSet, blue
 // http://r6.ca/blog/20120206T005236Z.html.
 //
 // This function MUST be called with the chain state lock held (for reads).
-func (dag *BlockDAG) ensureNoDuplicateTx(node *blockNode, block *util.Block) error {
+func ensureNoDuplicateTx(block *provisionalNode, utxoSet UTXOSet) error {
 	// Fetch utxos for all of the transaction ouputs in this block.
 	// Typically, there will not be any utxos for any of the outputs.
 	fetchSet := make(map[wire.OutPoint]struct{})
-	for _, tx := range block.Transactions() {
+	for _, tx := range block.transactions {
 		prevOut := wire.OutPoint{Hash: *tx.Hash()}
 		for txOutIdx := range tx.MsgTx().TxOut {
 			prevOut.Index = uint32(txOutIdx)
@@ -793,7 +793,7 @@ func (dag *BlockDAG) ensureNoDuplicateTx(node *blockNode, block *util.Block) err
 	// Duplicate transactions are only allowed if the previous transaction
 	// is fully spent.
 	for outpoint := range fetchSet {
-		utxo, ok := dag.GetUTXOEntry(outpoint)
+		utxo, ok := utxoSet.Get(outpoint)
 		if ok {
 			str := fmt.Sprintf("tried to overwrite transaction %v "+
 				"at block height %d that is not fully spent",
@@ -905,12 +905,8 @@ func CheckTransactionInputs(tx *util.Tx, txHeight int32, utxoSet UTXOSet, dagPar
 	return txFeeInSatoshi, nil
 }
 
-// checkConnectBlock performs several checks to confirm connecting the passed
+// checkConnectToPastUTXO performs several checks to confirm connecting the passed
-// block to the chain represented by the passed view does not violate any rules.
+// block to the DAG represented by the passed view does not violate any rules.
-// In addition, the passed view is updated to spend all of the referenced
-// outputs and add all of the new utxos created by block.  Thus, the view will
-// represent the state of the chain as if the block were actually connected and
-// consequently the best hash for the view is also updated to passed block.
 //
 // An example of some of the checks performed are ensuring connecting the block
 // would not cause any duplicate transaction hashes for old transactions that
@@ -918,29 +914,10 @@ func CheckTransactionInputs(tx *util.Tx, txHeight int32, utxoSet UTXOSet, dagPar
 // signature operations per block, invalid values in relation to the expected
 // block subsidy, or fail transaction script validation.
 //
-// The CheckConnectBlockTemplate function makes use of this function to perform
+// This function MUST be called with the dag state lock held (for writes).
-// the bulk of its work.  The only difference is this function accepts a node
+func (dag *BlockDAG) checkConnectToPastUTXO(block *provisionalNode, pastUTXO UTXOSet) error {
-// which may or may not require reorganization to connect it to the main chain
-// whereas CheckConnectBlockTemplate creates a new node which specifically
-// connects to the end of the current main chain and then calls this function
-// with that node.
-//
-// This function MUST be called with the chain state lock held (for writes).
-func (dag *BlockDAG) checkConnectBlock(node *blockNode, block *util.Block) error {
-	// If the side chain blocks end up in the database, a call to
-	// CheckBlockSanity should be done here in case a previous version
-	// allowed a block that is no longer valid.  However, since the
-	// implementation only currently uses memory for the side chain blocks,
-	// it isn't currently necessary.
 
-	// The coinbase for the Genesis block is not spendable, so just return
+	err := ensureNoDuplicateTx(block, pastUTXO)
-	// an error now.
-	if node.hash.IsEqual(dag.dagParams.GenesisHash) {
-		str := "the coinbase for the genesis block is not spendable"
-		return ruleError(ErrMissingTxOut, str)
-	}
-
-	err := dag.ensureNoDuplicateTx(node, block)
 	if err != nil {
 		return err
 	}
@@ -951,9 +928,8 @@ func (dag *BlockDAG) checkConnectBlock(node *blockNode, block *util.Block) error
 	// expands the count to include a precise count of pay-to-script-hash
 	// signature operations in each of the input transaction public key
 	// scripts.
-	transactions := block.Transactions()
 	totalSigOps := 0
-	for i, tx := range transactions {
+	for i, tx := range block.transactions {
 		numsigOps := CountSigOps(tx)
 		// Since the first (and only the first) transaction has
 		// already been verified to be a coinbase transaction,
@@ -961,7 +937,7 @@ func (dag *BlockDAG) checkConnectBlock(node *blockNode, block *util.Block) error
 		// countP2SHSigOps for whether or not the transaction is
 		// a coinbase transaction rather than having to do a
 		// full coinbase check again.
-		numP2SHSigOps, err := CountP2SHSigOps(tx, i == 0, dag.virtual.utxoSet)
+		numP2SHSigOps, err := CountP2SHSigOps(tx, i == 0, pastUTXO)
 		if err != nil {
 			return err
 		}
@@ -987,8 +963,8 @@ func (dag *BlockDAG) checkConnectBlock(node *blockNode, block *util.Block) error
 	// against all the inputs when the signature operations are out of
 	// bounds.
 	var totalFees uint64
-	for _, tx := range transactions {
+	for _, tx := range block.transactions {
-		txFee, err := CheckTransactionInputs(tx, node.height, dag.virtual.utxoSet,
+		txFee, err := CheckTransactionInputs(tx, block.original.height, pastUTXO,
 			dag.dagParams)
 		if err != nil {
 			return err
@@ -1010,10 +986,10 @@ func (dag *BlockDAG) checkConnectBlock(node *blockNode, block *util.Block) error
 	// errors here because those error conditions would have already been
 	// caught by checkTransactionSanity.
 	var totalSatoshiOut uint64
-	for _, txOut := range transactions[0].MsgTx().TxOut {
+	for _, txOut := range block.transactions[0].MsgTx().TxOut {
 		totalSatoshiOut += txOut.Value
 	}
-	expectedSatoshiOut := CalcBlockSubsidy(node.height, dag.dagParams) +
+	expectedSatoshiOut := CalcBlockSubsidy(block.original.height, dag.dagParams) +
 		totalFees
 	if totalSatoshiOut > expectedSatoshiOut {
 		str := fmt.Sprintf("coinbase transaction for block pays %v "+
@@ -1030,7 +1006,7 @@ func (dag *BlockDAG) checkConnectBlock(node *blockNode, block *util.Block) error
 	// portion of block handling.
 	checkpoint := dag.LatestCheckpoint()
 	runScripts := true
-	if checkpoint != nil && node.height <= checkpoint.Height {
+	if checkpoint != nil && block.original.height <= checkpoint.Height {
 		runScripts = false
 	}
 
@@ -1038,20 +1014,20 @@ func (dag *BlockDAG) checkConnectBlock(node *blockNode, block *util.Block) error
 
 	// We obtain the MTP of the *previous* block in order to
 	// determine if transactions in the current block are final.
-	medianTime := node.selectedParent.CalcPastMedianTime()
+	medianTime := block.original.selectedParent.CalcPastMedianTime()
 
 	// We also enforce the relative sequence number based
 	// lock-times within the inputs of all transactions in this
 	// candidate block.
-	for _, tx := range block.Transactions() {
+	for _, tx := range block.transactions {
 		// A transaction can only be included within a block
 		// once the sequence locks of *all* its inputs are
 		// active.
-		sequenceLock, err := dag.calcSequenceLock(node, dag.virtual.utxoSet, tx, false)
+		sequenceLock, err := dag.calcSequenceLock(block.original, pastUTXO, tx, false)
 		if err != nil {
 			return err
 		}
-		if !SequenceLockActive(sequenceLock, node.height,
+		if !SequenceLockActive(sequenceLock, block.original.height,
 			medianTime) {
 			str := fmt.Sprintf("block contains " +
 				"transaction whose input sequence " +
@@ -1065,7 +1041,7 @@ func (dag *BlockDAG) checkConnectBlock(node *blockNode, block *util.Block) error
 	// expensive ECDSA signature check scripts.  Doing this last helps
 	// prevent CPU exhaustion attacks.
 	if runScripts {
-		err := checkBlockScripts(block, dag.virtual.utxoSet, scriptFlags, dag.sigCache)
+		err := checkBlockScripts(block, pastUTXO, scriptFlags, dag.sigCache)
 		if err != nil {
 			return err
 		}
@@ -1122,6 +1098,9 @@ func (dag *BlockDAG) CheckConnectBlockTemplate(block *util.Block) error {
 		return err
 	}
 
-	newNode := newBlockNode(&header, dag.virtual.tips(), dag.dagParams.K)
+	newProvisionalNode := &provisionalNode{
-	return dag.checkConnectBlock(newNode, block)
+		original:     newBlockNode(&header, dag.virtual.tips(), dag.dagParams.K),
+		transactions: block.Transactions(),
+	}
+	return dag.checkConnectToPastUTXO(newProvisionalNode, dag.UTXOSet())
 }