Implement and integrate an ECDSA signature verification cache into btcd #506

Roasbeef · 2015-09-25T00:00:52Z

This PR proposes the addition of an ECDSA signature verification cache
with a randomized entry eviction policy into btcd.

The benefits of SigCache are two fold. Firstly, usage of SigCache mitigates
a DoS attack wherein an attacker causes a victim's client to hang due to worst-case
behavior triggered while processing attacker crafted invalid transactions. A
detailed description of the mitigated DoS attack can be found here.
Secondly, usage of the SigCache introduces a signature verification
optimization which speeds up the validation of transactions within a block,
if they've already once been seen and verified within the mempool.

Implementation wise:

The server itself manages the sigCache instance. The blockManager and
txMempool both respectively now receive pointers to the created sigCache
instance. All read (sig triplet existence) operations on the sigCache
will not block unless a separate goroutine is adding an entry (writing)
to the sigCache. GetBlockTemplate generation now also utilizes the
sigCache in order to avoid unnecessarily double checking signatures
when generating a template after previously accepting a txn to the
mempool. Consequently, the CPU miner now also employs the same
optimization.

The maximum number of entires for the sigCache has been introduced as a
config parameter in order to allow users to configure the amount of
memory consumed by this new additional caching.

wallclockbuilder · 2015-09-25T11:29:07Z

config.go

@@ -116,6 +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:"hello"

--->

SigCacheMaxSize int long:"sigcachemaxsize" description:"Allows users to configure the amount of memory consumed by this new additional caching."

Fixed.

The hello was left over from when I just needed it all to compile :)

wallclockbuilder · 2015-09-25T11:45:10Z

💯

