From 684cf4b5fa8abdfead92b595d15555cfd0d8e068 Mon Sep 17 00:00:00 2001 From: Ori Newman Date: Thu, 14 Nov 2019 17:40:05 +0200 Subject: [PATCH] [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 | 10 ++++ blockdag/utxo_ecmh.go | 11 ++-- blockdag/utxoio.go | 8 ++- blockdag/utxoset.go | 110 ++++++++++++++++++++++++++++----------- blockdag/utxoset_test.go | 69 +++++++++++++----------- mempool/mempool.go | 6 +-- mining/mining.go | 4 +- 7 files changed, 144 insertions(+), 74 deletions(-) diff --git a/blockdag/dag.go b/blockdag/dag.go index f7a86e741..903794405 100644 --- a/blockdag/dag.go +++ b/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() diff --git a/blockdag/utxo_ecmh.go b/blockdag/utxo_ecmh.go index c5dd747ae..a5069c559 100644 --- a/blockdag/utxo_ecmh.go +++ b/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) - ecmhCacheLock sync.Mutex + utxoToECMHCache = lru.New(ecmhCacheSize) ) 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() - defer ecmhCacheLock.Unlock() - - if cachedMSPoint, ok := ecmhCache.Get(utxoHash); ok { + if cachedMSPoint, ok := utxoToECMHCache.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 } diff --git a/blockdag/utxoio.go b/blockdag/utxoio.go index 2c4caa94c..5c486dde3 100644 --- a/blockdag/utxoio.go +++ b/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 diff --git a/blockdag/utxoset.go b/blockdag/utxoset.go index a495dbbee..3666bede2 100644 --- a/blockdag/utxoset.go +++ b/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)), - toRemove: make(utxoCollection, len(d.toAdd)+len(other.toRemove)), - diffMultiset: btcec.NewMultiset(btcec.S256()), + toAdd: make(utxoCollection, len(d.toRemove)+len(other.toAdd)), + toRemove: make(utxoCollection, len(d.toAdd)+len(other.toRemove)), + 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. - result.diffMultiset = other.diffMultiset.Subtract(d.diffMultiset) + if d.useMultiset { + // 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)), - toRemove: make(utxoCollection, len(d.toRemove)+len(diff.toRemove)), - diffMultiset: btcec.NewMultiset(btcec.S256()), + toAdd: make(utxoCollection, len(d.toAdd)+len(diff.toAdd)), + toRemove: make(utxoCollection, len(d.toRemove)+len(diff.toRemove)), + 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{ - toAdd: d.toAdd.clone(), - toRemove: d.toRemove.clone(), - diffMultiset: d.diffMultiset.Clone(), + clone := &UTXODiff{ + toAdd: d.toAdd.clone(), + toRemove: d.toRemove.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 - newMs, err := addUTXOToMultiset(d.diffMultiset, entry, &outpoint) - if err != nil { - return err + if d.useMultiset { + newMs, err := addUTXOToMultiset(d.diffMultiset, entry, &outpoint) + if err != nil { + 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 - newMs, err := removeUTXOFromMultiset(d.diffMultiset, entry, &outpoint) - if err != nil { - return err + if d.useMultiset { + newMs, err := removeUTXOFromMultiset(d.diffMultiset, entry, &outpoint) + if err != nil { + 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 { diff --git a/blockdag/utxoset_test.go b/blockdag/utxoset_test.go index b14e56389..09bba7841 100644 --- a/blockdag/utxoset_test.go +++ b/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 - diff := NewUTXODiff() - if len(diff.toAdd) != 0 || len(diff.toRemove) != 0 { - t.Errorf("new diff is not empty") - } + for i := 0; i < 2; i++ { + withMultiset := i == 0 + // Test utxoDiff creation + 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) - if err != nil { - t.Fatalf("Error adding entry to utxo diff: %s", err) - } + err := diff.AddEntry(outpoint0, utxoEntry0) + if err != nil { + t.Fatalf("error adding entry to utxo diff: %s", err) + } - err = diff.RemoveEntry(outpoint1, utxoEntry1) - if err != nil { - t.Fatalf("Error adding entry to utxo diff: %s", err) - } + err = diff.RemoveEntry(outpoint1, utxoEntry1) + if err != nil { + t.Fatalf("error adding entry to utxo diff: %s", err) + } - // Test utxoDiff cloning - clonedDiff := diff.clone() - if clonedDiff == diff { - t.Errorf("cloned diff is reference-equal to the original") - } - if !reflect.DeepEqual(clonedDiff, diff) { - t.Errorf("cloned diff not equal to the original"+ - "Original: \"%v\", cloned: \"%v\".", diff, clonedDiff) - } + // Test utxoDiff cloning + clonedDiff := diff.clone() + if clonedDiff == diff { + t.Errorf("cloned diff is reference-equal to the original") + } + if !reflect.DeepEqual(clonedDiff, diff) { + t.Errorf("cloned diff not equal to the original"+ + "Original: \"%v\", cloned: \"%v\".", diff, clonedDiff) + } - // Test utxoDiff string representation - expectedDiffString := "toAdd: [ (0000000000000000000000000000000000000000000000000000000000000000, 0) => 10, blueScore: 0 ]; toRemove: [ (1111111111111111111111111111111111111111111111111111111111111111, 0) => 20, blueScore: 1 ], Multiset-Hash: 7cb61e48005b0c817211d04589d719bff87d86a6a6ce2454515f57265382ded7" - diffString := clonedDiff.String() - if diffString != expectedDiffString { - t.Errorf("unexpected diff string. "+ - "Expected: \"%s\", got: \"%s\".", expectedDiffString, diffString) + // Test utxoDiff string representation + expectedDiffString := "toAdd: [ (0000000000000000000000000000000000000000000000000000000000000000, 0) => 10, blueScore: 0 ]; toRemove: [ (1111111111111111111111111111111111111111111111111111111111111111, 0) => 20, blueScore: 1 ]" + if withMultiset { + expectedDiffString = "toAdd: [ (0000000000000000000000000000000000000000000000000000000000000000, 0) => 10, blueScore: 0 ]; toRemove: [ (1111111111111111111111111111111111111111111111111111111111111111, 0) => 20, blueScore: 1 ], Multiset-Hash: 7cb61e48005b0c817211d04589d719bff87d86a6a6ce2454515f57265382ded7" + } + diffString := clonedDiff.String() + if diffString != expectedDiffString { + t.Errorf("unexpected diff string. "+ + "Expected: \"%s\", got: \"%s\".", expectedDiffString, diffString) + } } } diff --git a/mempool/mempool.go b/mempool/mempool.go index f616c4574..779f73bdb 100644 --- a/mempool/mempool.go +++ b/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), diff --git a/mining/mining.go b/mining/mining.go index 9cf022490..e1351a9d4 100644 --- a/mining/mining.go +++ b/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() - defer g.dag.RUnlock() + g.dag.Lock() + defer g.dag.Unlock() txsForBlockTemplate, err := g.selectTxs(payToAddress) if err != nil {