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

routing: add build route functionality #3440

Merged
merged 6 commits into from Sep 25, 2019

Conversation

@joostjager
Copy link
Collaborator

commented Aug 30, 2019

This PR adds a new rpc for advanced users that allows them to build a fully specified route from a list of pubkeys. The call looks up the relevant channel policies from the graph and calculates the required fees and time lock values.

Route building is exposed via lncli. Example command to build a two hop route:

lncli buildroute --hops <pubkey1>,<pubkey2> --amt 500

It is also possible to leave out the --amt parameter. In that case, a route is built for the minimum amount that it can carry. This option is useful to execute probe payments for which the primary objective is to pick up black holes (stuck htlc). If the payment gets stuck, the time-locked amount is minimal.

@joostjager joostjager requested review from halseth and Roasbeef as code owners Aug 30, 2019
@joostjager joostjager removed request for Roasbeef and halseth Aug 30, 2019
@joostjager joostjager force-pushed the joostjager:buildroute branch 2 times, most recently from 1158ce8 to 038d33b Aug 30, 2019
@joostjager joostjager requested review from halseth and wpaulino Aug 30, 2019
@wpaulino wpaulino added this to the 0.8.0 milestone Aug 30, 2019
lnrpc/routerrpc/router.proto Outdated Show resolved Hide resolved
routing/router.go Outdated Show resolved Hide resolved
routing/router.go Outdated Show resolved Hide resolved
routing/router.go Outdated Show resolved Hide resolved
routing/router.go Outdated Show resolved Hide resolved
cmd/lncli/routerrpc_active.go Outdated Show resolved Hide resolved
cmd/lncli/cmd_build_route.go Show resolved Hide resolved
cmd/lncli/cmd_build_route.go Outdated Show resolved Hide resolved
cmd/lncli/cmd_build_route.go Outdated Show resolved Hide resolved
cmd/lncli/commands.go Show resolved Hide resolved
cmd/lncli/cmd_build_route.go Outdated Show resolved Hide resolved
lnrpc/routerrpc/router.proto Outdated Show resolved Hide resolved
routing/router.go Outdated Show resolved Hide resolved
@joostjager joostjager force-pushed the joostjager:buildroute branch 7 times, most recently from 7da0d23 to c175ba8 Sep 2, 2019
@joostjager joostjager requested review from halseth and wpaulino Sep 2, 2019
@joostjager joostjager force-pushed the joostjager:buildroute branch 2 times, most recently from 769d394 to e24a703 Sep 3, 2019
Copy link
Collaborator

left a comment

Looks pretty good to me :)

