[htlcnotifier 3/4]: Add HTLC Notifier #3781

carlaKC · 2019-12-02T07:23:40Z

This PR is change 3 of 4 proposed to add a htlc event notifier to lnd.
1: #3843
2: #3844
3: This PR
4: #3848

Since the PR is dependent on the preceding PRs, it has been rebased on #3844.
Only the last 9 commits are eligible for review.

This PR adds a htlcnotifier to the htlcswitch which provides a stream of notifications with the following types of events:

Forwards: when we forward a HTLC or initiate a send from our node
Forward Failures: when a HTLC we forwarded fails down the line
Settles: when a HTLC we forwarded is settled, or we receive a payment
Link Failures: when we fail a HTLC on our incoming or outgoing link

lntest/itest/lnd_test.go

joostjager

First pass done, I like the direction this is going. My two main thoughts:

Push the refactor of ForwardingError further. Get rid of ExtraMsg completely and make setting FailureDetail mandatory when instantiating the ForwardingError, possibly by creating a constructor function that takes both FailureMessage and FailureDetail. Otherwise it remains a bit loose and bugs may sneak in.
Check carefully whether the notification interface is consistent. I am a little worried about mixing up fail+settle (link level) with forwarding (two links involved), but maybe there is no reason to be worried. I think that going through the various scenarios and validating that the produced event(s) are logical will help (if you haven't already done so)

htlcswitch/switch.go

htlcswitch/failure.go

htlcswitch/htlcnotifier/htlcnotifier.go

htlcswitch/link.go

htlcswitch/htlcnotifier/htlcnotifier.go

carlaKC · 2019-12-02T11:45:09Z

Running through some different scenarios, as suggested by @joostjager, to make sure we can distinguish between different types of events.

✅ = Value set ; ❌ = Value not set

Forward Events: Ok

Forward Event	Incoming Key	Outgoing Key
Send	❌	✅
Forward	✅	✅

Forward Failure: Ok

Forward Event	Incoming Key	Outgoing Key
Send	✅	❌
Forward	✅	✅

Settle Event: Potential overlap

Settle Event	Incoming Key	Outgoing Key
Own Receive	✅	❌
Own Send	✅	❌
Forward	✅	✅

In the ideal case, we anything with a zero outgoing key belonging to us then check for matching forward events to determine whether the settle is for a send or receive. However, we will not have a forward event for sends which were forwarded before restart and settled afterwards.

Link Failures: Overlap

Link Failure	Incoming Key	Outgoing Key	IncomingFailed	Detail
Receive fails	✅	❌	true	Invoice specific
HODL Cancel	✅	❌	true	HODL Cancel
Send fails	❌	✅	false	failure specific
Fwd fails incoming	✅	❌	true	failure specific
Fwd Fails outgoing	✅	✅	false	failure specific

For this set of events, there is an overlap issue between forwarding failures which occur on the incoming link and failures paying our own invoices. Receives have a specific set of failure details, which we could use to distinguish, but that is not forward thinking because we may have a failure which is common for receives and forwards.

Solution:
The overlap issues here are mostly due to the inclusion of local sends/receives. A naive solution would be to match on payment hashes from listinvoices/payments, but that is clunky and not suitable in the case of reused payment hashes. Given that we aren't guaranteed to have forward events for local sends (which differentiate them from receives), I would say that a type field with forward, send and receive would be the simplest way to go? This will solve all the overlap issues that the current event structs have.

htlcswitch/switch_test.go

joostjager · 2019-12-02T12:56:46Z

With regards to the event sequences, would it make sense to have only a single event per htlc as it is handled by our system? So a forward will generate just a single even, even though there are an incoming and an outgoing link involved. Or is that what you suggest with the send/receive/fwd event type?

carlaKC · 2019-12-03T07:52:03Z

With regards to the event sequences, would it make sense to have only a single event per htlc as it is handled by our system? So a forward will generate just a single even, even though there are an incoming and an outgoing link involved.

As is we have a single event for a forward, which has the information for the incoming and outgoing links in HTLCInfo. Running through scenarios is a great idea, this is how events will be generated for the scenarios you've provided:

Incoming exit hop htlc rejected
Link failure {Incoming link key, rejection reason, incomingLink = true}

Incoming exit hop htlc settled
Settle {Incoming link key, payment hash}

Incoming exit hop htlc accepted but not yet resolved (hodl invoice), then settled
No event on acceptance
Settle {Incoming link key, payment hash}

Incoming exit hop htlc accepted but not yet resolved (hodl invoice), then rejected
No event on acceptance
Link failure {Incoming link key, fail detail = hodlCancel, incomingLink = true}

Outgoing origin htlc (our payment) not handed out (didn't even leave the link)
Link failure {Incoming & outgoing link key, failure detail, incomingLink = false}

Outgoing origin htlc (our payment) failed back by peer
Forward {Incoming & outgoing key, payment hash}
Forward Failure {Incoming & outgoing key, payment hash}

Outgoing origin htlc (our payment) settled
Forward {Incoming & outgoing key, payment hash}
Settle {Incoming link key, payment hash} + TBD: would set type = receive

Incoming htlc to forward not accepted (assuming this is on incoming link?):
Link failure {Incoming key, fail detail, incomingLink = true}

Incoming htlc to forward accepted, but rejected by outgoing link
Link failure {Incoming key, fail detail, incomingLink = false}

Htlc forwarded, but failed back by downstream peer
Forward {Incoming & outgoing key, payment hash}
Forward Failure {Incoming & outgoing key, payment hash}

Htlc forwarded and settled
Forward {Incoming & outgoing key, payment hash}
Settle {Incoming link key, payment hash}

Or is that what you suggest with the send/receive/fwd event type?

A type enum would go in the settle events to distinguish our own sends being settled from our receives (since they have the same data right now). Unrelated to the four main event types. I will update the PR with this today, which will hopefully clarify what I mean by it.

joostjager · 2019-12-03T10:58:19Z

Ok, nice overview. I see that there are multiple events fired for a single forward, one for the forward-going path and one for the backward path. This is also the case for an outgoing origin htlc. For incoming there is only one event. Maybe it is more consistent to always do two events and cover the hodl invoice accept too? So one event that describes what is requested from us by the htlc metadata and the second event what happened to the request.

Btw, don't forget the on-chain resolution flow where htlc decision are made too.

htlcswitch/htlcnotifier/htlcnotifier.go

htlcswitch/link.go

htlcswitch/failure.go

htlcswitch/htlcnotifier/htlcnotifier.go

htlcswitch/link.go

carlaKC · 2019-12-04T12:50:06Z

PR needs some work adding on chain events, not ready for review right now.

manreo · 2019-12-08T23:09:51Z

as specified here: #3420

carlaKC · 2019-12-11T09:51:30Z

make setting FailureDetail mandatory when instantiating the ForwardingError, possibly by creating a constructor function that takes both FailureMessage and FailureDetail. Otherwise it remains a bit loose and bugs may sneak in.

This is a little tricky because the point of FailureDetail was for it to be optional (otherwise we end up with duplicate messages to those in lnwire), and there are some legacy cases where we don't even set a failure message (SphinxErrorDecrypter.DecryptError).

One approach for this would be to implement an internal error interface htlcSwitch.linkError which will map to wire messages with somegetWireMessage which does not implement lnwire.FailureMessage itself, so there are no accidental overlaps. You suggested something of this sort in an earlier comment?

Check carefully whether the notification interface is consistent.

Updated in this version. There is some ambiguity for certain events (eg an incoming link failure for a forward and an invoice look the same), so I have gone with a event type enum which indicates whether an event is linked to a send/receive/forward. This makes it clear to the user, and is easily implemented because forwards and our own sends/receives are mostly handled by different code.

carlaKC · 2019-12-11T11:13:46Z

Requesting review on this, although I suspect there's more discussion to be had around ForwardingError, so after a conceptual pass at this stage rather than in depth review.

htlcswitch/htlcnotifier.go

joostjager · 2020-02-17T08:14:55Z

htlcswitch/htlcnotifier.go

+		HtlcKey:   newHtlcKey(pkt),
+		HtlcInfo:  newHtlcInfo(pkt, hash, getEventType(pkt)),
+		Timestamp: time.Now(),
+	}


I think it is much cleaner now that you keep the packet in the link/switch.

htlcswitch/htlcnotifier.go

htlcswitch/link.go

joostjager · 2020-02-17T08:24:15Z

htlcswitch/switch_test.go

+		}
+
+		assertCircuit(t, zeroCircuit, settle.IncomingCircuit)
+		assertCircuit(t, aliceToBob, settle.OutgoingCircuit)


