From e14249f66ad4a07edef334e34b8c83716e081532 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A9ter=20Szil=C3=A1gyi?= Date: Tue, 23 Apr 2019 14:42:40 +0300 Subject: [PATCH] eth/downloader, trie: address review comments --- eth/downloader/statesync.go | 2 +- trie/sync.go | 23 +++++++++++++++++------ trie/sync_bloom.go | 12 +++++++----- 3 files changed, 25 insertions(+), 12 deletions(-) diff --git a/eth/downloader/statesync.go b/eth/downloader/statesync.go index da73daa764..e1cf6efc56 100644 --- a/eth/downloader/statesync.go +++ b/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) } diff --git a/trie/sync.go b/trie/sync.go index bbdea5cd55..d9564d7831 100644 --- a/trie/sync.go +++ b/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() - blob, _ := s.database.Get(key) - if local, err := decodeNode(key, blob); local != nil && err == nil { - return + if s.bloom.Contains(root[:]) { + // Bloom filter says this might be a duplicate, double check + blob, _ := s.database.Get(root[:]) + 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 { - return + if s.bloom.Contains(hash[:]) { + // 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{ diff --git a/trie/sync_bloom.go b/trie/sync_bloom.go index b6f26d13af..5bf193818a 100644 --- a/trie/sync_bloom.go +++ b/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())