wtclient: add DeactivateTower and TerminateSession commands #8239

ellemouton · 2023-11-30T11:03:11Z

The Issues

This PR addresses 2 main issues that exist today:

On startup, the wtclient iterates through all the sessions that it has and then uses those to determine which towers can be seen as candidates. This doesnt really make sense for a few reasons:
- We could have added a tower and the done a restart before any sessions get created with that tower. Today, this tower would not be considered for session negotiation on restart since it has no sessions.
- We could have sessions with an old tower that is no longer usable and yet we will still iterate through all of its sessions to see if any of them are active and then we might still load this tower. This is very inefficient & will result in many error logs about the tower not being connectable.
If anything strange happens with the current active session resulting in it not being able to send backups to the server, there is no way for a user to get the client to move on to another session. This can cause the entire client to be frozen just on one session. Sessions do have a "Status" but that status is either "Active" or "inactive" and it is entirely controlled by the tower being created (activate all sessions) or removed (inactivate all sessions). So there is no fine grained control available for sessions yet.

The Solutions:

To solve issue 1) we introduce a Status to a persisted Tower along with a DeactivateTower method and command that a user can use. If a tower's status is "Inactive" then it wont be loaded on start-up and neither will any of its sessions.
We change the meaning of a session's Inactive status to rather mean "Terminal" and we change the logic so that re-adding a tower through "CreateTower" no longer changes the status of sessions & "RemoveTower" also does not. Only a call to "DeleteCommittedUpdates" or a manual instruction from the user via the "TerminateSession" call will now set a session's status to "terminal".

Side Effects:

Changing the session "Inactive" status to mean "terminal" means that any session currently in the "inactive" state will never be "active" again. I argue that this is ok since: 1) the session would only have been in the "inactive" state if a user manually requested "RemoveTower" for the corresponding tower meaning they were probably having issues with that tower anyways. 2) All tower clients are still altruistic meaning that this will not mean that they paid for sessions that they can no longer use.

Fixes: #8221 (step 2)
Fixes: #7949
Fixes: #6397
Replaces: #7545

ellemouton · 2024-01-19T14:47:21Z

note to self to add a commit here which ensures that a tower is loaded as an active tower even if all the sessions we have for the tower are exhausted.

lightninglabs-deploy · 2024-02-01T12:00:21Z

@bitromortac: review reminder
@guggero: review reminder

guggero

Very nice! Many users will be very happy about these changes 🎉

As always, super nice commit structure and great unit test coverage! I wonder if we have any existing integration test cases around towers that we could add these new RPC calls to?

watchtower/wtdb/client_db.go

watchtower/wtdb/tower.go

watchtower/wtclient/client.go

watchtower/wtclient/manager.go

lnrpc/wtclientrpc/wtclient.proto

watchtower/wtclient/client.go

lnrpc/wtclientrpc/wtclient.proto

bitromortac

Looks really great on first pass, awesome work 🚀! Will need to do a second pass to internalize the session/tower state changes better, might be useful to have documentation on that in some place.

watchtower/wtdb/client_db.go

watchtower/wtclient/client_test.go

watchtower/wtclient/client.go

watchtower/wtdb/client_db.go

watchtower/wtdb/client_db_test.go

watchtower/wtclient/client_test.go

coderabbitai · 2024-02-09T13:12:29Z

Important

Auto Review Skipped

Auto reviews are limited to the following labels: llm-review. Please add one of these labels to enable auto reviews.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository.

To trigger a single review, invoke the @coderabbitai review command.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit-tests for this file.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit tests for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository from git and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit tests.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

ellemouton

Thanks for the reviews @bitromortac & @guggero 🚀

I've updated accordingly and also added an itest as was suggested.

Will re-request once the CI is green

watchtower/wtdb/client_db.go

watchtower/wtclient/manager.go

watchtower/wtdb/client_db_test.go

lnrpc/wtclientrpc/wtclient.proto

watchtower/wtclient/client.go

guggero

Amazing work, LGTM 🎉

watchtower/wtdb/tower.go

bitromortac

This is really close and very cool itest updates 🔥, I've got some final small questions.

watchtower/wtdb/client_db.go