Some method extraction is possible to reduce duplication

joostjager · 2020-02-17T08:26:06Z

htlcswitch/switch_test.go

+// checkSuccessful is a function that checks that Alice, Bob and Carol
+// have the correct set of event for a payment that is successfully
+// routed over Alice -> Bob -> Carol.
+func checkSuccessful(t *testing.T, alice, bob, carol <-chan interface{},


There is quite a bit of assertion code here. Can't this test be written by just having three (one for every node) lists of fully-specified events that are expected? The actual events are then just read from the channels and deep-equaled with the expectations.

Switched over to a set of results, still quite a lot of setup code, but I think it's better. Less duplication at least.

joostjager · 2020-02-19T07:14:03Z

htlcswitch/htlcnotifier.go

+// SettleEvent represents a htlc that was settled.
+type SettleEvent struct {
+	// HtlcKey uniquely identifies the htlc.
+	HtlcKey


No HtlcInfo here?

joostjager · 2020-02-19T07:14:49Z

htlcswitch/htlcnotifier.go

+// because errors returned down the route are encrypted.
+type ForwardingFailEvent struct {
+	// HtlcKey uniquely identifies the htlc.
+	HtlcKey


No HtlcInfo here?

Wasnt't it also suggested earlier to merge HlcJey and HtlcInfo?

joostjager · 2020-02-19T07:17:08Z

htlcswitch/switch.go

+	case *lnwire.UpdateFailHTLC:
+		s.cfg.HtlcNotifier.NotifyForwardingFailEvent(key, eventType)
+	}
+


joostjager · 2020-02-19T07:19:08Z

htlcswitch/switch_test.go

+			HtlcEventType: HtlcEventTypeReceive,
+			Timestamp:     ts,
+		},
+	}


