[NOD-406] Don't do ECMH operations on mempool (#467)

* [NOD-406] Don't do ECMH operations on mempool

* [NOD-406] Change NewUTXODiff(false) to NewUTXODiffWithoutMultiset

* [NOD-406] Rename dClone -> clone

* [NOD-406] Remove redudnant assignment

* [NOD-406] Remove dag.UTXOToECMHCacheLock and make NewBlockTemplate use dag's write lock

* [NOD-406] Add tests to UTXO diffs without multiset

blockdag/dag.go

@@ -1767,6 +1767,16 @@ func (dag *BlockDAG) GetTopHeaders(startHash *daghash.Hash) ([]*wire.BlockHeader
 	return headers, nil
 }
 
+// Lock locks the DAG's UTXO set for writing.
+func (dag *BlockDAG) Lock() {
+	dag.dagLock.Lock()
+}
+
+// Unlock unlocks the DAG's UTXO set for writing.
+func (dag *BlockDAG) Unlock() {
+	dag.dagLock.Unlock()
+}
+
 // RLock locks the DAG's UTXO set for reading.
 func (dag *BlockDAG) RLock() {
 	dag.dagLock.RLock()

blockdag/utxo_ecmh.go

@@ -6,14 +6,12 @@ import (
 	"github.com/daglabs/btcd/util/daghash"
 	"github.com/daglabs/btcd/wire"
 	"github.com/golang/groupcache/lru"
-	"sync"
 )
 
 const ecmhCacheSize = 4_000_000
 
 var (
-	ecmhCache     = lru.New(ecmhCacheSize)
+	utxoToECMHCache = lru.New(ecmhCacheSize)
-	ecmhCacheLock sync.Mutex
 )
 
 func utxoMultiset(entry *UTXOEntry, outpoint *wire.Outpoint) (*btcec.Multiset, error) {
@@ -25,13 +23,10 @@ func utxoMultiset(entry *UTXOEntry, outpoint *wire.Outpoint) (*btcec.Multiset, e
 	serializedUTXO := w.Bytes()
 	utxoHash := daghash.DoubleHashH(serializedUTXO)
 
-	ecmhCacheLock.Lock()
+	if cachedMSPoint, ok := utxoToECMHCache.Get(utxoHash); ok {
-	defer ecmhCacheLock.Unlock()
-
-	if cachedMSPoint, ok := ecmhCache.Get(utxoHash); ok {
 		return cachedMSPoint.(*btcec.Multiset), nil
 	}
 	msPoint := btcec.NewMultiset(btcec.S256()).Add(serializedUTXO)
-	ecmhCache.Add(utxoHash, msPoint)
+	utxoToECMHCache.Add(utxoHash, msPoint)
 	return msPoint, nil
 }

blockdag/utxoio.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"encoding/binary"
 	"fmt"
+	"github.com/pkg/errors"
 	"io"
 	"math/big"
 
@@ -73,7 +74,9 @@ func (diffStore *utxoDiffStore) deserializeBlockUTXODiffData(serializedDiffDataB
 		diffData.diffChild = diffStore.dag.index.LookupNode(hash)
 	}
 
-	diffData.diff = &UTXODiff{}
+	diffData.diff = &UTXODiff{
+		useMultiset: true,
+	}
 
 	diffData.diff.toAdd, err = deserializeDiffEntries(serializedDiffData)
 	if err != nil {
@@ -155,6 +158,9 @@ func deserializeMultiset(r io.Reader) (*btcec.Multiset, error) {
 // serializeUTXODiff serializes UTXODiff by serializing
 // UTXODiff.toAdd, UTXODiff.toRemove and UTXODiff.Multiset one after the other.
 func serializeUTXODiff(w io.Writer, diff *UTXODiff) error {
+	if !diff.useMultiset {
+		return errors.New("Cannot serialize a UTXO diff without a multiset")
+	}
 	err := serializeUTXOCollection(w, diff.toAdd)
 	if err != nil {
 		return err

blockdag/utxoset.go

@@ -155,13 +155,25 @@ type UTXODiff struct {
 	toAdd        utxoCollection
 	toRemove     utxoCollection
 	diffMultiset *btcec.Multiset
+	useMultiset  bool
 }
 
-// NewUTXODiff creates a new, empty utxoDiff
+// NewUTXODiffWithoutMultiset creates a new, empty utxoDiff
+// without a multiset.
+func NewUTXODiffWithoutMultiset() *UTXODiff {
+	return &UTXODiff{
+		toAdd:       utxoCollection{},
+		toRemove:    utxoCollection{},
+		useMultiset: false,
+	}
+}
+
+// NewUTXODiff creates a new, empty utxoDiff.
 func NewUTXODiff() *UTXODiff {
 	return &UTXODiff{
 		toAdd:        utxoCollection{},
 		toRemove:     utxoCollection{},
+		useMultiset:  true,
 		diffMultiset: btcec.NewMultiset(btcec.S256()),
 	}
 }
@@ -196,9 +208,9 @@ func NewUTXODiff() *UTXODiff {
 //    diffFrom results in the UTXO being added to toAdd
 func (d *UTXODiff) diffFrom(other *UTXODiff) (*UTXODiff, error) {
 	result := UTXODiff{
-		toAdd:        make(utxoCollection, len(d.toRemove)+len(other.toAdd)),
+		toAdd:       make(utxoCollection, len(d.toRemove)+len(other.toAdd)),
-		toRemove:     make(utxoCollection, len(d.toAdd)+len(other.toRemove)),
+		toRemove:    make(utxoCollection, len(d.toAdd)+len(other.toRemove)),
-		diffMultiset: btcec.NewMultiset(btcec.S256()),
+		useMultiset: d.useMultiset,
 	}
 
 	// Note that the following cases are not accounted for, as they are impossible
@@ -264,8 +276,10 @@ func (d *UTXODiff) diffFrom(other *UTXODiff) (*UTXODiff, error) {
 		}
 	}
 
-	// Create a new diffMultiset as the subtraction of the two diffs.
+	if d.useMultiset {
-	result.diffMultiset = other.diffMultiset.Subtract(d.diffMultiset)
+		// Create a new diffMultiset as the subtraction of the two diffs.
+		result.diffMultiset = other.diffMultiset.Subtract(d.diffMultiset)
+	}
 
 	return &result, nil
 }
@@ -298,9 +312,9 @@ func (d *UTXODiff) diffFrom(other *UTXODiff) (*UTXODiff, error) {
 //    WithDiff results in the UTXO being added to toRemove
 func (d *UTXODiff) WithDiff(diff *UTXODiff) (*UTXODiff, error) {
 	result := UTXODiff{
-		toAdd:        make(utxoCollection, len(d.toAdd)+len(diff.toAdd)),
+		toAdd:       make(utxoCollection, len(d.toAdd)+len(diff.toAdd)),
-		toRemove:     make(utxoCollection, len(d.toRemove)+len(diff.toRemove)),
+		toRemove:    make(utxoCollection, len(d.toRemove)+len(diff.toRemove)),
-		diffMultiset: btcec.NewMultiset(btcec.S256()),
+		useMultiset: d.useMultiset,
 	}
 
 	// All transactions in d.toAdd:
@@ -364,21 +378,30 @@ func (d *UTXODiff) WithDiff(diff *UTXODiff) (*UTXODiff, error) {
 	}
 
 	// Apply diff.diffMultiset to d.diffMultiset
-	result.diffMultiset = d.diffMultiset.Union(diff.diffMultiset)
+	if d.useMultiset {
+		result.diffMultiset = d.diffMultiset.Union(diff.diffMultiset)
+	}
 
 	return &result, nil
 }
 
 // clone returns a clone of this utxoDiff
 func (d *UTXODiff) clone() *UTXODiff {
-	return &UTXODiff{
+	clone := &UTXODiff{
-		toAdd:        d.toAdd.clone(),
+		toAdd:       d.toAdd.clone(),
-		toRemove:     d.toRemove.clone(),
+		toRemove:    d.toRemove.clone(),
-		diffMultiset: d.diffMultiset.Clone(),
+		useMultiset: d.useMultiset,
 	}
+	if d.useMultiset {
+		clone.diffMultiset = d.diffMultiset.Clone()
+	}
+	return clone
 }
 
 // AddEntry adds a UTXOEntry to the diff
+//
+// If d.useMultiset is true, this function MUST be
+// called with the DAG lock held.
 func (d *UTXODiff) AddEntry(outpoint wire.Outpoint, entry *UTXOEntry) error {
 	if d.toRemove.containsWithBlueScore(outpoint, entry.blockBlueScore) {
 		d.toRemove.remove(outpoint)
@@ -388,16 +411,20 @@ func (d *UTXODiff) AddEntry(outpoint wire.Outpoint, entry *UTXOEntry) error {
 		d.toAdd.add(outpoint, entry)
 	}
 
-	var err error
+	if d.useMultiset {
-	newMs, err := addUTXOToMultiset(d.diffMultiset, entry, &outpoint)
+		newMs, err := addUTXOToMultiset(d.diffMultiset, entry, &outpoint)
-	if err != nil {
+		if err != nil {
-		return err
+			return err
+		}
+		d.diffMultiset = newMs
 	}
-	d.diffMultiset = newMs
 	return nil
 }
 
-// RemoveEntry removes a UTXOEntry from the diff
+// RemoveEntry removes a UTXOEntry from the diff.
+//
+// If d.useMultiset is true, this function MUST be
+// called with the DAG lock held.
 func (d *UTXODiff) RemoveEntry(outpoint wire.Outpoint, entry *UTXOEntry) error {
 	if d.toAdd.containsWithBlueScore(outpoint, entry.blockBlueScore) {
 		d.toAdd.remove(outpoint)
@@ -407,17 +434,21 @@ func (d *UTXODiff) RemoveEntry(outpoint wire.Outpoint, entry *UTXOEntry) error {
 		d.toRemove.add(outpoint, entry)
 	}
 
-	var err error
+	if d.useMultiset {
-	newMs, err := removeUTXOFromMultiset(d.diffMultiset, entry, &outpoint)
+		newMs, err := removeUTXOFromMultiset(d.diffMultiset, entry, &outpoint)
-	if err != nil {
+		if err != nil {
-		return err
+			return err
+		}
+		d.diffMultiset = newMs
 	}
-	d.diffMultiset = newMs
 	return nil
 }
 
 func (d UTXODiff) String() string {
-	return fmt.Sprintf("toAdd: %s; toRemove: %s, Multiset-Hash: %s", d.toAdd, d.toRemove, d.diffMultiset.Hash())
+	if d.useMultiset {
+		return fmt.Sprintf("toAdd: %s; toRemove: %s, Multiset-Hash: %s", d.toAdd, d.toRemove, d.diffMultiset.Hash())
+	}
+	return fmt.Sprintf("toAdd: %s; toRemove: %s", d.toAdd, d.toRemove)
 }
 
 // UTXOSet represents a set of unspent transaction outputs
@@ -523,6 +554,7 @@ func NewFullUTXOSet() *FullUTXOSet {
 	}
 }
 
+// newFullUTXOSetFromUTXOCollection converts a utxoCollection to a FullUTXOSet
 func newFullUTXOSetFromUTXOCollection(collection utxoCollection) (*FullUTXOSet, error) {
 	var err error
 	multiset := btcec.NewMultiset(btcec.S256())
@@ -561,6 +593,8 @@ func (fus *FullUTXOSet) WithDiff(other *UTXODiff) (UTXOSet, error) {
 // AddTx adds a transaction to this utxoSet and returns isAccepted=true iff it's valid in this UTXO's context.
 // It returns error if something unexpected happens, such as serialization error (isAccepted=false doesn't
 // necessarily means there's an error).
+//
+// This function MUST be called with the DAG lock held.
 func (fus *FullUTXOSet) AddTx(tx *wire.MsgTx, blueScore uint64) (isAccepted bool, err error) {
 	isCoinbase := tx.IsCoinBase()
 	if !isCoinbase {
@@ -654,7 +688,9 @@ func (fus *FullUTXOSet) removeAndUpdateMultiset(outpoint wire.Outpoint) error {
 	return nil
 }
 
-// WithTransactions returns a new UTXO Set with the added transactions
+// WithTransactions returns a new UTXO Set with the added transactions.
+//
+// This function MUST be called with the DAG lock held.
 func (fus *FullUTXOSet) WithTransactions(transactions []*wire.MsgTx, blockBlueScore uint64, ignoreDoubleSpends bool) (UTXOSet, error) {
 	diffSet := NewDiffUTXOSet(fus, NewUTXODiff())
 	for _, tx := range transactions {
@@ -708,7 +744,10 @@ func (dus *DiffUTXOSet) WithDiff(other *UTXODiff) (UTXOSet, error) {
 	return NewDiffUTXOSet(dus.base, diff), nil
 }
 
-// AddTx adds a transaction to this utxoSet and returns true iff it's valid in this UTXO's context
+// AddTx adds a transaction to this utxoSet and returns true iff it's valid in this UTXO's context.
+//
+// If dus.UTXODiff.useMultiset is true, this function MUST be
+// called with the DAG lock held.
 func (dus *DiffUTXOSet) AddTx(tx *wire.MsgTx, blockBlueScore uint64) (bool, error) {
 	isCoinbase := tx.IsCoinBase()
 	if !isCoinbase && !dus.containsInputs(tx) {
@@ -778,9 +817,15 @@ func (dus *DiffUTXOSet) meldToBase() error {
 		dus.base.add(outpoint, utxoEntry)
 	}
 
-	dus.base.UTXOMultiset = dus.base.UTXOMultiset.Union(dus.UTXODiff.diffMultiset)
+	if dus.UTXODiff.useMultiset {
+		dus.base.UTXOMultiset = dus.base.UTXOMultiset.Union(dus.UTXODiff.diffMultiset)
+	}
 
-	dus.UTXODiff = NewUTXODiff()
+	if dus.UTXODiff.useMultiset {
+		dus.UTXODiff = NewUTXODiff()
+	} else {
+		dus.UTXODiff = NewUTXODiffWithoutMultiset()
+	}
 	return nil
 }
 
@@ -826,7 +871,10 @@ func (dus *DiffUTXOSet) Multiset() *btcec.Multiset {
 	return dus.base.UTXOMultiset.Union(dus.UTXODiff.diffMultiset)
 }
 
-// WithTransactions returns a new UTXO Set with the added transactions
+// WithTransactions returns a new UTXO Set with the added transactions.
+//
+// If dus.UTXODiff.useMultiset is true, this function MUST be
+// called with the DAG lock held.
 func (dus *DiffUTXOSet) WithTransactions(transactions []*wire.MsgTx, blockBlueScore uint64, ignoreDoubleSpends bool) (UTXOSet, error) {
 	diffSet := NewDiffUTXOSet(dus.base, dus.UTXODiff.clone())
 	for _, tx := range transactions {

blockdag/utxoset_test.go

@@ -21,7 +21,7 @@ func TestUTXOCollection(t *testing.T) {
 	utxoEntry1 := NewUTXOEntry(&wire.TxOut{ScriptPubKey: []byte{}, Value: 20}, false, 1)
 
 	// For each of the following test cases, we will:
-	// .String() the given collection and compare it to expectedString
+	// .String() the given collection and compare it to expectedStringWithMultiset
 	// .clone() the given collection and compare its value to itself (expected: equals) and its reference to itself (expected: not equal)
 	tests := []struct {
 		name           string
@@ -79,38 +79,49 @@ func TestUTXODiff(t *testing.T) {
 	utxoEntry0 := NewUTXOEntry(&wire.TxOut{ScriptPubKey: []byte{}, Value: 10}, true, 0)
 	utxoEntry1 := NewUTXOEntry(&wire.TxOut{ScriptPubKey: []byte{}, Value: 20}, false, 1)
 
-	// Test utxoDiff creation
+	for i := 0; i < 2; i++ {
-	diff := NewUTXODiff()
+		withMultiset := i == 0
-	if len(diff.toAdd) != 0 || len(diff.toRemove) != 0 {
+		// Test utxoDiff creation
-		t.Errorf("new diff is not empty")
+		var diff *UTXODiff
-	}
+		if withMultiset {
+			diff = NewUTXODiff()
+		} else {
+			diff = NewUTXODiffWithoutMultiset()
+		}
+		if len(diff.toAdd) != 0 || len(diff.toRemove) != 0 {
+			t.Errorf("new diff is not empty")
+		}
 
-	err := diff.AddEntry(outpoint0, utxoEntry0)
+		err := diff.AddEntry(outpoint0, utxoEntry0)
-	if err != nil {
+		if err != nil {
-		t.Fatalf("Error adding entry to utxo diff: %s", err)
+			t.Fatalf("error adding entry to utxo diff: %s", err)
-	}
+		}
 
-	err = diff.RemoveEntry(outpoint1, utxoEntry1)
+		err = diff.RemoveEntry(outpoint1, utxoEntry1)
-	if err != nil {
+		if err != nil {
-		t.Fatalf("Error adding entry to utxo diff: %s", err)
+			t.Fatalf("error adding entry to utxo diff: %s", err)
-	}
+		}
 
-	// Test utxoDiff cloning
+		// Test utxoDiff cloning
-	clonedDiff := diff.clone()
+		clonedDiff := diff.clone()
-	if clonedDiff == diff {
+		if clonedDiff == diff {
-		t.Errorf("cloned diff is reference-equal to the original")
+			t.Errorf("cloned diff is reference-equal to the original")
-	}
+		}
-	if !reflect.DeepEqual(clonedDiff, diff) {
+		if !reflect.DeepEqual(clonedDiff, diff) {
-		t.Errorf("cloned diff not equal to the original"+
+			t.Errorf("cloned diff not equal to the original"+
-			"Original: \"%v\", cloned: \"%v\".", diff, clonedDiff)
+				"Original: \"%v\", cloned: \"%v\".", diff, clonedDiff)
-	}
+		}
 
-	// Test utxoDiff string representation
+		// Test utxoDiff string representation
-	expectedDiffString := "toAdd: [ (0000000000000000000000000000000000000000000000000000000000000000, 0) => 10, blueScore: 0 ]; toRemove: [ (1111111111111111111111111111111111111111111111111111111111111111, 0) => 20, blueScore: 1 ], Multiset-Hash: 7cb61e48005b0c817211d04589d719bff87d86a6a6ce2454515f57265382ded7"
+		expectedDiffString := "toAdd: [ (0000000000000000000000000000000000000000000000000000000000000000, 0) => 10, blueScore: 0 ]; toRemove: [ (1111111111111111111111111111111111111111111111111111111111111111, 0) => 20, blueScore: 1 ]"
-	diffString := clonedDiff.String()
+		if withMultiset {
-	if diffString != expectedDiffString {
+			expectedDiffString = "toAdd: [ (0000000000000000000000000000000000000000000000000000000000000000, 0) => 10, blueScore: 0 ]; toRemove: [ (1111111111111111111111111111111111111111111111111111111111111111, 0) => 20, blueScore: 1 ], Multiset-Hash: 7cb61e48005b0c817211d04589d719bff87d86a6a6ce2454515f57265382ded7"
-		t.Errorf("unexpected diff string. "+
+		}
-			"Expected: \"%s\", got: \"%s\".", expectedDiffString, diffString)
+		diffString := clonedDiff.String()
+		if diffString != expectedDiffString {
+			t.Errorf("unexpected diff string. "+
+				"Expected: \"%s\", got: \"%s\".", expectedDiffString, diffString)
+		}
 	}
 }
 

mempool/mempool.go

@@ -466,7 +466,7 @@ func (mp *TxPool) HaveTransaction(hash *daghash.TxID) bool {
 //
 // This function MUST be called with the mempool lock held (for writes).
 func (mp *TxPool) removeTransactions(txs []*util.Tx) error {
-	diff := blockdag.NewUTXODiff()
+	diff := blockdag.NewUTXODiffWithoutMultiset()
 
 	for _, tx := range txs {
 		txID := tx.ID()
@@ -514,7 +514,7 @@ func (mp *TxPool) removeTransaction(tx *util.Tx, removeDependants bool, restoreI
 		return nil
 	}
 
-	diff := blockdag.NewUTXODiff()
+	diff := blockdag.NewUTXODiffWithoutMultiset()
 	err := mp.removeTransactionWithDiff(tx, diff, restoreInputs)
 	if err != nil {
 		return err
@@ -1375,7 +1375,7 @@ func (mp *TxPool) HandleNewBlock(block *util.Block, txChan chan NewBlockMsg) err
 // transactions until they are mined into a block.
 func New(cfg *Config) *TxPool {
 	virtualUTXO := cfg.DAG.UTXOSet()
-	mpUTXO := blockdag.NewDiffUTXOSet(virtualUTXO, blockdag.NewUTXODiff())
+	mpUTXO := blockdag.NewDiffUTXOSet(virtualUTXO, blockdag.NewUTXODiffWithoutMultiset())
 	return &TxPool{
 		cfg:            *cfg,
 		pool:           make(map[daghash.TxID]*TxDesc),

mining/mining.go

@@ -219,8 +219,8 @@ func NewBlkTmplGenerator(policy *Policy, params *dagconfig.Params,
 //  |  <= policy.BlockMinSize)          |   |
 //   -----------------------------------  --
 func (g *BlkTmplGenerator) NewBlockTemplate(payToAddress util.Address) (*BlockTemplate, error) {
-	g.dag.RLock()
+	g.dag.Lock()
-	defer g.dag.RUnlock()
+	defer g.dag.Unlock()
 
 	txsForBlockTemplate, err := g.selectTxs(payToAddress)
 	if err != nil {