watchtower/wtclient/manager.go

watchtower/wtclient/client.go

watchtower/wtdb/client_db_test.go

watchtower/wtclient/manager.go

itest/lnd_watchtower_test.go

lntest/rpc/watchtower.go

This commit updates the DeleteCommittedUpdate DB method to delete all of a given session's committed updates instead of just one at a time. The reason for this is that in an upcoming commit, we will introduce a "Terminal" session state - once we have deleted a committed update for a session it should be considered "Terminal" and there is never a case where we would only want to delete one committed update and not the rest. So we want these two actions (deleting committed updates of a session and setting it's status to terminal) to be atomic.

This is added as a TLV record meaning that all the towers currently on disk that don't have this new field will be seen as Active.

And then only load active towers on client start up.

This new method sets the tower's status to inactive so that it is not loaded at startup as a candidate tower. We also ensure that a tower's status is set to active if the CreateTower is called when the tower already exists.

This commit adds the DeactiateTower method to the wtclient.ClientManager interface along with its implementation. A test is also added for the new method.

Since we will now change this to mean that the session should not ever be activated again.

In this commit, we adjust the DeleteCommitmentUpdate method so that it marks a session as Terminal (if there are updates to delete) since once we have deleted a commitment update from a session - the session is no longer useable.

Load an active tower into memory regardless of whether or not it has any sessions.

This will be used in the itest added in an upcoming commit so that we can test the TerminateSession command.

ellemouton · 2024-02-20T12:46:42Z

Thanks @bitromortac 🙏 I think I've addressed all your comments

bitromortac

LGTM 🎉 awesome work!

watchtower/wtclient/manager.go

ellemouton added rpc Related to the RPC interface lncli watchtower performance labels Nov 30, 2023

ellemouton self-assigned this Nov 30, 2023

ellemouton mentioned this pull request Nov 30, 2023

wtclient: ability to mark a session as borked #7545

Closed

ellemouton force-pushed the loadSessionByActiveTower branch from 1e7955d to 4e2811f Compare November 30, 2023 11:04

ellemouton changed the title ~~wtclient: add DeactivateTower and Terminate session commands~~ wtclient: add DeactivateTower and TerminateSession commands Nov 30, 2023

saubyk added this to the v0.18.0 milestone Nov 30, 2023

ellemouton force-pushed the loadSessionByActiveTower branch from 4e2811f to d20b640 Compare December 5, 2023 11:13

ellemouton mentioned this pull request Jan 2, 2024

wtclient: improve startup performance #8221

Closed

2 tasks

saubyk requested review from bitromortac and guggero January 23, 2024 17:41

guggero reviewed Feb 5, 2024

View reviewed changes

bitromortac reviewed Feb 6, 2024

View reviewed changes

ellemouton force-pushed the loadSessionByActiveTower branch from d20b640 to 2ff3979 Compare February 9, 2024 13:11

ellemouton requested a review from yyforyongyu as a code owner February 9, 2024 13:11

ellemouton removed the request for review from yyforyongyu February 9, 2024 13:13

ellemouton force-pushed the loadSessionByActiveTower branch 2 times, most recently from 57e8fac to ed8f4da Compare February 9, 2024 14:20

ellemouton commented Feb 9, 2024

View reviewed changes

ellemouton force-pushed the loadSessionByActiveTower branch from ed8f4da to 242bc17 Compare February 12, 2024 06:41

ellemouton requested review from guggero and bitromortac February 12, 2024 08:06

guggero approved these changes Feb 13, 2024

View reviewed changes

watchtower/wtdb/tower.go Outdated Show resolved Hide resolved

ellemouton force-pushed the loadSessionByActiveTower branch from 242bc17 to 9fd9bd2 Compare February 13, 2024 10:34

bitromortac reviewed Feb 19, 2024

View reviewed changes

ellemouton added 4 commits February 20, 2024 08:58

wtdb: add TowerStatus to Tower

ffd355c

This is added as a TLV record meaning that all the towers currently on disk that don't have this new field will be seen as Active.

watchtower: add filter function to ListTowers

0bb1816

And then only load active towers on client start up.

wtdb: add a DeactivateTower method

4548e72

