wtclient: prune closable sessions when the tower is unreachable#10947
wtclient: prune closable sessions when the tower is unreachable#10947jkuchar wants to merge 2 commits into
Conversation
🟠 PR Severity: HIGH
🟠 High (3 files)
🟢 Low (1 file)
AnalysisAll substantive changes are confined to No bump conditions apply:
No override labels or prior severity classification were present, so this is a fresh classification. To override, add a |
ellemouton
left a comment
ellemouton
left a comment
There was a problem hiding this comment.
good idea!
one question
| if dbTower.Status == wtdb.TowerStatusInactive { | ||
| return errTowerInactive | ||
| } | ||
|
|
There was a problem hiding this comment.
why put it here? this will only catch things if ErrTowerNotInIterator is returned.. what about the other case?
ie, why not put it on line 589?
There was a problem hiding this comment.
The original placement only caught the inactive case because a deactivated (or removed) tower is taken out of the candidate iterator via RemoveCandidate, so GetTower always returns ErrTowerNotInIterator for it. But relying on that inactive ⇔ not-in-iterator invariant is fragile, so this is a fair point.
Putting it literally on line 589 doesn't work either: the in-memory Tower doesn't carry the tower's status, so there's nothing to check there. I've instead moved it to the top of the function — load the DB tower and check Status before the iterator lookup — so both the in-iterator and DB-loaded paths are covered and we no longer depend on the iterator invariant. Updated in the latest push.
When all of a watchtower session's channels are closed, the session becomes "closable" and the client attempts to tell the tower it may delete the session before removing its own copy. Previously, if the tower could not be reached, deleteSessionFromTower returned an error and the session was never deleted from the client's own database. On every restart the client re-loaded these sessions and re-dialed the dead tower, which could keep lnd busy for a very long time on startup, and the tower could never be fully removed because a tower that still has sessions is only ever marked inactive. Notifying the tower is only a courtesy so that it can reclaim disk space: a closable session's channels are all closed, so the local copy no longer protects anything. We therefore now delete the local session copy on a best-effort basis even when the tower cannot be reached. In addition, if the tower has been deactivated by the user, we skip contacting it entirely and prune the session locally. Fixes lightningnetwork#10646.
Add a release note for the fix that prunes closable watchtower sessions locally when the tower is permanently unreachable or has been deactivated, and add the author to the contributor list (lightningnetwork#10646).
jkuchar
force-pushed
the
fix-wtclient-dead-tower-cleanup
branch
from
c7b6a46 to
c3b159a
Compare
|
@ellemouton is there anything work that needs to be done on this PR? Or should I just wait for the second reviewer? |
2 participants
Fixes #10646
Problem
When every channel backed up by a watchtower session is closed, the session becomes "closable" and
handleClosableSessionstries to notify the tower that it may delete the session (viadeleteSessionFromTower) before removing the client's own copy.If the tower is permanently unreachable (offline, or deactivated/removed by the user), that dial fails, the loop
continues, andDB.DeleteSessionis never reached. As a result:ListClosableSessionsreloads it and the client re-dials the dead tower. With many sessions this keepslndbusy for a very long time on startup (the reporter saw "hours"), emitting a stream oferror deleting session ... from tower: failed to dial tower(...).wtdb.RemoveToweronly deletes a tower that has zero sessions — otherwise it just marks it inactive. So the dead tower's sessions, and the tower itself, are stuck forever (also the subject of [feature]: cleanup of wtclient.db / removal of watchtower #7035).Fix
Notifying the tower is only a courtesy so that it can reclaim disk space: a closable session's channels are all closed, so the local copy no longer protects anything (a breach remedy is impossible once the funding output has been spent by a confirmed close). So:
deleteSessionFromTowerfails, we log a warning and still delete the local session copy, so a dead/removed tower can no longer make closable sessions accumulate or block startup.Testing
Two new cases in
TestClient:The full
watchtower/wtclientsuite passes.Not in this PR (possible follow-ups)
--force/purge escape hatch so an operator can immediately remove a dead tower and its remaining sessions (relates to [feature]: cleanup of wtclient.db / removal of watchtower #7035).ConnectionTimeoutper address). With the two changes here a dead tower's sessions are pruned after a single failed dial rather than re-dialed forever, so this is an optional optimization.