[htlcswitch]: revert replace link, ensure removed links are stopped #1551

cfromknecht · 2018-07-13T01:00:27Z

This PR reverts the switch's current AddLink behavior, such that it rejects duplicate links with the same channel id. At the moment, we will hot swap in another link without fully shutting down the first, which is dangerous as the two can share a mailbox.

In a related commit, we changed RemoveLink such that it called go link.Stop(), and processed in the background to avoid deadlocking on the forwarding index mutex. The result of this is that it is possible for a prior Stop() to still be executing by the time a new link is added with the same channel id.

In order to recreate the behavior of replacing the old link with a new one, RemoveLink has been altered to return the removed link if it was found, and to not call/spawn link.Stop() at all. Instead, the caller is now responsible for stopping the link, which gets around the deadlocking w/in the switch.

Finally, though AddLink rejects duplicate links, we've modified the peer to first try and remove any links going by the same channel id. If one is found, we ensure that link.Stop() finishes before moving on to adding the link to the switch.

halseth

Concept ACK!

halseth · 2018-07-17T14:11:35Z

server.go

@@ -632,7 +632,11 @@ func newServer(listenAddrs []net.Addr, chanDB *channeldb.DB, cc *chainControl,
 		ChainIO:      cc.chainIO,
 		MarkLinkInactive: func(chanPoint wire.OutPoint) error {
 			chanID := lnwire.NewChanIDFromOutPoint(&chanPoint)
-			return s.htlcSwitch.RemoveLink(chanID)
+			link, err := s.htlcSwitch.RemoveLink(chanID)
+			if err == nil {


style nit: return early in case of error

halseth · 2018-07-17T14:12:37Z

peer.go

+	// down to ensure that the mailboxes are only ever under the control of
+	// one link.
+	oldLink, err := p.server.htlcSwitch.RemoveLink(link.ChanID())
+	if err == nil {


halseth · 2018-07-17T14:14:29Z

peer.go

-				quit:              p.quit,
+				channel: channel,
+				unregisterChannel: func(chanID lnwire.ChannelID) error {
+					_, err := p.server.htlcSwitch.RemoveLink(chanID)


link should not be stopped here?

halseth · 2018-07-17T14:15:00Z

peer.go

+		return nil
+
+	case err == htlcswitch.ErrChannelLinkNotFound:
+		peerLog.Warnf("unable remove channel link with "+


"unable to"

wpaulino · 2018-07-18T02:16:42Z

htlcswitch/switch.go

@@ -1851,7 +1853,7 @@ func (s *Switch) getLinkByShortID(chanID lnwire.ShortChannelID) (ChannelLink, er

 // RemoveLink is used to initiate the handling of the remove link command. The
 // request will be propagated/handled to/in the main goroutine.
-func (s *Switch) RemoveLink(chanID lnwire.ChannelID) error {
+func (s *Switch) RemoveLink(chanID lnwire.ChannelID) (ChannelLink, error) {


godoc should be updated to reflect that it is now the caller's responsibility to stop the link.

wpaulino · 2018-07-18T02:20:15Z

htlcswitch/link.go

@@ -431,6 +416,24 @@ func (l *channelLink) Start() error {
 			return fmt.Errorf("unable to trim circuits above "+
 				"local htlc index %d: %v", localHtlcIndex, err)
 		}
+
+		// Sine the link is live, before we start the link we'll update


nit: s/Sine/Since/

Roasbeef · 2018-07-27T14:30:44Z

htlcswitch/switch.go

+		// Now that all pending and live links have been removed from
+		// the forwarding indexes, stop each one before shutting down.
+		for _, link := range linksToStop {
+			link.Stop()


The danger here is that a link could be sending a message to the switch for forwarding. So this could result in a deadlock. This is why we typically stopped the link in a goroutine. We'll need to do a sweep to ensure that any time the link is trying to forward, we're able to reliably cause it to exit via a signal from the switch.

Fixed, we now thread through the link's quit channel into ForwardPackets so that calling Stop breaks and blocking forwarding calls

Roasbeef · 2018-07-27T14:31:17Z

htlcswitch/switch.go


-	return s.removeLink(chanID)
+	if link != nil {
+		link.Stop()


Similar comment here.

Roasbeef · 2018-07-27T14:32:21Z

htlcswitch/switch_test.go

-	if err := s.RemoveLink(chanID1); err != nil {
-		t.Fatalf("unable to remove alice link: %v", err)
-	}
+	s.RemoveLink(chanID1)


Why are we no longer checking the return value here?

The semantics are now such that it can't fail, it will block until all state related to a channel is removed or doesn't exist. The new function signature on RemoveLink doesn't return anything.

Roasbeef · 2018-07-27T14:33:43Z

server.go

-			srvrLog.Errorf("unable to remove channel link: %v",
-				err)
-		}
+		p.server.htlcSwitch.RemoveLink(link.ChanID())


Commit message looks to not actually match what's in the commit itself.

Not blocking, but still looks diff, it says:

server: use blocking RemoveLink to shutdown links

Yet it just now ignores the error.

RemoveLink doesn't return an error. It just blocks until everything is purged

Roasbeef · 2018-07-27T14:34:45Z

peer.go

@@ -556,6 +553,12 @@ func (p *peer) addLink(chanPoint *wire.OutPoint,

 	link := htlcswitch.NewChannelLink(linkCfg, lnChan)

+	// Before adding our new link, purge the switch of any pending or live


👍

I think we might also need this in the execution path from funding mgr -> channel manager in the peer.

Don't both pathways call addLink?

Yep, scratch that.

Roasbeef · 2018-07-27T14:37:11Z

htlcswitch/link.go

@@ -431,6 +416,24 @@ func (l *channelLink) Start() error {
 			return fmt.Errorf("unable to trim circuits above "+
 				"local htlc index %d: %v", localHtlcIndex, err)
 		}
+
+		// Since the link is live, before we start the link we'll update


Thoughts on making this synchronous instead of dispatching? As is, it's not guaranteed to register before making a state update

Roasbeef · 2018-07-27T14:37:38Z

htlcswitch/link_test.go

@@ -3725,7 +3725,6 @@ func (h *persistentLinkHarness) restart(restartSwitch bool,

 	// First, remove the link from the switch.
 	h.coreLink.cfg.Switch.RemoveLink(h.link.ChanID())
-	h.coreLink.WaitForShutdown()


Rational behind removing this?

new RemoveLink inherently blocks until shutdown

Its godoc should be updated to note this.

cfromknecht · 2018-07-30T21:37:05Z

Added a test to ensure that it is safe to call Stop(), even when forwarding incoming Adds (which must be done synchronously)

Roasbeef · 2018-08-10T03:55:56Z

Can be rebased now that #1668 is in!

This reverts commit e60d2b7.

current initialization methods

The new RemoveLink method blocks until the link has been fully stopped, so we no longer need to wait for it explicitly.

This commit adds a test that verifies Stop does not block if the link is concurrently forwarding incoming Adds to the switch. This test fails prior to the commits that thread through the link's quit channel.

In this commit, we thread through a link's quit channel into routeAsync, the primary helper method allowing links to send htlcPackets through the switch. This is intended to remove deadlocks from happening, where the link is synchronously blocking on forwarding packets to the switch, but also needs to shutdown.

cfromknecht · 2018-08-10T18:45:58Z

rebased, PTAL

Roasbeef

LGTM 🍕

Will run this on the faucet for a bit to attempt to flake out any regressions before merging into master.

Roasbeef · 2018-08-11T03:04:51Z

server.go

-			srvrLog.Errorf("unable to remove channel link: %v",
-				err)
-		}
+		p.server.htlcSwitch.RemoveLink(link.ChanID())


Not blocking, but still looks diff, it says:

server: use blocking RemoveLink to shutdown links

Yet it just now ignores the error.

Roasbeef · 2018-08-11T03:06:45Z

peer.go

@@ -556,6 +553,12 @@ func (p *peer) addLink(chanPoint *wire.OutPoint,

 	link := htlcswitch.NewChannelLink(linkCfg, lnChan)

+	// Before adding our new link, purge the switch of any pending or live


Yep, scratch that.

Roasbeef · 2018-08-11T03:06:58Z

htlcswitch/link.go

+	// switch, any failed packets will be returned to the provided
+	// ChannelLink. The link's quit signal should be provided to allow
+	// cancellation of forwarding during link shutdown.
+	ForwardPackets func(chan struct{}, ...*htlcPacket) chan error


Roasbeef · 2018-08-11T03:11:15Z

htlcswitch/link_test.go

+		bobSwitch := n.firstBobChannelLink.cfg.Switch
+		ticker := bobSwitch.cfg.LogEventTicker.(*ticker.Mock)
+		timeout := time.After(15 * time.Second)
+		for {


Excellent usage of the for/select idiom!

cfromknecht changed the title ~~[htlcswitch]: revert replace link, ensure removed links are stoped~~ [htlcswitch]: revert replace link, ensure removed links are stopped Jul 13, 2018

cfromknecht force-pushed the switch-revert-replace-link branch from c69203f to f09660b Compare July 13, 2018 01:01

cfromknecht added needs review PR needs review by regular contributors htlcswitch safety General label for issues/PRs related to the safety of using the software labels Jul 13, 2018

Roasbeef added P2 should be fixed if one has time bug fix labels Jul 13, 2018

cfromknecht force-pushed the switch-revert-replace-link branch from 2293ccf to 982b394 Compare July 17, 2018 01:32

Roasbeef requested a review from wpaulino July 17, 2018 06:25

Roasbeef added this to the 0.5 milestone Jul 17, 2018

halseth reviewed Jul 17, 2018

View reviewed changes

halseth added the needs testing PR hasn't yet been actively tested on testnet/mainnet label Jul 17, 2018

wpaulino reviewed Jul 18, 2018

View reviewed changes

wpaulino added first pass review done PR has had first pass of review, needs more tho and removed needs review PR needs review by regular contributors labels Jul 18, 2018

cfromknecht force-pushed the switch-revert-replace-link branch 4 times, most recently from 7836565 to 26732c4 Compare July 24, 2018 21:04

Roasbeef requested changes Jul 27, 2018

View reviewed changes

cfromknecht force-pushed the switch-revert-replace-link branch 4 times, most recently from ca941d8 to 0807d61 Compare July 30, 2018 21:35

cfromknecht force-pushed the switch-revert-replace-link branch 5 times, most recently from f78a8c3 to c8d624e Compare August 1, 2018 00:41

cfromknecht force-pushed the switch-revert-replace-link branch 9 times, most recently from f4bfa51 to 9d26e3c Compare August 9, 2018 09:33

cfromknecht removed the blocked label Aug 10, 2018

cfromknecht and others added 11 commits August 10, 2018 11:42

Revert "htlcswitch: in event of duplicate link add, prefer newer link"

1113684

This reverts commit e60d2b7.

htlcswitch/switch_test: update reverted test to use...

258019e

current initialization methods

htlcswitch/switch: synchronous stop of links after removal

c78e81d

server: use blocking RemoveLink to shutdown links

a6e7b35

peer: always purge link before adding new one

5f90973

htlcswitch/link: only update contract signals on start if live link

b507d26

htlcswitch/link_test: removes unnecessary WaitForShutdown

11f02de

The new RemoveLink method blocks until the link has been fully stopped, so we no longer need to wait for it explicitly.

chancloser: remove error returned from unregisterChannel

ad68009

htlcswitch/link_test: adds TestChannelLinkShutdownDuringForward

d08003b

This commit adds a test that verifies Stop does not block if the link is concurrently forwarding incoming Adds to the switch. This test fails prior to the commits that thread through the link's quit channel.

htlcswitch/link: pass link quit to ForwardPackets

0fef1c7

cfromknecht force-pushed the switch-revert-replace-link branch from 9d26e3c to f84cd14 Compare August 10, 2018 18:45

Roasbeef reviewed Aug 11, 2018

View reviewed changes

Roasbeef approved these changes Aug 11, 2018

View reviewed changes

Roasbeef merged commit 7a113d4 into lightningnetwork:master Aug 14, 2018

This was referenced Aug 19, 2018

[htlcswitch]: Relax Circuit Deletion #1748

Merged

channels won't go active due to some htlc things #1642

Closed

cfromknecht mentioned this pull request Nov 7, 2019

config+itest: allow unsafe disconnect by default #3648

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[htlcswitch]: revert replace link, ensure removed links are stopped #1551

[htlcswitch]: revert replace link, ensure removed links are stopped #1551

cfromknecht commented Jul 13, 2018 •

edited

Loading

halseth left a comment

halseth Jul 17, 2018

halseth Jul 17, 2018

halseth Jul 17, 2018

halseth Jul 17, 2018

wpaulino Jul 18, 2018

wpaulino Jul 18, 2018

Roasbeef Jul 27, 2018

cfromknecht Jul 30, 2018

Roasbeef Jul 27, 2018

Roasbeef Jul 27, 2018

cfromknecht Jul 30, 2018

Roasbeef Jul 27, 2018

cfromknecht Jul 30, 2018

Roasbeef Aug 11, 2018

cfromknecht Aug 11, 2018

Roasbeef Jul 27, 2018

cfromknecht Jul 30, 2018

Roasbeef Aug 11, 2018

Roasbeef Jul 27, 2018

cfromknecht Jul 30, 2018

Roasbeef Jul 27, 2018

cfromknecht Jul 30, 2018 •

edited

Loading

wpaulino Aug 3, 2018

cfromknecht Aug 4, 2018

cfromknecht commented Jul 30, 2018

Roasbeef commented Aug 10, 2018

cfromknecht commented Aug 10, 2018

Roasbeef left a comment

Roasbeef Aug 11, 2018

Roasbeef Aug 11, 2018

Roasbeef Aug 11, 2018

Roasbeef Aug 11, 2018

		@@ -556,6 +553,12 @@ func (p peer) addLink(chanPoint wire.OutPoint,

		link := htlcswitch.NewChannelLink(linkCfg, lnChan)

		// Before adding our new link, purge the switch of any pending or live

[htlcswitch]: revert replace link, ensure removed links are stopped #1551

[htlcswitch]: revert replace link, ensure removed links are stopped #1551

Conversation

cfromknecht commented Jul 13, 2018 • edited Loading

halseth 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

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 Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfromknecht commented Jul 30, 2018

Roasbeef commented Aug 10, 2018

cfromknecht commented Aug 10, 2018

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

cfromknecht commented Jul 13, 2018 •

edited

Loading

cfromknecht Jul 30, 2018 •

edited

Loading