This new method sets the tower's status to inactive so that it is not loaded at startup as a candidate tower. We also ensure that a tower's status is set to active if the CreateTower is called when the tower already exists.

ellemouton force-pushed the loadSessionByActiveTower branch from 9fd9bd2 to f16190c Compare February 20, 2024 10:17

ellemouton added 14 commits February 20, 2024 14:44

wtclient: add DeactivateTower method

beb9b2e

This commit adds the DeactiateTower method to the wtclient.ClientManager interface along with its implementation. A test is also added for the new method.

lnrpc+lncli: add DeactivateTower to rpc

2643235

wtdb: rename CSessionInactive to CSessionTerminal

5cb8c8d

Since we will now change this to mean that the session should not ever be activated again.

wtdb: let DeleteCommitedUpdates move session to terminal

cbf0894

In this commit, we adjust the DeleteCommitmentUpdate method so that it marks a session as Terminal (if there are updates to delete) since once we have deleted a commitment update from a session - the session is no longer useable.

wtdb: create and remove tower should no longer change session status

41582c0

wtdb: add TerminateSession method

05c0340

wtclient: add TerminateSession method

24702ed

lnrpc+lncli: add TerminateSession command

5d16491

wtclient: load all active towers into memory

a9648cb

Load an active tower into memory regardless of whether or not it has any sessions.

lnrpc: add session ID to tower session info

1c7a754

This will be used in the itest added in an upcoming commit so that we can test the TerminateSession command.

itest: refactor watchtower related tests

d4424fb

itest+refactor: extract various tower test helpers

59a4bfb

itest+lntest: add wtclient session & tower management test

4ab1752

docs: update release notes

a3ccae9

ellemouton force-pushed the loadSessionByActiveTower branch from f16190c to a3ccae9 Compare February 20, 2024 12:44

ellemouton requested a review from bitromortac February 20, 2024 12:46

bitromortac approved these changes Feb 21, 2024

View reviewed changes

watchtower/wtclient/manager.go Show resolved Hide resolved

ellemouton merged commit c398b0c into lightningnetwork:master Feb 21, 2024
24 of 25 checks passed

ellemouton deleted the loadSessionByActiveTower branch February 21, 2024 09:28

This was referenced Feb 21, 2024

[bug]: WTCL: received error code CodePermanentFailure in StateUpdateReply for seqnum=xxx #7949

Closed

Can't remove remote watchtower that is reporting error "StateUpdateCodeSeqNumOutOfOrder in StateUpdateReply for seqnum=1021". #6397

Closed

ellemouton mentioned this pull request Mar 14, 2024

[bug]: Watchtower Servers Cannot be Removed #8547

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wtclient: add DeactivateTower and TerminateSession commands #8239

wtclient: add DeactivateTower and TerminateSession commands #8239

ellemouton commented Nov 30, 2023 •

edited

ellemouton commented Jan 19, 2024

lightninglabs-deploy commented Feb 1, 2024

guggero left a comment

bitromortac left a comment

coderabbitai bot commented Feb 9, 2024

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

CodeRabbit Discord Community

ellemouton left a comment

guggero left a comment

bitromortac left a comment

ellemouton commented Feb 20, 2024

bitromortac left a comment

wtclient: add DeactivateTower and TerminateSession commands #8239

wtclient: add DeactivateTower and TerminateSession commands #8239

Conversation

ellemouton commented Nov 30, 2023 • edited

The Issues

The Solutions:

Side Effects:

ellemouton commented Jan 19, 2024

lightninglabs-deploy commented Feb 1, 2024

guggero left a comment

Choose a reason for hiding this comment

bitromortac left a comment

Choose a reason for hiding this comment

coderabbitai bot commented Feb 9, 2024

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

CodeRabbit Discord Community

ellemouton left a comment

Choose a reason for hiding this comment

guggero left a comment

Choose a reason for hiding this comment

bitromortac left a comment

Choose a reason for hiding this comment

ellemouton commented Feb 20, 2024

bitromortac left a comment

Choose a reason for hiding this comment

ellemouton commented Nov 30, 2023 •

edited

CodeRabbit Configration File (`.coderabbit.yaml`)