wtclient: Tower Client Multiplexer #7702

ellemouton · 2023-05-16T16:42:12Z

Currently in LND, two separate TowerClients get created, one for legacy
channels and one for anchor channels. When taproot channels come along, we
would have to have yet another one of these clients with more switch statements.

So with this PR, a Client Multiplexer is added which manages the tower clients so
that only one object needs to be passed around and called from other parts of the
code base. The multiplexer will handle the separate clients.

Fixes #7540

This is a pre-requisite for #7733

saubyk

Concept ACK

guggero

Great work, as always super clean commit structure and easy to follow story telling 💯
And I love the optimizations and cleanup that went into the code as well.

Do we want to update this to 0-18-staging?

watchtower/wtclient/manager.go

lnrpc/wtclientrpc/wtclient.go

watchtower/wtclient/manager.go

watchtower/wtclient/client.go

lnrpc/wtclientrpc/wtclient.go

peer/brontide.go

server.go

watchtower/wtclient/manager.go

ellemouton

Thanks @guggero !

Do we want to update this to 0-18-staging?

Since that is about to be merged into master - I'll wait for that :)

watchtower/wtclient/client.go

lnrpc/wtclientrpc/wtclient.go

server.go

peer/brontide.go

watchtower/wtclient/manager.go

docs/release-notes/release-notes-0.18.0.md

watchtower/wtclient/manager.go

lnrpc/wtclientrpc/wtclient.go

peer/brontide.go

guggero

Awesome, this is very very close! Just two remaining questions.

guggero · 2023-10-27T10:14:35Z

watchtower/wtclient/manager.go

+					log.Errorf("no client currently " +
+						"active for the session type")
+
+					return


What happens when we return here and below?
Won't the manager basically be in a state where it doesn't continue to work and there might be a memory buildup?
Not sure if it would make sense to either continue here or log a critical error to force a shutdown? Depending on the severity of the error?

ah good catch - I think it makes sense to continue here since if anything fails, it is fine for us to just drop it since it will retry on start up since the session's will still be around & marked as closable

bitromortac

Really nice PR, great to follow along with this commit structure 🏅! Will also do a test run as the next step, but overall the PR looks good

watchtower/wtclient/manager.go

server.go

watchtower/wtclient/client.go

ellemouton

Thanks for the review @bitromortac !

watchtower/wtclient/client.go

watchtower/wtclient/manager.go

bitromortac

LGTM, tACK 🎉

Introduce a wtclient `Manager` which handles tower clients. It indexes clients by the policy used. The policy field is thus removed from the `Config` struct which configures the Manager and is instead added to a new `towerClientCfg` which configures a specific client managed by the manager. For now, only the `NewClient` method is added to the Manager. It can be used to construct a new `TowerClient`. The Manager currently does notthing with the clients added to it.

In this commit, the `Stop` and `Start` methods are removed from the `Client` interface and instead added to the new `Manager`. Callers now only need to call the Manager to start or stop the clients instead of needing to call stop/start on each individual client.

In this commit we move the AddTower method from the Client interface to the TowerClientManager interface. The wtclientrpc is updated to call the `AddTower` method of the Manager instead of calling the `AddTower` method of each individual client. The TowerClient now is also only concerned with adding a new tower (or new tower address) to its in-memory state; the tower Manager will handle adding the tower to the DB.

Simiarly to the previous commit, this commit moves the RemoveTower method from the Client to the TowerClientManager interface. The manager handles any DB related handling. The manager will first attempt to remove the tower from the in-memory state of each client and then will attempt to remove the tower from the DB. If the removal from the DB fails, the manager will re-add the tower to the in-memory state of each client.

This commit removes the mutex from ClientStats and instead puts that in clientStats which wraps ClientStats with a mutex. This is so that the tower client interface can return a ClientStats struct without worrying about copying a mutex.

Move the `Stats` method from the `Client` to the `Manager` interface.

Move the `RegisteredTowers` method from the `Client` to the `Manager` interface.

ellemouton · 2023-11-28T08:07:03Z

working on the rebase 👍

This commit moves over the last two methods, `RegisterChannel` and `BackupState` from the `Client` to the `Manager` interface. With this change, we no longer need to pass around the individual clients around and now only need to pass the manager around. To do this change, all the goroutines that handle channel closes, closable sessions needed to be moved to the Manager and so a large part of this commit is just moving this code from the TowerClient to the Manager.

Rename and unexport the `TowerClient` struct to `client` and rename the `TowerClientManager` interface to `ClientManager`.

lightninglabs-deploy · 2023-12-05T08:59:31Z

@guggero: review reminder
@ellemouton, remember to re-request review from reviewers when ready

guggero

Very nice, LGTM 🎉

ellemouton force-pushed the towerClientMux branch from 7fbbf06 to 0c86191 Compare May 16, 2023 18:01

ellemouton added code health Related to code commenting, refactoring, and other non-behaviour improvements watchtower labels May 16, 2023

ellemouton force-pushed the towerClientMux branch from ea88b2d to 986edc2 Compare May 26, 2023 15:08

ellemouton mentioned this pull request May 28, 2023

watchtower: support taproot channel commitments #7733

Merged

saubyk added this to the v0.17.0 milestone May 30, 2023

saubyk approved these changes Jun 1, 2023

View reviewed changes

saubyk assigned ellemouton Jun 14, 2023

