miner: exit loop when downloader Done or Failed

miner/miner.go

@@ -80,47 +80,66 @@ func New(eth Backend, config *Config, chainConfig ctypes.ChainConfigurator, mux
 	return miner
 }
 
-// update keeps track of the downloader events. Please be aware that this is a one shot type of update loop.
-// It's entered once and as soon as `Done` or `Failed` has been broadcasted the events are unregistered and
-// the loop is exited. This to prevent a major security vuln where external parties can DOS you with blocks
-// and halt your mining operation for as long as the DOS continues.
+// update handles miner start/stop/exit events, as well as a temporary subscription to downloader events.
+// Downloader events are used to pause and restart mining pending a successful sync operation.
 func (miner *Miner) update() {
-	events := miner.mux.Subscribe(downloader.StartEvent{}, downloader.DoneEvent{}, downloader.FailedEvent{})
-	defer events.Unsubscribe()
+	downloaderEvents := miner.mux.Subscribe(downloader.StartEvent{}, downloader.DoneEvent{}, downloader.FailedEvent{})
+	defer func() {
+		if !downloaderEvents.Closed() {
+			downloaderEvents.Unsubscribe()
+		}
+	}()
 
-	shouldStart := false
+	miningRequested := false
 	canStart := true
+
+	// handleDownloaderEvents handles the downloader events. Please be aware that this is a one-shot type of logic.
+	// As soon as `Done` has been broadcast subsequent downloader events are not handled, and the event subscription is closed.
+	//
+	// This prevents a major security vulnerability where external parties can DOS you with blocks
+	// and halt your mining operation for as long as the DOS continues.
+	handleDownloaderEvents := func(ev *event.TypeMuxEvent) {
+		switch ev.Data.(type) {
+		case downloader.StartEvent:
+			wasMining := miner.Mining()
+			miner.worker.stop()
+			canStart = false
+			if wasMining {
+				// Resume mining after sync was finished
+				miningRequested = true
+				log.Info("Mining aborted due to sync")
+			}
+		case downloader.FailedEvent:
+			canStart = true
+			if miningRequested {
+				miner.SetEtherbase(miner.coinbase)
+				miner.worker.start()
+			}
+		case downloader.DoneEvent:
+			canStart = true
+			if miningRequested {
+				miner.SetEtherbase(miner.coinbase)
+				miner.worker.start()
+			}
+			downloaderEvents.Unsubscribe()
+		}
+	}
+
 	for {
 		select {
-		case ev := <-events.Chan():
+		case ev := <-downloaderEvents.Chan():
 			if ev == nil {
 				return
 			}
-			switch ev.Data.(type) {
-			case downloader.StartEvent:
-				wasMining := miner.Mining()
-				miner.worker.stop()
-				canStart = false
-				if wasMining {
-					// Resume mining after sync was finished
-					shouldStart = true
-					log.Info("Mining aborted due to sync")
-				}
-			case downloader.DoneEvent, downloader.FailedEvent:
-				canStart = true
-				if shouldStart {
-					miner.SetEtherbase(miner.coinbase)
-					miner.worker.start()
-				}
-			}
+			handleDownloaderEvents(ev)
 		case addr := <-miner.startCh:
 			if canStart {
 				miner.SetEtherbase(addr)
 				miner.worker.start()
 			}
-			shouldStart = true
+			miningRequested = true
 		case <-miner.stopCh:
-			shouldStart = false
+			miningRequested = false
 			miner.worker.stop()
 		case <-miner.exitCh:
 			miner.worker.close()

miner/miner_test.go

@@ -89,12 +89,53 @@ func TestMiner(t *testing.T) {
 	// Stop the downloader and wait for the update loop to run
 	mux.Post(downloader.DoneEvent{})
 	waitForMiningState(t, miner, true)
-	// Start the downloader and wait for the update loop to run
+
+	// Subsequent downloader events after a successful DoneEvent should not cause the
+	// miner to start or stop. This prevents a security vulnerability
+	// that would allow entities to present fake high blocks that would
+	// stop mining operations by causing a downloader sync
+	// until it was discovered they were invalid, whereon mining would resume.
+	mux.Post(downloader.StartEvent{})
+	waitForMiningState(t, miner, true)
+
+	mux.Post(downloader.FailedEvent{})
+	waitForMiningState(t, miner, true)
+}
+
+// TestMinerDownloaderFirstFails tests that mining is only
+// permitted to run indefinitely once the downloader sees a DoneEvent (success).
+// An initial FailedEvent should allow mining to stop on a subsequent
+// downloader StartEvent.
+func TestMinerDownloaderFirstFails(t *testing.T) {
+	miner, mux := createMiner(t)
+	miner.Start(common.HexToAddress("0x12345"))
+	waitForMiningState(t, miner, true)
+	// Start the downloader
 	mux.Post(downloader.StartEvent{})
 	waitForMiningState(t, miner, false)
+
 	// Stop the downloader and wait for the update loop to run
 	mux.Post(downloader.FailedEvent{})
 	waitForMiningState(t, miner, true)
+
+	// Since the downloader hasn't yet emitted a successful DoneEvent,
+	// we expect the miner to stop on next StartEvent.
+	mux.Post(downloader.StartEvent{})
+	waitForMiningState(t, miner, false)
+
+	// Downloader finally succeeds.
+	mux.Post(downloader.DoneEvent{})
+	waitForMiningState(t, miner, true)
+
+	// Downloader starts again.
+	// Since it has achieved a DoneEvent once, we expect miner
+	// state to be unchanged.
+	mux.Post(downloader.StartEvent{})
+	waitForMiningState(t, miner, true)
+
+	mux.Post(downloader.FailedEvent{})
+	waitForMiningState(t, miner, true)
+
 }
 
 func TestStartWhileDownload(t *testing.T) {
@@ -137,10 +178,10 @@ func waitForMiningState(t *testing.T, m *Miner, mining bool) {
 
 	var state bool
 	for i := 0; i < 100; i++ {
+		time.Sleep(10 * time.Millisecond)
 		if state = m.Mining(); state == mining {
 			return
 		}
-		time.Sleep(10 * time.Millisecond)
 	}
 	t.Fatalf("Mining() == %t, want %t", state, mining)
 }