Initial round of code review

* Embed serializations of sigs and pubkeys into `sigInfo`. Previously
it held pointers, so it wasn’t caching the actual value.

* Implement caching within CHECKMULTISIG

* Update doc.go and sample-btcd.conf

* Fix some tests that weren’t updated to the new API

blockchain/common_test.go

@@ -116,7 +116,7 @@ func chainSetup(dbName string) (*blockchain.BlockChain, func(), error) {
 		return nil, nil, err
 	}
 
-	chain := blockchain.New(db, &chaincfg.MainNetParams, nil)
+	chain := blockchain.New(db, &chaincfg.MainNetParams, nil, nil)
 	return chain, teardown, nil
 }
 

blockchain/example_test.go

@@ -42,9 +42,10 @@ func ExampleBlockChain_ProcessBlock() {
 		return
 	}
 
-	// Create a new BlockChain instance using the underlying database for
-	// the main bitcoin network and ignore notifications.
-	chain := blockchain.New(db, &chaincfg.MainNetParams, nil)
+	// Create a new BlockChain instance without an initialized sigCache,
+	// using the underlying database for the main bitcoin network and ignore
+	// notifications.
+	chain := blockchain.New(db, &chaincfg.MainNetParams, nil, nil)
 
 	// Create a new median time source that is required by the upcoming
 	// call to ProcessBlock.  Ordinarily this would also add time values

blockchain/scriptval_test.go

@@ -37,7 +37,7 @@ func TestCheckBlockScripts(t *testing.T) {
 	}
 
 	scriptFlags := txscript.ScriptBip16
