diff --git a/docs/release-notes/release-notes-0.22.0.md b/docs/release-notes/release-notes-0.22.0.md index 260c099f1f2..9030a3fdfc0 100644 --- a/docs/release-notes/release-notes-0.22.0.md +++ b/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ř diff --git a/watchtower/wtclient/client.go b/watchtower/wtclient/client.go index f8f3d3e618b..cad6b197b27 100644 --- a/watchtower/wtclient/client.go +++ b/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 diff --git a/watchtower/wtclient/client_test.go b/watchtower/wtclient/client_test.go index b44d3e2df91..fe30ba69069 100644 --- a/watchtower/wtclient/client_test.go +++ b/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 diff --git a/watchtower/wtclient/errors.go b/watchtower/wtclient/errors.go index 2cb070ac45a..6bc87c92721 100644 --- a/watchtower/wtclient/errors.go +++ b/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") ) diff --git a/watchtower/wtclient/manager.go b/watchtower/wtclient/manager.go index 03a344a15d8..78b5ecc7f53 100644 --- a/watchtower/wtclient/manager.go +++ b/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