[htlcnotifier 4/4]: lnrpc: Add HTLC Event Subscription #3848

carlaKC · 2019-12-17T15:29:50Z

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

This PR adds a htlcnotifier subscription to routerrpc:

We can either present wire message + detail separately (current approach), or combine them into one error code (I think this isn't great for adding more details, permutations go up).

Fixes #3420

lnrpc/routerrpc/subscribe_events.go

carlaKC · 2020-03-10T08:43:20Z

Rebased to address some merge conflicts, ready for a look @cfromknecht @joostjager :)

joostjager

Only a few questions about quitting left.

Tested using https://github.com/carlaKC/lndwrap to monitor a three node network. Looks good 👍

joostjager · 2020-03-10T11:10:50Z

lnrpc/routerrpc/router_server.go

@@ -167,6 +170,7 @@ func (s *Server) Start() error {
 //
 // NOTE: This is part of the lnrpc.SubServer interface.
 func (s *Server) Stop() error {
+	close(s.quit)


Does this need a waitgroup too, to wait for completion?

Offline discussion about this, no reason for subscriptions not to be wait grouped (I thought there could be some grpc magic in the background). Added wait group for routerrpc subserver + trackPayment.

do we want to add sync.Once here? o/w calling Stop() multiple times will panic

lnrpc/routerrpc/router_server.go

lnrpc/routerrpc/router.proto

lnrpc/routerrpc/router_server.go

joostjager · 2020-03-10T19:10:38Z

lnrpc/routerrpc/router_server.go

@@ -449,6 +454,9 @@ func (s *Server) trackPayment(paymentHash lntypes.Hash,
 		}
 	}

+	s.wg.Add(1)


Still this feels a bit strange. Normally we add to the waitgroup right before we start the goroutine. In this case, grpc started the goroutine already, so are we too late here? I don't know the answer really, but maybe it doesn't make sense after all to do this?

On the router server level, adding this wait group means that we don't consider the server to be stopped before all the subscriptions have received the quit signal. So I think that adds something, although all the other subservers are shutting down fine without it, so also unsure.

cfromknecht · 2020-03-12T22:41:19Z

lnrpc/routerrpc/router.proto

+
+    // The amount of the outgoing htlc.
+    uint64 outgoing_amt_msat = 4;
+}


is there a reason for not exposing payment hash? perhaps i'm just forgetting something that was already decided on this front

Yeah we discussed it in the previous notifier PRs, wanted to force people towards identifying htlcs by incoming/outgoing circuit keys rather than payment hash, since it's not a unique identifier. If it's there, chances are people with use it as a uuid. It's specifically a concern here since forwards need to be matched with settles/fails, and doing so with payment hash could go very wrong.

makes sense, i think that's a reasonable assumption 👍

lnrpc/routerrpc/subscribe_events.go

lnrpc/routerrpc/router_server.go

cfromknecht · 2020-03-12T23:09:45Z

lnrpc/routerrpc/router_server.go

