multi: add watchtower client RPC subserver#3184
multi: add watchtower client RPC subserver#3184Roasbeef merged 12 commits intolightningnetwork:masterfrom wpaulino:wtclient-subserver
Conversation
Roasbeef
added
enhancement
cfromknecht
left a comment
cfromknecht
left a comment
There was a problem hiding this comment.
did an initial pass, looks solid so far! tested on my node and was able to modify towers and get info about towers+sessions
cfromknecht
left a comment
cfromknecht
left a comment
There was a problem hiding this comment.
@wpaulino latest changes look solid! no major comments apart form maybe keeping the existing wtclient.private-tower-uris in the config to maintain backcompat for the point release, and removing fully in next major release
watchtower/wtmock/client_db.go
Outdated
There was a problem hiding this comment.
Out of the scope of this PR, so just reflecting, but it seems that we can remove much of the code in this mock tower, if we abstracted the storage backend from the actual tower DB a bit. So in this case, the business logic would be shared across the regular and the mock, with the only difference being storage in a map vs actually on disk.
Each time we modify the behavior of the actual tower DB, we must also take care to ensure that this is also updated, and implements the actual behavior perfectly (ideally). This adds an additional cost/diff to modifying this area of the codebase, as we essentially need two maintain the logic of two structs that do the same thing.
There was a problem hiding this comment.
The mock DB had it place during initial development as we wanted to flesh out the biz logic without being burdened with actual DB details such as bucket structure, serialization, etc. However at this point, I don't see a good reason for it to exists in its current state.
There was a problem hiding this comment.
In other parts of the codebase we have mock DBs for unit tests to speed them up, but doesn't seem like there's something to gain here given that the tests still run with the actual bolt implementation.
There was a problem hiding this comment.
the db tests run both mock and boltdb implementations to assert they have identical behavior. all of the other packages use the mockdb to speed them up
lnrpc/wtclientrpc/wtclient.go
Outdated
There was a problem hiding this comment.
This would be nice, as otherwise, the same macaroon (atm) that can add/remove towers is also able to do things like send coins on chain. On the other-hand, one could argue that being able to remove towers is possibly move dangerous than sending coins on chain...
I think we can leave this out for now, and leave the time from now until the first 0.8 release candidate to mull things over.
The state updates that are pending to be acked by the tower are already loaded within newSessionQueue.
This currently takes O(N) time as there does not exist an index of active client sessions for each watchtower within the client's database. This index is likely to be added in the future.
These operations are currently unused, but will be integrated into the TowerClient at a later point as future preparation for the WatchtowerClient RPC subserver, which will allow users to add, remove, and list the watchtowers currntly in use.
In this commit, we extend the wtclient.Client interface with the following methods: * AddTower * RemoveTower * RegisteredTowers * LookupTower * Stats Care has been taken to ensure that any in-memory state updates are _only_ performed after a successful database update. These methods are currently unused, but they serve as a dependency for the upcoming WatchtowerClient RPC subserver.
We do this as a convenience for WatchtowerClient users so that they do not need to re-add towers upon restarts. We ensure not to re-add towers that have been previously removed by determining whether it has any lingering active sessions.
With the introduction of the WatchtowerClient RPC subserver, the lnd configuration flag to specify private watchtowers for the client is no longer needed and can lead to confusion upon users. Therefore, we remove the flag completely, and only rely on the watchtower client being active through a new --wtclient.active flag.
…didates Doing so allows us to load balance sessions better amongst all of the tower candidates.
Roasbeef
left a comment
Roasbeef
left a comment
There was a problem hiding this comment.
LGTM 🌊
Gave this a spin on my testnet nodes, was able to modify the set of towers it was connected to w/o any issue!
3 participants
In this PR, we introduce a new RPC subserver:
WatchtowerClientRPC. So far, this RPC subserver allows us to interact with the daemon's backing watchtower client at runtime. Certain changes were needed for thewtclient.TowerClientandwtclient.DBinterfaces in order to provide the functionality required by the RPC subserver.