Merge pull request ethereum#208 from etclabscore/feat/refactor-miner-…

…update

Feat/refactor miner update

miner/miner.go

@@ -80,83 +80,57 @@ func New(eth Backend, config *Config, chainConfig ctypes.ChainConfigurator, mux
 	return miner
 }
 
-// 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.
+// 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.
 func (miner *Miner) update() {
-	downloaderEvents := miner.mux.Subscribe(downloader.StartEvent{}, downloader.DoneEvent{}, downloader.FailedEvent{})
+	events := miner.mux.Subscribe(downloader.StartEvent{}, downloader.DoneEvent{}, downloader.FailedEvent{})
 	defer func() {
-		if !downloaderEvents.Closed() {
-			downloaderEvents.Unsubscribe()
+		if !events.Closed() {
+			events.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()
-		}
-	}
-
-	// 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:
+	dlEventCh := events.Chan()
 	for {
 		select {
-		case ev := <-downloaderEvents.Chan():
+		case ev := <-dlEventCh:
 			if ev == nil {
-				break withDownloaderState
+				// Unsubscription done, stop listening
+				dlEventCh = nil
+				continue
 			}
-			handleDownloaderEvents(ev)
-		case addr := <-miner.startCh:
-			if !isDownloading {
-				miner.SetEtherbase(addr)
-				miner.worker.start()
+			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()
+				}
+				// Stop reacting to downloader events
+				events.Unsubscribe()
 			}
-			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:
+			miner.SetEtherbase(addr)
 			if !isDownloading {
-				miner.SetEtherbase(addr)
 				miner.worker.start()
 			}
 			miningRequested = true

miner/miner_test.go

@@ -192,6 +192,28 @@ func TestCloseMiner(t *testing.T) {
 	waitForMiningState(t, miner, false)
 }
 
+// TestMinerSetEtherbase checks that etherbase becomes set even if mining isn't
+// possible at the moment
+func TestMinerSetEtherbase(t *testing.T) {
+	miner, mux := createMiner(t)
+	// Start with a 'bad' mining address
+	miner.Start(common.HexToAddress("0xdead"))
+	waitForMiningState(t, miner, true)
+	// Start the downloader
+	mux.Post(downloader.StartEvent{})
+	waitForMiningState(t, miner, false)
+	// Now user tries to configure proper mining address
+	miner.Start(common.HexToAddress("0x1337"))
+	// Stop the downloader and wait for the update loop to run
+	mux.Post(downloader.DoneEvent{})
+
+	waitForMiningState(t, miner, true)
+	// The miner should now be using the good address
+	if got, exp := miner.coinbase, common.HexToAddress("0x1337"); got != exp {
+		t.Fatalf("Wrong coinbase, got %x expected %x", got, exp)
+	}
+}
+
 // waitForMiningState waits until either
 // * the desired mining state was reached
 // * a timeout was reached which fails the test