De-duplicate announcements in the AuthenticatedGossiper -#275 #322

flaurida · 2017-09-08T01:26:45Z

Issue #275. De-duplicate announcements added to the announcementBatch within the networkHandler goroutine. Make TrickleDelay configurable and exposed as a command line option. Also fix a few minor typos.

Roasbeef

Thanks for the PR!

Most of my comments in line are rather minor. However, there's one major issue with this PR: it has no tests. A set of tests within the discovery package should be added to exercise the new behavior, namely that announcements are properly de-duplicated when multiple announcements (with the same de-dup key) are added between trickle tickers.

Roasbeef · 2017-09-25T23:57:00Z

discovery/gossiper.go

-// expected to be properly signed as dictated in BOLT#7, additionally, all
-// incoming message are expected to be well formed and signed. Invalid messages
-// will be rejected by this struct.
+// announcements, validating them and applying the changes to router, syncing


Scanning this diff fragment, I can't see any change other than the line-wrapping being modified (to a smaller col limit?). Can you revert this section of the diff if there're no material change? Thanks!

Roasbeef · 2017-09-25T23:57:17Z

discovery/gossiper.go

+// channel updates can be identified by the (ShortChannelID, Flags)
+// tuple.
+type ChannelUpdateID struct {
+	// ShortChannelID represents the set of data which is needed to


The godoc comment should start with: channelID.

Roasbeef · 2017-09-25T23:57:49Z

discovery/gossiper.go

+	channelUpdates map[ChannelUpdateID]lnwire.Message
+
+	// Node announcements are identified by node id field.
+	nodeAnnouncements map[*btcec.PublicKey]lnwire.Message


Each of the godoc comment on the three member variables should begin with the name of the variable itself.

Roasbeef · 2017-09-25T23:58:36Z

discovery/gossiper.go

+	// updates, node announcements) is set to an empty map where the
+	// approprate key points to the corresponding lnwire.Message.
+	channelAnnouncements := make(map[lnwire.ShortChannelID]lnwire.Message)
+	d.channelAnnouncements = channelAnnouncements


Each of these mutations can be done in line, like so:

d.channelAnnouncements = make(map[lnwire.ShortChannelID]lnwire.Message)

This'll shave a few lines of the total diff.

Roasbeef · 2017-09-25T23:59:22Z

discovery/gossiper.go

 	//  * buffer recv'd node ann until after chan ann that includes is
 	//    created
 	//    * can use mostly empty struct in db as place holder
-	var announcementBatch []lnwire.Message
+
+	// initialize empty deDupedAnnouncements to store announcement batch


Minor nit: should be a full sentence.

Roasbeef · 2017-09-26T00:00:57Z

config.go

@@ -35,6 +35,7 @@ const (
 	defaultRPCHost            = "localhost"
 	defaultMaxPendingChannels = 1
 	defaultNumChanConfs       = 1
+	defaultTrickleDelay       = 300


The default should be something much higher than 300 ms. Instead, it should be on the order of 10s of seconds.

Roasbeef · 2017-09-26T00:02:24Z

networktest.go

@@ -127,6 +131,7 @@ func newLightningNode(btcrpcConfig *rpcclient.ConnConfig, lndArgs []string) (*li
 			RPCUser: btcrpcConfig.User,
 			RPCPass: btcrpcConfig.Pass,
 		},
+		TrickleDelay: trickleDelay,


Setting TrickleDelay here doesn't actually do anything as it isn't being passed into the arguments populated within genArgs.

The default delay should be something on the order of 10s of seconds while within the integration tests, we'll use the 300ms value in order to keep the tests snappy (especially when running on Travis).

Roasbeef

This is getting very close! I just have some minor comments w.r.t the current defaultTrickleDelay value and a struct which shouldn't be publicly exported.

Once these comments have been addressed, and the PR is rebased to master (getting rid of the merge conflict in the process), I'll get this merged!

Roasbeef · 2017-10-27T23:10:45Z

config.go

@@ -35,6 +35,7 @@ const (
 	defaultRPCHost            = "localhost"
 	defaultMaxPendingChannels = 1
 	defaultNumChanConfs       = 1
+	defaultTrickleDelay       = 10


10ms as a default trickle delay is far too aggressive. It should instead be within the range of 30 - 60 seconds. It was only set low before this PR as we needed snappiness within the integration tests. With this PR now, then a lower value (in the ms) is appropriate for integration test runs.

Roasbeef · 2017-10-27T23:11:38Z

discovery/gossiper.go

@@ -323,6 +323,112 @@ func (d *AuthenticatedGossiper) ProcessLocalAnnouncement(msg lnwire.Message,
 	return nMsg.err
 }

+// ChannelUpdateID is a unique identifier for ChannelUpdate messages, as


Minor nit: this doesn't need to be publicly exported, as it's only used for dedup purposes within the gossiper.

The same can be said for the Flags field within the struct as well.

Roasbeef · 2017-10-27T23:12:45Z

discovery/gossiper_test.go

+// TestDeDuplicatedAnnouncements ensures that the deDupedAnnouncements
+// struct properly stores and delivers the set of de-duplicated
+// announcements.
+func TestDeDuplicatedAnnouncements(t *testing.T) {


As this test is completely independant of the other tests, and doesn't share any global state it should be run in parallel. We can achieve this by adding t.Parallel() at the very top of this test. Thanks!

halseth · 2017-10-29T10:11:48Z

discovery/gossiper.go

+}
+
+// AddMsg adds a new message to the current batch.
+func (d *deDupedAnnouncements) AddMsg(message lnwire.Message) {


This method (and AddMsgs) should probably also not be exported, as the maps it accesses are not protected by mutexes, and it is only supposed to be used internally within the gossiper?

Functionally, it doesn't matter if the method is exported or not. The type that the method targets itself is un-exported. Within the codebase we have a general convention of exporting methods of a private struct to indicate that those methods are meant to be used freely.

A mutex isn't needed as this is only currently used within the main event loop of the gossiper, just as the prior slice used was never accessed concurrently.

Ah, I did not know that! :D

Roasbeef · 2017-10-29T20:31:14Z

discovery/gossiper.go

+	channelUpdates map[ChannelUpdateID]lnwire.Message
+
+	// nodeAnnouncements are identified by node id field.
+	nodeAnnouncements map[*btcec.PublicKey]lnwire.Message


It may not be immediately obvious, but this wouldn't properly de-duplicate the NodeAnnouncement for a particular public key if the NodeAnnouncement repeatedly added is a unique allocation each time. Using pointers as a key to a map is something that must be done carefully, otherwise the resulting behavior may not be what is expected.

Instead the key to the map should be a [33]byte, or just the vertex type which is already present within this package. A vertex can be created with a newVertex function taking a *btcec.PublicKey as its sole argument. The tests should also be updated in order to assert that when adding multiple node announcements each from distinct allocations (with the same advertising public key), then assert that each instance was properly de-duplicated

For Part 1 of Issue lightningnetwork#275. Create isolated private struct in networkHandler goroutine that will de-duplicate announcements added to the batch. The struct contains maps for each of channel announcements, channel updates, and node announcements to keep track of unique announcements. The struct has a Reset method to reset stored announcements, an AddMsg(lnwire.Message) method to add a new message to the current batch, and a Batch method to return the set of de-duplicated announcements. Also fix a few minor typos.

Add option to set trickleDelay for AuthenticatedGossiper in command line, with default value of 300 milliseconds. Pass this value to newServer, which uses it when creating a new instance of AuthenticatedGossiper. Also set this value to 300 milliseconds when creating nodes in integration tests.

Add tests for new deDupedAnnouncements struct in gossiper_test. Test the various functionalities of the struct - that empty struct contains no announcements, that announcements of each type can be added and properly de-duplicated, that the batch of announcements is delivered correctly, and that after reset the struct again contains no announcements.

Run go fmt so config file is formatted correctly. Also rename newVertex to NewVertex in pathfind_test and notifications_test as it is now exported from the routing package.

Roasbeef

LGTM 🤺

flaurida closed this Sep 8, 2017

flaurida reopened this Sep 8, 2017

flaurida force-pushed the 275 branch 5 times, most recently from efeeb3f to da5f37d Compare September 14, 2017 13:40

flaurida changed the title ~~create deDupedAnnouncements struct in networkHandler -#275~~ De-duplicate announcements in the AuthenticatedGossiper -#275 Sep 14, 2017

flaurida force-pushed the 275 branch 3 times, most recently from a9a107e to f882d62 Compare September 21, 2017 01:58

Roasbeef requested changes Sep 26, 2017

View reviewed changes

flaurida force-pushed the 275 branch from f882d62 to a7ac482 Compare October 18, 2017 02:11

Roasbeef requested changes Oct 27, 2017

View reviewed changes

halseth reviewed Oct 29, 2017

View reviewed changes

Roasbeef reviewed Oct 29, 2017

View reviewed changes

flaurida force-pushed the 275 branch 8 times, most recently from 79ea451 to 996444e Compare November 2, 2017 23:24

flaurida added 3 commits November 2, 2017 19:38

flaurida force-pushed the 275 branch 2 times, most recently from b93196d to e646e5e Compare November 3, 2017 21:52

config+pathfind_test+notifications_test: format and fix newVertex import

aa2f237

Run go fmt so config file is formatted correctly. Also rename newVertex to NewVertex in pathfind_test and notifications_test as it is now exported from the routing package.

flaurida force-pushed the 275 branch from e646e5e to aa2f237 Compare November 3, 2017 22:06

Roasbeef approved these changes Nov 16, 2017

View reviewed changes

Roasbeef merged commit 7408aa6 into lightningnetwork:master Nov 16, 2017

Roasbeef mentioned this pull request Nov 16, 2017

De-duplicate announcements in the AuthenticatedGossiper #275

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

De-duplicate announcements in the AuthenticatedGossiper -#275 #322

De-duplicate announcements in the AuthenticatedGossiper -#275 #322

flaurida commented Sep 8, 2017 •

edited

Roasbeef left a comment

Roasbeef Sep 25, 2017

Roasbeef Sep 25, 2017

Roasbeef Sep 25, 2017

Roasbeef Sep 25, 2017

Roasbeef Sep 25, 2017

Roasbeef Sep 26, 2017

Roasbeef Sep 26, 2017

Roasbeef left a comment •

edited

Roasbeef Oct 27, 2017

Roasbeef Oct 27, 2017

Roasbeef Oct 27, 2017

halseth Oct 29, 2017 •

edited

Roasbeef Oct 29, 2017

halseth Oct 30, 2017

Roasbeef Oct 29, 2017

Roasbeef left a comment

De-duplicate announcements in the AuthenticatedGossiper -#275 #322

De-duplicate announcements in the AuthenticatedGossiper -#275 #322

Conversation

flaurida commented Sep 8, 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

Choose a reason for hiding this comment

Roasbeef left a comment • 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

halseth Oct 29, 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

Roasbeef left a comment

Choose a reason for hiding this comment

flaurida commented Sep 8, 2017 •

edited

Roasbeef left a comment •

edited

halseth Oct 29, 2017 •

edited