miner: don't interrupt mining after successful sync (#21701)

* miner: exit loop when downloader Done or Failed

Following the logic of the comment at the method,
this fixes a regression introduced at 7cf56d6
, which would allow external parties to DoS with
blocks, preventing mining progress.

Signed-off-by: meows <[email protected]>

* miner: remove ineff assign (lint)

Signed-off-by: meows <[email protected]>

* miner: update test re downloader events

Signed-off-by: meows <[email protected]>

* Revert "miner: remove ineff assign (lint)"

This reverts commit eaefcd3.

* Revert "miner: exit loop when downloader Done or Failed"

This reverts commit 23abd34.

* miner: add test showing imprecise TestMiner

Signed-off-by: meows <[email protected]>

* miner: fix waitForMiningState precision

This helper function would return an affirmation
on the first positive match on a desired bool.

This was imprecise; it return false positives
by not waiting initially for an 'updated' value.

This fix causes TestMiner_2 to fail, which is
expected.

Signed-off-by: meows <[email protected]>

* miner: remove TestMiner_2 demonstrating broken test

This test demonstrated the imprecision of the test
helper function waitForMiningState. This function
has been fixed with 6d365c2, and this test test
may now be removed.

Signed-off-by: meows <[email protected]>

* miner: fix test regarding downloader event/mining expectations

See comment for logic.

Signed-off-by: meows <[email protected]>

* miner: add test describing expectations for downloader/mining events

We expect that once the downloader emits a DoneEvent,
signaling a successful sync, that subsequent StartEvents
are not longer permitted to stop the miner.

This prevents a security vulnerability where forced syncs via
fake high blocks would stall mining operation.

Signed-off-by: meows <[email protected]>

* miner: use 'canStop' state to fix downloader event handling

- Break downloader event handling into event
separating Done and Failed events. We need to
treat these cases differently since a DoneEvent
should prevent the miner from being stopped on
subsequent downloader Start events.

- Use canStop state to handle the one-off
case when a downloader first succeeds.

Signed-off-by: meows <[email protected]>

* miner: improve comment wording

Signed-off-by: meows <[email protected]>

* miner: start mining on downloader events iff not already mining

Signed-off-by: meows <[email protected]>

* miner: refactor miner update logic w/r/t downloader events

This makes mining pause/start logic regarding downloader
events more explicit. Instead of eternally handling downloader
events after the first done event, the subscription is closed
when downloader events are no longer actionable.

Signed-off-by: meows <[email protected]>

* miner: fix handling downloader events on subcription closed

Signed-off-by: meows <[email protected]>

* miner: (lint:gosimple) use range over chan instead of for/select

Signed-off-by: meows <[email protected]>

* miner: refactor update loop to remove race condition

The go routine handling the downloader events handling
vars in parallel with the parent routine, causing a
race condition.

This change, though ugly, remove the condition while
still allowing the downloader event subscription to be
closed when the miner has no further use for it (ie DoneEvent).

* miner: alternate fix for miner-flaw

Co-authored-by: meows <[email protected]>

Author: holiman

miner/miner.go

@@ -85,15 +85,22 @@ func New(eth Backend, config *Config, chainConfig *params.ChainConfig, mux *even
 // and halt your mining operation for as long as the DOS continues.
 func (miner *Miner) update() {
 	events := miner.mux.Subscribe(downloader.StartEvent{}, downloader.DoneEvent{}, downloader.FailedEvent{})
-	defer events.Unsubscribe()
+	defer func() {
+		if !events.Closed() {
+			events.Unsubscribe()
+		}
+	}()
 
 	shouldStart := false
 	canStart := true
+	dlEventCh := events.Chan()
 	for {
 		select {
-		case ev := <-events.Chan():
+		case ev := <-dlEventCh:
 			if ev == nil {
-				return
+				// Unsubscription done, stop listening
+				dlEventCh = nil
+				continue
 			}
 			switch ev.Data.(type) {
 			case downloader.StartEvent:
@@ -105,12 +112,20 @@ func (miner *Miner) update() {
 					shouldStart = true
 					log.Info("Mining aborted due to sync")
 				}
-			case downloader.DoneEvent, downloader.FailedEvent:
+			case downloader.FailedEvent:
+				canStart = true
+				if shouldStart {
+					miner.SetEtherbase(miner.coinbase)
+					miner.worker.start()
+				}
+			case downloader.DoneEvent:
 				canStart = true
 				if shouldStart {
 					miner.SetEtherbase(miner.coinbase)
 					miner.worker.start()
 				}
+				// Stop reacting to downloader events
+				events.Unsubscribe()
 			}
 		case addr := <-miner.startCh:
 			if canStart {

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)
 }