lnwire: Update NodeAnnouncement address advertisement to match spec #154

bryanvu · 2017-02-27T07:02:28Z

This PR should address issues #131, #135, and #141.

andrewshvv · 2017-02-27T07:51:45Z

channeldb/graph.go

@@ -1142,7 +1142,7 @@ func (c *ChannelGraph) NewChannelEdge() *ChannelEdge {

 func putLightningNode(nodeBucket *bolt.Bucket, aliasBucket *bolt.Bucket, node *LightningNode) error {
 	var (
-		scratch [8]byte
+		scratch [16]byte


Why it was changed to 16?

@AndrewSamokhvalov thanks for the review and the comments! Good catch here. I made a change that required the additional bytes, and then backed that out and left this in. Fixed now.

andrewshvv · 2017-02-27T07:51:54Z

channeldb/graph.go

@@ -1199,27 +1209,45 @@ func fetchLightningNode(nodeBucket *bolt.Bucket,
 func deserializeLightningNode(r io.Reader) (*LightningNode, error) {
 	node := &LightningNode{}

-	var scratch [8]byte
-	if _, err := r.Read(scratch[:]); err != nil {
+	var scratch [16]byte


andrewshvv · 2017-02-27T08:13:46Z

lnwire/node_announcement.go


-	// 158
-	return length
+	// Base size, 147, but can be variable due to multiple addresses


Maybe it is not so important as far as it will change in near future , but it seems for me that base size should be 140 because length of addresses might be equal to 0 according to spec.

andrewshvv · 2017-02-27T08:16:41Z

lnwire/node_announcement_test.go

-		t.Fatalf("payload length estimate is incorrect: expected %v "+
-			"got %v", serializedLength, na.MaxPayloadLength(0))
-	}
-}


Maybe rather than removing it, we consider check the exact length of message instead?

Re-added. I saw that a few of the other variable length messages had the payload length test taken out, but I think it makes sense to have it.

andrewshvv · 2017-02-27T08:53:00Z

server.go

+				Permanent: true,
+			}
+			s.persistentConnReqs[pubStr] = connReq
+			go s.connMgr.Connect(connReq)


I believe we should think more about this logic:

We are rewriting the connection request on every iteration now:(s.persistentConnReqs[pubStr] = connReq).

I am not sure but it seems for me that such approach might lead to duplicate creation of peers. Maybe we should add additional check in addPeer method (server.go:559-560)?

Added a slice of connReqs to address issue #1. For #2, I believe @Roasbeef was going to add something in the connection manager to ensure that when a connection is established, the other connection attempts are cancelled. I believe this should prevent the [inbound|outbound]peerConnected from being called more than once, ensuring that duplicate peers won't be created.

andrewshvv · 2017-02-28T22:13:13Z

LGTM! 👍

Roasbeef · 2017-03-01T05:27:10Z

channeldb/graph.go

 		return err
 	}

+	for _, address := range node.Addresses {
+		if err := wire.WriteVarString(&b, 0, address.Network()); err != nil {


iirc whether an address is IPv4 or IPv6 it'll always return a "tcp" if it's a net.TCPAddr. Therefore, instead I think we can just rely on the String method of the net.Addr itself here. It would also be a bit more compact (as we can leave out the network).

Roasbeef · 2017-03-01T05:39:58Z

channeldb/graph.go

+		address, err := net.ResolveTCPAddr(network, addr)
+		if err != nil {
+			return nil, err
+		}


In light of my comment above, I think we should instead add a single byte prefix before the encoded address so we know how to properly decode it.

Also the address resolution above wouldn't work with say, .onion addresses.

Roasbeef · 2017-03-01T05:52:14Z

lnwire/lnwire.go

-		if _, err := w.Write(ip[:]); err != nil {
-			return err
-		}
+		switch net := e.Network(); net {


This won't work as expected for IPv6 addresses as they'll still always return "tcp" from the Network() method.

I think instead, we might be able to use the To4 method on the IP struct to discern if an address if IPv6 or not. The method will return nil if the address an IPv6 address.

Roasbeef · 2017-03-01T05:54:33Z

lnwire/lnwire_test.go

@@ -56,7 +56,10 @@ var (
 	someSig, _   = btcec.ParseSignature(sigStr, btcec.S256())
 	someSigBytes = someSig.Serialize()

-	someAddress = &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 8333}
+	//TODO(bvu): one of the following addresses should be an IPv6 address.


This TODO should be addressed before we merge this PR.

Roasbeef · 2017-03-01T05:55:33Z

routing/pathfind_test.go

@@ -109,10 +109,12 @@ func parseTestGraph(path string) (*channeldb.ChannelGraph, func(), aliasMap, err
 	// We'll use this fake address for the IP address of all the nodes in
 	// our tests. This value isn't needed for path finding so it doesn't
 	// need to be unique.
+	testAddrs := make([]net.Addr, 0)


Style nit, this can instead be declared as:

var testAddrs []net.Addr

Roasbeef · 2017-03-01T05:56:15Z

server.go

-		connReq := &connmgr.ConnReq{
-			Addr:      lnAddr,
-			Permanent: true,
+		// In case a node has multiple addresses, attempt to connect to


A new line should be inserted above this comment.

Roasbeef · 2017-03-01T05:57:11Z

server.go

 	}
+	delete(s.persistentConnReqs, pubStr)


The deletion should be moved into the if ... ok clause.

Roasbeef · 2017-03-01T05:57:21Z

server.go

+	// ongoing connection attempts to ensure that we don't end up with a
+	// duplicate connecting to the same peer.
+	s.pendingConnMtx.RLock()
+	delete(s.persistentConnReqs, pubStr)


Upon further consideration, I don't think deleting items from the persistentConnReqs map is required after all. Honestly, I think the management of the peer connection state needs a bit of a re-write...

bryanvu · 2017-03-03T05:34:26Z

Updated. Previous version can be found at: https://github.com/bryanvu/lnd/tree/nodeannounce-1. Not sure the way I did the constants for TCP/ONION is idiomatic in Go, suggestions welcome.

Roasbeef · 2017-03-09T00:40:41Z

channeldb/graph.go

@@ -111,6 +111,12 @@ type ChannelGraph struct {
 	//  * LRU cache for edges?
 }

+// TODO(bvu): add comments


We can add a bit more typing to this definition with something along the lines of:

type addressType uint8 const ( tcp4Addr addressType = 0 tcp6Addr addressType = 1 onionAddr addressType = 2 )

Roasbeef · 2017-03-09T00:42:21Z

channeldb/graph.go

+	for _, address := range node.Addresses {
+		if address.Network() == "tcp" {
+			scratch[0] = byte(TCP)
+			if _, err := b.Write(scratch[0:0]); err != nil {


scratch[0:0] can safely be replaced with scratch[0]

Roasbeef · 2017-03-09T00:47:48Z

channeldb/graph.go

+		var address net.Addr
+		switch addrType {
+		case TCP:
+			address, err = net.ResolveTCPAddr("tcp", addr)


As is now, this'll force an outbound name resolution for each address on db read which should be able to be avoided.

We can avoid this by writing the raw byte representation of a TCPAddr rather than using the supplied .String() method. So this would entail writing the raw bytes of the IP, Zone and Port fields.

Roasbeef · 2017-03-09T00:48:47Z

lnd_test.go

+	for _, node := range chanGraph.Nodes {
+		if node.PubKey == dave.PubKeyStr {
+			if len(node.Addresses) != 3 {
+				t.Fatalf("expecting dave to have two addresses, got: %v",


Looks the the error message wasn't updated when the additional addresses were added (reads "two", should be "three").

Roasbeef · 2017-03-09T00:49:16Z

lnd_test.go

+	}
+
+	chanAmt := btcutil.Amount(btcutil.SatoshiPerBitcoin / 2)
+	chanPointAlice := openChannelAndAssert(ctxt, t, net, net.Alice, dave,


I don't think opening a new channel is necessary for this test as it only deals with the proper propagation of a node announcement within the network.

Since I was using DescribeGraph to check the contents of the NodeAnnouncement, I had to open a channel here, I believe.

Roasbeef · 2017-03-09T00:55:25Z

lnwire/lnwire.go

+			copy(ip[:], e.IP.To16())
+			if _, err := w.Write(ip[:]); err != nil {
+				return err
+			}


Hmm just realized the spec currently leaves out the concept of IPv6 zones...

coveralls · 2017-03-20T18:31:58Z

Coverage decreased (-0.4%) to 67.35% when pulling cae47da on bryanvu:nodeannounce into 5d43483 on lightningnetwork:master.

coveralls · 2017-03-20T18:31:58Z

Coverage decreased (-0.4%) to 67.35% when pulling cae47da on bryanvu:nodeannounce into 5d43483 on lightningnetwork:master.

bryanvu · 2017-03-20T18:34:20Z

Updated with requested changes. Previous version can be found at: https://github.com/bryanvu/lnd/tree/nodeannounce-2.

coveralls · 2017-03-20T18:46:11Z

Coverage decreased (-0.4%) to 67.35% when pulling 9c9add9 on bryanvu:nodeannounce into 5d43483 on lightningnetwork:master.

coveralls · 2017-03-20T22:37:08Z

Coverage decreased (-0.3%) to 67.46% when pulling 8162a66 on bryanvu:nodeannounce into 5d43483 on lightningnetwork:master.

coveralls · 2017-03-20T23:05:41Z

Coverage decreased (-0.07%) to 67.648% when pulling db803b8 on bryanvu:nodeannounce into 5d43483 on lightningnetwork:master.

Roasbeef · 2017-03-21T22:00:18Z

channeldb/graph.go

@@ -819,6 +830,8 @@ type LightningNode struct {
 	//
 	// TODO(roasbeef): hook into serialization once full verification is in
 	AuthSig *btcec.Signature
+	// Features is the list of protocol features supported by this node.


Minor nit: missing a newline above this variable definition.

Roasbeef · 2017-03-21T22:03:25Z

channeldb/graph.go

+	for _, address := range node.Addresses {
+		if address.Network() == "tcp" {
+			if address.(*net.TCPAddr).IP.To4() != nil {
+				scratch[0] = 0


To improve the readability, I think the enum defined above should be used directly here:

scratch[0] = tcp4Addr

Roasbeef · 2017-03-21T22:03:39Z

channeldb/graph.go

+					return err
+				}
+			} else {
+				scratch[0] = 1


Similar comment here about using the enum directly.

Roasbeef · 2017-03-21T22:05:31Z

channeldb/graph.go

+					return err
+				}
+
+				copy(scratch[:4], address.(*net.TCPAddr).IP.To4())


From my reading, this looks like it'll override the address type which is the first element in the slice. To fix this, the bounds on this write should instead be:

scratch[1:5]

Roasbeef · 2017-03-21T22:06:49Z

channeldb/graph.go

+				if _, err := b.Write(scratch[:1]); err != nil {
+					return err
+				}
+				copy(scratch[:], address.(*net.TCPAddr).IP.To16())


Similar comment here about the copy overriding the addrType set as the first variable.

Roasbeef · 2017-03-21T22:09:45Z

lnd_test.go

+
+	for _, node := range chanGraph.Nodes {
+		if node.PubKey == dave.PubKeyStr {
+			if len(node.Addresses) != 3 {


For the sake of additional coverage, I think this test should also be expanded to assert the exact existence of the three addresses listed above.

Roasbeef · 2017-03-21T22:13:51Z

lnwire/node_announcement.go

+	Addresses []net.Addr
+
+	// pad is used to reserve to additional bytes for future usage.
+	pad uint16


Why was this padding added? It isn't a part of the struct.

You can also remove the TODO(roasbeef) above.

Roasbeef · 2017-03-21T22:19:30Z

server.go

-	if connReq, ok := s.persistentConnReqs[pubStr]; ok {
-		s.connMgr.Remove(connReq.ID())
+	if connReqs, ok := s.persistentConnReqs[pubStr]; ok {
+		for _, connReq := range connReqs {


A similar code block (removing all the connection requests for the particular node pubkey) needs to be inserted below within the outboundPeerConnected method. We'll need to similarly cancel all the pending outbound requests (launched within newServer) as we've successfully established a connection.

Not totally sure how this works, but when I added this to outboundPeerConnected, the nodes didn't reconnect upon startup. Is there something else I have to do to make that happen?

Roasbeef · 2017-03-21T22:21:07Z

lnwire/lnwire.go

-		}
+		if e.IP.To4() != nil {
+			var descriptor [1]byte
+			descriptor[0] = 1


Can you lift the 1 and 2 values, into an addrType enum similar to what was done in the channeldb package? Thanks!

Roasbeef

Meant to request changes rather than approve...

coveralls · 2017-03-22T23:23:50Z

Coverage decreased (-0.03%) to 68.995% when pulling 57ec79a on bryanvu:nodeannounce into 1efbaee on lightningnetwork:master.

This commit modifies address handling in the NodeAnnouncement struct, switching from net.TCPAddr to []net.Addr. This enables more flexible address handling with multiple types and multiple addresses for each node. This commit addresses the first part of issue lightningnetwork#131 .

Minor change to server.go to add ExternalIPs to channeldb.LightningNode. Also, added a test that utilizes this functionality and exercises multiple addresses in NodeAnnouncement.

address.

Add support for Features in NodeAnnouncment according to spec.

Use addresses and ports from NodeAnnouncement messages for reconnection attempts. For those nodes that don't explicitly report IP addresses, use the IP address from previous connections connection request along with the default peer port number.

coveralls · 2017-03-29T04:05:43Z

Coverage decreased (-0.03%) to 69.01% when pulling 8d4f94d on bryanvu:nodeannounce into 65c15c4 on lightningnetwork:master.

Roasbeef

Alright, I've run some final tests locally and everything seems to be in order. This directly and indirectly addresses a number of lingering issues and opens the door to allow us to do things like experiment with automated channel creation with a fixed pool of funds for a node.

LGTM 🏂

bryanvu force-pushed the nodeannounce branch from 46d474b to 4ae62c9 Compare February 27, 2017 07:14

andrewshvv reviewed Feb 27, 2017

View reviewed changes

bryanvu force-pushed the nodeannounce branch 2 times, most recently from 83e2821 to a874ce9 Compare February 28, 2017 02:20

bryanvu force-pushed the nodeannounce branch from a874ce9 to c05d979 Compare March 1, 2017 01:26

Roasbeef requested changes Mar 1, 2017

View reviewed changes

bryanvu force-pushed the nodeannounce branch 2 times, most recently from be4f621 to 3e95cd9 Compare March 3, 2017 05:28

Roasbeef requested changes Mar 9, 2017

View reviewed changes

bryanvu force-pushed the nodeannounce branch 6 times, most recently from ad66bb2 to cae47da Compare March 20, 2017 18:26

bryanvu force-pushed the nodeannounce branch from cae47da to 9c9add9 Compare March 20, 2017 18:40

bryanvu force-pushed the nodeannounce branch from 9c9add9 to 8162a66 Compare March 20, 2017 22:29

bryanvu force-pushed the nodeannounce branch from 8162a66 to db803b8 Compare March 20, 2017 23:00

Roasbeef requested changes Mar 21, 2017

View reviewed changes

Roasbeef approved these changes Mar 21, 2017

View reviewed changes

Roasbeef requested changes Mar 21, 2017

View reviewed changes

bryanvu force-pushed the nodeannounce branch from db803b8 to 2de5f6f Compare March 22, 2017 23:00

bryanvu force-pushed the nodeannounce branch 2 times, most recently from 8651277 to 57ec79a Compare March 22, 2017 23:18

bryanvu added 5 commits March 28, 2017 21:00

config: added support, tests for --externalip config option

0211d9f

Minor change to server.go to add ExternalIPs to channeldb.LightningNode. Also, added a test that utilizes this functionality and exercises multiple addresses in NodeAnnouncement.

connmgr: for nodes with multiple addresses, attempt to connect to each

3227f3a

address.

lnwire: add support for Features in NodeAnnouncement

d6ab84b

Add support for Features in NodeAnnouncment according to spec.

bryanvu force-pushed the nodeannounce branch from fbaa4e9 to 8d4f94d Compare March 29, 2017 04:01

Roasbeef approved these changes Mar 29, 2017

View reviewed changes

Roasbeef merged commit c9c2848 into lightningnetwork:master Mar 29, 2017

This was referenced Mar 29, 2017

Update NodeAnnouncement address advertisement to match spec #131

Closed

Add an --external_ip configruation option to the daemon #135

Closed

Ensure the connmgr doesn't attempt to establish connections based off of incoming connection info #141

Closed

lnwire: Update NodeAnnouncement address advertisement to match spec #154

lnwire: Update NodeAnnouncement address advertisement to match spec #154

Conversation

bryanvu commented Feb 27, 2017 • edited by Roasbeef

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

andrewshvv Feb 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewshvv commented Feb 28, 2017

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

Roasbeef Mar 1, 2017 • edited

Choose a reason for hiding this comment

bryanvu commented Mar 3, 2017

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

coveralls commented Mar 20, 2017

coveralls commented Mar 20, 2017

bryanvu commented Mar 20, 2017

coveralls commented Mar 20, 2017

coveralls commented Mar 20, 2017

coveralls commented Mar 20, 2017 • edited

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

Roasbeef left a comment

Choose a reason for hiding this comment

coveralls commented Mar 22, 2017

coveralls commented Mar 29, 2017 • edited

Roasbeef left a comment

Choose a reason for hiding this comment

bryanvu commented Feb 27, 2017 •

edited by Roasbeef

andrewshvv Feb 27, 2017 •

edited

Roasbeef Mar 1, 2017 •

edited

coveralls commented Mar 20, 2017 •

edited

coveralls commented Mar 29, 2017 •

edited