htlcswitch: sync local payment hand-off to link #4183

joostjager · 2020-04-13T15:41:18Z

This PR converts the switch and link to synchronous hand-off of local payments.

Tried it out on the mpp send payment itest. To make sure that the async hand-off would lead to an extra payment attempt, I added a 100 ms sleep in the link before adding the htlc to the channel.

This is the original timing diagram, which shows the temp. chan failure originating from ourselves (@ 0)

With sync hand-off, it looks like this:

Fixes #4181

joostjager · 2020-04-13T15:42:15Z

~~Current problem is that the link balance isn't yet updated after channel.AddHtlc has been called.~~

Faulty analysis. Nothing wrong with calculating the balance, I just printed the wrong number.

joostjager · 2020-04-15T08:41:28Z

No attention paid to unit tests and itests yet. Looking for a concept ack.

joostjager · 2020-04-15T08:52:56Z

Wondering if there is a good reason to pass local sends through the event loop?

carlaKC

Of the options in # #4181, I prefer synchronous handoff to creating some workaround which tries to track pending balance deltas; that feels messy. There are always tradeoffs when making changes in the switch of course, but I think this change moves in the direction of less complexity, which I like a lot.

Not critical, but might also be worth thinking about how this would interact with an interceptor (eg #4018), since that is on the table for 0.11, if we do go ahead with this approach.

htlcswitch/switch.go

htlcswitch/interfaces.go

cfromknecht

Nice changes, concept ACK from me. Should be gg after addressing carla's comments

htlcswitch/link.go

cfromknecht · 2020-04-30T02:07:09Z

htlcswitch/link.go

+		hodlQueue:    queue.NewConcurrentQueue(10),
+		log:          build.NewPrefixLog(logPrefix, log),
+		quit:         make(chan struct{}),
+		downstreamIn: make(chan *downstreamMsg),


group with other downstream chnanel

the other downstream chan isn't initialized here

Crypt-iQ · 2020-05-02T20:11:31Z

Solid set of changes, the way you changed the switch was straightforward and actually reduced complexity. Before I read through the changes, I was worried there would be more complexity. LGTM upon travis pass

joostjager · 2020-05-06T14:01:07Z

Not critical, but might also be worth thinking about how this would interact with an interceptor (eg #4018), since that is on the table for 0.11, if we do go ahead with this approach.

@carlaKC I think the interceptor skips local payments.

joostjager · 2020-05-06T14:58:27Z

Todo: itest fails because link does not cancel back local htlcs when commitment is full

joostjager · 2020-05-13T15:46:51Z

itest is now stuck on sphinx_replay_persistence

Makes sense because it relies on the mailbox to simulate resending of a packet. Maybe it is an option to modify the test to use three hops.

Anyone suggestions on more direct ways to force a replay?

Move creation of the goroutine as a preparation for sync local routing

joostjager · 2020-05-14T10:10:13Z

Integration test fixed

htlcswitch/switch_test.go

carlaKC · 2020-05-18T09:16:49Z

htlcswitch/link.go

+	l.log.Tracef("received switch packet outkey=%v", pkt.outKey())
+
+	// Create a buffered result channel to prevent the link from blocking.
+	errChan := make(chan error, 1)


Are we buffering this to account for the case where the link gets a shutdown instruction and stops listening, or to account for the time between this function sending and starting to listen?

Yes, the former. If this function HandleLocalAddPacket quits, the link event loop can't deliver the message anymore.

htlcswitch/switch.go

Roasbeef

LGTM 📊

htlcswitch/link.go

Previously the forward(...) method was used in forwarding tests, while that code path isn't used for forwards in reality.

Embed forward method into SendHTLC and remove redundant type check.

There is no concurrent access in this code path, so there is no need to pass the call through the main event loop.

Deduplicate code and prepare for further split of handleDownstreamPkt.

Unroll common code to allow splitting in separate handlers per message type.

To be able to call just the UpdateAdd logic for synchronously handled local adds in a later commit.

This commit extends the link with a new synchronous delivery point for local UpdateAddHTLC messages. The switch method SendHTLC is updated to use this delivery point and thereby becomes a synchronous call. For MPP payments, synchronous hand-off is important. Otherwise the next pathfinding round could start without the channel balance updated yet.

Fixes a pre-existing issue where nil was returned when a failure had occurred during commiting of the circuit.

Align function name with the contained logic.

cfromknecht

LGTM! Nice to see those refactors bundled in here that make the logic overall more readable 🔥

carlaKC

Great change, really improves readability of this area. LGTM!

Roasbeef · 2022-05-03T19:27:34Z

Potential unintended consequence of this change: #6485

joostjager mentioned this pull request Apr 13, 2020

routing: pathfind with up-to-date bandwidth hints #4181

Closed

Roasbeef added this to the 0.11.0 milestone Apr 14, 2020

Roasbeef added bug fix htlcswitch routing v0.11 labels Apr 14, 2020

joostjager force-pushed the sync-local-payment branch 2 times, most recently from b7be1bb to 86a7a48 Compare April 15, 2020 08:40

joostjager requested review from carlaKC and cfromknecht April 15, 2020 08:41

joostjager marked this pull request as ready for review April 15, 2020 08:41

joostjager requested a review from Roasbeef as a code owner April 15, 2020 08:41

carlaKC reviewed Apr 15, 2020

View reviewed changes

htlcswitch/switch.go Outdated Show resolved Hide resolved

htlcswitch/switch.go Show resolved Hide resolved

htlcswitch/switch.go Outdated Show resolved Hide resolved

htlcswitch/interfaces.go Outdated Show resolved Hide resolved

joostjager removed the request for review from Roasbeef April 28, 2020 07:25

joostjager force-pushed the sync-local-payment branch from 86a7a48 to ab0cb46 Compare April 28, 2020 07:30

cfromknecht reviewed Apr 30, 2020

View reviewed changes

joostjager force-pushed the sync-local-payment branch 2 times, most recently from 2726fcd to 38e1a94 Compare May 6, 2020 14:44

cfromknecht self-requested a review May 6, 2020 17:56

cfromknecht mentioned this pull request May 8, 2020

First payment fails with FAILURE_REASON_TIMEOUT, subsequent succeds #4267

Closed

joostjager mentioned this pull request May 11, 2020

Intercept forward htlc #4018

Merged

htlcswitch: refactor handleLocalDispatch

3413083

Move creation of the goroutine as a preparation for sync local routing

joostjager force-pushed the sync-local-payment branch 3 times, most recently from 4893eca to ab2bb78 Compare May 14, 2020 09:25

joostjager requested review from carlaKC and Roasbeef May 14, 2020 10:10

carlaKC reviewed May 19, 2020

View reviewed changes

Roasbeef approved these changes May 20, 2020

View reviewed changes

htlcswitch/link.go Outdated Show resolved Hide resolved

joostjager force-pushed the sync-local-payment branch from ab2bb78 to fe96330 Compare May 20, 2020 05:11

joostjager requested a review from carlaKC May 20, 2020 05:11

joostjager added 9 commits May 20, 2020 08:03

switch/test: use external interface for testing

babb0a3

Previously the forward(...) method was used in forwarding tests, while that code path isn't used for forwards in reality.

htlcswitch: simplify local sends

fe35d35

Embed forward method into SendHTLC and remove redundant type check.

htlcswitch: bypass main loop for local sends

7afb43a

There is no concurrent access in this code path, so there is no need to pass the call through the main event loop.

htlcswitch: extract updateCommitTxOrFail in link

b559811

Deduplicate code and prepare for further split of handleDownstreamPkt.

htlcswitch: update commit tx per downstream msg type

55930df

Unroll common code to allow splitting in separate handlers per message type.

htlcswitch: extract handleDownstreamUpdateAdd in link

de2df56

To be able to call just the UpdateAdd logic for synchronously handled local adds in a later commit.

htlcswitch: return local add failure to caller

e00840e

Fixes a pre-existing issue where nil was returned when a failure had occurred during commiting of the circuit.

htlcswitch: rename to getLocalLink

8119dc7

Align function name with the contained logic.

joostjager force-pushed the sync-local-payment branch from fe96330 to 8119dc7 Compare May 20, 2020 06:04

cfromknecht approved these changes May 20, 2020

View reviewed changes

carlaKC approved these changes May 20, 2020

View reviewed changes

joostjager merged commit ca9c58a into lightningnetwork:master May 20, 2020

Roasbeef mentioned this pull request Jun 2, 2020

Cant pay through channel untill reconnect with this peer #4270

Closed

Roasbeef mentioned this pull request May 3, 2022

failing link: unable to update commitment: cannot add duplicate keystone with error: internal error #6485

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

htlcswitch: sync local payment hand-off to link #4183

htlcswitch: sync local payment hand-off to link #4183

joostjager commented Apr 13, 2020 •

edited

Loading

joostjager commented Apr 13, 2020 •

edited

Loading

joostjager commented Apr 15, 2020

joostjager commented Apr 15, 2020

carlaKC left a comment

cfromknecht left a comment

cfromknecht Apr 30, 2020

joostjager May 6, 2020

Crypt-iQ commented May 2, 2020

joostjager commented May 6, 2020

joostjager commented May 6, 2020

joostjager commented May 13, 2020 •

edited

Loading

joostjager commented May 14, 2020

carlaKC May 18, 2020

joostjager May 20, 2020

Roasbeef left a comment

cfromknecht left a comment

carlaKC left a comment

Roasbeef commented May 3, 2022

htlcswitch: sync local payment hand-off to link #4183

htlcswitch: sync local payment hand-off to link #4183

Conversation

joostjager commented Apr 13, 2020 • edited Loading

joostjager commented Apr 13, 2020 • edited Loading

joostjager commented Apr 15, 2020

joostjager commented Apr 15, 2020

carlaKC left a comment

Choose a reason for hiding this comment

cfromknecht left a comment

Choose a reason for hiding this comment

cfromknecht Apr 30, 2020

Choose a reason for hiding this comment

joostjager May 6, 2020

Choose a reason for hiding this comment

Crypt-iQ commented May 2, 2020

joostjager commented May 6, 2020

joostjager commented May 6, 2020

joostjager commented May 13, 2020 • edited Loading

joostjager commented May 14, 2020

carlaKC May 18, 2020

Choose a reason for hiding this comment

joostjager May 20, 2020

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

cfromknecht left a comment

Choose a reason for hiding this comment

carlaKC left a comment

Choose a reason for hiding this comment

Roasbeef commented May 3, 2022

joostjager commented Apr 13, 2020 •

edited

Loading

joostjager commented Apr 13, 2020 •

edited

Loading

joostjager commented May 13, 2020 •

edited

Loading