txscript: optimize sigcache lookup (#598)

Profiles discovered that lookups into the signature cache included an
expensive comparison to the stored `sigInfo` struct. This lookup had the
potential to be more expensive than directly verifying the signature
itself!

In addition, evictions were rather expensive because they involved
reading from /dev/urandom, or equivalent, for each eviction once the
signature cache was full as well as potentially iterating over every
item in the cache in the worst-case.

To remedy this poor performance several changes have been made:
* Change the lookup key to the fixed sized 32-byte signature hash
* Perform a full equality check only if there is a cache hit which
    results in a significant  speed up for both insertions and existence
checks
* Override entries in the case of a colliding hash on insert Add an
* .IsEqual() method to the Signature and PublicKey types in the
  btcec package to facilitate easy equivalence testing
* Allocate the signature cache map with the max number of entries in
  order to avoid unnecessary map re-sizes/allocations
* Optimize evictions from the signature cache Delete the first entry
* seen which is safe from manipulation due to
    the pre image resistance of the hash function
* Double the default maximum number of entries within the signature
  cache due to the reduction in the size of a cache entry
  * With this eviction scheme, removals are effectively O(1)

Fixes #575.

btcec/pubkey.go

@@ -153,6 +153,14 @@ func (p *PublicKey) SerializeHybrid() []byte {
 	return paddedAppend(32, b, p.Y.Bytes())
 }
 
+// IsEqual compares this PublicKey instance to the one passed, returning true if
+// both PublicKeys are equivalent. A PublicKey is equivalent to another, if they
+// both have the same X and Y coordinate.
+func (p *PublicKey) IsEqual(otherPubKey *PublicKey) bool {
+	return p.X.Cmp(otherPubKey.X) == 0 &&
+		p.Y.Cmp(otherPubKey.Y) == 0
+}
+
 // paddedAppend appends the src byte slice to dst, returning the new slice.
 // If the length of the source is smaller than the passed size, leading zero
 // bytes are appended to the dst slice before appending src.

btcec/pubkey_test.go

@@ -247,3 +247,39 @@ func TestPubKeys(t *testing.T) {
 		}
 	}
 }
+
+func TestPublicKeyIsEqual(t *testing.T) {
+	pubKey1, err := btcec.ParsePubKey(
+		[]byte{0x03, 0x26, 0x89, 0xc7, 0xc2, 0xda, 0xb1, 0x33,
+			0x09, 0xfb, 0x14, 0x3e, 0x0e, 0x8f, 0xe3, 0x96, 0x34,
+			0x25, 0x21, 0x88, 0x7e, 0x97, 0x66, 0x90, 0xb6, 0xb4,
+			0x7f, 0x5b, 0x2a, 0x4b, 0x7d, 0x44, 0x8e,
+		},
+		btcec.S256(),
+	)
+	if err != nil {
+		t.Fatalf("failed to parse raw bytes for pubKey1: %v", err)
+	}
+
+	pubKey2, err := btcec.ParsePubKey(
+		[]byte{0x02, 0xce, 0x0b, 0x14, 0xfb, 0x84, 0x2b, 0x1b,
+			0xa5, 0x49, 0xfd, 0xd6, 0x75, 0xc9, 0x80, 0x75, 0xf1,
+			0x2e, 0x9c, 0x51, 0x0f, 0x8e, 0xf5, 0x2b, 0xd0, 0x21,
+			0xa9, 0xa1, 0xf4, 0x80, 0x9d, 0x3b, 0x4d,
+		},
+		btcec.S256(),
+	)
+	if err != nil {
+		t.Fatalf("failed to parse raw bytes for pubKey2: %v", err)
+	}
+
+	if !pubKey1.IsEqual(pubKey1) {
+		t.Fatalf("value of IsEqual is incorrect, %v is "+
+			"equal to %v", pubKey1, pubKey1)
+	}
+
+	if pubKey1.IsEqual(pubKey2) {
+		t.Fatalf("value of IsEqual is incorrect, %v is not "+
+			"equal to %v", pubKey1, pubKey2)
+	}
+}

