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: allow route to self #3736

Merged
merged 5 commits into from Nov 26, 2019

Conversation

@joostjager
Copy link
Collaborator

joostjager commented Nov 18, 2019

This PR makes it possible to find routes to yourself. It is useful for rebalancing purposes if combined with #3739 and an outgoing channel restriction.

Usage from the command line:

  1. lncli addinvoice <amt>
  2. lncli payinvoice --allow_self_payment --outgoing_chan_id <outgoing_chan> --last_hop <last_hop> <payreq_from_1>

This will push out amt through outgoing_chan back to yourself via one of your channels with your peer last_hop. The exact incoming channel cannot be specified because routing nodes forward non-strict.

Note that if you pay to yourself without any of these restrictions, it may happen that the payment comes back in through the same channel that was used to send it out. In that case balances don't change. They may only go down because of the routing fees paid.

Fixes #1645

@joostjager joostjager force-pushed the joostjager:route-to-self branch 2 times, most recently from f128194 to bfc27d6 Nov 18, 2019
@joostjager joostjager changed the title route to self routing: allow route to self Nov 18, 2019
@joostjager joostjager force-pushed the joostjager:route-to-self branch 3 times, most recently from e651429 to 86027ba Nov 18, 2019
@joostjager joostjager marked this pull request as ready for review Nov 18, 2019
@joostjager joostjager requested a review from Roasbeef as a code owner Nov 18, 2019
@joostjager joostjager force-pushed the joostjager:route-to-self branch from 86027ba to 0542f81 Nov 19, 2019
@joostjager joostjager removed the request for review from Roasbeef Nov 19, 2019
@joostjager joostjager self-assigned this Nov 19, 2019
@joostjager joostjager added this to WIP in v0.9.0-beta via automation Nov 19, 2019
@joostjager joostjager added this to the 0.9.0 milestone Nov 19, 2019
@joostjager joostjager requested review from guggero and wpaulino Nov 19, 2019
@joostjager joostjager moved this from WIP to Needs Review in v0.9.0-beta Nov 19, 2019
@joostjager joostjager added the v0.9.0 label Nov 20, 2019
Copy link
Collaborator

guggero left a comment

Clean and well crafted PR!
It's very nice to see that we can now rebalance easily, even through the command line.

Only found a few nits, nothing critical.

routing/pathfind_test.go Outdated Show resolved Hide resolved
routing/pathfind.go Outdated Show resolved Hide resolved
routing/pathfind.go Outdated Show resolved Hide resolved
@joostjager joostjager force-pushed the joostjager:route-to-self branch from 0542f81 to ad7d590 Nov 21, 2019
Copy link
Collaborator

wpaulino left a comment

Note that if you pay to yourself without any of these restrictions, it may happen that the payment comes back in through the same channel that was used to send it out. In that case balances don't change. They may only go down because of the routing fees paid.

Given that this doesn't really have a use case, perhaps we could detect this and return an error?

routing/pathfind_test.go Outdated Show resolved Hide resolved
routing/pathfind_test.go Show resolved Hide resolved
@joostjager joostjager force-pushed the joostjager:route-to-self branch from ad7d590 to 3406c11 Nov 22, 2019
@joostjager joostjager requested a review from wpaulino Nov 22, 2019
@joostjager

This comment has been minimized.

Copy link
Collaborator Author

joostjager commented Nov 22, 2019

Given that this doesn't really have a use case, perhaps we could detect this and return an error?

I wouldn't really want to hard block this. I think we can assume that people using send to self know what they are doing. I also think we cannot really rule out that such use case would emerge at some point. There is for example the trick where you drop off more routing fees than required as a way to do spontaneous payment.

@joostjager joostjager force-pushed the joostjager:route-to-self branch from 3406c11 to 336e71f Nov 22, 2019
v0.9.0-beta automation moved this from Needs Review to Approved Nov 22, 2019
Copy link
Collaborator

cfromknecht left a comment

The exact incoming channel cannot be specified because routing nodes forward non-strict.

We may not be able to choose which channel the HTLC is forwarded over, but we can reject the payment if it doesn't come over the intended channel and retry. Now that we index the invoice htlcs by circuit key, it should be straight forward (apart from needing to add this restriction on the invoice).

Note that if you pay to yourself without any of these restrictions, it may happen that the payment comes back in through the same channel that was used to send it out. In that case balances don't change. They may only go down because of the routing fees paid.

I don't think this would be the expected behavior for simply calling payinvoice <payreq>, and seems pretty easy to mess up. IMO this should produce a warning when source==destination, especially considering it changes the existing behavior which would normally fail. Perhaps adding an --allow-cycle flag would suffice?

routing/pathfind.go Show resolved Hide resolved
@joostjager joostjager force-pushed the joostjager:route-to-self branch 2 times, most recently from f91c130 to 637fcc3 Nov 25, 2019
@joostjager

This comment has been minimized.

Copy link
Collaborator Author

joostjager commented Nov 25, 2019

We may not be able to choose which channel the HTLC is forwarded over, but we can reject the payment if it doesn't come over the intended channel and retry. Now that we index the invoice htlcs by circuit key, it should be straight forward (apart from needing to add this restriction on the invoice).

I consider this out of scope for this PR? It is definitely useful functionality, also to prevent tying up capital on expensive peers.

I don't think this would be the expected behavior for simply calling payinvoice , and seems pretty easy to mess up. IMO this should produce a warning when source==destination, especially considering it changes the existing behavior which would normally fail. Perhaps adding an --allow-cycle flag would suffice?

Flag added

@joostjager joostjager requested a review from cfromknecht Nov 25, 2019
joostjager added 5 commits Nov 18, 2019
This prepares for routing to self. When checking the condition at the
start, the loop would terminate immediately because the source is equal
to the target.
This prepares for routing to self.
@joostjager joostjager force-pushed the joostjager:route-to-self branch from 637fcc3 to 2d19201 Nov 26, 2019
Copy link
Collaborator

cfromknecht left a comment

LGTM 🍣

@joostjager joostjager merged commit 949a5c5 into lightningnetwork:master Nov 26, 2019
1 of 2 checks passed
1 of 2 checks passed
coverage/coveralls Coverage decreased (-0.07%) to 62.694%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
v0.9.0-beta automation moved this from Approved to Done Nov 26, 2019
@ZapUser77

This comment has been minimized.

Copy link

ZapUser77 commented Nov 27, 2019

Thanks. Glad to see that merged in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.