Implement the P2P discovery mechanism #51 #72

andrewshvv · 2016-11-15T16:43:09Z

In this PR as a stop-gap I have implemented the IRC discovery mechanism. The implementation include addition of standalone discover package. I tried to keep the IRC specific implementation as much decoupled from discovery service as possible, for that reason follow structures and interfaces was created:

Transport: interface is used to create an abstraction layer over system-dependable representation of discovery service messages (IRC). It gives the ability to discovery service operate only with final representation of messages like ChanAnnouncement, NodeAnnouncement thereby decouple this systems and give them ability to be easily testable.
IRCTransport: is an implementation of discovery message transport which uses IRC bot and IRC channels as carrier of announcement data. This structure keeps all IRC specific stuff in it.
DiscoveryService: is a service which serves as control layer over data transport. It operates with announce messages rather than transport dependable, raw data.

`Questions:`

How to retrieve the block height? (I think this topic should be discussed a little bit more than we do it in offline conversation, because I don't want create the mess of strange functions in BlockchainIO interface 😄)
How properly calculate the channel fees?

Roasbeef

Excellent work!

I really like the direction you've laid out in the discovery package. The abstractions are quite general and should be easy to extend and add more concrete implementations in the future. I'll also be purposing much of this code as I start to implement the framing and routing portions of BOLT0007.

However, most of my comments are centered around my sentiment that atm they're a bit too general. I think they can be reduced in scope a bit to be made more specific so we can regain access to the stronger compile time guarantees provided.

Roasbeef · 2016-12-01T06:44:09Z

discover/messages.go

+	Validate() error
+}
+
+var _ Validatable = (*NodeAnnouncement)(nil)


Minor nit: can you place a comment here describing the rationale for these lines (compile time interface adherence assertion).

Roasbeef · 2016-12-01T06:45:48Z

discover/messages.go