btcec/signature.go

@@ -82,6 +82,14 @@ func (sig *Signature) Verify(hash []byte, pubKey *PublicKey) bool {
 	return ecdsa.Verify(pubKey.ToECDSA(), hash, sig.R, sig.S)
 }
 
+// IsEqual compares this Signature instance to the one passed, returning true
+// if both Signatures are equivalent. A signature is equivalent to another, if
+// they both have the same scalar value for R and S.
+func (sig *Signature) IsEqual(otherSig *Signature) bool {
+	return sig.R.Cmp(otherSig.R) == 0 &&
+		sig.S.Cmp(otherSig.S) == 0
+}
+
 func parseSig(sigStr []byte, curve elliptic.Curve, der bool) (*Signature, error) {
 	// Originally this code used encoding/asn1 in order to parse the
 	// signature, but a number of problems were found with this approach.

btcec/signature_test.go

@@ -588,3 +588,24 @@ func TestRFC6979(t *testing.T) {
 		}
 	}
 }
+
+func TestSignatureIsEqual(t *testing.T) {
+	sig1 := &btcec.Signature{
+		R: fromHex("0082235e21a2300022738dabb8e1bbd9d19cfb1e7ab8c30a23b0afbb8d178abcf3"),
+		S: fromHex("24bf68e256c534ddfaf966bf908deb944305596f7bdcc38d69acad7f9c868724"),
+	}
+	sig2 := &btcec.Signature{
+		R: fromHex("4e45e16932b8af514961a1d3a1a25fdf3f4f7732e9d624c6c61548ab5fb8cd41"),
+		S: fromHex("181522ec8eca07de4860a4acdd12909d831cc56cbbac4622082221a8768d1d09"),
+	}
+
+	if !sig1.IsEqual(sig1) {
+		t.Fatalf("value of IsEqual is incorrect, %v is "+
+			"equal to %v", sig1, sig1)
+	}
+
+	if sig1.IsEqual(sig2) {
+		t.Fatalf("value of IsEqual is incorrect, %v is not "+
+			"equal to %v", sig1, sig2)
+	}
+}

config.go

@@ -46,7 +46,7 @@ const (
 	defaultGenerate              = false
 	defaultMaxOrphanTransactions = 1000
 	defaultMaxOrphanTxSize       = 5000
-	defaultSigCacheMaxSize       = 50000
+	defaultSigCacheMaxSize       = 100000
 	defaultTxIndex               = false
 	defaultAddrIndex             = false
 )

txscript/reference_test.go

@@ -194,6 +194,7 @@ func TestScriptInvalidTests(t *testing.T) {
 		return
 	}
 	sigCache := NewSigCache(10)
