Skip to content
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

lnrpc: structured SendToRoute failure reporting #1662

Merged
merged 4 commits into from May 15, 2019

Conversation

@joostjager
Copy link
Collaborator

@joostjager joostjager commented Jul 31, 2018

Background

Many ideas float around to build functions/algorithms on top of lnd. For example: rebalancing, probing, routing service, etc. The current way for that kind of scripts to operate is to make calls to SendToRoute.
Unfortunately limited error information is returned to the caller. router will keep trying the supplied routes and if none of them works, return a routes exhausted error without the real cause. Even if only one route is supplied. This makes it harder for scripts to guess what happened.

Implementation details

  • In order to keep it simple, the option to pass in multiple routes is removed. Only a single route per payment is allowed. If multiple routes need to be tried (with or without waiting for the result), they can still be passed in one by one. Otherwise, to get a proper error result for every route in a list of routes, the result object should also need to become a list. In the streaming version of the SendToRoute call, it would become a list (of routes) in a list (of payments).
  • Structured error information is represented as a (flat) list of properties. The alternative would be to use protobuf oneof. This would for example result in the following json:
{
    "payment_error": "UnknownPaymentHash",
    "StructuredResult": {
        "FailUnknownPaymentHash": {}
    }
}

Maybe not the most intuitive representation in json and I am not sure how this is supported in the proto generators for various languages. It also seems that the oneof key cannot be assigned a json_name.

Usage

A new SendToRoute rpc has been implemented in the router sub server. In order to use it, lnd needs to be compiled with tags="routerrpc".

The command has not been added to lncli and is only available on the rpc interface. See router.proto for more information.

@joostjager joostjager force-pushed the raw branch 2 times, most recently from 15be4a3 to e0d8f80 Aug 2, 2018
@joostjager joostjager changed the title Raw SendToRoute RPC lnrpc: allow pubkey in SendToRoute and better failure reporting Aug 2, 2018
@joostjager joostjager force-pushed the raw branch 2 times, most recently from 30e785a to 7602d53 Aug 9, 2018
@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented Aug 9, 2018

PR is ready for a first pass review

Loading

@joostjager joostjager force-pushed the raw branch 2 times, most recently from 1bba17e to bfac456 Aug 9, 2018
@Roasbeef Roasbeef added this to the 0.6 milestone Aug 17, 2018
@joostjager joostjager changed the title lnrpc: allow pubkey in SendToRoute and better failure reporting lnrpc: better payment failure reporting Sep 20, 2018
@Roasbeef Roasbeef requested review from halseth and removed request for halseth Oct 16, 2018
@joostjager joostjager force-pushed the raw branch 2 times, most recently from f0ceaf6 to cb1dc9b Oct 29, 2018
cmd/lncli/commands.go Outdated Show resolved Hide resolved
Loading
cmd/lncli/commands.go Outdated Show resolved Hide resolved
Loading
lnd_test.go Outdated
@@ -1,5 +1,8 @@
// +build rpctest

// TODO(joostjager): Assert on structured errors instead of
Copy link
Member

@Roasbeef Roasbeef Nov 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO should be carried out before this is merged in.

Loading

Copy link
Collaborator Author

@joostjager joostjager May 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the router rpc is still in experimental state and its interface may change, we could also wait until we actually deprecate the main SendToRoute rpc.

Changing it now also moves coverage away from the main rpc and it may be safer to not yet do that.

Loading

lnrpc/rpc.proto Outdated
uint64 htlc_msat = 5;
bytes onion_sha_256 = 8;
uint32 cltv_expiry = 9;
uint32 flags = 11;
Copy link
Member

@Roasbeef Roasbeef Nov 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the flags here? Aren't they already contained in the channel update?

Loading

Copy link
Collaborator Author

@joostjager joostjager Mar 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually don't know. Can you tell me what the flags field in the onion failure message channel_disabled is for?

Loading

lnrpc/rpc.proto Outdated
ChannelUpdate channel_update = 6 [json_name = "channel_update"];
uint64 htlc_msat = 5;
bytes onion_sha_256 = 8;
uint32 cltv_expiry = 9;
Copy link
Member

@Roasbeef Roasbeef Nov 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the outgoing or incoming CLTV expiry for that hop?

Loading

Copy link
Collaborator Author

@joostjager joostjager Mar 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above

Loading

Copy link
Collaborator Author

@joostjager joostjager May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the field as defined in the lightning spec.

Loading

lnrpc/rpc.proto Outdated
PaymentError payment_error = 4 [json_name = "payment_error"];
bytes error_source = 7 [json_name = "error_source"];
ChannelUpdate channel_update = 6 [json_name = "channel_update"];
uint64 htlc_msat = 5;
Copy link
Member

@Roasbeef Roasbeef Nov 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar q here: outgoing or incoming?

Loading

Copy link
Collaborator Author

@joostjager joostjager Mar 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field and other fields in this message contain the raw onion error. I chose not to make it one giant oneof construct, but instead created a union of all failure message fields. The meaning of htlc_msat and also cltv_expiry depends on the actual error.

Loading

lnrpc/rpc.proto Outdated
}

PaymentError payment_error = 4 [json_name = "payment_error"];
bytes error_source = 7 [json_name = "error_source"];
Copy link
Member

