Add retransmission subsystem in channel link #231

andrewshvv · 2017-07-10T14:04:21Z

ask Laolu about counter persistence, and add it.
check the generateRevocation function and make sure that we are passing right indexes.

coveralls · 2017-07-10T14:12:02Z

Coverage decreased (-0.7%) to 73.298% when pulling 49aa23a263861a1a9c116ecdbac36406fd9284cf on AndrewSamokhvalov:retransmission_in_channellink into 2e6800e on lightningnetwork:master.

coveralls · 2017-07-11T18:51:07Z

Coverage decreased (-0.5%) to 73.52% when pulling e2eaf0cddaa4d720d7443941f54cb43d7bc3b937 on AndrewSamokhvalov:retransmission_in_channellink into 2e6800e on lightningnetwork:master.

coveralls · 2017-07-11T20:43:03Z

Coverage decreased (-0.5%) to 73.528% when pulling ade906540d38e3d4bbd360d47db0b2c8ed7819c5 on AndrewSamokhvalov:retransmission_in_channellink into 2e6800e on lightningnetwork:master.

…dust Note that this commit is temporary, and should be reverted once #231 is merged. The reason we need to do this for now, is that we don’t properly track the exact state of the remote party’s commitment. In this test case, the resulting HTLC’s added are dust to one party, but non-dust to another. So upon restart, the states (balance wise) has diverged.

Roasbeef

Thanks for this PR! Looking forward to finally getting in proper channel message retransmission into our next major release.

The set of changes in this diff are very clean, however they're incomplete. You'll find most of the details within the body of the review, but at a high level, the following is missing:

We need to track the delta between the prior state, and the state we created for the remote party. This is required as we must retransmit all update_* messages, not only just the HTLC additions.
We also need to store the exact state of the current commitment chain tail for the remote party. This is required as due to the possibility of asymmetric dust thresholds for both parties, upon restart, it's possible for our commitment states to have diverged.

Roasbeef · 2017-08-02T03:46:19Z

htlcswitch/link_test.go

@@ -48,23 +46,92 @@ func messageToString(msg lnwire.Message) string {
 	return spew.Sdump(msg)
 }

+// expectedMessage struct hols the message which travels from one peer to


hols -> holds

Roasbeef · 2017-08-02T03:47:18Z