davecgh · 2015-09-27T05:04:19Z

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) error {
+func (state *gbtWorkState) updateBlockTemplate(s *rpcServer, useCoinbaseValue bool, sigCache *txscript.SigCache) error {


Rather than passing the sigCache as a separate parameter, why not just access it via s.server.sigCache?

Hmm I can't recall why I previously passed the sigCache in as a separate parameter.

I've updated the fragment and surrounding code to just use: s.server.sigCache.

davecgh · 2015-09-27T05:27:14Z

The new parameter will need to be added to doc.go and sample-btcd.conf.

davecgh · 2015-09-27T05:36:22Z

I think this should also be applied to opcodeCheckMultiSig where it has:

if parsedSig.Verify(hash, parsedPubKey) {

davecgh · 2015-09-27T05:40:26Z

txscript/reference_test.go

+	sigCacheToggle := []bool{true, false}
+	for _, useSigCache := range sigCacheToggle {
+		for i, test := range tests {
+			var vm *Engine


This should be declared just before the if useSigCache branch below. Also err doesn't need to be declared since it will already exist by virtual of the previous statements doing foo, err := ....

I see you noted fixed, but it's still here. Lost in a rebase?

Hmm seems some line numbers got mixed up. I corrected the issue in TestScriptValidTests, but not TestScriptInvalidTests. Should be all remedied now in the latest snapshot.

davecgh · 2015-09-27T06:09:54Z

Also, looks like a few updates in the tests were missed:

blockchain/common_test.go:119: not enough arguments in call to blockchain.New
blockchain/example_test.go:47: not enough arguments in call to blockchain.New
blockchain/scriptval_test.go:40: not enough arguments in call to blockchain.TstCheckBlockScripts

Roasbeef · 2015-09-30T21:41:02Z

Updated doc.go, sample-btcd.conf, added usage of the sigCache to opcodeCheckMultiSig, and addressed the above code review comments.

davecgh · 2015-09-30T22:26:59Z

doc.go

+      --rpckey=            File containing the certificate key
+      --rpcmaxclients=     Max number of RPC clients for standard connections
+                           (10)
+      --rpcmaxwebsockets=  Max number of RPC clients for standard connections


Look like another one got changed incorrectly. It should be Max number of RPC websocket connections (25).

davecgh · 2015-09-30T23:00:33Z

Speaking of the doc.go changes, I don't see why those lines would even need to be changed. It looks like they've all had a single space removed (so they don't line up with the --nopeerbloomfilters= line). The new option is not wider than the existing ones, so really the only thing should be the new option. I'd suggest checking out the existing doc.go from master and just copying the single new --sigcachemaxsize line into it. That should mean only a single one-line diff.

davecgh · 2015-09-30T23:04:37Z

mining.go

@@ -366,7 +366,7 @@ func medianAdjustedTime(chainState *chainState, timeSource blockchain.MedianTime
 //  |  transactions (while block size   |   |
 //  |  <= cfg.BlockMinSize)             |   |
 //   -----------------------------------  --
-func NewBlockTemplate(mempool *txMemPool, payToAddress btcutil.Address) (*BlockTemplate, error) {
+func NewBlockTemplate(mempool *txMemPool, payToAddress btcutil.Address, sigCache *txscript.SigCache) (*BlockTemplate, error) {


Maybe change this to accept the server instead of the separate mempool and sigCache? I suggest that because I noticed all the callers have s.server.txMemPool and s.server.sigCache for the params. Then the callers could just pass s.server.

I've changed NewBlockTemplate to directly take the server as a parameter as suggested.

davecgh · 2015-10-01T01:48:13Z

txscript/sigcache_test.go

+
+		sigCopy, _ := btcec.ParseSignature(sig.Serialize(), btcec.S256())
+		keyCopy, _ := btcec.ParsePubKey(key.SerializeCompressed(), btcec.S256())
+


Inconsistent blank line.

davecgh · 2015-10-01T01:50:35Z

I'm done with this round of review. It looks like functionally, everything is good to go. I'm going to do some manual testing on it as well.

davecgh · 2015-10-04T06:26:10Z

txscript/sigcache_test.go

+// triplet is added to a full signature cache which should trigger randomized
+// eviction, followed by adding the new element to the cache.
+func TestSigCacheAddEvictEntry(t *testing.T) {
+	// Create a sigcache that can hold up to 10 entries.


Comment is incorrect here.

davecgh · 2015-10-04T21:05:47Z

For reference, I've done a full chain download with checkpoints disabled (to force every script in the chain to be executed), and I've also confirmed that it's using cached signatures (by adding a temporary logging statement) for transactions in the memory pool that are included in new block as expected.

Aside from the incorrect comment, this is ready. Please fix the comment and rebase/squash to the latest master.

Roasbeef · 2015-10-04T22:15:44Z

I've fixed the comment (also clarifying that the cache is only for valid signatures), and rebase/squashed to the latest master 👍

davecgh · 2015-10-04T22:38:42Z

OK

jrick · 2015-10-05T13:19:52Z

txscript/sigcache.go

+// if they've already been seen and verified within the mempool.
+type SigCache struct {
+	sync.RWMutex
+	validSigs  map[sigInfo]struct{}


This might not work as you expect since sigInfo contains string fields (the shallow equality operator might not work if the two strings are different e.g. one is a static string, and the other was created at runtime).

nevermind: http://play.golang.org/p/Phwr-adqy7

davecgh · 2015-10-08T20:18:28Z

Latest changes look good.

OK after rebase/squash.

Introduce an ECDSA signature verification into btcd in order to mitigate a certain DoS attack and as a performance optimization. The benefits of SigCache are two fold. Firstly, usage of SigCache mitigates a DoS attack wherein an attacker causes a victim's client to hang due to worst-case behavior triggered while processing attacker crafted invalid transactions. A detailed description of the mitigated DoS attack can be found here: https://bitslog.wordpress.com/2013/01/23/fixed-bitcoin-vulnerability-explanation-why-the-signature-cache-is-a-dos-protection/ Secondly, usage of the SigCache introduces a signature verification optimization which speeds up the validation of transactions within a block, if they've already been seen and verified within the mempool. The server itself manages the sigCache instance. The blockManager and txMempool respectively now receive pointers to the created sigCache instance. All read (sig triplet existence) operations on the sigCache will not block unless a separate goroutine is adding an entry (writing) to the sigCache. GetBlockTemplate generation now also utilizes the sigCache in order to avoid unnecessarily double checking signatures when generating a template after previously accepting a txn to the mempool. Consequently, the CPU miner now also employs the same optimization. The maximum number of entries for the sigCache has been introduced as a config parameter in order to allow users to configure the amount of memory consumed by this new additional caching.

This caused unhelpful clutter in travis output. Closes btcsuite#506

Roasbeef force-pushed the sigcache branch from 2c1cec7 to f24108f Compare September 25, 2015 00:09

wallclockbuilder reviewed Sep 25, 2015
View reviewed changes

davecgh reviewed Sep 27, 2015
View reviewed changes

Roasbeef force-pushed the sigcache branch from a7c3a8e to 0412c8f Compare September 30, 2015 21:39

davecgh reviewed Sep 30, 2015
View reviewed changes

davecgh reviewed Oct 1, 2015
View reviewed changes

davecgh reviewed Oct 4, 2015
View reviewed changes

Roasbeef force-pushed the sigcache branch 2 times, most recently from ee27e62 to c942901 Compare October 4, 2015 22:10

Roasbeef force-pushed the sigcache branch 2 times, most recently from ea20ec2 to 422da66 Compare October 4, 2015 22:33

jrick reviewed Oct 5, 2015
View reviewed changes

Roasbeef force-pushed the sigcache branch from 471a63f to 0029905 Compare October 9, 2015 00:32

conformal-deploy merged commit 0029905 into btcsuite:master Oct 9, 2015

jcvernaleo added a commit to jcvernaleo/btcd that referenced this pull request Dec 7, 2016

Remove -v from go test.

b89abb7

This caused unhelpful clutter in travis output. Closes btcsuite#506

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement and integrate an ECDSA signature verification cache into btcd #506

Implement and integrate an ECDSA signature verification cache into btcd #506

Roasbeef commented Sep 25, 2015

wallclockbuilder Sep 25, 2015

Roasbeef Sep 30, 2015

wallclockbuilder commented Sep 25, 2015

davecgh Sep 27, 2015

Roasbeef Sep 30, 2015

davecgh commented Sep 27, 2015

davecgh commented Sep 27, 2015

davecgh Sep 27, 2015

Roasbeef Sep 30, 2015

davecgh Oct 1, 2015

Roasbeef Oct 3, 2015

davecgh commented Sep 27, 2015

Roasbeef commented Sep 30, 2015

davecgh Sep 30, 2015

Roasbeef Sep 30, 2015

davecgh commented Sep 30, 2015

davecgh Sep 30, 2015

Roasbeef Oct 3, 2015

davecgh Oct 1, 2015

Roasbeef Oct 3, 2015

davecgh commented Oct 1, 2015

davecgh Oct 4, 2015

davecgh commented Oct 4, 2015

Roasbeef commented Oct 4, 2015

davecgh commented Oct 4, 2015

jrick Oct 5, 2015

jrick Oct 5, 2015

davecgh commented Oct 8, 2015


		sigCopy, _ := btcec.ParseSignature(sig.Serialize(), btcec.S256())
		keyCopy, _ := btcec.ParsePubKey(key.SerializeCompressed(), btcec.S256())

Implement and integrate an ECDSA signature verification cache into btcd #506

Implement and integrate an ECDSA signature verification cache into btcd #506

Conversation

Roasbeef commented Sep 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wallclockbuilder commented Sep 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davecgh commented Sep 27, 2015

davecgh commented Sep 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davecgh commented Sep 27, 2015

Roasbeef commented Sep 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davecgh commented Sep 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davecgh commented Oct 1, 2015

Choose a reason for hiding this comment

davecgh commented Oct 4, 2015

Roasbeef commented Oct 4, 2015

davecgh commented Oct 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davecgh commented Oct 8, 2015