@Roasbeef Roasbeef Nov 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could add a suffix like _pubkey_bytes, just to clarify what the opaque bytes represent.

Loading

Copy link
Collaborator Author

@joostjager joostjager Mar 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added _pubkey. Also adding bytes is maybe too much and also not what we do in other places in the proto.

Loading

lnrpc/rpc.proto Outdated
message ChannelUpdate {
// Signature is used to validate the announced data and prove the
// ownership of node id.
bytes signature = 1 [json_name = "signature"];
Copy link
Member

@Roasbeef Roasbeef Nov 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to expose the sig on the RPC level? As could say the client already fully trusts the server itself.

Loading

Copy link
Contributor

@alexbosworth alexbosworth Nov 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like the signatures, for data portability

Loading

routing/pathfind.go Outdated Show resolved Hide resolved
Loading
cmd/lncli/commands.go Outdated Show resolved Hide resolved
Loading
lnrpc/rpc.proto Outdated Show resolved Hide resolved
Loading
routing/router.go Outdated Show resolved Hide resolved
Loading
lnrpc/rpc.proto Outdated Show resolved Hide resolved
Loading
rpcserver.go Outdated Show resolved Hide resolved
Loading
rpcserver.go Outdated Show resolved Hide resolved
Loading
@esneider
Copy link
Contributor

@esneider esneider commented May 1, 2019

What's the status of this PR? Is it on its way to be merged, or has it been replaced/deprioritized?

I'm asking mainly due to the fact that good error reporting is needed in order to take full advantage of the new interface of QueryRoutes.

Loading

@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented May 2, 2019

@esneider The plan is to move ahead with this after the release of 0.6.1, as it contains a breaking change.

Loading

@joostjager joostjager force-pushed the raw branch 2 times, most recently from 4f9207c to e498f01 May 6, 2019
@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented May 6, 2019

Loading

@cfromknecht cfromknecht added this to the 0.7 milestone May 7, 2019
High Priority automation moved this from In progress to Needs review May 11, 2019
routing/router.go Outdated Show resolved Hide resolved
Loading
routing/router.go Outdated Show resolved Hide resolved
Loading
lnrpc/routerrpc/router.proto Show resolved Hide resolved
Loading
lnrpc/routerrpc/router.proto Show resolved Hide resolved
Loading
lnrpc/routerrpc/router.proto Show resolved Hide resolved
Loading
lnrpc/routerrpc/router_server.go Show resolved Hide resolved
Loading
cmd/lncli/commands.go Outdated Show resolved Hide resolved
Loading
@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented May 13, 2019

@Roasbeef ptal

Loading

Copy link
Collaborator

@halseth halseth left a comment

Looks more or less good to me, just a few comments.

Loading

cmd/lncli/commands.go Show resolved Hide resolved
Loading
)

return r.sendPayment(payment, paySession)
preimage, err := r.sendToSwitch(route, hash)
Copy link
Collaborator

@halseth halseth May 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need to be shuffled around a little bit after reliable payments are in.

Loading

Copy link
Collaborator Author

@joostjager joostjager May 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but as we already merged the main refactoring of router.go some time ago, it indeed won't be much.

Loading

Copy link
Collaborator Author

@joostjager joostjager May 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted to using sendPayment for SendToRoute also, to connect better with #2761.

Loading

lnrpc/routerrpc/router.proto Show resolved Hide resolved
Loading
lnrpc/routerrpc/router.proto Outdated Show resolved Hide resolved
Loading
// Timestamp allows ordering in the case of multiple announcements. We
// should ignore the message if timestamp is not greater than
// the last-received.
uint32 timestamp = 4;
Copy link
Collaborator

@halseth halseth May 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uint64?

Loading

Copy link
Collaborator Author

@joostjager joostjager May 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is uint32 in the wire protocol.

Loading

lnrpc/routerrpc/router_server.go Show resolved Hide resolved
Loading
// marshallChannelUpdate marshalls a channel update as received over the wire to
// the router rpc format.
func marshallChannelUpdate(update *lnwire.ChannelUpdate) *ChannelUpdate {
if update == nil {
Copy link
Collaborator

@halseth halseth May 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error case?

Loading

Copy link
Collaborator Author

@joostjager joostjager May 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, no error because the update can be optional.

Loading

@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented May 13, 2019

ptal @halseth

Loading

Copy link
Member

@Roasbeef Roasbeef left a comment

LGTM 🐊

contingent removal of the last commit

Loading

@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented May 15, 2019

Debug commit removed

Loading

High Priority automation moved this from Needs review to Final Testing -- Ready For Merge May 15, 2019
Copy link
Collaborator

@halseth halseth left a comment

Alright, this LGTM now 🔥

Loading

@joostjager joostjager merged commit 863bf2f into lightningnetwork:master May 15, 2019
2 checks passed
Loading
High Priority automation moved this from Final Testing -- Ready For Merge to Done May 15, 2019
@joostjager joostjager deleted the raw branch May 15, 2019
@joostjager joostjager changed the title lnrpc: better payment failure reporting lnrpc: structured SendToRoute failure reporting May 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
High Priority
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants