server: Optimize map limiting in block manager. (#658)

This optimizes the way in which the maps are limited by the block
manager.

Previously the code would read a cryptographically random value large
enough to construct a hash, find the first entry larger than that value,
and evict it.

That approach is quite inefficient and could easily become a bottleneck
when processing transactions due to the need to read from a source such
as /dev/urandom and all of the subsequent hash comparisons.

Luckily, strong cryptographic randomness is not needed here. The primary
intent of limiting the maps is to control memory usage with a secondary
concern of making it difficult for adversaries to force eviction of
specific entries.

Consequently, this changes the code to make use of the pseudorandom
iteration order of Go's maps along with the preimage resistance of the
hashing function to provide the desired functionality.  It has
previously been discussed that the specific pseudorandom iteration order
is not guaranteed by the Go spec even though in practice that is how it
is implemented.  This is not a concern however because even if the
specific compiler doesn't implement that, the preimage resistance of the
hashing function alone is enough.

Thanks to @Roasbeef for pointing out the efficiency concerns and the
fact that strong cryptographic randomness is not necessary.

blockmanager.go

@@ -6,8 +6,6 @@ package main
 
 import (
 	"container/list"
-	"crypto/rand"
-	"math/big"
 	"net"
 	"os"
 	"path/filepath"
@@ -519,12 +517,7 @@ func (b *blockManager) handleTxMsg(tmsg *txMsg) {
 		// Do not request this transaction again until a new block
 		// has been processed.
 		b.rejectedTxns[*txHash] = struct{}{}
-		lerr := b.limitMap(b.rejectedTxns, maxRejectedTxns)
+		b.limitMap(b.rejectedTxns, maxRejectedTxns)
-		if lerr != nil {
-			bmgrLog.Warnf("Failed to limit the number of "+
-				"rejected transactions: %v", lerr)
-			delete(b.rejectedTxns, *txHash)
-		}
 
 		// When the error is a rule error, it means the transaction was
 		// simply rejected as opposed to something actually going wrong,
@@ -1096,16 +1089,7 @@ func (b *blockManager) handleInvMsg(imsg *invMsg) {
 			// request.
 			if _, exists := b.requestedBlocks[iv.Hash]; !exists {
 				b.requestedBlocks[iv.Hash] = struct{}{}
-				err := b.limitMap(b.requestedBlocks,
+				b.limitMap(b.requestedBlocks, maxRequestedBlocks)
-					maxRequestedBlocks)
-				if err != nil {
-					bmgrLog.Warnf("Failed to limit the "+
-						"number of requested "+
-						"blocks: %v", err)
-					delete(b.requestedBlocks, iv.Hash)
-					continue
-				}
-
 				imsg.peer.requestedBlocks[iv.Hash] = struct{}{}
 				gdmsg.AddInvVect(iv)
 				numRequested++
@@ -1116,15 +1100,7 @@ func (b *blockManager) handleInvMsg(imsg *invMsg) {
 			// pending request.
 			if _, exists := b.requestedTxns[iv.Hash]; !exists {
 				b.requestedTxns[iv.Hash] = struct{}{}
-				err := b.limitMap(b.requestedTxns,
+				b.limitMap(b.requestedTxns, maxRequestedTxns)
-					maxRequestedTxns)
-				if err != nil {
-					bmgrLog.Warnf("Failed to limit the "+
-						"number of requested "+
-						"transactions: %v", err)
-					delete(b.requestedTxns, iv.Hash)
-					continue
-				}
 				imsg.peer.requestedTxns[iv.Hash] = struct{}{}
 				gdmsg.AddInvVect(iv)
 				numRequested++
@@ -1142,37 +1118,21 @@ func (b *blockManager) handleInvMsg(imsg *invMsg) {
 }
 
 // limitMap is a helper function for maps that require a maximum limit by
-// evicting a random rejected transaction if adding a new value would cause it
+// evicting a random transaction if adding a new value would cause it to
-// to overflow the maximum allowed.
+// overflow the maximum allowed.
-func (b *blockManager) limitMap(m map[wire.ShaHash]struct{}, limit int) error {
+func (b *blockManager) limitMap(m map[wire.ShaHash]struct{}, limit int) {
 	if len(m)+1 > limit {
-		// Generate a cryptographically random hash.
+		// Remove a random entry from the map.  For most compilers, Go's
-		randHashBytes := make([]byte, wire.HashSize)
+		// range statement iterates starting at a random item although
-		_, err := rand.Read(randHashBytes)
+		// that is not 100% guaranteed by the spec.  The iteration order
-		if err != nil {
+		// is not important here because an adversary would have to be
-			return err
+		// able to pull off preimage attacks on the hashing function in
-		}
+		// order to target eviction of specific entries anyways.
-		randHashNum := new(big.Int).SetBytes(randHashBytes)
-
-		// Try to find the first entry that is greater than the random
-		// hash.  Use the first entry (which is already pseudorandom due
-		// to Go's range statement over maps) as a fallback if none of
-		// the hashes in the map are larger than the random hash.
-		var foundHash *wire.ShaHash
 		for txHash := range m {
-			if foundHash == nil {
+			delete(m, txHash)
-				foundHash = &txHash
+			return
-			}
-			txHashNum := blockchain.ShaHashToBig(&txHash)
-			if txHashNum.Cmp(randHashNum) > 0 {
-				foundHash = &txHash
-				break
 		}
 	}
-		delete(m, *foundHash)
-	}
-
-	return nil
 }
 
 // blockHandler is the main handler for the block manager.  It must be run