From e299fa75ad53592c36305834ad928d84fa709074 Mon Sep 17 00:00:00 2001 From: meows Date: Fri, 2 Oct 2020 16:14:57 -0500 Subject: [PATCH 01/18] miner: exit loop when downloader Done or Failed Following the logic of the comment at the method, this fixes a regression introduced at 7cf56d6f064869cb62b1673f9ee437020c595391 , which would allow external parties to DoS with blocks, preventing mining progress. Signed-off-by: meows --- miner/miner.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/miner/miner.go b/miner/miner.go index 8cbd70b424b5..4f8f76d4f941 100644 --- a/miner/miner.go +++ b/miner/miner.go @@ -111,6 +111,8 @@ func (miner *Miner) update() { miner.SetEtherbase(miner.coinbase) miner.worker.start() } + // stop immediately and ignore all further pending events + return } case addr := <-miner.startCh: if canStart { From 2d7fd86a1e4f1327b3e3e23008462a177e6e1127 Mon Sep 17 00:00:00 2001 From: meows Date: Fri, 2 Oct 2020 16:28:20 -0500 Subject: [PATCH 02/18] miner: remove ineff assign (lint) Signed-off-by: meows --- miner/miner.go | 1 - 1 file changed, 1 deletion(-) diff --git a/miner/miner.go b/miner/miner.go index 4f8f76d4f941..1deb8641afe8 100644 --- a/miner/miner.go +++ b/miner/miner.go @@ -106,7 +106,6 @@ func (miner *Miner) update() { log.Info("Mining aborted due to sync") } case downloader.DoneEvent, downloader.FailedEvent: - canStart = true if shouldStart { miner.SetEtherbase(miner.coinbase) miner.worker.start() From 6addca758632cff7b21df04f753ddaff0f6d2b85 Mon Sep 17 00:00:00 2001 From: meows Date: Sat, 3 Oct 2020 08:29:04 -0500 Subject: [PATCH 03/18] miner: update test re downloader events Signed-off-by: meows --- miner/miner_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/miner/miner_test.go b/miner/miner_test.go index 2ed03a2397a9..5b5f0b881c43 100644 --- a/miner/miner_test.go +++ b/miner/miner_test.go @@ -89,10 +89,10 @@ 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 + // Start the downloader, the mining state will not change because the update loop has exited mux.Post(downloader.StartEvent{}) - waitForMiningState(t, miner, false) - // Stop the downloader and wait for the update loop to run + waitForMiningState(t, miner, true) + // Stop the downloader, the mining state will not change mux.Post(downloader.FailedEvent{}) waitForMiningState(t, miner, true) } From 8100b35d9b5b1961c17714da78ca495f1e42f535 Mon Sep 17 00:00:00 2001 From: meows Date: Mon, 5 Oct 2020 08:22:39 -0500 Subject: [PATCH 04/18] Revert "miner: remove ineff assign (lint)" This reverts commit eaefcd34ab4862ebc936fb8a07578aa2744bc058. --- miner/miner.go | 1 + 1 file changed, 1 insertion(+) diff --git a/miner/miner.go b/miner/miner.go index 1deb8641afe8..4f8f76d4f941 100644 --- a/miner/miner.go +++ b/miner/miner.go @@ -106,6 +106,7 @@ func (miner *Miner) update() { log.Info("Mining aborted due to sync") } case downloader.DoneEvent, downloader.FailedEvent: + canStart = true if shouldStart { miner.SetEtherbase(miner.coinbase) miner.worker.start() From 5ccaeb7709ac183199ac76ddc19ca445cb33814d Mon Sep 17 00:00:00 2001 From: meows Date: Mon, 5 Oct 2020 08:22:54 -0500 Subject: [PATCH 05/18] Revert "miner: exit loop when downloader Done or Failed" This reverts commit 23abd34265aa246c38fc390bb72572ad6ae9fe3b. --- miner/miner.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/miner/miner.go b/miner/miner.go index 4f8f76d4f941..8cbd70b424b5 100644 --- a/miner/miner.go +++ b/miner/miner.go @@ -111,8 +111,6 @@ func (miner *Miner) update() { miner.SetEtherbase(miner.coinbase) miner.worker.start() } - // stop immediately and ignore all further pending events - return } case addr := <-miner.startCh: if canStart { From 3b783dd1ed668e1eff7b19ced86b46accf12a73f Mon Sep 17 00:00:00 2001 From: meows Date: Mon, 5 Oct 2020 08:30:43 -0500 Subject: [PATCH 06/18] miner: add test showing imprecise TestMiner Signed-off-by: meows --- miner/miner_test.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/miner/miner_test.go b/miner/miner_test.go index 5b5f0b881c43..61ebc4c705e0 100644 --- a/miner/miner_test.go +++ b/miner/miner_test.go @@ -97,6 +97,27 @@ func TestMiner(t *testing.T) { waitForMiningState(t, miner, true) } +// TestMiner_2 shows that TestMiner assertions 'waitForMiningState' are +// imprecise measurements. The last two assertions use 'false' where the original +// test uses 'true'. The test passes. +func TestMiner_2(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.DoneEvent{}) + waitForMiningState(t, miner, true) + // Start the downloader, the mining state will not change because the update loop has exited + mux.Post(downloader.StartEvent{}) + waitForMiningState(t, miner, false) + // Stop the downloader, the mining state will not change + mux.Post(downloader.FailedEvent{}) + waitForMiningState(t, miner, false) +} + func TestStartWhileDownload(t *testing.T) { miner, mux := createMiner(t) waitForMiningState(t, miner, false) From 6d365c28514c983be4239d74128ceff2dcd8a6cb Mon Sep 17 00:00:00 2001 From: meows Date: Mon, 5 Oct 2020 08:44:00 -0500 Subject: [PATCH 07/18] 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 --- miner/miner_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/miner/miner_test.go b/miner/miner_test.go index 61ebc4c705e0..fa5b8c4768f2 100644 --- a/miner/miner_test.go +++ b/miner/miner_test.go @@ -89,10 +89,10 @@ 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, the mining state will not change because the update loop has exited + mux.Post(downloader.StartEvent{}) - waitForMiningState(t, miner, true) - // Stop the downloader, the mining state will not change + waitForMiningState(t, miner, false) + mux.Post(downloader.FailedEvent{}) waitForMiningState(t, miner, true) } @@ -158,10 +158,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) } From 2d872951159995592f9c8dfd1d908e7c6dcee8c5 Mon Sep 17 00:00:00 2001 From: meows Date: Mon, 5 Oct 2020 08:45:26 -0500 Subject: [PATCH 08/18] miner: remove TestMiner_2 demonstrating broken test This test demonstrated the imprecision of the test helper function waitForMiningState. This function has been fixed with 6d365c2851, and this test test may now be removed. Signed-off-by: meows --- miner/miner_test.go | 21 --------------------- 1 file changed, 21 deletions(-) diff --git a/miner/miner_test.go b/miner/miner_test.go index fa5b8c4768f2..81f6e00ffe11 100644 --- a/miner/miner_test.go +++ b/miner/miner_test.go @@ -97,27 +97,6 @@ func TestMiner(t *testing.T) { waitForMiningState(t, miner, true) } -// TestMiner_2 shows that TestMiner assertions 'waitForMiningState' are -// imprecise measurements. The last two assertions use 'false' where the original -// test uses 'true'. The test passes. -func TestMiner_2(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.DoneEvent{}) - waitForMiningState(t, miner, true) - // Start the downloader, the mining state will not change because the update loop has exited - mux.Post(downloader.StartEvent{}) - waitForMiningState(t, miner, false) - // Stop the downloader, the mining state will not change - mux.Post(downloader.FailedEvent{}) - waitForMiningState(t, miner, false) -} - func TestStartWhileDownload(t *testing.T) { miner, mux := createMiner(t) waitForMiningState(t, miner, false) From 91dcfb5f08ec55f62ac50aceaf45559792fa952c Mon Sep 17 00:00:00 2001 From: meows Date: Mon, 5 Oct 2020 08:54:56 -0500 Subject: [PATCH 09/18] miner: fix test regarding downloader event/mining expectations See comment for logic. Signed-off-by: meows --- miner/miner_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/miner/miner_test.go b/miner/miner_test.go index 81f6e00ffe11..185ade29936e 100644 --- a/miner/miner_test.go +++ b/miner/miner_test.go @@ -90,8 +90,13 @@ func TestMiner(t *testing.T) { mux.Post(downloader.DoneEvent{}) waitForMiningState(t, miner, true) + // Subsequent downloader events 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, false) + waitForMiningState(t, miner, true) mux.Post(downloader.FailedEvent{}) waitForMiningState(t, miner, true) From e136e5fc0a46761ca98715bb8b42b58d7f3cacc3 Mon Sep 17 00:00:00 2001 From: meows Date: Mon, 5 Oct 2020 09:07:39 -0500 Subject: [PATCH 10/18] 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 --- miner/miner_test.go | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/miner/miner_test.go b/miner/miner_test.go index 185ade29936e..2e6286cc8b5e 100644 --- a/miner/miner_test.go +++ b/miner/miner_test.go @@ -90,7 +90,7 @@ func TestMiner(t *testing.T) { mux.Post(downloader.DoneEvent{}) waitForMiningState(t, miner, true) - // Subsequent downloader events should not cause the + // 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 @@ -102,6 +102,42 @@ func TestMiner(t *testing.T) { waitForMiningState(t, miner, true) } +// TestMinerDownloaderFirstFails tests that mining is only +// permitted to run indefinitely once the downloader sees a DoneEvent (success). +// With this, a FailEvent should not prohibit mining stopping 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) { miner, mux := createMiner(t) waitForMiningState(t, miner, false) From c76054d058f16b8d774d5854dc537d3f10570408 Mon Sep 17 00:00:00 2001 From: meows Date: Mon, 5 Oct 2020 09:10:30 -0500 Subject: [PATCH 11/18] 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 --- miner/miner.go | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/miner/miner.go b/miner/miner.go index 8cbd70b424b5..9db5e8b09cdc 100644 --- a/miner/miner.go +++ b/miner/miner.go @@ -87,7 +87,8 @@ func (miner *Miner) update() { events := miner.mux.Subscribe(downloader.StartEvent{}, downloader.DoneEvent{}, downloader.FailedEvent{}) defer events.Unsubscribe() - shouldStart := false + downloaderCanStop := true + miningRequested := false canStart := true for { select { @@ -97,17 +98,26 @@ func (miner *Miner) update() { } 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") + if downloaderCanStop { + 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.DoneEvent, downloader.FailedEvent: + case downloader.FailedEvent: canStart = true - if shouldStart { + if miningRequested { + miner.SetEtherbase(miner.coinbase) + miner.worker.start() + } + case downloader.DoneEvent: + canStart = true + downloaderCanStop = false + if miningRequested { miner.SetEtherbase(miner.coinbase) miner.worker.start() } @@ -117,9 +127,9 @@ func (miner *Miner) update() { 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() From a8c825f28237140d101acfb098162dcf8996e4d6 Mon Sep 17 00:00:00 2001 From: meows Date: Mon, 5 Oct 2020 09:15:29 -0500 Subject: [PATCH 12/18] miner: improve comment wording Signed-off-by: meows --- miner/miner_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/miner/miner_test.go b/miner/miner_test.go index 2e6286cc8b5e..148f14933628 100644 --- a/miner/miner_test.go +++ b/miner/miner_test.go @@ -104,7 +104,7 @@ func TestMiner(t *testing.T) { // TestMinerDownloaderFirstFails tests that mining is only // permitted to run indefinitely once the downloader sees a DoneEvent (success). -// With this, a FailEvent should not prohibit mining stopping on a subsequent +// An initial FailedEvent should allow mining to stop on a subsequent // downloader StartEvent. func TestMinerDownloaderFirstFails(t *testing.T) { miner, mux := createMiner(t) From bfe27281531049af12ed7f79bfbecbfd3f04d974 Mon Sep 17 00:00:00 2001 From: meows Date: Mon, 5 Oct 2020 09:28:18 -0500 Subject: [PATCH 13/18] miner: start mining on downloader events iff not already mining Signed-off-by: meows --- miner/miner.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/miner/miner.go b/miner/miner.go index 9db5e8b09cdc..df06a7a1a1bf 100644 --- a/miner/miner.go +++ b/miner/miner.go @@ -110,14 +110,14 @@ func (miner *Miner) update() { } case downloader.FailedEvent: canStart = true - if miningRequested { + if miningRequested && !miner.Mining() { miner.SetEtherbase(miner.coinbase) miner.worker.start() } case downloader.DoneEvent: canStart = true downloaderCanStop = false - if miningRequested { + if miningRequested && !miner.Mining() { miner.SetEtherbase(miner.coinbase) miner.worker.start() } From 284f533acc421f1ed17fc671145bc9e64686e999 Mon Sep 17 00:00:00 2001 From: meows Date: Mon, 5 Oct 2020 09:54:34 -0500 Subject: [PATCH 14/18] 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 --- miner/miner.go | 77 ++++++++++++++++++++++++++++---------------------- 1 file changed, 43 insertions(+), 34 deletions(-) diff --git a/miner/miner.go b/miner/miner.go index df06a7a1a1bf..cb9c9e0327c5 100644 --- a/miner/miner.go +++ b/miner/miner.go @@ -79,49 +79,58 @@ func New(eth Backend, config *Config, chainConfig *params.ChainConfig, mux *even 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() + } + }() - downloaderCanStop := true 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: - if downloaderCanStop { - 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.Mining() { - miner.SetEtherbase(miner.coinbase) - miner.worker.start() - } - case downloader.DoneEvent: - canStart = true - downloaderCanStop = false - if miningRequested && !miner.Mining() { - miner.SetEtherbase(miner.coinbase) - miner.worker.start() - } - } + handleDownloaderEvents(ev) case addr := <-miner.startCh: if canStart { miner.SetEtherbase(addr) From e84dd728dc9fa13a984651fde2400d459cd0f7a9 Mon Sep 17 00:00:00 2001 From: meows Date: Mon, 5 Oct 2020 11:25:25 -0500 Subject: [PATCH 15/18] miner: fix handling downloader events on subcription closed Signed-off-by: meows --- miner/miner.go | 18 +++++++++++++----- miner/miner_test.go | 22 ++++++++++++++++++++++ 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/miner/miner.go b/miner/miner.go index cb9c9e0327c5..9630c79a1257 100644 --- a/miner/miner.go +++ b/miner/miner.go @@ -124,13 +124,21 @@ func (miner *Miner) update() { } } + // Handle downloader events while subscription is open. + go func() { + for { + select { + case ev := <-downloaderEvents.Chan(): + if ev == nil { + return + } + handleDownloaderEvents(ev) + } + } + }() + for { select { - case ev := <-downloaderEvents.Chan(): - if ev == nil { - return - } - handleDownloaderEvents(ev) case addr := <-miner.startCh: if canStart { miner.SetEtherbase(addr) diff --git a/miner/miner_test.go b/miner/miner_test.go index 148f14933628..20bf2534c2ea 100644 --- a/miner/miner_test.go +++ b/miner/miner_test.go @@ -135,7 +135,29 @@ func TestMinerDownloaderFirstFails(t *testing.T) { 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) { From 08ac45d3adf5454babee2c5b8c66ede419dcec52 Mon Sep 17 00:00:00 2001 From: meows Date: Mon, 5 Oct 2020 11:44:15 -0500 Subject: [PATCH 16/18] miner: (lint:gosimple) use range over chan instead of for/select Signed-off-by: meows --- miner/miner.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/miner/miner.go b/miner/miner.go index 9630c79a1257..6c7d8b7dd04a 100644 --- a/miner/miner.go +++ b/miner/miner.go @@ -126,14 +126,11 @@ func (miner *Miner) update() { // Handle downloader events while subscription is open. go func() { - for { - select { - case ev := <-downloaderEvents.Chan(): - if ev == nil { - return - } - handleDownloaderEvents(ev) + for ev := range downloaderEvents.Chan() { + if ev == nil { + return } + handleDownloaderEvents(ev) } }() From 68946a9f05365018d2df1d6788d8da878fe439c5 Mon Sep 17 00:00:00 2001 From: meows Date: Mon, 5 Oct 2020 15:03:40 -0500 Subject: [PATCH 17/18] 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/miner.go | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/miner/miner.go b/miner/miner.go index 6c7d8b7dd04a..c8bb7fbca9c5 100644 --- a/miner/miner.go +++ b/miner/miner.go @@ -90,7 +90,7 @@ func (miner *Miner) update() { }() miningRequested := false - canStart := true + 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. @@ -102,20 +102,20 @@ func (miner *Miner) update() { case downloader.StartEvent: wasMining := miner.Mining() miner.worker.stop() - canStart = false + isDownloading = true if wasMining { // Resume mining after sync was finished miningRequested = true log.Info("Mining aborted due to sync") } case downloader.FailedEvent: - canStart = true + isDownloading = false if miningRequested { miner.SetEtherbase(miner.coinbase) miner.worker.start() } case downloader.DoneEvent: - canStart = true + isDownloading = false if miningRequested { miner.SetEtherbase(miner.coinbase) miner.worker.start() @@ -124,20 +124,37 @@ func (miner *Miner) update() { } } - // Handle downloader events while subscription is open. - go func() { - for ev := range downloaderEvents.Chan() { + // 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 := <-downloaderEvents.Chan(): if ev == nil { - return + break withDownloaderState } 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() } From a1b454711615a8a0b1d360cab3a7d6d305573a3f Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Tue, 13 Oct 2020 13:14:37 +0200 Subject: [PATCH 18/18] miner: alternate fix for miner-flaw --- miner/miner.go | 108 +++++++++++++++++++------------------------------ 1 file changed, 41 insertions(+), 67 deletions(-) diff --git a/miner/miner.go b/miner/miner.go index c8bb7fbca9c5..35c036ba75bf 100644 --- a/miner/miner.go +++ b/miner/miner.go @@ -79,88 +79,62 @@ func New(eth Backend, config *Config, chainConfig *params.ChainConfig, mux *even 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: + shouldStart := false + canStart := true + 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() + canStart = false + if wasMining { + // Resume mining after sync was finished + shouldStart = true + log.Info("Mining aborted due to sync") + } + 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() } - 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 !isDownloading { + if canStart { miner.SetEtherbase(addr) miner.worker.start() } - miningRequested = true + shouldStart = true case <-miner.stopCh: - miningRequested = false + shouldStart = false miner.worker.stop() case <-miner.exitCh: miner.worker.close()