[watchtower/server] Server-Side Wire Protocol #1535

cfromknecht · 2018-07-11T00:27:52Z

This PR builds on #1512, by adding the server-side handling of watchtower clients. This allows clients to negotiate sessions and send state updates. The wtwire package has been extended with more concrete fail codes, and the SessionInitReply has been altered to return an arbitrary Data field (instead of RewardPubKey), which seems more useful for negotiating session parameters.

The server's db and clients have been mocked out and protected using a build flag, so that we can have adequate black-box testing of the server's behavior. A fairly comprehensive set of test vectors have been added for state update sequences and checking that the errors from the database are being converted into the appropriate wire fail codes.

Things not in the this PR:

watchtower acceptance policy, current accepts any new session id
cleaning up old sessions, have some ideas on how to do this

cfromknecht · 2018-07-19T03:36:00Z

rebased on latest #1512

Roasbeef

So stoked to finally get the initial MVP watch tower code up! I've completed an initial review and dint' have any substantial comments. Most of the comments can be converted into TODO's as they aren't considered blocking critical for this PR.

I really dig the interface-by-first-principles approach that was carried out in the PR as it allows one to write completely "pure" unit tests, so no need to worry about creating a test database or anything like that. This is generally one item we can improve on within lnd as a hole. I think the major culprits are: the db, the peer struct, and at times the channel state machine itself.

Roasbeef · 2018-07-13T03:43:14Z

watchtower/server/server.go

+
+	// NewAddress is used to generate reward addresses, where a cut of
+	// successfully sent funds can be received.
+	NewAddress func() (btcutil.Address, error)


Roasbeef · 2018-07-13T03:50:10Z

watchtower/server/server.go

+func New(cfg *Config) (*server, error) {
+	// Create a brontide listener for each of the configured addrs.
+	listeners := make([]net.Listener, len(cfg.ListenAddrs))
+	for _, addr := range cfg.ListenAddrs {


This isn't a blocker, but the config should also eventually integrate the tor package to allow towers to listen of onion services. I don't think it's a blocker for MVP towers, but is worthy of a TOOD/issue now.

We can actually just pass in the listeners directly even, this would remove any internal details from the watch tower server itself.

yeah passing in the listeners sounds like a good approach. i have the tor configuration done in the top level watchtower package, so server logic should be agnostic to network :)

Roasbeef · 2018-07-13T03:54:28Z

watchtower/server/server.go

+		)
+	}
+
+	rewardAddrBytes := []byte(rewardAddress.EncodeAddress())


We can save a bit of space where by using a signalling protocol to encode the addresses (so byte version, then raw bytes) rather than sending the full strings.

what version byte would we use? can we use ScriptAddress() directly?

Roasbeef · 2018-07-13T04:00:09Z

watchtower/wtdb/breach_hint.go

+)
+
+// BreachHintSize parses 16-bytes from a txid.
+const BreachHintSize = 16


godoc comment is off.

Roasbeef · 2018-07-13T04:02:44Z

watchtower/wtdb/session_info.go

+// AcceptUpdateSequence validates that a state update's sequence number and last
+// applied are valid given our past history with the client. These checks ensure
+// that clients are properly in sync and following the update protocol properly.
+// If t


Fragment is cut off here.

Roasbeef · 2018-07-24T00:13:39Z

watchtower/server/server.go

