eth/fetcher, trie: unit test reliability fixes #23020 #23415 (#1070)

Some tests take quite some time during exit, which I think causes
some appveyor fails like this:

    https://ci.appveyor.com/project/ethereum/go-ethereum/builds/39511210/job/xhom84eg2e4uulq3

One of the things that seem to take time during exit is waiting
(up to 100ms) for the syncbloom to close. This PR changes it to use
a channel, instead of looping with a 100ms wait.

This also includes some unrelated changes improving the reliability of
eth/fetcher tests, which fail a lot because they are time-dependent.

Co-authored-by: Martin Holst Swende <martin@swende.se>

eth/fetcher/fetcher.go

@@ -753,9 +753,10 @@ func (f *Fetcher) insert(peer string, block *types.Block) {
 // internal state.
 func (f *Fetcher) forgetHash(hash common.Hash) {
 	// Remove all pending announces and decrement DOS counters
-	for _, announce := range f.announced[hash] {
+	if announceMap, ok := f.announced[hash]; ok {
+		for _, announce := range announceMap {
 			f.announces[announce.origin]--
-		if f.announces[announce.origin] == 0 {
+			if f.announces[announce.origin] <= 0 {
 				delete(f.announces, announce.origin)
 			}
 		}
@@ -763,6 +764,7 @@ func (f *Fetcher) forgetHash(hash common.Hash) {
 		if f.announceChangeHook != nil {
 			f.announceChangeHook(hash, false)
 		}
+	}
 	// Remove any pending fetches and decrement the DOS counters
 	if announce := f.fetching[hash]; announce != nil {
 		f.announces[announce.origin]--

eth/fetcher/fetcher_test.go

@@ -677,15 +677,30 @@ func testInvalidNumberAnnouncement(t *testing.T, protocol int) {
 	badBodyFetcher := tester.makeBodyFetcher("bad", blocks, 0)
 
 	imported := make(chan *types.Block)
+	announced := make(chan interface{})
 	tester.fetcher.signHook = func(block *types.Block) error {
 		imported <- block
 		return nil
 	}
 
 	// Announce a block with a bad number, check for immediate drop
+	tester.fetcher.announceChangeHook = func(hash common.Hash, b bool) {
+		announced <- nil
+	}
 	tester.fetcher.Notify("bad", hashes[0], 2, time.Now().Add(-arriveTimeout), badHeaderFetcher, badBodyFetcher)
+	verifyAnnounce := func() {
+		for i := 0; i < 2; i++ {
+			select {
+			case <-announced:
+				continue
+			case <-time.After(1 * time.Second):
+				t.Fatal("announce timeout")
+				return
+			}
+		}
+	}
+	verifyAnnounce()
 	verifyImportEvent(t, imported, false)
-
 	tester.lock.RLock()
 	dropped := tester.drops["bad"]
 	tester.lock.RUnlock()
@@ -698,6 +713,7 @@ func testInvalidNumberAnnouncement(t *testing.T, protocol int) {
 	goodBodyFetcher := tester.makeBodyFetcher("good", blocks, 0)
 	// Make sure a good announcement passes without a drop
 	tester.fetcher.Notify("good", hashes[0], 1, time.Now().Add(-arriveTimeout), goodHeaderFetcher, goodBodyFetcher)
+	verifyAnnounce()
 	verifyImportEvent(t, imported, true)
 
 	tester.lock.RLock()

trie/sync_bloom.go

@@ -50,6 +50,7 @@ type SyncBloom struct {
 	closer  sync.Once
 	closed  uint32
 	pend    sync.WaitGroup
+	closeCh chan struct{}
 }
 
 // NewSyncBloom creates a new bloom filter of the given size (in megabytes) and
@@ -65,6 +66,7 @@ func NewSyncBloom(memory uint64, database ethdb.Iteratee) *SyncBloom {
 	// Assemble the fast sync bloom and init it from previous sessions
 	b := &SyncBloom{
 		bloom:   bloom,
+		closeCh: make(chan struct{}),
 	}
 	b.pend.Add(2)
 	go func() {
@@ -125,17 +127,18 @@ func (b *SyncBloom) init(database ethdb.Iteratee) {
 // meter periodically recalculates the false positive error rate of the bloom
 // filter and reports it in a metric.
 func (b *SyncBloom) meter() {
+	// check every second
+	tick := time.NewTicker(1 * time.Second)
+	defer tick.Stop()
+
 	for {
+		select {
+		case <-tick.C:
 			// Report the current error ration. No floats, lame, scale it up.
 			bloomErrorGauge.Update(int64(b.bloom.FalsePosititveProbability() * 100000))
-
+		case <-b.closeCh:
-		// Wait one second, but check termination more frequently
-		for i := 0; i < 10; i++ {
-			if atomic.LoadUint32(&b.closed) == 1 {
 			return
 		}
-			time.Sleep(100 * time.Millisecond)
-		}
 	}
 }
 
@@ -145,6 +148,7 @@ func (b *SyncBloom) Close() error {
 	b.closer.Do(func() {
 		// Ensure the initializer is stopped
 		atomic.StoreUint32(&b.closed, 1)
+		close(b.closeCh)
 		b.pend.Wait()
 
 		// Wipe the bloom, but mark it "uninited" just in case someone attempts an access