saubyk modified the milestones: v0.17.0, v0.17.1 Jun 15, 2023

saubyk added the P1 MUST be fixed or reviewed label Aug 8, 2023

saubyk modified the milestones: High Priority, v0.18.0 Aug 8, 2023

ellemouton force-pushed the towerClientMux branch from 986edc2 to f93b193 Compare August 11, 2023 13:14

ellemouton force-pushed the towerClientMux branch 5 times, most recently from 24cc3f8 to 1ee86b4 Compare August 29, 2023 07:21

ellemouton mentioned this pull request Aug 29, 2023

watchtower+refactor: CommitmentType and JusticeKit interface #7736

Merged

saubyk requested review from bitromortac and guggero August 31, 2023 15:25

guggero reviewed Sep 22, 2023

View reviewed changes

ellemouton commented Oct 9, 2023

View reviewed changes

ellemouton force-pushed the towerClientMux branch 2 times, most recently from 93d06ad to 2b15920 Compare October 9, 2023 07:31

guggero self-requested a review October 9, 2023 07:39

ellemouton force-pushed the towerClientMux branch from 2b15920 to 61b04dc Compare October 10, 2023 07:05

Chinwendu20 reviewed Oct 16, 2023

View reviewed changes

docs/release-notes/release-notes-0.18.0.md Outdated Show resolved Hide resolved

guggero reviewed Oct 27, 2023

View reviewed changes

watchtower/wtclient/manager.go Outdated Show resolved Hide resolved

watchtower/wtclient/manager.go Outdated Show resolved Hide resolved

lnrpc/wtclientrpc/wtclient.go Show resolved Hide resolved

peer/brontide.go Show resolved Hide resolved

guggero reviewed Oct 27, 2023

View reviewed changes

ellemouton force-pushed the towerClientMux branch from 61b04dc to 51abaad Compare November 8, 2023 13:53

ellemouton requested a review from guggero November 9, 2023 10:10

bitromortac reviewed Nov 14, 2023

View reviewed changes

ellemouton force-pushed the towerClientMux branch from 51abaad to ae9ab62 Compare November 15, 2023 07:20

ellemouton commented Nov 15, 2023

View reviewed changes

watchtower/wtclient/client.go Outdated Show resolved Hide resolved

watchtower/wtclient/manager.go Show resolved Hide resolved

ellemouton requested a review from bitromortac November 15, 2023 10:32

bitromortac approved these changes Nov 15, 2023

View reviewed changes

ellemouton added 9 commits November 28, 2023 09:51

lnrpc+wtclient: refactor ClientStats

f38b5cf

This commit removes the mutex from ClientStats and instead puts that in clientStats which wraps ClientStats with a mutex. This is so that the tower client interface can return a ClientStats struct without worrying about copying a mutex.

wtclient+lnrpc: move Stats to Manager

4348f20

Move the `Stats` method from the `Client` to the `Manager` interface.

wtclient+lnrpc: move RegisteredTowers to Manager

0b3d751

Move the `RegisteredTowers` method from the `Client` to the `Manager` interface.

wtclient+lnrpc: move LookupTower to Manager

4e51bf3

wtclient+lnrpc: move Policy to Manager

ab2f781

ellemouton added 3 commits November 28, 2023 10:59

wtclient+server: unexport and rename TowerClient

e800aac

Rename and unexport the `TowerClient` struct to `client` and rename the `TowerClientManager` interface to `ClientManager`.

docs: update release notes

59ebe02

ellemouton force-pushed the towerClientMux branch from ae9ab62 to 59ebe02 Compare November 28, 2023 09:02

guggero approved these changes Dec 5, 2023

View reviewed changes

ellemouton merged commit 4fa483f into lightningnetwork:master Dec 5, 2023
24 of 25 checks passed

ellemouton deleted the towerClientMux branch December 5, 2023 10:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wtclient: Tower Client Multiplexer #7702

wtclient: Tower Client Multiplexer #7702

ellemouton commented May 16, 2023 •

edited

Loading

saubyk left a comment

guggero left a comment

ellemouton left a comment •

edited

Loading

guggero left a comment

guggero Oct 27, 2023

ellemouton Nov 8, 2023

bitromortac left a comment

ellemouton left a comment

bitromortac left a comment

ellemouton commented Nov 28, 2023

lightninglabs-deploy commented Dec 5, 2023

guggero left a comment

wtclient: Tower Client Multiplexer #7702

wtclient: Tower Client Multiplexer #7702

Conversation

ellemouton commented May 16, 2023 • edited Loading

saubyk left a comment

Choose a reason for hiding this comment

guggero left a comment

Choose a reason for hiding this comment

ellemouton left a comment • edited Loading

Choose a reason for hiding this comment

guggero left a comment

Choose a reason for hiding this comment

guggero Oct 27, 2023

Choose a reason for hiding this comment

ellemouton Nov 8, 2023

Choose a reason for hiding this comment

bitromortac left a comment

Choose a reason for hiding this comment

ellemouton left a comment

Choose a reason for hiding this comment

bitromortac left a comment

Choose a reason for hiding this comment

ellemouton commented Nov 28, 2023

lightninglabs-deploy commented Dec 5, 2023

guggero left a comment

Choose a reason for hiding this comment

ellemouton commented May 16, 2023 •

edited

Loading

ellemouton left a comment •

edited

Loading