From 36956d390fa77906cffb91eea50ac39403aad35e Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 5 Dec 2017 17:48:28 -0800 Subject: [PATCH 01/10] htlcswitch: add new method to the ChannelLink interface, EligibleToForward MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In this commit, we add a new method to the ChanneLink interface: EligibleToForward. This method allows a link to be added to the switch, but in an intermediate state which indicates that it isn’t yet ready to forward any incoming HTLC’s. --- htlcswitch/interfaces.go | 7 +++++++ htlcswitch/link.go | 8 ++++++++ htlcswitch/mock.go | 1 + 3 files changed, 16 insertions(+) diff --git a/htlcswitch/interfaces.go b/htlcswitch/interfaces.go index 8f1a48f7..6ba43466 100644 --- a/htlcswitch/interfaces.go +++ b/htlcswitch/interfaces.go @@ -85,6 +85,13 @@ type ChannelLink interface { // the channel link opened. Peer() Peer + // EligibleToForward returns a bool indicating if the channel is able + // to actively accept requests to forward HTLC's. A channel may be + // active, but not able to forward HTLC's if it hasn't yet finalized + // the pre-channel operation protocol with the remote peer. The switch + // will use this function in forwarding decisions accordingly. + EligibleToForward() bool + // Start/Stop are used to initiate the start/stop of the channel link // functioning. Start() error diff --git a/htlcswitch/link.go b/htlcswitch/link.go index bc0556cc..f96acec5 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -292,6 +292,14 @@ func (l *channelLink) Stop() { l.cfg.BlockEpochs.Cancel() } +// EligibleToForward returns a bool indicating if the channel is able to +// actively accept requests to forward HTLC's. We're able to forward HTLC's if +// we know the remote party's next revocation point. Otherwise, we can't +// initiate new channel state. +func (l *channelLink) EligibleToForward() bool { + return l.channel.RemoteNextRevocation() != nil +} + // sampleNetworkFee samples the current fee rate on the network to get into the // chain in a timely manner. The returned value is expressed in fee-per-kw, as // this is the native rate used when computing the fee for commitment diff --git a/htlcswitch/mock.go b/htlcswitch/mock.go index 8721ccb3..73e2c214 100644 --- a/htlcswitch/mock.go +++ b/htlcswitch/mock.go @@ -430,6 +430,7 @@ func (f *mockChannelLink) Bandwidth() lnwire.MilliSatoshi { return 99999999 func (f *mockChannelLink) Peer() Peer { return f.peer } func (f *mockChannelLink) Start() error { return nil } func (f *mockChannelLink) Stop() {} +func (f *mockChannelLink) EligibleToForward() bool { return true } var _ ChannelLink = (*mockChannelLink)(nil) From be1a96b78ae673e3b54ea179827a2bdf354a6aa2 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 5 Dec 2017 17:49:55 -0800 Subject: [PATCH 02/10] htlcswitch: ensure links are eligible to forward when selecting outgoing links MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In this commit, when selecting a candidate link to forward a payment, we’ll ensure that it’s actually able to take on the HTLC. Otherwise, we’ll skip over the link itself. Currently, a link is only fully eligible for forwarding, *after* we’ve received and fully processed the FundingLocked message. --- htlcswitch/switch.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/htlcswitch/switch.go b/htlcswitch/switch.go index dda6b911..5bc3d3cb 100644 --- a/htlcswitch/switch.go +++ b/htlcswitch/switch.go @@ -366,7 +366,9 @@ func (s *Switch) handleLocalDispatch(payment *pendingPayment, packet *htlcPacket ) for _, link := range links { bandwidth := link.Bandwidth() - if bandwidth > largestBandwidth { + if link.EligibleToForward() && + bandwidth > largestBandwidth { + largestBandwidth = bandwidth } @@ -488,7 +490,9 @@ func (s *Switch) handlePacketForward(packet *htlcPacket) error { // bandwidth. var destination ChannelLink for _, link := range interfaceLinks { - if link.Bandwidth() >= htlc.Amount { + if link.EligibleToForward() && + link.Bandwidth() >= htlc.Amount { + destination = link break } From 9a32bee17171dd8a3435cc66f5f7bf8db080f8aa Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 5 Dec 2017 17:51:06 -0800 Subject: [PATCH 03/10] rpc: a link is now only active if it is eligible to forward HTLCs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In this commit, we further constrain the candidacy for an “active” channel. In addition to being present within the link, it *must* also have the RemoteNextRevocation set. Otherwise, this indicates that we haven’t yet processed a FundingLocked message for this channel. --- rpcserver.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/rpcserver.go b/rpcserver.go index 0ebf06fc..f6c85783 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -1465,8 +1465,11 @@ func (r *rpcServer) ListChannels(ctx context.Context, channelID := lnwire.NewChanIDFromOutPoint(&chanPoint) var linkActive bool - if _, err := r.server.htlcSwitch.GetLink(channelID); err == nil { - linkActive = true + if link, err := r.server.htlcSwitch.GetLink(channelID); err == nil { + // A channel is only considered active if it is known + // by the switch *and* able to forward + // incoming/outgoing payments. + linkActive = link.EligibleToForward() } // As this is required for display purposes, we'll calculate From ddc5a0fc858556227eb7a1e35fb894a87ca8c003 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 5 Dec 2017 17:53:03 -0800 Subject: [PATCH 04/10] peer: unconditionally add a channel to the switch if it's open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In this commit we revert a prior change which was added after FundingLocked retransmission was implemented. This prior change didn’t factor in the fact that the FundingLocked message will *only* be re-sent after both sides receive the ChannelReestablishment message. With the prior code, as we never added the channel to the link, we’d never re-send the ChannelReestablishment, meaning the other side would never send the FundingLocked message. By unconditionally adding the channel to the switch, we ensure that we’ll always properly retransmit the FundingLocked message. --- peer.go | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/peer.go b/peer.go index 6df43af8..2a273a68 100644 --- a/peer.go +++ b/peer.go @@ -308,17 +308,6 @@ func (p *peer) loadActiveChannels(chans []*channeldb.OpenChannel) error { chanPoint := &dbChan.FundingOutpoint - // If the channel we read form disk has a nil next revocation - // key, then we'll skip loading this channel. We must do this - // as it doesn't yet have the needed items required to initiate - // a local state transition, or one triggered by forwarding an - // HTLC. - if lnChan.RemoteNextRevocation() == nil { - peerLog.Debugf("Skipping ChannelPoint(%v), lacking "+ - "next commit point", chanPoint) - continue - } - chanID := lnwire.NewChanIDFromOutPoint(chanPoint) p.activeChanMtx.Lock() From 3bc248e01c1ffaeb7be3a5fc057165d468f39445 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 5 Dec 2017 18:00:33 -0800 Subject: [PATCH 05/10] peer: properly process retransmitted FundingLocked message we've never processed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In this commit, we modify the logic within the channelManager to be able to process any retransmitted FundingLocked messages. Before this commit, we would simply ignore any new channels sent to us, iff, we already had an active channel with the same channel point. With the recent change to the loadActiveChannels method in the peer, this is now incorrect. When a peer retransmits the FundingLocked message, it goes through to the fundingManager. The fundingMgr will then (if we haven’t already processed it), send the channel to the breach arbiter and also to the peer’s channelManager. In order to handle this case properly, if we already have the channel, we’ll check if our current channel *doesn’t* already have the RemoteNextRevocation field set. If it doesn’t, then this means that we haven’t yet processed the FundingLcoked message, so we’ll process it for the first time. This new logic will properly: * ensure that the breachArbiter still has the most up to date channel * allow us to update the state of the link has been added to the switch at this point * this link will now be eligible for forwarding after this sequence --- peer.go | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/peer.go b/peer.go index 2a273a68..f00afccc 100644 --- a/peer.go +++ b/peer.go @@ -1171,13 +1171,46 @@ out: // Make sure this channel is not already active. p.activeChanMtx.Lock() - if _, ok := p.activeChannels[chanID]; ok { + if currentChan, ok := p.activeChannels[chanID]; ok { peerLog.Infof("Already have ChannelPoint(%v), "+ "ignoring.", chanPoint) + p.activeChanMtx.Unlock() close(newChanReq.done) newChanReq.channel.Stop() newChanReq.channel.CancelObserver() + + // We'll re-send our current channel to the + // breachArbiter to ensure that it has the most + // up to date version. + select { + case p.server.breachArbiter.newContracts <- currentChan: + case <-p.server.quit: + return + case <-p.quit: + return + } + + // If we're being sent a new channel, and our + // existing channel doesn't have the next + // revocation, then we need to update the + // current exsiting channel. + if currentChan.RemoteNextRevocation() != nil { + continue + } + + peerLog.Infof("Processing retransmitted "+ + "FundingLocked for ChannelPoint(%v)", + chanPoint) + + nextRevoke := newChan.RemoteNextRevocation() + err := currentChan.InitNextRevocation(nextRevoke) + if err != nil { + peerLog.Errorf("unable to init chan "+ + "revocation: %v", err) + continue + } + continue } From 7b10f5421604c50a007c80fd5fdd0dd725aa5e1e Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 6 Dec 2017 16:27:56 -0800 Subject: [PATCH 06/10] peer: in logWireMessage also unset curve field for lnwire.ChannelReestablish This will allow users to run in trace mode again, without having an excessive amount of spam in their logs. --- peer.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/peer.go b/peer.go index f00afccc..65f6106e 100644 --- a/peer.go +++ b/peer.go @@ -922,6 +922,8 @@ func (p *peer) logWireMessage(msg lnwire.Message, read bool) { })) switch m := msg.(type) { + case *lnwire.ChannelReestablish: + m.LocalUnrevokedCommitPoint.Curve = nil case *lnwire.RevokeAndAck: m.NextRevocationKey.Curve = nil case *lnwire.NodeAnnouncement: From 084d477ec30eb316f8a5575b7f8845ef24b00776 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 6 Dec 2017 16:29:35 -0800 Subject: [PATCH 07/10] peer: always load active channels upon connection reestablishment with peer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In this commit, we modify the logic within loadActiveChannels to *always* load a channel, even if it isn’t yet fully confirmed. With this change, we ensure that we’ll always send a channel_reestablish message upon reconnection. Fixes #458. --- peer.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/peer.go b/peer.go index 65f6106e..5e86945e 100644 --- a/peer.go +++ b/peer.go @@ -294,12 +294,6 @@ func (p *peer) Start() error { // channels returned by the database. func (p *peer) loadActiveChannels(chans []*channeldb.OpenChannel) error { for _, dbChan := range chans { - // If the channel isn't yet open, then we don't need to process - // it any further. - if dbChan.IsPending { - continue - } - lnChan, err := lnwallet.NewLightningChannel(p.server.cc.signer, p.server.cc.chainNotifier, p.server.cc.feeEstimator, dbChan) if err != nil { From 7960b5240f797f2c1d0b2addd3031b412ab4b2bd Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 6 Dec 2017 16:30:50 -0800 Subject: [PATCH 08/10] peer: when processing a msg, skip he funding barrier if it's a ChanSync message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit is a follow up to the prior commit: as it’s possible for the channel_reestablish message to be sent *before* the channel has been fully confirmed, we’ll now ensure that we process it to the link even if the channel isn’t yet open. --- peer.go | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/peer.go b/peer.go index 5e86945e..9a17497b 100644 --- a/peer.go +++ b/peer.go @@ -583,13 +583,25 @@ func newChanMsgStream(p *peer, cid lnwire.ChannelID) *msgStream { fmt.Sprintf("Update stream for ChannelID(%x) created", cid), fmt.Sprintf("Update stream for ChannelID(%x) exiting", cid), func(msg lnwire.Message) { - // We'll send a message to the funding manager and wait iff an - // active funding process for this channel hasn't yet completed. - // We do this in order to account for the following scenario: we - // send the funding locked message to the other side, they - // immediately send a channel update message, but we haven't yet - // sent the channel to the channelManager. - p.server.fundingMgr.waitUntilChannelOpen(cid) + _, isChanSycMsg := msg.(*lnwire.ChannelReestablish) + + // If this is the chanSync message, then we'll develri + // it imemdately to the active link. + if !isChanSycMsg { + // We'll send a message to the funding manager + // and wait iff an active funding process for + // this channel hasn't yet completed. We do + // this in order to account for the following + // scenario: we send the funding locked message + // to the other side, they immediately send a + // channel update message, but we haven't yet + // sent the channel to the channelManager. + peerLog.Infof("waiting on chan open to deliver: %v", + spew.Sdump(msg)) + p.server.fundingMgr.waitUntilChannelOpen(cid) + } + + // TODO(roasbeef): only wait if not chan sync // Dispatch the commitment update message to the proper active // goroutine dedicated to this channel. From d6dcc4276cc73f7c0b8f96f861f2eacab24b0517 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 6 Dec 2017 16:31:52 -0800 Subject: [PATCH 09/10] lnwallet: add new IsPending method to the channel state machine The IsPending method will allow callers to determine if a channel has been fully confirmed or not. --- lnwallet/channel.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 3a41944d..47c980ef 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -5115,3 +5115,12 @@ func (lc *LightningChannel) CommitFeeRate() btcutil.Amount { return lc.channelState.LocalCommitment.FeePerKw } + +// IsPending returns true if the channel's funding transaction has been fully +// confirmed, and false otherwise. +func (lc *LightningChannel) IsPending() bool { + lc.RLock() + defer lc.RUnlock() + + return lc.channelState.IsPending +} From 669c2ee1a044c6fb04f4d6215efd152875bdd5dc Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 6 Dec 2017 16:32:24 -0800 Subject: [PATCH 10/10] htlcswitch: only re-send FundingLocked if the channel is fully confirmed --- htlcswitch/link.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/htlcswitch/link.go b/htlcswitch/link.go index f96acec5..fc02d3a5 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -405,10 +405,12 @@ func (l *channelLink) htlcManager() { // this, as at this point we can't be sure if they've // really received the FundingLocked message. if remoteChanSyncMsg.NextLocalCommitHeight == 1 && - localChanSyncMsg.NextLocalCommitHeight == 1 { + localChanSyncMsg.NextLocalCommitHeight == 1 && + !l.channel.IsPending() { - log.Debugf("Resending fundingLocked message " + - "to peer") + log.Infof("ChannelPoint(%v): resending "+ + "FundingLocked message to peer", + l.channel.ChannelPoint()) nextRevocation, err := l.channel.NextRevocationKey() if err != nil { @@ -453,7 +455,7 @@ out: for { select { // A new block has arrived, we'll check the network fee to see - // if we should adjust our commitment fee , and also update our + // if we should adjust our commitment fee, and also update our // track of the best current height. case blockEpoch, ok := <-l.cfg.BlockEpochs.Epochs: if !ok {