@@ -167,6 +170,7 @@ func (s *Server) Start() error {
 //
 // NOTE: This is part of the lnrpc.SubServer interface.
 func (s *Server) Stop() error {
+	close(s.quit)


do we want to add sync.Once here? o/w calling Stop() multiple times will panic

Add SubscribeHtlcEvents to RouterBackend in preparation for the addition of a HtlcNotifier stream.

carlaKC · 2020-03-13T07:58:03Z

@joostjager re-requesting to make sure approval still stands. Removed wait group (see comment) and converted timestamps to ns.

joostjager · 2020-03-13T08:40:44Z

Yes approval still stands.

cfromknecht

LGTM 🏄‍♂️

cfromknecht · 2020-03-17T19:24:09Z

lnrpc/routerrpc/router.proto

+
+    // The amount of the outgoing htlc.
+    uint64 outgoing_amt_msat = 4;
+}


makes sense, i think that's a reasonable assumption 👍

carlaKC · 2020-03-20T09:16:36Z

it seems like this file is missing make rpc-format...

Last push build, so must have missed the travis build check merge in some werid order. Fixed in #4105.

This was referenced Dec 17, 2019

[htlcnotifier 1/4]: Introduce Internal Errors #3843

Merged

invoiceregistry+htlcswitch: Introduce resolution types and add link errors #3844

Merged

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

Merged

carlaKC changed the title ~~lnrpc: Add HTLC Event Subscription~~ [htlcnotifier 4/4]: lnrpc: Add HTLC Event Subscription Dec 17, 2019

carlaKC added HTLC rpc Related to the RPC interface labels Dec 17, 2019

carlaKC marked this pull request as ready for review December 17, 2019 21:30

carlaKC requested review from cfromknecht, joostjager and Roasbeef as code owners December 17, 2019 21:30

carlaKC removed request for Roasbeef, cfromknecht and joostjager December 17, 2019 21:30

Roasbeef added this to the 0.10.0 milestone Jan 14, 2020

Roasbeef added the v0.10 label Jan 17, 2020

carlaKC force-pushed the htlcnotifier-3-addhtlcnotifierrpc branch from aff9bb4 to a8cdea4 Compare February 6, 2020 19:01

carlaKC force-pushed the htlcnotifier-3-addhtlcnotifierrpc branch 3 times, most recently from 5edad82 to 680aaae Compare February 12, 2020 11:06

carlaKC force-pushed the htlcnotifier-3-addhtlcnotifierrpc branch 6 times, most recently from 62ea380 to 8988ee8 Compare February 21, 2020 10:04

carlaKC requested review from joostjager and cfromknecht and removed request for halseth February 26, 2020 21:08

joostjager reviewed Mar 5, 2020

View reviewed changes

lnrpc/routerrpc/subscribe_events.go Show resolved Hide resolved

carlaKC force-pushed the htlcnotifier-3-addhtlcnotifierrpc branch from 54b1e9b to 4403e4c Compare March 9, 2020 09:07

carlaKC requested a review from joostjager March 9, 2020 09:07

carlaKC force-pushed the htlcnotifier-3-addhtlcnotifierrpc branch 2 times, most recently from a89412e to b7f9e46 Compare March 10, 2020 08:24

joostjager reviewed Mar 10, 2020

View reviewed changes

carlaKC force-pushed the htlcnotifier-3-addhtlcnotifierrpc branch from b7f9e46 to f6b5b32 Compare March 10, 2020 18:19

carlaKC requested a review from joostjager March 10, 2020 18:55

joostjager approved these changes Mar 10, 2020

View reviewed changes

cfromknecht reviewed Mar 12, 2020

View reviewed changes

carlaKC added 2 commits March 13, 2020 09:30

rpcserver+routerrpc: add subscribe events to router backend

67d4bad

Add SubscribeHtlcEvents to RouterBackend in preparation for the addition of a HtlcNotifier stream.

routerrpc: add quit channel to signal shutdown

aa70d5d

carlaKC force-pushed the htlcnotifier-3-addhtlcnotifierrpc branch from f6b5b32 to 891a9fd Compare March 13, 2020 07:41

routerrpc: add htlcnotifier subscription

0a5b918

carlaKC force-pushed the htlcnotifier-3-addhtlcnotifierrpc branch from 891a9fd to 0a5b918 Compare March 13, 2020 07:54

carlaKC requested review from cfromknecht and joostjager March 13, 2020 07:58

joostjager approved these changes Mar 13, 2020

View reviewed changes

cfromknecht approved these changes Mar 17, 2020

View reviewed changes

cfromknecht merged commit b6a0bfc into lightningnetwork:master Mar 17, 2020

carlaKC mentioned this pull request Mar 20, 2020

routerrpc: run rpc-format on routerrpc #4105

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[htlcnotifier 4/4]: lnrpc: Add HTLC Event Subscription #3848

[htlcnotifier 4/4]: lnrpc: Add HTLC Event Subscription #3848

carlaKC commented Dec 17, 2019 •

edited

Loading

carlaKC commented Mar 10, 2020

joostjager left a comment

joostjager Mar 10, 2020

carlaKC Mar 10, 2020

cfromknecht Mar 12, 2020

joostjager Mar 10, 2020

carlaKC Mar 11, 2020

cfromknecht Mar 12, 2020

carlaKC Mar 13, 2020

cfromknecht Mar 17, 2020

cfromknecht Mar 12, 2020

carlaKC commented Mar 13, 2020

joostjager commented Mar 13, 2020

cfromknecht left a comment

cfromknecht Mar 17, 2020

carlaKC commented Mar 20, 2020

[htlcnotifier 4/4]: lnrpc: Add HTLC Event Subscription #3848

[htlcnotifier 4/4]: lnrpc: Add HTLC Event Subscription #3848

Conversation

carlaKC commented Dec 17, 2019 • edited Loading

carlaKC commented Mar 10, 2020

joostjager 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

carlaKC commented Mar 13, 2020

joostjager commented Mar 13, 2020

cfromknecht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC commented Mar 20, 2020

carlaKC commented Dec 17, 2019 •

edited

Loading