+
 	sigCacheToggle := []bool{true, false}
 	for _, useSigCache := range sigCacheToggle {
 		for i, test := range tests {
@@ -258,7 +259,9 @@ func TestScriptValidTests(t *testing.T) {
 			err)
 		return
 	}
+
 	sigCache := NewSigCache(10)
+
 	sigCacheToggle := []bool{true, false}
 	for _, useSigCache := range sigCacheToggle {
 		for i, test := range tests {

txscript/sigcache.go

@@ -5,20 +5,21 @@
 package txscript
 
 import (
-	"bytes"
-	"crypto/rand"
 	"sync"
 
 	"github.com/btcsuite/btcd/btcec"
 	"github.com/btcsuite/btcd/wire"
 )
 
-// sigInfo represents an entry in the SigCache. Entries in the sigcache are a
-// 3-tuple: (sigHash, sig, pubKey).
-type sigInfo struct {
-	sigHash wire.ShaHash
-	sig     string
-	pubKey  string
+// sigCacheEntry represents an entry in the SigCache. Entries within the
+// SigCache are keyed according to the sigHash of the signature. In the
+// scenario of a cache-hit (according to the sigHash), an additional comparison
+// of the signature, and public key will be executed in order to ensure a complete
+// match. In the occasion that two sigHashes collide, the newer sigHash will
+// simply overwrite the existing entry.
+type sigCacheEntry struct {
+	sig    *btcec.Signature
+	pubKey *btcec.PublicKey
 }
 
 // SigCache implements an ECDSA signature verification cache with a randomized
@@ -33,7 +34,7 @@ type sigInfo struct {
 // if they've already been seen and verified within the mempool.
 type SigCache struct {
 	sync.RWMutex
-	validSigs  map[sigInfo]struct{}
+	validSigs  map[wire.ShaHash]sigCacheEntry
 	maxEntries uint
 }
 
@@ -43,7 +44,10 @@ type SigCache struct {
 // to make room for new entries that would cause the number of entries in the
 // cache to exceed the max.
 func NewSigCache(maxEntries uint) *SigCache {
-	return &SigCache{validSigs: make(map[sigInfo]struct{}), maxEntries: maxEntries}
+	return &SigCache{
+		validSigs:  make(map[wire.ShaHash]sigCacheEntry, maxEntries),
+		maxEntries: maxEntries,
+	}
 }
 
 // Exists returns true if an existing entry of 'sig' over 'sigHash' for public
@@ -52,13 +56,14 @@ func NewSigCache(maxEntries uint) *SigCache {
 // NOTE: This function is safe for concurrent access. Readers won't be blocked
 // unless there exists a writer, adding an entry to the SigCache.
 func (s *SigCache) Exists(sigHash wire.ShaHash, sig *btcec.Signature, pubKey *btcec.PublicKey) bool {
-	info := sigInfo{sigHash, string(sig.Serialize()),
-		string(pubKey.SerializeCompressed())}
-
 	s.RLock()
-	_, ok := s.validSigs[info]
-	s.RUnlock()
-	return ok
+	defer s.RUnlock()
+
+	if entry, ok := s.validSigs[sigHash]; ok {
+		return entry.pubKey.IsEqual(pubKey) && entry.sig.IsEqual(sig)
+	}
+
+	return false
 }
 
 // Add adds an entry for a signature over 'sigHash' under public key 'pubKey'
@@ -79,35 +84,19 @@ func (s *SigCache) Add(sigHash wire.ShaHash, sig *btcec.Signature, pubKey *btcec
 	// If adding this new entry will put us over the max number of allowed
 	// entries, then evict an entry.
 	if uint(len(s.validSigs)+1) > s.maxEntries {
-		// Generate a cryptographically random hash.
-		randHashBytes := make([]byte, wire.HashSize)
-		_, err := rand.Read(randHashBytes)
-		if err != nil {
-			// Failure to read a random hash results in the proposed
-			// entry not being added to the cache since we are
-			// unable to evict any existing entries.
-			return
-		}
-
-		// Try to find the first entry that is greater than the random
-		// hash. Use the first entry (which is already pseudo random due
-		// to Go's range statement over maps) as a fall back if none of
-		// the hashes in the rejected transactions pool are larger than
-		// the random hash.
-		var foundEntry sigInfo
+		// Remove a random entry from the map relaying on the random
+		// starting point of Go's map iteration. It's worth noting that
+		// the random iteration starting point is not 100% guaranteed
+		// by the spec, however most Go compilers support it.
+		// Ultimately, the iteration order isn't important here because
+		// in order to manipulate which items are evicted, an adversary
+		// would need to be able to execute preimage attacks on the
+		// hashing function in order to start eviction at a specific
+		// entry.
 		for sigEntry := range s.validSigs {
-			if foundEntry.sig == "" {
-				foundEntry = sigEntry
-			}
-			if bytes.Compare(sigEntry.sigHash.Bytes(), randHashBytes) > 0 {
-				foundEntry = sigEntry
-				break
-			}
+			delete(s.validSigs, sigEntry)
+			break
 		}
-		delete(s.validSigs, foundEntry)
 	}
-
-	info := sigInfo{sigHash, string(sig.Serialize()),
-		string(pubKey.SerializeCompressed())}
-	s.validSigs[info] = struct{}{}
+	s.validSigs[sigHash] = sigCacheEntry{sig, pubKey}
 }