eth/downloader, trie: address review comments

eth/downloader/statesync.go

@@ -61,7 +61,7 @@ type stateSyncStats struct {
 func (d *Downloader) syncState(root common.Hash) *stateSync {
 	// The downloader was requested to start a state sync. If the state sync bloom
 	// filter was not yet initialized, create it now. This lazy creation ensures we
-	// only allocate if if really really really needed.
+	// only allocate if really really really needed.
 	if d.stateBloom == nil {
 		d.stateBloom = trie.NewSyncBloom(d.stateBloomSize, d.stateDatabase)
 	}

trie/sync.go

@@ -101,10 +101,14 @@ func (s *Sync) AddSubTrie(root common.Hash, depth int, parent common.Hash, callb
 	if _, ok := s.membatch.batch[root]; ok {
 		return
 	}
-	key := root.Bytes()
+	if s.bloom.Contains(root[:]) {
-	blob, _ := s.database.Get(key)
+		// Bloom filter says this might be a duplicate, double check
-	if local, err := decodeNode(key, blob); local != nil && err == nil {
+		blob, _ := s.database.Get(root[:])
-		return
+		if local, err := decodeNode(root[:], blob); local != nil && err == nil {
+			return
+		}
+		// False positive, bump fault meter
+		bloomFaultMeter.Mark(1)
 	}
 	// Assemble the new sub-trie sync request
 	req := &request{
@@ -136,8 +140,13 @@ func (s *Sync) AddRawEntry(hash common.Hash, depth int, parent common.Hash) {
 	if _, ok := s.membatch.batch[hash]; ok {
 		return
 	}
-	if ok, _ := s.database.Has(hash.Bytes()); ok {
+	if s.bloom.Contains(hash[:]) {
-		return
+		// Bloom filter says this might be a duplicate, double check
+		if ok, _ := s.database.Has(hash[:]); ok {
+			return
+		}
+		// False positive, bump fault meter
+		bloomFaultMeter.Mark(1)
 	}
 	// Assemble the new sub-trie sync request
 	req := &request{
@@ -300,6 +309,8 @@ func (s *Sync) children(req *request, object node) ([]*request, error) {
 				if ok, _ := s.database.Has(node); ok {
 					continue
 				}
+				// False positive, bump fault meter
+				bloomFaultMeter.Mark(1)
 			}
 			// Locally unknown node, schedule for retrieval
 			requests = append(requests, &request{

trie/sync_bloom.go

@@ -36,6 +36,7 @@ var (
 	bloomLoadMeter  = metrics.NewRegisteredMeter("trie/bloom/load", nil)
 	bloomTestMeter  = metrics.NewRegisteredMeter("trie/bloom/test", nil)
 	bloomMissMeter  = metrics.NewRegisteredMeter("trie/bloom/miss", nil)
+	bloomFaultMeter = metrics.NewRegisteredMeter("trie/bloom/fault", nil)
 	bloomErrorGauge = metrics.NewRegisteredGauge("trie/bloom/error", nil)
 )
 
@@ -94,16 +95,17 @@ func (b *SyncBloom) init(database ethdb.Iteratee) {
 	it := database.NewIterator()
 	defer it.Release()
 
+	start := time.Now()
 	for it.Next() && atomic.LoadUint32(&b.closed) == 0 {
 		if key := it.Key(); len(key) == common.HashLength {
 			b.bloom.Add(syncBloomHasher(key))
 			bloomLoadMeter.Mark(1)
 		}
 	}
-	log.Info("Initialized fast sync bloom", "items", b.bloom.N(), "errorrate", b.errorRate())
+	log.Info("Initialized fast sync bloom", "items", b.bloom.N(), "errorrate", b.errorRate(), "elapsed", time.Since(start))
 
 	// Mark the bloom filter inited and return
-	defer atomic.StoreUint32(&b.inited, 1)
+	atomic.StoreUint32(&b.inited, 1)
 }
 
 // meter periodically recalculates the false positive error rate of the bloom
@@ -118,7 +120,7 @@ func (b *SyncBloom) meter() {
 			if atomic.LoadUint32(&b.closed) == 1 {
 				return
 			}
-			time.Sleep(100 * time.Second)
+			time.Sleep(100 * time.Millisecond)
 		}
 	}
 }
@@ -149,7 +151,7 @@ func (b *SyncBloom) Add(hash []byte) {
 	bloomAddMeter.Mark(1)
 }
 
-// Contains tests if the bloom filter contains  the given hash:
+// Contains tests if the bloom filter contains the given hash:
 //   - false: the bloom definitely does not contain hash
 //   - true:  the bloom maybe contains hash
 //
@@ -174,7 +176,7 @@ func (b *SyncBloom) Contains(hash []byte) bool {
 // false positive.
 //
 // We're calculating it ourselves because the bloom library we used missed a
-// paranthesis in the formula and calculates it wrong. And it's discontinued...
+// parentheses in the formula and calculates it wrong. And it's discontinued...
 func (b *SyncBloom) errorRate() float64 {
 	k := float64(b.bloom.K())
 	n := float64(b.bloom.N())