btcsuite / btcwallet

A secure bitcoin wallet daemon written in Go (golang)
ISC License
1.15k stars 590 forks source link

end recovery on shutdown #944

Closed buck54321 closed 3 months ago

buck54321 commented 3 months ago

The (*Wallet).recovery loop is unmonitored, and shutting down the wallet during recovery without locking the wallet first will hang. This change ensures that the recovery loop is ended when (*Wallet).Stop is called.

Roasbeef commented 3 months ago

Scratch the comment above, I needed another change to the test, stopping it from explicitly calling endRecovery:

diff --git a/wallet/wallet.go b/wallet/wallet.go
index 4bde6225..7d93d153 100644
--- a/wallet/wallet.go
+++ b/wallet/wallet.go
@@ -280,8 +280,6 @@ func (w *Wallet) quitChan() <-chan struct{} {

 // Stop signals all wallet goroutines to shutdown.
 func (w *Wallet) Stop() {
-   <-w.endRecovery()
-
    w.quitMu.Lock()
    quit := w.quit
    w.quitMu.Unlock()
diff --git a/wallet/wallet_test.go b/wallet/wallet_test.go
index 4c1efd7f..06ebb45c 100644
--- a/wallet/wallet_test.go
+++ b/wallet/wallet_test.go
@@ -420,11 +420,9 @@ func TestEndRecovery(t *testing.T) {
    // Recovery is running
    getBlockHashCalls(3)

-   // Closing the quit channel, e.g. Stop() without endRecovery, alone will not
-   // end the recovery loop.
-   w.quitMu.Lock()
-   close(w.quit)
-   w.quitMu.Unlock()
+   // Call stop directly simulating a normal shutdown.
+   w.Stop()
+
    // Continues scanning.
    getBlockHashCalls(3)

@@ -461,19 +459,9 @@ func TestEndRecovery(t *testing.T) {
    // Recovery is running
    getBlockHashCalls(3)

-   // endRecovery is required to exit the unmonitored goroutine.
-   end := w.endRecovery()
-   select {
-   case <-blockHashCalled:
-   case <-recoveryDone:
-   }
-   <-end
-
    // testWallet starts a couple of other unrelated goroutines that need to be
    // killed, so we still need to close the quit channel.
-   w.quitMu.Lock()
-   close(w.quit)
-   w.quitMu.Unlock()
+   w.Stop()

    select {
    case <-waitedForShutdown:
@@ -481,6 +469,7 @@ func TestEndRecovery(t *testing.T) {
        t.Fatal("WaitForShutdown never returned")
    }

    if !strings.EqualFold(err.Error(), "recovery: forced shutdown") {
        t.Fatal("wrong error")
    }