watchtower: anchor channel support by cfromknecht · Pull Request #4782 · lightningnetwork/lnd

cfromknecht · 2020-11-18T23:51:04Z

This PR adds watchtower client support for anchor channels. Previously, no backup
was attempted for anchor channels meaning that, under certain breach scenarios, they
are more vulnerable then the legacy channels. This PR resolves this lingering gap in the
security features lnd offers for the two channel types, making the security of anchor
channels equal or better than the legacy channel type across the relevant dimensions.
This gives us the confidence to make anchor channels the default channel type in 0.12
since it offers a universal improvement to our users regardless of their node configuration.

Summary of changes:

adds a new blob.FlagAnchorChannel bit communicated during session negotiation
modifies the wtclient's signing logic to generate valid signatures for anchor justice txns
adds wtwire.AnchorCommit feature bits for determining client/server feature support
adds separate AnchorTowerClient for anchor channels to use to back up their states. this
allows us to reuse most of the existing tower logic without needing to add a greater degree of
multiplexing internally
exposes both the legacy TowerClient and AnchorTowerClient via rpc

TODO:

add wtclient unit test cases
stricter feature bit/blob type assertions in wtserver
move remainder of wtclient package over to prefix logging
lncli integration

Builds on #4781

Roasbeef

First pass, need to revisit the codebase as a whole before the second pass as it's been a while since I've reviewed code in this area. At a high level though, I think the duplicate client route in the short-term makes the set of changes quite easy to follow, and it can eventually be generalized to a specific channel for each possibly future channel type.

Roasbeef · 2020-11-19T02:55:22Z

Alternatively, this could be abstracted to use something similar to the session filter higher level function: the link calls something like TowerClient(chanType) and has that return the proper tower client. Once we also add more commitment types, this'll allow the link to be less cluttered by this selection logic.

i was also thinking of another approach, where the peer selects the appropriate tower client to give to the link. since the two clients share the same interface, the link can be completely unaware as to which is the underlying. main benefit would be less code changes in the link and no function invocation overhead

halseth

quick first pass

halseth · 2020-11-19T15:01:29Z

what is the reason we need it to be separate?

Currently each "client" has a full pipeline that is capable of processing backups for a specific channel type, each maintains its own set tower candidates/state machines/session queues/etc. The option was either to multiplex inside the client, but that seemed pretty invasive and more risky given that the full flow operates pretty reliably today. We decided to go with the approach of just spinning up two separate clients so that each is fully isolated and we know their wont be any unexpected bugs introduced there.

Down the road, the goal would be to add another layer of multiplexing between the links and the "clients", which facilitates sending channel updates to the correct pipelines. For now since we only have two, it is easier just to expose the multiplexing in the link/peer. However, as we start to add more channel types, e.g. anchor-zero-fee-htlc, anchor-taproot, etc, we plan on circling back so that lnd just sees one unified megaclient that handles everything under the hood.

Adds the optional/required feature bit pair for negotiating sessions for anchor channels under features bits 2+3.

Roasbeef

Changes looking really solid now! Main thing I want to do before merge is update my own client to get a feel for if we're missing anything UX wise on the CLI with the new set of commands.

LGTM ☄️

halseth

Looks very good! Just a few questions really, otherwise this LGTM 💯

halseth · 2020-11-26T15:04:11Z

+		// transactions.
+		if t.chanType.HasAnchors() {
+			weightEstimate.AddWitnessInput(
+				input.ToLocalPenaltyWitnessSize,


Hm, yeah this is something I must keep in mind when changing weight estimate constants: #4775

yeah, definitely a little subtle that some of these are now fixed params in the wt protocol. will double check that #4775 doesn't touch any of the required constants for anchors

halseth · 2020-11-26T15:13:19Z


+	chanType := lnChan.State().ChanType
+
+	// Select the appropriate tower client based on the channel type. It's


halseth · 2020-11-26T15:16:00Z

 			subCfgValue := extractReflectValue(subCfg)

-			if towerClient != nil {
+			if towerClient != nil && anchorTowerClient != nil {


why &&? Because none or both are active?

yeah, currently it's only possible to activate both. we could do them separately if we think that's better

halseth · 2020-11-26T15:19:03Z

+    /*
+    The client type from which to retrieve the active offering policy.
+    */
+    PolicyType policy_type = 1;


could instead return a list of all active policies?

the only way you can distinguish them atm would be to parse the blob-type feature bits, which seems less user friendly imo

cfromknecht added watchtower wtserver wtclient v0.12 labels Nov 18, 2020

cfromknecht added this to the 0.12.0 milestone Nov 18, 2020

cfromknecht requested a review from Roasbeef as a code owner November 18, 2020 23:51

cfromknecht changed the title ~~Anchor wtserver~~ watchtower: anchor channel support Nov 18, 2020

cfromknecht force-pushed the anchor-wtserver branch from c731379 to fb4abac Compare November 18, 2020 23:58

Roasbeef requested changes Nov 19, 2020

View reviewed changes

halseth suggested changes Nov 19, 2020

View reviewed changes

cfromknecht added 9 commits November 25, 2020 15:03

watchtower/wtwire: add AnchorCommit feature bits

069296c

Adds the optional/required feature bit pair for negotiating sessions for anchor channels under features bits 2+3.

wathtower/blob+wtserver: support anchor sessions

3856acc

watchtower/wtclient: parameterize backup task with channel type

dd325f0

watchtower/wtpolicy: add IsAnchorChannel helper

74416c6

wtclient: error on unknown witness type for backup task

97cb030

watchtower/wtclient: add anchor backup tasks

eb00e49

wtclient: add anchor-aware session negotiation and filtering

781c6e5

server: initialize distinct anchor tower client

60ad01e

server: log tower client error on stop (lint)

094ce09

cfromknecht force-pushed the anchor-wtserver branch from fb4abac to c7a30f6 Compare November 25, 2020 23:10

cfromknecht added 2 commits November 25, 2020 15:39

link+peer: thread anchor tower client to link

0d0f22a

wtclient: prefix client logs w/ legacy or anchor

7fc4c7c

cfromknecht force-pushed the anchor-wtserver branch from c82ed78 to 014f914 Compare November 25, 2020 23:42

cfromknecht added 4 commits November 25, 2020 16:16

lnrpc/wtclientrpc: add anchor_client bool to all requests

a5c4085

multi: thread anchor client down to wtclient subserver

5b3a08a

wtclinet+itest: expose anchor client via rpc + add itest

b28bbc3

lncli: add policy type flags to wtclient policy

f3c8311

cfromknecht force-pushed the anchor-wtserver branch from 014f914 to f3c8311 Compare November 26, 2020 00:17

Roasbeef approved these changes Nov 26, 2020

View reviewed changes

Comment thread watchtower/wtclient/backup_task.go

Comment thread peer/brontide.go

Comment thread lnrpc/wtclientrpc/wtclient.proto

Comment thread lntest/itest/lnd_test.go

halseth approved these changes Nov 26, 2020

View reviewed changes

Roasbeef merged commit 94f8311 into lightningnetwork:master Dec 1, 2020

halseth mentioned this pull request Dec 1, 2020

multi: add new config option to toggle gossip rate limiting #4810

Merged

cfromknecht mentioned this pull request Dec 27, 2020

watchtower/wtclient: add prefix logging #4885

Merged


		chanType := lnChan.State().ChanType

		// Select the appropriate tower client based on the channel type. It's

Conversation

cfromknecht commented Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

halseth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

halseth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cfromknecht commented Nov 18, 2020 •

edited

Loading