+	"github.com/roasbeef/btcutil"
+)
+
+type Validatable interface {


Missing a godoc comment here.

Roasbeef · 2016-12-01T06:47:49Z

discover/messages.go

+	SecondNodeSig []byte
+}
+
+func (a *ChannelAnnouncement) Validate() error {


Missing a godoc comment here.

This function should also be amended to calculate the two digests as dictated in BOLT0007, with the signature verification (4 signatures total) modified accordingly to verify against the two digests.

Roasbeef · 2016-12-01T07:41:10Z

discover/service.go

+// discovery service was stopped with error.
+func (s *DiscoveryService) Wait() chan error {
+	wait := make(chan error)
+	go func() {


I think this goroutine is unnecessary, and the Wait method on the WaitGroup can instead be called directly. As a result, the return value wouldn't be necessary.

Roasbeef · 2016-12-01T07:41:13Z

discover/service.go

+	}
+
+	s.err = err
+	s.quit <- true


In order to be consistent with the rest of the codebase, then channel should be closed rather than sent upon.

Roasbeef · 2016-12-01T23:17:55Z

discover/transport.go

+}
+
+func (t *IRCTransport) onDisconnected(conn *client.Conn, line *client.Line) {
+	t.quit <- true


Channel should be closed instead to be conformant with the rest of the codebase.

Roasbeef · 2016-12-01T23:24:07Z

discover/utils.go

+// createHandlerFunc takes the interface as input and checks that interface is
+// corresponding to the handler function with specific schema, if schema is
+// wrong or input isn't a function it panics.
+func createHandlerFunc(f interface{}) (string, Handler) {


In my opinion, this package currently over-uses reflection. Instead, (as I mentioned above) I think a more concrete interface should be used, with further type assertions to obtain concrete types usable depending on the transport.

Roasbeef · 2016-12-01T23:31:18Z

glide.yaml

@@ -56,3 +56,7 @@ import:
  version: ^1.1.0
 - package: github.com/aead/chacha20
 - package: github.com/go-errors/errors
+- package: github.com/fluffle/goirc
+- package: github.com/golang/mock


Why is the mock package necessary? I don't see it used anywhere in this PR.

Roasbeef · 2016-12-01T23:33:01Z

routing/manager.go

@@ -182,6 +200,8 @@ type RoutingManager struct {
 	ChDone    chan struct{}
 }

+var _ ChannelRouter = (*RoutingManager)(nil)


Can you please add a rationale comment here (the one I mentioned above a few times)? Thanks!

Roasbeef · 2016-12-01T23:33:59Z

server.go

@@ -490,6 +521,21 @@ func (s *server) Peers() []*peer {
 	return <-resp
 }

+// Channels returns a slice of all channels.
+func (s *server) Channels() ([]*channeldb.OpenChannel, error) {
+	respChan := make(chan []*channeldb.OpenChannel)


Since access to the database is already serialized, channeldb.FetchAllChannels() can be accessed directly.

Roasbeef

The above comment should be included in the change request ;)

andrewshvv · 2016-12-07T17:11:24Z

As we discussed earlier we moved from IRC discovery to P2P discovery and for that reason the previous implementation was significantly reduced.

Open issues:

after merge with last Roasbeef changes we should add TxIndex and BlockHeight during channel announcement without doing any btcd rpc calls.
additional spec signatures like node-signature-2,bitcoin-signature-1,bitcoin-signature-2 should be added in OpenChannel structure during channel opening process in order have ability properly announce the channel.

Roasbeef · 2016-12-09T06:55:06Z

discover/service.go

+//
+// NOTE: This MUST be run as a goroutine.
+func (s *DiscoveryService) announce() {
+	s.wg.Add(1)


The WaitGroup should be incremented before the goroutine is launched. So, within the Start method right before the announce goroutine is launched.

Roasbeef · 2016-12-09T06:58:31Z

discover/utils.go

+)
+
+func getRandomDelay(maxDelay int) time.Duration {
+	return time.Second * time.Duration(rand.Intn(maxDelay))


The prng is currently being used without seeding the source first. Somewhere in this package we need to add an init function that seeds the prng. So it would look something like:

func init() { rand.Seed(time.Now().UTC().UnixNano()) }

Nice catch! Thanks!

Roasbeef

LGTM!

Once the final two comments are addressed, and the PR is rebased to master, I'll merge this PR.

Roasbeef · 2016-12-12T02:17:35Z

glide.lock

@@ -53,6 +53,8 @@ imports:
  - spew
 - name: github.com/go-errors/errors
  version: a41850380601eeb43f4350f7d17c6bbd8944aaf8
+- name: github.com/go-errors/errors


This line is a duplication of the one above above ^

coveralls · 2017-03-19T21:08:58Z

Coverage increased (+0.2%) to 67.908% when pulling 4c015a79db200b3938b41d5e65dfeb2bcba973eb on AndrewSamokhvalov:add_irc_discovery into 5d43483 on lightningnetwork:master.

coveralls · 2017-03-19T21:12:11Z

Coverage increased (+0.2%) to 67.908% when pulling 4c015a79db200b3938b41d5e65dfeb2bcba973eb on AndrewSamokhvalov:add_irc_discovery into 5d43483 on lightningnetwork:master.

coveralls · 2017-03-19T21:13:47Z

Coverage increased (+0.2%) to 67.908% when pulling 4c015a79db200b3938b41d5e65dfeb2bcba973eb on AndrewSamokhvalov:add_irc_discovery into 5d43483 on lightningnetwork:master.

Roasbeef · 2017-03-21T23:23:05Z

channeldb/graph.go

@@ -1387,16 +1387,24 @@ func putChanEdgeInfo(edgeIndex *bolt.Bucket, edgeInfo *ChannelEdgeInfo, chanID [
 	}

 	authProof := edgeInfo.AuthProof
-	if err := wire.WriteVarBytes(&b, 0, authProof.NodeSig1.Serialize()); err != nil {
+	var nodeSig1, nodeSig2, bitcoinSig1, bitcoinSig2 []byte
+	if authProof != nil {


Hmm, I don't think this change is needed or really adds much. The signatures must be present within a ChannelEdgeInfo struct. Otherwise, we won't be able to properly serve them to the network.

As we discussed offline the signatures/proof might be equal to nil in the case if we don't want to announce the channel.

Roasbeef · 2017-03-21T23:23:36Z

channeldb/graph.go

@@ -1470,7 +1478,10 @@ func deserializeChanEdgeInfo(r io.Reader) (*ChannelEdgeInfo, error) {
 			return nil, err
 		}

-		return btcec.ParseSignature(sigBytes, btcec.S256())
+		if len(sigBytes) != 0 {


If the signature isn't present, then an error should occur rather than silently ignore the lack of signature data.

As far as I understand after discussion the nil signature does not designate something broken.

Roasbeef · 2017-03-21T23:23:59Z

discover/log.go

@@ -0,0 +1,72 @@
+package discover


Minor nit: the package name should be discovery rather than discover.

Roasbeef · 2017-03-21T23:29:45Z

discover/service.go

+}
+
+// Discovery is a subsystem which is responsible for:
+// * receiving announcements validate them and apply the changes to router.


These bullet points below should be formatted into a series of compete sentences.

Roasbeef · 2017-03-21T23:31:53Z

discover/service.go

+
+// ProcessRemoteAnnouncement sends a new remote announcement message along with
+// the peer that sent the routing message. The announcement will be processed then
+// added to a queue for batched tickled announcement to all connected peers.


tickled -> trickled

Roasbeef · 2017-03-22T00:11:51Z

routing/router.go

-				return false
+			if edge2Timestamp.After(msg.LastUpdate) ||
+				edge2Timestamp.Equal(msg.LastUpdate) {
+				return errors.Errorf("ignoring announcement "+


Style nit: a new-line should be inserted here.

Roasbeef · 2017-03-22T00:12:36Z

routing/router.go

-				chanID, err)
-			return false
+			return errors.Errorf("unable to fetch utxo for "+
+				"chan_id=%v: %v", msg.ChannelID, err)


With the discovery package handling full validation, this fragment can be removed all together as the ChannelRouter should only receive fully valid data.

I believe this comment should be addressed by the comment above.

Roasbeef · 2017-03-22T00:13:23Z

routing/router.go

-		log.Errorf("unable to sync edges w/ peer: %v", err)
-		return err
+	default:
+		return errors.Errorf("wrong  routing msg message type")


There's an extra space before routing here.

Roasbeef · 2017-03-22T00:13:41Z

routing/router.go

-				FeeProportionalMillionths: uint32(e2.FeeProportionalMillionths),
-			})
-		}
+		log.Infof("New channel msg applied: %v",


The log message was altered, it should be New channel update applied.

Roasbeef · 2017-03-22T00:14:31Z

routing/router.go

+
+// ForEachSelfEdgePolicy is used to iterate over all self channels info.
+// NOTE: Part of the Router interface.
+func (r *ChannelRouter) ForEachSelfEdgePolicy(cb func(c *channeldb.ChannelEdgePolicy) error) error {


Naming suggestion: ForAllOutgoingChannels.

coveralls · 2017-03-27T12:22:24Z

Coverage increased (+0.4%) to 69.428% when pulling 8b753c4e966401448aa12f461c395080932dda19 on AndrewSamokhvalov:add_irc_discovery into 505421d on lightningnetwork:master.

coveralls · 2017-03-27T12:23:35Z

Coverage increased (+0.4%) to 69.408% when pulling 8b753c4e966401448aa12f461c395080932dda19 on AndrewSamokhvalov:add_irc_discovery into 505421d on lightningnetwork:master.

coveralls · 2017-03-29T20:28:13Z

Coverage decreased (-0.2%) to 68.81% when pulling 4bf3ee6e290653ce4b54b422cf38c3a594493a90 on AndrewSamokhvalov:add_irc_discovery into c9c2848 on lightningnetwork:master.

coveralls · 2017-03-29T20:42:34Z

Coverage decreased (-0.1%) to 68.875% when pulling 70f325eb22d747fadfcf7f907b98312fb70cf084 on AndrewSamokhvalov:add_irc_discovery into c9c2848 on lightningnetwork:master.

coveralls · 2017-03-29T20:45:56Z

Coverage decreased (-0.1%) to 68.875% when pulling 70f325eb22d747fadfcf7f907b98312fb70cf084 on AndrewSamokhvalov:add_irc_discovery into c9c2848 on lightningnetwork:master.

coveralls · 2017-03-29T20:45:56Z

Coverage decreased (-0.1%) to 68.875% when pulling 70f325eb22d747fadfcf7f907b98312fb70cf084 on AndrewSamokhvalov:add_irc_discovery into c9c2848 on lightningnetwork:master.

coveralls · 2017-03-29T20:55:51Z

Coverage decreased (-0.1%) to 68.875% when pulling 70f325eb22d747fadfcf7f907b98312fb70cf084 on AndrewSamokhvalov:add_irc_discovery into c9c2848 on lightningnetwork:master.

In this commit the routing package was divided on two separete one, this was done because 'routing' package start take too much responsibily on themself, so with following commit: Routing pacakge: Enitites: * channeldb.ChannelEdge * channeldb.ChannelPolicy * channeldb.NodeLightning Responsibilities: * send topology notification * find payment paths * send payment * apply topology changes to the graph * prune graph * validate that funding point exist and corresponds to given one * to be the source of topology data Discovery package: Entities: * lnwire.AnnounceSignature * lnwire.ChannelAnnouncement * lnwire.NodeAnnouncement * lnwire.ChannelUpdateAnnouncement Responsibilities: * validate announcement signatures * sync topology with newly connected peers * handle the premature annoucement * redirect topology changes to the router susbsystem * broadcast announcement to the rest of the network * exchange channel announcement proofs Before that moment all that was in the 'routing' which is quite big for one subsystem. split

Add usage of the 'discovery' package in the lnd, now discovery service will be handle all lnwire announcement messages and send them to the remote party.

Validation of the announcements message has been removed and it will be added in the discovery package latter.

In this commit announcement signature message has been added which is needed when peers want to announce their channel to the rest of the network. This message acts as half proof carrier, nodes exchanges their half proofs with each other and after that they are able to construct the full proof.

Change the name of fields of messages which are belong to the discovery subsystem in a such way so they were the same with the names that are defined in the specification.

In case if the channel shouldn't be announced to the rest of the network the proof, which is needed to announce the channel, will not be populated, fot that reason the ability to store the empty proof has been added.

In order to properly announce the channel the announcements proofs should be persistent in boltdb.

Add check that the edge that was received really exist in the blockchain and that the announced funding keys and capcity corresponds to reality.

Originally we adding the edge without proof in order to able to use it for payments path constrcution. This method will allow us to populate the announcement proof after the exchange of the half proofs and constrcutrion of full proof finished.

Add the interaction between nodes of announce signature messages, which will allow us to exhcnage the half channel announcemen proofs later.

Added the signer which will be needed in the funding manager to sign the lnwaire announcement message before sending them to discovery package. Also in the future the message signer will be used to sign the users data.

Add validation functions and include validation checks in the annoncement process function.

coveralls · 2017-03-30T02:14:16Z

Coverage decreased (-0.1%) to 68.887% when pulling 11ef161 on AndrewSamokhvalov:add_irc_discovery into 0e96d27 on lightningnetwork:master.

coveralls · 2017-03-30T02:14:16Z

Coverage decreased (-0.1%) to 68.887% when pulling 11ef161 on AndrewSamokhvalov:add_irc_discovery into 0e96d27 on lightningnetwork:master.

Roasbeef

I've done some extensive testing locally and found that everything works as advertised. This PR really plugs up a major whole in our current implementation of the gossip protocol: actual validation!

With this we can also safely perform a "hard" reset on our current lnd testnet channel graph as all channels created prior to this commit will be rejected whole sale by upgraded nodes.

Thanks for bearing with my over this long review process! By merging this we knock out the oldest outstanding PR currently which is a nice bonus.

LGTM ⚡️

andrewshvv force-pushed the add_irc_discovery branch 2 times, most recently from 6646b92 to 9dc9fa5 Compare November 15, 2016 17:56

Roasbeef mentioned this pull request Nov 23, 2016

Implement the temporary IRC discovery mechanism #51

Closed

andrewshvv force-pushed the add_irc_discovery branch from 9dc9fa5 to 1b9591b Compare November 27, 2016 17:10

Roasbeef reviewed Dec 1, 2016

View reviewed changes

Roasbeef requested changes Dec 1, 2016

View reviewed changes

andrewshvv force-pushed the add_irc_discovery branch from 1b9591b to 3c94ea4 Compare December 7, 2016 16:35

andrewshvv changed the title ~~[WIP] Implement the temporary IRC discovery mechanism #51~~ Implement the P2P discovery mechanism #51 Dec 7, 2016

andrewshvv force-pushed the add_irc_discovery branch 2 times, most recently from 72764b7 to 29276a6 Compare December 7, 2016 17:01

Roasbeef reviewed Dec 12, 2016

View reviewed changes

Roasbeef approved these changes Dec 12, 2016

View reviewed changes

Roasbeef reviewed Dec 12, 2016

View reviewed changes

andrewshvv force-pushed the add_irc_discovery branch 3 times, most recently from 2828e1a to 8e01d93 Compare December 13, 2016 20:34

Roasbeef force-pushed the master branch 2 times, most recently from d86107c to a34df2f Compare February 8, 2017 04:54

andrewshvv force-pushed the add_irc_discovery branch 3 times, most recently from 33916f4 to 4c015a7 Compare March 19, 2017 21:08

Roasbeef requested changes Mar 22, 2017

View reviewed changes

andrewshvv force-pushed the add_irc_discovery branch 2 times, most recently from 140f0f8 to 8b753c4 Compare March 27, 2017 12:18

andrewshvv force-pushed the add_irc_discovery branch from 4bf3ee6 to 70f325e Compare March 29, 2017 20:38

andrewshvv force-pushed the add_irc_discovery branch 4 times, most recently from ed97ce4 to 57abb2a Compare March 30, 2017 01:52

Roasbeef force-pushed the master branch from cd54b55 to 0e96d27 Compare March 30, 2017 02:01

andrewshvv added 13 commits March 30, 2017 05:08

lnd: replace 'routing' with 'discovery' package

fe75fb2

Add usage of the 'discovery' package in the lnd, now discovery service will be handle all lnwire announcement messages and send them to the remote party.

lnwire: remove not working test

533239f

Validation of the announcements message has been removed and it will be added in the discovery package latter.

lnwire: converge discovery part of messages with specification

96b1c90

Change the name of fields of messages which are belong to the discovery subsystem in a such way so they were the same with the names that are defined in the specification.

channeldb: add ErrEdgeAlreadyExist error

7d89dc9

channeldb: add ability to store empty auth proof

858137a

In case if the channel shouldn't be announced to the rest of the network the proof, which is needed to announce the channel, will not be populated, fot that reason the ability to store the empty proof has been added.

channeldb: add storing of node signature and add edge signature

48ea232

In order to properly announce the channel the announcements proofs should be persistent in boltdb.

routing: add validation of utxo

18e082a

Add check that the edge that was received really exist in the blockchain and that the announced funding keys and capcity corresponds to reality.

routing+channeldb: add AddProof method

8afdd5f

Originally we adding the edge without proof in order to able to use it for payments path constrcution. This method will allow us to populate the announcement proof after the exchange of the half proofs and constrcutrion of full proof finished.

discovery+funding: add 'AnnounceSignature' proof exchange

ba7de62

Add the interaction between nodes of announce signature messages, which will allow us to exhcnage the half channel announcemen proofs later.

lnwallet: add message signer

8ffa66c

Added the signer which will be needed in the funding manager to sign the lnwaire announcement message before sending them to discovery package. Also in the future the message signer will be used to sign the users data.

discovery+funding: add validation of the announcement messages

11ef161

Add validation functions and include validation checks in the annoncement process function.

andrewshvv force-pushed the add_irc_discovery branch from 57abb2a to 11ef161 Compare March 30, 2017 02:08

Roasbeef approved these changes Mar 30, 2017

View reviewed changes

Roasbeef merged commit d4055d7 into lightningnetwork:master Mar 30, 2017

Implement the P2P discovery mechanism #51 #72

Implement the P2P discovery mechanism #51 #72

Conversation

andrewshvv commented Nov 15, 2016 • edited Loading

Questions:

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

Roasbeef left a comment

Choose a reason for hiding this comment

andrewshvv commented Dec 7, 2016

Roasbeef Dec 9, 2016 • edited Loading

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

Choose a reason for hiding this comment

coveralls commented Mar 19, 2017 • edited Loading

coveralls commented Mar 19, 2017

coveralls commented Mar 19, 2017 • edited Loading

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

coveralls commented Mar 27, 2017

coveralls commented Mar 27, 2017 • edited Loading

coveralls commented Mar 29, 2017 • edited Loading

coveralls commented Mar 29, 2017 • edited Loading

coveralls commented Mar 29, 2017

coveralls commented Mar 29, 2017

coveralls commented Mar 29, 2017

coveralls commented Mar 30, 2017

coveralls commented Mar 30, 2017

Roasbeef left a comment

Choose a reason for hiding this comment

andrewshvv commented Nov 15, 2016 •

edited

Loading

`Questions:`

Roasbeef Dec 9, 2016 •

edited

Loading

coveralls commented Mar 19, 2017 •

edited

Loading

coveralls commented Mar 19, 2017 •

edited

Loading

coveralls commented Mar 27, 2017 •

edited

Loading

coveralls commented Mar 29, 2017 •

edited

Loading

coveralls commented Mar 29, 2017 •

edited

Loading