htlcswitch/link_test.go

 			// Skip logging of extend revocation window messages.
 			switch m := m.(type) {
 			case *lnwire.RevokeAndAck:
 				var zeroHash chainhash.Hash
 				if bytes.Equal(zeroHash[:], m.Revocation[:]) {
-					return
+					return false, nil


The above fragment can now be removed as of the latest master, as there is no longer a revocation window to be extended.

Roasbeef · 2017-08-02T03:48:52Z

htlcswitch/link_test.go

 // createLogFunc is a helper function which returns the function which will be
 // used for logging message are received from another peer.
 func createLogFunc(name string, channelID lnwire.ChannelID) messageInterceptor {
-	return func(m lnwire.Message) {
-		if getChanID(m) == channelID {
+	return func(m lnwire.Message) (bool, error) {


What's the significance of the first parameter? The godoc comment on this function should be modified to detail it.

Roasbeef · 2017-08-02T03:52:55Z

htlcswitch/link_test.go

+			}
+
+			expectedMessage := expectToReceive[0]
+			expectToReceive = expectToReceive[1:]


The element at expectToReceive[0] should be set to nil, otherwise we risk a memory leak.

Roasbeef · 2017-08-02T03:55:03Z

htlcswitch/mock.go


 // Record is used to set the function which will be triggered when new
 // lnwire message was received.
-func (s *mockServer) record(f messageInterceptor) {
-	s.recordFuncs = append(s.recordFuncs, f)
+func (s *mockServer) intersect(f messageInterceptor) {


The godoc comment above wasn't updated.

Also I think you meant to call this function intercept instead?

Roasbeef · 2017-08-02T04:28:29Z

htlcswitch/link.go

-		logCommitTimer:    time.NewTimer(300 * time.Millisecond),
-		overflowQueue:     newWaitingQueue(),
-		quit:              make(chan struct{}),
+		cfg:            cfg,


Can you reference #194 in the commit body? This commit resolves the issue encountered, thanks!

Roasbeef · 2017-08-02T04:30:55Z

htlcswitch/switch.go

 //
 // NOTE: This MUST be run as a goroutine.
-func (s *Switch) htlcForwarder() {
-	defer s.wg.Done()
+func (s *Switch) handleControl() {


What deadlock? Can you add some detail to the commit's message explaining the scenario that caused you to make this change? Thanks!

Roasbeef · 2017-08-02T04:31:01Z

htlcswitch/switch.go

-// down active onion routed payments. Each active channel is modeled as
-// networked device with metadata such as the available payment bandwidth, and
-// total link capacity.
+// handleControl...


Missing a godoc comment here.

Roasbeef · 2017-08-02T04:31:48Z

htlcswitch/switch.go

+		case req := <-s.linkControl:
+			switch cmd := req.(type) {
+			case *addLinkCmd:
+				cmd.err <- s.addLink(cmd.link)


It seems that we'll need to introduce mutexes internally for this new set of changes to be concurrent safe.

Roasbeef · 2017-08-02T04:32:53Z

htlcswitch/switch.go

@@ -751,6 +751,7 @@ func (s *Switch) addLink(link ChannelLink) error {
 	s.interfaceIndex[peerPub][link] = struct{}{}

 	if err := link.Start(); err != nil {
+		s.removeLink(link.ChanID())


Nice catch!

In this commit the reestablish message have been added, which serves as channel state synchronization message. Before exchanging the messages for particular channel peers have to send it to each other as the first message in order to be sure that non of the updates have been lost because of the previous disconnect.

Add message interceptor which checks the order and may skip the messages which were denoted to be skipeed.

This commit where added as a measure to avoid the panic during several server simultanoius fault. The panic happened becuase *t.Testing structure is not concurrent safe.

Add js-like future object which might be used to wait for the response to be received or return the error otherwise.

In order to be able to properly restart switch several times we should have the sequential process of channel link stop. In other words if we stopped the switch we should be sure that all channel links have been stopped too. Addition of the goroutine during the force close was added because of the deadlock: Trace: 1. link:force_close_notification 2. link:wipe_channel 3. peer:switch_remove_link 4. switch:stop_link 5. link:wait <-- deadlock

In this commit BOLT№2 retranmission logic for the channel link have been added. Now if channel link have been initialised with the 'SyncState' field than it will send the lnwire.ChannelReestablish message and will be waiting for receiving the same message from remote side. Exchange of this message allow both sides understand which updates they should exchange with each other in order sync their states.

After addition of the channel reestablish message exchange we couldn't use the Bandwidth() function, at least in the test framework.

After addition of the retransmission logic in the channel link, we should make the onion blobs persistant, the proper way to do this is include the onion blobs in the payment descriptor rather than storing them in the distinct struct in the channel link.

In this commit htlc channeldb representation have been augmented with onion blob field, and (de)serialisaion functions have been changed to make the onion blob persistant.

Roasbeef · 2017-11-16T00:20:16Z

Closing at this has been taken over and fully implemented.

andrewshvv force-pushed the retransmission_in_channellink branch 2 times, most recently from 0584e7e to e2eaf0c Compare July 11, 2017 18:44

andrewshvv changed the title ~~Add retransmission subsytem in channell ink~~ Add retransmission subsystem in channel link Jul 11, 2017

andrewshvv force-pushed the retransmission_in_channellink branch 2 times, most recently from 122bd8f to ade9065 Compare July 11, 2017 19:56

Roasbeef mentioned this pull request Jul 12, 2017

BOLT#2: Add message retransmission sub-system #156

Closed

Roasbeef requested changes Aug 2, 2017

View reviewed changes

andrewshvv added 15 commits August 11, 2017 15:42

htlcswitch.test: add message interceptor handler

5bf25c0

Add message interceptor which checks the order and may skip the messages which were denoted to be skipeed.

htlcswitch.test: add server error channel to concurrent access panic

c132199

This commit where added as a measure to avoid the panic during several server simultanoius fault. The panic happened becuase *t.Testing structure is not concurrent safe.

htlcswicth.test: add 'future' payment response

ea1efd0

Add js-like future object which might be used to wait for the response to be received or return the error otherwise.

htlcswicth: fix deadlock

4387e5e

After addition of the channel reestablish message exchange we couldn't use the Bandwidth() function, at least in the test framework.

htlcswitch: fix clean of channel link

61812b4

htlcswitch+channel: remove cancel reasons from channel link

0a5acb1

htlcswitch+lnwallet: add malformed payment descriptor

85eb0e4

channeldb: add persistance of the onion blob

1435b25

In this commit htlc channeldb representation have been augmented with onion blob field, and (de)serialisaion functions have been changed to make the onion blob persistant.

build: move check of the ports to integration tests

54ac366

channeldb+lnwallet: add remote pending commiment persistance

402e204

not finished index persistence

ab9d847

andrewshvv force-pushed the retransmission_in_channellink branch from ade9065 to ab9d847 Compare August 15, 2017 17:09

Roasbeef mentioned this pull request Aug 30, 2017

Re-send FundingLocked in ChannelLink upon re-connect if at state zero #303

Closed

3 tasks

halseth mentioned this pull request Sep 13, 2017

FundingLocked improvements #328

Merged

Roasbeef closed this Nov 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add retransmission subsystem in channel link #231

Add retransmission subsystem in channel link #231

andrewshvv commented Jul 10, 2017

coveralls commented Jul 10, 2017

coveralls commented Jul 11, 2017

coveralls commented Jul 11, 2017 •

edited

Loading

Roasbeef left a comment

Roasbeef Aug 2, 2017

Roasbeef Aug 2, 2017

Roasbeef Aug 2, 2017

Roasbeef Aug 2, 2017

Roasbeef Aug 2, 2017

Roasbeef Aug 2, 2017

Roasbeef Aug 2, 2017

Roasbeef Aug 2, 2017

Roasbeef Aug 2, 2017

Roasbeef Aug 2, 2017

Roasbeef commented Nov 16, 2017

Add retransmission subsystem in channel link #231

Add retransmission subsystem in channel link #231

Conversation

andrewshvv commented Jul 10, 2017

coveralls commented Jul 10, 2017

coveralls commented Jul 11, 2017

coveralls commented Jul 11, 2017 • edited Loading

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

Roasbeef commented Nov 16, 2017

coveralls commented Jul 11, 2017 •

edited

Loading