Nice set of expectations

halseth

echoing @joostjager's question about HtlcKey/HtlcInfo, otherwise this looks good to me now :=

halseth · 2020-02-19T10:18:08Z

htlcswitch/htlcnotifier.go

+// because errors returned down the route are encrypted.
+type ForwardingFailEvent struct {
+	// HtlcKey uniquely identifies the htlc.
+	HtlcKey


Wasnt't it also suggested earlier to merge HlcJey and HtlcInfo?

halseth · 2020-02-19T10:24:31Z

htlcswitch/switch.go

-		incomingHTLCID: packet.incomingHTLCID,
-		circuit:        packet.circuit,
-		linkFailure:    failure,
+		sourceRef:       packet.sourceRef,


nit: this change could be made its own commit with an explaining commit message. Reason being this is touching existing code, so it would be nice (IMO) to see it in isolation to deem whether it is safe.

halseth · 2020-02-19T10:27:39Z

htlcswitch/switch.go

@@ -811,6 +782,47 @@ func (s *Switch) handleLocalDispatch(pkt *htlcPacket) error {
 	return nil
 }

+// handleLocalAddHTLC handles the addition of a htlc for a send that


nice to do code moves in separate commits 👍

htlcswitch/switch_test.go

halseth · 2020-02-19T10:33:18Z

htlcswitch/test_utils.go

+	carol *HtlcNotifier) serverOption {
+
+	return func(aliceServer, bobServer, carolServer *mockServer) {
+		aliceServer.htlcSwitch.cfg.HtlcNotifier = alice


could we add the htlc notifier by default when creating the mockServers? It should not interfere with existing test cases,

This gets a bit more involved, because we then need to pass mocked time into newThreeHopNetwork (which would probably need to be with functional options like this anyway, so all the other calls don't need to be updated), thread the notifier down to initSwithcWithDB (more functional opts so we don't need to update every caller) and either expand the htlcNotifier interface to include Start/Stop/SubscribeEvents or do a type assertion in the test to get the event subscription. If it's not a blocker, I think I'd rather leave it like this?

carlaKC · 2020-02-19T11:30:22Z

Some clarification on HtlcKey/HtlcInfo: we do not have the information we need for HtlcInfo reliably around for settles/fails, particularly on startup so I thought it would be better to split them out and exclude them entirely from settle and fail events. I see this being used in rate limiting/ performance metrics rather than exact accounting of htlc movement. If people need to look up additional information for events, they can look in forwarding log/payments/invoices.

This commit adds a HTLCNotifier to htlcswitch which HTLC events will be piped through to provide clients with subscriptions to HTLC level events. The event types added are forward events (which occur for sends from and forwards through our node), forward failues (when a send or forward fails down the line), settles for forwards or receives to our node and link failures which occur when a htlc is failed at our node (which may occur for a send, receive or foreward).

In this commit, a htlcNotifier interface is added to allow for easy unit testing. Instances of the HtlcNotifier are added to the server, switch and link.

This commit sets more fields on the htlcPacket created to fail adding a htlc packet to the switch for notification purposes. This new data is copied by value from the original packet. The packet is then failed back to the peer that forwarded us the packet, which is handled by handledownstream packet. The values added to the packet are not used in the handling of a failed packet.

This commit adds notifications for htlcs which are forwarded through our node. Forwards are notified when the htlc is added on our ougoing link, settles when we send a settle message to the downstream peer. If a failure occurs, we check whether it occurred at our node, then notify a link or forwarding failure accordingly. Note that this change also adds forward event notifications for sends which are initiated by our node because the handling code for adding a htlc which originates from our node is the same as that for handling forwards. Htlcs for our locally initiated sends have our internal pid set in the incoming htlcs id field, so we extract this value and notify with a zero htlc id to be consistent with receives (which have zero outgoing circuits). Subsequent settles or failures are not noitfied for local sends in this commit, and will be handled in a follow up.

Split handleLocalDispatch into an extra handleLocalAddHTLC function so we can easily notify an error should one occur adding the htlc.

Notify link failures for our own payments. Separate handling code is required for local payment link failures because we do not pass these failures back through the switch (like we do for link failures for forwards), but rather send them straight back to the router. Our own sends have the payment ID saved in the incoming htlc ID of the packet's incoming circuit. This change replaces that value with for the sake of consistent notifying of sends and receives from our node.

Add notifications for local initiated sends settles and forwarding failures. As with link failures, local send settles and forwarding failures are reported directly to the router so must have their own notification handling.

This commit adds link failure notifications for failures which occur on our incoming link. These failures may be receives which we failed or forwards which we could not parse.

joostjager

It has been quite an evolution to design the event structure and get access to all the required data in a clean way, but I think it was worth it. Not only are those events exposed now, but also technical debt was cleaned up along the way. That will benefit further development in this area.

Final question: did you try running a three hop regtest network and sending some payments back and forth manually with log on trace? As a rough sanity check to see that the log messages make sense.

halseth

Very happy about the final outcome of this PR, I agree that the extra effort put into it was worth it in the end. LGTM ✅

carlaKC · 2020-02-20T12:33:36Z

Thank you for all the review @halseth and @joostjager! The saga is almost complete 🎢

Final question: did you try running a three hop regtest network and sending some payments back and forth manually with log on trace? As a rough sanity check to see that the log messages make sense.

Indeed, I have a very rich local forwarding node after all the iterations of this PR.

Send from Alice -> Bob -> Carol looks like this in logs:

Alice:

Notifying forward event: send over (Chan ID=6160:1:1, HTLC ID=4), outgoing amount: 2000 mSAT, outgoing timelock: 6260

Notifying settle event: send over (Chan ID=6160:1:1, HTLC ID=4)

Bob:

Notifying forward event: forward over (Chan ID=6160:1:1, HTLC ID=4) -> (Chan ID=6172:1:1, HTLC ID=0), incoming amount: 2000 mSAT, incoming timelock: 6260, outgoing amount: 1000 mSAT, outgoing timelock: 6220

Notifying settle event: forward over (Chan ID=6160:1:1, HTLC ID=4) -> (Chan ID=6172:1:1, HTLC ID=0)

Carol:

Notifying settle event: receive over (Chan ID=6172:1:1, HTLC ID=0)

carlaKC requested review from cfromknecht and Roasbeef as code owners December 2, 2019 07:23

carlaKC added htlcswitch rpc Related to the RPC interface v0.9.0 labels Dec 2, 2019

carlaKC mentioned this pull request Dec 2, 2019

[WIP]: HTLC Forwarder Events #3639

Closed

carlaKC added HTLC P3 might get fixed, nice to have labels Dec 2, 2019

carlaKC removed the request for review from cfromknecht December 2, 2019 07:26

carlaKC commented Dec 2, 2019

View reviewed changes

lntest/itest/lnd_test.go Outdated Show resolved Hide resolved

carlaKC requested review from halseth and joostjager December 2, 2019 08:24

joostjager reviewed Dec 2, 2019

View reviewed changes

htlcswitch/switch.go Outdated Show resolved Hide resolved

htlcswitch/failure.go Outdated Show resolved Hide resolved

htlcswitch/htlcnotifier/htlcnotifier.go Outdated Show resolved Hide resolved

htlcswitch/link.go Outdated Show resolved Hide resolved

joostjager reviewed Dec 2, 2019

View reviewed changes

htlcswitch/htlcnotifier/htlcnotifier.go Outdated Show resolved Hide resolved

carlaKC commented Dec 2, 2019

View reviewed changes

htlcswitch/switch_test.go Outdated Show resolved Hide resolved

halseth suggested changes Dec 3, 2019

View reviewed changes

joostjager added this to the 0.9.0 milestone Dec 3, 2019

carlaKC force-pushed the 3420-htlcForwarding branch from 0f81309 to 49a2f90 Compare December 8, 2019 15:38

carlaKC force-pushed the 3420-htlcForwarding branch from 49a2f90 to daf27ce Compare December 11, 2019 07:21

carlaKC requested review from joostjager and halseth December 11, 2019 11:13

carlaKC requested a review from joostjager February 14, 2020 14:38

joostjager reviewed Feb 17, 2020

View reviewed changes

carlaKC force-pushed the 3420-htlcForwarding branch 3 times, most recently from dc28cdf to 5bd23a2 Compare February 18, 2020 17:13

carlaKC requested a review from joostjager February 18, 2020 19:12

joostjager reviewed Feb 19, 2020

View reviewed changes

halseth reviewed Feb 19, 2020

View reviewed changes

carlaKC added 10 commits February 19, 2020 17:50

multi: add htlcNotifier interface to switch and link

abf780b

In this commit, a htlcNotifier interface is added to allow for easy unit testing. Instances of the HtlcNotifier are added to the server, switch and link.

htlcswitch: refactor handleLocalDispatch

937062b

Split handleLocalDispatch into an extra handleLocalAddHTLC function so we can easily notify an error should one occur adding the htlc.

htlcswitch: notify send forward failures and settles

22d370a

Add notifications for local initiated sends settles and forwarding failures. As with link failures, local send settles and forwarding failures are reported directly to the router so must have their own notification handling.

htlcswitch: notify incoming link failures

6bd0de2

This commit adds link failure notifications for failures which occur on our incoming link. These failures may be receives which we failed or forwards which we could not parse.

htlcswitch: notify local receive settles

71fdd75

htlcswitch: add htlc notifier test

c0a4923

carlaKC force-pushed the 3420-htlcForwarding branch from 5bd23a2 to c0a4923 Compare February 20, 2020 07:37

carlaKC requested review from halseth and joostjager February 20, 2020 07:44

joostjager approved these changes Feb 20, 2020

View reviewed changes

halseth approved these changes Feb 20, 2020

View reviewed changes

halseth mentioned this pull request Feb 20, 2020

Intercept forward htlc #4018

Merged

carlaKC merged commit 80af295 into lightningnetwork:master Feb 20, 2020

joostjager mentioned this pull request Oct 10, 2022

htlcswitch: final settle signal #6517

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[htlcnotifier 3/4]: Add HTLC Notifier #3781

[htlcnotifier 3/4]: Add HTLC Notifier #3781

carlaKC commented Dec 2, 2019 •

edited

Loading

joostjager left a comment

carlaKC commented Dec 2, 2019

joostjager commented Dec 2, 2019

carlaKC commented Dec 3, 2019

joostjager commented Dec 3, 2019

carlaKC commented Dec 4, 2019

manreo commented Dec 8, 2019

carlaKC commented Dec 11, 2019

carlaKC commented Dec 11, 2019

joostjager Feb 17, 2020

joostjager Feb 17, 2020

joostjager Feb 17, 2020

carlaKC Feb 18, 2020

joostjager Feb 19, 2020

joostjager Feb 19, 2020

halseth Feb 19, 2020

joostjager Feb 19, 2020

joostjager Feb 19, 2020

halseth left a comment

halseth Feb 19, 2020

halseth Feb 19, 2020

halseth Feb 19, 2020

halseth Feb 19, 2020

carlaKC Feb 20, 2020

carlaKC commented Feb 19, 2020

joostjager left a comment

halseth left a comment

carlaKC commented Feb 20, 2020

[htlcnotifier 3/4]: Add HTLC Notifier #3781

[htlcnotifier 3/4]: Add HTLC Notifier #3781

Conversation

carlaKC commented Dec 2, 2019 • edited Loading

joostjager left a comment

Choose a reason for hiding this comment

carlaKC commented Dec 2, 2019

joostjager commented Dec 2, 2019

carlaKC commented Dec 3, 2019

joostjager commented Dec 3, 2019

carlaKC commented Dec 4, 2019

manreo commented Dec 8, 2019

carlaKC commented Dec 11, 2019

carlaKC commented Dec 11, 2019

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

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

carlaKC commented Feb 19, 2020

joostjager left a comment

Choose a reason for hiding this comment

halseth left a comment

Choose a reason for hiding this comment

carlaKC commented Feb 20, 2020

carlaKC commented Dec 2, 2019 •

edited

Loading