channeldb/graph.go Outdated Show resolved Hide resolved
lnrpc/routerrpc/router_server.go Outdated Show resolved Hide resolved
lnrpc/routerrpc/router_server.go Outdated Show resolved Hide resolved
routing/router.go Outdated Show resolved Hide resolved
routing/router.go Show resolved Hide resolved
cmd/lncli/cmd_build_route.go Outdated Show resolved Hide resolved
cmd/lncli/cmd_build_route.go Show resolved Hide resolved
cmd/lncli/cmd_build_route.go Outdated Show resolved Hide resolved
cmd/lncli/cmd_build_route.go Outdated Show resolved Hide resolved
routing/router.go Show resolved Hide resolved
@joostjager joostjager force-pushed the joostjager:buildroute branch from e24a703 to 24742c7 Sep 3, 2019
@joostjager joostjager requested a review from halseth Sep 3, 2019
@joostjager joostjager force-pushed the joostjager:buildroute branch from 24742c7 to 00e13a6 Sep 6, 2019
@joostjager joostjager force-pushed the joostjager:buildroute branch from 00e13a6 to f06a175 Sep 6, 2019
@joostjager joostjager force-pushed the joostjager:buildroute branch 2 times, most recently from ba36796 to 94bbee4 Sep 16, 2019
lnrpc/routerrpc/router_server.go Show resolved Hide resolved
cmd/lncli/cmd_build_route.go Outdated Show resolved Hide resolved
routing/pathfind.go Show resolved Hide resolved
_, err = ctx.router.BuildRoute(
nil, hops, nil, 40,
)
if _, ok := err.(ErrNoChannel); !ok {

This comment has been minimized.

Copy link
@wpaulino

wpaulino Sep 16, 2019

Collaborator

Asserts fields of error as well?

This comment has been minimized.

Copy link
@joostjager

joostjager Sep 17, 2019

Author Collaborator

Added

joostjager added 3 commits Jan 24, 2019
@joostjager joostjager force-pushed the joostjager:buildroute branch 2 times, most recently from 35ad463 to 396c5b6 Sep 17, 2019
@joostjager joostjager requested a review from wpaulino Sep 17, 2019
lnrpc/routerrpc/router.proto Show resolved Hide resolved
routing/router.go Outdated Show resolved Hide resolved
routing/router.go Show resolved Hide resolved
@joostjager joostjager force-pushed the joostjager:buildroute branch 2 times, most recently from 8e8fa7c to 8712e88 Sep 18, 2019
@joostjager

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 18, 2019

Also extended the test so that it also exercises selecting the best channel when there are multiple channels between a pair of nodes.

@joostjager joostjager requested a review from wpaulino Sep 18, 2019
lnrpc/routerrpc/router.proto Outdated Show resolved Hide resolved
routing/router.go Outdated Show resolved Hide resolved
routing/router.go Outdated Show resolved Hide resolved
routing/router.go Outdated Show resolved Hide resolved
routing/router.go Show resolved Hide resolved
routing/router_test.go Show resolved Hide resolved

// Determine max htlc value.
maxHtlc := lnwire.NewMSatFromSatoshis(capacity)
if policy.MaxHTLC != 0 {

This comment has been minimized.

Copy link
@halseth

halseth Sep 18, 2019

Collaborator

Should use HasMaxHtlc?

This comment has been minimized.

Copy link
@joostjager

joostjager Sep 18, 2019

Author Collaborator

Changed. This is the confusion that I think we shouldn't need to have.

This comment has been minimized.

Copy link
@joostjager

joostjager Sep 18, 2019

Author Collaborator

Hmm, looking in the link, it also checks for MaxHTLC == 0. I left the check identical to what happens in the link.

This comment has been minimized.

Copy link
@wpaulino

wpaulino Sep 24, 2019

Collaborator

That's because the link uses ForwardingPolicy, which doesn't contain information about whether the bit is set or not. I'm in favor of adding the check for the bit being set here.

This comment has been minimized.

Copy link
@joostjager

joostjager Sep 24, 2019

Author Collaborator

Ok, it is indeed a good idea, because this function is also used for non-local channels. Changed

routing/router.go Show resolved Hide resolved
// incompatible channel policies.
//
// There is possibility with pubkey addressing that we should
// have selected a different channel downstream, but we don't

This comment has been minimized.

Copy link
@halseth

halseth Sep 18, 2019

Collaborator

Why don't we fix that? Should this be a TODO, or is there another reason?

This comment has been minimized.

Copy link
@joostjager

joostjager Sep 18, 2019

Author Collaborator

It would complicate path finding while we expect this situation to be rare. The spec recommends to keep all policies towards a peer identical. If that is the case, there isn't a better channel that we should have selected.

Added this as a comment

cmd/lncli/cmd_build_route.go Outdated Show resolved Hide resolved
@joostjager joostjager requested a review from halseth Sep 18, 2019
@joostjager joostjager force-pushed the joostjager:buildroute branch from 8712e88 to e05425d Sep 18, 2019
Copy link
Collaborator

left a comment

Only a few nits and suggestions from me, LGTM 🏆

lnrpc/routerrpc/router.proto Show resolved Hide resolved
// If we have a specific amount for which we are building the route,
// validate it against the channel constraints and return the new
// running amount.
if !canIncreaseAmt {

This comment has been minimized.

Copy link
@halseth

halseth Sep 19, 2019

Collaborator

maybe this had would been less confusing if the param was called findMinAmt or similar.

This comment has been minimized.

Copy link
@joostjager

joostjager Sep 19, 2019

Author Collaborator

I purposely renamed it to canIncreaseAmt because I realized that it is a more generic function here.

This comment has been minimized.

Copy link
@halseth

halseth Sep 20, 2019

Collaborator

Would it work to not pass this bool to the method, and instead let the caller check the returned amount? So if the amount has increased, but we cannot increase it, it will take action.

It should be equivalent, I just find the current branching within the prependChannel a bit confusing.

This comment has been minimized.

Copy link
@joostjager

joostjager Sep 23, 2019

Author Collaborator

I understand your suggestion, but I think it may be confusing in another way. The current code has already seen a few earlier iterations. If there isn't a way that very clearly improves the readability, I'd rather leave it as is.

routing/router.go Outdated Show resolved Hide resolved
)
if err != nil {
log.Tracef("Skipping chan %v: %v",
inEdge.ChannelID, err)

This comment has been minimized.

Copy link
@halseth

halseth Sep 19, 2019

Collaborator

Is this an expected error? If not maybe log as Warn

This comment has been minimized.

Copy link
@joostjager

joostjager Sep 19, 2019

Author Collaborator

Yes, it can happen if channels have distinct policies. If all of them do not match, we return a proper error. Warn is definitely too heavy in this situation.

This comment has been minimized.

Copy link
@halseth

halseth Sep 20, 2019

Collaborator

It's just that if this is expected to happen, and it leads us to not be able to build a route, the reason will likely never surface, as it's on trace.

This comment has been minimized.

Copy link
@joostjager

joostjager Sep 23, 2019

Author Collaborator

The trace log is intended for use during development. The problem with reporting the exact reason to the caller is similar to the situation that we have in path finding. There may be multiple reasons. I am afraid that just exposing a random one of them will lead to the same incorrect assumptions that we currently have for path finding.

This comment has been minimized.

Copy link
@joostjager

joostjager Sep 23, 2019

Author Collaborator

To be clear, even if the call succeeds in building a route, some channels may have been skipped. We can't log it on Warn, because it may really not be a warning.

cmd/lncli/cmd_build_route.go Outdated Show resolved Hide resolved
cmd/lncli/cmd_build_route.go Outdated Show resolved Hide resolved
@joostjager joostjager force-pushed the joostjager:buildroute branch from e05425d to 21f3a06 Sep 19, 2019
@wpaulino wpaulino removed their request for review Sep 24, 2019
joostjager added 3 commits Aug 29, 2019
@joostjager joostjager force-pushed the joostjager:buildroute branch from 21f3a06 to 03d33cb Sep 24, 2019
Copy link
Collaborator

left a comment

LGTM 💫

if hasAmt {
amtMsat = ctx.Int64("amt") * 1000
if amtMsat == 0 {
return fmt.Errorf("non-zero amount required")

This comment has been minimized.

Copy link
@wpaulino

wpaulino Sep 24, 2019

Collaborator

One last nit: do we still need all of this? Seems like AmtMsat can just be ctx.Uint64("amt") * 1000.

@Roasbeef Roasbeef merged commit 18f88cb into lightningnetwork:master Sep 25, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.08%) to 63.084%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.