+		case *wtwire.SessionInit:
+			// Ensure SessionInit can only be sent as the first
+			// message.
+			if stateUpdateOnlyMode {


So if the session for a client expires, then we force it to reconnect and initiate a new session? Why not just allow them to continue using the same connection?

This condition enforces that a SessionInit is only sent as the first message, and cannot follow a StateUpdate. For a single conn, we permit:

1 SessionInit

n StateUpdates

When parsing the first message, we don't know (without querying the db) which case we're dealing with. After finishing the first loop, this becomes true to prevent an ordering of StateUpdate+, SessionInit.

If the connection expires, the client can resume an existing session using the same session keypair. Rationale for not having long-lived connections is it makes it more difficult for the tower to track when clients are on/offline, and also prevents clients from consuming tower resources endlessly.

Roasbeef · 2018-07-24T00:14:23Z

watchtower/server/server.go

+		// A StateUpdate indicates an existing client attempting to
+		// back-up a revoked commitment state.
+		case *wtwire.StateUpdate:
+			log.Infof("Received SessionUpdate from %s, seqnum=%d "+


We'll likely eventually want to demote this to a debug, and later trace, but for now it'll likely be helpful in finding bugs in the initial implementation.

Roasbeef · 2018-07-24T00:15:55Z

watchtower/server/server.go

+// addPeer stores a client in the server's client map. An error is returned if a
+// client with the same session id already exists.
+func (s *server) addPeer(id *wtdb.SessionID, peer Peer) error {
+	s.clientMtx.Lock()


Could just use a defer here to make things a bit easier ;)

Roasbeef · 2018-07-24T00:17:44Z

watchtower/server/server_test.go

+		// Assert that the server closes the connection after processing
+		// the SessionInit. We sleep to ensure we don't check too soon,
+		// as the check also attempts to close the connection.
+		time.Sleep(2 * timeoutDuration)


Ideaaally we remove this sleep in the future, but don't think it's a strong blocker on the current PR.

Fixed all the sleeps, also made the tests faster

Roasbeef · 2018-07-24T00:20:58Z

watchtower/server/server_test.go

+
+		// Check that the server closed the connection used to register
+		// the session.
+		time.Sleep(2 * timeoutDuration)


Similar comment here, ideally this is event driven rather than relying on a sleep.

halseth

woops, old lingering review. Posting and checking newest changes.

halseth · 2018-07-17T14:29:01Z

watchtower/server/server.go

+
+	// ReadTimeout specifies how long a client may go without sending a
+	// message.
+	ReadTimeout time.Duration


What are ballpark values for these timeouts?

was thinking about 15-30s, to allow the watchtower to clean up resources if the client disappears

halseth · 2018-07-17T14:32:42Z

watchtower/server/server.go

+
+	log.Infof("Accepted incoming peer %s@%s",
+		id, peer.RemoteAddr())
+	defer log.Infof("Releasing incoming peer %s@%s",


move this together withe the defer removePeer?

halseth · 2018-07-17T14:59:55Z

watchtower/server/server.go

+		// A StateUpdate indicates an existing client attempting to
+		// back-up a revoked commitment state.
+		case *wtwire.StateUpdate:
+			log.Infof("Received SessionUpdate from %s, seqnum=%d "+


Make Debug or Trace?

halseth · 2018-07-17T15:05:56Z

watchtower/wtdb/breach_hint.go

+import (
+	"encoding/hex"
+
+	"github.com/roasbeef/btcd/chaincfg/chainhash"


should be btcsuite now. Maybe just needs a rebase? :)

cfromknecht · 2018-07-24T23:35:58Z

Thank you @Roasbeef @halseth almost all the feedback has been addressed

Last major thing seem to be fleshing out the reward addr encoding

Roasbeef

LGTM 🎸

Left a few comments during this final pass over. However, I don't consider any of the comments to be blocking.

Roasbeef · 2018-10-25T03:23:37Z

watchtower/wtdb/session_id.go

+	"github.com/btcsuite/btcd/btcec"
+)
+
+// SessionIDSize is 33-bytes; it is a serialized, compressed public key.


Typo in commit:

adds SessoinID

Should be SessionID

Roasbeef · 2018-10-25T03:45:37Z

watchtower/server/server.go

+	return nil
+}
+
+func (s *Server) readMessage(peer Peer) (wtwire.Message, error) {


Missing godoc comment here and above.

Roasbeef · 2018-10-25T03:48:06Z

watchtower/server/log.go

+// log is a logger that is initialized with no output filters.  This
+// means the package will not perform any logging by default until the caller
+// requests it.
+var log btclog.Logger


With the new logging format, I think this still needs to register itself as a logger in log.go within the main lnd directory.

was thinking about doing that when this is no longer dead code, but happy to do it now

Roasbeef · 2018-10-25T03:49:36Z

watchtower/server/interface.go

+
+// Interface represents a simple, listen-only service that accepts watchtower
+// clients, and provides responses to their requests.
+type Interface interface {


Bit of a naming collision here ;)

Although it's not so bad when using the full import: server.Interface. Although in isolation, the server package name may conflict a bit with local variables we have in lnd or w/e else since it's so plain.

yeah was thinking about renaming it to something better, any suggetions??

TowerInterface? Lol, not blocking atm, so we can pick a better name in the future.

Roasbeef · 2018-10-25T03:49:55Z

watchtower/server/interface.go

+
+// Peer is the primary interface used to abstract watchtower clients.
+type Peer interface {
+	io.WriteCloser


Roasbeef · 2018-10-25T03:51:13Z

watchtower/server/server_test.go

@@ -0,0 +1,634 @@
+// +build dev


Are the build tags necessary here? As is, a simple go test -v in this directory won't execute these tests. We can't always assume that the user is using our set of makefile tools. Seems this is just to have access to the mocked variants of the major interfaces?

the linter (and other go tools) complain because the mock classes aren't accessible w/o the dev tag, it is nice that putting them behind a flag keeps mock structs from being rendered on godoc.org

can't help those that do not follow the install docs :P a sizable portion of the tests won't pass/compile without the dev tag anyway, seems they're already out of luck

cfromknecht force-pushed the wtwire-server branch from aec9867 to 1adcb0b Compare July 11, 2018 00:29

cfromknecht added watchtower wtserver wtwire needs review PR needs review by regular contributors labels Jul 11, 2018

cfromknecht force-pushed the wtwire-server branch 4 times, most recently from 452ed56 to 793cbf4 Compare July 11, 2018 05:45

Roasbeef added the P2 should be fixed if one has time label Jul 12, 2018

Roasbeef added this to the 0.5 milestone Jul 17, 2018

cfromknecht force-pushed the wtwire-server branch from 793cbf4 to ef52d35 Compare July 19, 2018 03:35

cfromknecht force-pushed the wtwire-server branch from ef52d35 to 9de04ac Compare July 19, 2018 07:08

Roasbeef reviewed Jul 24, 2018

View reviewed changes

Roasbeef added first pass review done PR has had first pass of review, needs more tho and removed needs review PR needs review by regular contributors labels Jul 24, 2018

halseth reviewed Jul 24, 2018

View reviewed changes

cfromknecht force-pushed the wtwire-server branch 2 times, most recently from 027f495 to c9aff17 Compare July 24, 2018 23:32

cfromknecht force-pushed the wtwire-server branch from c9aff17 to bea88e0 Compare July 24, 2018 23:53

Roasbeef modified the milestones: 0.5, 0.6 Aug 1, 2018

cfromknecht force-pushed the wtwire-server branch 3 times, most recently from 688dea1 to 43fa563 Compare September 7, 2018 04:21

cfromknecht force-pushed the wtwire-server branch 3 times, most recently from 9982ee4 to b9c30c2 Compare October 24, 2018 01:24

cfromknecht and others added 10 commits October 24, 2018 20:18

watchtower/wtdb/breach_hint: adds BreachHint, txid prefix

bc86ccf

watchtower/wtdb/session_id: adds SessoinID, client pubkey

e825a75

watchtower/wtdb/session_info: adds SessionInfo

405838d

watchtower/wtdb/session_state_update: adds session state

8b47a96

watchtower/wtdb/mock: adds MockDB for debug build

fd6024c

watchtower/server/server: server skeleton

7370953

watchtower/server/log: adds wt SRVR sublogger

a3e1bb1

watchtower/server/interface: adds Server, Peer and DB

48643a1

watchtower/server/mock: create MockPeer for debug build

db4c448

watchtower/server/server_test: add test vectors for server behavior

712e7f0

cfromknecht force-pushed the wtwire-server branch from b9c30c2 to 712e7f0 Compare October 25, 2018 03:18

Roasbeef approved these changes Oct 25, 2018

View reviewed changes

Roasbeef merged commit 5c6c966 into lightningnetwork:master Oct 25, 2018

[watchtower/server] Server-Side Wire Protocol #1535

[watchtower/server] Server-Side Wire Protocol #1535

Conversation

cfromknecht commented Jul 11, 2018 • edited

cfromknecht commented Jul 19, 2018

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halseth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfromknecht commented Jul 24, 2018

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfromknecht commented Jul 11, 2018 •

edited