-	err = blockchain.TstCheckBlockScripts(blocks[0], txStore, scriptFlags)
+	err = blockchain.TstCheckBlockScripts(blocks[0], txStore, scriptFlags, nil)
 	if err != nil {
 		t.Errorf("Transaction script validation failed: %v\n",
 			err)

config.go

@@ -117,7 +117,7 @@ type config struct {
 	GetWorkKeys        []string      `long:"getworkkey" description:"DEPRECATED -- Use the --miningaddr option instead"`
 	AddrIndex          bool          `long:"addrindex" description:"Build and maintain a full address index. Currently only supported by leveldb."`
 	DropAddrIndex      bool          `long:"dropaddrindex" description:"Deletes the address-based transaction index from the database on start up, and the exits."`
-	SigCacheMaxSize    int           `long:"sigcachemaxsize" description:"hello"`
+	SigCacheMaxSize    int           `long:"sigcachemaxsize" description:"The maximum number of entries in the signature verification cache."`
 	onionlookup        func(string) ([]net.IP, error)
 	lookup             func(string) ([]net.IP, error)
 	oniondial          func(string, string) (net.Conn, error)

doc.go

@@ -101,6 +101,8 @@ Application Options:
                            only supported by leveldb.
       --dropaddrindex=     Deletes the address-based transaction index from the
                            database on start up, and the exits.
+      --sigcachemaxsize=   The maximum number of entries in the signature
+                           verification cache.
 Help Options:
   -h, --help           Show this help message
 

rpcserver.go

@@ -1406,7 +1406,7 @@ func (state *gbtWorkState) templateUpdateChan(prevHash *wire.ShaHash, lastGenera
 // addresses.
 //
 // This function MUST be called with the state locked.
-func (state *gbtWorkState) updateBlockTemplate(s *rpcServer, useCoinbaseValue bool, sigCache *txscript.SigCache) error {
+func (state *gbtWorkState) updateBlockTemplate(s *rpcServer, useCoinbaseValue bool) error {
 	lastTxUpdate := s.server.txMemPool.LastUpdated()
 	if lastTxUpdate.IsZero() {
 		lastTxUpdate = time.Now()
@@ -1444,7 +1444,8 @@ func (state *gbtWorkState) updateBlockTemplate(s *rpcServer, useCoinbaseValue bo
 		// block template doesn't include the coinbase, so the caller
 		// will ultimately create their own coinbase which pays to the
 		// appropriate address(es).
-		blkTemplate, err := NewBlockTemplate(s.server.txMemPool, payAddr, sigCache)
+		blkTemplate, err := NewBlockTemplate(s.server.txMemPool, payAddr,
+			s.server.sigCache)
 		if err != nil {
 			return internalRPCError("Failed to create new block "+
 				"template: "+err.Error(), "")
@@ -1685,7 +1686,7 @@ func handleGetBlockTemplateLongPoll(s *rpcServer, longPollID string, useCoinbase
 	// be manually unlocked before waiting for a notification about block
 	// template changes.
 
-	if err := state.updateBlockTemplate(s, useCoinbaseValue, s.server.sigCache); err != nil {
+	if err := state.updateBlockTemplate(s, useCoinbaseValue); err != nil {
 		state.Unlock()
 		return nil, err
 	}
@@ -1748,7 +1749,7 @@ func handleGetBlockTemplateLongPoll(s *rpcServer, longPollID string, useCoinbase
 	state.Lock()
 	defer state.Unlock()
 
-	if err := state.updateBlockTemplate(s, useCoinbaseValue, s.server.sigCache); err != nil {
+	if err := state.updateBlockTemplate(s, useCoinbaseValue); err != nil {
 		return nil, err
 	}
 
@@ -1842,7 +1843,7 @@ func handleGetBlockTemplateRequest(s *rpcServer, request *btcjson.TemplateReques
 	// seconds since the last template was generated.  Otherwise, the
 	// timestamp for the existing block template is updated (and possibly
 	// the difficulty on testnet per the consesus rules).
-	if err := state.updateBlockTemplate(s, useCoinbaseValue, s.server.sigCache); err != nil {
+	if err := state.updateBlockTemplate(s, useCoinbaseValue); err != nil {
 		return nil, err
 	}
 	return state.blockTemplateResult(useCoinbaseValue, nil)

sample-btcd.conf

@@ -231,6 +231,13 @@
 ; Delete the entire address index on start up, then exit.
 ; dropaddrindex=0
 
+; ------------------------------------------------------------------------------
+; Signature Verification Cache
+; ------------------------------------------------------------------------------
+
+; Limit the signature cache to a max of 50000 entries.
+; sigcachemaxsize=50000
+
 ; ------------------------------------------------------------------------------
 ; Coin Generation (Mining) Settings - The following options control the
 ; generation of block templates used by external mining applications through RPC

txscript/engine.go

@@ -75,7 +75,6 @@ var halfOrder = new(big.Int).Rsh(btcec.S256().N, 1)
 
 // Engine is the virtual machine that executes scripts.
 type Engine struct {
-	sigCache        *SigCache
 	scripts         [][]parsedOpcode
 	scriptIdx       int
 	scriptOff       int
@@ -87,6 +86,7 @@ type Engine struct {
 	condStack       []int
 	numOps          int
 	flags           ScriptFlags
+	sigCache        *SigCache
 	bip16           bool     // treat execution as pay-to-script-hash
 	savedFirstStack [][]byte // stack from first script for bip16 scripts
 }

txscript/opcode.go

@@ -2061,7 +2061,18 @@ func opcodeCheckMultiSig(op *parsedOpcode, vm *Engine) error {
 		// Generate the signature hash based on the signature hash type.
 		hash := calcSignatureHash(script, hashType, &vm.tx, vm.txIdx)
 
-		if parsedSig.Verify(hash, parsedPubKey) {
+		var valid bool
+		if vm.sigCache != nil {
+			valid = vm.sigCache.Exists(hash, parsedSig, parsedPubKey)
+			if !valid && parsedSig.Verify(hash, parsedPubKey) {
+				vm.sigCache.Add(hash, parsedSig, parsedPubKey)
+				valid = true
+			}
+		} else {
+			valid = parsedSig.Verify(hash, parsedPubKey)
+		}
+
+		if valid {
 			// PubKey verified, move on to the next signature.
 			signatureIdx++
 			numSignatures--

txscript/reference_test.go

@@ -261,9 +261,6 @@ func TestScriptValidTests(t *testing.T) {
 	sigCacheToggle := []bool{true, false}
 	for _, useSigCache := range sigCacheToggle {
 		for i, test := range tests {
-			var vm *Engine
-			var err error
-
 			// Skip comments
 			if len(test) == 1 {
 				continue
@@ -291,6 +288,7 @@ func TestScriptValidTests(t *testing.T) {
 			}
 			tx := createSpendingTx(scriptSig, scriptPubKey)
 
+			var vm *Engine
 			if useSigCache {
 				vm, err = NewEngine(scriptPubKey, tx, 0, flags, sigCache)
 			} else {

txscript/sigcache.go

@@ -16,11 +16,9 @@ import (
 // sigInfo represents an entry in the SigCache. Entries in the sigcache are a
 // 3-tuple: (sigCache, sig, pubKey).
 type sigInfo struct {
-	// TODO(roasbeef): Make this a string so it's hash func blocksize
-	// generic?
 	sigHash wire.ShaHash
-	sig     btcec.Signature
-	pubKey  btcec.PublicKey
+	sig     string
+	pubKey  string
 }
 
 // SigCache implements an ECDSA signature verification cache with a randomized
@@ -53,18 +51,19 @@ func NewSigCache(maxEntries int) *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 []byte, sig *btcec.Signature, pubKey *btcec.PublicKey) bool {
-	s.RLock()
-	defer s.RUnlock()
-
 	var candidateHash wire.ShaHash
 	copy(candidateHash[:], sigHash)
 
-	info := sigInfo{candidateHash, *sig, *pubKey}
+	info := sigInfo{candidateHash, string(sig.Serialize()),
+		string(pubKey.SerializeCompressed())}
+
+	s.RLock()
 	_, ok := s.validSigs[info]
+	s.RUnlock()
 	return ok
 }
 
-// Add adds an entry for a signature over 'sigHash' under public kye 'pubKey'
+// Add adds an entry for a signature over 'sigHash' under public key 'pubKey'
 // to the signature cache. In the event that the SigCache is 'full', an
 // existing entry it randomly chosen to be evicted in order to make space for
 // the new entry.
@@ -112,7 +111,8 @@ func (s *SigCache) Add(sigHash []byte, sig *btcec.Signature, pubKey *btcec.Publi
 
 	var newSigHash wire.ShaHash
 	copy(newSigHash[:], sigHash)
-	info := sigInfo{newSigHash, *sig, *pubKey}
+	info := sigInfo{newSigHash, string(sig.Serialize()),
+		string(pubKey.SerializeCompressed())}
 	s.validSigs[info] = struct{}{}
 
 	return

txscript/sigcache_test.go

@@ -34,8 +34,8 @@ func genRandomSig() ([]byte, *btcec.Signature, *btcec.PublicKey, error) {
 	return msgHash, sig, privKey.PubKey(), nil
 }
 
-// TestSigCacheAdd tests the ability to add, and later check the existence of a
-// signature triplet in the signature cache.
+// TestSigCacheAddExists tests the ability to add, and later check the
+// existence of a signature triplet in the signature cache.
 func TestSigCacheAddExists(t *testing.T) {
 	sigCache := NewSigCache(200)
 
@@ -49,7 +49,9 @@ func TestSigCacheAddExists(t *testing.T) {
 	sigCache.Add(msg1, sig1, key1)
 
 	// The previously added triplet should now be found within the sigcache.
-	if !sigCache.Exists(msg1, sig1, key1) {
+	sig1Copy, _ := btcec.ParseSignature(sig1.Serialize(), btcec.S256())
+	key1Copy, _ := btcec.ParsePubKey(key1.SerializeCompressed(), btcec.S256())
+	if !sigCache.Exists(msg1, sig1Copy, key1Copy) {
 		t.Errorf("previously added item not found in signature cache")
 	}
 }
@@ -70,7 +72,11 @@ func TestSigCacheAddEvictEntry(t *testing.T) {
 		}
 
 		sigCache.Add(msg, sig, key)
-		if !sigCache.Exists(msg, sig, key) {
+
+		sigCopy, _ := btcec.ParseSignature(sig.Serialize(), btcec.S256())
+		keyCopy, _ := btcec.ParsePubKey(key.SerializeCompressed(), btcec.S256())
+
+		if !sigCache.Exists(msg, sigCopy, keyCopy) {
 			t.Errorf("previously added item not found in signature" +
 				"cache")
 		}
@@ -96,8 +102,11 @@ func TestSigCacheAddEvictEntry(t *testing.T) {
 			sigCacheSize, len(sigCache.validSigs))
 	}
 
+	sigNewCopy, _ := btcec.ParseSignature(sigNew.Serialize(), btcec.S256())
+	keyNewCopy, _ := btcec.ParsePubKey(keyNew.SerializeCompressed(), btcec.S256())
+
 	// The entry added above should be found within the sigcache.
-	if !sigCache.Exists(msgNew, sigNew, keyNew) {
+	if !sigCache.Exists(msgNew, sigNewCopy, keyNewCopy) {
 		t.Fatalf("previously added item not found in signature cache")
 	}
 }
@@ -118,7 +127,9 @@ func TestSigCacheAddMaxEntriesZeroOrNegative(t *testing.T) {
 	sigCache.Add(msg1, sig1, key1)
 
 	// The generated triplet should not be found.
-	if sigCache.Exists(msg1, sig1, key1) {
+	sig1Copy, _ := btcec.ParseSignature(sig1.Serialize(), btcec.S256())
+	key1Copy, _ := btcec.ParsePubKey(key1.SerializeCompressed(), btcec.S256())
+	if sigCache.Exists(msg1, sig1Copy, key1Copy) {
 		t.Errorf("previously added not found in sigcache, but shouldn't" +
 			" have been")
 	}