wtclient: prune closable sessions when the tower is unreachable

docs/release-notes/release-notes-0.22.0.md

@@ -42,6 +42,17 @@
   regardless of peer connectivity. Uptime is now seeded from the peer's
   actual connection state.
 
+* [Fixed a bug](https://github.com/lightningnetwork/lnd/pull/10947) where a
+  watchtower that became permanently unreachable (for example a tower that
+  went offline or was deactivated with `lncli wtclient deactivate`) could make
+  `lnd` spend a very long time cleaning up on startup. Closable watchtower
+  sessions (those whose channels are all closed) are now pruned from the client
+  database on a best-effort basis even when the tower cannot be reached, and
+  the client no longer contacts a deactivated tower when cleaning them up.
+  Previously such sessions could never be deleted, so the client re-dialed the
+  dead tower on every restart and the tower could never be fully removed (see
+  [issue 10646](https://github.com/lightningnetwork/lnd/issues/10646)).
+
 # New Features
 
 ## Functional Enhancements
@@ -134,3 +145,4 @@
 * bitromortac
 * Boris Nagaev
 * Erick Cestari
+* Jan Kuchař

watchtower/wtclient/client.go

@@ -561,16 +561,25 @@ func (c *client) stopAndRemoveSession(id wtdb.SessionID, final bool) error {
 // deleteSessionFromTower dials the tower that we created the session with and
 // attempts to send the tower the DeleteSession message.
 func (c *client) deleteSessionFromTower(sess *wtdb.ClientSession) error {
-	// First, we check if we have already loaded this tower in our
-	// candidate towers iterator.
+	// Load the tower from the DB so that we can check its status. If the
+	// tower has been deactivated by the user, then there is no point in
+	// attempting to reach it to notify it of the session deletion; we
+	// signal this to the caller so that our local copy of the (closable)
+	// session can still be pruned. We check the DB tower directly since
+	// the in-memory Tower does not carry the tower's status.
+	dbTower, err := c.cfg.DB.LoadTowerByID(sess.TowerID)
+	if err != nil {
+		return err
+	}
+	if dbTower.Status == wtdb.TowerStatusInactive {
+		return errTowerInactive
+	}
+
+	// Use the tower from our candidate iterator if we have already loaded
+	// it, so that we share its address iterator. Otherwise, construct one
+	// from the DB tower.
 	tower, err := c.candidateTowers.GetTower(sess.TowerID)
 	if errors.Is(err, ErrTowerNotInIterator) {
-		// If not, then we attempt to load it from the DB.
-		dbTower, err := c.cfg.DB.LoadTowerByID(sess.TowerID)
-		if err != nil {
-			return err
-		}
-
 		tower, err = NewTowerFromDBTower(dbTower)
 		if err != nil {
 			return err

watchtower/wtclient/client_test.go

@@ -2093,6 +2093,162 @@ var clientTests = []clientTest{
 			require.NoError(h.t, err)
 		},
 	},
+	{
+		// Assert that a closable session is still deleted from the
+		// client's own database even when the tower it was negotiated
+		// with can no longer be reached. Informing the tower that it
+		// may delete the session is best-effort: a session is only
+		// closable once all of its channels are closed, so the local
+		// copy no longer protects anything. A permanently unreachable
+		// tower must therefore not prevent local cleanup, otherwise the
+		// session lingers forever and the client re-dials the dead
+		// tower on every restart (see issue #10646).
+		name: "assert that closable sessions are deleted even if " +
+			"the tower is unreachable",
+		cfg: harnessCfg{
+			localBalance:  localBalance,
+			remoteBalance: remoteBalance,
+			policy: wtpolicy.Policy{
+				TxPolicy:   defaultTxPolicy,
+				MaxUpdates: 5,
+			},
+		},
+		fn: func(h *testHarness) {
+			const numUpdates = 5
+
+			h.sendUpdatesOn = true
+
+			// Saturate a session with updates for channel 0 and
+			// ensure the tower receives them all.
+			hints := h.advanceChannelN(0, numUpdates)
+			h.backupStates(0, 0, numUpdates, nil)
+			h.server.waitForUpdates(hints, waitTime)
+
+			// Exactly one session should hold updates for the
+			// channel.
+			sessionIDs := h.relevantSessions(0)
+			require.Len(h.t, sessionIDs, 1)
+
+			// Close the channel so that the now-exhausted session
+			// becomes closable.
+			h.closeChannel(0, 1)
+
+			err := wait.Predicate(func() bool {
+				return h.isSessionClosable(sessionIDs[0])
+			}, waitTime)
+			require.NoError(h.t, err)
+
+			// Simulate the tower disappearing from the network by
+			// removing its dial callback from the mock net. Any
+			// dial to it now fails, just like a tower whose host is
+			// permanently unreachable.
+			h.net.removeConnCallback(h.server.addr)
+
+			// Mine enough blocks to exceed the session-close-range
+			// so that the client tries to hand the session off to
+			// the tower and then delete it.
+			h.mine(3)
+
+			// Even though the tower can no longer be reached, the
+			// client must still remove the closable session from
+			// its own database.
+			err = wait.Predicate(func() bool {
+				cs, err := h.clientDB.ListClosableSessions()
+				require.NoError(h.t, err)
+
+				sessions, err := h.clientDB.ListClientSessions(
+					nil,
+				)
+				require.NoError(h.t, err)
+
+				_, ok := sessions[sessionIDs[0]]
+
+				return len(cs) == 0 && !ok
+			}, waitTime)
+			require.NoError(h.t, err)
+		},
+	},
+	{
+		// Assert that when a closable session belongs to a tower that
+		// the user has deactivated, the client prunes the session from
+		// its own database without attempting to contact the tower. We
+		// verify that no contact is made by asserting that the (still
+		// reachable) tower server retains the session in its own DB
+		// (see issue #10646).
+		name: "assert that closable sessions of a deactivated tower " +
+			"are deleted without contacting the tower",
+		cfg: harnessCfg{
+			localBalance:  localBalance,
+			remoteBalance: remoteBalance,
+			policy: wtpolicy.Policy{
+				TxPolicy:   defaultTxPolicy,
+				MaxUpdates: 5,
+			},
+		},
+		fn: func(h *testHarness) {
+			const numUpdates = 5
+
+			h.sendUpdatesOn = true
+
+			// Saturate a session with updates for channel 0 and
+			// ensure the tower receives them all.
+			hints := h.advanceChannelN(0, numUpdates)
+			h.backupStates(0, 0, numUpdates, nil)
+			h.server.waitForUpdates(hints, waitTime)
+
+			sessionIDs := h.relevantSessions(0)
+			require.Len(h.t, sessionIDs, 1)
+
+			// The tower server should be aware of the session.
+			_, err := h.server.db.GetSessionInfo(&sessionIDs[0])
+			require.NoError(h.t, err)
+
+			// Close the channel so that the now-exhausted session
+			// becomes closable.
+			h.closeChannel(0, 1)
+
+			err = wait.Predicate(func() bool {
+				return h.isSessionClosable(sessionIDs[0])
+			}, waitTime)
+			require.NoError(h.t, err)
+
+			// Deactivate the tower. The tower server is still
+			// up and reachable; only the client has marked it
+			// inactive.
+			err = h.clientMgr.DeactivateTower(
+				h.server.addr.IdentityKey,
+			)
+			require.NoError(h.t, err)
+
+			// Mine enough blocks to exceed the session-close-range
+			// so that the client processes the closable session.
+			h.mine(3)
+
+			// The client should delete the closable session
+			// from its own database.
+			err = wait.Predicate(func() bool {
+				cs, err := h.clientDB.ListClosableSessions()
+				require.NoError(h.t, err)
+
+				sessions, err := h.clientDB.ListClientSessions(
+					nil,
+				)
+				require.NoError(h.t, err)
+
+				_, ok := sessions[sessionIDs[0]]
+
+				return len(cs) == 0 && !ok
+			}, waitTime)
+			require.NoError(h.t, err)
+
+			// Crucially, since the tower was deactivated, the
+			// client should not have contacted it, so the tower
+			// server must still have the session in its own
+			// database.
+			_, err = h.server.db.GetSessionInfo(&sessionIDs[0])
+			require.NoError(h.t, err)
+		},
+	},
 	{
 		// Demonstrate that the client is able to recover after
 		// deleting its database by skipping through key indices until

watchtower/wtclient/errors.go

@@ -39,4 +39,9 @@ var (
 	// create a new session with a tower with a session key that has already
 	// been used in the past.
 	ErrSessionKeyAlreadyUsed = errors.New("session key already used")
+
+	// errTowerInactive signals that a session's tower has been deactivated
+	// by the user, so no attempt should be made to contact it when cleaning
+	// up a closable session.
+	errTowerInactive = errors.New("tower is inactive")
 )

watchtower/wtclient/manager.go

@@ -937,19 +937,37 @@ func (m *Manager) handleClosableSessions(
 					continue
 				}
 
+				// Tell the tower it can delete the
+				// session. This is best-effort: the
+				// session is closable, meaning all of
+				// its channels are closed, so our local
+				// copy no longer protects anything and
+				// contacting the tower only lets it free
+				// disk space. We therefore always delete
+				// our local copy below, even if the tower
+				// is unreachable or has been deactivated,
+				// so that a dead or removed tower cannot
+				// make closable sessions accumulate and
+				// be re-dialed on every startup (see
+				// issue #10646).
 				err = client.deleteSessionFromTower(sess)
-				if err != nil {
-					log.Errorf("error deleting "+
-						"session %s from tower: %v",
-						sess.ID, err)
-
-					continue
+				switch {
+				case errors.Is(err, errTowerInactive):
+					log.Debugf("Tower for session %s "+
+						"is inactive, deleting "+
+						"local session copy", sess.ID)
+
+				case err != nil:
+					log.Warnf("Could not delete "+
+						"session %s from tower, "+
+						"deleting local copy "+
+						"anyway: %v", sess.ID, err)
 				}
 
 				err = m.cfg.DB.DeleteSession(item.sessionID)
 				if err != nil {
 					log.Errorf("could not delete "+
-						"session(%s) from DB: %w",
+						"session(%s) from DB: %v",
 						sess.ID, err)
 
 					continue