-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
routing: extend trackpayment with htlc events #3972
routing: extend trackpayment with htlc events #3972
Conversation
9415ddd
to
097b5db
Compare
6152911
to
874c0e1
Compare
Ready for concept ack Todo: add |
313d220
to
40e8ac4
Compare
|
40e8ac4
to
d5daad5
Compare
d5daad5
to
5929f0e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent PR, really makes it easier to reason about the control tower subscription logic 👍
} | ||
|
||
// Close outgoing channel. | ||
close(cq.chanOut) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this should be done on quit also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. It would signal a clean end to the queue, which isn't the case for quit
// a given hash will hold the mutex to be used by all | ||
// callers requesting access for the hash, in addition to | ||
// the count of callers. | ||
mutexes map[lntypes.Hash]*cntMutex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we just take 4 bytes of the hash and use the existing uint64
mutex? Alternatively use interface{}
as the key type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting suggestion. That will work, unless people use non-random hashes (for invoices that are never settled)? There is some cost in terms of understandability. Not sure if it weighs up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worst case it will just have to wait a bit longer before accessing the DB in those cases. I think it is nice to avoid the code duplication, as long as we don't have generics.
Do avoid the scenario you mention, could also a add a simple method
func (h Hash) Uint64Key() uint64 {
return uint64(sha256(h[:])[0:4])
}
but maybe that is taking it too far
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will leave this thread open for others to comment on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no strong preference fro me, tho i think a hash map would be a useful addition to multimutex
as a library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leaving hash map in then
5929f0e
to
35cbdde
Compare
// a given hash will hold the mutex to be used by all | ||
// callers requesting access for the hash, in addition to | ||
// the count of callers. | ||
mutexes map[lntypes.Hash]*cntMutex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no strong preference fro me, tho i think a hash map would be a useful addition to multimutex
as a library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🦊
Re the breaking proto changes: as mentioned offline, in the end we haven't committed to making this a stable API yet. I think we also envisioned from the start that AMP/MPP would likely result in a series of changes in the API as things became more concrete through development. With 0.10, as we're now moving to always compile it in, and we may also have the API docs render the sub-servers as well, we can now start to publicly commit to gradual freezing of the interface.
As mentioned offline, a blog post explaining how to interact with and track MPP for 0.10 will go a long way.
35cbdde
to
97fa277
Compare
3cdbd28
to
10919c4
Compare
Similar to what is done for SettleAttempt. Co-authored-by: Johan T. Halseth <[email protected]>
10919c4
to
a086a48
Compare
This commit fixes the inconsistency between the payment state as reported by routerrpc.SendPayment/routerrpc.TrackPayment and the main rpc ListPayments call. In addition to that, payment state changes are now sent out for every state change. This opens the door to user interfaces giving more feedback to the user about the payment process. This is especially interesting for multi-part payments.
a086a48
to
3ed3c90
Compare
This PR replaces the messages in the stream of
|
That's pretty rough, it is because it is not a new RPC, it's an existing RPC that was overwritten in a way that gRPC was not designed to support |
That's perfectively acceptable as this is an experimental RPC server, which was only envisioned to only near a final form after MPP/AMP was implemented, which is now. Compare this to the external sendpayment and llist/subscribe invoice calls which haven't changed significantly since they were created. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🔥
The main SendPayment
rpcs have been in place for 3+ years now w/o breaking changes. IMO it's better to break the API before moving routerrpc
out of experimental than end up in a mediocre middle ground. If we're lucky routerrpc.SendPayment
will be more Lindy than the original, so in the grand scheme of things the amortized costs are pretty marginal for users who happen to be using the experimental RPCs early. Seems a little cart before the horse to have experimental integrations prevent shipping a polished product for the masses.
By rough I mean that it's more difficult to deal with as a consumer. So if you were building on this, you will now have a harder job adapting to the change than otherwise might be possible |
Currently there is inconsistency between the payment status as reported via
ListPayment
and the status notified throughrouterrpc.SendPayment/TrackPayment
. With this PR, the same full payment struct is returned by both rpc calls.In addition to that,
routerrpc
notifications are now sent out for every change to the payment. This will allow users to track progress of their (multi-parts) payments, either vialncli
(use the newly added--show_inflight
flag) or via graphical user interfaces/apps if support is added there. Previously only the final state of the payment was reported.Note: The
HTLCAttempt_IN_FLIGHT
update will be sent out before the htlc is actually in flight on the link. Launching aQueryRoutes
at that point will not see the balance change yet. This is relevant for applications that use the output ofTrackPayment
to implement their own payment loop.