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: last hop restriction #3739

Merged
merged 4 commits into from Nov 19, 2019

Conversation

joostjager
Copy link
Collaborator

@joostjager joostjager commented Nov 18, 2019

This PR adds an optional last hop restriction to payments.

joostjager added 2 commits Nov 18, 2019
Previously both paths were equal cost, so it could also be a
coincedence that the path with the outgoing restriction would be chosen.
@joostjager joostjager requested a review from Roasbeef as a code owner Nov 18, 2019
@joostjager joostjager removed the request for review from Roasbeef Nov 18, 2019
@joostjager joostjager force-pushed the incoming-restriction branch 2 times, most recently from 9f28670 to 59ec4fe Compare Nov 18, 2019
@joostjager joostjager self-assigned this Nov 18, 2019
@joostjager joostjager added this to WIP in v0.9.0-beta via automation Nov 18, 2019
@joostjager joostjager added this to the 0.9.0 milestone Nov 18, 2019
@joostjager joostjager requested review from guggero and wpaulino Nov 18, 2019
@joostjager joostjager moved this from WIP to Needs Review in v0.9.0-beta Nov 18, 2019
Copy link
Collaborator

@guggero guggero left a comment

Very nice and clean PR! This will allow us to craft rebalance payments without external tools, right?
LGTM, there's only a small comment that is non-blocking.

routing/pathfind_test.go Show resolved Hide resolved
routing/pathfind_test.go Show resolved Hide resolved
@joostjager joostjager force-pushed the incoming-restriction branch 2 times, most recently from 190416d to a21c7d7 Compare Nov 18, 2019
@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented Nov 18, 2019

Very nice and clean PR! This will allow us to craft rebalance payments without external tools, right?

In combination with #3736 it does.

Also we can use this to loop into a specific channel.

routing/pathfind_test.go Outdated Show resolved Hide resolved
@@ -1596,6 +1596,10 @@ type LightningPayment struct {
// hop. If nil, any channel may be used.
OutgoingChannelID *uint64

// LastHop is the pubkey of the last node before the final destination
// is reached. If nil, any node may be used.
LastHop *route.Vertex
Copy link
Collaborator

@wpaulino wpaulino Nov 18, 2019

Choose a reason for hiding this comment

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

Do we care about validating whether this is an existing node in the graph? Path finding would result in unable to find a path to destination, which can be ambiguous.

Copy link
Collaborator Author

@joostjager joostjager Nov 18, 2019

Choose a reason for hiding this comment

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

We also don't do this for the outgoing channel restriction. As this is advanced functionality, I am ok with leaving it up to the caller to make sure this doesn't happen. What do you think?

Maybe if we start fixing #1680, we can add two new payment failures:
no_outgoing_channel (with enough balance and not filtered out) and no_last_hop (with enough capacity and not filtered out).

Copy link
Collaborator

@wpaulino wpaulino Nov 18, 2019

Choose a reason for hiding this comment

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

We can defer it until then.

@joostjager joostjager requested a review from wpaulino Nov 18, 2019
v0.9.0-beta automation moved this from Needs Review to Approved Nov 18, 2019
@joostjager joostjager merged commit a00a360 into lightningnetwork:master Nov 19, 2019
1 of 2 checks passed
v0.9.0-beta automation moved this from Approved to Done Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v0.9.0-beta
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants