Ignore node announcements for nodes not found in any existing channel #227

halseth · 2017-06-26T15:40:46Z

As specified in BOLT#7, we should ignore any node announcements for nodes that we have not gotten any channel announcements for. This is to prevent an attacker to DoSing the network by creating a lot of cheap nodes.

We are now sending a node announcement (self) after every channel announcement
When we receive a channel announcement we are adding a node to the graph which only has the pubkey field set. The rest will be set when a node announcement for the same node is received.

Fixes #138.

Roasbeef

I still need to take a close look at the tests, but I've completed an initial round of review.

Roasbeef · 2017-07-04T04:18:03Z

channeldb/graph.go

+
+	nodePub := node.PubKey.SerializeCompressed()
+
+	if err := aliasBucket.Put(nodePub, []byte(node.Alias)); err != nil {


In the scenario that we're adding a partial node, then we don't yet know it's alias.

Also the order here is off, the alias is written much after the public key. As a result, alias bucket shouldn't yet be updated as we don't have a valid alias for a node if we've only discovered it via a ChannelUpdate message.

Roasbeef · 2017-07-04T04:20:08Z

discovery/service.go

@@ -537,6 +538,10 @@ func (d *AuthenticatedGossiper) processNetworkAnnouncement(nMsg *networkMsg) []l
 			BitcoinKey2: msg.BitcoinKey2,
 			AuthProof:   proof,
 		}
+
+		// We will add the edge to the channel router. If the nodes present in this
+		// channel is not present in the database from before, a partial node will


is not -> are not

Also "from before" can be dropped.

Roasbeef · 2017-07-04T04:20:37Z

discovery/service.go

-	// both of the directed infos that make up the channel.
+	// As peers are expecting channel announcements before node announcements, we
+	// first retrieve the initial announcement, as well as the latest channel
+	// update announcement for both of the directed infos that make up each


directed infos -> directed edges

Roasbeef · 2017-07-04T04:25:06Z

fundingmanager.go

@@ -1246,6 +1246,8 @@ func (f *fundingManager) announceChannel(localIDKey, remoteIDKey, localFundingKe
 	f.cfg.SendAnnouncement(ann.chanAnn)
 	f.cfg.SendAnnouncement(ann.chanUpdateAnn)
 	f.cfg.SendAnnouncement(ann.chanProof)
+
+	// TODO: Send node announcement?


After reflecting, I think a node announcement should indeed follow any local announcements we send out.

Roasbeef · 2017-07-04T04:30:39Z

discovery/service_test.go

@@ -828,3 +828,85 @@ func TestOrphanSignatureAnnouncement(t *testing.T) {
 		t.Fatal("wrong number of objects in storage")
 	}
 }
+
+// We should ignore node announcements of nodes not associated with any


Comment should be prefixed with TestOrphanNodeAnnouncement.

Also I'm not sure that this is the correct package for this test. Correct me if I'm wrong, but the underlying logic of ignoring a node announcement if there aren't any matching channels for it is implemented in the router itself.

halseth · 2017-07-12T13:32:54Z

I added a new field to the LightningNode struct, HaveNodeAnnouncement, which is set to false if we want to add a node to the graph which we found in a channel announcement. That way we can use the AddLightningNode method to add it to the graph as before, we just return early after writing the pubkey if HaveNodeAnnouncement is false.

Now we also sign and send a new NodeAnnouncement after each channel annoucement, and the test were updated to expect this extra node announcement notification.

halseth · 2017-07-12T13:32:58Z

PTAL

coveralls · 2017-07-20T09:20:48Z

Coverage increased (+0.02%) to 73.89% when pulling 75abb2c on halseth:channel-announcement-ordering into 78f6caf on lightningnetwork:master.

coveralls · 2017-07-20T09:28:49Z

Coverage increased (+0.03%) to 73.898% when pulling dbc194b on halseth:channel-announcement-ordering into 78f6caf on lightningnetwork:master.

Roasbeef

Just completed a final round of review. I've tested this PR locally, and within a sub-graph of the current Bitcoin testnet deployment and confirmed everything works as advertised. Nice work!

I have a few final comments, none of them major. The most pertinent one is an attempt to ensure that we don't cause extraneous messages to propagate through the network by always updating the timestamp of our NodeAnnouncement each time a channel is funded.

Also this will need to be rebased to the latest master, as there's a conflict in the fundingManager. Within the wallet, what used to be the ChannelDB field is now accessed as wallet.Cfg.Database.

Roasbeef · 2017-08-02T00:29:01Z

channeldb/graph.go

+	PubKey *btcec.PublicKey
+
+	// HaveNodeAnnouncement indicates whether we received a node annoucement
+	// for this particular node. If true, the remaining fiels will be set,


fiels -> fields

Roasbeef · 2017-08-02T01:13:03Z

fundingmanager.go

+	// is only accepted after a channel is known for that particular node,
+	// and this might be our first channel.
+	graph := f.cfg.Wallet.ChannelDB.ChannelGraph()
+	self, err := graph.FetchLightningNode(f.cfg.IDKey)


The graph.SourceNode method can be used in place of FetchLightningNode. Worth noting though that I don't consider this a blocker to merging the PR.

Though my comment above invalidates this comment.

Roasbeef · 2017-08-02T01:15:55Z

fundingmanager.go

@@ -152,6 +152,10 @@ type fundingConfig struct {
 	// cannot be located, then an error is returned.
 	SignMessage func(pubKey *btcec.PublicKey, msg []byte) (*btcec.Signature, error)

+	// SignNodeAnnouncement is used by the fundingManager to sign the
+	// updated self node announcements sent after each channel announcement.
+	SignNodeAnnouncement func(nodeAnn *lnwire.NodeAnnouncement) (*btcec.Signature, error)


Rather than requesting a signature for a node announcement, we should instead simply be fetching the latest pre-signed NodeAnnouncment. We currently create, and sign one at start up within the Start method of the server struct.

If we take this path, then we avoid unnecessarily updating our the timestamp on our node announcement. By avoiding this update, we save the network a bit of bandwidth as nodes now don't need to add this (unchanged) announcement to their re-broadcast buffers.

Adds a HaveNodeAnnouncement field to the LightningNode struct, which is used to indicate if we have gotten all the necessary information to fill the remaining fields in the struct. If we haven't gotten a node announcement for this specific node, then we only know the pubkey, and can only fill that field in the struct. Still, we should be able to add it to the channel graph and use it for routes, as long as we know about channels to this node.

This commit introduces the requirement specified in BOLT#7, where we ignore any node announcements for a specific node if we yet haven't seen any channel announcements where this node takes part. This is to prevent someone DoS-ing the network with cheap node announcements. In the router this is enforced by requiring a call to AddNode(node_id) to be preceded by an AddEdge(edge_id) call, where node_id is one of the nodes in edge_id.

According to BOLT#7, nodes will ignore node announcements for nodes not found in any previous channel announcements. This commit makes the discovery service send its known channels before its known nodes when syncing the channel graph with a peer.

Make the fundingmanager send an updated node announcement each time it opens a new channel. This is to make sure our node announcement is propagated in the network, since peers will ignore our node announcements if we haven't opened any channels yet.

coveralls · 2017-08-02T20:36:07Z

Coverage increased (+0.05%) to 73.785% when pulling 7aada05 on halseth:channel-announcement-ordering into 24db310 on lightningnetwork:master.

Roasbeef

LGTM 🗼

Roasbeef requested changes Jul 4, 2017

View reviewed changes

halseth force-pushed the channel-announcement-ordering branch from 7a3dcad to b455494 Compare July 12, 2017 13:24

halseth force-pushed the channel-announcement-ordering branch 2 times, most recently from a581497 to 8fc68b6 Compare July 19, 2017 20:45

halseth force-pushed the channel-announcement-ordering branch from 75abb2c to dbc194b Compare July 20, 2017 09:24

halseth mentioned this pull request Jul 31, 2017

routing/notifs: order invariant testing of ntfn delivery #238

Merged

Roasbeef requested changes Aug 2, 2017

View reviewed changes

halseth added 5 commits August 2, 2017 22:28

server: set HaveNodeAnnouncement=true for self LightningNode

946fe9f

halseth force-pushed the channel-announcement-ordering branch from dbc194b to 7aada05 Compare August 2, 2017 20:29

Roasbeef approved these changes Aug 2, 2017

View reviewed changes

Roasbeef merged commit e211270 into lightningnetwork:master Aug 2, 2017

halseth deleted the channel-announcement-ordering branch July 12, 2018 13:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore node announcements for nodes not found in any existing channel #227

Ignore node announcements for nodes not found in any existing channel #227

halseth commented Jun 26, 2017 •

edited

Roasbeef left a comment

Roasbeef Jul 4, 2017

Roasbeef Jul 4, 2017

Roasbeef Jul 4, 2017

Roasbeef Jul 4, 2017

Roasbeef Jul 4, 2017

Roasbeef Jul 4, 2017

halseth commented Jul 12, 2017

halseth commented Jul 12, 2017

coveralls commented Jul 20, 2017

coveralls commented Jul 20, 2017

Roasbeef left a comment

Roasbeef Aug 2, 2017

Roasbeef Aug 2, 2017

Roasbeef Aug 2, 2017

Roasbeef Aug 2, 2017

coveralls commented Aug 2, 2017 •

edited

Roasbeef left a comment


		nodePub := node.PubKey.SerializeCompressed()

		if err := aliasBucket.Put(nodePub, []byte(node.Alias)); err != nil {

Ignore node announcements for nodes not found in any existing channel #227

Ignore node announcements for nodes not found in any existing channel #227

Conversation

halseth commented Jun 26, 2017 • edited

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

halseth commented Jul 12, 2017

halseth commented Jul 12, 2017

coveralls commented Jul 20, 2017

coveralls commented Jul 20, 2017

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

coveralls commented Aug 2, 2017 • edited

Roasbeef left a comment

Choose a reason for hiding this comment

halseth commented Jun 26, 2017 •

edited

coveralls commented Aug 2, 2017 •

edited