[NOD-471] Make AddTx return false for duplicate coinbase, and make pastUTXO return accepted transaction with the accepting block blue score (#523)

* [NOD-471] Make AddTx return false for duplicate coinbase, and make pastUTXO return accepted transaction with the accepting block blue score

* [NOD-471] Remove diffFromAcceptanceData

* [NOD-471] Make fetchBlueBlocks return also selected parent

* [NOD-471] Skip adding coinbase transactions on applyBlueBlocks

* [NOD-471] Use tx.IsCoinbase() instead of i == util.CoinbaseTransactionIndex

addrmgr/addrmanager_test.go

@@ -154,15 +154,15 @@ func TestAddAddressByIP(t *testing.T) {
 	for i, test := range tests {
 		err := amgr.AddAddressByIP(test.addrIP, nil)
 		if test.err != nil && err == nil {
-			t.Errorf("TestGood test %d failed expected an error and got none", i)
+			t.Errorf("TestAddAddressByIP test %d failed expected an error and got none", i)
 			continue
 		}
 		if test.err == nil && err != nil {
-			t.Errorf("TestGood test %d failed expected no error and got one", i)
+			t.Errorf("TestAddAddressByIP test %d failed expected no error and got one", i)
 			continue
 		}
 		if reflect.TypeOf(err) != reflect.TypeOf(test.err) {
-			t.Errorf("TestGood test %d failed got %v, want %v", i,
+			t.Errorf("TestAddAddressByIP test %d failed got %v, want %v", i,
 				reflect.TypeOf(err), reflect.TypeOf(test.err))
 			continue
 		}

blockdag/dag.go

@@ -583,7 +583,7 @@ func (dag *BlockDAG) connectBlock(node *blockNode,
 	}
 
 	// Apply all changes to the DAG.
-	virtualUTXODiff, virtualTxsAcceptanceData, chainUpdates, err := dag.applyDAGChanges(node, block, newBlockUTXO, fastAdd)
+	virtualUTXODiff, virtualTxsAcceptanceData, chainUpdates, err := dag.applyDAGChanges(node, newBlockUTXO, fastAdd)
 	if err != nil {
 		// Since all validation logic has already ran, if applyDAGChanges errors out,
 		// this means we have a problem in the internal structure of the DAG - a problem which is
@@ -889,37 +889,27 @@ func (dag *BlockDAG) BlockPastUTXO(blockHash *daghash.Hash) (UTXOSet, error) {
 // It returns the diff in the virtual block's UTXO set.
 //
 // This function MUST be called with the DAG state lock held (for writes).
-func (dag *BlockDAG) applyDAGChanges(node *blockNode, block *util.Block, newBlockUTXO UTXOSet, fastAdd bool) (
+func (dag *BlockDAG) applyDAGChanges(node *blockNode, newBlockUTXO UTXOSet, fastAdd bool) (
 	virtualUTXODiff *UTXODiff, virtualTxsAcceptanceData MultiBlockTxsAcceptanceData,
 	chainUpdates *chainUpdates, err error) {
 
 	if err = node.updateParents(dag, newBlockUTXO); err != nil {
-		return nil, nil, nil, errors.Errorf("failed updating parents of %s: %s", node, err)
+		return nil, nil, nil, errors.Wrapf(err, "failed updating parents of %s", node)
 	}
 
 	// Update the virtual block's parents (the DAG tips) to include the new block.
 	chainUpdates = dag.virtual.AddTip(node)
 
 	// Build a UTXO set for the new virtual block
-	newVirtualPastUTXO, virtualTxsAcceptanceData, err := dag.pastUTXO(&dag.virtual.blockNode)
+	newVirtualUTXO, virtualTxsAcceptanceData, err := dag.pastUTXO(&dag.virtual.blockNode)
 	if err != nil {
-		return nil, nil, nil, errors.Errorf("could not restore past UTXO for virtual %s: %s", dag.virtual, err)
+		return nil, nil, nil, errors.Wrap(err, "could not restore past UTXO for virtual")
-	}
-
-	// Apply the new virtual's blue score to all the unaccepted UTXOs
-	diffFromAcceptanceData, err := dag.virtual.diffFromAcceptanceData(newVirtualPastUTXO, virtualTxsAcceptanceData)
-	if err != nil {
-		return nil, nil, nil, err
-	}
-	newVirtualUTXO, err := newVirtualPastUTXO.WithDiff(diffFromAcceptanceData)
-	if err != nil {
-		return nil, nil, nil, err
 	}
 
 	// Apply new utxoDiffs to all the tips
 	err = updateTipsUTXO(dag, newVirtualUTXO)
 	if err != nil {
-		return nil, nil, nil, errors.Errorf("failed updating the tips' UTXO: %s", err)
+		return nil, nil, nil, errors.Wrap(err, "failed updating the tips' UTXO")
 	}
 
 	// It is now safe to meld the UTXO set to base.
@@ -927,7 +917,7 @@ func (dag *BlockDAG) applyDAGChanges(node *blockNode, block *util.Block, newBloc
 	virtualUTXODiff = diffSet.UTXODiff
 	err = dag.meldVirtualUTXO(diffSet)
 	if err != nil {
-		return nil, nil, nil, errors.Errorf("failed melding the virtual UTXO: %s", err)
+		return nil, nil, nil, errors.Wrap(err, "failed melding the virtual UTXO")
 	}
 
 	dag.index.SetStatusFlags(node, statusValid)
@@ -961,29 +951,6 @@ func (node *blockNode) diffFromTxs(pastUTXO UTXOSet, transactions []*util.Tx) (*
 	return diff, nil
 }
 
-// diffFromAccpetanceData creates a diff that "updates" the blue scores of the given
-// UTXOSet with the node's blueScore according to the given acceptance data.
-func (node *blockNode) diffFromAcceptanceData(pastUTXO UTXOSet, multiBlockTxsAcceptanceData MultiBlockTxsAcceptanceData) (*UTXODiff, error) {
-	diff := NewUTXODiff()
-
-	for _, blockTxsAcceptanceData := range multiBlockTxsAcceptanceData {
-		for _, txAcceptanceData := range blockTxsAcceptanceData.TxAcceptanceData {
-			if txAcceptanceData.IsAccepted {
-				acceptanceDiff, err := pastUTXO.diffFromAcceptedTx(txAcceptanceData.Tx.MsgTx(), node.blueScore)
-				if err != nil {
-					return nil, err
-				}
-				diff, err = diff.WithDiff(acceptanceDiff)
-				if err != nil {
-					return nil, err
-				}
-			}
-		}
-	}
-
-	return diff, nil
-}
-
 // verifyAndBuildUTXO verifies all transactions in the given block and builds its UTXO
 // to save extra traversals it returns the transactions acceptance data and the compactFeeData for the new block
 func (node *blockNode) verifyAndBuildUTXO(dag *BlockDAG, transactions []*util.Tx, fastAdd bool) (
@@ -1004,22 +971,11 @@ func (node *blockNode) verifyAndBuildUTXO(dag *BlockDAG, transactions []*util.Tx
 		return nil, nil, nil, err
 	}
 
-	// We diff from the acceptance data here to "replace" the blueScore that was diff-ed
+	diffFromTxs, err := node.diffFromTxs(pastUTXO, transactions)
-	// out of the virtual's UTXO in pastUTXO with this node's blueScore.
-	diffFromAcceptanceData, err := node.diffFromAcceptanceData(pastUTXO, txsAcceptanceData)
 	if err != nil {
 		return nil, nil, nil, err
 	}
-	utxo, err := pastUTXO.WithDiff(diffFromAcceptanceData)
+	utxo, err := pastUTXO.WithDiff(diffFromTxs)
-	if err != nil {
-		return nil, nil, nil, err
-	}
-
-	diffFromTxs, err := node.diffFromTxs(utxo, transactions)
-	if err != nil {
-		return nil, nil, nil, err
-	}
-	utxo, err = utxo.WithDiff(diffFromTxs)
 	if err != nil {
 		return nil, nil, nil, err
 	}
@@ -1075,21 +1031,15 @@ func genesisPastUTXO(virtual *virtualBlock) UTXOSet {
 }
 
 func (node *blockNode) fetchBlueBlocks(db database.DB) ([]*util.Block, error) {
-	// Fetch from the database all the transactions for this block's blue set
+	blueBlocks := make([]*util.Block, len(node.blues))
-	blueBlocks := make([]*util.Block, 0, len(node.blues))
 	err := db.View(func(dbTx database.Tx) error {
-		// Precalculate the amount of transactions in this block's blue set, besides the selected parent.
+		for i, blueBlockNode := range node.blues {
-		// This is to avoid an attack in which an attacker fabricates a block that will deliberately cause
-		// a lot of copying, causing a high cost to the whole network.
-		for i := len(node.blues) - 1; i >= 0; i-- {
-			blueBlockNode := node.blues[i]
-
 			blueBlock, err := dbFetchBlockByNode(dbTx, blueBlockNode)
 			if err != nil {
 				return err
 			}
 
-			blueBlocks = append(blueBlocks, blueBlock)
+			blueBlocks[i] = blueBlock
 		}
 
 		return nil
@@ -1100,27 +1050,32 @@ func (node *blockNode) fetchBlueBlocks(db database.DB) ([]*util.Block, error) {
 // applyBlueBlocks adds all transactions in the blue blocks to the selectedParent's UTXO set
 // Purposefully ignoring failures - these are just unaccepted transactions
 // Writing down which transactions were accepted or not in txsAcceptanceData
-func (node *blockNode) applyBlueBlocks(selectedParentUTXO UTXOSet, blueBlocks []*util.Block) (
+func (node *blockNode) applyBlueBlocks(acceptedSelectedParentUTXO UTXOSet, selectedParentAcceptanceData []TxAcceptanceData, blueBlocks []*util.Block) (
 	pastUTXO UTXOSet, multiBlockTxsAcceptanceData MultiBlockTxsAcceptanceData, err error) {
 
-	pastUTXO = selectedParentUTXO
+	pastUTXO = acceptedSelectedParentUTXO
-	multiBlockTxsAcceptanceData = MultiBlockTxsAcceptanceData{}
+	multiBlockTxsAcceptanceData = MultiBlockTxsAcceptanceData{BlockTxsAcceptanceData{
+		BlockHash:        *node.selectedParent.hash,
+		TxAcceptanceData: selectedParentAcceptanceData,
+	}}
 
 	// Add blueBlocks to multiBlockTxsAcceptanceData bottom-to-top instead of
 	// top-to-bottom. This is so that anyone who iterates over it would process
 	// blocks (and transactions) in their order of appearance in the DAG.
-	for i := len(blueBlocks) - 1; i >= 0; i-- {
+	// We skip the selected parent, because we calculated its UTXO before.
+	for i := len(blueBlocks) - 2; i >= 0; i-- {
 		blueBlock := blueBlocks[i]
 		transactions := blueBlock.Transactions()
 		blockTxsAcceptanceData := BlockTxsAcceptanceData{
 			BlockHash:        *blueBlock.Hash(),
 			TxAcceptanceData: make([]TxAcceptanceData, len(transactions)),
 		}
-		isSelectedParent := blueBlock.Hash().IsEqual(node.selectedParent.hash)
 		for i, tx := range blueBlock.Transactions() {
 			var isAccepted bool
-			if isSelectedParent {
+			// Coinbase transaction outputs are added to the UTXO
-				isAccepted = true
+			// only if they are in the selected parent chain.
+			if tx.IsCoinBase() {
+				isAccepted = false
 			} else {
 				isAccepted, err = pastUTXO.AddTx(tx.MsgTx(), node.blueScore)
 				if err != nil {
@@ -1201,7 +1156,37 @@ func (dag *BlockDAG) pastUTXO(node *blockNode) (
 		return nil, nil, err
 	}
 
-	return node.applyBlueBlocks(selectedParentUTXO, blueBlocks)
+	selectedParent := blueBlocks[len(blueBlocks)-1]
+	acceptedSelectedParentUTXO, selectedParentAcceptanceData, err := node.acceptSelectedParentTransactions(selectedParent, selectedParentUTXO)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	return node.applyBlueBlocks(acceptedSelectedParentUTXO, selectedParentAcceptanceData, blueBlocks)
+}
+
+func (node *blockNode) acceptSelectedParentTransactions(selectedParent *util.Block, selectedParentUTXO UTXOSet) (acceptedSelectedParentUTXO UTXOSet, txAcceptanceData []TxAcceptanceData, err error) {
+	diff := NewUTXODiff()
+	txAcceptanceData = make([]TxAcceptanceData, len(selectedParent.Transactions()))
+	for i, tx := range selectedParent.Transactions() {
+		txAcceptanceData[i] = TxAcceptanceData{
+			Tx:         tx,
+			IsAccepted: true,
+		}
+		acceptanceDiff, err := selectedParentUTXO.diffFromAcceptedTx(tx.MsgTx(), node.blueScore)
+		if err != nil {
+			return nil, nil, err
+		}
+		diff, err = diff.WithDiff(acceptanceDiff)
+		if err != nil {
+			return nil, nil, err
+		}
+	}
+	acceptedSelectedParentUTXO, err = selectedParentUTXO.WithDiff(diff)
+	if err != nil {
+		return nil, nil, err
+	}
+	return acceptedSelectedParentUTXO, txAcceptanceData, nil
 }
 
 // restoreUTXO restores the UTXO of a given block from its diff

blockdag/test_utils.go

@@ -183,19 +183,11 @@ func GetVirtualFromParentsForTest(dag *BlockDAG, parentHashes []*daghash.Hash) (
 	}
 	virtual := newVirtualBlock(parents, dag.dagParams.K)
 
-	pastUTXO, acceptanceData, err := dag.pastUTXO(&virtual.blockNode)
+	pastUTXO, _, err := dag.pastUTXO(&virtual.blockNode)
 	if err != nil {
 		return nil, err
 	}
-	diffFromAcceptanceData, err := virtual.blockNode.diffFromAcceptanceData(pastUTXO, acceptanceData)
+	diffUTXO := pastUTXO.clone().(*DiffUTXOSet)
-	if err != nil {
-		return nil, err
-	}
-	utxo, err := pastUTXO.WithDiff(diffFromAcceptanceData)
-	if err != nil {
-		return nil, err
-	}
-	diffUTXO := utxo.clone().(*DiffUTXOSet)
 	err = diffUTXO.meldToBase()
 	if err != nil {
 		return nil, err

mining/mining.go

@@ -224,7 +224,7 @@ func (g *BlkTmplGenerator) NewBlockTemplate(payToAddress util.Address) (*BlockTe
 
 	txsForBlockTemplate, err := g.selectTxs(payToAddress)
 	if err != nil {
-		return nil, errors.Errorf("failed to select txs: %s", err)
+		return nil, errors.Errorf("failed to select transactions: %s", err)
 	}
 
 	// Calculate the required difficulty for the block.  The timestamp