eth: close miner on exit (instead of just stopping)

[holiman]

This PR fixes #19965, I think.

Previously, when shutting down the Ethereum service, the miner was stopped before the blockchain, which is fine. However, Stop and Close are diferent:

func (miner *Miner) Stop() {
	miner.stopCh <- struct{}{}
}

func (miner *Miner) Close() {
	close(miner.exitCh)
}

The Stop tells the worker routine to stop, but stays in the loop. The Close causes the miner to also shut down the worker routine, and then exit the main loop:

		case <-miner.stopCh:
			shouldStart = false
			miner.worker.stop()
		case <-miner.exitCh:
			miner.worker.close()
			return

In the worker, it's also different:

// stop sets the running status as 0.
func (w *worker) stop() {
	atomic.StoreInt32(&w.running, 0)
}

// close terminates all background threads maintained by the worker.
// Note the worker does not support being closed multiple times.
func (w *worker) close() {
	atomic.StoreInt32(&w.running, 0)
	close(w.exitCh)
}

They both set w.running to 0, but close also signals on the exitCh, which causes the worker loop to exit, and thus not do any more commit operations.
I'm fairly sure this change is for the better, however, I'm not 100% sure it solves the issue, since we're closing the worker.exitCh, but we don't actually wait for the worker to react to it -- so there's still a potential commit operation already happening.

If the issue still persist, we might do better by also adding a waitgroup to wait for the miner workers to actually exit before allowing the blockchain to shut down.

[ryanschneider]

I would love to see this in master soon, we update our rinkeby signer bi-weekly and we hit this panic pretty much every time.

edit: I added more logs to #19965, hope they help

[holiman]

@rjl493456442 PTAL

[holiman]

ping @rjl493456442 PTAL

[fjl]

Please also add a sync.WaitGroup in miner/miner.go to wait for the shutdown of the Miner.update goroutine when Miner.Close is called.

[holiman]

Please also add a sync.WaitGroup in miner/miner.go to wait for the shutdown of the Miner.update goroutine when Miner.Close is called.

Fixed, ptal. I have not tested it yet, let's see what the tests say first

[holiman]

I'll put this PR on one of our PoA signers

[holiman]

rebased

[piersy]

I'm suggesting an approach which I think makes managing the wg a bit easier, by keeping all the code contained in one place.

Suggested change

	worker.wg.Add(4)
	go worker.mainLoop()
	go worker.newWorkLoop(recommit)
	go worker.resultLoop()
	worker.wg.Add(4)
	go func() {
	defer worker.wg.Done()
	worker.mainLoop()
	}()
	go func() {
	defer worker.wg.Done()
	worker.newWorkLoop(recommit)
	}()
	go func() {
	defer worker.wg.Done()
	worker.resultLoop()
	}()
	go func() {
	defer worker.wg.Done()
	worker.taskLoop()
	}()