fundingmanager: send messages directly to peers #1505

wpaulino · 2018-07-06T03:44:12Z

In this PR, we introduce a similar optimization to the one done in #1345. Currently, each call to server.SendToPeer acquires the server's mutex to obtain the active peer. We work around this by passing the peer struct itself within the internal fundingManager messages.

Fixes #1495.

cfromknecht · 2018-07-06T19:45:10Z

discovery/gossiper.go

@@ -2399,11 +2399,11 @@ func (d *AuthenticatedGossiper) sendAnnSigReliably(
 				"to peer(%x): %v. Will retry when online.",
 				remotePeer.SerializeCompressed(), err)

-			connected := make(chan struct{})
-			d.cfg.NotifyWhenOnline(remotePeer, connected)
+			peerChan := make(chan lnpeer.Peer)


buffer this as well?

cfromknecht · 2018-07-06T19:49:09Z

fundingmanager_test.go

+	f.cfg.NotifyWhenOnline = func(peer *btcec.PublicKey,
+		connectedChan chan<- lnpeer.Peer) {
+
+		go func() {


will these tests pass w/o the goroutine now that we're buffering?

Yep! Fixed.

cfromknecht · 2018-07-06T19:50:11Z

peer.go

+
+// AddNewChannel adds a new channel to the peer. The channel should fail to be
+// added if the fundingQuit channel is closed.
+func (p *peer) AddNewChannel(channel *lnwallet.LightningChannel,


cfromknecht · 2018-07-06T19:51:01Z

discovery/gossiper.go

@@ -107,7 +107,7 @@ type Config struct {
 	// NotifyWhenOnline is a function that allows the gossiper to be
 	// notified when a certain peer comes online, allowing it to
 	// retry sending a peer message.
-	NotifyWhenOnline func(peer *btcec.PublicKey, connectedChan chan<- struct{})
+	NotifyWhenOnline func(peer *btcec.PublicKey, peerChan chan<- lnpeer.Peer)


Perhaps make a note in the godoc that peerChan MUST be buffered

cfromknecht

Awesome! Fairly familiar with this PR as spent a good amount of time working through it w/ @wpaulino. LGTM, but would appreciate feedback from others.

halseth

Did a first pass, and like what I see! I dig the commit format you have been using lately (small, independent commits), makes reviewing a joy!

Main request I have is to try to simplify the logic sending to the peer when it might not be online, avoiding the nil parameter.

halseth · 2018-07-11T13:16:56Z

peer.go

-				p.server.fundingMgr.processFundingError(msg, p.addr)
+			case p.server.fundingMgr.IsPendingChannel(
+				msg.ChanID, p.addr.IdentityKey,
+			):


hm, weird format? Maybe just do key := addr.IdentityKey to keep on one line.

halseth · 2018-07-11T13:31:54Z

fundingmanager.go

+	lnpeer.Peer
+
+	// AddNewChannel adds a new channel to the peer. The channel should fail
+	// to be added if the fundingQuit channel is closed.


I think cancel would be a better name for the chan.

halseth · 2018-07-11T13:33:07Z

peer.go

@@ -223,6 +223,9 @@ func newPeer(conn net.Conn, connReq *connmgr.ConnReq, server *server,
 	return p, nil
 }

+// A compile-time check to ensure that peer satisfies the FundingPeer interface.
+var _ FundingPeer = (*peer)(nil)


Hm, this has the unfortunate effect of keeping the dependency cycle peer<->fundingmanager. Why not make AddNewChannel part of the regular Peer interface?

halseth · 2018-07-11T13:43:11Z

fundingmanager.go

@@ -630,7 +633,9 @@ func (f *fundingManager) Start() error {
 			go func(dbChan *channeldb.OpenChannel) {
 				defer f.wg.Done()

-				err := f.handleFundingConfirmation(dbChan, shortChanID)
+				err := f.handleFundingConfirmation(
+					nil, dbChan, shortChanID,


not sure if passing nil here is the best option, can make it easy to make errors down the line. Instead, can you fetch the peer (NotifyWhenOnline ) right before it is used, or make sure that it will never be passed in as nil?

Changed it so that we fetch the peer before handleFundingConfirmation.

halseth · 2018-07-11T13:43:45Z

fundingmanager.go

@@ -337,7 +337,7 @@ type fundingConfig struct {
 	// the channel to the ChainArbitrator so it can watch for any on-chain
 	// events related to the channel. We also provide the address of the
 	// node we're establishing a channel with for reconnection purposes.
-	WatchNewChannel func(*channeldb.OpenChannel, *lnwire.NetAddress) error
+	WatchNewChannel func(*channeldb.OpenChannel, *btcec.PublicKey) error


nit: update comment

halseth · 2018-07-11T13:44:25Z

discovery/gossiper.go

@@ -9,7 +9,7 @@ import (
 	"sync/atomic"
 	"time"

-	"github.com/coreos/bbolt"
+	bolt "github.com/coreos/bbolt"


why this change?

Had to add it because it wouldn't compile otherwise. Although it no longer seems to be a problem...

halseth · 2018-07-11T13:45:21Z

fundingmanager.go

+				fndgLog.Warnf("Unable to send fundingLocked "+
+					"to peer %x: %v",
+					peerKey.SerializeCompressed(), err)
+			} else {


Can peer be nil here? If not, this can be written

err = sendMessage(..) if err == nil { break } print(err) ... go on ...

EDIT: see other comment about nil peer

halseth · 2018-07-16T10:36:32Z

peer.go

+	// before we close the channel barrier corresponding to the channel.
+	select {
+	case <-newChanDone:
+	case <-cancel:


Not sure if cancelling at this point will actually stop the channel from being added to the peer?

Yeah, not much we can do after the request has already been sent. Just ended up removing this instead as this wasn't being handled previously anyway.

halseth · 2018-07-16T10:37:33Z

peer.go

-			// handle the error.
-			case p.server.fundingMgr.IsPendingChannel(msg.ChanID, p.addr):
-				p.server.fundingMgr.processFundingError(msg, p.addr)
+				// If the channel ID for the error message corresponds


halseth · 2018-07-16T10:42:50Z

fundingmanager.go

+				f.cfg.NotifyWhenOnline(ch.IdentityPub, peerChan)
+
+				select {
+				case peer := <-peerChan:


Can do

var peer lnpeer.Peer select { case peer = <-peerChan: case <-f.quit: return } err := f.handleFundingConfirmation ....

halseth · 2018-07-16T10:42:59Z

fundingmanager.go

+				f.cfg.NotifyWhenOnline(dbChan.IdentityPub, peerChan)
+
+				select {
+				case peer := <-peerChan:


same as above

halseth · 2018-07-16T10:48:03Z

fundingmanager.go

-		return
-	case <-newChanDone: // Fallthrough if we're not quitting.
-	}
+	fmsg.peer.AddNewChannel(channel, f.quit)


Does it make sense to let this return an error that we can log here?

halseth · 2018-07-16T10:49:09Z

fundingmanager_test.go

+	// sender. Since the fundingManager now has a reference to peers itself,
+	// alice.sendMessage will be triggered when Bob's funding manager
+	// attempts to send a message to Alice and vice versa.
+	alice.remotePeer = bob


cfromknecht · 2018-07-19T01:53:56Z

needs rebase

In this commit, we modify the existing message sending functionality within the fundingmanager. Due to each mesage send requiring to hold the server's lock to retrieve the peer, we might run into a case where the lock is held for a larger than usual amount of time and would therefore block on sending the message within the fundingmanager. We remedy this by taking a similar approach to some recent changes within the gossiper. We now keep track of each peer within the internal fundingmanager messages and send messages directly to them.

callbacks The FindPeer and SendToPeer callbacks are no longer needed within the fundingManager due to the previous commit allowing us to send messages to peers directly.

wpaulino · 2018-07-19T19:47:16Z

Rebased.

Roasbeef

LGTM 🏌🏾

Really dig the incremental step towards removing all references to *peer within the main package all together!

wpaulino force-pushed the fundingmanager-send-peer-directly branch from f97bf6e to 08ea215 Compare July 6, 2018 19:27

cfromknecht suggested changes Jul 6, 2018

View reviewed changes

wpaulino force-pushed the fundingmanager-send-peer-directly branch from 08ea215 to 4a7b000 Compare July 7, 2018 01:23

cfromknecht reviewed Jul 7, 2018

View reviewed changes

Roasbeef requested a review from halseth July 7, 2018 02:48

Roasbeef added P2 should be fixed if one has time needs review PR needs review by regular contributors needs testing PR hasn't yet been actively tested on testnet/mainnet first pass review done PR has had first pass of review, needs more tho labels Jul 11, 2018

halseth suggested changes Jul 11, 2018

View reviewed changes

wpaulino force-pushed the fundingmanager-send-peer-directly branch from 4a7b000 to a8a9e26 Compare July 12, 2018 01:53

Roasbeef added funding Related to the opening of new channels with funding transactions on the blockchain server and removed needs review PR needs review by regular contributors labels Jul 13, 2018

halseth suggested changes Jul 16, 2018

View reviewed changes

Roasbeef mentioned this pull request Jul 16, 2018

lncli hangs opening channel #1388

Closed

wpaulino force-pushed the fundingmanager-send-peer-directly branch from a8a9e26 to 6f618f0 Compare July 16, 2018 21:20

Roasbeef added this to the 0.5 milestone Jul 17, 2018

wpaulino added 11 commits July 19, 2018 12:32

fundingmanager: return ErrFundingManagerShuttingDown on shutdown

8c92df0

lnpeer: extend interface Address and AddNewChannel methods

77d2853

multi: satisfy new lnpeer interface

3ab1706

fundingmanager+lnd: modify WatchNewChannel callback to take in peer key

6b1982f

fundingmanager+peer: modify fundingErrorMsg to use peer key

6504a9c

server: modify NotifyWhenOnline to return the peer once connected

04c5eba

discovery/gossiper: update to latest NotifyWhenOnline changes

6d4da72

fundingmanager: retrieve peer before resuming funding flow

e669e90

fundingmanager+lnd: remove no longer needed FindPeer and SendToPeer

38e01b2

callbacks The FindPeer and SendToPeer callbacks are no longer needed within the fundingManager due to the previous commit allowing us to send messages to peers directly.

fundingmanager: update tests to latest changes

d54d41e

wpaulino force-pushed the fundingmanager-send-peer-directly branch from 6f618f0 to d54d41e Compare July 19, 2018 19:34

wpaulino mentioned this pull request Jul 20, 2018

discovery/gossiper: reliably send channel update msg to remote peer #1595

Merged

Roasbeef approved these changes Jul 21, 2018

View reviewed changes

Roasbeef merged commit 271db7d into lightningnetwork:master Jul 21, 2018

wpaulino deleted the fundingmanager-send-peer-directly branch July 21, 2018 00:51

halseth mentioned this pull request Sep 24, 2019

Fundingmanager state machine cleanup #1910

Merged

fundingmanager: send messages directly to peers #1505

fundingmanager: send messages directly to peers #1505

Conversation

wpaulino commented Jul 6, 2018 • 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

cfromknecht left a comment

Choose a reason for hiding this comment

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

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

cfromknecht commented Jul 19, 2018

wpaulino commented Jul 19, 2018

Roasbeef left a comment

Choose a reason for hiding this comment

wpaulino commented Jul 6, 2018 •

edited

Loading

halseth left a comment •

edited

Loading