From 3b39edcaa1e867efc4223d95ca1496aaadf8eca3 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 13 Apr 2016 19:56:10 -0700 Subject: [PATCH] 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 | 8 ++++ btcec/pubkey_test.go | 36 ++++++++++++++++++ btcec/signature.go | 8 ++++ btcec/signature_test.go | 21 +++++++++++ config.go | 2 +- txscript/reference_test.go | 3 ++ txscript/sigcache.go | 77 ++++++++++++++++---------------------- 7 files changed, 110 insertions(+), 45 deletions(-) diff --git a/btcec/pubkey.go b/btcec/pubkey.go index e05092d1f7..be6f850978 100644 --- a/btcec/pubkey.go +++ b/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. diff --git a/btcec/pubkey_test.go b/btcec/pubkey_test.go index a499655d53..16c6f25720 100644 --- a/btcec/pubkey_test.go +++ b/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) + } +} diff --git a/btcec/signature.go b/btcec/signature.go index 126f8944ba..ab5a345c4c 100644 --- a/btcec/signature.go +++ b/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. diff --git a/btcec/signature_test.go b/btcec/signature_test.go index 739a0fba1e..c13bbe0f9b 100644 --- a/btcec/signature_test.go +++ b/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) + } +} diff --git a/config.go b/config.go index 3c26bc140d..0e00caa0de 100644 --- a/config.go +++ b/config.go @@ -46,7 +46,7 @@ const ( defaultGenerate = false defaultMaxOrphanTransactions = 1000 defaultMaxOrphanTxSize = 5000 - defaultSigCacheMaxSize = 50000 + defaultSigCacheMaxSize = 100000 defaultTxIndex = false defaultAddrIndex = false ) diff --git a/txscript/reference_test.go b/txscript/reference_test.go index e180993cfb..9abab7d0f7 100644 --- a/txscript/reference_test.go +++ b/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 { diff --git a/txscript/sigcache.go b/txscript/sigcache.go index a49a1b792a..028a267cc1 100644 --- a/txscript/sigcache.go +++ b/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} }