Merge pull request #200 from etclabscore/fix/miner-dl-stop-break

miner: exit loop when downloader Done or Failed

This fixes an issue where downloader syncronizations would temporarily halt mining operation.
The issue was inherited from upstream ethereum/go-ethereum via a refactoring of the miner.update loop.

miner/miner.go

@@ -80,47 +80,88 @@ 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()
+		}
+	}()
+
+	miningRequested := false
+	isDownloading := false
+
+	// 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()
+			isDownloading = true
+			if wasMining {
+				// Resume mining after sync was finished
+				miningRequested = true
+				log.Info("Mining aborted due to sync")
+			}
+		case downloader.FailedEvent:
+			isDownloading = false
+			if miningRequested {
+				miner.SetEtherbase(miner.coinbase)
+				miner.worker.start()
+			}
+		case downloader.DoneEvent:
+			isDownloading = false
+			if miningRequested {
+				miner.SetEtherbase(miner.coinbase)
+				miner.worker.start()
+			}
+			downloaderEvents.Unsubscribe()
+		}
+	}
 
-	shouldStart := false
-	canStart := true
+	// withDownloaderState loop handles channeled events while the miner cares about the downloader events.
+	// This loop will be broken once the downloader emits a DoneEvent, signaling a successful sync.
+	// The following unnamed loop should be equivalent to this loop, but without the downloader event handling.
+withDownloaderState:
 	for {
 		select {
-		case ev := <-events.Chan():
+		case ev := <-downloaderEvents.Chan():
 			if ev == nil {
-				return
+				break withDownloaderState
 			}
-			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 !isDownloading {
+				miner.SetEtherbase(addr)
+				miner.worker.start()
 			}
+			miningRequested = true
+		case <-miner.stopCh:
+			miningRequested = false
+			miner.worker.stop()
+		case <-miner.exitCh:
+			miner.worker.close()
+			return
+		}
+	}
+
+	// Same loop as above, minus downloader event handling.
+	for {
+		select {
 		case addr := <-miner.startCh:
-			if canStart {
+			if !isDownloading {
 				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,75 @@ 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 TestMinerStartStopAfterDownloaderEvents(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)
+
+	// Downloader finally succeeds.
+	mux.Post(downloader.DoneEvent{})
+	waitForMiningState(t, miner, true)
+
+	miner.Stop()
+	waitForMiningState(t, miner, false)
+
+	miner.Start(common.HexToAddress("0x678910"))
+	waitForMiningState(t, miner, true)
+
+	miner.Stop()
+	waitForMiningState(t, miner, false)
 }
 
 func TestStartWhileDownload(t *testing.T) {
@@ -137,10 +200,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)
 }